1073215 - Throttle adding Loop/Hello button to default toolbar

Status: VERIFIED FIXED

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

Description

[Adam Roach [:abr]]

In Bug 1055319, we added a "soft start" mechanism for Loop in Firefox 34, where the feature would appear for users in a slow, controlled fashion, but in a relatively obscure location (the button customization palette).

For Firefox 35, we're going to want a similar slow transition that moves the button from the customization palette to the default set of visible buttons. Much of the design of Bug 1055319 should be reusable here, but the result of being selected will be moving the button, rather than activating the feature.

Note that the patch that performs this change should remove the previous code that throttled feature activation. In Firefox 35, the plan is to have the feature always on, with the throttling being applied to the location of the button.

Comment 1

[Romain Testard [:RT]]

One issue we need to consider is how this will work with the "What's new" experience - we plan on adding a Firefox Hello brief to the "What's new" tour prompted to users updating their browser and we'll need to ensure that no Hello related material is displayed to users who don't have the feature.

Arcadio can you please cc the person in charge of the "What's new" experience to help us understand how we could address this potential issue?

Comment 2

[Adam Roach [:abr]]

See Bug 1084097 comment 6.

Comment 3

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: make sure that the Loop button shows up in the navbar when unthrottled

Comment 4

[Adam Roach [:abr]]

Comment on attachment 8509655 [details] [diff] [review]
Patch v1: make sure that the Loop button shows up in the navbar when unthrottled

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

The logic here isn't quite what's been specified.

For 35, the throttling behavior doesn't change whether the feature is activated or if the button is hidden; it only (only!) changes the default placement of the button. See Comment 2.

throttled = true:  Button in palette
throttled = false: Button in navbar

Also, this would be much easier to review if you did it in two patches: one would be a mechanical "downlift" of the patch that we landed on Beta 34; and the other, which applies on top of the first, would be the behavior modifications for 35. If it's too much hassle to split the patch, don't worry about it.

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +925,4 @@
>      type: "custom",
>      label: "Hello",
>      tooltiptext: "loop-call-button2.tooltiptext",
>      defaultArea: CustomizableUI.AREA_NAVBAR,

defaultArea:Services.prefs.getBoolPref("loop.throttled") ?
                                       CustomizableUI.AREA_PANEL : 
                                       CustomizableUI.AREA_NAVBAR)

@@ +925,5 @@
>      type: "custom",
>      label: "Hello",
>      tooltiptext: "loop-call-button2.tooltiptext",
>      defaultArea: CustomizableUI.AREA_NAVBAR,
> +    introducedInVersion: 3,

Shouldn't this be 2?

