| Summary: | CurrentTime on mediaController is set as 0 when playback is completed. | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | sangdeug <sangdeug.kim> | ||||||||||
| Component: | Media | Assignee: | Nobody <webkit-unassigned> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | commit-queue, eric.carlson, jer.noble, sangdeug.kim | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||
| Hardware: | All | ||||||||||||
| OS: | All | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
sangdeug
2015-09-15 01:20:37 PDT
Created attachment 261182 [details]
Patch
Can a regression test be added for this fix? Created attachment 261294 [details]
Patch
Add test case for this fix. Check the currentTime of mediacontroller is greater than 0 on ended event. 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 }; 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.
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; 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? 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. Created attachment 261465 [details]
Patch
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. 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. Created attachment 261479 [details]
Patch
Comment on attachment 261479 [details]
Patch
Thank you!
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. Comment on attachment 261479 [details] Patch Rejecting attachment 261479 [details] from commit-queue. sangdeug.kim@samsung.com 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. Comment on attachment 261479 [details] Patch Rejecting attachment 261479 [details] from commit-queue. sangdeug.kim@samsung.com 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. Comment on attachment 261479 [details] Patch Clearing flags on attachment: 261479 Committed r190114: <http://trac.webkit.org/changeset/190114> All reviewed patches have been landed. Closing bug. |