-
Notifications
You must be signed in to change notification settings - Fork 197
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
Generalize protocol extension mechanism #1177
Conversation
The facilities offered for protocol extensions are intended to be used by both web standards and browser vendors. Prior to the application of this commit, the specification text inconsistently described these two distinct use cases [1]. The mechanism itself was biased towards the "browser vendor" use case because it required the specification of an "extension command prefix" which is not appropriate for all "web standards" use cases [2]. Explicitly describe both use cases in the informative text and generalize the normative text to better-support extensions by web standards. [1] w3c#1142 [2] w3c#1170
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good to me, thanks for making this change!
The only comment I have is that we shouldn’t relax the requirement that vendors use a namespace when introducing vendor-specific behaviour. Otherwise, this is great.
webdriver-spec.html
Outdated
along with the HTTP method and <a>extension command</a>, | ||
is added to the <a>table of endpoints</a> | ||
and thus follows the same rules for <a>request routing</a> | ||
as that of other built-in <a>commands</a>. | ||
|
||
<p>In order to avoid potential resource conflicts with other implementations, | ||
vendor-specific <a>extension command URI Template</a>s SHOULD begin with one |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can add <dfn data-lt="extension command URI Templates">…</dfn>
for the plural form on :1805.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the convention is to prefer the singular for term definitions, so I've done the inverse:
<a data-lt="extension command URI Template">extension command URI Templates</a>
Does that seem okay to you?
webdriver-spec.html
Outdated
along with the HTTP method and <a>extension command</a>, | ||
is added to the <a>table of endpoints</a> | ||
and thus follows the same rules for <a>request routing</a> | ||
as that of other built-in <a>commands</a>. | ||
|
||
<p>In order to avoid potential resource conflicts with other implementations, | ||
vendor-specific <a>extension command URI Template</a>s SHOULD begin with one |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the vendor namespace was a requirement before and we shouldn’t loosen this by saying vendors should be using a vendor prefix.
I suggest s/SHOULD/must/ here or just rephrasing it to have a normative sound to it:
… vendor-specific extension command URI templates beginw tih on or more path segments …
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing!
webdriver-spec.html
Outdated
vendor-specific <a>extension command URI Template</a>s SHOULD begin with one | ||
or more path segments which uniquely identifies the vendor and UA. | ||
It is suggested that vendors use their vendor prefixes | ||
without additional characters as outlined in [[CSS21]], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I filed #1179 about defining what ‘additional characters’ mean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, very good
@jugglinmike Do you have time to address the nits? |
Thanks for the review, @andreastt! I've pushed up a commit to implement your suggestions--sorry for the delay. |
Thanks! It looks great! |
Agreed :) Thanks again for the help--I'll be following up with a patch for the Permissions specification soon |
The WebDriver specification was recently updated to better-support extension by web standards [1]. This change allows extension commands to be defined without a dedicated "name". Because the Automation section only defines a single command, the name "set" is superfluous. Remove it in order to expose a more idiomatic HTTP API. [1] w3c/webdriver#1177
The WebDriver specification was recently updated to better-support extension by web standards [1]. This change allows extension commands to be defined without a dedicated "name". Because the Automation section only defines a single command, the name "set" is superfluous. Remove it in order to expose a more idiomatic HTTP API. [1] w3c/webdriver#1177
The facilities offered for protocol extensions are intended to be used
by both web standards and browser vendors. Prior to the application of
this commit, the specification text inconsistently described these two
distinct use cases [1]. The mechanism itself was biased towards the
"browser vendor" use case because it required the specification of an
"extension command prefix" which is not appropriate for all "web
standards" use cases [2].
Explicitly describe both use cases in the informative text and
generalize the normative text to better-support extensions by web
standards.
[1] #1142
[2] #1170
This is an attempt to resolve gh-1142 and gh-1170 as recently discussed on IRC.
The Permissions specification was recently patched to define an extension command according to the specification text today. As described in gh-1170, this meant defining a resource of the form
session/{session id}/permissions/set
. If this patch is accepted, I plan to update the template tosession/{session id}/permissions
.Preview | Diff