Sets the media element network state to idle until playback is requested.

Sets the media element network state to idle until playback is requested.

This is the expected behavior if the user agent intends to not attempt to fetch
the resource until the user requests it explicitly.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226440

[qinmin, 2013-09-03 14:22:46]

https://codereview.chromium.org/23797004/diff/1/content/renderer/media/androi...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/23797004/diff/1/content/renderer/media/androi...
content/renderer/media/android/webmediaplayer_android.cc:299:
UpdateNetworkState(WebMediaPlayer::NetworkStateIdle);
this is an optional step in HTML5 spec, do we really need this? and this should
be done before CORS  fetch

[Jinsuk Kim, 2013-09-04 01:32:03]

https://codereview.chromium.org/23797004/diff/1/content/renderer/media/androi...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/23797004/diff/1/content/renderer/media/androi...
content/renderer/media/android/webmediaplayer_android.cc:299:
UpdateNetworkState(WebMediaPlayer::NetworkStateIdle);
On 2013/09/03 14:22:46, qinmin wrote:
> this is an optional step in HTML5 spec, do we really need this? and this
should
> be done before CORS  fetch

Though optional, I see benefit in having this - it makes Clank handle the media
loading state same as desktop Chrome does. When tested with preload attr -
default/none/metadata/auto, this CL ensures both get same sorts of events and
reach the same networkState.

What I understand from the code flow is that the state gets set to loading
initially, and then set to idle only when it reaches the step getting metadata.
In case it fails the state will remain loading and stalled event will be fired. 

Not sure where this should be moved to address your comment. It seems that CORS
info is fetched by MediaInfoLoader which is initialized in
WebMediaPlayerAndroid::load(), but it is also in this method that the initial
state is set to loading as well. Any more hint as to where it should go?

[qinmin, 2013-09-04 14:10:12]

https://codereview.chromium.org/23797004/diff/1/content/renderer/media/androi...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/23797004/diff/1/content/renderer/media/androi...
content/renderer/media/android/webmediaplayer_android.cc:299:
UpdateNetworkState(WebMediaPlayer::NetworkStateIdle);
can you run all the layout test with content shell and make sure there is no
regressions? and also add a layout test to test the code path if there is no
such one available
On 2013/09/04 01:32:03, jindor wrote:
> On 2013/09/03 14:22:46, qinmin wrote:
> > this is an optional step in HTML5 spec, do we really need this? and this
> should
> > be done before CORS  fetch
> 
> Though optional, I see benefit in having this - it makes Clank handle the
media
> loading state same as desktop Chrome does. When tested with preload attr -
> default/none/metadata/auto, this CL ensures both get same sorts of events and
> reach the same networkState.
> 
> What I understand from the code flow is that the state gets set to loading
> initially, and then set to idle only when it reaches the step getting
metadata.
> In case it fails the state will remain loading and stalled event will be
fired. 
> 
> Not sure where this should be moved to address your comment. It seems that
CORS
> info is fetched by MediaInfoLoader which is initialized in
> WebMediaPlayerAndroid::load(), but it is also in this method that the initial
> state is set to loading as well. Any more hint as to where it should go?
> 
>

[Jinsuk Kim, 2013-09-13 05:50:06]

https://codereview.chromium.org/23797004/diff/1/content/renderer/media/androi...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/23797004/diff/1/content/renderer/media/androi...
content/renderer/media/android/webmediaplayer_android.cc:299:
UpdateNetworkState(WebMediaPlayer::NetworkStateIdle);
On 2013/09/04 14:10:12, qinmin wrote:
> can you run all the layout test with content shell and make sure there is no
> regressions? and also add a layout test to test the code path if there is no
> such one available
> On 2013/09/04 01:32:03, jindor wrote:
> > On 2013/09/03 14:22:46, qinmin wrote:
> > > this is an optional step in HTML5 spec, do we really need this? and this
> > should
> > > be done before CORS  fetch
> > 
> > Though optional, I see benefit in having this - it makes Clank handle the
> media
> > loading state same as desktop Chrome does. When tested with preload attr -
> > default/none/metadata/auto, this CL ensures both get same sorts of events
and
> > reach the same networkState.
> > 
> > What I understand from the code flow is that the state gets set to loading
> > initially, and then set to idle only when it reaches the step getting
> metadata.
> > In case it fails the state will remain loading and stalled event will be
> fired. 
> > 
> > Not sure where this should be moved to address your comment. It seems that
> CORS
> > info is fetched by MediaInfoLoader which is initialized in
> > WebMediaPlayerAndroid::load(), but it is also in this method that the
initial
> > state is set to loading as well. Any more hint as to where it should go?
> > 
> > 
> 
Ran the tests and confirmed there's no regression. 

Let me add the test for the new codepath in a follow-up CL which will go under
WebKit/LayoutTest.

[scherkus (not reviewing), 2013-09-13 16:04:47]

