677999 - Make home screen bookmark icons pretty

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Bookmarks, defect)

Description

[Ian Barlow (:ibarlow)]

Attachment: Assets for Home Screen Bookmark icons

Let's add a background to our homescreen bookmarks, as shown in these mocks: http://cl.ly/2M1O213w3C3g451o3k1S

1. If no favicon is available, use a star as the default icon
2. If a favicon is available, scale the icon to 32px and place in the center of the background

Comment 1

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

Attachment: patch

This patch impls the logic in comment 0

Comment 2

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

Attachment: example screenshot

here's what it looks like. "About:" has no favicon, so it uses the "star" overlay

Comment 3

[Wesley Johnston (:wesj)]

Comment on attachment 552496 [details] [diff] [review]
patch

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

I'm not a huge fan of this, but the code looks fine to me.

Are we trying to create another distinction between installed webapps and bookmarks here? If not, I think we should probably reuse/extend:

http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/browser-ui.js#1044

and only do this if there is nothing big and shiny to use.

::: mobile/chrome/content/BookmarkHelper.js
@@ +73,5 @@
>      this.box.hidden = true;
>    },
>  
>    createShortcut: function BH_createShortcut(aTitle, aURL, aIconURL) {
> +    // The background images are 72px, but Android will resize ass needed.

I see you've added a secret spelling "error" here.

Comment 4

[Ian Barlow (:ibarlow)]

Mark, these are looking great. 

One comment -- the favicons are a bit fuzzy, is this screenshot using mbrubeck's crisp scaling yet (instead of bilinear)?

Comment 5

[Ian Barlow (:ibarlow)]

Attachment: Inner shadow for favicons

If it's possible, let's layer this shadow / glow image on top of our favicons, to give them some depth.

Comment 6

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

(In reply to Wesley Johnston (:wesj) from comment #3)

> I'm not a huge fan of this, but the code looks fine to me.

Not a fan of using backgrounds? Or the way we use Image to overlay layers?

> 
> Are we trying to create another distinction between installed webapps and
> bookmarks here? If not, I think we should probably reuse/extend:

We will look into reusing what we can

> >    createShortcut: function BH_createShortcut(aTitle, aURL, aIconURL) {
> > +    // The background images are 72px, but Android will resize ass needed.
> 
> I see you've added a secret spelling "error" here.

fixed :)

Comment 7

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

http://hg.mozilla.org/mozilla-central/rev/1dd81c324ac7

Comment 8

[Andreea Pod]

Verified fixed on: Mozilla /5.0 (Android;Linux armv7l;rv:9.0a1) Gecko/20110824 Firefox/9.0a1 Fennec/9.0a1
Device: LG Optimus 2X (Android 2.2)

Added google.com and about: as home screen bookmarks and checked if they look like in the screenshot.