Closed Bug 1304898 Opened 4 years ago Closed 2 years ago

Sync Ping should actually report "why"

Categories

(Firefox :: Sync, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Firefox 59
Tracking Status
firefox59 --- fixed

People

(Reporter: tcsc, Assigned: eoger)

Details

Attachments

(1 file)

Similar to bug 1304896, this is part of the spec and the schema, but is always left out since it is NYI.

It should report one of "startup", "schedule", "score", "user", "tabs" and indicate the cause of the sync.
Priority: -- → P3
Assignee: nobody → eoger
Status: NEW → ASSIGNED
Comment on attachment 8928717 [details]
Bug 1304898 - Include why in sync ping.

https://reviewboard.mozilla.org/r/199982/#review205094

Can you do a quick look for other places we could provide this relatively easily? You may need to/should change the schema to accept strings for "why" instead of an enum.

Specifically, other explicit calls to service.sync where we know the cause. I think https://searchfox.org/mozilla-central/search?q=Service.sync&path= indicates there's one in browser-pageActions we'd want to get anyway, and the browserid_identity one is probably reason: "login" or similar.

::: services/sync/modules/SyncedTabs.jsm:164
(Diff revision 1)
>        return false;
>      }
>      // Ask Sync to just do the tabs engine if it can.
>      try {
>        log.info("Doing a tab sync.");
> -      await Weave.Service.sync(["tabs"]);
> +      await Weave.Service.sync({engines: ["tabs"]});

reason: "tabs" is documented in telemetry, so we might as well provide it here.

::: services/sync/modules/service.js:436
(Diff revision 1)
>          // clients engine.
>          if (data.includes("clients") && !Svc.Prefs.get("testing.tps", false)) {
>            // Sync in the background (it's fine not to wait on the returned promise
>            // because sync() has a lock).
>            // [] = clients collection only
> -          this.sync([]).catch(e => {
> +          this.sync({engines: []}).catch(e => {

reason: "collection_changed" sounds good to me.
Attachment #8928717 - Flags: review?(tchiovoloni)
Comment on attachment 8928717 [details]
Bug 1304898 - Include why in sync ping.

https://reviewboard.mozilla.org/r/199982/#review205456

Thanks! I bugged you about most issues in IRC, and this seems to hae fixed all of them.
Attachment #8928717 - Flags: review?(tchiovoloni) → review+
https://hg.mozilla.org/mozilla-central/rev/79e949bc0f7b
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
You need to log in before you can comment on or make changes to this bug.