Closed Bug 748583 Opened 12 years ago Closed 12 years ago

Combined view fetches bookmark ids even when bookmark is deleted

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(firefox18 verified, blocking-fennec1.0 -)

VERIFIED FIXED
Firefox 16
Tracking Status
firefox18 --- verified
blocking-fennec1.0 --- -

People

(Reporter: lucasr, Assigned: lucasr)

References

Details

Attachments

(1 file, 2 obsolete files)

Found this bug while testing my patches for bug 701330. The star would still show even after removing the corresponding bookmark.
Attachment #618082 - Flags: review?(margaret.leibovic)
Comment on attachment 618082 [details] [diff] [review]
Fix bookmarks id fetching in the Combined view

I'm just giving an r- because I think we should also add a testcase to cover this in testBrowserProviders's TestCombinedView. We could just do something like delete one of the bookmarks already added in the test, then run the query again and check the bookmark id. 

>diff --git a/mobile/android/base/AwesomeBarTabs.java b/mobile/android/base/AwesomeBarTabs.java

>@@ -172,17 +172,17 @@ public class AwesomeBarTabs extends TabH

>             Long bookmarkId = (Long) historyItem.get(Combined.BOOKMARK_ID);
>-            int visibility = (bookmarkId == 0 ? View.GONE : View.VISIBLE);
>+            int visibility = (bookmarkId > 0 ? View.VISIBLE : View.GONE);

What's the reason for this switch? Just to make things a little safer in case things change such that we get -1 for the id?

>@@ -482,16 +482,19 @@ public class AwesomeBarTabs extends TabH

>+            Log.d("BOOM", "bookmarkId is: " + bookmarkId);
>+            Log.d("BOOM", "bookmarkId is null: " + (bookmarkId == null));
>+            Log.d("BOOM", "bookmarkId is null (c): " + (cursor.getType(cursor.getColumnIndexOrThrow(Combined.BOOKMARK_ID)) == Cursor.FIELD_TYPE_NULL));

Looks like you left some debug logging in this patch :)
Attachment #618082 - Flags: review?(margaret.leibovic) → review-
This should be fixed before the bookmark star is uplifted to Aurora.
Blocks: 701330
blocking-fennec1.0: ? → -
New patch with tests.
Attachment #631407 - Flags: review?(margaret.leibovic)
Attachment #618082 - Attachment is obsolete: true
Blocks: reader
Attachment #631407 - Attachment is obsolete: true
Attachment #631407 - Flags: review?(margaret.leibovic)
Attachment #633563 - Flags: review?(margaret.leibovic)
Comment on attachment 633563 [details] [diff] [review]
Fix bookmark id fetching in the Combined view

These giant copy/pasted SQL concatenations make me scared, but I ran a diff between createCombinedWithImagesViewOn9 and createCombinedWithImagesViewOn10 to verify that only the change we wanted made it in:

<                         " SELECT " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks._ID) + " AS " + Combined.BOOKMARK_ID + ", " +
---
>                         " SELECT " + "CASE " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.IS_DELETED) + " WHEN 0 THEN " +
>                                      qualifyColumn(TABLE_BOOKMARKS, Bookmarks._ID) +  " ELSE NULL END AS " + Combined.BOOKMARK_ID + ", " +

Also, adding a test makes me less afraid :)
Attachment #633563 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/060b4cd23ee2
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
Changes were applied on the latest Nightly. Closing bug as verified fixed on:

Firefox 18.0a1 (2012-09-19)
Device: Galaxy Note
OS: Android 4.0.4
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.