Closed
Bug 961392
Opened 11 years ago
Closed 11 years ago
B2G RemoteDebugger.start() and toolkit DebuggerServer.addBrowserActors() duplicate code
Categories
(DevTools :: Debugger, defect)
DevTools
Debugger
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 29
People
(Reporter: janx, Assigned: janx)
References
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
janx
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•11 years ago
|
||
Note: This patch probably collisions with paul's bug #942756.
Flags: needinfo?(paul)
Assignee | ||
Comment 2•11 years ago
|
||
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•11 years ago
|
||
Alex should review that.
Updated•11 years ago
|
Attachment #8362099 -
Flags: review?(poirot.alex)
Flags: needinfo?(paul)
Comment 5•11 years ago
|
||
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 6•11 years ago
|
||
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+
Assignee | ||
Comment 7•11 years ago
|
||
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 8•11 years ago
|
||
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)
Assignee | ||
Comment 9•11 years ago
|
||
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.
Assignee | ||
Updated•11 years ago
|
Whiteboard: checkin-needed
Comment 11•11 years ago
|
||
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
Updated•11 years ago
|
Attachment #8365279 -
Attachment is obsolete: true
Attachment #8365279 -
Flags: checkin?(past)
Updated•11 years ago
|
Assignee: janx → past
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Whiteboard: checkin-needed
Updated•11 years ago
|
Assignee: past → janx
Assignee | ||
Comment 12•11 years ago
|
||
Thanks for the help! Let's wait for the tree to cycle green.
Assignee | ||
Updated•11 years ago
|
Attachment #8365301 -
Flags: review+
Comment 13•11 years ago
|
||
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Comment 14•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 29
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•