752052 - updateBookmark updates all bookmarks with the same URL, not a specific bookmark

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/LocalBrowserDB.java#504

We end up calling this when the user edits a bookmark through the edit bookmark context menu item. We should really just be editing the specific bookmark the user chose to edit. To do this, we'll need to pass the bookmark id around instead of just the url.

Comment 1

[:Margaret Leibovic]

Attachment: patch

This was a pretty straightforward fix. I decided to just get rid of the AndroidBrowserDB implementation, rather than update it, since that's all going away eventually (soon?) anyway.

Comment 2

[:Margaret Leibovic]

Noming as a blocker, since without this fix, users can accidentally mass-edit their synced bookmarks.

Comment 3

[Richard Newman [:rnewman]]

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

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

Looks sane to me. Lucas has a bank holiday today, so thought I'd drive-by :)

::: mobile/android/base/db/AndroidBrowserDB.java
@@ +267,5 @@
>              addBookmarkPre11(cr, title, uri);
>      }
>  
> +    public void updateBookmark(ContentResolver cr, int id, String uri, String title, String keyword) {
> +        // Not implemented

Throw here.

Comment 4

[Richard Newman [:rnewman]]

(In reply to Margaret Leibovic [:margaret] from comment #1)

> This was a pretty straightforward fix. I decided to just get rid of the
> AndroidBrowserDB implementation, rather than update it, since that's all
> going away eventually (soon?) anyway.

I thought we'd already deleted it!

Ideally this would come with a test, but I'd be happy for that to be a follow-up.

Comment 5

[:Margaret Leibovic]

(In reply to Richard Newman [:rnewman] from comment #3)

> Looks sane to me. Lucas has a bank holiday today, so thought I'd drive-by :)

I wasn't aware of that, and appreciate the review! :)

> ::: mobile/android/base/db/AndroidBrowserDB.java
> @@ +267,5 @@
> >              addBookmarkPre11(cr, title, uri);
> >      }
> >  
> > +    public void updateBookmark(ContentResolver cr, int id, String uri, String title, String keyword) {
> > +        // Not implemented
> 
> Throw here.

I don't think it's worth it, since this code never even gets used ever, it's just here so that the app will compile (we already do this |// not implemented| in multiple other methods in AndroidBrowserDB).

(In reply to Richard Newman [:rnewman] from comment #4)
> (In reply to Margaret Leibovic [:margaret] from comment #1)
> 
> > This was a pretty straightforward fix. I decided to just get rid of the
> > AndroidBrowserDB implementation, rather than update it, since that's all
> > going away eventually (soon?) anyway.
> 
> I thought we'd already deleted it!

See bug 723623. Seems like it got held up in discussion about the patch.

> Ideally this would come with a test, but I'd be happy for that to be a
> follow-up.

Sigh, we don't have LocalBrowserDB tests, just BrowserProvider tests. We need to get on that. I want to look into alternate ways to write unit tests, other than Robocop, since that's not well suited for non-UI testing.

Comment 6

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/764c37699d2a

Comment 7

[Richard Newman [:rnewman]]

(In reply to Margaret Leibovic [:margaret] from comment #5)

> I don't think it's worth it, since this code never even gets used ever, it's
> just here so that the app will compile (we already do this |// not
> implemented| in multiple other methods in AndroidBrowserDB).

Fair enough!

> > Ideally this would come with a test, but I'd be happy for that to be a
> > follow-up.
> 
> Sigh, we don't have LocalBrowserDB tests, just BrowserProvider tests. We
> need to get on that. I want to look into alternate ways to write unit tests,
> other than Robocop, since that's not well suited for non-UI testing.

One of my goals for later this year is to get our on-device JUnit 3 tests running in Tinderbox. In the mean time we're tracking integration of those into ci.mozilla.org, so at least we'll have some automated test coverage there.

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/764c37699d2a

Comment 9

[:Margaret Leibovic]

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

[Approval Request Comment]
Regression caused by (bug #): n/a
User impact if declined: editing a bookmark will edit all bookmarks with that same url, which can cause unexpected edits to bookmarks
Testing completed (on m-c, etc.): landed on m-c
Risk to taking this patch (and alternatives if risky): mobile-only, relatively self-contained change, only affects editing bookmarks from Fennec (no sync code)
String changes made by this patch: n/a

Comment 10

[Alex Keybl [:akeybl]]

We are leaving all non-beta+ bugs nominated for Aurora approval in the queue until FN14 Beta 1 is signed off on by QA.

Comment 11

[:Margaret Leibovic]

https://hg.mozilla.org/releases/mozilla-aurora/rev/80ee58f5f6bf