Skip to content

Commit

Permalink
ChromeImpl does not use DevToolsHttpClient
Browse files Browse the repository at this point in the history 
DevToolsHttpClient client cannot be used if communication between
ChromeDriver and Chrome is accomplished via pipes. Therefore ChromeImpl
has been refactored to eliminate its dependency on DevToolsHttpClient.

Bug: chromedriver:3480
Change-Id: Iee65669997492327b57cb47f80dc25d9ef356637
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4571690
Commit-Queue: Vladimir Nechaev <nechaev@chromium.org>
Reviewed-by: Thiago Perrotta <tperrotta@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1150526}
  • Loading branch information
@nechaev-chromium
nechaev-chromium authored and Chromium LUCI CQ committed May 30, 2023
1 parent 96fa1d2 commit 085a57e
Show file tree
Hide file tree
Showing 18 changed files with 197 additions and 143 deletions.
48 changes: 32 additions & 16 deletions chrome/test/chromedriver/chrome/browser_info.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,30 @@
#include "base/values.h"
#include "chrome/test/chromedriver/constants/version.h"

BrowserInfo::BrowserInfo()
: major_version(0),
build_no(kToTBuildNo),
blink_revision(kToTBlinkRevision),
is_android(false),
is_headless(false) {}
BrowserInfo::BrowserInfo() = default;

BrowserInfo::~BrowserInfo() {}
BrowserInfo::~BrowserInfo() = default;

