Closed Bug 748583 Opened 9 years ago Closed 9 years ago
Combined view fetches bookmark ids even when bookmark is deleted
Found this bug while testing my patches for bug 701330. The star would still show even after removing the corresponding bookmark.
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.
blocking-fennec1.0: ? → -
New patch with tests.
Attachment #618082 - Attachment is obsolete: true
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+
Status: NEW → RESOLVED
Closed: 9 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
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.