Summary: | Add stub implementation of MediaSession, part of Media Session spec | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Matt Rajca <mrajca> | ||||||||||
Component: | Media | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | commit-queue, conrad_shultz, eric.carlson, jer.noble, mrajca, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 145411 | ||||||||||||
Attachments: |
|
Description
Matt Rajca
2015-06-01 15:43:53 PDT
Created attachment 254028 [details]
Patch
Comment on attachment 254028 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254028&action=review If we have MediaSession.idl we need to have JSMediaSession.cpp and JSMediaSession.h. > Source/WebCore/Modules/mediasession/MediaSession.h:32 > +#include <wtf/RefCounted.h> If we include "MediaRemoteControls.h" it seems unlikely we need to include this also. > Source/WebCore/Modules/mediasession/MediaSession.h:33 > +#include <wtf/RefPtr.h> Ditto. > Source/WebCore/Modules/mediasession/MediaSession.h:37 > +class MediaSession : public RefCounted<MediaSession> { I suggest marking this class final. > Source/WebCore/Modules/mediasession/MediaSession.h:46 > + MediaSession(ScriptExecutionContext&, Kind); Does this compile? Does the bindings generator really turn those strings into MediaSession::Kind values? > Source/WebCore/Modules/mediasession/MediaSession.h:47 > + virtual ~MediaSession(); Why virtual? There doesn’t seem to be any polymorphism in this class, so it seems we should avoid marking it virtual. > Source/WebCore/Modules/mediasession/MediaSession.h:50 > + MediaRemoteControls *controls() const { return m_controls.get(); } Wrong position of the * character here. Comment on attachment 254028 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254028&action=review >> Source/WebCore/Modules/mediasession/MediaSession.h:32 >> +#include <wtf/RefCounted.h> > > If we include "MediaRemoteControls.h" it seems unlikely we need to include this also. Removed. >> Source/WebCore/Modules/mediasession/MediaSession.h:37 >> +class MediaSession : public RefCounted<MediaSession> { > > I suggest marking this class final. I agree. >> Source/WebCore/Modules/mediasession/MediaSession.h:46 >> + MediaSession(ScriptExecutionContext&, Kind); > > Does this compile? Does the bindings generator really turn those strings into MediaSession::Kind values? Not if we compile JSMediaSession with the project. Fixed. >> Source/WebCore/Modules/mediasession/MediaSession.h:47 >> + virtual ~MediaSession(); > > Why virtual? There doesn’t seem to be any polymorphism in this class, so it seems we should avoid marking it virtual. Done, and annotated the interface in the IDL file with ImplementationLacksVTable. >> Source/WebCore/Modules/mediasession/MediaSession.h:50 >> + MediaRemoteControls *controls() const { return m_controls.get(); } > > Wrong position of the * character here. Fixed. Created attachment 254071 [details]
Patch (revised)
Comment on attachment 254071 [details] Patch (revised) View in context: https://bugs.webkit.org/attachment.cgi?id=254071&action=review > Source/WebCore/Modules/mediasession/MediaSession.h:46 > + MediaRemoteControls* controls(bool& isNull); Where does this signature come from? We haven’t ever used this in the past for bindings. Is this some new approach? Comment on attachment 254071 [details] Patch (revised) View in context: https://bugs.webkit.org/attachment.cgi?id=254071&action=review >> Source/WebCore/Modules/mediasession/MediaSession.h:46 >> + MediaRemoteControls* controls(bool& isNull); > > Where does this signature come from? We haven’t ever used this in the past for bindings. Is this some new approach? In the Media Session spec, the 'controls' attribute is a nullable type (denoted by a ?). Accessors for nullable types are expected to set a reference parameter indicating whether the value is null. We can also see this pattern in geolocation/Coordinates. Comment on attachment 254071 [details] Patch (revised) View in context: https://bugs.webkit.org/attachment.cgi?id=254071&action=review > Source/WebCore/Modules/mediasession/MediaSession.cpp:48 > + MediaRemoteControls* controls = m_controls.get(); > + isNull = controls == nullptr; > + return controls; WebKit style is to test for a NULL object with "!", so something like the following would be better: isNull = !m_controls; return controls.get(); Created attachment 254091 [details]
Patch (style fix)
Comment on attachment 254091 [details] Patch (style fix) Rejecting attachment 254091 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 254091, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: at 13247. Hunk #3 succeeded at 14609 (offset 12 lines). Hunk #4 succeeded at 19235 (offset 18 lines). Hunk #5 succeeded at 25843 (offset 20 lines). Hunk #6 succeeded at 28940 (offset 24 lines). Hunk #7 succeeded at 29429 (offset 24 lines). 1 out of 7 hunks FAILED -- saving rejects to file Source/WebCore/WebCore.xcodeproj/project.pbxproj.rej Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Eric Carlson']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.appspot.com/results/5129197307559936 Created attachment 254096 [details]
Patch (rebased)
Comment on attachment 254096 [details] Patch (rebased) Clearing flags on attachment: 254096 Committed r185126: <http://trac.webkit.org/changeset/185126> All reviewed patches have been landed. Closing bug. |