961392 - B2G RemoteDebugger.start() and toolkit DebuggerServer.addBrowserActors() duplicate code

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Jan Keromnes [:janx]]

Attachment: Patch deduplicating DebuggerServer.addBrowserActors() and RemoteDebugger.start().

The initialization instructions in:

http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/main.js#344

and:

http://dxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#1084

are mostly the same, and they load almost exactly the same actors. This code duplication can be fixed if DebuggerServer.addBrowserActors() is refactored to allow for privileged actor restriction, e.g. when certified apps are forbidden in B2G.

Comment 1

[Jan Keromnes [:janx]]

Note: This patch probably collisions with paul's bug #942756.

Comment 2

[Jan Keromnes [:janx]]

Comment on attachment 8362099 [details] [diff] [review]
Patch deduplicating DebuggerServer.addBrowserActors() and RemoteDebugger.start().

Review of attachment 8362099 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/server/main.js
@@ +361,3 @@
>      this.registerModule("devtools/server/actors/device");
> +
> +    if (Services.appinfo.name != "B2G") {

In IRC, jryans told me that such platform specific code should be avoided as much as possible in toolkit. Thankfully, past's bug #933212 should make both the tracer and shader editor work for b2g.

Comment 4

[Paul Rouget [:paul]]

Alex should review that.

Comment 5

[Alexandre Poirot [:ochameau]]

Comment on attachment 8362099 [details] [diff] [review]
Patch deduplicating DebuggerServer.addBrowserActors() and RemoteDebugger.start().

Review of attachment 8362099 [details] [diff] [review]:
-----------------------------------------------------------------

restrictPrivileges sounds weird as any actor is a privilege.
From my point of view, I'd call them tab actors.
It would make even more sense if we also factorize addChildActor, which also register mostly all of them but chromeDebugger.

addBrowserActor: function (chrome, registerTabActors) {
  chromeWindowType = chrome
  addActor(webbrowser)
  if (registerTabActors) {
    this.addTabActor()
    addActor(chromeDebugger)
  }
  addActor(webapps)
  addActor(device)
},
addChildActor: function () {
  if (!BrowserTabActor in this) {
    addActor(webbrowser)
    addTabActor()
  }
  addActor(childtab)
},
addTabActor: function () {
  addActor(script, webconsole, gcli, profiler, inpsector,
           stylesheets, styleeditor)
  if (!B2G) {
    addActor(webgl, tracer)
    // note that webgl is currently loaded on b2g, in child...
    // is it really not working on b2g, or added by mistake in child?
  }
}

But I'd like to know Panos opinion on that.

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8362099 [details] [diff] [review]
Patch deduplicating DebuggerServer.addBrowserActors() and RemoteDebugger.start().

Review of attachment 8362099 [details] [diff] [review]:
-----------------------------------------------------------------

I agree with both Jan's refactoring and Alex's suggested improvement. As for naming things, I like restrictPrivileges better, as it is more indicative of why we do something, compared to registerTabActors that focuses on what we are doing instead (in addition to the fact that adding the chrome debugger when this flag is on makes things confusing).

I imagine that when we make the debugger server e10s-ready it might make sense to rename addBrowserActors to addMainProcessActors or something similar, which would move even closer to the B2G model semantically.

(In reply to Alexandre Poirot (:ochameau) from comment #5)
>   if (!B2G) {
>     addActor(webgl, tracer)
>     // note that webgl is currently loaded on b2g, in child...
>     // is it really not working on b2g, or added by mistake in child?

I'm pretty sure it was left out of shell.js by mistake, which is precisely why we need a patch like this!

::: b2g/chrome/content/shell.js
@@ +1101,3 @@
>      try {
>        DebuggerServer.openListener(path);
>        // Temporary event, until bug 942756 lands and offer a way to know

Since you are fixing typos: s/offer/offers/

::: toolkit/devtools/server/main.js
@@ +345,1 @@
>      this.chromeWindowType = aWindowType ? aWindowType : "navigator:browser";

It would be even better to use the new syntax for default parameters (for both of them) and avoid checking chromeWindowType here.

@@ +362,5 @@
> +
> +    if (Services.appinfo.name != "B2G") {
> +      this.registerModule("devtools/server/actors/webgl");
> +      this.registerModule("devtools/server/actors/tracer");
> +    }

As noted already, special-casing those two is not necessary.

Comment 7

[Jan Keromnes [:janx]]

Attachment: refactor.diff

Thanks for the review! I've included ochameau's suggested improvements and addressed the nits. Past, could you please check this into fx-team?

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8365251 [details] [diff] [review]
refactor.diff

Needs rebasing first.

Comment 9

[Jan Keromnes [:janx]]

Attachment: refactor.diff

Rebased on top of fx-team to include the memory actor.

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Jan, you can also set the "checkin-needed" keyword on the bug, instead of "checkin?" on the patch.  Then one of the sheriffs will check it in for you now that you have r+.  This is the more "typical" workflow.

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: refactor.diff

Ryan is right, checkin-needed is simpler, but since I was around I tried to land it, only to discover the tree was closed. I'm attaching the patch with the updated commit message and author email. Here is what you need to do for future reference:

https://developer.mozilla.org/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 12

[Jan Keromnes [:janx]]

Thanks for the help! Let's wait for the tree to cycle green.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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