1215086 - Support same URL being bookmarked twice

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Sync, defect, P1)

Description

[Michiel de Jong [:michielbdejong]]

It seems that it's possible to bookmark the same URL in two different bookmarks from Firefox Desktop. Our bookmarks DataAdapter currently chokes on that with an error like:

Records should have the same Firefox Sync ID Object { id: "https://support.mozilla.org/product…", url: "https://support.mozilla.org/product…", name: "Firefox：技术支持", type: "url", iconable: false, icon: "", fxsyncPayload: Object, fxsyncId: "r1gHNBtunO1X" } Object { id: "https://support.mozilla.org/product…", url: "https://support.mozilla.org/product…", name: "Firefox: Support", type: "url", iconable: false, icon: "", fxsyncPayload: Object, fxsyncId: "qyiXfgB5q3dm" }

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] michielbdejong:1215086-improve-bookmarks-datastore > mozilla-b2g:master

Comment 2

[Michiel de Jong [:michielbdejong]]

Comment on attachment 8674832 [details] [review]
[gaia] michielbdejong:1215086-improve-bookmarks-datastore > mozilla-b2g:master

Needs rebase and unit tests.

Comment 3

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8674832 [details] [review]
[gaia] michielbdejong:1215086-improve-bookmarks-datastore > mozilla-b2g:master

Hey Michiel, Thanks a lot for your patch and the idea of the solution is practical! I will provide my tested patch for you to give me a feedback.

Comment 4

[GitHub Autolander Bot]

Attachment: [gaia] weilonge:seanlee/DataSync/master/Bug1215086 > mozilla-b2g:master

Comment 5

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8675988 [details] [review]
[gaia] weilonge:seanlee/DataSync/master/Bug1215086 > mozilla-b2g:master

Hey Michiel, Could you give the patch a feedback? Thank you!

Comment 6

[Michiel de Jong [:michielbdejong]]

Comment on attachment 8675988 [details] [review]
[gaia] weilonge:seanlee/DataSync/master/Bug1215086 > mozilla-b2g:master

As discussed with the whiteboard, looks good, with two remarks:

* { deleted: true, fxSyncId: ... } -> { deleted: true, id: ... } (so we can just store FxSyncRecords as-is without looking at what we're storing, apart from determining the id for type === bookmark / type !== bookmark), and simplify the DataAdapter code a lot)
* Before deleting a DataStore record (the 'isEmpty' code), check for the record.createdLocally boolean. We can then set that boolean in the phone-readonly case which we newly identified.

Comment 7

[Sean Lee [:seanlee][:weilonge]]

(In reply to Michiel de Jong [:michielbdejong] from comment #6)
> Comment on attachment 8675988 [details] [review]
> [gaia] weilonge:seanlee/DataSync/master/Bug1215086 > mozilla-b2g:master
> 
> As discussed with the whiteboard, looks good, with two remarks:
> 
> * { deleted: true, fxSyncId: ... } -> { deleted: true, id: ... } (so we can
> just store FxSyncRecords as-is without looking at what we're storing, apart
> from determining the id for type === bookmark / type !== bookmark), and
> simplify the DataAdapter code a lot)
> * Before deleting a DataStore record (the 'isEmpty' code), check for the
> record.createdLocally boolean. We can then set that boolean in the
I would like to use createdRemotely here, so the default value of createdRemotely will be logical false.
DataAdapter has to assign createdRemotely to true explicitly when adding new record from FxSync server.
If other apps try to add new record, it won't sync up to server. 

> phone-readonly case which we newly identified.

Comment 8

[Michiel de Jong [:michielbdejong]]

(In reply to Sean Lee [:seanlee] from comment #7)
> I would like to use createdRemotely here, so the default value of
> createdRemotely will be logical false.
> DataAdapter has to assign createdRemotely to true explicitly when adding new
> record from FxSync server.

We can improve the naming, but if a bookmark or history entry is created *both* locally and remotely, the information that interests us is if it was createdLocally, because that's the case in which we don't delete it.

Comment 9

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8675988 [details] [review]
[gaia] weilonge:seanlee/DataSync/master/Bug1215086 > mozilla-b2g:master

Hi Michiel,

The patch is updated based on your suggestions at comment 6, and I rename 'createdRemotely' (bad naming :( ) to 'syncNeeded'.

Could you help to review the patch?

Thank you!

Comment 10

[Michiel de Jong [:michielbdejong]]

Comment on attachment 8675988 [details] [review]
[gaia] weilonge:seanlee/DataSync/master/Bug1215086 > mozilla-b2g:master

Given that this code is working, I think we should merge it tonight, and try to flash it onto a TV tomorrow. I see a few opportunities for refactor, but let's leave those for next week, in a follow-up bug. Fernando, do you agree?

Comment 11

[Fernando Jiménez Moreno [:ferjm]]

Go for it!

Just clean it up a little bit from local testing stuff (scratchpad, console.log, commented code, etc.) before landing, please.

Comment 12

[Michiel de Jong [:michielbdejong]]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #11)
> Go for it!
> 
> Just clean it up a little bit from local testing stuff (scratchpad,
> console.log, commented code, etc.) before landing, please.

Sean, can you take care of that, and also squash the commits into one?

Comment 13

[Sean Lee [:seanlee][:weilonge]]

The patch is squashed and waiting for merging...

Comment 14

[Sean Lee [:seanlee][:weilonge]]

landed on master: https://github.com/mozilla-b2g/gaia/commit/0fa4c88952c8d0e217805c9630d8dedf0b1bade5
gaia-test: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=6d9762ff5db24ce697e693b3dc4a72475395f9ba