Read-only Bookmarks data adapter

RESOLVED FIXED in FxOS-S8 (02Oct)

Status

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mbdejong, Assigned: selee, NeedInfo)

Tracking

unspecified
FxOS-S8 (02Oct)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [partner-cherry-picked<2015/11/10>])

Attachments

(3 attachments)

(Reporter)

Description

3 years ago
The bookmarks data items on the Sync server usually have base-64 id's, but they can also have one of the special id's "menu" or "toolbar" (are there more?).
Their payload contains string fields like 'URL' and 'name'.

The bookmarks data items in the DataStore are really simple, they have two string fields: 'url', and 'name' (where 'url' should be a valid URL).

So it looks very doable to create a Bookmarks data adapter once the History data adapter (bug 1191773) has landed.
(Reporter)

Comment 1

3 years ago
Sorry, the data fields in the payload from the Sync server are bmkUri and title. You can see the bookmarks coming in if you run the sync app from my michielbdejong/1200539-sync-app-integration-tests branch.
Depends on: 1191773
Duplicate of this bug: 1206009
Blocks: 824026
Target Milestone: --- → FxOS-S8 (02Oct)
Sean, I believe you are working on this. So I am assigning the bug to you.
Assignee: nobody → selee
(Assignee)

Updated

3 years ago
Depends on: 1208352
(Assignee)

Comment 4

3 years ago
Created attachment 8665816 [details] [review]
WIP patch: Bookmarks Adapter
(Assignee)

Comment 5

3 years ago
Created attachment 8665871 [details]
example_bookmarks_2.json

Hey Michiel,

I got these error messages in SyncResults when retrieving(collection.sync()) the records from Bookmarks Collection.
    "Invalid UUID: places"
    "Invalid UUID: toolbar"
    "Invalid UUID: menu"
    "Invalid UUID: unfiled"

The 4 errors exactly have the same property that the type is "folder" in the JSON file dumped from decoding payload, so I use this workaround to validate the ID in 2 to 12 characters:
https://github.com/weilonge/gaia/commit/7c774ffc21307b490c7abc6eb1c88a4d48471dc1

Shall we provide a way for adapters to customize the validate function?

Thanks for your comments. :)
Flags: needinfo?(mbdejong)
(Reporter)

Comment 6

3 years ago
(In reply to Sean Lee [:seanlee] from comment #5)
> Created attachment 8665871 [details]
> example_bookmarks_2.json
> 
> Hey Michiel,
> 
> I got these error messages in SyncResults when retrieving(collection.sync())
> the records from Bookmarks Collection.
>     "Invalid UUID: places"
>     "Invalid UUID: toolbar"
>     "Invalid UUID: menu"
>     "Invalid UUID: unfiled"
> 
> The 4 errors exactly have the same property that the type is "folder" in the
> JSON file dumped from decoding payload, so I use this workaround to validate
> the ID in 2 to 12 characters:
> https://github.com/weilonge/gaia/commit/
> 7c774ffc21307b490c7abc6eb1c88a4d48471dc1
> 
> Shall we provide a way for adapters to customize the validate function?
> 
> Thanks for your comments. :)

Yes, this is implemented in https://github.com/michielbdejong/gaia/commit/ada4fc02c015134432f0c6c1ddeeb1fb5b2eb688

Which version are you using?
Flags: needinfo?(mbdejong)
(Assignee)

Comment 7

3 years ago
I use the latest version in master branch.
Is this patch landed to master?
(Assignee)

Comment 8

3 years ago
The patch works! Thank you, Michiel. :)
(Reporter)

Comment 9

3 years ago
> I use the latest version in master branch.

A lot of things are not in master yet, that's why it's better to test with my integration branch, but I realize this is a bit of a messy situation, so sorry about that. Should be resolved by next week.

Anyway, if it works for you in master, then that's solved. :)
(Assignee)

Comment 10

3 years ago
Hi Fernando, Michiel,

"bookmarks_store" uses URL as DataStore ID (key), but there are some types in FxSync Bookmarks without a URL (type: 'folder', 'separator') or a general URL (type: 'query').
I would like to use FxSync ID (Syncto ID) as the ID for 'firefox-sync-bookmarks'. When deleting deleted: true records coming from FxSync, we don't have to look up the ID for datastore (in history adapter, it uses asyncStorage). It is also easier to build a folder tree by FxSync ID.

