[webkit-reviews] review granted: [Bug 217797] Add skeleton implementation of Media Session API : [Attachment 411657] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Jer Noble <jer.noble at apple.com>'s
request for review:
Bug 217797: Add skeleton implementation of Media Session API
https://bugs.webkit.org/show_bug.cgi?id=217797

Attachment 411657: Patch

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




--- Comment #8 from Darin Adler <darin at apple.com> ---
Comment on attachment 411657
  --> https://bugs.webkit.org/attachment.cgi?id=411657
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.