1084097 - Loop button appears on toolbar by default for users who customized their palette

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect)

Description

[Adam Roach [:abr]]

STR:

1. Create a new profile
2. Run 33.0b3; note that the Loop icon is in the toolbar.
3. Customise the toolbar.
4. Run 34.0b1; note that the icon is not displayed in the toolbar or customization pallette
5. Set loop.soft_start_ticket_number to 1
6. Restart twice -> icon is on toolbar

The icon should be in the customization palette rather than the toolbar.

Comment 1

[Maire Reavy [:mreavy]]

[Tracking Requested - why for this release]:

We found this problem when someone with a customized palette flipped the throttle pref and saw the Hello button in their toolbar.   We will be putting together a patch today for Beta and plan to have it available for QA in the morning.  We will work hard to have this in for Beta 2.

Comment 2

[Matthew N. [:MattN]]

Attachment: v.1 Beta patch to remove #loop-call-button

This will remove it from any customizable area so when it gets unthrottled for the user it will appear back in the palette instead of the area.

Comment 3

[Matthew N. [:MattN]]

Attachment: v.1 Trunk & Aurora patch to reserve UI_VERSION = 24

Comment 4

[Matthew N. [:MattN]]

(In reply to Matthew N. [:MattN] (behind on reviews - focused on Loop) from comment #2)
> Created attachment 8506692 [details] [diff] [review]
> v.1 Beta patch to remove #loop-call-button

Now that my Beta build finally finished I see that this isn't working. I wonder if it has something to do with doing this too early e.g. before there is a window. We'd like to land this today so do any of you reviewers have ideas?

Comment 5

[Matthew N. [:MattN]]

So far I see that gPlacements only has PanelUI-contents in it at the time this is run. What's the best way to handle this? Can I cause the nav-bar area to be setup at this time?

Comment 6

[Adam Roach [:abr]]

After having a discussion with Blair McBride, we've concluded that the approach that has the best properties (including being tractable enough to land by 35) is:

 * For 34 and 35, change the button name to loop-button-throttled. This ID
   is not in the default set.

 * For 34, the throttling mechanism will determine whether the button
   appears in the palette (versus not appearing at all) [Bug 1055319]

 * For 35, the throttling mechanism will determine whether the button
   appears by default in the navbar (versus in the palette) [Bug 1073215]

 * For 35: if user resets to default, check the loop.throttled pref. Based
   on its value, we put the button in the navbar or palette.

 * For 36, the button will appear in the navbar by default. There is no
   throttling mechanism. [Bug 1073218]

 * For 36, change the name to loop-button. This name is in the default set.

 * In 36, add migration code to move loop-button to the navbar if it's not
   present anywhere. This is the typical code that is present whenever a new
   button is added to the default set.

 * In 36, add migration code that looks for loop-button in any customizable
   area, and replaces it with loop-button-throttled. This preserves the button's
   position for users who have customized it already.

We take as a principle that the "fail safe" behavior for 34 should be the button
appearing in the palette rather than the navbar, and the the "fail safe" behavior
for 35 and later should be the button appearing in the navbar rather than the
palette.

It is a known defect that this plan will cause the button to reappear in 36 for
those users who have removed it from all customizable areas during 35. Addressing
this flaw requires some very fragile handling, the failure of which will cause
the feature not to become visible for some users. As this does not align with
the "fail safe" principles described above, we are willing to live with this defect.

Comment 7

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] (behind on reviews - focused on Loop) from comment #5)
> So far I see that gPlacements only has PanelUI-contents in it at the time
> this is run. What's the best way to handle this? Can I cause the nav-bar
> area to be setup at this time?

I don't understand why this code wouldn't work, and comment #6 seems entirely orthogonal (and I will additionally admit I do not understand it at all). Presumably there was more discussion about how/why. Can you elaborate here in order for the history/reasoning here not to get lost?

Comment 8

[Mike de Boer [:mikedeboer]]

