1285408 - Add source tracking to the Places methods used by Sync

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

[Lina Butler [:lina]]

As discussed in London, let's add an optional "source" param to all the Places methods that Sync uses. The methods will use this to decide whether to increment the syncChangeCounter.

Comment 1

[Lina Butler [:lina]]

Attachment: Bug 1285408 - Add change source tracking for bookmarks.

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

Comment 2

[Lina Butler [:lina]]

Comment on attachment 8769006 [details]
Bug 1285408 - Add change source tracking for bookmarks.

Mak, is this what you had in mind? I didn't add it for all methods in the IDLs, only the ones that PlacesSyncUtils uses.

Comment 3

[Lina Butler [:lina]]

Comment on attachment 8769006 [details]
Bug 1285408 - Add change source tracking for bookmarks.

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

Comment 4

[Lina Butler [:lina]]

Comment on attachment 8769006 [details]
Bug 1285408 - Add change source tracking for bookmarks.

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

Comment 5

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/62988/#review60440

::: services/sync/modules/engines/bookmarks.js:1111
(Diff revision 3)
>      let title = Str.sync.get("mobile.label");
>  
>      // Don't add OR remove the mobile bookmarks if there's nothing.
>      if (PlacesUtils.bookmarks.getIdForItemAt(BookmarkSpecialIds.mobile, 0) == -1) {
>        if (mobile.length != 0)
> -        PlacesUtils.bookmarks.removeItem(mobile[0]);
> +        PlacesUtils.bookmarks.removeItem(mobile[0],

I left these in the bookmarks engine (instead of PlacesSyncUtils) for now, because I suspect we'll rewrite this in bug 647605.

::: services/sync/modules/engines/bookmarks.js:1172
(Diff revision 3)
>        this._add(itemId, guid);
>        this._add(newParent, newParentGuid);
>      }
>  
>      // Remove any position annotations now that the user moved the item
> -    PlacesUtils.annotations.removeItemAnnotation(itemId, BookmarkAnnos.PARENT_ANNO);
> +    PlacesUtils.annotations.removeItemAnnotation(itemId,

This could be rolled up into bug 1258127.

Comment 6

[Lina Butler [:lina]]

Comment on attachment 8769006 [details]
Bug 1285408 - Add change source tracking for bookmarks.

Just a couple of changes to get the tests in bug 1258127 passing.

Comment 7

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

https://reviewboard.mozilla.org/r/62988/#review60560

This LGTM and reflects what I understood Mak suggested we do.

::: services/sync/modules/engines/bookmarks.js:1111
(Diff revision 3)
>      let title = Str.sync.get("mobile.label");
>  
>      // Don't add OR remove the mobile bookmarks if there's nothing.
>      if (PlacesUtils.bookmarks.getIdForItemAt(BookmarkSpecialIds.mobile, 0) == -1) {
>        if (mobile.length != 0)
> -        PlacesUtils.bookmarks.removeItem(mobile[0]);
> +        PlacesUtils.bookmarks.removeItem(mobile[0],

I'd be inclined to collapse this into a single line - it would be roughly the same length as a few lines just below - but your call.

Comment 8

[Lina Butler [:lina]]

Comment on attachment 8769006 [details]
Bug 1285408 - Add change source tracking for bookmarks.

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

Comment 9

[Lina Butler [:lina]]

Comment on attachment 8769006 [details]
Bug 1285408 - Add change source tracking for bookmarks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62988/diff/4-5/

Comment 10

[Lina Butler [:lina]]

Attachment: Bug 1285408 - Add source tracking for bookmark import and restore.

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

Comment 11

[Lina Butler [:lina]]

Comment on attachment 8769006 [details]
Bug 1285408 - Add change source tracking for bookmarks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62988/diff/5-6/

Comment 12

[Lina Butler [:lina]]

Comment on attachment 8769006 [details]
Bug 1285408 - Add change source tracking for bookmarks.

This is part two of the tracker refactor, which adds change source tracking to all methods that Sync uses. This lets us avoid tracking remote changes that Sync makes as it's applying bookmarks.

Comment 13

[Lina Butler [:lina]]

Comment on attachment 8771710 [details]
Bug 1285408 - Add source tracking for bookmark import and restore.

I went with a similar approach for bookmark import and restore, which we'll also need to handle specially for Sync. ("IMPORT" should track changes; "IMPORT_REPLACE" will mark the bookmark as "UNKNOWN" and require a full reconcile with the server).

Comment 14

[Marco Bonardo [:mak]]

https://reviewboard.mozilla.org/r/62988/#review62554

As discussed in London, I'm fine with this approach. The patch is quite big and I am not sure I have time to do a line by line review. But it's not really scary looking so I'm going to trust Mark's review on it.

::: toolkit/components/places/Bookmarks.jsm:420
(Diff revision 6)
>  
>      // Even if we ignore any other unneeded property, we still validate any
>      // known property to reduce likelihood of hidden bugs.
> -    let removeInfo = validateBookmarkObject(info);
> +    let removeInfo = validateBookmarkObject(info,
> +      { source: { defaultValue: this.SOURCE_DEFAULT }
> +      });

nit: the new line here is not needed

::: toolkit/components/places/Bookmarks.jsm:1347
(Diff revision 6)
>          return new URL(v.spec);
>        return v;
> -    }
> +    },
> +    source: simpleValidateFunc(v => Number.isInteger(v) &&
> +                                    [ Bookmarks.SOURCE_DEFAULT
> +                                    , Bookmarks.SOURCE_SYNC ].includes(v)),

nit: Alternatively, you could do for SOURCES the same I did for History.TRANSITIONS and use Object.values()

::: toolkit/components/places/nsITaggingService.idl:33
(Diff revision 6)
>     * @param aTags
>     *        Array of tags to set for the given URL.  Each element within the
>     *        array can be either a tag name (non-empty string) or a concrete
>     *        itemId of a tag container.
>     */
> -  void tagURI(in nsIURI aURI, in nsIVariant aTags);
> +  void tagURI(in nsIURI aURI, in nsIVariant aTags, [optional] in unsigned short aSource);

please each argument on a new line

Comment 15

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 8769006 [details]
Bug 1285408 - Add change source tracking for bookmarks.

https://reviewboard.mozilla.org/r/62988/#review64978

This is looking good, but there are a few minor things I raise below that should either be fixed or disussed.

::: services/sync/modules/engines/bookmarks.js:1049
(Diff revision 6)
>  
>      // Don't add OR remove the mobile bookmarks if there's nothing.
>      if (PlacesUtils.bookmarks.getIdForItemAt(BookmarkSpecialIds.mobile, 0) == -1) {
>        if (mobile.length != 0)
> -        PlacesUtils.bookmarks.removeItem(mobile[0]);
> +        PlacesUtils.bookmarks.removeItem(mobile[0],
> +          PlacesUtils.bookmarks.SOURCE_SYNC);

nit: indentation here - I think this could just be 1 line

::: services/sync/modules/engines/bookmarks.js:1053
(Diff revision 6)
> -        PlacesUtils.bookmarks.removeItem(mobile[0]);
> +        PlacesUtils.bookmarks.removeItem(mobile[0],
> +          PlacesUtils.bookmarks.SOURCE_SYNC);
>      }
>      // Add the mobile bookmarks query if it doesn't exist
>      else if (mobile.length == 0) {
> -      let query = PlacesUtils.bookmarks.insertBookmark(all[0], queryURI, -1, title);
> +      let query = PlacesUtils.bookmarks.insertBookmark(all[0], queryURI, -1,

indentation here looks odd too, but that already existed :)

::: services/sync/modules/engines/bookmarks.js:1109
(Diff revision 6)
>        this._add(itemId, guid);
>        this._add(newParent, newParentGuid);
>      }
>  
>      // Remove any position annotations now that the user moved the item
> -    PlacesUtils.annotations.removeItemAnnotation(itemId, BookmarkAnnos.PARENT_ANNO);
> +    PlacesUtils.annotations.removeItemAnnotation(itemId,

same here re indentation. You could also consider having SOURCE_SYNC be a const setup at the top of the file and leave it on 1 line, much like you've done in Bookmarks.jsm

::: toolkit/components/places/PlacesUtils.jsm:2186
(Diff revision 6)
>     * @param keyword
>     *        The keyword to remove.
>     * @return {Promise}
>     * @resolves when the removal is complete.
>     */
> -  remove(keyword) {
> +  remove(keywordOrEntry) {

This change looks odd - the only name referenced in keywordOrEntry is "keyword" - I'm expecting to see a reference to keywordOrEntry.source

::: toolkit/components/places/nsINavBookmarksService.idl:310
(Diff revision 6)
>     *         generated.  Unless you've a very sound reason, such as an undo
>     *         manager implementation, do not pass this argument.
>     *  @return The ID of the newly-created bookmark.
>     *
>     *  @note aTitle will be truncated to TITLE_LENGTH_MAX and
>     *        aURI will be truncated to URI_LENGTH_MAX.

here and below: the new source param should have @param docs (which should explicitly state the default is SOURCE_DEFAULT)

::: toolkit/components/places/nsNavBookmarks.cpp:1295
(Diff revision 6)
>                                 newIndex,
>                                 bookmark.type,
>                                 bookmark.guid,
>                                 bookmark.parentGuid,
> -                               newParentGuid));
> +                               newParentGuid,
> +                               SOURCE_DEFAULT));

Why isn't source an arg here like everywhere else? Same question below.

::: toolkit/components/places/nsTaggingService.js:139
(Diff revision 6)
>        return tag;
>      });
>    },
>  
>    // nsITaggingService
> -  tagURI: function TS_tagURI(aURI, aTags)
> +  tagURI: function TS_tagURI(aURI, aTags, aSource)

