[chromedriver] Added option to make element references W3C compliant.

[chromedriver] Added option to make element references W3C compliant.

When a chromedrvier session is created with a flag for W3C compliance,
chromedriver will reference the element with a UUID in place of the
existing ID generation method.

Chromedriver wrapping/unwrapping functions have been changed to match
the new referencing when the W3C flag is true.


BUG=

[roisinmcl, 2016-08-10 00:44:46]

Description was changed from

==========
[chromedriver] Added option to make element references W3C compliant.

When a chromedrvier session is created with a flag for W3C compliance,
chromedriver will reference the element with a UUID in place of the
existing ID generation method.

Chromedriver wrapping/unwrapping functions have been changed to match
the new referencing when the W3C flag is true.


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

to

==========
[chromedriver] Added option to make element references W3C compliant.

When a chromedrvier session is created with a flag for W3C compliance,
chromedriver will reference the element with a UUID in place of the
existing ID generation method.

Chromedriver wrapping/unwrapping functions have been changed to match
the new referencing when the W3C flag is true.


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

[roisinmcl, 2016-08-10 00:44:47]

roisinmcl@google.com changed reviewers:
+ prasadv@chromium.org, samuong@chromium.org

[samuong, 2016-08-10 17:14:31]

This patch set doesn't apply cleanly to the current HEAD. I think this is due to
conflicts with some of Eva's CLs which landed recently. Can you work with her to
make sure you're using the same w3c capabilities and flags?

[samuong, 2016-08-11 18:13:20]

I'm still trying to figure out whether Chrome's implementation of Math.random()
is suitable for generating UUIDs. I'll get back to you about this, but here are
the comments for everything else.

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

https://codereview.chromium.org/2230053002/diff/20001/chrome/test/chromedrive...
chrome/test/chromedriver/chrome/web_view_impl.cc:130: const bool w3c_compliant)
nit: move the |w3c_compliant| parameter up to the second parameter

https://codereview.chromium.org/2230053002/diff/20001/chrome/test/chromedrive...
File chrome/test/chromedriver/chrome/web_view_impl.h (right):

https://codereview.chromium.org/2230053002/diff/20001/chrome/test/chromedrive...
chrome/test/chromedriver/chrome/web_view_impl.h:112: //void
SetW3CCompliant(const bool w3c_compliant) override;
delete this line?

https://codereview.chromium.org/2230053002/diff/20001/chrome/test/chromedrive...
File chrome/test/chromedriver/client/chromedriver.py (right):

https://codereview.chromium.org/2230053002/diff/20001/chrome/test/chromedrive...
chrome/test/chromedriver/client/chromedriver.py:115: 
nit: no need for a new blank line

https://codereview.chromium.org/2230053002/diff/20001/chrome/test/chromedrive...
chrome/test/chromedriver/client/chromedriver.py:223: return {'ELEMENT':
value._id}
let's create constants for the element keys, since they get used in a few
different places

https://codereview.chromium.org/2230053002/diff/20001/chrome/test/chromedrive...
chrome/test/chromedriver/client/chromedriver.py:235:
value['element-6066-11e4-a52e-4f735466cecf'], basestring)):
this is a big conjunction, let's do the len(value)==1 check first, and then do
the w3c/non-w3c inside that:

if len(value) == 1:
  if self.w3c_compliant and ...
    # ...
  elif not self.w3c_compliant and ...
    # ...

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

