Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1481913002:
Remove userCancelledLoad and refactor the code at the call sites (Closed)
|
Created:
5 years ago by Srirama Modified:
5 years ago Reviewers:
philipj_slow CC:
chromium-reviews, nessy, mlamouri+watch-blink_chromium.org, blink-reviews-html_chromium.org, gasubic, fs, eric.carlson_apple.com, feature-media-reviews_chromium.org, dglazkov+blink, blink-reviews, vcarbune.chromium Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove userCancelledLoad and refactor the code at the call sites
The function userCancelledLoad isn't called when the user cancels
the load (there's no way to do that) and yet it implements that
bit of the spec. So removing this function and adding only
the necessary bits at the call sites.
Committed: https://crrev.com/fa2a5db2eb8b92301b06f65aa644aef1a5bf709c
Cr-Commit-Position: refs/heads/master@{#363193}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Address review comments #
Total comments: 2
Patch Set 3 : Add test case #Patch Set 4 : Updated test case #
Total comments: 13
Patch Set 5 : Address comments and fix failing test #Patch Set 6 : Add Doctype in the test case #
Total comments: 4
Patch Set 7 : address nits #
Messages
Total messages: 35 (12 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== fix BUG= ========== to ========== Rename and refactor userCancelledLoad as per its usage The function userCancelledLoad isn't called when the user cancels load (there's no way to do that) and yet it implements that bit of the spec. So cleaning it up as per its actual usage. ==========
srirama.m@samsung.com changed reviewers: + philipj@opera.com
As per our discussion in https://codereview.chromium.org/1416073002/#msg27 cleaned up the userCancelledLoad function. PTAL.
Promising cleanup! https://codereview.chromium.org/1481913002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1481913002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:387: // which could result in cancelledLoad() being called back. This comment seems inaccurate, destroying the player doesn't cause cancelledLoad() to be called, at least not "called back." The bug is 403, but I assume that if userCancelledLoad() was called, it was via stop(). It doesn't really make sense to set something in the destructor for the benefit of a later call on that stale object, but perhaps it fixed a crash that was otherwise too hard to track down. I think that if cancelledLoad() is simplified a bit more to not use m_isFinalizing at all, this could be removed. https://codereview.chromium.org/1481913002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2979: void HTMLMediaElement::cancelledLoad() OK, so this is called from didMoveToNewDocument() and stop(). I don't think both call sites actually need the same things. In didMoveToDocument(), the key thing is that the WebMediaPlayer must be destroyed, but otherwise it would be nice if the element were still usable. How about replacing this with the equivalent of "invoke the resource load algorithm", i.e. the same two lines that are in parseAttribute for srcAttr? It still wouldn't be per spec, but could at least be explained in terms of the spec. In stop(), I think it's enough to just clear the player and reset enough state such that hasPendingActivity() will return false. In other words, clear m_shouldDelayLoadEvent and m_networkState. I think the rest is already taken care of. It should in other words be possible to remove this method entirely. https://codereview.chromium.org/1481913002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3051: // Close the async event queue so that no events are enqueued by cancelledLoad. If cancelledLoad() is removed, in principle I think you could remove the ->close() call, but I'd keep it and "so that no further events can be enqueued."
Modified as per review comments but scheduleDelayedAction(LoadMediaResource); in didMoveToNewDocument is creating problem. https://codereview.chromium.org/1481913002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1481913002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:387: // which could result in cancelledLoad() being called back. On 2015/11/27 14:01:28, philipj wrote: > This comment seems inaccurate, destroying the player doesn't cause > cancelledLoad() to be called, at least not "called back." The bug is 403, but I > assume that if userCancelledLoad() was called, it was via stop(). > > It doesn't really make sense to set something in the destructor for the benefit > of a later call on that stale object, but perhaps it fixed a crash that was > otherwise too hard to track down. > > I think that if cancelledLoad() is simplified a bit more to not use > m_isFinalizing at all, this could be removed. Done. https://codereview.chromium.org/1481913002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2979: void HTMLMediaElement::cancelledLoad() On 2015/11/27 14:01:27, philipj wrote: > OK, so this is called from didMoveToNewDocument() and stop(). I don't think both > call sites actually need the same things. > > In didMoveToDocument(), the key thing is that the WebMediaPlayer must be > destroyed, but otherwise it would be nice if the element were still usable. How > about replacing this with the equivalent of "invoke the resource load > algorithm", i.e. the same two lines that are in parseAttribute for srcAttr? It > still wouldn't be per spec, but could at least be explained in terms of the > spec. > > In stop(), I think it's enough to just clear the player and reset enough state > such that hasPendingActivity() will return false. In other words, clear > m_shouldDelayLoadEvent and m_networkState. I think the rest is already taken > care of. > > It should in other words be possible to remove this method entirely. Done, But the changes for didMoveToNewDocument() are creating problem for gc-pending-event-inactive-document.html test, it is timing out. https://codereview.chromium.org/1481913002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3051: // Close the async event queue so that no events are enqueued by cancelledLoad. On 2015/11/27 14:01:28, philipj wrote: > If cancelledLoad() is removed, in principle I think you could remove the > ->close() call, but I'd keep it and "so that no further events can be enqueued." Acknowledged.
I suppose that the problem with gc-pending-event-inactive-document.html is the cancelPendingEventsAndCallbacks() call in HTMLMediaElement::prepareForLoad()? That test both moves the element and causes it to be GC'd, can you write a new test for just moving to a new document? That should indeed cancel events and start a new load. So getting the volumechange event should fail the test, and then you can pass the test if you get an emptied event and if the media element manages to load all the way to the loadeddata event. https://codereview.chromium.org/1481913002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1481913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3020: // Close the async event queue so that no events are enqueued by cancelledLoad. Update this comment. https://codereview.chromium.org/1481913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3024: clearMediaPlayer(-1); I'd say move these bits into the "Stop the playback without generating events" bit, with these three lines before the existing three.
On 2015/11/30 12:55:17, philipj wrote: > I suppose that the problem with gc-pending-event-inactive-document.html is the > cancelPendingEventsAndCallbacks() call in HTMLMediaElement::prepareForLoad()? Yes, this will be the reason. > That test both moves the element and causes it to be GC'd, can you write a new > test for just moving to a new document? That should indeed cancel events and > start a new load. So getting the volumechange event should fail the test, and > then you can pass the test if you get an emptied event and if the media element > manages to load all the way to the loadeddata event. So do you want to me to replace the existing test with a new async_test and test for emptied and loadeddata events? I tried a few experiments by just having the element moved to a new document in the test 1) volumechange is fired without the patch and is not fired with the patch 2) emptied event is not fired but when i add src attribute to the audio element, it is fired. (should we add emptied event in didMoveToNewDocument after clearing the player?) 3) Even after adding src attribute, there is no loadeddata or loadedmetadata event. So our aim is to get the element till loadeddata state even after clearing the player in didMoveToNewDocument? > https://codereview.chromium.org/1481913002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): > > https://codereview.chromium.org/1481913002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3020: // Close the > async event queue so that no events are enqueued by cancelledLoad. > Update this comment. > > https://codereview.chromium.org/1481913002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3024: > clearMediaPlayer(-1); > I'd say move these bits into the "Stop the playback without generating events" > bit, with these three lines before the existing three.
On 2015/11/30 14:47:28, Srirama wrote:
> On 2015/11/30 12:55:17, philipj wrote:
> > I suppose that the problem with gc-pending-event-inactive-document.html is
the
> > cancelPendingEventsAndCallbacks() call in
HTMLMediaElement::prepareForLoad()?
>
> Yes, this will be the reason.
>
> > That test both moves the element and causes it to be GC'd, can you write a
new
> > test for just moving to a new document? That should indeed cancel events and
> > start a new load. So getting the volumechange event should fail the test,
and
> > then you can pass the test if you get an emptied event and if the media
> element
> > manages to load all the way to the loadeddata event.
>
> So do you want to me to replace the existing test with a new async_test and
test
> for emptied and loadeddata events?
> I tried a few experiments by just having the element moved to a new document
in
> the test
> 1) volumechange is fired without the patch and is not fired with the patch
> 2) emptied event is not fired but when i add src attribute to the audio
element,
> it is fired. (should we add emptied event in didMoveToNewDocument after
clearing
> the player?)
> 3) Even after adding src attribute, there is no loadeddata or loadedmetadata
> event.
>
> So our aim is to get the element till loadeddata state even after clearing the
> player in didMoveToNewDocument?
I think the test should be roughly on this form:
async_test(function() {
var v = document.querySelector('video');
v.src = something;
v.onloadeddata = this.step_func(function() {
v.onloadeddata = null;
assert_true(v.networkState == v.NETWORK_IDLE || v.networkState ==
v.NETWORK_LOADING);
assert_greater_than(v.readyState, v.HAVE_NOTHING);
moveToOtherDocument(v);
assert_equals(v.networkState, v.NETWORK_NO_SOURCE);
assert_equals(v.readyState, v.HAVE_NOTHING);
var events = [];
// set up some event handlers that push the event type to events
// once the loadeddata event is received, check that events is [emptied,
loadstart, loadeddata]
});
});
As for the existing test, I think it should be rewritten to test only the GC
case without moving the element, and the event should be fired.
On 2015/11/30 15:16:19, philipj wrote:
> On 2015/11/30 14:47:28, Srirama wrote:
> > On 2015/11/30 12:55:17, philipj wrote:
> > > I suppose that the problem with gc-pending-event-inactive-document.html is
> the
> > > cancelPendingEventsAndCallbacks() call in
> HTMLMediaElement::prepareForLoad()?
> >
> > Yes, this will be the reason.
> >
> > > That test both moves the element and causes it to be GC'd, can you write a
> new
> > > test for just moving to a new document? That should indeed cancel events
and
> > > start a new load. So getting the volumechange event should fail the test,
> and
> > > then you can pass the test if you get an emptied event and if the media
> > element
> > > manages to load all the way to the loadeddata event.
> >
> > So do you want to me to replace the existing test with a new async_test and
> test
> > for emptied and loadeddata events?
> > I tried a few experiments by just having the element moved to a new document
> in
> > the test
> > 1) volumechange is fired without the patch and is not fired with the patch
> > 2) emptied event is not fired but when i add src attribute to the audio
> element,
> > it is fired. (should we add emptied event in didMoveToNewDocument after
> clearing
> > the player?)
> > 3) Even after adding src attribute, there is no loadeddata or loadedmetadata
> > event.
> >
> > So our aim is to get the element till loadeddata state even after clearing
the
> > player in didMoveToNewDocument?
>
> I think the test should be roughly on this form:
>
> async_test(function() {
> var v = document.querySelector('video');
> v.src = something;
> v.onloadeddata = this.step_func(function() {
> v.onloadeddata = null;
> assert_true(v.networkState == v.NETWORK_IDLE || v.networkState ==
> v.NETWORK_LOADING);
> assert_greater_than(v.readyState, v.HAVE_NOTHING);
> moveToOtherDocument(v);
> assert_equals(v.networkState, v.NETWORK_NO_SOURCE);
> assert_equals(v.readyState, v.HAVE_NOTHING);
> var events = [];
> // set up some event handlers that push the event type to events
> // once the loadeddata event is received, check that events is [emptied,
> loadstart, loadeddata]
> });
> });
>
> As for the existing test, I think it should be rewritten to test only the GC
> case without moving the element, and the event should be fired.
Added test case, but there seems to be some problem with moving the video to new
document. Video is trying to load and fails strangely as the url is coming empty
in selectMediaResource. Please let me know if you have some inputs, i will check
further why it is coming empty.
On 2015/12/02 07:54:24, Srirama wrote: > Added test case, but there seems to be some problem with moving the video to new > document. Video is trying to load and fails strangely as the url is coming empty > in selectMediaResource. Please let me know if you have some inputs, i will check > further why it is coming empty. Are you getting all the way to the resource fetch algorithm (loadResource), or does the resource selection algorithm (selectMediaResource) not find any element?
On 2015/12/02 08:29:08, philipj wrote:
> On 2015/12/02 07:54:24, Srirama wrote:
> > Added test case, but there seems to be some problem with moving the video to
> new
> > document. Video is trying to load and fails strangely as the url is coming
> empty
> > in selectMediaResource. Please let me know if you have some inputs, i will
> check
> > further why it is coming empty.
>
> Are you getting all the way to the resource fetch algorithm (loadResource), or
> does the resource selection algorithm (selectMediaResource) not find any
> element?
SelectMediaResource itself is failing.
if (mediaURL.isEmpty()) {
mediaLoadingFailed(WebMediaPlayer::NetworkStateFormatError);
WTF_LOG(Media, "HTMLMediaElement::selectMediaResource(%p), empty 'src'",
this);
return;
}
On 2015/12/02 08:56:46, Srirama wrote:
> On 2015/12/02 08:29:08, philipj wrote:
> > On 2015/12/02 07:54:24, Srirama wrote:
> > > Added test case, but there seems to be some problem with moving the video
to
> > new
> > > document. Video is trying to load and fails strangely as the url is coming
> > empty
> > > in selectMediaResource. Please let me know if you have some inputs, i will
> > check
> > > further why it is coming empty.
> >
> > Are you getting all the way to the resource fetch algorithm (loadResource),
or
> > does the resource selection algorithm (selectMediaResource) not find any
> > element?
>
> SelectMediaResource itself is failing.
> if (mediaURL.isEmpty()) {
> mediaLoadingFailed(WebMediaPlayer::NetworkStateFormatError);
> WTF_LOG(Media, "HTMLMediaElement::selectMediaResource(%p), empty 'src'",
> this);
> return;
> }
Well that's really weird, what did you set the src attribute to? I had
`something` in my example, if you've gotten the media element to
HAVE_CURRENT_DATA before moving it to the new document, there must have been a
src attribute or <source> child elements.
On 2015/12/02 09:05:43, philipj wrote:
> On 2015/12/02 08:56:46, Srirama wrote:
> > On 2015/12/02 08:29:08, philipj wrote:
> > > On 2015/12/02 07:54:24, Srirama wrote:
> > > > Added test case, but there seems to be some problem with moving the
video
> to
> > > new
> > > > document. Video is trying to load and fails strangely as the url is
coming
> > > empty
> > > > in selectMediaResource. Please let me know if you have some inputs, i
will
> > > check
> > > > further why it is coming empty.
> > >
> > > Are you getting all the way to the resource fetch algorithm
(loadResource),
> or
> > > does the resource selection algorithm (selectMediaResource) not find any
> > > element?
> >
> > SelectMediaResource itself is failing.
> > if (mediaURL.isEmpty()) {
> > mediaLoadingFailed(WebMediaPlayer::NetworkStateFormatError);
> > WTF_LOG(Media, "HTMLMediaElement::selectMediaResource(%p), empty 'src'",
> > this);
> > return;
> > }
>
> Well that's really weird, what did you set the src attribute to? I had
> `something` in my example, if you've gotten the media element to
> HAVE_CURRENT_DATA before moving it to the new document, there must have been a
> src attribute or <source> child elements.
It is set to test.ogv, you can have a look at the test case in patchset3. It is
coming to loadeddata and passing all assertions, and once we move it to new
document something is going wrong. It is even strange that the condition
fastHasAttribute(srcAttr) (not sure if it is empty string though) is passing in
selectMediaResource(). I will add more logs and check.
On 2015/12/02 09:14:50, Srirama wrote:
> On 2015/12/02 09:05:43, philipj wrote:
> > On 2015/12/02 08:56:46, Srirama wrote:
> > > On 2015/12/02 08:29:08, philipj wrote:
> > > > On 2015/12/02 07:54:24, Srirama wrote:
> > > > > Added test case, but there seems to be some problem with moving the
> video
> > to
> > > > new
> > > > > document. Video is trying to load and fails strangely as the url is
> coming
> > > > empty
> > > > > in selectMediaResource. Please let me know if you have some inputs, i
> will
> > > > check
> > > > > further why it is coming empty.
> > > >
> > > > Are you getting all the way to the resource fetch algorithm
> (loadResource),
> > or
> > > > does the resource selection algorithm (selectMediaResource) not find any
> > > > element?
> > >
> > > SelectMediaResource itself is failing.
> > > if (mediaURL.isEmpty()) {
> > > mediaLoadingFailed(WebMediaPlayer::NetworkStateFormatError);
> > > WTF_LOG(Media, "HTMLMediaElement::selectMediaResource(%p), empty
'src'",
> > > this);
> > > return;
> > > }
> >
> > Well that's really weird, what did you set the src attribute to? I had
> > `something` in my example, if you've gotten the media element to
> > HAVE_CURRENT_DATA before moving it to the new document, there must have been
a
> > src attribute or <source> child elements.
>
> It is set to test.ogv, you can have a look at the test case in patchset3. It
is
> coming to loadeddata and passing all assertions, and once we move it to new
> document something is going wrong. It is even strange that the condition
> fastHasAttribute(srcAttr) (not sure if it is empty string though) is passing
in
> selectMediaResource(). I will add more logs and check.
It could be HTMLMediaElement::insertedInto that's messing things up with a
second call to scheduleDelayedAction(LoadMediaResource). That actually isn't
supposed to be there per spec, so try commenting it out and see if it's related.
On 2015/12/02 09:35:34, philipj wrote:
> On 2015/12/02 09:14:50, Srirama wrote:
> > On 2015/12/02 09:05:43, philipj wrote:
> > > On 2015/12/02 08:56:46, Srirama wrote:
> > > > On 2015/12/02 08:29:08, philipj wrote:
> > > > > On 2015/12/02 07:54:24, Srirama wrote:
> > > > > > Added test case, but there seems to be some problem with moving the
> > video
> > > to
> > > > > new
> > > > > > document. Video is trying to load and fails strangely as the url is
> > coming
> > > > > empty
> > > > > > in selectMediaResource. Please let me know if you have some inputs,
i
> > will
> > > > > check
> > > > > > further why it is coming empty.
> > > > >
> > > > > Are you getting all the way to the resource fetch algorithm
> > (loadResource),
> > > or
> > > > > does the resource selection algorithm (selectMediaResource) not find
any
> > > > > element?
> > > >
> > > > SelectMediaResource itself is failing.
> > > > if (mediaURL.isEmpty()) {
> > > > mediaLoadingFailed(WebMediaPlayer::NetworkStateFormatError);
> > > > WTF_LOG(Media, "HTMLMediaElement::selectMediaResource(%p), empty
> 'src'",
> > > > this);
> > > > return;
> > > > }
> > >
> > > Well that's really weird, what did you set the src attribute to? I had
> > > `something` in my example, if you've gotten the media element to
> > > HAVE_CURRENT_DATA before moving it to the new document, there must have
been
> a
> > > src attribute or <source> child elements.
> >
> > It is set to test.ogv, you can have a look at the test case in patchset3. It
> is
> > coming to loadeddata and passing all assertions, and once we move it to new
> > document something is going wrong. It is even strange that the condition
> > fastHasAttribute(srcAttr) (not sure if it is empty string though) is passing
> in
> > selectMediaResource(). I will add more logs and check.
>
> It could be HTMLMediaElement::insertedInto that's messing things up with a
> second call to scheduleDelayedAction(LoadMediaResource). That actually isn't
> supposed to be there per spec, so try commenting it out and see if it's
related.
It isn't going there as the networkState is NETWORK_NO_SOURCE, looks to be a
problem with Element::getNonEmptyURLAttribute().
May be something to do with baseUrl or document url, as we haven't set any url
to the new document.
On 2015/12/02 10:02:46, Srirama wrote:
> On 2015/12/02 09:35:34, philipj wrote:
> > On 2015/12/02 09:14:50, Srirama wrote:
> > > On 2015/12/02 09:05:43, philipj wrote:
> > > > On 2015/12/02 08:56:46, Srirama wrote:
> > > > > On 2015/12/02 08:29:08, philipj wrote:
> > > > > > On 2015/12/02 07:54:24, Srirama wrote:
> > > > > > > Added test case, but there seems to be some problem with moving
the
> > > video
> > > > to
> > > > > > new
> > > > > > > document. Video is trying to load and fails strangely as the url
is
> > > coming
> > > > > > empty
> > > > > > > in selectMediaResource. Please let me know if you have some
inputs,
> i
> > > will
> > > > > > check
> > > > > > > further why it is coming empty.
> > > > > >
> > > > > > Are you getting all the way to the resource fetch algorithm
> > > (loadResource),
> > > > or
> > > > > > does the resource selection algorithm (selectMediaResource) not find
> any
> > > > > > element?
> > > > >
> > > > > SelectMediaResource itself is failing.
> > > > > if (mediaURL.isEmpty()) {
> > > > > mediaLoadingFailed(WebMediaPlayer::NetworkStateFormatError);
> > > > > WTF_LOG(Media, "HTMLMediaElement::selectMediaResource(%p), empty
> > 'src'",
> > > > > this);
> > > > > return;
> > > > > }
> > > >
> > > > Well that's really weird, what did you set the src attribute to? I had
> > > > `something` in my example, if you've gotten the media element to
> > > > HAVE_CURRENT_DATA before moving it to the new document, there must have
> been
> > a
> > > > src attribute or <source> child elements.
> > >
> > > It is set to test.ogv, you can have a look at the test case in patchset3.
It
> > is
> > > coming to loadeddata and passing all assertions, and once we move it to
new
> > > document something is going wrong. It is even strange that the condition
> > > fastHasAttribute(srcAttr) (not sure if it is empty string though) is
passing
> > in
> > > selectMediaResource(). I will add more logs and check.
> >
> > It could be HTMLMediaElement::insertedInto that's messing things up with a
> > second call to scheduleDelayedAction(LoadMediaResource). That actually isn't
> > supposed to be there per spec, so try commenting it out and see if it's
> related.
>
> It isn't going there as the networkState is NETWORK_NO_SOURCE, looks to be a
> problem with Element::getNonEmptyURLAttribute().
> May be something to do with baseUrl or document url, as we haven't set any url
> to the new document.
Ah, that's probably right. What is the other document you're moving into? If
it's an iframe that's in the same document that the video is, I think it should
inherit the base URL. If it doesn't, then you could just set the src attribute
to an absolute URL using something like `video.src = new URL('something
relative', document.location)`.
Followed the test case ManualTests/media-elements/video-moved-from-iframe-to-main-page.html and finally made the test case work. If this is fine i can go ahead and refactor the failing test case gc-pending-event-inactive-document.html https://codereview.chromium.org/1481913002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/video-move-to-new-document.html (right): https://codereview.chromium.org/1481913002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-move-to-new-document.html:40: <iframe id="iframe1" srcdoc="<html><body><video controls></video></body></html>"></iframe> Couldn't find a better way than using srcdoc. Keeping iframe in resources folder is causing issues with resolving video src url with respect to iframe and main document.
I like where this is going, test needs a little bit of work and HTMLMediaElement::stop() too, but I think this will work out. https://codereview.chromium.org/1481913002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/video-move-to-new-document.html (right): https://codereview.chromium.org/1481913002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-move-to-new-document.html:2: <html> You can omit <html>, <head> and such: https://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-... You should, however, add a <title>, because otherwise the async_test doesn't get a title at all. (It falls back to document.title if you don't provide one.) https://codereview.chromium.org/1481913002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-move-to-new-document.html:9: function start() { You don't need a start(), just put the script after the things it needs: <video></video> <iframe></iframe> <script> async_test(...). </script> https://codereview.chromium.org/1481913002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-move-to-new-document.html:17: assert_greater_than(video.readyState, video.HAVE_NOTHING); Actually s/HAVE_NOTHING/HAVE_METADATA/ since we just got the loadeddata event. https://codereview.chromium.org/1481913002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-move-to-new-document.html:40: <iframe id="iframe1" srcdoc="<html><body><video controls></video></body></html>"></iframe> On 2015/12/02 13:30:52, Srirama wrote: > Couldn't find a better way than using srcdoc. Keeping iframe in resources folder > is causing issues with resolving video src url with respect to iframe and main > document. What you can do is to have an empty video and an empty iframe in this document. Then resolve the URL you want to use using new URL(mediaURL, document.location) and set the videos src attribute to that. When it's time move the video, just call iframe.contentDocument.body.appendChild(video). https://codereview.chromium.org/1481913002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1481913002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:473: clearMediaPlayer(LoadMediaResource); Perhaps document that this restarts the load, as if the src attribute had been set. https://codereview.chromium.org/1481913002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3026: setNetworkState(NETWORK_EMPTY); Better also clear m_readyState and m_readyStateMaximum, so that we don't end up in an inconsistent state. It is possible for scripts to hold references to HTMLMediaElement instaces after stop() has been called, after all. For the same reason also clear m_currentSourceNode, and call invalidateCachedTime(). Not so sure about cueTimeline().updateActiveCues(0), but since it was done before it's safe to continue doing so.
Patchset #5 (id:100001) has been deleted
Patchset #5 (id:120001) has been deleted
Patchset #5 (id:140001) has been deleted
Thanks for the inputs. Cleaned up as suggested. https://codereview.chromium.org/1481913002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/video-move-to-new-document.html (right): https://codereview.chromium.org/1481913002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-move-to-new-document.html:2: <html> On 2015/12/02 14:02:14, philipj wrote: > You can omit <html>, <head> and such: > https://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-... > > You should, however, add a <title>, because otherwise the async_test doesn't get > a title at all. (It falls back to document.title if you don't provide one.) Done. https://codereview.chromium.org/1481913002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-move-to-new-document.html:9: function start() { On 2015/12/02 14:02:14, philipj wrote: > You don't need a start(), just put the script after the things it needs: > > <video></video> > <iframe></iframe> > <script> > async_test(...). > </script> Done. https://codereview.chromium.org/1481913002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-move-to-new-document.html:17: assert_greater_than(video.readyState, video.HAVE_NOTHING); On 2015/12/02 14:02:14, philipj wrote: > Actually s/HAVE_NOTHING/HAVE_METADATA/ since we just got the loadeddata event. Done. https://codereview.chromium.org/1481913002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-move-to-new-document.html:40: <iframe id="iframe1" srcdoc="<html><body><video controls></video></body></html>"></iframe> On 2015/12/02 14:02:14, philipj wrote: > On 2015/12/02 13:30:52, Srirama wrote: > > Couldn't find a better way than using srcdoc. Keeping iframe in resources > folder > > is causing issues with resolving video src url with respect to iframe and main > > document. > > What you can do is to have an empty video and an empty iframe in this document. > Then resolve the URL you want to use using new URL(mediaURL, document.location) > and set the videos src attribute to that. When it's time move the video, just > call iframe.contentDocument.body.appendChild(video). Done. https://codereview.chromium.org/1481913002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1481913002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:473: clearMediaPlayer(LoadMediaResource); On 2015/12/02 14:02:14, philipj wrote: > Perhaps document that this restarts the load, as if the src attribute had been > set. Done. https://codereview.chromium.org/1481913002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3026: setNetworkState(NETWORK_EMPTY); On 2015/12/02 14:02:14, philipj wrote: > Better also clear m_readyState and m_readyStateMaximum, so that we don't end up > in an inconsistent state. It is possible for scripts to hold references to > HTMLMediaElement instaces after stop() has been called, after all. For the same > reason also clear m_currentSourceNode, and call invalidateCachedTime(). > > Not so sure about cueTimeline().updateActiveCues(0), but since it was done > before it's safe to continue doing so. Done.
Description was changed from ========== Rename and refactor userCancelledLoad as per its usage The function userCancelledLoad isn't called when the user cancels load (there's no way to do that) and yet it implements that bit of the spec. So cleaning it up as per its actual usage. ========== to ========== Remove userCancelledLoad and refactor the code at the call sites The function userCancelledLoad isn't called when the user cancels the load (there's no way to do that) and yet it implements that bit of the spec. So removing this function and adding only the necessary bits at the call sites. ==========
lgtm, but the added test is of the variety that can tend to be flaky, so can you run it a few hundred times locally to make sure it's stable for you? https://codereview.chromium.org/1481913002/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/media/gc-pending-event-inactive-document.html (right): https://codereview.chromium.org/1481913002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/gc-pending-event-inactive-document.html:2: <title>GC with a pending event in an inactive document</title> Can you also rename the test to reflect that it now only testing that a pending event prevents GC, the "inactive document" is no more. https://codereview.chromium.org/1481913002/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/media/video-move-to-new-document.html (right): https://codereview.chromium.org/1481913002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/video-move-to-new-document.html:7: <iframe id="iframe1"></iframe> You can use document.querySelector('iframe') and drop the id.
Patchset #7 (id:200001) has been deleted
Patchset #7 (id:220001) has been deleted
Ran the test case 5k times and it is fine. Hopefully it will maintain the same thing on other platforms. https://codereview.chromium.org/1481913002/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/media/gc-pending-event-inactive-document.html (right): https://codereview.chromium.org/1481913002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/gc-pending-event-inactive-document.html:2: <title>GC with a pending event in an inactive document</title> On 2015/12/04 10:58:38, philipj wrote: > Can you also rename the test to reflect that it now only testing that a pending > event prevents GC, the "inactive document" is no more. Done. https://codereview.chromium.org/1481913002/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/media/video-move-to-new-document.html (right): https://codereview.chromium.org/1481913002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/video-move-to-new-document.html:7: <iframe id="iframe1"></iframe> On 2015/12/04 10:58:38, philipj wrote: > You can use document.querySelector('iframe') and drop the id. Done.
5k LGTM :)
The CQ bit was checked by philipj@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1481913002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1481913002/240001
Message was sent while issue was closed.
Description was changed from ========== Remove userCancelledLoad and refactor the code at the call sites The function userCancelledLoad isn't called when the user cancels the load (there's no way to do that) and yet it implements that bit of the spec. So removing this function and adding only the necessary bits at the call sites. ========== to ========== Remove userCancelledLoad and refactor the code at the call sites The function userCancelledLoad isn't called when the user cancels the load (there's no way to do that) and yet it implements that bit of the spec. So removing this function and adding only the necessary bits at the call sites. ==========
Message was sent while issue was closed.
Committed patchset #7 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Remove userCancelledLoad and refactor the code at the call sites The function userCancelledLoad isn't called when the user cancels the load (there's no way to do that) and yet it implements that bit of the spec. So removing this function and adding only the necessary bits at the call sites. ========== to ========== Remove userCancelledLoad and refactor the code at the call sites The function userCancelledLoad isn't called when the user cancels the load (there's no way to do that) and yet it implements that bit of the spec. So removing this function and adding only the necessary bits at the call sites. Committed: https://crrev.com/fa2a5db2eb8b92301b06f65aa644aef1a5bf709c Cr-Commit-Position: refs/heads/master@{#363193} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/fa2a5db2eb8b92301b06f65aa644aef1a5bf709c Cr-Commit-Position: refs/heads/master@{#363193} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
