[chromedriver] Accept git hashes in blink version strings from devtools

[chromedriver] Accept git hashes in blink version strings from devtools

BUG=chromedriver:906
Committed: https://crrev.com/1bfa9a7b7ba1233055cfb8449b9e7b9b3ce364c9
Cr-Commit-Position: refs/heads/master@{#297496}

[samuong, 2014-09-26 06:41:50]

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

[samuong, 2014-09-26 06:41:50]

ChromeDriver ToT is currently broken when running with Chrome OS ToT, because
Chrome OS now reports its Blink Version as a git hash rather than an svn
revision. With git hashes, we're not able to do any sort of range comparisons,
so we can't use this for backwards compatibility.

I've taken the opportunity to move the version parsing code out of
DevToolsHttpClient::Init() and into separate functions. They are now separated
from the code that accesses <server_url>/json/version, so they can be unit
tested easily.

Now that this the BrowserInfo parsing code is separated from DevToolsHttpClient,
we could also consider moving these functions into version.cc (BrowserInfo is
defined in version.h) or a new file browser_info.cc (and move the BrowserInfo
struct definition to browser_info.h). Let me know what you think.

Sam.

[stgao, 2014-09-29 23:09:54]

Either version.cc /version.h or new browser_info.cc/h are fine to me.

https://codereview.chromium.org/605143002/diff/1/chrome/test/chromedriver/chr...
File chrome/test/chromedriver/chrome/devtools_http_client.cc (right):

https://codereview.chromium.org/605143002/diff/1/chrome/test/chromedriver/chr...
chrome/test/chromedriver/chrome/devtools_http_client.cc:80: while
(!FetchUrlAndLog(version_url, context_getter_.get(), &data)) {
I see once the returned data is empty.
Add "|| data.empty()" here?

https://codereview.chromium.org/605143002/diff/1/chrome/test/chromedriver/chr...
chrome/test/chromedriver/chrome/devtools_http_client.cc:334: *browser_name =
std::string("chrome");
remove std::string?

https://codereview.chromium.org/605143002/diff/1/chrome/test/chromedriver/chr...
chrome/test/chromedriver/chrome/devtools_http_client.cc:362: // this case,
ignore it and use the default blink revision, otherwise convert
Will using the default blink revision support backward compatibility?
How about using build no if blink revision is not available like this in CrOS?

https://codereview.chromium.org/605143002/diff/1/chrome/test/chromedriver/chr...
chrome/test/chromedriver/chrome/devtools_http_client.cc:373: const int
kGitHashLength = 40;
Constant variable could be defined outside of method.

https://codereview.chromium.org/605143002/diff/1/chrome/test/chromedriver/chr...
chrome/test/chromedriver/chrome/devtools_http_client.cc:375: &&
base::ContainsOnlyChars(revision, "0123456789abcdef");
Does the function test with uppercase and lowercase?
If so, to be future-proof, maybe convert |revision| to lowercase first.

https://codereview.chromium.org/605143002/diff/1/chrome/test/chromedriver/chr...
File chrome/test/chromedriver/chrome/devtools_http_client.h (right):

https://codereview.chromium.org/605143002/diff/1/chrome/test/chromedriver/chr...
chrome/test/chromedriver/chrome/devtools_http_client.h:109: Status
ParseBrowserString(const std::string& browser,
Name |browser| is not that clear.
When I first read, I thought it was the browser name.

Maybe rename it to |browser_entry| or something?

[samuong, 2014-09-30 04:03:49]

OK, I've moved BrowserInfo and related functions into browser_info.cc/h.

https://codereview.chromium.org/605143002/diff/1/chrome/test/chromedriver/chr...
File chrome/test/chromedriver/chrome/devtools_http_client.cc (right):

https://codereview.chromium.org/605143002/diff/1/chrome/test/chromedriver/chr...
chrome/test/chromedriver/chrome/devtools_http_client.cc:80: while
(!FetchUrlAndLog(version_url, context_getter_.get(), &data)) {
On 2014/09/29 23:09:54, Shuotao wrote:
> I see once the returned data is empty.
> Add "|| data.empty()" here?

Done.

https://codereview.chromium.org/605143002/diff/1/chrome/test/chromedriver/chr...
chrome/test/chromedriver/chrome/devtools_http_client.cc:334: *browser_name =
std::string("chrome");
On 2014/09/29 23:09:54, Shuotao wrote:
> remove std::string?

Done.

https://codereview.chromium.org/605143002/diff/1/chrome/test/chromedriver/chr...
chrome/test/chromedriver/chrome/devtools_http_client.cc:362: // this case,
ignore it and use the default blink revision, otherwise convert
On 2014/09/29 23:09:54, Shuotao wrote:
> Will using the default blink revision support backward compatibility?
> How about using build no if blink revision is not available like this in CrOS?

This is just the code to parse the version json and extract the build number and
blink revision. The code that decides how to use this information is elsewhere,
but whenever we talk to Chrome we use the build number (blink revisions are used
for non-Chrome content embedders).

I've revised the comment to mention that we use build numbers for backwards
compatibility on Chrome/Chrome OS.

https://codereview.chromium.org/605143002/diff/1/chrome/test/chromedriver/chr...
chrome/test/chromedriver/chrome/devtools_http_client.cc:373: const int
kGitHashLength = 40;
On 2014/09/29 23:09:54, Shuotao wrote:
> Constant variable could be defined outside of method.

Is there any reason you prefer this? Defining it in the function reduces the
scope of the constant to a minimum. The style guide talks about "compile-time
constants of local scope" so I don't think this breaks the style guide...

http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Constant_Names

https://codereview.chromium.org/605143002/diff/1/chrome/test/chromedriver/chr...
chrome/test/chromedriver/chrome/devtools_http_client.cc:375: &&
base::ContainsOnlyChars(revision, "0123456789abcdef");
On 2014/09/29 23:09:54, Shuotao wrote:
> Does the function test with uppercase and lowercase?
> If so, to be future-proof, maybe convert |revision| to lowercase first.

Done.

https://codereview.chromium.org/605143002/diff/1/chrome/test/chromedriver/chr...
File chrome/test/chromedriver/chrome/devtools_http_client.h (right):

https://codereview.chromium.org/605143002/diff/1/chrome/test/chromedriver/chr...
chrome/test/chromedriver/chrome/devtools_http_client.h:109: Status
ParseBrowserString(const std::string& browser,
On 2014/09/29 23:09:54, Shuotao wrote:
> Name |browser| is not that clear.
> When I first read, I thought it was the browser name.
> 
> Maybe rename it to |browser_entry| or something?

How's |browser_string|? This is what it is called in the .cc file, and I've
changed the .h file to match.

[stgao, 2014-09-30 18:13:38]

lgtm if comments are addressed.

https://codereview.chromium.org/605143002/diff/1/chrome/test/chromedriver/chr...
File chrome/test/chromedriver/chrome/devtools_http_client.cc (right):

https://codereview.chromium.org/605143002/diff/1/chrome/test/chromedriver/chr...
chrome/test/chromedriver/chrome/devtools_http_client.cc:373: const int
kGitHashLength = 40;
On 2014/09/30 04:03:48, samuong wrote:
> On 2014/09/29 23:09:54, Shuotao wrote:
> > Constant variable could be defined outside of method.
> 
> Is there any reason you prefer this? Defining it in the function reduces the
> scope of the constant to a minimum. The style guide talks about "compile-time
> constants of local scope" so I don't think this breaks the style guide...
> 
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Constant_Names

The style guide says it is optional, not required.

I think we rarely use local constant for such cases.
If you prefer this style, that's fine.

https://codereview.chromium.org/605143002/diff/1/chrome/test/chromedriver/chr...
File chrome/test/chromedriver/chrome/devtools_http_client.h (right):

https://codereview.chromium.org/605143002/diff/1/chrome/test/chromedriver/chr...
chrome/test/chromedriver/chrome/devtools_http_client.h:109: Status
ParseBrowserString(const std::string& browser,
On 2014/09/30 04:03:49, samuong wrote:
> On 2014/09/29 23:09:54, Shuotao wrote:
> > Name |browser| is not that clear.
> > When I first read, I thought it was the browser name.
> > 
> > Maybe rename it to |browser_entry| or something?
> 
> How's |browser_string|? This is what it is called in the .cc file, and I've
> changed the .h file to match.
That's good.

https://codereview.chromium.org/605143002/diff/60001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome/browser_info_unittest.cc (right):

https://codereview.chromium.org/605143002/diff/60001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/browser_info_unittest.cc:85:
ASSERT_TRUE(IsGitHash("28f741cfcabffe68a9c12c4e7152569c906bd88f"));
Please also add one test for upper case.

https://codereview.chromium.org/605143002/diff/60001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome/navigation_tracker.cc (right):

https://codereview.chromium.org/605143002/diff/60001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/navigation_tracker.cc:12: #include
"chrome/test/chromedriver/chrome/version.h"
Is version.h still needed?
Same for other places.

[samuong, 2014-09-30 19:03:17]

https://codereview.chromium.org/605143002/diff/60001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome/browser_info_unittest.cc (right):

https://codereview.chromium.org/605143002/diff/60001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/browser_info_unittest.cc:85:
ASSERT_TRUE(IsGitHash("28f741cfcabffe68a9c12c4e7152569c906bd88f"));
On 2014/09/30 18:13:37, Shuotao wrote:
> Please also add one test for upper case.

Done.

https://codereview.chromium.org/605143002/diff/60001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome/navigation_tracker.cc (right):

https://codereview.chromium.org/605143002/diff/60001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/navigation_tracker.cc:12: #include
"chrome/test/chromedriver/chrome/version.h"
On 2014/09/30 18:13:37, Shuotao wrote:
> Is version.h still needed?
> Same for other places.

Done.

[samuong, 2014-09-30 19:03:31]

The CQ bit was checked by samuong@chromium.org

[commit-bot: I haz the power, 2014-09-30 19:04:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/605143002/80001

[commit-bot: I haz the power, 2014-09-30 20:56:10]

Committed patchset #5 (id:80001) as 4c40f336120d63d1a9231f023d998462402b097a

[commit-bot: I haz the power, 2014-09-30 20:56:58]

Patchset 5 (id:??) landed as
https://crrev.com/1bfa9a7b7ba1233055cfb8449b9e7b9b3ce364c9
Cr-Commit-Position: refs/heads/master@{#297496}