Page MenuHomePhabricator

Move OutputPage::setTarget() call from BeforePageDisplay to an earlier hook
Closed, ResolvedPublic

Description

Background:

Having confidence in progress made on T127268 is dependent on Logstash telemetry from violations, which would currently be too spammy due to Gadgets being loaded unconditionally and violating the targets filter by default without a way for users to fix that. (T171180).

Analysis from @TheDJ shows that the way Gadgets enqueues the enabled Gadget modules could easily filter by target, but this is currently not working as intended because the OutputPage::mTarget member is not set by the time Gadgets queues its modules.

Further progress on the matter is blocked on moving the setTarget() call in MobileFrontend to an earlier hook. This is expected to be uncontroversial given the target state is declarative and aside from ResourceLoader (which only consumes it upon skin rendering) nothing reads from this variable.

Developer notes

  • Existing code inside the onBeforePageDisplay hook setting target is removed.
  • The following code is added to onOutputPageBeforeHTML in includes/MobileFrontend.hooks.php:
		if ( $context->shouldDisplayMobileView() ) {
						// set the mobile target
						if ( !$context->isBlacklistedPage() ) {
							$out->setTarget( 'mobile' );
						}
		}

QA steps

  • When I visit a mobile page all requests for JavaScript and stylesheets append the query string parameter target=mobile

Acceptance criteria / Sign off steps

  • There has been no increase in JS/CSS related to this change
  • MediaWiki:Mobile.css/js is loaded on the mobile domain
  • MediaWiki:Common.css/js is not loaded on the mobile domain

Details

Related Gerrit Patches:
mediawiki/extensions/MobileFrontend : masterMove target=mobile override to 'BeforeInitialize' hook
mediawiki/extensions/MobileFrontend : masterAvoid expensive WebRequest::getText() in early init code

Event Timeline

Krinkle created this task.Jan 9 2019, 5:29 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 9 2019, 5:29 AM
Jdlrobson triaged this task as Low priority.Jan 9 2019, 9:25 PM

Further progress on the matter is blocked on moving the setTarget() call in MobileFrontend to an earlier hook.

Which hook were you thinking of? onBeforePageDisplay is the one we currently use. I don't know exactly what early needs, but this should be non-controversial and easy to get done once we know where to put it.

BeforePageDisplay is the very last (relevant) hook that can be used to change the state of OutputPage. It is most commonly used to queue modules.

MobileFrontend needs to be changed to change the "target" state from an earlier hook (any will do, can be sightly earlier, or very early; doesn't matter) so that e.g. Gadgets can queue its modules in a way that varies by the target.

Working on this is just blocked on deciding which hook to use - any straw-man proposals off the top of your head? Is hook order documented anywhere?

Jdlrobson updated the task description. (Show Details)
Krinkle added a comment.EditedFeb 19 2019, 1:13 PM

Hm.. I couldn't tell whether that actually runs first, nor whether it only runs once. But yeah, if that works, let's do it!

Literally any hook that runs on page views before BeforePageDisplay will do.

Change 539722 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/MobileFrontend@master] Avoid expensive WebRequest::getText() in early init code

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

Change 539723 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/MobileFrontend@master] Move target=mobile override to MediaWikiPerformAction hook

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

Krinkle moved this task from Triaged but Future to Incoming on the Readers-Web-Backlog board.

Change 539722 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Avoid expensive WebRequest::getText() in early init code

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

Change 539723 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Move target=mobile override to 'BeforeInitialize' hook

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

phuedx claimed this task.Oct 9 2019, 5:04 PM
phuedx updated the task description. (Show Details)Tue, Oct 15, 1:15 PM
phuedx updated the task description. (Show Details)EditedTue, Oct 15, 1:21 PM

MediaWiki:Mobile.css/js is loaded on the mobile domain

Confirmed. I still see requests for the mobile.site and mobile.site.styles modules, which contain the contents of the MediaWiki:Mobile.{css,js} pages.

phuedx updated the task description. (Show Details)Tue, Oct 15, 1:25 PM

I tested this on enwiki in production. I enabled a couple of mobile-specific gadgets (presumably @TheDJ's!) and proving to myself that they were only loading when I was accessing the site using a (virtual) mobile device. I saw no relevant errors in the JavaScript console while testing.

phuedx closed this task as Resolved.Tue, Oct 15, 1:36 PM