Bug 1370518 - Don't completely detach/attach the autocomplete controller on TabSelect. r=mconley
authorMarco Bonardo <mbonardo@mozilla.com>
Tue, 06 Jun 2017 18:47:19 +0200
changeset 411458 650ad20e9fea9b94e64de273578eced87250deb7
parent 411457 d843dbc23708d592850e0381b8b9a872bc91345d
child 411459 44a734e1209d8c164a9b1b7e56fc9cb65440efab
push id7391
push usermtabara@mozilla.com
push dateMon, 12 Jun 2017 13:08:53 +0000
treeherdermozilla-beta@2191d7f87e2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmconley
bugs1370518
milestone55.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 1370518 - Don't completely detach/attach the autocomplete controller on TabSelect. r=mconley Due to recent changes to tabbrowser focus behavior, now the "focus" event to the location bar happens before the "TabSelect" event. On "focus" we would like to open the location bar popup, but detaching the controller would immediately close it. Thus we don't want "TabSelect" to detach the controller just to reset its internal state. Moreover, this should be cheaper. MozReview-Commit-ID: 5NZ1TTI9NFW
browser/base/content/test/urlbar/browser_urlbarSearchSuggestions_opt-out.js
browser/base/content/urlbarBindings.xml
toolkit/components/autocomplete/nsAutoCompleteController.cpp
toolkit/components/autocomplete/nsIAutoCompleteController.idl
--- a/browser/base/content/test/urlbar/browser_urlbarSearchSuggestions_opt-out.js
+++ b/browser/base/content/test/urlbar/browser_urlbarSearchSuggestions_opt-out.js
@@ -57,16 +57,32 @@ add_task(async function focus() {
   // Check the Change Options link.
   let changeOptionsLink = document.getElementById("search-suggestions-change-settings");
   let prefsPromise = BrowserTestUtils.waitForLocationChange(gBrowser, "about:preferences#general-search");
   changeOptionsLink.click();
   await prefsPromise;
   Assert.ok(!gURLBar.popup.popupOpen, "popup should be closed");
 });
 
+add_task(async function new_tab() {
+  // Opening a new tab when the urlbar is unfocused, should focusing it and thus
+  // open the popup in order to show the notification.
+  setupVisibleHint();
+  gURLBar.blur();
+  let popupPromise = promisePopupShown(gURLBar.popup);
+  // openNewForegroundTab doesn't focus the urlbar.
+  await BrowserTestUtils.synthesizeKey("t", { accelKey: true }, gBrowser.selectedBrowser);
+  await popupPromise;
+  Assert.ok(gURLBar.popup.popupOpen, "popup should be open");
+  assertVisible(true);
+  assertFooterVisible(false);
+  Assert.equal(gURLBar.popup._matchCount, 0, "popup should have no results");
+  await BrowserTestUtils.removeTab(gBrowser.selectedTab);
+  Assert.ok(!gURLBar.popup.popupOpen, "popup should be closed");
+});
 
 add_task(async function privateWindow() {
   // Since suggestions are disabled in private windows, the notification should
   // not appear even when suggestions are otherwise enabled.
   setupVisibleHint();
   let win = await BrowserTestUtils.openNewBrowserWindow({ private: true });
   await promiseAutocompleteResultPopup("foo", win);
   assertVisible(false, win);
--- a/browser/base/content/urlbarBindings.xml
+++ b/browser/base/content/urlbarBindings.xml
@@ -1158,18 +1158,17 @@ file, You can obtain one at http://mozil
               }
               this.setAttribute("textoverflow", "true");
               break;
             case "underflow":
               this.removeAttribute("textoverflow");
               this._hideURLTooltip();
               break;
             case "TabSelect":
-              this.detachController();
-              this.attachController();
+              this.controller.resetInternalState();
               break;
           }
         ]]></body>
       </method>
 
       <!--
         onBeforeTextValueSet is called by the base-binding's .textValue getter.
         It should return the value that the getter should use.
--- a/toolkit/components/autocomplete/nsAutoCompleteController.cpp
+++ b/toolkit/components/autocomplete/nsAutoCompleteController.cpp
@@ -15,19 +15,22 @@
 #include "nsUnicharUtils.h"
 #include "nsIScriptSecurityManager.h"
 #include "nsITreeBoxObject.h"
 #include "nsITreeColumns.h"
 #include "nsIObserverService.h"
 #include "nsIDOMKeyEvent.h"
 #include "mozilla/Services.h"
 #include "mozilla/ModuleUtils.h"
+#include "mozilla/Unused.h"
 
 static const char *kAutoCompleteSearchCID = "@mozilla.org/autocomplete/search;1?name=";
 
