1062257 - Handle HomeFragment deletions by panel/type instead of universally

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Jan Henning [:JanH]]

Steps to reproduce:
1. Use a mobile device that is synced to a desktop machine.
2. Create a bookmark on the desktop.
3a. Visit the page on mobile so it appears in the history list.
3b. Alternatively, you might have an already existing entry in your top sites list, which has a corresponding bookmark on your desktop machine. Or you might find some other way to make a desktop bookmark show up on the Top Sites page on your mobile device.
4. On the history respectively Top Sites page, delete that entry via the context menu.
5. Make sure that Sync has handled your actions both on the mobile device and on the desktop machine.

What happens:
Not only has the history/Top Sites entry disappeared (as it should) but the corresponding bookmark is gone, too, both on mobile *and* desktop.

What should happen:
Bookmarks should remain unaffected by deleting history or especially Top Sites entries.

I understand this behaviour changed as part of bug 913457, however I'm still not sure whether I'm really happy about it, for two reasons:
- On Desktop, you can prevent a page from showing up in about:newtab (or delete its history entries for that instance, although I'm mainly concerned about the Top Sites page) without affecting its bookmarks, so my expectation is that you could do the same thing on Mobile's Top Sites page, which seems to be equivalent of about:newtab.
- While Sync is very useful (and one of the reasons I started using Firefox on my phone), it has the side effect that my Top Sites page is partly dominated by my desktop browsing behaviour. Now two things come into play:
1. There are some pages on Desktop which I've removed from the about:newtab page, and I'd like to remove them from the Top Sites page as well.
2. There are some pages I frequently use on Desktop (and therefore they get pushed onto the Top Sites page through Sync), but rarely need on my phone, so I'd like to remove them as well.

This new behaviour means I can't delete those entries without removing their bookmarks as well, and because of Sync, including their bookmarks on my desktop machine.

My device:
FF32 on Samsung Galaxy S3 Mine, Android 4.1.2

Comment 1

[Richard Newman [:rnewman]]

> Bookmarks should remain unaffected by deleting history or especially Top Sites entries.

Agree entirely!

Related issue: Bug 771278.

Comment 2

[:Margaret Leibovic]

This was done on purpose in bug 913457, but in triage we decided this was a mistake, and we shouldn't be deleting the bookmark.

Yuan, what do you think?

Lucas or Chenxia, can one of you take this?

Comment 3

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

I agree that bookmarks shouldn't be affected by deleting browsing history or deleting Top Sites.

Deleting a top site is often driven by making space for another site that's more useful. That doesn't indicate the original site doesn't have value any more. 

Deleting history is often for privacy concerns. If the users were concerned about a certain site being remembered by the browser, they wouldn't bookmark it at the first place.

Comment 4

[Lucas Rocha (:lucasr)]

liuche took it (thanks!), clearing NEEDINFO.

Comment 5

[Chenxia Liu [:liuche]]

Attachment: Patch: remove home panel list items by panel

So this turned out to be more complex than I initially thought, partly because we can't just remove bookmark/history/reading list in one code path anymore.

Also, the way the Android FragmentManager handles sending context menu clicks is to iterate through *each* fragment. So we actually need to handle within each fragment whether or not we want to handle the ContextMenu click. This took me a while to figure out because I assumed it was something we were doing...

Anyways, I used flags for RemoveItemByUrl because that way, we can decide to OR them if we want to do multiple removals, and we need some sort of flag to differentiate to RemoveItemByUrl anyways now that the removal behavior is panel-specific.

Comment 6

[:Margaret Leibovic]

Comment on attachment 8486877 [details] [diff] [review]
Patch: remove home panel list items by panel

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

Sigh, this context menu code is always so hard to understand. I think we should really try to avoid extra layers of complexity if we can. I would really like to see if we can just set a "this is what you should remove" flag directly on the context menu info, rather than setting a panel type that gets passed along to a removeItem method implementation.

It would be extra nice if instead of explictly setting what thing to remove, we could just rely on bookmark/history ids for that, but that might not work for reading list items. I also don't want to scope creep this bug to change the details of how we remove items, unless that might make it easier to actually fix this bug.

It seems to me like we could try going back to something closer to what was there before this patch happened:
https://hg.mozilla.org/mozilla-central/rev/41548309c22f

::: mobile/android/base/home/HistoryPanel.java
@@ +96,5 @@
>                  final HomeContextMenuInfo info = new HomeContextMenuInfo(view, position, id);
>                  info.url = cursor.getString(cursor.getColumnIndexOrThrow(Combined.URL));
>                  info.title = cursor.getString(cursor.getColumnIndexOrThrow(Combined.TITLE));
>                  info.historyId = cursor.getInt(cursor.getColumnIndexOrThrow(Combined.HISTORY_ID));
> +                info.panelType = PanelType.HISTORY;

Instead of keeping track of a panelType in the HomeContextMenuInfo, and then checking it in removeItem, could you instead set a flag on the HomeContextMenuInfo that indicates what thing should be removed?

We used to do this by keeping track of whether or not the info had specific ids, like historyId or bookmarkId. For example, in the bookmarks panel, the items won't have a history id, so we shouldn't be able to remove them from history.

@@ +196,5 @@
> +    protected boolean removeItem(Context context, HomeContextMenuInfo info) {
> +        if (info.panelType != PanelType.HISTORY) {
> +            return false;
> +        }
> +        (new RemoveItemByUrlTask(context, info.url, FLAG_TYPE_HISTORY)).execute();

Now that we're not removing the world, we should be able to go back to removing the item by id. That should help only remove the thing the user is trying to remove.

::: mobile/android/base/home/HomeFragment.java
@@ +62,5 @@
>  
> +    // Flags for removal type.
> +    protected static final int FLAG_TYPE_HISTORY = 0x001;
> +    protected static final int FLAG_TYPE_BOOKMARK = 0x010;
> +    protected static final int FLAG_TYPE_READING_LIST = 0x100;

I don't see the need for bit-wise flags here, since it doesn't look like you ever combine any of these.

@@ +332,5 @@
>          load();
>          mIsLoaded = true;
>      }
>  
> +    protected static class RemoveItemByUrlTask extends UIAsyncTask.WithoutParams<Void> {

As I mentioned in a previous comment, can we get rid of RemoveItemByUrlTask and just go back to directly removing items by id?

Comment 7

[Chenxia Liu [:liuche]]

Attachment: Patch: remove home panel list items by type v2

Comment 8

[Chenxia Liu [:liuche]]

> As I mentioned in a previous comment, can we get rid of RemoveItemByUrlTask
> and just go back to directly removing items by id?

The old code actually used Remove*Task as well - we just had 3 of them, for bookmarks, reading list items, and history. Since the callback code is the same, we should just have one Remove*Task that handles an item type that's passed in.

Comment 9

[:Margaret Leibovic]

Comment on attachment 8488463 [details] [diff] [review]
Patch: remove home panel list items by type v2

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

Thanks, I think this is much better. I just have one main review comment about cleaning up the logic in the home_remove handler.

::: mobile/android/base/home/BookmarksPanel.java
@@ +17,5 @@
>  import org.mozilla.gecko.home.HomePager.OnUrlOpenListener;
>  
>  import android.app.Activity;
>  import android.content.Context;
> +import android.content.res.Configuration;

Nit: Unused import.

::: mobile/android/base/home/HistoryPanel.java
@@ +18,5 @@
>  import org.mozilla.gecko.db.BrowserContract.Combined;
>  import org.mozilla.gecko.db.BrowserContract.History;
>  import org.mozilla.gecko.db.BrowserDB;
> +import org.mozilla.gecko.home.HomeContextMenuInfo.ItemType;
> +import org.mozilla.gecko.home.HomeFragment.RemoveItemByUrlTask;

Nit: Remove unused import.

::: mobile/android/base/home/HomeContextMenuInfo.java
@@ +26,5 @@
>      public int readingListItemId = -1;
> +    public ItemType itemType = null;
> +
> +    public static enum ItemType {
> +        BOOKMARKS, HISTORY, READING_LIST

Nit: Add a comment indicating that this is used to determine what to remove when the "Remove" context menu item is selected. Or maybe we should even rename this RemoveItemType? I just want to make sure it's not confusing that top sites items have a HISTORY item type.

::: mobile/android/base/home/HomeFragment.java
@@ +266,1 @@
>                  return true;

Now that we're keeping track of the item type, we could check for that instead of checking hasHistoryId/hasBookmarkId/isInReadingList.

Or actually, since all of these calls are just passing itemType along to the remove task to evaluate, we could just get rid of the if statements.

@@ +396,4 @@
>  
> +                    GeckoEvent e = GeckoEvent.createBroadcastEvent("Reader:Remove", json.toString());
> +                    GeckoAppShell.sendEventToGecko(e);
> +                    break;

Nice, this is really clear :)

::: mobile/android/base/home/RecentTabsPanel.java
@@ +18,5 @@
>  import org.mozilla.gecko.Telemetry;
>  import org.mozilla.gecko.TelemetryContract;
>  import org.mozilla.gecko.db.BrowserContract.CommonColumns;
>  import org.mozilla.gecko.db.BrowserContract.URLColumns;
> +import org.mozilla.gecko.home.HomeContextMenuInfo.ItemType;

Is this import necessary?

Comment 10

[Chenxia Liu [:liuche]]

Attachment: Patch: Remove items by type v3

Carrying over r+, will land when tree opens.

Comment 11

[Chenxia Liu [:liuche]]

https://hg.mozilla.org/integration/fx-team/rev/f6c986420f67

Comment 12

[Chenxia Liu [:liuche]]

I'll let this bake a bit on Nightly before requesting Aurora and Beta uplift.

Comment 13

[Phil Ringnalda (:philor)]

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

Comment 14

[Chenxia Liu [:liuche]]

Comment on attachment 8488909 [details] [diff] [review]
Patch: Remove items by type v3

Approval Request Comment
[Feature/regressing bug #]: bug 913457 changed behavior of "remove"
[User impact if declined]: "Remove" will remove history, bookmark, reading list items (which will then be synced)
[Describe test coverage new/current, TBPL]: Nightly, local aurora testing
[Risks and why]: low, some refactoring to be clearer
[String/UUID change made/needed]: none

Comment 15

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8488909 [details] [diff] [review]
Patch: Remove items by type v3

Aurora+

Bug 913457 shipped in 32. Have users been hitting this enough to warrant uplift to beta 33?

Comment 16

[Chenxia Liu [:liuche]]

Attachment: Beta patch: Handle remove behavior per panel

I think we should uplift as well, actually. Carrying over r+.

Approval Request Comment
[Feature/regressing bug #]: bug 913457 made "remove" behavior universal
[User impact if declined]: removing item from one panel will remove it from all panels (and all synced devices)
[Describe test coverage new/current, TBPL]: local, nightly baking
[Risks and why]: low, refactoring for cleaner code
[String/UUID change made/needed]: none

Aurora try: https://tbpl.mozilla.org/?tree=Try&rev=f92a119c7f26
Beta try: https://tbpl.mozilla.org/?tree=Try&rev=b1e0eda7d62c

Comment 17

[Ryan VanderMeulen (PTO, back 6-April)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/32c3c24b52d2

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8492263 [details] [diff] [review]
Beta patch: Handle remove behavior per panel

Taking it for beta 6

Comment 19

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

(In reply to Lawrence Mandel [:lmandel] from comment #15)
> Comment on attachment 8488909 [details] [diff] [review]
> Patch: Remove items by type v3
> 
> Aurora+
> 
> Bug 913457 shipped in 32. Have users been hitting this enough to warrant
> uplift to beta 33?

Turns out that the context menu is one of our most heavily used actions. Thanks for taking this in Beta 6.

Comment 20

[Ryan VanderMeulen (PTO, back 6-April)]

https://hg.mozilla.org/releases/mozilla-beta/rev/ae87b325401d

Comment 22

[Cristina Madaras, QA [:CristinaM]]

Verified as fixed on
Builds:
Firefox for Android 33
Firefox for Android 34 Beta 1
Firefox for Android 35.0a2 (2014-10-15)

Device: Asus Transformer Pad TF300T (Android 4.2.1)