837132 - The 'Remove' action should make thumbnails never show up on about:home again... evar

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

Currently, when one removes "Firefox: Support" and "Firefox: Customize with add-ons", they get re-inserted into the top-site thumbnails on about:home the next time about:home is loaded.

Steps to reproduce:

i) Long-tap and remove "Firefox: Support" and "Firefox: Customize with add-ons"
ii) On the AwesomeScreen, tap "Firefox: About Your Browser"
iii) Tap the back button to go back to about:home

--
Nightly (02/01)
Samsung Galaxy Nexus (Android 4.2)

Comment 1

[Aaron Train [:aaronmt]]

I'm guessing it's because of they are the only two top-sites and they are added when loadTopSitesThumbnails() is called? Shouldn't the choice to remove them "stick"?

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Aaron Train [:aaronmt] from comment #1)
> I'm guessing it's because of they are the only two top-sites and they are
> added when loadTopSitesThumbnails() is called? Shouldn't the choice to
> remove them "stick"?

Not currently. We only remove them from the History DB. When you visit the site, it is put back in History and will show up on about:home.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

They are also bookmarks, which gives them power to re-appear in "top sites" queries.

Comment 4

[Kevin Brosnan [Ex-Mozilla]]

What behavior do we want here?

Comment 5

[Aaron Train [:aaronmt]]

ping?

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Updated the bug title to reflect the intent of this bug. If we do this, do we need a UI to "Unremove"/"Unbanish" pages from the top sites?

Comment 7

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #6)
> Updated the bug title to reflect the intent of this bug. If we do this, do
> we need a UI to "Unremove"/"Unbanish" pages from the top sites?

I think we do. Desktop shows "Undo" and "Restore All" links after getting rid of a new tab thumbnail. Maybe this is a job for... super toast!

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Ian - This will change when we make the new About:Home design. In the short term, should we just remove "Remove" on Aurora and Beta?

Comment 9

[Ian Barlow (:ibarlow)]

That sounds reasonable.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Wes - Can we get a simple backout out patch for Aurora and Beta. I'd be fine with just remving the menu itself and leaving the code.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Heck, might as well back out for Fx23 too.

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

need info for the backout patch

Comment 13

[Wesley Johnston (:wesj)]

Attachment: Patch v1

Removes the "Remove" option entirely.

Comment 14

[:Margaret Leibovic]

Comment on attachment 737721 [details] [diff] [review]
Patch v1

Let's make sure there's a follow up bug on file to fix this the right way.

I was going to remind you to remove the strings that go along with this, but if we can make a fix quickly that we want to uplift, I suppose it's better to reduce the string churn. (Also, I remember there was a post recently about how uplifting string removals also causes churn.)

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to :Margaret Leibovic from comment #14)
> Comment on attachment 737721 [details] [diff] [review]
> Patch v1
> 
> Let's make sure there's a follow up bug on file to fix this the right way.

The new about:home screen only shows "bookmarked" sites in the "Top Sites" thumbnails. This means we don't need any Remove action in the future. The Remove action was supposed to remove "visited" sites from the thumbnails because the sites might be of a private nature, or you just don't want the site shown.

If thumbnails are only bookmarks, the user has much more control over what is shown and what is not, without needing a "never show this on thumbnails" feature.

Comment 16

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #15)
> (In reply to :Margaret Leibovic from comment #14)
> > Comment on attachment 737721 [details] [diff] [review]
> > Patch v1
> > 
> > Let's make sure there's a follow up bug on file to fix this the right way.
> 
> The new about:home screen only shows "bookmarked" sites in the "Top Sites"
> thumbnails. This means we don't need any Remove action in the future. The
> Remove action was supposed to remove "visited" sites from the thumbnails
> because the sites might be of a private nature, or you just don't want the
> site shown.
> 
> If thumbnails are only bookmarks, the user has much more control over what
> is shown and what is not, without needing a "never show this on thumbnails"
> feature.

Sounds good to me. Wes, kill the string when you land this!

Comment 17

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa3760831c60

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/aa3760831c60

Comment 19

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
Build: Firefox for Android 23.0a1( 2013-04-21)
Device: LG Nexus 4
OS: Android 4.2.2

Comment 20

[Aaron Train [:aaronmt]]

Status of follow-ups mentioned?

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Aaron Train [:aaronmt] from comment #20)
> Status of follow-ups mentioned?

As I mention in comment 10, the new behavior will be part of the design for the new Home page. We won't need an explicit bug for this feature. It's part of how the new Home page will work.

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

And by comment 10, I meant comment 15