Closed Bug 1163888 Opened 5 years ago Closed 5 years ago
No favicon for "host" autocomplete entries
Best viewed in the screenshot at in attachment 8603949 [details]. In the entry labeled "Unified=on: enter", note how "Visit arstechnica.com" doesn't display the favicon for the site even though the entry below (an article on arstechnica from history) does.
There are 2 things conspiring against us here: * Entries from the "hosts" table aren't complete URLs, just host names. Thus, when we execute |NetUtil.newURI(untrimmedHost)| we are passing "arstechnica.com" which fails with NS_ERROR_MALFORMED_URI. I fixed this by passing the hostname to Services.uriFixup.getFixupURIInfo() with no flags. This gives us a valid URL (but I'm not sure the flags are correct - or even if that's the correct way to get the full URL) * The code naively assumes a path of /favicon.ico is correct, which is often wrong. In the arstechnica case, the favicon is https://cdn.arstechnica.net/favicon.ico. Obtaining the correct favicon for the host requires calling PlacesUtils.promiseFaviconLinkUrl() - which returns a promise - but none of the _process*Row() functions are setup to handle promises. So I also changed _onResultRow() so the returned "match" objects can be promises and this._addMatch() is called when it resolves. This patch works for me. It has no tests and I'm not particularly confident with the promise changes, but worth getting feedback!
Attachment #8604440 - Flags: feedback?(mak77)
Comment on attachment 8604440 [details] [diff] [review] 0006-Bug-1163888-ensure-autocomplete-host-matches-display.patch Review of attachment 8604440 [details] [diff] [review]: ----------------------------------------------------------------- the part about building a complete host looks good and we could land it. but the promise part looks a little bit hack-ish, and I fear it might be fragile to future changes. It might also end up adding matches in unexpected order, if I read it correctly. The best solution would probably be to add a JOIN with moz_favicons in the queries, but that might slowdown the queries. Could be worth a try though. For example, you can try this query in Sqlite Manager to check which hosts have a favicon: SELECT host, f.url FROM moz_hosts LEFT JOIN moz_places h ON h.url = IFNULL(prefix, "http://") || host || "/" LEFT JOIN moz_favicons f ON h.favicon_id = f.id WHERE f.url NOTNULL From what I see on my database the guess we do with /favicon.ico is very bad. Using a similar query, we could basically add a subquery in the host/url queries where iconurl is supposed to be fetched (const QUERYINDEX_ICONURL = 3; so the 4th column in the query) (SELECT f.url FROM moz_favicons f JOIN moz_places h ON h.favicon_id = f.id WHERE h.url = IFNULL(prefix, "http://") || host || "/") Then it would only be matter of teaching _processHostRow to fetch QUERYINDEX_ICONURL, like _processRow does. changing urlQuery should be easier since there we already have h.url, a simple LEFT JOIN with moz_favicons should do. The reason I didn't join originally was to save some resources, but since doesn't look like we have cheaper solutions, we should probably do it properly.
Attachment #8604440 - Flags: feedback?(mak77) → feedback-
as a side note, since that might look surprising, '||' is the append-string magic wand in SQLite.
Comment on attachment 8604440 [details] [diff] [review] 0006-Bug-1163888-ensure-autocomplete-host-matches-display.patch (In reply to Marco Bonardo [::mak] from comment #2) > but the promise part looks a little bit hack-ish, and I fear it might be > fragile to future changes. It might also end up adding matches in unexpected > order, if I read it correctly. Well, lots of this world is already promise based - this code called from sqlite completion callbacks already, right? > The best solution would probably be to add a JOIN with moz_favicons Hmm - so even though we already have async functions (one promise-based, the other callback-based) that perfectly solves our problem, we can't use it here and instead must add many complex lines of sub-queries with SQL and related support code etc? So yeah, count me out :)
Attachment #8604440 - Attachment is obsolete: true
(In reply to Mark Hammond [:markh] from comment #4) > Hmm - so even though we already have async functions (one promise-based, the > other callback-based) that perfectly solves our problem, we can't use it > here and instead must add many complex lines of sub-queries with SQL and > related support code etc? The problem is that we must retain certainty of the order results are added, that would require, the promise solution has an high risk of adding races where that might not be true anymore. The only viable fix would be to make the onRow handler of Sqlite.jsm run tasks instead of simple callbacks, but then we'd have risk that some consumer yields infinitely blocking the connection. The other fact is that promiseFaviconLinkUrl is much slower than directly querying, since it has to run a similar query, but with more overhead. The 2 things are basically doing the same, but one is more performant.
Dave, what's the right way to nominate something for triage for the team backlog? Is this OK?
I think so but I'll defer to Shell
Flags: needinfo?(dtownsend) → needinfo?(sescalante)
I'll take this bug as a side project, while it's not blocking unified complete, it's polish work for it, and the solution is basically already expressed in the previous comments. I just need to figure out a testing story.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Attachment #8612912 - Flags: review?(markh) → review+
Points: --- → 3
You need to log in before you can comment on or make changes to this bug.