(In reply to Adam Roach [:abr] from comment #6)
>  * For 34 and 35, change the button name to loop-button-throttled. This ID
>    is not in the default set.

As an addendum, this is the actual plan:

 * For 34, the button ID will stay 'loop-call-button'. This ID is _not_ in the default set.
 * For 35, the button ID will change to 'loop-button-throttled', to differentiate between the default position that changes from the palette to the nav-bar. This ID is also _not_ in the defaultset.
 * For 36, the button ID will change to 'loop-button' - the finale. This is done to be able to differentiate the change of default area.

Comment 9

[Adam Roach [:abr]]

(In reply to :Gijs Kruitbosch from comment #7)
> [C]omment #6 seems
> entirely orthogonal (and I will additionally admit I do not understand it at
> all). Presumably there was more discussion about how/why. Can you elaborate
> here in order for the history/reasoning here not to get lost?

So, the issue at hand is that the way we handled adding the button to the navbar for 33b1 through 33b3 causes it to appear on the navbar by default for a significant proportion of beta users. We don't want that.

There are effectively two ways to deal with the situation: (1) write migration code to clean up the mess we created in 33; or (2) abandon the old button name altogether so we don't need to worry about the mess.

After significant exploration of option (1), Mike and Matt determined that it was not particularly feasible, so we've opted to go with an ID change. I don't fully understand the nature of the complications.

So we've decided to go with option (2). For 34, all this requires is a change to the button ID (which will address this bug). This is very simple, but requires changes to a relatively large number of files.

In order to prevent similar difficulties as we move through 35 and 36, we evaluated what behavior is necessary to create the desired user experience. The remainder of comment 6 is an exploration of that behavior, to ensure that it is consistent with the change proposed for 34.

Does that clear things up, or would you prefer to set up some time to talk?

Comment 10

[Adam Roach [:abr]]

(In reply to Mike de Boer [:mikedeboer] from comment #8)
>  * For 34, the button ID will stay 'loop-call-button'. This ID is _not_ in
> the default set.

I'm confused about this point. If we leave the ID the same, won't it continue to appear in the navbar for all beta users who have customized the palette? That's exactly what we're trying to fix.

Comment 11

[Mike de Boer [:mikedeboer]]

(In reply to Adam Roach [:abr] from comment #10)
> I'm confused about this point. If we leave the ID the same, won't it
> continue to appear in the navbar for all beta users who have customized the
> palette? That's exactly what we're trying to fix.

Whoops, you're right. The name will change indeed.

Comment 12

[u279076]

Nominating to block Loop 34 since this impacts our soft-launch plans.

Comment 13

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1: [Beta only] make sure that the Loop button only shows up in the palette when unthrottled

/me very sure I got all the renames right. Try push to double-check coming up.

Comment 14

[Mike de Boer [:mikedeboer]]

Try push for attachment 8507087 [details] [diff] [review], comment 13: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f4dbdc3d7f47

Comment 15

[Adam Roach [:abr]]

Comment on attachment 8507087 [details] [diff] [review]
Patch 1: [Beta only] make sure that the Loop button only shows up in the palette when unthrottled

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

r- for missing files, deactivating the throttle.

Ths ID replacement looks right to me. The changes in browser-loop.js remove the throttle behavior, and leave the feature deactivated for all users forever. That must be fixed.

The code that adds the loop button to the palette has been moved to LoopButton.inc.js, which is not included in this patch, so I can't review it.

I do not understand the changes to CustomizeMode.jsm, and am relying on Blair to handle reviewing them.

Based on your "very sure I got all the renames right" comment, I do not plan to audit that this patch removes all instances of loop-call-button from the tree.

::: browser/base/content/browser-loop.js
@@ -49,5 @@
> -        if (toolbarButton && toolbarButton.node) {
> -          toolbarButton.node.hidden = true;
> -        }
> -        return;
> -      }

No, we need this code still. If the feature is disabled, we don't want to run any of the init code. You probably meant to remove only the button handling code, but this cuts too deep.

@@ -61,5 @@
> -          toolbarButton.node.hidden = true;
> -        }
> -        MozLoopService.checkSoftStart(toolbarButton && toolbarButton.node);
> -        return;
> -      }

This removes the only call we make to checkSoftStart(). Without this call, the feature will never activate. Again, I can see that you want to remove the button handling here, but we still need to call checkSoftStart() if throttled is true.

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +902,5 @@
>        win.MailIntegration.sendLinkForWindow(win.content);
>      }
>    }];
>  
> +#include ../loop/LoopButton.inc.js

This file isn't in the patch.

Comment 16

[Mike de Boer [:mikedeboer]]

So sorry, I forgot to add the file indeed, which explains everything.

Comment 17

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.1: [Beta only] make sure that the Loop button only shows up in the palette when unthrottled

Accompanying try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=355da34b7a95

Comment 18

[Adam Roach [:abr]]

Comment on attachment 8507135 [details] [diff] [review]
Patch 1.1: [Beta only] make sure that the Loop button only shows up in the palette when unthrottled

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

r+, predicated on a fix for the UX defect I describe in LoopButton.inc.js

I'm a little sad that the mochi tests no longer have any way to check that the UX changes are happening as expected. I'd normally ask you to open a new bug to address testing the UX portions (so we catch regressions); but, since this code is going away in 36, I guess we can live with manual testing for now.

