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

chrome/test/chromedriver/chrome/devtools_http_client.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/test/chromedriver/chrome/devtools_http_client.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/json/json_reader.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
+#include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/threading/platform_thread.h"
 #include "base/time/time.h"
 #include "base/values.h"
 #include "chrome/test/chromedriver/chrome/device_metrics.h"
 #include "chrome/test/chromedriver/chrome/devtools_client_impl.h"
 #include "chrome/test/chromedriver/chrome/log.h"
 #include "chrome/test/chromedriver/chrome/status.h"
 #include "chrome/test/chromedriver/chrome/version.h"
 #include "chrome/test/chromedriver/chrome/web_view_impl.h"
@@ skipping 44 matching lines @@
       socket_factory_(socket_factory),
       server_url_("http://" + address.ToString()),
       web_socket_url_prefix_(base::StringPrintf(
           "ws://%s/devtools/page/", address.ToString().c_str())),
       device_metrics_(device_metrics.Pass()) {}
 
 DevToolsHttpClient::~DevToolsHttpClient() {}
 
 Status DevToolsHttpClient::Init(const base::TimeDelta& timeout) {
   base::TimeTicks deadline = base::TimeTicks::Now() + timeout;
-  std::string browser_version;
-  std::string blink_version;
+  std::string version_url = server_url_ + "/json/version";
+  std::string data;
 
-  while (true) {
-    Status status = GetVersion(&browser_version, &blink_version);
-    if (status.IsOk())
-      break;
-    if (status.code() != kChromeNotReachable ||
-        base::TimeTicks::Now() > deadline) {
-      return status;
-    }
+  while (!FetchUrlAndLog(version_url, context_getter_.get(), &data)) {

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

I see once the returned data is empty.
Add "|| data.empty()" here?

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

Done.

+    if (base::TimeTicks::Now() > deadline)
+      return Status(kChromeNotReachable);
     base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(50));
   }
 
-  // |blink_version| is should look something like "537.36 (@159105)", and for
-  // this example |blink_revision| should be 159105
-  size_t before = blink_version.find('@');
-  size_t after = blink_version.find(')');
-  if (before == std::string::npos || after == std::string::npos) {
-    return Status(kUnknownError,
-                  "unrecognized Blink version: " + blink_version);
-  }
-
-  std::string blink_revision_string = blink_version.substr(before + 1,
-                                                           after - before - 1);
-  int blink_revision_int;
-  if (!base::StringToInt(blink_revision_string, &blink_revision_int)) {
-    return Status(kUnknownError,
-                  "unrecognized Blink revision: " + blink_revision_string);
-  }
-
-  browser_info_.blink_revision = blink_revision_int;
-
-  if (browser_version.empty()) {
-    browser_info_.browser_name = "content shell";
-    return Status(kOk);
-  }
-  if (browser_version.find("Version/") == 0u) {
-    browser_info_.browser_name = "webview";
-    return Status(kOk);
-  }
-  std::string prefix = "Chrome/";
-  if (browser_version.find(prefix) != 0u) {
-    return Status(kUnknownError,
-                  "unrecognized Chrome version: " + browser_version);
-  }
-
-  std::string stripped_version = browser_version.substr(prefix.length());
-  int temp_build_no;
-  std::vector<std::string> version_parts;
-  base::SplitString(stripped_version, '.', &version_parts);
-  if (version_parts.size() != 4 ||
-      !base::StringToInt(version_parts[2], &temp_build_no)) {
-    return Status(kUnknownError,
-                  "unrecognized Chrome version: " + browser_version);
-  }
-
-  browser_info_.browser_name = "chrome";
-  browser_info_.browser_version = stripped_version;
-  browser_info_.build_no = temp_build_no;
-
-  return Status(kOk);
+  return internal::ParseBrowserInfo(data, &browser_info_);
 }
 
 Status DevToolsHttpClient::GetWebViewsInfo(WebViewsInfo* views_info) {
   std::string data;
   if (!FetchUrlAndLog(server_url_ + "/json", context_getter_.get(), &data))
     return Status(kChromeNotReachable);
 
   return internal::ParseWebViewsInfo(data, views_info);
 }
 
@@ skipping 40 matching lines @@
 }
 
 const BrowserInfo* DevToolsHttpClient::browser_info() {
   return &browser_info_;
 }
 
 const DeviceMetrics* DevToolsHttpClient::device_metrics() {
   return device_metrics_.get();
 }
 
