Page MenuHomePhabricator

The `site` and `site.styles` module should be explictly disabled on mobile rather than abuse the targets system
Closed, ResolvedPublic3 Estimate Story Points

Description

For a skin running on the mobile domain, MediaWiki:Mobile.(css|js) should be run. (the mobile.site and mobile.site.styles modules)
For a skin running on the desktop domain, MediaWiki:Common.(css|js) should be run (the site and site.styles modules)

Acceptance criteria

  • The site and site.styles modules no longer use a ResoureLoader targets filter.
  • MobileFrontend still ensures the desktop-specific pages of these modules don't load on mobile, and that the mobile versions of these load instead.

Developer notes

Rather than relying on the targets system, MobileFrontend could remove the site module explicitly via a hook

This hook would need to run at the start of OutputPage::loadSkinModules or at the end of Skin:::getDefaultModules. Given Minerva runs a hook in the latter, it seems we have precedent for that hook to run there.

		Hooks::run( 'SkinMinervaDefaultModules', [ $this, &$modules ] );

MobileFrontend can subscribe to the new hook and eventually SkinMinervaDefaultModules can be removed.

Sign off

  • Test and sign off T140045 which should be fixed by this change.

Event Timeline

Jdlrobson updated the task description. (Show Details)
Krinkle moved this task from Limbo to Perf recommendation on the Performance-Team (Radar) board.
Krinkle removed a subscriber: Krinkle.

Change 547643 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Introducing SkinDefaultModules hook

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

Jdlrobson renamed this task from The `site` module should be explictly disabled on mobile rather than abuse the targets system to The `site` and `site.styles` module should be explictly disabled on mobile rather than abuse the targets system.Oct 31 2019, 7:54 PM

From IRC

Perhaps we could make site.style itself extendable then?
2:15 PM Rather than for MF to register a different one, which is incompatible with the idea of a bundle representing a purpose to be delivered to a page
2:15 PM and wastes startup bytes (a little bit)
2:16 PM The power problem is with extensions using it to add or replace modules, instead of just removing. As well as those decisions being potentially based on user or title which makes it uncacheable and incompatible with various long-term directions.
2:17 PM What would you think about making site.style itself possible to turn into mobile.site.styles?

How would this work in practice? A hook inside the SiteModule ? A setter somewhere that allows us to set the main style/main script module?

Krinkle added a comment.EditedNov 1 2019, 2:53 PM

How would this work in practice? A hook inside the SiteModule?

Yep, that sounds good to me. This would be a stateless hook with the limited power to deterministically change the array of pages from getPages of SkinModule and SkinStylesModule are composed of. Stateless in so far that it would not be allowed to vary only by ResourceLoaderContext and Mobile's side-wide context (e.g. isMobileDevice), and not by user or page title, given that it would be evaluated by the startup module which is a cachable and user/page-agnostic execution context.


EDIT: Consolidating these two faces of the coin also helps remove another module from the registry :)

Krinkle updated the task description. (Show Details)Nov 5 2019, 8:37 PM
Jdlrobson moved this task from Inbox to OKR Backlog on the User-Jdlrobson board.

Change 549225 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Enable site modules on mobile domain

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

Jdlrobson moved this task from OKR Backlog to Doing on the User-Jdlrobson board.Nov 6 2019, 11:56 PM
Jdlrobson moved this task from Doing to Blocked on the User-Jdlrobson board.Dec 12 2019, 9:16 PM

Change 562380 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Remove mobile.site and mobile.styles modules

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

Change 547643 merged by jenkins-bot:
[mediawiki/core@master] resourceloader: Introduce hooks in ResourceLoaderSiteModule

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

