Bug 1034381 - Enhance previous searches in awesomebar dropdown by removing URL. r=mak
authorPaolo Amadini <paolo.mozmail@amadzone.org>
Fri, 01 Aug 2014 16:41:32 +0100
changeset 197190 55f293b3d8163f18982aa007d46b75de4ddc78fb
parent 197189 2bbd5e4afe2d94306d6a4a60694989a7efadd7fd
child 197191 e3e739ad7b5503f973dfea8fbe891ef1a995f575
push id8015
push userpaolo.mozmail@amadzone.org
push dateFri, 01 Aug 2014 15:42:15 +0000
treeherderfx-team@55f293b3d816 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1034381
milestone34.0a1
Bug 1034381 - Enhance previous searches in awesomebar dropdown by removing URL. r=mak
toolkit/components/places/UnifiedComplete.js
toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js
toolkit/components/places/tests/unifiedcomplete/test_searchEngine.js
toolkit/components/places/tests/unifiedcomplete/xpcshell.ini
toolkit/content/widgets/autocomplete.xml
--- a/toolkit/components/places/UnifiedComplete.js
+++ b/toolkit/components/places/UnifiedComplete.js
@@ -17,35 +17,35 @@ const Cu = Components.utils;
 const TOPIC_SHUTDOWN = "places-shutdown";
 const TOPIC_PREFCHANGED = "nsPref:changed";
 
 const DEFAULT_BEHAVIOR = 0;
 
 const PREF_BRANCH = "browser.urlbar.";
 
 // Prefs are defined as [pref name, default value].
-const PREF_ENABLED =            [ "autocomplete.enabled", true ];
-const PREF_AUTOFILL =           [ "autoFill",             true ];
-const PREF_AUTOFILL_TYPED =     [ "autoFill.typed",       true ];
-const PREF_AUTOFILL_PRIORITY =  [ "autoFill.priority",    true ];
-const PREF_DELAY =              [ "delay",                  50 ];
-const PREF_BEHAVIOR =           [ "matchBehavior", MATCH_BOUNDARY_ANYWHERE ];
-const PREF_DEFAULT_BEHAVIOR =   [ "default.behavior", DEFAULT_BEHAVIOR ];
-const PREF_EMPTY_BEHAVIOR =     [ "default.behavior.emptyRestriction",
-                                  Ci.mozIPlacesAutoComplete.BEHAVIOR_HISTORY |
-                                  Ci.mozIPlacesAutoComplete.BEHAVIOR_TYPED ];
-const PREF_FILTER_JS =          [ "filter.javascript",    true ];
-const PREF_MAXRESULTS =         [ "maxRichResults",         25 ];
-const PREF_RESTRICT_HISTORY =   [ "restrict.history",      "^" ];
-const PREF_RESTRICT_BOOKMARKS = [ "restrict.bookmark",     "*" ];
-const PREF_RESTRICT_TYPED =     [ "restrict.typed",        "~" ];
-const PREF_RESTRICT_TAG =       [ "restrict.tag",          "+" ];
-const PREF_RESTRICT_SWITCHTAB = [ "restrict.openpage",     "%" ];
-const PREF_MATCH_TITLE =        [ "match.title",           "#" ];
-const PREF_MATCH_URL =          [ "match.url",             "@" ];
+const PREF_ENABLED =                [ "autocomplete.enabled",   true ];
+const PREF_AUTOFILL =               [ "autoFill",               true ];
+const PREF_AUTOFILL_TYPED =         [ "autoFill.typed",         true ];
+const PREF_AUTOFILL_SEARCHENGINES = [ "autoFill.searchEngines", true ];
+const PREF_DELAY =                  [ "delay",                  50 ];
+const PREF_BEHAVIOR =               [ "matchBehavior", MATCH_BOUNDARY_ANYWHERE ];
+const PREF_DEFAULT_BEHAVIOR =       [ "default.behavior", DEFAULT_BEHAVIOR ];
+const PREF_EMPTY_BEHAVIOR =         [ "default.behavior.emptyRestriction",
+                                      Ci.mozIPlacesAutoComplete.BEHAVIOR_HISTORY |
+                                      Ci.mozIPlacesAutoComplete.BEHAVIOR_TYPED ];
+const PREF_FILTER_JS =              [ "filter.javascript",      true ];
+const PREF_MAXRESULTS =             [ "maxRichResults",         25 ];
+const PREF_RESTRICT_HISTORY =       [ "restrict.history",       "^" ];
+const PREF_RESTRICT_BOOKMARKS =     [ "restrict.bookmark",      "*" ];
+const PREF_RESTRICT_TYPED =         [ "restrict.typed",         "~" ];
+const PREF_RESTRICT_TAG =           [ "restrict.tag",           "+" ];
+const PREF_RESTRICT_SWITCHTAB =     [ "restrict.openpage",      "%" ];
+const PREF_MATCH_TITLE =            [ "match.title",            "#" ];
+const PREF_MATCH_URL =              [ "match.url",              "@" ];
 
 // Match type constants.
 // These indicate what type of search function we should be using.
 const MATCH_ANYWHERE = Ci.mozIPlacesAutoComplete.MATCH_ANYWHERE;
 const MATCH_BOUNDARY_ANYWHERE = Ci.mozIPlacesAutoComplete.MATCH_BOUNDARY_ANYWHERE;
 const MATCH_BOUNDARY = Ci.mozIPlacesAutoComplete.MATCH_BOUNDARY;
 const MATCH_BEGINNING = Ci.mozIPlacesAutoComplete.MATCH_BEGINNING;
 const MATCH_BEGINNING_CASE_SENSITIVE = Ci.mozIPlacesAutoComplete.MATCH_BEGINNING_CASE_SENSITIVE;
