Page MenuHomePhabricator

[EPIC] Cleanup MobileFormatter and MobileViewAPI where they overlap
Closed, DeclinedPublic

Description

An instance of MobileFormatter is created in ApiMobileView.php, ApiParseExtender.php and includes/MobileFrontend.body.php

There is lots of overlap in the work they do.
The code in includes/MobileFrontend.body.php does work inside MobileFormatter that is repeated by ApiMobileView.php

I see several possible improvements here that could be made by distangling these two things.

  • There is memcached caching inside ApiMobileView.php - let's move this to MobileFormatter. If you view a page Foo in the mobile site and then request it via the API, the same code should not be run again - they should share a cache.
  • The code for creating the structured data entity should be moved to MobileFormatter to allow the SkinMinerva to use it.
  • Skins should be generated via the structured data entity rather than stitching and calculating data themselves. The API does additional processing to split the page into sections and provide a nice JSON of all the information needed to render a page. This would be extremely useful for the skin which duplicates a lot of this logic (for example it checks whether a page has languages and it should render a language button, it relies on the HTML output to construct its sections.
  • Edit icon rendering should be moved out of the MobileFormatter and into the Skin. This would bring us more in line with how apps do this.
  • For development purposes and to encourage us to be consistent with the mobile content service, the MobileFormatted API should be consistent in output and interchangeable. As demonstrated by the POC patch we should be able to use real world content on our local development wikis by doing this.
  • In future we may want to experiment with using the mobile content service for all requests

Composer library?

As HtmlFormatter has been moved to a composer library, in order to make MobileFrontend leaner, and improve the consistency of the api, modularity and quality of the formatter, we want to extract it to a composer library.

Overview

  • Create composer package with boilerplate
    • Get it into CI
    • Migrate MobileFrontend's code and tests to the library
      • Decouple from MobileFrontend's globals and code
      • Thoroughly document changes for later migration
    • Ensure library is continuously tested and works well
  • Swap implementation in MobileFrontend for the composer library
    • Security review?
    • Verify all is well
  • Iterate on mobile formatter to improve the quality of the code
    • Make the external api consistent (for example, use constructor configuration everywhere instead of using some times setters and some times not, etc)
    • ...

Event Timeline

@phuedx would appreciate input here in helping flesh this out.

@phuedx and I should chat about this, but there are a few things we'll need to get our heads round first before we can start working on this.

This looks like a potential area for improvement. Do we want to analyze the MF codebase for such problems and then make an informed decision with all developers with clear paths forward?

Change 313611 had a related patch set uploaded (by Jdlrobson):
POC: MobileFrontend should be able to use MobileContentService

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

Jdlrobson renamed this task from Skin should be more API driven via MobileFormatter to [EPIC] Skin should be API driven / cleanup MobileFormatter.Sep 30 2016, 7:13 PM
Jdlrobson added a project: Epic.
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)

@phuedx @jhobs @Jhernandez @pmiazga What do you think about this task? Do we need to have a discussion before working on it? Is the path forward clear? Can you voice your concerns?

It's on my todo list to setup a meeting to talk to this. It relates to conversations I've been having with Adam and I'm not sure whether those same conversations have been had with others. A discussion is definitely needed and the path is definitely not clear imo!

Since this is not urgent I've setup a conversation around the all hands - scheduled for Tue 17 Jan 2017. Happy to add anyone to that meeting who's following along at home - just let me know :)

@Jdlrobson - can you comment with the summary of the meeting?

Some notes were made here:
https://etherpad.wikimedia.org/p/restbasefrontend

Worries include scalability, not breaking apps and supporting 3rd parties without RESTBase.

We agreed T156408 was probably the best starting point for this work.

Change 313611 abandoned by Jdlrobson:
POC: MobileFrontend should be able to use MobileContentService

Reason:
POC so abandoning. Idea has been expressed in a bug which this patch links to.

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

Jdlrobson renamed this task from [EPIC] Skin should be API driven / cleanup MobileFormatter to [EPIC] Cleanup MobileFormatter and MobileViewAPI where they overlap.Jun 2 2017, 4:29 PM

Declined per the eventual plan to remove mobile view api T186627