:Paolo Amadini Assignee
	Description • 13 years ago

See bug 713642 for background.

Here we have the application chooser dialog that is invoked for handling
external content related to specific protocols (nsIContentDispatchChooser).
This area of the code hasn't undergone relevant modifications years.

It uses favicons to represent the remote web applications that are available
for handling the content. To see the dialog, for example, in a clean profile
you can type "webcal:" in the navigation bar.

The code is here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/handling/content/dialog.js#150

Basically, if a favicon has been explicitly associated to the *root* of the
site that hosts the application, fetch that icon directly from the network.
Otherwise, fetch "favicon.ico" located under the root of the site.

Questions:
 - Do we associate favicons to the site root explicitly somewhere?
 - Is it done systematically, and in advance of displaying the dialog?
 - If it's not done systematically, does it make sense to use the
    sporadically associated favicon at all?

Without really investigating what we do, my impression is that we might just
end up using the icon associated with the application's root web page if the
user navigated to it (which can be different from "favicon.ico"), otherwise
just use "favicon.ico".

In this case, the change can be to fetch "favicon.ico" in all cases.
Optionally, we can use the favicon service to cache it, but it should be
associated to a fake page URI (like "urn:app-favicon:<site root>", not to
the URI of a page the user can navigate to (the page can have a potentially
different <link> for the favicon).

	:Paolo Amadini Assignee
	Comment 1 • 13 years ago

I've found the code that mirrors this behavior in the preferences dialog:

http://hg.mozilla.org/mozilla-central/file/7d0d1108a14e/browser/components/preferences/applications.js#l1835

This confirms the whole story:

    // Unfortunately we can't use the favicon service to get the favicon,
    // because the service looks in the annotations table for a record with
    // the exact URL we give it, and users won't have such records for URLs
    // they don't visit, and users won't visit the web app's URL template,
    // they'll only visit URLs derived from that template (i.e. with %s
    // in the template replaced by the URL of the content being handled).

We can't even associate the icon to a fake URI because favicons that are not
associated to a visited page are expired in a short time.

So I've proceeded with the patch to always use "favicon.ico", and copied the
comment. I've also added the same switch for "browser.chrome.favicons", if
the preference exists in the application, to make the behavior consistent.

	Marco Bonardo [:mak]
	Comment 2 • 13 years ago

Comment on attachment 603217 [details] [diff] [review]
The patch

Review of attachment 603217 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/handling/content/dialog.js
@@ +154,2 @@
>            try {
> +            useIcon = Services.prefs.getBoolPref("browser.chrome.favicons");

This is toolkit/mozapps, we should not rely on a browser pref.
What this dialog could do is to receive a "disableIcons" argument from the surrounding app, though doing that would solve a completely different bug, that is not even in the scope for this one.
Sorry, we can't do this here.

@@ +155,5 @@
> +            useIcon = Services.prefs.getBoolPref("browser.chrome.favicons");
> +          } catch (ex) { }
> +          if (useIcon) {
> +            // Unfortunately we can't use the favicon service to get the
> +            // favicon, because the service looks in the annotations table for a

remove "in the annotations table", since it's completely wrong (if you would like to fix the other comment as well, will be fine)

@@ +158,5 @@
> +            // Unfortunately we can't use the favicon service to get the
> +            // favicon, because the service looks in the annotations table for a
> +            // record with the exact URL we give it, and users won't have such
> +            // records for URLs they don't visit, and users won't visit the web
> +            // app's URL template, they'll only visit URLs derived from that

s/web app/handler/

@@ +160,5 @@
> +            // record with the exact URL we give it, and users won't have such
> +            // records for URLs they don't visit, and users won't visit the web
> +            // app's URL template, they'll only visit URLs derived from that
> +            // template (i.e. with %s in the template replaced by the URL of the
> +            // content being handled).

This should probably talk about the root folder of the handler, rather than a "template" that is a not well defined entity here, may make more sense for web apps.

	:Paolo Amadini Assignee
	Comment 3 • 13 years ago

(In reply to Marco Bonardo [:mak] from comment #2)
> This is toolkit/mozapps, we should not rely on a browser pref.

I thought about this (hence the check for the missing preference case), but
it's unclear which preferences in the "browser." namespace can be used by
toolkit components, because many components actually use preferences in that
namespace.

> This should probably talk about the root folder of the handler, rather than
> a "template" that is a not well defined entity here, may make more sense for
> web apps.

Not sure what you're asking, "handler's URL template" seemed clear to me.

	Marco Bonardo [:mak]
	Comment 4 • 13 years ago

Comment on attachment 606166 [details] [diff] [review]
Revised patch

Review of attachment 606166 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Paolo Amadini from comment #3)
> I thought about this (hence the check for the missing preference case), but
> it's unclear which preferences in the "browser." namespace can be used by
> toolkit components, because many components actually use preferences in that
> namespace.

ideally none, in some case we failed to properly delineate the limit and we just live with those, or someone just decided the browser namespace is easier to understand for users.  Regardless, this is a discussion that should happen elsewhere, not related to this bug.

	:Paolo Amadini Assignee
	Comment 5 • 13 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/a94264d93205

	Mounir Lamouri (:mounir)
	Comment 6 • 13 years ago

https://hg.mozilla.org/mozilla-central/rev/a94264d93205