Bug 145523

<rdar://problem/21188725>

Created attachment 254012 [details] Patch

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

Created attachment 254014 [details] Patch (revised)

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 on attachment 254014 [details] Patch (revised) Clearing flags on attachment: 254014 Committed r185077: <http://trac.webkit.org/changeset/185077>

All reviewed patches have been landed. Closing bug.

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