Last Comment Bug 712559 - top sites section of about:home resizes when thumbnails populate
: top sites section of about:home resizes when thumbnails populate
Status: VERIFIED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P3 normal (vote)
: Firefox 12
Assigned To: Brad Lassey [:blassey] (use needinfo?)
:
: Sebastian Kaspari (:sebastian)
Mentors:
: 710327 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-20 23:26 PST by Brad Lassey [:blassey] (use needinfo?)
Modified: 2016-07-29 14:21 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
verified
11+


Attachments
patch (1.82 KB, patch)
2011-12-20 23:26 PST, Brad Lassey [:blassey] (use needinfo?)
mark.finkle: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Brad Lassey [:blassey] (use needinfo?) 2011-12-20 23:26:57 PST
Created attachment 583406 [details] [diff] [review]
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?) 2011-12-26 06:53:25 PST
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?) 2011-12-27 10:00:01 PST
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?) 2011-12-27 10:45:37 PST
(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?) 2011-12-27 10:51:28 PST
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?) 2011-12-27 11:23:54 PST
density = desityDPI / 160
Comment 6 Matt Brubeck (:mbrubeck) 2011-12-28 11:07:13 PST
https://hg.mozilla.org/mozilla-central/rev/d7999840fac5

Do we want this on Aurora?
Comment 7 Brad Lassey [:blassey] (use needinfo?) 2011-12-29 11:19:12 PST
*** Bug 710327 has been marked as a duplicate of this bug. ***
Comment 8 Aaron Train [:aaronmt] 2011-12-30 06:54:14 PST
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?) 2012-01-09 14:43:39 PST
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] 2012-01-09 15:14:32 PST
Comment on attachment 583406 [details] [diff] [review]
patch

[Triage Comment]
Mobile only - approving for Aurora.
Comment 11 Brad Lassey [:blassey] (use needinfo?) 2012-01-10 11:06:27 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/1e8584b9d3b1

Note You need to log in before you can comment on or make changes to this bug.