Closed Bug 1215086 Opened 4 years ago Closed 4 years ago

Support same URL being bookmarked twice

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(Not tracked)

RESOLVED FIXED
FxOS-S10 (30Oct)

People

(Reporter: mbdejong, Assigned: selee)

References

Details

(Whiteboard: [partner-cherry-pick])

Attachments

(2 files)

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" }
Assignee: nobody → mbdejong
Blocks: 1195647
Target Milestone: --- → FxOS-S10 (30Oct)
Comment on attachment 8674832 [details] [review]
[gaia] michielbdejong:1215086-improve-bookmarks-datastore > mozilla-b2g:master

Needs rebase and unit tests.
Attachment #8674832 - Flags: feedback?(selee)
Blocks: 1215482
Blocks: TV_FxAccount
Priority: -- → P1
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.
Attachment #8674832 - Flags: feedback?(selee) → feedback+
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!
Attachment #8675988 - Flags: feedback?(mbdejong)
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.
Attachment #8675988 - Flags: feedback?(mbdejong) → feedback+
(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.
(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.
Assignee: mbdejong → selee
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!
Attachment #8675988 - Flags: review?(mbdejong)
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?
Flags: needinfo?(ferjmoreno)
Attachment #8675988 - Flags: review?(mbdejong) → review+
Go for it!

Just clean it up a little bit from local testing stuff (scratchpad, console.log, commented code, etc.) before landing, please.
Flags: needinfo?(ferjmoreno)
(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?
Flags: needinfo?(selee)
The patch is squashed and waiting for merging...
Flags: needinfo?(selee)
Whiteboard: [partner-cherry-pick]
You need to log in before you can comment on or make changes to this bug.