Bug 1560228 - Strip only leading question marks from search string. r=adw a=RyanVM
authorMarco Bonardo <mbonardo@mozilla.com>
Thu, 25 Jul 2019 10:20:59 +0000
changeset 544782 c8e5ac81cde26c979183b839f51dedff71125e9a
parent 544781 d65aa43e6960433cc6eacb88887c3184079cd66f
child 544783 81429c99783c16e8056af18656d913a2b984b64f
push id2131
push userffxbld-merge
push dateMon, 26 Aug 2019 18:30:20 +0000
treeherdermozilla-release@b19ffb3ca153 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersadw, RyanVM
bugs1560228
milestone69.0
Bug 1560228 - Strip only leading question marks from search string. r=adw a=RyanVM Differential Revision: https://phabricator.services.mozilla.com/D39206
browser/components/urlbar/tests/UrlbarTestUtils.jsm
browser/components/urlbar/tests/browser/browser_keyword.js
toolkit/components/places/UnifiedComplete.jsm
toolkit/components/places/tests/unifiedcomplete/test_restrict_searchstring.js
toolkit/components/places/tests/unifiedcomplete/test_search_engine_alias.js
toolkit/components/places/tests/unifiedcomplete/test_tab_matches.js
toolkit/components/places/tests/unifiedcomplete/xpcshell.ini
--- a/browser/components/urlbar/tests/UrlbarTestUtils.jsm
+++ b/browser/components/urlbar/tests/UrlbarTestUtils.jsm
@@ -571,18 +571,16 @@ class UrlbarAbstraction {
         url: element._urlText,
       };
       if (details.type == UrlbarUtils.RESULT_TYPE.SEARCH && action) {
         // Strip restriction tokens from input.
         let query = action.params.input;
         let restrictTokens = Object.values(UrlbarTokenizer.RESTRICT);
         if (restrictTokens.includes(query[0])) {
           query = query.substring(1).trim();
-        } else if (restrictTokens.includes(query[query.length - 1])) {
-          query = query.substring(0, query.length - 1).trim();
         }
         details.searchParams = {
           engine: action.params.engineName,
           keyword: action.params.alias,
           query,
           suggestion: action.params.searchSuggestion,
         };
       } else if (details.type == UrlbarUtils.RESULT_TYPE.KEYWORD) {
--- a/browser/components/urlbar/tests/browser/browser_keyword.js
+++ b/browser/components/urlbar/tests/browser/browser_keyword.js
@@ -228,17 +228,17 @@ add_task(async function test_keyword_wit
   // TODO Bug 1517140: keywords containing restriction chars should not be
   // allowed, or properly supported.
   let result = await promise_first_result("question?");
   Assert.equal(
     result.type,
     UrlbarUtils.RESULT_TYPE.SEARCH,
     "Result should be a search"
   );
-  Assert.equal(result.searchParams.query, "question", "Check search query");
+  Assert.equal(result.searchParams.query, "question?", "Check search query");
 
   result = await promise_first_result("question? something");
   Assert.equal(
     result.type,
     UrlbarUtils.RESULT_TYPE.KEYWORD,
     "Result should be a keyword"
   );
   Assert.equal(result.keyword, "question?", "Check search query");
--- a/toolkit/components/places/UnifiedComplete.jsm
+++ b/toolkit/components/places/UnifiedComplete.jsm
@@ -621,32 +621,24 @@ function Search(
   // Use the original string here, not the stripped one, so the tokenizer can
   // properly recognize token types.
   let { tokens } = UrlbarTokenizer.tokenize({
     searchString: unescapedSearchString,
   });
 
   // This allows to handle leading or trailing restriction characters specially.
   this._leadingRestrictionToken = null;
-  this._trailingRestrictionToken = null;
   if (tokens.length > 0) {
     if (
       UrlbarTokenizer.isRestrictionToken(tokens[0]) &&
       (tokens.length > 1 ||
         tokens[0].type == UrlbarTokenizer.TYPE.RESTRICT_SEARCH)
     ) {
       this._leadingRestrictionToken = tokens[0].value;
     }
-    if (
-      UrlbarTokenizer.isRestrictionToken(tokens[tokens.length - 1]) &&
-      (tokens.length > 1 ||
-        tokens[tokens.length - 1].type == UrlbarTokenizer.TYPE.RESTRICT_SEARCH)
-    ) {
-      this._trailingRestrictionToken = tokens[tokens.length - 1].value;
-    }
 
     // Check if the first token has a strippable prefix and remove it, but don't
     // create an empty token.
     if (prefix && tokens[0].value.length > prefix.length) {
       tokens[0].value = tokens[0].value.substring(prefix.length);
     }
   }
 
@@ -1713,27 +1705,24 @@ Search.prototype = {
     return true;
   },
 
   async _matchCurrentSearchEngine() {
     let engine = await PlacesSearchAutocompleteProvider.currentEngine();
     if (!engine || !this.pending) {
       return false;
     }
-    // Strip a leading or trailing restriction char.
+    // Strip a leading search restriction char, because we prepend it to text
+    // when the search shortcut is used and it's not user typed. Don't strip
+    // other restriction chars, so that it's possible to search for things
+    // including one of those (e.g. "c#").
     let query = this._trimmedOriginalSearchString;
-    if (this._leadingRestrictionToken) {
+    if (this._leadingRestrictionToken === UrlbarTokenizer.RESTRICT.SEARCH) {
       query = substringAfter(query, this._leadingRestrictionToken).trim();
     }
-    if (this._trailingRestrictionToken) {
-      query = query.substring(
-        0,
-        query.lastIndexOf(this._trailingRestrictionToken)
-      );
-    }
     this._addSearchEngineMatch({ engine, query });
     return true;
   },
 
   _addExtensionMatch(content, comment) {
     let count =
       this._counts[UrlbarUtils.RESULT_GROUP.EXTENSION] +
       this._counts[UrlbarUtils.RESULT_GROUP.HEURISTIC];
new file mode 100644
--- /dev/null
+++ b/toolkit/components/places/tests/unifiedcomplete/test_restrict_searchstring.js
@@ -0,0 +1,32 @@
+/* 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/. */
+
+/**
+ * Test that restriction tokens are not removed from the search string, apart
+ * from a leading search restriction token.
+ */
+
+add_task(async function test_searchstring() {
+  for (let token of Object.values(UrlbarTokenizer.RESTRICT)) {
+    for (let search of [`${token} query`, `query ${token}`]) {
+      let searchQuery =
+        token == UrlbarTokenizer.RESTRICT.SEARCH &&
+        search.startsWith(UrlbarTokenizer.RESTRICT.SEARCH)
+          ? search.substring(2)
+          : search;
+      info(`Searching for "${search}", expecting "${searchQuery}"`);
+      await check_autocomplete({
+        search,
+        searchParam: "enable-actions",
+        matches: [
+          makeSearchMatch(search, {
+            engineName: "MozSearch",
+            searchQuery,
+            heuristic: true,
+          }),
+        ],
+      });
+    }
+  }
+});
--- a/toolkit/components/places/tests/unifiedcomplete/test_search_engine_alias.js
+++ b/toolkit/components/places/tests/unifiedcomplete/test_search_engine_alias.js
@@ -106,25 +106,30 @@ add_task(async function basicGetAndPost(
         }),
       ],
     });
 
     // When a restriction token is used before the alias, the alias should *not*
     // be recognized.  It should be treated as part of the search string.  Try
     // all the restriction tokens to test that.  We should get a single "search
     // with" heuristic result without an alias.
-    for (let restrictToken in UrlbarTokenizer.RESTRICT) {
-      let search = `${restrictToken} ${alias} query string`;
+    for (let token of Object.values(UrlbarTokenizer.RESTRICT)) {
+      let search = `${token} ${alias} query string`;
+      let searchQuery =
+        token == UrlbarTokenizer.RESTRICT.SEARCH &&
+        search.startsWith(UrlbarTokenizer.RESTRICT.SEARCH)
+          ? search.substring(2)
+          : search;
       await check_autocomplete({
         search,
         searchParam: "enable-actions",
         matches: [
           makeSearchMatch(search, {
             engineName: "MozSearch",
-            searchQuery: search,
+            searchQuery,
             heuristic: true,
           }),
         ],
       });
     }
   }
   await cleanup();
 });