Status ParseBrowserInfo(const std::string& data, BrowserInfo* browser_info) {
BrowserInfo::BrowserInfo(const BrowserInfo&) = default;

BrowserInfo& BrowserInfo::operator=(const BrowserInfo&) = default;

Status BrowserInfo::FillFromBrowserVersionResponse(
const base::Value::Dict& response) {
const std::string* browser_string = response.FindString("product");
if (!browser_string) {
return Status(kUnknownError, "version doesn't include 'Browser'");
}

return ParseBrowserString(false, *browser_string, this);
}

Status BrowserInfo::ParseBrowserInfo(const std::string& data) {
return ParseBrowserInfo(data, this);
}

Status BrowserInfo::ParseBrowserInfo(const std::string& data,
BrowserInfo* browser_info) {
absl::optional<base::Value> value = base::JSONReader::Read(data);
if (!value) {
return Status(kUnknownError, "version info not in JSON");
Expand Down Expand Up @@ -68,9 +82,9 @@ Status ParseBrowserInfo(const std::string& data, BrowserInfo* browser_info) {
return ParseBlinkVersionString(*blink_version, &browser_info->blink_revision);
}

Status ParseBrowserString(bool has_android_package,
const std::string& browser_string,
BrowserInfo* browser_info) {
Status BrowserInfo::ParseBrowserString(bool has_android_package,
const std::string& browser_string,
BrowserInfo* browser_info) {
if (has_android_package)
browser_info->is_android = true;

Expand Down Expand Up @@ -135,8 +149,10 @@ Status ParseBrowserString(bool has_android_package,
kBrowserShortName, browser_string.c_str()));
}

Status ParseBrowserVersionString(const std::string& browser_version,
int* major_version, int* build_no) {
Status BrowserInfo::ParseBrowserVersionString(
const std::string& browser_version,
int* major_version,
int* build_no) {
std::vector<base::StringPiece> version_parts = base::SplitStringPiece(
browser_version, ".", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
if (version_parts.size() != 4 ||
Expand All @@ -148,8 +164,8 @@ Status ParseBrowserVersionString(const std::string& browser_version,
return Status(kOk);
}

Status ParseBlinkVersionString(const std::string& blink_version,
int* blink_revision) {
Status BrowserInfo::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) {
Expand All @@ -171,7 +187,7 @@ Status ParseBlinkVersionString(const std::string& blink_version,
return Status(kOk);
}

bool IsGitHash(const std::string& revision) {
bool BrowserInfo::IsGitHash(const std::string& revision) {
const int kShortGitHashLength = 7;
const int kFullGitHashLength = 40;
return kShortGitHashLength <= revision.size()
Expand Down
40 changes: 24 additions & 16 deletions chrome/test/chromedriver/chrome/browser_info.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#ifndef CHROME_TEST_CHROMEDRIVER_CHROME_BROWSER_INFO_H_
#define CHROME_TEST_CHROMEDRIVER_CHROME_BROWSER_INFO_H_

#include "base/values.h"
#include "chrome/test/chromedriver/chrome/devtools_endpoint.h"
#include "chrome/test/chromedriver/chrome/status.h"

Expand All @@ -18,33 +19,40 @@ static const int kToTBlinkRevision = 999999;

struct BrowserInfo {
BrowserInfo();
BrowserInfo(const BrowserInfo&);
BrowserInfo& operator=(const BrowserInfo&);
~BrowserInfo();

std::string android_package;
std::string browser_name;
std::string browser_version;
std::string web_socket_url;
DevToolsEndpoint debugger_endpoint;
int major_version;
int build_no;
int blink_revision;
bool is_android;
bool is_headless;
};
int major_version = 0;
int build_no = kToTBuildNo;
int blink_revision = kToTBlinkRevision;
bool is_android = false;
bool is_headless = false;

Status FillFromBrowserVersionResponse(const base::Value::Dict& response);

Status ParseBrowserInfo(const std::string& data,
BrowserInfo* browser_info);
Status ParseBrowserInfo(const std::string& data);

Status ParseBrowserString(bool has_android_package,
const std::string& browser_string,
BrowserInfo* browser_info);
static Status ParseBrowserInfo(const std::string& data,
BrowserInfo* browser_info);

Status ParseBrowserVersionString(const std::string& browser_version,
int* major_version, int* build_no);
static Status ParseBrowserString(bool has_android_package,
const std::string& browser_string,
BrowserInfo* browser_info);

Status ParseBlinkVersionString(const std::string& blink_version,
int* blink_revision);
static Status ParseBrowserVersionString(const std::string& browser_version,
int* major_version,
int* build_no);

bool IsGitHash(const std::string& revision);
static Status ParseBlinkVersionString(const std::string& blink_version,
int* blink_revision);

static bool IsGitHash(const std::string& revision);
};

#endif // CHROME_TEST_CHROMEDRIVER_CHROME_BROWSER_INFO_H_
66 changes: 48 additions & 18 deletions chrome/test/chromedriver/chrome/browser_info_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ namespace {

void AssertParseBrowserInfoFails(const std::string& data) {
BrowserInfo browser_info;
Status status = ParseBrowserInfo(data, &browser_info);
Status status = BrowserInfo::ParseBrowserInfo(data, &browser_info);
ASSERT_TRUE(status.IsError());
}

Expand All @@ -32,7 +32,7 @@ TEST(ParseBrowserInfo, BlinkVersionContainsSvnRevision) {
std::string data("{\"Browser\": \"Chrome/37.0.2062.124\","
" \"WebKit-Version\": \"537.36 (@181352)\"}");
BrowserInfo browser_info;
Status status = ParseBrowserInfo(data, &browser_info);
Status status = browser_info.ParseBrowserInfo(data);
ASSERT_TRUE(status.IsOk());
ASSERT_EQ("chrome", browser_info.browser_name);
ASSERT_EQ("37.0.2062.124", browser_info.browser_version);
Expand All @@ -48,7 +48,7 @@ TEST(ParseBrowserInfo, BlinkVersionContainsGitHash) {
" \"537.36 (@28f741cfcabffe68a9c12c4e7152569c906bd88f)\"}");
BrowserInfo browser_info;
const int default_blink_revision = browser_info.blink_revision;
Status status = ParseBrowserInfo(data, &browser_info);
Status status = browser_info.ParseBrowserInfo(data);
ASSERT_TRUE(status.IsOk());
ASSERT_EQ("chrome", browser_info.browser_name);
ASSERT_EQ("37.0.2062.124", browser_info.browser_version);
Expand All @@ -59,8 +59,8 @@ TEST(ParseBrowserInfo, BlinkVersionContainsGitHash) {

TEST(ParseBrowserString, KitKatWebView) {
BrowserInfo browser_info;
Status status =
ParseBrowserString(false, "Version/4.0 Chrome/30.0.0.0", &browser_info);
Status status = BrowserInfo::ParseBrowserString(
false, "Version/4.0 Chrome/30.0.0.0", &browser_info);
ASSERT_TRUE(status.IsOk());
ASSERT_EQ("webview", browser_info.browser_name);
ASSERT_EQ("30.0.0.0", browser_info.browser_version);
Expand All @@ -72,7 +72,8 @@ TEST(ParseBrowserString, KitKatWebView) {

TEST(ParseBrowserString, LollipopWebView) {
BrowserInfo browser_info;
Status status = ParseBrowserString(true, "Chrome/37.0.0.0", &browser_info);
Status status =
BrowserInfo::ParseBrowserString(true, "Chrome/37.0.0.0", &browser_info);
ASSERT_TRUE(status.IsOk());
ASSERT_EQ("webview", browser_info.browser_name);
ASSERT_EQ("37.0.0.0", browser_info.browser_version);
Expand All @@ -84,8 +85,8 @@ TEST(ParseBrowserString, LollipopWebView) {

TEST(ParseBrowserString, AndroidChrome) {
BrowserInfo browser_info;
Status status =
ParseBrowserString(true, "Chrome/39.0.2171.59", &browser_info);
Status status = BrowserInfo::ParseBrowserString(true, "Chrome/39.0.2171.59",
&browser_info);
ASSERT_TRUE(status.IsOk());
ASSERT_EQ("chrome", browser_info.browser_name);
ASSERT_EQ("39.0.2171.59", browser_info.browser_version);
Expand All @@ -97,8 +98,8 @@ TEST(ParseBrowserString, AndroidChrome) {

TEST(ParseBrowserString, DesktopChrome) {
BrowserInfo browser_info;
Status status =
ParseBrowserString(false, "Chrome/39.0.2171.59", &browser_info);
Status status = BrowserInfo::ParseBrowserString(false, "Chrome/39.0.2171.59",
&browser_info);
ASSERT_TRUE(status.IsOk());
ASSERT_EQ("chrome", browser_info.browser_name);
ASSERT_EQ("39.0.2171.59", browser_info.browser_version);
Expand All @@ -110,8 +111,8 @@ TEST(ParseBrowserString, DesktopChrome) {

TEST(ParseBrowserString, HeadlessChrome) {
BrowserInfo browser_info;
Status status =
ParseBrowserString(false, "HeadlessChrome/39.0.2171.59", &browser_info);
Status status = BrowserInfo::ParseBrowserString(
false, "HeadlessChrome/39.0.2171.59", &browser_info);
ASSERT_TRUE(status.IsOk());
ASSERT_EQ("headless chrome", browser_info.browser_name);
ASSERT_EQ("39.0.2171.59", browser_info.browser_version);
Expand All @@ -123,31 +124,60 @@ TEST(ParseBrowserString, HeadlessChrome) {

TEST(ParseBlinkVersionString, GitHash) {
int rev = -1;
Status status = ParseBlinkVersionString(
Status status = BrowserInfo::ParseBlinkVersionString(
"537.36 (@28f741cfcabffe68a9c12c4e7152569c906bd88f)", &rev);
ASSERT_TRUE(status.IsOk());
ASSERT_EQ(-1, rev);
}

TEST(ParseBlinkVersionString, SvnRevision) {
int blink_revision = -1;
Status status = ParseBlinkVersionString("537.36 (@159105)", &blink_revision);
Status status =
BrowserInfo::ParseBlinkVersionString("537.36 (@159105)", &blink_revision);
ASSERT_TRUE(status.IsOk());
ASSERT_EQ(159105, blink_revision);
}

TEST(IsGitHash, GitHash) {
ASSERT_TRUE(IsGitHash("28f741cfcabffe68a9c12c4e7152569c906bd88f"));
ASSERT_TRUE(
BrowserInfo::IsGitHash("28f741cfcabffe68a9c12c4e7152569c906bd88f"));
}

TEST(IsGitHash, ShortGitHash) {
ASSERT_TRUE(IsGitHash("1493aa5"));
ASSERT_TRUE(BrowserInfo::IsGitHash("1493aa5"));
}

TEST(IsGitHash, GitHashWithUpperCaseCharacters) {
ASSERT_TRUE(IsGitHash("28F741CFCABFFE68A9C12C4E7152569C906BD88F"));
ASSERT_TRUE(
BrowserInfo::IsGitHash("28F741CFCABFFE68A9C12C4E7152569C906BD88F"));
}

TEST(IsGitHash, SvnRevision) {
ASSERT_FALSE(IsGitHash("159105"));
ASSERT_FALSE(BrowserInfo::IsGitHash("159105"));
}

TEST(FillFromBrowserVersionResponse, Chrome) {
base::Value::Dict response;
response.Set("product", "Chrome/37.0.2062.124");
BrowserInfo browser_info;
Status status = browser_info.FillFromBrowserVersionResponse(response);
ASSERT_TRUE(status.IsOk());
ASSERT_EQ("chrome", browser_info.browser_name);
ASSERT_EQ("37.0.2062.124", browser_info.browser_version);
ASSERT_EQ(37, browser_info.major_version);
ASSERT_EQ(2062, browser_info.build_no);
ASSERT_FALSE(browser_info.is_headless);
}

TEST(FillFromBrowserVersionResponse, HeadlessChrome) {
base::Value::Dict response;
response.Set("product", "HeadlessChrome/39.0.2171.59");
BrowserInfo browser_info;
Status status = browser_info.FillFromBrowserVersionResponse(response);
ASSERT_TRUE(status.IsOk());
ASSERT_EQ("headless chrome", browser_info.browser_name);
ASSERT_EQ("39.0.2171.59", browser_info.browser_version);
ASSERT_EQ(39, browser_info.major_version);
ASSERT_EQ(2171, browser_info.build_no);
ASSERT_TRUE(browser_info.is_headless);
}
6 changes: 4 additions & 2 deletions chrome/test/chromedriver/chrome/chrome_android_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,16 @@
#include "chrome/test/chromedriver/chrome/web_view_impl.h"

ChromeAndroidImpl::ChromeAndroidImpl(
std::unique_ptr<DevToolsHttpClient> http_client,
BrowserInfo browser_info,
std::set<WebViewInfo::Type> window_types,
std::unique_ptr<DevToolsClient> websocket_client,
std::vector<std::unique_ptr<DevToolsEventListener>>
devtools_event_listeners,
absl::optional<MobileDevice> mobile_device,
std::string page_load_strategy,
std::unique_ptr<Device> device)
: ChromeImpl(std::move(http_client),
: ChromeImpl(std::move(browser_info),
std::move(window_types),
std::move(websocket_client),
std::move(devtools_event_listeners),
std::move(mobile_device),
Expand Down
4 changes: 2 additions & 2 deletions chrome/test/chromedriver/chrome/chrome_android_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,11 @@

class Device;
class DevToolsClient;
class DevToolsHttpClient;

class ChromeAndroidImpl : public ChromeImpl {
public:
ChromeAndroidImpl(std::unique_ptr<DevToolsHttpClient> http_client,
ChromeAndroidImpl(BrowserInfo browser_info,
std::set<WebViewInfo::Type> window_types,
std::unique_ptr<DevToolsClient> websocket_client,
std::vector<std::unique_ptr<DevToolsEventListener>>
devtools_event_listeners,
Expand Down
12 changes: 7 additions & 5 deletions chrome/test/chromedriver/chrome/chrome_desktop_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,8 @@ bool KillProcess(const base::Process& process, bool kill_gracefully) {
} // namespace

ChromeDesktopImpl::ChromeDesktopImpl(
std::unique_ptr<DevToolsHttpClient> http_client,
BrowserInfo browser_info,
std::set<WebViewInfo::Type> window_types,
std::unique_ptr<DevToolsClient> websocket_client,
std::vector<std::unique_ptr<DevToolsEventListener>>
devtools_event_listeners,
Expand All @@ -84,7 +85,8 @@ ChromeDesktopImpl::ChromeDesktopImpl(
base::ScopedTempDir* user_data_dir,
base::ScopedTempDir* extension_dir,
bool network_emulation_enabled)
: ChromeImpl(std::move(http_client),
: ChromeImpl(std::move(browser_info),
std::move(window_types),
std::move(websocket_client),
std::move(devtools_event_listeners),
std::move(mobile_device),
Expand Down Expand Up @@ -159,9 +161,9 @@ Status ChromeDesktopImpl::WaitForPageToLoad(
Status status = CreateClient(id, &client);
if (status.IsError())
return status;
std::unique_ptr<WebViewImpl> web_view_tmp(new WebViewImpl(
id, w3c_compliant, nullptr, devtools_http_client_->browser_info(),
std::move(client), mobile_device, page_load_strategy()));
std::unique_ptr<WebViewImpl> web_view_tmp(
new WebViewImpl(id, w3c_compliant, nullptr, &browser_info_,
std::move(client), mobile_device, page_load_strategy()));
DevToolsClientImpl* parent =
static_cast<DevToolsClientImpl*>(devtools_websocket_client_.get());
status = web_view_tmp->AttachTo(parent);
Expand Down
4 changes: 2 additions & 2 deletions chrome/test/chromedriver/chrome/chrome_desktop_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,13 @@ class TimeDelta;
}

class DevToolsClient;
class DevToolsHttpClient;
class Status;
class WebView;

class ChromeDesktopImpl : public ChromeImpl {
public:
ChromeDesktopImpl(std::unique_ptr<DevToolsHttpClient> http_client,
ChromeDesktopImpl(BrowserInfo browser_info,
std::set<WebViewInfo::Type> window_types,
std::unique_ptr<DevToolsClient> websocket_client,
std::vector<std::unique_ptr<DevToolsEventListener>>
devtools_event_listeners,
Expand Down

0 comments on commit 085a57e

Please sign in to comment.