Closed
Bug 880047
Opened 10 years ago
Closed 10 years ago
[fig] Kill unused AboutHome code
Categories
(Firefox for Android Graveyard :: General, defect, P1)
Tracking
(Not tracked)
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.
Assignee | ||
Comment 1•10 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•10 years ago
|
||
First patch just to remove the LastTabsSection related code...
Attachment #766396 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 3•10 years ago
|
||
Removes the Addons Section
Attachment #766429 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 4•10 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•10 years ago
|
||
Ouch! This one was a little more involved ... :-D
Attachment #766516 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 6•10 years ago
|
||
An easy one ...
Attachment #766640 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 7•10 years ago
|
||
More good stuff...
Attachment #766666 -
Flags: review?(margaret.leibovic)
Reporter | ||
Comment 8•10 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•10 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•10 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•10 years ago
|
Attachment #766482 -
Flags: review?(margaret.leibovic)
Reporter | ||
Updated•10 years ago
|
Attachment #766516 -
Flags: review?(margaret.leibovic)
Reporter | ||
Updated•10 years ago
|
Attachment #766640 -
Flags: review?(margaret.leibovic)
Reporter | ||
Updated•10 years ago
|
Attachment #766666 -
Flags: review?(margaret.leibovic)
Updated•10 years ago
|
Priority: -- → P1
Assignee | ||
Comment 11•10 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•10 years ago
|
||
Let's try the first one :D
Attachment #766482 -
Attachment is obsolete: true
Attachment #773860 -
Flags: feedback?(margaret.leibovic)
Reporter | ||
Comment 13•10 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•10 years ago
|
||
Scooping up the field mice and bonk them on the head !!! http://hg.mozilla.org/projects/fig/rev/101158e5e302
Reporter | ||
Comment 15•10 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•10 years ago
|
||
Attachment #766429 -
Attachment is obsolete: true
Attachment #777230 -
Flags: review?(margaret.leibovic)
Reporter | ||
Comment 17•10 years ago
|
||
Comment on attachment 777230 [details] [diff] [review] AddonsSection Removal (v2) Looking good.
Attachment #777230 -
Flags: review?(margaret.leibovic) → review+
Assignee | ||
Comment 18•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Fig&rev=c2cab1e6f266
Assignee | ||
Updated•10 years ago
|
Attachment #777230 -
Flags: checkin+
Assignee | ||
Comment 19•10 years ago
|
||
Kill more thingz :-P
Attachment #766396 -
Attachment is obsolete: true
Attachment #777767 -
Flags: review?(margaret.leibovic)
Reporter | ||
Comment 20•10 years ago
|
||
Comment on attachment 777767 [details] [diff] [review] LastTabsSection Removal (v2) LGTM.
Attachment #777767 -
Flags: review?(margaret.leibovic) → review+
Assignee | ||
Comment 21•10 years ago
|
||
forgot to post in comment 19 https://hg.mozilla.org/projects/fig/rev/a4c2fc0828c7
Assignee | ||
Comment 22•10 years ago
|
||
This is last tabs section https://hg.mozilla.org/projects/fig/rev/03ff897ef95d
Assignee | ||
Comment 23•10 years ago
|
||
Comment on attachment 777767 [details] [diff] [review] LastTabsSection Removal (v2) And then sriram takes over :)
Attachment #777767 -
Flags: checkin+
Comment 24•10 years ago
|
||
This cleans up TopSitesView.
Attachment #778061 -
Flags: review?(margaret.leibovic)
Comment 25•10 years ago
|
||
We don't use this widget anymore. That small little "Link>>". Purged.
Attachment #778063 -
Flags: review?(margaret.leibovic)
Comment 26•10 years ago
|
||
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)
Comment 27•10 years ago
|
||
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•10 years ago
|
Attachment #766516 -
Attachment is obsolete: true
Reporter | ||
Updated•10 years ago
|
Attachment #766640 -
Attachment is obsolete: true
Reporter | ||
Updated•10 years ago
|
Attachment #766666 -
Attachment is obsolete: true
Reporter | ||
Comment 28•10 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•10 years ago
|
Attachment #778063 -
Flags: review?(margaret.leibovic) → review+
Comment 29•10 years ago
|
||
(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•10 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-
Comment 31•10 years ago
|
||
(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•10 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-
Comment 33•10 years ago
|
||
(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•10 years ago
|
||
The diff might be bigger, but the file size stays the same. ;)
Reporter | ||
Comment 35•10 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•10 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+
Comment 37•10 years ago
|
||
Removed all PromoBox stuff. Make required changes.
Attachment #778066 -
Attachment is obsolete: true
Attachment #778178 -
Flags: review?(margaret.leibovic)
Comment 38•10 years ago
|
||
Made required changes.
Attachment #778087 -
Attachment is obsolete: true
Attachment #778180 -
Flags: review?(margaret.leibovic)
Reporter | ||
Comment 39•10 years ago
|
||
Comment on attachment 778178 [details] [diff] [review] About:home Removal Looks good, thanks!
Attachment #778178 -
Flags: review?(margaret.leibovic) → review+
Reporter | ||
Updated•10 years ago
|
Attachment #778180 -
Flags: review?(margaret.leibovic) → review+
Comment 40•10 years ago
|
||
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
Whiteboard: fixed-fig
Comment 41•10 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•