@@ +947,5 @@
> +
> +      // If we're throttled, check to see if it's our turn to be unthrottled
> +      if (Services.prefs.getBoolPref("loop.throttled")) {
> +        // If we're throttled, hide the button.
> +        node.setAttribute("hidden", true);

In 35, the button is never hidden. It's just sometimes in the palette and sometimes in the navbar. So, this line should be removed (along with its comments).

@@ +951,5 @@
> +        node.setAttribute("hidden", true);
> +        aDocument.defaultView.MozLoopService.checkSoftStart(() => {
> +          // If the check unthrottled us, reveal the button.
> +          if (!Services.prefs.getBoolPref("loop.throttled")) {
> +            node.removeAttribute("hidden");

This line should also be removed, since we're never hidden.

What needs to happen here is:

* Check all the customizable areas to see if the button has been customized.
* If the button *has* been customized, leave it alone.
* If the button has *not* been customized:

  CustomizableUI.addWidgetToArea("loop-button-throttled", 
                                 CustomizableUI.AREA_NAVBAR);

::: browser/components/loop/test/mochitest/head.js
@@ +215,5 @@
>  };
>  
> +// Add the Loop button to the navbar.
> +CustomizableUI.addWidgetToArea("loop-button-throttled", CustomizableUI.AREA_NAVBAR);
> + 

Trailing whitespace

Comment 5

[Mike de Boer [:mikedeboer]]

Comment on attachment 8509655 [details] [diff] [review]
Patch v1: make sure that the Loop button shows up in the navbar when unthrottled

You're right, Adam, new patch will be the correct one. (will be tomorrow, my morning)

Comment 6

[Mike de Boer [:mikedeboer]]

(In reply to Adam Roach [:abr] from comment #4)
> Shouldn't this be 2?

No, version 2 was/ is Beta.

Comment 7

[Mike de Boer [:mikedeboer]]

Attachment: Patch v2: make sure that the Loop button shows up in the navbar when unthrottled

Comment 8

[Mike de Boer [:mikedeboer]]

Attachment: Patch v3: make sure that the Loop button shows up in the navbar when unthrottled

Comment 9

[Adam Roach [:abr]]

Comment on attachment 8510172 [details] [diff] [review]
Patch v2: make sure that the Loop button shows up in the navbar when unthrottled

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

Yes, the logic here looks correct now. As we discussed in IRC, we need to use a new pref, like "loop.throttled2", for 35, and add code to remove the "loop.throttled" user pref that 34 used.

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +948,5 @@
> +      // If we're throttled, check to see if it's our turn to be unthrottled
> +      if (Services.prefs.getBoolPref("loop.throttled")) {
> +        aDocument.defaultView.MozLoopService.checkSoftStart(() => {
> +          // If the check unthrottled us and the button was not customized to an
> +          // area by the user, move it to the nav-bar. 

Trailing WS

Comment 10

[Adam Roach [:abr]]

Comment on attachment 8510320 [details] [diff] [review]
Patch v3: make sure that the Loop button shows up in the navbar when unthrottled

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

LGTM.

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +948,5 @@
> +      // If we're throttled, check to see if it's our turn to be unthrottled
> +      if (Services.prefs.getBoolPref("loop.throttled2")) {
> +        aDocument.defaultView.MozLoopService.checkSoftStart(() => {
> +          // If the check unthrottled us and the button was not customized to an
> +          // area by the user, move it to the nav-bar. 

Trailing WS

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Romain Testard [:RT] from comment #1)
> One issue we need to consider is how this will work with the "What's new"
> experience - we plan on adding a Firefox Hello brief to the "What's new"
> tour prompted to users updating their browser and we'll need to ensure that
> no Hello related material is displayed to users who don't have the feature.
> 
> Arcadio can you please cc the person in charge of the "What's new"
> experience to help us understand how we could address this potential issue?

The UITour API already exposes what would be needed here - the page just needs to check whether "loop" is in availableTargets (retrievable via getConfigration).

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8510320 [details] [diff] [review]
Patch v3: make sure that the Loop button shows up in the navbar when unthrottled

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

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +946,5 @@
>        });
> +
> +      // If we're throttled, check to see if it's our turn to be unthrottled
> +      if (Services.prefs.getBoolPref("loop.throttled2")) {
> +        aDocument.defaultView.MozLoopService.checkSoftStart(() => {

Having this done in onBuild means few people will see it, as this will only be triggered when they open Customize Mode. It'll be a small fraction of what the throttling intends.

Plus, adding this to the navbar automatically when opening Customize Mode is unexpected magic.

Better place may be delayed startup.

::: browser/components/customizableui/CustomizeMode.jsm
@@ +804,5 @@
>        this.window.gNavToolbox.palette = this._stowedPalette;
>      }.bind(this)).then(null, ERROR);
>    },
>  
> +  repopulatePalette: function() {

Unused now, right?

Keeping this around concerns me if it's both unused and untested.

::: browser/components/loop/MozLoopService.jsm
@@ +1206,5 @@
>      // stub out API functions for unit testing
>      Object.freeze(this);
>  
> +    // Clear the old throttling mechanism.
> +    if (Services.prefs.getPrefType("loop.throttled")) {

Don't need this check - you can just clear it.

Comment 13

[Maire Reavy [:mreavy]]

This is targeted for Fx35, but has nothing to do with rooms so I'm removing it from the whiteboard.

Comment 14

[Mike de Boer [:mikedeboer]]

Attachment: Patch v4: make sure that the Loop button shows up in the navbar when unthrottled

Apology for the delay in following up... I was quite busy with Loop Rooms for a bit...

Carrying over r=abr.

Comment 15

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8513581 [details] [diff] [review]
Patch v4: make sure that the Loop button shows up in the navbar when unthrottled

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

(Selectively) ship it!

Comment 16

[Mike de Boer [:mikedeboer]]

Comment on attachment 8513581 [details] [diff] [review]
Patch v4: make sure that the Loop button shows up in the navbar when unthrottled

Approval Request Comment
[Feature/regressing bug #]: Loop
[User impact if declined]: Hello toolbar button will be put on the navbar unconditionally, whilst we need the throttling mechanism to take effect here.
[Describe test coverage new/current, TBPL]: n/a
[Risks and why]: minor
[String/UUID change made/needed]: n/a

Comment 17

[Mike de Boer [:mikedeboer]]

Try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=cb3d6921f8c3

Comment 18

[Mike de Boer [:mikedeboer]]

Attachment: Patch v4.1: make sure that the Loop button shows up in the navbar when unthrottled

Unbitrotted patch.

For approval request, see comment 16.

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking, waiting for landing on central before uplift approval.

Comment 20

[Maire Reavy [:mreavy]]

(In reply to Lukas Blakk [:lsblakk] use ?needinfo from comment #19)
> Tracking, waiting for landing on central before uplift approval.

This bug will land on Aurora only.  Bug 1073218 will land on Nightly (likely today, when the tree reopens).

Comment 21

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8516048 [details] [diff] [review]
Patch v4.1: make sure that the Loop button shows up in the navbar when unthrottled

In that case, approving for Aurora uplift only then.

Comment 22

[Mike de Boer [:mikedeboer]]

Pushed to m-a: https://hg.mozilla.org/releases/mozilla-aurora/rev/34839eeada8d

Comment 23

[Maire Reavy [:mreavy]]

Mike -- Can you provide steps for QA to verify this patch is "working as expected" on Aurora?

Comment 24

[Mike de Boer [:mikedeboer]]

The steps to verify this on Aurora is exactly the same as the steps Maire provided at https://bugzilla.mozilla.org/show_bug.cgi?id=1081192#c24, except that the button should now appear on the nav-bar, next to the menu-button.

Comment 25

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e41fbb567e15

Comment 26

[Vasilica Tamas, QA [:vtamas]]

What is the expected behavior in Firefox 35 beta 4? 

I tried to verify this issue on Firefox 35 beta 4 following the steps mentioned by Maire in https://bugzilla.mozilla.org/show_bug.cgi?id=1081192#c24 and I met this behavior:

 - the pref loop.throttled is set by default to false and the Hello button is by default displayed in toolbar
 - if I change the pref to true, the Hello button is still displayed in toolbar and I can not access the customize mode anymore

What is the expected behavior in Firefox 35 beta 4?

Could you provide me the proper steps in order to verify this issue?

Comment 27

[Paul Silaghi, QA [:pauly]]

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #26)
> I can not access the customize mode anymore
Please file a new bug for this and mark it as blocking bug 1097597

Comment 28

[Mike de Boer [:mikedeboer]]

Please hold off testing this feature until Beta5 is available, which should include the fix for bug 1113163.

Comment 29

[Maire Reavy [:mreavy]]

(In reply to Mike de Boer [:mikedeboer] from comment #28)
> Please hold off testing this feature until Beta5 is available, which should
> include the fix for bug 1113163.

Just to clarify: the fix should be in Beta6 which "goes to build" on Monday, Dec 22.  I believe Beta5 was built yesterday.

Comment 30

[Romain Testard [:RT]]

(In reply to Romain Testard [:RT] from comment #1)
> One issue we need to consider is how this will work with the "What's new"
> experience - we plan on adding a Firefox Hello brief to the "What's new"
> tour prompted to users updating their browser and we'll need to ensure that
> no Hello related material is displayed to users who don't have the feature.
> 
> Arcadio can you please cc the person in charge of the "What's new"
> experience to help us understand how we could address this potential issue?

FYI in Fx35 Hello won't be part of the "What's new" tour so this is not an issue.
Beyond Fx35 we won't use throttling anymore so no issues there too.

Comment 31

[Paul Silaghi, QA [:pauly]]

Verified in https://bugzilla.mozilla.org/show_bug.cgi?id=1113163#c25