Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2021573002:
MSE: Reset delaying-the-load-event-flag on attachment (Closed)
|
Created:
4 years, 6 months ago by wolenetz Modified:
4 years, 3 months ago CC:
blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, fs, gasubic, mlamouri+watch-blink_chromium.org, nessy, vcarbune.chromium Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMSE: Reset delaying-the-load-event-flag on attachment
Currently, the earliest non-error case reset of media element's
delaying-the-load-event-flag when MSE is attached occurs at either the
'stalled' event scheduling or once first reaching >= HAVE_CURRENT_DATA.
This change makes Blink compliant with recent MSE spec
clarification [1] that resets this flag immediately upon MSE attachment.
Non-MSE media element delaying-the-load-event-flag behavior remains
unchanged.
[1] https://github.com/w3c/media-source/pull/85
TEST=http/tests/media/media-source/mediasource-attach-stops-delaying-load-event.html
BUG=615481
Committed: https://crrev.com/ea101249f079972e2dd6b6448bb73c1b0437c8d8
Cr-Commit-Position: refs/heads/master@{#401516}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Rebase #Patch Set 3 : Addressed mlamouri's comment intent: load must occur before sourceopen in test #
Total comments: 9
Patch Set 4 : rebase #Patch Set 5 : Addressed foolip@'s nits #
Messages
Total messages: 39 (12 generated)
The CQ bit was checked by wolenetz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2021573002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2021573002/1
Please take a look: mlamouri@: OWNER review of HTMLME.cpp, committer review of the layout test chcunningham@: FYI - review if you wish. Note, with just the new layout test, but not the .cpp change, the test fails as expected with result: This is a testharness.js-based test. FAIL MediaSource attachment should immediately stop delaying the load event assert_equals: document should be complete expected "complete" but got "interactive" Harness: the test ran to completion.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with comment applied. https://codereview.chromium.org/2021573002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-stops-delaying-load-event.html (right): https://codereview.chromium.org/2021573002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-stops-delaying-load-event.html:28: // Let any pending "load" event also fire. Then verify we've received one. Shouldn't you `assert_false(isLoaded)` here? I believe Blink should not have fired the event at that point.
https://codereview.chromium.org/2021573002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-stops-delaying-load-event.html (right): https://codereview.chromium.org/2021573002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-stops-delaying-load-event.html:28: // Let any pending "load" event also fire. Then verify we've received one. On 2016/05/31 15:30:48, Mounir Lamouri (slow) wrote: > Shouldn't you `assert_false(isLoaded)` here? I believe Blink should not have > fired the event at that point. Good point, that was my intent and I missed adding it! Thanks for the review. I'll fix and CQ this shortly.
On 2016/05/31 17:50:53, wolenetz_slow_reviews wrote: > https://codereview.chromium.org/2021573002/diff/1/third_party/WebKit/LayoutTe... > File > third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-stops-delaying-load-event.html > (right): > > https://codereview.chromium.org/2021573002/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-stops-delaying-load-event.html:28: > // Let any pending "load" event also fire. Then verify we've received one. > On 2016/05/31 15:30:48, Mounir Lamouri (slow) wrote: > > Shouldn't you `assert_false(isLoaded)` here? I believe Blink should not have > > fired the event at that point. > > Good point, that was my intent and I missed adding it! Thanks for the review. > I'll fix and CQ this shortly. s/and CQ// until foolip@'s related questions on https://crbug.com/615481 are resolved.
https://codereview.chromium.org/2021573002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-stops-delaying-load-event.html (right): https://codereview.chromium.org/2021573002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-stops-delaying-load-event.html:28: // Let any pending "load" event also fire. Then verify we've received one. On 2016/05/31 17:50:53, wolenetz_slow_reviews wrote: > On 2016/05/31 15:30:48, Mounir Lamouri (slow) wrote: > > Shouldn't you `assert_false(isLoaded)` here? I believe Blink should not have > > fired the event at that point. > > Good point, that was my intent and I missed adding it! Thanks for the review. > I'll fix and CQ this shortly. I looked a little closer and have further question around this: are we 100% guaranteed that HTMLMediaElement's "load" event will not have been fired prior to MediaSource's "sourceopen" event? Though they are enqueued with sourceopen event first, they are enqueued on different objects. Could that sequencing thus become reversed? Also, even if Blink guarantees ordering, when I upstream this MSE test to the interop compliance suite (w3c/web-platform-tests), we may not have the same guarantee, so the assert_false(isLoaded) may be flaky there. Would a better test that obviates any reliance on MS.onsourceopen vs HTMLME.onload ordering be desired here (e.g. set it up so that the verification logic runs only once *both* have fired, regardless of their ordering)?
If the events are enqueued in the same task source in a specific order, I think it is fine to assume they will happen in a similar order. They actually should happen in that order per spec at that point.
On 2016/06/03 14:39:09, Mounir Lamouri (slow) wrote: > If the events are enqueued in the same task source in a specific order, I think > it is fine to assume they will happen in a similar order. They actually should > happen in that order per spec at that point. That makes sense. I hadn't internalized any of HTML5 "task source" specs before this. Thanks for the opportunity :)
On 2016/06/15 20:27:37, wolenetz_slow_reviews wrote: > On 2016/06/03 14:39:09, Mounir Lamouri (slow) wrote: > > If the events are enqueued in the same task source in a specific order, I > think > > it is fine to assume they will happen in a similar order. They actually should > > happen in that order per spec at that point. > > That makes sense. I hadn't internalized any of HTML5 "task source" specs before > this. Thanks for the opportunity :) Hmm. Your suggested assert_false(isLoaded) at top of "sourceopen" event handler fails. I'm investigating how the load event could be scheduled and fired so early.
On 2016/06/15 20:58:47, wolenetz_slow_reviews wrote: > On 2016/06/15 20:27:37, wolenetz_slow_reviews wrote: > > On 2016/06/03 14:39:09, Mounir Lamouri (slow) wrote: > > > If the events are enqueued in the same task source in a specific order, I > > think > > > it is fine to assume they will happen in a similar order. They actually > should > > > happen in that order per spec at that point. > > > > That makes sense. I hadn't internalized any of HTML5 "task source" specs > before > > this. Thanks for the opportunity :) > > Hmm. Your suggested assert_false(isLoaded) at top of "sourceopen" event handler > fails. > I'm investigating how the load event could be scheduled and fired so early. I added some log statements locally. Net-net: Though enqueue of MediaSource "sourceopen" event occurs first, Document::implicitClose() occurs prior to "sourceopen" firing. And Document::implicitClose() synchronously calls LocalDOMWindow::documentWasClosed(), which synchronously calls dispatchWindowLoadEvent(). tl;dr: task to fire "load" event is enqueued *before* the "sourceopen" in both our code and in the current MSE spec [1] [1]: ... Set the media element's delaying-the-load-event-flag to false. Set the readyState attribute to "open". Queue a task to fire a simple event named sourceopen at the MediaSource. ... Specs actually align with this, so I'll update the layout test to verify !isSourceOpened in the 'load' event handler, and assert(isLoaded) without any further setTimeout in the sourceopen handler.
On 2016/06/15 21:55:18, wolenetz_slow_reviews wrote: > On 2016/06/15 20:58:47, wolenetz_slow_reviews wrote: > > On 2016/06/15 20:27:37, wolenetz_slow_reviews wrote: > > > On 2016/06/03 14:39:09, Mounir Lamouri (slow) wrote: > > > > If the events are enqueued in the same task source in a specific order, I > > > think > > > > it is fine to assume they will happen in a similar order. They actually > > should > > > > happen in that order per spec at that point. > > > > > > That makes sense. I hadn't internalized any of HTML5 "task source" specs > > before > > > this. Thanks for the opportunity :) > > > > Hmm. Your suggested assert_false(isLoaded) at top of "sourceopen" event > handler > > fails. > > I'm investigating how the load event could be scheduled and fired so early. > > I added some log statements locally. Net-net: Though enqueue of MediaSource > "sourceopen" event occurs first, Document::implicitClose() occurs prior to > "sourceopen" firing. And Document::implicitClose() synchronously calls > LocalDOMWindow::documentWasClosed(), which synchronously calls > dispatchWindowLoadEvent(). > tl;dr: task to fire "load" event is enqueued *before* the "sourceopen" in both > our code and in the current MSE spec [1] > > [1]: > ... > Set the media element's delaying-the-load-event-flag to false. > Set the readyState attribute to "open". > Queue a task to fire a simple event named sourceopen at the MediaSource. > ... > > Specs actually align with this, so I'll update the layout test to verify > !isSourceOpened in the 'load' event handler, and assert(isLoaded) without any > further setTimeout in the sourceopen handler. Strike out my last responses text segment "Though enqueue of MediaSource "sourceopen" event occurs first". Otherwise my comment and plan to update this CL is the same.
The CQ bit was checked by wolenetz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2021573002/40001
Please take a look: mlamouri@: re-review the layout-test please foolip@: please review all per our discussion on bug 615481 (comments 9-11). As before, chcunningham@, review if you wish. I won't pend your l*tm unless you chime in :) Thanks!
Description was changed from ========== MSE: Reset delaying-the-load-event-flag on attachment Currently, the earliest non-error case reset of media element's delaying-the-load-event-flag when MSE is attached occurs at either the 'stalled' event scheduling or once first reaching >= HAVE_CURRENT_DATA. This change makes Blink compliant with anticipated MSE spec clarification [1] that resets this flag immediately upon MSE attachment. Non-MSE media element delaying-the-load-event-flag behavior remains unchanged. [1] https://github.com/w3c/media-source/pull/85 TEST=http/tests/media/media-source/mediasource-attach-stops-delaying-load-event.html BUG=615481 R=chcunningham@chromium.org, mlamouri@chromium.org ========== to ========== MSE: Reset delaying-the-load-event-flag on attachment Currently, the earliest non-error case reset of media element's delaying-the-load-event-flag when MSE is attached occurs at either the 'stalled' event scheduling or once first reaching >= HAVE_CURRENT_DATA. This change makes Blink compliant with recent MSE spec clarification [1] that resets this flag immediately upon MSE attachment. Non-MSE media element delaying-the-load-event-flag behavior remains unchanged. [1] https://github.com/w3c/media-source/pull/85 TEST=http/tests/media/media-source/mediasource-attach-stops-delaying-load-event.html BUG=615481 ==========
wolenetz@chromium.org changed reviewers: + foolip@chromium.org
On 2016/06/15 22:11:56, wolenetz_slow_reviews wrote: > Please take a look: > > mlamouri@: re-review the layout-test please > foolip@: please review all per our discussion on bug 615481 (comments 9-11). > > As before, chcunningham@, review if you wish. I won't pend your l*tm unless you > chime in :) > > Thanks! I really added foolip@ to R= now.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/06/15 22:11:56, wolenetz wrote: > Please take a look: > > mlamouri@: re-review the layout-test please > foolip@: please review all per our discussion on bug 615481 (comments 9-11). > > As before, chcunningham@, review if you wish. I won't pend your l*tm unless you > chime in :) > > Thanks! Friendly ping.
lgtm % nits, but it looks like more work is needed on the spec here, and to ensure interop with other implementations. https://codereview.chromium.org/2021573002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-stops-delaying-load-event.html (right): https://codereview.chromium.org/2021573002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-stops-delaying-load-event.html:16: }), false); Trailing false is not needed. https://codereview.chromium.org/2021573002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-stops-delaying-load-event.html:32: assert_true(receivedLoadEvent, "load event should have been received first"); It looks to me like the implementation would first fire the window load event and then the mediaSource's sourceopen event, but the spec requires the opposite. That's because https://html.spec.whatwg.org/#delay-the-load-event waits until after the task that causes the load event to not be delayed before queuing the task, but Document::checkLoadEventSoon() would start that timer while the task is still running. This seems like a detail that could easily be different in different implementations, so sharing a test like this with others would be great :) https://codereview.chromium.org/2021573002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-stops-delaying-load-event.html:40: document.body.appendChild(video); Test should run fine without appending unless there's a GC bug? https://codereview.chromium.org/2021573002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-stops-delaying-load-event.html:43: document.body.removeChild(video); Removing the video from the document ought not matter unless there's a leak or something to work around.
Thanks for the reviews. https://codereview.chromium.org/2021573002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-stops-delaying-load-event.html (right): https://codereview.chromium.org/2021573002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-stops-delaying-load-event.html:16: }), false); On 2016/06/20 09:30:12, Philip Jägenstedt wrote: > Trailing false is not needed. Done. https://codereview.chromium.org/2021573002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-stops-delaying-load-event.html:32: assert_true(receivedLoadEvent, "load event should have been received first"); On 2016/06/20 09:30:12, Philip Jägenstedt wrote: > It looks to me like the implementation would first fire the window load event > and then the mediaSource's sourceopen event, but the spec requires the opposite. > > That's because https://html.spec.whatwg.org/#delay-the-load-event waits until > after the task that causes the load event to not be delayed before queuing the > task, but Document::checkLoadEventSoon() would start that timer while the task > is still running. > > This seems like a detail that could easily be different in different > implementations, so sharing a test like this with others would be great :) Acknowledged. https://codereview.chromium.org/2021573002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-stops-delaying-load-event.html:40: document.body.appendChild(video); On 2016/06/20 09:30:12, Philip Jägenstedt wrote: > Test should run fine without appending unless there's a GC bug? Done. And reconfirmed that without the .cpp portion of this change, this test fails because load happens after sourceopen. https://codereview.chromium.org/2021573002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-stops-delaying-load-event.html:43: document.body.removeChild(video); On 2016/06/20 09:30:12, Philip Jägenstedt wrote: > Removing the video from the document ought not matter unless there's a leak or > something to work around. Done.
The CQ bit was checked by wolenetz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mlamouri@chromium.org, foolip@chromium.org Link to the patchset: https://codereview.chromium.org/2021573002/#ps80001 (title: "Addressed foolip@'s nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2021573002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
still lgtm style: can you remove the <html>, <head> and <body> elements? Make everything with 0 indentation. This is the coding style of Web Platform Tests.
On 2016/06/22 09:08:27, Mounir Lamouri (slow) wrote: > still lgtm > > style: can you remove the <html>, <head> and <body> elements? Make everything > with 0 indentation. This is the coding style of Web Platform Tests. Interesting - I was unaware of that style until you mentioned it now. Thanks for pointing that out. I'll take that reformatting as part of the bulk push to wpt of these MSE layout tests (and I'll update the individual PR https://github.com/w3c/web-platform-tests/pull/3082 for this particular new test added in this CL).
The CQ bit was checked by wolenetz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2021573002/80001
Message was sent while issue was closed.
Description was changed from ========== MSE: Reset delaying-the-load-event-flag on attachment Currently, the earliest non-error case reset of media element's delaying-the-load-event-flag when MSE is attached occurs at either the 'stalled' event scheduling or once first reaching >= HAVE_CURRENT_DATA. This change makes Blink compliant with recent MSE spec clarification [1] that resets this flag immediately upon MSE attachment. Non-MSE media element delaying-the-load-event-flag behavior remains unchanged. [1] https://github.com/w3c/media-source/pull/85 TEST=http/tests/media/media-source/mediasource-attach-stops-delaying-load-event.html BUG=615481 ========== to ========== MSE: Reset delaying-the-load-event-flag on attachment Currently, the earliest non-error case reset of media element's delaying-the-load-event-flag when MSE is attached occurs at either the 'stalled' event scheduling or once first reaching >= HAVE_CURRENT_DATA. This change makes Blink compliant with recent MSE spec clarification [1] that resets this flag immediately upon MSE attachment. Non-MSE media element delaying-the-load-event-flag behavior remains unchanged. [1] https://github.com/w3c/media-source/pull/85 TEST=http/tests/media/media-source/mediasource-attach-stops-delaying-load-event.html BUG=615481 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== MSE: Reset delaying-the-load-event-flag on attachment Currently, the earliest non-error case reset of media element's delaying-the-load-event-flag when MSE is attached occurs at either the 'stalled' event scheduling or once first reaching >= HAVE_CURRENT_DATA. This change makes Blink compliant with recent MSE spec clarification [1] that resets this flag immediately upon MSE attachment. Non-MSE media element delaying-the-load-event-flag behavior remains unchanged. [1] https://github.com/w3c/media-source/pull/85 TEST=http/tests/media/media-source/mediasource-attach-stops-delaying-load-event.html BUG=615481 ========== to ========== MSE: Reset delaying-the-load-event-flag on attachment Currently, the earliest non-error case reset of media element's delaying-the-load-event-flag when MSE is attached occurs at either the 'stalled' event scheduling or once first reaching >= HAVE_CURRENT_DATA. This change makes Blink compliant with recent MSE spec clarification [1] that resets this flag immediately upon MSE attachment. Non-MSE media element delaying-the-load-event-flag behavior remains unchanged. [1] https://github.com/w3c/media-source/pull/85 TEST=http/tests/media/media-source/mediasource-attach-stops-delaying-load-event.html BUG=615481 Committed: https://crrev.com/ea101249f079972e2dd6b6448bb73c1b0437c8d8 Cr-Commit-Position: refs/heads/master@{#401516} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/ea101249f079972e2dd6b6448bb73c1b0437c8d8 Cr-Commit-Position: refs/heads/master@{#401516}
Message was sent while issue was closed.
https://codereview.chromium.org/2021573002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-stops-delaying-load-event.html (right): https://codereview.chromium.org/2021573002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-stops-delaying-load-event.html:32: assert_true(receivedLoadEvent, "load event should have been received first"); On 2016/06/21 22:25:47, wolenetz wrote: > On 2016/06/20 09:30:12, Philip Jägenstedt wrote: > > It looks to me like the implementation would first fire the window load event > > and then the mediaSource's sourceopen event, but the spec requires the > opposite. > > > > That's because https://html.spec.whatwg.org/#delay-the-load-event waits until > > after the task that causes the load event to not be delayed before queuing the > > task, but Document::checkLoadEventSoon() would start that timer while the task > > is still running. > > > > This seems like a detail that could easily be different in different > > implementations, so sharing a test like this with others would be great :) > > Acknowledged. I've filed https://crbug.com/643846 to follow-up on at least Chrome's impl which mismatches spec. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
