Closed Bug 1073215 Opened 5 years ago Closed 5 years ago

Throttle adding Loop/Hello button to default toolbar

Categories

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

defect
Points:
2

Tracking

(firefox35+ verified, firefox36 wontfix)

VERIFIED FIXED
mozilla35
Iteration:
36.2
Tracking Status
firefox35 + verified
firefox36 --- wontfix
Blocking Flags:
backlog Fx35+

People

(Reporter: abr, Assigned: mikedeboer)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
Blocks: 1073218
Blocks: 1074659
Whiteboard: [rooms]
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?
Flags: needinfo?(alainez)
Depends on: 1081192
See Bug 1084097 comment 6.
Flags: qe-verify?
QA Contact: anthony.s.hughes → drno
backlog: --- → Fx35+
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 36.1
Points: --- → 2
Flags: qe-verify?
Flags: qe-verify+
Flags: firefox-backlog+
OS: Mac OS X → All
Hardware: x86 → All
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
Attachment #8509655 - Flags: review?(adam) → review-
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)
Attachment #8509655 - Flags: review?(bmcbride)
(In reply to Adam Roach [:abr] from comment #4)
> Shouldn't this be 2?

No, version 2 was/ is Beta.
Attachment #8509655 - Attachment is obsolete: true
Attachment #8510172 - Flags: review?(bmcbride)
Attachment #8510172 - Flags: review?(adam)
Attachment #8510172 - Attachment is obsolete: true
Attachment #8510172 - Flags: review?(bmcbride)
Attachment #8510172 - Flags: review?(adam)
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
Attachment #8510172 - Attachment is obsolete: false
Attachment #8510172 - Flags: feedback+
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
Attachment #8510320 - Flags: review?(adam) → review+
Attachment #8510172 - Attachment is obsolete: true
(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 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.
Attachment #8510320 - Flags: review?(bmcbride) → review-
This is targeted for Fx35, but has nothing to do with rooms so I'm removing it from the whiteboard.
Whiteboard: [rooms]
No longer blocks: 1074659
Priority: -- → P1
Iteration: 36.1 → ---
Iteration: --- → 36.2
Flags: needinfo?(mmucci)
Iteration: 36.2 → ---
Flags: needinfo?(mmucci)
Iteration: --- → 36.2
Apology for the delay in following up... I was quite busy with Loop Rooms for a bit...

Carrying over r=abr.
Attachment #8510320 - Attachment is obsolete: true
Attachment #8513581 - Flags: review?(bmcbride)
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!
Attachment #8513581 - Flags: review?(bmcbride) → review+
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
Attachment #8513581 - Flags: approval-mozilla-aurora?
Unbitrotted patch.

For approval request, see comment 16.
Attachment #8513581 - Attachment is obsolete: true
Attachment #8513581 - Flags: approval-mozilla-aurora?
Attachment #8516048 - Flags: review+
Attachment #8516048 - Flags: approval-mozilla-aurora?
Tracking, waiting for landing on central before uplift approval.
(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).
Flags: needinfo?(lsblakk)
Flags: needinfo?(lsblakk)
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.
Attachment #8516048 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Mike -- Can you provide steps for QA to verify this patch is "working as expected" on Aurora?
Flags: needinfo?(mdeboer)
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.
Flags: needinfo?(mdeboer)
Target Milestone: --- → mozilla35
Depends on: 1096326
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?
Flags: needinfo?(mdeboer)
(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
Please hold off testing this feature until Beta5 is available, which should include the fix for bug 1113163.
Depends on: 1113163
Flags: needinfo?(mdeboer)
(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.
(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.
Flags: needinfo?(alainez)
Blocks: 1136300
You need to log in before you can comment on or make changes to this bug.