Closed Bug 708485 Opened 13 years ago Closed 13 years ago

Add Special Folders with Sync guids to Bookmarks Store

Categories

(Firefox for Android Graveyard :: General, defect, P3)

Tracking

(firefox11 fixed, fennec11+)

RESOLVED FIXED
Tracking Status
firefox11 --- fixed
fennec 11+ ---

People

(Reporter: jvoll, Unassigned)

References

Details

Attachments

(2 files, 3 obsolete files)

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.
Blocks: 704490
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.
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!
Severity: normal → major
OS: Mac OS X → All
Hardware: x86 → All
Priority: -- → P3
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.
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.
Attachment #581099 - Flags: review?(blassey.bugs)
Attachment #581099 - Flags: feedback?(rnewman)
Attachment #581101 - Flags: review?(blassey.bugs)
Attachment #581101 - Flags: feedback?(rnewman)
Comment on attachment 581099 [details] [diff] [review]
Create views for bookmarks and history adding image columns

Sorry, attached to wrong bug.
Attachment #581099 - Attachment is obsolete: true
Attachment #581099 - Flags: review?(blassey.bugs)
Attachment #581099 - Flags: feedback?(rnewman)
Attachment #581102 - Flags: review?(blassey.bugs)
Attachment #581102 - Flags: feedback?(rnewman)
Attachment #581101 - Flags: review?(blassey.bugs) → review+
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?
Attachment #581102 - Flags: review?(blassey.bugs) → review-
(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.
Attachment #581102 - Attachment is obsolete: true
Attachment #581102 - Flags: feedback?(rnewman)
Attachment #581223 - Flags: review?(blassey.bugs)
Sorry, attached the wrong file. This is the right one.
Attachment #581223 - Attachment is obsolete: true
Attachment #581223 - Flags: review?(blassey.bugs)
Attachment #581225 - Flags: review?(blassey.bugs)
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?
Attachment #581225 - Flags: review?(blassey.bugs) → review+
(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 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.
Attachment #581101 - Flags: feedback?(rnewman) → feedback+
(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.
Pushed:
http://hg.mozilla.org/mozilla-central/rev/096f185e573d
http://hg.mozilla.org/mozilla-central/rev/0e227df7b536
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
tracking-fennec: --- → 11+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: