Add playbackState attribute and remove playpause action #152

xxyzzzq · 2016-12-14T14:42:09Z

Changes:

Added playback state
Removed playpause action
Some structural change in "Processing model" section

The temporary spec after the change can be found at:
http://xxyzzzq.github.io/mediasession/

mounirlamouri

First comments after a quick look.

mounirlamouri · 2016-12-14T14:50:32Z

explainer.md

+playback state. If it's currently playing, the user agent will send pause events
+to the page and if it's currently paused, the user agent will send play events
+to the page. The default `playbackState` is `none`, the UA should use its best
+guess to decide the current playback state.


What about "stop"?

Removed "stop" for now per offline discussion

mounirlamouri · 2016-12-14T14:51:11Z

explainer.md

 interface MediaSession : EventTarget {
    attribute MediaMetadata? metadata;

+    attribute MediaSessionPlaybackState playbackState = "none";


nit: I would do // defaults no "none" instead of the non-allowed IDL syntax.

mounirlamouri · 2016-12-14T14:52:42Z

index.bs

+  </tr>
+  <tr>
+    <td>
+      <code>none</code>


You should link this with the declaration. I think <dfn for='MediaSessionPlaybackState' enum>none</dfn>

Done (actually it should be "enum-value").

mounirlamouri · 2016-12-14T14:54:58Z

index.bs

-user experience.
+user experience. An exception is that if the <a>actual playback state</a> is
+<code>"stopped"</code>, then it MUST not become a <a>tab-level active media
+session</a>.


I would say that "stopped" should be used to express that the page is no longer playing and the UA should not attempt to resume it. Saying that it should not become the active tab seems too much because if it starts playing for some reasons, it will, right? I would recommend "stopped" to be used when a playlist is finished instead of switching to "paused".

Ditto (removed stop for now)

mounirlamouri · 2016-12-15T11:18:44Z

explainer.md

+For action `play` and `pause`, in many situations, `play` and `pause` share the
+same media button. When the button is pressed, the user agent should fire
+`pause` action when the page is playing and fire `play` when the page is paused.
+The page can use attribute `playbackState` to tell the UA about the current


'can use the playbackState attribute" (inverse attribute and playbackstate)

mounirlamouri · 2016-12-15T11:20:33Z

explainer.md

+**Note** It is still an open question if we want a `stopped` value for
+  `playbackState`. The state could be useful when a playlist has finished.
+  However, it is a bit confusing with `paused`, and we should added it if
+  there's good justification. See https://github.com/WICG/mediasession/issues/xxx


Maybe we shouldn't have this in the explainer in addition of an issue. I think it's better to open an issue.

Done. Removed and filed an issue.

mounirlamouri · 2016-12-15T11:23:57Z

explainer.md

+The page can use attribute `playbackState` to tell the UA about the current
+playback state. If it's currently playing, the user agent will send pause events
+to the page and if it's currently paused, the user agent will send play events
+to the page. The default `playbackState` is `none`, the UA should use its best


Instead of making the UA guess specific to none, what about:
"""
The default playbackState is none, representing no active media session. The playbackState attribute is a hint to the UA which could ignore it in case of, for example, a page appears to be playing but has a playbackState sets to paused.
"""

mounirlamouri · 2016-12-15T11:24:17Z

index.bs

 [Exposed=Window]
 interface MediaSession : EventHandler {
  attribute MediaMetadata? metadata;

+  attribute MediaSessionPlaybackState playbackState;  // defaults to "none"


I don't think the comment is needed here.

mounirlamouri · 2016-12-15T11:25:57Z

index.bs

-metadata">metadata</a>.
+The <dfn attribute for="MediaSession"><code>playbackState</code></dfn> attribute
+represents the <dfn lt="playback state">playback state</dfn> of the <a>media
+session</a>. The attribute MUST take one value of the


No need to say which values it must take. It is implicit with the IDL.

mounirlamouri · 2016-12-15T11:27:20Z

index.bs

+{{MediaSessionPlaybackState}} enum. The default value is <a enum-value
+for="MediaSessionPlaybackState">none</a>. On setting, the user agent MUST update
+the <a>most meaningful media session</a> and run the <a>media control actions
+update algorithm</a> if needed.


On getting, the user agent MUST return the last valid value that was set. If none, it MUST <a enum-value for="MediaSessionPlaybackState">none</a>.

mounirlamouri · 2016-12-15T11:35:27Z

index.bs

+override is called <dfn>actual playback state</dfn> and could be either <a
+enum-value for="MediaSessionPlaybackState">playing</a> or <a enum-value
+for="MediaSessionPlaybackState">paused</a>. The rules for determining the
+<a>actual playback state</a> is as follows:


I think we can simplify this. In my opinion, tables make things harder to read :) What about something similar to:

The <dfn>actual playback state</dfn> SHOULD return the last value that was set on <a attribute for="MediaSession">playbackState</a>. If the user agent believes the actual playback state is <a enum-value for="MediaSessionPlaybackState">playing</a> and the <a attribute for="MediaSession">playbackState</a> returns a different value, it MAY return <a enum-value for="MediaSessionPlaybackState">playing</a> instead.

mounirlamouri · 2016-12-15T11:36:03Z

index.bs

+  </tr>
+  <tr>
+    <td>
+      <dfn enum-value for="MediaSessionPlaybackState">none</dfn>


It would be great if you could define what the states mean next to the IDL definition instead.

Done, put it as a unordered list in the section for the IDL interfaces.

mounirlamouri · 2016-12-15T11:38:22Z

index.bs

+            <a>event pause</a>.
+          </li>
+          <li>
+            If <a>actual playback state</a> is <a enum-value


s/If/Otherwise,/

mounirlamouri

Thanks! 😃

mounirlamouri · 2016-12-15T14:07:27Z

index.bs

+          </li>
+          <li>
+            Otherwise, show a button for
+            <a>event play</a>.


nit: no need for lign wrap, right?

xxyzzzq requested a review from mounirlamouri December 14, 2016 14:42

mounirlamouri reviewed Dec 14, 2016

View reviewed changes

xxyzzzq force-pushed the playback_state branch 2 times, most recently from c04ef68 to 669a822 Compare December 14, 2016 17:58

xxyzzzq changed the title ~~Add playbackState attribute and remove playpause action (#141)~~ Add playbackState attribute and remove playpause action Dec 15, 2016

mounirlamouri suggested changes Dec 15, 2016

View reviewed changes

xxyzzzq mentioned this pull request Dec 15, 2016

Do we need "stopped" value for the playbackState attribute? #153

Open

xxyzzzq force-pushed the playback_state branch 2 times, most recently from e607339 to b462b29 Compare December 15, 2016 13:47

mounirlamouri approved these changes Dec 15, 2016

View reviewed changes

index.bs

</li>

<li>

Otherwise, show a button for

<a>event play</a>.

Copy link

Member

mounirlamouri Dec 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: no need for lign wrap, right?

adding playbackState attribute

d44cff6

xxyzzzq force-pushed the playback_state branch from b462b29 to d44cff6 Compare December 15, 2016 14:43

xxyzzzq merged commit b354c48 into w3c:master Dec 15, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add playbackState attribute and remove playpause action #152

Add playbackState attribute and remove playpause action #152

xxyzzzq commented Dec 14, 2016

mounirlamouri left a comment

mounirlamouri Dec 14, 2016

xxyzzzq Dec 14, 2016

mounirlamouri Dec 14, 2016

xxyzzzq Dec 14, 2016

mounirlamouri Dec 14, 2016

xxyzzzq Dec 14, 2016

mounirlamouri Dec 14, 2016

xxyzzzq Dec 14, 2016

mounirlamouri Dec 15, 2016

xxyzzzq Dec 15, 2016

mounirlamouri Dec 15, 2016

xxyzzzq Dec 15, 2016

mounirlamouri Dec 15, 2016

xxyzzzq Dec 15, 2016

mounirlamouri Dec 15, 2016

xxyzzzq Dec 15, 2016

mounirlamouri Dec 15, 2016

xxyzzzq Dec 15, 2016

mounirlamouri Dec 15, 2016

xxyzzzq Dec 15, 2016

mounirlamouri Dec 15, 2016

xxyzzzq Dec 15, 2016

mounirlamouri Dec 15, 2016

xxyzzzq Dec 15, 2016

mounirlamouri Dec 15, 2016

xxyzzzq Dec 15, 2016

mounirlamouri left a comment

mounirlamouri Dec 15, 2016

Add playbackState attribute and remove playpause action #152

Add playbackState attribute and remove playpause action #152

Conversation

xxyzzzq commented Dec 14, 2016

mounirlamouri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mounirlamouri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment