1046579 - Up scaled and low resolution icons on bookmarks panel, history panel and recent tabs panel

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Flaviu Cos]

Environment: 
Device: Asus Transformer Tab (Android 4.2.1);
Build: Nightly 34.0a1 (2014-07-31);

Steps to reproduce:
1. Start Firefox with a clean profile;
2. Delete the default bookmarks from the bookmarks panel;
3. Compare the icons from the empty panels (e.g.: bookmarks, reading list, history and recent tabs). 

Expected result:
All the icons have the same dimension, position and resolution.

Actual result:
The icons from bookmarks panel, history panel and recent tabs panel look up scaled and low resolution.

Comment 1

[Flaviu Cos]

Attachment: bookmarks panel.png

Please check the attached screenshots.

Comment 2

[Flaviu Cos]

Attachment: reading list panel.png

Comment 3

[Flaviu Cos]

Attachment: history panel.png

Comment 4

[Flaviu Cos]

Attachment: recent tabs panel.png

Comment 5

[Flaviu Cos]

Regression range:
Last good build: 2014-06-15
First bad build: 2014-06-16

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a6a457fe2a2a&tochange=80431d4fd0da

Comment 6

[:Margaret Leibovic]

Shoot, this was probably caused by bug 1023914.

Comment 7

[:Margaret Leibovic]

The problem here is that our built-in empty view images are 90x90, but the default empty view images for hub panels is 150x150 (and that's the size we encourage add-on authors to use). We fixed bug 996708 and bug 1023914 to make this icon size explicitly 150x150 to help add-on authors.

So, for now, I'm just going to try backing out my patches from bug 996708 and bug 1023914 to restore the original behavior, which was that we wouldn't attempt to crop or scale the icons at all. This means that some add-on empty views might look kinda bad if add-on authors use the wrong icon size, but that's better than regressing the look of our built-in panels.

And in fact, I don't even think it's very easy for add-on authors to specify the right images even if they wanted to, since we use Picasso to load these images, and it's currently hard for add-ons to specify local images (bug 1004517).

Comment 8

[:Margaret Leibovic]

Attachment: Don't scale empty view images

Sigh, somehow I didn't even manage to land the patch for bug 1023914 correctly (I forgot to land the bottom margin change).

This reverts the explicit width/height and scaling, but I decided to keep the margin change in case we do want to try explicitly setting the width/height in the future.

If we do look into that in the future, we'll want to update our built-in panel empty view images to also be the same size as the dynamic panel empty view images.

I tested with my hub-kitchensink add-on to verify that both built-in and add-on empty views look correct with this change.

Comment 9

[Ian Barlow (:ibarlow)]

(In reply to :Margaret Leibovic from comment #7)
> The problem here is that our built-in empty view images are 90x90, but the
> default empty view images for hub panels is 150x150 (and that's the size we
> encourage add-on authors to use). We fixed bug 996708 and bug 1023914 to
> make this icon size explicitly 150x150 to help add-on authors.

Can we reset the rules for add-on panels to use 90x90 images as well, instead of 150x150? This sounds like we (as in I) just forgot that we already had a rule for empty pages...

Comment 10

[Lucas Rocha (:lucasr)]

Comment on attachment 8465759 [details] [diff] [review]
Don't scale empty view images

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

This will potential cut off the edges of the empty view images if they're bigger than the allocated size. Isn't that something we changed some time ago for the hub stuff?

Comment 11

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #10)
> Comment on attachment 8465759 [details] [diff] [review]
> Don't scale empty view images
> 
> Review of attachment 8465759 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This will potential cut off the edges of the empty view images if they're
> bigger than the allocated size. Isn't that something we changed some time
> ago for the hub stuff?

Yes, that change is what caused this regression in our built-in panels :)

Comment 12

[:Margaret Leibovic]

(In reply to Ian Barlow (:ibarlow) from comment #9)
> (In reply to :Margaret Leibovic from comment #7)
> > The problem here is that our built-in empty view images are 90x90, but the
> > default empty view images for hub panels is 150x150 (and that's the size we
> > encourage add-on authors to use). We fixed bug 996708 and bug 1023914 to
> > make this icon size explicitly 150x150 to help add-on authors.
> 
> Can we reset the rules for add-on panels to use 90x90 images as well,
> instead of 150x150? This sounds like we (as in I) just forgot that we
> already had a rule for empty pages...

Sure, we can do that. We would just need a 90x90 icon for the default dynamic panel empty image. This icon:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/drawable-mdpi/icon_home_empty_firefox.png

Comment 13

[:Margaret Leibovic]

I think the real fix here is to get an updated 90x90 icon_home_empty_firefox resource, then update the layout file to specify 90dp for the height/width. So then all empty view images would be 90x90.

However, in the meantime, I think we should just revert the changes we made in bug bug 996708 and bug 1023914, since they managed to break our built-in panel empty views.

We already shipped this panel API without the scaling in place, so I'm not worried about images accidentally getting cut off. As it is, the empty view should not be visible often, and I'm not even sure add-ons are taking advantage of this API to specify their own image (I know my add-ons aren't).

What do you think?

Comment 14

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #13)
> I think the real fix here is to get an updated 90x90 icon_home_empty_firefox
> resource, then update the layout file to specify 90dp for the height/width.
> So then all empty view images would be 90x90.
> 
> However, in the meantime, I think we should just revert the changes we made
> in bug bug 996708 and bug 1023914, since they managed to break our built-in
> panel empty views.
> 
> We already shipped this panel API without the scaling in place, so I'm not
> worried about images accidentally getting cut off. As it is, the empty view
> should not be visible often, and I'm not even sure add-ons are taking
> advantage of this API to specify their own image (I know my add-ons aren't).
> 
> What do you think?

Fair enough. File a follow-up to get the current images resized to the new 90x90 standard?

Comment 15

[:Margaret Leibovic]

Comment on attachment 8465759 [details] [diff] [review]
Don't scale empty view images

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

I filed bug 1048941 as the follow-up for 90x90 images. So could you give this an r+ to land in the mean time? :)

Comment 16

[Lucas Rocha (:lucasr)]

Comment on attachment 8465759 [details] [diff] [review]
Don't scale empty view images

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

Ok.

Comment 17

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/abc8062899d3

I'll request uplift when this is verified on Nightly.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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

Comment 19

[Flaviu Cos]

Verified as fixed in build 34.0a1 (2014-08-08);
Device: Samsung Galaxy Nexus (Android 4.2.1).

Comment 20

[:Margaret Leibovic]

After working on bug 1048941, I actually think we should uplift that patch instead of this one, since it's a better fix for the same problem.