Closed
Bug 1039263
Opened 7 years ago
Closed 7 years ago
Loop standalone client can't restart a call (regression)
Categories
(Hello (Loop) :: Client, defect)
Hello (Loop)
Client
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla33
People
(Reporter: NiKo, Unassigned)
Details
(Keywords: regression)
Attachments
(1 file, 1 obsolete file)
Steps to reproduce:
- Standalone webapp opens a loop call link
- Clicks "Start the call"
- Accepts media permissions
- Hang up
- Clicks again on "Start the call"
Results:
The new call won't start.
Expected:
The new call should start.
Technical details:
The console shows an error which is probably related:
> Error: Invariant Violation: getDOMNode(): A component must be mounted to have a DOM node. localhost:3000/content/shared/libs/react-0.10.0.js:17415
Assignee | ||
Comment 1•7 years ago
|
||
Extracted conversation from IRC: [14:20:25] Standard8: NiKo`: it'd be interesting to know what regressed it [14:20:38] NiKo`: Standard8: porting it to react [14:20:43] Standard8: ah [14:21:02] NiKo`: Standard8: we were starting subscribing to model event when the component was about to mount [14:21:14] Standard8: weird though, I thought I tested all that [14:21:16] NiKo`: but the callback assumed that it was mounted already [14:21:20] NiKo`: race conditions as usual [14:21:37] NiKo`: so the fix is to start listening when the component is mounted [14:21:45] NiKo`: so we're sure to have a DOM [14:21:54] NiKo`: available for the component [14:21:56] Standard8: ah ok [14:23:02] NiKo`: actually we were assuming that componentWillMount was the equivalent of Backbone.View's initialize() [14:23:08] NiKo`: but it's componentDidMount really [14:23:42] NiKo`: (actually these are two different beasts, but componentDidMount is the closest to what initialize is) Also, switched to using Backbone.Events#listenTo and Backbone.Events#stopListening for consistency and ease of unregistration.
Attachment #8456875 -
Flags: review?(dmose)
Comment 2•7 years ago
|
||
Comment on attachment 8456875 [details] [diff] [review] 0001-Bug-1039263-Fixed-standalone-Loop-UI-couldn-t-restar.patch Review of attachment 8456875 [details] [diff] [review]: ----------------------------------------------------------------- Looks good; thanks for the patch. r=dmose
Attachment #8456875 -
Flags: review?(dmose) → review+
Assignee | ||
Comment 3•7 years ago
|
||
Addressed comments from Mark: [09:46:06] Standard8: NiKo`: there's an issue [09:46:27] Standard8: this.TypeError: event is undefined views.js:289 [09:46:32] Standard8: s/this.// [09:46:54] Standard8: NiKo`: looks like the streamCreated listener Added missing tests for this to prevent further unnoticed regressions.
Attachment #8456875 -
Attachment is obsolete: true
Attachment #8457834 -
Flags: review?(standard8)
Comment 4•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/01f973e7aa61
Assignee: nobody → nperriault
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla33
Comment 5•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/01f973e7aa61
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Updated•7 years ago
|
Attachment #8457834 -
Flags: review?(standard8)
Given this was a regression, does this need a manual regression test or is the automation coverage sufficient?
Keywords: regression
Whiteboard: [qa?]
Comment 7•7 years ago
|
||
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #6) > Given this was a regression, does this need a manual regression test or is > the automation coverage sufficient? This would be covered by a manual smoketest by making a call, or by the functional tests once they are automated (we have some functional tests in the tree now, but we've still got to automate them).
I confirm that this is working as expected in Firefox 33.0a2 and 34.0a1. Can someone please review the following smoketest to confirm it's sufficient? https://moztrap.mozilla.org/manage/case/14532/
Status: RESOLVED → VERIFIED
Flags: qe-verify+
QA Contact: anthony.s.hughes
Whiteboard: [qa?]
You need to log in
before you can comment on or make changes to this bug.
Description
•