we are storing guids in the backups, but not restoring them.

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.

Could you please assign this bug to me and review my patch? Johannes

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.

Okay. This should include all of these.

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.

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?

(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?

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.

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.

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?

Closing old needinfo request...

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.

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

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!