Bug 217797 – Add skeleton implementation of Media Session API

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

Add skeleton implementation of Media Session API

[Jer Noble]

Created attachment 411538 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/70367487>

[Jer Noble]

Created attachment 411585 [details]
Patch

[Jer Noble]

Created attachment 411587 [details]
Patch

[Jer Noble]

Created attachment 411589 [details]
Patch

[Jer Noble]

Created attachment 411625 [details]
Patch

[Jer Noble]

Created attachment 411657 [details]
Patch

[Darin Adler]

Comment on attachment 411657 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=411657&action=review

> Source/WebCore/Modules/mediasession/MediaMetadata.cpp:68
> +    m_title = title;
> +    metadataUpdated();

Sometimes we optimize these kinds of functions for the cases where the new value is == the old, and sometimes that’s an important property for the functions to have, because the case is either common enough to be worth optimizing or in some cases because  adding the check avoids infinite recursion. What about here?

> Source/WebCore/Modules/mediasession/MediaMetadata.cpp:74
> +    m_artist = artist;
> +    metadataUpdated();

Ditto.

> Source/WebCore/Modules/mediasession/MediaMetadata.cpp:80
> +    m_album = album;
> +    metadataUpdated();

Ditto.

> Source/WebCore/Modules/mediasession/MediaMetadata.h:51
> +    void setTitle(String);

Often we’d use const String& instead of String here.

> Source/WebCore/Modules/mediasession/MediaMetadata.h:54
> +    void setArtist(String);

Ditto.

> Source/WebCore/Modules/mediasession/MediaMetadata.h:57
> +    void setAlbum(String);

Ditto.

> Source/WebCore/Modules/mediasession/MediaMetadata.idl:35
> +  [CallWith=ScriptExecutionContext, MayThrowException] constructor(optional MediaMetadataInit init);
> +  attribute DOMString title;
> +  attribute DOMString artist;
> +  attribute DOMString album;
> +  [SetterCallWith=ScriptExecutionContext, MayThrowException] attribute FrozenArray<MediaImage> artwork;

Should indent 4?

Should we use Document instead of ScriptExecutionContext? I can think of two reasons to use ScriptExecution context:

1) Some day we might want this to be worker-compatible.
2) The concept is clearly about script execution, not about documents.

And two reasons to use Document:

1) The concept of a base URL is something associated with documents, not "script execution".
2) It has a shorter name that is easier to understand, more concrete, and used more often.

> Source/WebCore/Modules/mediasession/MediaMetadataInit.h:37
> +    String title;
> +    String artist;
> +    String album;

These are defaulting to null string. Would we like them to default to empty string instead? Does it even matter?

> Source/WebCore/Modules/mediasession/MediaMetadataInit.idl:33
> +  DOMString title = "";
> +  DOMString artist = "";
> +  DOMString album = "";
> +  sequence<MediaImage> artwork = [];

Indent 4 please.

> Source/WebCore/Modules/mediasession/MediaPositionState.h:35
> +    double duration;
> +    double playbackRate;
> +    double position;

Should these have defaults? Seems unimportant when used for bindings I guess, since the defaults are done by the bindings generator, but nice to have everything initialized generally.

> Source/WebCore/Modules/mediasession/MediaPositionState.idl:32
> +  double duration;
> +  double playbackRate = 1;
> +  double position = 0;

Indent 4 please. Why no default for duration?

> Source/WebCore/Modules/mediasession/MediaSession.cpp:62
> +    notImplemented();

Are these really valuable? There are so many things we have unimplemented without calling these functions. I am not sure they help much.

> Source/WebCore/Modules/mediasession/MediaSession.cpp:67
> +    notImplemented();

Ditto (won’t say it again).

> Source/WebCore/Modules/mediasession/MediaSession.cpp:80
> +    if (state->duration < 0
> +        || state->position < 0
> +        || state->playbackRate == 0
> +        || state->position > state->duration)

These checks aren’t written in the NaN-proof way. Maybe we should. We do that by writing positive checks and using !, since expressions involving NaN evaluate false: if (!good) rather than if (bad).

