Small test and ChromeDriver fixes to enable additional tests.

Small test and ChromeDriver fixes to enable additional webdriver python tests.
BUG=none
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=79484

[Joe, 2011-03-15 21:22:16]

http://codereview.chromium.org/6694007/diff/2001/chrome/test/webdriver/webdri...
File chrome/test/webdriver/webdriver_key_converter.cc (right):

http://codereview.chromium.org/6694007/diff/2001/chrome/test/webdriver/webdri...
chrome/test/webdriver/webdriver_key_converter.cc:127: if (key == '\n') {
While \n is a newline for unix isn't \r\n a newline for windows?  Will the tests
just assume unix coded strings?

http://codereview.chromium.org/6694007/diff/2001/chrome/test/webdriver/webdri...
chrome/test/webdriver/webdriver_key_converter.cc:232: LOG(ERROR) << "Unknown
WebDriver key: " << static_cast<int>(key);
If an unknown key is requested shouldn't it break with an error rather than
continue?

[kkania, 2011-03-18 21:45:12]

http://codereview.chromium.org/6694007/diff/2001/chrome/test/webdriver/webdri...
File chrome/test/webdriver/webdriver_key_converter.cc (right):

http://codereview.chromium.org/6694007/diff/2001/chrome/test/webdriver/webdri...
chrome/test/webdriver/webdriver_key_converter.cc:127: if (key == '\n') {
On 2011/03/15 21:22:17, Joe wrote:
> While \n is a newline for unix isn't \r\n a newline for windows?  Will the
tests
> just assume unix coded strings?

Yes, I support \r\n and \n. I do this by skipping \r and just doing newline on
\n.  Is that good enough?

http://codereview.chromium.org/6694007/diff/2001/chrome/test/webdriver/webdri...
chrome/test/webdriver/webdriver_key_converter.cc:232: LOG(ERROR) << "Unknown
WebDriver key: " << static_cast<int>(key);
On 2011/03/15 21:22:17, Joe wrote:
> If an unknown key is requested shouldn't it break with an error rather than
> continue?

Done.

[Joe, 2011-03-22 00:45:50]

LTGM

[kkania, 2011-03-23 00:53:27]

+dennis because with the new owners system I need a chromium committer to review

[dennis_jeffrey, 2011-03-23 16:57:02]

http://codereview.chromium.org/6694007/diff/13001/chrome/test/webdriver/run_w...
File chrome/test/webdriver/run_webdriver_tests.py (right):

http://codereview.chromium.org/6694007/diff/13001/chrome/test/webdriver/run_w...
chrome/test/webdriver/run_webdriver_tests.py:249: This is a hack for getting the
text_handling_tests to run. It is the only
Optional: you may want to describe why this hack enables the text_handling_tests
to run.

http://codereview.chromium.org/6694007/diff/13001/chrome/test/webdriver/run_w...
chrome/test/webdriver/run_webdriver_tests.py:251: into chromium yet.
This comment may be ambiguous: what is it that you're not sure you want to pull
into chromium yet, this file or the pytest module?

http://codereview.chromium.org/6694007/diff/13001/chrome/test/webdriver/sessi...
File chrome/test/webdriver/session.cc (right):

http://codereview.chromium.org/6694007/diff/13001/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.cc:177: ErrorCode code =
IsElementDisplayed(current_target_, element, &is_displayed);
It looks like we're assuming that the IsElementDisplayed function will fill in
either "true" or "false" for the "is_displayed" variable.  I think it would be
safer to initialize "is_displayed" to "false" at line 176 above (unless C++ will
initialize it to that default value for us?).

http://codereview.chromium.org/6694007/diff/13001/chrome/test/webdriver/webdr...
File chrome/test/webdriver/webdriver_key_converter.cc (right):

http://codereview.chromium.org/6694007/diff/13001/chrome/test/webdriver/webdr...
chrome/test/webdriver/webdriver_key_converter.cc:114: // Gets the key code
associated with |key|, if it is a special shorthand key,
Change the comma at the end of this line into a period.

http://codereview.chromium.org/6694007/diff/13001/chrome/test/webdriver/webdr...
chrome/test/webdriver/webdriver_key_converter.cc:117: // a shorthand key. If
true, |key_code| will be set and |should_skip| will be
"should_skip" --> "client_should_skip"

http://codereview.chromium.org/6694007/diff/13001/chrome/test/webdriver/webdr...
chrome/test/webdriver/webdriver_key_converter.cc:238: "Unknown WebDriver key(%d)
at string index (%" PRIuS ")",
This might be fine, but the examples I saw in the C++ style guide don't have the
spaces within " PRIus ".  Is it fine to have the spaces there?

[kkania, 2011-03-26 01:58:32]

http://codereview.chromium.org/6694007/diff/13001/chrome/test/webdriver/run_w...
File chrome/test/webdriver/run_webdriver_tests.py (right):

http://codereview.chromium.org/6694007/diff/13001/chrome/test/webdriver/run_w...
chrome/test/webdriver/run_webdriver_tests.py:249: This is a hack for getting the
text_handling_tests to run. It is the only
On 2011/03/23 16:57:02, dennis_jeffrey wrote:
> Optional: you may want to describe why this hack enables the
text_handling_tests
> to run.

Done.

http://codereview.chromium.org/6694007/diff/13001/chrome/test/webdriver/run_w...
chrome/test/webdriver/run_webdriver_tests.py:251: into chromium yet.
On 2011/03/23 16:57:02, dennis_jeffrey wrote:
> This comment may be ambiguous: what is it that you're not sure you want to
pull
> into chromium yet, this file or the pytest module?

Done.

http://codereview.chromium.org/6694007/diff/13001/chrome/test/webdriver/sessi...
File chrome/test/webdriver/session.cc (right):

http://codereview.chromium.org/6694007/diff/13001/chrome/test/webdriver/sessi...
chrome/test/webdriver/session.cc:177: ErrorCode code =
IsElementDisplayed(current_target_, element, &is_displayed);
On 2011/03/23 16:57:02, dennis_jeffrey wrote:
> It looks like we're assuming that the IsElementDisplayed function will fill in
> either "true" or "false" for the "is_displayed" variable.  I think it would be
> safer to initialize "is_displayed" to "false" at line 176 above (unless C++
will
> initialize it to that default value for us?).

Done.

http://codereview.chromium.org/6694007/diff/13001/chrome/test/webdriver/webdr...
File chrome/test/webdriver/webdriver_key_converter.cc (right):

http://codereview.chromium.org/6694007/diff/13001/chrome/test/webdriver/webdr...
chrome/test/webdriver/webdriver_key_converter.cc:114: // Gets the key code
associated with |key|, if it is a special shorthand key,
On 2011/03/23 16:57:02, dennis_jeffrey wrote:
> Change the comma at the end of this line into a period.

Done.

http://codereview.chromium.org/6694007/diff/13001/chrome/test/webdriver/webdr...
chrome/test/webdriver/webdriver_key_converter.cc:117: // a shorthand key. If
true, |key_code| will be set and |should_skip| will be
On 2011/03/23 16:57:02, dennis_jeffrey wrote:
> "should_skip" --> "client_should_skip"

Done.

http://codereview.chromium.org/6694007/diff/13001/chrome/test/webdriver/webdr...
chrome/test/webdriver/webdriver_key_converter.cc:238: "Unknown WebDriver key(%d)
at string index (%" PRIuS ")",
Yes, i noticed that the style guide didn't have spaces too. Either way is fine.
Most chromium code seems to use it this way, and I think it is more readable
too.

[dennis_jeffrey, 2011-03-26 02:05:33]

LGTM

Thanks!