1103361 - Start/stop sync tracker when engine is enabled/disabled by flipping the pref

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Hector Zhao [:hectorz]]

+++ This bug was initially created as a clone of Bug #947772 +++

In bug 947772, |onEngineEnabledChanged| of a sync tracker will only be called if the corresponding sync engine is enabled/disabled by setting its |enabled| property, not when the underlying pref is flipped.

Comment 1

[Hector Zhao [:hectorz]]

Attachment: Patch

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a7567d90b5bd

Comment 2

[Richard Newman [:rnewman]]

I'm not sure why this is necessary. Sync always calls through .enabled when changing state. Could you explain?

Comment 3

[Hector Zhao [:hectorz]]

(In reply to Richard Newman [:rnewman] from comment #2)
> I'm not sure why this is necessary. Sync always calls through .enabled when
> changing state. Could you explain?

I think toggling an engine/item in sync pane of preferences doesn't go through .enabled.

STR:
1. Start Fx with prefs engine enabled;
2. Uncheck the prefs engine in preferences;
3. |Weave.Service.engineManager.get("prefs").enabled| is false;
4. |Weave.Service.engineManager.get("prefs")._tracker._isTracking| is true.

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 8527182 [details] [diff] [review]
Patch

Review of attachment 8527182 [details] [diff] [review]:
-----------------------------------------------------------------

Reasoning behind moving this to the tracker: the tracker is already watching for this kind of thing, and the thing we want to poke is in the tracker.

::: services/sync/modules/engines.js
@@ +645,5 @@
>      this.__defineGetter__("_tracker", function() tracker);
>      return tracker;
>    },
>  
> +  observe: function(subject, topic, data) {

Each of the trackers already observes, so I think what you want to do is add a line like:

  Svc.Prefs.observe("engine". + this.name, this);

to the Tracker constructor, and then move the new logic here to the existing observe handler in Tracker.

@@ +648,5 @@
>  
> +  observe: function(subject, topic, data) {
> +    switch (topic) {
> +      case "nsPref:changed":
> +        let engine = data.slice((PREFS_BRANCH + "engine.").length);

This isn't right. I think you want (inside the tracker):

  if (data == PREFS_BRANCH + "engine." + this.name) {
    this.onEngineEnabledChanged(...);
  }

Comment 5

[Hector Zhao [:hectorz]]

Attachment: Patch, moved to tracker

(In reply to Richard Newman [:rnewman] from comment #4)
> Each of the trackers already observes, so I think what you want to do is add
> a line like:
> 
>   Svc.Prefs.observe("engine". + this.name, this);
> 
> to the Tracker constructor, and then move the new logic here to the existing
> observe handler in Tracker.
Done.
> 
> This isn't right. I think you want (inside the tracker):
> 
>   if (data == PREFS_BRANCH + "engine." + this.name) {
>     this.onEngineEnabledChanged(...);
>   }
Fixed.

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 8534136 [details] [diff] [review]
Patch, moved to tracker

Review of attachment 8534136 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! Please make sure you've tested this thoroughly before landing.

::: services/sync/modules/engines.js
@@ +53,5 @@
>    this.loadChangedIDs();
>  
>    Svc.Obs.add("weave:engine:start-tracking", this);
>    Svc.Obs.add("weave:engine:stop-tracking", this);
> +  

Nit: trailing whitespace.

Comment 7

[Hector Zhao [:hectorz]]

Attachment: Patch, remove trailing whitespaces

(In reply to Richard Newman [:rnewman] from comment #6)
> 
> Looks good! Please make sure you've tested this thoroughly before landing.
Sorry but I'm not quite sure what kind of tests are considered thorough, could you be more specific? Should I run more tests (other than xpcshell) on try, like tps tests? Or should I manually confirm a try build works as expected? Thanks.
> 
> Nit: trailing whitespace.
Removed. Carrying over r+.

Comment 8

[Richard Newman [:rnewman]]

Mostly looking for smoke tests: run the build locally, and make sure that changes are tracked when turned on, not tracked when turned off, recovered and tracked again when turned back on.

Comment 9

[Hector Zhao [:hectorz]]

(In reply to Richard Newman [:rnewman] from comment #8)
> Mostly looking for smoke tests: run the build locally, and make sure that
> changes are tracked when turned on, not tracked when turned off, recovered
> and tracked again when turned back on.

Thanks for the clarification. Will do before requesting checkin.

Comment 10

[Hector Zhao [:hectorz]]

(In reply to Richard Newman [:rnewman] from comment #8)
> Mostly looking for smoke tests: run the build locally, and make sure that
> changes are tracked when turned on, not tracked when turned off, recovered
> and tracked again when turned back on.

I tested the try build (w/o the whitespace fix) and it works as expected.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b0f4f21987a1

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9772a3da28b8