Fix progress event not firing when load has completed.

Fix progress event not firing when load has completed.

When media has fully loaded, make sure we transition through
the 'loaded' network state, otherwise the progress event on
final load won't be fired.

There probably should be a layout test for this. Will raise an issue.

BUG=59372
TEST=local handcrafted repro, media layout tests.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=75985

[acolwell GONE FROM CHROMIUM, 2011-02-24 00:43:32]

LGTM

[sjl, 2011-02-24 00:51:03]

On 2011/02/24 00:43:32, acolwell wrote:
> LGTM

Thanks - pls take a look at the second patch ;-)

[acolwell GONE FROM CHROMIUM, 2011-02-24 19:19:08]

http://codereview.chromium.org/6581012/diff/2001/webkit/glue/webmediaplayer_i...
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/6581012/diff/2001/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.cc:832:
SetNetworkState(WebKit::WebMediaPlayer::Loaded);
Shouldn't this be inside the condition with the Repaint()? Then you at least
know that you are transitioning when you have downloaded all the bytes.

http://codereview.chromium.org/6581012/diff/2001/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.cc:834:
SetNetworkState(WebKit::WebMediaPlayer::Idle);
Why is there an immediate transition from Loaded to Idle?

[sjl, 2011-02-24 21:24:41]

Thx for the review. Comments addressed...

http://codereview.chromium.org/6581012/diff/2001/webkit/glue/webmediaplayer_i...
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/6581012/diff/2001/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.cc:832:
SetNetworkState(WebKit::WebMediaPlayer::Loaded);
On 2011/02/24 19:19:08, acolwell wrote:
> Shouldn't this be inside the condition with the Repaint()? Then you at least
> know that you are transitioning when you have downloaded all the bytes.

Good point. Fixed.

http://codereview.chromium.org/6581012/diff/2001/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.cc:834:
SetNetworkState(WebKit::WebMediaPlayer::Idle);
On 2011/02/24 19:19:08, acolwell wrote:
> Why is there an immediate transition from Loaded to Idle?

We need to switch to idle when we've finished downloading and we're done with
network activity, but before that we need to transition through the loaded state
(which was being skipped previously). It's in this transition that webkit sends
out the final progress event (as per spec).

[acolwell GONE FROM CHROMIUM, 2011-02-24 21:38:40]

got it. LGTM.

On 2011/02/24 21:24:41, sjl wrote:
> Thx for the review. Comments addressed...
> 
>
http://codereview.chromium.org/6581012/diff/2001/webkit/glue/webmediaplayer_i...
> File webkit/glue/webmediaplayer_impl.cc (right):
> 
>
http://codereview.chromium.org/6581012/diff/2001/webkit/glue/webmediaplayer_i...
> webkit/glue/webmediaplayer_impl.cc:832:
> SetNetworkState(WebKit::WebMediaPlayer::Loaded);
> On 2011/02/24 19:19:08, acolwell wrote:
> > Shouldn't this be inside the condition with the Repaint()? Then you at least
> > know that you are transitioning when you have downloaded all the bytes.
> 
> Good point. Fixed.
> 
>
http://codereview.chromium.org/6581012/diff/2001/webkit/glue/webmediaplayer_i...
> webkit/glue/webmediaplayer_impl.cc:834:
> SetNetworkState(WebKit::WebMediaPlayer::Idle);
> On 2011/02/24 19:19:08, acolwell wrote:
> > Why is there an immediate transition from Loaded to Idle?
> 
> We need to switch to idle when we've finished downloading and we're done with
> network activity, but before that we need to transition through the loaded
state
> (which was being skipped previously). It's in this transition that webkit
sends
> out the final progress event (as per spec).

[jamesr, 2011-02-25 00:08:23]

I'm pretty sure this broke http/tests/media/video-play-suspend.html:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=htt...

[sjl, 2011-02-25 00:44:13]

On 2011/02/25 00:08:23, jamesr wrote:
> I'm pretty sure this broke http/tests/media/video-play-suspend.html:
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=htt...

As per my IM, the layout test seems bust.

It is checking for a suspend event, which is as per spec gets fired when the the
network stops loading, but hasn't completed loading all the data.

This event is fired by webkit when the mediaplayer transitions from loading ->
idle, which we would previously do (incorrectly) when we've finished loading all
data.

The correct transition is loading -> loaded -> idle (which this cl does), which
causes a progress event to be fired on the loaded transistion (as per spec), but
a 'suspend' event is not fired as the we haven't suspended loading (again, as
per spec).

Running the same test under safari generates the same (new) behaviour as chrome
- the test hangs...

Therefore, the test looks busted...