Closed Bug 1079811 Opened 5 years ago Closed 5 years ago

A new call won't start if the outgoing call window is opened (showing feedback or retry/cancel)

Categories

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

x86_64
Windows 7
defect

Tracking

(firefox33 unaffected, firefox34+ verified, firefox35+ verified, firefox36+ verified)

VERIFIED FIXED
mozilla36
Iteration:
36.1
Tracking Status
firefox33 --- unaffected
firefox34 + verified
firefox35 + verified
firefox36 + verified
Blocking Flags:
backlog Fx34+

People

(Reporter: pauly, Assigned: rgauthier)

References

Details

Attachments

(1 file, 2 obsolete files)

A new call won't start if the outgoing call window is opened (showing feedback or retry/cancel)
Shell -- can we add this to our trello?
Flags: needinfo?(sescalante)
Priority: -- → P2
added to trello and xls bug list
Flags: needinfo?(sescalante)
Flags: qe-verify?
Flags: firefox-backlog+
The work here is to port the existing calls/lines of code:

 navigator.mozLoop.releaseCallData(this.props.conversation.get("callId"));

to the conversationStore in equivalent places.
backlog: --- → Fx36+
backlog: Fx36+ → Fx35+
Flags: qe-verify? → qe-verify+
QA Contact: anthony.s.hughes → paul.silaghi
Thanks for taking this, Romain
Assignee: nobody → rgauthier
backlog: Fx35+ → Fx34+
Attachment #8506153 - Flags: feedback?(standard8)
Comment on attachment 8506153 [details] [diff] [review]
WIP: Bug 1079811 fix outgoing calls overlaping

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

This good, I've just tested this and it seems to do the right thing.

All you need now is to extend the existing tests for it. You should be able to base the tests on the ones where we check for closing the websocket already.

::: browser/components/loop/content/shared/js/conversationStore.js
@@ +374,5 @@
>          this._websocket.close();
>          delete this._websocket;
>        }
> +
> +      // The internal callId is different from this.get("callId");

Please make this an XXX and reference bug 1084228.
Attachment #8506153 - Flags: feedback?(standard8) → feedback+
[Tracking Requested - why for this release]:
Tracking fix for uplift.  We should have a patch for review today.
Attachment #8506153 - Attachment is obsolete: true
Attachment #8506901 - Flags: review?(standard8)
Comment on attachment 8506901 [details] [diff] [review]
Bug 1079811 fix outgoing calls overlaping

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

::: browser/components/loop/content/shared/js/conversationStore.js
@@ +408,5 @@
>        }
> +
> +      // XXX: The internal callId is different from
> +      // this.get("callId"), see bug 1084228 for more info.
> +      var callId = window.location.href.match(/\#outgoing\/(.*)/)[1];

Please use something like:

var locationHash = new loop.shared.utils.Helper().locationHash();
var callId = locationHash.match( ... )[1];

We've created that in the helper to make it easier to stub for tests...

::: browser/components/loop/test/shared/conversationStore_test.js
@@ +124,5 @@
>  
>    describe("#connectionFailure", function() {
>      beforeEach(function() {
>        store._websocket = fakeWebsocket;
> +      window.location.hash = "outgoing/42";

Please stub the helper#locationHash function as we do elsewhere. This saves changing the side effects of changing the hash of the test page.
Attachment #8506901 - Flags: review?(standard8) → review-
Attachment #8506901 - Attachment is obsolete: true
Attachment #8506936 - Flags: review?(standard8)
Comment on attachment 8506936 [details] [diff] [review]
Bug 1079811 fix outgoing calls overlaping

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

Looks good. r=Standard8
Attachment #8506936 - Flags: review?(standard8) → review+
https://hg.mozilla.org/integration/fx-team/rev/808971467f9f
Iteration: --- → 36.1
Target Milestone: --- → mozilla36
https://hg.mozilla.org/mozilla-central/rev/808971467f9f
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Tested on Windows and Win32 Nightly 10/18; works!
Comment on attachment 8506936 [details] [diff] [review]
Bug 1079811 fix outgoing calls overlaping

Approval Request Comment
[Feature/regressing bug #]: N/A

[User impact if declined]: Inability to make/receive new calls while old call window is still open

[Describe test coverage new/current, TBPL]: Tested manually on multiple OS's in Nightly.

[Risks and why]: Needed for feature release

[String/UUID change made/needed]: none
Attachment #8506936 - Flags: approval-mozilla-beta?
Attachment #8506936 - Flags: approval-mozilla-aurora?
Comment on attachment 8506936 [details] [diff] [review]
Bug 1079811 fix outgoing calls overlaping

Thanks for testing jesup. Approved for Aurora. If everything goes well with your testing on Aurora on Sunday, we'll get this uplifted for beta2.
Attachment #8506936 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Tested and good on Aurora nightly build; windows and linux.
Comment on attachment 8506936 [details] [diff] [review]
Bug 1079811 fix outgoing calls overlaping

Previously approved offline. Adding approval to the bug.
Attachment #8506936 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Marking verified based on Randell's testing, though this will still need verification in Beta once it uplifts.
Status: RESOLVED → VERIFIED
I'm not sure if this is expected, but I can't make/receive another call if I have 4 outgoing call windows open in the retry/cancel state.
Flags: needinfo?(rgauthier)
(In reply to Paul Silaghi, QA [:pauly] from comment #22)
> I'm not sure if this is expected, but I can't make/receive another call if I
> have 4 outgoing call windows open in the retry/cancel state.

Pauly -- can you open a new bug about this?  Having 4 open outgoing call windows in the retry/cancel state is an edge case no one had tried.  Thanks.
Flags: needinfo?(paul.silaghi)
Blocks: 1086434
Flags: needinfo?(paul.silaghi)
Flags: needinfo?(rgauthier)
Verified fixed FF 34b2 Win 7
You need to log in before you can comment on or make changes to this bug.