Change "Turn Off Provider-Name" in toolbar menu

RESOLVED FIXED in Firefox 27

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

Tracking

26 Branch
Firefox 27
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
We now disable multiple providers, it doesn't make sense to specify only one.
Summary: Change "Turn Of Provider-Name" in toolbar menu → Change "Turn Off Provider-Name" in toolbar menu
(Assignee)

Comment 1

5 years ago
suggestion: "Disable all Services"

Boriss, any thoughts on this?
Flags: needinfo?(jboriss)
(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
Flags: needinfo?(jboriss)
(Assignee)

Comment 3

5 years ago
Created attachment 803883 [details] [diff] [review]
change menu string
Assignee: nobody → mixedpuppy
Attachment #803883 - Flags: review?(felipc)
(Assignee)

Comment 4

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

|| markh
Attachment #803883 - Flags: review?(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+ :)
Attachment #803883 - Flags: review?(mhammond)
(Assignee)

Comment 6

5 years ago
Created attachment 804635 [details] [diff] [review]
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.
Attachment #803883 - Attachment is obsolete: true
Attachment #803883 - Flags: review?(felipc)
Attachment #804635 - Flags: review?(felipc)
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
Attachment #804635 - Flags: review?(felipc) → review+
(Assignee)

Comment 8

5 years ago
Created attachment 804734 [details] [diff] [review]
change menu string

feedback and try

https://tbpl.mozilla.org/?tree=Try&rev=fcba93f98273
Attachment #804635 - Attachment is obsolete: true
Attachment #804734 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/1d12266ddc4c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 26 → ---
(Assignee)

Comment 13

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

Comment 14

5 years ago
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
Attachment #804734 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/183f4c8a4b8f
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
(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.
(Assignee)

Comment 17

5 years ago
(In reply to lsblakk@mozilla.com [: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.
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.
Flags: needinfo?(l10n)

Comment 19

5 years ago
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?
Flags: needinfo?(l10n)
(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.
(Assignee)

Comment 21

5 years ago
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 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.
Attachment #804734 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
You need to log in before you can comment on or make changes to this bug.