[Fig] AboutHomeTest.updateBookmarks uses incorrect reflection

RESOLVED FIXED in Firefox 26

Status

()

Firefox for Android
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: liuche, Assigned: liuche)

Tracking

unspecified
Firefox 26
ARM
Android
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Method signature for updateBookmarks is incorrect in AboutHomeTest.
Created attachment 791569 [details] [diff] [review]
Patch: updateBookmarks

Since updateBookmark takes an id but addBookmark does not, it doesn't really make sense to combine the two, so I just removed the addOrUpdateBookmark method. It should also be possible to add two bookmarks with the same url, so the isBookmark check doesn't really make sense. Additionally, the only uses of addOrUpdateMobileBookmark were for adding, so I just switched those over.
Assignee: nobody → liuche
Status: NEW → ASSIGNED
Attachment #791569 - Flags: review?(margaret.leibovic)

Comment 2

5 years ago
Comment on attachment 791569 [details] [diff] [review]
Patch: updateBookmarks

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

Good catch. The LocalBrowserDB addBookmark implementation actually does update or insert for us:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/LocalBrowserDB.java#632

So this addOrUpdateMobileBookmark method wasn't even necessary.

::: mobile/android/base/tests/AboutHomeTest.java.in
@@ +77,1 @@
>      protected void addMobileBookmark(String title, String url) {

Can you add some documentation here? You could mention that the LocalBrowserDB.addBookmark implementation will update an existing bookmark with that URL if it already exists.

@@ +86,5 @@
>              mAsserter.ok(false, "Exception adding bookmark: ", e.toString());
>          }
>      }
>  
> +    protected void updateBookmark(int id, String url, String title, String keyword) {

This isn't used anymore. Can we get rid of it?
Attachment #791569 - Flags: review?(margaret.leibovic) → review+
Created attachment 792945 [details] [diff] [review]
Patch: update/insert bookmarks for tests

Carrying r+ over from previous review.

Added documentation, remove updateBookmarks.
Attachment #791569 - Attachment is obsolete: true
Attachment #792945 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/df05f5edf8ca
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.