Page MenuHomePhabricator

Create interface between Skin and MobileFormatter
Closed, ResolvedPublic

Description

Interface should define what is needed to render the mobile (Minerva) skin and should be agnostic of where it comes from. It is passed into the skin or used by the skin during construction.

The MobileFormatter::newFromContext method will be passed an implementation of the IContentProvider which will provide access to the HTML for the current page. A DefaultContentProvider will obtain the chunk of HTML generated by the PHP parser.

The idea is that future implementations of IContentProvider can provide content from places such as RESTBase and the MediaWiki API.

Motivations

  • The developer tool using the live mediawiki api will be an alternative solution to T104561
  • It will help us debug certain issues as proven by our work in T149852 and T151838

Acceptance criteria

  • Define IContentProvider which provides a basic interface that is implemented by DefaultContentProvider which is passed to MobileFormatter.
  • IContentProvider provides a getHTML method. The constructor is passed an instance of GlobalVarConfig and OutputPage to allow calls to addModule, addModuleStyles and getTitle
  • ExtMobileFrontend::DOMParse will be refactored so it uses IContentProvider to obtain the HTML that should be formatted by MobileFormatter
  • For development purposes introduce MwApiContentProvider which obtains its HTML from a foreign (or local) mediawiki api. The api should be configurable so we can point to any wiki of our choosing. This will allow us to test MobileFormatter and styles on live Wikipedia articles.
  • Make it possible to configure DefaultContentProvider or MwApiContentProvider as the default formatting engine for the wiki.

e.g. $wgMFContentProviderClass = 'MwApiContentProvider'

  • The URL used by MwApiContentProvider is configurable so we can use it for any wiki in any language

e.g. $wgMFMwApiMobileFormatterBaseUri = 'https://en.wikipedia.org/w/api.php'

Future work

In future we may also want to create an interface for MobileFormatter. The RESTBase mobile content service for instance is a ContentProvider and a formatter so running MobileFormatter on the content provided from the mobile content service would be wasteful. This is out of scope for this particular task.

Related Objects

StatusAssignedTask
DeclinedNone
ResolvedCatrope
ResolvedSbailey
Resolved GWicke
Resolvedssastry
ResolvedNone
ResolvedDbrant
ResolvedbearND
ResolvedMholloway
ResolvedNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
ResolvedJdforrester-WMF
DuplicateNone
ResolvedbearND
OpenNone
OpenNone
DeclinedNone
DuplicateNone
DeclinedNone
OpenNone
OpenNone
ResolvedArlolra
ResolvedMooeypoo
ResolvedCatrope
Resolved GWicke
OpenNone
Resolvedmarcoil
Resolvedmarcoil
Resolved GWicke
DuplicateNone
DeclinedNone
OpenNone
OpenNone
OpenNone
OpenNone
Resolvedphuedx

Event Timeline

@Jdlrobson - does this still belong in needs analysis or can it be moved to triaged but future?

Jdlrobson updated the task description. (Show Details)Apr 26 2017, 11:50 PM

@pmiazga I'm still trying to flesh this card out. Since you understand what I'm doing the most here and hopefully see the value - care to help me flesh out the details?

  • Refactor ::newFromContext so this it creates an instance of MobileFormatter. It should not execute any transformations until #format is called.

That's what it does, right?

  • For development purposes introduce MwApiMobileFormatter which obtains its HTML from a foreign (or local) mediawiki api. The api should be configurable so we can point to any wiki of our choosing. This will allow us to test MobileFormatter and styles on live Wikipedia articles which will help us debug certain issues as proven by our work in T149852 and T151838
  • Make it possible to configure MobileFormatter or MwApiMobileFormatter as the default formatting engine for the wiki.

These two AC conflict with the first line of the description of this task:

Interface should define what is needed to render the mobile (Minerva) skin and should be agnostic of where it comes from.

The MobileFormatter formats HTML. It doesn't retrieve it. That's the responsibility of another collaborator. ExtMobileFrontend::DOMParse should probably end up looking like:

$contentProvider = ContentProvider::newFromContext( $context ); // Generally speaking, I'm in favour of factories but this mirrors the next line...
$formatter = MobileFormatter::newFromContext( $context );

$formattedHtml = $formatter->format( $contentProvider->getContent() );
  • Refactor ::newFromContext so this it creates an instance of MobileFormatter. It should not execute any transformations until #format is called.

That's what it does, right?

It does yup. I just want to ensure we don't actually do any transforming inside it e.g. keep the status quo.
However DOMParse also runs additional setters and getters - maybe they need to be moved into this method? I'm not sure.

With regards to your other comment - I like your thinking with separating ContentProvider. The only thing I'm not sure about is how to unwire our use of DOMParse inside onOutputPageBeforeHTML so that the text it operates on goes via the ContentProvider (which is currently provided by code in core)

Jdlrobson updated the task description. (Show Details)Apr 27 2017, 4:04 PM

Change 349130 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Introduce IContentProvider

https://gerrit.wikimedia.org/r/349130

Jdlrobson updated the task description. (Show Details)Apr 28 2017, 9:39 PM
Jdlrobson reassigned this task from Jdlrobson to phuedx.Apr 28 2017, 9:43 PM
Jdlrobson updated the task description. (Show Details)

@phuedx @pmiazga I think I've got this worked out now.
What do you think? OK to move to triaged but future?

phuedx removed phuedx as the assignee of this task.May 2 2017, 10:03 AM

👍

Jdlrobson updated the task description. (Show Details)May 16 2017, 3:43 PM

While most of the work is done on this (according to @pmiazga this is done). Once merged I'll update documentation.

Change 349130 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Introduce IContentProvider

https://gerrit.wikimedia.org/r/349130

Jdlrobson removed a project: Patch-For-Review.

I'll document and resolve this card today.

Jdlrobson assigned this task to phuedx.May 17 2017, 8:40 AM

In a way yes, but it has been for past 3 sprints. I'm still not sure how best to use that tag.
I've added it to the kanban board for visibility for the rest of the team.
@phuedx would you like to sign it off? I don't think there is any follow up work here right now.

phuedx closed this task as Resolved.May 17 2017, 12:22 PM

This is an👌 first step.

I'm still not sure how I feel about this as an alternative to T104561: Gather a set of sample articles to load on test/staging instances., though I'll admit that the change was a driver for a good separation of concerns in the MobileFrontend codebase, of which many are needed.