author | Marco Bonardo <mbonardo@mozilla.com> |
Tue, 13 Feb 2018 14:34:44 +0100 | |
changeset 404306 | 6cfa628374192bc053d5837c892fc5d6f44a6778 |
parent 404305 | 3f96ca5e9d2deeaa6206313c981fa32d46062725 |
child 404307 | 90e5ccdd8c5fbd87e427e16dd5fd226fe79cedf6 |
child 404320 | be58311261fcad925faeaceefe3303ceb1653865 |
push id | 33461 |
push user | apavel@mozilla.com |
push date | Sat, 17 Feb 2018 21:43:24 +0000 |
treeherder | mozilla-central@90e5ccdd8c5f [default view] [failures only] |
perfherder | [talos] [build metrics] [platform microbench] (compared to previous push) |
reviewers | adw |
bugs | 1433938 |
milestone | 60.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
|
--- 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 }),