724756 - removeBookmark can remove an arbitrary number of bookmarks

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Richard Newman [:rnewman]]

public void removeBookmark(ContentResolver cr, String uri) {
        cr.delete(appendProfile(Bookmarks.CONTENT_URI),
                  Bookmarks.URL + " = ?",
                  new String[] { uri });
    }

Remove Slashdot from somewhere in your bookmarks menu, and it'll disappear from your toolbar. Not cool. "removeBookmark" should remove 0 or 1 bookmarks.

This should take a bookmark ID, not a URI.

Potential for confusing data loss here.

Comment 1

[Lucas Rocha (:lucasr)]

So, same URL can be bookmarked several times and reside on multiple directories on desktop? 

Right now, once you access a web page, we check if the there's any bookmark entry pointing to that URL and update the main menu accordingly (enable/disable the bookmark menu item). It works like a simple toggle so we don't really know "which" bookmark entry is being considered.

This is different than, say, directly removing a specific bookmark in awesomebar (in which you have some context as to which specific bookmark entry you're removing).

Comment 2

[:Margaret Leibovic]

I'm going to take this from Lucas, but we'll have to figure out the issue in comment 1.

Comment 3

[Richard Newman [:rnewman]]

(In reply to Lucas Rocha (:lucasr) from comment #1)
> So, same URL can be bookmarked several times and reside on multiple
> directories on desktop? 

Correct -- or even multiple times in the same folder, right next to each other. (Try it!)

> Right now, once you access a web page, we check if the there's any bookmark
> entry pointing to that URL and update the main menu accordingly
> (enable/disable the bookmark menu item). It works like a simple toggle so we
> don't really know "which" bookmark entry is being considered.

Which is like the Star on desktop. That behaves like this:

  http://people.mozilla.com/~rnewman/bugs/724756/remove.png

"Remove 2 bookmarks" button when you try to un-star.

> This is different than, say, directly removing a specific bookmark in
> awesomebar (in which you have some context as to which specific bookmark
> entry you're removing).

Or in the bookmark menu.

Comment 4

[Richard Newman [:rnewman]]

I view this as three pieces:

* Ensuring that there's an API that works in terms of identifiers, rather than the URL attribute.

* If we keep it around, rejigging the "by URL" version to have the same hooks as the identifier version: for example, bumping the modified time of each parent folder. (Bug 724745.)

* Deciding how the UI uses the available API to delete one or all bookmarks. I imagine tap-and-hold in the bookmarks UI, versus an equivalent to Star > Remove all.

Comment 5

[Lucas Rocha (:lucasr)]

(In reply to Richard Newman [:rnewman] from comment #4)
> I view this as three pieces:
> 
> * Ensuring that there's an API that works in terms of identifiers, rather
> than the URL attribute.
> 
> * If we keep it around, rejigging the "by URL" version to have the same
> hooks as the identifier version: for example, bumping the modified time of
> each parent folder. (Bug 724745.)
> 
> * Deciding how the UI uses the available API to delete one or all bookmarks.
> I imagine tap-and-hold in the bookmarks UI, versus an equivalent to Star >
> Remove all.

Those are exactly the conclusions we reached after discussing a bit on IRC. The plan is to rename the current removeBookmark to removeBookmarksWithURL and add a new removeBookmark that takes an ID instead of URL. The removeBookmarksWithURL will be used on the star menu item. And the removeBookmark(id) will be used when tapping-and-holding on awesomebar's bookmarks tab.

Comment 6

[Richard Newman [:rnewman]]

(In reply to Lucas Rocha (:lucasr) from comment #5)

> Those are exactly the conclusions we reached after discussing a bit on IRC.
> The plan is to rename the current removeBookmark to removeBookmarksWithURL
> and add a new removeBookmark that takes an ID instead of URL. The
> removeBookmarksWithURL will be used on the star menu item. And the
> removeBookmark(id) will be used when tapping-and-holding on awesomebar's
> bookmarks tab.

\o/

I'd better finish up my parent-timestamp patch, mm? :D

Comment 7

[:Margaret Leibovic]

Attachment: patch

This is hard to test right now because bug 716918 broke the functionality added in bug 721776, but hopefully bug 722413 will fix that soon. (Gah, too many bug numbers!)

Comment 8

[Lucas Rocha (:lucasr)]

Comment on attachment 595477 [details] [diff] [review]
patch

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

::: mobile/android/base/AwesomeBar.java
@@ +472,5 @@
>      public boolean onContextItemSelected(MenuItem item) {
>          if (mContextMenuSubject == null)
>              return false;
>  
> +        Cursor cursor = null;

Maybe it would cleaner if you have something like a Long id = null here and you only assign a value to it when mContextMenuSubject is a cursor.

@@ +499,5 @@
>                  Toast.makeText(this, R.string.new_tab_opened, Toast.LENGTH_SHORT).show();
>                  break;
>              }
>              case R.id.remove_bookmark: {
> +                if (cursor == null) {

Why is this check necessary? What are the cases where cursor is null when removing a bookmark? Replace that with the "id" idea above?

::: mobile/android/base/db/BrowserDB.java
@@ +89,5 @@
>          public void addBookmark(ContentResolver cr, String title, String uri);
>  
> +        public void removeBookmark(ContentResolver cr, int id);
> +
> +        public void removeBookmarkWithURL(ContentResolver cr, String uri);

removeBookmarksWithURL (plural) would be slightly better.

Comment 9

[:Margaret Leibovic]

Attachment: patch

(In reply to Lucas Rocha (:lucasr) from comment #8)

> @@ +499,5 @@
> >                  Toast.makeText(this, R.string.new_tab_opened, Toast.LENGTH_SHORT).show();
> >                  break;
> >              }
> >              case R.id.remove_bookmark: {
> > +                if (cursor == null) {
> 
> Why is this check necessary? What are the cases where cursor is null when
> removing a bookmark? Replace that with the "id" idea above?

I did this as a safety precaution, since a check in onCreateContextMenu is the only thing making sure we don't show R.id.remove_bookmarks for non-bookmark items. I realize now that ensuring that we have a cursor doesn't actually prevent possible error cases - if we have a top sites cursor, the id wouldn't be correct for the bookmark row, so this could cause problems. I think we may want to look into more robust ways to ensure we're not doing bad things like accidentally removing the wrong bookmarks.

I initially tried the "id" idea, but I was getting warnings about the variable potentially not being initialized down below. Because id needs to be final, I can't initialize it then just set the value if mContextMenuSubject is a Cursor. Given these constraints, this is what I came up with - let me know if you can think of a better way to do this.

Comment 10

[Lucas Rocha (:lucasr)]

(In reply to Margaret Leibovic [:margaret] from comment #9)
> Created attachment 595796 [details] [diff] [review]
> patch
> 
> (In reply to Lucas Rocha (:lucasr) from comment #8)
> 
> > @@ +499,5 @@
> > >                  Toast.makeText(this, R.string.new_tab_opened, Toast.LENGTH_SHORT).show();
> > >                  break;
> > >              }
> > >              case R.id.remove_bookmark: {
> > > +                if (cursor == null) {
> > 
> > Why is this check necessary? What are the cases where cursor is null when
> > removing a bookmark? Replace that with the "id" idea above?
> 
> I did this as a safety precaution, since a check in onCreateContextMenu is
> the only thing making sure we don't show R.id.remove_bookmarks for
> non-bookmark items. I realize now that ensuring that we have a cursor
> doesn't actually prevent possible error cases - if we have a top sites
> cursor, the id wouldn't be correct for the bookmark row, so this could cause
> problems. I think we may want to look into more robust ways to ensure we're
> not doing bad things like accidentally removing the wrong bookmarks.

I only asked because I'm always suspicious of null checks :-) They often hide bugs instead of actually fixing them. If something is null when it shouldn't, this is probably a bug somewhere else—the null check will only make things worse.

Comment 11

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #10)

> I only asked because I'm always suspicious of null checks :-) They often
> hide bugs instead of actually fixing them. If something is null when it
> shouldn't, this is probably a bug somewhere else—the null check will only
> make things worse.

True enough!

Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/01827d3f5693

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/01827d3f5693

Comment 13

[Cristian Nicolae (:xti)]

Verified fixed on:

Firefox 13.0a1 (2012-03-02)
20120302031112
http://hg.mozilla.org/mozilla-central/rev/3a7b9e61c263

--
Device: Samsung Galaxy S2
OS: Android 2.3.4