1019226 - Use GUID-aware record creation primitives in bookmarks.js

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P2)

Description

[Richard Newman [:rnewman]]

Spawned from Bug 1012597.

Sync still uses this pattern:

      newId = PlacesUtils.bookmarks.insertBookmark(
        record._parent, uri, PlacesUtils.bookmarks.DEFAULT_INDEX, record.title);

...


    if (newId) {
      // Livemarks can set the GUID through the API, so there's no need to
      // do that here.
      this._log.trace("Setting GUID of new item " + newId + " to " + record.id);
      this._setGUID(newId, record.id);
    }



We should be creating records with the right GUID up-front.

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

yes please, this is pretty much important for perf and sanity.

Comment 2

[Thom Chiovoloni [:tcsc] (ex-moco)]

Hm, my initial impression was that this probably would just be a matter of adding the extra GUID argument to the end of the relevant functions (insertBookmark, createFolder, insertSeparator), and removing the this._setGUID call.

Unfortunately, doing this results in some test failures stemming from a failed NS_ENSURE_SUCCESS in nsINavBookmarksService.getFolderIdForItem after a FetchItemInfo call (specifically here[0]). I'll keep digging and try to figure out what the issue is, but any pointers would be appreciated, thanks.

[0]: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavBookmarks.cpp#2036-2037

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

the only things that come to my mind off-hand are:
- setting a guid that already exist and not checking for an exception...
- maybe the test was expecting to get the "wrong" guid since guid changes made by sync are not notified to anyone?

The fetchItemCall can only fail if the id is invalid... and I suppose you get the id from some guidToId method?

Comment 4

[Thom Chiovoloni [:tcsc] (ex-moco)]

Attachment: MozReview Request: Bug 1019226 - Pass GUID into bookmark creation functions functions during bookmark syncing, and update relevant tests to use valid guids. r?mak

Review commit: https://reviewboard.mozilla.org/r/43889/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43889/

Comment 5

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8737314 [details]
MozReview Request: Bug 1019226 - Pass GUID into bookmark creation functions functions during bookmark syncing, and update relevant tests to use valid guids. r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43889/diff/1-2/

Comment 6

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8737314 [details]
MozReview Request: Bug 1019226 - Pass GUID into bookmark creation functions functions during bookmark syncing, and update relevant tests to use valid guids. r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43889/diff/2-3/

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

what was the problem with the tests in the end?

Comment 8

[Thom Chiovoloni [:tcsc] (ex-moco)]

They were using invalid GUIDs. The mocked Utils.makeGUID would return 11 character guids the first 10 times it ran (and times past 100). "fake-guid-0", etc. needed to be changed due to this since they're 11 characters, and the GUIDs in test_bookmark_order.js were 2 or 3 characters and not 12.

This caused the failures from nsNavBookmarks that I was asking about in the earlier comment.

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8737314 [details]
MozReview Request: Bug 1019226 - Pass GUID into bookmark creation functions functions during bookmark syncing, and update relevant tests to use valid guids. r?mak

https://reviewboard.mozilla.org/r/43889/#review41711

Looks good, thank you!

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/a1880ae30781

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a1880ae30781