Bug 188208 – Refactoring: Convert HTMLMediaElement::scheduleDelayedAction() to individually schedulable & cancelable tasks

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

Refactoring: Convert HTMLMediaElement::scheduleDelayedAction() to individually schedulable & cancelable tasks

[Jer Noble]

Created attachment 346216 [details]
Patch

[Eric Carlson]

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.

[Jer Noble]

(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.

[Jer Noble]

Created attachment 346634 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Sam Weinig]

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.

[Jer Noble]

Created attachment 352470 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 352470 [details]
Patch

Clearing flags on attachment: 352470

Committed r237207: <https://trac.webkit.org/changeset/237207>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45322136>