Bug 1374672 - Don't wait for page load events for identical hash parameters. r=automatedtester
authorHenrik Skupin <mail@hskupin.info>
Fri, 07 Jul 2017 17:34:27 +0200
changeset 368145 914fc90581c481dc80b97da80e22b42c69f1068d
parent 368144 f6817b119e1c847e83c5fac110b555ac5c53418f
child 368146 9b8398ced330159a68a772cc25e9c33609c2a44f
push id32158
push usercbook@mozilla.com
push dateTue, 11 Jul 2017 10:48:59 +0000
treeherdermozilla-central@5e2692f8a367 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersautomatedtester
bugs1374672
milestone56.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1374672 - Don't wait for page load events for identical hash parameters. r=automatedtester If the target URL has the exact same hash parameter as the current URL no page load will happen. As such Marionette should not wait for the appropriate events. MozReview-Commit-ID: LNbGJQgChya
testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py
testing/marionette/listener.js
testing/marionette/navigate.js
testing/marionette/test_navigate.js
--- a/testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py
+++ b/testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py
@@ -192,16 +192,32 @@ class TestNavigate(BaseNavigationTestCas
     def test_navigate_hash_change(self):
         doc = inline("<p id=foo>")
         self.marionette.navigate(doc)
         self.marionette.execute_script("window.visited = true", sandbox=None)
         self.marionette.navigate("{}#foo".format(doc))
         self.assertTrue(self.marionette.execute_script(
             "return window.visited", sandbox=None))
 
+    def test_navigate_hash_argument_identical(self):
+        test_page = "{}#foo".format(inline("<p id=foo>"))
+
+        self.marionette.navigate(test_page)
+        self.marionette.find_element(By.ID, "foo")
+        self.marionette.navigate(test_page)
+        self.marionette.find_element(By.ID, "foo")
+
+    def test_navigate_hash_argument_differnt(self):
+        test_page = "{}#Foo".format(inline("<p id=foo>"))
+
+        self.marionette.navigate(test_page)
+        self.marionette.find_element(By.ID, "foo")
+        self.marionette.navigate(test_page.lower())
+        self.marionette.find_element(By.ID, "foo")
+
     @skip_if_mobile("Test file is only located on host machine")
     def test_navigate_file_url(self):
         self.marionette.navigate(self.test_page_file_url)
         self.marionette.find_element(By.ID, "file-url")
         self.marionette.navigate(self.test_page_remote)
 
     @run_if_e10s("Requires e10s mode enabled")
     @skip_if_mobile("Test file is only located on host machine")
--- a/testing/marionette/listener.js
+++ b/testing/marionette/listener.js
@@ -1132,19 +1132,19 @@ function waitForPageLoaded(msg) {
  * (in chrome space).
  */
 function get(msg) {
   let {command_id, pageTimeout, url, loadEventExpected = null} = msg.json;
 
   try {
     if (typeof url == "string") {
       try {
-        let requestedURL = new URL(url).toString();
         if (loadEventExpected === null) {
-          loadEventExpected = navigate.isLoadEventExpected(requestedURL);
+          loadEventExpected = navigate.isLoadEventExpected(
+              curContainer.frame.location, url);
         }
       } catch (e) {
         let err = new InvalidArgumentError("Malformed URL: " + e.message);
         sendError(err, command_id);
         return;
       }
     }
 
--- a/testing/marionette/navigate.js
+++ b/testing/marionette/navigate.js
@@ -9,35 +9,51 @@ const {classes: Cc, interfaces: Ci, util
 Cu.importGlobalProperties(["URL"]);
 
 this.EXPORTED_SYMBOLS = ["navigate"];
 
 this.navigate = {};
 
 /**
  * Determines if we expect to get a DOM load event (DOMContentLoaded)
- * on navigating to the |url|.
+ * on navigating to the |future| URL.
  *
- * @param {string} url
- *     Destination URL
+ * @param {string} current
+ *     URL the browser is currently visiting.
+ * @param {string=} future
+ *     Destination URL, if known.
  *
  * @return {boolean}
- *     Full page load would be expected if url gets loaded.
+ *     Full page load would be expected if future is followed.
  *
  * @throws TypeError
- *     If |url| is an invalid URL.
+ *     If |current| is not defined, or any of |current| or |future|
+ *     are invalid URLs.
  */
-navigate.isLoadEventExpected = function(url) {
+navigate.isLoadEventExpected = function(current, future = undefined) {
   // assume we will go somewhere exciting
-  if (typeof url == "undefined") {
-    throw TypeError("Expected destination URL");
+  if (typeof current == "undefined") {
+    throw TypeError("Expected at least one URL");
+  }
+
+  // Assume we will go somewhere exciting
+  if (typeof future == "undefined") {
+    return true;
   }
 
-  switch (new URL(url).protocol) {
-    // assume javascript:<whatever> will modify current document
-    // but this is not an entirely safe assumption to make,
-    // considering it could be used to set window.location
-    case "javascript:":
-      return false;
+  let cur = new URL(current);
+  let fut = new URL(future);
+
+  // Assume javascript:<whatever> will modify the current document
+  // but this is not an entirely safe assumption to make,
+  // considering it could be used to set window.location
+  if (fut.protocol == "javascript:") {
+    return false;
+  }
+
+  // If hashes are present and identical
+  if (cur.href.includes("#") && fut.href.includes("#") &&
+      cur.hash === fut.hash) {
+    return false;
   }
 
   return true;
 };
--- a/testing/marionette/test_navigate.js
+++ b/testing/marionette/test_navigate.js
@@ -5,15 +5,22 @@
 const {utils: Cu} = Components;
 
 Cu.importGlobalProperties(["URL"]);
 
 Cu.import("chrome://marionette/content/navigate.js");
 
 add_test(function test_isLoadEventExpected() {
   Assert.throws(() => navigate.isLoadEventExpected(undefined),
-      /Expected destination URL/);
+      /Expected at least one URL/);
 
   equal(true, navigate.isLoadEventExpected("http://a/"));
-  equal(false, navigate.isLoadEventExpected("javascript:whatever"));
+  equal(true, navigate.isLoadEventExpected("http://a/", "http://a/"));
+
+  equal(true, navigate.isLoadEventExpected("http://a/", "http://a/#"));
+  equal(true, navigate.isLoadEventExpected("http://a/#", "http://a/"));
+  equal(true, navigate.isLoadEventExpected("http://a/#a", "http://a/#A"));
+  equal(false, navigate.isLoadEventExpected("http://a/#a", "http://a/#a"));
+
+  equal(false, navigate.isLoadEventExpected("http://a/", "javascript:whatever"));
 
   run_next_test();
 });