735741 - Clean up about:home

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sriram Ramasubramanian [:sriram]]

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]]

Attachment: Patch

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]]

Attachment: 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?)]

Comment on attachment 605822 [details] [diff] [review]
Patch

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?)]

Comment on attachment 605828 [details] [diff] [review]
Patch 2: Remove callback

>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java
>-                    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]]

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]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/0e65d7c19bea - Patch (1/2) pushed.

Comment 7

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/mozilla-central/rev/0e65d7c19bea

Comment 8

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/489cd94e6bb7