::: browser/components/loop/LoopButton.inc.js
@@ +6,5 @@
> +
> +CustomizableWidgets.push({
> +  id: "loop-button-throttled",
> +  type: "custom",
> +  // XXX Bug 1013989 will provide a label for the button

No longer applicable (this bug has landed) -- please remove the comment.

@@ +12,5 @@
> +  tooltiptext: "loop-call-button.tooltiptext",
> +  onBuild: function(aDocument) {
> +    // Nuke the old 'loop-call-button' out of orbit.
> +    CustomizableUI.removeWidgetFromArea("loop-call-button");
> +    

Trailing whitespace

@@ +38,5 @@
> +        if (Services.prefs.getBoolPref("loop.throttled")) {
> +          node.setAttribute("hidden", true);
> +        }
> +      });
> +    }

There's an odd corner case that this does not handle well: if the DNS response takes more than a few milliseconds, this can result in a situation where the button is briefly visible, but then disappears. I think users would understand if the button appears after a brief delay and then hangs around forever; but being briefly visible and then going away is going to be jarring and frustrating. (Also, the "suddenly appearing" defect can only appear once, upon initial activation: the "appear then disappear" situation can arise many times while the feature is throttled).

Suggest restructuring as:

if (Services.prefs.getBoolPref("loop.throttled")) {
  // If we're throttled, hide the button.
  node.setAttribute("hidden", true);
  aDocument.defaultView.MozLoopService.checkSoftStart(() => {
    // If the check unthrottled us, reveal the button.
    if (!Services.prefs.getBoolPref("loop.throttled")) {
      node.setAttribute("hidden", false);
    }
  });
}

Comment 19

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.2: [Beta only] make sure that the Loop button only shows up in the palette when unthrottled

Patch with comments addressed. Thanks Adam! Carrying over r=abr.

Comment 20

[Adam Roach [:abr]]

Since this is moving the logic for unthrottling around, I think we need to double-check that the work from Bug 1055319 is still working. We know that the DNS part of this already works, so I think we can go with a simplified test that tweaks /etc/hosts rather than setting up a DNS server. Of course, this means testing on OS X or Linux rather than Windows, but I don't get the impression that this should be a major issue.

Instructions from Bug 1055319 comment 31, modified to suit these circumstances:

1) Set "loop.soft_start_hostname" to "softstart.localhost"

2) Edit /etc/hosts and add a line to the end of the file: "127.0.0.0 softstart.localhost"

3) With "loop.enabled" and "loop.throttled" set to true, reset "loop.soft_start_ticket_number" to -1. Restart the browser. 

4) Verify that "loop.soft_start_ticket_number" is greater than 0, and less than 16777215.

5) Verify that "loop.throttled" is still "true" and that the Loop button is NOT in the customization palette.

6) Change "loop.soft_start_ticket_number" to 8388607

7) Edit the line in /etc/hosts to read "127.127.255.255 softstart.localhost"

8) Restart the browser. Verify that "loop.soft_start_ticket_number" is still 8388607. Verify that "loop.throttled" is still "true" and that the Loop button is NOT in the customization palette.

8) Edit the line in /etc/hosts to read "127.128.0.0 softstart.localhost"

9) Restart the browser. Verify that "loop.soft_start_ticket_number" is still 8388607. Verify that "loop.throttled" is now FALSE, and that the Loop button IS in the customization palette.

Comment 21

[Mike de Boer [:mikedeboer]]

A patch for Aurora is up next, therefore tracking Fx35.

Comment 22

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8507183 [details] [diff] [review]
Patch 1.2: [Beta only] make sure that the Loop button only shows up in the palette when unthrottled

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

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +902,5 @@
>        win.MailIntegration.sendLinkForWindow(win.content);
>      }
>    }];
>  
> +#include ../loop/LoopButton.inc.js

I rather expected that to be more complex, but there's not that much different, since most of the logic is in MozLoopService.jsm. Better to just keep this in this file, IMO.

