Fix incorrect set of playing state in WebMediaPlayerAndroid.

Fix incorrect set of playing state in WebMediaPlayerAndroid.

BUG=414066
TEST=none

Committed: https://crrev.com/348153027e63683d4e943f249a384ca8bfc113e4
Cr-Commit-Position: refs/heads/master@{#295115}

[DaleCurtis, 2014-09-16 17:10:12]

dalecurtis@chromium.org changed reviewers:
+ qinmin@chromium.org

[qinmin, 2014-09-16 17:28:24]

lgtm % comments

https://codereview.chromium.org/557363004/diff/20001/content/renderer/media/a...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/557363004/diff/20001/content/renderer/media/a...
content/renderer/media/android/webmediaplayer_android.cc:769:
UpdatePlayingState(false);
you can just remove this statement, it does nothing here.
client_->timeChanged() will call pause() if time_==duration(), so when this line
is reached, player should already in a paused state.

https://codereview.chromium.org/557363004/diff/20001/content/renderer/media/a...
content/renderer/media/android/webmediaplayer_android.cc:1321: if (delegate_) {
Thanks for fixing my mistake here.
I remember i put the interpolator code in front of the if(delegate_) statement
below in https://codereview.chromium.org/545993002/, but while resolving all the
merge conflicts I mistakenly reversed the order

[DaleCurtis, 2014-09-16 17:30:28]

https://codereview.chromium.org/557363004/diff/20001/content/renderer/media/a...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/557363004/diff/20001/content/renderer/media/a...
content/renderer/media/android/webmediaplayer_android.cc:769:
UpdatePlayingState(false);
On 2014/09/16 17:28:24, qinmin wrote:
> you can just remove this statement, it does nothing here.
> client_->timeChanged() will call pause() if time_==duration(), so when this
line
> is reached, player should already in a paused state.

I'm not sure about that.  If the element is set to loop, pause() will not be
called here.

[qinmin, 2014-09-16 17:35:17]

On 2014/09/16 17:30:28, DaleCurtis wrote:
>
https://codereview.chromium.org/557363004/diff/20001/content/renderer/media/a...
> File content/renderer/media/android/webmediaplayer_android.cc (right):
> 
>
https://codereview.chromium.org/557363004/diff/20001/content/renderer/media/a...
> content/renderer/media/android/webmediaplayer_android.cc:769:
> UpdatePlayingState(false);
> On 2014/09/16 17:28:24, qinmin wrote:
> > you can just remove this statement, it does nothing here.
> > client_->timeChanged() will call pause() if time_==duration(), so when this
> line
> > is reached, player should already in a paused state.
> 
> I'm not sure about that.  If the element is set to loop, pause() will not be
> called here.

if it is set to loop, then we don't need to set is_playing_ to false either

[DaleCurtis, 2014-09-16 17:37:19]

The code seems to be using pending_playback_ to special case the seek back to
zero in the loop case. Do you know what that's about?

[qinmin, 2014-09-16 17:51:04]

On 2014/09/16 17:37:19, DaleCurtis wrote:
> The code seems to be using pending_playback_ to special case the seek back to
> zero in the loop case. Do you know what that's about?


This change adds the pending_playback_ and "is_playing_ = false":
https://chromiumcodereview.appspot.com/23442021

I am wondering whether is_playing_=false is correct for loop. From the spec, i
don't see that MediaElement should be in a paused state when loop is set.

"When the current playback position reaches the end of the media resource when
the direction of playback is forwards, then the user agent must follow these
steps:
1.If the media element has a loop attribute specified, then seek to the earliest
possible position of the media resource and abort these steps.
2.Stop playback.
The ended attribute becomes true.
3.The user agent must queue a task to fire a simple event named timeupdate at
the element.
4.The user agent must queue a task to fire a simple event named ended at the
element."

[DaleCurtis, 2014-09-16 17:51:20]

https://chromiumcodereview.appspot.com/23442021 gives no hints about why this
was implemented this way.  If we removed the is_playing_ is false, we can
probably remove the pending_playback_ business I think.

[DaleCurtis, 2014-09-16 17:54:17]

Whoops looks like we tracked down the original change at the same time.  I think
your interpretation is correct, but I don't know this code well or have a test
setup.

How about I land this and you can cleanup the implementation in a followup patch
set after doing some testing with looping media?

[qinmin, 2014-09-16 17:58:02]

On 2014/09/16 17:54:17, DaleCurtis wrote:
> Whoops looks like we tracked down the original change at the same time.  I
think
> your interpretation is correct, but I don't know this code well or have a test
> setup.
> 
> How about I land this and you can cleanup the implementation in a followup
patch
> set after doing some testing with looping media?

Sure, thanks for looking into this

[DaleCurtis, 2014-09-16 17:58:45]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2014-09-16 18:00:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/557363004/20001

[commit-bot: I haz the power, 2014-09-16 19:04:17]

Committed patchset #2 (id:20001) as 4d2b4ff2c993c63493979c9c6e84300559e5511d

[commit-bot: I haz the power, 2014-09-16 19:05:38]

Patchset 2 (id:??) landed as
https://crrev.com/348153027e63683d4e943f249a384ca8bfc113e4
Cr-Commit-Position: refs/heads/master@{#295115}