Transition networkState back to NETWORK_IDLE on fetch if preload=none

Transition networkState back to NETWORK_IDLE on fetch if preload=none

When a media element is marked with the preload hint 'none', a load is not
started until data is explicitly requested. The progress timer is however
still started, which means that a 'stalled' event will be fired after ~3s.
This is confusing, since loading never actually started in the first place.
Fix this by implementing (the "optional") step 3 from the 'resource fetch
algorithm'.

BUG=382505
BUG=284413
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176199

[fs, 2014-06-09 16:12:23]

https://codereview.chromium.org/327553002/diff/20001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/327553002/diff/20001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:898:
changeNetworkStateFromLoadingToIdle();
This means that we'll also fire a progress event - which could be a bad thing
(since no 'progress' will have actually taken place yet.)

https://codereview.chromium.org/327553002/diff/20001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:901: // 4. Wait for the task to be run.
Neither queuing a nor waiting for a task here, which I presume means the load
event could end up being delivered within the current task. Don't know how much
to worry about that one. Would be nice to fix of course.

[acolwell GONE FROM CHROMIUM, 2014-06-09 17:54:17]

looks pretty good. Just a few comments.

https://codereview.chromium.org/327553002/diff/20001/LayoutTests/media/video-...
File LayoutTests/media/video-preload-none-no-stalled-event.html (right):

https://codereview.chromium.org/327553002/diff/20001/LayoutTests/media/video-...
LayoutTests/media/video-preload-none-no-stalled-event.html:15: // Give the
progress timer enoug time to fire a 'stalled' event.
nit: s/enoug/enough/

https://codereview.chromium.org/327553002/diff/20001/LayoutTests/media/video-...
LayoutTests/media/video-preload-none-no-stalled-event.html:17:
setTimeout(endTest, 3000 + 500);
This seems brittle. How about using waitForEventAndEnd("suspend") instead and
let the normal timeout cause the test to fail?

https://codereview.chromium.org/327553002/diff/20001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/327553002/diff/20001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:898:
changeNetworkStateFromLoadingToIdle();
On 2014/06/09 16:12:23, fs wrote:
> This means that we'll also fire a progress event - which could be a bad thing
> (since no 'progress' will have actually taken place yet.)

I think we should avoid firing the progress event in this case. ISTM that the
progress event in this method should be conditional on
m_player->didLoadingProgress(). Otherwise just add an enum param that lets you
control whether the event is fired or not and update the various call sites.

https://codereview.chromium.org/327553002/diff/20001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:901: // 4. Wait for the task to be run.
On 2014/06/09 16:12:23, fs wrote:
> Neither queuing a nor waiting for a task here, which I presume means the load
> event could end up being delivered within the current task. Don't know how
much
> to worry about that one. Would be nice to fix of course.

Why don't you just add a timer here to actually implement the queue a task
functionality?

[fs, 2014-06-10 12:36:25]

https://codereview.chromium.org/327553002/diff/20001/LayoutTests/media/video-...
File LayoutTests/media/video-preload-none-no-stalled-event.html (right):

https://codereview.chromium.org/327553002/diff/20001/LayoutTests/media/video-...
LayoutTests/media/video-preload-none-no-stalled-event.html:17:
setTimeout(endTest, 3000 + 500);
On 2014/06/09 17:54:17, acolwell wrote:
> This seems brittle. How about using waitForEventAndEnd("suspend") instead and
> let the normal timeout cause the test to fail?

SGTM. Done.

https://codereview.chromium.org/327553002/diff/20001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/327553002/diff/20001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:898:
changeNetworkStateFromLoadingToIdle();
On 2014/06/09 17:54:17, acolwell wrote:
> On 2014/06/09 16:12:23, fs wrote:
> > This means that we'll also fire a progress event - which could be a bad
thing
> > (since no 'progress' will have actually taken place yet.)
> 
> I think we should avoid firing the progress event in this case. ISTM that the
> progress event in this method should be conditional on
> m_player->didLoadingProgress(). Otherwise just add an enum param that lets you
> control whether the event is fired or not and update the various call sites.

Added gating of 'progress' dispatch on m_player->didLoadingProgress.

https://codereview.chromium.org/327553002/diff/20001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:901: // 4. Wait for the task to be run.
On 2014/06/09 17:54:17, acolwell wrote:
> On 2014/06/09 16:12:23, fs wrote:
> > Neither queuing a nor waiting for a task here, which I presume means the
load
> > event could end up being delivered within the current task. Don't know how
> much
> > to worry about that one. Would be nice to fix of course.
> 
> Why don't you just add a timer here to actually implement the queue a task
> functionality?

Done (PS3). But to facilitate the wait in step 4 some additional state was
needed, so I refactored the code a bit - hopefully it's not too big a change
(not a lot of "collateral damage").
Also ended up renaming from "delayed load" to "deferred load" (in part because
it easier to make the required changes) - will happily change that back to
'delayed' (or anything else, really) though.

[acolwell GONE FROM CHROMIUM, 2014-06-13 18:54:51]

lgtm. Thanks for fixing this. :)

https://codereview.chromium.org/327553002/diff/80001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/327553002/diff/80001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:896: deferredLoad();
nit: s/deferredLoad/deferLoad/ ? That implies that you are initiating the
deferral more I think.

[fs, 2014-06-16 10:39:53]

https://codereview.chromium.org/327553002/diff/80001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/327553002/diff/80001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:896: deferredLoad();
On 2014/06/13 18:54:51, acolwell wrote:
> nit: s/deferredLoad/deferLoad/ ? That implies that you are initiating the
> deferral more I think.

Renamed as suggested.

[fs, 2014-06-16 10:40:00]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2014-06-16 10:40:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/fs@opera.com/327553002/100001

[commit-bot: I haz the power, 2014-06-16 10:44:44]

Change committed as 176199