Remove Loop/Hello service throttle

VERIFIED FIXED in Firefox 36

Status

Hello (Loop)
Client
P1
normal
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: abr, Assigned: mikedeboer)

Tracking

unspecified
mozilla36
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify +

Firefox Tracking Flags

(firefox36 verified)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
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.
(Reporter)

Comment 1

3 years ago
See Bug 1084097 comment 6.
backlog: --- → Fx36+
Priority: -- → P1
Blocks: 1088581
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Iteration: --- → 36.2
Points: --- → 3
Flags: qe-verify+
Flags: needinfo?(mmucci)
Flags: firefox-backlog+
Added to IT 36.2
Flags: needinfo?(mmucci)
Created attachment 8516693 [details] [diff] [review]
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!
Attachment #8516693 - Flags: review?(bmcbride)
Attachment #8516693 - Flags: review?(adam)
(Reporter)

Comment 4

3 years ago
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.
Attachment #8516693 - Flags: review?(adam) → review+
Created attachment 8517347 [details] [diff] [review]
Patch v2: remove the soft start mechanism for full Hello rollout

Patch with comments addressed. Carrying over r=abr.
Attachment #8516693 - Attachment is obsolete: true
Attachment #8516693 - Flags: review?(bmcbride)
Attachment #8517347 - Flags: review?(bmcbride)
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?
Attachment #8517347 - Flags: review?(bmcbride) → review+
Blocks: 1094915
Duplicate of this bug: 1088581
Pushed to fx-team as https://hg.mozilla.org/integration/fx-team/rev/2d40e7b097d1
https://hg.mozilla.org/mozilla-central/rev/2d40e7b097d1
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Blocks: 1108087
Confirmed the throttle is gone in FF 36b2.
Status: RESOLVED → VERIFIED
status-firefox36: --- → verified
You need to log in before you can comment on or make changes to this bug.