Last Comment Bug 698828 - Adding bookmarks doesn't work (Honeycomb, maybe ICS?)
: Adding bookmarks doesn't work (Honeycomb, maybe ICS?)
Status: VERIFIED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P3 normal (vote)
: ---
Assigned To: Lucas Rocha (:lucasr)
:
Mentors:
: 701925 (view as bug list)
Depends on: 704490
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-01 11:34 PDT by Kartikaya Gupta (email:kats@mozilla.com)
Modified: 2012-01-09 11:45 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
Dump of BOOKMARKS_URI before starring page (7.68 KB, text/plain)
2011-11-01 11:35 PDT, Kartikaya Gupta (email:kats@mozilla.com)
no flags Details
Dump of BOOKMARKS_URI after starring page (7.68 KB, text/plain)
2011-11-01 11:36 PDT, Kartikaya Gupta (email:kats@mozilla.com)
no flags Details
Fix add bookmark operation in Honeycomb/ICS (2.38 KB, patch)
2011-12-05 09:40 PST, Lucas Rocha (:lucasr)
no flags Details | Diff | Review
Fix add bookmark operation in Honeycomb/ICS (3.58 KB, patch)
2011-12-12 04:43 PST, Lucas Rocha (:lucasr)
blassey.bugs: review+
Details | Diff | Review

Description Kartikaya Gupta (email:kats@mozilla.com) 2011-11-01 11:34:52 PDT
1. Go to a page that is not in the bookmarks
2. Bookmark the page by hitting the "star" icon (you may need to tap the menu to make the star icon appear - see bug 698748)
3. Note that the toast notifications indicates the page has been bookmarked, and the star icon is updated to match.
4. Bring up the awesome bar bookmarks list and look for the page you just bookmarked; it is not there. Expected result: the page should be there.
5. Close the awesome bar and reload the page you were on. Note that the "star" icon again indicates an un-bookmarked page. Expected result: the star icon should indicate the page is bookmarked.

The attachments show a dump of the Browser.BOOKMARKS_URI table as obtained via this line of code:
android.database.DatabaseUtils.dumpCursor(getContentResolver().query(android.provider.Browser.BOOKMARKS_URI, null, null, null, null));

The dump from the "before" attachment was taken after I (1) cleared the history and (2) went to google.com, which redirected me to google.ca. The "after" dump was obtained after hitting the star icon at that point.

However, I observed that if I go to the page, THEN clear the history, and THEN star the page, bookmarking works as expected.
Comment 1 Kartikaya Gupta (email:kats@mozilla.com) 2011-11-01 11:35:57 PDT
Created attachment 571077 [details]
Dump of BOOKMARKS_URI before starring page
Comment 2 Kartikaya Gupta (email:kats@mozilla.com) 2011-11-01 11:36:23 PDT
Created attachment 571079 [details]
Dump of BOOKMARKS_URI after starring page
Comment 3 Aaron Train [:aaronmt] 2011-11-04 14:14:40 PDT
Confirming that on my Samsung Galaxy SII (Android 2.3.4), bookmarks added under native Fennec, are not added in the default "Internet" browser under TouchWiz 4.0.
Comment 4 Aaron Train [:aaronmt] 2011-11-04 14:28:25 PDT
My mistake, this looks like a bug in the TouchWiz 4.0 browser; or just an implementation detail. After adding a bookmark in native Fennec successfully, you can access the bookmark by querying it in the browser; albeit for some reason the bookmark is not displayed in their "Thumbnail View" (where I had first looked).
Comment 5 John Hammink 2011-11-14 09:43:41 PST
*** Bug 701925 has been marked as a duplicate of this bug. ***
Comment 6 Lucas Rocha (:lucasr) 2011-11-25 04:30:40 PST
Taking as I'm working on bug 701913 which is directly related.
Comment 7 Lucas Rocha (:lucasr) 2011-12-05 09:40:33 PST
Created attachment 579094 [details] [diff] [review]
Fix add bookmark operation in Honeycomb/ICS

Build on top of changes in bug 704490.
Comment 8 Brad Lassey [:blassey] (use needinfo?) 2011-12-09 22:53:07 PST
Comment on attachment 579094 [details] [diff] [review]
Fix add bookmark operation in Honeycomb/ICS

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

::: mobile/android/base/db/AndroidBrowserDB.java
@@ +183,5 @@
> +        values.put(Browser.BookmarkColumns.BOOKMARK, "1");
> +        values.put(Browser.BookmarkColumns.TITLE, title);
> +        values.put(Browser.BookmarkColumns.URL, uri);
> +
> +        cr.insert(Browser.BOOKMARKS_URI, values);

why don't you have to check that the value exists post 11?
Comment 9 Lucas Rocha (:lucasr) 2011-12-12 04:43:04 PST
(In reply to Brad Lassey [:blassey] from comment #8)
> Comment on attachment 579094 [details] [diff] [review]
> Fix add bookmark operation in Honeycomb/ICS
> 
> Review of attachment 579094 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/db/AndroidBrowserDB.java
> @@ +183,5 @@
> > +        values.put(Browser.BookmarkColumns.BOOKMARK, "1");
> > +        values.put(Browser.BookmarkColumns.TITLE, title);
> > +        values.put(Browser.BookmarkColumns.URL, uri);
> > +
> > +        cr.insert(Browser.BOOKMARKS_URI, values);
> 
> why don't you have to check that the value exists post 11?

Misread Android's source code a bit. Submitting the correct patch now.
Comment 10 Lucas Rocha (:lucasr) 2011-12-12 04:43:48 PST
Created attachment 580875 [details] [diff] [review]
Fix add bookmark operation in Honeycomb/ICS
Comment 11 Lucas Rocha (:lucasr) 2011-12-13 05:47:53 PST
Pushed: http://hg.mozilla.org/mozilla-central/rev/71a7af64da86
Comment 12 Aaron Train [:aaronmt] 2011-12-13 07:00:04 PST
Samsung Galaxy Tab 10.1 (Android 3.1)
20111213061518
http://hg.mozilla.org/mozilla-central/rev/e79b3396889c

Looking to also verify on ICS.

Note You need to log in before you can comment on or make changes to this bug.