Capabilities strategies#755
Conversation
|
There's a further step where we hook this up into the |
3b77773 to
f0baef3
Compare
|
OK, done a first pass, but I don't think I got everything. I'll need to re-check the logic later. Reviewed 1 of 1 files at r2. webdriver-spec.html, line 1979 at r2 (raw file):
spelling webdriver-spec.html, line 1980 at r2 (raw file):
Not necessarily an issue for here, but I think that this construct is confusing. The fact that you have to infer semantics from the variable names is bad. You should probably iterate over the keys corresponding to enumerable own properties and get the value inside the loop. webdriver-spec.html, line 2062 at r2 (raw file):
You could make webdriver-spec.html, line 2068 at r2 (raw file):
I think we shouldn't call this webdriver-spec.html, line 2083 at r2 (raw file):
You still can't spell webdriver-spec.html, line 2090 at r2 (raw file):
Huh? So we just filter out all the ones that don't validate? That seems wrong. Surely we want to return an error? webdriver-spec.html, line 2122 at r2 (raw file):
webdriver-spec.html, line 2131 at r2 (raw file):
You can't use webdriver-spec.html, line 2172 at r2 (raw file):
This still doesn't make sense, because you can't work out what these things are until you have processed the passed in capabilities (at least in a real implementation). webdriver-spec.html, line 2191 at r2 (raw file):
This is also confusing. Real implementations mostly use parameters passed to new session as configuration options. It shouldn't be required to return every configuration option that was passed in (e.g. profile because of efficiency concerns). webdriver-spec.html, line 2204 at r2 (raw file):
I feel like this deserialization strategy stuff is mostly useless indirection. We could simply have a table of allowed names and types here. We also need to deal with the possibility of extension properties which should have a name containing a colon. webdriver-spec.html, line 2285 at r2 (raw file):
I don't think this is implementable. For starters, does not support" isn't well defined. But even if it was it's generally impossible to tell if a proxy configuration is supported without actually trying it (with PAC files it is arguably equivalent to the halting problem), which you can't do until you actually start the session. webdriver-spec.html, line 2290 at r2 (raw file):
What does "recognised" mean? Can't we constrain nonstandard options to have a webdriver-spec.html, line 2306 at r2 (raw file):
This seems like a layer of indirection that isn't necessary. Why not just call the appropriate deserialization steps directly rather than having this intermediate table? webdriver-spec.html, line 2321 at r2 (raw file):
I don't think that this kind of simple "deserializer" is that helpful. Elsewhere we just inlined the logic. webdriver-spec.html, line 2334 at r2 (raw file):
This is only for webdriver-spec.html, line 2358 at r2 (raw file):
Is this used? It seems pretty useless anyway and hopefully goes away with a refactor. webdriver-spec.html, line 2822 at r2 (raw file):
No need to describe these as webdriver-spec.html, line 2842 at r2 (raw file):
This algorithm doesn't return anything; it has to return webdriver-spec.html, line 2924 at r2 (raw file):
Quotes inside webdriver-spec.html, line 2933 at r2 (raw file):
You have to return webdriver-spec.html, line 8198 at r2 (raw file):
"When required to", or similar. webdriver-spec.html, line 8209 at r2 (raw file):
Comments from Reviewable |
|
Review status: all files reviewed at latest revision, 23 unresolved discussions. webdriver-spec.html, line 1980 at r2 (raw file): Previously, jgraham wrote…
I followed the same convention here that we have in other parts of the spec to do with iterating over webdriver-spec.html, line 2062 at r2 (raw file): Previously, jgraham wrote…
Good point :) webdriver-spec.html, line 2068 at r2 (raw file): Previously, jgraham wrote…
SGTM. Will change. webdriver-spec.html, line 2083 at r2 (raw file): Previously, jgraham wrote…
It's weekness…. webdriver-spec.html, line 2090 at r2 (raw file): Previously, jgraham wrote…
There are some capabilities that are valid but non-applicable (eg. "browserName -> chromedriver" being sent to geckodriver. webdriver-spec.html, line 2131 at r2 (raw file): Previously, jgraham wrote…
I didn't realise that. Will reword things. webdriver-spec.html, line 2191 at r2 (raw file): Previously, jgraham wrote…
I wanted to put a reasonable place for implementations to add their own information. Otherwise, we never return anything other than the capabilities that people ask for. In the extreme case, this may mean people can't even tell the "browserName" of the remote end, which is less than ideal. Perhaps "optionally" would help? webdriver-spec.html, line 2204 at r2 (raw file): Previously, jgraham wrote…
As a developer, this made sense to me --- basically a lookup in a map with a default return value. I could do the table thing too, but the nice thing here is we avoid duplicating information that we've already made normative in the spec. webdriver-spec.html, line 2285 at r2 (raw file): Previously, jgraham wrote…
This is the language that was there before. I just left it alone. Things like safaridriver can probably tell up front that they're not allowed to change system proxy settings. webdriver-spec.html, line 2290 at r2 (raw file): Previously, jgraham wrote…
Not unless we tighten the language in webdriver-spec.html, line 2306 at r2 (raw file): Previously, jgraham wrote…
Because there are multiple steps that follow the same deserialisation strategy (browserName and browserVersion, for example), and I hate copy-and-pasting. webdriver-spec.html, line 2321 at r2 (raw file): Previously, jgraham wrote…
Boolean has the quirk that webdriver-spec.html, line 2334 at r2 (raw file): Previously, jgraham wrote…
I like that idea. Will amend. webdriver-spec.html, line 2358 at r2 (raw file): Previously, jgraham wrote…
Identity does get used. It's for intermediary nodes so that they pass capabilities through without changing them. webdriver-spec.html, line 2924 at r2 (raw file): Previously, jgraham wrote…
That's not how @andreastt likes things to be done, and I've adopted his preferred style. I'm happy to go either way. Comments from Reviewable |
|
I need to make the amendments, but there are some questions and comments that I hope make some bits clearer. Follow up diffs tomorrow afternoon. Review status: all files reviewed at latest revision, 23 unresolved discussions. Comments from Reviewable |
|
Review status: all files reviewed at latest revision, 23 unresolved discussions, some commit checks failed. webdriver-spec.html, line 1979 at r2 (raw file): Previously, jgraham wrote…
Fixed webdriver-spec.html, line 2172 at r2 (raw file): Previously, jgraham wrote…
We're building up the set of capabilities that have been matched. We'll either return them or webdriver-spec.html, line 2822 at r2 (raw file): Previously, jgraham wrote…
Fixed. webdriver-spec.html, line 2842 at r2 (raw file): Previously, jgraham wrote…
Fixed. webdriver-spec.html, line 2933 at r2 (raw file): Previously, jgraham wrote…
Fixed webdriver-spec.html, line 8198 at r2 (raw file): Previously, jgraham wrote…
Done webdriver-spec.html, line 8209 at r2 (raw file): Previously, jgraham wrote…
Done Comments from Reviewable |
f0baef3 to
48e327c
Compare
|
Review status: 0 of 1 files reviewed at latest revision, 23 unresolved discussions. webdriver-spec.html, line 2122 at r2 (raw file): Previously, jgraham wrote…
Fixed Comments from Reviewable |
|
Review status: 0 of 1 files reviewed at latest revision, 16 unresolved discussions. webdriver-spec.html, line 1980 at r2 (raw file): Previously, shs96c (Simon Stewart) wrote…
webdriver-spec.html, line 2285 at r2 (raw file): Previously, shs96c (Simon Stewart) wrote…
I still think this language needs to be weakened. It seems like you're saying there are exceptional circumstances in which an implementation can know things about the allowed proxy configuration upfront. I agree in that case it should be permitted to error here, but there shouldn't be a must-level requirement to determine that. webdriver-spec.html, line 2290 at r2 (raw file): Previously, shs96c (Simon Stewart) wrote…
I think we should have this discussion. People making up random stuff in a global namespace limits our ability to standardise going forward (we have to know about all existing extensions to avoid name clashes). webdriver-spec.html, line 2924 at r2 (raw file): Previously, shs96c (Simon Stewart) wrote…
I… don't see what the argument for the alternative is. The code here is a string literal. Surely then the markers of it being such form part of the webdriver-spec.html, line 2113 at r3 (raw file):
The "s" should be inside the webdriver-spec.html, line 2154 at r3 (raw file):
"witha" webdriver-spec.html, line 2185 at r3 (raw file):
webdriver-spec.html, line 2209 at r3 (raw file):
Set a property on result with name[…] webdriver-spec.html, line 2227 at r3 (raw file):
Is there a use case for allowing this, rather than just rejecting cases where there's a key in common? Note that "equal" is not obviously well defined here (does the empty string equal null? Do two Objects with equal keys and values equal each other?). webdriver-spec.html, line 2254 at r3 (raw file):
I still think we need at least a note to indicate that in a real implementation these values may depend on other capabilities that are passed in. Designing that is the central problem in actually implementing this stuff so it is helpful if the spec acknowledges that its model is simplified. webdriver-spec.html, line 2343 at r3 (raw file):
Why are we returning an error in this case rather than null? You might have a second set of capabilities that don't require accepting insecure SSL. webdriver-spec.html, line 2361 at r3 (raw file):
What does "create a new entry" mean? Comments from Reviewable |
|
Reviewed 1 of 1 files at r3. Comments from Reviewable |
|
Review status: all files reviewed at latest revision, 16 unresolved discussions. webdriver-spec.html, line 2285 at r2 (raw file): Previously, jgraham wrote…
I think there does need to be a check once somewhere in the end node that ensures that both "acceptInsecureCerts" and the "proxy" capabilities have been set and will be honoured. webdriver-spec.html, line 2290 at r2 (raw file): Previously, jgraham wrote…
Realistically, people are already extending the spec with non-standard capability names that don't contain a colon, and there's nothing to say that there can't be consensus which has yet to be formalised around a capability name (the Appium, Selendroid, WebDriver Agent folks have some agreed upon capability names that are shared across multiple implementations). I'll use "implementation-specific" as elsewhere for this text. webdriver-spec.html, line 2924 at r2 (raw file): Previously, jgraham wrote…
@andreastt's argument is that there might be confusion about whether or not the quote marks should be part of the string or not. In his view, the quote marks indicate that the type is a string, and value within the
|
88ba7d4 to
126794b
Compare
|
Review status: 0 of 1 files reviewed at latest revision, 10 unresolved discussions, some commit checks failed. webdriver-spec.html, line 2172 at r2 (raw file): Previously, shs96c (Simon Stewart) wrote…
I think we agreed to add a note here? webdriver-spec.html, line 2285 at r2 (raw file): Previously, shs96c (Simon Stewart) wrote…
We can probably tell if that configuration is set. We can't tell if it's supported in the sense of "actually works". The language here is still wrong. I at least want an issue open to fix this. webdriver-spec.html, line 2290 at r2 (raw file): Previously, shs96c (Simon Stewart) wrote…
So we can't ever add a capability without auditing every extant implementation? That seems like a disaster. webdriver-spec.html, line 2924 at r2 (raw file): Previously, shs96c (Simon Stewart) wrote…
|
126794b to
3550e55
Compare
|
Reviewed 1 of 1 files at r5. Comments from Reviewable |
|
Review status: all files reviewed at latest revision, 9 unresolved discussions. webdriver-spec.html, line 2172 at r2 (raw file): Previously, jgraham wrote…
We did, and it's further down. :) webdriver-spec.html, line 2285 at r2 (raw file): Previously, jgraham wrote…
Discussed on IRC. I've raised #786 to address this. webdriver-spec.html, line 2290 at r2 (raw file): Previously, jgraham wrote…
We followed the CSS spec for protocol extensions, and they don't require extensions to use the vendor prefix --- https://www.w3.org/TR/CSS21/syndata.html#vendor-keywords If we're doomed, at least it'll be a party with plenty of company. I can add a note in the protocol extensions section to say that no spec-supplied capability name will ever have a colon in it. webdriver-spec.html, line 2227 at r3 (raw file): Previously, jgraham wrote…
I can see the argument for a lack of interoperability. I guess if the validation step only checked that deserialisation was possible we'd still have the raw JSON blob at this point. I'd be amazed if most libraries for JSON in any language didn't have some mechanism for verifying that two blobs are equivalent. I'll remove the equality check, but I think this will cause problems for users further down the line since local end bindings are written by folks not used to reading specs. webdriver-spec.html, line 2254 at r3 (raw file): Previously, jgraham wrote…
sigh Marketing. webdriver-spec.html, line 2210 at r4 (raw file): Previously, jgraham wrote…
Done webdriver-spec.html, line 2300 at r4 (raw file): Previously, jgraham wrote…
I'll move this note up. Comments from Reviewable |
72909ca to
222a526
Compare
This provides stronger language about how to convert a given capability to something that can be used in the rest of the spec.
222a526 to
071c3ec
Compare
|
Review status: 0 of 1 files reviewed at latest revision, 9 unresolved discussions. webdriver-spec.html, line 2290 at r2 (raw file): Previously, shs96c (Simon Stewart) wrote…
I've now linked all of these to extension capabilities. The definition of that says that capability keys must have a ":". I think that properly addresses your concern. Comments from Reviewable |
This will mean that remote ends will fail in more predictable ways.
071c3ec to
2562064
Compare
|
Reviewed 1 of 1 files at r6. webdriver-spec.html, line 2227 at r3 (raw file): Previously, shs96c (Simon Stewart) wrote…
Well it seems easier to allow this in the future if there's a problem than the reverse. Comments from Reviewable |
Closes #636 amongst others.
This change is