710325 - about:home - show icon and version for each addon entry

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P3)

Description

[Lucas Rocha (:lucasr)]

See UI specs attached in bug 706667.

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: (1/3) Show addon version in about:home

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: (2/3) Thread-safety fix in Favicons

Comment 3

[Lucas Rocha (:lucasr)]

Attachment: (3/3) Show addon icon in about:home

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 589863 [details] [diff] [review]
(3/3) Show addon icon in about:home


>+    private String getPageUrlFromIconUrl(String iconUrl) {
>+        // Addon icon URLs come with a query argument that is usually
>+        // used for expiration purposes. We want the "page URL" here to be
>+        // stable enough to avoid unnecessary duplicate records of the
>+        // same addon.
>+        String pageUrl = iconUrl;
>+
>+        try {
>+            URL urlForIcon = new URL(iconUrl);
>+            URL urlForPage = new URL(urlForIcon.getProtocol(), urlForIcon.getAuthority(), urlForIcon.getPath());
>+            pageUrl = urlForPage.toString();
>+        } catch (MalformedURLException e) {
>+            // Defaults to pageUrl = iconUrl in case of error
>+        }
>+
>+        return pageUrl;
>+    }

So this makes a "pageUrl" that is specifically for the ICON, not the ADDON? So we want get a conflicting favicon if we actually visited the ADDON url in the browser, right? What are the benefits of using the favicon system for this instead of just downloading directly from the URL? I assume it might be: built-in background download and potentially caching.

r+, but I just wanted to make sure we didn't get into some conflict situation by using the favicon system here.

Comment 5

[Lucas Rocha (:lucasr)]

(In reply to Mark Finkle (:mfinkle) from comment #4)
> Comment on attachment 589863 [details] [diff] [review]
> (3/3) Show addon icon in about:home
> 
> 
> >+    private String getPageUrlFromIconUrl(String iconUrl) {
> >+        // Addon icon URLs come with a query argument that is usually
> >+        // used for expiration purposes. We want the "page URL" here to be
> >+        // stable enough to avoid unnecessary duplicate records of the
> >+        // same addon.
> >+        String pageUrl = iconUrl;
> >+
> >+        try {
> >+            URL urlForIcon = new URL(iconUrl);
> >+            URL urlForPage = new URL(urlForIcon.getProtocol(), urlForIcon.getAuthority(), urlForIcon.getPath());
> >+            pageUrl = urlForPage.toString();
> >+        } catch (MalformedURLException e) {
> >+            // Defaults to pageUrl = iconUrl in case of error
> >+        }
> >+
> >+        return pageUrl;
> >+    }
> 
> So this makes a "pageUrl" that is specifically for the ICON, not the ADDON?
> So we want get a conflicting favicon if we actually visited the ADDON url in
> the browser, right? What are the benefits of using the favicon system for
> this instead of just downloading directly from the URL? I assume it might
> be: built-in background download and potentially caching.

Precisely: built-in background download and caching. Re-implementing the same thing just for addon icons would be a bit overkill imo. My assumption here is that addon icon URLs are stable enough to be used as the pageUrl here (without the query args, in this case). 

Ideally, we'd use the actual addon page URL here but unfortunately this is not available on the recommended addons file.

> r+, but I just wanted to make sure we didn't get into some conflict
> situation by using the favicon system here.

The fact that we're using the image URL as the page URL makes URL conflicts on favicon database very unlikely here.

Comment 6

[Lucas Rocha (:lucasr)]

Pushed:
http://hg.mozilla.org/integration/mozilla-inbound/rev/5efbddddb54f
http://hg.mozilla.org/integration/mozilla-inbound/rev/c1a1a872ecaa
http://hg.mozilla.org/integration/mozilla-inbound/rev/3ac73fd59a12

Comment 7

[Lucas Rocha (:lucasr)]

Comment on attachment 589861 [details] [diff] [review]
(1/3) Show addon version in about:home

Mobile-only. Missing feature in aurora.

Comment 8

[Lucas Rocha (:lucasr)]

Comment on attachment 589862 [details] [diff] [review]
(2/3) Thread-safety fix in Favicons

Mobile-only. Missing feature in aurora.

Comment 9

[Lucas Rocha (:lucasr)]

Comment on attachment 589863 [details] [diff] [review]
(3/3) Show addon icon in about:home

Mobile-only. Missing feature in aurora.

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5efbddddb54f
https://hg.mozilla.org/mozilla-central/rev/c1a1a872ecaa
https://hg.mozilla.org/mozilla-central/rev/3ac73fd59a12

Comment 11

[Alex Keybl [:akeybl]]

Comment on attachment 589861 [details] [diff] [review]
(1/3) Show addon version in about:home

[Triage Comment]
Mobile only - approved for Aurora.

Comment 12

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5d741b581324
https://hg.mozilla.org/releases/mozilla-aurora/rev/fa4d1d02170c
https://hg.mozilla.org/releases/mozilla-aurora/rev/ca4032ba20fe

Comment 13

[Camelia Urian]

Nightly 13.0a1 (2012-02-02)
Aurora 12.0a2 (2012-02-02)
Device: Samsung Google Nexus S - Android 2.3.6

Only 1st  addon has the correct icon displayed in start page. Icon for Clear Mobile History is not not displayed correctly. There is a puzzle icon for Clear Mobile History. See attached screen shot.

Comment 14

[Camelia Urian]

Attachment: ScreenShot

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

This bug is too "finished". Please file a new bug. The add-ons show the image and the version. That was the point of this bug. If the image shown is not correct, that's a new bug.

Comment 16

[Camelia Urian]

Build ID:
  Nightly 13.0a1 (2012-02-03)
  Aurora 12.0a2 (2012-02-02)
  Beta 11.0 (2012-02-01)
Device: Samsung Google Nexus S - Android 2.3.6

Will log a new bug for wrong icon issue from comment 14.
Marking this bug as Verified-Fixed.