Last Comment Bug 686528 - Start Page (about:home) could be better on tablets
: Start Page (about:home) could be better on tablets
Status: VERIFIED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: Theme and Visual Design (show other bugs)
: Trunk
: All Android
: -- normal (vote)
: Firefox 17
Assigned To: Michael Comella (:mcomella) [not actively working on fennec: expect slow responses]
:
Mentors:
: 688442 (view as bug list)
Depends on: 780367 780481 780486
Blocks: 655762
  Show dependency treegraph
 
Reported: 2011-09-13 13:46 PDT by Lucas Rocha (:lucasr)
Modified: 2016-07-29 14:19 PDT (History)
14 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified
verified
verified
15+


Attachments
rough mockup (99.76 KB, image/png)
2011-09-23 12:38 PDT, Ian Barlow (:ibarlow)
no flags Details
Rough WIP (8.76 KB, patch)
2011-10-06 14:29 PDT, Sriram Ramasubramanian [:sriram]
no flags Details | Diff | Splinter Review
Top Sites Change WIP (11.34 KB, patch)
2012-07-25 12:50 PDT, Michael Comella (:mcomella) [not actively working on fennec: expect slow responses]
sriram.mozilla: feedback-
Details | Diff | Splinter Review
Patch 01: Integer Resources (14.04 KB, patch)
2012-07-25 15:17 PDT, Michael Comella (:mcomella) [not actively working on fennec: expect slow responses]
sriram.mozilla: feedback+
Details | Diff | Splinter Review
01a: Integer Resources (12.05 KB, patch)
2012-07-25 15:48 PDT, Michael Comella (:mcomella) [not actively working on fennec: expect slow responses]
sriram.mozilla: review+
Details | Diff | Splinter Review
02: Created 10" tablet layout. (14.23 KB, text/plain)
2012-07-25 22:00 PDT, Michael Comella (:mcomella) [not actively working on fennec: expect slow responses]
no flags Details
02a: Created 10" tablet layout. (25.80 KB, patch)
2012-07-25 22:11 PDT, Michael Comella (:mcomella) [not actively working on fennec: expect slow responses]
sriram.mozilla: feedback-
Details | Diff | Splinter Review
10'' Tab landscape after patch 02a. (559.15 KB, image/jpeg)
2012-07-26 10:39 PDT, Michael Comella (:mcomella) [not actively working on fennec: expect slow responses]
no flags Details
02b: Created 10" tablet layout. (13.14 KB, patch)
2012-07-27 13:37 PDT, Michael Comella (:mcomella) [not actively working on fennec: expect slow responses]
no flags Details | Diff | Splinter Review
about:home, 10" landscape, after patch 02b. (583.04 KB, image/jpeg)
2012-07-27 13:40 PDT, Michael Comella (:mcomella) [not actively working on fennec: expect slow responses]
no flags Details
02c: Cleaned about:home and created 10" tablet layout. (29.26 KB, patch)
2012-07-27 17:31 PDT, Michael Comella (:mcomella) [not actively working on fennec: expect slow responses]
no flags Details | Diff | Splinter Review
about:home, 10" landscape, after patch 02c. (572.38 KB, image/jpeg)
2012-07-27 17:31 PDT, Michael Comella (:mcomella) [not actively working on fennec: expect slow responses]
no flags Details
02d: Cleaned about:home and created 10" tablet layout. (29.26 KB, patch)
2012-07-27 17:41 PDT, Michael Comella (:mcomella) [not actively working on fennec: expect slow responses]
sriram.mozilla: feedback+
Details | Diff | Splinter Review
about:home, 10" landscape, after patch 02d. (565.68 KB, image/jpeg)
2012-07-27 17:43 PDT, Michael Comella (:mcomella) [not actively working on fennec: expect slow responses]
no flags Details
02e: Cleaned about:home and created 10" tablet layout. (28.99 KB, patch)
2012-07-30 14:03 PDT, Michael Comella (:mcomella) [not actively working on fennec: expect slow responses]
sriram.mozilla: review+
Details | Diff | Splinter Review
03: Added padding for tablet layout. (6.27 KB, patch)
2012-07-30 17:13 PDT, Michael Comella (:mcomella) [not actively working on fennec: expect slow responses]
no flags Details | Diff | Splinter Review
about:home, 10" landscape after patch 03 (575.77 KB, image/jpeg)
2012-07-30 17:14 PDT, Michael Comella (:mcomella) [not actively working on fennec: expect slow responses]
no flags Details
03a: Added padding for tablet layout. (9.12 KB, patch)
2012-07-30 18:14 PDT, Michael Comella (:mcomella) [not actively working on fennec: expect slow responses]
sriram.mozilla: review-
Details | Diff | Splinter Review
about:home, 10" portrait after patch 03a (584.34 KB, image/jpeg)
2012-07-30 18:15 PDT, Michael Comella (:mcomella) [not actively working on fennec: expect slow responses]
no flags Details
04: kTopSitesItemHeight now uses a dimen constant. (3.50 KB, patch)
2012-07-30 19:34 PDT, Michael Comella (:mcomella) [not actively working on fennec: expect slow responses]
sriram.mozilla: review-
Details | Diff | Splinter Review
03b: Added padding for tablet layout. (5.67 KB, patch)
2012-07-31 11:09 PDT, Michael Comella (:mcomella) [not actively working on fennec: expect slow responses]
sriram.mozilla: review+
Details | Diff | Splinter Review
03c: Added padding for tablet layout. (5.72 KB, patch)
2012-07-31 11:15 PDT, Michael Comella (:mcomella) [not actively working on fennec: expect slow responses]
michael.l.comella: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review
04a: TopSitesItemHeight uses a constant and is retrieved dynamically (3.63 KB, patch)
2012-07-31 11:40 PDT, Michael Comella (:mcomella) [not actively working on fennec: expect slow responses]
sriram.mozilla: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review
05: about:home correctly adjusts itself on configuration change. (4.98 KB, patch)
2012-07-31 17:20 PDT, Michael Comella (:mcomella) [not actively working on fennec: expect slow responses]
sriram.mozilla: review+
Details | Diff | Splinter Review
about:home, 10" landscape after patch 05 w/ synced tabs (598.53 KB, image/jpeg)
2012-07-31 17:23 PDT, Michael Comella (:mcomella) [not actively working on fennec: expect slow responses]
no flags Details
01b: Integer Resources (11.89 KB, patch)
2012-08-01 14:34 PDT, Michael Comella (:mcomella) [not actively working on fennec: expect slow responses]
michael.l.comella: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review
02f: Cleaned about:home and created 10" tablet layout. (28.94 KB, patch)
2012-08-01 14:36 PDT, Michael Comella (:mcomella) [not actively working on fennec: expect slow responses]
michael.l.comella: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review
05a: about:home correctly adjusts itself on configuration change. (4.96 KB, patch)
2012-08-01 14:52 PDT, Michael Comella (:mcomella) [not actively working on fennec: expect slow responses]
michael.l.comella: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Lucas Rocha (:lucasr) 2011-09-13 13:46:16 PDT
We could do a more interesting about:home for the larger screen on tablets.
Comment 1 Lucas Rocha (:lucasr) 2011-09-22 07:37:28 PDT
*** Bug 688442 has been marked as a duplicate of this bug. ***
Comment 2 Ian Barlow (:ibarlow) 2011-09-23 12:38:33 PDT
Created attachment 562134 [details]
rough mockup

