Backed out changeset 9242458b79d5 (bug 1539804) for high frequency browser/browser_urlbarStopSearchOnSelection.js failures CLOSED TREE
authorCiure Andrei <aciure@mozilla.com>
Wed, 01 May 2019 14:45:59 +0300
changeset 472066 99548c52b81f7b1ff00f0fef2eb713911ab4e830
parent 472065 9f1a8afe2391904da48baaca3066f8d25bfd80e9
child 472067 6843d141c496ade289d5d162532d9fc6d479e78c
push id35946
push userapavel@mozilla.com
push dateWed, 01 May 2019 15:54:31 +0000
treeherdermozilla-central@a027a998b8b7 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs1539804
milestone68.0a1
backs out9242458b79d5ae6e9d48c9a2a591680d79c13211
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
Backed out changeset 9242458b79d5 (bug 1539804) for high frequency browser/browser_urlbarStopSearchOnSelection.js failures CLOSED TREE
browser/components/search/content/search-one-offs.js
browser/components/urlbar/UrlbarController.jsm
browser/components/urlbar/UrlbarProviderUnifiedComplete.jsm
browser/components/urlbar/tests/browser/browser.ini
browser/components/urlbar/tests/browser/browser_urlbarStopSearchOnSelection.js
browser/components/urlbar/tests/browser/searchSuggestionEngineSlow.xml
--- a/browser/components/search/content/search-one-offs.js
+++ b/browser/components/search/content/search-one-offs.js
@@ -824,18 +824,18 @@ class SearchOneOffs {
 
   /**
    * This handles key presses specific to the one-off buttons like Tab and
    * Alt+Up/Down, and Up/Down keys within the buttons.  Since one-off buttons
    * are always used in conjunction with a list of some sort (in this.popup),
    * it also handles Up/Down keys that cross the boundaries between list
    * items and the one-off buttons.
    *
-   * If this method handles the key press, then it will call
-   * event.preventDefault() and return true.
+   * If this method handles the key press, then event.defaultPrevented will
+   * be true when it returns.
    *
    * @param {Event} event
    *        The key event.
    * @param {number} numListItems
    *        The number of items in the list.  The reason that this is a
    *        parameter at all is that the list may contain items at the end
    *        that should be ignored, depending on the consumer.  That's true
    *        for the urlbar for example.
@@ -844,30 +844,28 @@ class SearchOneOffs {
    *        buttons contains a selection.  Pass false if either the list or
    *        the one-off buttons (or both) should always contain a selection.
    * @param {string} [textboxUserValue]
    *        When the last list item is selected and the user presses Down,
    *        the first one-off becomes selected and the textbox value is
    *        restored to the value that the user typed.  Pass that value here.
    *        However, if you pass true for allowEmptySelection, you don't need
    *        to pass anything for this parameter.  (Pass undefined or null.)
-   * @returns {boolean} True if the one-offs handled the key press.
    */
   handleKeyPress(event, numListItems, allowEmptySelection, textboxUserValue) {
     if (!this.popup) {
-      return false;
+      return;
     }
     let handled = this._handleKeyPress(event, numListItems,
       allowEmptySelection,
       textboxUserValue);
     if (handled) {
       event.preventDefault();
       event.stopPropagation();
     }
-    return handled;
   }
 
   _handleKeyPress(event, numListItems, allowEmptySelection, textboxUserValue) {
     if (this.compact && this.buttons.collapsed) {
       return false;
     }
     if (event.keyCode == KeyEvent.DOM_VK_RIGHT &&
         this.selectedButton &&
--- a/browser/components/urlbar/UrlbarController.jsm
+++ b/browser/components/urlbar/UrlbarController.jsm
@@ -253,22 +253,22 @@ class UrlbarController {
       }
       event.preventDefault();
       return;
     }
 
     if (this.view.isOpen && executeAction) {
       let queryContext = this._lastQueryContext;
       if (queryContext) {
-        let handled = this.view.oneOffSearchButtons.handleKeyPress(
+        this.view.oneOffSearchButtons.handleKeyPress(
           event,
           queryContext.results.length,
           this.view.allowEmptySelection,
           queryContext.searchString);
-        if (handled) {
+        if (event.defaultPrevented) {
           return;
         }
       }
     }
 
     switch (event.keyCode) {
       case KeyEvent.DOM_VK_ESCAPE:
         if (executeAction) {
--- a/browser/components/urlbar/UrlbarProviderUnifiedComplete.jsm
+++ b/browser/components/urlbar/UrlbarProviderUnifiedComplete.jsm
@@ -122,22 +122,20 @@ class ProviderUnifiedComplete extends Ur
     await new Promise(resolve => {
       let listener = {
         onSearchResult(_, result) {
           let {done, matches} = convertResultToMatches(queryContext, result, urls);
           for (let match of matches) {
             addCallback(UrlbarProviderUnifiedComplete, match);
           }
           if (done) {
-            delete this._resolveSearch;
             resolve();
           }
         },
       };
-      this._resolveSearch = resolve;
       unifiedComplete.startSearch(queryContext.searchString,
                                   params.join(" "),
                                   null, // previousResult
                                   listener);
     });
 
     // We are done.
     this.queries.delete(queryContext);
@@ -147,19 +145,16 @@ class ProviderUnifiedComplete extends Ur
    * Cancels a running query.
    * @param {object} queryContext The query context object
    */
   cancelQuery(queryContext) {
     logger.info(`Canceling query for ${queryContext.searchString}`);
     // This doesn't properly support being used concurrently by multiple fields.
     this.queries.delete(queryContext);
     unifiedComplete.stopSearch();
-    if (this._resolveSearch) {
-      this._resolveSearch();
-    }
   }
 }
 
 var UrlbarProviderUnifiedComplete = new ProviderUnifiedComplete();
 
 /**
  * Convert from a nsIAutocompleteResult to a list of new matches.
  * Note that at every call we get the full set of matches, included the
--- a/browser/components/urlbar/tests/browser/browser.ini
+++ b/browser/components/urlbar/tests/browser/browser.ini
@@ -161,16 +161,17 @@ support-files =
 [browser_urlbarSearchSuggestions.js]
 support-files =
   searchSuggestionEngine.xml
   searchSuggestionEngine.sjs
 [browser_URLBarSetURI.js]
 skip-if = (os == "linux" || os == "mac") && debug # bug 970052, bug 970053
 [browser_urlbarStop.js]
 [browser_urlbarStopSearchOnSelection.js]
+skip-if = true # Bug 1524510 - Not implemented yet.
 support-files =
   searchSuggestionEngineSlow.xml
   searchSuggestionEngine.sjs
 [browser_urlbarTokenAlias.js]
 [browser_urlbarUpdateForDomainCompletion.js]
 [browser_urlbarValueOnTabSwitch.js]
 [browser_userTypedValue.js]
 support-files = file_userTypedValue.html
--- a/browser/components/urlbar/tests/browser/browser_urlbarStopSearchOnSelection.js
+++ b/browser/components/urlbar/tests/browser/browser_urlbarStopSearchOnSelection.js
@@ -8,17 +8,17 @@
  */
 
 "use strict";
 
 const TEST_ENGINE_BASENAME = "searchSuggestionEngineSlow.xml";
 
 // This should match the `timeout` query param used in the suggestions URL in
 // the test engine.
-const TEST_ENGINE_SUGGESTIONS_TIMEOUT = 3000;
+const TEST_ENGINE_SUGGESTIONS_TIMEOUT = 1000;
 
 // The number of suggestions returned by the test engine.
 const TEST_ENGINE_NUM_EXPECTED_RESULTS = 2;
 
 add_task(async function init() {
   await PlacesUtils.history.clear();
   await SpecialPowers.pushPrefEnv({
     set: [["browser.urlbar.suggest.searches", true]],
@@ -31,73 +31,53 @@ add_task(async function init() {
   await Services.search.setDefault(engine);
   registerCleanupFunction(async () => {
     await Services.search.setDefault(oldDefaultEngine);
     await PlacesUtils.history.clear();
   });
 });
 
 add_task(async function mainTest() {
-  // Open a tab that will match the search string below so that we're guaranteed
-  // to have more than one result (the heuristic result) so that we can change
-  // the selected result.  We open a tab instead of adding a page in history
-  // because open tabs are kept in a memory SQLite table, so open-tab results
-  // are more likely than history results to be fetched before our slow search
-  // suggestions.  This is important when the test runs on slow debug builds on
-  // slow machines.
-  await BrowserTestUtils.withNewTab("http://example.com/", async () => {
-    await BrowserTestUtils.withNewTab("about:blank", async () => {
-      // Do an initial search.  There should be 4 results: heuristic, open tab,
-      // and the two suggestions.
-      await promiseAutocompleteResultPopup("amp");
-      await TestUtils.waitForCondition(() => {
-        return UrlbarTestUtils.getResultCount(window) ==
-               2 + TEST_ENGINE_NUM_EXPECTED_RESULTS;
-      });
+  // Trigger an initial search.  Restrict matches to search suggestions.
+  await promiseAutocompleteResultPopup(`${UrlbarTokenizer.RESTRICT.SEARCH} test`, window);
+  await promiseSuggestionsPresent("Waiting for initial suggestions");
+
+  // Now synthesize typing a character.  promiseAutocompleteResultPopup doesn't
+  // work in this case because it causes the popup to close and re-open with the
+  // new input text.
+  await new Promise(r => EventUtils.synthesizeKey("x", {}, window, r));
 
-      // Type a character to start a new search.  The new search should still
-      // match the open tab so that the open-tab result appears again.
-      EventUtils.synthesizeKey("l");
+  // At this point, a new search starts, the autocomplete's _invalidate is
+  // called, _appendCurrentResult is called, and matchCount remains 3 from the
+  // previous search (the heuristic result plus the two search suggestions).
+  // The suggestion results should not outwardly change, however, since the
+  // autocomplete controller should still be returning the initial suggestions.
+  // What has changed, though, is the search string, which is kept in the
+  // results' ac-text attributes.  Call waitForAutocompleteResultAt to wait for
+  // the results' ac-text to be set to the new search string, "testx", to make
+  // sure the autocomplete has seen the new search.
+  await waitForAutocompleteResultAt(TEST_ENGINE_NUM_EXPECTED_RESULTS);
 
-      // There should be 2 results immediately: heuristic and open tab.  (On
-      // quantumbar, there will be only 2, but on awesomebar, there will be 4
-      // since awesomebar will keep showing the excess old results for a bit.)
-      await TestUtils.waitForCondition(() => {
-        return UrlbarTestUtils.getResultCount(window) >= 2;
-      });
+  // Now press the Down arrow key to change the selection.
+  await new Promise(r => EventUtils.synthesizeKey("VK_DOWN", {}, window, r));
 
-      // Before the search completes, change the selected result.  Pressing only
-      // the down arrow key ends up selecting the first one-off on Linux debug
-      // builds on the infrastructure for some reason, so arrow back up to
-      // select the heuristic result again.  The important thing is to change
-      // the selection.  It doesn't matter which result ends up selected.
-      EventUtils.synthesizeKey("KEY_ArrowDown");
-      EventUtils.synthesizeKey("KEY_ArrowUp");
+  // Changing the selection should have stopped the new search triggered by
+  // typing the "x" character.  Wait a bit to make sure it really stopped.
+  await new Promise(r => setTimeout(r, 2 * TEST_ENGINE_SUGGESTIONS_TIMEOUT));
 
-      // Wait for the new search to complete.  It should be canceled due to the
-      // selection change, but it should still complete.
-      await promiseSearchComplete();
-
-      // To make absolutely sure the suggestions don't appear after the search
-      // completes, wait a bit.
-      await new Promise(r => setTimeout(r, 1 + TEST_ENGINE_SUGGESTIONS_TIMEOUT));
+  // Both of the suggestion results should retain their initial values,
+  // "testfoo" and "testbar".  They should *not* be "testxfoo" and "textxbar".
 
-      // The heuristic result should reflect the new search, "ampl".
-      let result = await UrlbarTestUtils.getDetailsOfResultAt(window, 0);
-      Assert.equal(result.type, UrlbarUtils.RESULT_TYPE.SEARCH,
-        "Should have the correct result type");
-      Assert.equal(result.searchParams.query, "ampl",
-        "Should have the correct query");
+  // + 1 for the heuristic result
+  let numExpectedResults = TEST_ENGINE_NUM_EXPECTED_RESULTS + 1;
+  Assert.equal(UrlbarTestUtils.getResultCount(window), numExpectedResults,
+    "Should have the correct number of results displayed");
 
-      // None of the other results should be "ampl" suggestions, i.e., amplfoo
-      // and amplbar should not be in the results.
-      let count = UrlbarTestUtils.getResultCount(window);
-      for (let i = 1; i < count; i++) {
-        result = await UrlbarTestUtils.getDetailsOfResultAt(window, i);
-        Assert.ok(
-          result.type != UrlbarUtils.RESULT_TYPE.SEARCH ||
-          !["amplfoo", "amplbar"].includes(result.searchParams.suggestion),
-          "Suggestions should not contain the typed l char"
-        );
-      }
-    });
-  });
+  let expectedSuggestions = ["testfoo", "testbar"];
+  for (let i = 0; i < TEST_ENGINE_NUM_EXPECTED_RESULTS; i++) {
+    // + 1 to skip the heuristic result
+    let result = await UrlbarTestUtils.getDetailsOfResultAt(window, i + 1);
+    Assert.equal(result.type, UrlbarUtils.RESULT_TYPE.SEARCH,
+      "Should have the correct result type");
+    Assert.equal(result.searchParams.suggestion, expectedSuggestions[i],
+      "Should have the correct suggestion");
+  }
 });
--- a/browser/components/urlbar/tests/browser/searchSuggestionEngineSlow.xml
+++ b/browser/components/urlbar/tests/browser/searchSuggestionEngineSlow.xml
@@ -1,11 +1,11 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <!-- Any copyright is dedicated to the Public Domain.
    - http://creativecommons.org/publicdomain/zero/1.0/ -->
 
 <SearchPlugin xmlns="http://www.mozilla.org/2006/browser/search/">
 <ShortName>searchSuggestionEngineSlow.xml</ShortName>
-<Url type="application/x-suggestions+json" method="GET" template="http://mochi.test:8888/browser/browser/components/urlbar/tests/browser/searchSuggestionEngine.sjs?query={searchTerms}&amp;timeout=3000"/>
+<Url type="application/x-suggestions+json" method="GET" template="http://mochi.test:8888/browser/browser/components/urlbar/tests/browser/searchSuggestionEngine.sjs?query={searchTerms}&amp;timeout=1000"/>
 <Url type="text/html" method="GET" template="http://mochi.test:8888/" rel="searchform">
   <Param name="terms" value="{searchTerms}"/>
 </Url>
 </SearchPlugin>