don't the changes here require a change to nsITaggingService.idl (ie, with a new optional arg specified and the implementation here allowing a default?

Comment 16

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/62988/#review64978

> This change looks odd - the only name referenced in keywordOrEntry is "keyword" - I'm expecting to see a reference to keywordOrEntry.source

It's used in https://reviewboard.mozilla.org/r/51873/diff/10#chunk13.13, but not here. Although, I think we can still pass `keywordOrEntry.source` to the observer for the keyword change.

> Why isn't source an arg here like everywhere else? Same question below.

You bring up a good point. I only added source for methods that are called by `PlacesSyncUtils` and import, and neither calls `moveItem`. But it's not at all obvious why some methods take a source and others don't, so I'll add it for consistency.

> don't the changes here require a change to nsITaggingService.idl (ie, with a new optional arg specified and the implementation here allowing a default?

Hmm, I did change `nsITaggingService.idl` in this commit. Was your comment meant for a different line? Good point on making the default more explicit in the implementation. I think this works now because XPConnect will pass 0 for optional numeric arguments, and `SOURCE_DEFAULT = 0`.

Comment 17

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/62988/#review62554

Thanks very much for taking a look, Mak! I'll incorporate the feedback from you and Mark.

Comment 18

[Lina Butler [:lina]]

Attachment: Bug 1285408 - Update bookmark observer tests.

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

