1047146 - Add current username to the Loop panel footer

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect)

Description

[Matthew N. [:MattN]]

Add the link which calls the code from bug 1047130 to initiate a login to Loop and updates the UI to show the username upon successful login.

Out of scope for this bug:
* Persisting login state between browser sessions (bug 1047133)
* Avatar display (bug 1047137)
* Gear menu in the footer (bug 1047144)

Comment 1

[Matthew N. [:MattN]]

Attachment: promiseFxAProfile function WIP

Here is a function modified from Vlad's PoC which may be useful to implement this.

Comment 2

[Chris Karlof [:ckarlof]]

> const PROFILE_SERVER_URL = Services.prefs.getCharPref("identity.fxaccounts.profile.uri");

I think it's a good idea to add this pref to be the "default" profile server, but our intention of having it returned by the Loop server as part of its config was to allow services to easily override it as a default. Why?

Some services can be self-hosted, and some users may have self-hosted FxA or profile servers. These self-hosted users may also want to concurrently use services in Firefox that are provided by us. This situation is not too different than the one faced by our QA or dev team that needs to point to different deployments in the browser.

Comment 3

[Matthew N. [:MattN]]

Chris, do we need to get the username from the profile server or can we get it returned by the Loop server when we get the token or from the OAuth authorization web flow web channel? Getting it from one of those two earlier phases will allow us to give faster feedback to users when they login. Otherwise they'll have to wait for the profile request. It's not a big deal but it affects how I can implement displaying the username after login.

Comment 4

[Chris Karlof [:ckarlof]]

Getting the user's email directly from the profile server with the token is the most straightforward thing to do because that's what is available right now.

As we extend the data attached to Loop (e.g., avatar, contacts, screen name, etc.), I think it makes more sense for the client to talk to the appropriate services directly rather than proxying it all through the Loop server. Having the loop server return the email with the token is indeed an optimization, but it's not clear to me that it's worth it at this point.

Comment 5

[Chris Karlof [:ckarlof]]

On a related note, although there is currently no way to change the user's email address associated with an account, there might be one in the future, so we should think about how often we should re-fetch the user's email associated with a token. It might be the case we clear all tokens after an email change, but that policy hasn't been decided yet.

Comment 6

[Matthew N. [:MattN]]

Note that I was saying that the username could be passed back to the browser at the end of the web flow (through the Web Channel) as another option. I agree the client should talk directly to the profile server for extended profile data but it would be nice to have the username immediately after login so we don't need to show some intermediate placeholder text to replace "Guest" while we are fetching the username. If we have the username, it could act as the placeholder until the screen name is retrieved.

My understanding is that the Loop server will need the username regardless so that it can associate the username with the session.

If the UX suffers because of the extra round trip we may want to reevaluate getting the username earlier.

Comment 7

[Chris Karlof [:ckarlof]]

> If the UX suffers because of the extra round trip we may want to reevaluate getting the username earlier.

I'd prefer to see what it looks like in practice before we make any optimizations. I don't recall any weird delays from Vlad's POC, but I suppose it would depend on when you choose to re-open the panel, and if you open it before fetching the user's email, what the UX is like.

The risk of passing it back from the WebChannel is it opens the door to more foot-guns. For example, a naive relying service may just use the returned email address and not complete the OAuth flow.

Comment 8

[Matthew N. [:MattN]]