> Source/WebCore/Modules/mediasession/MediaSession.h:48
> +    const RefPtr<MediaMetadata>& metadata() const { return m_metadata; };

Return type could be MediaMetadata*.

> Source/WebCore/Modules/mediasession/MediaSession.h:60
> +    MediaSession(Navigator&);

explicit please

> Source/WebCore/Modules/mediasession/MediaSession.idl:37
> +  attribute MediaMetadata? metadata;
> +
> +  attribute MediaSessionPlaybackState playbackState;
> +
> +  undefined setActionHandler(MediaSessionAction action, MediaSessionActionHandler? handler);
> +
> +  [MayThrowException] undefined setPositionState(optional MediaPositionState? state = null);

indent 4?

> Source/WebCore/Modules/mediasession/MediaSessionAction.idl:37
> +  "play",
> +  "pause",
> +  "seekbackward",
> +  "seekforward",
> +  "previoustrack",
> +  "nexttrack",
> +  "skipad",
> +  "stop",
> +  "seekto"

Indent 4

> Source/WebCore/Modules/mediasession/MediaSessionActionDetails.h:36
> +    MediaSessionAction action;

Default value? (See above for rationale).

> Source/WebCore/Modules/mediasession/MediaSessionActionDetails.idl:29
> +] dictionary MediaSessionActionDetails {

Do these dictionaries really each need to be in separate files?

> Source/WebCore/Modules/mediasession/MediaSessionPlaybackState.idl:31
> +  "none",
> +  "paused",
> +  "playing"

Indent 4.

> Source/WebCore/Modules/mediasession/Navigator+MediaSession.idl:32
> +  [SameObject] readonly attribute MediaSession mediaSession;

Indent 4.

[Jer Noble]

(In reply to Darin Adler from comment #8)
> Comment on attachment 411657 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411657&action=review
> 
> > Source/WebCore/Modules/mediasession/MediaMetadata.cpp:68
> > +    m_title = title;
> > +    metadataUpdated();
> 
> Sometimes we optimize these kinds of functions for the cases where the new
> value is == the old, and sometimes that’s an important property for the
> functions to have, because the case is either common enough to be worth
> optimizing or in some cases because  adding the check avoids infinite
> recursion. What about here?

Sure; these were intended purely as skeleton implementations, but you

> > Source/WebCore/Modules/mediasession/MediaMetadata.cpp:74
> > +    m_artist = artist;
> > +    metadataUpdated();
> 
> Ditto.
> 
> > Source/WebCore/Modules/mediasession/MediaMetadata.cpp:80
> > +    m_album = album;
> > +    metadataUpdated();
> 
> Ditto.
> 
> > Source/WebCore/Modules/mediasession/MediaMetadata.h:51
> > +    void setTitle(String);
> 
> Often we’d use const String& instead of String here.
> 
> > Source/WebCore/Modules/mediasession/MediaMetadata.h:54
> > +    void setArtist(String);
> 
> Ditto.
> 
> > Source/WebCore/Modules/mediasession/MediaMetadata.h:57
> > +    void setAlbum(String);
> 
> Ditto.
> 
> > Source/WebCore/Modules/mediasession/MediaMetadata.idl:35
> > +  [CallWith=ScriptExecutionContext, MayThrowException] constructor(optional MediaMetadataInit init);
> > +  attribute DOMString title;
> > +  attribute DOMString artist;
> > +  attribute DOMString album;
> > +  [SetterCallWith=ScriptExecutionContext, MayThrowException] attribute FrozenArray<MediaImage> artwork;
> 
> Should indent 4?
> 
> Should we use Document instead of ScriptExecutionContext? I can think of two
> reasons to use ScriptExecution context:
> 
> 1) Some day we might want this to be worker-compatible.
> 2) The concept is clearly about script execution, not about documents.
> 
> And two reasons to use Document:
> 
> 1) The concept of a base URL is something associated with documents, not
> "script execution".
> 2) It has a shorter name that is easier to understand, more concrete, and
> used more often.
> 
> > Source/WebCore/Modules/mediasession/MediaMetadataInit.h:37
> > +    String title;
> > +    String artist;
> > +    String album;
> 
> These are defaulting to null string. Would we like them to default to empty
> string instead? Does it even matter?
> 
> > Source/WebCore/Modules/mediasession/MediaMetadataInit.idl:33
> > +  DOMString title = "";
> > +  DOMString artist = "";
> > +  DOMString album = "";
> > +  sequence<MediaImage> artwork = [];
> 
> Indent 4 please.
> 
> > Source/WebCore/Modules/mediasession/MediaPositionState.h:35
> > +    double duration;
> > +    double playbackRate;
> > +    double position;
> 
> Should these have defaults? Seems unimportant when used for bindings I
> guess, since the defaults are done by the bindings generator, but nice to
> have everything initialized generally.
> 
> > Source/WebCore/Modules/mediasession/MediaPositionState.idl:32
> > +  double duration;
> > +  double playbackRate = 1;
> > +  double position = 0;
> 
> Indent 4 please. Why no default for duration?
> 
> > Source/WebCore/Modules/mediasession/MediaSession.cpp:62
> > +    notImplemented();
> 
> Are these really valuable? There are so many things we have unimplemented
> without calling these functions. I am not sure they help much.
> 
> > Source/WebCore/Modules/mediasession/MediaSession.cpp:67
> > +    notImplemented();
> 
> Ditto (won’t say it again).
> 
> > Source/WebCore/Modules/mediasession/MediaSession.cpp:80
> > +    if (state->duration < 0
> > +        || state->position < 0
> > +        || state->playbackRate == 0
> > +        || state->position > state->duration)
> 
> These checks aren’t written in the NaN-proof way. Maybe we should. We do
> that by writing positive checks and using !, since expressions involving NaN
> evaluate false: if (!good) rather than if (bad).
> 
> > Source/WebCore/Modules/mediasession/MediaSession.h:48
> > +    const RefPtr<MediaMetadata>& metadata() const { return m_metadata; };
> 
> Return type could be MediaMetadata*.
> 
> > Source/WebCore/Modules/mediasession/MediaSession.h:60
> > +    MediaSession(Navigator&);
> 
> explicit please
> 
> > Source/WebCore/Modules/mediasession/MediaSession.idl:37
> > +  attribute MediaMetadata? metadata;
> > +
> > +  attribute MediaSessionPlaybackState playbackState;
> > +
> > +  undefined setActionHandler(MediaSessionAction action, MediaSessionActionHandler? handler);
> > +
> > +  [MayThrowException] undefined setPositionState(optional MediaPositionState? state = null);
> 
> indent 4?
> 
> > Source/WebCore/Modules/mediasession/MediaSessionAction.idl:37
> > +  "play",
> > +  "pause",
> > +  "seekbackward",
> > +  "seekforward",
> > +  "previoustrack",
> > +  "nexttrack",
> > +  "skipad",
> > +  "stop",
> > +  "seekto"
> 
> Indent 4
> 
> > Source/WebCore/Modules/mediasession/MediaSessionActionDetails.h:36
> > +    MediaSessionAction action;
> 
> Default value? (See above for rationale).
> 
> > Source/WebCore/Modules/mediasession/MediaSessionActionDetails.idl:29
> > +] dictionary MediaSessionActionDetails {
> 
> Do these dictionaries really each need to be in separate files?
> 
> > Source/WebCore/Modules/mediasession/MediaSessionPlaybackState.idl:31
> > +  "none",
> > +  "paused",
> > +  "playing"
> 
> Indent 4.
> 
> > Source/WebCore/Modules/mediasession/Navigator+MediaSession.idl:32
> > +  [SameObject] readonly attribute MediaSession mediaSession;
> 
> Indent 4.

[Jer Noble]

(In reply to Darin Adler from comment #8)
> Comment on attachment 411657 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411657&action=review
> 
> > Source/WebCore/Modules/mediasession/MediaMetadata.cpp:68
> > +    m_title = title;
> > +    metadataUpdated();
> 
> Sometimes we optimize these kinds of functions for the cases where the new
> value is == the old, and sometimes that’s an important property for the
> functions to have, because the case is either common enough to be worth
> optimizing or in some cases because  adding the check avoids infinite
> recursion. What about here?

Sure. These were intended as purely skeleton, no-op implementations, but we can absolutely bail early in each of these setters.

> > Source/WebCore/Modules/mediasession/MediaMetadata.h:51
> > +    void setTitle(String);
> 
> Often we’d use const String& instead of String here.

Ok.

> > Source/WebCore/Modules/mediasession/MediaMetadata.idl:35
> > +  [CallWith=ScriptExecutionContext, MayThrowException] constructor(optional MediaMetadataInit init);
> > +  attribute DOMString title;
> > +  attribute DOMString artist;
> > +  attribute DOMString album;
> > +  [SetterCallWith=ScriptExecutionContext, MayThrowException] attribute FrozenArray<MediaImage> artwork;
> 
> Should indent 4?

Absolutely.

> Should we use Document instead of ScriptExecutionContext? I can think of two
> reasons to use ScriptExecution context:
> 
> 1) Some day we might want this to be worker-compatible.
> 2) The concept is clearly about script execution, not about documents.
> 
> And two reasons to use Document:
> 
> 1) The concept of a base URL is something associated with documents, not
> "script execution".
> 2) It has a shorter name that is easier to understand, more concrete, and
> used more often.