Okay, this should be taken care of in MobileFrontend (https://gerrit.wikimedia.org/r/562380) and then finally updating targets in core (https://gerrit.wikimedia.org/r/549225) which should be merged for the same release.

Jdlrobson moved this task from Incoming to Needs Prioritization on the Readers-Web-Backlog board.

Change 549225 merged by jenkins-bot:
[mediawiki/core@master] Enable 'site' and 'site.styles' modules on mobile target

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

ovasileva removed ovasileva as the assignee of this task.Jan 13 2020, 11:22 AM
ovasileva added a subscriber: ovasileva.
Jdlrobson triaged this task as Unbreak Now! priority.Jan 13 2020, 11:22 PM

I'm seeing a significant 4kb jump in CSS.. The metrics for the beta cluster suggest this will lead to a significant 0.5s first paint for 3G users (larger for slower connections).

We should either revert the change or merge the MobileFrontend change before the next branch cut.

this should block the train until one of those has been merged.

Restricted Application added a subscriber: Liuxinyu970226. · View Herald TranscriptJan 13 2020, 11:22 PM

Train operator: If by Wednesday/group1, the MobileFrontend change (https://gerrit.wikimedia.org/r/562380/) has not landed into branch (either before the branch cut, or afterward with backport) then consider the train blocked until its logical parent in core (https://gerrit.wikimedia.org/r/564150) has been reverted in-branch - by cherry-picking that revert to wmf.15.

Change 564156 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] "Enable 'site' and 'site.styles' modules on mobile target (take 2)

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

Jdlrobson lowered the priority of this task from Unbreak Now! to Medium.Jan 14 2020, 12:06 AM

I've reverted the change to give us more time to work through the details and give it the attention it deserves. No longer a performance blocker

Bumping out sprint for a bit to focus on other things that are more important right now. I will get back to this!

Jdlrobson moved this task from Blocked to Doing on the User-Jdlrobson board.Jan 14 2020, 6:22 PM

I've been asked to focus on T232140 - but this will be my priority after that one's dealt with.

@Krinkle on review wrote:

I'm not confident this is compatible with current MW and cache setup. How will the cached pages work if the module is served as empty?
If I understand correctly, what you need here is to keep the old modules as-is, unchanged, undeprecated, but no longer queued for new page views. Then next week, it can be removed entirely without deprecation.

That way, cached views continue to load the correct code via the old module, and newer views will load the correct code via the new module.

To explain my thinking around the patch order. It's not a big deal if the two patches get merged together (as they should be) but I want to mitigate the risk one of the 2 scenarios that can occur if one patch doesn't merge for some reason.

Depending on the order of the patches two scenarios arise:

  1. If core patch is present but not MobileFrontend patch, then mobile.site AND site etc will be loaded. This increases CSS payload and negatively impacts performance.
  2. If MobileFrontend patch is present but not core, then no site styles of any nature will be loaded and a hook callback will be registered but never called.

Ideally, both of the patches land together to avoid these issues however given site styles on mobile are being discouraged for template styles, I think scenario 2 is the lesser problematic. In the case of a revert #2 can be fixed using mobile gadgets if necessary. #1 is much more damaging in production due to the caching implications.

Both patches will be going out in the same train.I have no concern about any kind of intermediary stage where one patch hasn't applied.

My CR on the MF patch has to do with the contents of the MF patch, and already builds on the assumption that both go out at once. I'm happy to elaborate if needed, no problem, but perhaps this clarifies it already. Could you re-read the CR with that in mind and let me know if you have a more specific question?

Krinkle added a comment.EditedFeb 12 2020, 6:27 PM

@Jdlrobson Currently MF loads mobile.site and mobile.site.styles on page views via the onBeforePageDisplay hook. Page view HTML is cached. Specifically:

<script>  mw.loader.load( [  "mobile.site"  </script></head>

The end-result of what you're proposing is to:

  • make mobile.site no longer included in the page view modules queue,
  • make mobile.site no longer exist (or do nothing),
  • make site be what mobile.site was,
  • let site become valid for targets=mobile so that core's inclusion of site in the page view modules queue will start applying in MF context and thus end up in the queue output in HTML.

The current two patches do this all in one go. If we look ahead how that would behave, it would break the mobile site code for all page views by unregistered users, because those page view HTML responses are cached by the CDN for upto 7 days.

Their HTML will still only be trying to load mobile.site, it doesn't yet have site in its queue. Their startup module will have been updated immediately and no longer contain mobile.site (it will contain site though). Regardless of startup module, when the browser requests mobile.site from the server, all MediaWiki knows is the current version which is an empty file module that only contains the generated line console.warn(" … deprecation message …");.

There are two ways you can solve this.

  1. Keep the old mobile.site still registered in extension.json, exactly as it works today, in the MobileFrontend patch. Then remove it in a separate commit to be merged one week later (no need for deprecation).
  2. Keep the old mobile.site still registered in extension.json, as empty module aliasing the new site module (dependencies: ["site"]). No deprecation. And removed one week later.

This amended version of the MobileFrontend patch will then land around the same time as the core patch. The order truly doesn't matter. I'll +2 them together. They'lll go out in the same wmf branch and end up on Beta around the same time as well. The second MF patch can be merged one week later (more specifically, at least 7 days after the next train has succesfully rolled out). Does that make sense?

@ovasileva This was nearly done, I think. Would it be possible to finish this off while we still have it fresh in mind? It's nearly done and shouldn't take more than half an hour to look over and finalise – however it could easily cost half a day or more (for multiple people) next time.

Jdlrobson moved this task from Doing to Next up on the User-Jdlrobson board.Mar 2 2020, 10:41 PM
Jdlrobson updated the task description. (Show Details)
ovasileva removed ovasileva as the assignee of this task.Wed, Mar 11, 5:07 PM
ovasileva set the point value for this task to 3.Wed, Mar 11, 5:20 PM

@Krinkle is code reviewing if anyone in the team can grok https://gerrit.wikimedia.org/r/c/562380 and give it a +1 that would be helpful

Change 564156 merged by jenkins-bot:
[mediawiki/core@master] "Enable 'site' and 'site.styles' modules on mobile target (take 2)

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

Change 562380 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Remove mobile.site and mobile.styles modules

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

Jdlrobson closed this task as Resolved.Fri, Mar 13, 9:53 PM
Jdlrobson claimed this task.
Jdlrobson updated the task description. (Show Details)

This LGTM

Checking Common.css vs Mobile.css in style tag

Mobile is Empty (styles are loaded in site module on mobile)
desktop not empty (loads content of MediaWiki:Common.css)

Checking JS

mobile contains Mobile.css and Mobile.js as expected
desktop is empty - there is no Common.js on the beta cluster.