--- a/toolkit/components/places/tests/unifiedcomplete/test_tab_matches.js
+++ b/toolkit/components/places/tests/unifiedcomplete/test_tab_matches.js
@@ -219,17 +219,17 @@ add_task(async function test_tab_matches
   info("tab match search with restriction character");
   addOpenPages(uri1, 1);
   await check_autocomplete({
     search: UrlbarTokenizer.RESTRICT.OPENPAGE + " abc",
     searchParam: "enable-actions",
     matches: [
       makeSearchMatch(UrlbarTokenizer.RESTRICT.OPENPAGE + " abc", {
         heuristic: true,
-        searchQuery: "abc",
+        searchQuery: UrlbarTokenizer.RESTRICT.OPENPAGE + " abc",
       }),
       makeSwitchToTabMatch("http://abc.com/", { title: "ABC rocks" }),
     ],
   });
 
   info("tab match with not-addable pages");
   await check_autocomplete({
     search: "mozilla",
@@ -242,17 +242,17 @@ add_task(async function test_tab_matches
 
   info("tab match with not-addable pages and restriction character");
   await check_autocomplete({
     search: UrlbarTokenizer.RESTRICT.OPENPAGE + " mozilla",
     searchParam: "enable-actions",
     matches: [
       makeSearchMatch(UrlbarTokenizer.RESTRICT.OPENPAGE + " mozilla", {
         heuristic: true,
-        searchQuery: "mozilla",
+        searchQuery: UrlbarTokenizer.RESTRICT.OPENPAGE + " mozilla",
       }),
       makeSwitchToTabMatch("about:mozilla"),
     ],
   });
 
   info("tab match with not-addable pages and only restriction character");
   await check_autocomplete({
     search: UrlbarTokenizer.RESTRICT.OPENPAGE,
--- a/toolkit/components/places/tests/unifiedcomplete/xpcshell.ini
+++ b/toolkit/components/places/tests/unifiedcomplete/xpcshell.ini
@@ -38,16 +38,17 @@ support-files =
 [test_keywords.js]
 [test_multi_word_search.js]
 [test_PlacesSearchAutocompleteProvider.js]
 skip-if = appname == "thunderbird"
 [test_preloaded_sites.js]
 [test_query_url.js]
 [test_remote_tab_matches.js]
 skip-if = !sync
+[test_restrict_searchstring.js]
 [test_search_engine_alias.js]
 [test_search_engine_default.js]
 [test_search_engine_host.js]
 [test_search_engine_restyle.js]
 [test_search_suggestions.js]
 [test_special_search.js]
 [test_swap_protocol.js]
 [test_tab_matches.js]