There is some weirdness here in the spec, where this method is defined to reference the "entry settings object" <https://html.spec.whatwg.org/multipage/webappapis.html#entry-settings-object>, which is very much a "script execution" thing, but isn't something we currently support in our own IDL/bindings.  So neither is really "correct", and one of the metatada tests fails as a result. HTML is attempting to remove this concept from specs, so it may not be worth adding support for this concept.

> > Source/WebCore/Modules/mediasession/MediaMetadataInit.h:37
> > +    String title;
> > +    String artist;
> > +    String album;
> 
> These are defaulting to null string. Would we like them to default to empty
> string instead? Does it even matter?

I don't think it matters.

> > Source/WebCore/Modules/mediasession/MediaMetadataInit.idl:33
> > +  DOMString title = "";
> > +  DOMString artist = "";
> > +  DOMString album = "";
> > +  sequence<MediaImage> artwork = [];
> 
> Indent 4 please.

Ok.

> > Source/WebCore/Modules/mediasession/MediaPositionState.h:35
> > +    double duration;
> > +    double playbackRate;
> > +    double position;
> 
> Should these have defaults? Seems unimportant when used for bindings I
> guess, since the defaults are done by the bindings generator, but nice to
> have everything initialized generally.

We could have defaults here. Duration is weird because the spec does not specify a default value, the idl is a restricted double (so no NaN), and zero is illegal. Any default will be arbitrary.

