Bug 1241085 - part 2: rip out userTypedClear and replace it with more self-documenting stuff, r=mconley

userTypedClear was used for two cases:
1) to keep track of whether we were in the middle of a loadURI call. This use is replaced by inLoadURI, which is
more sane when using e10s (though it's hard to be precise there because we're sending all web navigation calls to
the content process and this introduces a degree of asynchronousness that we just have to live with...).
2) to keep track of whether we were between a network start and a corresponding network stop, and whether the user
typed since the load properly started. This is now tracked on a small object on the browser binding, which has
appropriately named method so we're not just incrementing some magic number but actually understand what
we're saying, and so the information we get out (did the user type since this load started or not?) makes sense.

Note that we're keeping userTypedClear in session store information in order to remain backwards compatible.
It becomes a simple boolean-stored-as-int (1 or 0) that indicates whether we quit/crashed/stopped while a load
was pending, or not.

MozReview-Commit-ID: 5NbmVueocC7

--HG--
extra : rebase_source : 55cd9f3513c0a985580957a5157d47853a8822bf
extra : source : 386b9c750bd2ed458112acd29eb72e4e1371af9d

browser/base/content/browser.js

@@ -816,10 +816,6 @@ function _loadURIWithFlags(browser, uri, params) {
   let charset = params.charset;
   let postData = params.postData;
 
-  if (!(flags & browser.webNavigation.LOAD_FLAGS_FROM_EXTERNAL)) {
-    browser.userTypedClear++;
-  }
-
   let wasRemote = browser.isRemoteBrowser;
 
   let process = browser.isRemoteBrowser ? Ci.nsIXULRuntime.PROCESS_TYPE_CONTENT
@@ -867,9 +863,6 @@ function _loadURIWithFlags(browser, uri, params) {
         (wasRemote && mustChangeProcess)) {
       browser.inLoadURI = false;
     }
-    if (browser.userTypedClear) {
-      browser.userTypedClear--;
-    }
   }
 }
 

browser/base/content/tabbrowser.xml

@@ -614,6 +614,12 @@
 
               const nsIWebProgressListener = Components.interfaces.nsIWebProgressListener;
               const nsIChannel = Components.interfaces.nsIChannel;