https://codereview.chromium.org/2230053002/diff/20001/chrome/test/chromedrive...
chrome/test/chromedriver/commands.cc:257:
session->chrome->GetWebViewIds(&web_view_ids, session->w3c_compliant);
nit: this line should be indented 4 spaces. break it after the ( instead so that
it fits

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

https://codereview.chromium.org/2230053002/diff/20001/chrome/test/chromedrive...
chrome/test/chromedriver/element_util.cc:29: Session* session =
GetThreadLocalSession();
put a CHECK(session) here, just in case

https://codereview.chromium.org/2230053002/diff/20001/chrome/test/chromedrive...
File chrome/test/chromedriver/js/call_function_test.html (right):

https://codereview.chromium.org/2230053002/diff/20001/chrome/test/chromedrive...
chrome/test/chromedriver/js/call_function_test.html:89: */
why is this commented out?

https://codereview.chromium.org/2230053002/diff/20001/chrome/test/chromedrive...
chrome/test/chromedriver/js/call_function_test.html:187: /*
if these tests are broken, can you file a bug at
https://bugs.chromium.org/p/chromedriver/issues/entry, and put a comment with a
link to the bug?

https://codereview.chromium.org/2230053002/diff/20001/chrome/test/chromedrive...
File chrome/test/chromedriver/session.h (right):

https://codereview.chromium.org/2230053002/diff/20001/chrome/test/chromedrive...
chrome/test/chromedriver/session.h:19: #include
"chrome/test/chromedriver/chrome/web_view_impl.h"
is this necessary?

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

https://codereview.chromium.org/2230053002/diff/20001/chrome/test/chromedrive...
chrome/test/chromedriver/test/run_py_tests.py:1294: print
self._driver.FindElements('tag name', 'br')
we should have a test that explicitly tests the w3c element references

[samuong, 2016-08-11 20:45:14]

https://codereview.chromium.org/2230053002/diff/20001/chrome/test/chromedrive...
File chrome/test/chromedriver/js/call_function.js (right):

https://codereview.chromium.org/2230053002/diff/20001/chrome/test/chromedrive...
chrome/test/chromedriver/js/call_function.js:35: var w3cEnabled = false;
if you're going to have this here, is there a need to add an argument to
getPageCache()?

although i think it might be slightly better to just pass it to wrap as an
argument...

https://codereview.chromium.org/2230053002/diff/20001/chrome/test/chromedrive...
chrome/test/chromedriver/js/call_function.js:55: UUID += "01";
If I'm reading https://tools.ietf.org/html/rfc4122 correctly, version 1 UUIDs
are generated using the MAC address and a timer. Since you're generating these
from random numbers, you should be setting the version (the 13th hex digit) to
4.

For the 17th hex digit, the most significant two bits should be 10, which means
that the hex representation for this octet should be one of [89ab].

Other octets should be generated randomly, not set to 00 or 01.

https://codereview.chromium.org/2230053002/diff/20001/chrome/test/chromedrive...
chrome/test/chromedriver/js/call_function.js:57: var rand =
Math.round(Math.random() * 225).toString(16);
Let's use window.crypto.getRandomValues() instead of Math.random():

https://developer.mozilla.org/en-US/docs/Web/API/RandomSource/getRandomValues

Also, a UUID should always have 32 hex digits, but I don't always get the same
length each time I run this function:

generateUUID().replace(/-/g, '').length
32
generateUUID().replace(/-/g, '').length
31
generateUUID().replace(/-/g, '').length
32
generateUUID().replace(/-/g, '').length
32
generateUUID().replace(/-/g, '').length
31
generateUUID().replace(/-/g, '').length
30

This is because the toString() function will not always left-pad with 0s.

Please add a test that generates many UUIDs and checks that they are all valid.
It should look at the length of the UUID (including the length of each
individual field) and check that all the characters are hex (0-9a-f) except for
the hyphens, which should be in the right place.

https://codereview.chromium.org/2230053002/diff/20001/chrome/test/chromedrive...
chrome/test/chromedriver/js/call_function.js:227: * Returns the global object
cache for the page.
don't forget to add a comment here for the w3c arg

https://codereview.chromium.org/2230053002/diff/20001/chrome/test/chromedrive...
chrome/test/chromedriver/js/call_function.js:234: var w3c = opt_w3c || false;
is there ever a case where we don't pass a value for opt_w3c?
maybe we should make it non-optional? (and move it to the first arg)

https://codereview.chromium.org/2230053002/diff/20001/chrome/test/chromedrive...
chrome/test/chromedriver/js/call_function.js:244: }
if you make the "if (w3c)" check the inner condition, then you won't need to
duplicate "if (!(key in doc))" and "return doc[key];"

[roisinmcl, 2016-08-13 01:47:26]

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

https://codereview.chromium.org/2230053002/diff/20001/chrome/test/chromedrive...
chrome/test/chromedriver/chrome/web_view_impl.cc:130: const bool w3c_compliant)
On 2016/08/11 18:13:20, samuong wrote:
> nit: move the |w3c_compliant| parameter up to the second parameter

Done.

https://codereview.chromium.org/2230053002/diff/20001/chrome/test/chromedrive...
File chrome/test/chromedriver/chrome/web_view_impl.h (right):

https://codereview.chromium.org/2230053002/diff/20001/chrome/test/chromedrive...
chrome/test/chromedriver/chrome/web_view_impl.h:112: //void
SetW3CCompliant(const bool w3c_compliant) override;
On 2016/08/11 18:13:20, samuong wrote:
> delete this line?

Done.

https://codereview.chromium.org/2230053002/diff/20001/chrome/test/chromedrive...
File chrome/test/chromedriver/client/chromedriver.py (right):

https://codereview.chromium.org/2230053002/diff/20001/chrome/test/chromedrive...
chrome/test/chromedriver/client/chromedriver.py:115: 
On 2016/08/11 18:13:20, samuong wrote:
> nit: no need for a new blank line

Done.

https://codereview.chromium.org/2230053002/diff/20001/chrome/test/chromedrive...
chrome/test/chromedriver/client/chromedriver.py:223: return {'ELEMENT':
value._id}
On 2016/08/11 18:13:20, samuong wrote:
> let's create constants for the element keys, since they get used in a few
> different places

Done.

https://codereview.chromium.org/2230053002/diff/20001/chrome/test/chromedrive...
chrome/test/chromedriver/client/chromedriver.py:235:
value['element-6066-11e4-a52e-4f735466cecf'], basestring)):
On 2016/08/11 18:13:20, samuong wrote:
> this is a big conjunction, let's do the len(value)==1 check first, and then do
> the w3c/non-w3c inside that:
> 
> if len(value) == 1:
>   if self.w3c_compliant and ...
>     # ...
>   elif not self.w3c_compliant and ...
>     # ...

Done.

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

https://codereview.chromium.org/2230053002/diff/20001/chrome/test/chromedrive...
chrome/test/chromedriver/commands.cc:257:
session->chrome->GetWebViewIds(&web_view_ids, session->w3c_compliant);
On 2016/08/11 18:13:20, samuong wrote:
> nit: this line should be indented 4 spaces. break it after the ( instead so
that
> it fits

Done.

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

https://codereview.chromium.org/2230053002/diff/20001/chrome/test/chromedrive...
chrome/test/chromedriver/element_util.cc:29: Session* session =
GetThreadLocalSession();
On 2016/08/11 18:13:20, samuong wrote:
> put a CHECK(session) here, just in case

Done.

https://codereview.chromium.org/2230053002/diff/20001/chrome/test/chromedrive...
File chrome/test/chromedriver/js/call_function.js (right):

https://codereview.chromium.org/2230053002/diff/20001/chrome/test/chromedrive...
chrome/test/chromedriver/js/call_function.js:234: var w3c = opt_w3c || false;
On 2016/08/11 20:45:14, samuong wrote:
> is there ever a case where we don't pass a value for opt_w3c?
> maybe we should make it non-optional? (and move it to the first arg)

Done.

https://codereview.chromium.org/2230053002/diff/20001/chrome/test/chromedrive...
chrome/test/chromedriver/js/call_function.js:244: }
On 2016/08/11 20:45:14, samuong wrote:
> if you make the "if (w3c)" check the inner condition, then you won't need to
> duplicate "if (!(key in doc))" and "return doc[key];"

Done.

https://codereview.chromium.org/2230053002/diff/20001/chrome/test/chromedrive...
File chrome/test/chromedriver/js/call_function_test.html (right):

https://codereview.chromium.org/2230053002/diff/20001/chrome/test/chromedrive...
chrome/test/chromedriver/js/call_function_test.html:89: */
On 2016/08/11 18:13:20, samuong wrote:
> why is this commented out?

Forgot to uncomment this after I finished testing. Fixed it.

https://codereview.chromium.org/2230053002/diff/20001/chrome/test/chromedrive...
chrome/test/chromedriver/js/call_function_test.html:187: /*
On 2016/08/11 18:13:20, samuong wrote:
> if these tests are broken, can you file a bug at
> https://bugs.chromium.org/p/chromedriver/issues/entry, and put a comment with
a
> link to the bug?

https://bugs.chromium.org/p/chromedriver/issues/detail?id=1461

https://codereview.chromium.org/2230053002/diff/20001/chrome/test/chromedrive...
File chrome/test/chromedriver/session.h (right):

https://codereview.chromium.org/2230053002/diff/20001/chrome/test/chromedrive...
chrome/test/chromedriver/session.h:19: #include
"chrome/test/chromedriver/chrome/web_view_impl.h"
On 2016/08/11 18:13:20, samuong wrote:
> is this necessary?

This is unnecessary, I forgot to remove this.

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

https://codereview.chromium.org/2230053002/diff/20001/chrome/test/chromedrive...
chrome/test/chromedriver/test/run_py_tests.py:1294: print
self._driver.FindElements('tag name', 'br')
On 2016/08/11 18:13:20, samuong wrote:
> we should have a test that explicitly tests the w3c element references

I added a statement in this test to check the length of the W3C element
reference to check that it's using the new ID type.

[samuong, 2016-08-13 01:50:34]

lgtm

[roisinmcl, 2016-08-13 02:35:52]

The CQ bit was checked by roisinmcl@google.com

[roisinmcl, 2016-08-13 02:35:52]

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/2230053002/#ps60001
(title: "Fixed presubmit errors.")

[commit-bot: I haz the power, 2016-08-13 02:36: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, 2016-08-13 03:06:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-13 03:06:38]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)