Update ChromeDriver to accept W3C format InitSession

Update ChromeDriver to accept W3C format InitSession

W3C spec on WebDriver uses alwaysMatch and firstMatch under capabilities
to specify required and desired capabilities. (See
https://w3c.github.io/webdriver/webdriver-spec.html#processing-capabilities
for details.) This CL is the first step towards making ChromeDriver
compliant to this spec. It adds support for capabilities.alwaysMatch, and
removes the existing experimental chromeOptions.w3c flag. Support for
capabilities.firstMatch will be added in the near future.

BUG=chromedriver:1740
Review-Url: https://codereview.chromium.org/2776313003
Cr-Commit-Position: refs/heads/master@{#460451}
Committed: https://chromium.googlesource.com/chromium/src/+/1c59cb32b65444780200c5147fe3d53d4b03ac37

[johnchen, 2017-03-28 21:12:48]

johnchen@chromium.org changed reviewers:
+ samuong@chromium.org

[samuong, 2017-03-28 21:30:00]

Could you please file a tracking bug at
https://bugs.chromium.org/p/chromedriver/issues/entry, and change the BUG= line
in the description to BUG=chromedriver:X, where X is the issue number?

https://codereview.chromium.org/2776313003/diff/1/chrome/test/chromedriver/se...
File chrome/test/chromedriver/session_commands.cc (right):

https://codereview.chromium.org/2776313003/diff/1/chrome/test/chromedriver/se...
chrome/test/chromedriver/session_commands.cc:179: const base::DictionaryValue
default_desired_caps;
What is the difference between |desired_caps| and |default_desired_caps|?

https://codereview.chromium.org/2776313003/diff/1/chrome/test/chromedriver/se...
chrome/test/chromedriver/session_commands.cc:181:
params.GetDictionary("capabilities.desiredCapabilities", &desired_caps)) {
If we're going to continue supporting capabilties.desiredCapabilities, then we
should treat these clients as W3C-compliant, not as legacy clients.

https://codereview.chromium.org/2776313003/diff/1/chrome/test/chromedriver/se...
chrome/test/chromedriver/session_commands.cc:189: "cannot find dict
'capabilities' or 'desiredCapabilities'");
This error message needs to be updated

[johnchen, 2017-03-28 22:30:07]

Description was changed from

==========
Update ChromeDriver to accept W3C format InitSession

W3C spec on WebDriver uses alwaysMatch and firstMatch under capabilities
to specify required and desired capabilities. (See
https://w3c.github.io/webdriver/webdriver-spec.html#processing-capabilities
for details.) This CL is the first step towards making ChromeDriver
compliant to this spec. It adds support for capabilities.alwaysMatch, and
removes the existing experimental chromeOptions.w3c flag. Support for
capabilities.firstMatch will be added in the near future.

BUG=
==========

to

==========
Update ChromeDriver to accept W3C format InitSession

W3C spec on WebDriver uses alwaysMatch and firstMatch under capabilities
to specify required and desired capabilities. (See
https://w3c.github.io/webdriver/webdriver-spec.html#processing-capabilities
for details.) This CL is the first step towards making ChromeDriver
compliant to this spec. It adds support for capabilities.alwaysMatch, and
removes the existing experimental chromeOptions.w3c flag. Support for
capabilities.firstMatch will be added in the near future.

BUG=chromedriver:1740
==========

[johnchen, 2017-03-28 22:35:41]

On 2017/03/28 21:30:00, samuong wrote:
> Could you please file a tracking bug at
> https://bugs.chromium.org/p/chromedriver/issues/entry, and change the BUG=
line
> in the description to BUG=chromedriver:X, where X is the issue number?

Done. BUG=chromedriver:1740

>
https://codereview.chromium.org/2776313003/diff/1/chrome/test/chromedriver/se...
> File chrome/test/chromedriver/session_commands.cc (right):
> 
>
https://codereview.chromium.org/2776313003/diff/1/chrome/test/chromedriver/se...
> chrome/test/chromedriver/session_commands.cc:179: const base::DictionaryValue
> default_desired_caps;
> What is the difference between |desired_caps| and |default_desired_caps|?

It was added to handle a corner case, when a client sends neither alwaysMatch
nor firstMatch. My understanding of W3C spec is both alwaysMatch and firstMatch
are optional, and it's allowed to send a request with an empty capabilities
node. In this case |default_desired_caps| serves as an empty |desired_caps|.

Now that we're restoring the requirement for chromeOptions.w3c, the capabilities
node can no longer be empty, so |default_desired_caps| isn't necessary. I
removed it.
 
>
https://codereview.chromium.org/2776313003/diff/1/chrome/test/chromedriver/se...
> chrome/test/chromedriver/session_commands.cc:181:
> params.GetDictionary("capabilities.desiredCapabilities", &desired_caps)) {
> If we're going to continue supporting capabilties.desiredCapabilities, then we
> should treat these clients as W3C-compliant, not as legacy clients.

W3C-compliant will still be determined by chromeOptions.w3c flag.

>
https://codereview.chromium.org/2776313003/diff/1/chrome/test/chromedriver/se...
> chrome/test/chromedriver/session_commands.cc:189: "cannot find dict
> 'capabilities' or 'desiredCapabilities'");
> This error message needs to be updated

Done

[samuong, 2017-03-28 23:02:48]

https://codereview.chromium.org/2776313003/diff/20001/chrome/test/chromedrive...
File chrome/test/chromedriver/session_commands.cc (right):

https://codereview.chromium.org/2776313003/diff/20001/chrome/test/chromedrive...
chrome/test/chromedriver/session_commands.cc:184: // TBD: Handle
capabilities.firstMatch.
s/TBD/TODO(johnchen)/

https://codereview.chromium.org/2776313003/diff/20001/chrome/test/chromedrive...
chrome/test/chromedriver/session_commands.cc:188:
!params.GetDictionary("capabilities.desiredCapabilities", &desired_caps))
I think these last two checks are redundant, since we already checked for
alwaysMatch and desiredCapabilities in the first if?

