Bug 1433938 - Move Synced Tab matches above general history matches in the Address Bar. r=adw
Puts Remote (Synced) Tab matches before other history results.
Changes deduping algorithm to replace simple history matches with tab matches when the url is the same.
Keeps overriding a Remote Tab with a Local Tab when the url is the same.
MozReview-Commit-ID: 76urDklKtRF
--- a/browser/base/content/test/urlbar/browser_autocomplete_enter_race.js
+++ b/browser/base/content/test/urlbar/browser_autocomplete_enter_race.js
@@ -42,23 +42,24 @@ add_task(async function test_sametext()
info("wait for the page to load");
await BrowserTestUtils.browserLoaded(gBrowser.selectedTab.linkedBrowser,
false, "http://example.com/");
});
add_task(async function test_after_empty_search() {
await promiseAutocompleteResultPopup("");
gURLBar.focus();
- gURLBar.value = "e";
+ // Add www. to avoid a switch-to-tab.
+ gURLBar.value = "www.e";
EventUtils.synthesizeKey("x", {});
EventUtils.synthesizeKey("VK_RETURN", {});
info("wait for the page to load");
await BrowserTestUtils.browserLoaded(gBrowser.selectedTab.linkedBrowser,
- false, "http://example.com/");
+ false, "http://www.example.com/");
});
add_task(async function test_disabled_ac() {
// Disable autocomplete.
let suggestHistory = Preferences.get("browser.urlbar.suggest.history");
Preferences.set("browser.urlbar.suggest.history", false);
let suggestBookmarks = Preferences.get("browser.urlbar.suggest.bookmark");
Preferences.set("browser.urlbar.suggest.bookmark", false);
--- a/browser/base/content/test/urlbar/browser_search_favicon.js
+++ b/browser/base/content/test/urlbar/browser_search_favicon.js
@@ -17,17 +17,17 @@ add_task(async function() {
Services.search.addEngineWithDetails("SearchEngine", "", "", "",
"GET", "http://s.example.com/search");
gEngine = Services.search.getEngineByName("SearchEngine");
gEngine.addParam("q", "{searchTerms}", null);
gOriginalEngine = Services.search.currentEngine;
Services.search.currentEngine = gEngine;
- let uri = NetUtil.newURI("http://s.example.com/search?q=foo&client=1");
+ let uri = NetUtil.newURI("http://s.example.com/search?q=foobar&client=1");
await PlacesTestUtils.addVisits({ uri, title: "Foo - SearchEngine Search" });
await BrowserTestUtils.openNewForegroundTab(gBrowser, "about:mozilla");
// The first autocomplete result has the action searchengine, while
// the second result is the "search favicon" element.
await promiseAutocompleteResultPopup("foo");
let result = await waitForAutocompleteResultAt(1);
--- a/browser/base/content/test/urlbar/browser_urlbar_remove_match.js
+++ b/browser/base/content/test/urlbar/browser_urlbar_remove_match.js
@@ -8,17 +8,17 @@ add_task(async function test_remove_hist
registerCleanupFunction(async function() {
await PlacesUtils.history.clear();
});
let promiseVisitRemoved = PlacesTestUtils.waitForNotification(
"onDeleteURI", uri => uri.spec == TEST_URL, "history");
- await promiseAutocompleteResultPopup("remove.me/from_urlbar");
+ await promiseAutocompleteResultPopup("from_urlbar");
let result = await waitForAutocompleteResultAt(1);
Assert.equal(result.getAttribute("ac-value"), TEST_URL, "Found the expected result");
EventUtils.synthesizeKey("VK_DOWN", {});
Assert.equal(gURLBar.popup.richlistbox.selectedIndex, 1);
let options = AppConstants.platform == "macosx" ? { shiftKey: true } : {};
EventUtils.synthesizeKey("VK_DELETE", options);
await promiseVisitRemoved;
--- a/toolkit/components/places/PlacesUtils.jsm
+++ b/toolkit/components/places/PlacesUtils.jsm
@@ -16,16 +16,20 @@ XPCOMUtils.defineLazyModuleGetters(this,
OS: "resource://gre/modules/osfile.jsm",
Sqlite: "resource://gre/modules/Sqlite.jsm",
Bookmarks: "resource://gre/modules/Bookmarks.jsm",
History: "resource://gre/modules/History.jsm",
AsyncShutdown: "resource://gre/modules/AsyncShutdown.jsm",
PlacesSyncUtils: "resource://gre/modules/PlacesSyncUtils.jsm",
});
+XPCOMUtils.defineLazyGetter(this, "MOZ_ACTION_REGEX", () => {
+ return /^moz-action:([^,]+),(.*)$/;
+});
+
// On Mac OSX, the transferable system converts "\r\n" to "\n\n", where
// we really just want "\n". On other platforms, the transferable system
// converts "\r\n" to "\n".
const NEWLINE = AppConstants.platform == "macosx" ? "\n" : "\r\n";
// Timers resolution is not always good, it can have a 16ms precision on Win.
const TIMERS_RESOLUTION_SKEW_MS = 16;
@@ -313,16 +317,18 @@ this.PlacesUtils = {
TOPIC_EXPIRATION_FINISHED: "places-expiration-finished",
TOPIC_FEEDBACK_UPDATED: "places-autocomplete-feedback-updated",
TOPIC_FAVICONS_EXPIRED: "places-favicons-expired",
TOPIC_VACUUM_STARTING: "places-vacuum-starting",
TOPIC_BOOKMARKS_RESTORE_BEGIN: "bookmarks-restore-begin",
TOPIC_BOOKMARKS_RESTORE_SUCCESS: "bookmarks-restore-success",
TOPIC_BOOKMARKS_RESTORE_FAILED: "bookmarks-restore-failed",
+ ACTION_SCHEME: "moz-action:",
+
asContainer: aNode => asContainer(aNode),
asQuery: aNode => asQuery(aNode),
endl: NEWLINE,
/**
* Is a string a valid GUID?
*
@@ -408,17 +414,48 @@ this.PlacesUtils = {
for (let key in params) {
// Strip null or undefined.
// Regardless, don't encode them or they would be converted to a string.
if (params[key] === null || params[key] === undefined) {
continue;
}
encodedParams[key] = encodeURIComponent(params[key]);
}
- return "moz-action:" + type + "," + JSON.stringify(encodedParams);
+ return this.ACTION_SCHEME + type + "," + JSON.stringify(encodedParams);
+ },
+
+ /**
+ * Parses a moz-action URL and returns its parts.
+ *
+ * @param url A moz-action URI.
+ * @note URL is in the format moz-action:ACTION,JSON_ENCODED_PARAMS
+ */
+ parseActionUrl(url) {
+ if (url instanceof Ci.nsIURI)
+ url = url.spec;
+ else if (url instanceof URL)
+ url = url.href;
+ // Faster bailout.
+ if (!url.startsWith(this.ACTION_SCHEME))
+ return null;
+
+ try {
+ let [, type, params] = url.match(MOZ_ACTION_REGEX);
+ let action = {
+ type,
+ params: JSON.parse(params)
+ };
+ for (let key in action.params) {
+ action.params[key] = decodeURIComponent(action.params[key]);
+ }
+ return action;
+ } catch (ex) {
+ Cu.reportError(`Invalid action url "${url}"`);
+ return null;
+ }
},
/**
* Parses matchBuckets strings (for example, "suggestion:4,general:Infinity")
* like those used in the browser.urlbar.matchBuckets preference.
*
* @param str
* A matchBuckets string.
--- a/toolkit/components/places/UnifiedComplete.js
+++ b/toolkit/components/places/UnifiedComplete.js
@@ -708,50 +708,28 @@ function stripHttpAndTrim(spec, trimSlas
/**
* Returns the key to be used for a match in a map for the purposes of removing
* duplicate entries - any 2 URLs that should be considered the same should
* return the same key. For some moz-action URLs this will unwrap the params
* and return a key based on the wrapped URL.
*/
function makeKeyForURL(match) {
- let actionUrl = match.value;
-
+ let url = match.value;
+ let action = PlacesUtils.parseActionUrl(url);
// At this stage we only consider moz-action URLs.
- if (!actionUrl.startsWith("moz-action:")) {
+ if (!action || !("url" in action.params)) {
// For autofill entries, we need to have a key based on the comment rather
// than the value field, because the latter may have been trimmed.
if (match.hasOwnProperty("style") && match.style.includes("autofill")) {
- return stripHttpAndTrim(match.comment);
+ url = match.comment;
}
- return stripHttpAndTrim(actionUrl);
- }
- let [, type, params] = actionUrl.match(/^moz-action:([^,]+),(.*)$/);
- try {
- params = JSON.parse(params);
- } catch (ex) {
- // This is unexpected in this context, so just return the input.
- return stripHttpAndTrim(actionUrl);
+ return [stripHttpAndTrim(url), null];
}
- // For now we only handle these 2 action types and treat them as the same.
- switch (type) {
- case "remotetab":
- case "switchtab":
- if (params.url) {
- return "moz-action:tab:" + stripHttpAndTrim(params.url);
- }
- break;
- // TODO (bug 1222435) - "switchtab" should be handled as an "autofill"
- // entry.
- default:
- // do nothing.
- // TODO (bug 1222436) - extend this method so it can be used instead of
- // the |placeId| that's also used to remove duplicate entries.
- }
- return stripHttpAndTrim(actionUrl);
+ return [stripHttpAndTrim(action.params.url), action];
}
/**
* Returns whether the passed in string looks like a url.
*/
function looksLikeUrl(str, ignoreAlphanumericHosts = false) {
// Single word not including special chars.
return !REGEXP_SPACES.test(str) &&
@@ -863,17 +841,17 @@ function Search(searchString, searchPara
}
// This is a replacement for this._result.matchCount, to be used when you need
// to check how many "current" matches have been inserted.
// Indeed this._result.matchCount may include matches from the previous search.
this._currentMatchCount = 0;
// These are used to avoid adding duplicate entries to the results.
- this._usedURLs = new Set();
+ this._usedURLs = [];
this._usedPlaceIds = new Set();
// Counters for the number of matches per MATCHTYPE.
this._counts = Object.values(MATCHTYPE)
.reduce((o, p) => { o[p] = 0; return o; }, {});
this._searchStringHasWWW = this._strippedPrefix.endsWith("www.");
this._searchStringWWW = this._searchStringHasWWW ? "www." : "";
@@ -1055,18 +1033,17 @@ Search.prototype = {
// enabled, the first result is always a special result (resulting from one
// of the queries between (1) and (6) inclusive). As such, the UI is
// expected to auto-select the first result when actions are enabled. If the
// first result is an inline completion result, that will also be the
// default result and therefore be autofilled (this also happens if actions
// are not enabled).
// Get the final query, based on the tokens found in the search string.
- let queries = [ this._adaptiveQuery ];
-
+ let queries = [];
// "openpage" behavior is supported by the default query.
// _switchToTabQuery instead returns only pages not supported by history.
if (this.hasBehavior("openpage")) {
queries.push(this._switchToTabQuery);
}
queries.push(this._searchQuery);
// Check for Preloaded Sites Expiry before Autofill
@@ -1127,24 +1104,32 @@ Search.prototype = {
}
}
}
// In any case, clear previous suggestions.
searchSuggestionsCompletePromise.then(() => {
this._cleanUpNonCurrentMatches(MATCHTYPE.SUGGESTION);
});
- for (let [query, params] of queries) {
- await conn.executeCached(query, params, this._onResultRow.bind(this));
+ // Run the adaptive query first.
+ await conn.executeCached(this._adaptiveQuery[0], this._adaptiveQuery[1],
+ this._onResultRow.bind(this));
+ if (!this.pending)
+ return;
+
+ // Then fetch remote tabs.
+ if (this._enableActions && this.hasBehavior("openpage")) {
+ await this._matchRemoteTabs();
if (!this.pending)
return;
}
- if (this._enableActions && this.hasBehavior("openpage")) {
- await this._matchRemoteTabs();
+ // Finally run all the other queries.
+ for (let [query, params] of queries) {
+ await conn.executeCached(query, params, this._onResultRow.bind(this));
if (!this.pending)
return;
}
// Ideally we should wait until MATCH_BOUNDARY_ANYWHERE, but that query
// may be really slow and we may end up showing old results for too long.
this._cleanUpNonCurrentMatches(MATCHTYPE.GENERAL);
@@ -1854,48 +1839,33 @@ Search.prototype = {
// We fallback to match.value, as that's what autocomplete does if
// finalCompleteValue is null.
// Trim only if the value looks like a domain, we want to retain the
// trailing slash if we're completing a url to the next slash.
match.comment = stripHttpAndTrim(match.finalCompleteValue || match.value,
!this._searchString.includes("/"));
}
- // Must check both id and url, cause keywords dynamically modify the url.
- let urlMapKey = makeKeyForURL(match);
- if ((match.placeId && this._usedPlaceIds.has(match.placeId)) ||
- this._usedURLs.has(urlMapKey)) {
- return;
- }
-
- // Add this to our internal tracker to ensure duplicates do not end up in
- // the result.
- // 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);
-
match.style = match.style || "favicon";
// Restyle past searches, unless they are bookmarks or special results.
if (Prefs.get("restyleSearches") && match.style == "favicon") {
this._maybeRestyleSearchMatch(match);
}
if (this._addingHeuristicFirstMatch) {
match.style += " heuristic";
}
match.icon = match.icon || "";
match.finalCompleteValue = match.finalCompleteValue || "";
let {index, replace} = this._getInsertIndexForMatch(match);
+ if (index == -1)
+ return;
if (replace) { // Replacing an existing match from the previous search.
this._result.removeMatchAt(index);
}
this._result.insertMatchAt(index,
match.value,
match.comment,
match.icon,
match.style,
@@ -1906,16 +1876,53 @@ Search.prototype = {
if (this._currentMatchCount == 1)
TelemetryStopwatch.finish(TELEMETRY_1ST_RESULT, this);
if (this._currentMatchCount == 6)
TelemetryStopwatch.finish(TELEMETRY_6_FIRST_RESULTS, this);
this.notifyResult(true, match.type == MATCHTYPE.HEURISTIC);
},
_getInsertIndexForMatch(match) {
+ // Check for duplicates and either discard (by returning -1) the duplicate
+ // or suggest to replace the original match, in case the new one is more
+ // specific (for example a Remote Tab wins over History, and a Switch to Tab
+ // wins over a Remote Tab).
+ // Must check both id and url, cause keywords dynamically modify the url.
+ // Note: this partially fixes Bug 1222435, but not if the urls differ more
+ // than just by "http://". We should still evaluate www and other schemes
+ // equivalences.
+ let [urlMapKey, action] = makeKeyForURL(match);
+ if ((match.placeId && this._usedPlaceIds.has(match.placeId)) ||
+ this._usedURLs.map(e => e.key).includes(urlMapKey)) {
+ // it's a duplicate.
+ if (action && ["switchtab", "remotetab"].includes(action.type)) {
+ // Look for the duplicate among current matches.
+ for (let i = 0; i < this._usedURLs.length; ++i) {
+ let {key: matchKey, action: matchAction} = this._usedURLs[i];
+ if (matchKey == urlMapKey) {
+ if (!matchAction || action.type == "switchtab") {
+ this._usedURLs[i] = {key: urlMapKey, action};
+ return { index: i, replace: true };
+ }
+ break; // Found the duplicate, no reason to continue.
+ }
+ }
+ }
+ return { index: -1, replace: false };
+ }
+
+ // Add this to our internal tracker to ensure duplicates do not end up in
+ // the result.
+ // 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);
+
let index = 0;
// The buckets change depending on the context, that is currently decided by
// the first added match (the heuristic one).
if (!this._buckets) {
// Convert the buckets to readable objects with a count property.
let buckets = match.type == MATCHTYPE.HEURISTIC &&
match.style.includes("searchengine") ? Prefs.get("matchBucketsSearch")
: Prefs.get("matchBuckets");
@@ -1942,17 +1949,17 @@ Search.prototype = {
bucket.count++;
break;
}
}
}
}
}
- let replace = false;
+ let replace = 0;
for (let bucket of this._buckets) {
// Move to the next bucket if the match type is incompatible, or if there
// is no available space or if the frecency is below the threshold.
if (match.type != bucket.type || !bucket.available) {
index += bucket.count;
continue;
}
@@ -1961,16 +1968,17 @@ Search.prototype = {
if (bucket.insertIndex < bucket.count) {
replace = true;
} else {
bucket.count++;
}
bucket.insertIndex++;
break;
}
+ this._usedURLs[index] = {key: urlMapKey, action};
return { index, replace };
},
/**
* Removes matches from a previous search, that are no more returned by the
* current search
* @param type
* The MATCHTYPE to clean up.
--- a/toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js
+++ b/toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js
@@ -115,17 +115,17 @@ async function _check_autocomplete_match
let { uri, title, tags, style } = match;
if (tags)
title += " \u2013 " + tags.sort().join(", ");
if (style)
style = style.sort();
else
style = ["favicon"];
- info(`Checking against expected "${uri.spec}", "${title}"`);
+ info(`Checking against expected "${uri.spec}", comment: "${title}", style: "${style}"`);
// Got a match on both uri and title?
if (stripPrefix(uri.spec) != stripPrefix(result.value) || title != result.comment) {
return false;
}
let actualStyle = result.style.split(/\s+/).sort();
if (style)
Assert.equal(actualStyle.toString(), style.toString(), "Match should have expected style");
--- a/toolkit/components/places/tests/unifiedcomplete/test_remote_tab_matches.js
+++ b/toolkit/components/places/tests/unifiedcomplete/test_remote_tab_matches.js
@@ -198,8 +198,36 @@ add_task(async function test_localtab_ma
await check_autocomplete({
search: "ex",
searchParam: "enable-actions",
matches: [ makeSearchMatch("ex", { heuristic: true }),
makeSwitchToTabMatch("http://foo.com/", { title: "An Example" }),
],
});
});
+
+add_task(async function test_remotetab_matches_override() {
+ // If We have an history result to the same page, we should only get the
+ // remote tab match.
+ let url = "http://foo.remote.com/";
+ // First setup Sync to have the page as a remote tab.
+ configureEngine({
+ guid_mobile: {
+ clientName: "My Phone",
+ tabs: [{
+ urlHistory: [url],
+ title: "An Example",
+ }],
+ }
+ });
+
+ // Setup Places to think the tab is open locally.
+ await PlacesTestUtils.addVisits(url);
+
+ await check_autocomplete({
+ search: "rem",
+ searchParam: "enable-actions",
+ matches: [ makeSearchMatch("rem", { heuristic: true }),
+ makeRemoteTabMatch("http://foo.remote.com/", "My Phone",
+ { title: "An Example" }),
+ ],
+ });
+});
--- a/toolkit/components/places/tests/unifiedcomplete/test_tab_matches.js
+++ b/toolkit/components/places/tests/unifiedcomplete/test_tab_matches.js
@@ -21,18 +21,17 @@ add_task(async function test_tab_matches
// Pages that cannot be registered in history.
addOpenPages(uri3, 1);
addOpenPages(uri4, 1);
info("two results, normal result is a tab match");
await check_autocomplete({
search: "abc.com",
searchParam: "enable-actions",
- matches: [ makeVisitMatch("abc.com", "http://abc.com/", { heuristic: true }),
- makeSwitchToTabMatch("http://abc.com/", { title: "ABC rocks" }),
+ matches: [ makeSwitchToTabMatch("http://abc.com/", { title: "ABC rocks" }),
makeSearchMatch("abc.com", { heuristic: false }) ]
});
info("three results, one tab match");
await check_autocomplete({
search: "abc",
searchParam: "enable-actions",
matches: [ makeSearchMatch("abc", { heuristic: true }),