Page MenuHomePhabricator

Dynamically loading SpecialPage ResourceLoader modules causes unnecessary processing on ALL requests
Closed, ResolvedPublic

Description

Changesets https://gerrit.wikimedia.org/r/#/c/43979/ and https://gerrit.wikimedia.org/r/#/c/44442/ introduce changes that make mobile ResourceLoader modules for SpecialPages get dynamically defined. This is neat, however the current implementation in master (https://gerrit.wikimedia.org/r/#/c/44442/) makes it so that the modules get defined on EVERY request, including API requests, which adds unnecessary overhead.

We initially attempted to have the processing happen ONLY inside of the invocation of the ResourceLoaderRegisterModules hook, which works in MobileFrontend alpha/beta, but not in production. There are a couple of reasons for this:

  1. The ResourceLoaderRegisterModules hook runs AFTER SkinMobile::attachAdditionalPageResources() attaches resources (when RL is not fully in use, eg in production). You can try to get around that by forcing the hook to run by calling OutputPage::getResourceLoader() in SkinMobile::attachAdditionalPageResources(), but that bumps into another problem:
  2. using $resourceLoader->registerModule() in an invocation of the ResourceLoaderRegisterModules hook appears to not work properly for modules that need to be loaded at the bottom of the page. I'll be filing a separate bug for that.

We should get this fixed sooner rather than later. A few options I can think of:


Version: unspecified
Severity: normal
See Also:
https://bugzilla.wikimedia.org/show_bug.cgi?id=41340
https://bugzilla.wikimedia.org/show_bug.cgi?id=44072

Details

Reference
bz44070

Event Timeline

bzimport raised the priority of this task from to Needs Triage.Nov 22 2014, 1:21 AM
bzimport set Reference to bz44070.

Actually it sounds like the issue I mentioned in #2 above is NOTE a ResourceLoader issue, but rather a problem in MobileFrontend: https://bugzilla.wikimedia.org/show_bug.cgi?id=44072

No, the SpecialPage related modules are still getting prepared on every request. They should get handled in MobileFrontendHooks::onResourceLoaderRegisterModules().

Since bug 44072 is fixed now, this should be totally doable.

I've seen this code is proving to cause troubles for people understanding our code, so I put my hand up and say this was a bad idea. Instead I will manually define the modules. Patch on it's way.

I don't know any context behind this bug, but I just want to make a quick point.

Remember that load.php/startup module needs it, since they are loaded not from within the page.

Module definitions mustn't rely on page context. The startup module needs a complete overview (which is by design).