| Summary: | Refactoring: Convert HTMLMediaElement::scheduleDelayedAction() to individually schedulable & cancelable tasks | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jer Noble <jer.noble> | ||||||||||||||
| Component: | New Bugs | Assignee: | Jer Noble <jer.noble> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | commit-queue, eric.carlson, ews-watchlist, rniwa, sam, webkit-bug-importer | ||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||
| OS: | Unspecified | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Jer Noble
2018-07-31 16:16:01 PDT
Created attachment 346216 [details]
Patch
Comment on attachment 346216 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=346216&action=review r=me once the bots are happy > Source/WebCore/html/HTMLMediaElement.cpp:1090 > + ALWAYS_LOG(LOGIDENTIFIER, "task fired"); I think it make more sense to log from the target method, the __func__ preprocessor macro isn't very helpful inside of a lambda. > Source/WebCore/html/HTMLMediaElement.cpp:4394 > + ALWAYS_LOG(LOGIDENTIFIER, "task fired"); Ditto > Source/WebCore/html/HTMLMediaElement.cpp:5248 > + ALWAYS_LOG(LOGIDENTIFIER, "task fired"); Ditto > Source/WebCore/html/HTMLMediaElement.cpp:5557 > + m_loadMediaResourceTask.close(); > + m_configureTextTracksTask.close(); > + m_checkPlaybackTargetCompatablityTask.close(); > + m_updateMediaStateTask.close(); > + m_mediaEngineUpdatedTask.close(); > + m_updatePlayStateTask.close(); > + m_resumeTaskQueue.close(); > + m_seekTaskQueue.close(); > + m_playbackControlsManagerBehaviorRestrictionsQueue.close(); > m_seekTaskQueue.close(); > m_resumeTaskQueue.close(); > - m_shadowDOMTaskQueue.close(); > m_promiseTaskQueue.close(); > m_pauseAfterDetachedTaskQueue.close(); Nit: it looks like this is the same as the code in HTMLMediaElement::stop, might as well put it in a method you can share. > Source/WebCore/html/HTMLMediaElement.cpp:7586 > + ALWAYS_LOG(LOGIDENTIFIER, "task fired"); Ditto the comment about logging in the target method. (In reply to Eric Carlson from comment #2) > Comment on attachment 346216 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=346216&action=review > > r=me once the bots are happy > > > Source/WebCore/html/HTMLMediaElement.cpp:1090 > > + ALWAYS_LOG(LOGIDENTIFIER, "task fired"); > > I think it make more sense to log from the target method, the __func__ > preprocessor macro isn't very helpful inside of a lambda. While that's true, doing it this way means it's possible to match up the task being scheduled to the task being fired; if we move this inside the called method, we will log whenever that method is called, regardless of whether it was scheduled or not. > > > Source/WebCore/html/HTMLMediaElement.cpp:5557 > > + m_loadMediaResourceTask.close(); > > + m_configureTextTracksTask.close(); > > + m_checkPlaybackTargetCompatablityTask.close(); > > + m_updateMediaStateTask.close(); > > + m_mediaEngineUpdatedTask.close(); > > + m_updatePlayStateTask.close(); > > + m_resumeTaskQueue.close(); > > + m_seekTaskQueue.close(); > > + m_playbackControlsManagerBehaviorRestrictionsQueue.close(); > > m_seekTaskQueue.close(); > > m_resumeTaskQueue.close(); > > - m_shadowDOMTaskQueue.close(); > > m_promiseTaskQueue.close(); > > m_pauseAfterDetachedTaskQueue.close(); > > Nit: it looks like this is the same as the code in HTMLMediaElement::stop, > might as well put it in a method you can share. Will do. Created attachment 346634 [details]
Patch
Comment on attachment 346634 [details] Patch Attachment 346634 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/8778145 New failing tests: media/video-controls-no-display-with-text-track.html Created attachment 346639 [details]
Archive of layout-test-results from ews103 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 346634 [details] Patch Attachment 346634 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/8778197 New failing tests: media/track/track-cue-rendering-horizontal.html Created attachment 346640 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 346634 [details] Patch Attachment 346634 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/8778279 New failing tests: media/video-controls-no-display-with-text-track.html Created attachment 346643 [details]
Archive of layout-test-results from ews117 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 346634 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=346634&action=review > Source/WebCore/html/HTMLMediaElement.h:943 > + DeferrableTask<Timer> m_loadMediaResourceTask; It doesn't seem like this one is ever scheduled. Created attachment 352470 [details]
Patch
Comment on attachment 352470 [details] Patch Clearing flags on attachment: 352470 Committed r237207: <https://trac.webkit.org/changeset/237207> All reviewed patches have been landed. Closing bug. |