@@ -57,21 +57,24 @@ const QUERYTYPE_FILTERED      = 1;
 const QUERYTYPE_AUTOFILL_HOST = 2;
 const QUERYTYPE_AUTOFILL_URL  = 3;
 
 // This separator is used as an RTL-friendly way to split the title and tags.
 // It can also be used by an nsIAutoCompleteResult consumer to re-split the
 // "comment" back into the title and the tag.
 const TITLE_TAGS_SEPARATOR = " \u2013 ";
 
+// This separator identifies the search engine name in the title.
+const TITLE_SEARCH_ENGINE_SEPARATOR = " \u00B7\u2013\u00B7 ";
+
 // Telemetry probes.
 const TELEMETRY_1ST_RESULT = "PLACES_AUTOCOMPLETE_1ST_RESULT_TIME_MS";
 
-// The default frecency value used when inserting priority results.
-const FRECENCY_PRIORITY_DEFAULT = 1000;
+// The default frecency value used when inserting search engine results.
+const FRECENCY_SEARCHENGINES_DEFAULT = 1000;
 
 // Sqlite result row index constants.
 const QUERYINDEX_QUERYTYPE     = 0;
 const QUERYINDEX_URL           = 1;
 const QUERYINDEX_TITLE         = 2;
 const QUERYINDEX_ICONURL       = 3;
 const QUERYINDEX_BOOKMARKED    = 4;
 const QUERYINDEX_BOOKMARKTITLE = 5;
@@ -354,17 +357,17 @@ XPCOMUtils.defineLazyGetter(this, "Switc
  */
 XPCOMUtils.defineLazyGetter(this, "Prefs", () => {
   let prefs = new Preferences(PREF_BRANCH);
 
   function loadPrefs() {
     store.enabled = prefs.get(...PREF_ENABLED);
     store.autofill = prefs.get(...PREF_AUTOFILL);
     store.autofillTyped = prefs.get(...PREF_AUTOFILL_TYPED);
-    store.autofillPriority = prefs.get(...PREF_AUTOFILL_PRIORITY);
+    store.autofillSearchEngines = prefs.get(...PREF_AUTOFILL_SEARCHENGINES);
     store.delay = prefs.get(...PREF_DELAY);
     store.matchBehavior = prefs.get(...PREF_BEHAVIOR);
     store.filterJavaScript = prefs.get(...PREF_FILTER_JS);
     store.maxRichResults = prefs.get(...PREF_MAXRESULTS);
     store.restrictHistoryToken = prefs.get(...PREF_RESTRICT_HISTORY);
     store.restrictBookmarkToken = prefs.get(...PREF_RESTRICT_BOOKMARKS);
     store.restrictTypedToken = prefs.get(...PREF_RESTRICT_TYPED);
     store.restrictTagToken = prefs.get(...PREF_RESTRICT_TAG);
@@ -624,18 +627,22 @@ Search.prototype = {
    * Execute the search and populate results.
    * @param conn
    *        The Sqlite connection.
    */
   execute: Task.async(function* (conn) {
     this._pendingQuery = true;
     TelemetryStopwatch.start(TELEMETRY_1ST_RESULT);
 
+    // Since we call the synchronous parseSubmissionURL function later, we must
+    // wait for the initialization of PlacesSearchAutocompleteProvider first.
+    yield PlacesSearchAutocompleteProvider.ensureInitialized();
+
     // For any given search, we run many queries:
-    // 1) priority domains
+    // 1) search engine domains
     // 2) inline completion
     // 3) keywords (this._keywordQuery)
     // 4) adaptive learning (this._adaptiveQuery)
     // 5) open pages not supported by history (this._switchToTabQuery)
     // 6) query based on match behavior
     //
     // (3) only gets ran if we get any filtered tokens, since if there are no
     // tokens, there is nothing to match.
@@ -644,17 +651,17 @@ Search.prototype = {
     let queries = [ this._adaptiveQuery,
                     this._switchToTabQuery,
                     this._searchQuery ];
 
     if (this._searchTokens.length > 0 &&
         PlacesUtils.bookmarks.getURIForKeyword(this._searchTokens[0])) {
       queries.unshift(this._keywordQuery);
     } else if (this._searchTokens.length == 1) {
-      yield this._matchPriorityUrl();
+      yield this._matchSearchEngineUrl();
     }
 
     if (this._shouldAutofill) {
       // Hosts have no "/" in them.
       let lastSlashIndex = this._searchString.lastIndexOf("/");
       // Search only URLs if there's a slash in the search string...
       if (lastSlashIndex != -1) {
         // ...but not if it's exactly at the end of the search string.
@@ -695,32 +702,31 @@ Search.prototype = {
 
     // If we didn't find enough matches and we have some frecency-driven
     // matches, add them.
     if (this._frecencyMatches) {
       this._frecencyMatches.forEach(this._addMatch, this);
     }
   }),
 
-  _matchPriorityUrl: function* () {
-    if (!Prefs.autofillPriority)
+  _matchSearchEngineUrl: function* () {
+    if (!Prefs.autofillSearchEngines)
       return;
 
-    // Handle priority matches for search engine domains.
-    let priorityMatch =
-        yield PlacesSearchAutocompleteProvider.findMatchByToken(this._searchString);
-    if (priorityMatch) {
+    let match = yield PlacesSearchAutocompleteProvider.findMatchByToken(
+                                                           this._searchString);
+    if (match) {
       this._result.setDefaultIndex(0);
       this._addFrecencyMatch({
-        value: priorityMatch.token,
-        comment: priorityMatch.engineName,
-        icon: priorityMatch.iconUrl,
+        value: match.token,
+        comment: match.engineName,
+        icon: match.iconUrl,
         style: "priority-search",
-        finalCompleteValue: priorityMatch.url,
-        frecency: FRECENCY_PRIORITY_DEFAULT
+        finalCompleteValue: match.url,
+        frecency: FRECENCY_SEARCHENGINES_DEFAULT
       });
     }
   },
 
   _onResultRow: function (row) {
     TelemetryStopwatch.finish(TELEMETRY_1ST_RESULT);
     let queryType = row.getResultByIndex(QUERYINDEX_QUERYTYPE);
     let match;
@@ -749,16 +755,40 @@ Search.prototype = {
       this._frecencyMatches = [];
     this._frecencyMatches.push(match);
     // We keep this array in reverse order, so we can walk it and remove stuff
     // from it in one pass.  Notice that for frecency reverse order means from
     // lower to higher.
     this._frecencyMatches.sort((a, b) => a.frecency - b.frecency);
   },
 
+  _maybeRestyleSearchMatch: function (match) {
+    // Return if the URL does not represent a search result.
+    let parseResult =
+      PlacesSearchAutocompleteProvider.parseSubmissionURL(match.value);
+    if (!parseResult) {
+      return;
+    }
+
+    // Do not apply the special style if the user is doing a search from the
+    // location bar but the entered terms match an irrelevant portion of the
+    // URL. For example, "https://www.google.com/search?q=terms&client=firefox"
+    // when searching for "Firefox".
+    let terms = parseResult.terms.toLowerCase();
+    if (this._searchTokens.length > 0 &&
+        this._searchTokens.every(token => terms.indexOf(token) == -1)) {
+      return;
+    }
+
+    // Use the special separator that the binding will use to style the item.
+    match.style = "search " + match.style;
+    match.comment = parseResult.terms + TITLE_SEARCH_ENGINE_SEPARATOR +
+                    parseResult.engineName;
+  },
+
   _addMatch: function (match) {
     let notifyResults = false;
 
     if (this._frecencyMatches) {
       for (let i = this._frecencyMatches.length - 1;  i >= 0 ; i--) {
         if (this._frecencyMatches[i].frecency > match.frecency) {
           this._addMatch(this._frecencyMatches.splice(i, 1)[0]);
         }
@@ -774,20 +804,29 @@ Search.prototype = {
       // Not all entries have a place id, thus we fallback to the url for them.
       // We cannot use only the url since keywords entries are modified to
       // include the search string, and would be returned multiple times.  Ids
       // are faster too.
       if (match.placeId)
         this._usedPlaceIds.add(match.placeId);
       this._usedURLs.add(urlMapKey);
 
+      if (!match.style) {
+        match.style = "favicon";
+      }
+
+      // Restyle past searches, unless they are bookmarks or special results.
+      if (match.style == "favicon") {
+        this._maybeRestyleSearchMatch(match);
+      }
+
       this._result.appendMatch(match.value,
                                match.comment,
                                match.icon || PlacesUtils.favicons.defaultFavicon.spec,
-                               match.style || "favicon",
+                               match.style,
                                match.finalCompleteValue);
       notifyResults = true;
     }
 
     if (this._result.matchCount == Prefs.maxRichResults || !this.pending) {
       // We have enough results, so stop running our search.
       this.cancel();
       // This tells Sqlite.jsm to stop providing us results and cancel the
--- a/toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js
+++ b/toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js
@@ -12,16 +12,18 @@ Cu.import("resource://gre/modules/Servic
 // Import common head.
 let (commonFile = do_get_file("../head_common.js", false)) {
   let uri = Services.io.newFileURI(commonFile);
   Services.scriptloader.loadSubScript(uri.spec, this);
 }
 
 // Put any other stuff relative to this test folder below.
 
+const TITLE_SEARCH_ENGINE_SEPARATOR = " \u00B7\u2013\u00B7 ";
+
 function run_test() {
   run_next_test();
 }
 
 function* cleanup() {
   Services.prefs.clearUserPref("browser.urlbar.autocomplete.enabled");
   Services.prefs.clearUserPref("browser.urlbar.autoFill");
   Services.prefs.clearUserPref("browser.urlbar.autoFill.typed");
@@ -142,19 +144,21 @@ function* check_autocomplete(test) {
       let comment = controller.getCommentAt(i);
       do_log_info("Looking for '" + value + "', '" + comment + "' in expected results...");
       let j;
       for (j = 0; j < matches.length; j++) {
         // Skip processed expected results
         if (matches[j] == undefined)
           continue;
 
-        let { uri, title, tags } = matches[j];
+        let { uri, title, tags, searchEngine } = matches[j];
         if (tags)
           title += " \u2013 " + tags.sort().join(", ");
+        if (searchEngine)
+          title += TITLE_SEARCH_ENGINE_SEPARATOR + searchEngine;
 
         do_log_info("Checking against expected '" + uri.spec + "', '" + title + "'...");
         // Got a match on both uri and title?
         if (stripPrefix(uri.spec) == stripPrefix(value) && title == comment) {
           do_log_info("Got a match at index " + j + "!");
           // Make it undefined so we don't process it again
           matches[j] = undefined;
           if (uri.spec.startsWith("moz-action:")) {
new file mode 100644
--- /dev/null
+++ b/toolkit/components/places/tests/unifiedcomplete/test_searchEngine.js
@@ -0,0 +1,25 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+add_task(function* test_searchEngine() {
+  Services.search.addEngineWithDetails("SearchEngine", "", "", "",
+                                       "GET", "http://s.example.com/search");
+  let engine = Services.search.getEngineByName("SearchEngine");
+  engine.addParam("q", "{searchTerms}", null);
+  do_register_cleanup(() => Services.search.removeEngine(engine));
+
+  let uri1 = NetUtil.newURI("http://s.example.com/search?q=Terms&client=1");
+  let uri2 = NetUtil.newURI("http://s.example.com/search?q=Terms&client=2");
+  yield promiseAddVisits({ uri: uri1, title: "Terms - SearchEngine Search" });
+  addBookmark({ uri: uri2, title: "Terms - SearchEngine Search" });
+
+  do_log_info("Past search terms should be styled, unless bookmarked");
+  yield check_autocomplete({
+    search: "term",
+    matches: [ { uri: uri1, title: "Terms", searchEngine: "SearchEngine" },
+               { uri: uri2, title: "Terms - SearchEngine Search" } ]
+  });
+
+  yield cleanup();
+});
--- a/toolkit/components/places/tests/unifiedcomplete/xpcshell.ini
+++ b/toolkit/components/places/tests/unifiedcomplete/xpcshell.ini
@@ -18,15 +18,16 @@ tail =
 [test_enabled.js]
 [test_escape_self.js]
 [test_ignore_protocol.js]
 [test_keyword_search.js]
 [test_keywords.js]
 [test_match_beginning.js]
 [test_multi_word_search.js]
 [test_queryurl.js]
+[test_searchEngine.js]
 [test_special_search.js]
 [test_swap_protocol.js]
 [test_tabmatches.js]
 [test_trimming.js]
 [test_typed.js]
 [test_word_boundary_search.js]
 [test_zero_frecency.js]
--- a/toolkit/content/widgets/autocomplete.xml
+++ b/toolkit/content/widgets/autocomplete.xml
@@ -1431,16 +1431,30 @@ extends="chrome://global/content/binding
             this._setUpDescription(this._action, desc, true);
 
             // Remove the "action" substring so that the correct style, if any,
             // is applied below.
             types.splice(actionIndex, 1);
             type = types.join(" ");
           }
 
+          // Check if we have a search engine name
+          let searchEngine = "";
+          let searchIndex = types.indexOf("search");
+          if (searchIndex >= 0) {
+            const TITLE_SEARCH_ENGINE_SEPARATOR = " \u00B7\u2013\u00B7 ";
+
+            [title, searchEngine] = title.split(TITLE_SEARCH_ENGINE_SEPARATOR);
+
+            // Remove the "search" substring so that the correct style, if any,
+            // is applied below.
+            types.splice(searchIndex, 1);
+            type = types.join(" ");
+          }
+
           // If we have a tag match, show the tags and icon
           if (type == "tag") {
             // Configure the extra box for tags display
             this._extraBox.hidden = false;
             this._extraBox.childNodes[0].hidden = false;
             this._extraBox.childNodes[1].hidden = true;
             this._extraBox.pack = "end";
             this._titleBox.flex = 1;
@@ -1491,17 +1505,22 @@ extends="chrome://global/content/binding
             (type ? " ac-result-type-" + type : "");
 
           // Show the url as the title if we don't have a title
           if (title == "")
             title = url;
 
           // Emphasize the matching search terms for the description
           this._setUpDescription(this._title, title);
-          this._setUpDescription(this._url, url);
+          if (!searchEngine) {
+            this._setUpDescription(this._url, url);
+          } else {
+            // The search engine name, when present, is not emphasized.
+            this._setUpDescription(this._url, searchEngine, true);
+          }
 
           // Set up overflow on a timeout because the contents of the box
           // might not have a width yet even though we just changed them
           setTimeout(this._setUpOverflow, 0, this._titleBox, this._titleOverflowEllipsis);
           setTimeout(this._setUpOverflow, 0, this._urlBox, this._urlOverflowEllipsis);
           ]]>
         </body>
       </method>