Closed Bug 1103267 Opened 5 years ago Closed 5 years ago

Follow-up: Improve Favicon handling by about:passwords

Categories

(Firefox for Android :: Logins, Passwords and Form Fill, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 38
Tracking Status
firefox38 --- fixed

People

(Reporter: liuche, Assigned: vivek)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Instead hard-coding fetching the /favicon.ico resource from sites, we should try to use the favicon cache (if that's even possible).
Attached patch 1103267.patch (obsolete) — Splinter Review
Patch description:
Loads favicon from cache as data uri.
fallback to favicon url in case of error.
Attachment #8554868 - Flags: review?(liuche)
Attachment #8554868 - Flags: feedback?(margaret.leibovic)
Comment on attachment 8554868 [details] [diff] [review]
1103267.patch

This is a good approach. I don't think we should make it "Password:LoadFavicon" though. We already have "Reader:FaviconRequest" which does something similar.

Both of those features are BrowserApp (Firefox) and not GeckoApp (GeckoView) level features. You add you message handler to BrowserApp, which is fine.

Let's just make "Favicon:Xxx" messages, so we could use the code from anywhere.

Couple of thoughts:
1. Since we are loading from cache only, we could use "Favicon:Load"
2. Do we need to send the "faviconUrl" here? I'd rather not send anything.
3. I'd rather send back a JSON object. For success, we could send back the "url" that was sent, that "faviconURL" which should be a data: URL. If we fail, send back the "url" but don't send the "faviconURL". We can test for the json.faviconURL in the callback (in JS) and use a hard coded default icon. Do not use the hostname + "/favicon.ico" anymore.

4. In the future, we could convert the ReadingList code to use a similar "Favicon:Xxx" message.

Thoughts?
(In reply to Mark Finkle (:mfinkle) from comment #2)
> Comment on attachment 8554868 [details] [diff] [review]
> 1103267.patch
> 
> This is a good approach. I don't think we should make it
> "Password:LoadFavicon" though. We already have "Reader:FaviconRequest" which
> does something similar.

This reader mode message request just returns the faviconUrl, not actual image data, so it is quite a bit simpler that what we're trying to do here. That logic is just used to set the favicon url for the about:reader URL so that we show the appropriate favicon in the toolbar.

> Both of those features are BrowserApp (Firefox) and not GeckoApp (GeckoView)
> level features. You add you message handler to BrowserApp, which is fine.
> 
> Let's just make "Favicon:Xxx" messages, so we could use the code from
> anywhere.

I also wonder if there's a helper place we could put this, perhaps in Favicons.java, if only to avoid adding more things to BrowserApp.

> Couple of thoughts:
> 1. Since we are loading from cache only, we could use "Favicon:Load"
> 2. Do we need to send the "faviconUrl" here? I'd rather not send anything.

I agree, I think it would be best if we could just say to Java "Here's a URL, what's the favicon we should show for it?"

> 3. I'd rather send back a JSON object. For success, we could send back the
> "url" that was sent, that "faviconURL" which should be a data: URL. If we
> fail, send back the "url" but don't send the "faviconURL". We can test for
> the json.faviconURL in the callback (in JS) and use a hard coded default
> icon. Do not use the hostname + "/favicon.ico" anymore.

If we can't find an image in the cache, I think we should just do callback.sendError() and handle showing a default icon in the about:passwords list.
Comment on attachment 8554868 [details] [diff] [review]
1103267.patch

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

::: mobile/android/base/BrowserApp.java
@@ +1571,5 @@
> +                            out = new ByteArrayOutputStream();
> +                            out.write("data:image/png;base64,".getBytes());
> +                            b64 = new Base64OutputStream(out, Base64.NO_WRAP);
> +                            favicon.compress(Bitmap.CompressFormat.PNG, 100, b64);
> +                            callback.sendSuccess(new String(out.toByteArray()));

It feels rough to send all this image data through the Android bridge, but I suppose this isn't *too* terrible if we're just doing this when the user opens about:passwords. But even so, maybe we should batch these requests in aboutPasswords.js to make sure we only request icons that will be visible, plus a few more for when the user starts scrolling. This will probably just come down to batching creating elements in the list that we're using to display passwords.

@@ +1575,5 @@
> +                            callback.sendSuccess(new String(out.toByteArray()));
> +                        } catch (IOException e) {
> +                            // Fallback to faviconURL
> +                            Log.w(LOGTAG, "Failed to convert to base64 data URI");
> +                            callback.sendSuccess(faviconURL);

Use callback.sendError for this case.

@@ +1592,5 @@
> +                    }
> +                });
> +            }
> +        };
> +        Favicons.getSizedFavicon(getContext(),

Reading through the getSizedFavicon implementation, it looks like it will fall back to a network request if necessary. It looks like what we actually want to use is getSizedFaviconForPageFromLocal.

::: mobile/android/chrome/content/aboutPasswords.js
@@ +137,5 @@
> +    // Load favicon from cache.
> +    Messaging.sendRequestForResult({
> +      type: "Password:LoadFavicon",
> +      url: login.hostname,
> +      faviconUrl: login.hostname + "/favicon.ico"

As mfinkle mentioned, let's not send along this faviconUrl.

@@ +140,5 @@
> +      url: login.hostname,
> +      faviconUrl: login.hostname + "/favicon.ico"
> +    }).then(function(aData) {
> +      img.setAttribute("src", aData);
> +    });

Let's add a catch here to set a default favicon if we don't get an icon back.
Attachment #8554868 - Flags: feedback?(margaret.leibovic) → feedback+
Comment on attachment 8554868 [details] [diff] [review]
1103267.patch

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

Good direction - I also agree with the comments from Margaret and mfinkle.

::: mobile/android/base/BrowserApp.java
@@ +614,5 @@
>              "Feedback:MaybeLater",
>              "Feedback:OpenPlayStore",
>              "Menu:Add",
>              "Menu:Remove",
> +            "Password:LoadFavicon",

Agreed, with margaret and mfinkle that this could be a more general message - fetching a Favicon from cache is a useful utility that isn't just limited to Passwords.

@@ +1575,5 @@
> +                            callback.sendSuccess(new String(out.toByteArray()));
> +                        } catch (IOException e) {
> +                            // Fallback to faviconURL
> +                            Log.w(LOGTAG, "Failed to convert to base64 data URI");
> +                            callback.sendSuccess(faviconURL);

Agreed - since this can be a more general utility, we should leave it to the caller to decide what happens if the favicon could not be fetched from the cache/network call (if we decide to do that).

@@ +1592,5 @@
> +                    }
> +                });
> +            }
> +        };
> +        Favicons.getSizedFavicon(getContext(),

Margaret: do we prefer the default favicon to letting Favicons make the network request to get the favicon?
Attachment #8554868 - Flags: review?(liuche)
Assignee: nobody → vivekb.balakrishnan
(In reply to Chenxia Liu [:liuche] from comment #5)

> > +                });
> > +            }
> > +        };
> > +        Favicons.getSizedFavicon(getContext(),
> 
> Margaret: do we prefer the default favicon to letting Favicons make the
> network request to get the favicon?

IMO, we should not attempt to fetch an icon using the | hostname + "/favicon.ico" | approach. I added that to the add-on as a hack. In practice, this forces HTTP Auth dialogs to appear for some of my passwords. We don't want that in production. I think we should just use a default "login" style icon.

Also, I don't mind having this API used to "check the local cache and return a data: URI" because that makes sense for this use case. Other use cases, like Reading List, want a "check the local cache and return the saved URL to a favicon" and we should have a separate message API or separate JSON to handle that case. We kind of want to grow an API for JS to access the Java Favicons service, which has multiple methods for returning data. Each has it's own use case.

For now, lets focus on the single use case needed here.
No longer depends on: password-ui
Attached patch 1103267.patch (obsolete) — Splinter Review
A new patch thats loads favicon from local cache if available or fallback to default.
Attachment #8554868 - Attachment is obsolete: true
Attachment #8558259 - Flags: review?(liuche)
Comment on attachment 8558259 [details] [diff] [review]
1103267.patch

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

Looks good, one change to how we fetch the default favicon.

::: mobile/android/base/BrowserApp.java
@@ +769,5 @@
>          EventDispatcher.getInstance().registerGeckoThreadListener((NativeEventListener)this,
>              "Accounts:Create",
>              "CharEncoding:Data",
>              "CharEncoding:State",
> +            "Favicon:Load",

I'd prefer we name this Favicon:CacheLoad - we're not fetching the Favicon in any case, only if it's in the cache, so let's be explicit about it.

::: mobile/android/chrome/content/aboutPasswords.js
@@ +140,5 @@
> +      url: login.hostname,
> +    }).then(function(aData) {
> +      img.setAttribute("src", aData);
> +    }, function(aData) {
> +      img.setAttribute("src", "chrome://browser/skin/images/favicon.png");

See comment below on using @media css to do this.

::: mobile/android/themes/core/jar.mn
@@ +67,5 @@
>    skin/images/dropmarker.svg                (images/dropmarker.svg)
>    skin/images/dropmarker-right.svg          (images/dropmarker-right.svg)
>    skin/images/errorpage-warning.png         (images/errorpage-warning.png)
>    skin/images/exitfullscreen-hdpi.png       (images/exitfullscreen-hdpi.png)
> +  skin/images/favicon.png                   (images/favicon.png)

I talked to margaret about sharing resources between Java and Gecko, and she suggested using a css media query, like in [1], to set the default favicon, and then using it if we don't find the favicon in the cache.

That way we don't need to duplicate this resource!

[1] https://hg.mozilla.org/mozilla-central/rev/0f784b15c6c7#l3.63
Attachment #8558259 - Flags: review?(liuche) → feedback+
Attached patch 1103267.patch (obsolete) — Splinter Review
A new patch changes:
* favicon loaded from cache
* fallback : default favicon loaded using media query.
Attachment #8558259 - Attachment is obsolete: true
Attachment #8559945 - Flags: review?(liuche)
Attachment #8559945 - Flags: feedback?(margaret.leibovic)
Comment on attachment 8559945 [details] [diff] [review]
1103267.patch

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

Looking good! I didn't look too closely, but overall this is definitely the direction I think we should go.

::: mobile/android/base/BrowserApp.java
@@ +1737,5 @@
> +                        Base64OutputStream b64 = null;
> +
> +                        // Failed to load favicon from local.
> +                        if (favicon == null) {
> +                            callback.sendError(null);

I would send an error message here, in case future consumers don't expect an error.

::: mobile/android/chrome/content/aboutPasswords.js
@@ +137,5 @@
> +    // Load favicon from cache.
> +    Messaging.sendRequestForResult({
> +      type: "Favicon:CacheLoad",
> +      url: login.hostname,
> +    }).then(function(aData) {

Nit: drop the a prefix. And maybe call this parameter "url" or "faviconUrl", since you know that's what you'll be getting here.

@@ +138,5 @@
> +    Messaging.sendRequestForResult({
> +      type: "Favicon:CacheLoad",
> +      url: login.hostname,
> +    }).then(function(aData) {
> +      img.style.backgroundImage= "url('" + aData + "')";

I wonder what the resolution for this image will look like. Maybe we should try asking the favicon service for a desired size depending on the resolution of the screen (or maybe the favicon service is already smart about screen resolution).

@@ +141,5 @@
> +    }).then(function(aData) {
> +      img.style.backgroundImage= "url('" + aData + "')";
> +    }, function(aData) {
> +      // Do nothing, default icon handled through media queries.
> +    });

If you're not going to do anything with the error, you can just put null as a parameter here, instead of creating an empty function.

::: mobile/android/themes/core/aboutPasswords.css
@@ +83,5 @@
>    margin: 0 5px;
>  }
>  
> +.icon {
> +  background-image: url("resource://android/res/drawable-mdpi-v4/favicon.png");

Excellent, this is a great way to avoid adding more resources.
Attachment #8559945 - Flags: feedback?(margaret.leibovic) → feedback+
Comment on attachment 8559945 [details] [diff] [review]
1103267.patch

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

I built this and it's really good! No more broken favicon links :) Agreed with Margaret's comments too.

This also fixes a bug with HTTP basic auth websites, where attempting to fetch the icon will hit the website and pop up the password prompt :)

::: mobile/android/chrome/content/aboutPasswords.js
@@ +138,5 @@
> +    Messaging.sendRequestForResult({
> +      type: "Favicon:CacheLoad",
> +      url: login.hostname,
> +    }).then(function(aData) {
> +      img.style.backgroundImage= "url('" + aData + "')";

Not sure if we can just set a default size, the way we do with other favicons? We know the size that we want, right? You could try some sizes and see what looks best.

@@ +140,5 @@
> +      url: login.hostname,
> +    }).then(function(aData) {
> +      img.style.backgroundImage= "url('" + aData + "')";
> +    }, function(aData) {
> +      // Do nothing, default icon handled through media queries.

Going through this, I see some icon flickering between the globe and actual favicon. Nit: set the icon invisible initially and in each of these callbacks set the icon visible again. How does that look? I think it might be better go from "blank" to "favicon".

@@ +201,5 @@
>    _showDetails: function (listItem) {
>      let detailItem = document.querySelector("#login-details > .login-item");
>      let login = detailItem.login = listItem.login;
>      let favicon = detailItem.querySelector(".icon");
> +    Messaging.sendRequestForResult({

I wonder if we can just set the favicon from the listItem arg without making another call to the cache?
What happens if this gets set by viewing Details of a password that has a favicon, and then you try to view the Details of a password that doesn't have an icon? I don't think the icon gets set back to the globe.
Attachment #8559945 - Flags: review?(liuche)
Attached patch 1103267.patchSplinter Review
A new patch with following added changes:
* Flickering in favicon load handled. Img becomes visible only after the promise resolves.
* Avoided redundant Favicon fetch in detailed item view.
Attachment #8559945 - Attachment is obsolete: true
Attachment #8565655 - Flags: review?(liuche)
Attachment #8565655 - Flags: feedback?(margaret.leibovic)
Comment on attachment 8565655 [details] [diff] [review]
1103267.patch

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

This is great, Vivek! :)

The favicon switching is much smoother now. Thanks for the patch!
Attachment #8565655 - Flags: review?(liuche) → review+
https://hg.mozilla.org/integration/fx-team/rev/8ef3725e6a66
Target Milestone: --- → Firefox 38
https://hg.mozilla.org/mozilla-central/rev/8ef3725e6a66
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Attachment #8565655 - Flags: feedback?(margaret.leibovic)
Depends on: 1140267
You need to log in before you can comment on or make changes to this bug.