Could you give me some suggestions here? Thank you.

[1] https://docs.services.mozilla.com/sync/objectformats.html#bookmarks
Status: NEW → ASSIGNED
Flags: needinfo?(mbdejong)
Flags: needinfo?(ferjmoreno)
(Reporter)

Comment 11

3 years ago
(In reply to Sean Lee [:seanlee] from comment #10)
> Hi Fernando, Michiel,
> 
> "bookmarks_store" uses URL as DataStore ID (key), but there are some types
> in FxSync Bookmarks without a URL (type: 'folder', 'separator') or a general
> URL (type: 'query').
> I would like to use FxSync ID (Syncto ID) as the ID for
> 'firefox-sync-bookmarks'. When deleting deleted: true records coming from
> FxSync, we don't have to look up the ID for datastore (in history adapter,
> it uses asyncStorage). It is also easier to build a folder tree by FxSync ID.
> 
> Could you give me some suggestions here? Thank you.
> 
> [1] https://docs.services.mozilla.com/sync/objectformats.html#bookmarks

There's probably a reason why the URL is used as the id? I think it would be easier to keep it that way, and for bookmark-folders (which don't themselves have a URL), maybe you can add a special string to act as their "URL", for instance instead of "http://whatever.com/", their URL field would be "folder:whatever"
Flags: needinfo?(mbdejong)
(Assignee)

Comment 12

