1299338 - Replace `ignoreAll` with change source checks in the bookmarks engine

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

[Lina Butler [:lina]]

Now that PlacesSyncUtils tags its own changes with SOURCE_SYNC, we don't need to disable tracking when Sync is running. Instead, we can check the source in the observer, and ignore changes made by applying incoming bookmarks.

The devil's in the details, of course, but this should be a big incremental win for missing changes during a sync, until bug 1258127 lands.

Comment 1

[Lina Butler [:lina]]

Attachment: Bug 1299338 - Replace `ignoreAll` with change source checks in the Sync bookmarks engine.

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

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8788697 [details]
Bug 1299338 - Replace `ignoreAll` with change source checks in the Sync bookmarks engine.

https://reviewboard.mozilla.org/r/77096/#review75322

Looks good and is a high impact change, so I'm really happy to see this :)

::: services/sync/modules/engines/bookmarks.js:912
(Diff revision 1)
>  BookmarksTracker.prototype = {
>    __proto__: Tracker.prototype,
>  
> +  // The tracker checks the change source for each observer notification, so we
> +  // don't want to let it ignore all changes during a sync.
> +  get ignoreAll() {

Do we actually need these changes? It looks like only the tracker checks this value, and your changes mean it no longer does.

(I'm not too bothered by this, but wouldn't mind understanding it)

::: services/sync/modules/engines/bookmarks.js:1101
(Diff revision 1)
>    // *each change*.
>    onItemChanged: function BMT_onItemChanged(itemId, property, isAnno, value,
>                                              lastModified, itemType, parentId,
> -                                            guid, parentGuid) {
> +                                            guid, parentGuid, source) {
>      // Quicker checks first.
> -    if (this.ignoreAll)
> +    if (this._shouldIgnoreSource(source)) {

TBH I don't really see why we need this "quicker checks first" block as all checks below before the `_ignore()` call seem fast enough. That would let you inline `_shouldIgnoreSource` directly in `_ignore`, which might reduce the complexity a little.

Again, not too bothered by this though...

Comment 3

[Lina Butler [:lina]]

Comment on attachment 8788697 [details]
Bug 1299338 - Replace `ignoreAll` with change source checks in the Sync bookmarks engine.

https://reviewboard.mozilla.org/r/77096/#review75322

> Do we actually need these changes? It looks like only the tracker checks this value, and your changes mean it no longer does.
> 
> (I'm not too bothered by this, but wouldn't mind understanding it)

The base engine sets (`doApplyBatch` and `_wipeClient`) and reads (`addChangedID`, `removeChangedID`) it, too. We could override those methods, too, but this feels neater.

> TBH I don't really see why we need this "quicker checks first" block as all checks below before the `_ignore()` call seem fast enough. That would let you inline `_shouldIgnoreSource` directly in `_ignore`, which might reduce the complexity a little.
> 
> Again, not too bothered by this though...

Great suggestion! Done.

Comment 4

[Lina Butler [:lina]]

Comment on attachment 8788697 [details]
Bug 1299338 - Replace `ignoreAll` with change source checks in the Sync bookmarks engine.

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/db9dfcdbef4a
Replace `ignoreAll` with change source checks in the Sync bookmarks engine. r=markh

Comment 6

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

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