Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add setActionHandler() and rewrite parts of the specification related to controls handling (closes #147, closes #148) #159

Merged
merged 1 commit into from Jan 4, 2017

Conversation

mounirlamouri
Copy link
Member

@xxyzzzq PTAL

@mounirlamouri
Copy link
Member Author

@xxyzzzq this isn't finished but I thought it might be good for you to have an early look :)

@mounirlamouri
Copy link
Member Author

A preview should be available here: https://mounirlamouri.github.io/sandbox/mediasession.html

};
</pre>

<p>
A {{MediaSession}} objects represents a media session for a given document and
allows a document to communicate to the user agent some information about the
playback and how to handle it.
playback and how to handlei it.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/handlei/handle

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

the user agent MUST dispatch either a <a enum-value
for=MediaSessionAction>play</a> or <a enum-value
for=MediaSessionAction>pause</a> to the <a>media control event handler</a> of
the <a>most meaningful media session</a> based on the <a>actual playback
Copy link
Contributor

Choose a reason for hiding this comment

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

You might need to rebase onto my latest change to active media session

Copy link
Member Author

Choose a reason for hiding this comment

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

Just did 😄

<p>
A <dfn>media session action</dfn> is an action that the page can handle in
order for the user to interact with the {{MediaSession}}. For example, a page
can handle some actions that will then be triggered when the user presses
Copy link
Contributor

Choose a reason for hiding this comment

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

s/then be triggered/be fired?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fired is used for events so I'm afraid this could be confusing.

</p>

<p>
A <a>media session action</a> is described by a {{MediaSessionAction}} which
Copy link
Contributor

Choose a reason for hiding this comment

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

a {{MediaSessionAction}} enum value?

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed "described" by "represented". {{MediaSessionAction}} is an enum so I'm not sure if we need to repeat it in the text.

</li>
<li>
<dfn enum-value for=MediaSessionAction>seekbackward</dfn>: the action
intent to move the playback time backward by a few second.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/second/seconds
Or maybe we should say "by a short period"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Used "by a short period (eg. few seconds)".

</li>
<li>
<dfn enum-value for=MediaSessionAction>seekforward</dfn>: the action
intent is to move the playback time forward by a few second.
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Member Author

Choose a reason for hiding this comment

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

ditto

</li>
<li>
<dfn enum-value for=MediaSessionAction>previoustrack</dfn>: the action
intent is to either starts the current playback from the beginning if the
Copy link
Contributor

Choose a reason for hiding this comment

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

s/starts/start

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

<p>
A <dfn title='media session action source'>media session action source</dfn>
is a source that might produce a <a>media session action</a>. Such source can
be the system or the user agent for UI surfaces created by it.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/system/platform to be consistent?

s/the user agent for UI surfaces created by it/the UI surfaces created by the user agent?

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the concept of "action source" :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done for both.

agent MAY <a>queue a task</a> in order to notify all the known <a>media
session action sources</a> in order to avoid UI flickering when multiple
actions are modified in the same event loop.
</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Add some word saying that the media session action sources should update themselves to be ready to receive the supported actions?

Maybe mention the "Processing media control actions" section? Maybe some merging is needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a note so it is not normative. I will check that the spec says that the sources should be updated. Otherwise, I will add this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I think I confused this with something else. Done.

<p>
It is RECOMMENDED for user agents to only handle the <a enum-value
for=MediaSessionAction>play</a> and <a enum-value
for=MediaSessionAction>pause</a> <a>media session actions</a> by default.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/by default/using internal implementation by default?
s/"media session actions"/"media session actions" (ditto for other places)

Copy link
Member Author

Choose a reason for hiding this comment

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

I rephrased a bit the RECOMMENDED part.

Not sure what the s/"media session actions"/"media session actions" meant. It looks like the same string to me.

Copy link
Contributor

@xxyzzzq xxyzzzq Jan 3, 2017

Choose a reason for hiding this comment

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

Not sure what the s/"media session actions"/"media session actions" meant. It looks like the same string to me.

Sorry, I mean to change it to <a lt="media session action">media session actions</a> because we don't have <dfn>media session actions</dfn> (the plural form).

Just realized that GitHub removes the tags unless I quote them in ``.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think bikeshed handles the plural form automatically (at least, it seems) :)

@mounirlamouri mounirlamouri force-pushed the set-action-handler branch 2 times, most recently from 6ae46fe to 29c2360 Compare January 3, 2017 15:47
@mounirlamouri
Copy link
Member Author

mounirlamouri commented Jan 3, 2017

PTAL. I've updated https://mounirlamouri.github.io/sandbox/mediasession.html but it might take a few minutes to propagate.

Copy link
Contributor

@xxyzzzq xxyzzzq left a comment

Choose a reason for hiding this comment

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

LGTM w/ nits. Thanks!

<ul>
<li>
<dfn enum-value for=MediaSessionAction>play</dfn>: the action intent is to
resume the playback. The current <a>playback state</a> should not be
Copy link
Contributor

Choose a reason for hiding this comment

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

When this action is triggered from a media session action source, the current playback state should not be ...?

I mean both play and pause handlers can present at the same time, while the page can specify one play/pause playback state, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed.

<li>
<dfn enum-value for=MediaSessionAction>pause</dfn>: the action intent is
to pause a currently active playback. The current <a>playback state</a>
should be <a enum-value for=MediaSessionPlaybackState>playing</a>.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

Copy link
Member Author

Choose a reason for hiding this comment

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

ditto

</li>
</ol>
Whenever the <a>active media session</a> is changed, the user agent MUST run the
<a>media session actions update algorithm</a>.
Copy link
Contributor

Choose a reason for hiding this comment

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

It's my mistake in the last PR. The UA must also run the metadata update algorithm as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@mounirlamouri mounirlamouri merged commit 3d36a82 into master Jan 4, 2017
@mounirlamouri mounirlamouri deleted the set-action-handler branch January 4, 2017 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants