topSites frecency results should match URL bar re hidden=1 places
Categories
(WebExtensions :: General, defect, P3)
Tracking
(Not tracked)
People
(Reporter: jscher2000, Unassigned)
Details
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:77.0) Gecko/20100101 Firefox/77.0
Steps to reproduce:
I created an extension that uses the topSites API to list frecent sites. (https://addons.mozilla.org/firefox/addon/show-history-top-sites-button/) I compared the results with the list available from the address bar using ^ or a space (native frecent list)
Actual results:
The topSites API omits URLs where hidden=1 in moz_places, but these URLs are included in the native frecent list results. The API uses ActivityStreamProvider.getTopFrecentSites() which uses _commonPlacesWhere in the SQL WHERE clause and that requires hidden = 0.
https://searchfox.org/mozilla-central/source/toolkit/modules/NewTabUtils.jsm#1267
Expected results:
The topSites API should have parity with the address bar results, especially due to user migration to extensions to replace the "Show History" button removed from the address bar in Firefox 75.
(I don't know why one of my most frequently used sites has hidden = 1 in the first place, but since there is no UI to undo that, and since it seems to be disregarded in address bar lists, I would prefer that the API ignore it.)
|
Reporter | |
Updated•5 years ago
|
|
||
Comment 1•5 years ago
|
||
Drew, could you triage this? There are enough changes here I'm not certain what the expected behavior is.
|
||
Comment 2•5 years ago
|
||
Hidden has to do with redirects -- they can be marked hidden, but not always.
The request seems reasonable to me:
Expected results:
The topSites API should have parity with the address bar results, especially due to user migration to extensions to replace the "Show History" button removed from the address bar in Firefox 75.
We could modify ActivityStreamProvider.getTopFrecentSites to optionally include hidden URLs, but the problem is passing that option from ext-topSites.js all the way down to the function. My understanding is that the search/urlbar team will be taking over the top sites code from Pocket or user journey or whoever owns it right now, so that might be more feasible in the future.
We could also just add a new API or option to the top sites API -- or history? -- that bypasses ActivityStreamProvider/AboutNewTab/NewTabUtils altogether and calls into Places/urlbar directly so it can use the same SQL query we use in the urlbar when you type ^ or space. That might be the better option.
I'll go ahead and set priority and severity since you asked me to triage, but please update them if you disagree.
|
|
||
Comment 3•5 years ago
|
||
Thanks for that Drew. I'll ni?standard8 to look at this as well, if search/urlbar team are taking over top sites code.
|
|
||
Comment 4•5 years ago
|
||
Sorry, I probably should have just said the urlbar team -- the urlbar/search team is kind of one combined team, but there's a group of us focused on the search service and related code and another on urlbar. My understanding is that since top sites are relevant to urlbar, it'll be the urlbar part of the team taking it over. I'm not sure when that's supposed to happen. I can ask Mike in our 1:1 tomorrow for details and get back to you.
|
|
||
Comment 5•5 years ago
|
||
Also, the more I think about this, the more option 2 seems like the way to go. The top sites API is really about the sites that appear on about:newtab, and now the sites that appear when you focus the urlbar as well. What jscher2000 is requesting isn't actually related to that. jscher2000 wants the list of most frecent history, and that's owned by Places/urlbar. Instead of modifying the activity stream SQL query to match the Places/urlbar query (and keeping it in sync), we should use the Places/urlbar query.
|
||
Updated•5 years ago
|
|
|
Reporter | |
Comment 6•5 years ago
|
||
Just a note that the API will still have a connection to new tab:
- browser.topSites.get({newtab: true}) => Top Sites from the new tab
- browser.topSites.get({newtab: false}) => frecent sites
but you could change the function call for the false case here:
https://searchfox.org/mozilla-central/source/browser/components/extensions/parent/ext-topSites.js#31
|
|
Reporter | |
Comment 7•5 years ago
|
||
(In reply to jscher2000 from comment #0)
(I don't know why one of my most frequently used sites has hidden = 1 in the first place, but since there is no UI to undo that, and since it seems to be disregarded in address bar lists, I would prefer that the API ignore it.)
After further review, that URL is redirected with a new session ID in the URL on each visit. Redirected URLs that I use all the time can be frecent for purposes of the address bar but not for purposes of Top Sites because they have hidden=1.
Description
•