Last Comment Bug 735741 - Clean up about:home
: Clean up about:home
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: Trunk
: ARM Android
-- normal (vote)
: Firefox 14
Assigned To: Sriram Ramasubramanian [:sriram]
: Sebastian Kaspari (:sebastian)
Depends on:
Blocks: 736422
  Show dependency treegraph
Reported: 2012-03-14 10:07 PDT by Sriram Ramasubramanian [:sriram]
Modified: 2012-03-24 08:54 PDT (History)
2 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (31.48 KB, patch)
2012-03-14 10:09 PDT, Sriram Ramasubramanian [:sriram]
mark.finkle: review+
Details | Diff | Splinter Review
Patch 2: Remove callback (6.53 KB, patch)
2012-03-14 10:16 PDT, Sriram Ramasubramanian [:sriram]
mark.finkle: review-
Details | Diff | Splinter Review

Description User image Sriram Ramasubramanian [:sriram] 2012-03-14 10:07:22 PDT
There are 3-4 sections in about:home that use similar style:
* Title
* Subtitle (at times)
* A container to show a list of items
* A link to show more

This can be factored out to reusable UI, there by we can avoid a lot of "findViewById()" in AboutHomeContent.

Also, the UI for rows use many views. They can be shrunk to use minimal views to display the content.
Comment 1 User image Sriram Ramasubramanian [:sriram] 2012-03-14 10:09:33 PDT
Created attachment 605822 [details] [diff] [review]

This patch factors out each section as reusable UI component. This uses "gecko" namespace to show title, subtitle and more_text.
Comment 2 User image Sriram Ramasubramanian [:sriram] 2012-03-14 10:16:51 PDT
Created attachment 605828 [details] [diff] [review]
Patch 2: Remove callback

This patch removes an unwanted callback that was used to load the url. loadUrl() sends a message to Gecko, which eventually starts the spinner. Loading the spinner before Gecko sends it is not needed (or, that can be moved into loadUrl to be consistent with others).
Comment 3 User image Mark Finkle (:mfinkle) (use needinfo?) 2012-03-16 09:16:40 PDT
Comment on attachment 605822 [details] [diff] [review]

Removing a few findViewById calls might help performance a bit. I don't know if any other changes could be negatively affecting performance though. It doesn't seem like it should.

The sections you are changing can have a lot of variations. They can be visible/invisible at various times in various situations. We need to test to make sure you have not broken anything in these situations.

We should start thinking about how to test those situations. File a bug to add tests for about home content.
Comment 4 User image Mark Finkle (:mfinkle) (use needinfo?) 2012-03-16 09:22:00 PDT
Comment on attachment 605828 [details] [diff] [review]
Patch 2: Remove callback

>diff --git a/mobile/android/base/ b/mobile/android/base/
>-                    mAboutHomeContent.setUriLoadCallback(new AboutHomeContent.UriLoadCallback() {
>-                        public void callback(String url) {
>-                            mBrowserToolbar.setProgressVisibility(true);
>-                            loadUrl(url, AwesomeBar.Type.EDIT);
>-                        }
>-                    });

I don't know if you realized it or not, but you are not keeping the setProgressVisibility(true) call. This call jump starts the throbber. I am not a big fan of jump starting the throbber because we can get into situations where the throbber never shuts off, but it has not hurt us yet. I do like the de-coupling that the callback gives us.

Let's just keep it.
Comment 5 User image Sriram Ramasubramanian [:sriram] 2012-03-16 12:14:51 PDT
I've removed jump starting the progress bar because it starts when Gecko sends us a message of location change. I felt it unnecessary and thought of having things work the usual way.
Comment 6 User image Sriram Ramasubramanian [:sriram] 2012-03-19 16:16:30 PDT - Patch (1/2) pushed.
Comment 7 User image Mounir Lamouri (:mounir) 2012-03-20 03:49:24 PDT
Comment 8 User image Mark Finkle (:mfinkle) (use needinfo?) 2012-03-24 08:54:43 PDT

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