Page MenuHomePhabricator

mw.MwEmbedSupport.style should only load on the pages that need it
Closed, ResolvedPublic

Description

'mw.MwEmbedSupport.style' is injected unconditionally into all pages. There's a TODO in MwEmbedSupport.hooks.php for loading it only when it is needed. It is a proper bug, IMO, since it exacts a nontrivial toll on page performance and bandwidth.


Version: unspecified
Severity: normal

Details

Reference
bz58086

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 2:24 AM
bzimport set Reference to bz58086.

Given ResourceLoader, this should be pretty easy, right?

(In reply to comment #1)

Given ResourceLoader, this should be pretty easy, right?

The difficulty is correctly identified in a comment in TimedMediaHandler.hooks.php:

		// We should probably move this script output to a parser function but not working correctly in
		// dynamic contexts ( for example in special upload, when there is an "existing file" warning. )

In other words, it's trivial to only include the stylesheet on article pages that contain timed media, but that doesn't cover cases in which a page's content is loaded via AJAX and injected into the current page. There are probably only a few such dynamic cases. My preference -- for this and for the JavaScript payload which accompanies it -- would be to expose them by reverting the load-everywhere workaround, and then wait for the bugs to trickle in.

(In reply to comment #2)

(In reply to comment #1)

Given ResourceLoader, this should be pretty easy, right?

The difficulty is correctly identified in a comment in
TimedMediaHandler.hooks.php:

// We should probably move this script output to a parser function

but
not working correctly in

// dynamic contexts ( for example in special upload, when there is an

"existing file" warning. )

In other words, it's trivial to only include the stylesheet on article pages
that contain timed media, but that doesn't cover cases in which a page's
content is loaded via AJAX and injected into the current page. There are
probably only a few such dynamic cases. My preference -- for this and for the
JavaScript payload which accompanies it -- would be to expose them by
reverting
the load-everywhere workaround, and then wait for the bugs to trickle in.

Special:upload would be the main example. Also possible when fetching file desc from commons on a foreign image (if for example desc includes video but image isn't one)

(In reply to Ori Livneh from comment #2)

My preference -- for this and for
the JavaScript payload which accompanies it -- would be to expose them by
reverting the load-everywhere workaround, and then wait for the bugs to
trickle in.

Sounds sensible.

The fix should take care of the clear cases up front (e.g. categories, Special:ListFiles, Special:Upload (the one flagged in the comment itself), Special:UploadWizard). Any remaining ones that can't be thought of up front can be caught like that.

This was resolved by the fixes for bug 55550, no?

Gilles raised the priority of this task from Medium to Unbreak Now!.Dec 4 2014, 10:19 AM
Gilles added a project: Multimedia.
Gilles moved this task from Untriaged to Done on the Multimedia board.
Gilles lowered the priority of this task from Unbreak Now! to Medium.Dec 4 2014, 11:22 AM