Bug 145523 - Media Session API: Implement methods required by the EventTarget interface in MediaRemoteControls
Summary: Media Session API: Implement methods required by the EventTarget interface in...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 145411
  Show dependency treegraph
 
Reported: 2015-06-01 13:47 PDT by Matt Rajca
Modified: 2015-09-14 10:55 PDT (History)
6 users (show)

See Also:


Attachments
Patch (9.37 KB, patch)
2015-06-01 14:01 PDT, Matt Rajca
no flags Details | Formatted Diff | Diff
Patch (revised) (9.44 KB, patch)
2015-06-01 14:35 PDT, Matt Rajca
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Rajca 2015-06-01 13:47:36 PDT
Implement the `eventTargetInterface` and `scriptExecutionContext` methods required by EventTarget, as well as some required infrastructure.
Comment 1 Radar WebKit Bug Importer 2015-06-01 13:48:44 PDT
<rdar://problem/21188725>
Comment 2 Matt Rajca 2015-06-01 14:01:51 PDT
Created attachment 254012 [details]
Patch
Comment 3 Conrad Shultz 2015-06-01 14:07:04 PDT
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.)
Comment 4 Eric Carlson 2015-06-01 14:25:14 PDT
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.
Comment 5 Matt Rajca 2015-06-01 14:35:43 PDT
Created attachment 254014 [details]
Patch (revised)
Comment 6 Matt Rajca 2015-06-01 14:36:58 PDT
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.
Comment 7 WebKit Commit Bot 2015-06-01 15:26:43 PDT
Comment on attachment 254014 [details]
Patch (revised)

Clearing flags on attachment: 254014

Committed r185077: <http://trac.webkit.org/changeset/185077>
Comment 8 WebKit Commit Bot 2015-06-01 15:26:46 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Csaba Osztrogonác 2015-09-14 10:55:13 PDT
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).