(In reply to Chris Karlof [:ckarlof] from comment #7)
> > If the UX suffers because of the extra round trip we may want to reevaluate getting the username earlier.
> 
> I'd prefer to see what it looks like in practice before we make any
> optimizations. I don't recall any weird delays from Vlad's POC, but I
> suppose it would depend on when you choose to re-open the panel, and if you
> open it before fetching the user's email, what the UX is like.

I agree that we should wait to see in practice how it is. The experience will vary depending on the server response times and the user's internet connection though.

> The risk of passing it back from the WebChannel is it opens the door to more
> foot-guns. For example, a naive relying service may just use the returned
> email address and not complete the OAuth flow.

I agree that this is a downside.

Comment 9

[Chris Karlof [:ckarlof]]

> The experience will vary depending on the server response times and the user's internet connection though.

I agree. Hopefully we can provide some user feedback (e.g., a spinner of some kind) in this situation.

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 8468316 [details]
promiseFxAProfile function WIP

Moved to bug 1058424

Comment 11

[Matthew N. [:MattN]]

I'm splitting off "Sign in or Sign up" link (for guests) part into bug 1059021 as that doesn't depend on fetching data from the profile server. This bug, if implemented after that one, will be responsible for hiding the sign in/up text upon login.

Comment 12

[Maire Reavy [:mreavy]]

Matt -- Do you have what you need to land the strings for this?

Comment 13

[Romain Testard [:RT]]

I assume this bug covers the ability to display "Guest" per https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#precall-firstrun.
Bug 1015938 won't make it to FF34 so the "Guest" string is not customizable but should be present.

Comment 14

[Matthew N. [:MattN]]

I think we will need the following strings:
* "Guest"

I debated having one for a case where we couldn't fetch the username but I'm not sure what we would say for that case. I think we could maybe leave the username blank and put up an error bar with the message:

"Profile information could not be retrieved [Retry]"

I think this case should be quite rare so I'm not sure this is needed for 34.

Comment 15

[Romain Testard [:RT]]

(In reply to Matthew N. [:MattN] from comment #14)
> I think we will need the following strings:
> * "Guest"
> 
> I debated having one for a case where we couldn't fetch the username but I'm
> not sure what we would say for that case. I think we could maybe leave the
> username blank and put up an error bar with the message:
> 
> "Profile information could not be retrieved [Retry]"
> 
> I think this case should be quite rare so I'm not sure this is needed for 34.
Agreed, not a "must have" for 34

Comment 16

[Matthew N. [:MattN]]

Attachment: String v.1

String only

Comment 17

[Mark Banner (:standard8)]

Comment on attachment 8482375 [details] [diff] [review]
String v.1

r=Standard8

Comment 18

[Matthew N. [:MattN]]

Pushed string: https://hg.mozilla.org/integration/fx-team/rev/11a82a856100

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/11a82a856100

Comment 20

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

Attachment: Patch for feedback

Niko, this patch should work but `this.state.email` is null when the panel is reopened after a successful login attempt. Do you know why the state is getting lost here? The subsequent immediate render has the correct state, but when the panel is reopened `this.state.email` is back to null.

Comment 21

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8483099 [details] [diff] [review]
Patch for feedback

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

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #20)
> Niko, this patch should work but `this.state.email` is null when the panel
> is reopened after a successful login attempt. Do you know why the state is
> getting lost here?

That's because the panel is reset everytime it's reopened (bug 994146) — see implementation in PanelView's initialize() and _registerVisibilityChangeEvent() methods. I think it's because we want a brand new call URL being generated each time the user opens the panel, to avoid mistakenly reusing a previous used call URL. And also so the end user has already a call URL to use with no further action than opening the panel.

I've filed bug 1062126 to fix that situation (will mark it as a dependency for this one), so we won't be resetting the whole panel UI everytime the panel is opened; we just actually want a new call URL.

Dunno what to do with feedback status, setting feedback- because of the discovered issue.

::: browser/components/loop/content/js/panel.jsx
@@ +417,5 @@
> +      }
> +    },
> +
> +    componentDidMount: function() {
> +      window.addEventListener("message", this);

We need to register this at a lower level, eg. in <PanelView/>'s componentDidMount, and propagate the user profile through props to child components — like this one — because for example, <AuthLink/> and <SettingsDropdown/> will need this information as well.

@@ +438,5 @@
> +                          this.state.email :
> +                          __("display_name_guest");
> +      return (
> +        <p className="user-identity">
> +          {this.state.profile.email}

Careful, this.state.profile is undefined here. I guess this is WiP.

Comment 22

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

Attachment: Patch

Patch with tests that cover the code path up to MozLoopAPI.

Comment 23

[Matthew N. [:MattN]]

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

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

I'd like to review this again and test it.

::: browser/components/loop/MozLoopAPI.jsm
@@ +409,5 @@
>        }
>      },
>    };
>  
> +  let onStatusChanged = function(aSubject, aTopic, aData) {

Why not the more common syntax:
function onStatusChanged(aSubject, aTopic, aData) {

@@ +410,5 @@
>      },
>    };
>  
> +  let onStatusChanged = function(aSubject, aTopic, aData) {
> +    let event = new targetWindow.CustomEvent("LoopStatusChanged", {});

Nit: The 2nd argument is supposedly optional so you don't need {}

@@ +414,5 @@
> +    let event = new targetWindow.CustomEvent("LoopStatusChanged", {});
> +    targetWindow.dispatchEvent(event)
> +  };
> +
> +  let onDOMWindowDestroyed = function(aSubject, aTopic, aData) {

Ditto

@@ +418,5 @@
> +  let onDOMWindowDestroyed = function(aSubject, aTopic, aData) {
> +    if (aSubject != targetWindow)
> +      return;
> +    Services.obs.removeObserver(onDOMWindowDestroyed, "dom-window-destroyed", false);
> +    Services.obs.removeObserver(onStatusChanged, "loop-status-changed", false);

There are only 2 parameters to Services.obs.removeObserver (x2)

::: browser/components/loop/MozLoopService.jsm
@@ +931,5 @@
>        gFxAOAuthTokenData = null;
>        throw error;
> +    }).then(tokenData => {
> +      let FxAccountsProfileClient = {};
> +      Cu.import("resource://gre/modules/FxAccountsProfileClient.jsm", FxAccountsProfileClient);

I suspect we'll need this outside of this function (e.g. to check for profile updates) so we should just make this a lazy getter in global scope.

::: browser/components/loop/content/js/panel.jsx
@@ +436,5 @@
> +      if (aEvent.type != "LoopStatusChanged")
> +        return;
> +      this.setProps({
> +        email: navigator.mozLoop.userProfile.email,
> +        uid: navigator.mozLoop.userProfile.uid

In this file, you should update the references to navigator.mozLoop.loggedInToFxA to check the profile.

Also, this code should run at initialization too even without the event to handle the case where we are already logged in at startup.

::: browser/components/loop/content/shared/css/panel.css
@@ -152,5 @@
>  
>  /* Sign in/up link */
>  
>  .signin-link {
> -  display: none; /* XXX This should be displayed as soon bug 979845 lands */

Until bug 1061711 is fixed and it's follow-ups plus bug 1047667 for logout, I don't think we should make this visible as it won't work properly and users won't be able to logout.

::: browser/components/loop/test/mochitest/browser_fxa_login.js
@@ +161,5 @@
>      state: "state",
>    };
>    yield promiseOAuthParamsSetup(BASE_URL, params);
>  
> +  yield loadLoopPanel();

Are you sure you need this? If so, why?

Comment 24

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

Attachment: Patch v1.1

(In reply to Matthew N. [:MattN] from comment #23)
> Comment on attachment 8485936 [details] [diff] [review]
> Patch
> 
> Review of attachment 8485936 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd like to review this again and test it.
> 
> ::: browser/components/loop/MozLoopAPI.jsm
> @@ +409,5 @@
> >        }
> >      },
> >    };
> >  
> > +  let onStatusChanged = function(aSubject, aTopic, aData) {
> 
> Why not the more common syntax:
> function onStatusChanged(aSubject, aTopic, aData) {

Just personal style. I've changed it per your request.

> In this file, you should update the references to
> navigator.mozLoop.loggedInToFxA to check the profile.

I had held off from doing this as I didn't want the patch to start growing (as logOutFromFxA is not implemented either).

> ::: browser/components/loop/test/mochitest/browser_fxa_login.js
> @@ +161,5 @@
> >      state: "state",
> >    };
> >    yield promiseOAuthParamsSetup(BASE_URL, params);
> >  
> > +  yield loadLoopPanel();
> 
> Are you sure you need this? If so, why?

This was leftover from when I was trying to have the test look at the state of the HTML, but this doesn't seem so simple right now.

Comment 25

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

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

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

This generally looks good but you'll probably want to address some concerns here.

More importantly: content code needs testing, which is hard to achieve atm because we don't have any convenient way to mock native DOM apis like window.addEventListener and fake event dispatching (I'm working on solving this problem in bug 1062126).

::: browser/components/loop/content/js/panel.jsx
@@ +434,5 @@
> +    _setUserProfile: function() {
> +      this.setProps({
> +        email: navigator.mozLoop.userProfile.email,
> +        uid: navigator.mozLoop.userProfile.uid
> +      });

1. I'd rather use state to persist the current profile information here, so anytime it changes you get a rerender for free.

2. I can't see use of the uid field anywhere in the code.

3. How about passing the profile object instead of individual fields? Possibly YAGNI but I suspect we'll possibly want to use other properties in UserIdentity in a near future…

@@ +445,5 @@
> +    },
> +
> +    componentDidMount: function() {
> +      this._setUserProfile();
> +      window.addEventListener("LoopStatusChanged", this);

Nit: I'd rather implement & pass something like this._onAuthStatusChange instead of relying on the impl. of the EventListener interface here; to me:

- that would be more meaningfull;
- would allow defining other event listeners instead of relying on a sole handleEvent method;
- would match how it's done elsewhere in Loop content code.

@@ +455,5 @@
>  
>      render: function() {
> +      var displayName = this.props.email ?
> +                          this.props.email :
> +                          __("display_name_guest");

Nit: Shorter:

var displayName = this.props.email || __("display_name_guest");

Comment 26

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

(In reply to Nicolas Perriault (:NiKo`) from comment #25)
> More importantly: content code needs testing, which is hard to achieve atm
> because we don't have any convenient way to mock native DOM apis like
> window.addEventListener and fake event dispatching (I'm working on solving
> this problem in bug 1062126).

The cost/benefit here of mocking out native DOM apis doesn't seem like a good trade here. Are you expecting the DOM apis here to fail?

We could write a test that checks the content of the DOM after login has finished, but that is just waiting on bug 1062126, so nothing special is needed here besides fixing bug 1062126.
 
> ::: browser/components/loop/content/js/panel.jsx
> @@ +434,5 @@
> > +    _setUserProfile: function() {
> > +      this.setProps({
> > +        email: navigator.mozLoop.userProfile.email,
> > +        uid: navigator.mozLoop.userProfile.uid
> > +      });
> 
> 1. I'd rather use state to persist the current profile information here, so
> anytime it changes you get a rerender for free.

Your review in comment #21 said "propagate the user profile through props to child components". Did I misunderstand something here?

> 2. I can't see use of the uid field anywhere in the code.
> 3. How about passing the profile object instead of individual fields?
> Possibly YAGNI but I suspect we'll possibly want to use other properties in
> UserIdentity in a near future…

2 + 3 are contradictory. If we are worried about fields that we don't need, then we should remove them, not expand the surface area by passing the profile object. I can remove the `uid` property.

> @@ +445,5 @@
> > +    },
> > +
> > +    componentDidMount: function() {
> > +      this._setUserProfile();
> > +      window.addEventListener("LoopStatusChanged", this);
> 
> Nit: I'd rather implement & pass something like this._onAuthStatusChange
> instead of relying on the impl. of the EventListener interface here; to me:
> 
> - that would be more meaningfull;
> - would allow defining other event listeners instead of relying on a sole
> handleEvent method;
> - would match how it's done elsewhere in Loop content code.

`handleEvent` is a pattern that is heavily used within other Firefox chrome code. It is a well-defined pattern that has meaningful semantics. It scales well with multiple event listeners too (use a switch statement that defers to other functions if changes are too large). I'm not sure why we would need to do something different here.

Comment 27

[Matthew N. [:MattN]]

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

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

::: browser/components/loop/content/js/panel.jsx
@@ +206,5 @@
>        }
>      },
>  
>      _isSignedIn: function() {
> +      return !!navigator.mozLoop.userProfile;

!!{} === True so I think you would need .uid ideally (or .email)

Comment 28

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #26)
> The cost/benefit here of mocking out native DOM apis doesn't seem like a
> good trade here. Are you expecting the DOM apis here to fail?

No, I'd want to ensure that our code react to these events as expected.

> We could write a test that checks the content of the DOM after login has
> finished, but that is just waiting on bug 1062126, so nothing special is
> needed here besides fixing bug 1062126.

Patch for bug 1062126 suggests we introduce mixins responsible of listening to a specific DOM event and executing callbacks defined in the component implementing the mixin: https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1062126&attachment=8486425

Even if that doesn't solve the pb with simulating native DOM events, I'm finding this pattern easier to share & maintain. WDYT?

> > ::: browser/components/loop/content/js/panel.jsx
> > @@ +434,5 @@
> > > +    _setUserProfile: function() {
> > > +      this.setProps({
> > > +        email: navigator.mozLoop.userProfile.email,
> > > +        uid: navigator.mozLoop.userProfile.uid
> > > +      });
> > 
> > 1. I'd rather use state to persist the current profile information here, so
> > anytime it changes you get a rerender for free.
> 
> Your review in comment #21 said "propagate the user profile through props to
> child components". Did I misunderstand something here?

Possibly; basically I was thinking of:

getInitialState: function() {
  return {
    profile: this.props.userProfile || navigator.mozLoop.userProfile
  };
}

This initiates the profile state property with any profile object passed as a props (useful for simulating rendering in the UI component showcase), and falls back to fetching it right from the mozLoop API.

And thus, as updating state triggers a rerender of the component, once you receive the profile information out of the LoopStatusChanged event, you can update the state accordingly:

componentDidMount: function() {
  window.addEventListener("LoopStatusChanged", this._setUserProfile);
},

_setUserProfile: function(userProfile) {
  this.setState({userProfile: userProfile}); // rerenders the component with that updated information
}

> 2 + 3 are contradictory. If we are worried about fields that we don't need,
> then we should remove them, not expand the surface area by passing the
> profile object. 

I was suggesting removing both email and uid to only keep the profile object. Sorry if I was unclear :(

> `handleEvent` is a pattern that is heavily used within other Firefox chrome
> code.

Well, we're in content here. If we really want this, we also need to update the rest of the Loop content codebase. 

> It scales well with multiple event listeners too (use a switch statement 

That's matter of taste. I'm concerned this is a totally different direction the rest of the team has been taking for content dev since the beginning of this project though. I'd really want we keep consistent in the way we're dealing with event listening for now. Please file a bug so we can start a discussion about migrating to the new approch you're suggesting here. Thanks :)

Comment 29

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

(In reply to Matthew N. [:MattN] from comment #27)
> >      _isSignedIn: function() {
> > +      return !!navigator.mozLoop.userProfile;
> 
> !!{} === True so I think you would need .uid ideally (or .email)

Very good point :)

Comment 30

[Matthew N. [:MattN]]

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

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

::: browser/components/loop/MozLoopAPI.jsm
@@ +103,5 @@
> +    userProfile: {
> +      enumerable: true,
> +      get: function() {
> +        if (!MozLoopService.userProfile)
> +          return {};

(follow-up to my last comment): Or you can have null propagate to the DOM instead of using an empty object here and making it inconsistent with result from the service.

Comment 31

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

Attachment: Patch v1.2

I've addressed the issues found. I'm still not sure if I understood what you were asking in regards to the state vs. props, but I think this is what you meant.

Comment 32

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

Also, I added tests that check that the content reflects what the API is exposing.

Comment 33

[Matthew N. [:MattN]]

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

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

I was in the middle of reviewing your older revision:

::: browser/components/loop/MozLoopService.jsm
@@ +931,5 @@
>        return tokenData;
>      },
>      error => {
>        gFxAOAuthTokenData = null;
>        throw error;

I think you want to clear gFxAOAuthProfile here too. Whenever gFxAOAuthTokenData is cleared, gFxAOAuthProfile should be too (but not the converse).

::: browser/components/loop/test/mochitest/browser_fxa_login.js
@@ +7,5 @@
>  
>  "use strict";
>  
>  const gFxAOAuthTokenData = Cu.import("resource:///modules/loop/MozLoopService.jsm", {}).gFxAOAuthTokenData;
> +const gFxAOAuthProfile = Cu.import("resource:///modules/loop/MozLoopService.jsm", {}).gFxAOAuthProfile;

We can use object destructuring assignment here (like I'm going to in bug 1065052).

::: browser/components/loop/test/mochitest/head.js
@@ +107,1 @@
>                createInstance(Ci.nsIXMLHttpRequest);

Please clear gFxAOAuthProfile in resetFxA()

Comment 34

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

Attachment: Patch v1.3

Comment 35

[Matthew N. [:MattN]]

Comment on attachment 8486863 [details] [diff] [review]
Patch v1.3

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

::: browser/components/loop/MozLoopService.jsm
@@ +939,5 @@
> +        serverURL: gFxAOAuthClient.parameters.profile_uri,
> +        token: tokenData.access_token
> +      });
> +      client.fetchProfile().then(result => {
> +        gFxAOAuthProfile = result;

I'm undecided on whether we should have logintoFxA wait to resolve until we have the profile. In that case I think this is fine until it's more clear on what the desired behaviour is.

::: browser/components/loop/content/js/panel.jsx
@@ +461,1 @@
>              <AvailabilityDropdown />

It seems like you forgot to do the styling so that the username is above the availability selector. See https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#precall-signedin

The panel layout looks broken as-is IMO.

Comment 36

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8486863 [details] [diff] [review]
Patch v1.3

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

Looks better, thanks!

Things we might want to have when landing this:

- Tests (which now highly depends on bug 1062126 to land)
- Styling
- UI component showcase integration

We could probably pair on this, or with other people from the content team with better timezone compatibility :)

::: browser/components/loop/content/js/panel.jsx
@@ +461,1 @@
>              <AvailabilityDropdown />

Agreed. We should use the UI component showcase page (available in the ui/ directory) to ease styling this component without having to actually sign in to FxA.

Comment 37

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

Attachment: Patch v1.4

I've added the props to the PanelView so that this can be instrumented from the ui-showcase and also fixed the styling so that the username is above the Availability dropdown.

I didn't go much deeper with the styling as I'd like the styling of the panel to be split out in to a separate bug.

Comment 38

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

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

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

Looks good, though you'll need to update the ui-showcase.jsx file along its compiled version. r=me with this addressed. 

I'll take care of writing unit tests for the component. Please ensure to file bugs for this and the missing styles.

::: browser/components/loop/ui/ui-showcase.js
@@ +125,5 @@
> +            Example({summary: "Call URL retrieved - authenticated", dashed: "true", style: {width: "332px"}}, 
> +              PanelView({client: mockClient, notifications: notifications, 
> +                         callUrl: "http://invalid.example.url/",
> +                         userProfile: {email: "[e-mail]"}})
> +            ), 

Please update both the ui-showcase.jsx file as well as the compiled version.

Comment 39

[Matthew N. [:MattN]]

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

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

::: browser/components/loop/MozLoopService.jsm
@@ +1009,1 @@
>        throw error;

I think this catch function should be at the end of the chain so it can catch errors from your function below too. i.e. move it to .catch(…) after your function.

@@ +1015,5 @@
> +      client.fetchProfile().then(result => {
> +        gFxAOAuthProfile = result;
> +        MozLoopServiceInternal.notifyStatusChanged();
> +      }, error => {
> +        Cu.reportError("Failed to retrieve profile: " + error.errno);

You could also use console.error("Failed to retrieve profile: ", error) so the whole error object is shown.

Comment 40

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

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

Comment 41

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

Pushed a follow-up to fix panel_test.js,
https://hg.mozilla.org/integration/fx-team/rev/26b240144648

Comment 42

[Ryan VanderMeulen [:RyanVM]]

Backed out for the mochitest-bc crashes covered by bug 1066333.
https://hg.mozilla.org/integration/fx-team/rev/f60d9f6955e6

Comment 43

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

Attachment: rebased patch

Comment 44

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

Hey Matt, after http://hg.mozilla.org/mozilla-central/rev/2767fb9328d0 landed, I get test failures in browser_fxa_login.js::loginWithParams401 and browser_fxa_login.js::loginWithRegistration401. The error.code is null and the error has a message of NS_ERROR_OFFLINE.

I think the server URL is getting corrupted here, through debugging I'm seeing a request to http://localhost//fxa-oauth/params.

http://screencast.com/t/6g0EPTwvu

Can you help me out here?

Comment 45

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

Attachment: Patch v1.5

I figured out the cause for the test failures. In head.js, loadLoopPanel was changing the loop server URL and also going into offline mode (hence the wrong URL and also the NS_ERROR_OFFLINE).

I fixed this by adding in the ability to tweak the behavior of loadLoopPanel, since it is already being used in other tests.

The crash appears to be fixed by adding going from:
if (aSubject != targetWindow)
to
if (targetWindow && aSubject != targetWindow)

This was following the code at http://hg.mozilla.org/mozilla-central/annotate/3b7921328fc1/browser/modules/test/browser_UITour_detach_tab.js#l35.

I pushed this to tryserver, https://tbpl.mozilla.org/?tree=Try&rev=573848ffd985

If for some reason we still see failures here, bholley said we could go with https://hg.mozilla.org/try/rev/b4d471628c58 but that will effectively remove the MOZ_ASSERT a few lines below that checks for a non-null winObj.

Comment 46

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

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #45)
> If for some reason we still see failures here, bholley said we could go with
> https://hg.mozilla.org/try/rev/b4d471628c58 but that will effectively remove
> the MOZ_ASSERT a few lines below that checks for a non-null winObj.

For posterity, since Try server is cleaned periodically,

     1.1 --- a/dom/base/nsGlobalWindow.cpp
     1.2 +++ b/dom/base/nsGlobalWindow.cpp
     1.3 @@ -1954,17 +1954,17 @@ nsGlobalWindow::TraceGlobalJSObject(JSTr
     1.4  
     1.5  /* static */
     1.6  JSObject*
     1.7  nsGlobalWindow::OuterObject(JSContext* aCx, JS::Handle<JSObject*> aObj)
     1.8  {
     1.9    nsGlobalWindow* origWin = UnwrapDOMObject<nsGlobalWindow>(aObj);
    1.10    nsGlobalWindow* win = origWin->GetOuterWindowInternal();
    1.11  
    1.12 -  if (!win) {
    1.13 +  if (!win || !win->FastGetGlobalJSObject()) {
    1.14      // If we no longer have an outer window. No code should ever be
    1.15      // running on a window w/o an outer, which means this hook should

Comment 47

[Matthew N. [:MattN]]

Comment on attachment 8490216 [details] [diff] [review]
Patch v1.5

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

You can probably fold the commit message now.

r=me

::: browser/components/loop/test/mochitest/head.js
@@ +57,5 @@
>   * It also registers
>   *
>   * This assumes that the tests are running in a generatorTest.
>   */
> +function loadLoopPanel(aOverrideOptions={}) {

Nit: spaces around the assignment operator

@@ +62,3 @@
>    // Set prefs to ensure we don't access the network externally.
> +  Services.prefs.setCharPref("services.push.serverURL", aOverrideOptions.pushURL || "ws://localhost/");
> +  Services.prefs.setCharPref("loop.server", aOverrideOptions.loopURL || "http://localhost/");

Ideally this pref stuff wouldn't be done in this function and would be done in the pref files for the harnesses. That should be fine for loop.server but maybe not for services.push.serverURL which is also used outside of Loop.

Comment 48

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

Addressed review comments and pushed to fx-team:
https://hg.mozilla.org/integration/fx-team/rev/fe5776e8ed1c

Try push shows that the crash has been fixed and is all green:
https://tbpl.mozilla.org/?tree=Try&rev=573848ffd985

Comment 49

[Ryan VanderMeulen [:RyanVM]]

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

Comment 50

[u279076]

Until dependencies are resolved this will not be verifiable by QE.

Comment 51

[u279076]

Paul, this should now be unblocked for verification. 

Please test in the latest Nightly and the following Try builds:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f9eb2cbac352/

Thanks

Comment 52

[u279076]

Upon further review of the pre-uplift verifications I don't think verification of this needs to block uplift. Paul, please just verify against Nightly and we can make sure this is covered for 34 via Moztrap.

Comment 53

[Paul Silaghi, QA [:pauly]]

After login, the user's email address is properly displayed in the Loop panel footer.
Verified fixed 35.0a1 (2014-09-29) Win 7

Comment 54

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

Comment on attachment 8490216 [details] [diff] [review]
Patch v1.5

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

Comment 55

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/94dadf55808a
https://hg.mozilla.org/releases/mozilla-aurora/rev/1062e41cfaf8
https://hg.mozilla.org/releases/mozilla-aurora/rev/2d94ee52c6d8

Comment 56

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

Comment on attachment 8490216 [details] [diff] [review]
Patch v1.5

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 57

[u279076]

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