Closed Bug 880047 Opened 7 years ago Closed 7 years ago

[fig] Kill unused AboutHome code

Categories

(Firefox for Android :: General, defect, P1)

ARM
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 26

People

(Reporter: Margaret, Assigned: capella)

References

Details

(Whiteboard: fixed-fig)

Attachments

(7 files, 8 obsolete files)

15.97 KB, patch
Margaret
: review+
Margaret
: checkin+
Details | Diff | Splinter Review
24.81 KB, patch
Margaret
: review+
capella
: checkin+
Details | Diff | Splinter Review
17.61 KB, patch
Margaret
: review+
capella
: checkin+
Details | Diff | Splinter Review
56.58 KB, patch
Margaret
: review+
Details | Diff | Splinter Review
9.90 KB, patch
Margaret
: review+
Details | Diff | Splinter Review
56.67 KB, patch
Margaret
: review+
Details | Diff | Splinter Review
26.96 KB, patch
Margaret
: review+
Details | Diff | Splinter Review
Same idea as bug 876712, but I didn't want to make the scope of that bug too big. Things to remove:

AboutHome
AboutHomeView
AboutHomeSection
AddonsSection
LastTabsSection
RemoteTabsSection
TopSitesView

And of course all the various layout files that go along with these.
This seems like a good starting point... I like r̶i̶p̶p̶i̶n̶g̶ ̶o̶u̶t̶  simplifying code :-D
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attached patch LastTabsSection Removal (v1) (obsolete) — Splinter Review
First patch just to remove the LastTabsSection related code...
Attachment #766396 - Flags: review?(margaret.leibovic)
Attached patch AddonsSection Removal (v1) (obsolete) — Splinter Review
Removes the Addons Section
Attachment #766429 - Flags: review?(margaret.leibovic)
Attached patch RemoteTabsSection Removal (v1) (obsolete) — Splinter Review
Similar to the first two patches, plus removes some <style> entries previously shared by all three
Attachment #766482 - Flags: review?(margaret.leibovic)
Attached patch TopSitesView Removal (v1) (obsolete) — Splinter Review
Ouch! This one was a little more involved ... :-D
Attachment #766516 - Flags: review?(margaret.leibovic)
Attached patch AboutHomeSection Removal (v1) (obsolete) — Splinter Review
An easy one ...
Attachment #766640 - Flags: review?(margaret.leibovic)
Attached patch PromoBox Removal (v1) (obsolete) — Splinter Review
More good stuff...
Attachment #766666 - Flags: review?(margaret.leibovic)
Comment on attachment 766396 [details] [diff] [review]
LastTabsSection Removal (v1)

Review of attachment 766396 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/BrowserApp.java
@@ -358,5 @@
> -            }
> -        });
> -
> -        super.onStatePurged();
> -    }

I don't think we want to be getting rid of all of this, since there are plans for a "Tabs from last time" section in the visited page:
https://bug862793.bugzilla.mozilla.org/attachment.cgi?id=739728

Let's restrict this to just getting rid of the unused UI pieces of about:home. What we can do is make sure there's a bug filed to implement this new "Tabs from last time" section, and put a FIXME comment in here referencing that bug number.

I'm actually wondering if we might want to just update LastTabsSection to implement that new feature, so maybe we shouldn't get rid of that just yet either.
Attachment #766396 - Flags: review?(margaret.leibovic) → review-
Comment on attachment 766429 [details] [diff] [review]
AddonsSection Removal (v1)

