Bug 145530 - Add stub implementation of MediaSession, part of Media Session spec
Summary: Add stub implementation of MediaSession, part of Media Session spec
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 145411
  Show dependency treegraph
 
Reported: 2015-06-01 15:43 PDT by Matt Rajca
Modified: 2015-06-02 14:54 PDT (History)
6 users (show)

See Also:


Attachments
Patch (13.34 KB, patch)
2015-06-01 16:59 PDT, Matt Rajca
no flags Details | Formatted Diff | Diff
Patch (revised) (17.55 KB, patch)
2015-06-02 10:10 PDT, Matt Rajca
no flags Details | Formatted Diff | Diff
Patch (style fix) (17.54 KB, patch)
2015-06-02 13:25 PDT, Matt Rajca
no flags Details | Formatted Diff | Diff
Patch (rebased) (16.70 KB, patch)
2015-06-02 13: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 15:43:53 PDT
Add a stub implementation of MediaSession, part of the Media Session spec: https://mediasession.spec.whatwg.org/
Comment 1 Radar WebKit Bug Importer 2015-06-01 15:44:59 PDT
<rdar://problem/21191606>
Comment 2 Matt Rajca 2015-06-01 16:59:06 PDT
Created attachment 254028 [details]
Patch
Comment 3 Darin Adler 2015-06-01 17:06:10 PDT
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 4 Matt Rajca 2015-06-02 10:02:03 PDT
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.
Comment 5 Matt Rajca 2015-06-02 10:10:04 PDT
Created attachment 254071 [details]
Patch (revised)
Comment 6 Darin Adler 2015-06-02 11:25:05 PDT
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 7 Matt Rajca 2015-06-02 12:18:21 PDT
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 8 Eric Carlson 2015-06-02 13:22:27 PDT
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();
Comment 9 Matt Rajca 2015-06-02 13:25:08 PDT
Created attachment 254091 [details]
Patch (style fix)
Comment 10 WebKit Commit Bot 2015-06-02 13:30:55 PDT
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
Comment 11 Matt Rajca 2015-06-02 13:35:17 PDT
Created attachment 254096 [details]
Patch (rebased)
Comment 12 WebKit Commit Bot 2015-06-02 14:54:38 PDT
Comment on attachment 254096 [details]
Patch (rebased)

Clearing flags on attachment: 254096

Committed r185126: <http://trac.webkit.org/changeset/185126>
Comment 13 WebKit Commit Bot 2015-06-02 14:54:41 PDT
All reviewed patches have been landed.  Closing bug.