Closed Bug 906221 Opened 7 years ago Closed 7 years ago

[Fig] AboutHomeTest.updateBookmarks uses incorrect reflection

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 26

People

(Reporter: liuche, Assigned: liuche)

Details

Attachments

(1 file, 1 obsolete file)

Method signature for updateBookmarks is incorrect in AboutHomeTest.
Attached patch Patch: updateBookmarks (obsolete) — Splinter Review
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 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+
Carrying r+ over from previous review.

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