712559 - top sites section of about:home resizes when thumbnails populate

Status: VERIFIED FIXED

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

Description

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

I am pretty sure there is already a bug filed on this, but I can't find it

Comment 1

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

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


>diff --git a/mobile/android/base/AboutHomeContent.java b/mobile/android/base/AboutHomeContent.java

>     public static class TopSitesGridView extends GridView {
>+        private static final int kTopSiteItemHeight = 160;
>+

This is the only part I am worried about. Can we find a different way to get the single item height? Can we get the actual height and add a constant margin?

I suppose we could do that in a followup bug. Please file one.

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/d7999840fac5

(In reply to Mark Finkle (:mfinkle) from comment #1)
> Comment on attachment 583406 [details] [diff] [review]
> patch
> 
> 
> >diff --git a/mobile/android/base/AboutHomeContent.java b/mobile/android/base/AboutHomeContent.java
> 
> >     public static class TopSitesGridView extends GridView {
> >+        private static final int kTopSiteItemHeight = 160;
> >+
> 
> This is the only part I am worried about. Can we find a different way to get
> the single item height? Can we get the actual height and add a constant
> margin?
We can't get the actual ("measured" in Android-ese) until the element is constructed, which is what the code was doing. The problem is that these elements take a while to construct because of the DB access.

The idea here is to pre-calculate it ourselves. I changed the code to hard code the 109dip height of the tiles and apply the display density scale to that. I also added a comment showing my math to arrive at 109dip.

Comment 3

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

(In reply to Brad Lassey [:blassey] from comment #2)

> The idea here is to pre-calculate it ourselves. I changed the code to hard
> code the 109dip height of the tiles and apply the display density scale to
> that. I also added a comment showing my math to arrive at 109dip.

That sounds good to me. No need for any followup bugs.

Comment 4

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

I did notice you are using "dm.density" and we have a GeckoAppShell.getDpi() that returns "dm.densityDpi":
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoAppShell.java#1015

I'm not 100% sure of the difference, but we do use GeckoAppShell.getDpi() in another place where we scale a dip size.

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

density = desityDPI / 160

Comment 6

[Matt Brubeck (:mbrubeck)]

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

Do we want this on Aurora?

Comment 8

[Aaron Train [:aaronmt]]

Samsung Nexus S (Android 4.0.3)
Mozilla/5.0 (Android; Linux armv7l; rv:12.0a1) Gecko/20111230 Firefox/12.0a1 Fennec/12.0a1

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

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

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky):

Comment 10

[Alex Keybl [:akeybl]]

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

[Triage Comment]
Mobile only - approving for Aurora.

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1e8584b9d3b1