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]
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 | Review
Patch 2: Remove callback (6.53 KB, patch)
2012-03-14 10:16 PDT, Sriram Ramasubramanian [:sriram]
mark.finkle: review-
Details | Diff | Review

Description 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 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 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 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 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 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 Sriram Ramasubramanian [:sriram] 2012-03-19 16:16:30 PDT - Patch (1/2) pushed.
Comment 7 Mounir Lamouri (:mounir) 2012-03-20 03:49:24 PDT
Comment 8 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.