HTMLMediaElement::LoadFromSourceChildren() flushes layout

RESOLVED FIXED in Firefox 56

Status

()

Core
Audio/Video
P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: mats, Assigned: mats)

Tracking

({perf})

Trunk
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

a year ago
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.
Flags: needinfo?(cpearce)
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.
Flags: needinfo?(cpearce)
(Assignee)

Comment 3

a year ago
Created attachment 8880412 [details] [diff] [review]
Remove a FlushPendingNotifications that we don't need anymore

Yeah, that makes sense.
Attachment #8880014 - Attachment is obsolete: true
Attachment #8880412 - Flags: review?(cpearce)
Comment on attachment 8880412 [details] [diff] [review]
Remove a FlushPendingNotifications that we don't need anymore

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

Thanks!
Attachment #8880412 - Flags: review?(cpearce) → review+

Updated

a year ago
Whiteboard: [qf] → [qf:p1]

Comment 5

a year ago
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d1a43684bde
Remove a FlushPendingNotifications that we don't need anymore.  r=cpearce

Comment 6

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6d1a43684bde
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.