3 years ago
(In reply to Michiel de Jong [:michielbdejong] from comment #11)
> There's probably a reason why the URL is used as the id? I think it would be
If you try to add a bookmark with the same URL in another bookmark record in Firefox browser, the browser will let you "edit" the existing one rather than adding a new one. I think that's probably one of the reasons.

> easier to keep it that way, and for bookmark-folders (which don't themselves
> have a URL), maybe you can add a special string to act as their "URL", for
> instance instead of "http://whatever.com/", their URL field would be
> "folder:whatever"

I ever consider this way, and my idea is like this:
ID for DataStore := [Record.Type]::[Record.URL or Record.FxSyncID]

For separator or folder without URL, I will use FxSyncID as it's unique ID.

How do you think about this? :)
(In reply to Sean Lee [:seanlee] from comment #12)
> (In reply to Michiel de Jong [:michielbdejong] from comment #11)
> > There's probably a reason why the URL is used as the id? I think it would be
> If you try to add a bookmark with the same URL in another bookmark record in
> Firefox browser, the browser will let you "edit" the existing one rather
> than adding a new one. I think that's probably one of the reasons.
> 

Also note that the browser needs an easy and fast way to display the bookmark icon (the star) in the awesome bar when the user types an URL that is stored as a bookmark. If we have the FxSyncID as the id of firefox_sync_bookmarks we'll need to iterate over the whole DS or have a local IDB with the bookmarks indexed by URL.

Given that DataStore doesn't allow indexes, we need records stored in the firefox_sync_bookmarks DS to have the URL as its key, just like the bookmarks_store does.

> > easier to keep it that way, and for bookmark-folders (which don't themselves
> > have a URL), maybe you can add a special string to act as their "URL", for
> > instance instead of "http://whatever.com/", their URL field would be
> > "folder:whatever"
> 
> I ever consider this way, and my idea is like this:
> ID for DataStore := [Record.Type]::[Record.URL or Record.FxSyncID]
> 
> For separator or folder without URL, I will use FxSyncID as it's unique ID.
> 
> How do you think about this? :)

I'd go with Michiel's proposal.

For regular records, (only) the URL should be the key, so we can easily do ds.get(url).

For folder or separator records, keys can be folder|separator:whatever or even the fxsyncID as you suggest, it doesn't really matter that much as long as keys are unique.

You'll also need to store the fxSyncID separately on each record (even if you use if for the key of folders or separators).
Flags: needinfo?(ferjmoreno)

Updated

3 years ago
Blocks: 1207521
(Reporter)

Comment 14

3 years ago
(In reply to Sean Lee [:seanlee] from comment #5)
> I got these error messages in SyncResults when retrieving(collection.sync())
> the records from Bookmarks Collection.
>     "Invalid UUID: places"
>     "Invalid UUID: toolbar"
>     "Invalid UUID: menu"
>     "Invalid UUID: unfiled"

You mean 'mobile', not 'places', right?
(Reporter)

Updated

3 years ago
Depends on: 1209906
(Assignee)

Comment 15

3 years ago
(In reply to Michiel de Jong [:michielbdejong] from comment #14)
> (In reply to Sean Lee [:seanlee] from comment #5)
> > I got these error messages in SyncResults when retrieving(collection.sync())
> > the records from Bookmarks Collection.
> >     "Invalid UUID: places"
> >     "Invalid UUID: toolbar"
> >     "Invalid UUID: menu"
> >     "Invalid UUID: unfiled"
> 
> You mean 'mobile', not 'places', right?

'places' is one of folder in my test bookmarks.
(Assignee)

Comment 16

3 years ago
Here is the specialRecords that I am using:
  specialRecords: [
    'menu',
    'mobile',
    'toolbar',
    'places',
    'unfiled'
  ]
(Assignee)

Updated

3 years ago
Summary: Bookmarks data adapter → Read-only Bookmarks data adapter
Created attachment 8667832 [details] [review]
[gaia] weilonge:seanlee/DataSync/master/Bug1202382 > mozilla-b2g:master
(Assignee)

Comment 18

3 years ago
Comment on attachment 8667832 [details] [review]
[gaia] weilonge:seanlee/DataSync/master/Bug1202382 > mozilla-b2g:master

Hi Fernando, Michiel,

Bookmarks adapter is implemented in the similar login with History adapter.
Could you help to review the patch?

Thank you! :)
Attachment #8667832 - Flags: review?(ferjmoreno)
Attachment #8667832 - Flags: feedback?(mbdejong)
Comment on attachment 8667832 [details] [review]
[gaia] weilonge:seanlee/DataSync/master/Bug1202382 > mozilla-b2g:master

Nice work Sean! I left a few comments in the PR. The code looks good to me but there are some small things that needs to be sorted out. Also, I am missing a few test cases.
Thank you!
Attachment #8667832 - Flags: review?(ferjmoreno) → feedback+
(Assignee)

Comment 20

3 years ago
Hi Vivien,

I am trying to build a datastore for FxSync Bookmarks.
When I am considering the compatibility to bookmarks_store, could you give a hint that how many types would be in bookmarks_store? I suppose that 'url' is the only one.
 https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_chrome.js#L1043

Thank you! :)
Flags: needinfo?(21)
(Reporter)

Comment 21

3 years ago
Comment on attachment 8667832 [details] [review]
[gaia] weilonge:seanlee/DataSync/master/Bug1202382 > mozilla-b2g:master

Great! Left some feedback on github. My main concern is why IIUC we're now using two different bookmark stores, 'firefox-sync-bookmarks' and 'bookmarks_store'.
Attachment #8667832 - Flags: feedback?(mbdejong) → feedback+
(Assignee)

Comment 22

3 years ago
Comment on attachment 8667832 [details] [review]
[gaia] weilonge:seanlee/DataSync/master/Bug1202382 > mozilla-b2g:master

Hey Fernando,

I have updated the patch based on your and Michiel's comment.
Could you help to review it again?

Thank you!


Hey Michiel,

Thank you for giving the feedback. :)
Attachment #8667832 - Flags: review?(ferjmoreno)
(Assignee)

Updated

3 years ago
Whiteboard: partner-cherry-pick
(Assignee)

Updated

3 years ago
Whiteboard: partner-cherry-pick → [partner-cherry-pick]
(Assignee)

Updated

3 years ago
Whiteboard: [partner-cherry-pick]
(Assignee)

Updated

3 years ago
Whiteboard: [partner-cherry-pick]

Updated

3 years ago
Blocks: 1210697

Updated

3 years ago
No longer blocks: 1210697
Comment on attachment 8667832 [details] [review]
[gaia] weilonge:seanlee/DataSync/master/Bug1202382 > mozilla-b2g:master

Thank you Sean! r=me with the comments addressed, please.
Attachment #8667832 - Flags: review?(ferjmoreno) → review+
(Assignee)

Comment 24

3 years ago
landed on master: https://github.com/mozilla-b2g/gaia/commit/5e10cdeb6c9ebe0312c0283a894c227f79edcb1c
gaia test: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=b7cbb6b202eaac5875c70dc4f682bc7884553c9b

Thank you, Fernando! :)
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Updated

3 years ago
Depends on: 1217340

Updated

3 years ago
Whiteboard: [partner-cherry-pick] → [partner-cherry-picked<2015/11/10>]
You need to log in before you can comment on or make changes to this bug.