1047181 - Change the Loop toolbar button when FxA sign in completes

Status: VERIFIED FIXED

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

Description

[Matthew N. [:MattN]]

Toolbar design is at https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#toolbar

Comment 1

[Romain Testard [:RT]]

Is that a duplicate of https://bugzilla.mozilla.org/show_bug.cgi?id=1028894?

Comment 2

[Matthew N. [:MattN]]

This is a subset of that which is specific to FxA integration so I will make it block that.

Comment 3

[Matthew N. [:MattN]]

I mean that this bug is specific to FxA integration. I don't think it makes sense to implement all of different icon changes in the one bug since they are from different areas of code.

Comment 4

[Romain Testard [:RT]]

OK, makes sense, thanks for clarifying.

Comment 5

[Mike de Boer [:mikedeboer]]

Bug 1047284 contains a code sample on how to do this.

Comment 6

[Maire Reavy [:mreavy]]

After talking with Mike, I think we want this for Fx34, if possible.

Comment 7

[Maire Reavy [:mreavy]]

Darrin, Do we need any strings for this bug?  I'm thinking specifically about tooltips.  Or can we go just with icons (especially for Fx34)?

Comment 8

[Darrin Henein [:darrin]]

I think just the icon state is enough for 34, especially given that this will likely occur in short-relation to the user's action of signing in (the association should be there).

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 8488230 [details] [diff] [review]
Patch

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

::: browser/base/content/browser-loop.js
@@ +78,5 @@
>          state = "error";
>        } else if (MozLoopService.doNotDisturb) {
>          state = "disabled";
> +      } else if (MozLoopService.userProfile) {
> +        state = "active";

I think opening the panel should clear the "active" state so I think you have more to do here.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.1

I added a new "authenticated" state so that we could remove it when the panel opened, but not remove the "active" state if the panel is opened during a call.

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 8490271 [details] [diff] [review]
Patch v1.1

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

::: browser/base/content/browser-loop.js
@@ +32,5 @@
>          }, true);
>        };
>  
> +      if (this.toolbarButton.node.getAttribute("state") == "authenticated") {
> +        this.toolbarButton.node.setAttribute("state", "");

This shouldn't assume "" and should call updateToolbarState (after that function changes).

This also doesn't work with multiple windows. It will only clear the state in one.

@@ +82,5 @@
>          state = "error";
>        } else if (MozLoopService.doNotDisturb) {
>          state = "disabled";
> +      } else if (MozLoopService.userProfile) {
> +        state = "authenticated";

This isn't going to work if we get an error (error state applies) and then we clear the error OR after a call completes (when that's implemented).

I think we could pass info with notification and then to this function (via an optional argument) that indicates it's the result of a login. If reason = "login" (or whatever) then we set to active, otherwise we go down the existing if/else statements. That way we will never fallback to the login state later when .userProfile is truthy.

e.g.: updateToolbarState: function(reason = null) {

To clear the state in all windows, perhaps we would need another reason related to either the first panel open after a login (would require storing state in the JSM though which is not ideal) or just have a notification with a reason whenever the panel opens and in most cases we would do nothing in updateToolbarState for that reason but we would clear the authenticated state if it was the current one.

Please add tests for the above issues: primarily so we never default back to "authenticated" after the panel opens.

::: browser/components/loop/test/mochitest/head.js
@@ +48,5 @@
>      loopPanel.hidePopup();
> +    let frameId = btn.getAttribute("notificationFrameId");
> +    let frame = document.getElementById(frameId);
> +    if (frame) {
> +      loopPanel.removeChild(frame);

Is this related to some intermittent failure?

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.4

Updated based on review comments.

(In reply to Matthew N. [:MattN] from comment #12)
> ::: browser/components/loop/test/mochitest/head.js
> @@ +48,5 @@
> >      loopPanel.hidePopup();
> > +    let frameId = btn.getAttribute("notificationFrameId");
> > +    let frame = document.getElementById(frameId);
> > +    if (frame) {
> > +      loopPanel.removeChild(frame);
> 
> Is this related to some intermittent failure?

Yes, if promiseGetMozLoopAPI is called multiple times within a test (such as making multiple calls to loadLoopPanel, then multiple cleanup functions will be registered and only the first one would succeed.

Comment 14

[Matthew N. [:MattN]]

Comment on attachment 8490942 [details] [diff] [review]
Patch v1.4

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

Please add tests to browser/components/loop/test/mochitest/browser_toolbarbutton.js for the cases where we need to fallback to a non-default option (e.g. DND) after opening the panel to clear the login state. You should be able to reach in and manipulate gFxAOAuthProfile for this to avoid the actual login overhead.

::: browser/base/content/browser-loop.js
@@ +79,5 @@
> +     *
> +     * @param {string} aReason (optional) Some states are only
> +     *                 shown if a related reason is provided.
> +     */
> +    updateToolbarState: function(aReason) {

Please use default argument syntax to indicate that an argument in optional: e.g. aReason = null

In JSDoc syntax, you would put the argument name in square brackets: [aReason]

@@ +86,5 @@
>          state = "error";
>        } else if (MozLoopService.doNotDisturb) {
>          state = "disabled";
> +      } else if (aReason == "login" && MozLoopService.userProfile) {
> +        state = "authenticated";

I was thinking this would have precedence (at least over things like DND but perhaps errors should be first). Consider someone with DND on and then logs in.

@@ +89,5 @@
> +      } else if (aReason == "login" && MozLoopService.userProfile) {
> +        state = "authenticated";
> +      } else if (aReason == "panelshown" &&
> +                 this.toolbarButton.node.getAttribute("state") == "authenticated") {
> +        state = "";

Since this is in the last |else if| and the default value of state is "", this isn't doing anything AFAICT. Is that intentional so people can follow the flow? If so, I think a comment about why you're doing nothing would be clearer instead of setting |state = ""| again. I think setting it to "" might lead others to believe the state should always be "" when the conditions are met when the state should actually default to the normal calculations (which could, for example, lead to the "disabled" state for DND). I'm wondering if this if block can just be removed? Is the getAttribute check supposed to be doing something?

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.5

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.5 (w/ css reverted)

No need to keep the "authenticated" state value since it was only needed in earlier versions of this patch to know that it could be removed. The aReason parameter to updateToolbarState obsoleted it.

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.5 (w/ css reverted, qref'd)

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.6

Comment 19

[Matthew N. [:MattN]]

Comment on attachment 8491746 [details] [diff] [review]
Patch v1.6

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

::: browser/base/content/browser-loop.js
@@ +78,5 @@
> +    /**
> +     * Updates the toolbar/menu-button state to reflect Loop status.
> +     *
> +     * @param {string} [aReason] (optional) Some states are only
> +     *                 shown if a related reason is provided.

You can remove "(optional)" now and then wrap this so that the second line aligns with the start of the argument description (starting with "Some") like is more commonly done.

::: browser/components/loop/test/mochitest/browser_toolbarbutton.js
@@ +30,5 @@
> +  Assert.strictEqual(LoopUI.toolbarButton.node.getAttribute("state"), "", "Check button is in default state after opening panel");
> +  let loopPanel = document.getElementById("loop-notification-panel");
> +  loopPanel.hidePopup();
> +  yield MozLoopService.doNotDisturb = true;
> +  Assert.strictEqual(LoopUI.toolbarButton.node.getAttribute("state"), "disabled", "Check button is in disabled state");

This isn't testing the case I mentioned where DND is set *before* login so the "active" state from login replaces the "disabled" state from DND. I think you can probably change this test to test that case as I'm not sure this current function is testing more than test_active and test_doNotDisturb do together.

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/5710731f09e9

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out in https://hg.mozilla.org/integration/fx-team/rev/10a23714dd6a for browser-chrome-1 failures: 
Non-e10s was failing with https://tbpl.mozilla.org/php/getParsedLog.php?id=48406490&tree=Fx-Team#error0
e10s was failing with https://tbpl.mozilla.org/php/getParsedLog.php?id=48404430&tree=Fx-Team

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: 1047181-followup.patch

This fixes the test failure by caching the state of the offline flag once at the beginning of tests instead of each time a helper function is called.

Comment 23

[Matthew N. [:MattN]]

Comment on attachment 8491855 [details] [diff] [review]
1047181-followup.patch

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

::: browser/components/loop/test/mochitest/head.js
@@ +6,5 @@
>    LOOP_SESSION_TYPE,
>    MozLoopServiceInternal,
>  } = Cu.import("resource:///modules/loop/MozLoopService.jsm", {});
>  
> +// Cache this value only once, at at the beginning of a

s/at at/at/

Comment 24

[Jared Wein [:jaws] (please needinfo? me)]

Relanded with typo fix and both patches folded,
https://hg.mozilla.org/integration/fx-team/rev/f9a9f246b0d8

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f9a9f246b0d8

Comment 26

[u279076]

Paul, please verify this in the latest Nightly and the Try-server build here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f9eb2cbac352

Comment 27

[u279076]

Upon further review of the issues flagged for pre-uplift verification, I do not think this needs verification before we uplift. Paul, please still verify this in the latest Nightly but skip verification in the Fig branch.

Comment 28

[Paul Silaghi, QA [:pauly]]

After sign in, the Loop button color changes to blue. It gets back to the normal gray after click.
35.0a1 (2014-10-01)
Please reopen if the blue color should persist.

Comment 29

[u279076]

(In reply to Paul Silaghi, QA [:pauly] from comment #28)
> After sign in, the Loop button color changes to blue. It gets back to the
> normal gray after click.
> 35.0a1 (2014-10-01)
> Please reopen if the blue color should persist.

Yeah, that's what I see too. I personally think this state should persist as long as a user is authenticated. However, perhaps that's out of scope for the original intent of this bug.

Comment 30

[Matthew N. [:MattN]]

It's working as intended. What you're saying would make sense if the toolbar button change for login was different than the color used for other states such as an incoming call but since they're the same clearing make the most sense IMO.

Comment 31

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

Comment on attachment 8491746 [details] [diff] [review]
Patch v1.6

Approval Request Comment
Uplift request for patches staged and tested on Fig

Comment 32

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/13a9118e44e4
https://hg.mozilla.org/releases/mozilla-aurora/rev/340da2f01cc2

Comment 33

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

Comment on attachment 8491746 [details] [diff] [review]
Patch v1.6

I worked with Randell and Maire on uplifting a large number of Loop bugs at once. All of the bugs have been staged on Fig and tested by QE before uplift to Aurora. As well, all of the bugs are isolated to the Loop client. Randell handled the uplift with my approval. I am adding approval to the bug after the fact for bookkeeping.

Comment 34

[u279076]

I've seen this in Aurora testing over the last few days. Marking this verified fixed for Firefox 34.