Closed
Bug 716643
Opened 13 years ago
Closed 12 years ago
Sync promotion in add-on installed doorhanger
Categories
(Toolkit :: Add-ons Manager, defect)
Toolkit
Add-ons Manager
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: philikon, Assigned: theo)
References
Details
(Whiteboard: [good first bug][lang=xul][mentor=???])
Attachments
(3 files, 6 obsolete files)
52.37 KB,
image/jpeg
|
Details | |
5.62 KB,
patch
|
Details | Diff | Splinter Review | |
56.30 KB,
image/jpeg
|
Details |
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".
Assignee | ||
Comment 1•12 years ago
|
||
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)
Reporter | ||
Comment 2•12 years ago
|
||
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+
Assignee | ||
Comment 3•12 years ago
|
||
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 4•12 years ago
|
||
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
Comment 5•12 years ago
|
||
ehr, looks like I somehow reviewed the wrong patch
Assignee | ||
Comment 6•12 years ago
|
||
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 :)
Updated•12 years ago
|
Attachment #600018 -
Attachment is obsolete: true
Comment 7•12 years ago
|
||
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.
Assignee | ||
Comment 8•12 years ago
|
||
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)
Comment 9•12 years ago
|
||
(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.
Comment 10•12 years ago
|
||
(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.
Assignee | ||
Comment 11•12 years ago
|
||
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."
Reporter | ||
Comment 12•12 years ago
|
||
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_"
Assignee | ||
Comment 13•12 years ago
|
||
(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?
Comment 14•12 years ago
|
||
(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...)
Assignee | ||
Comment 15•12 years ago
|
||
(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 16•12 years ago
|
||
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+
Assignee | ||
Comment 17•12 years ago
|
||
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)
Comment 18•12 years ago
|
||
(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
Assignee | ||
Comment 19•12 years ago
|
||
(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_"?
Assignee | ||
Comment 20•12 years ago
|
||
>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
Comment 21•12 years ago
|
||
could you please post the link to the tbpl for the push? that may just be a random failure.
Assignee | ||
Comment 22•12 years ago
|
||
(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
Comment 23•12 years ago
|
||
yeah, this is not your fault, error: TEST FAILED: test-xhr.testDelegatedReturns so you can ignore it.
Comment 24•12 years ago
|
||
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+
Assignee | ||
Comment 25•12 years ago
|
||
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
Comment 26•12 years ago
|
||
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!
Comment 27•12 years ago
|
||
Boriss, fligtar: Can you please give your blessing?
Comment 28•12 years ago
|
||
(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_"
Assignee | ||
Comment 29•12 years ago
|
||
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)
Assignee | ||
Comment 30•12 years ago
|
||
Hm... I've to find how to replace %S by syncBrand.fullName.label. I don't know how it currently works.
Comment 31•12 years ago
|
||
(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 32•12 years ago
|
||
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+
Assignee | ||
Comment 33•12 years ago
|
||
(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.
Comment 34•12 years ago
|
||
(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.
Comment 35•12 years ago
|
||
that said, may we just use Sync here as we did in all the other messages? I don't think we should add complication
Comment 36•12 years ago
|
||
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 :)
Assignee | ||
Comment 37•12 years ago
|
||
Yeah, you probably right :) (Filed Bug 741738 about impressions count)
Attachment #611482 -
Attachment is obsolete: true
Assignee | ||
Comment 38•12 years ago
|
||
Screenshot updated with the final wording.
Comment 39•12 years ago
|
||
What's the status of this patch? Did this get ui-review?
Assignee | ||
Comment 40•12 years ago
|
||
(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
Assignee | ||
Comment 41•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/efeaf7b68f58
Comment 42•12 years ago
|
||
Sorry, I backed out these patches because they caused exceptions in mochitest-browser-chrome: https://hg.mozilla.org/integration/mozilla-inbound/rev/04d508016743 https://tbpl.mozilla.org/php/getParsedLog.php?id=13809261&tree=Mozilla-Inbound
Assignee | ||
Comment 43•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7bb5f1ebb833
Assignee | ||
Comment 44•12 years ago
|
||
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
Comment 45•12 years ago
|
||
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.
Description
•