From cbd56158933271417af119ee8d2c34ffedef634a Mon Sep 17 00:00:00 2001 From: samuong Date: Mon, 27 Jul 2015 12:28:10 -0700 Subject: [PATCH] [chromedriver] Use document.readyState to determine loading state for M44+. As of crrev.com/323900 (Chrome 44+) we no longer receive Page.frameStartedLoading and Page.frameStoppedLoading events immediately after clicking on an element. This can cause tests to fail if the click command results in a page navigation, such as in the case of PageLoadingTest.testShouldTimeoutIfAPageTakesTooLongToLoadAfterClick from the Selenium test suite. Unfortunately, this breaks PageLoadingTest.testShouldNotHangIfDocumentOpenCallIsNeverFollowedByDocumentCloseCall, which I've also disabled in this CL. A long-term fix will require a new DevTools command to query the page loading state. BUG=chromedriver:1158,chromedriver:1167 Review URL: https://codereview.chromium.org/1250333002 Cr-Commit-Position: refs/heads/master@{#340529} --- chrome/test/chromedriver/chrome/browser_info.cc | 31 ++++-- chrome/test/chromedriver/chrome/browser_info.h | 5 + .../chromedriver/chrome/browser_info_unittest.cc | 6 + .../chromedriver/chrome/chrome_android_impl.cc | 18 +-- .../test/chromedriver/chrome/navigation_tracker.cc | 121 +++++++++++++-------- chrome/test/chromedriver/test/run_py_tests.py | 11 ++ chrome/test/chromedriver/test/test_expectations | 2 + chrome/test/data/chromedriver/nested.html | 2 +- chrome/test/data/chromedriver/outer.html | 2 +- 9 files changed, 125 insertions(+), 73 deletions(-) diff --git a/chrome/test/chromedriver/chrome/browser_info.cc b/chrome/test/chromedriver/chrome/browser_info.cc index 4f79ee3b0254..8440773c72a6 100644 --- a/chrome/test/chromedriver/chrome/browser_info.cc +++ b/chrome/test/chromedriver/chrome/browser_info.cc @@ -20,6 +20,7 @@ const std::string kVersionPrefix = "Chrome/"; BrowserInfo::BrowserInfo() : browser_name(std::string()), browser_version(std::string()), + major_version(0), build_no(kToTBuildNo), blink_revision(kToTBlinkRevision), is_android(false) { @@ -27,11 +28,13 @@ BrowserInfo::BrowserInfo() BrowserInfo::BrowserInfo(std::string browser_name, std::string browser_version, + int major_version, int build_no, int blink_revision, bool is_android) : browser_name(browser_name), browser_version(browser_version), + major_version(major_version), build_no(build_no), blink_revision(blink_revision), is_android(is_android) { @@ -77,18 +80,15 @@ Status ParseBrowserString(bool has_android_package, int build_no = 0; if (browser_string.find(kVersionPrefix) == 0u) { std::string version = browser_string.substr(kVersionPrefix.length()); - std::vector version_parts = base::SplitStringPiece( - version, ".", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL); - if (version_parts.size() != 4 || - !base::StringToInt(version_parts[2], &build_no)) { - return Status(kUnknownError, "unrecognized Chrome version: " + version); - } + Status status = ParseBrowserVersionString( + version, &browser_info->major_version, &build_no); + if (status.IsError()) + return status; if (build_no != 0) { browser_info->browser_name = "chrome"; - browser_info->browser_version = - browser_string.substr(kVersionPrefix.length()); + browser_info->browser_version = version; browser_info->build_no = build_no; return Status(kOk); } @@ -102,6 +102,8 @@ Status ParseBrowserString(bool has_android_package, browser_info->browser_version = browser_string.substr(pos + kVersionPrefix.length()); browser_info->is_android = true; + return ParseBrowserVersionString(browser_info->browser_version, + &browser_info->major_version, &build_no); } return Status(kOk); } @@ -110,6 +112,19 @@ Status ParseBrowserString(bool has_android_package, "unrecognized Chrome version: " + browser_string); } +Status ParseBrowserVersionString(const std::string& browser_version, + int* major_version, int* build_no) { + std::vector version_parts = base::SplitStringPiece( + browser_version, ".", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL); + if (version_parts.size() != 4 || + !base::StringToInt(version_parts[0], major_version) || + !base::StringToInt(version_parts[2], build_no)) { + return Status(kUnknownError, + "unrecognized browser version: " + browser_version); + } + return Status(kOk); +} + Status ParseBlinkVersionString(const std::string& blink_version, int* blink_revision) { size_t before = blink_version.find('@'); diff --git a/chrome/test/chromedriver/chrome/browser_info.h b/chrome/test/chromedriver/chrome/browser_info.h index ce328507e558..f97215c4464b 100644 --- a/chrome/test/chromedriver/chrome/browser_info.h +++ b/chrome/test/chromedriver/chrome/browser_info.h @@ -19,12 +19,14 @@ struct BrowserInfo { BrowserInfo(); BrowserInfo(std::string browser_name_, std::string browser_version_, + int major_version_, int build_no_, int blink_revision_, bool is_android_); std::string browser_name; std::string browser_version; + int major_version; int build_no; int blink_revision; bool is_android; @@ -37,6 +39,9 @@ Status ParseBrowserString(bool has_android_package, const std::string& browser_string, BrowserInfo* browser_info); +Status ParseBrowserVersionString(const std::string& browser_version, + int* major_version, int* build_no); + Status ParseBlinkVersionString(const std::string& blink_version, int* blink_revision); diff --git a/chrome/test/chromedriver/chrome/browser_info_unittest.cc b/chrome/test/chromedriver/chrome/browser_info_unittest.cc index 04236a14f061..c33f77118c5e 100644 --- a/chrome/test/chromedriver/chrome/browser_info_unittest.cc +++ b/chrome/test/chromedriver/chrome/browser_info_unittest.cc @@ -36,6 +36,7 @@ TEST(ParseBrowserInfo, BlinkVersionContainsSvnRevision) { 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_EQ(181352, browser_info.blink_revision); } @@ -50,6 +51,7 @@ TEST(ParseBrowserInfo, BlinkVersionContainsGitHash) { 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_EQ(default_blink_revision, browser_info.blink_revision); } @@ -61,6 +63,7 @@ TEST(ParseBrowserString, KitKatWebView) { ASSERT_TRUE(status.IsOk()); ASSERT_EQ("webview", browser_info.browser_name); ASSERT_EQ("30.0.0.0", browser_info.browser_version); + ASSERT_EQ(30, browser_info.major_version); ASSERT_EQ(kToTBuildNo, browser_info.build_no); ASSERT_TRUE(browser_info.is_android); } @@ -71,6 +74,7 @@ TEST(ParseBrowserString, LollipopWebView) { ASSERT_TRUE(status.IsOk()); ASSERT_EQ("webview", browser_info.browser_name); ASSERT_EQ("37.0.0.0", browser_info.browser_version); + ASSERT_EQ(37, browser_info.major_version); ASSERT_EQ(kToTBuildNo, browser_info.build_no); ASSERT_TRUE(browser_info.is_android); } @@ -82,6 +86,7 @@ TEST(ParseBrowserString, AndroidChrome) { ASSERT_TRUE(status.IsOk()); ASSERT_EQ("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_android); } @@ -93,6 +98,7 @@ TEST(ParseBrowserString, DesktopChrome) { ASSERT_TRUE(status.IsOk()); ASSERT_EQ("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_FALSE(browser_info.is_android); } diff --git a/chrome/test/chromedriver/chrome/chrome_android_impl.cc b/chrome/test/chromedriver/chrome/chrome_android_impl.cc index 5cfcec05e6f1..2002baaf628d 100644 --- a/chrome/test/chromedriver/chrome/chrome_android_impl.cc +++ b/chrome/test/chromedriver/chrome/chrome_android_impl.cc @@ -4,7 +4,6 @@ #include "chrome/test/chromedriver/chrome/chrome_android_impl.h" -#include "base/strings/string_number_conversions.h" #include "base/strings/string_split.h" #include "chrome/test/chromedriver/chrome/device_manager.h" #include "chrome/test/chromedriver/chrome/devtools_client.h" @@ -36,21 +35,10 @@ std::string ChromeAndroidImpl::GetOperatingSystemName() { bool ChromeAndroidImpl::HasTouchScreen() const { const BrowserInfo* browser_info = GetBrowserInfo(); - if (browser_info->browser_name == "webview") { - std::vector version_parts = base::SplitStringPiece( - browser_info->browser_version, ".", - base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL); - int major_version; - if (version_parts.size() != 4 || - !base::StringToInt(version_parts[0], &major_version)) { - LOG(WARNING) << "Unrecognized webview version: " - << browser_info->browser_version; - return false; - } - return major_version >= 44; - } else { + if (browser_info->browser_name == "webview") + return browser_info->major_version >= 44; + else return browser_info->build_no >= 2388; - } } Status ChromeAndroidImpl::QuitImpl() { diff --git a/chrome/test/chromedriver/chrome/navigation_tracker.cc b/chrome/test/chromedriver/chrome/navigation_tracker.cc index 540e83dfef38..9e7fadd80f67 100644 --- a/chrome/test/chromedriver/chrome/navigation_tracker.cc +++ b/chrome/test/chromedriver/chrome/navigation_tracker.cc @@ -31,57 +31,82 @@ NavigationTracker::~NavigationTracker() {} Status NavigationTracker::IsPendingNavigation(const std::string& frame_id, bool* is_pending) { - if (loading_state_ == kUnknown) { + // As of crrev.com/323900 (Chrome 44+) we no longer receive + // Page.frameStartedLoading and Page.frameStoppedLoading events immediately + // after clicking on an element. This can cause tests to fail if the click + // command results in a page navigation, such as in the case of + // PageLoadingTest.testShouldTimeoutIfAPageTakesTooLongToLoadAfterClick from + // the Selenium test suite. + // TODO(samuong): Remove this once we stop supporting M43. + bool expecting_frame_loading_events; + if (browser_info_->browser_name == "chrome") + expecting_frame_loading_events = browser_info_->build_no < 2358; + else + expecting_frame_loading_events = browser_info_->major_version < 44; + + if (!expecting_frame_loading_events) { + base::DictionaryValue params; + params.SetString("expression", "document.readyState"); scoped_ptr result; - - // In the case that a http request is sent to server to fetch the page - // content and the server hasn't responded at all, a dummy page is created - // for the new window. In such case, the baseURL will be empty. - base::DictionaryValue empty_params; - Status status = client_->SendCommandAndGetResult( - "DOM.getDocument", empty_params, &result); - std::string base_url; - if (status.IsError() || !result->GetString("root.baseURL", &base_url)) - return Status(kUnknownError, "cannot determine loading status", status); - if (base_url.empty()) { - *is_pending = true; - loading_state_ = kLoading; - return Status(kOk); + Status status = + client_->SendCommandAndGetResult("Runtime.evaluate", params, &result); + std::string ready_state; + if (status.IsError() || !result->GetString("result.value", &ready_state)) + return status; + *is_pending = ready_state != "complete"; + } else { + if (loading_state_ == kUnknown) { + scoped_ptr result; + + // In the case that a http request is sent to server to fetch the page + // content and the server hasn't responded at all, a dummy page is created + // for the new window. In such case, the baseURL will be empty. + base::DictionaryValue empty_params; + Status status = client_->SendCommandAndGetResult( + "DOM.getDocument", empty_params, &result); + std::string base_url; + if (status.IsError() || !result->GetString("root.baseURL", &base_url)) + return Status(kUnknownError, "cannot determine loading status", status); + if (base_url.empty()) { + *is_pending = true; + loading_state_ = kLoading; + return Status(kOk); + } + + // If the loading state is unknown (which happens after first connecting), + // force loading to start and set the state to loading. This will cause a + // frame start event to be received, and the frame stop event will not be + // received until all frames are loaded. Loading is forced to start by + // attaching a temporary iframe. Forcing loading to start is not + // necessary if the main frame is not yet loaded. + const char kStartLoadingIfMainFrameNotLoading[] = + "var isLoaded = document.readyState == 'complete' ||" + " document.readyState == 'interactive';" + "if (isLoaded) {" + " var frame = document.createElement('iframe');" + " frame.src = 'about:blank';" + " document.body.appendChild(frame);" + " window.setTimeout(function() {" + " document.body.removeChild(frame);" + " }, 0);" + "}"; + base::DictionaryValue params; + params.SetString("expression", kStartLoadingIfMainFrameNotLoading); + status = client_->SendCommandAndGetResult( + "Runtime.evaluate", params, &result); + if (status.IsError()) + return Status(kUnknownError, "cannot determine loading status", status); + + // Between the time the JavaScript is evaluated and + // SendCommandAndGetResult returns, OnEvent may have received info about + // the loading state. This is only possible during a nested command. Only + // set the loading state if the loading state is still unknown. + if (loading_state_ == kUnknown) + loading_state_ = kLoading; } - - // If the loading state is unknown (which happens after first connecting), - // force loading to start and set the state to loading. This will - // cause a frame start event to be received, and the frame stop event - // will not be received until all frames are loaded. - // Loading is forced to start by attaching a temporary iframe. - // Forcing loading to start is not necessary if the main frame is not yet - // loaded. - const char kStartLoadingIfMainFrameNotLoading[] = - "var isLoaded = document.readyState == 'complete' ||" - " document.readyState == 'interactive';" - "if (isLoaded) {" - " var frame = document.createElement('iframe');" - " frame.src = 'about:blank';" - " document.body.appendChild(frame);" - " window.setTimeout(function() {" - " document.body.removeChild(frame);" - " }, 0);" - "}"; - base::DictionaryValue params; - params.SetString("expression", kStartLoadingIfMainFrameNotLoading); - status = client_->SendCommandAndGetResult( - "Runtime.evaluate", params, &result); - if (status.IsError()) - return Status(kUnknownError, "cannot determine loading status", status); - - // Between the time the JavaScript is evaluated and SendCommandAndGetResult - // returns, OnEvent may have received info about the loading state. - // This is only possible during a nested command. Only set the loading state - // if the loading state is still unknown. - if (loading_state_ == kUnknown) - loading_state_ = kLoading; + *is_pending = loading_state_ == kLoading; } - *is_pending = loading_state_ == kLoading; + if (frame_id.empty()) { *is_pending |= scheduled_frame_set_.size() > 0; *is_pending |= pending_frame_set_.size() > 0; diff --git a/chrome/test/chromedriver/test/run_py_tests.py b/chrome/test/chromedriver/test/run_py_tests.py index ea80ee429cfb..1159b0027c87 100755 --- a/chrome/test/chromedriver/test/run_py_tests.py +++ b/chrome/test/chromedriver/test/run_py_tests.py @@ -1092,6 +1092,17 @@ class ChromeDriverTest(ChromeDriverBaseTest): self._driver.SwitchToWindow(new_window_handle) self.assertEquals('chrome://print/', self._driver.GetCurrentUrl()) + def testCanClickInIframes(self): + self._driver.Load(self.GetHttpUrlForFile('/chromedriver/nested.html')) + a = self._driver.FindElement('tag name', 'a') + a.Click() + self.assertTrue(self._driver.GetCurrentUrl().endswith('#one')) + frame = self._driver.FindElement('tag name', 'iframe') + self._driver.SwitchToFrame(frame) + a = self._driver.FindElement('tag name', 'a') + a.Click() + self.assertTrue(self._driver.GetCurrentUrl().endswith('#two')) + class ChromeDriverAndroidTest(ChromeDriverBaseTest): """End to end tests for Android-specific tests.""" diff --git a/chrome/test/chromedriver/test/test_expectations b/chrome/test/chromedriver/test/test_expectations index 836ae7332dbc..12832b7fc8ca 100644 --- a/chrome/test/chromedriver/test/test_expectations +++ b/chrome/test/chromedriver/test/test_expectations @@ -96,6 +96,8 @@ _REVISION_NEGATIVE_FILTER['HEAD'] = [ 'DragAndDropTest.testDragAndDrop', 'DragAndDropTest.testShouldAllowUsersToDragAndDropToElementsOffTheCurrentViewPort', 'DragAndDropTest.testElementInDiv', + # https://code.google.com/p/chromedriver/issues/detail?id=1167 + 'PageLoadingTest.testShouldNotHangIfDocumentOpenCallIsNeverFollowedByDocumentCloseCall', ] diff --git a/chrome/test/data/chromedriver/nested.html b/chrome/test/data/chromedriver/nested.html index 4c85355974e8..5a5f31db6c36 100644 --- a/chrome/test/data/chromedriver/nested.html +++ b/chrome/test/data/chromedriver/nested.html @@ -4,7 +4,7 @@ Contains a nested iframe -

One

+

One

-- 2.11.4.GIT