+              let location, originalLocation;
+              try {
+                aRequest.QueryInterface(nsIChannel)
+                location = aRequest.URI;
+                originalLocation = aRequest.originalURI;
+              } catch (ex) {}
 
               if (aStateFlags & nsIWebProgressListener.STATE_START) {
                 this.mRequestCount++;
@@ -632,16 +638,8 @@
 
               if (aStateFlags & nsIWebProgressListener.STATE_START &&
                   aStateFlags & nsIWebProgressListener.STATE_IS_NETWORK) {
-                // It's okay to clear what the user typed when we start
-                // loading a document. If the user types, this counter gets
-                // set to zero, if the document load ends without an
-                // onLocationChange, this counter gets decremented
-                // (so we keep it while switching tabs after failed loads)
-                // We need to add 2 because loadURIWithFlags may have
-                // cancelled a pending load which would have cleared
-                // its anchor scroll detection temporary increment.
                 if (aWebProgress.isTopLevel) {
-                  this.mBrowser.userTypedClear += 2;
+                  this.mBrowser.urlbarChangeTracker.startedLoad();
 
                   // If the browser is loading it must not be crashed anymore
                   this.mTab.removeAttribute("crashed");
@@ -672,8 +670,8 @@
                 this.mTab.removeAttribute("progress");
 
                 if (aWebProgress.isTopLevel) {
-                  if (!Components.isSuccessCode(aStatus) &&
+                  let isSuccessful = Components.isSuccessCode(aStatus);
-                      !isTabEmpty(this.mTab)) {
+                  if (!isSuccessful && !isTabEmpty(this.mTab)) {
                     // Restore the current document's location in case the
                     // request was stopped (possibly from a content script)
                     // before the location changed.
@@ -684,14 +682,8 @@
                     if (this.mTab.selected && gURLBar && !inLoadURI) {
                       URLBarSetURI();
                     }
-                  } else {
+                  } else if (isSuccessful) {
-                    // The document is done loading, we no longer want the
+                    this.mBrowser.urlbarChangeTracker.finishedLoad();
-                    // value cleared.
-
-                    if (this.mBrowser.userTypedClear > 1)
-                      this.mBrowser.userTypedClear -= 2;
-                    else if (this.mBrowser.userTypedClear > 0)
-                      this.mBrowser.userTypedClear--;
                   }
 
                   if (!this.mBrowser.mIconURL)
@@ -701,8 +693,6 @@
                 if (this.mBlank)
                   this.mBlank = false;
 
-                var location = aRequest.QueryInterface(nsIChannel).URI;
-
                 // For keyword URIs clear the user typed value since they will be changed into real URIs
                 if (location.scheme == "keyword")
                   this.mBrowser.userTypedValue = null;
@@ -747,18 +737,17 @@
               if (topLevel) {
                 let isSameDocument =
                   !!(aFlags & Ci.nsIWebProgressListener.LOCATION_CHANGE_SAME_DOCUMENT);
-                // If userTypedClear > 0, the document loaded correctly and we should be
+                // We need to clear the typed value
-                // clearing the user typed value. We also need to clear the typed value
                 // if the document failed to load, to make sure the urlbar reflects the
                 // failed URI (particularly for SSL errors). However, don't clear the value
                 // if the error page's URI is about:blank, because that causes complete
                 // loss of urlbar contents for invalid URI errors (see bug 867957).
                 // Another reason to clear the userTypedValue is if this was an anchor
                 // navigation initiated by the user.
-                if (this.mBrowser.userTypedClear > 0 ||
+                if (this.mBrowser.didStartLoadSinceLastUserTyping() ||
                     ((aFlags & Ci.nsIWebProgressListener.LOCATION_CHANGE_ERROR_PAGE) &&
                      aLocation.spec != "about:blank") ||
-                     (isSameDocument && this.mBrowser.inLoadURI)) {
+                    (isSameDocument && this.mBrowser.inLoadURI)) {
                   this.mBrowser.userTypedValue = null;
                 }
 
@@ -4171,10 +4160,6 @@
         ]]></body>
       </method>
 
-      <property name="userTypedClear"
-                onget="return this.mCurrentBrowser.userTypedClear;"
-                onset="return this.mCurrentBrowser.userTypedClear = val;"/>
-
       <property name="userTypedValue"
                 onget="return this.mCurrentBrowser.userTypedValue;"
                 onset="return this.mCurrentBrowser.userTypedValue = val;"/>

browser/components/sessionstore/SessionStore.jsm

@@ -821,7 +821,7 @@ var SessionStoreInternal = {
           if (tabData.userTypedValue && !tabData.userTypedClear) {
             browser.userTypedValue = tabData.userTypedValue;
             if (data.didStartLoad) {
-              browser.userTypedClear++;
+              browser.urlbarChangeTracker.startedLoad();
             }
             win.URLBarSetURI();
           }

browser/components/sessionstore/TabState.jsm

@@ -129,12 +129,17 @@ var TabStateInternal = {
     }
 
     // If there is a userTypedValue set, then either the user has typed something
-    // in the URL bar, or a new tab was opened with a URI to load. userTypedClear
+    // in the URL bar, or a new tab was opened with a URI to load.
-    // is used to indicate whether the tab was in some sort of loading state with
+    // If so, we also track whether we were still in the process of loading something.
-    // userTypedValue.
     if (!("userTypedValue" in tabData) && browser.userTypedValue) {
       tabData.userTypedValue = browser.userTypedValue;
-      tabData.userTypedClear = browser.userTypedClear;
+      // We always used to keep track of the loading state as an integer, where
+      // '0' indicated the user had typed since the last load (or no load was
+      // ongoing), and any positive value indicated we had started a load since
+      // the last time the user typed in the URL bar. Mimic this to keep the
+      // session store representation in sync, even though we now represent this
+      // more explicitly:
+      tabData.userTypedClear = browser.didStartLoadSinceLastUserTyping() ? 1 : 0;
     }
 
     return tabData;

browser/components/sessionstore/test/browser_522545.js

@@ -28,8 +28,8 @@ function test() {
          "No history entries still sets currentURI to about:blank");
       is(browser.userTypedValue, "example.com",
          "userTypedValue was correctly restored");
-      is(browser.userTypedClear, 0,
+      ok(!browser.didStartLoadSinceLastUserTyping(),
-         "userTypeClear restored as expected");
+         "We still know that no load is ongoing");
       is(gURLBar.value, "example.com",
          "Address bar's value correctly restored");
       // Change tabs to make sure address bar value gets updated
@@ -60,8 +60,8 @@ function test() {
          "No history entries still sets currentURI to about:blank");
       is(browser.userTypedValue, "example.org",
          "userTypedValue was correctly restored");
-      is(browser.userTypedClear, 0,
+      ok(!browser.didStartLoadSinceLastUserTyping(),
-         "userTypeClear restored as expected");
+         "We still know that no load is ongoing");
       is(gURLBar.value, "about:mozilla",
          "Address bar's value correctly restored");
       // Change tabs to make sure address bar value gets updated
@@ -93,8 +93,8 @@ function test() {
          "browser.currentURI set to current entry in SH");
       is(browser.userTypedValue, "example.com",
          "userTypedValue was correctly restored");
-      is(browser.userTypedClear, 0,
+      ok(!browser.didStartLoadSinceLastUserTyping(),
-         "userTypeClear restored as expected");
+         "We still know that no load is ongoing");
       is(gURLBar.value, "example.com",
          "Address bar's value correctly restored to userTypedValue");
       runNextTest();
@@ -122,8 +122,8 @@ function test() {
          "browser.currentURI set to current entry in SH");
       is(browser.userTypedValue, "example.org",
          "userTypedValue was correctly restored");
-      is(browser.userTypedClear, 0,
+      ok(!browser.didStartLoadSinceLastUserTyping(),
-         "userTypeClear restored as expected");
+         "We still know that no load is ongoing");
       is(gURLBar.value, "example.org",
          "Address bar's value correctly restored to userTypedValue");
       runNextTest();
@@ -186,7 +186,8 @@ function test() {
       let browser = gBrowser.selectedBrowser;
       // Make sure this tab isn't loading and state is clear before we test.
       is(browser.userTypedValue, null, "userTypedValue is empty to start");
-      is(browser.userTypedClear, 0, "userTypedClear is 0 to start");
+      ok(!browser.didStartLoadSinceLastUserTyping(),
+         "Initially, no load should be ongoing");
 
       let inputText = "example.org";
       gURLBar.focus();
@@ -196,8 +197,8 @@ function test() {
       executeSoon(function () {
         is(browser.userTypedValue, "example.org",
            "userTypedValue was set when changing URLBar value");
-        is(browser.userTypedClear, 0,
+        ok(!browser.didStartLoadSinceLastUserTyping(),
-           "userTypedClear was not changed when changing URLBar value");
+           "No load started since changing URLBar value");
 
         // Now make sure ss gets these values too
         let newState = JSON.parse(ss.getBrowserState());
@@ -228,8 +229,8 @@ function test() {
          "userTypedClear=2 caused userTypedValue to be loaded");
       is(browser.userTypedValue, null,
          "userTypedValue was null after loading a URI");
-      is(browser.userTypedClear, 0,
+      ok(!browser.didStartLoadSinceLastUserTyping(),
-         "userTypeClear reset to 0");
+         "We should have reset the load state when the tab loaded");
       is(gURLBar.textValue, gURLBar.trimValue("http://example.com/"),
          "Address bar's value set after loading URI");
       runNextTest();

toolkit/content/widgets/browser.xml

@@ -62,15 +62,8 @@
         <body>
           <![CDATA[
             var webNavigation = this.webNavigation;
-            if (webNavigation.canGoBack) {
+            if (webNavigation.canGoBack)
-              try {
+              this._wrapURIChangeCall(() => webNavigation.goBack());
-                this.userTypedClear++;
-                this._wrapURIChangeCall(() => webNavigation.goBack());
-              } finally {
-                if (this.userTypedClear)
-                  this.userTypedClear--;
-              }
-            }
           ]]>
         </body>
       </method>
@@ -79,15 +72,8 @@
         <body>
           <![CDATA[
             var webNavigation = this.webNavigation;
-            if (webNavigation.canGoForward) {
+            if (webNavigation.canGoForward)
-              try {
+              this._wrapURIChangeCall(() => webNavigation.goForward());
-                this.userTypedClear++;
-                this._wrapURIChangeCall(() => webNavigation.goForward());
-              } finally {
-                if (this.userTypedClear)
-                  this.userTypedClear--;
-              }
-            }
           ]]>
         </body>
       </method>
@@ -162,18 +148,10 @@
               aPostData = params.postData;
             }
 
-            if (!(aFlags & this.webNavigation.LOAD_FLAGS_FROM_EXTERNAL))
-              this.userTypedClear++;
-
-            try {
             this._wrapURIChangeCall(() =>
               this.webNavigation.loadURIWithOptions(
                   aURI, aFlags, aReferrerURI, aReferrerPolicy,
                   aPostData, null, null));
-            } finally {
-              if (this.userTypedClear)
-                this.userTypedClear--;
-            }
           ]]>
         </body>
       </method>
@@ -215,13 +193,7 @@
         <parameter name="aIndex"/>
         <body>
           <![CDATA[
-            try {
+            this._wrapURIChangeCall(() => this.webNavigation.gotoIndex(aIndex));
-              this.userTypedClear++;
-              this._wrapURIChangeCall(() => this.webNavigation.gotoIndex(aIndex));
-            } finally {
-              if (this.userTypedClear)
-                this.userTypedClear--;
-            }
           ]]>
         </body>
       </method>
@@ -838,49 +810,29 @@
         ]]></body>
       </method>
 
-      <!--
+      <field name="urlbarChangeTracker">
-        This field tracks the location bar state. The value that the user typed
+        ({
-        in to the location bar may not be changed while this field is zero.
+          _startedLoadSinceLastUserTyping: false,
-        However invoking a load will temporarily increase this field to allow
-        the location bar to be updated to the new URL.
 
-        Case 1: Anchor scroll
+          startedLoad() {
-          The user appends the anchor to the URL. This sets the location bar
+            this._startedLoadSinceLastUserTyping = true;
-          into typed state, and disables changes to the location bar. The user
+          },
-          then requests the scroll. loadURIWithFlags temporarily increases the
+          finishedLoad() {
-          flag by 1 so that the anchor scroll's location change resets the
+            this._startedLoadSinceLastUserTyping = false;
-          location bar state.
+          },
-
+          userTyped() {
-        Case 2: Interrupted load
+            this._startedLoadSinceLastUserTyping = false;
-          The user types in and submits the URL. This triggers an asynchronous
+          },
-          network load which increases the flag by 2. (The temporary increase
+        })
-          from loadURIWithFlags is not noticeable in this case.) When the load
-          is interrupted the flag returns to zero, and the location bar stays
-          in typed state.
-
-        Case 3: New load
-          This works like case 2, but as the load is not interrupted the
-          location changes while the flag is still 2 thus resetting the
-          location bar state.
-
-        Case 4: Corrected load
-          This is a combination of case 2 and case 3, except that the original
-          load is interrupted by the new load. Normally cancelling and starting
-          a new load would reset the flag to 0 and then increase it to 2 again.
-          However both actions occur as a consequence of the loadURIWithFlags
-          invocation, which adds its temporary increase in to the mix. Since
-          the new URL would have been typed in the flag would have been reset
-          before loadURIWithFlags incremented it. The interruption resets the
-          flag to 0 and increases it to 2. Although loadURIWithFlags will
-          decrement the flag it remains at 1 thus allowing the location bar
-          state to be reset when the new load changes the location.
-          This case also applies when loading into a new browser, as this
-          interrupts the default load of about:blank.
-      -->
-      <field name="userTypedClear">
-        1
       </field>
 
+      <method name="didStartLoadSinceLastUserTyping">
+        <body><![CDATA[
+          return !this.inLoadURI &&
+                 this.urlbarChangeTracker._startedLoadSinceLastUserTyping;
+        ]]></body>
+      </method>
+
       <field name="_userTypedValue">
         null
       </field>
@@ -888,7 +840,7 @@
       <property name="userTypedValue"
                 onget="return this._userTypedValue;">
         <setter><![CDATA[
-          this.userTypedClear = 0;
+          this.urlbarChangeTracker.userTyped();
           this._userTypedValue = val;
           return val;
         ]]></setter>