-Status DevToolsHttpClient::GetVersion(std::string* browser_version,
-                                      std::string* blink_version) {
-  std::string data;
-  if (!FetchUrlAndLog(
-          server_url_ + "/json/version", context_getter_.get(), &data))
-    return Status(kChromeNotReachable);
-
-  return internal::ParseVersionInfo(data, browser_version, blink_version);
-}
-
 Status DevToolsHttpClient::CloseFrontends(const std::string& for_client_id) {
   WebViewsInfo views_info;
   Status status = GetWebViewsInfo(&views_info);
   if (status.IsError())
     return status;
 
   // Close frontends. Usually frontends are docked in the same page, although
   // some may be in tabs (undocked, chrome://inspect, the DevTools
   // discovery page, etc.). Tabs can be closed via the DevTools HTTP close
   // URL, but docked frontends can only be closed, by design, by connecting
@@ skipping 118 matching lines @@
       type = WebViewInfo::kOther;
     else
       return Status(kUnknownError,
                     "DevTools returned unknown type:" + type_as_string);
     temp_views_info.push_back(WebViewInfo(id, debugger_url, url, type));
   }
   *views_info = WebViewsInfo(temp_views_info);
   return Status(kOk);
 }
 
-Status ParseVersionInfo(const std::string& data,
-                        std::string* browser_version,
-                        std::string* blink_version) {
+Status ParseBrowserInfo(const std::string& data, BrowserInfo* browser_info) {
   scoped_ptr<base::Value> value(base::JSONReader::Read(data));
   if (!value.get())
     return Status(kUnknownError, "version info not in JSON");
+
   base::DictionaryValue* dict;
   if (!value->GetAsDictionary(&dict))
     return Status(kUnknownError, "version info not a dictionary");
-  if (!dict->GetString("Browser", browser_version)) {
-    return Status(
-        kUnknownError,
-        "Chrome version must be >= " + GetMinimumSupportedChromeVersion(),
-        Status(kUnknownError, "version info doesn't include string 'Browser'"));
+
+  std::string browser;
+  if (!dict->GetString("Browser", &browser)) {
+    return Status(kUnknownError,
+                  "version info doesn't include string 'Browser'");
   }
-  if (!dict->GetString("WebKit-Version", blink_version)) {
+
+  std::string blink_version;
+  if (!dict->GetString("WebKit-Version", &blink_version)) {
     return Status(kUnknownError,
                   "version info doesn't include string 'WebKit-Version'");
   }
+
+  Status status = internal::ParseBrowserString(browser,
+                                               &browser_info->browser_name,
+                                               &browser_info->browser_version,
+                                               &browser_info->build_no);
+
+  if (status.IsError())
+    return status;
+
+  return internal::ParseBlinkVersionString(blink_version,
+                                           &browser_info->blink_revision);
+}
+
+Status ParseBrowserString(const std::string& browser_string,
+                          std::string* browser_name,
+                          std::string* browser_version,
+                          int* build_no) {
+  if (browser_string.empty()) {
+    *browser_name = "content shell";
+    return Status(kOk);
+  }
+
+  if (browser_string.find("Version/") == 0u) {
+    *browser_name = "webview";
+    return Status(kOk);
+  }
+
+  std::string prefix = "Chrome/";
+  if (browser_string.find(prefix) == 0u) {
+    *browser_name = std::string("chrome");

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

remove std::string?

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

Done.

+    *browser_version = browser_string.substr(prefix.length());
+
+    std::vector<std::string> version_parts;
+    base::SplitString(*browser_version, '.', &version_parts);
+    if (version_parts.size() != 4 ||
+        !base::StringToInt(version_parts[2], build_no)) {
+      return Status(kUnknownError,
+                    "unrecognized Chrome version: " + *browser_version);
+    }
+
+    return Status(kOk);
+  }
+
+  return Status(kUnknownError,
+                "unrecognized Chrome version: " + browser_string);
+}
+
+Status ParseBlinkVersionString(const std::string& blink_version,
+                               int* blink_revision) {
+  size_t before = blink_version.find('@');
+  size_t after = blink_version.find(')');
+  if (before == std::string::npos || after == std::string::npos) {
+    return Status(kUnknownError,
+                  "unrecognized Blink version string: " + blink_version);
+  }
+
+  // Chrome OS reports its Blink revision as a (non-abbreviated) git hash. In
+  // this case, ignore it and use the default blink revision, otherwise convert

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

Will using the default blink revision support backward compatibility?
How about using build no if blink revision is not available like this in CrOS?

[samuong, 2014/09/30 04:03:48]

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.

+  // the svn revision number to an int.
+  std::string revision = blink_version.substr(before + 1, after - before - 1);
+  if (!IsGitHash(revision) && !base::StringToInt(revision, blink_revision)) {
+    return Status(kUnknownError, "unrecognized Blink revision: " + revision);
+  }
+
   return Status(kOk);
 }
 
+bool IsGitHash(const std::string& revision) {
+  const int kGitHashLength = 40;

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

Constant variable could be defined outside of method.

[samuong, 2014/09/30 04:03:48]

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

[stgao, 2014/09/30 18:13:37]

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.

+  return revision.size() == kGitHashLength
+      && base::ContainsOnlyChars(revision, "0123456789abcdef");

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

Does the function test with uppercase and lowercase?
If so, to be future-proof, maybe convert |revision| to lowercase first.

[samuong, 2014/09/30 04:03:48]

Done.

+}
+
 }  // namespace internal