1375075 - HTMLMediaElement::LoadFromSourceChildren() flushes layout

Status: RESOLVED FIXED

(Core :: Audio/Video, enhancement, P3)

Description

[Mats Palmgren (inactive)]

HTMLMediaElement::LoadFromSourceChildren() flushes layout:
http://searchfox.org/mozilla-central/rev/714606a8145636d93b116943d3a65a6a49d2acf8/dom/html/HTMLMediaElement.cpp#2249

This seems unnecessary since the code there only looks at DOM nodes
and attributes, none of which should be affected by layout/style changes.
I think we can get away with just doing a FlushType::ContentAndNotify here.

Try run looks green-ish:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f530321e3d221a3375af3132d410c73696c4201

For background, it appears the flush was added in bug 449363, which
discusses it a bit, Boris mentions in bug 449363 comment 22 that:
"you just want its viewport to have the right size, right?"
but I don't see how that would affect which <source> child is
selected in this method.

Comment 1

[Mats Palmgren (inactive)]

Attachment: fix

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

I think we can remove the flush entirely. It was added in https://hg.mozilla.org/mozilla-central/rev/b15a3c5dd0e6 so that the MediaQueries in the @media attribute of HTMLSourceElement children of HTMLMediaElements would resolve correctly (i.e. reflect the right viewport size), but we removed support for the media attribute in HTMLMediaElement's HTMLSourceElement children in bug 1325053.

So I think we should be able to remove the flush here.

Comment 3

[Mats Palmgren (inactive)]

Attachment: Remove a FlushPendingNotifications that we don't need anymore

Yeah, that makes sense.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8880412 [details] [diff] [review]
Remove a FlushPendingNotifications that we don't need anymore

Review of attachment 8880412 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d1a43684bde
Remove a FlushPendingNotifications that we don't need anymore.  r=cpearce

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6d1a43684bde