Last Comment Bug 708485 - Add Special Folders with Sync guids to Bookmarks Store
: Add Special Folders with Sync guids to Bookmarks Store
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: P3 major (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks: 704490
  Show dependency treegraph
 
Reported: 2011-12-07 17:27 PST by Jason Voll [:jvoll]
Modified: 2012-01-09 14:44 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
Create views for bookmarks and history adding image columns (17.01 KB, patch)
2011-12-12 16:46 PST, Lucas Rocha (:lucasr)
no flags Details | Diff | Splinter Review
(1/2) Create "mobile" special bookmarks folder on DB creation (3.30 KB, patch)
2011-12-12 16:49 PST, Lucas Rocha (:lucasr)
blassey.bugs: review+
rnewman: feedback+
Details | Diff | Splinter Review
(2/2) Add Fennec bookmarks to special "mobile" folder (1.88 KB, patch)
2011-12-12 16:51 PST, Lucas Rocha (:lucasr)
blassey.bugs: review-
Details | Diff | Splinter Review
(2/2) Add Fennec bookmarks to special "mobile" folder (1.88 KB, patch)
2011-12-13 03:57 PST, Lucas Rocha (:lucasr)
no flags Details | Diff | Splinter Review
(2/2) Add Fennec bookmarks to special "mobile" folder (2.81 KB, patch)
2011-12-13 03:59 PST, Lucas Rocha (:lucasr)
blassey.bugs: review+
Details | Diff | Splinter Review

Description Jason Voll [:jvoll] 2011-12-07 17:27:04 PST
We need to be able to map the following "special" guids to some appropriate place in the local bookmarks store:
["menu", "places", "tags", "toolbar", "unfiled", "mobile"]

Assuming we want to mirror the existing mobile browser, I think this schema will work:
-Folder with the guid "mobile" is the root folder for all the bookmarks in Fennec
--Folder called "Desktop Bookmarks", with guid of "places"
---Folder called "Unsorted Bookmarks" with guid "unfiled"
---Folder called "Bookmarks Menu" with guid "menu"
---Folder called "Bookmarks Toolbar" with guid "toolbar"

These folders need to be created and always exist in the store in order for sync to work.
Comment 1 Jason Voll [:jvoll] 2011-12-07 17:29:50 PST
I'm having trouble ensuring that the guid of "places" is what we currently do for the folder "Desktop Bookmarks". Can somebody verify this for me or point me to where I might find that out? We need this to be the same schema as the existing browser.
Comment 2 Richard Newman [:rnewman] 2011-12-07 17:38:02 PST
To add a little flavor: Sync processes records with incoming folder guids that are in that set. For example, a bookmark that a user created with XUL Fennec might have a folder of "mobile", corresponding to the Mobile Bookmarks folder in Places. An item in the desktop Bookmarks menu would have the folder guid "menu".

We need to have a place to put those bookmarks.

XUL Fennec would show Mobile Bookmarks as the root, with a "Desktop Bookmarks" folder corresponding to the actual Places root. Ideally Native Fennec will do something similar. Even if it doesn't, we need the data model to be similar enough to round-trip.

On desktop, we'd keep a Mobile Bookmarks folder in sync by translating the guid from a local real guid into one of these special pseudo-guids.

The simplest way to achieve this goal is for Fennec to keep a predefined set of folders, corresponding to those guids. They don't necessarily need to *have* those guids, so long as we have an equivalent way to ask "OK, so what record corresponds to Mobile Bookmarks?". In desktop Firefox, that's provided by PlacesUtils. Those folders should exist so long as the profile does.

We are able to invoke whatever logic you need prior to starting to query, if you can't pre-create these.

I think we've talked this through before at various times, but now we're in the crunch of actually needing it. Sorry for not giving you more warning!
Comment 3 Jason Voll [:jvoll] 2011-12-11 12:30:38 PST
So I've done some work in this area and have it setup so that we add these special folders instead of you guys. The logic for this is that this way before a sync I can verify that the folders exist and more importantly, if a user doesn't have sync setup they probably don't want folders showing up in their bookmarks labelled "Desktop Bookmarks", "Bookmarks Toolbar", "Bookmarks Menu", etc.

That being said, there is still one thing I do need done on your side relating to this: the root folder in which you store bookmarks must at all times have the special guid of "mobile". If you do that for me, then there shouldn't need to be any additional work in terms of special guids/folders.
Comment 4 Lucas Rocha (:lucasr) 2011-12-12 16:46:08 PST
Created attachment 581099 [details] [diff] [review]
Create views for bookmarks and history adding image columns

Creates enhanced views for bookmarks and history tables that include all columns from original table plus the image columns (favicon and thumbnail). This simplifies a bunch of parts of the code that required explicit casting of columns with table names. The view are only used in queries with a projection that includes image columns. Use the table directly otherwise.
Comment 5 Lucas Rocha (:lucasr) 2011-12-12 16:49:29 PST
Created attachment 581101 [details] [diff] [review]
(1/2) Create "mobile" special bookmarks folder on DB creation
Comment 6 Lucas Rocha (:lucasr) 2011-12-12 16:50:18 PST
Comment on attachment 581099 [details] [diff] [review]
Create views for bookmarks and history adding image columns

Sorry, attached to wrong bug.
Comment 7 Lucas Rocha (:lucasr) 2011-12-12 16:51:15 PST
Created attachment 581102 [details] [diff] [review]
(2/2) Add Fennec bookmarks to special "mobile" folder
Comment 8 Brad Lassey [:blassey] (use needinfo?) 2011-12-12 22:22:12 PST
Comment on attachment 581102 [details] [diff] [review]
(2/2) Add Fennec bookmarks to special "mobile" folder

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

::: mobile/android/base/db/LocalBrowserDB.java
@@ +232,5 @@
>  
>          return (count == 1);
>      }
>  
> +    private long getMobileBookmarksFolderId(ContentResolver cr) {

calling this every time looks unnecessarily expensive, we should cache this value.

@@ +238,5 @@
> +
> +        try {
> +            c = cr.query(appendProfile(Bookmarks.CONTENT_URI),
> +                         new String[] { Bookmarks._ID },
> +                         Images.GUID + " = ?",

why Images.GUID? I understand it should be the same value, but shouldn't it be Bookmarks.GUID?
Comment 9 Lucas Rocha (:lucasr) 2011-12-13 03:57:43 PST
Created attachment 581223 [details] [diff] [review]
(2/2) Add Fennec bookmarks to special "mobile" folder

(In reply to Brad Lassey [:blassey] from comment #8)
> Comment on attachment 581102 [details] [diff] [review]
> (2/2) Add Fennec bookmarks to special "mobile" folder
> 
> Review of attachment 581102 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/db/LocalBrowserDB.java
> @@ +232,5 @@
> >  
> >          return (count == 1);
> >      }
> >  
> > +    private long getMobileBookmarksFolderId(ContentResolver cr) {
> 
> calling this every time looks unnecessarily expensive, we should cache this
> value.

Good point. Done.
 
> @@ +238,5 @@
> > +
> > +        try {
> > +            c = cr.query(appendProfile(Bookmarks.CONTENT_URI),
> > +                         new String[] { Bookmarks._ID },
> > +                         Images.GUID + " = ?",
> 
> why Images.GUID? I understand it should be the same value, but shouldn't it
> be Bookmarks.GUID?

You're right. Fixed.
Comment 10 Lucas Rocha (:lucasr) 2011-12-13 03:59:22 PST
Created attachment 581225 [details] [diff] [review]
(2/2) Add Fennec bookmarks to special "mobile" folder

Sorry, attached the wrong file. This is the right one.
Comment 11 Brad Lassey [:blassey] (use needinfo?) 2011-12-13 07:15:34 PST
Comment on attachment 581225 [details] [diff] [review]
(2/2) Add Fennec bookmarks to special "mobile" folder

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

::: mobile/android/base/db/LocalBrowserDB.java
@@ +236,5 @@
>      }
>  
> +    private long getMobileBookmarksFolderId(ContentResolver cr) {
> +        if (mMobileFolderId >= 0)
> +            return mMobileFolderId;

are you sure the folder id is always positive? Are we generating the ids?
Comment 12 Lucas Rocha (:lucasr) 2011-12-13 08:00:01 PST
(In reply to Brad Lassey [:blassey] from comment #11)
> Comment on attachment 581225 [details] [diff] [review]
> (2/2) Add Fennec bookmarks to special "mobile" folder
> 
> Review of attachment 581225 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/db/LocalBrowserDB.java
> @@ +236,5 @@
> >      }
> >  
> > +    private long getMobileBookmarksFolderId(ContentResolver cr) {
> > +        if (mMobileFolderId >= 0)
> > +            return mMobileFolderId;
> 
> are you sure the folder id is always positive? Are we generating the ids?

The ids are autogenerated by sqlite and are always positive. From the docs:

"If no negative ROWID values are inserted explicitly, then automatically generated ROWID values will always be greater than zero."
Comment 13 Richard Newman [:rnewman] 2011-12-13 08:16:52 PST
Comment on attachment 581101 [details] [diff] [review]
(1/2) Create "mobile" special bookmarks folder on DB creation

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

::: mobile/android/base/db/BrowserProvider.java
@@ +320,5 @@
>          }
>  
> +        private void createMobileBookmarksFolder(SQLiteDatabase db) {
> +            ContentValues values = new ContentValues();
> +            values.put(Bookmarks.GUID, Bookmarks.MOBILE_FOLDER_GUID);

Sync will most likely set a title for this when pulling down data from existing Fennec users, FYI.
Comment 14 Lucas Rocha (:lucasr) 2011-12-13 11:49:57 PST
(In reply to Richard Newman [:rnewman] from comment #13)
> Comment on attachment 581101 [details] [diff] [review]
> (1/2) Create "mobile" special bookmarks folder on DB creation
> 
> Review of attachment 581101 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/db/BrowserProvider.java
> @@ +320,5 @@
> >          }
> >  
> > +        private void createMobileBookmarksFolder(SQLiteDatabase db) {
> > +            ContentValues values = new ContentValues();
> > +            values.put(Bookmarks.GUID, Bookmarks.MOBILE_FOLDER_GUID);
> 
> Sync will most likely set a title for this when pulling down data from
> existing Fennec users, FYI.

No problem for us.

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