Review of attachment 766429 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/widget/AddonsSection.java
@@ -61,5 @@
> -        setOnMoreTextClickListener(new View.OnClickListener() {
> -            @Override
> -            public void onClick(View v) {
> -                if (mUriLoadListener != null)
> -                    mUriLoadListener.onAboutHomeUriLoad(mContext.getString(R.string.bookmarkdefaults_url_addons));

This string is unused now, but we can keep it around, since we'll probably need it for the new add-ons page.

@@ -143,5 @@
> -    public void readRecommendedAddons() {
> -        new UiAsyncTask<Void, Void, JSONArray>(ThreadUtils.getBackgroundHandler()) {
> -            @Override
> -            public JSONArray doInBackground(Void... params) {
> -                final String addonsFilename = "recommended-addons.json";

The add-ons section in the new about:home also has a recommended add-ons section, so I guess it makes sense to keep recommended-addons.json around, too. I'm starting to think that it may make sense to keep this AddonsSection file around, since we'll want to use some of this logic for the new about:home (similarly to LastTabsSection).
Comment on attachment 766429 [details] [diff] [review]
AddonsSection Removal (v1)

Clearing review on these for now... Capella and I discussed on IRC that we should probably keep this code around for now until we're done porting things over into the new about:home pages.

We can revisit this bug when we get closer to merging back to m-c.
Attachment #766429 - Flags: review?(margaret.leibovic)
Attachment #766482 - Flags: review?(margaret.leibovic)
Attachment #766516 - Flags: review?(margaret.leibovic)
Attachment #766640 - Flags: review?(margaret.leibovic)
Attachment #766666 - Flags: review?(margaret.leibovic)
Priority: -- → P1
Ok Margaret where were we?

The first two chunks I originally pulled out were the LastTabsSection and the AddonsSection.

You were thinking since those were to be mostly re-used, we might want to leave those alone for now. 

(Alternatively we could rebase and remove those two sections, and let the next dev / implementer refer to these patches for the affected code while restoring just the pieces parts they need ... ?)

Staying with your thought, I'll skip that for now, and jump to re-basing the next patches after those two, removing the RemoteTabsSection, then TopSitesView, AboutHomeSection, and PromoBox and see what's left somewhere along at about that point.

Yes...?

( Also, I'm assuming you won't need me flagging this for |feedback| everytime I leave a comment from now on, since you're actively involved  :-D )
Let's try the first one :D
Attachment #766482 - Attachment is obsolete: true
Attachment #773860 - Flags: feedback?(margaret.leibovic)
Comment on attachment 773860 [details] [diff] [review]
RemoteTabsSection Removal (v2)

Review of attachment 773860 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good! I don't think any of this is worth keeping, so I'm just upgrading your feedback? to a review+.
Attachment #773860 - Flags: feedback?(margaret.leibovic) → review+
Scooping up the field mice and bonk them on the head !!!
http://hg.mozilla.org/projects/fig/rev/101158e5e302
Comment on attachment 773860 [details] [diff] [review]
RemoteTabsSection Removal (v2)

If we're going to land patches incrementally, let's use the checkin flag to keep track of which of these patches are checked in or not.
Attachment #773860 - Flags: checkin+
Attachment #766429 - Attachment is obsolete: true
Attachment #777230 - Flags: review?(margaret.leibovic)
Comment on attachment 777230 [details] [diff] [review]
AddonsSection Removal (v2)

Looking good.
Attachment #777230 - Flags: review?(margaret.leibovic) → review+
Attachment #777230 - Flags: checkin+
Kill more thingz :-P
Attachment #766396 - Attachment is obsolete: true
Attachment #777767 - Flags: review?(margaret.leibovic)
Comment on attachment 777767 [details] [diff] [review]
LastTabsSection Removal (v2)

LGTM.
Attachment #777767 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 777767 [details] [diff] [review]
LastTabsSection Removal (v2)

And then sriram takes over :)
Attachment #777767 - Flags: checkin+
This cleans up TopSitesView.
Attachment #778061 - Flags: review?(margaret.leibovic)
We don't use this widget anymore. That small little "Link>>". Purged.
Attachment #778063 - Flags: review?(margaret.leibovic)
Attached patch About:home Removal (obsolete) — Splinter Review
This kills anything about:home! We don't need PromoBox anymore. So, that's removed. The new design doesn't reflect any of that here.

I've retained the text used for apps and sync in PromoBox, just in case we re-use them. Other than that nothing else is used from that.

There are few resources still named/re-used as "abouthome_*". I'll rename them and post a new patch (may be in this bug itself).
Attachment #778066 - Flags: review?(margaret.leibovic)
Attached patch Rename old abouthome_* (obsolete) — Splinter Review
Renamed all old abouthome_* to home_* for consistency with layout files. Also, styles are made to follow the new scheme of "Widget.Home" and "TextAppearance.Widget.Home".

Note: I've left abouthome_thumbnail be the same, as it makes more sense than home_thumbnail.
Attachment #778087 - Flags: review?(margaret.leibovic)
Attachment #766516 - Attachment is obsolete: true
Attachment #766640 - Attachment is obsolete: true
Attachment #766666 - Attachment is obsolete: true
Comment on attachment 778061 [details] [diff] [review]
TopSitesView Removal

Review of attachment 778061 [details] [diff] [review]:
-----------------------------------------------------------------

r- until we make sure that clearing history clears items on about:home.

::: mobile/android/base/BrowserApp.java
@@ -245,5 @@
>      @Override
> -    void handleClearHistory() {
> -        super.handleClearHistory();
> -        updateAboutHomeTopSites();
> -    }

I think we need to keep this... we need to do something to clear items from about:home when the user clears their history. Or will the adapters handle that for us? We should verify the behavior before getting rid of this.

::: mobile/android/base/Makefile.in
@@ -618,5 @@
>    res/drawable-mdpi/abouthome_promo_box_bg.9.png \
>    res/drawable-mdpi/abouthome_promo_box_pressed_bg.9.png \
>    res/drawable-mdpi/abouthome_promo_logo_apps.png \
>    res/drawable-mdpi/abouthome_promo_logo_sync.png \
>    res/drawable-mdpi/abouthome_thumbnail.png \

Note to self: this is a thumbnail of about:home used in TabsTray, so it should stay.

@@ -624,1 @@
>    res/drawable-mdpi/abouthome_thumbnail_add.png \

I see that we're using abouthome_thumbnail_add in TopBookmarksView. Maybe we should rename it something like top_bookmark_add. But that can be a follow-up (or go in your "rename stuff" patch).

::: mobile/android/base/strings.xml.in
@@ -240,5 @@
>    <string name="abouthome_about_sync">&abouthome_about_sync3;</string>
>    <string name="abouthome_about_apps">&abouthome_about_apps3;</string>
>    <string name="abouthome_sync_bold_name">&abouthome_sync_bold_name;</string>
>    <string name="abouthome_apps_bold_name">&abouthome_apps_bold_name2;</string>
>    

Nit: kill this whitespace while you're here.
Attachment #778061 - Flags: review?(margaret.leibovic) → review-
Attachment #778063 - Flags: review?(margaret.leibovic) → review+
(In reply to :Margaret Leibovic from comment #28)
> Comment on attachment 778061 [details] [diff] [review]
> TopSitesView Removal
> 
> Review of attachment 778061 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- until we make sure that clearing history clears items on about:home.

We needed that code because we never updated about:home, whenever we showed it. Now that we kill about:home fragment and reload cursors when we switch to it, this is not needed.
I tried navigating to a page and clearing history, and back to about:home -- the page wasn't found.
Comment on attachment 778066 [details] [diff] [review]
About:home Removal

Review of attachment 778066 [details] [diff] [review]:
-----------------------------------------------------------------

Similar question about handling onStatePurged.

::: mobile/android/base/BrowserApp.java
@@ -389,5 @@
> -                mHomePager.setAboutHomeLastTabsVisibility(false);
> -            }
> -        });
> -
> -        super.onStatePurged();

Do we need to update LastTabsPage to update itself in this case? If so, we should keep this and file a follow-up bug to fix that.

@@ -1392,5 @@
>  
>          // FIXME: do animation if animate is true
>          mHomePager.show(getSupportFragmentManager());
> -
> -        mBrowserToolbar.setNextFocusDownId(R.id.abouthome_content);

I realize this must not actually be doing anything useful in fig right now, but do we need to file a follow-up bug for adding back this behavior for accessibility? I see bnicholson added this, we should ask him.

::: mobile/android/base/Makefile.in
@@ -617,2 @@
>    res/drawable-mdpi/abouthome_promo_logo_apps.png \
>    res/drawable-mdpi/abouthome_promo_logo_sync.png \

We can get rid of these icons, too.
Attachment #778066 - Flags: review?(margaret.leibovic) → review-
(In reply to :Margaret Leibovic from comment #30)
> Comment on attachment 778066 [details] [diff] [review]
> About:home Removal
> 
> Review of attachment 778066 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Similar question about handling onStatePurged.
> 
> ::: mobile/android/base/BrowserApp.java
> @@ -389,5 @@
> > -                mHomePager.setAboutHomeLastTabsVisibility(false);
> > -            }
> > -        });
> > -
> > -        super.onStatePurged();
> 
> Do we need to update LastTabsPage to update itself in this case? If so, we
> should keep this and file a follow-up bug to fix that.

The adapter will take care of it.

> 
> @@ -1392,5 @@
> >  
> >          // FIXME: do animation if animate is true
> >          mHomePager.show(getSupportFragmentManager());
> > -
> > -        mBrowserToolbar.setNextFocusDownId(R.id.abouthome_content);
> 
> I realize this must not actually be doing anything useful in fig right now,
> but do we need to file a follow-up bug for adding back this behavior for
> accessibility? I see bnicholson added this, we should ask him.

I'll file a followup.

> 
> ::: mobile/android/base/Makefile.in
> @@ -617,2 @@
> >    res/drawable-mdpi/abouthome_promo_logo_apps.png \
> >    res/drawable-mdpi/abouthome_promo_logo_sync.png \
> 
> We can get rid of these icons, too.

Don't we need these icons? Do you need a new patch or same patch with this removed? My rename patch depends on this. I can post a new patch if you want.
Comment on attachment 778087 [details] [diff] [review]
Rename old abouthome_*

Review of attachment 778087 [details] [diff] [review]:
-----------------------------------------------------------------

r- because we should kill off the PromoBox related things because they're not used anymore (you can do that in the other patch).

::: mobile/android/base/Makefile.in
@@ +602,2 @@
>    res/drawable-mdpi/abouthome_thumbnail.png \
> +  res/drawable-mdpi/home_add_bookmark.png \

To be consistent with the other TopBookmark resources, I think it would be better to name this top_bookmark_add or something like that.

@@ +602,4 @@
>    res/drawable-mdpi/abouthome_thumbnail.png \
> +  res/drawable-mdpi/home_add_bookmark.png \
> +  res/drawable-mdpi/home_promo_logo_apps.png \
> +  res/drawable-mdpi/home_promo_logo_sync.png \

I mentioned this in my comments for the last patch - I don't think we need these two icons anymore.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +282,4 @@
>       strings above. These strings should be a subset of the strings above and
>       generally be the name of the product the string describes. -->
> +<!ENTITY home_sync_bold_name "Firefox Sync">
> +<!ENTITY home_apps_bold_name2 "Firefox Marketplace">

We can just kill all these strings with the patch that killed the PromoBox, since that was the only place they were used.

::: mobile/android/base/resources/values/colors.xml
@@ +72,5 @@
>    <color name="url_bar_text_highlight_pb">#FFD06BFF</color>
>    <color name="suggestion_primary">#dddddd</color>
>    <color name="suggestion_pressed">#bbbbbb</color>
>    <color name="tab_row_pressed">#4D000000</color>
> +  <color name="home_topsite_pin">#55000000</color>

How about top_bookmark_pin?

::: mobile/android/base/resources/values/dimens.xml
@@ +4,5 @@
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
>  <resources>
>  
> +    <dimen name="home_topsite_pinsize">20dp</dimen>

And top_bookmark_pinsize?

::: mobile/android/base/resources/values/styles.xml
@@ +192,5 @@
> +        <item name="android:focusable">false</item>
> +        <item name="android:gravity">center|left</item>
> +        <item name="android:paddingLeft">10dip</item>
> +        <item name="android:paddingRight">10dip</item>
> +    </style>

Does this really need to move to a different part of the file? It makes the diff bigger.
Attachment #778087 - Flags: review?(margaret.leibovic) → review-
(In reply to :Margaret Leibovic from comment #32)
> Comment on attachment 778087 [details] [diff] [review]
> Rename old abouthome_*
> 
> Review of attachment 778087 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- because we should kill off the PromoBox related things because they're
> not used anymore (you can do that in the other patch).
> 

Could those be in a separate patch on top of this?

> ::: mobile/android/base/Makefile.in
> @@ +602,2 @@
> >    res/drawable-mdpi/abouthome_thumbnail.png \
> > +  res/drawable-mdpi/home_add_bookmark.png \
> 
> To be consistent with the other TopBookmark resources, I think it would be
> better to name this top_bookmark_add or something like that.
> 

I named it "home_add_bookmark" as it corresponds to "Add a bookmark" text. I can rename it.

> @@ +602,4 @@
> >    res/drawable-mdpi/abouthome_thumbnail.png \
> > +  res/drawable-mdpi/home_add_bookmark.png \
> > +  res/drawable-mdpi/home_promo_logo_apps.png \
> > +  res/drawable-mdpi/home_promo_logo_sync.png \
> 
> I mentioned this in my comments for the last patch - I don't think we need
> these two icons anymore.

Separate patch?

> 
> ::: mobile/android/base/locales/en-US/android_strings.dtd
> @@ +282,4 @@
> >       strings above. These strings should be a subset of the strings above and
> >       generally be the name of the product the string describes. -->
> > +<!ENTITY home_sync_bold_name "Firefox Sync">
> > +<!ENTITY home_apps_bold_name2 "Firefox Marketplace">
> 
> We can just kill all these strings with the patch that killed the PromoBox,
> since that was the only place they were used.
> 
> ::: mobile/android/base/resources/values/colors.xml
> @@ +72,5 @@
> >    <color name="url_bar_text_highlight_pb">#FFD06BFF</color>
> >    <color name="suggestion_primary">#dddddd</color>
> >    <color name="suggestion_pressed">#bbbbbb</color>
> >    <color name="tab_row_pressed">#4D000000</color>
> > +  <color name="home_topsite_pin">#55000000</color>
> 
> How about top_bookmark_pin?
> 

Oops. I wanted to change it. But forgot.

> ::: mobile/android/base/resources/values/dimens.xml
> @@ +4,5 @@
> >     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> >  
> >  <resources>
> >  
> > +    <dimen name="home_topsite_pinsize">20dp</dimen>
> 
> And top_bookmark_pinsize?
> 
> ::: mobile/android/base/resources/values/styles.xml
> @@ +192,5 @@
> > +        <item name="android:focusable">false</item>
> > +        <item name="android:gravity">center|left</item>
> > +        <item name="android:paddingLeft">10dip</item>
> > +        <item name="android:paddingRight">10dip</item>
> > +    </style>
> 
> Does this really need to move to a different part of the file? It makes the
> diff bigger.

I prefer keeping all "Widget" styles and "TextAppearance" styles closer. That way we can keep track of inconsistent ones easily.
The diff might be bigger, but the file size stays the same. ;)
Thanks for addressing my concerns about the clearHistory/statePurged issues. It's convenient that about:home isn't really a real page anymore.

(In reply to Sriram Ramasubramanian [:sriram] from comment #31)

> > 
> > ::: mobile/android/base/Makefile.in
> > @@ -617,2 @@
> > >    res/drawable-mdpi/abouthome_promo_logo_apps.png \
> > >    res/drawable-mdpi/abouthome_promo_logo_sync.png \
> > 
> > We can get rid of these icons, too.
> 
> Don't we need these icons? Do you need a new patch or same patch with this
> removed? My rename patch depends on this. I can post a new patch if you want.

If it's not too difficult for you, I feel like it makes more sense to just get rid of them in this patch, then you don't need to do any renaming in the next patch.
Comment on attachment 778061 [details] [diff] [review]
TopSitesView Removal

Review of attachment 778061 [details] [diff] [review]:
-----------------------------------------------------------------

Flipping around to an r+, since this is good to land.
Attachment #778061 - Flags: review- → review+
Removed all PromoBox stuff. Make required changes.
Attachment #778066 - Attachment is obsolete: true
Attachment #778178 - Flags: review?(margaret.leibovic)
Made required changes.
Attachment #778087 - Attachment is obsolete: true
Attachment #778180 - Flags: review?(margaret.leibovic)
Comment on attachment 778178 [details] [diff] [review]
About:home Removal

Looks good, thanks!
Attachment #778178 - Flags: review?(margaret.leibovic) → review+
Attachment #778180 - Flags: review?(margaret.leibovic) → review+
You need to log in before you can comment on or make changes to this bug.