> > Source/WebCore/Modules/mediasession/MediaPositionState.idl:32
> > +  double duration;
> > +  double playbackRate = 1;
> > +  double position = 0;
> 
> Indent 4 please. Why no default for duration?

This is a bit of spec hacking.  The IDL from the spec doesn't define a default value but the spec text does. As I alluded to above, the spec doesn't define a default for duration (you are required to throw an error if no value is provided, or if zero or NaN is provided).

> > Source/WebCore/Modules/mediasession/MediaSession.cpp:62
> > +    notImplemented();
> 
> Are these really valuable? There are so many things we have unimplemented
> without calling these functions. I am not sure they help much.

These are just a note to future implementors (i.e., me) to: "please fill out this method".

> > Source/WebCore/Modules/mediasession/MediaSession.cpp:80
> > +    if (state->duration < 0
> > +        || state->position < 0
> > +        || state->playbackRate == 0
> > +        || state->position > state->duration)
> 
> These checks aren’t written in the NaN-proof way. Maybe we should. We do
> that by writing positive checks and using !, since expressions involving NaN
> evaluate false: if (!good) rather than if (bad).

I'm pretty sure that the IDL should keep out NaN values, but in case these are initialized by another C++ method, I'll add explicit NaN checks here.

> > Source/WebCore/Modules/mediasession/MediaSession.h:48
> > +    const RefPtr<MediaMetadata>& metadata() const { return m_metadata; };
> 
> Return type could be MediaMetadata*.

I wasn't sure what the current recommendations were w.r.t. "raw pointer return values". I'll ping Ryosuke.

