Closed
Bug 1375075
Opened 7 years ago
Closed 7 years ago
HTMLMediaElement::LoadFromSourceChildren() flushes layout
Categories
(Core :: Audio/Video, enhancement, P3)
Core
Audio/Video
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox56 | --- | fixed |
People
(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)
Details
(Keywords: perf)
Attachments
(1 file, 1 obsolete file)
|
1.17 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
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)
|
Assignee | |
Comment 1•7 years ago
|
||
|
||
Comment 2•7 years ago
|
||
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•7 years ago
|
||
Yeah, that makes sense.
Attachment #8880014 -
Attachment is obsolete: true
Attachment #8880412 -
Flags: review?(cpearce)
|
|
||
Comment 4•7 years ago
|
||
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•7 years ago
|
Whiteboard: [qf] → [qf:p1]
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•7 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/6d1a43684bde
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
|
||
Updated•2 years ago
|
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in
before you can comment on or make changes to this bug.
Description
•