Bug 145523 – Media Session API: Implement methods required by the EventTarget interface in MediaRemoteControls

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

[Matt Rajca]

Implement the `eventTargetInterface` and `scriptExecutionContext` methods required by EventTarget, as well as some required infrastructure.

[Radar WebKit Bug Importer]

<rdar://problem/21188725>

[Matt Rajca]

Created attachment 254012 [details]
Patch

[Conrad Shultz]

Comment on attachment 254012 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        * Modules/mediasession/MediaRemoteControls.idl: Indicate MediaRemoteControls now takes a constructor that is passed in a script execution context. Event handlers have been uncommented until they are implemented to prevent build errors.

I think you meant commented?

> Source/WebCore/Modules/mediasession/MediaRemoteControls.idl:40
> +    //attribute EventHandler onseekbackward;

I think we usually avoid checking in commented-out code. Could this just temporarily be deleted? (If we do commit commented-out code, a comment to make it clear this wasn't an accidental check-in might be warranted.)

[Eric Carlson]

Comment on attachment 254012 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        Implemented the `eventTargetInterface` and `scriptExecutionContext` methods required by EventTarget, as well as some required infrastructure.
> +        https://bugs.webkit.org/show_bug.cgi?id=145523
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * Modules/mediasession/MediaRemoteControls.cpp:
> +        (WebCore::MediaRemoteControls::MediaRemoteControls): Initialize all instance variables.
> +        * Modules/mediasession/MediaRemoteControls.h: MediaRemoteControl's constructor now takes a script execution context, which we provide to EventTarget. The required eventTargetInterface method has also been implemented.

Please wrap these very long lines to a reasonable window width

> Source/WebCore/Modules/mediasession/MediaRemoteControls.cpp:38
> +    , m_previousTrackEnabled(false)
> +    , m_nextTrackEnabled(false)
> +    , m_seekForwardEnabled(false)
> +    , m_seekBackwardEnabled(false)

These should be initialized in the header with class member initializers, eg:

bool m_previousTrackEnabled { false };

etc

>> Source/WebCore/Modules/mediasession/MediaRemoteControls.idl:40
>> +    //attribute EventHandler onseekbackward;
> 
> I think we usually avoid checking in commented-out code. Could this just temporarily be deleted? (If we do commit commented-out code, a comment to make it clear this wasn't an accidental check-in might be warranted.)

Agreed, we generally don't check in commented-out code.

[Matt Rajca]

Created attachment 254014 [details]
Patch (revised)

[Matt Rajca]

Comment on attachment 254012 [details]
Patch

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

>> Source/WebCore/ChangeLog:10
>> +        * Modules/mediasession/MediaRemoteControls.h: MediaRemoteControl's constructor now takes a script execution context, which we provide to EventTarget. The required eventTargetInterface method has also been implemented.
> 
> Please wrap these very long lines to a reasonable window width

Done.

>> Source/WebCore/Modules/mediasession/MediaRemoteControls.cpp:38
>> +    , m_seekBackwardEnabled(false)
> 
> These should be initialized in the header with class member initializers, eg:
> 
> bool m_previousTrackEnabled { false };
> 
> etc

Done.

>>> Source/WebCore/Modules/mediasession/MediaRemoteControls.idl:40
>>> +    //attribute EventHandler onseekbackward;
>> 
>> I think we usually avoid checking in commented-out code. Could this just temporarily be deleted? (If we do commit commented-out code, a comment to make it clear this wasn't an accidental check-in might be warranted.)
> 
> Agreed, we generally don't check in commented-out code.

Removed.

[WebKit Commit Bot]

Comment on attachment 254014 [details]
Patch (revised)

Clearing flags on attachment: 254014

Committed r185077: <http://trac.webkit.org/changeset/185077>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Comment on attachment 254012 [details]
Patch

Cleared review? from obsolete attachment 254012 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).