Closed Bug 1211726 Opened 9 years ago Closed 8 years ago

Pre-seed awesomebar (location bar) with a list of top web sites for better autocompletion on empty profiles

Categories

(Firefox :: Address Bar, defect, P3)

Product:
Firefox
Component:
Address Bar
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 54
Tracking Flags:
Tracking Status
firefox44 --- affected
firefox54 --- fixed

People

(Reporter: Dolske, Assigned: sveta.orlik.code, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxsearch][ui])

Attachments

(1 file, 3 obsolete files)

Bug 1211726 - part 1: add results from a hardcoded list of top sites,
58 bytes, text/x-review-board-request
Gijs
: review+
Details
In a new profile (ie, new install), the Firefox Awesomebar is not very awesome. This results in a poor first-use experience. The user hasn't visited any web pages, and so the browser has nothing to suggest. Well, actually, it may suggest some of the default bookmarks we ship, but those are almost certainly not where the user wants to go. The Awesomebar would be a lot more useful in a new profile if it was pre-seeded with a list of top web sites, so that it could return relevant results until it learns from the user's actual history. We would ship this as a static list in the browser so it's immediately ready for use. A few open questions: * What list of sites? The Alexia Top 500 looks good. Especially the per-region lists in http://www.alexa.com/topsites/countries. * Alternatively, we could focus on optimizing the first-urlbar-use experience for enabling the search-suggestions opt-in. * We would want and pre-seeded URL list to expire in awesomebar results fairly quickly. Say, after a week or two. After that point we should only be suggesting sites from the user's own history. * Can we add these in a way that doesn't cause them to show up as actual history visits? * (Aside: similarly, can we have the default bookmarks not show up?) * Favicons for the sites would be great too -- do we pre-seed those as part of the build process, or have the client fetch them? Lots of opportunities here for incremental implementation and testing, by doing small trials to see if there's enough impact to invest in this more completely. [This bug has been discussed on and off for years, but as far as I can tell hasn't ever been actually filed!]
Marco: Any general comments on feasibility of this from a Places point-of-view?
Flags: needinfo?(mak77)
(In reply to Justin Dolske [:Dolske] from comment #0) > * Can we add these in a way that doesn't cause them to show up as actual > history visits? I think this is the biggest issue to solve, from a privacy point of view. For the same reason, if the search suggestions return a url, we are currently filtering it out. The user can be really surprised by results coming from "nowhere". > * Favicons for the sites would be great too -- do we pre-seed those as part > of the build process, or have the client fetch them? We never fetch favicons in background, and we might have security implications in doing that (cause the favicon should be associated to the principal of the page using it) that we need to take into account. I think the proposal has value, but we need to figure a way to present it to the user the right way (so it doesn't look advertising, it doesn't look like coming from history, and so on...)
Flags: needinfo?(mak77)
Priority: -- → P3
Whiteboard: [fxsearch][ui]
from a technical point of view, we just need to build an autofill provider and hook it up in UnifiedComplete were autofill happens, it's quite easy with the new architecture. We were doing the same for Search provider top suggestions (until we disabled the feature cause not all search engines had a good form url, see bug 958188).
Rank: 35
Android just did this in bug 858829. We can probably crib their list, at least... though that's maybe the least complicated thing here. :-(
Assignee: nobody → sveta.orlik.code
Mentor: gijskruitbosch+bugs
Status: NEW → ASSIGNED
Comment on attachment 8818509 [details] Bug 1211726 - part 1: add results from a hardcoded list of top sites, https://reviewboard.mozilla.org/r/98586/#review98842 This is a great start! In addition to the smaller issues below, here are some general points: - we should add a preference that we implement this behind. This will involve adding a preference in browser/app/profile/firefox.js , probably called something like 'browser.urlbar.suggest.defaulttopsites'. Then we'll need to make sure to only insert our matches when the preference is set to true (which should be the default value for now - but this will allow us to easily turn the feature off for tests or if we find problems with it close to release). - I expect some current autocomplete tests will fail. In those tests, you can use `pushPrefEnv` to turn off the pref that you'd create in the previous step. We are probably already using that for some of the other prefs. - we should add a test specifically for this functionality - we should make sure that the results are ranked below local user history and bookmarks. So if the user types `oo` and they've visited `yahoo.com`, the entry for yahoo should come before our suggestions of google and facebook. We should make sure to test this in the automated test. ::: toolkit/components/places/UnifiedComplete.js:12 (Diff revision 1) > "use strict"; > > // Constants > > +//-------------------------------------------------------------------- > +const TOP500_SITES = [ As I said on IRC, I think it would be a good idea to use the website titles instead of the Alexa descriptions. This list should also probably live after the definition of `const TITLE_TAGS_SEPARATOR = " \u2013 ";`, instead of all the way at the top. ::: toolkit/components/places/UnifiedComplete.js:773 (Diff revision 1) > this._localMatchesCount = 0; > // Counts the number of inserted remote matches. > this._remoteMatchesCount = 0; > // Counts the number of inserted extension matches. > this._extensionMatchesCount = 0; > + Please remove this extra empty line. :-) ::: toolkit/components/places/UnifiedComplete.js:953 (Diff revision 1) > let hasHeuristic = yield this._matchFirstHeuristicResult(conn); > this._addingHeuristicFirstMatch = false; > if (!this.pending) > return; > > + //-------------------------------------------------------------------- Nit: we don't need the extra commented `------` lines. ::: toolkit/components/places/UnifiedComplete.js:954 (Diff revision 1) > this._addingHeuristicFirstMatch = false; > if (!this.pending) > return; > > + //-------------------------------------------------------------------- > + yield this._matchTop500() This function isn't currently a generator function, so I don't think we need to yield for it. ::: toolkit/components/places/UnifiedComplete.js:955 (Diff revision 1) > if (!this.pending) > return; > > + //-------------------------------------------------------------------- > + yield this._matchTop500() > + if (!this.pending) and then we probably don't need to re-check `this.pending`, because we will have been running code on this thread non-stop. ::: toolkit/components/places/UnifiedComplete.js:1017 (Diff revision 1) > > // Ensure to fill any remaining space. Suggestions which come from extensions are > // inserted at the beginning, so any suggestions > yield Promise.all(this._remoteMatchesPromises); > }), > + Nit: there's leftover spaces here. ::: toolkit/components/places/UnifiedComplete.js:1025 (Diff revision 1) > + for (let [url, title] of TOP500_SITES) { > + if (url.toLowerCase().includes(this._searchString) || > + title.toLowerCase().includes(this._searchString)) { > + let match = { > + value: url, > + comment: url + " - " + title, // More convenient look I think this should only have the title. ::: toolkit/components/places/UnifiedComplete.js:1026 (Diff revision 1) > + if (url.toLowerCase().includes(this._searchString) || > + title.toLowerCase().includes(this._searchString)) { > + let match = { > + value: url, > + comment: url + " - " + title, // More convenient look > + style: "bookmark", We should use something else here. I guess this just gets turned into a CSS class in the result? We should come up with our own name, maybe "prefill-site" or something like this, and style it appropriately. ::: toolkit/components/places/UnifiedComplete.js:1027 (Diff revision 1) > + title.toLowerCase().includes(this._searchString)) { > + let match = { > + value: url, > + comment: url + " - " + title, // More convenient look > + style: "bookmark", > + frecency: FRECENCY_DEFAULT + 1, We should make sure that we use a low-ish frecency, so that sites the user visits take precedence over this result.
Attachment #8818509 - Flags: review?(gijskruitbosch+bugs)
Are we planning to roll this out as experiment to see how it changes Awesomebar engagement?
(In reply to Harald Kirschner :digitarald from comment #7) > Are we planning to roll this out as experiment to see how it changes > Awesomebar engagement? No.
(In reply to :Gijs (gone 24 dec - 3 jan) from comment #8) > (In reply to Harald Kirschner :digitarald from comment #7) > > Are we planning to roll this out as experiment to see how it changes > > Awesomebar engagement? > > No. I think at least a shield study, if not a test pilot experiment, would be a good idea here. Tagging javaun to see if he has any thoughts.
Flags: needinfo?(jmoradi)
> I think at least a shield study, if not a test pilot experiment, would be a good idea here. To be extra clear: I think an experimental study is warranted because placing specific domains in Firefox by default might be seen, by some of our users, as a paid placement deal, or otherwise erode their trust in Firefox. We can't assume we know how users will react to such a change--particularly users who may be trying Firefox for the first time. I would suggest we measure whether first-run users with and without this feature have higher engagement over time.
It's a really interesting idea. Going back to Dolske's original (problem comment 1). We're solving for new users (or new installs) with a clean profile, no history. For these users, the Awesome Bar is pretty useless because 1. there's nothing but canned bookmarks to suggest, no history, and search suggestions are off by default. We have an Awesome Bar search shield study in-flight right now. I don't want to talk about experiment groups until S&I tells me that it's safe to do so without "unblinding" the study for some users, but suffice to say we are looking at whether changes can improve engagement. The specific use case of clean profile is tricky to test with Shield or Test Pilot though. That's an ephemeral situation. We may be losing retention because of it. If we're really concerned about clean profiles, a Funnel cake is probably a better place to test. We'll see what this current Shield study reveals and share it w/ the group.
Flags: needinfo?(jmoradi)
Some notes: This is a mentored implementation bug. Please take concerns about the eventual route-to-shipping of it to email or a separate bug so the bug can stay focused on the specific set of patches here. They'll be behind a pref so we're not tying ourselves to shipping this to everybody in whatever cycle this lands in. The suggestion in the bug has already been implemented by us before, but right now that behaviour is only available in Fennec. I am not aware of any negative feedback about it there. The presence of a domain in the list of completions when typing in the location bar also has very different characteristics to tiles or ads, and the list will be objectively sourced rather than special-cased based on "deals", so it seems pretty far-fetched that people will take offence at it. It will also be displaced by history (unlike ads or "enhanced" tiles and the like). Finally, as Javaun said, new users can't be targeted by SHIELD or test pilot, and so if we wanted to experiment with this before launching it, a funnelcake is pretty much our only option. For that to happen, implementing the feature behind a pref will be the right approach anyway.
Comment on attachment 8821485 [details] Bug 1211726 - fix nits, https://reviewboard.mozilla.org/r/100750/#review101234
Attachment #8821485 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8821486 [details] Bug 1211726 - fix array with titles, https://reviewboard.mozilla.org/r/100752/#review101236
Attachment #8821486 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8821487 [details] Bug 1211726 - fix: set low frecency, https://reviewboard.mozilla.org/r/100754/#review101238 For the next version, please combine these (and future fixes) with the main commit. :-)
Attachment #8821487 - Flags: review?(gijskruitbosch+bugs) → review+
Attachment #8821485 - Attachment is obsolete: true
Attachment #8821486 - Attachment is obsolete: true
Attachment #8821487 - Attachment is obsolete: true
Comment on attachment 8818509 [details] Bug 1211726 - part 1: add results from a hardcoded list of top sites, https://reviewboard.mozilla.org/r/98586/#review104120 This looks OK, but how are you getting on with the use of a pref and the tests (see https://bugzilla.mozilla.org/show_bug.cgi?id=1211726#c6 ) ? Those changes will likely need to be part of this commit. :-) ::: toolkit/components/places/UnifiedComplete.js:65 (Diff revision 2) > > +const TOP500_SITES = [ > + ["Google.com", "Google"], > + ["Youtube.com", "YouTube"], > + ["Facebook.com", "Facebook"], > + ["Baidu.com", "百度一下，你就知道"], When I get this result in the location bar, I actually see broken unicode characters (boxes with numbers in) rather than the original characters. I'm not sure where we're messing up the encoding, but it looks like the same thing doesn't happen if we use: "\u767E\u5EA6\u4E00\u4E0B\uFF0C\u4F60\u5C31\u77E5\u9053" instead, which should be the same?
Attachment #8818509 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8818509 [details] Bug 1211726 - part 1: add results from a hardcoded list of top sites, https://reviewboard.mozilla.org/r/98586/#review104132 Yes, this looks good to me. I would set "r+", but because of bug 1195661 you wouldn't easily be able to request review again once you add the pref / fix tests, so I will clear it for now. :-) I'll do a push to try in a second to see what breaks.
Attachment #8818509 - Flags: review?(gijskruitbosch+bugs)
Hey Eric, Philipp tells me you might be able to provide us with an icon/design to use for these items in the location bar's popup. For comparison, check out the icons that already appear in the popup next to e.g. bookmarked entries. We should have a distinguishing icon to help the user know the difference between these items and "real" history entries.
Flags: needinfo?(epang)
Summary: Pre-seed awesomebar with a list of top web sites → Pre-seed awesomebar (location bar) with a list of top web sites for better autocompletion on empty profiles
(In reply to :Gijs from comment #24) > We should have a distinguishing icon to help the > user know the difference between these items and "real" history entries. Do we really need one? Seems like for the most part only new users are going to see these entries (i.e. until they've generated history), and that it would be challenging to communicate anything meaningful about these preloaded entries with just an icon.
(In reply to Justin Dolske [:Dolske] from comment #25) > (In reply to :Gijs from comment #24) > > > We should have a distinguishing icon to help the > > user know the difference between these items and "real" history entries. > > Do we really need one? Seems like for the most part only new users are going > to see these entries (i.e. until they've generated history), and that it > would be challenging to communicate anything meaningful about these > preloaded entries with just an icon. Well, your comment #0 states: > * Can we add these in a way that doesn't cause them to show up as actual history visits? This seems valuable to me, also given the concerns expressed in comment #10. Note that we already have distinguishing UI for bookmarks, switch-to-tab, search suggestions, and (by exclusion of all other possibilities), "real" history results. It seems reasonable to want to distinguish "actual" history visits from these suggestions. I'm also curious if you have concrete ideas about when to disable these suggestions, as you're saying users will stop seeing them after they have history, which would mitigate this concern. Certainly on android, my experience has been that occasionally I saw these suggestions long after I started using the browser, because it didn't really expire them (or maybe took a long time or whatever). I also suspect that for people who e.g. use permanent private browsing mode, they might be useful longer-term. So I'm not really sure at what point we should get rid of them, though it's clear that they should take lower precedence than 'real' history. :-)
Flags: needinfo?(dolske)
Comment on attachment 8818509 [details] Bug 1211726 - part 1: add results from a hardcoded list of top sites, https://reviewboard.mozilla.org/r/98586/#review105644 We discussed how to set the pref for the xpcshell tests on IRC. On the whole this is still heading in the right direction - keep it up. :-) ::: toolkit/components/places/UnifiedComplete.js:952 (Diff revision 5) > let hasHeuristic = yield this._matchFirstHeuristicResult(conn); > this._addingHeuristicFirstMatch = false; > if (!this.pending) > return; > > + dump("++++++++++++++++++++++++++++++++++ " + Services.prefs.getBoolPref("browser.urlbar.defaultdomaincompletion.enabled")); Nit: probably want to get rid of this dump() once tests run OK. Same thing for the do_print in the test. ::: toolkit/components/places/UnifiedComplete.js:1018 (Diff revision 5) > + if (url.toLowerCase().includes(this._searchString) || > + title.toLowerCase().includes(this._searchString)) { We should probably include/cache the lowercased title and URL in the top site data structure. Maybe we can use objects instead of nested arrays. Then we can write a method to cache the lowercase values that we run right before matching: ```js _ensureLowerCasedValues() { if (TOP500_SITES[0]._lowerURL) { return; } for (let site of TOP500_SITES) { site._lowerURL = site.url.toLowerCase(); site._lowerTitle = site.title.toLowerCase(); } }, ``` and call this method right before matching, and then match against `_lowerURL` and `_lowerTitle`. At this point, is the `_searchString` guaranteed to be lowercase? If not it probably makes sense to store a lowercased copy of it in a variable in this function. ::: toolkit/components/places/tests/unifiedcomplete/test_1211726.js:6 (Diff revision 5) > +/* 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_it_works() { > + do_print("Searching in 'TOP 500 SITES' hard-codded array"); Right... rather than including harcoded test values in the array we ship, I suspect we will need to add methods to manipulate the list from the test to the UnifiedComplete object. See e.g. the `registerOpenPage` and `unregisterOpenPage` methods, and how they are invoked from the head_autocomplete.js file. We probably want similar-ish methods that force the use of a different list of prefilled sites.
Attachment #8818509 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8818509 [details] Bug 1211726 - part 1: add results from a hardcoded list of top sites, https://reviewboard.mozilla.org/r/98586/#review105662 ::: toolkit/components/places/tests/head_common.js:75 (Diff revision 7) > }); > > var gTestDir = do_get_cwd(); > > // Initialize profile. > var gProfD = do_get_profile(); For now, set the pref after this line... ::: toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js:48 (Diff revision 7) > ]; > for (let type of suggestPrefs) { > Services.prefs.clearUserPref("browser.urlbar.suggest." + type); > } > Services.prefs.clearUserPref("browser.search.suggest.enabled"); > + Services.prefs.clearUserPref("browser.urlbar.defaultdomaincompletion.enabled"); ... and move this to a: ```js do_register_cleanup(() => Services.prefs.clearUserPref("browser.urlbar.defaultdomaincompletion.enabled")) ``` right above setting the pref. That should avoid the pref being reset in the middle of some tests (which call cleanup() in the middle of the test).
Attachment #8818509 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8818509 [details] Bug 1211726 - part 1: add results from a hardcoded list of top sites, https://reviewboard.mozilla.org/r/98586/#review105666 ::: toolkit/components/places/tests/unifiedcomplete/xpcshell.ini:35 (Diff revision 7) > [test_remote_tab_matches.js] > +[test_1211726.js] > skip-if = !sync This is a .ini file, so the `skip-if` applies to the test listed above it. So make sure you insert the new test underneath any skip-if/run-if/... lines, not above them. It looks like the other tests here are all named after what they test, so the test would probably be better off being called something like "test_default_site_completions.js" or something like that.
(In reply to :Gijs from comment #24) > Hey Eric, Philipp tells me you might be able to provide us with an > icon/design to use for these items in the location bar's popup. For > comparison, check out the icons that already appear in the popup next to > e.g. bookmarked entries. We should have a distinguishing icon to help the > user know the difference between these items and "real" history entries. I did some exploration for an icon but felt it ended up causing more confusion then clarity similar to what Dolske mentioned in comment 25: > it would be challenging to communicate anything meaningful about these preloaded entries with just an icon From a UX perspective I don't think a user will question the appearance of these "suggestions" (they may not even realize they are suggestions). My recommendation would be to try them without an icon for now but it would be great to test the user's reaction. In the end they will help get the user to where they want to go faster. It will be important to expire these results within a week or two once the user builds up their history/bookmarks. Any sites they don't visit within this time will end up being clutter that will slow down their searches. At this point a user will begin to question why they keep showing up.
Flags: needinfo?(epang)
(In reply to :Gijs from comment #26) > Well, your comment #0 states: > > > * Can we add these in a way that doesn't cause them to show up as actual history visits? Sorry, that's missing context... An early thought was to literally inject these sites as history visits, which wouldn't involve building anything extra. But the downside of that is that they'd show up in other places as actual visits, when they're not. (Most obviously in the Places Library, but also in, say, the Page Info "have I visited this site before" thing.) > I'm also curious if you have concrete ideas about when to disable these > suggestions, as you're saying users will stop seeing them after they have > history, which would mitigate this concern. I'm open to suggestions on the specifics. The basic goal here was just to be able to provide something for new users so that they don't experience an empty (and thus useless) awesomebar. Once you've actually started to use the browser, I would expect your own history to be much more valuable, and these suggestions to become increasingly undesirable. A simple approach would be to just disable this feature soon after install. Or you could try to be fancy and base it on actual usage hours (or actual history accumulation?), to adjust for different usage patterns. You could also be super-fancy, and _gradually_ disable the feature by requiring a more and more strict matching. (e.g. on day 1 "g" would match "google.com", on day 2 it would take "go", day 3 "goo", etc). I'd be perfectly happy with the simplest approach, and then we can run a funnelcake to see how it's doing. Maybe even a couple of variations, to see if a 1/2/4 week expiration matters. > I also suspect that for people who e.g. use > permanent private browsing mode, they might be useful longer-term. True, but I'm not really interested in optimizing for that scenario right now.
Flags: needinfo?(dolske)
Comment on attachment 8818509 [details] Bug 1211726 - part 1: add results from a hardcoded list of top sites, https://reviewboard.mozilla.org/r/98586/#review106614 just couple thoughts. ::: modules/libpref/init/all.js:1064 (Diff revision 7) > pref("browser.fixup.dns_first_for_single_words", false); > pref("browser.fixup.hide_user_pass", true); > > // Location Bar AutoComplete > pref("browser.urlbar.autocomplete.enabled", true); > +pref("browser.urlbar.defaultdomaincompletion.enabled", true); can we find a more specific name for the pref please, this looks so generic that one may think it's about autoFill in the urlbar in general. ::: toolkit/components/places/UnifiedComplete.js:955 (Diff revision 7) > return; > > + dump("++++++++++++++++++++++++++++++++++ " + Services.prefs.getBoolPref("browser.urlbar.defaultdomaincompletion.enabled")); > + if ( Services.prefs.getBoolPref("browser.urlbar.defaultdomaincompletion.enabled") ) { > + this._matchTop500() > + } What about we instead push these out as host autoFill? wouldn't that work better since they are just domains? It would just be matter of moving this code after the _matchKnownUrl and _matchSearchEngineUrl() and build with match.style = "autofill"; As autoFill results, these will be replaced by history, but still, if some pages are never used, the user may not want "f" completely to facebook, so some sort of timed expiration could still do. As it is right now, these results will always come even before adaptive history results, that doesn't sound good. If you want to retain the simple row in autocomplete behavior, this should be at least moved as last matches resource, just before the final yield Promise.all... in a new profile there should be no perf hit by previous queries, so this should appear quick enough. and it will be pushed down by other more frecenct result. (again, a temporal expiration could make sense, or these may still appear even after months for certain queries with few matches or for users who clear history on shutdown)
Comment on attachment 8818509 [details] Bug 1211726 - part 1: add results from a hardcoded list of top sites, https://reviewboard.mozilla.org/r/98586/#review107014 ::: modules/libpref/init/all.js:1064 (Diff revision 8) > pref("browser.fixup.dns_first_for_single_words", false); > pref("browser.fixup.hide_user_pass", true); > > // Location Bar AutoComplete > pref("browser.urlbar.autocomplete.enabled", true); > +pref("browser.urlbar.prefillsites.enabled", true); I think this name unfortunately might still be misunderstood. How about "browser.urlbar.usepreloadedtopurls.enabled" ? ::: toolkit/components/places/UnifiedComplete.js:61 (Diff revision 8) > +const TOP500_SITES = [ > + ["Google.com", "Google"], > + ["Youtube.com", "YouTube"], > + ["Facebook.com", "Facebook"], > + ["Baidu.com", "\u767E\u5EA6\u4E00\u4E0B\uFF0C\u4F60\u5C31\u77E5\u9053"], > + ["Wikipedia.org", "Wikipedia"], > + ["toop500.org", "TOP500 test site"], > +]; > +const TOP500_FRECENCY = 1; It looks like this can now be removed? ::: toolkit/components/places/UnifiedComplete.js:137 (Diff revision 8) > +function addPrefillSite(url, title) { > + prefillSites.push( new PrefillSite(url, title) ); > +} Nit: here and below, we don't normally have spacing between the opening and closing () of a method call and the first/last argument(s), or inside the () of an if statement (so use `if (foo)` rather than `if ( foo )` ::: toolkit/components/places/UnifiedComplete.js:137 (Diff revision 8) > +} > + > +var prefillSites = []; > +const PREFILL_SITE_FRECENCY = 1; > + > +function addPrefillSite(url, title) { So, if in the UnifiedComplete object, you include this helper (you can just reference it, so after the `registerOpenPage` and `unregisterOpenPage` methods here: http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/toolkit/components/places/UnifiedComplete.js#2018 just insert: `addPrefillSite,` and it will be exposed ), you can then call it from the test you're writing as follows: ```js let ac = Cc["@mozilla.org/autocomplete/search;1?name=unifiedcomplete"] .getService(Ci.mozIPlacesAutoComplete); ac.addPrefillSite("test-top-500.org", "Testing the top 500"); ``` and then check that the result shows up in the test. ::: toolkit/components/places/UnifiedComplete.js:1036 (Diff revision 8) > // Ensure to fill any remaining space. Suggestions which come from extensions are > // inserted at the beginning, so any suggestions > yield Promise.all(this._remoteMatchesPromises); > }), > > + _matchTop500() { This is unused, too, I think? :-) ::: toolkit/components/places/UnifiedComplete.js:1059 (Diff revision 8) > + if (site._lowerURL.includes(this._searchString) || > + site._lowerTitle.includes(this._searchString)) { > + let match = { > + value: site.url, > + comment: site.title, > +// style: "autofill", Yeah, I think we should keep these as history results for now. For autofill, I'm not sure if we would need to add the same entry once as an autofill result and once normally, or only as an autofill result and that will automatically add the simple row in the dropdown. Marco will know. Either way, we're currently substring matching for the items in the list, and autofill results should be matched with `startsWith`.
Attachment #8818509 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs from comment #38) > ::: toolkit/components/places/UnifiedComplete.js:1059 > (Diff revision 8) > > + if (site._lowerURL.includes(this._searchString) || > > + site._lowerTitle.includes(this._searchString)) { > > + let match = { > > + value: site.url, > > + comment: site.title, > > +// style: "autofill", > > Yeah, I think we should keep these as history results for now. For autofill, > I'm not sure if we would need to add the same entry once as an autofill > result and once normally, or only as an autofill result and that will > automatically add the simple row in the dropdown. Marco will know. Marco, can you clarify? For instance, what happens if we have a match for "google.com" and my history has a match for "gijsk.com" to autofill with (so presumably the history match overrides this one)? Does 'google.com' still appear in the list in the popup, even if we use style=autofill, or do we need to add two matches for that to happen?
Flags: needinfo?(mak77)
> Either way, we're currently substring matching for the items in the list, > and autofill results should be matched with `startsWith`. Maybe substring matching is not the proper thing in this case. User types "y" - he wants "Youtube" or "Yahoo", but not "ebaY". Although if he types "b", he may want "eBay" or "faceBook"...
(In reply to Svetlana Orlik from comment #40) > > Either way, we're currently substring matching for the items in the list, > > and autofill results should be matched with `startsWith`. > > Maybe substring matching is not the proper thing in this case. > User types "y" - he wants "Youtube" or "Yahoo", but not "ebaY". > Although if he types "b", he may want "eBay" or "faceBook"... For history, we use substring matching as well, so I don't know that we want something else for these items. I agree though that matches that match at the start should ideally come first.
(In reply to :Gijs from comment #38) > Yeah, I think we should keep these as history results for now. For autofill, > I'm not sure if we would need to add the same entry once as an autofill > result and once normally, or only as an autofill result and that will > automatically add the simple row in the dropdown. Marco will know. Autofill is an heuristic result (filling the action row). If you want both autoFill AND eventually an history entry, you have to add both (one to _matchFirstHeuristicResult and one as last resource before the Promise.all). But I'd not see the reason, one of the 2 should suffice for the scope of this bug, since we also want things to disappear when new history is made? It's just matter of UX, I guess. (In reply to :Gijs from comment #39) > Marco, can you clarify? For instance, what happens if we have a match for > "google.com" and my history has a match for "gijsk.com" to autofill with (so > presumably the history match overrides this one)? Does 'google.com' still > appear in the list in the popup, even if we use style=autofill, or do we > need to add two matches for that to happen? If you add your google.com match as autoFill, and gijsk.com is also an autofill result, gijsk.com will win just because you will try to add your autofill result later in the code. If you look at _matchFirstHeuristicResult, as soon as we get a match, we bailout. So we'd match gijsk.com and bail out before we reach the code adding google.com code. Note that not everything in history autoFills. And, as I said before, if you want both autofill and an history entry, you need code in both places. Note that addMatch does deduplication (some sort of), so if you add your result as autofill, and then also add it as an history result, the latter will be filtered out (our policy is to have no duped urls in autocomplete). This happens transparently as long as the uris are identical, so it would pretty well cover your need (if you want both).
Flags: needinfo?(mak77)
Comment on attachment 8818509 [details] Bug 1211726 - part 1: add results from a hardcoded list of top sites, https://reviewboard.mozilla.org/r/98586/#review107286 It seems the only change here is that the test got renamed, so clearing review for now. :-)
Attachment #8818509 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8818509 [details] Bug 1211726 - part 1: add results from a hardcoded list of top sites, https://reviewboard.mozilla.org/r/98586/#review107688 Svetlana and I talked on IRC - clearing review for now. Next steps include expanding the test, adding autofill and expiring the feature (by flipping the pref) after 2 weeks.
Attachment #8818509 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8818509 [details] Bug 1211726 - part 1: add results from a hardcoded list of top sites, https://reviewboard.mozilla.org/r/98586/#review108864 ::: toolkit/components/places/UnifiedComplete.js:117 (Diff revision 11) > FROM moz_bookmarks b > JOIN moz_bookmarks t ON t.id = +b.parent AND t.parent = :parent > WHERE b.fk = h.id > ) AS tags`; > > +function PrefillSite(host, title) { Nit: "host" is normally used for "google.com", so technically what you're passing would be more accurately called a url or uri. Can you update the name of the property 'this.host' and the argument to this function? ::: toolkit/components/places/UnifiedComplete.js:120 (Diff revision 11) > + this._matchHost = stripPrefix(host).toLowerCase(); > + this._matchTitle = title.toLowerCase(); Nice! (To be very clear, calling this `_matchHost` *is* correct, because after you strip the prefix (ie "https://") only the host is left.) ::: toolkit/components/places/UnifiedComplete.js:131 (Diff revision 11) > + > +function addPrefillSite(host, title) { > + prefillSites.push(new PrefillSite(host, title)); > +} > + > +addPrefillSite( "https://Google.com", "Google" ); Nit: here and for the other calls, there's still spacing before the first and after the last argument that should go away. :-) ::: toolkit/components/places/UnifiedComplete.js:408 (Diff revision 11) > WHERE url = :url > AND userContextId = :userContextId` > , { url: uri.spec, userContextId }); > }, > > - shutdown() { > + shutdown: function() { Yeah, these changes should be reverted. You might even be able to use: ``` ./mach eslint --fix toolkit/components/places/UnifiedComplete.js ``` to do it automatically. ::: toolkit/components/places/UnifiedComplete.js:1033 (Diff revision 11) > +2. Should we use the workaround for protocol and "www." like in _matchSearchEngineUrl()? > + Change the _matchSearchEngineUrl() code for reuse? (Is changing appropriate?) > +3. ESlint errors - fix? New branch/bug? Or leave? > +4. style: "autofill" - Shows host instead of title, and "Visit" instead of host. > + Hence, doubles the result. > +4. Interference with History / Known URLs - doubling. Research. I answered these via email. :-) ::: toolkit/components/places/UnifiedComplete.js:1036 (Diff revision 11) > +4. style: "autofill" - Shows host instead of title, and "Visit" instead of host. > + Hence, doubles the result. > +4. Interference with History / Known URLs - doubling. Research. > + > +TODO: > +- Handle prference like other ones. Can you clarify what you mean by this? ::: toolkit/components/places/tests/unifiedcomplete/test_prefill_sites.js:18 (Diff revision 11) > + .getService(Ci.mozIPlacesAutoComplete); > + > +add_task(function* test_list_results() { > + // Trailing slash is mandatory > + autocompleteObject.addPrefillSite("https://test-top-abracadabra-site.org/", > + "Testing the top 500"); Nit: Here and below reviewboard highlights tabs in red. Please replace them with spaces. :-) ::: toolkit/components/places/tests/unifiedcomplete/test_prefill_sites.js:56 (Diff revision 11) > + do_print("Prefence can turn Autofill off"); > + yield check_autocomplete({ > + search: "test-top-abracadabra-", > + autofilled: "test-top-abracadabra-", > + completed: "test-top-abracadabra-", > + }); I'm confused by this bit of the test. Shouldn't there be no autofilled/completed match because we turned the feature off? ::: toolkit/components/places/tests/unifiedcomplete/test_prefill_sites.js:68 (Diff revision 11) > + yield cleanup(); > +}); > + > + > +add_task(function* test_list_results_sorting() { > + Nit: please remove the extra line of whitespace here. ::: toolkit/components/places/tests/unifiedcomplete/test_prefill_sites.js:81 (Diff revision 11) > + yield check_autocomplete({ > + checkSorting: true, Marco would know for sure, but I think this already checks that there are no other items and that the items in question are sorted as they are in the array you're passing, so I think this is a good test? :-)
Attachment #8818509 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs from comment #48) > ::: toolkit/components/places/tests/unifiedcomplete/test_prefill_sites.js:81 > (Diff revision 11) > > + yield check_autocomplete({ > > + checkSorting: true, > > Marco would know for sure, but I think this already checks that there are no > other items and that the items in question are sorted as they are in the > array you're passing, so I think this is a good test? :-) yes, check_autocomplete verifies that there is same number of results and that each result matches. The checkSorting option additionally ensures the sorting is the same.
Comment on attachment 8818509 [details] Bug 1211726 - part 1: add results from a hardcoded list of top sites, https://reviewboard.mozilla.org/r/98586/#review108864 > Can you clarify what you mean by this? I mean like other preferences, make a constant like: const PREF_ENABLED = [ "autocomplete.enabled", true ]; const PREF_AUTOFILL = [ "autoFill", true ]; const PREF_AUTOFILL_TYPED = [ "autoFill.typed", true ]; and so on. For uniformity. > I'm confused by this bit of the test. Shouldn't there be no autofilled/completed match because we turned the feature off? As I understand, "search" is what you type, "autofilled" is what you get in response in the url bar, and "completed" is what you get there when you hit enter. So if they are all equal - it's the proof there was no autofill (knowing we have a suitable prefill site). > Marco would know for sure, but I think this already checks that there are no other items and that the items in question are sorted as they are in the array you're passing, so I think this is a good test? :-) It checks that our entry is lower than bookmark and history entries only. No search suggestions or remote tabs or anyting else. Also sorting a bookmark against history is hardcoded here, which is not really fine.
(In reply to Svetlana Orlik from comment #50) > Comment on attachment 8818509 [details] > Bug 1211726 - part 1: add results from a hardcoded list of top sites, > > https://reviewboard.mozilla.org/r/98586/#review108864 > > > Can you clarify what you mean by this? > > I mean like other preferences, > make a constant like: > const PREF_ENABLED = [ "autocomplete.enabled", true ]; > const PREF_AUTOFILL = [ "autoFill", true ]; > const PREF_AUTOFILL_TYPED = [ "autoFill.typed", true ]; > and so on. > For uniformity. OK, that makes sense. > > I'm confused by this bit of the test. Shouldn't there be no autofilled/completed match because we turned the feature off? > > As I understand, "search" is what you type, "autofilled" is what you get in > response in the url bar, and "completed" is what you get there when you hit > enter. So if they are all equal - it's the proof there was no autofill > (knowing we have a suitable prefill site). Ah, right. A comment would be useful, I think. > > Marco would know for sure, but I think this already checks that there are no other items and that the items in question are sorted as they are in the array you're passing, so I think this is a good test? :-) > > It checks that our entry is lower than bookmark and history entries only. No > search suggestions or remote tabs or anyting else. I think that's OK, per comment #49 from Marco. > Also sorting a bookmark against history is hardcoded here, which is not > really fine. Ah. If we wanted to avoid that, we could do two separate test assertions, one with a string that matches a bookmark and the prefill site (but not history), and one with a string that matches history and the prefill site (but not the bookmark). With that, given comment #49, I think this is fine.
> > It checks that our entry is lower than bookmark and history entries only. No > > search suggestions or remote tabs or anyting else. > > I think that's OK, per comment #49 from Marco. But shouldn't we check that our entry is lower than, say, Remote Tab suggestion?
(In reply to Svetlana Orlik from comment #52) > > > It checks that our entry is lower than bookmark and history entries only. No > > > search suggestions or remote tabs or anyting else. > > > > I think that's OK, per comment #49 from Marco. > > But shouldn't we check that our entry is lower than, say, Remote Tab > suggestion? Oh, interesting. Yes, probably, though if adding an automated test for this is hard and you've tested that in practice, this is currently working as designed, I'm happy to do that in a followup bug. If you do want to add an automated test for it now, it might be easiest to do it as an added task in https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/unifiedcomplete/test_remote_tab_matches.js .
Comment on attachment 8818509 [details] Bug 1211726 - part 1: add results from a hardcoded list of top sites, https://reviewboard.mozilla.org/r/98586/#review109214 As noted on IRC, code-wise this looks fine to me besides what you already note in the patch is still missing: tidying up the pref, and adding the ProfileAge-related expiry check.
Attachment #8818509 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8818509 [details] Bug 1211726 - part 1: add results from a hardcoded list of top sites, https://reviewboard.mozilla.org/r/98586/#review110586 I think this is basically ready to go as a proof of concept, with the nits below addressed. We can do everything else (storing a list of URLs in a file, improving expiry, etc.) in followup bugs. Thanks for all your hard work! I have added my nits below, but I'm also going to request review from Marco, who should make sure that I'm not crazy when I say this can land on Nightly, preffed off everywhere else. :-) ::: modules/libpref/init/all.js:1059 (Diff revision 14) > pref("browser.fixup.dns_first_for_single_words", false); > pref("browser.fixup.hide_user_pass", true); > > // Location Bar AutoComplete > pref("browser.urlbar.autocomplete.enabled", true); > +pref("browser.urlbar.usepreloadedtopurls.enabled", true); Can you wrap this in an ifdef like so: #ifdef NIGHTLY_BUILD pref("browser.urlbar.usepreloadedtopurls.enabled", true); #else pref("browser.urlbar.usepreloadedtopurls.enabled", false); #endif so we can perfect this on Nightly and not ship until we're ready? :-) (The expiry days thing should stay outside the ifdef.) ::: toolkit/components/places/UnifiedComplete.js:554 (Diff revision 14) > +// Prefill Sites related > + > +let profileCreationDatePromise = (new ProfileAge(null, null)).created; > + > +function PrefillSite(url, title) { > +// this.uri = NetUtil.newURI(url); Nit: here and elsewhere, can you remove the commented out lines of code? ::: toolkit/components/places/UnifiedComplete.js:561 (Diff revision 14) > + this.title = title; > + this._matchHost = stripPrefix(url).toLowerCase(); > + this._matchTitle = title.toLowerCase(); > +} > + > +var prefillSites = []; Nit: judging by the naming of "Prefs" and "SwitchToTabStorage", I guess we should call this "PrefillSiteStorage" or something else starting with a capital letter (I'm adding "Storage" to avoid confusion with the "PrefillSite" constructor). ::: toolkit/components/places/UnifiedComplete.js:1045 (Diff revision 14) > + *_checkPrefillSitesExpiry() { > + if (!Prefs.prefillSitesEnabled) > + return; > + let profileCreationDate = yield profileCreationDatePromise; > + const ONE_DAY = 24 * 60 * 60 * 1000; > + let daysSinceProfileCreation = ( Date.now() - profileCreationDate) / ONE_DAY Nit: no space after the first `(`, and please add a semicolon at the end. ::: toolkit/components/places/mozIPlacesAutoComplete.idl:139 (Diff revision 14) > * @param aUserContextId > * The Container Id of the tab. > */ > void unregisterOpenPage(in nsIURI aURI, in uint32_t aUserContextId); > + > + void addPrefillSite(in AUTF8String url, in AUTF8String title); Nit: add a docstring comment like for the method above this one. ::: toolkit/components/places/tests/unifiedcomplete/test_prefill_sites.js:26 (Diff revision 14) > +autocompleteObject.addPrefillSite(yahoooURI.spec, "Yahooo"); > +autocompleteObject.addPrefillSite(gooogleURI.spec, "Gooogle"); > + > + > +function *assert_feature_works(condition) { > + Nit: here and below, remove the empty lines at the start and end of functions.
Attachment #8818509 - Flags: review?(gijskruitbosch+bugs) → review+
Attachment #8818509 - Flags: review?(mak77)
Comment on attachment 8818509 [details] Bug 1211726 - part 1: add results from a hardcoded list of top sites, Reinstating the r?mak...
Attachment #8818509 - Flags: review?(mak77)
Blocks: 1336946
Comment on attachment 8818509 [details] Bug 1211726 - part 1: add results from a hardcoded list of top sites, https://reviewboard.mozilla.org/r/98586/#review111526 I do have some comments, especially about some additional lazy getters, but nothing blocking. I leave to Gijs the finally-final-review. I love all those ooooooo in the test \o/\o/\o/ ::: toolkit/components/places/UnifiedComplete.js:51 (Diff revision 15) > const PREF_SUGGEST_SEARCHES = [ "suggest.searches", false ]; > > const PREF_MAX_CHARS_FOR_SUGGEST = [ "maxCharsForSearchSuggestions", 20]; > > +const PREF_PREFILL_SITES_ENABLED = [ "usepreloadedtopurls.enabled", true ]; > +const PREF_PREFILL_SITES_EXPIRE_DAYS = [ "usepreloadedtopurls.expire_days", 1 ]; nit: a 1 day default sounds a little bit "random" picked. maybe 7 or 14? ::: toolkit/components/places/UnifiedComplete.js:551 (Diff revision 15) > return Object.seal(store); > }); > > +// Prefill Sites related > + > +let profileCreationDatePromise = (new ProfileAge(null, null)).created; What's the point of lazy loading ProfileAge, if in the global object we immediatly use it like this? Personally I'd rather use defineLazyGetter to define profileCreationDatePromise, and import the module inside it (we don't need it anywhere else). That way when the feature is not enabled, we won't need to load and run ProfileAge. ::: toolkit/components/places/UnifiedComplete.js:554 (Diff revision 15) > +// Prefill Sites related > + > +let profileCreationDatePromise = (new ProfileAge(null, null)).created; > + > +function PrefillSite(url, title) { > + this.url = url; you could store a URL object in url, and you'd get _matchHost for free through url.host Moreover by storing a URL you get input checking for free. Remember you are exposing an API, you can't trust your input. ::: toolkit/components/places/UnifiedComplete.js:568 (Diff revision 15) > +function addPrefillSite(url, title) { > + let site = new PrefillSite(url, title); > + prefillSites.push(site); > +} > + > +addPrefillSite("https://Google.com", "Google"); For the same reason, I'd defineLazyGetter prefillSites, so we only build it when needed. Also, please make these proper urls... Why are the domains uppercase, and why are they missing the trailing "/"? (there may be a reason, that I don't know) ::: toolkit/components/places/UnifiedComplete.js:1043 (Diff revision 15) > + > + *_checkPrefillSitesExpiry() { > + if (!Prefs.prefillSitesEnabled) > + return; > + let profileCreationDate = yield profileCreationDatePromise; > + const ONE_DAY = 24 * 60 * 60 * 1000; nit: not a big deal, I'd also be fine if you just define once at the top of the file: const MS_PER_DAY = 86400000; ::: toolkit/components/places/UnifiedComplete.js:1058 (Diff revision 15) > + if (site._matchHost.includes(this._searchString) || > + site._matchTitle.includes(this._searchString)) { > + let match = { > + value: site._matchHost, > + comment: site.title, > + style: "prefill-site", We could try to guess an icon here, just use `page-icon:${site.url}` and if we have one we'll return it, otherwise it will just be the same default favicon. ::: toolkit/components/places/UnifiedComplete.js:1060 (Diff revision 15) > + let match = { > + value: site._matchHost, > + comment: site.title, > + style: "prefill-site", > + finalCompleteValue: site.url, > + frecency: PREFILL_SITE_FRECENCY, Just use FRECENCY_DEFAULT - 1 for now and remove PREFILL_SITE_FRECENCY, we don't have frecency mixup atm, so it doesn't really matter. The only thing that matters is that in case remote matches can push this away, and thus it should just be below FRECENCY_DEFAULT. ::: toolkit/components/places/tests/head_common.js:82 (Diff revision 15) > // Initialize profile. > var gProfD = do_get_profile(); > > +Services.prefs.setBoolPref("browser.urlbar.usepreloadedtopurls.enabled", false); > +do_register_cleanup(() => > + Services.prefs.clearUserPref("browser.urlbar.usepreloadedtopurls.enabled")); Isn't prefs_general enough to set this to false? (I'm not 100% sure it covers all the harnesses, but it sounds dumb if it doesn't cover xpcshell) ::: toolkit/components/places/tests/unifiedcomplete/test_prefill_sites.js:3 (Diff revision 15) > +/* 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/. */ nit: If you don't mind, I'd prefer to keep test code Public Domain. That means basically just remove the license header. But it's your code so up to you. ::: toolkit/components/places/tests/unifiedcomplete/test_prefill_sites.js:19 (Diff revision 15) > + .getService(Ci.mozIPlacesAutoComplete); > + > +// With or without trailing slash - no matter. URI.spec does have it always. > +// Then, check_autocomplete() doesn't cut it off (uses stripPrefix()). > +let yahoooURI = NetUtil.newURI("https://yahooo.com/"); > +let gooogleURI = NetUtil.newURI("https://gooogle.com"); nit: let's be consistent with trailing slashes ::: toolkit/components/places/tests/unifiedcomplete/xpcshell.ini:49 (Diff revision 15) > [test_trimming.js] > [test_typed.js] > [test_visit_url.js] > [test_word_boundary_search.js] > [test_zero_frecency.js] > +[test_prefill_sites.js] these ini files should be keps in alphabetical order.
Attachment #8818509 - Flags: review?(mak77) → review+
(In reply to Marco Bonardo [::mak] from comment #61) > > +Services.prefs.setBoolPref("browser.urlbar.usepreloadedtopurls.enabled", false); > > +do_register_cleanup(() => > > + Services.prefs.clearUserPref("browser.urlbar.usepreloadedtopurls.enabled")); > > Isn't prefs_general enough to set this to false? (I'm not 100% sure it > covers all the harnesses, but it sounds dumb if it doesn't cover xpcshell) prefs_general.js is used in mochitest-* profiles, but xpcshell doesn't use those so no, this doesn't cover xpcshell. It surprised me, too! Happy to file a followup to get xpcshell to use these if that's possible (not entirely sure because xpcshell vs. profiles is a complicated story).
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8818509 [details] Bug 1211726 - part 1: add results from a hardcoded list of top sites, https://reviewboard.mozilla.org/r/98586/#review114114 This looks good to me. I'll push to try and then land, if all is well. ::: toolkit/components/places/UnifiedComplete.js (Diff revisions 16 - 17) > if (site.uri.host.includes(this._searchString) || > site._matchTitle.includes(this._searchString)) { > let match = { > value: site.uri.spec, > comment: site.title, > - // style: `page-icon:${site.uri.spec}`, So I think this would work if you used: ```js let match = { ..., icon: `page-icon:${site.uri.spec}`, }; ``` However, I tested it, and I don't see any difference because we (presumably?) merge entries with history-related ones if there are any, and if there aren't any history entries then there is also no icon. So I don't think it makes a difference.
Flags: needinfo?(gijskruitbosch+bugs)
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/1bf558a9bf87 part 1: add results from a hardcoded list of top sites, r=Gijs,mak
https://hg.mozilla.org/mozilla-central/rev/1bf558a9bf87
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Depends on: 1340108
is there already a bug filed to check what is needed to let this reach release?
(In reply to Marco Bonardo [::mak] from comment #68) > is there already a bug filed to check what is needed to let this reach > release? Not yet, feel free to file one.
Blocks: 1340663
Depends on: 1341081
Blocks: 1344276
(In reply to :Gijs from comment #12) > Some notes: > > This is a mentored implementation bug. Please take concerns about the > eventual route-to-shipping of it to email or a separate bug so the bug can > stay focused on the specific set of patches here. They'll be behind a pref > so we're not tying ourselves to shipping this to everybody in whatever cycle > this lands in. > > The suggestion in the bug has already been implemented by us before, but > right now that behaviour is only available in Fennec. I am not aware of any > negative feedback about it there. The presence of a domain in the list of > completions when typing in the location bar also has very different > characteristics to tiles or ads, and the list will be objectively sourced > rather than special-cased based on "deals", so it seems pretty far-fetched > that people will take offence at it. It will also be displaced by history > (unlike ads or "enhanced" tiles and the like). I just came across this in Fennec with no way to disable it. I couldn't find anything in about:config resembling the proposed pref above; regardless, the ability to do so should be surfaced, not buried ("do you want suggestions of top sites?" when first typing or similar) For people who want their browser to do what they tell it, this is just annoying. Worse, for *everyone*, it also entrenches dominant commercial entities whose domains start with their business type. * "Hey, I want to open a new bank account" * b..a...n... * ...kofamerica.com -> Bank of America gets free advertising because they have the right name/domain structure and are big enough to be high up on Alexa and therefore included/suggested. Not cool.
This is a Firefox Desktop bug, and the feature is not enabled on Desktop. The mobile version has a completely different code base, you should file your issue in the Firefox for Android product.
(In reply to mozilla from comment #70) > I just came across this in Fennec with no way to disable it. This bug isn't about Fennec. I don't know much about their implementation, and was not involved with it. Raise concerns with fennec's implementation with them, e.g. via the mobile-firefox-dev list - https://mail.mozilla.org/listinfo/mobile-firefox-dev . The extant *desktop* implementation is disabled by default right now. However, this is *also* not the right place to raise concerns about the desktop feature. Consider using https://wiki.mozilla.org/Firefox/firefox-dev instead, but be aware that: - there's a pref for this feature, so if you feel very strongly you can turn it off - even if we turn it on, it will turn itself off on any profile older than 2 weeks (or after a new profile has been in use for 2 weeks). I think both of these aspects mitigate your concerns, but as noted, this is not the right place to continue to discuss it.
OK, I mentioned Fennec as that was what led me to discover this feature was being introduced on both mobile and desktop. I will file a bug for Fennec for the lack of a pref there. I hope that someone involved in the desktop implementation can see their way to providing a *visible* way of disabling this up front (as is done with search suggestions). It's still unfortunate that it effectively gives free keyword advertising to several companies. Auto Trader's another example, as is Credit Karma. The full list will be far longer. Basically any new user who wants to search for banks, autos, credit matters, whatever, will get offered a specific company's site based on market dominance and convenient naming.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: