906221 - [Fig] AboutHomeTest.updateBookmarks uses incorrect reflection

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chenxia Liu [:liuche]]

Method signature for updateBookmarks is incorrect in AboutHomeTest.

Comment 1

[Chenxia Liu [:liuche]]

Attachment: 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.

Comment 2

[: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?

Comment 3

[Chenxia Liu [:liuche]]

Attachment: Patch: update/insert bookmarks for tests

Carrying r+ over from previous review.

Added documentation, remove updateBookmarks.

Comment 4

[Chenxia Liu [:liuche]]

https://hg.mozilla.org/integration/fx-team/rev/df05f5edf8ca

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/df05f5edf8ca