Closed Bug 716643 Opened 12 years ago Closed 12 years ago

Sync promotion in add-on installed doorhanger

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: philikon, Assigned: theo)

References

Details

(Whiteboard: [good first bug][lang=xul][mentor=???])

Attachments

(3 files, 6 obsolete files)

When you add a bookmark or password and don't have Sync set up, we attach a little box to the corresponding doorhanger advertising Sync (see bug 618913). IMHO we should do the same for the add-on doorhanger.

In fact, we can go further than that. Because the add-ons engine is disabled by default for existing users, so if you're an existing user and haven't enabled add-on sync yet, we can also show something along the lines of "ohai btw you now can haz add-on sync".
Attached patch Patch V1 (obsolete) — Splinter Review
PatchV1 - The promobox is added to addon-install-complete popup when Sync isn't set.

I'm watching to add a promobox when Sync is set.
Attachment #600018 - Flags: feedback?(philipp)
Comment on attachment 600018 [details] [diff] [review]
Patch V1

Nice!

403 mak who implemented the original promotion stuff
Attachment #600018 - Flags: review?(mak77)
Attachment #600018 - Flags: feedback?(philipp)
Attachment #600018 - Flags: feedback+
Attached patch Patch V2 (obsolete) — Splinter Review
Well, I think it's okay.
I'm not sure about the sentence "You can enable add-ons sync in %S preferences.", maybe it could be improved.

FYI, I launch a test build.
Attachment #600018 - Attachment is obsolete: true
Attachment #600039 - Flags: review?(mak77)
Attachment #600018 - Flags: review?(mak77)
Comment on attachment 600018 [details] [diff] [review]
Patch V1

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

The patch is fine, though I have some questions that I'd like being answered before final review:
1. did you check that the panel aspect is fine? May we get a screenshot?
2. did you run this through tryserver? My fear is that some addon manager tests may rely on the panels contents and be confused (ideally should not happen, but better being sure)

While the following one is for Sync team:
3. this will clearly be visible only to new users since existing users by now will likely have expired all impressions, is that fine? do we maybe want to count impressions for each notification type, so when adding new ones they will appear? If so, please file a separate bug to make it happen.
Attachment #600018 - Attachment is obsolete: false
ehr, looks like I somehow reviewed the wrong patch
Attached image Screenshot Patch V1
Here is a screenshot of patch V1, given by Try-server.
For more safety, I'll run a build on my machine too for the patch V2 screenshot.

Thanks for your feedback :)
Attachment #600018 - Attachment is obsolete: true
Comment on attachment 600039 [details] [diff] [review]
Patch V2

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

::: browser/base/content/urlbarBindings.xml
@@ +1426,5 @@
> +              Services.prefs.getBoolPref("services.sync.engine.addons") &&
> +              this._notificationType == "addons") {
> +            addonsSyncEngineDisabled = true;
> +            this._notificationType = "addons-sync-engine-disabled";
> +          }

you should handle this in the the notificationType property getter and return a different type from there. you don't want to use this addonsSyncEngineDisabled variable, you can just check the notificationType later.

