No favicon for "host" autocomplete entries

RESOLVED FIXED in Firefox 41

Status

()

Firefox
Location Bar
P2
normal
Rank:
25
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: markh, Assigned: mak)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 41
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox41 fixed)

Details

(Whiteboard: [unifiedautocomplete][fxsearch])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
Created attachment 8604440 [details] [diff] [review]
0006-Bug-1163888-ensure-autocomplete-host-matches-display.patch

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)
(Assignee)

Comment 2

2 years ago
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-
(Assignee)

Comment 3

2 years ago
as a side note, since that might look surprising, '||' is the append-string magic wand in SQLite.
(Reporter)

Comment 4

2 years ago
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
(Assignee)

Comment 5

2 years ago
(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.
(Assignee)

Comment 6

2 years ago
Dave, what's the right way to nominate something for triage for the team backlog? Is this OK?
Flags: needinfo?(dtownsend)
Flags: firefox-backlog+
Whiteboard: [unifiedautocomplete][fxsearch]
I think so but I'll defer to Shell
Flags: needinfo?(dtownsend) → needinfo?(sescalante)
(Assignee)

Comment 8

2 years ago
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

Updated

2 years ago
Rank: 25
Flags: needinfo?(sescalante)
Priority: -- → P2
(Assignee)

Comment 9

2 years ago
Created attachment 8612912 [details] [diff] [review]
patch v1
Attachment #8612912 - Flags: review?(markh)
(Reporter)

Updated

2 years ago
Attachment #8612912 - Flags: review?(markh) → review+

Comment 10

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/7969c82a42df
(Assignee)

Updated

2 years ago
Points: --- → 3
Flags: qe-verify-
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/7969c82a42df
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Blocks: 1158856
You need to log in before you can comment on or make changes to this bug.