Comment 19

[Lina Butler [:lina]]

Comment on attachment 8769006 [details]
Bug 1285408 - Add change source tracking for bookmarks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62988/diff/6-7/

Comment 20

[Lina Butler [:lina]]

Comment on attachment 8771710 [details]
Bug 1285408 - Add source tracking for bookmark import and restore.

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

Comment 21

[Lina Butler [:lina]]

Comment on attachment 8769006 [details]
Bug 1285408 - Add change source tracking for bookmarks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62988/diff/7-8/

Comment 22

[Lina Butler [:lina]]

Comment on attachment 8771710 [details]
Bug 1285408 - Add source tracking for bookmark import and restore.

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

Comment 23

[Lina Butler [:lina]]

Comment on attachment 8777803 [details]
Bug 1285408 - Update bookmark observer tests.

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

Comment 24

[Lina Butler [:lina]]

Comment on attachment 8769006 [details]
Bug 1285408 - Add change source tracking for bookmarks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62988/diff/8-9/

Comment 25

[Lina Butler [:lina]]

Comment on attachment 8771710 [details]
Bug 1285408 - Add source tracking for bookmark import and restore.

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

Comment 26

[Lina Butler [:lina]]

Comment on attachment 8777803 [details]
Bug 1285408 - Update bookmark observer tests.

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

Comment 27

[Lina Butler [:lina]]

