Closed
Bug 1087528
Opened 10 years ago
Closed 10 years ago
Show callID, sessionID and timestamp to the user for debugging purposes
Categories
(Hello (Loop) :: Client, defect, P1)
Hello (Loop)
Client
Tracking
(firefox35 fixed, firefox36 fixed, firefox37 fixed)
backlog | Fx35+ |
People
(Reporter: mreavy, Assigned: jib)
References
Details
Attachments
(7 files, 11 obsolete files)
(deleted),
patch
|
jib
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jib
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
jib
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jib
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jib
:
review+
|
Details | Diff | Splinter Review |
(deleted),
image/png
|
Details |
If there is a problem with a call, we want the user to be able to report the callID and sessionID (as well as the time) of the call to our "support" so we can get answers.
Initial thoughts are that this info could be on an "about:loopcalls" (or equivalent) page with 3 columns for time, callID, and session. Ideas welcome. The user would see this if he/she had trouble with a call and wanted to debug it; so we want to talk to UX before adding this into the product.
Comment 1•10 years ago
|
||
(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #0)
> If there is a problem with a call, we want the user to be able to report the
> callID and sessionID (as well as the time) of the call to our "support" so
> we can get answers.
Who in support is going to have access to the information behind these? This could have an impact on where we store it.
What are the situations in which these should be stored? (any call, accepted calls, just ones that successfully or partly connected...)
Comment 2•10 years ago
|
||
Oh, and I'd rather we get those background systems up and available before we add any UI for this. There's no point in having it presented to the user anywhere, if we haven't got the information available.
Reporter | ||
Updated•10 years ago
|
backlog: --- → Fx35?
Updated•10 years ago
|
backlog: Fx35? → Fx35+
Priority: -- → P2
Comment 4•10 years ago
|
||
Shell: I don't think we can do this for Fx35: Firstly the information to store & how/where it can be used isn't defined, secondly I'm struggling to think of a situation where we wouldn't need to add a string or two - but we also don't have any UX for this.
Severity: normal → enhancement
Flags: needinfo?(sescalante)
Whiteboard: [needs definition][needs UX]
Reporter | ||
Comment 5•10 years ago
|
||
Darrin -- We have a request from our partner to show to our user the simple mapping between callID and sessionID for debugging purposes. The primary use case/user story for this would be "as a user I want to be able to give my support people as much information as possible in order to solve a technical problem I may have."
Do you want us to put this on a page that the user would need to browse to (like "about:loopcalls", for example) -- which is officially outside the Hello UX (and therefore wouldn't involve strings)? Or do we want to add this to the UX of Hello?
Adam -- I believe the intention of this request is not to store this information, but merely display the information somewhere that the user could view (similar to about:webtrc in that regard). I'm looking at Mark's comments in comment 1, comment 2, and comment 4 -- and want to verify how much we need to do to meet the requirement of this bug (and satisfy our partner).
backlog: Fx35+ → Fx35?
Flags: needinfo?(dhenein)
Flags: needinfo?(adam)
Comment 6•10 years ago
|
||
(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #5)
> Do you want us to put this on a page that the user would need to browse to
> (like "about:loopcalls", for example) -- which is officially outside the
> Hello UX (and therefore wouldn't involve strings)?
Yes.
> Or do we want to add this to the UX of Hello?
No.
This is an advanced debugging process, not user-friendly error reporting. I think down the road we should integrate a "Send diagnostics" button or something to that effect to the call failed screen, but for now where users will need to be walked through the steps anyway, I'd rather not surface it in the UI. To most users, "the simple mapping between callID and sessionID" will be meaningless.
Flags: needinfo?(dhenein)
Comment 7•10 years ago
|
||
This came out of discussions with TokBox, and the idea was that it would allow some level of debugging based on server logs. This is a stopgap measure that is intended to very roughly fill a gap until Bug 1024568 can be implemented.
Based on the discussions we had on the topic, the idea was that we would have a new about screen (e.g., "about:loopcalls") that was simply a table with three columns: Date/Time, callId, sessionId. This data does not need to persist across sessions.
So the implementation of this could be extremely basic: any time we get a sessionID/callID from the Loop server, append it to an array. Then, implement an about:loopcalls page that iterates the array and builds an html table out of them. That's very quick to code, and satisfies the request from TokBox.
I agree with Maire that this isn't really part of the product UX, and so it doesn't need new strings before we can deploy.
Flags: needinfo?(adam)
Updated•10 years ago
|
backlog: Fx35? → Fx35+
Flags: needinfo?(sescalante)
Updated•10 years ago
|
Whiteboard: [needs definition][needs UX]
Reporter | ||
Comment 8•10 years ago
|
||
I'm not sure yet how often we'll need this info from users, and as Adam says above it's a temporary measure, but I think we need to get this into fx35 to cover our bases in case there are issues in Release that require this debugging info.
Priority: P2 → P1
Comment 9•10 years ago
|
||
After I had to debug an issue with the backend today I think we *really* needs this in the version where we officially release this to everyone.
Having the customers to open the browser console and pull the sessionId out of the HTTP response is going to drive users away once they experienced a call issue which we could not debug, because of the missing sessionId. And yes we don't know how many support request we will get where we needs this, but I would not want to take chance on this.
I'm not sure if it makes things easier or harder to implement, but my assumption is that for more most call problems we will ask the users to capture the full content of "about:webrtc". Having to ask them to capture the content of another about screen, which needs to correlates somehow with the information from about:webrtc sounds like a bad idea to me. We already have too many places where a user needs to capture logs to debug problems.
Therefore I would really like to have the sessionId's getting added to the about:webrtc screen rather then adding a new about screen.
Updated•10 years ago
|
Severity: enhancement → major
OS: Windows 7 → All
Hardware: x86_64 → All
Comment 10•10 years ago
|
||
Having these info into about:webrtc should be simple as long as we have them in WebrtcGlobalInformation attached to the corresponding reports.
Then we can add a "Firefox Hello" tab displaying these.
However, I don't know how we can add SessionId and CallId to WebrtcGlobalInformation as it seems deeply tied to C++ WebRTC internal code.
Nils, do you have an idea about how we can achieve that?
Flags: needinfo?(drno)
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → rgauthier
Comment 11•10 years ago
|
||
Do we really want to add Loop call information in the WebrtcGlobalInformation object? I would suggest that the about:webrtc code call something in MozLoopService to get the callID and sessionID and merge that into the about view when the call is driven from Loop. This would handle the case where Loop is not the user of the WebRTC API. For example, when the test page is used. This would remove the need to somehow push this information into WebrtcGlobalInformation.
Comment 12•10 years ago
|
||
(In reply to Paul Kerr [:pkerr] from comment #11)
> Do we really want to add Loop call information in the
> WebrtcGlobalInformation object? I would suggest that the about:webrtc code
> call something in MozLoopService to get the callID and sessionID and merge
> that into the about view when the call is driven from Loop. This would
> handle the case where Loop is not the user of the WebRTC API. For example,
> when the test page is used. This would remove the need to somehow push this
> information into WebrtcGlobalInformation.
Yes that sounds like a really good idea to me. Define an interface which can be queried by the about:webrtc on a per need base and delivers callID and sessionID from the HTTP calls. Romain what do you think?
Flags: needinfo?(drno)
Comment 13•10 years ago
|
||
Here is a WIP patch that retrieves data to be displayed in about:webrtc.
The hard part has been done, now, we need to store the transcient data and make it accessible from about:webrtc.
I have no clue how to achieve that. @Standard8, do you have any thoughts on that?
Flags: needinfo?(standard8)
Comment 14•10 years ago
|
||
Paul/Nils, can you advise, Romain on comment 13 please? I don't know that code at all.
Flags: needinfo?(standard8)
Flags: needinfo?(pkerr)
Flags: needinfo?(drno)
Comment 15•10 years ago
|
||
I actually kinda like the idea of adding a simple DOMString to PeerConnection that can be set from content, that will be placed in the PC's "name" that gets used in logging and on about:webrtc. Just a simple debug correlator that anyone can use to help add context to the logging/debug we have already.
Reporter | ||
Comment 16•10 years ago
|
||
I've asked Jib if he could finish off this patch based on Byron's suggestion (Comment 15) since we're running short on time and he's comfortable in this area.
tOkeshu -- jib may need to reach out to you for help finishing this off.
Assignee: rgauthier → jib
Flags: needinfo?(pkerr)
Flags: needinfo?(drno)
Assignee | ||
Comment 17•10 years ago
|
||
Lets chrome-code modify the unique pc.id attribute that's shown in about:webrtc.
Note: There's some preexisting ugliness internally here in that pc.id - the default one - is overloaded with data like the page's url and a simpler id number, which about:webrtc parses out (ick!) - The solution to this is probably to extricate this information into separate a pc.url and ... something (why do we have both a short and a long id number?)
For example: a default
pc.id = "1416468038884710 (id=28 url=file:///Users/Jan/pc_test.html)"
yields the following output in about:webrtc:
[28] file:///Users/Jan/pc_test.html 02:20:44 GMT-0500 (EST)
PeerConnection ID: 1416468038884710 (id=28 url=file:///Users/Jan/pc_test.html)
So one way to add info for now might be like this: pc1.id = "my-debug-info " + pc1.id;
[28] file:///Users/Jan/pc_test.html 02:20:44 GMT-0500 (EST)
PeerConnection ID: my-debug-info 1416468038884710 (id=28 url=file:///Users/Jan/pc_test.html)
Then we can test and iterate. Romain, feel free to hook this up as you probably know where the PCs get created.
Attachment #8525850 -
Flags: review?(docfaraday)
Assignee | ||
Comment 18•10 years ago
|
||
Comment on attachment 8523003 [details] [diff] [review]
WIP Call Ids in about:webrtc
Review of attachment 8523003 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/components/loop/MozLoopService.jsm
@@ +938,5 @@
> + getCallIds: function(winID) {
> + return new Promise((resolve, reject) => {
> + let callIds = this.callIds.get(winID);
> + if (callIds) {
> + resolve(callIds);
I'm curious, this looks synchronous so why use promises?
Assignee | ||
Comment 19•10 years ago
|
||
(In reply to Jan-Ivar Bruaroey [:jib] from comment #17)
> The solution to this is probably to extricate this information
> into separate a pc.url and ... something
pc.winid
Comment 20•10 years ago
|
||
(In reply to Jan-Ivar Bruaroey [:jib] from comment #17)
> Created attachment 8525850 [details] [diff] [review]
> make pc.id settable from chrome content
>
> Lets chrome-code modify the unique pc.id attribute that's shown in
> about:webrtc.
>
> Note: There's some preexisting ugliness internally here in that pc.id - the
> default one - is overloaded with data like the page's url and a simpler id
> number, which about:webrtc parses out (ick!) - The solution to this is
> probably to extricate this information into separate a pc.url and ...
> something (why do we have both a short and a long id number?)
>
So, the win id is to help disambiguate when we have multiple PCs on the same URL but different windows (eg; tab-to-tab testing), and the long id is a timestamp as a last resort in cases where we have multiple PCs on the same window.
I wonder if it would be safe to allow even content code to set the id, maybe with some sanity checking for length. Let me think about it.
Comment 21•10 years ago
|
||
Comment on attachment 8525850 [details] [diff] [review]
make pc.id settable from chrome content
Review of attachment 8525850 [details] [diff] [review]:
-----------------------------------------------------------------
This looks sensible to me. I suppose this will require a DOM peer, and maybe UUID updates?
::: dom/media/PeerConnection.js
@@ +983,5 @@
> },
>
> get peerIdentity() { return this._peerIdentity; },
> get id() { return this._impl.id; },
> + set id(n) { this._impl.id = n; },
'n' carries an implication that this thing is meant to be an integer.
Attachment #8525850 -
Flags: review?(docfaraday) → review+
Assignee | ||
Comment 22•10 years ago
|
||
(In reply to Byron Campen [:bwc] from comment #21)
> I suppose this will require a DOM peer, and maybe UUID updates?
Yes on DOM review, should be swift hopefully since this is a ChromeOnly change.
Don't need to worry about UUIDs anymore with webidl thankfully.
Attachment #8525850 -
Attachment is obsolete: true
Attachment #8526225 -
Flags: review?(bugs)
Updated•10 years ago
|
Attachment #8526225 -
Flags: review?(bugs) → review+
Assignee | ||
Updated•10 years ago
|
Attachment #8526225 -
Attachment description: make pc.id settable from chrome content (2) r=bwc → Part 1: make pc.id settable from chrome content (2) r=bwc
Assignee | ||
Comment 23•10 years ago
|
||
Update makes call- and session-ids visible in about:webrtc (not pretty).
I understand from Maire that we desire this info from *all* loop calls, not just when *receiving loop calls from contacts* the way this patch does. Romain or pkerr hopefully know better how to do that.
Attachment #8523003 -
Attachment is obsolete: true
Attachment #8527038 -
Flags: review?(pkerr)
Assignee | ||
Updated•10 years ago
|
Attachment #8527038 -
Attachment description: Call Ids in about:webrtc (2) → Part 2: Call Ids in about:webrtc (2)
Assignee | ||
Comment 24•10 years ago
|
||
Assignee | ||
Comment 25•10 years ago
|
||
Let me know if you wanted it to look differently.
Flags: needinfo?(mreavy)
Updated•10 years ago
|
Attachment #8527038 -
Flags: review?(pkerr) → review+
Comment 26•10 years ago
|
||
Comment on attachment 8527038 [details] [diff] [review]
Part 2: Call Ids in about:webrtc (2)
Review of attachment 8527038 [details] [diff] [review]:
-----------------------------------------------------------------
Maire asked me to comment about how to get this to cover all types of calls/conversations. I noticed a few other things that I think need addressing as well.
To cover outgoing calls, and rooms, you need to add a call to addCallContext into conversationStore#connectionProgress and activeRoomStore#joinedRoom. If we didn't have to log the callId, it'd be simpler just to add them to sdkDriver.js (but I don't want to pass the callId there just for it to be logged). Please also extend the existing unit tests to make sure they are called.
::: browser/components/loop/MozLoopAPI.jsm
@@ +701,5 @@
> request.send();
> }
> + },
> +
> + addCallContext: {
Please can you add a jsdoc comment to this.
I'm also wondering if it shouldn't be something like addConversationContext or addSdkContext.
::: browser/components/loop/MozLoopService.jsm
@@ +829,5 @@
> + // - 000 (id=00 url=http)
> + // + 000 (call=000 session=000 id=00 url=http)
> + var pair = pc.id.split("("); //)
> + if (pair.length == 2) {
> + pc.id = pair[0] + "(call=" + context.callId +
Note that we're not going to have callId all the time - rooms don't have callId.
I don't think that makes a different to this code, but we need to be careful.
::: browser/components/loop/content/shared/js/models.js
@@ +180,5 @@
> this.session.connect(this.get("apiKey"), this.get("sessionToken"),
> this._onConnectCompletion.bind(this));
> +
> + // We store the call credentials for debugging purposes.
> + navigator.mozLoop.addCallContext(this.get("windowId"),
Did you check adding this didn't break the unit tests? Especially the shared ones (hint: you can run the ones concerned with ./browser/components/loop/run-all-loop-tests.sh.
I suspect this could also break the standalone code, since that doesn't have mozLoop available. If this does then as we've now got the standaloneMozLoop available, this could be fixed by passing an instance of standaloneMozLoop into this model, and defining an empty dummy function for addCallContext.
In any case, this should have a test that it gets called, somewhere around here:
http://hg.mozilla.org/mozilla-central/annotate/8c02f3280d0c/browser/components/loop/test/shared/models_test.js#l147
Attachment #8527038 -
Flags: feedback-
Assignee | ||
Comment 27•10 years ago
|
||
Incorporated feedback. Passes unit-tests. Thanks! Carrying forward r=pkerr.
- Works for incoming and outgoing contact calls. Still doesn't work for rooms. Unsure why.
Will investigate more tomorrow.
- "session=" is now listed before "call=" in about:webrtc since latter is optional.
One thing I was unsure about from your feedback was if you wanted the mozLoop argument to be required or not, as there were 17 call-sites, which seemed a lot for this marginal narrow-use debug feature. Let me know if you still prefer that. I made it optional for now.
Attachment #8527038 -
Attachment is obsolete: true
Attachment #8528007 -
Flags: review?(standard8)
Reporter | ||
Comment 28•10 years ago
|
||
I'm ok with how it's displayed -- except it looks like we display the call ID 3 times (which makes it look a lot busier and harder to read than it needs to). We're looking to timestamp (which we have) and then callID, sessionID right next to it or immediately below. I'm fine with leaving PeerConnectionID there. Maybe we just want to get rid of "url=" and everything after it?
Flags: needinfo?(mreavy)
Assignee | ||
Comment 29•10 years ago
|
||
Thanks, I actually didn't notice that the number in the URL was the call id until you pointed it out.
Unfortunately, it's not there on outgoing calls. Here's two (collapsed) calls, outbound then inbound:
[46] about:loopconversation#1 (closed) 18:43:03 GMT-0500 (EST)
[34] about:loopconversation#cbac5932d09a337bcba8f77046e7692a (closed) 18:42:14 GMT-0500 (EST)
I'll modify the about:webrtc parsing code to erase info it elevates to the header rather than duplicating it (winId and url):
[46] about:loopconversation#1 (closed)18:43:03 GMT-0500 (EST)
PeerConnection ID: 1416872553120174 (session=1_MX40NDgzNTg5Mn5-
MTQxNjg3MjU0OTE3OH43YUg2QkhsNmJ5UjJFMnpuZzQ5L2cvZm9-UH4 call=9554c43dc5b9a990ce8996bd03669b69)
[34] about:loopconversation#cbac5932d09a337bcba8f77046e7692a (closed)18:42:14 GMT-0500 (EST)
PeerConnection ID: 1416872480576192 (session=2_MX40NDY2NzU2Mn5-
MTQxNjg3MjQ3MTU4Nn5hcDQvM05PWFpldnNZYXk0MWRLSjVLTE5-UH4 call=cbac5932d09a337bcba8f77046e7692a)
But if we erase "call=" on the latter, will people get that the number in the url is the call-id on the second one, or does it not matter? Add to this that rooms wont have call-ids.
Flags: needinfo?(mreavy)
Reporter | ||
Comment 30•10 years ago
|
||
(In reply to Jan-Ivar Bruaroey [:jib] from comment #29)
> But if we erase "call=" on the latter, will people get that the number in
> the url is the call-id on the second one, or does it not matter? Add to this
> that rooms wont have call-ids.
Ah, can we choose not to show callID next to the "about:loopcoversation"? That would eliminate the redundancy and keep the two (outbound and inbound) very similar in format.
I would not elminate "call=". I would keep that in both cases. (I'd rather keep redundant info than have info be tough to find.)
Also, we need the info for Rooms as well here.
Flags: needinfo?(mreavy)
Assignee | ||
Comment 31•10 years ago
|
||
(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #30)
> Ah, can we choose not to show callID next to the "about:loopcoversation"?
It's literally part of the URL for the relevant page.
> I would not elminate "call=". I would keep that in both cases. (I'd rather
> keep redundant info than have info be tough to find.)
Will do.
Assignee | ||
Comment 32•10 years ago
|
||
Attachment #8528458 -
Flags: review?(docfaraday)
Comment 33•10 years ago
|
||
Comment on attachment 8528458 [details] [diff] [review]
Part 3: avoid showing (win)id and url twice for peerConnections in about:webrtc
Review of attachment 8528458 [details] [diff] [review]:
-----------------------------------------------------------------
::: toolkit/content/aboutwebrtc/aboutWebrtc.js
@@ +121,5 @@
> },
> + getPCId: function(report) {
> + // Erase extracted info to avoid showing duplicate information
> + return report.pcid.replace(/id=\S+/, "").replace(/url=[^)]+/, "")
> + .replace(/\s+[)]/, ")").replace(" ()", "");
Can't we just trim off everything past the first ' ' with one regex? Or everything after and including the '('?
Assignee | ||
Comment 34•10 years ago
|
||
(In reply to Byron Campen [:bwc] from comment #33)
> > + getPCId: function(report) {
> > + // Erase extracted info to avoid showing duplicate information
> > + return report.pcid.replace(/id=\S+/, "").replace(/url=[^)]+/, "")
> > + .replace(/\s+[)]/, ")").replace(" ()", "");
>
> Can't we just trim off everything past the first ' ' with one regex? Or
> everything after and including the '('?
I didn't want to rule out future chrome content adding more info to pcid. This conservatively removes only what getPCInfo extracts with minimal order dependency. I also hoped the matching regular expressions would make it easier to maintain.
Assignee | ||
Comment 35•10 years ago
|
||
Fixes rooms. Carrying forward r=pkerr.
Attachment #8528733 -
Flags: review?(standard8)
Assignee | ||
Updated•10 years ago
|
Attachment #8528007 -
Attachment is obsolete: true
Attachment #8528007 -
Flags: review?(standard8)
Assignee | ||
Comment 36•10 years ago
|
||
Unbitrot.
Attachment #8528733 -
Attachment is obsolete: true
Attachment #8528733 -
Flags: review?(standard8)
Attachment #8528739 -
Flags: review?(standard8)
Comment 37•10 years ago
|
||
Comment on attachment 8528739 [details] [diff] [review]
Part 2: Call Ids in about:webrtc (5) r=pkerr
Review of attachment 8528739 [details] [diff] [review]:
-----------------------------------------------------------------
The general direction is looking good, there's some changes we can make to re-use existing data and improve the flow a bit.
::: browser/components/loop/MozLoopAPI.jsm
@@ +711,5 @@
> + */
> + addConversationContext: {
> + enumerable: true,
> + writable: true,
> + value: function(sWindowId, sSessionId, sCallid) {
Please drop the s prefix, as per the style in the rest of the file.
::: browser/components/loop/content/js/conversation.js
@@ +661,5 @@
> dispatcher: dispatcher,
> sdkDriver: sdkDriver
> });
> +
> + // Obtain the windowId and pass it through
These changes need to be done to the jsx version of this file, and then this one re-generated with jsx (see the end of browser/components/loop/README.txt)
@@ +675,4 @@
> var activeRoomStore = new loop.store.ActiveRoomStore(dispatcher, {
> mozLoop: navigator.mozLoop,
> + sdkDriver: sdkDriver,
> + windowId: windowId
activeRoomStore already gets the windowId from the SetupWindowData action - see conversationAppStore - though it isn't explicitly saved there. It would be better just to update activeRoomStore's state from setupWindowData with the windowId (and avoids duplicate code, as this doesn't handle every state).
@@ +689,5 @@
> // we transition across (bug 1072323).
> var conversation = new sharedModels.ConversationModel(
> {}, // Model attributes
> + {sdk: window.OT,
> + mozLoop: navigator.mozLoop} // Model dependencies
Nit: Please use this format:
var conversation = new sharedModels.ConversationModel({}, {
sdk: windowOT,
mozLoop: navigator.mozLoop
});
I think we can drop the comments there.
::: browser/components/loop/content/shared/js/activeRoomStore.js
@@ +294,5 @@
>
> this._setRefreshTimeout(actionData.expires);
> this._sdkDriver.connectSession(actionData);
> +
> + this._mozLoop.addConversationContext(this._windowId, actionData.sessionId, "");
You need to add a dummy function in standaloneMozLoop.js to avoid the standalone implmentation throwing an exception.
::: browser/components/loop/content/shared/js/models.js
@@ +66,5 @@
> * @param {Object} options Options object.
> */
> initialize: function(attributes, options) {
> options = options || {};
> + this.mozLoop = options.mozLoop;
Just ftr, I'm ok with this being optional - as we need it only for desktop for now. If that changes later, we might want to revisit, but hopefully we'll rewrite this code before then.
::: browser/components/loop/test/shared/activeRoomStore_test.js
@@ +374,5 @@
> sinon.assert.calledOnce(fakeSdkDriver.connectSession);
> sinon.assert.calledWithExactly(fakeSdkDriver.connectSession,
> actionData);
> + sinon.assert.calledOnce(fakeMozLoop.addConversationContext);
> + sinon.assert.calledWithExactly(fakeMozLoop.addConversationContext,
Please make these new checks a separate it() test. We generally keep one check per test (a check being a function call with params, or some other event).
::: browser/components/loop/test/shared/conversationStore_test.js
@@ +223,5 @@
> sessionId: "321456",
> sessionToken: "341256"
> });
> + sinon.assert.calledOnce(navigator.mozLoop.addConversationContext);
> + sinon.assert.calledWithExactly(navigator.mozLoop.addConversationContext,
Again, please separate this out.
::: browser/components/loop/test/shared/models_test.js
@@ +187,5 @@
> +
> + sinon.assert.calledOnce(fakeMozLoop.addConversationContext);
> + sinon.assert.calledWithExactly(fakeMozLoop.addConversationContext,
> + sinon.match.string, sinon.match.string,
> + sinon.match.string);
Really, these should be checking for exact parameters - you can set them directly on the model if you need to:
model.set({
windowId: "fakeWinId",
...
});
model.startSession();
etc
Attachment #8528739 -
Flags: review?(standard8) → review-
Assignee | ||
Comment 38•10 years ago
|
||
Incorporate feedback. Carrying forward r=pkerr.
- Should take care of it. Will test marionette as soon as try opens.
Attachment #8528739 -
Attachment is obsolete: true
Attachment #8529232 -
Flags: review?(standard8)
Assignee | ||
Comment 39•10 years ago
|
||
Comment 40•10 years ago
|
||
Comment on attachment 8529232 [details] [diff] [review]
Part 2: Call Ids in about:webrtc (6) r=pkerr
Review of attachment 8529232 [details] [diff] [review]:
-----------------------------------------------------------------
Looking better, a few small things to be addressed still.
The conversation.jsx/conversation.js parts of this are missing - so logging the ids for incoming calls is not working at the moment.
::: browser/components/loop/content/shared/js/activeRoomStore.js
@@ +76,5 @@
> if (!options.sdkDriver) {
> throw new Error("Missing option sdkDriver");
> }
> this._sdkDriver = options.sdkDriver;
> + this._windowId = options.windowId;
This line should be dropped.
@@ +154,5 @@
> return;
> }
>
> this._registerPostSetupActions();
> + this._windowId = actionData.windowId;
What I meant here was to incorporate this in the setStoreState just below, i.e.
this.setStoreState({
roomState: ROOM_STATES.GATHER,
windowId: actionData.windowId
});
@@ +295,5 @@
>
> this._setRefreshTimeout(actionData.expires);
> this._sdkDriver.connectSession(actionData);
> +
> + this._mozLoop.addConversationContext(this._windowId, actionData.sessionId, "");
Here, it then becomes this._storeState.windowId
Attachment #8529232 -
Flags: review?(standard8) → review-
Assignee | ||
Comment 41•10 years ago
|
||
(In reply to Mark Banner (:standard8) from comment #40)
> The conversation.jsx/conversation.js parts of this are missing - so logging
> the ids for incoming calls is not working at the moment.
Whoops, dropped those files by accident. Sorry.
> What I meant here was to incorporate this in the setStoreState just below,
> i.e.
>
> this.setStoreState({
> roomState: ROOM_STATES.GATHER,
> windowId: actionData.windowId
> });
>
> @@ +295,5 @@
> >
> > this._setRefreshTimeout(actionData.expires);
> > this._sdkDriver.connectSession(actionData);
> > +
> > + this._mozLoop.addConversationContext(this._windowId, actionData.sessionId, "");
>
> Here, it then becomes this._storeState.windowId
Thanks, I'm still feeling my way around here.
Carrying forward r=pkerr.
Attachment #8529232 -
Attachment is obsolete: true
Attachment #8529942 -
Flags: review?(standard8)
Assignee | ||
Updated•10 years ago
|
Attachment #8527042 -
Attachment is obsolete: true
Comment 42•10 years ago
|
||
Comment on attachment 8529942 [details] [diff] [review]
Part 2: Call Ids in about:webrtc (7) r=pkerr
Review of attachment 8529942 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good, just one minor nit to address. There's a little bit of bitrot against latest head, but should be easy to fix.
::: browser/components/loop/content/shared/js/activeRoomStore.js
@@ +153,5 @@
> return;
> }
>
> this._registerPostSetupActions();
> + this._windowId = actionData.windowId;
nit: you forgot to remove this line.
Attachment #8529942 -
Flags: review?(standard8) → review+
Assignee | ||
Comment 43•10 years ago
|
||
Addressed nit. Carrying forward r=standard8, pkerr
Try - https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=17f6c66baec9
Attachment #8529942 -
Attachment is obsolete: true
Attachment #8529991 -
Flags: review+
Assignee | ||
Comment 44•10 years ago
|
||
Updated commit msg. Carrying forward r=smaug, bwc
Attachment #8526225 -
Attachment is obsolete: true
Attachment #8529992 -
Flags: review+
Assignee | ||
Comment 45•10 years ago
|
||
Comment on attachment 8528458 [details] [diff] [review]
Part 3: avoid showing (win)id and url twice for peerConnections in about:webrtc
Taking first available.
Attachment #8528458 -
Flags: review?(rjesup)
Attachment #8528458 -
Flags: review?(pkerr)
Assignee | ||
Comment 46•10 years ago
|
||
Updated screenshot. Btw, why do I get two peerConnections per loop call?
Comment 47•10 years ago
|
||
Comment on attachment 8528458 [details] [diff] [review]
Part 3: avoid showing (win)id and url twice for peerConnections in about:webrtc
Review of attachment 8528458 [details] [diff] [review]:
-----------------------------------------------------------------
r+, though I find regexps dangerous to parse in one's head - at least describe what you're removing syntax-wise in a comment. And if there's a simpler regexp, r+ for that. :-)
Attachment #8528458 -
Flags: review?(rjesup) → review+
Assignee | ||
Comment 48•10 years ago
|
||
Added comments on regexp's. Carrying forward r=jesup.
Attachment #8528458 -
Attachment is obsolete: true
Attachment #8528458 -
Flags: review?(pkerr)
Attachment #8528458 -
Flags: review?(docfaraday)
Attachment #8530079 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 49•10 years ago
|
||
(In reply to Jan-Ivar Bruaroey [:jib] from comment #46)
> Created attachment 8530005 [details]
> Screenshot of raw about:webrtc info (2)
>
> Updated screenshot. Btw, why do I get two peerConnections per loop call?
tokbox uses two one-way connections per call.
Comment 50•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f53d58c49f6a
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebca7b404b37
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3a493731980
Keywords: checkin-needed
Comment 51•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f53d58c49f6a
https://hg.mozilla.org/mozilla-central/rev/ebca7b404b37
https://hg.mozilla.org/mozilla-central/rev/d3a493731980
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment 52•10 years ago
|
||
Comment on attachment 8529992 [details] [diff] [review]
Part 1: make pc.id settable from chrome content (3) r=smaug, bwc
Approval Request Comment
[Feature/regressing bug #]: Update of about:webrtc to be interactive (in 35).
[User impact if declined]: hard to use info from the about:webrtc info to correlate to server info if a problem is reported with Loop/Hello
[Describe test coverage new/current, TBPL]: Manual verification.
[Risks and why]: Low risk; just adds some labeling information to peerconnections and shows it on about:webrtc. Missed the merges for 36 uplift to Aurora (Target is mis-marked since it landed on central before the uplift, but was not included in the uplift); since the visual update landed in 35 we'd like to see it there as well, since 35 is when we expect to really start promoting Hello.
[String/UUID change made/needed]: none
(Request is for all 3 patches)
Attachment #8529992 -
Flags: approval-mozilla-beta?
Attachment #8529992 -
Flags: approval-mozilla-aurora?
Updated•10 years ago
|
Attachment #8529991 -
Flags: approval-mozilla-beta?
Attachment #8529991 -
Flags: approval-mozilla-aurora?
Updated•10 years ago
|
Attachment #8530079 -
Flags: approval-mozilla-beta?
Attachment #8530079 -
Flags: approval-mozilla-aurora?
Comment 53•10 years ago
|
||
Looks like it landed after the merge
Target Milestone: mozilla36 → 34 Sprint 3- 9/1
Updated•10 years ago
|
Updated•10 years ago
|
Target Milestone: 34 Sprint 3- 9/1 → mozilla37
Updated•10 years ago
|
Attachment #8529991 -
Flags: approval-mozilla-beta?
Attachment #8529991 -
Flags: approval-mozilla-beta+
Attachment #8529991 -
Flags: approval-mozilla-aurora?
Attachment #8529991 -
Flags: approval-mozilla-aurora+
Updated•10 years ago
|
Attachment #8529992 -
Flags: approval-mozilla-beta?
Attachment #8529992 -
Flags: approval-mozilla-beta+
Attachment #8529992 -
Flags: approval-mozilla-aurora?
Attachment #8529992 -
Flags: approval-mozilla-aurora+
Updated•10 years ago
|
Attachment #8530079 -
Flags: approval-mozilla-beta?
Attachment #8530079 -
Flags: approval-mozilla-beta+
Attachment #8530079 -
Flags: approval-mozilla-aurora?
Attachment #8530079 -
Flags: approval-mozilla-aurora+
Comment 54•10 years ago
|
||
Updated•10 years ago
|
Comment 55•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/b384f0bb30bf
https://hg.mozilla.org/releases/mozilla-aurora/rev/005e8f993800
Whiteboard: Previous aurora uplift missed parts 1 & 3 which are not in browser/components/loop due to confusion over what made the merge.
Comment 56•10 years ago
|
||
Previous aurora uplift missed parts 1 & 3 which are not in browser/components/loop due to confusion over what made the merge.
Whiteboard: Previous aurora uplift missed parts 1 & 3 which are not in browser/components/loop due to confusion over what made the merge.
Assignee | ||
Comment 57•10 years ago
|
||
Subset patch for mozilla-beta's ugly old about:webrtc page.
- Basically omits aboutWebrtc.js changes, since the old page just
shows the raw id.
Carrying forward r=smaug, bwc.
Attachment #8534026 -
Flags: review+
Assignee | ||
Comment 58•10 years ago
|
||
No change, just offset. Carrying forward r=standard8, pkerr.
Part 3 is not needed for mozilla-beta, since old page just shows raw id.
Attachment #8534028 -
Flags: review+
Assignee | ||
Comment 59•10 years ago
|
||
I've verified locally on (OSX) Beta that sessionId and sometimes callId show up in about:webrtc in the following cases:
1. Starting a conversation.
2. Calling a contact.
3. Receiving call from a contact.
The screenshot shows the busiest scenario where both sessionId and callId are present and the URL itself contains the callid to boot. The other two cases are less busy.
Assignee | ||
Updated•10 years ago
|
Attachment #8534034 -
Attachment description: Screenshot of info in about:webrtc on Beta → Screenshot of raw about:webrtc info on Beta
Comment 60•10 years ago
|
||
Comment 61•10 years ago
|
||
I verified on beta locally too that the beta merge of the patches was correct.
Updated•10 years ago
|
Iteration: --- → 37.1
You need to log in
before you can comment on or make changes to this bug.
Description
•