+using namespace mozilla;
+
 namespace {
 
 void
 SetTextValue(nsIAutoCompleteInput* aInput,
              const nsString& aValue,
              uint16_t aReason) {
   nsresult rv = aInput->SetTextValueWithReason(aValue, aReason);
   if (NS_FAILED(rv)) {
@@ -125,63 +128,52 @@ nsAutoCompleteController::SetInitiallySe
 
 NS_IMETHODIMP
 nsAutoCompleteController::SetInput(nsIAutoCompleteInput *aInput)
 {
   // Don't do anything if the input isn't changing.
   if (mInput == aInput)
     return NS_OK;
 
-  // Clear out the current search context
+  Unused << ResetInternalState();
   if (mInput) {
-    // Stop all searches in case they are async.
-    StopSearch();
-    ClearResults();
+    mSearches.Clear();
     ClosePopup();
-    mSearches.Clear();
   }
 
   mInput = aInput;
 
   // Nothing more to do if the input was just being set to null.
-  if (!aInput)
+  if (!mInput) {
     return NS_OK;
+  }
+  nsCOMPtr<nsIAutoCompleteInput> input(mInput);
 
-  nsAutoString newValue;
-  aInput->GetTextValue(newValue);
+  // Reset the current search string.
+  input->GetTextValue(mSearchString);
 
   // Clear out this reference in case the new input's popup has no tree
   mTree = nullptr;
 
-  // Reset all search state members to default values
-  mSearchString = newValue;
-  mPlaceholderCompletionString.Truncate();
-  mDefaultIndexCompleted = false;
-  mProhibitAutoFill = false;
-  mSearchStatus = nsIAutoCompleteController::STATUS_NONE;
-  mRowCount = 0;
-  mSearchesOngoing = 0;
-  mCompletedSelectionIndex = -1;
-
   // Initialize our list of search objects
   uint32_t searchCount;
-  aInput->GetSearchCount(&searchCount);
+  input->GetSearchCount(&searchCount);
   mResults.SetCapacity(searchCount);
   mSearches.SetCapacity(searchCount);
   mImmediateSearchesCount = 0;
 
   const char *searchCID = kAutoCompleteSearchCID;
 
   // Since the controller can be used as a service it's important to reset this.
   mClearingAutoFillSearchesAgain = false;
 
   for (uint32_t i = 0; i < searchCount; ++i) {
     // Use the search name to create the contract id string for the search service
     nsAutoCString searchName;
-    aInput->GetSearchAt(i, searchName);
+    input->GetSearchAt(i, searchName);
     nsAutoCString cid(searchCID);
     cid.Append(searchName);
 
     // Use the created cid to get a pointer to the search service and store it for later
     nsCOMPtr<nsIAutoCompleteSearch> search = do_GetService(cid.get());
     if (search) {
       mSearches.AppendObject(search);
 
@@ -201,16 +193,39 @@ nsAutoCompleteController::SetInput(nsIAu
       }
     }
   }
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
+nsAutoCompleteController::ResetInternalState()
+{
+  // Clear out the current search context
+  if (mInput) {
+    nsAutoString value;
+    mInput->GetTextValue(value);
+    // Stop all searches in case they are async.
+    Unused << StopSearch();
+    Unused << ClearResults();
+    mSearchString = value;
+  }
+
+  mPlaceholderCompletionString.Truncate();
+  mDefaultIndexCompleted = false;
+  mProhibitAutoFill = false;
+  mSearchStatus = nsIAutoCompleteController::STATUS_NONE;
+  mRowCount = 0;
+  mCompletedSelectionIndex = -1;
+
+  return NS_OK;
+}
+
+NS_IMETHODIMP
 nsAutoCompleteController::StartSearch(const nsAString &aSearchString)
 {
   mSearchString = aSearchString;
   StartSearches();
   return NS_OK;
 }
 
 NS_IMETHODIMP
@@ -1538,18 +1553,17 @@ nsAutoCompleteController::EnterMatch(boo
             result->GetFinalCompleteValueAt(defaultIndex, value);
             break;
           }
         }
       }
     }
   }
 
-  nsCOMPtr<nsIObserverService> obsSvc =
-    mozilla::services::GetObserverService();
+  nsCOMPtr<nsIObserverService> obsSvc = services::GetObserverService();
   NS_ENSURE_STATE(obsSvc);
   obsSvc->NotifyObservers(input, "autocomplete-will-enter-text", nullptr);
 
   if (!value.IsEmpty()) {
     SetTextValue(input, value, nsIAutoCompleteInput::TEXTVALUE_REASON_ENTERMATCH);
     input->SelectTextRange(value.Length(), value.Length());
     mSearchString = value;
   }
@@ -1574,18 +1588,17 @@ nsAutoCompleteController::RevertTextValu
 
   nsAutoString oldValue(mSearchString);
   nsCOMPtr<nsIAutoCompleteInput> input(mInput);
 
   bool cancel = false;
   input->OnTextReverted(&cancel);
 
   if (!cancel) {
-    nsCOMPtr<nsIObserverService> obsSvc =
-      mozilla::services::GetObserverService();
+    nsCOMPtr<nsIObserverService> obsSvc = services::GetObserverService();
     NS_ENSURE_STATE(obsSvc);
     obsSvc->NotifyObservers(input, "autocomplete-will-revert-text", nullptr);
 
     nsAutoString inputValue;
     input->GetTextValue(inputValue);
     // Don't change the value if it is the same to prevent sending useless events.
     // NOTE: how can |RevertTextValue| be called with inputValue != oldValue?
     if (!oldValue.Equals(inputValue)) {
--- a/toolkit/components/autocomplete/nsIAutoCompleteController.idl
+++ b/toolkit/components/autocomplete/nsIAutoCompleteController.idl
@@ -165,9 +165,16 @@ interface nsIAutoCompleteController : ns
    * This should be used when a search wants to pre-select an element before
    * the user starts using results.
    *
    * @note Setting this is not the same as just setting selectedIndex in
    * nsIAutocompletePopup, since this will take care of updating any internal
    * tracking variables of features like completeSelectedIndex.
    */
   void setInitiallySelectedIndex(in long index);
+
+  /*
+   * Reset controller internal caches for cases where the input doesn't change
+   * but its context resets, thus it is about to start a completely new search
+   * session.
+   */
+  void resetInternalState();
 };