Bug 149154 – CurrentTime on mediaController is set as 0 when playback is completed.

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[sangdeug]

Clear timing for current time need to be changed to next playback, not playback complete timing.

Test site :
http://www.w3.org/2010/05/video/mediaevents.html

[sangdeug]

Created attachment 261182 [details]
Patch

[Alexey Proskuryakov]

Can a regression test be added for this fix?

[sangdeug]

Created attachment 261294 [details]
Patch

[sangdeug]

Add test case for this fix. 
Check the currentTime of mediacontroller is greater than 0 on ended event.

[Darin Adler]

Comment on attachment 261294 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=261294&action=review

> Source/WebCore/html/MediaController.h:157
> +    bool m_resetCurrentTimeInNextPlay;

For new code, we should initialize in the header:

    bool m_resetCurrentTimeInNextPlay { false };

[Darin Adler]

Comment on attachment 261294 [details]
Patch

I’d like to review, but I think a media expert should review this to see if it’s done correctly.

[Eric Carlson]

Comment on attachment 261294 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=261294&action=review

> Source/WebCore/html/MediaController.cpp:458
>      case ENDED:
>          eventName = eventNames().endedEvent;
> +        m_resetCurrentTimeInNextPlay = true;
>          m_clock->stop();
> -        m_clock->setCurrentTime(0);
>          m_timeupdateTimer.stop();
>          break;

Instead of adding a new instance variable to track state, can you just set current time to 0 when playback state changes from ENDED to PLAYING?

    case PLAYING:
        if (oldReadyState == ENDED)
            m_clock->setCurrentTime(0);
        eventName = eventNames().playingEvent;
        m_clock->start();
        startTimeupdateTimer();
        break;

[sangdeug]

Comment on attachment 261294 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=261294&action=review

>> Source/WebCore/html/MediaController.cpp:458
>>          break;
> 
> Instead of adding a new instance variable to track state, can you just set current time to 0 when playback state changes from ENDED to PLAYING?
> 
>     case PLAYING:
>         if (oldReadyState == ENDED)
>             m_clock->setCurrentTime(0);
>         eventName = eventNames().playingEvent;
>         m_clock->start();
>         startTimeupdateTimer();
>         break;

I tried to using oldPlaybackState but during my test, sometimes I observed that the WAITING event is comming between ENDED and PLAYING. 

e.g) 
ENDED -> PLAYING
ENDED -> WAITING -> PLAYING

To cover both case, I added a new variable to track state.

>> Source/WebCore/html/MediaController.h:157
>> +    bool m_resetCurrentTimeInNextPlay;
> 
> For new code, we should initialize in the header:
> 
>     bool m_resetCurrentTimeInNextPlay { false };

It's initialized in constructor, do I need initialize in head also?

[Eric Carlson]

Comment on attachment 261294 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=261294&action=review

>>> Source/WebCore/html/MediaController.cpp:458
>>>          break;
>> 
>> Instead of adding a new instance variable to track state, can you just set current time to 0 when playback state changes from ENDED to PLAYING?
>> 
>>     case PLAYING:
>>         if (oldReadyState == ENDED)
>>             m_clock->setCurrentTime(0);
>>         eventName = eventNames().playingEvent;
>>         m_clock->start();
>>         startTimeupdateTimer();
>>         break;
> 
> I tried to using oldPlaybackState but during my test, sometimes I observed that the WAITING event is comming between ENDED and PLAYING. 
> 
> e.g) 
> ENDED -> PLAYING
> ENDED -> WAITING -> PLAYING
> 
> To cover both case, I added a new variable to track state.

OK. In any case, you should handle this in the switch statement:

    case PLAYING:
        if (m_resetCurrentTimeInNextPlay)
            m_clock->setCurrentTime(0);
        m_resetCurrentTimeInNextPlay = false;
        eventName = eventNames().playingEvent;
        m_clock->start();
        startTimeupdateTimer();
        break;

>>> Source/WebCore/html/MediaController.h:157
>>> +    bool m_resetCurrentTimeInNextPlay;
>> 
>> For new code, we should initialize in the header:
>> 
>>     bool m_resetCurrentTimeInNextPlay { false };
> 
> It's initialized in constructor, do I need initialize in head also?

For new code, the WebKit style is initialize in the header instead of in the constructor.

[sangdeug]

Created attachment 261465 [details]
Patch

[sangdeug]

Comment on attachment 261294 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=261294&action=review

>>>> Source/WebCore/html/MediaController.cpp:458
>>>>          break;
>>> 
>>> Instead of adding a new instance variable to track state, can you just set current time to 0 when playback state changes from ENDED to PLAYING?
>>> 
>>>     case PLAYING:
>>>         if (oldReadyState == ENDED)
>>>             m_clock->setCurrentTime(0);
>>>         eventName = eventNames().playingEvent;
>>>         m_clock->start();
>>>         startTimeupdateTimer();
>>>         break;
>> 
>> I tried to using oldPlaybackState but during my test, sometimes I observed that the WAITING event is comming between ENDED and PLAYING. 
>> 
>> e.g) 
>> ENDED -> PLAYING
>> ENDED -> WAITING -> PLAYING
>> 
>> To cover both case, I added a new variable to track state.
> 
> OK. In any case, you should handle this in the switch statement:
> 
>     case PLAYING:
>         if (m_resetCurrentTimeInNextPlay)
>             m_clock->setCurrentTime(0);
>         m_resetCurrentTimeInNextPlay = false;
>         eventName = eventNames().playingEvent;
>         m_clock->start();
>         startTimeupdateTimer();
>         break;

Done.

>>>> Source/WebCore/html/MediaController.h:157
>>>> +    bool m_resetCurrentTimeInNextPlay;
>>> 
>>> For new code, we should initialize in the header:
>>> 
>>>     bool m_resetCurrentTimeInNextPlay { false };
>> 
>> It's initialized in constructor, do I need initialize in head also?
> 
> For new code, the WebKit style is initialize in the header instead of in the constructor.

Done.

[Eric Carlson]

Comment on attachment 261465 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=261465&action=review

r=me with one minor nit.

Thanks!

> Source/WebCore/html/MediaController.cpp:60
> +    , m_resetCurrentTimeInNextPlay(false)

Nit: this is unnecessary because you have already initialized it in the header.

[sangdeug]

Created attachment 261479 [details]
Patch

[Eric Carlson]

Comment on attachment 261479 [details]
Patch

Thank you!

[Eric Carlson]

sangdeug - if you want someone to land your patch, mark it "cq?" and any committer can land it manually or mark it "cq+" and the commit bot will land it automatically.

[WebKit Commit Bot]

Comment on attachment 261479 [details]
Patch

Rejecting attachment 261479 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Commit Bot]

Comment on attachment 261479 [details]
Patch

Rejecting attachment 261479 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Commit Bot]

Comment on attachment 261479 [details]
Patch

Clearing flags on attachment: 261479

Committed r190114: <http://trac.webkit.org/changeset/190114>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.