[johnchen, 2017-03-28 23:11:57]

On 2017/03/28 23:02:48, samuong wrote:
>
https://codereview.chromium.org/2776313003/diff/20001/chrome/test/chromedrive...
> File chrome/test/chromedriver/session_commands.cc (right):
> 
>
https://codereview.chromium.org/2776313003/diff/20001/chrome/test/chromedrive...
> chrome/test/chromedriver/session_commands.cc:184: // TBD: Handle
> capabilities.firstMatch.
> s/TBD/TODO(johnchen)/

Fixed

>
https://codereview.chromium.org/2776313003/diff/20001/chrome/test/chromedrive...
> chrome/test/chromedriver/session_commands.cc:188:
> !params.GetDictionary("capabilities.desiredCapabilities", &desired_caps))
> I think these last two checks are redundant, since we already checked for
> alwaysMatch and desiredCapabilities in the first if?

We would reach here if the client sends capabilities.desiredCapabilities or
capabilities.alwaysMatch without setting the chromeOptions.w3c flag.

[johnchen, 2017-03-29 00:32:21]

Per offline discussion with samuong, we decided to disallow using
capabilities.alwaysMatch or capabilities.desiredCapabilities without setting
chromeOptions.w3c flag to true.

[samuong, 2017-03-29 17:07:48]

lgtm with nits

https://codereview.chromium.org/2776313003/diff/60001/chrome/test/chromedrive...
File chrome/test/chromedriver/session_commands.cc (right):

https://codereview.chromium.org/2776313003/diff/60001/chrome/test/chromedrive...
chrome/test/chromedriver/session_commands.cc:183: // TODO(johnchen): Handle
capabilities.firstMatch.
When you go and implement this TODO, you'll need to modify |desired_caps|, which
is currently const. But you can change this in a future CL.

https://codereview.chromium.org/2776313003/diff/60001/chrome/test/chromedrive...
chrome/test/chromedriver/session_commands.cc:190: session->w3c_compliant = true;
nit: style guide says to use {}s when there are more than two lines, so use them
everywhere in this if-else-if-else-if block

https://codereview.chromium.org/2776313003/diff/60001/chrome/test/chromedrive...
chrome/test/chromedriver/session_commands.cc:192: return Status(kUnknownError,
can you change this to kSessionNotCreatedException?

https://codereview.chromium.org/2776313003/diff/60001/chrome/test/chromedrive...
chrome/test/chromedriver/session_commands.cc:193: "cannot find dict
'alwaysMatch' or 'desiredCapabilities'");
nit: update error message

https://codereview.chromium.org/2776313003/diff/60001/chrome/test/chromedrive...
File chrome/test/chromedriver/test/run_py_tests.py (right):

https://codereview.chromium.org/2776313003/diff/60001/chrome/test/chromedrive...
chrome/test/chromedriver/test/run_py_tests.py:2128: # It's an error to send w3c
format request without w3c capability flag.
nit: w3c -> W3C

[johnchen, 2017-03-29 17:42:12]

Made suggested changes in Patch Set 5.

[johnchen, 2017-03-29 17:42:21]

The CQ bit was checked by johnchen@chromium.org

[johnchen, 2017-03-29 17:42:22]

The patchset sent to the CQ was uploaded after l-g-t-m from samuong@chromium.org
Link to the patchset: https://codereview.chromium.org/2776313003/#ps80001
(title: "Update exception type, code formatting, and comments")

[commit-bot: I haz the power, 2017-03-29 17:43:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 17:58:13]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490809341593610,
"parent_rev": "fbb14599c4d794c4fc3ab13e243ef7c7c0416fa7", "commit_rev":
"1c59cb32b65444780200c5147fe3d53d4b03ac37"}

[commit-bot: I haz the power, 2017-03-29 17:59:08]

Description was changed from

==========
Update ChromeDriver to accept W3C format InitSession

W3C spec on WebDriver uses alwaysMatch and firstMatch under capabilities
to specify required and desired capabilities. (See
https://w3c.github.io/webdriver/webdriver-spec.html#processing-capabilities
for details.) This CL is the first step towards making ChromeDriver
compliant to this spec. It adds support for capabilities.alwaysMatch, and
removes the existing experimental chromeOptions.w3c flag. Support for
capabilities.firstMatch will be added in the near future.

BUG=chromedriver:1740
==========

to

==========
Update ChromeDriver to accept W3C format InitSession

W3C spec on WebDriver uses alwaysMatch and firstMatch under capabilities
to specify required and desired capabilities. (See
https://w3c.github.io/webdriver/webdriver-spec.html#processing-capabilities
for details.) This CL is the first step towards making ChromeDriver
compliant to this spec. It adds support for capabilities.alwaysMatch, and
removes the existing experimental chromeOptions.w3c flag. Support for
capabilities.firstMatch will be added in the near future.

BUG=chromedriver:1740

Review-Url: https://codereview.chromium.org/2776313003
Cr-Commit-Position: refs/heads/master@{#460451}
Committed:
https://chromium.googlesource.com/chromium/src/+/1c59cb32b65444780200c5147fe3...
==========

[commit-bot: I haz the power, 2017-03-29 17:59:09]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/1c59cb32b65444780200c5147fe3...