Closed Bug 1000240 Opened 6 years ago Closed 5 years ago

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

Categories

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

defect

Tracking

(firefox34 fixed, firefox35 fixed)

RESOLVED FIXED
mozilla35
Iteration:
35.1
Tracking Status
firefox34 --- fixed
firefox35 --- fixed

People

(Reporter: RT, Unassigned)

References

Details

(Whiteboard: [mozilla33 carry over, p=1 spike][standalone-uplift][loop-inccall1])

User Story

As a WebRTC browser user calling a reachable user, I get notified that the other user could not be reached when a SDK error occurs so that I can try again.

Rough Impl:

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

* Create a new "Call Failed" view (or extend existing, if appropriate)
* For existing cases where the call fails due to an error, transition to this view rather than the "End Call" view.

Attachments

(2 files, 11 obsolete files)

288.05 KB, audio/mpeg
Details
74.37 KB, patch
standard8
: review+
Details | Diff | Splinter Review
No description provided.
User Story: (updated)
Blocks: 994274
No longer blocks: 1000127
Blocks: 1000778
No longer blocks: 994274
Priority: -- → P1
Whiteboard: [p=?, s=UI32]
Target Milestone: --- → mozilla32
Target Milestone: mozilla32 → mozilla33
Whiteboard: [p=?, s=UI32] → [p=?]
Blocks: 1015857
Depends on: 1023507
User Story: (updated)
User Story: (updated)
User Story: (updated)
User Story: (updated)
Depends on: 1030097
Assignee: nobody → dhenein
Status: NEW → ASSIGNED
Whiteboard: [p=?] → p=3 s=33.3 [qa-]
UI: https://people.mozilla.org/~dhenein/labs/loop-link-spec/#call-failed
Assignee: dhenein → nobody
Whiteboard: p=3 s=33.3 [qa-] → p=?
Whiteboard: p=? → p=4
Blocks: 1000186
Attached audio Call Failed.mp3
Initial sound file which may change later.
Whiteboard: p=4 → [p=4]
Whiteboard: [p=4] → --do_not_change-- [mozilla33 carry over]
Target Milestone: mozilla33 → mozilla34
Whiteboard: --do_not_change-- [mozilla33 carry over] → [mozilla33 carry over]
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.
Moving the GMP case to bug 1030097, that should be able to handle everything here.
User Story: (updated)
Blocks: 1030097
No longer depends on: 1030097
Status: ASSIGNED → NEW
User Story: (updated)
Blocks: 1034041
Blocks: 1046959
Depends on: 1000237
Moving most use cases to bug 1046959.
User Story: (updated)
User Story: (updated)
Blocks: 1047410
Whiteboard: [mozilla33 carry over] → [mozilla33 carry over, p=1 spike]
Assignee: nobody → aoprea
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?
Flags: needinfo?(nperriault)
(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".
Flags: needinfo?(nperriault)
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.
Attachment #8484723 - Flags: feedback?(nperriault) → feedback-
(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.
(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...
Flags: needinfo?(adam)
Hey Andrei, i remember we moved this to blocked - have the replies below unblocked it?
Flags: needinfo?(aoprea)
Visual uplift was blocking. I updated the trello board. Thanks.
Flags: needinfo?(aoprea)
(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?
Flags: needinfo?(adam) → needinfo?(andrei.br92)
This code is for standalone only. We already have audio notifications in the desktop client.
Flags: needinfo?(andrei.br92)
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>

:(
Attachment #8485303 - Flags: review?(nperriault) → review-
(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 ?
(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 :)
(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
Iteration: --- → 35.1
Whiteboard: [mozilla33 carry over, p=1 spike] → [mozilla33 carry over, p=1 spike][loop-uplift]
Target Milestone: mozilla34 → mozilla35
Attachment #8485303 - Attachment is obsolete: true
Attachment #8487498 - Flags: review?(nperriault)
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.
Attachment #8487498 - Flags: review?(nperriault) → review-
Whiteboard: [mozilla33 carry over, p=1 spike][loop-uplift] → [mozilla33 carry over, p=1 spike][loop-uplift][loop-inccall1]
(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.
Duplicate of this bug: 1067653
Attachment #8489093 - Attachment is obsolete: true
Attachment #8489776 - Flags: review?(nperriault)
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.
Attachment #8489776 - Flags: review?(nperriault) → feedback-
Whiteboard: [mozilla33 carry over, p=1 spike][loop-uplift][loop-inccall1] → [mozilla33 carry over, p=1 spike][standalone-uplift][loop-inccall1]
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?
Flags: needinfo?(standard8)
Flags: needinfo?(nperriault)
Assignee: aoprea → nperriault
Flags: needinfo?(nperriault)
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).
Attachment #8489776 - Attachment is obsolete: true
Attachment #8494684 - Flags: feedback?(standard8)
Factored call initiation button as a reusable component. Still needs test updates and style polishing.
Attachment #8494684 - Attachment is obsolete: true
Attachment #8494684 - Flags: feedback?(standard8)
Attachment #8494841 - Flags: feedback?(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.
Attachment #8494841 - Flags: feedback?(standard8) → feedback+
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.
Attachment #8494841 - Attachment is obsolete: true
Attachment #8495174 - Flags: review?(standard8)
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).
Attachment #8495174 - Attachment is obsolete: true
Attachment #8495174 - Flags: review?(standard8)
Attachment #8495543 - Flags: review?(standard8)
Updated patch with missing bits for the UI showcase.
Attachment #8495543 - Attachment is obsolete: true
Attachment #8495543 - Flags: review?(standard8)
Attachment #8495545 - Flags: review?(standard8)
Removed obsolete code, added missing tests for model changes.
Attachment #8495545 - Attachment is obsolete: true
Attachment #8495545 - Flags: review?(standard8)
Attachment #8495879 - Flags: review?(standard8)
Code removal again, fixed positionning of conversation footer. Sorry for bugnoise.
Attachment #8495879 - Attachment is obsolete: true
Attachment #8495879 - Flags: review?(standard8)
Attachment #8495883 - Flags: review?(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.
Flags: needinfo?(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.
Attachment #8495883 - Flags: review?(standard8) → review+
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
Keywords: leave-open
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.
Depends on: 1054793
Flags: needinfo?(nperriault)
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.
Flags: needinfo?(standard8)
Depends on: 1073991
Filed bug 1073991 as I can't see any reason for the assertion.
Filed bug 1075509 as a spin-off of this bug, for audio notification, so we can land the current patch.
Summary: Standalone UI for link clickers needs "call failed" visual and audio notification → Standalone UI for link clickers needs "call failed" visual notification
Relanded now the dependent bug is fixed (also with bitrot fixes).

https://hg.mozilla.org/integration/fx-team/rev/736437b0f970
Niko -- With bug 1075509 (for the sound notification) now filed, can we remove the "leave open" on this?
Flags: needinfo?(nperriault)
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.
Flags: needinfo?(nperriault)
(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).
Flags: qe-verify+
QA Contact: anthony.s.hughes
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
Filed bug 1080540 as a follow up for the missing wheel icon.
With bug 1075509 and bug 1080540 filed, we can close this bug.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Anthony, This is on the standalone -- so we don't need this verified for uplift to Aurora.
Flags: qe-verify+ → qe-verify-
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
Attachment #8495883 - Flags: approval-mozilla-aurora?
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.
Attachment #8495883 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: in-moztrap-
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.