Closed
Bug 1215086
Opened 8 years ago
Closed 8 years ago
Support same URL being bookmarked twice
Categories
(Firefox OS Graveyard :: Sync, defect, P1)
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" }
Reporter | ||
Updated•8 years ago
|
Comment 1•8 years ago
|
||
Reporter | ||
Comment 2•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Blocks: TV_FxAccount
Reporter | ||
Updated•8 years ago
|
Priority: -- → P1
Assignee | ||
Comment 3•8 years ago
|
||
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 4•8 years ago
|
||
Assignee | ||
Comment 5•8 years ago
|
||
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)
Reporter | ||
Comment 6•8 years ago
|
||
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+
Assignee | ||
Comment 7•8 years ago
|
||
(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.
Reporter | ||
Comment 8•8 years ago
|
||
(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.
Reporter | ||
Updated•8 years ago
|
Assignee: mbdejong → selee
Assignee | ||
Comment 9•8 years ago
|
||
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)
Reporter | ||
Comment 10•8 years ago
|
||
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+
Comment 11•8 years ago
|
||
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)
Reporter | ||
Comment 12•8 years ago
|
||
(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)
Assignee | ||
Comment 13•8 years ago
|
||
The patch is squashed and waiting for merging...
Flags: needinfo?(selee)
Assignee | ||
Comment 14•8 years ago
|
||
landed on master: https://github.com/mozilla-b2g/gaia/commit/0fa4c88952c8d0e217805c9630d8dedf0b1bade5 gaia-test: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=6d9762ff5db24ce697e693b3dc4a72475395f9ba
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•8 years ago
|
Whiteboard: [partner-cherry-pick]
You need to log in
before you can comment on or make changes to this bug.
Description
•