[fig] Kill unused AboutHome code

RESOLVED FIXED in Firefox 26

Status

()

defect
P1
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Margaret, Assigned: capella)

Tracking

Trunk
Firefox 26
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-fig)

Attachments

(7 attachments, 8 obsolete attachments)

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
Reporter

Description

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

Comment 1

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

Comment 2

6 years ago
Posted patch LastTabsSection Removal (v1) (obsolete) — Splinter Review
First patch just to remove the LastTabsSection related code...
Attachment #766396 - Flags: review?(margaret.leibovic)
Assignee

Comment 3

6 years ago
Posted patch AddonsSection Removal (v1) (obsolete) — Splinter Review
Removes the Addons Section
Attachment #766429 - Flags: review?(margaret.leibovic)
Assignee

Comment 4

6 years ago
Similar to the first two patches, plus removes some <style> entries previously shared by all three
Attachment #766482 - Flags: review?(margaret.leibovic)
Assignee

Comment 5

6 years ago
Posted patch TopSitesView Removal (v1) (obsolete) — Splinter Review
Ouch! This one was a little more involved ... :-D
Attachment #766516 - Flags: review?(margaret.leibovic)
Assignee

Comment 6

6 years ago
Posted patch AboutHomeSection Removal (v1) (obsolete) — Splinter Review
An easy one ...
Attachment #766640 - Flags: review?(margaret.leibovic)
Assignee

Comment 7

6 years ago
Posted patch PromoBox Removal (v1) (obsolete) — Splinter Review
More good stuff...
Attachment #766666 - Flags: review?(margaret.leibovic)
Reporter

Comment 8

6 years ago
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-
Reporter

Comment 9

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

Comment 10

6 years ago
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)
Reporter

Updated

6 years ago
Attachment #766482 - Flags: review?(margaret.leibovic)
Reporter

Updated

6 years ago
Attachment #766516 - Flags: review?(margaret.leibovic)
Reporter

Updated

6 years ago
Attachment #766640 - Flags: review?(margaret.leibovic)
Reporter

Updated

6 years ago
Attachment #766666 - Flags: review?(margaret.leibovic)
Priority: -- → P1
Assignee

Comment 11

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

Comment 12

6 years ago
Let's try the first one :D
Attachment #766482 - Attachment is obsolete: true
Attachment #773860 - Flags: feedback?(margaret.leibovic)
Reporter

Comment 13

6 years ago
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+
Assignee

Comment 14

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

Comment 15

6 years ago
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+
Assignee

Comment 16

6 years ago
Attachment #766429 - Attachment is obsolete: true
Attachment #777230 - Flags: review?(margaret.leibovic)
Reporter

Comment 17

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

Looking good.
Attachment #777230 - Flags: review?(margaret.leibovic) → review+
Assignee

Updated

6 years ago
Attachment #777230 - Flags: checkin+
Assignee

Comment 19

6 years ago
Kill more thingz :-P
Attachment #766396 - Attachment is obsolete: true
Attachment #777767 - Flags: review?(margaret.leibovic)
Reporter

Comment 20

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

LGTM.
Attachment #777767 - Flags: review?(margaret.leibovic) → review+
Assignee

Comment 23

6 years ago
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)
Posted 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)
Posted 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)
Reporter

Updated

6 years ago
Attachment #766516 - Attachment is obsolete: true
Reporter

Updated

6 years ago
Attachment #766640 - Attachment is obsolete: true
Reporter

Updated

6 years ago
Attachment #766666 - Attachment is obsolete: true
Reporter

Comment 28

6 years ago
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-
Reporter

Updated

6 years ago
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.
Reporter

Comment 30

6 years ago
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.
Reporter

Comment 32

6 years ago
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. ;)
Reporter

Comment 35

6 years ago
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.
Reporter

Comment 36

6 years ago
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)
Reporter

Comment 39

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

Looks good, thanks!
Attachment #778178 - Flags: review?(margaret.leibovic) → review+
Reporter

Updated

6 years ago
Attachment #778180 - Flags: review?(margaret.leibovic) → review+
You need to log in before you can comment on or make changes to this bug.