1073218 - Remove Loop/Hello service throttle

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Adam Roach [:abr]]

Bug 1055319 and Bug 1073215 add mechanisms to allow a gradual introduction of the Hello/Loop service. Once the service is fully deployed, this code becomes unnecessary, and should be removed.

Comment 1

[Adam Roach [:abr]]

See Bug 1084097 comment 6.

Comment 2

[Marco Mucci [:MarcoM]]

Added to IT 36.2

Comment 3

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: remove the soft start mechanism for full Hello rollout

Blair, I'd love to use review board for this ;-) Is there a wiki page that offers a good introduction to fit it into my workflow? Thanks!

Comment 4

[Adam Roach [:abr]]

Comment on attachment 8516693 [details] [diff] [review]
Patch v1: remove the soft start mechanism for full Hello rollout

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

I'm going to use splinter for this review, simply because I don't want the complication of figuring out reviewboard on flaky in-flight wifi with limited typing room. :)

In terms of the behavioral mechanics, I think this is correct. As before, I'm going to rely on Blair to make sure the button handling is all sensible.

r+ predicated on the three requested changes being made, assuming l10n is okay with the string name change.

::: browser/app/profile/firefox.js
@@ +1630,1 @@
>  #endif

The ifdef here is no longer needed.

::: browser/components/loop/MozLoopService.jsm
@@ +921,5 @@
> +    Services.prefs.clearUserPref("loop.throttled");
> +    Services.prefs.clearUserPref("loop.throttled2");
> +    Services.prefs.clearUserPref("loop.soft_start_ticket_number");
> +    Services.prefs.clearUserPref("loop.soft_start_hostname");
> +

File a bug to remove this code at some reasonable point in the future (say, FFx39?). Cite the bug number here.

::: browser/components/loop/test/mochitest/head.js
@@ +227,5 @@
> +
> +registerCleanupFunction(function() {
> +  CustomizableUI.reset();
> +});
> +

Remote this code. This shouldn't be necessary any more. The default position is in the navbar, and we're running in a new profile.

::: browser/locales/en-US/chrome/browser/customizableui/customizableWidgets.properties
@@ +99,5 @@
>  
> +# LOCALIZATION NOTE(loop-button.label): This is a brand name, request approval
> +# before you change it.
> +loop-button.label = Hello
> +loop-button.tooltiptext = Start a conversation

Check with someone in l10n whether this is okay. It seems like an arbitrary name change that's going to cause localizers extra work for a pretty questionable benefit.

Comment 5

[Mike de Boer [:mikedeboer]]

Attachment: Patch v2: remove the soft start mechanism for full Hello rollout

Patch with comments addressed. Carrying over r=abr.

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8517347 [details] [diff] [review]
Patch v2: remove the soft start mechanism for full Hello rollout

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

::: browser/components/loop/MozLoopService.jsm
@@ +920,5 @@
> +    // Clear the old throttling mechanism.
> +    Services.prefs.clearUserPref("loop.throttled");
> +    Services.prefs.clearUserPref("loop.throttled2");
> +    Services.prefs.clearUserPref("loop.soft_start_ticket_number");
> +    Services.prefs.clearUserPref("loop.soft_start_hostname");

Did you file a bug to eventually remove this?

Comment 8

[Mike de Boer [:mikedeboer]]

Pushed to fx-team as https://hg.mozilla.org/integration/fx-team/rev/2d40e7b097d1

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2d40e7b097d1

Comment 10

[Paul Silaghi, QA [:pauly]]

Confirmed the throttle is gone in FF 36b2.