With the work we did for bug 917227, this should be straightforward.

This patch gets the network monitor working with e10s (remote tabs) in firefox desktop. Also tested with a b2g device: things continue to work with no changes.

Comment on attachment 8398077 [details] [diff] [review] bug989043-1.diff Review of attachment 8398077 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but I would like to clarify the RemoteBrowserTabActor thing. Can you also update this file: http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/devtools.js#125 It is used to implement the Developer HUD on FxOS. ::: toolkit/devtools/server/actors/webbrowser.js @@ +1046,5 @@ > + > + get chromeEventHandler() { > + return this._browser; > + }, > + Is it actually used? RemoveBrowserTabActor name is misleading as it is a weird beast, that fakes an actor. `form` attribute is actually the ContentActor grip instanciated by connectToChild. And RemoteBrowserTabActor doesn't inherit from any anything (TabActor). ::: toolkit/devtools/server/actors/webconsole.js @@ +145,5 @@ > get _parentIsContentActor() { > + return ("ContentActor" in DebuggerServer && > + this.parentActor instanceof DebuggerServer.ContentActor) || > + ("RemoteBrowserTabActor" in DebuggerServer && > + this.parentActor instanceof DebuggerServer.RemoteBrowserTabActor); Does it really happen `this.parentActor instanceof DebuggerServer.RemoteBrowserTabActor`? I'm totally lost if it does. For me RemoteBrowserTabActor is kind of hack that shouldn't be parent of anything. It should just be a fake class containing ContentActor grip... @@ +513,2 @@ > appId = this.parentActor.docShell.appId; > + messageManager = this.parentActor.chromeEventHandler; We should expose an explicit `messageManager` attribute on TabActor. Today, chromeEventHandler appears to be a message manager, but it may not be always true. ::: toolkit/devtools/webconsole/network-monitor.js @@ +1084,5 @@ > this._messageManager.removeMessageListener("debug:netmonitor:updateEvent", this._onUpdateEvent); > + this._messageManager.sendAsyncMessage("debug:netmonitor", { > + managerID: this.connID, > + action: "disconnect", > + }); Instead of filtering in each message listener by managerID, you may use one message by network monitor. sendAsyncMessage("debug:netmonitor:" + this.connID, ...); Same thing for newEvent/updateEvent. It's not a big deal, so it is up to you.

(In reply to Alexandre Poirot (:ochameau) from comment #2) > Comment on attachment 8398077 [details] [diff] [review] > bug989043-1.diff > > Review of attachment 8398077 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good, but I would like to clarify the RemoteBrowserTabActor thing. > > Can you also update this file: > > http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/devtools. > js#125 > It is used to implement the Developer HUD on FxOS. Will do. > ::: toolkit/devtools/server/actors/webbrowser.js > @@ +1046,5 @@ > > + > > + get chromeEventHandler() { > > + return this._browser; > > + }, > > + > > Is it actually used? Yes. For e10s tabs RBTA is used. > RemoveBrowserTabActor name is misleading as it is a weird beast, > that fakes an actor. `form` attribute is actually the ContentActor grip > instanciated by connectToChild. > And RemoteBrowserTabActor doesn't inherit from any anything (TabActor). Yes, I was also surprised to see how RBTA is implemented. I expected we use ContentActor for e10s tabs, but we dont... I kept changes minimal because I think reorganizing the code to use ContentActor for e10s tabs was beyond the purpose of this bug/patch. Do you want me to do this? I can look into how much work would be needed. > ::: toolkit/devtools/server/actors/webconsole.js > @@ +145,5 @@ > > get _parentIsContentActor() { > > + return ("ContentActor" in DebuggerServer && > > + this.parentActor instanceof DebuggerServer.ContentActor) || > > + ("RemoteBrowserTabActor" in DebuggerServer && > > + this.parentActor instanceof DebuggerServer.RemoteBrowserTabActor); > > Does it really happen `this.parentActor instanceof > DebuggerServer.RemoteBrowserTabActor`? > I'm totally lost if it does. Yes, otherwise the webconsole wouldnt get any net events. > @@ +513,2 @@ > > appId = this.parentActor.docShell.appId; > > + messageManager = this.parentActor.chromeEventHandler; > > We should expose an explicit `messageManager` attribute on TabActor. Today, > chromeEventHandler appears to be a message manager, > but it may not be always true. Will do. > ::: toolkit/devtools/webconsole/network-monitor.js > @@ +1084,5 @@ > > this._messageManager.removeMessageListener("debug:netmonitor:updateEvent", this._onUpdateEvent); > > + this._messageManager.sendAsyncMessage("debug:netmonitor", { > > + managerID: this.connID, > > + action: "disconnect", > > + }); > > Instead of filtering in each message listener by managerID, > you may use one message by network monitor. > sendAsyncMessage("debug:netmonitor:" + this.connID, ...); > Same thing for newEvent/updateEvent. > It's not a big deal, so it is up to you. Will do. Thanks for the review!

Updated the patch to address review comments. RemoteBrowserTabActor is confusingly named. This shouldnt be named like that. You are right, it's not an actor, it's just a wrapper object of some sorts for the ContentActor.

Comment on attachment 8400084 [details] [diff] [review] bug989043-2.diff Review of attachment 8400084 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks! Sorry for the delay, it tooks me some time to give it a try on a device.

Thanks Alex! This is the patch with a small test fix and a greenish tbpl push. https://tbpl.mozilla.org/?tree=Try&rev=40c9ca7e03d1