Closed Bug 752052 Opened 10 years ago Closed 10 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox14 fixed, blocking-fennec1.0 soft)

RESOLVED FIXED
Firefox 15
Tracking Status
firefox14 --- fixed
blocking-fennec1.0 --- soft

People

(Reporter: Margaret, Assigned: Margaret)

Details

Attachments

(1 file)

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.
Attached patch patchSplinter Review
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.
Assignee: nobody → margaret.leibovic
Attachment #621200 - Flags: review?(lucasr.at.mozilla)
Noming as a blocker, since without this fix, users can accidentally mass-edit their synced bookmarks.
blocking-fennec1.0: --- → ?
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.
Attachment #621200 - Flags: review+
(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.
Status: NEW → ASSIGNED
Whiteboard: [has driveby review]
blocking-fennec1.0: ? → soft
(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.
Attachment #621200 - Flags: review?(lucasr.at.mozilla)
https://hg.mozilla.org/integration/mozilla-inbound/rev/764c37699d2a
Whiteboard: [has driveby review] → [landed on inbound]
Target Milestone: --- → Firefox 15
(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.
https://hg.mozilla.org/mozilla-central/rev/764c37699d2a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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
Attachment #621200 - Flags: approval-mozilla-aurora?
Whiteboard: [landed on inbound]
We are leaving all non-beta+ bugs nominated for Aurora approval in the queue until FN14 Beta 1 is signed off on by QA.
Attachment #621200 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.