Clean up about:home

RESOLVED FIXED in Firefox 13

Status

()

Firefox for Android
General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: sriram, Assigned: sriram)

Tracking

Trunk
Firefox 14
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox13 fixed, firefox14 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 605822 [details] [diff] [review]
Patch

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)
(Assignee)

Comment 2

6 years ago
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).
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-
Blocks: 736422
(Assignee)

Comment 5

6 years ago
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.
(Assignee)

Comment 6

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/0e65d7c19bea - Patch (1/2) pushed.
https://hg.mozilla.org/mozilla-central/rev/0e65d7c19bea
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
Version: unspecified → Trunk
https://hg.mozilla.org/releases/mozilla-aurora/rev/489cd94e6bb7
status-firefox13: --- → fixed
status-firefox14: --- → fixed
You need to log in before you can comment on or make changes to this bug.