> > Source/WebCore/Modules/mediasession/MediaSession.h:60
> > +    MediaSession(Navigator&);
> 
> explicit please

Ok.

> > Source/WebCore/Modules/mediasession/MediaSession.idl:37
> > +  attribute MediaMetadata? metadata;
> > +
> > +  attribute MediaSessionPlaybackState playbackState;
> > +
> > +  undefined setActionHandler(MediaSessionAction action, MediaSessionActionHandler? handler);
> > +
> > +  [MayThrowException] undefined setPositionState(optional MediaPositionState? state = null);
> 
> indent 4?

I have no idea how all the .idl ended up with only 2 space indents. :-/

> > Source/WebCore/Modules/mediasession/MediaSessionAction.idl:37
> > +  "play",
> > +  "pause",
> > +  "seekbackward",
> > +  "seekforward",
> > +  "previoustrack",
> > +  "nexttrack",
> > +  "skipad",
> > +  "stop",
> > +  "seekto"
> 
> Indent 4

Ok.

> > Source/WebCore/Modules/mediasession/MediaSessionActionDetails.h:36
> > +    MediaSessionAction action;
> 
> Default value? (See above for rationale).

I don't love this; there's no explicit "none" enum, and so we'd have to say that the default action is "play", which seems arbitrary.  But so is uninitialized memory, so :shrug:, "play" it is.

