Last modified: 2014-11-17 09:21:28 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T57550, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 55550 - Reduce amount of code loaded in MwEmbedSupport and TimedMediaHandler startup and BeforePageDisplay
Reduce amount of code loaded in MwEmbedSupport and TimedMediaHandler startup ...
Status: RESOLVED FIXED
Product: MediaWiki extensions
Classification: Unclassified
TimedMediaHandler (Other open bugs)
master
All All
: High normal with 1 vote (vote)
: ---
Assigned To: Nobody - You can work on this!
: performance
Depends on: 58086
Blocks: 55615 55683 58083
  Show dependency treegraph
 
Reported: 2013-10-10 08:16 UTC by Matthew Flaschen
Modified: 2014-11-17 09:21 UTC (History)
15 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description Matthew Flaschen 2013-10-10 08:16:39 UTC
Whenever possible, modules should be loaded on demand (e.g. specific pages, or in response to a user action), not in startup.  If startup modules are needed, they should be as svelte as possible.

MwEmbedSupport.hooks.php adds five, some of which have their own dependencies: https://git.wikimedia.org/blob/mediawiki%2Fextensions%2FMwEmbedSupport.git/45a59b6ee3b9de6dc764cbd3fd6c9d7b3139718b/MwEmbedSupport.hooks.php#L28
Comment 1 Matthew Flaschen 2013-10-10 09:18:35 UTC
The same goes for BeforePageDisplay in TimedMediaHandler.  BTW, I'm fine with splitting this bug, but there is no component for MwEmbedSupport yet.
Comment 2 Gerrit Notification Bot 2013-10-10 09:29:44 UTC
Change 88943 had a related patch set uploaded by Mattflaschen:
Remove dialogFitWindow and jquery.ui.dialog from mwEmbedUtil

https://gerrit.wikimedia.org/r/88943
Comment 3 Gerrit Notification Bot 2013-10-10 09:34:14 UTC
Change 88944 had a related patch set uploaded by Mattflaschen:
Correct mw.PopUpMediaTransform dependency

https://gerrit.wikimedia.org/r/88944
Comment 4 Matthew Flaschen 2013-10-10 09:39:51 UTC
On my local wiki, those are enough to stop it from loading jquery.ui.dialog (and all of its dependencies, e.g. jquery.ui.core) on unrelated pages (e.g. the Main Page).

Thanks to Ori for his new mw.loader.inspect method which led me into checking this out.
Comment 5 Matthew Flaschen 2013-10-10 09:42:56 UTC
https://git.wikimedia.org/blob/mediawiki%2Fextensions%2FTimedMediaHandler.git/af412751c8bea1012f9715c16d49ffa5ca29f7aa/TimedMediaHandler.hooks.php#L350 is also double-loading the mw.PopUpMediaTransform CSS.

I'm pretty sure the CSS is useless without the JavaScript.  If so, I think the addModuleStyles can be removed.
Comment 6 Gerrit Notification Bot 2013-10-11 14:31:02 UTC
Change 88944 merged by Mdale:
Correct mw.PopUpMediaTransform dependency

https://gerrit.wikimedia.org/r/88944
Comment 7 spage 2013-10-26 07:02:21 UTC
Matt, great sleuthing.

I agree the 
        $out->addModuleStyles( 'mw.PopUpMediaTransform' );
is redundant, also probably
        $out->addModuleStyles( 'mw.PopUpMediaTransform' );
for the similar parser hook.

TMH's BeforePageOutput hook loads 'mw.PopUpMediaTransform', which specifies the dependency on 'mw.MwEmbedSupport'.  But MwEmbedSupport in addStartupModules() explicitly loads 'mw.MwEmbedSupport' and all the files that it depends on. I think none of that should be necessary with latest MediaWiki, maybe it's there for backward compatibility with 1.17.  I submitted Gerrit change #92052.
Comment 8 Gerrit Notification Bot 2013-10-26 07:06:36 UTC
Change 88943 merged by jenkins-bot:
Remove dialogFitWindow and jquery.ui.dialog from mwEmbedUtil

https://gerrit.wikimedia.org/r/88943
Comment 9 MZMcBride 2013-10-29 15:31:40 UTC
It looks like <https://gerrit.wikimedia.org/r/88943> and <https://gerrit.wikimedia.org/r/88944> have been merged. I suppose we're waiting on <https://gerrit.wikimedia.org/r/92052> to be merged in order to mark this bug as resolved/fixed.
Comment 10 Matthew Flaschen 2013-10-31 04:45:30 UTC
This will stop jquery.ui from loading by default on some WMF wikis.  Specifically, those where only jquery.mwEmbedUtil depends on jquery.ui.dialog, and nothing else depends on the part of jquery.ui in question.

For gadgets and user scripts, they just need to declare their dependency, either using the gadget syntax or mw.loader.using.  Scripts should do this in general.

