880047 - [fig] Kill unused AboutHome code

Status: RESOLVED FIXED

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

Description

[:Margaret Leibovic]

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.

Comment 1

[Mark Capella [:capella]]

This seems like a good starting point... I like r̶i̶p̶p̶i̶n̶g̶ ̶o̶u̶t̶  simplifying code :-D

Comment 2

[Mark Capella [:capella]]

Attachment: LastTabsSection Removal (v1)

First patch just to remove the LastTabsSection related code...

Comment 3

[Mark Capella [:capella]]

Attachment: AddonsSection Removal (v1)

Removes the Addons Section

Comment 4

[Mark Capella [:capella]]

Attachment: RemoteTabsSection Removal (v1)

Similar to the first two patches, plus removes some <style> entries previously shared by all three

Comment 5

[Mark Capella [:capella]]

Attachment: TopSitesView Removal (v1)

Ouch! This one was a little more involved ... :-D

Comment 6

[Mark Capella [:capella]]

Attachment: AboutHomeSection Removal (v1)

An easy one ...

Comment 7

[Mark Capella [:capella]]

Attachment: PromoBox Removal (v1)

More good stuff...

Comment 8

[: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.

Comment 9

[:Margaret Leibovic]

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 10

[:Margaret Leibovic]

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.

Comment 11

[Mark Capella [:capella]]

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 )

Comment 12

[Mark Capella [:capella]]

Attachment: RemoteTabsSection Removal (v2)

Let's try the first one :D

Comment 13

[: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+.

Comment 14

[Mark Capella [:capella]]

Scooping up the field mice and bonk them on the head !!!
http://hg.mozilla.org/projects/fig/rev/101158e5e302

Comment 15

[:Margaret Leibovic]

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.

Comment 16

[Mark Capella [:capella]]

Attachment: AddonsSection Removal (v2)

Comment 17

[:Margaret Leibovic]

Comment on attachment 777230 [details] [diff] [review]
AddonsSection Removal (v2)

Looking good.

Comment 18

[Mark Capella [:capella]]

https://tbpl.mozilla.org/?tree=Fig&rev=c2cab1e6f266

Comment 19

[Mark Capella [:capella]]

Attachment: LastTabsSection Removal (v2)

Kill more thingz :-P

Comment 20

[:Margaret Leibovic]

Comment on attachment 777767 [details] [diff] [review]
LastTabsSection Removal (v2)

LGTM.

Comment 21

[Mark Capella [:capella]]

forgot to post in comment 19
https://hg.mozilla.org/projects/fig/rev/a4c2fc0828c7

Comment 22

[Mark Capella [:capella]]

This is last tabs section
https://hg.mozilla.org/projects/fig/rev/03ff897ef95d

Comment 23

[Mark Capella [:capella]]

Comment on attachment 777767 [details] [diff] [review]
LastTabsSection Removal (v2)

And then sriram takes over :)

Comment 24

[Sriram Ramasubramanian [:sriram]]

Attachment: TopSitesView Removal

This cleans up TopSitesView.

Comment 25

[Sriram Ramasubramanian [:sriram]]

Attachment: LinkTextView Removal

We don't use this widget anymore. That small little "Link>>". Purged.

Comment 26

[Sriram Ramasubramanian [:sriram]]

Attachment: About:home Removal

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).

Comment 27

[Sriram Ramasubramanian [:sriram]]

Attachment: Rename old abouthome_*

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.

Comment 28

[:Margaret Leibovic]

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.

Comment 29

[Sriram Ramasubramanian [:sriram]]

(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 30

[:Margaret Leibovic]

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.

Comment 31

[Sriram Ramasubramanian [:sriram]]

(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 32

[:Margaret Leibovic]

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.

Comment 33

[Sriram Ramasubramanian [:sriram]]

(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.

Comment 34

[Sriram Ramasubramanian [:sriram]]

The diff might be bigger, but the file size stays the same. ;)

Comment 35

[:Margaret Leibovic]

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 36

[:Margaret Leibovic]

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.

Comment 37

[Sriram Ramasubramanian [:sriram]]

Attachment: About:home Removal

Removed all PromoBox stuff. Make required changes.

Comment 38

[Sriram Ramasubramanian [:sriram]]

Attachment: Rename old abouthome_*

Made required changes.

Comment 39

[:Margaret Leibovic]

Comment on attachment 778178 [details] [diff] [review]
About:home Removal

Looks good, thanks!

Comment 40

[Sriram Ramasubramanian [:sriram]]

https://hg.mozilla.org/projects/fig/rev/13888d1c3b50
https://hg.mozilla.org/projects/fig/rev/c4599977df51
https://hg.mozilla.org/projects/fig/rev/7d2228024886
https://hg.mozilla.org/projects/fig/rev/57d099227525

Comment 41

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/101158e5e302
https://hg.mozilla.org/mozilla-central/rev/c2cab1e6f266
https://hg.mozilla.org/mozilla-central/rev/03ff897ef95d
https://hg.mozilla.org/mozilla-central/rev/13888d1c3b50
https://hg.mozilla.org/mozilla-central/rev/c4599977df51
https://hg.mozilla.org/mozilla-central/rev/7d2228024886
https://hg.mozilla.org/mozilla-central/rev/57d099227525