> > Source/WebCore/Modules/mediasession/MediaSessionActionDetails.idl:29
> > +] dictionary MediaSessionActionDetails {
> 
> Do these dictionaries really each need to be in separate files?

Yes, unfortunately, they do. The bindings generator will look for JSMediaSessionActionDetails.h.

There may be a way to add "ImplementedBy" (or similar) flags to control this behavior, but I'm not aware of it off the top of my head.

> > Source/WebCore/Modules/mediasession/MediaSessionPlaybackState.idl:31
> > +  "none",
> > +  "paused",
> > +  "playing"
> 
> Indent 4.
> 
> > Source/WebCore/Modules/mediasession/Navigator+MediaSession.idl:32
> > +  [SameObject] readonly attribute MediaSession mediaSession;
> 
> Indent 4.

Ok.

[Darin Adler]

Comment on attachment 411657 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=411657&action=review

>>> Source/WebCore/Modules/mediasession/MediaPositionState.h:35
>>> +    double position;
>> 
>> Should these have defaults? Seems unimportant when used for bindings I guess, since the defaults are done by the bindings generator, but nice to have everything initialized generally.
> 
> We could have defaults here. Duration is weird because the spec does not specify a default value, the idl is a restricted double (so no NaN), and zero is illegal. Any default will be arbitrary.

OK. Here’s my last argument: Setting a default that never has any effect still reduces the chance of reading an uninitialized value if we make a coding mistake. I suggest we always set defaults for scalars in classes and structures, even when they have no effect, unless omitting the default is an intentional, important optimization.

[Darin Adler]

Comment on attachment 411657 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=411657&action=review

>>> Source/WebCore/Modules/mediasession/MediaSession.cpp:62
>>> +    notImplemented();
>> 
>> Are these really valuable? There are so many things we have unimplemented without calling these functions. I am not sure they help much.
> 
> These are just a note to future implementors (i.e., me) to: "please fill out this method".

But is that valuable? We’ll know to fill out methods when tests don’t pass.

[Darin Adler]

Comment on attachment 411657 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=411657&action=review

>>> Source/WebCore/Modules/mediasession/MediaSession.h:48
>>> +    const RefPtr<MediaMetadata>& metadata() const { return m_metadata; };
>> 
>> Return type could be MediaMetadata*.
> 
> I wasn't sure what the current recommendations were w.r.t. "raw pointer return values". I'll ping Ryosuke.

Good point. I have been using just RefPtr<X> and I see how const RefPtr<X>& has some value if you don’t put the result into a local variable (and there’s nothing super-tricky about the object you are getting the pointer from), so I retract my suggestion to use a raw pointer.

[Darin Adler]

Comment on attachment 411657 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=411657&action=review

>>>> Source/WebCore/Modules/mediasession/MediaSession.h:48
>>>> +    const RefPtr<MediaMetadata>& metadata() const { return m_metadata; };
>>> 
>>> Return type could be MediaMetadata*.
>> 
>> I wasn't sure what the current recommendations were w.r.t. "raw pointer return values". I'll ping Ryosuke.
> 
> Good point. I have been using just RefPtr<X> and I see how const RefPtr<X>& has some value if you don’t put the result into a local variable (and there’s nothing super-tricky about the object you are getting the pointer from), so I retract my suggestion to use a raw pointer.

Here’s some thoughts on the risk of returning const RefPtr<X>& vs. RefPtr<X>:

The *risk* of returning X* is if we keep it around after the value is set to something new, or after the object we got it from is itself destroyed. This can happen surprisingly subtly and quickly, even during the lifetime of an expression if there are other functions being called, say to evaluate other arguments to a function.

Both those dangers also exist with const RefPtr<X>& if we keep a *reference* to it around, say with auto& or by passing the value to a function that takes an argument of type const RefPtr<X>&, or if we call get() on it to pass it as a function argument, while they don’t with RefPtr<X> in any of those cases. If we put the result into a local variable, then const RefPtr<X>& is better than X*, if we use *auto*. If we use RefPtr<X> for the local variable type explicitly, then there is no difference.

The *value* of returning const RefPtr<X>& is that there is no reference count churn if you simply get the object and call a function on it, just as with X*.

So note that the main value of const RefPtr<X>& as better than X* occurs if we use "auto". I think I prefer RefPtr<X> even with the churn. And a RefPtr<X> return value just gives us a compile error if we use auto& with it, which is better I think.

[Darin Adler]

By the way, my intent was to say review+ as-is, and raise these other minor issues as a sidebar.

[Jer Noble]

(In reply to Darin Adler from comment #15)
> By the way, my intent was to say review+ as-is, and raise these other minor
> issues as a sidebar.

(In reply to Darin Adler from comment #12)
> Comment on attachment 411657 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411657&action=review
> 
> >>> Source/WebCore/Modules/mediasession/MediaSession.cpp:62
> >>> +    notImplemented();
> >> 
> >> Are these really valuable? There are so many things we have unimplemented without calling these functions. I am not sure they help much.
> > 
> > These are just a note to future implementors (i.e., me) to: "please fill out this method".
> 
> But is that valuable? We’ll know to fill out methods when tests don’t pass.

I take your point. This is the equivalent of "this page left intentionally blank", and could just as easily be a comment rather than code, if that.

[Jer Noble]

(In reply to Darin Adler from comment #13)
> Comment on attachment 411657 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411657&action=review
> 
> >>> Source/WebCore/Modules/mediasession/MediaSession.h:48
> >>> +    const RefPtr<MediaMetadata>& metadata() const { return m_metadata; };
> >> 
> >> Return type could be MediaMetadata*.
> > 
> > I wasn't sure what the current recommendations were w.r.t. "raw pointer return values". I'll ping Ryosuke.
> 
> Good point. I have been using just RefPtr<X> and I see how const RefPtr<X>&
> has some value if you don’t put the result into a local variable (and
> there’s nothing super-tricky about the object you are getting the pointer
> from), so I retract my suggestion to use a raw pointer.

Ryosuke says that the current recommendation is that refcounting is the responsibility of the caller, not the callee, to implement. So it's still OK to return raw pointers and references, and require the caller to put it into a RefPtr or Ref. So I'll make this change too.

[Jer Noble]

Created attachment 411761 [details]
Patch for landing

[Jer Noble]

Created attachment 411762 [details]
Patch for landing

[Jer Noble]

Created attachment 411798 [details]
Patch for landing

[Darin Adler]

Comment on attachment 411798 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=411798&action=review

> Source/WTF/wtf/PlatformEnable.h:361
> +#if !defined(ENABLE_MEDIA_SESSION)
> +#define ENABLE_MEDIA_SESSION 0
> +#endif

I don’t think we need this. Everything defaults to off, don’t need to set it explicitly.

> Source/WebCore/Modules/mediasession/MediaMetadata.cpp:112
> +}
> +
> +
> +}

Extra blank line here we normally don’t leave.

> Source/WebCore/Modules/mediasession/MediaMetadata.h:39
> +class MediaSession;
> +struct MediaImage;

Normally leave a blank line here for separate paragraphs.

> Source/WebCore/Modules/mediasession/MediaSession.cpp:79
> +    if (std::isnan(state->duration)
> +        || std::isnan(state->position)
> +        || std::isnan(state->playbackRate)
> +        || state->duration < 0
> +        || state->position < 0
> +        || state->playbackRate == 0
> +        || state->position > state->duration)
> +        return Exception { TypeError };

Explicit checks for NaN were not what I was hoping for. However if you are checking that then you could check std::isfinite instead. Or you could try the !(good) version instead of the (bad) version that I was suggesting.

> Source/WebCore/Modules/mediasession/NavigatorMediaSession.h:45
> +    static Ref<MediaSession> mediaSession(Navigator&);
> +    Ref<MediaSession> mediaSession();

If we are not trying to do smart pointers for return values, these could be MediaSession&.

[Jer Noble]

(In reply to Darin Adler from comment #21)
> Comment on attachment 411798 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411798&action=review
> 
> > Source/WTF/wtf/PlatformEnable.h:361
> > +#if !defined(ENABLE_MEDIA_SESSION)
> > +#define ENABLE_MEDIA_SESSION 0
> > +#endif
> 
> I don’t think we need this. Everything defaults to off, don’t need to set it
> explicitly.

That's true, but it really appears to be the convention.  Maybe that convention is just an artifact from splitting off all the "Platform<Platform>.h" files. I'll remove it.

> > Source/WebCore/Modules/mediasession/MediaMetadata.cpp:112
> > +}
> > +
> > +
> > +}
> 
> Extra blank line here we normally don’t leave.

Removed.

> > Source/WebCore/Modules/mediasession/MediaMetadata.h:39
> > +class MediaSession;
> > +struct MediaImage;
> 
> Normally leave a blank line here for separate paragraphs.
> 
> > Source/WebCore/Modules/mediasession/MediaSession.cpp:79
> > +    if (std::isnan(state->duration)
> > +        || std::isnan(state->position)
> > +        || std::isnan(state->playbackRate)
> > +        || state->duration < 0
> > +        || state->position < 0
> > +        || state->playbackRate == 0
> > +        || state->position > state->duration)
> > +        return Exception { TypeError };
> 
> Explicit checks for NaN were not what I was hoping for. However if you are
> checking that then you could check std::isfinite instead. Or you could try
> the !(good) version instead of the (bad) version that I was suggesting.

Okay, I'll flip it around. I just also realized the spec disallows infinite values for each of these members; that seems correct for playbackRate but very wrong for duration, where the convention is that live streams are infinite in duration.

> > Source/WebCore/Modules/mediasession/NavigatorMediaSession.h:45
> > +    static Ref<MediaSession> mediaSession(Navigator&);
> > +    Ref<MediaSession> mediaSession();
> 
> If we are not trying to do smart pointers for return values, these could be
> MediaSession&.

Ok. I stole that convention from another NavigatorFoo.h file. And if we do this, we could just use RefPtr<> rather than Optional<Ref<>> to store the member.

[Darin Adler]

Comment on attachment 411798 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=411798&action=review

>>> Source/WebCore/Modules/mediasession/NavigatorMediaSession.h:45
>>> +    Ref<MediaSession> mediaSession();
>> 
>> If we are not trying to do smart pointers for return values, these could be MediaSession&.
> 
> Ok. I stole that convention from another NavigatorFoo.h file. And if we do this, we could just use RefPtr<> rather than Optional<Ref<>> to store the member.

We could do that in either case. To return a Ref<> from a RefPtr<>, just return *m_mediaSession.

[Jer Noble]

Created attachment 411807 [details]
Patch for landing

[EWS]

ChangeLog entry in LayoutTests/imported/w3c/ChangeLog contains OOPS!.

[Jer Noble]

Committed r268592: <https://trac.webkit.org/changeset/268592/webkit>

[Jer Noble]

Whoops, wrong revision. Correct commit:

Committed r268735: <https://trac.webkit.org/changeset/268735/webkit>