However, I thought of another issue.  If sites are using jquery.ui buttons in regular wikitext, that will also stop working unless jquery.ui.button is loaded some other way (e.g. English Wikipedia has ext.gadget.teahouse loaded by default, and it depends on jquery.ui.button).

See https://en.wikipedia.org/wiki/Template:Clickable_button_2 for an example of this in use.

This will roll out with 1.23wmf2, which means it hits the first non 'test' wiki on the 4th.  I'll also send out an email to wikitech-ambassadors about this.
Comment 11 Ori Livneh 2013-10-31 05:02:49 UTC
(In reply to comment #10)
> This will stop jquery.ui from loading by default on some WMF wikis. 
> Specifically, those where only jquery.mwEmbedUtil depends on
> jquery.ui.dialog,
> and nothing else depends on the part of jquery.ui in question.

Well done! This is fantastic work.
Comment 12 Krinkle 2013-11-05 00:50:03 UTC
(In reply to comment #7)
> Matt, great sleuthing.
> 
> I agree the 
>         $out->addModuleStyles( 'mw.PopUpMediaTransform' );
> is redundant, [..]
> 
> TMH's BeforePageOutput hook loads 'mw.PopUpMediaTransform'
> [..]  I submitted Gerrit change #92052.

One thing to keep in mind when "fixing" issues like this is the styling if non-JavaScript elements (eg. PHP-served HTML output) as, unless position/top is set these modules will load async and would cause a FOUC.

Of course, that doesn't justify silly code that loads the stylesheet twice but it isn't always obvious that one can simply be removed.
Comment 14 Gerrit Notification Bot 2013-12-06 08:01:44 UTC
Change 99597 had a related patch set uploaded by Ori.livneh:
Don't load mw.PopUpMediaTransform unconditionally

https://gerrit.wikimedia.org/r/99597
Comment 15 Gerrit Notification Bot 2014-01-21 03:01:04 UTC
Change 99597 merged by jenkins-bot:
Only load mw.PopUpMediaTransform on pages that plausibly need it

https://gerrit.wikimedia.org/r/99597
Comment 16 Gerrit Notification Bot 2014-01-21 03:34:09 UTC
Change 108649 had a related patch set uploaded by Ori.livneh:
Only load mw.PopUpMediaTransform on pages that plausibly need it

https://gerrit.wikimedia.org/r/108649
Comment 17 Gerrit Notification Bot 2014-01-21 03:37:04 UTC
Change 108650 had a related patch set uploaded by Ori.livneh:
Only load mw.PopUpMediaTransform on pages that plausibly need it

https://gerrit.wikimedia.org/r/108650
Comment 18 Gerrit Notification Bot 2014-01-21 03:37:36 UTC
Change 108649 merged by jenkins-bot:
Only load mw.PopUpMediaTransform on pages that plausibly need it

https://gerrit.wikimedia.org/r/108649
Comment 19 Gerrit Notification Bot 2014-01-21 03:37:44 UTC
Change 108650 merged by jenkins-bot:
Only load mw.PopUpMediaTransform on pages that plausibly need it

https://gerrit.wikimedia.org/r/108650
Comment 20 Andre Klapper 2014-03-13 11:42:48 UTC
All patches merged -> resetting status. 

Is more work planned to do here, or shall this be closed as FIXED?
Comment 21 Matthew Flaschen 2014-03-21 20:32:12 UTC
Marking FIXED.  If another part is found and we want a bug, someone can re-open or file something more specific.
Comment 22 Ori Livneh 2014-04-20 23:02:21 UTC
Not fixed: MwEmbedSupport.hooks.php still says "TODO look into loading this on-demand instead of all pages", as well it should. There is no reason for loading these modules on all pages: 'mw.MwEmbedSupport.style', 'jquery.triggerQueueCallback', 'Spinner', 'jquery.loadingSpinner', 'jquery.mwEmbedUtil', 'mw.MwEmbedSupport'.

Re-opening and bumping priority, for two reasons:
1) It's a site performance issue. A longstanding one, but no less severe for it.
2) The hook that it depends on the ResourceLoaderGetStartupModules hook, which is deprecated as of Ic48ad39c6.
Comment 23 Gerrit Notification Bot 2014-07-11 17:02:24 UTC
Change 145584 had a related patch set uploaded by Gilles:
Load most of TMH and its dependencies on demand

https://gerrit.wikimedia.org/r/145584
Comment 24 Gerrit Notification Bot 2014-07-11 17:03:00 UTC
Change 145583 had a related patch set uploaded by Gilles:
Load most of TMH and its dependencies on demand

https://gerrit.wikimedia.org/r/145583
Comment 25 Gerrit Notification Bot 2014-07-11 20:02:48 UTC
Change 145584 merged by jenkins-bot:
Load most of TMH and its dependencies on demand

https://gerrit.wikimedia.org/r/145584
Comment 26 Gerrit Notification Bot 2014-07-11 20:02:55 UTC
Change 145583 merged by jenkins-bot:
Load most of TMH and its dependencies on demand

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

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links