914927 - Change "Turn Off Provider-Name" in toolbar menu

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Shane Caraveo (:mixedpuppy)]

We now disable multiple providers, it doesn't make sense to specify only one.

Comment 1

[Shane Caraveo (:mixedpuppy)]

suggestion: "Disable all Services"

Boriss, any thoughts on this?

Comment 2

[Jennifer Morrow [:Boriss] (UX)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #1)
> suggestion: "Disable all Services"
> 
> Boriss, any thoughts on this?

You're right - Services rather than Providers makes sense.  "Turn off all Services" seems the most consistent string

Comment 3

[Shane Caraveo (:mixedpuppy)]

Attachment: change menu string

Comment 4

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 803883 [details] [diff] [review]
change menu string

|| markh

Comment 5

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

Comment on attachment 803883 [details] [diff] [review]
change menu string

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

Declining review for the moment as I don't think changing the menu to, eg, "Turn on all Services" when it doesn't actually turn on all services is particularly good.  I'd have thought we would want to keep the old strings around, and only use the new strings when (a) there is > 1 provider and (b) we allow multiple enabled providers.

The patch looks good otherwise - if Boriss weighs in and takes responsibility for this aspect of the change I'll r+ :)

Comment 6

[Shane Caraveo (:mixedpuppy)]

Attachment: change menu string

Updated with comments from markh with regard to string when there is only one provider.  Since share was introduced, more than one provider can actually show up in the UI, so I think the "all services" string is appropriate even without multiple workers being turned on.

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 804635 [details] [diff] [review]
change menu string

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

::: browser/base/content/browser-social.js
@@ +216,5 @@
>        let provider = Social.provider || Social.defaultProvider;
>        // We only need to update the command itself - all our menu items use it.
> +      let label;
> +      if (Social.providers.length == 1) {
> +        label = gNavigatorBundle.getFormattedString(Social.provider ?

not trying to nitpick, but the indentation confused me a bit. One suggestion that I find it clearer is to put the ? : below each other, like

> label = gNavigatorBundle.getFormattedString(Social.provider
>                                             ? "social.turnOff.label"
>                                             : "social.turnOn.label",

up to you

Comment 8

[Shane Caraveo (:mixedpuppy)]

Attachment: change menu string

feedback and try

https://tbpl.mozilla.org/?tree=Try&rev=fcba93f98273

Comment 9

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/fx-team/rev/1d12266ddc4c

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1d12266ddc4c

Comment 11

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/1c73bcaef990 while chasing bug 916757.

Feel free to reland after a debug Windows try push with like 30 browser-chrome runs on WinXP and Win8, since we're still not especially sure what got rid of it.

Comment 12

[Shane Caraveo (:mixedpuppy)]

new try for winxp/8

https://tbpl.mozilla.org/?tree=Try&rev=d5f1f29d2a43

winxp/8 try with bug 904104, bug  914926, bug 914927 and bug 914435

https://tbpl.mozilla.org/?tree=Try&rev=6d7e6966f505

Comment 13

[Shane Caraveo (:mixedpuppy)]

green try run at https://tbpl.mozilla.org/?tree=Try&rev=35545c71cc2a (one intermittent from newtab, but no crashes)

Comment 14

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 804734 [details] [diff] [review]
change menu string

relanded: https://hg.mozilla.org/integration/fx-team/rev/183f4c8a4b8f

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 916757
User impact if declined: misleading menu text
Testing completed (on m-c, etc.): fx-team
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: two new strings in browser.properties

This was backed out along with several others due to winXP crashes, I narrowed the crash down to bug 914435

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/183f4c8a4b8f

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Shane Caraveo (:mixedpuppy) from comment #14)
> Comment on attachment 804734 [details] [diff] [review]
> change menu string
> 
> relanded: https://hg.mozilla.org/integration/fx-team/rev/183f4c8a4b8f
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): 916757
> User impact if declined: misleading menu text
> Testing completed (on m-c, etc.): fx-team
> Risk to taking this patch (and alternatives if risky): low
> String or IDL/UUID changes made by this patch: two new strings in
> browser.properties
> 
> This was backed out along with several others due to winXP crashes, I
> narrowed the crash down to bug 914435

Any chance of doing this without string changes?  Not keen to break string freeze and I don't know how urgent it is to uplift this.

Comment 17

[Shane Caraveo (:mixedpuppy)]

(In reply to [e-mail] [:lsblakk] from comment #16)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #14)
> > Comment on attachment 804734 [details] [diff] [review]
> > change menu string
> > 
> > relanded: https://hg.mozilla.org/integration/fx-team/rev/183f4c8a4b8f
> > 
> > [Approval Request Comment]
> > Bug caused by (feature/regressing bug #): 916757
> > User impact if declined: misleading menu text
> > Testing completed (on m-c, etc.): fx-team
> > Risk to taking this patch (and alternatives if risky): low
> > String or IDL/UUID changes made by this patch: two new strings in
> > browser.properties
> > 
> > This was backed out along with several others due to winXP crashes, I
> > narrowed the crash down to bug 914435
> 
> Any chance of doing this without string changes?  Not keen to break string
> freeze and I don't know how urgent it is to uplift this.

No, the whole point is the string changes.

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

Axel - can this be broadcast to localizers to get attention on the late string change?  Alternately we could go with leaving it as is until FF27 which would mean only one release where there's a slightly off phrasing.

Comment 19

[Axel Hecht [:Pike]]

I'd not break string freeze for this, I don't think there's a lot of people actually using SocialAPI, let alone using multiple providers and removing them.

Also, I'm surprised that there's only one or all, is that really the case?

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Axel Hecht [:Pike] from comment #19)
> I'd not break string freeze for this, I don't think there's a lot of people
> actually using SocialAPI, let alone using multiple providers and removing
> them.

Rather than just go on gut feeling, if we are basing this decision on the number of users we should have hard numbers to base our decision on.

Comment 21

[Shane Caraveo (:mixedpuppy)]

I've been thinking about this, and while is is really sub-optimal (and slightly annoying it was backed out, but I understand the reasoning), we could live without it for fx26.

(In reply to Axel Hecht [:Pike] from comment #19)
> I'd not break string freeze for this, I don't think there's a lot of people
> actually using SocialAPI, let alone using multiple providers and removing
> them.

User count is in the low hundreds of thousands, I have good reason to believe that will start changing by year end.

> Also, I'm surprised that there's only one or all, is that really the case?

This disables the entire social feature, not a single provider.  It used to be that only one provider was enabled at any given time but that has changed.

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 804734 [details] [diff] [review]
change menu string

Minor annoyance, and regardless of users let's avoid breaking string freeze where there's not an urgent product need.  This can ride and get out to users in FF27.