This is a ROUGH work in progess mockup of a potential first version of about:home for tablets. Functionally, it is the same as the phone version, but giving the CSS a good looking over to make it feel more tablet friendly. 

I'm not done yet, but wanted to post something to gauge people's reaction so far.
Comment 3 Philipp von Weitershausen [:philikon] 2011-10-06 12:46:04 PDT
Note that for the Sync Setup Improvements feature, we're adding a consistent way to discover Sync across all devices, using "Set up Sync" and "Pair a Device" links on the home tabs. This already landed in mobile in bug 675820.
Comment 4 Sriram Ramasubramanian [:sriram] 2011-10-06 14:29:06 PDT
Created attachment 565356 [details] [diff] [review]
Rough WIP

This patch adds a new file for honeycomb and makes the changes as per the mockup.
The padding and margin values are not exact as the mockup yet.
The width of the section halves when in landscape mode -- however, they don't switch to two different columns.

I feel, there should be more concrete decisions on what sections should be where, how each section will be when they are at their maximum and minimum heights.
Comment 5 Jennifer Morrow [:Boriss] (UX) 2012-01-20 15:40:25 PST
Ian, we should probably sync up on the designs of about:home on desktop and the start page on tablets.  Overall, we could probably use the same design on both with a few tweaks (bigger targets on tablet, more prominence for sync, etc).  I'm working on iteration 2 of about:home right now, but here's iteration 1: http://people.mozilla.com/~jboriss/specs/home_tab_first_iteration_spec1.png.  If you think tabs from last time is very important, perhaps we could display those at the bottom where the launch targets are in that spec.  Or, maybe we should just wait until the second iteration of desktop about:home is further along so it will be easier to add tablet-specific content
Comment 6 Ian Barlow (:ibarlow) 2012-01-23 07:07:24 PST
Hi Boriss, that start page is looking great, I look forward to seeing the next round of designs. I'll ping you this week to show you what we're thinking about on tablets. 

