1000240 - Standalone UI for link clickers needs "call failed" visual notification

Status: RESOLVED FIXED

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

Comment 1

[:shell escalante]

UI: https://people.mozilla.org/~dhenein/labs/loop-link-spec/#call-failed

Comment 2

[Romain Testard [:RT]]

Attachment: Call Failed.mp3

Initial sound file which may change later.

Comment 3

[Romain Testard [:RT]]

Please note that as per discussion with Arcadio yesterday we should not be using the "Firefox Hello" brand for now (likely we'll use it when we get to beta). For Nightly and Aurora we should replace "Hello" by "WebRTC" wherever "Hello" appears in the UX and also use the Firefox logo instead of the speech bubble for now.

Comment 4

[Mark Banner (:standard8)]

Moving the GMP case to bug 1030097, that should be able to handle everything here.

Comment 5

[Mark Banner (:standard8)]

Moving most use cases to bug 1046959.

Comment 6

[Andrei Oprea [mention me on [e-mail]]]

Attachment: Implement call failed UI in standalone view

Comment 7

[Andrei Oprea [mention me on [e-mail]]]

On call failed the current implementation navigates to /home. Do we still want that now that we have a "Retry" button?
The way the patch works is that on retry you attempt to call with the same call type (audio / audio+video) do we want an option to repick on retry?

Comment 8

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

(In reply to Andrei Oprea [:andreio] from comment #7)
> On call failed the current implementation navigates to /home. Do we still
> want that now that we have a "Retry" button?

Nope, you can remove that redirection. The /home route is still needed as a default catch-all route though; maybe be we should rename it to "default".

Comment 9

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

Comment on attachment 8484723 [details] [diff] [review]
Implement call failed UI in standalone view

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

I think sound handling wants to be moved into the mozLoop api. Also, I can see room for component code factorization here.

::: browser/components/loop/standalone/content/js/webapp.jsx
@@ +206,5 @@
>  
> +    _loadCallErrorSound: function() {
> +      var assetURL = location.origin + "/content/shared/sounds/call-failed.mp3";
> +      this.state.errorSoundNotification = new Audio(assetURL);
> +    },

As we already have sound alerts being handled in gecko land (which shouldn't imho, as sound is content to me), we'll probably want to move this over there as well, so everything sound management is handled in the same place.

@@ +281,5 @@
>        var tosClasses = React.addons.classSet({
>          "terms-service": true,
>          hide: (localStorage.getItem("has-seen-tos") === "true")
>        });
> +      var displayCallFailedMsg = React.addons.classSet({

Nit: This needs a "Classes" suffix, "Msg" is a bit misleading.

@@ +284,5 @@
>        });
> +      var displayCallFailedMsg = React.addons.classSet({
> +        "hide": !this.state.callFailed
> +      });
> +      var displayInitiateCallMsg = React.addons.classSet({

You could alias React.addons.classSet to cx here, as it's common convention in React.

@@ +396,5 @@
> +            <div className="flex-padding-1"></div>
> +          </div>
> +          /* jshint ignore:end */
> +        );
> +      }

The two different outputs depending on that if test probably wants to be two different components.

Comment 10

[Mark Banner (:standard8)]

(In reply to Andrei Oprea [:andreio] from comment #7)
> The way the patch works is that on retry you attempt to call with the same
> call type (audio / audio+video) do we want an option to repick on retry?

No, lets go simple for now, if its required later we can always re-add it.

Comment 11

[Mark Banner (:standard8)]

(In reply to Nicolas Perriault (:NiKo`) from comment #9)
> > +    _loadCallErrorSound: function() {
> > +      var assetURL = location.origin + "/content/shared/sounds/call-failed.mp3";
> > +      this.state.errorSoundNotification = new Audio(assetURL);
> > +    },
> 
> As we already have sound alerts being handled in gecko land (which shouldn't
> imho, as sound is content to me), we'll probably want to move this over
> there as well, so everything sound management is handled in the same place.

I thought that sound management was in gecko to prevent two ringing noises at the same time. However, I might be miss-remembering, and this could be due to the content/chrome situation.

Hopefully abr can remember...

Comment 12

[:shell escalante]

Hey Andrei, i remember we moved this to blocked - have the replies below unblocked it?

Comment 13

[Andrei Oprea [mention me on [e-mail]]]

Visual uplift was blocking. I updated the trello board. Thanks.

Comment 14

[Andrei Oprea [mention me on [e-mail]]]

Attachment: Implement call failed UI in standalone view

Comment 15

[Adam Roach [:abr]]

(In reply to Mark Banner (:standard8) from comment #11)
> (In reply to Nicolas Perriault (:NiKo`) from comment #9)
> > > +    _loadCallErrorSound: function() {
> > > +      var assetURL = location.origin + "/content/shared/sounds/call-failed.mp3";
> > > +      this.state.errorSoundNotification = new Audio(assetURL);
> > > +    },
> > 
> > As we already have sound alerts being handled in gecko land (which shouldn't
> > imho, as sound is content to me), we'll probably want to move this over
> > there as well, so everything sound management is handled in the same place.
> 
> I thought that sound management was in gecko to prevent two ringing noises
> at the same time. However, I might be miss-remembering, and this could be
> due to the content/chrome situation.

There's nothing that stops two ringtones from playing on top of each other right now. And playing sounds from MozLoopAPI is, believe it or not, actually more difficult than playing from content: you can't have an audio element without a DOM, and we don't have a DOM in MozLoopAPI. If you look at the code, you'll see that we have to jump through some kind of odd hoops to make this work.

 
> Hopefully abr can remember...

There were a handful of considerations that went into putting the ringtone into privileged code. In part, they included factors like:

 * I wanted the ability to add new behavior (e.g. Growl or Growl-like integration) to
   incoming call alerts, and it seemed cleaner to have a single MozLoopAPI function
   that performed all of the alerting behavior.

 * I wanted the ringtone iteto be a pref that points to arbitrary URLs (e.g., chrome: or file:)
   so that advanced users (or a really simple add-on) could easily swap out the ringtone
   for one of their choosing. In order to be able to point at file: and chrome: URLs, we needed
   the audio playout to have non-content permissions.

 * When Firefox is running but no windows are open (as can happen on OS X), we're going
   to want to support playing ringtone for inbound calls, even though there cannot be
   an associated loop panel. I note that this currently does not work, but we really don't
   want to be forced rearchitect our sound playout when we address this situation.

I've got a nagging voice in the back of my head that says there was a fourth reason (and maybe more), but I can't for the life of me recall what it was.

In any case, none of the reasons that I can remember are applicable to the other tones that we'll be playing. And ringtone is also somewhat unique in that it's the only sound that the standalone client never has to worry about.

So for sounds other than the ringtone, I think a (mostly) unified, content-permissions-based system with hardcoded pointers to the sound assets is a reasonable design. (I say "mostly" because I think we'll eventually want hooks that do things like: check if mozLoop is defined; and, if so, retrieve the user's configured "loop tone volume" pref and apply it to sound playout -- but, as long as we can check for mozLoop and have default behavior when it isn't defined, it's still pretty much unified).

That said: what is the value of "location.origin" in about:looppanel? I'm surprised that |location.origin + "/content/shared/sounds/call-failed.mp3"| works, given our continuing struggles around origins.

(needinfo because) Andrei: Have you tested that this approach works in the desktop client?

Comment 16

[Andrei Oprea [:andreio]]

This code is for standalone only. We already have audio notifications in the desktop client.

Comment 17

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

Comment on attachment 8485303 [details] [diff] [review]
Implement call failed UI in standalone view

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

I'm a bit overwhelmed with work so I've only spent 20mn reviewing this patch, so it's probably fully reviewed. But I've seen many things you'll probably want to revamp before I make another pass :)

::: browser/components/loop/content/shared/css/common.css
@@ +79,5 @@
>   * together with .btn-*
>   */
>  
> +.btn,
> +.standalone-btn-retry-call {

This is too specific to be defined in common.css (most likely because this is not "common" as it's standalone specific).

Why not applying the .btn class to the element targeted by this .standalone-btn-retry-call selector instead?

::: browser/components/loop/standalone/content/css/webapp.css
@@ +179,5 @@
>    }
>  }
>  
> +.btn-large,
> +.standalone-btn-retry-call {

Why don't just apply .btn-large to the element initially targeted by this .standalone-btn-retry-call selector?

::: browser/components/loop/standalone/content/js/webapp.jsx
@@ +167,5 @@
>      },
>  
>      componentDidMount: function() {
>        // Listen for events & hide dropdown menu if user clicks away
> +      window.addEventListener("click", this._clickHandler);

You should have a look at the DropdownMenuMixin we created for the panel; it could be easily reused here if shared.

@@ +193,5 @@
> +      this.setState({
> +        // Display call failed error message
> +        callFailed: true,
> +        // Enable the retry button
> +        disableCallRetryButton: false

For each case, callFailed & disableCallRetryButton are always inversed; really you only need a single value here.

@@ +205,5 @@
>      },
>  
> +    _loadCallErrorSound: function() {
> +      var assetURL = location.origin + "/content/shared/sounds/call-failed.mp3";
> +      this.state.errorSoundNotification = new Audio(assetURL);

A sound file shouldn't be attached to state. We should rather define the Audio object as a local property. And start playing it only when the call has failed.

@@ +233,5 @@
> +      this.setState({
> +        // Disable the retry button
> +        disableCallRetryButton: true,
> +        // Remove the call failed message
> +        callFailed: false

See previous comment on always inverted values.

@@ +281,5 @@
> +      var displayCallFailedMsgClasses = cx({
> +        "hide": !this.state.callFailed
> +      });
> +      var displayInitiateCallMsg = cx({
> +        "hide": this.state.callFailed,

I'm uneasy dealing with element visibility using CSS; rather, React allows us to render a given subset of UI depending on state, which is probably what we want here.

@@ +299,5 @@
>  
> +            <div className={displayCallFailedMsgClasses}>
> +              <div className="standalone-call-loading-failed"></div>
> +              <p className="standalone-call-btn-label">
> +                {mozL10n.get("generic_failure_title")} {mozL10n.get("generic_failure_no_reason2")}

Nit: The second message should be put on a new line, for legibility.

@@ +350,5 @@
> +      });
> +      return (
> +        /* jshint ignore:start */
> +        <div className={callBtnClasses}>
> +          <div className="flex-padding-1"></div>

Do we actually want such non semantic class names? I don't think so :(

@@ +384,5 @@
> +              </ul>
> +
> +            </div>
> +          </div>
> +          <div className="flex-padding-1"></div>

:(

@@ +412,5 @@
> +            disabled={this.props.disableRetryBtn}
> +            onClick={this.props.retryCall} >
> +            {mozL10n.get("retry_call_button")}
> +          </button>
> +          <div className="flex-padding-1"></div>

:(

Comment 18

[Andrei Oprea [mention me on [e-mail]]]

(In reply to Nicolas Perriault (:NiKo`) from comment #17)
> Comment on attachment 8485303 [details] [diff] [review]
> Implement call failed UI in standalone view
> 
> Review of attachment 8485303 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm a bit overwhelmed with work so I've only spent 20mn reviewing this
> patch, so it's probably fully reviewed. But I've seen many things you'll
> probably want to revamp before I make another pass :)
> 
> ::: browser/components/loop/content/shared/css/common.css
> @@ +79,5 @@
> >   * together with .btn-*
> >   */
> >  
> > +.btn,
> > +.standalone-btn-retry-call {
> 
> This is too specific to be defined in common.css (most likely because this
> is not "common" as it's standalone specific).
> 
> Why not applying the .btn class to the element targeted by this
> .standalone-btn-retry-call selector instead?
> 
> ::: browser/components/loop/standalone/content/css/webapp.css
> @@ +179,5 @@
> >    }
> >  }
> >  
> > +.btn-large,
> > +.standalone-btn-retry-call {
> 
> Why don't just apply .btn-large to the element initially targeted by this
> .standalone-btn-retry-call selector?
> 

I am trying to mimic the behavior of a @extend functionality [0] which most CSS preprocessors have.

It doesn't look nice but the idea here is for a CSS class selector to contain all the styles required to make up that component.

A dev shouldn't have to know that it also has to add ".btn" to make it work. Also it's easier to debug in the Developer Tools because you don't have styles composed from X number of places and different selectors.

I'm also not happy about it being in common.css but it avoids code duplication. With this in mind do you still think I should make the change ?

[0] http://learnboost.github.io/stylus/docs/extend.html

> 
> @@ +281,5 @@
> > +      var displayCallFailedMsgClasses = cx({
> > +        "hide": !this.state.callFailed
> > +      });
> > +      var displayInitiateCallMsg = cx({
> > +        "hide": this.state.callFailed,
> 
> I'm uneasy dealing with element visibility using CSS; rather, React allows
> us to render a given subset of UI depending on state, which is probably what
> we want here.
> 

This handles rendering of a non-component piece inside of a component. It feels to difficult do to otherwise. Do you have any suggestions ? 

> @@ +350,5 @@
> > +      });
> > +      return (
> > +        /* jshint ignore:start */
> > +        <div className={callBtnClasses}>
> > +          <div className="flex-padding-1"></div>
> 
> Do we actually want such non semantic class names? I don't think so :(
> 

It's not supposed to be "semantic" in the HTML sense of the word. It won't be parsed by a screen reader or any machine. It completely describes it's behavior/effect to a developer; probably adding in "box" would make that clearer.

Do you have any suggestions ?

Comment 19

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

(In reply to Andrei Oprea [:andreio] from comment #18)
> I am trying to mimic the behavior of a @extend functionality [0] which most
> CSS preprocessors have.
> 
> It doesn't look nice but the idea here is for a CSS class selector to
> contain all the styles required to make up that component.

I'm concerned that we're trying to mimic the output of some tooling we don't use… If we were using a preprocessor, we would maintain the source file in its original format, which would make a lot of sense, but here…

> A dev shouldn't have to know that it also has to add ".btn" to make it work.

It's probably matter of documentation; projects like bootsrap or foundation use class composition a lot and I've not seen many devs complaining about it.

> Also it's easier to debug in the Developer Tools because you don't have
> styles composed from X number of places and different selectors.

I'm personally finding the DevTools quite efficient at displaying the ordered list of applied styles for a given element, but that may just be me?

> With this in mind do you still think I should make the change ?

Yes. But you'll probably want to hear from other people about this if it's a high concern :)

> > @@ +281,5 @@
> > > +      var displayCallFailedMsgClasses = cx({
> > > +        "hide": !this.state.callFailed
> > > +      });
> > > +      var displayInitiateCallMsg = cx({
> > > +        "hide": this.state.callFailed,
> > 
> > I'm uneasy dealing with element visibility using CSS; rather, React allows
> > us to render a given subset of UI depending on state, which is probably what
> > we want here.
> > 
> 
> This handles rendering of a non-component piece inside of a component. It
> feels to difficult do to otherwise. Do you have any suggestions ? 

render: function() {
  if (this.state.callFailed) {
    return <CompA/>;
  } else {
    return <CompB/>;
  }
}

But maybe I'm missing the exact concern here?

> It's not supposed to be "semantic" in the HTML sense of the word. It won't
> be parsed by a screen reader or any machine. 

But it will be parsed by humans ;)

> Do you have any suggestions ?

I'd just add the required padding information to targeted selector style rules, but once more time that may just be me and matter of taste. Don't hesitate requiring input from other people about this :)

Comment 20

[Andrei Oprea [mention me on [e-mail]]]

(In reply to Nicolas Perriault (:NiKo`) from comment #19)
> (In reply to Andrei Oprea [:andreio] from comment #18)
> > 
> > This handles rendering of a non-component piece inside of a component. It
> > feels to difficult do to otherwise. Do you have any suggestions ? 
> 
> render: function() {
>   if (this.state.callFailed) {
>     return <CompA/>;
>   } else {
>     return <CompB/>;
>   }
> }
> 
> But maybe I'm missing the exact concern here?
> 

There is only 1 component that displays either title1 or title2 inside of it. It doesn't make sense to me to make two components out of it (the overhead would be greater). See [0] & [1]

[0] https://github.com/piatra/gecko-dev/blob/0d4b21482a0970de43421da1d586f8f6f0bbab43/browser/components/loop/standalone/content/js/webapp.jsx#L287
[1] https://github.com/piatra/gecko-dev/blob/0d4b21482a0970de43421da1d586f8f6f0bbab43/browser/components/loop/standalone/content/js/webapp.jsx#L291

Comment 21

[Andrei Oprea [mention me on [e-mail]]]

Attachment: Implement call failed UI in standalone view

Comment 22

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

Comment on attachment 8487498 [details] [diff] [review]
Implement call failed UI in standalone view

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

Still a bunch of little things to fix here and there, but it's shaping good.

::: browser/components/loop/content/js/conversation.jsx
@@ +33,2 @@
>      componentDidMount: function() {
> +      window.addEventListener("blur", this.hideDropdownMenu);

This is probably something to add to the DropdownMenuMixin.

@@ +35,4 @@
>      },
>  
>      componentWillUnmount: function() {
> +      window.removeEventListener("blur", this.hideDropdownMenu);

Same.

@@ +57,5 @@
>      },
>  
>      _toggleDeclineMenu: function() {
> +      var currentState = this.state.showMenu;
> +      this.setState({showMenu: !currentState});

Nit: How about more simply

this.setState({showMenu: !this.state.showMenu});

::: browser/components/loop/content/shared/js/views.jsx
@@ +764,5 @@
> +
> +    hideDropdownMenu: function() {
> +      this.setState({showMenu: false});
> +    }
> +  };

Patch for bug 1062126 moves this mixin to a new `mixins` module, with separated tests and a way to properly emulate native DOM event in them; we should probably wait for it to land first…

You could also pick these changes and update this patch. We could also land this as a first step, and I'll update patch for bug 1062126. Whatever.

::: browser/components/loop/standalone/content/js/webapp.jsx
@@ +155,2 @@
>          disableCallButton: false,
> +        errorSoundNotification: false,

This is no more needed.

@@ +202,5 @@
>      },
>  
> +    _loadCallErrorSound: function() {
> +      var assetURL = location.origin + "/content/shared/sounds/call-failed.mp3";
> +      this.errorSoundNotification = new Audio(assetURL);

How about loading the sound as soon as it's attached to the component prototype? We could avoid loading delay before playing… But that would be loading data even if no error ever occurs. Though as this file would be cached client-side once loaded, it's probably no big deal.

@@ +257,5 @@
> +      if(!this.state.callFailed) {
> +        return (
> +          <ConversationViewCallBtn showMenu={this.state.showMenu}
> +                          initiateCall={this.initiateOutgoingCall}
> +                          disableCallButton={this.state.disableCallButton}

How about passing !this.state.callFailed here? Basically, callFailed and disableCallButton carry the same (inverted) information; so we could get rid of keeping state for disableCallButton in this very component.

@@ +325,5 @@
> +        <div>
> +          <p className="standalone-call-btn-label">
> +            {mozL10n.get("initiate_call_button_label2")}
> +          </p>
> +          <div id="messages"></div>

Note that notifications seems to be broken atm. This is probably to be addressed in a new bug to file.

@@ +379,5 @@
> +          <div className="standalone-call-btn-label">
> +            <div className="standalone-call-loading-failed"></div>
> +            <p className="standalone-call-btn-label">
> +              {mozL10n.get("generic_failure_title")}
> +               {mozL10n.get("generic_failure_no_reason2")}

Nit: indentation.

::: browser/components/loop/ui/ui-showcase.jsx
@@ +209,5 @@
> +              <div className="standalone">
> +                <StartConversationView model={mockConversationModel}
> +                                       client={mockClient}
> +                                       notifications={notifications}
> +                                       showCallOptionsMenu={true}

This is no more used, it's showMenu.

Comment 23

[Andrei Oprea [mention me on [e-mail]]]

(In reply to Nicolas Perriault (:NiKo`) from comment #22)
> Comment on attachment 8487498 [details] [diff] [review]
> Implement call failed UI in standalone view
> 
> Review of attachment 8487498 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 
> @@ +202,5 @@
> >      },
> >  
> > +    _loadCallErrorSound: function() {
> > +      var assetURL = location.origin + "/content/shared/sounds/call-failed.mp3";
> > +      this.errorSoundNotification = new Audio(assetURL);
> 
> How about loading the sound as soon as it's attached to the component
> prototype? We could avoid loading delay before playing… But that would be
> loading data even if no error ever occurs. Though as this file would be
> cached client-side once loaded, it's probably no big deal.
> 

I'm confused about this. So right now the sound is loaded at component load time and played on error (therefore no delay). What is wrong with this approach ?

> @@ +257,5 @@
> > +      if(!this.state.callFailed) {
> > +        return (
> > +          <ConversationViewCallBtn showMenu={this.state.showMenu}
> > +                          initiateCall={this.initiateOutgoingCall}
> > +                          disableCallButton={this.state.disableCallButton}
> 
> How about passing !this.state.callFailed here? Basically, callFailed and
> disableCallButton carry the same (inverted) information; so we could get rid
> of keeping state for disableCallButton in this very component.

It's not always like that. Both components (retry button and call button) need to go to a disabled state after clicking (after initiating a call). If callFailed=false (at initial standalone page load) and button.disabled=!callFailed you will never be able to initiate a call...(I think?!)

> 
> @@ +325,5 @@
> > +        <div>
> > +          <p className="standalone-call-btn-label">
> > +            {mozL10n.get("initiate_call_button_label2")}
> > +          </p>
> > +          <div id="messages"></div>
> 
> Note that notifications seems to be broken atm. This is probably to be
> addressed in a new bug to file.

No need. Handled in this one.

Comment 24

[Andrei Oprea [mention me on [e-mail]]]

Attachment: Implement call failed UI in standalone view

Comment 26

[Andrei Oprea [mention me on [e-mail]]]

Attachment: Implement call failed UI in standalone view

Comment 27

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

Comment on attachment 8489776 [details] [diff] [review]
Implement call failed UI in standalone view

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

This patch introduces a bug which makes restarting a call opening two conversation windows. After investigating this with :Standard8, we decided that this feature really wants to wait for the ongoing refactoring of the Webapp router as a component (in bug 1066567) to land first, so we have a better ground to build stuff like this.

::: browser/components/loop/standalone/content/js/webapp.jsx
@@ +16,5 @@
>  
>    var sharedModels = loop.shared.models,
>        sharedViews = loop.shared.views,
> +      baseServerUrl = loop.config.serverUrl,
> +      DropdownMenuMixin = loop.shared.views.DropdownMenuMixin;

This is broken, the mixin is in loop.shared.mixins.

Comment 28

[Maire Reavy [:mreavy]]

Niko, Mark -- Are we going to use this bug to give us  "call failed" visual and audio notification for Desktop as well as Standalone (as part of the rework for direct calling)?  Or will there be a separate follow up for Desktop?   Will we handle bug 1047410 at roughly the same time?

Comment 29

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

Attachment: Added a Call Failed view for Loop standalone (WiP).

This is a WiP, at this point I'm just asking for early feedback to check that I'm on the right track.

Part of the patch has been taken from the one previously attached to the bug, so there might be obsolete things to remove. Tests are to be written/updated. But it works, so I mainly ask for feedback regarding the call state handling, and the flow.

Note: The CSS is a complete mess. The flex box model we use in there is hardly maintainable, I think we'll need to reconsider this choice asap, or at least to identify a set of maintainers with expertise in this area (not me, please).

Comment 30

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

Attachment: Added a Call Failed view for Loop standalone.

Factored call initiation button as a reusable component. Still needs test updates and style polishing.

Comment 31

[Mark Banner (:standard8)]

Comment on attachment 8494841 [details] [diff] [review]
Added a Call Failed view for Loop standalone.

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

I like where this is going. We might want to consider, for some of the existing notifications, putting out the notification failure text (rather than the generic), but its probably worth considering those situations later, once we get the fact that it has failed in.

I definitely like the "wrapper" view, that will make things a lot easier!

::: browser/components/loop/standalone/content/js/webapp.jsx
@@ +725,5 @@
>       */
>      _handleCallTerminated: function(reason) {
> +      if (reason === "cancel") {
> +        this.setState({callStatus: "start"});
> +      } else {

nit: generally in this sort of situation, I think its preferred to do an early return, and drop the section, as it reduces indent-complexity.

Comment 32

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

Attachment: Added a Call Failed view for Loop standalone.

Updated patch, with tests and "working" styles - which needs updating and refactoring; I'll find bugs about this.

Also, not all the styles have been implemented, and the audio notification is missing. That's why this is a part 1, but this could land imho.

Comment 33

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

Attachment: Added a Call Failed view for Loop standalone.

Updated patch, rebased against latest fx-team. Created a new InitiateConversationView which is shared by the StartConversationView and a new FailedConversationView (yeah, naming things gets hard).

Comment 34

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

Attachment: Added a Call Failed view for Loop standalone.

Updated patch with missing bits for the UI showcase.

Comment 35

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

Attachment: Added a Call Failed view for Loop standalone.

Removed obsolete code, added missing tests for model changes.

Comment 36

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

Attachment: Added a Call Failed view for Loop standalone.

Code removal again, fixed positionning of conversation footer. Sorry for bugnoise.

Comment 37

[Mark Banner (:standard8)]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #28)
> Niko, Mark -- Are we going to use this bug to give us  "call failed" visual
> and audio notification for Desktop as well as Standalone (as part of the
> rework for direct calling)?  Or will there be a separate follow up for
> Desktop?   Will we handle bug 1047410 at roughly the same time?

I think there will be a separate follow-up for desktop. It isn't sensible to share the view as I first expected.

Comment 38

[Mark Banner (:standard8)]

Comment on attachment 8495883 [details] [diff] [review]
Added a Call Failed view for Loop standalone.

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

Ok, looks good, with one nit, which I'll fix on checking.

::: browser/components/loop/test/standalone/webapp_test.js
@@ +656,2 @@
>  
> +         var button = view.getDOMNode().querySelector(".start-audio-only-call");

nit: something up with the indentation in this section.

Comment 39

[Mark Banner (:standard8)]

Marking as leave-open as I think :NiKo` said this was effectively a part 1. :NiKo` if this is wrong, please just remove the keyword.

Landed here:

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

Comment 40

[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/04e0091575b9 for marionette failures:

https://tbpl.mozilla.org/php/getParsedLog.php?id=49008339&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=49009152&tree=Fx-Team

Comment 41

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

It seems this has hit the intermittent failure mentioned in bug 1054793 (grep for `self.marionette.find_element("id", 'complete')`); marking 1054793 as a blocker.

Comment 42

[Mark Banner (:standard8)]

From the logs:

16:46:07     INFO -  TEST-START | test_standalone_all.py TestDesktopUnits.test_units
16:46:16     INFO -  mozcrash Saved minidump as /builds/slave/test/build/blobber_upload_dir/13be9307-5298-d5e6-551a46cb-5b36b9cc.dmp
16:46:16     INFO -  mozcrash Saved app info as /builds/slave/test/build/blobber_upload_dir/13be9307-5298-d5e6-551a46cb-5b36b9cc.extra
16:46:16    ERROR -  PROCESS-CRASH | test_standalone_all.py TestDesktopUnits.test_units | application crashed [None]
16:46:16     INFO -  Crash dump filename: /tmp/tmpOmA5Gp.mozrunner/minidumps/13be9307-5298-d5e6-551a46cb-5b36b9cc.dmp
16:46:16     INFO -  No symbols path given, can't process dump.
16:46:16     INFO -  MINIDUMP_STACKWALK not set, can't process dump.
16:46:16    ERROR -  TEST-UNEXPECTED-ERROR | test_standalone_all.py TestDesktopUnits.test_units | IOError: Connection to Marionette server is lost. Check gecko.log (desktop firefox) or logcat (b2g) for errors.

So it looks like some sort of crash. From the detailed gecko logs downloaded via tbpl, I'm seeing:

Assertion failure: [infer failure] Missing type in object [0x140205ac0] stopListening: <0x14047f840>, at /builds/slave/fx-team-osx64-d-00000000000000/build/js/src/jsinfer.cpp:288
Hit MOZ_CRASH() at /builds/slave/fx-team-osx64-d-00000000000000/build/js/src/jsinfer.cpp:289

Unfortunately these builds don't have symbols available, although I don't think that blocks us resolving it, we do however need to find out why this is crashing.

I didn't see it crashing locally just before I landed it, but I'll update my tree and try again.

Comment 43

[Mark Banner (:standard8)]

Filed bug 1073991 as I can't see any reason for the assertion.

Comment 44

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

Filed bug 1075509 as a spin-off of this bug, for audio notification, so we can land the current patch.

Comment 45

[Mark Banner (:standard8)]

Relanded now the dependent bug is fixed (also with bitrot fixes).

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

Comment 46

[Maire Reavy [:mreavy]]

Niko -- With bug 1075509 (for the sound notification) now filed, can we remove the "leave open" on this?

Comment 47

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

It really depends if we want to fully match the UX specs (the patch misses some of the styles, and the wheel icon). We could totally file a new bug for these remaining bits and close this one, which has long lived.

Comment 48

[Maire Reavy [:mreavy]]

(In reply to Nicolas Perriault (:NiKo`) — needinfo me if you need my attention from comment #47)
> It really depends if we want to fully match the UX specs (the patch misses
> some of the styles, and the wheel icon). We could totally file a new bug for
> these remaining bits and close this one, which has long lived.

I think that probably makes sense to file a follow up for the remaining work.  It will be easier for someone to pick up because the scope and bug history is much smaller (and more digestible).

Comment 49

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

https://hg.mozilla.org/mozilla-central/rev/736437b0f970

Comment 50

[Mark Banner (:standard8)]

In landing the original patch and fixing bitrot, conversation.js wasn't re-generated, I've now fixed that here:

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

Comment 51

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5d0c25294add

Comment 52

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

Filed bug 1080540 as a follow up for the missing wheel icon.

Comment 53

[Maire Reavy [:mreavy]]

With bug 1075509 and bug 1080540 filed, we can close this bug.

Comment 54

[Maire Reavy [:mreavy]]

Anthony, This is on the standalone -- so we don't need this verified for uplift to Aurora.

Comment 55

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

Comment on attachment 8495883 [details] [diff] [review]
Added a Call Failed view for Loop standalone.

Approval Request Comment
Part of the staged Loop aurora second uplift set

Comment 56

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/d6652a6ad661
https://hg.mozilla.org/releases/mozilla-aurora/rev/192205ae85e4
https://hg.mozilla.org/releases/mozilla-aurora/rev/719db0ec068f

Comment 57

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

Comment on attachment 8495883 [details] [diff] [review]
Added a Call Failed view for Loop standalone.

Approval previously granted to jesup to land this change as part of the second batch of Loop fixes to land on Aurora. Marking the approval after the fact.

Comment 58

[Gregory Szorc [:gps]]

https://hg.mozilla.org/projects/fig/rev/3e8c6fa72734

Comment 59

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.