1351915 - `dedupeSyncBookmark` should fire `onItemChanged` when an item's GUID changes

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P2)

Description

[Lina Butler [:lina]]

`dedupeSyncBookmark` updates the GUID in the database, and calls `invalidateCachedGuidFor`, but doesn't fire `onItemChanged`. This means consumers that cache GUIDs (Web Extensions, for example, via the `chrome.bookmarks` API) might be working with stale ones.

Comment 1

[Richard Newman [:rnewman]]

Talking specifically about browser.bookmarks:

Note that the WE API is… optimistic in this regard.

"Each ID is unique within the user's profile and remains unchanged across browser restarts."

and the WE onChanged event is only defined for titles and URLs:

> Fired when there is a change to:
>
> -   the title or URL of a bookmark
> -   the name of a folder.

I'm not sure if this is just our docs or if that restriction is in the spec.

Comment 2

[Edouard Oger [:eoger]]

Attachment: Bug 1351915 - Fire onItemChanged when deduping sync bookmarks.

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

Comment 3

[Lina Butler [:lina]]

Comment on attachment 8858931 [details]
Bug 1351915 - Fire onItemChanged when deduping sync bookmarks.

https://reviewboard.mozilla.org/r/130926/#review133514

Looks great, thanks!

::: toolkit/components/places/PlacesSyncUtils.jsm:1776
(Diff revision 1)
>  
>  var dedupeSyncBookmark = Task.async(function* (db, localGuid, remoteGuid,
>                                                 remoteParentGuid) {
> +  let bookmarkItem = yield PlacesUtils.bookmarks.fetch(localGuid);
>    let rows = yield db.executeCached(`
> -    SELECT b.id, p.guid AS parentGuid, b.syncStatus
> +    SELECT b.id, p.id AS parentId, p.guid AS parentGuid, b.syncStatus

Might as well add `b.type` to the query, too, and remove the `fetch()` call.

Comment 4

[Edouard Oger [:eoger]]

Comment on attachment 8858931 [details]
Bug 1351915 - Fire onItemChanged when deduping sync bookmarks.

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

Comment 5

[Edouard Oger [:eoger]]

Thanks!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1eceb91e9437
Fire onItemChanged when deduping sync bookmarks. r=kitcambridge

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1eceb91e9437