Closed Bug 735741 Opened 8 years ago Closed 8 years ago

Clean up about:home

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 14
Tracking Status
firefox13 --- fixed
firefox14 --- fixed

People

(Reporter: sriram, Assigned: sriram)

References

Details

Attachments

(2 files)

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.
Attached patch PatchSplinter Review
This patch factors out each section as reusable UI component. This uses "gecko" namespace to show title, subtitle and more_text.
Assignee: nobody → sriram
Attachment #605822 - Flags: review?(mark.finkle)
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).
Attachment #605828 - Flags: review?(mark.finkle)
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.
Attachment #605822 - Flags: review?(mark.finkle) → review+
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.
Attachment #605828 - Flags: review?(mark.finkle) → review-
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.
https://hg.mozilla.org/mozilla-central/rev/0e65d7c19bea
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.