https://codereview.chromium.org/23797004/diff/1/content/renderer/media/androi...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/23797004/diff/1/content/renderer/media/androi...
content/renderer/media/android/webmediaplayer_android.cc:282: void
WebMediaPlayerAndroid::DidLoadMediaInfo(
considering this is a callback, are we sure HTMLMediaElement won't call
WebMediaPlayerAndroid::play() until after this method runs and updates the
readystate?

otherwise we'll end up with a network state of idle while playing

[Jinsuk Kim, 2013-09-16 02:20:18]

https://codereview.chromium.org/23797004/diff/1/content/renderer/media/androi...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/23797004/diff/1/content/renderer/media/androi...
content/renderer/media/android/webmediaplayer_android.cc:282: void
WebMediaPlayerAndroid::DidLoadMediaInfo(
On 2013/09/13 16:04:47, scherkus wrote:
> considering this is a callback, are we sure HTMLMediaElement won't call
> WebMediaPlayerAndroid::play() until after this method runs and updates the
> readystate?
> 
> otherwise we'll end up with a network state of idle while playing

Thanks for catching it. Changed to make sure it happens only when play() is not
started yet.

[scherkus (not reviewing), 2013-09-19 22:24:45]

does this still need reviewing?

[Jinsuk Kim, 2013-09-24 23:36:19]

On 2013/09/19 22:24:45, scherkus wrote:
> does this still need reviewing?

I need owner's LGTM for approval.

[scherkus (not reviewing), 2013-09-25 00:23:54]

https://codereview.chromium.org/23797004/diff/6001/content/renderer/media/and...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/23797004/diff/6001/content/renderer/media/and...
content/renderer/media/android/webmediaplayer_android.cc:303:
UpdateNetworkState(WebMediaPlayer::NetworkStateIdle);
this still isn't technically correct

for example, the following order will incorrectly lower the state to idle:

load()
play()  // user taps play button -> network state is loading
pause()  // user taps pause button -> network state is still loading
DidLoadMediaInfo()  // network state now idle, despite play() causing media
player to load

[Jinsuk Kim, 2013-09-25 01:42:11]

https://codereview.chromium.org/23797004/diff/6001/content/renderer/media/and...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/23797004/diff/6001/content/renderer/media/and...
content/renderer/media/android/webmediaplayer_android.cc:303:
UpdateNetworkState(WebMediaPlayer::NetworkStateIdle);
On 2013/09/25 00:23:54, scherkus wrote:
> this still isn't technically correct
> 
> for example, the following order will incorrectly lower the state to idle:
> 
> load()
> play()  // user taps play button -> network state is loading
> pause()  // user taps pause button -> network state is still loading
> DidLoadMediaInfo()  // network state now idle, despite play() causing media
> player to load

That's true. I think I need a new flag rather than relying on is_playing_. See
if playing_started_ addresses the issue.

[Jinsuk Kim, 2013-09-30 01:36:31]

On 2013/09/25 01:42:11, jindor wrote:
>
https://codereview.chromium.org/23797004/diff/6001/content/renderer/media/and...
> File content/renderer/media/android/webmediaplayer_android.cc (right):
> 
>
https://codereview.chromium.org/23797004/diff/6001/content/renderer/media/and...
> content/renderer/media/android/webmediaplayer_android.cc:303:
> UpdateNetworkState(WebMediaPlayer::NetworkStateIdle);
> On 2013/09/25 00:23:54, scherkus wrote:
> > this still isn't technically correct
> > 
> > for example, the following order will incorrectly lower the state to idle:
> > 
> > load()
> > play()  // user taps play button -> network state is loading
> > pause()  // user taps pause button -> network state is still loading
> > DidLoadMediaInfo()  // network state now idle, despite play() causing media
> > player to load
> 
> That's true. I think I need a new flag rather than relying on is_playing_. See
> if playing_started_ addresses the issue.

Please take another look.

[scherkus (not reviewing), 2013-09-30 17:31:52]

lgtm

[Jinsuk Kim, 2013-10-01 01:01:21]

On 2013/09/30 17:31:52, scherkus wrote:
> lgtm

Hi Min, does this change look ok to you now too? Could I get your lgtm if it
does? I'll work on tests once this is done.

[qinmin, 2013-10-01 03:34:28]

lgtm % nit

https://codereview.chromium.org/23797004/diff/14001/content/renderer/media/an...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/23797004/diff/14001/content/renderer/media/an...
content/renderer/media/android/webmediaplayer_android.cc:240: // event (e.g.
playback request) occurs. Sets to the network state to IDLE
nit: s/Sets to/Sets/

[commit-bot: I haz the power, 2013-10-01 04:11:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jinsukkim@chromium.org/23797004/22001

[Jinsuk Kim, 2013-10-01 04:14:47]

Thanks!

https://codereview.chromium.org/23797004/diff/14001/content/renderer/media/an...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/23797004/diff/14001/content/renderer/media/an...
content/renderer/media/android/webmediaplayer_android.cc:240: // event (e.g.
playback request) occurs. Sets to the network state to IDLE
On 2013/10/01 03:34:28, qinmin wrote:
> nit: s/Sets to/Sets/

Done.

[commit-bot: I haz the power, 2013-10-01 11:13:33]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-10-02 00:50:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jinsukkim@chromium.org/23797004/22001

[commit-bot: I haz the power, 2013-10-02 10:09:46]

Change committed as 226440

[Jinsuk Kim, 2013-10-03 23:33:51]

On 2013/10/02 10:09:46, I haz the power (commit-bot) wrote:
> Change committed as 226440

I found that the relevant test case is already in place
(http/tests/media/video-load-suspend.html) and was failing. I verified that the
test passes after the CL is applied. Thanks.