Closed Bug 967204 Opened 7 years ago Closed 4 years ago

Restoring a JSON backup should set stored guids

Categories

(Toolkit :: Places, defect, P1)

defect
Points:
3

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)

we are storing guids in the backups, but not restoring them.
Blocks: 1012597
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
Whiteboard: [mentor=mak][lang=js]p=3 → [good next bug][mentor=mak][lang=js]p=3
Mentor: mak77
Whiteboard: [good next bug][mentor=mak][lang=js]p=3 → [good next bug][lang=js]p=3
Attached patch JSONBackupGUID.patch (obsolete) — Splinter Review
Could you please assign this bug to me and review my patch?

Johannes
Attachment #8465689 - Flags: review?(mak77)
Assignee: nobody → johannes
Status: NEW → ASSIGNED
Points: --- → 3
Whiteboard: [good next bug][lang=js]p=3 → [good next bug][lang=js]
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+
Attached patch JSONBackupGUIDV2.patch (obsolete) — Splinter Review
Okay. This should include all of these.
Attachment #8465689 - Attachment is obsolete: true
Attachment #8469186 - Flags: review?(mak77)
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+
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?
Blocks: 1071511
(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)
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)
Status: ASSIGNED → NEW
Assignee: nobody → anush
Status: NEW → ASSIGNED
Flags: needinfo?(mak77)
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
(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)
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.
Anush, any update here?
Flags: needinfo?(anush)
Closing old needinfo request...
Assignee: anush → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(anush)
Priority: -- → P1
Flags: needinfo?(mak77)
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)
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+
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.
Try run LGTM - thanks Johannes!
Assignee: nobody → johannes
Flags: needinfo?(mak77)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/312c94f907b3
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.