Closed Bug 961392 Opened 6 years ago Closed 6 years ago

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

Categories

(DevTools :: Debugger, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 29

People

(Reporter: janx, Assigned: janx)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Note: This patch probably collisions with paul's bug #942756.
Flags: needinfo?(paul)
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.
Duplicate of this bug: 958451
Alex should review that.
Attachment #8362099 - Flags: review?(poirot.alex)
Flags: needinfo?(paul)
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.
Attachment #8362099 - Flags: review?(poirot.alex) → review?(past)
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.
Attachment #8362099 - Flags: review?(past) → review+
Attached patch refactor.diff (obsolete) — Splinter Review
Thanks for the review! I've included ochameau's suggested improvements and addressed the nits. Past, could you please check this into fx-team?
Attachment #8362099 - Attachment is obsolete: true
Attachment #8365251 - Flags: review?(past)
Attachment #8365251 - Flags: checkin?(past)
Comment on attachment 8365251 [details] [diff] [review]
refactor.diff

Needs rebasing first.
Attachment #8365251 - Flags: review?(past)
Attachment #8365251 - Flags: review+
Attachment #8365251 - Flags: checkin?(past)
Attached patch refactor.diff (obsolete) — Splinter Review
Rebased on top of fx-team to include the memory actor.
Attachment #8365251 - Attachment is obsolete: true
Attachment #8365279 - Flags: review+
Attachment #8365279 - Flags: checkin?(past)
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.
Whiteboard: checkin-needed
Attached patch refactor.diffSplinter Review
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
Attachment #8365279 - Attachment is obsolete: true
Attachment #8365279 - Flags: checkin?(past)
Assignee: janx → past
Keywords: checkin-needed
Whiteboard: checkin-needed
Assignee: past → janx
Thanks for the help! Let's wait for the tree to cycle green.
Attachment #8365301 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/a8ef73c97aa6
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 29
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.