Bug 145530 – Add stub implementation of MediaSession, part of Media Session spec

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

[Matt Rajca]

Add a stub implementation of MediaSession, part of the Media Session spec: https://mediasession.spec.whatwg.org/

[Radar WebKit Bug Importer]

<rdar://problem/21191606>

[Matt Rajca]

Created attachment 254028 [details]
Patch

[Darin Adler]

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.

[Matt Rajca]

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.

[Matt Rajca]

Created attachment 254071 [details]
Patch (revised)

[Darin Adler]

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?

[Matt Rajca]

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.

[Eric Carlson]

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();

[Matt Rajca]

Created attachment 254091 [details]
Patch (style fix)

[WebKit Commit Bot]

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

[Matt Rajca]

Created attachment 254096 [details]
Patch (rebased)

[WebKit Commit Bot]

Comment on attachment 254096 [details]
Patch (rebased)

Clearing flags on attachment: 254096

Committed r185126: <http://trac.webkit.org/changeset/185126>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.