Comment on attachment 8769006 [details]
Bug 1285408 - Add change source tracking for bookmarks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62988/diff/9-10/

Comment 28

[Lina Butler [:lina]]

Comment on attachment 8771710 [details]
Bug 1285408 - Add source tracking for bookmark import and restore.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64760/diff/4-5/

Comment 29

[Lina Butler [:lina]]

Comment on attachment 8777803 [details]
Bug 1285408 - Update bookmark observer tests.

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

Comment 30

[Lina Butler [:lina]]

Comment on attachment 8769006 [details]
Bug 1285408 - Add change source tracking for bookmarks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62988/diff/10-11/

Comment 31

[Lina Butler [:lina]]

Comment on attachment 8771710 [details]
Bug 1285408 - Add source tracking for bookmark import and restore.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64760/diff/5-6/

Comment 32

[Lina Butler [:lina]]

Comment on attachment 8777803 [details]
Bug 1285408 - Update bookmark observer tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69266/diff/4-5/

Comment 33

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=489ee59a788c

Comment 34

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 8769006 [details]
Bug 1285408 - Add change source tracking for bookmarks.

https://reviewboard.mozilla.org/r/62988/#review68202

This is looking good!

::: toolkit/components/places/Bookmarks.jsm:462
(Diff revision 11)
>    eraseEverything: function() {
>      return PlacesUtils.withConnectionWrapper("Bookmarks.jsm: eraseEverything",
>        db => db.executeTransaction(function* () {
>          const folderGuids = [this.toolbarGuid, this.menuGuid, this.unfiledGuid];
> -        yield removeFoldersContents(db, folderGuids);
> +        yield removeFoldersContents(db, {
> +          source: this.SOURCES.DEFAULT,

ISTM that Sync will later want to call this method (And reorder below), but will be unable to specify the source. But I guess that's something we can address later.

::: toolkit/components/places/Bookmarks.jsm:1094
(Diff revision 11)
>  }
>  
>  ////////////////////////////////////////////////////////////////////////////////
>  // Remove implementation.
>  
> -function removeBookmark(item, options) {
> +function removeBookmark(info, item, options) {

IIUC, info and item are subtly different representations of the item being removed, when in practice, we are using it just to carry the "source" around. ISTM that it might be better to be explicit here and either replace item with just the source, or possibly carry the source in options?

::: toolkit/components/places/PlacesSyncUtils.jsm:179
(Diff revision 11)
> +    });
>    }),
>  
>    /**
>     * Removes a folder's children. This is a temporary method that can be
>     * replaced by `eraseEverything` once Places supports the Sync-specific

as above: "and once eraseEverything allows us to specify a source" ;)

::: toolkit/components/places/PlacesUtils.jsm:2209
(Diff revision 11)
>                                          typeof(keywordEntry.postData) != "string")
>        throw new Error("Invalid POST data");
>      if (!("url" in keywordEntry))
>        throw new Error("undefined is not a valid URL");
> -    let { keyword, url } = keywordEntry;
> +    let { keyword, url,
> +          source = Ci.nsINavBookmarksService.SOURCE_DEFAULT } = keywordEntry;

huh - TIL that this synctax exists!

::: toolkit/components/places/nsAnnotationService.cpp:1460
(Diff revision 11)
>  
>    return NS_OK;
>  }
>  
>  
>  NS_IMETHODIMP

why no source param here and below?

::: toolkit/components/places/nsINavBookmarksService.idl:71
(Diff revision 11)
>                     in nsIURI aURI,
>                     in AUTF8String aTitle,
>                     in PRTime aDateAdded,
>                     in ACString aGuid,
> -                   in ACString aParentGuid);
> +                   in ACString aParentGuid,
> +                   in unsigned short aSource);

should these be specified as "optional"?

::: toolkit/components/places/nsNavBookmarks.cpp:1440
(Diff revision 11)
>                                   bookmark.type,
>                                   bookmark.parentId,
>                                   bookmark.guid,
>                                   bookmark.parentGuid,
> -                                 EmptyCString()));
> +                                 EmptyCString(),
> +                                 SOURCE_DEFAULT));

it still seems a little odd that these functions don't allow the source to be specified - although OTOH these are effectively deprecated, so I guess I don't care that much.

Comment 35

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 8771710 [details]
Bug 1285408 - Add source tracking for bookmark import and restore.

https://reviewboard.mozilla.org/r/64760/#review68204

This seems a little odd, in that much of the patch looks like it should really be in part 1, and the new source values introduced here aren't actually used yet. I'd be inclined to roll this up into part1, although I'm not too bothered if you'd prefer to keep them separate.

Comment 36

[Lina Butler [:lina]]

Comment on attachment 8769006 [details]
Bug 1285408 - Add change source tracking for bookmarks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62988/diff/11-12/

Comment 37

[Lina Butler [:lina]]

Comment on attachment 8769006 [details]
Bug 1285408 - Add change source tracking for bookmarks.

https://reviewboard.mozilla.org/r/62988/#review68202

> ISTM that Sync will later want to call this method (And reorder below), but will be unable to specify the source. But I guess that's something we can address later.

Good point. Let's just add it, while I'm here. :-)

> IIUC, info and item are subtly different representations of the item being removed, when in practice, we are using it just to carry the "source" around. ISTM that it might be better to be explicit here and either replace item with just the source, or possibly carry the source in options?

Makes sense! I passed the full `info` because that's what `insertBookmark` and `updateBookmark` do, but I agree passing them as part of the options is more explicit, and consistent with `reorder` and `eraseEverything`.

> huh - TIL that this synctax exists!

It just shipped earlier this year, I think!

> why no source param here and below?

Added.

> should these be specified as "optional"?

I don't think they need to be optional for the observers. JS code can omit the extra function param, and C++ code needs to specify it either way.

> it still seems a little odd that these functions don't allow the source to be specified - although OTOH these are effectively deprecated, so I guess I don't care that much.

`NotifyItemChanged` doesn't take a source because it's only called for history changes, and we don't track sources for history. I'll add a comment to that effect.

Comment 38

[Lina Butler [:lina]]

Comment on attachment 8771710 [details]
Bug 1285408 - Add source tracking for bookmark import and restore.

https://reviewboard.mozilla.org/r/64760/#review68204

Sorry, I'm not fully sure what you mean by "the new source values introduced here aren't actually used yet". SOURCE_SYNC isn't really used yet, either; at this point, they're just passed to the observers, which happily ignore them. They're only really used in bug 1258127.

I split them up mainly for ease of reviewing, but I agree this could be one patch instead of three. Squashed.

Comment 39

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 8769006 [details]
Bug 1285408 - Add change source tracking for bookmarks.

https://reviewboard.mozilla.org/r/62988/#review69036

Awesome job Kit - I'm really happy to see all this come together!

Land patch, go home! :)

::: toolkit/components/places/nsINavBookmarksService.idl:59
(Diff revision 12)
>     *        The stored date added value, in microseconds from the epoch.
>     * @param aGuid
>     *        The unique ID associated with the item.
>     * @param aParentGuid
>     *        The unique ID associated with the item's parent.
> +   * @param aSource

Might as well add "One of the nsINavWhatever:SOURCE\_ constants" or similar (here and anywhere similar, although there's probably no reason to bother where you document the default is SOURCE_DEFAULT, as that offers enough clues about the param)

::: toolkit/components/places/nsTaggingService.js:35
(Diff revision 12)
>    /**
>     * Creates a tag container under the tags-root with the given name.
>     *
>     * @param aTagName
>     *        the name for the new tag.
>     * @returns the id of the new tag container.

document aSource here.

::: toolkit/components/places/nsTaggingService.js:188
(Diff revision 12)
>     * Removes the tag container from the tags root if the given tag is empty.
>     *
>     * @param aTagId
>     *        the itemId of the tag element under the tags root
>     */
> -  _removeTagIfEmpty: function TS__removeTagIfEmpty(aTagId) {
> +  _removeTagIfEmpty: function TS__removeTagIfEmpty(aTagId, aSource) {

ditto

Comment 40

[Lina Butler [:lina]]

Comment on attachment 8769006 [details]
Bug 1285408 - Add change source tracking for bookmarks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62988/diff/12-13/

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f51506eaa048
Add change source tracking for bookmarks. r=markh

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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