958300 - UITour: give tour page ability to query browser for Sync configured/not configured state

Status: RESOLVED FIXED

(Firefox :: General, defect, P1)

Description

[:Gavin Sharp [email: [e-mail]]]

We'd like to limit Sync promotion in 29 to users who've not already set up Sync in versions prior to 29, for various reasons.

This means that if we have a "Hey, try out sync" in the Australis/29 whatsnew experience, it needs to be able to hide itself if the browser already has sync set up. That requires some way for the page to say "tell me whether sync is set up", and for the browser to respond with a yes/no.

Comment 1

[Marco Bonardo [:mak]]

is there a tracking bug for this Sync fadeout? Cause I suppose you may also want to hide the Sync promo links we show on the bookmarks and add-ons panels (fwiw there to detect Sync we just check Services.prefs.prefHasUserValue("services.sync.username")).

Comment 2

[:Gavin Sharp [email: [e-mail]]]

(In reply to Marco Bonardo [:mak] from comment #1)
> is there a tracking bug for this Sync fadeout? Cause I suppose you may also
> want to hide the Sync promo links we show on the bookmarks and add-ons
> panels (fwiw there to detect Sync we just check
> Services.prefs.prefHasUserValue("services.sync.username")).

The panel promo links don't show up for users with sync already set up, right? So we don't need to worry about that, but it is worth worrying about making sure those panel links work correctly in the new world, and also worth considering re-setting browser.syncPromoViewsLeftMap (annoying that it looks like we can't differentiate "user clicked the X" and "we showed the notification 5 times already").

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Filed bug 960047 for that.

Comment 4

[Matthew N. [:MattN]]

Attachment: v.1 getSyncConfiguration which checks services.sync.username

Mark, can you confirm that the existing of the services.sync.username pref will still be enough with FxA sync?

Comment 5

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

Comment on attachment 8361140 [details] [diff] [review]
v.1 getSyncConfiguration which checks services.sync.username

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

The "is sync configured" check of the username LGTM

::: browser/modules/test/browser.ini
@@ +9,5 @@
>  [browser_UITour.js]
>  skip-if = os == "linux" # Intermittent failures, bug 951965
>  [browser_UITour2.js]
>  [browser_UITour3.js]
> +[browser_UITour_sync.js]

is this file missing from the patch?

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8361140 [details] [diff] [review]
v.1 getSyncConfiguration which checks services.sync.username

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

I think a test file is sitting lonely on your hardrive somewhere.

::: browser/modules/UITour.jsm
@@ +235,5 @@
>          this.endUrlbarCapture(window);
>          break;
>        }
> +
> +      case "getSyncConfiguration": {

Make this "getConfiguration", and have the page pass in what they want to know (uitour.js library API can stay the same). Otherwise we're going to end up with several verbs like this, and a ton of boilerplate code.

Comment 7

[Matthew N. [:MattN]]

Attachment: v.2 getConfiguration with "sync" argument. r=mhammond

(In reply to Blair McBride [:Unfocused] from comment #6)
> I think a test file is sitting lonely on your hardrive somewhere.

Sorry, I attached that during a meeting so didn't double-check all changes were included. Carrying-forward r=mhammond on the pref checking.

Comment 8

[Matthew N. [:MattN]]

I believe we want to have the tour ready to test with users on Aurora so I don't think it's a good candidate for an Aurora fix.

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8361393 [details] [diff] [review]
v.2 getConfiguration with "sync" argument. r=mhammond

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

r+ with the following fix:

::: browser/modules/UITour.jsm
@@ +236,5 @@
>          break;
>        }
> +
> +      case "getConfiguration": {
> +        this.getConfiguration(contentDocument, data.configuration, data.callbackID);

We need to be paranoid about the data coming from these events - bail out early here if data.configuration isn't a string.

Comment 10

[Matthew N. [:MattN]]

(In reply to Blair McBride [:Unfocused] from comment #9)
> Comment on attachment 8361393 [details] [diff] [review]
> v.2 getConfiguration with "sync" argument. r=mhammond
>
> We need to be paranoid about the data coming from these events - bail out
> early here if data.configuration isn't a string.

OK, typeof check added.

https://hg.mozilla.org/integration/fx-team/rev/4129ed8dfd2b

Comment 11

[Matthew N. [:MattN]]

Attachment: Library pull request

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4129ed8dfd2b