@@ +1464,5 @@
>  
>            let viewsLeft = this._viewsLeft;
>            if (viewsLeft) {
> +            if (Services.prefs.prefHasUserValue("services.sync.username") &&
> +               !addonsSyncEngineDisabled) {

check notificationType here instead

@@ +1481,5 @@
> +              this._promolink.setAttribute("href", "https://support.mozilla.org/kb/how-do-i-enable-add-sync");
> +            }
> +            else {
> +              this._promolink.setAttribute("href", "https://services.mozilla.com/sync/");
> +            }

you may want to add a _notificationLink property that returns the address based on notificationType

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +326,5 @@
> +# the add-on install complete panel when Sync isn't set.  %S will be replaced by syncBrandShortName.
> +# The final space separates this text from the Learn More link.
> +syncPromoNotification.addons.description=You can access your add-ons on all your devices with %S.\u0020
> +# LOCALIZATION NOTE (syncPromoNotification.addons-sync-engine-disabled.label): This appears in
> +# the add-on install complete panel when Sync is set but addons sync isn't set.  %S will be replaced by syncBrandShortName.

crop to 80 chars (or whatever is the width in this file atm), looks like this need an additional newline.
Attached patch Patch V3 (obsolete) — Splinter Review
Thanks for your fast review! Here are the changes.
 
I'll launch a build to provide a screenshot of addons-sync-engine-disabled, and verify that's working.
Attachment #600039 - Attachment is obsolete: true
Attachment #600099 - Flags: review?(mak77)
Attachment #600039 - Flags: review?(mak77)
(In reply to Marco Bonardo [:mak] from comment #4)
> While the following one is for Sync team:
> 3. this will clearly be visible only to new users since existing users by
> now will likely have expired all impressions, is that fine? do we maybe want
> to count impressions for each notification type, so when adding new ones
> they will appear? If so, please file a separate bug to make it happen.

Since add-on sync is disabled by default for existing Sync users, we would like some kind of mechanism to facilitate add-on sync discovery for existing users. In fact, this bug was originally filed with that specifically in mind! If we need a follow-up, so be it.
(In reply to Gregory Szorc [:gps] from comment #9)
> Since add-on sync is disabled by default for existing Sync users, we would
> like some kind of mechanism to facilitate add-on sync discovery for existing
> users. In fact, this bug was originally filed with that specifically in
> mind! If we need a follow-up, so be it.

Yes in my opinion is better to handle the 2 issues separately, fix this and then fix the impressions count.
Attached image Screenshot patch V3 (obsolete) —
Promo box when Sync is set, but add-on sync isn't.

Maybe we can say something like "You can now activate $S for your add-ons! Check it in $S preferences."
Comment on attachment 600170 [details]
Screenshot patch V3

Nice! Though IMHO the string would be better if it said something like "Firefox Sync can synchronize your add-ons across multiple computers. _Learn more_"
(In reply to Philipp von Weitershausen [:philikon] from comment #12)
> Comment on attachment 600170 [details]
> Screenshot patch V3
> 
> Nice! Though IMHO the string would be better if it said something like
> "Firefox Sync can synchronize your add-ons across multiple computers. _Learn
> more_"

It's more or less the same thing that we say to enable Sync: "You can access your add-ons on all your devices with %S." I think we need to indicate to the user that he have to see the pref pane to activate it. Moreover he could think Sync isn't enabled if he see the same message. What do you think?
(In reply to Théo Chevalier [:tchevalier] from comment #13)

> It's more or less the same thing that we say to enable Sync: "You can access
> your add-ons on all your devices with %S." I think we need to indicate to
> the user that he have to see the pref pane to activate it. Moreover he could
> think Sync isn't enabled if he see the same message. What do you think?

The reason for a "Learn More" link is that prefs is differently named and accessed differently on different platforms, and we might want the opportunity to explain exactly what add-on sync will entail. (For example, AMO only, no syncing between mobile and desktop...)
(In reply to Richard Newman [:rnewman] from comment #14)
> The reason for a "Learn More" link is that prefs is differently named and
> accessed differently on different platforms, and we might want the
> opportunity to explain exactly what add-on sync will entail. (For example,
> AMO only, no syncing between mobile and desktop...)

Of course, the Learn More link stay after the sentence. We just want find a good sentence, but we dont want to say exactly the same thing that in attachment 600045 [details].
Comment on attachment 600099 [details] [diff] [review]
Patch V3

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

Looks good, thank you for fixing all comments.

Still needs a Try run, and please get a final decision on the string :)

::: browser/base/content/urlbarBindings.xml
@@ +1417,5 @@
> +          if (type == "addon-install-complete") {
> +            if (!Services.prefs.prefHasUserValue("services.sync.username"))
> +              return "addons";
> +            if (!Services.prefs.getBoolPref("services.sync.engine.addons"))
> +              return "addons-sync-engine-disabled";

I think -engine- is a useless part here, addons-sync-disabled is descriptive enough without falling into implementation details

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +328,5 @@
> +# The final space separates this text from the Learn More link.
> +syncPromoNotification.addons.description=You can access your add-ons on all your devices with %S.\u0020
> +# LOCALIZATION NOTE (syncPromoNotification.addons-sync-engine-disabled.label):
> +# This appears in the add-on install complete panel when Sync is set
> +# and addons sync isn't set. %S will be replaced by syncBrandShortName.

s/and/but/
s/isn't set/is not/
Attachment #600099 - Flags: review?(mak77) → review+
Attached patch Patch V4 (obsolete) — Splinter Review
Contains the last changes.

I propose "You can activate Sync to synchronize your add-ons across multiple computers. _Learn More_"

Try-build running with this patch, will be available here: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/theo.chevalier11@gmail.com-7062be72f513
Attachment #600099 - Attachment is obsolete: true
Attachment #600397 - Flags: review?(mak77)
(In reply to Théo Chevalier [:tchevalier] from comment #17)
> I propose "You can activate Sync to synchronize your add-ons across multiple
> computers. _Learn More_"

is not Sync already setup here, and just the add-ons engine disabled? sounds confusing then
(In reply to Marco Bonardo [:mak] from comment #18)
> (In reply to Théo Chevalier [:tchevalier] from comment #17)
> > I propose "You can activate Sync to synchronize your add-ons across multiple
> > computers. _Learn More_"
> 
> is not Sync already setup here, and just the add-ons engine disabled? sounds
> confusing then

hmmm, yeah. What about "You can activate add-ons synchronization in Sync. _Learn More_"?
>Your Try Server test (7062be72f513) failed to complete on builder try_xp_test-jetpack.
>Summary of test results:
>
>jetpack
>5193/1
>
>The full log for this test run is available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/theo.chevalier11@gmail.com-7062be72f513/try-win32/try_xp_test-jetpack-build2048.txt.gz.

Marco - Is that you were talking about?
> some addon manager tests may rely on the panels contents and be confused
could you please post the link to the tbpl for the push?
that may just be a random failure.
(In reply to Marco Bonardo [:mak] from comment #21)
> could you please post the link to the tbpl for the push?
> that may just be a random failure.

Yes, sorry. https://tbpl.mozilla.org/?tree=Try&rev=7062be72f513
yeah, this is not your fault, error: TEST FAILED: test-xhr.testDelegatedReturns so you can ignore it.
Comment on attachment 600397 [details] [diff] [review]
Patch V4

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

r=me with the following change to the phrase (and agreement from someone in the Sync team on that).

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +330,5 @@
> +# LOCALIZATION NOTE (syncPromoNotification.addons-sync-disabled.label):
> +# This appears in the add-on install complete panel when Sync is set
> +# but addons sync is not. %S will be replaced by syncBrandShortName.
> +# The final space separates this text from the Learn More link.
> +syncPromoNotification.addons-sync-disabled.description=You can activate %S to synchronize your add-ons across multiple computers.\u0020

hm, maybe s/activate/configure/, or setup? (I'm not English mother tongue, so I'm not sure which of the 2 sounds better, Richard is surely a better target than me for this question!)
Attachment #600397 - Flags: review?(mak77) → review+
rnewman, Hi, do you think you'll have the time soon to check this sentence, and tell us if the screenshots looks good for you, please?
Assignee: nobody → theo.chevalier11
Status: NEW → ASSIGNED
Sorry, this one slipped past me.

The wording we've used in the past is "set up Sync", so I suggest that instead of "activate":

syncPromoNotification.addons-sync-disabled.description=You can set up %S to synchronize your add-ons across multiple computers.\u0020

Otherwise, looks good, thanks!
Boriss, fligtar: Can you please give your blessing?
(In reply to Richard Newman [:rnewman] from comment #26)
> syncPromoNotification.addons-sync-disabled.description=You can set up %S to
> synchronize your add-ons across multiple computers.\u0020

I wonder if the string should be clearer that users already have an activated sync account.  Otherwise, it may appear that sync isn't set up or isn't working.  How about "You can use your current Firefox Sync account to synchronize your add-ons across multiple computers. _Learn more_"
Attached patch Patch V5 (obsolete) — Splinter Review
Patch updated regarding last comment.
I'll update the screenshot when Sync is set up, and Add-ons sync isn't, once try is built.
Attachment #600170 - Attachment is obsolete: true
Attachment #600397 - Attachment is obsolete: true
Attachment #611482 - Flags: review?(mak77)
Hm... I've to find how to replace %S by syncBrand.fullName.label. I don't know how it currently works.
(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #28)
> How about "You can use your current Firefox Sync account to
> synchronize your add-ons across multiple computers. _Learn more_"

I think "current" is over-engineered there, do past or future accounts matter? I'd just go for "your Firefox Sync account".  As well as I'd omit the "your" in "synchronize your add-ons". So that it sounds like

"You can use your Firefox Sync account to synchronize add-ons across multiple computers. _Learn more_"
Boriss?
Comment on attachment 611482 [details] [diff] [review]
Patch V5

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

This is valid regardless the phrase, you don't need another review just to change wording.
But, you still need the other bug to show this promo to users that already saw the original "set up sync" promo.
Attachment #611482 - Flags: review?(mak77) → review+
(In reply to Marco Bonardo [:mak] from comment #32)
> Comment on attachment 611482 [details] [diff] [review]
> Patch V5
> 
> Review of attachment 611482 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is valid regardless the phrase, you don't need another review just to
> change wording.

Hum, on my try build, %S has been replaced by syncBrandShortName, not by syncBrand.fullName.label. Is this will be done manually by localizers, or I should do something else?

> But, you still need the other bug to show this promo to users that already
> saw the original "set up sync" promo.

Ho yeah missed that part. I'll file another bug to fix it.
(In reply to Théo Chevalier [:tchevalier] from comment #33)
> Hum, on my try build, %S has been replaced by syncBrandShortName, not by
> syncBrand.fullName.label. Is this will be done manually by localizers, or I
> should do something else?

ehr, let me check, unfortunately bugzilla interdiff doesn't work, I thought you just had changed the prase :/

you have to special case _notificationMessage getter if you want to use syncBrand.fullName.label for this specific case, cause it generically replaces all %S with syncBrandShortName now.
that said, may we just use Sync here as we did in all the other messages? I don't think we should add complication
So imo we should just go for

"You can use your Sync account to synchronize add-ons across multiple computers. _Learn more_"

it's short and clear, doesn't add complications :)
Depends on: 741738
Attached patch Patch V6Splinter Review
Yeah, you probably right :)

(Filed Bug 741738 about impressions count)
Attachment #611482 - Attachment is obsolete: true
Screenshot updated with the final wording.
What's the status of this patch? Did this get ui-review?
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #39)
> What's the status of this patch? Did this get ui-review?

Yes, UX team (at least :boriss) is okay, but to land it we need bug 741738, I'm currently on it
Backed out https://hg.mozilla.org/integration/mozilla-inbound/rev/027dc449dee8
Because we should use "devices" instead of "computers"

Pushed to inbound again:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6e8ac5b0d7b
https://hg.mozilla.org/mozilla-central/rev/d6e8ac5b0d7b
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.