1234967 - Open and delete bookmarks by ID instead of by URL when possible.

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: Awesomescreen, defect, P5)

Description

[Andrzej Hunt :ahunt]

1. Create multiple bookmarks with the same URL, ideally with names 1, 2, 3, and in the same bookmark folder
(You'll need to have sync set up, and create the duplicate the bookmarks on desktop - mobile assumes we have just one bookmark per URL currently)
2. Find the relevant folder on mobile
3. For each bookmark:
 3.1 Long press the bookmark
 3.2 Select "Edit"
 3.3 Verify whether the title shown matches the bookmark's title.

The same title will be shown each time, e.g. "1" even when editing bookmark 2 or 3.

This is because we're only passing the URL and not the ID to the bookmark dialog [1], we should instead always use the ID since that should be unique (whereas the URL does not have to be unique).

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/EditBookmarkDialog.java#143

Comment 1

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

(In reply to Andrzej Hunt :ahunt from comment #0)

> This is because we're only passing the URL and not the ID to the bookmark
> dialog [1], we should instead always use the ID since that should be unique
> (whereas the URL does not have to be unique).

We should be able to pass the ID when editing from a list of bookmarks. The dialog also has to handle the case of editing from the "star", where we only have a URL. We need to be able to handle both cases.

Comment 2

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1234967 - Pre: pass entire HomeContextMenuInfo to RemoveItemByUrlTask r?rnewman

We'll need the bookmark id to delete the correct bookmark, it makes
sense just to pass the full info in instead of passing 3 separate variables.

Review commit: https://reviewboard.mozilla.org/r/43047/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43047/

Comment 3

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1234967 - Pre: implement deleting bookmarks by ID r?rnewman

Review commit: https://reviewboard.mozilla.org/r/43049/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43049/

Comment 4

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1234967 - Pre: implement getBookmarkForID r?rnewman

Review commit: https://reviewboard.mozilla.org/r/43051/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43051/

Comment 5

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1234967 - Delete only desired bookmark from the Bookmarks context menu r?rnewman

Previously we would delete all bookmarks with a given URL. It is possible
to have multiple bookmarks for a given URL - this is most likely to happen
when manually manipulating bookmarks on desktop (mobile doesn't allow
creating more than one bookmark per URL per folder, and doesn't allow
moving bookmarks, hence this can only happen with sync enabled).

Review commit: https://reviewboard.mozilla.org/r/43053/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43053/

Comment 6

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1234967 - Open correct bookmark in the Edit Bookmark dialog r?rnewman

Bookmark URLs are not unique, we should be using the ID whenever possible.

Review commit: https://reviewboard.mozilla.org/r/43055/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43055/

Comment 7

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1234967 - Post: Explain in which circumstances URLS can uniquely identify a bookmark r?rnewman

Review commit: https://reviewboard.mozilla.org/r/43057/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43057/

Comment 8

[Richard Newman [:rnewman]]

Let me know when you want me to look at this.

Comment 9

[Andrzej Hunt :ahunt]

Comment on attachment 8736032 [details]
MozReview Request: Bug 1234967 - Pre: pass entire HomeContextMenuInfo to RemoveItemByUrlTask r?rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43047/diff/1-2/

Comment 10

[Andrzej Hunt :ahunt]

Comment on attachment 8736033 [details]
MozReview Request: Bug 1234967 - Pre: implement deleting bookmarks by ID r?rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43049/diff/1-2/

Comment 11

[Andrzej Hunt :ahunt]

Comment on attachment 8736034 [details]
MozReview Request: Bug 1234967 - Pre: implement getBookmarkForID r?rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43051/diff/1-2/

Comment 12

[Andrzej Hunt :ahunt]

Comment on attachment 8736035 [details]
MozReview Request: Bug 1234967 - Delete only desired bookmark from the Bookmarks context menu r?rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43053/diff/1-2/

Comment 13

[Andrzej Hunt :ahunt]

Comment on attachment 8736036 [details]
MozReview Request: Bug 1234967 - Open correct bookmark in the Edit Bookmark dialog r?rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43055/diff/1-2/

Comment 14

[Andrzej Hunt :ahunt]

Comment on attachment 8736037 [details]
MozReview Request: Bug 1234967 - Post: Explain in which circumstances URLS can uniquely identify a bookmark r?rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43057/diff/1-2/

Comment 15

[Richard Newman [:rnewman]]

Comment on attachment 8736032 [details]
MozReview Request: Bug 1234967 - Pre: pass entire HomeContextMenuInfo to RemoveItemByUrlTask r?rnewman

https://reviewboard.mozilla.org/r/43047/#review41097

::: mobile/android/base/java/org/mozilla/gecko/home/HomeFragment.java:370
(Diff revision 2)
>  
>          load();
>          mIsLoaded = true;
>      }
>  
>      protected static class RemoveItemByUrlTask extends UIAsyncTask.WithoutParams<Void> {

Rename to `RemoveItemTask`.

Comment 16

[Richard Newman [:rnewman]]

Comment on attachment 8736034 [details]
MozReview Request: Bug 1234967 - Pre: implement getBookmarkForID r?rnewman

https://reviewboard.mozilla.org/r/43051/#review41099

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDB.java:112
(Diff revision 2)
>  
>      public abstract String getUrlForKeyword(ContentResolver cr, String keyword);
>  
>      public abstract boolean isBookmark(ContentResolver cr, String uri);
>      public abstract boolean addBookmark(ContentResolver cr, String title, String uri);
> +    public abstract Cursor getBookmarkForID(ContentResolver cr, int id);

Comment that this returns `null` if the bookmark isn't found, or an open cursor otherwise.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDB.java:113
(Diff revision 2)
>      public abstract String getUrlForKeyword(ContentResolver cr, String keyword);
>  
>      public abstract boolean isBookmark(ContentResolver cr, String uri);
>      public abstract boolean addBookmark(ContentResolver cr, String title, String uri);
> +    public abstract Cursor getBookmarkForID(ContentResolver cr, int id);
> +    /**

Nit: newline before comment block begins.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDB.java:115
(Diff revision 2)
>      public abstract boolean isBookmark(ContentResolver cr, String uri);
>      public abstract boolean addBookmark(ContentResolver cr, String title, String uri);
> +    public abstract Cursor getBookmarkForID(ContentResolver cr, int id);
> +    /**
> +     * Retrieve a (random) bookmark for a given URL. If there are multiple bookmarks with the same
> +     * URL there is no guarantee as to what bookmark will be returned.

s/what/which

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1597
(Diff revision 2)
>      }
>  
> +
> +    @Override
> +    @RobocopTarget
> +    public Cursor getBookmarkForID(ContentResolver cr, int id) {

Why did you settle on numeric IDs instead of GUIDs?

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1598
(Diff revision 2)
>  
> +
> +    @Override
> +    @RobocopTarget
> +    public Cursor getBookmarkForID(ContentResolver cr, int id) {
> +        Cursor c = cr.query(bookmarksUriWithLimit(1),

`final`

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1603
(Diff revision 2)
> +        Cursor c = cr.query(bookmarksUriWithLimit(1),
> +                new String[] { Bookmarks._ID,
> +                        Bookmarks.URL,
> +                        Bookmarks.TITLE,
> +                        Bookmarks.KEYWORD },
> +                Bookmarks._ID + " = ?",

With numeric IDs you can concatenate this directly:

```
   Bookmarks._ID + " = " + id;
```

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1607
(Diff revision 2)
> +                        Bookmarks.KEYWORD },
> +                Bookmarks._ID + " = ?",
> +                new String[] { String.valueOf(id) },
> +                null);
> +
> +        if (c != null && c.getCount() == 0) {

Clearer:

```
if (c == null) {
    return null;
}

// Non-empty.
if (c.moveToFirst()) {
    return c;
}

c.close();
return null;
```

Comment 17

[Richard Newman [:rnewman]]

Comment on attachment 8736035 [details]
MozReview Request: Bug 1234967 - Delete only desired bookmark from the Bookmarks context menu r?rnewman

https://reviewboard.mozilla.org/r/43053/#review41103

::: mobile/android/base/java/org/mozilla/gecko/home/HomeFragment.java:403
(Diff revision 2)
>              }
>  
>              switch(mInfo.itemType) {
>                  case BOOKMARKS:
>                      Telemetry.sendUIEvent(TelemetryContract.Event.UNSAVE, TelemetryContract.Method.CONTEXT_MENU, "bookmark");
> -                    mDB.removeBookmarksWithURL(cr, mInfo.url);
> +                    mDB.removeBookmarkWithID(cr, mInfo.bookmarkId);

`bookmarkID`

Comment 18

[Richard Newman [:rnewman]]

Comment on attachment 8736033 [details]
MozReview Request: Bug 1234967 - Pre: implement deleting bookmarks by ID r?rnewman

https://reviewboard.mozilla.org/r/43049/#review41105

Comment 19

[Richard Newman [:rnewman]]

Comment on attachment 8736036 [details]
MozReview Request: Bug 1234967 - Open correct bookmark in the Edit Bookmark dialog r?rnewman

https://reviewboard.mozilla.org/r/43055/#review41107

302 to a frontend dev (Michael? Chenxia?)

Comment 20

[Richard Newman [:rnewman]]

Comment on attachment 8736037 [details]
MozReview Request: Bug 1234967 - Post: Explain in which circumstances URLS can uniquely identify a bookmark r?rnewman

https://reviewboard.mozilla.org/r/43057/#review41109

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:1212
(Diff revision 2)
>                      Telemetry.sendUIEvent(TelemetryContract.Event.ACTION,
>                          TelemetryContract.Method.DIALOG, extrasId);
>  
> +                    // In this case we're allowed to use the bookmark URL (as opposed to id). We know the
> +                    // URL uniquely identifies the bookmark as it's just been created (the UI only lets us create
> +                    // a new bookmark if a given URL isn't bookmarked anywhere).

Urgh. Can we do better than this?

Comment 21

[Tom Klein]

Attachment: Bug 1234967 - Pre: pass entire HomeContextMenuInfo to RemoveItemByUrlTask

We'll need the bookmark ID to delete the correct bookmark. Rather than passing
3 separate variables we might as well just pass in the full site info.

Rebased, with review comments addressed, by Tom Klein.

Review commit: https://reviewboard.mozilla.org/r/98978/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/98978/

Comment 22

[Tom Klein]

Attachment: Bug 1234967 - Pre: implement deleting bookmarks by ID

Rebased by Tom Klein.

Review commit: https://reviewboard.mozilla.org/r/98980/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/98980/

Comment 23

[Tom Klein]

Attachment: Bug 1234967 - Pre: implement getBookmarkForID

Rebased, with review comments addressed, by Tom Klein.

Review commit: https://reviewboard.mozilla.org/r/98982/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/98982/

Comment 24

[Tom Klein]

Attachment: Bug 1234967 - Delete only desired bookmark from the Bookmarks context menu

Previously we would delete all bookmarks with a given URL. It is possible
to have multiple bookmarks for any given URL - this is most likely to happen
when manually manipulating bookmarks on desktop (mobile doesn't allow
creating more than one bookmark per URL per folder, and doesn't allow
moving bookmarks, hence this can only happen with sync enabled).

Rebased by Tom Klein.

Review commit: https://reviewboard.mozilla.org/r/98984/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/98984/

Comment 25

[Tom Klein]

Attachment: Bug 1234967 - Open correct bookmark in the Edit Bookmark dialog

Bookmark URLs are not unique, we should be using the ID whenever possible.

Note: we'll be cleaning up this dialog in Bug 1232439, we can restrict ourselves
to just adding the id/URL differentiation for now.

Rebased by Tom Klein.

Review commit: https://reviewboard.mozilla.org/r/98986/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/98986/

Comment 26

[Tom Klein]

Attachment: Bug 1234967 - Post: Explain in which circumstances URLS can uniquely identify a bookmark

Rebased by Tom Klein.

Review commit: https://reviewboard.mozilla.org/r/99010/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/99010/

Comment 27

[Tom Klein]

These are all just Andrzej's commits rebased and with Richard's review comments addressed.  I'm not sure how to proceed with review - could you advise, Andrzej?

I can't respond to Richard's comments directly in reviewboard, so I'll just add some notes here:

(In reply to Richard Newman [:rnewman] from comment #16)
> ::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1597
> (Diff revision 2)
> >      }
> >  
> > +
> > +    @Override
> > +    @RobocopTarget
> > +    public Cursor getBookmarkForID(ContentResolver cr, int id) {
> 
> Why did you settle on numeric IDs instead of GUIDs?

One answer is that HomeContextMenuInfo, which provides the ID which is used in the (currently) sole path that leads to a call of getBookmarkForID, needs ID for itself, not GUID, in its hasPartnerBookmark method:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/home/HomeContextMenuInfo.java#48

(In reply to Richard Newman [:rnewman] from comment #17)
> ::: mobile/android/base/java/org/mozilla/gecko/home/HomeFragment.java:403
> (Diff revision 2)
> >              }
> >  
> >              switch(mInfo.itemType) {
> >                  case BOOKMARKS:
> >                      Telemetry.sendUIEvent(TelemetryContract.Event.UNSAVE, TelemetryContract.Method.CONTEXT_MENU, "bookmark");
> > -                    mDB.removeBookmarksWithURL(cr, mInfo.url);
> > +                    mDB.removeBookmarkWithID(cr, mInfo.bookmarkId);
> 
> `bookmarkID`

bookmarkId is a public member of HomeContextMenuInfo - could we change that in a followup?  Maybe a good first bug?

(In reply to Richard Newman [:rnewman] from comment #20)
> ::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:1212
> (Diff revision 2)
> >                      Telemetry.sendUIEvent(TelemetryContract.Event.ACTION,
> >                          TelemetryContract.Method.DIALOG, extrasId);
> >  
> > +                    // In this case we're allowed to use the bookmark URL (as opposed to id). We know the
> > +                    // URL uniquely identifies the bookmark as it's just been created (the UI only lets us create
> > +                    // a new bookmark if a given URL isn't bookmarked anywhere).
> 
> Urgh. Can we do better than this?

I think the short answer is no - there's currently no mechanism for passing data from where the bookmark is created to where the EditBookmark dialog is shown (the path is through Tabs.notifyListeners).

Comment 28

[Tom Klein]

(In reply to Tom Klein from comment #27)
> I think the short answer is no - there's currently no mechanism for passing
> data from where the bookmark is created to where the EditBookmark dialog is
> shown (the path is through Tabs.notifyListeners).

A derp, that's completely wrong.  Sorry about that.

I think there are still reasons to not make the effort, at least not yet.
a) As the comment added in the patch states, this path only runs in the case where we've created a bookmark from the star, and the star only allows creation of a bookmark for a given url if a bookmark doesn't already exist.

b) Adding a bookmark runs an update query in case the bookmark already exists in the given folder (which seems a little bit weird to me, given that it's perfectly legal to have multiple bookmarks in the same folder with a given url, but I guess we're focusing on avoiding unintended duplicates), and the update can update multiple rows, so if we're going to accept that addBookmark needs to do an update then we can't expect addBookmark to return a single new id of the added bookmark (currently it just returns a bool for "already existed" or not).  At best (it's seeming to me) we could run a separate followup query after the add to get the ids of all the bookmarks with the given url in the "Mobile Bookmarks" folder (where the star adds bookmarks); if there was just one we could pass it on to EditBookmark, but for now if there was more than one we'd need to just pick one of them at (semi) random, which, granted, is what we do now (before this ticket!) anyway.  But it doesn't seem worth the effort (to me at least), at least until we can handle the multiple bookmarks with a given url case more cleanly, and maybe not even then given comment a) (but see my next comment!).

Comment 29

[Tom Klein]

I think there may actually be one case where the bookmark star can lie (though I haven't tested yet):

If you open an unbookmarked page, and then open the menu with the star in it, and then leave the menu open while sync adds a bookmark for that page in the background, does sync update the star?  I'm guessing no.  So sync could add one or more bookmarks for that url in the Mobile Bookmarks folder, and then you could click on the star to add a bookmark for that url and one of the random sync bookmarks would get its data updated.

Comment 30

[Tom Klein]

(In reply to Tom Klein from comment #29)
> one of the random sync bookmarks would get its data updated.

Make that "all of the sync bookmarks in the Mobile Bookmarks folder would get their data updated".

Comment 31

[Tom Klein]

(In reply to Tom Klein from comment #30)
> (In reply to Tom Klein from comment #29)
> > one of the random sync bookmarks would get its data updated.
> 
> Make that "all of the sync bookmarks in the Mobile Bookmarks folder would
> get their data updated".

Actually before we add a bookmark we check if it already exists; so ^ would only happen if sync adds its bookmark(s) between the time we check to see if the bookmark we're starring already exists and the time we actually perform the update/add transaction. (Apologies for the thinking out loud/learning as I go going on here...)

Comment 32

[Firefox Bug Husbandry Bot]

Re-triaging per https://bugzilla.mozilla.org/show_bug.cgi?id=1473195

Needinfo :susheel if you think this bug should be re-triaged.

Comment 33

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.