Closed Bug 710325 Opened 8 years ago Closed 8 years ago

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

Categories

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

All
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 12
Tracking Status
firefox11 --- verified
firefox12 --- verified
firefox13 --- verified
fennec 11+ ---

People

(Reporter: lucasr, Assigned: lucasr)

Details

Attachments

(4 files)

See UI specs attached in bug 706667.
Priority: -- → P3
tracking-fennec: --- → 11+
Attachment #589861 - Flags: review?(mark.finkle)
Attachment #589862 - Flags: review?(mark.finkle)
Attachment #589863 - Flags: review?(mark.finkle)
Attachment #589861 - Flags: review?(mark.finkle) → review+
Attachment #589862 - Flags: review?(mark.finkle) → review+
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.
Attachment #589863 - Flags: review?(mark.finkle) → review+
(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 on attachment 589861 [details] [diff] [review]
(1/3) Show addon version in about:home

Mobile-only. Missing feature in aurora.
Attachment #589861 - Flags: approval-mozilla-aurora?
Comment on attachment 589862 [details] [diff] [review]
(2/3) Thread-safety fix in Favicons

Mobile-only. Missing feature in aurora.
Attachment #589862 - Flags: approval-mozilla-aurora?
Comment on attachment 589863 [details] [diff] [review]
(3/3) Show addon icon in about:home

Mobile-only. Missing feature in aurora.
Attachment #589863 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/5efbddddb54f
https://hg.mozilla.org/mozilla-central/rev/c1a1a872ecaa
https://hg.mozilla.org/mozilla-central/rev/3ac73fd59a12
Assignee: nobody → lucasr.at.mozilla
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12
Comment on attachment 589861 [details] [diff] [review]
(1/3) Show addon version in about:home

[Triage Comment]
Mobile only - approved for Aurora.
Attachment #589861 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #589862 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #589863 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached image ScreenShot
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.
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.