Skip to content

Conversation

@AutomatedTester
Copy link
Contributor

Review on Reviewable

04_sessions.html Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

process capabilities is documented in #32

Copy link
Member

Choose a reason for hiding this comment

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

  1. Missing full stop at end of sentence.
  2. I think “resultant capabilities” sounds a bit weird. Maybe “processed capabilities” or just “capabilities”?

@AutomatedTester
Copy link
Contributor Author

@jgraham @andreastt I have updated everything after your awesome comments

@jgraham
Copy link
Member

jgraham commented May 1, 2015

04_sessions.html, line 128 [r1] (raw file):
You can't have normative languages in notes.

You can probably just make step 1 implementation-defined behaviour like:

"If the node is an intermediate node, take implementation-defined steps that either result in returning an error with status code <a>session not created</a> or in returning a <a>success</a> with data that is isomorphic to that returned by <a>remote ends</a> according to (some other steps in this algorithm).

<p class=note>This allows intermediary nodes to use the capabilities data in any way they want e.g. to select a specific browser to test based on a combination of the required and desired capabilities. Typically the new session response from the remote end selected in this process will then be  relayed directly to the client</p>"

04_sessions.html, line 133 [r1] (raw file):
That doesn't sound right? If it's supposed to be a constraint that we can only have one session per remote end ever why is it a list of active sessions rather than just an active session that's initially null?


04_sessions.html, line 144 [r1] (raw file):
I think something more is probably needed to ensure that the session is started with the right internal state e.g. timeouts.



Comments from the review on Reviewable.io

@jgraham
Copy link
Member

jgraham commented May 1, 2015

Reviewed files:

  • 04_sessions.html @ r1
  • webdriver-spec.html @ r1

Comments from the review on Reviewable.io

@AutomatedTester
Copy link
Contributor Author

04_sessions.html, line 133 [r1] (raw file):
My thinking was related to intermediary nodes. Intermediary nodes can keep a list of session ids but a remote end probably shouldn't allow more than one session at a time. I think if I do something based on your previous comment that should be good enough


04_sessions.html, line 144 [r1] (raw file):
This is actually something I wanted to email the working group about. Some have suggested that capabilities be a set of immutable objects. I am of the opinion that it should be a way to set up the session and what is returned is what was set (either by the browser or by the user)



Comments from the review on Reviewable.io

@jgraham
Copy link
Member

jgraham commented May 1, 2015

04_sessions.html, line 144 [r1] (raw file):
Either way you need to actually initalise the session in the remote end, I think.



Comments from the review on Reviewable.io

@jgraham
Copy link
Member

jgraham commented May 1, 2015

04_sessions.html, line 133 [r1] (raw file):
https://w3c.github.io/webdriver/webdriver-spec.html#sessions already defined a maximum number of sessions. So basically you just check if the length of the list of sessions is equal to the maximum number of sessions and, if it is, error, otherwise create the session.



Comments from the review on Reviewable.io

@andreastt
Copy link
Member

04_sessions.html, line 116 [r1] (raw file):
"<code>capabilities</code>" for consistency with the rest of the document.


Comments from the review on Reviewable.io

@andreastt
Copy link
Member

04_sessions.html, line 121 [r1] (raw file):
I think we should rename process capabilities to processing capabilities so the prose flows better:

Let resultant capabilities be the result of processing capabilities capabilities.


04_sessions.html, line 128 [r1] (raw file):
This isn't resolved AFAICT? (I don't know how to un-resovle in Reviewable.)


04_sessions.html, line 128 [r1] (raw file):
s/Remote End/remote end/


04_sessions.html, line 144 [r1] (raw file):
Most of the properties associated with session have a default, so I think most of them should be fine left untouched, but we definitely need to say something along the lines of creating a new instance of session and setting its session id property.


04_sessions.html, line 147 [r1] (raw file):
s/./:/


04_sessions.html, line 152 [r1] (raw file):
s/Id/id/


Comments from the review on Reviewable.io

@AutomatedTester
Copy link
Contributor Author

landed in 6b0b815

@AutomatedTester AutomatedTester deleted the dburns/newSession branch March 7, 2017 09:11
whimboo pushed a commit to whimboo/webdriver that referenced this pull request Sep 20, 2017
Implement to_json for NewSessionParameters
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.

3 participants