Closed
Bug 967204
Opened 11 years ago
Closed 9 years ago
Restoring a JSON backup should set stored guids
Categories
(Toolkit :: Places, defect, P1)
Toolkit
Places
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: mak, Assigned: johannes, Mentored)
References
(Blocks 1 open bug)
Details
(Whiteboard: [good next bug][lang=js])
Attachments
(1 file, 2 obsolete files)
26.33 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
we are storing guids in the backups, but not restoring them.
Reporter | ||
Comment 1•11 years ago
|
||
This is important for Sync, all bookmarking APIs allow to define a guid on bookmark creation, if the backup has the guid we should pass it to them.
Flags: firefox-backlog+
Whiteboard: [mentor=mak][lang=js]p=3
Reporter | ||
Updated•11 years ago
|
Whiteboard: [mentor=mak][lang=js]p=3 → [good next bug][mentor=mak][lang=js]p=3
Updated•10 years ago
|
Mentor: mak77
Whiteboard: [good next bug][mentor=mak][lang=js]p=3 → [good next bug][lang=js]p=3
Assignee | ||
Comment 2•10 years ago
|
||
Could you please assign this bug to me and review my patch?
Johannes
Attachment #8465689 -
Flags: review?(mak77)
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → johannes
Status: NEW → ASSIGNED
Updated•10 years ago
|
Points: --- → 3
Whiteboard: [good next bug][lang=js]p=3 → [good next bug][lang=js]
Reporter | ||
Comment 3•10 years ago
|
||
Comment on attachment 8465689 [details] [diff] [review]
JSONBackupGUID.patch
Review of attachment 8465689 [details] [diff] [review]:
-----------------------------------------------------------------
The approach is correct, but you should do the same also for createFolder and insertSeparator.
Moreover it would be nice if you might add an xpcshell-test that verifies we are restoring guids, or even better modify one of the existing ones... I think toolkit/components/places/tests/unit/test_bookmarks_json.js might be a good candidate. The test should just store the original guid before a backup, do a restore, and check the guid after the restore.
Attachment #8465689 -
Flags: review?(mak77) → feedback+
Assignee | ||
Comment 4•10 years ago
|
||
Okay. This should include all of these.
Attachment #8465689 -
Attachment is obsolete: true
Attachment #8469186 -
Flags: review?(mak77)
Reporter | ||
Comment 5•10 years ago
|
||
Comment on attachment 8469186 [details] [diff] [review]
JSONBackupGUIDV2.patch
Review of attachment 8469186 [details] [diff] [review]:
-----------------------------------------------------------------
r=me with a green tryserver run (if you don't have access to try please needinfo me and I will push for you) and the following comments answered/addressed
::: toolkit/components/places/tests/unit/test_bookmarks_json.js
@@ +186,5 @@
> do_check_true(base64Icon == aExpected.icon);
> }
> break;
> + case "guid":
> + if(aNode.guid){
space after if and after )
@@ +187,5 @@
> }
> break;
> + case "guid":
> + if(aNode.guid){
> + do_check_eq(aNode.guid, aExpected.guid);
did you check this test runs in the expected cases? I'd not want the if to suppress it everytime.
Attachment #8469186 -
Flags: review?(mak77) → review+
Assignee | ||
Comment 6•10 years ago
|
||
You're right: The test doesn't work as expected, the guid is alway undefined and does not need the if check if it works properly. How can I get the GUID of an item as the getItemGUID method (https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsINavBookmarksService#getItemGUID%28%29) was removed with Firefox 14?
Reporter | ||
Comment 7•10 years ago
|
||
(In reply to Johannes Mittendorfer from comment #6)
> You're right: The test doesn't work as expected, the guid is alway undefined
> and does not need the if check if it works properly. How can I get the GUID
> of an item as the getItemGUID method
> (https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/
> Interface/nsINavBookmarksService#getItemGUID%28%29) was removed with Firefox
> 14?
Sorry, I missed this question... next time please use needinfo flag, we get hundreds of bugmail and it's very easy to lose some comments.
you can use PlacesUtils.promiseItemGuid
do you still plan to complete work on this bug?
Flags: needinfo?(johannes)
Reporter | ||
Comment 8•10 years ago
|
||
Unassigning, looks like Johannes is not working on this anymore.
If anyone wants to complete the patch comment 5 is what is left to do.
Assignee: johannes → nobody
Flags: needinfo?(johannes)
Updated•10 years ago
|
Status: ASSIGNED → NEW
Updated•10 years ago
|
Assignee: nobody → anush
Status: NEW → ASSIGNED
Updated•10 years ago
|
Mentor: jaws
Updated•10 years ago
|
Flags: needinfo?(mak77)
Comment 9•10 years ago
|
||
The existing patch on comment #5 worked for me, except for the syntax format I don't find any problem ..can you have a look @mak
Reporter | ||
Comment 10•10 years ago
|
||
(In reply to Anush [:bmx] from comment #9)
> The existing patch on comment #5 worked for me, except for the syntax format
> I don't find any problem ..can you have a look @mak
Are you interested in completing the patch?
As I said, the remaining things to do are in comment 5, fix the code style, and verify the test conditioned by the if actually runs.
You can attach an updated patch and I'll gladly review it.
Flags: needinfo?(mak77)
Assignee | ||
Comment 11•10 years ago
|
||
Sorry for not replying in December, I somehow didn't notice the mails in my inbox. I can't work on it anymore due to lack of time.
@bmx: I remember that the guid was always null (or NaN in JavaScript) during writing the tests. This is why I asked for a method to access the real value. I'm pretty sure there is still a bug in there, at least in the test cases. I noticed it during fixing the coding style.
Comment 13•9 years ago
|
||
Closing old needinfo request...
Assignee: anush → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(anush)
Reporter | ||
Updated•9 years ago
|
Priority: -- → P1
Updated•9 years ago
|
Mentor: jaws
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(mak77)
Assignee | ||
Comment 14•9 years ago
|
||
After writing my bachelor thesis about developing a Firefox extension I finally know enough and had time to fix this.
The tests of the places component passed and eslint was happy.
Attachment #8469186 -
Attachment is obsolete: true
Attachment #8752466 -
Flags: review?(mak77)
Reporter | ||
Comment 15•9 years ago
|
||
Comment on attachment 8752466 [details] [diff] [review]
JSONBackupGUIDV3.patch
Review of attachment 8752466 [details] [diff] [review]:
-----------------------------------------------------------------
It looks good!
Do you have access to the Try Server? A try run is requested before you can set the checkin-needed keyword
Attachment #8752466 -
Flags: review?(mak77) → review+
Assignee | ||
Comment 16•9 years ago
|
||
I have Try Server access but I currently cannot push. (I don't have my PC with me)
Could you push it for me? Thanks.
Comment 17•9 years ago
|
||
Comment 18•9 years ago
|
||
Try run LGTM - thanks Johannes!
Comment 19•9 years ago
|
||
Keywords: checkin-needed
Comment 20•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in
before you can comment on or make changes to this bug.
Description
•