::: browser/components/customizableui/CustomizeMode.jsm
@@ +752,5 @@
>    makePaletteItem: function(aWidget, aPlace) {
>      let widgetNode = aWidget.forWindow(this.window).node;
> +    if (!widgetNode) {
> +      ERROR("Widget with id " + aWidget.id + " does not return a valid node");
> +      return;

Nit: Return null, to make it explicit that a value is expected. Ditto below.

::: browser/components/loop/LoopButton.inc.js
@@ +10,5 @@
> +  label: "loop-call-button.label",
> +  tooltiptext: "loop-call-button.tooltiptext",
> +  onBuild: function(aDocument) {
> +    // Nuke the old 'loop-call-button' out of orbit.
> +    CustomizableUI.removeWidgetFromArea("loop-call-button");

This would be better done in the migration function in CustomizableUI.jsm, since we only ever need to do this once.

@@ +33,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);
> +      aDocument.defaultView.MozLoopService.checkSoftStart(() => {

Hm, this can't go in onBuild. In the default initial case, it'll only get run when people enter customize mode (the widget won't get built otherwise, since it won't be placed in an area). And since checkSoftStart() is async, even if it becomes unthrottled, the widget won't show up the first time.

The original placement in browser-loop.js doewsn't seem right either, because we don't want that getting called multiple times for multiple windows. On a somewhat related note, why does it look like MozLoopService gets initialized multiple times if there are multiple windows?

Comment 23

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.3: [Beta only] make sure that the Loop button only shows up in the palette when unthrottled

Updated patch, comments addressed. Carrying over r=abr.

Thanks much for looking at this, Blair!

Comment 24

[Mike de Boer [:mikedeboer]]

(In reply to Blair McBride [:Unfocused] from comment #22)
> On a somewhat related note, why does it look like MozLoopService
> gets initialized multiple times if there are multiple windows?

Dunno, looks like a big, blistering, gaping hole of a bug(?)

Comment 25

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

Comment on attachment 8507493 [details] [diff] [review]
Patch 1.3: [Beta only] make sure that the Loop button only shows up in the palette when unthrottled

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

I noted applying this to beta that the string in the Customize menu has gone back to "Invite someone to Talk" instead of "Hello" (which a previous patch for beta/aurora only hard-coded to avoid a string change)

Comment 26

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

Comment on attachment 8507493 [details] [diff] [review]
Patch 1.3: [Beta only] make sure that the Loop button only shows up in the palette when unthrottled

Approval Request Comment
[Feature/regressing bug #]: loop

[User impact if declined]: Confusing issues with when and where the Loop button appears depending on user profile history

[Describe test coverage new/current, TBPL]: manual testing

[Risks and why]: much lower risk than plyaing whack-a-mole with re-using the old button id

[String/UUID change made/needed]: none

Comment 27

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

I've run through the steps from comment 20 with this patch in on beta (with all the other queued Beta patches).  All checks out as stated in the comment.

Comment 28

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8507493 [details] [diff] [review]
Patch 1.3: [Beta only] make sure that the Loop button only shows up in the palette when unthrottled

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

Ship it!

Comment 29

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

With nit fix ("Hello"):
https://hg.mozilla.org/releases/mozilla-beta/rev/5840764c4312

Leaving open for resolving anything needed on Aurora.

Comment 30

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8507493 [details] [diff] [review]
Patch 1.3: [Beta only] make sure that the Loop button only shows up in the palette when unthrottled

Previously approved offline. Adding approval to the bug.

Comment 31

[Maire Reavy [:mreavy]]

To avoid any possible confusion, I'd like to open 2 new bugs for the work we need to do for Aurora (Fx35) and Nightly (Fx36).  If we do this, I'd close this bug as resolved since the patch has landed on Beta.

Does that work for you?

Comment 32

[Adam Roach [:abr]]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #31)
> To avoid any possible confusion, I'd like to open 2 new bugs for the work we
> need to do for Aurora (Fx35) and Nightly (Fx36).  If we do this, I'd close
> this bug as resolved since the patch has landed on Beta.
> 
> Does that work for you?

You mean in addition to Bug 1073215 and Bug 1073218? If so, I am unclear on what they would cover.

Comment 33

[Maire Reavy [:mreavy]]

This bug is about Fx34.  Bug 1073215 and bug 1073218 will track work for Fx35 and Fx36, respectively.

Comment 34

[u279076]

Paul, can you please verify this in the next beta?

Comment 35

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 34b2 Win 7

Comment 38

[Paul Silaghi, QA [:pauly]]

I think this causes the Loop icon to disappear from the toolbar if starting Nightly with the same beta profile (with throttle=false and the Loop icon moved to toolbar). Thoughts ?

Comment 39

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

I tried a profile used for beta to test the icon/customize code, and loaded it with a copy of inbound, and the icon stayed in the toolbar.

Do you have STR for this?  Is it repeatable?

Comment 40

[Paul Silaghi, QA [:pauly]]

(In reply to Randell Jesup [:jesup] from comment #39)
> Do you have STR for this?  Is it repeatable?
Yes, see bug 1088581

Comment 41

[u279076]

Clearing firefox35 status flag since that's covered by bug 1073215.