1186665 - FaviconFetcher should try alternate urls is the requested one doesn't have anything nice

Status: RESOLVED WONTFIX

(Firefox for iOS :: Favicons, defect)

Description

[Wesley Johnston (:wesj)]

We can look at some other urls if the main one fails. There's chances for errors here as well, so this would need some bake time?

Comment 1

[Wesley Johnston (:wesj)]

Attachment: PR https://github.com/mozilla/firefox-ios/pull/787

This fallbacks through a series of possibilities. i.e. something like:

http://real-url-in-database       // This may redirect, but Alamofire does that for us
http://mail.google.com/u/234/asdf // Resolved url
http://mail.google.com/           // Remove any path components
http://www.google.com/            // Try the base domain. This is risky
http://m.google.com/              // Mobile sites sometimes specify different icons than desktop
http://mobile.google.com/

Comment 2

[Brian Nicholson (:bnicholson)]

Hm, this is an interesting idea, but I'm not sure I like this for several reasons:

* If I'm reading your example correctly, we could end up firing 6 separate network requests for a single favicon. That feels aggressive, especially on mobile.

* Checking a separate subdomain means that we're breaking out of the page's origin (protocol + host + port), which could result in unexpected behavior. Consider a somehostingsite.com where each member has a subdomain, such as foobar.somehostingsite.com. If foobar.somehostingsite.com doesn't supply a favicon, we'll fall back to www.somehostingsite.com, which isn't even the same owner.

* As you mentioned in the PR, there's lots of room for error here. This feels particularly risky as a non-tracking feature so close to v1 shipping, so I think we should at least hold this off until v1.1 (if we want it at all).

That said, might as well set tracking? now and get some group discussion since there's a PR up.

Comment 3

[Richard Newman [:rnewman]]

FWIW, on Android we only fall back to /favicon.ico.

I generally concur with Comment 2.

One thing we might consider is not doing the network fetch, but being looser when fetching from the database. If we're showing a tile for www.facebook.com and we have a favicon for m.facebook.com, it might be a fair fallback until we load that page and know what the true icon URL is.

Comment 4

[Richard Newman [:rnewman]]

Too much risk for v1, needs time to decide on the approach. Minus for now.

Comment 5

[Brian Nicholson (:bnicholson)]

Comment on attachment 8637505 [details] [review]
PR https://github.com/mozilla/firefox-ios/pull/787

Removing review for now; re-flag if you feel strongly about having this.

Comment 6

[Wesley Johnston (:wesj)]

(In reply to Brian Nicholson (:bnicholson) from comment #2)
> * Checking a separate subdomain means that we're breaking out of the page's
> origin (protocol + host + port), which could result in unexpected behavior.
> Consider a somehostingsite.com where each member has a subdomain, such as
> foobar.somehostingsite.com. If foobar.somehostingsite.com doesn't supply a
> favicon, we'll fall back to www.somehostingsite.com, which isn't even the
> same owner.

Yes. This is the fallback for when we're really out of options. i.e. http://foobar.foo.com/uasdf/asd provided nothing and http://foobar.foo.com also provided nothing. We're already deep down the guessing hole at this point.

My original impetus was hoping that desktop or mobile versions of sites provided different/better icons (that's true on google.com properties for instance), but it doesn't seem like special casing to avoid moving out of a subdomains is worth it. i.e. an "m" subdomain with bad icons isn't much better than a "foobar" one.

I have a script that runs against the Alexa top 1000 using these techinques. I can have it spit out when this is useful or not too, to see how much this helps (opengraph image support actually wound up helping google properties more if I remember right).

Comment 7

[Wesley Johnston (:wesj)]

I split things up so that we can land these in stages. If things get "good enough" without this, seems good to skip.

Comment 8

[Brian Nicholson (:bnicholson)]

Don't think this is worth the added cost, and I don't think anyone is planning on picking this up. Closing.