I think a key distinction on mobile will be to offer lots of ways to quickly and seamlessly get to your content without having to type anything. 

Currently we show (or are planning to show)
- Sync Setup
- Top Sites
- Add-ons
- Session Restore
- Remote Tabs

And down the road this could grow to include things like:
- Reading List
- Social
- Apps
- Pancake-style content stream

I will keep you posted, and share some more designs later this week :)
Comment 7 Ian Barlow (:ibarlow) 2012-07-24 13:12:11 PDT
sriram and mcomella, new mockups for about:home can be found here: http://www.flickr.com/photos/61892693@N03/sets/72157630730911182/detail/

And measurements for how to scale thumbnails across device sizes can be found here http://cl.ly/1s351T3u2o15

mcomella, I would suggest syncing up with Sriram on how to approach the layout changes -- there is probably a lot of styling that we can copy over from the current about:home implementation on phones.
Comment 8 Michael Comella (:mcomella) [not actively working on fennec: expect slow responses] 2012-07-25 12:50:02 PDT
Created attachment 645859 [details] [diff] [review]
Top Sites Change WIP

In addition to the XML changes, the top sites code needs to change. There are a few ways to do this. This is one of them.

I chose this way since it keeps the new code in the same place and requires the fewest changes to the existing code. I dislike this change because in order for TopSitesGridView to be inflated, it needs to be a static inner class (or at least, I believe that's the issue), meaning the mTopSites... etc. variables need to also be static. This assumes that we cannot have two simultaneous screen configurations with two AboutHomeContent instances... which seems like a reasonable assumption, given current devices, but it is not entirely future proof.

An alternative is to sub class AboutHomeContent for each layout type and have the mTopSites... variables get reset in those (also changing their access levels). This would work well to abstract things but it would be gross to have a new class for each layout. This also means the gecko_app.xml.in needs a new file for each layout to construct the appropriate AboutHomeContent (or alternatively, AboutHomeContent needs to wrap an appropriate subclass instance).

Is my solution ideal? Am I overthinking this?
Comment 9 Sriram Ramasubramanian [:sriram] 2012-07-25 13:29:51 PDT
Comment on attachment 645859 [details] [diff] [review]
Top Sites Change WIP

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

There is a better way to do this.
Please refer to values/dimens.xml and see how we use them.
Create appropriate integers.xml and get values from that.
Comment 10 Michael Comella (:mcomella) [not actively working on fennec: expect slow responses] 2012-07-25 15:17:35 PDT
Created attachment 645901 [details] [diff] [review]
Patch 01: Integer Resources

Moved to integer resources.
Comment 11 Sriram Ramasubramanian [:sriram] 2012-07-25 15:23:33 PDT
Comment on attachment 645901 [details] [diff] [review]
Patch 01: Integer Resources

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

Looks good overall.
I usually don't like doing this:
Resources res = getResources();
and then using it. Better use getResources() directly. It's not costly.

values-xlarge-land-v11 and values-land-v11 have the same values. You can axe values-xlarge-land-v11/. It will default to values-xlarge-v11/integers.xml.

And values-land/integers.xml overrides only one value. Please have only that value there.

setTopSitesConstants() can be moved further down -- to just before it's being used for the very first time.
Comment 12 Michael Comella (:mcomella) [not actively working on fennec: expect slow responses] 2012-07-25 15:48:07 PDT
Created attachment 645916 [details] [diff] [review]
01a: Integer Resources

Fixed the feedback.

(In reply to Sriram Ramasubramanian [:sriram] from comment #11)
> Comment on attachment 645901 [details] [diff] [review]
>
> setTopSitesConstants() can be moved further down -- to just before it's
> being used for the very first time.

Not sure what you meant by this so I moved both the declaration and the first call lower.
Comment 13 Michael Comella (:mcomella) [not actively working on fennec: expect slow responses] 2012-07-25 22:00:54 PDT
Created attachment 646025 [details]
02: Created 10" tablet layout.

I split the 10" layout into two columns with minimal copy-paste. Let me know what you think.

Also, abouthome_content_footer is a HORRIBLE name. I know. It will be changed.

Besides the TODOs in the code, here are some things that still need to happen:
* The layout does not change on rotation for the 10" tab. I tried adding requestLayout() to onConfigurationChanged but it did not seem to work.
* The padding between thumbnails needs to be adjusted.
* The thumbnail sizes needs to be adjusted (I will do this last as we spoke about)

There are also some implicit changes I noticed in the mocks that did not have specifications – font sizes, margins, and padding changes between different sized devices (for example, look at the thumbnail alignment with the device edge in relation to the "Top Sites" header in a portrait orientation). Should I include these changes?
Comment 14 Michael Comella (:mcomella) [not actively working on fennec: expect slow responses] 2012-07-25 22:04:21 PDT
I also was not sure where the sync container should go.
Comment 15 Michael Comella (:mcomella) [not actively working on fennec: expect slow responses] 2012-07-25 22:11:14 PDT
Created attachment 646030 [details] [diff] [review]
02a: Created 10" tablet layout.

Forgot to qref properly.
Comment 16 Michael Comella (:mcomella) [not actively working on fennec: expect slow responses] 2012-07-26 10:39:11 PDT
Created attachment 646199 [details]
10'' Tab landscape after patch 02a.
Comment 17 Sriram Ramasubramanian [:sriram] 2012-07-26 14:13:48 PDT
Comment on attachment 646030 [details] [diff] [review]
02a: Created 10" tablet layout.

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

We don't need to split the abouthome_content.xml into more resuable stuff. Because <include/> is costly.
On seeing the mockups, this is what I feel, everything except 10" landscape has one linear layout -- this will be satisfied by abouthome_content.xml
For the case of 10" landscape -- layout-xlarge-land-v11/ would satisfy.
Post a patch with this change -- just changing the layout as per mockup in layout-xlarge-land-v11/abouthome_content.xml(.in maybe)

The next patch would add necessary padding - both for entire about:home in 10", and between thumbnails. This can all be added in dimens.xml in various places and be used in layouts.

Please don't change the thumbnail size as of now.
Comment 18 Sriram Ramasubramanian [:sriram] 2012-07-26 14:14:34 PDT
Comment on attachment 645916 [details] [diff] [review]
01a: Integer Resources

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

This looks good to me.
Comment 19 Michael Comella (:mcomella) [not actively working on fennec: expect slow responses] 2012-07-27 13:37:48 PDT
Created attachment 646692 [details] [diff] [review]
02b: Created 10" tablet layout.

Updated the layout as per your comment.

I chose not to move the sync box as it's a little messy – it adds relative layout parameters in the code (AboutHomeContent.updateSyncLayout() – line 242), which would then be dependent on orientation and screen size.
Comment 20 Michael Comella (:mcomella) [not actively working on fennec: expect slow responses] 2012-07-27 13:38:42 PDT
> I chose not to move the sync box...

I think it would be more appropriate to do in a separate patch.
Comment 21 Michael Comella (:mcomella) [not actively working on fennec: expect slow responses] 2012-07-27 13:40:19 PDT
Created attachment 646694 [details]
about:home, 10" landscape, after patch 02b.
Comment 22 Michael Comella (:mcomella) [not actively working on fennec: expect slow responses] 2012-07-27 17:31:02 PDT
Created attachment 646779 [details] [diff] [review]
02c: Cleaned about:home and created 10" tablet layout.

As talked about in person, cleaned up about:home code and thus created a cleaner 10" tablet layout.
Comment 23 Michael Comella (:mcomella) [not actively working on fennec: expect slow responses] 2012-07-27 17:31:38 PDT
Created attachment 646781 [details]
about:home, 10" landscape, after patch 02c.
Comment 24 Michael Comella (:mcomella) [not actively working on fennec: expect slow responses] 2012-07-27 17:41:15 PDT
Created attachment 646783 [details] [diff] [review]
02d: Cleaned about:home and created 10" tablet layout.
Comment 25 Michael Comella (:mcomella) [not actively working on fennec: expect slow responses] 2012-07-27 17:43:50 PDT
Created attachment 646785 [details]
about:home, 10" landscape, after patch 02d.
Comment 26 Michael Comella (:mcomella) [not actively working on fennec: expect slow responses] 2012-07-27 17:48:41 PDT
Note that the screen still does not use the new resources on a configuration change (so starting in landscape and rotating will continue to use landscape about:home). I believe the View needs to be reinflated in onConfigurationChanged(). I will get to that ASAP.
Comment 27 Ian Barlow (:ibarlow) 2012-07-30 07:26:54 PDT
Hi mcomella, let me know when you feel like you are ready for a UI review. 

Happy to jump in whenever you're ready :)
Comment 28 Sriram Ramasubramanian [:sriram] 2012-07-30 11:01:10 PDT
Comment on attachment 646783 [details] [diff] [review]
02d: Cleaned about:home and created 10" tablet layout.

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

Overall looks good.
Few changes are needed:
1. android:layout_width="0dp" <--- use "wrap_content". It does the same job.
2. Instead of ".4", please use "0.4", looks better. (same for 0.6)
3. "match_parent" <-- this will crash in froyo. Please use "fill_parent".
4. The code logic has removed the need for "no_top_sites_text". Please remove it from both the XMLs.

And didn't we decide on a marginTop for the "top_sites_title"? How does that work without a marginTop?
Comment 29 Michael Comella (:mcomella) [not actively working on fennec: expect slow responses] 2012-07-30 14:03:26 PDT
Created attachment 647292 [details] [diff] [review]
02e: Cleaned about:home and created 10" tablet layout.

Note: had f+.

(In reply to Sriram Ramasubramanian [:sriram] from comment #28)
> Few changes are needed:
> 1. android:layout_width="0dp" <--- use "wrap_content". It does the same job.
As per talk in real-life, wrap_content does not work properly. Keeping 0dp.

> 2. Instead of ".4", please use "0.4", looks better. (same for 0.6)
Done.

> 3. "match_parent" <-- this will crash in froyo. Please use "fill_parent".
Done (additionally, bug 778862 to clean up additional uses).

> 4. The code logic has removed the need for "no_top_sites_text". Please
> remove it from both the XMLs.
Discussion for bug 778811.

> And didn't we decide on a marginTop for the "top_sites_title"? How does that
> work without a marginTop?
I don't remember discussing this. The element above it (the Firefox logo) uses marginBottom. Do you want this changed?

Things still to be changed (additional patches):
* Padding between thumbnails
* about:home doesn't change the view resources on orientation change. These must be re-inflated by hand.
Comment 30 Sriram Ramasubramanian [:sriram] 2012-07-30 14:08:45 PDT
Comment on attachment 647292 [details] [diff] [review]
02e: Cleaned about:home and created 10" tablet layout.

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

This looks good to me.
Comment 31 Michael Comella (:mcomella) [not actively working on fennec: expect slow responses] 2012-07-30 17:13:09 PDT
Created attachment 647359 [details] [diff] [review]
03: Added padding for tablet layout.
Comment 32 Michael Comella (:mcomella) [not actively working on fennec: expect slow responses] 2012-07-30 17:14:29 PDT
Created attachment 647360 [details]
about:home, 10" landscape after patch 03

Note: The FF icon (top-right) is slightly cut-off because the original graphic is created this way. ibarlow's mocks use the full image.
Comment 33 Michael Comella (:mcomella) [not actively working on fennec: expect slow responses] 2012-07-30 18:14:55 PDT
Created attachment 647387 [details] [diff] [review]
03a: Added padding for tablet layout.

Added padding for portrait mode too.
Comment 34 Michael Comella (:mcomella) [not actively working on fennec: expect slow responses] 2012-07-30 18:15:23 PDT
Created attachment 647388 [details]
about:home, 10" portrait after patch 03a
Comment 35 Michael Comella (:mcomella) [not actively working on fennec: expect slow responses] 2012-07-30 18:53:37 PDT
I added "onDestroy(); init();" to the onConfigurationChanged() code and it doesn't let me re-inflate because AboutHomeContent (as a Scrollview) can only have one direct child view.

I changed it to "onDestroy(); removeAllViews(); init();" and it does not crash on rotation, however, the child views do not repopulate their content properly. The top sites title, browse all your top sites text (which is positioned properly), and occasionally the sync box are all the elements that show. I'll keep looking into it but I was wondering if you had any idea why this would be.
Comment 36 Michael Comella (:mcomella) [not actively working on fennec: expect slow responses] 2012-07-30 19:34:03 PDT
Created attachment 647409 [details] [diff] [review]
04: kTopSitesItemHeight now uses a dimen constant.

Updated the value to 106 also.
Comment 37 Lucas Rocha (:lucasr) 2012-07-31 02:48:25 PDT
(In reply to Michael Comella (:mcomella) from comment #35)
> I added "onDestroy(); init();" to the onConfigurationChanged() code and it
> doesn't let me re-inflate because AboutHomeContent (as a Scrollview) can
> only have one direct child view.
> 
> I changed it to "onDestroy(); removeAllViews(); init();" and it does not
> crash on rotation, however, the child views do not repopulate their content
> properly. The top sites title, browse all your top sites text (which is
> positioned properly), and occasionally the sync box are all the elements
> that show. I'll keep looking into it but I was wondering if you had any idea
> why this would be.

Drive-by comment: calling API callbacks (especially life-cycle ones) directly is very rarely a good idea... Factor out the code into separate private methods and call them appropriately where needed.
Comment 38 Aaron Train [:aaronmt] 2012-07-31 06:48:45 PDT
Work in here might also want to consider bug 778914.
Comment 39 Sriram Ramasubramanian [:sriram] 2012-07-31 10:51:07 PDT
Comment on attachment 647387 [details] [diff] [review]
03a: Added padding for tablet layout.

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

The approach is good. But I would like following changes for the sake of symmetric behavior.
There is a "column_spacing". Instead, try applying the margin to the two vertical columns (instead of the parent).
Please kill the new folder values-xlarge-land-v11. (Based on above comment, we dont want it anymore. But still, it can always default to values-xlarge-v11/ folder if the dimen name is different).

So, if you could rename the dimens to:
abouthome_gutter_small - value of 20dp and
abouthome_gutter_large - value of 40dp, it should be good.
Comment 40 Sriram Ramasubramanian [:sriram] 2012-07-31 10:53:43 PDT
Comment on attachment 647409 [details] [diff] [review]
04: kTopSitesItemHeight now uses a dimen constant.

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

kTopStiesXxx.. will become mTopSitesXxx.
mDisplayDensity is no longer need.
It's better to set the value of mTopSitesXxx inside the constructor.
Please make these changes.
Comment 41 Michael Comella (:mcomella) [not actively working on fennec: expect slow responses] 2012-07-31 11:09:50 PDT
Created attachment 647604 [details] [diff] [review]
03b: Added padding for tablet layout.
Comment 42 Sriram Ramasubramanian [:sriram] 2012-07-31 11:11:58 PDT
Comment on attachment 647604 [details] [diff] [review]
03b: Added padding for tablet layout.

Perfect! :)
Though "abouthome_gutter_small" is not used in values/, please add it there too, for the sake of completion. It will be 0dp.
Comment 43 Michael Comella (:mcomella) [not actively working on fennec: expect slow responses] 2012-07-31 11:15:42 PDT
Created attachment 647607 [details] [diff] [review]
03c: Added padding for tablet layout.

Moved r+. Added abouthome_gutter_small to .../res/values/dimens.xml.
Comment 44 Michael Comella (:mcomella) [not actively working on fennec: expect slow responses] 2012-07-31 11:40:18 PDT
Created attachment 647615 [details] [diff] [review]
04a: TopSitesItemHeight uses a constant and is retrieved dynamically
Comment 45 Sriram Ramasubramanian [:sriram] 2012-07-31 11:49:19 PDT
Comment on attachment 647615 [details] [diff] [review]
04a: TopSitesItemHeight uses a constant and is retrieved dynamically

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

Looks good to me.
Comment 46 Michael Comella (:mcomella) [not actively working on fennec: expect slow responses] 2012-07-31 17:20:58 PDT
Created attachment 647774 [details] [diff] [review]
05: about:home correctly adjusts itself on configuration change.
Comment 47 Michael Comella (:mcomella) [not actively working on fennec: expect slow responses] 2012-07-31 17:23:06 PDT
Created attachment 647776 [details]
about:home, 10" landscape after patch 05 w/ synced tabs
Comment 48 Sriram Ramasubramanian [:sriram] 2012-08-01 10:23:13 PDT
Comment on attachment 647774 [details] [diff] [review]
05: about:home correctly adjusts itself on configuration change.

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

This looks good to me. Few changes:
1. Let's call init() as inflate() -- the one that inflates the view
2. The one that does a much of findViewById -- as init()
Comment 49 Sriram Ramasubramanian [:sriram] 2012-08-01 10:27:22 PDT
(In reply to Sriram Ramasubramanian [:sriram] from comment #48)
> Comment on attachment 647774 [details] [diff] [review]
> 05: about:home correctly adjusts itself on configuration change.
> 
> Review of attachment 647774 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good to me. Few changes:
> 1. Let's call init() as inflate() -- the one that inflates the view
> 2. The one that does a much of findViewById -- as init()

And when you change, you might need to change in BrowserApp as well.
Comment 50 Michael Comella (:mcomella) [not actively working on fennec: expect slow responses] 2012-08-01 14:34:38 PDT
Created attachment 648091 [details] [diff] [review]
01b: Integer Resources

Rebase to trunk. Moved r+.
Comment 51 Michael Comella (:mcomella) [not actively working on fennec: expect slow responses] 2012-08-01 14:36:55 PDT
Created attachment 648092 [details] [diff] [review]
02f: Cleaned about:home and created 10" tablet layout.

Rebase to trunk. Moved r+.
Comment 52 Michael Comella (:mcomella) [not actively working on fennec: expect slow responses] 2012-08-01 14:52:34 PDT
Created attachment 648097 [details] [diff] [review]
05a: about:home correctly adjusts itself on configuration change.

Made method name changes, moved r+. I have a bad build so I can't test it. I will mark checkin-needed if everything is okay afterward.
Comment 55 Aaron Train [:aaronmt] 2012-08-02 12:08:20 PDT
Verified via Galaxy Tab 10.1 (Android 4.0.3)
Comment 56 Sriram Ramasubramanian [:sriram] 2012-08-10 11:35:35 PDT
Comment on attachment 648091 [details] [diff] [review]
01b: Integer Resources

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New UI
User impact if declined: No optimized about:home for tablets
Testing completed (on m-c, etc.): Landed in m-c on 08/02
Risk to taking this patch (and alternatives if risky): None.
String or UUID changes made by this patch: None.
Comment 57 Sriram Ramasubramanian [:sriram] 2012-08-10 11:35:54 PDT
Comment on attachment 648092 [details] [diff] [review]
02f: Cleaned about:home and created 10" tablet layout.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New UI
User impact if declined: No optimized about:home for tablets
Testing completed (on m-c, etc.): Landed in m-c on 08/02
Risk to taking this patch (and alternatives if risky): None.
String or UUID changes made by this patch: None.
Comment 58 Sriram Ramasubramanian [:sriram] 2012-08-10 11:36:08 PDT
Comment on attachment 648097 [details] [diff] [review]
05a: about:home correctly adjusts itself on configuration change.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New UI
User impact if declined: No optimized about:home for tablets
Testing completed (on m-c, etc.): Landed in m-c on 08/02
Risk to taking this patch (and alternatives if risky): None.
String or UUID changes made by this patch: None.
Comment 59 Sriram Ramasubramanian [:sriram] 2012-08-10 11:36:24 PDT
Comment on attachment 647607 [details] [diff] [review]
03c: Added padding for tablet layout.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New UI
User impact if declined: No optimized about:home for tablets
Testing completed (on m-c, etc.): Landed in m-c on 08/02
Risk to taking this patch (and alternatives if risky): None.
String or UUID changes made by this patch: None.
Comment 60 Sriram Ramasubramanian [:sriram] 2012-08-10 11:36:41 PDT
Comment on attachment 647615 [details] [diff] [review]
04a: TopSitesItemHeight uses a constant and is retrieved dynamically

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New UI
User impact if declined: No optimized about:home for tablets
Testing completed (on m-c, etc.): Landed in m-c on 08/02
Risk to taking this patch (and alternatives if risky): None.
String or UUID changes made by this patch: None.
Comment 61 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-10 11:42:35 PDT
Approving for branches and adding qawanted - can QA please check once this has landed on Beta, against our shipping Android locales to make sure that this new layout doesn't break on any languages?
Comment 62 Michael Comella (:mcomella) [not actively working on fennec: expect slow responses] 2012-08-13 15:03:55 PDT
Please note these patches should be checked in, then the patches in bug 780481 and bug 780367 (these last two can be in any order).
Comment 64 Ryan VanderMeulen [:RyanVM] 2012-08-13 18:49:51 PDT
Had to push a bustage fix for part 5 to both branches. Based on code inspection, I'm assuming this was the correct fix.

https://hg.mozilla.org/releases/mozilla-aurora/rev/3b53726859fd
https://hg.mozilla.org/releases/mozilla-beta/rev/2de209a396f2

For posterity, the build error was:

/builds/slave/m-beta-andrd/build/mobile/android/base/AboutHomeContent.java:155: cannot find symbol
symbol  : variable mContext
location: class org.mozilla.gecko.AboutHomeContent
        mInflater = (LayoutInflater) mContext.getSystemService(Context.LAYOUT_INFLATER_SERVICE);
Comment 65 Ryan VanderMeulen [:RyanVM] 2012-08-13 19:07:18 PDT
Well, so much for that idea. Still busted. Please post branch-specific patches. Backed out from both branches.

https://hg.mozilla.org/releases/mozilla-aurora/rev/1b8af79d5194
https://hg.mozilla.org/releases/mozilla-beta/rev/09e2ae313229
Comment 66 Sriram Ramasubramanian [:sriram] 2012-08-13 23:14:32 PDT
So, are all patches backed out?
Comment 67 Ryan VanderMeulen [:RyanVM] 2012-08-14 06:56:14 PDT
Yes, per the commit message on the backouts.
Comment 70 Adrian Tamas (:AdrianT) 2012-09-27 02:47:34 PDT
New about:home page is present on:

Firefox Mobile 15 / Firefox Mobile 16.0b5 
Asus Transformer TF101 (Android 4.0.3)

Marking as verified.

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