1087528 - Show callID, sessionID and timestamp to the user for debugging purposes

Status: RESOLVED FIXED

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

Description

[Maire Reavy [:mreavy]]

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

[Mark Banner (:standard8)]

(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

[Mark Banner (:standard8)]

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.

Comment 4

[Mark Banner (:standard8)]

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.

Comment 5

[Maire Reavy [:mreavy]]

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).

Comment 6

[Darrin Henein [:darrin]]

(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.

Comment 7

[Adam Roach [:abr]]

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.

Comment 8

[Maire Reavy [:mreavy]]

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.

Comment 9

[Nils Ohlmeier [:drno]]

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.

Comment 10

[Romain Gauthier [:tOkeshu]]

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?

Comment 11

[Paul Kerr [:pkerr]]

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

[Nils Ohlmeier [:drno]]

(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?

Comment 13

[Romain Gauthier [:tOkeshu]]

Attachment: WIP Call Ids in about:webrtc

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?

Comment 14

[Mark Banner (:standard8)]

Paul/Nils, can you advise, Romain on comment 13 please? I don't know that code at all.

Comment 15

[Byron Campen [:bwc]]

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.

Comment 16

[Maire Reavy [:mreavy]]

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.

Comment 17

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: 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?)

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.

Comment 18

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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?

Comment 19

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(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

[Byron Campen [:bwc]]

(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

[Byron Campen [:bwc]]

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.

Comment 22

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 1: make pc.id settable from chrome content (2) r=bwc

(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.

Comment 23

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 2: Call Ids in about:webrtc (2)

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.

Comment 24

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Screenshot of raw about:webrtc info

Comment 25

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Let me know if you wanted it to look differently.

Comment 26

[Mark Banner (:standard8)]

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

Comment 27

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 2: Call Ids in about:webrtc (3) r=pkerr

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.

Comment 28

[Maire Reavy [:mreavy]]

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?

Comment 29

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 30

[Maire Reavy [:mreavy]]

(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.

Comment 31

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(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.

Comment 32

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 3: avoid showing (win)id and url twice for peerConnections in about:webrtc

Comment 33

[Byron Campen [:bwc]]

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 '('?

Comment 34

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(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.

Comment 35

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 2: Call Ids in about:webrtc (4) r=pkerr

Fixes rooms. Carrying forward r=pkerr.

Comment 36

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 2: Call Ids in about:webrtc (5) r=pkerr

Unbitrot.

Comment 37

[Mark Banner (:standard8)]

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

Comment 38

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 2: Call Ids in about:webrtc (6) r=pkerr

Incorporate feedback. Carrying forward r=pkerr.
- Should take care of it. Will test marionette as soon as try opens.

Comment 39

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Green try - https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d598a9c8514d

Comment 40

[Mark Banner (:standard8)]

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

Comment 41

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 2: Call Ids in about:webrtc (7) r=pkerr

(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.

Comment 42

[Mark Banner (:standard8)]

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.

Comment 43

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 2: Call Ids in about:webrtc (8) r=standard8, pkerr

Addressed nit. Carrying forward r=standard8, pkerr

Try - https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=17f6c66baec9

Comment 44

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 1: make pc.id settable from chrome content (3) r=smaug, bwc

Updated commit msg. Carrying forward r=smaug, bwc

Comment 45

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8528458 [details] [diff] [review]
Part 3: avoid showing (win)id and url twice for peerConnections in about:webrtc

Taking first available.

Comment 46

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Screenshot of raw about:webrtc info (2)

Updated screenshot. Btw, why do I get two peerConnections per loop call?

Comment 47

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

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. :-)

Comment 48

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 3: avoid showing (win)id and url twice for peerConnections in about:webrtc (2) r=jesup

Added comments on regexp's. Carrying forward r=jesup.

Comment 49

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

(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

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

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

Comment 51

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f53d58c49f6a
https://hg.mozilla.org/mozilla-central/rev/ebca7b404b37
https://hg.mozilla.org/mozilla-central/rev/d3a493731980

Comment 52

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

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)

Comment 53

[Sylvestre Ledru [:Sylvestre]]

Looks like it landed after the merge

Comment 54

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/e043a95d16fd

Comment 55

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/b384f0bb30bf
https://hg.mozilla.org/releases/mozilla-aurora/rev/005e8f993800

Comment 56

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

Previous aurora uplift missed parts 1 & 3 which are not in browser/components/loop due to confusion over what made the merge.

Comment 57

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Beta part 1: make pc.id settable from chrome content (3) r=smaug, bwc

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.

Comment 58

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Beta part 2: Call Ids in about:webrtc (8) r=standard8, pkerr

No change, just offset. Carrying forward r=standard8, pkerr.

Part 3 is not needed for mozilla-beta, since old page just shows raw id.

Comment 59

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Screenshot of raw about:webrtc info on Beta

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.

Comment 60

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

https://hg.mozilla.org/releases/mozilla-beta/rev/4c99b92af5cc
https://hg.mozilla.org/releases/mozilla-beta/rev/7c0b02faa71c

Comment 61

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

I verified on beta locally too that the beta merge of the patches was correct.