989043 - Network monitor support for e10s

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect)

Description

[Mihai Sucan [:msucan]]

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

Comment 1

[Mihai Sucan [:msucan]]

Attachment: bug989043-1.diff

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 2

[Alexandre Poirot [:ochameau]]

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.

Comment 3

[Mihai Sucan [:msucan]]

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

Comment 4

[Mihai Sucan [:msucan]]

Attachment: bug989043-2.diff

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 5

[Alexandre Poirot [:ochameau]]

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.

Comment 6

[Mihai Sucan [:msucan]]

Attachment: bug989043-3.diff

Thanks Alex!

This is the patch with a small test fix and a greenish tbpl push.

https://tbpl.mozilla.org/?tree=Try&rev=40c9ca7e03d1

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/ce3dc45e9490

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ce3dc45e9490