We should be able to start "porting" the Web Console to use the remote debug protocol.

This is the proposed patch. All tests pass, in dbg and opt builds. Asking for review from Rob for the Web Console changes. I'd also like Panos to review the debugger-related changes. Current concerns: 1. I went down the route of adding toolkit/devtools/webconsole - because we want to move more code there, later. Is this acceptable? 2. Do we need separate tests? This code is quite exercised by the Web Console tests. 3. The current approach uses a single WebConsoleActor as a child of the TabActor. It made sense to have only one actor, because having an actor for page errors, for console api listener and so on ... seemed to be overly structured. Beyond that, individual actors wouldn't have many capabilities alone - start/stop and pretty much that. I'd like to know if this is an acceptable approach or if changes are needed. Also, wrt documentation in the wiki: I am not sure how productive and useful that would be. This code is evolving faster than we can maintain wiki pages. I think we should make sure code is well documented. I could write a wiki page, once it's all done, but I feel it's too early for now. Thoughts? Looking forward to your reviews. Thank you! PS. To track the evolution of these patches, you can follow my patch queue here (where I commit my progress on a daily basis): https://github.com/mihaisucan/mozilla-patch-queue/tree/master/patches-webconsole

Comment on attachment 641584 [details] [diff] [review] proposed patch Review of attachment 641584 [details] [diff] [review]: ----------------------------------------------------------------- Hey, it's working! Woo! This is the first in-firefox extension to the debugger server, so kudos for the hard work. Out of curiosity, did you measure how much the remote protocol slows down the console? I mainly looked at debugger code, so some general comments first. Do you think we'll have more to put in toolkit/devtools/webconsole, or is it going to be just this one actor file? All this extra build time sure seems overkill right now. The other actors are not in jsm files, because the idea is that they are extensions to the debugger server, loaded on demand. In that sense, the expected usage pattern is for a tool to check if the server is initialized, and then add the actors it expects to interact with via DebuggerServer.addActors. One other thing I noticed is that even though opening the console sends the expected protocol messages, closing it doesn't seem to do the same (stopListeners, detach). ::: toolkit/devtools/debugger/dbg-client.jsm @@ +200,5 @@ > "scripts": "scripts", > + "setBreakpoint": "setBreakpoint", > + "getCachedMessages": "getCachedMessages", > + "startListeners": "startListeners", > + "stopListeners": "stopListeners", Is there a reason to not follow the common attach/detach naming scheme (with an extra listeners property) instead of start/stopListeners? @@ +309,2 @@ > if (!aResponse.error) { > + tabClient = new TabClient(self, aTabActor, aResponse.webConsoleActor); I think it would be best to not tie the console client lifetime to the tab client lifetime. For one thing, the debugger has no use for it. A separate attachConsole method would be cleaner IMO. I'd also combine the functionality of startListeners in this method, like I suggested above. ::: toolkit/devtools/debugger/server/dbg-browser-actors.js @@ +8,5 @@ > /** > * Browser-specific actors. > */ > > +Cu.import("resource://gre/modules/devtools/dbg-webconsole-actors.jsm"); I'd prefer to keep browser actors unaware of the existence of console actors, if possible. I realize that without bug 753401 this may be impossible, but I'll note it as a thing to improve in a followup. @@ +307,5 @@ > this._addDebuggees(this.browser.contentWindow.wrappedJSObject); > this._contextPool.addActor(this.threadActor); > + > + this.webConsoleActor = new WebConsoleActor(this); > + this._contextPool.addActor(this.webConsoleActor); Bug 753401 will give us a better way to lazily initialize the console actors, but in the meantime this can move to BTA_grip as mentioned. Also, the contextPool holds actors that are scoped to the thread/context, whereas the console seems more appropriate for the tabPool. @@ +367,5 @@ > > + return { > + type: "tabAttached", > + threadActor: this.threadActor.actorID, > + webConsoleActor: this.webConsoleActor.actorID, In bug 753401 jimb said that it would be better to return the web console actor (and other tab-scoped actors) in the listTabs response (essentially BTA_grip) instead of here. Also, I don't expect us to ship another console actor, so you could use the simpler consoleActor property name. ::: toolkit/devtools/webconsole/Makefile.in @@ +14,5 @@ > +include $(topsrcdir)/config/rules.mk > + > +libs:: > + # $(INSTALL) $(IFLAGS1) $(srcdir)/*.jsm $(FINAL_TARGET)/modules/devtools > + $(INSTALL) $(IFLAGS1) $(srcdir)/server/*.jsm $(FINAL_TARGET)/modules/devtools If we're keeping this new directory, I'd say the extra server/ path seems redundant (unless we expect to add client code as well). So is the commented-out rule. If we change the actor file to load with DebuggerServer.addActor, no special installation rules are required, either. ::: toolkit/devtools/webconsole/server/dbg-webconsole-actors.jsm @@ +15,5 @@ > +XPCOMUtils.defineLazyModuleGetter(this, "Services", > + "resource://gre/modules/Services.jsm"); > + > +XPCOMUtils.defineLazyModuleGetter(this, "DebuggerServer", > + "resource://gre/modules/devtools/dbg-server.jsm"); This looks unused. @@ +27,5 @@ > +}; > + > +const CACHED_MESSAGES = { > + PageError: 1, > + ConsoleAPI: 2, Why do you use magic numbers instead of self-evident strings in the protocol messages? One of the points of using JSON packets was to make debugging protocol interactions easier. Is there something I'm missing? @@ +40,5 @@ > + * @constructor > + * @param object aTabActor > + * The parent TabActor instance for which we live for. > + */ > +function WebConsoleActor(aTabActor) From an architectural POV, I think it would be better to explicitly pass the window and connection, instead of the tab actor. Also, I'm not sure that attaching to the tab will always be required in order to attach to the console. @@ +47,5 @@ > +} > + > +WebConsoleActor.prototype = > +{ > + get contentWindow() this.tabActor.browser.contentWindow, Since the console actor stores the contentWindow, perhaps a similar strategy as the one employed by the tab actor is necessary to monitor page navigation and reloads? If not, maybe this isn't needed? @@ +50,5 @@ > +{ > + get contentWindow() this.tabActor.browser.contentWindow, > + get conn() this.tabActor.conn, > + > + actorPrefix: "webConsole", Use "console", you were here first! ;-) @@ +61,5 @@ > + /** > + * Destroy the current WebConsoleActor instance. This is called by the > + * TabActor. > + */ > + exit: function WCA_exit() You also need a disconnect method that does the same, because it is being called by AP_cleanup in the connection tear down path. I'd make exit() call disconnect(). @@ +84,5 @@ > + case LISTENERS.PageError: > + if (this.pageErrorListener) { > + return { > + error: "wrongState", > + message: "The PageError subscription is already active.", I think it would be better to treat this case as a no-op instead of an error. For one thing, the request is not in conflict with the current state, so no surprise to the caller. Also, returning here, will mean that the whole request will fail, even though some listeners may have started successfully. Furthermore, when designing protocol clients, I believe it's a good idea to keep Postel's maxim in mind: be conservative in what you produce, liberal in what you accept ;-) @@ +97,5 @@ > + message: "Unknown value for listeners: " + listener, > + }; > + } > + } > + return { startListeners: this.actorID }; The startListeners property is redundant, since the response packet will contain a "from" property with the actor ID. On the other hand, a list of the started listeners may be actually useful to the client. @@ +106,5 @@ > + * > + * @param object aRequest > + * The JSON request object received from the Web Console client. > + */ > + onStopListeners: function WCA_onStopListeners(aRequest) The same points as in onStartListener apply here as well. @@ +138,5 @@ > + * > + * @param object aRequest > + * The JSON request object received from the Web Console client. > + */ > + onGetCachedMessages: function WCA_onGetCachedMessages(aRequest) Is this used anywhere? I didn't observe any such packets in my testing. When is this supposed to be called? In a polling fashion every few msec? @@ +155,5 @@ > + switch (types.shift()) { > + case CACHED_MESSAGES.ConsoleAPI: > + if (this.consoleAPIListener) { > + messages.push.apply(messages, > + this.consoleAPIListener.getCachedMessages()); The ConsoleAPI listener doesn't seem to be implemented yet, right? I don't see any protocol messages exchanged for console.log calls. @@ +188,5 @@ > + { > + let packet = { > + from: this.actorID, > + type: "pageError", > + pageError: aPageError, Hm, so you just serialize whatever is returned by the console service. This is not very nice design for a protocol, because protocols are supposed to be generalized, but maybe the ease of implementation should matter here? I'd like to keep our prospects of standardizing the remote debugging protocol at some point in the future open, and I doubt other vendors would accept this. Of course this holds for other extensions, like the profiler, NetMonitor, etc., so maybe we can deal with it when standardization becomes a goal. In any case I'm interested in Jim's thoughts on this.

Oops, Jim apparently wasn't CC'd before. Hey Jim, I have a question for you in comment 2, last paragraph.

...aaaaand I forgot to reply to those questions, duh! (In reply to Mihai Sucan [:msucan] from comment #1) > Created attachment 641584 [details] [diff] [review] > proposed patch > > This is the proposed patch. All tests pass, in dbg and opt builds. > > Asking for review from Rob for the Web Console changes. I'd also like Panos > to review the debugger-related changes. > > Current concerns: > > 1. I went down the route of adding toolkit/devtools/webconsole - because we > want to move more code there, later. Is this acceptable? Ah, so you want to move console code unrelated to the remote protocol to toolkit? That's fine by me. > 2. Do we need separate tests? This code is quite exercised by the Web > Console tests. It's telling that tests that exercise console.log calls work currently, even though they don't go through the protocol. Are you sure you'll catch such issues with the existing tests? Consider also the case where a remote device navigates away from a page and the client has no way to observe that, beyond a protocol notification. I think a bunch of simple xpcshell tests that exercise the protocol would give sufficient extra coverage. See toolkit/devtools/debugger/tests. > 3. The current approach uses a single WebConsoleActor as a child of the > TabActor. It made sense to have only one actor, because having an actor for > page errors, for console api listener and so on ... seemed to be overly > structured. Beyond that, individual actors wouldn't have many capabilities > alone - start/stop and pretty much that. > > I'd like to know if this is an acceptable approach or if changes are needed. Seems fine to me. > Also, wrt documentation in the wiki: I am not sure how productive and useful > that would be. This code is evolving faster than we can maintain wiki pages. > I think we should make sure code is well documented. I could write a wiki > page, once it's all done, but I feel it's too early for now. Thoughts? Agreed. The only benefit from a wiki page with sample protocol messages right now, is faster reviews ;-)

(In reply to Panos Astithas [:past] from comment #2) > Comment on attachment 641584 [details] [diff] [review] > proposed patch > > Review of attachment 641584 [details] [diff] [review]: > ----------------------------------------------------------------- > > Hey, it's working! Woo! > This is the first in-firefox extension to the debugger server, so kudos for > the hard work. Out of curiosity, did you measure how much the remote > protocol slows down the console? With all the async fun, measuring the performance impact is no longer trivial. UI performance and content script performance are not directly impacted, afaik. > I mainly looked at debugger code, so some general comments first. Do you > think we'll have more to put in toolkit/devtools/webconsole, or is it going > to be just this one actor file? All this extra build time sure seems > overkill right now. For now, for this bug: only the actor. Should I move the WebConsoleClient into its own jsm and take it out from the dbg-client.jsm? Later on I want to move most of the Web Console code into toolkit. I'd like Rob's input as well - I could, for now, just put the web console actor jsm in the debugger/server folder? > The other actors are not in jsm files, because the idea is that they are > extensions to the debugger server, loaded on demand. In that sense, the > expected usage pattern is for a tool to check if the server is initialized, > and then add the actors it expects to interact with via > DebuggerServer.addActors. True, but I am not too sure what you'd like me to change here. Just rename to .js and use addActors()? Where should I call addActors()? I assume in the initServer() method in the HUDService.jsm file. > One other thing I noticed is that even though opening the console sends the > expected protocol messages, closing it doesn't seem to do the same > (stopListeners, detach). Good points. I need to check this more. What I saw was that when I close the web console, the actor exit() was called, so independently calling stopListeners() from the web console doesn't seem needed, because I stop the listeners anyway (on disconnect, that is). I need to check how detach works. > ::: toolkit/devtools/debugger/dbg-client.jsm > @@ +200,5 @@ > > "scripts": "scripts", > > + "setBreakpoint": "setBreakpoint", > > + "getCachedMessages": "getCachedMessages", > > + "startListeners": "startListeners", > > + "stopListeners": "stopListeners", > > Is there a reason to not follow the common attach/detach naming scheme (with > an extra listeners property) instead of start/stopListeners? No specifing reason. It didn't seem like I am "attaching" to anything - I am starting/stopping listeners in the actor. Shall I change to attach/detach? I can do this, if preferred. > @@ +309,2 @@ > > if (!aResponse.error) { > > + tabClient = new TabClient(self, aTabActor, aResponse.webConsoleActor); > > I think it would be best to not tie the console client lifetime to the tab > client lifetime. For one thing, the debugger has no use for it. A separate > attachConsole method would be cleaner IMO. I'd also combine the > functionality of startListeners in this method, like I suggested above. If I correctly understand, you suggest I go for attachConsole(aTabActor, aListeners)? (something like that) So I can just list tabs and attach a console to a tab, and give the list of listeners I want, from one go. Sounds like a good idea, if I got it right. > ::: toolkit/devtools/debugger/server/dbg-browser-actors.js > @@ +8,5 @@ > > /** > > * Browser-specific actors. > > */ > > > > +Cu.import("resource://gre/modules/devtools/dbg-webconsole-actors.jsm"); > > I'd prefer to keep browser actors unaware of the existence of console > actors, if possible. I realize that without bug 753401 this may be > impossible, but I'll note it as a thing to improve in a followup. I'll look into this, if I can do it. > @@ +307,5 @@ > > this._addDebuggees(this.browser.contentWindow.wrappedJSObject); > > this._contextPool.addActor(this.threadActor); > > + > > + this.webConsoleActor = new WebConsoleActor(this); > > + this._contextPool.addActor(this.webConsoleActor); > > Bug 753401 will give us a better way to lazily initialize the console > actors, but in the meantime this can move to BTA_grip as mentioned. Also, > the contextPool holds actors that are scoped to the thread/context, whereas > the console seems more appropriate for the tabPool. Will look into this, thanks! > @@ +367,5 @@ > > > > + return { > > + type: "tabAttached", > > + threadActor: this.threadActor.actorID, > > + webConsoleActor: this.webConsoleActor.actorID, > > In bug 753401 jimb said that it would be better to return the web console > actor (and other tab-scoped actors) in the listTabs response (essentially > BTA_grip) instead of here. Also, I don't expect us to ship another console > actor, so you could use the simpler consoleActor property name. Ok then. > ::: toolkit/devtools/webconsole/Makefile.in > @@ +14,5 @@ > > +include $(topsrcdir)/config/rules.mk > > + > > +libs:: > > + # $(INSTALL) $(IFLAGS1) $(srcdir)/*.jsm $(FINAL_TARGET)/modules/devtools > > + $(INSTALL) $(IFLAGS1) $(srcdir)/server/*.jsm $(FINAL_TARGET)/modules/devtools > > If we're keeping this new directory, I'd say the extra server/ path seems > redundant (unless we expect to add client code as well). So is the > commented-out rule. > > If we change the actor file to load with DebuggerServer.addActor, no special > installation rules are required, either. Will look into doing this. > ::: toolkit/devtools/webconsole/server/dbg-webconsole-actors.jsm > @@ +15,5 @@ > > +XPCOMUtils.defineLazyModuleGetter(this, "Services", > > + "resource://gre/modules/Services.jsm"); > > + > > +XPCOMUtils.defineLazyModuleGetter(this, "DebuggerServer", > > + "resource://gre/modules/devtools/dbg-server.jsm"); > > This looks unused. Yep, forgot to clean it up. > @@ +27,5 @@ > > +}; > > + > > +const CACHED_MESSAGES = { > > + PageError: 1, > > + ConsoleAPI: 2, > > Why do you use magic numbers instead of self-evident strings in the protocol > messages? One of the points of using JSON packets was to make debugging > protocol interactions easier. Is there something I'm missing? Good point - I wanted to avoid too many redundant strings, but that makes the protocol less readable. Will fix. > @@ +40,5 @@ > > + * @constructor > > + * @param object aTabActor > > + * The parent TabActor instance for which we live for. > > + */ > > +function WebConsoleActor(aTabActor) > > From an architectural POV, I think it would be better to explicitly pass the > window and connection, instead of the tab actor. Also, I'm not sure that > attaching to the tab will always be required in order to attach to the > console. Sure, if I go the route of attachConsole() then this makes more sense. > @@ +47,5 @@ > > +} > > + > > +WebConsoleActor.prototype = > > +{ > > + get contentWindow() this.tabActor.browser.contentWindow, > > Since the console actor stores the contentWindow, perhaps a similar strategy > as the one employed by the tab actor is necessary to monitor page navigation > and reloads? If not, maybe this isn't needed? I looked into this and I didn't see a need to track page nav / reload, but I still need to access the contentWindow at times - when an error is logged I need to check if it belongs to the contentWindow. I don't want to store a reference, so I don't need to track page nav / reload. This getter was sufficient for my needs. > @@ +50,5 @@ > > +{ > > + get contentWindow() this.tabActor.browser.contentWindow, > > + get conn() this.tabActor.conn, > > + > > + actorPrefix: "webConsole", > > Use "console", you were here first! ;-) OK! :) > @@ +61,5 @@ > > + /** > > + * Destroy the current WebConsoleActor instance. This is called by the > > + * TabActor. > > + */ > > + exit: function WCA_exit() > > You also need a disconnect method that does the same, because it is being > called by AP_cleanup in the connection tear down path. I'd make exit() call > disconnect(). Ah, good point. I was wondering if exit() is sufficient or not. > @@ +84,5 @@ > > + case LISTENERS.PageError: > > + if (this.pageErrorListener) { > > + return { > > + error: "wrongState", > > + message: "The PageError subscription is already active.", > > I think it would be better to treat this case as a no-op instead of an > error. For one thing, the request is not in conflict with the current state, > so no surprise to the caller. Also, returning here, will mean that the whole > request will fail, even though some listeners may have started successfully. > Furthermore, when designing protocol clients, I believe it's a good idea to > keep Postel's maxim in mind: be conservative in what you produce, liberal in > what you accept ;-) Will do. I was interested to catch errors, but I see your point. > @@ +97,5 @@ > > + message: "Unknown value for listeners: " + listener, > > + }; > > + } > > + } > > + return { startListeners: this.actorID }; > > The startListeners property is redundant, since the response packet will > contain a "from" property with the actor ID. On the other hand, a list of > the started listeners may be actually useful to the client. Will do. > @@ +138,5 @@ > > + * > > + * @param object aRequest > > + * The JSON request object received from the Web Console client. > > + */ > > + onGetCachedMessages: function WCA_onGetCachedMessages(aRequest) > > Is this used anywhere? I didn't observe any such packets in my testing. When > is this supposed to be called? In a polling fashion every few msec? Not yet. It will be used in patch part 2. > @@ +155,5 @@ > > + switch (types.shift()) { > > + case CACHED_MESSAGES.ConsoleAPI: > > + if (this.consoleAPIListener) { > > + messages.push.apply(messages, > > + this.consoleAPIListener.getCachedMessages()); > > The ConsoleAPI listener doesn't seem to be implemented yet, right? I don't > see any protocol messages exchanged for console.log calls. Correct. That will be in part 2. Shall I remove all strictly unused code? I added these because I felt they "prepare grounds". I used this code for testing stuff. > @@ +188,5 @@ > > + { > > + let packet = { > > + from: this.actorID, > > + type: "pageError", > > + pageError: aPageError, > > Hm, so you just serialize whatever is returned by the console service. This > is not very nice design for a protocol, because protocols are supposed to be > generalized, but maybe the ease of implementation should matter here? I'd > like to keep our prospects of standardizing the remote debugging protocol at > some point in the future open, and I doubt other vendors would accept this. > Of course this holds for other extensions, like the profiler, NetMonitor, > etc., so maybe we can deal with it when standardization becomes a goal. In > any case I'm interested in Jim's thoughts on this. I see your point, but I'd like us to keep this as-is for a number of reasons: 1. abstracting away nsIScriptErrors for some potential benefit in the future has no immediate win. 2. potential benefit in the future is only abstract - anything we can come up with now will be changed when vendors get together to standardize the protocol. 3. standardization is slow. At this point I do not see any obvious benefit from abstracting things away. When any nsIScriptError change happens we would then need to update the protocol, instead of just the code that uses these objects. However, if Jim wants any changes, I am open to do them. Maybe I fail to see the importance of making some protocol changes. Thanks for your review, Panos!

(In reply to Mihai Sucan [:msucan] from comment #5) > (In reply to Panos Astithas [:past] from comment #2) > > Comment on attachment 641584 [details] [diff] [review] > > proposed patch > > > > Review of attachment 641584 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Hey, it's working! Woo! > > This is the first in-firefox extension to the debugger server, so kudos for > > the hard work. Out of curiosity, did you measure how much the remote > > protocol slows down the console? > > With all the async fun, measuring the performance impact is no longer > trivial. UI performance and content script performance are not directly > impacted, afaik. Can we at least verify that the stuff that you worked so hard on last month doesn't regress the tests you used back then? If I'm reading this patch correctly, the only significant ongoing change would be the fact that we're serializing messages and sending them over a pipe. We probably want a local-only transport that doesn't do all that extra work.

(In reply to Mihai Sucan [:msucan] from comment #5) > (In reply to Panos Astithas [:past] from comment #2) > > I mainly looked at debugger code, so some general comments first. Do you > > think we'll have more to put in toolkit/devtools/webconsole, or is it going > > to be just this one actor file? All this extra build time sure seems > > overkill right now. > > For now, for this bug: only the actor. Should I move the WebConsoleClient > into its own jsm and take it out from the dbg-client.jsm? Nah, there is some debugger client refactoring that will be happening soon, no need to split this one out. > Later on I want to move most of the Web Console code into toolkit. > > I'd like Rob's input as well - I could, for now, just put the web console > actor jsm in the debugger/server folder? That's fine by me. > > The other actors are not in jsm files, because the idea is that they are > > extensions to the debugger server, loaded on demand. In that sense, the > > expected usage pattern is for a tool to check if the server is initialized, > > and then add the actors it expects to interact with via > > DebuggerServer.addActors. > > True, but I am not too sure what you'd like me to change here. Just rename > to .js and use addActors()? Where should I call addActors()? I assume in the > initServer() method in the HUDService.jsm file. Exactly. > > ::: toolkit/devtools/debugger/dbg-client.jsm > > @@ +200,5 @@ > > > "scripts": "scripts", > > > + "setBreakpoint": "setBreakpoint", > > > + "getCachedMessages": "getCachedMessages", > > > + "startListeners": "startListeners", > > > + "stopListeners": "stopListeners", > > > > Is there a reason to not follow the common attach/detach naming scheme (with > > an extra listeners property) instead of start/stopListeners? > > No specifing reason. It didn't seem like I am "attaching" to anything - I am > starting/stopping listeners in the actor. Shall I change to attach/detach? I > can do this, if preferred. Well, there is a pervasive concept of attaching to an actor in the debugger protocol code. So in that sense it would be more in line with existing practice. > > @@ +309,2 @@ > > > if (!aResponse.error) { > > > + tabClient = new TabClient(self, aTabActor, aResponse.webConsoleActor); > > > > I think it would be best to not tie the console client lifetime to the tab > > client lifetime. For one thing, the debugger has no use for it. A separate > > attachConsole method would be cleaner IMO. I'd also combine the > > functionality of startListeners in this method, like I suggested above. > > If I correctly understand, you suggest I go for attachConsole(aTabActor, > aListeners)? (something like that) So I can just list tabs and attach a > console to a tab, and give the list of listeners I want, from one go. Sounds > like a good idea, if I got it right. Yes, with the only difference that the first argument would be the console actor, which would have been returned in the listTabs response. > > @@ +47,5 @@ > > > +} > > > + > > > +WebConsoleActor.prototype = > > > +{ > > > + get contentWindow() this.tabActor.browser.contentWindow, > > > > Since the console actor stores the contentWindow, perhaps a similar strategy > > as the one employed by the tab actor is necessary to monitor page navigation > > and reloads? If not, maybe this isn't needed? > > I looked into this and I didn't see a need to track page nav / reload, but I > still need to access the contentWindow at times - when an error is logged I > need to check if it belongs to the contentWindow. I don't want to store a > reference, so I don't need to track page nav / reload. This getter was > sufficient for my needs. OK, just be sure to test if the content window you get after a page reload/navigation is the one you expect. > > @@ +155,5 @@ > > > + switch (types.shift()) { > > > + case CACHED_MESSAGES.ConsoleAPI: > > > + if (this.consoleAPIListener) { > > > + messages.push.apply(messages, > > > + this.consoleAPIListener.getCachedMessages()); > > > > The ConsoleAPI listener doesn't seem to be implemented yet, right? I don't > > see any protocol messages exchanged for console.log calls. > > Correct. That will be in part 2. > > Shall I remove all strictly unused code? I added these because I felt they > "prepare grounds". I used this code for testing stuff. No, it's fine if you are landing this in stages.

(In reply to Dave Camp (:dcamp) from comment #6) > Can we at least verify that the stuff that you worked so hard on last month > doesn't regress the tests you used back then? > > If I'm reading this patch correctly, the only significant ongoing change > would be the fact that we're serializing messages and sending them over a > pipe. We probably want a local-only transport that doesn't do all that > extra work. This is definitely something I want to check. When I worked on perf stuff I mostly tested with the console API messages, not with script errors. Once I have the console messages as well, I will be able to better compare things. Sure, a local-only transport makes sense.

(In reply to Panos Astithas [:past] from comment #4) > ...aaaaand I forgot to reply to those questions, duh! > > > 2. Do we need separate tests? This code is quite exercised by the Web > > Console tests. > > It's telling that tests that exercise console.log calls work currently, even > though they don't go through the protocol. Are you sure you'll catch such > issues with the existing tests? Consider also the case where a remote device > navigates away from a page and the client has no way to observe that, beyond > a protocol notification. Web Console tests exercise a lot the page errors support, not only the window.console API messages. Page navigation and reloads are tested. Protocol notification for such activity is needed for other parts of the Web Console, and I'll add that in a later patch. I hope I can use the existing stuff in tab actors. > I think a bunch of simple xpcshell tests that exercise the protocol would > give sufficient extra coverage. See toolkit/devtools/debugger/tests. I will look into doing this. Thanks for your answers to my questions.

Changes: - Made BTA_grip() to hold the consoleActor ID, so we can attachConsole() after listTabs, without doing attachTab(). The code is rather weird now, I had to make "unpleasant" changes. We really need Bug 753401. - I couldn't avoid touching browser actors - again, we need bug 753401. - attach/detach versus start/stopListeners: during the lifetime of the consoleActor I want to allow the client to start/stop specific listeners. We can use the attach/detach naming, but it shouldn't be confused as "call attach once when you connect, and detach once you disconnect". I imagine the devtoolbar will attach to the page errors stuff and later when the user opens the web console the same connection is shared - I just attach/start the rest of listeners. - I feel that I'd better like start/stopListeners() than attach/detach(). Thoughts? Maybe I'm using attach/detach() in wrong ways... - I cannot use addActors() from HUDService.jsm. It works only if we ever use the Web Console only. If you start a Firefox session and open the debugger the server is initialized and later on, when you start the Web Console the server is no longer reinitialized and addActors() call doesn't happen and I don't get the web console actor. I had to put the code to always invoke addActors() in browser-actors. We can put it other places if you want, or we can wait for bug 753401. If I always call addActors() for the same file, in HUDService, the second time when I load the actors script, the code throws because it tries to redeclare already-declared variables. Obviously... - Decided against creating the new toolkit/devtools/webconsole folder for the moment - too early. I moved the new actors js file into toolkit/devtools/debugger/server. - Before I move to write xpcshell tests, I'd like to have the patch closer to something you would r+. Looking forward to your reviews. Thanks!

Here is the interdiff: https://github.com/mihaisucan/mozilla-patch-queue/commit/39d0c5e7dd26a75d14c89df4a56a0385b020238a

Comment on attachment 644287 [details] [diff] [review] address panos's review comments Review of attachment 644287 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Mihai Sucan [:msucan] from comment #10) > - Made BTA_grip() to hold the consoleActor ID, so we can attachConsole() > after listTabs, without doing attachTab(). The code is rather weird now, I > had to make "unpleasant" changes. We really need Bug 753401. OK, I'll make a note to clean this up when I get back to bug 753401 (Real Soon Now). > - attach/detach versus start/stopListeners: during the lifetime of the > consoleActor I want to allow the client to start/stop specific listeners. > We can use the attach/detach naming, but it shouldn't be confused as "call > attach once when you connect, and detach once you disconnect". I imagine > the devtoolbar will attach to the page errors stuff and later when the > user opens the web console the same connection is shared - I just > attach/start the rest of listeners. This maybe the root of our misunderstanding, I was assuming you'd have two separate connections to the same server. > - I feel that I'd better like start/stopListeners() than attach/detach(). > Thoughts? Maybe I'm using attach/detach() in wrong ways... OK, let's do it your way. > - I cannot use addActors() from HUDService.jsm. It works only if we ever use > the Web Console only. If you start a Firefox session and open the debugger > the server is initialized and later on, when you start the Web Console the > server is no longer reinitialized and addActors() call doesn't happen and I > don't get the web console actor. I had to put the code to always invoke > addActors() in browser-actors. We can put it other places if you want, or we > can wait for bug 753401. > > If I always call addActors() for the same file, in HUDService, the second > time when I load the actors script, the code throws because it tries to > redeclare already-declared variables. Obviously... OK, this can wait, as well. ::: toolkit/devtools/debugger/dbg-client.jsm @@ +175,5 @@ > const UnsolicitedNotifications = { > "newScript": "newScript", > "tabDetached": "tabDetached", > + "tabNavigated": "tabNavigated", > + "pageError": "pageError", Nit: we keep this list sorted. @@ +199,5 @@ > "prototypeAndProperties": "prototypeAndProperties", > "resume": "resume", > "scripts": "scripts", > + "setBreakpoint": "setBreakpoint", > + "getCachedMessages": "getCachedMessages", Same here. ::: toolkit/devtools/debugger/server/dbg-webconsole-actors.js @@ +22,5 @@ > + PageError: "PageError", > + ConsoleAPI: "ConsoleAPI", > +}; > + > +const CACHED_MESSAGES = { Do you envision a case in the future that would cause LISTENERS and CACHED_MESSAGES to not be the same, or can we get rid of one of them?

(In reply to Panos Astithas [:past] from comment #12) > Comment on attachment 644287 [details] [diff] [review] > address panos's review comments > > Review of attachment 644287 [details] [diff] [review]: > ----------------------------------------------------------------- > > > - I feel that I'd better like start/stopListeners() than attach/detach(). > > Thoughts? Maybe I'm using attach/detach() in wrong ways... > > OK, let's do it your way. Thank you! > ::: toolkit/devtools/debugger/server/dbg-webconsole-actors.js > @@ +22,5 @@ > > + PageError: "PageError", > > + ConsoleAPI: "ConsoleAPI", > > +}; > > + > > +const CACHED_MESSAGES = { > > Do you envision a case in the future that would cause LISTENERS and > CACHED_MESSAGES to not be the same, or can we get rid of one of them? Yes. For example, network logging has a listener but no cached messages. Thanks for your review+!

Thank you Panos! Rob: please review this patch and let me know if things are looking good. I am inclining to put the new WebConsoleProxy object in webconsole.js, to keep HUDService.jsm lightweight. Thoughts? I did put the message manager stuff in HUDService.jsm when I did the web console iframe split because it relied on the Firefox browser stuff. I now believe that WebConsoleProxy could nicely live in webconsole.js, for when we do the move to toolkit. HUDService.jsm will remain the "Firefox glue code". Others, for Thunderbird and Seamonkey, can do a separate lightweight integration JSM. Just my thoughts... I'm going to start work on part 2 - window.console API messages.

Adding bug 753401 as a dependency, as suggested by Rob. We need that for multiple reasons. The patches I have here do not yet require bug 753401 but it would nice to have that, as also discussed with Panos. ;)

Comment on attachment 649744 [details] [diff] [review] address latest comments from panos Review of attachment 649744 [details] [diff] [review]: ----------------------------------------------------------------- clearing review for now. Looks good. Only real concern is the commented code in HUDService.jsm and the duplicated console-type constants which is more of a nit of an over-specification. I'll give this another pass when you think this is ready to land. ::: browser/devtools/webconsole/HUDService.jsm @@ +1169,5 @@ > + /** > + * Tells if the connection was established. > + * @type boolean > + */ > + connected: false, should these be private and prefixed with underscores? Not sure if owner is referenced anywhere outside of WebConsoleConnectionProxy. @@ +1235,5 @@ > + aCallback && aCallback(); > + > + //let msg = [this.webConsoleClient.CACHED_MESSAGES.PageError]; > + //this.webConsoleClient.getCachedMessages(msgs, > + // this._onCachedMessages); presumably these can be removed in the final patch. If not, you need a TODO comment and bug number for followup. @@ +1240,5 @@ > + }, > + > + // TODO: This is almost working. Need to implement ConsoleAPI support in the > + // WebConsoleActor and it's done. > + _onCachedMessages: function WCCP__onCachedMessages(aResponse) ok. This is still a WIP. @@ +1247,5 @@ > + Cu.reportError("Web Console getCachedMessages error: " + aResponse.error + > + " " + aResponse.message); > + return; > + } > + //dump("WCCP__onCachedMessages " + aResponse.messages.length + "\n"); don't forget this. @@ +1274,5 @@ > + > + this.client = null; > + this.webConsoleClient = null; > + this.connected = false; > + }, do you still need the reference to owner here? Does a client connection ever get reused? ::: browser/devtools/webconsole/test/browser_webconsole_bug_597103_deactivateHUDForContext_unfocused_window.js @@ +111,2 @@ > } > no comment here other than it's nice to see arguments.callee going away. :) ::: browser/devtools/webconsole/test/browser_webconsole_bug_601909_remember_height.js @@ +105,2 @@ > }, true); > } mmm. generators. ::: toolkit/devtools/debugger/dbg-client.jsm @@ +560,5 @@ > + CACHED_MESSAGES: { > + PageError: "PageError", > + ConsoleAPI: "ConsoleAPI", > + }, > + do we really need separate type containers for these? Will they ever differ? Listeners and CachedMessages appear to be the same, so they could be consolidated. ::: toolkit/devtools/debugger/server/dbg-browser-actors.js @@ +292,5 @@ > } > > + // Shut down actors that belong to this tab's pool. > + this._removeTabActorPool(); > + I wish we could consolidate all of this detach-related code into a single place, but don't have to do that here. There's some refactoring that needs to be done around disconnection as Nick has discovered. ::: toolkit/devtools/debugger/server/dbg-webconsole-actors.js @@ +26,5 @@ > +const CACHED_MESSAGES = { > + PageError: "PageError", > + ConsoleAPI: "ConsoleAPI", > +}; > + ah, these again. Can we just have a single CONSOLE_TYPES or similar? Unless you feel it's important for readability, I'm not sure it buys us much. @@ +164,5 @@ > + let messages = []; > + > + while (types.length > 0) { > + switch (types.shift()) { > + case CACHED_MESSAGES.ConsoleAPI: I was looking at these to see if there's a solid reason to separate LISTENERS from CACHED_MESSAGES and I'm not really seeing it. Given that we're referencing these inside onGetCachedMessages, it's pretty evident that these types are cached. They're still the same type, they just live in different places.

Comment on attachment 649744 [details] [diff] [review] address latest comments from panos This is the first patch. Jim: please let us know what you think about how we use the protocol and any comments you have. You might want to read previous comments to understand some of the decisions we've taken. Thanks! (I'll address Rob's comments as soon as I have the second patch ready. Thank you Rob!)

This patch makes the Web Console use the remote debugging protocol for window.console API messages, JS evaluation, object inspection and autocomplete. There are some concerns: - Performance. I ran 50000 calls to console.log("foo " +i ), with the Web Console open. All messages were displayed in ~14 seconds without the remoting patches. With remoting patches it took ~3m40s... Shall we report a separate bug? - Please look in the patch for TODOs I marked. Some of them might need to be fixed here or in separate follow up bugs. - Code organization: - I'd like to move the WebConsoleConnectionProxy to webconsole.js. Reasoning: I want to keep HUDService.jsm as a minimal glue for Firefox and the Web Console. The connection stuff is something that's not Firefox specific. - I'd like to move PEL, CAL and JSTerm helpers from the web console actors script files into WebConsoleUtils.jsm (or a new jsm?). Reasoning: I want the debugger actors script to be minimal as well. These objects are well contained that they can live in a jsm from where they can be imported at any time, for any purpose. - Rob: please let me know if this is ok with you or if we should do things differently. Thank you! Looking forward to your reviews.

Comment on attachment 654728 [details] [diff] [review] part 2: window.console messages and JS evaluation Review of attachment 654728 [details] [diff] [review]: ----------------------------------------------------------------- Boy, was this long. But having almost all of the console working with the remote protocol was totally worth it! Excellent job Mihai! ::: browser/devtools/webconsole/WebConsoleUtils.jsm @@ +358,2 @@ > } > + catch (ex) { } In which cases can this throw? If we don't want to handle the errors, let's add a comment in the empty catch clause. @@ +511,5 @@ > } > } > + catch (ex if (ex.name == "NS_ERROR_XPC_BAD_CONVERT_JS" || > + ex.name == "NS_ERROR_XPC_BAD_OP_ON_WN_PROTO" || > + ex.name == "TypeError")) { How come we ignore TypeError now? Add to the comment, please. @@ +517,5 @@ > } > + try { > + aObject = Object.getPrototypeOf(aObject); > + } > + catch (ex if (ex.name == "TypeError")) { Ditto. @@ +645,5 @@ > + // Sort string. > + else if (a.name < b.name) { > + return -1; > + } > + else if (a.name > b.name) { These don't look l10n-friendly. Followup material? @@ +685,5 @@ > + > + properties.push(this.inspectObjectProperty(aObject, name, aObjectWrapper)); > + } > + > + return properties.sort(this.propertiesSort); How come you didn't use the same format as the debugger's prototypeAndProperties request? Besides being a familiar format, it would have ensured that when we are able to reuse the debugger's ObjectActors here, there would have to be no changes to the client side. Also, in that case you could reuse the parsing code from debugger-controller.js. Note that since this is a JSON format, expanding on a packet to add console-related stuff, like "inspectable", is totally fine. @@ +959,5 @@ > + * The object you want to get the class name for. > + * @return string > + * The object class name. > + */ > + getObjectClassName: function WCF_getObjectClassName(aObject) A lot of these helpers would be significantly simplified if we were using the Debugger API instead of a Sandbox, but that is definitely for another day. ::: browser/devtools/webconsole/test/browser_webconsole_bug_632347_iterators_generators.js @@ +116,5 @@ > > ok(find("iter1: Iterator", false), > "iter1 is correctly displayed in the Property Panel"); > > + ok(find("iter2: Iterator", false) || find("iter2: Object", false), How come it's not always the same? ::: toolkit/devtools/debugger/dbg-client.jsm @@ +198,5 @@ > "interrupt": "interrupt", > "listTabs": "listTabs", > "nameAndParameters": "nameAndParameters", > "ownPropertyNames": "ownPropertyNames", > + "prototypeAndProperties": "prototypeAndProperties", This seems to have been accidentally moved out of place. @@ +614,5 @@ > + * > + * @param string aActor > + * The WebConsoleObjectActor ID to send the request to. > + */ > + releaseObject: function WCC_releaseObject(aActor) { Since the "release" request is used in the debugger as well, could you move this definition to the DebuggerClient? There hasn't been a need for it in the debugger frontend so far, but it may well come up in the future. I'd also prefer it if we called it "releaseActor" or plain "release" instead. @@ +637,5 @@ > + let packet = { > + to: this._actor, > + type: DebugProtocolTypes.evaluateJS, > + requestId: aRequestId, > + str: aString, String or text sound better than str. @@ +660,5 @@ > + let packet = { > + to: this._actor, > + type: DebugProtocolTypes.autocomplete, > + requestId: aRequestId, > + str: aString, Ditto. ::: toolkit/devtools/debugger/server/dbg-browser-actors.js @@ +293,5 @@ > > + if (this.consoleActor) { > + this._tabPool && this._tabPool.removeActor(this.consoleActor.actorID); > + this.consoleActor.disconnect(); > + this.consoleActor = null; Instead of copying around the same code, how about an _exitConsole() or similar? ::: toolkit/devtools/debugger/server/dbg-webconsole-actors.js @@ +18,5 @@ > XPCOMUtils.defineLazyModuleGetter(this, "WebConsoleUtils", > "resource:///modules/WebConsoleUtils.jsm"); > > +XPCOMUtils.defineLazyModuleGetter(this, "JSPropertyProvider", > + "resource:///modules/WebConsoleUtils.jsm"); Creating references from toolkit/ to browser/ will make non-Firefox embeddings, like B2G or Fennec, unable to use the web console. Can't you move this stuff to toolkit/ or just define those helpers here? I looked specifically for createValueGrip, which doesn't look like it's used elsewhere. There may be others. @@ +51,5 @@ > + > + this._objectActorsPool = new ActorPool(this.conn); > + this.conn.addActorPool(this._objectActorsPool); > + > + this._objectActors = new Map(); Not WeakMap? @@ +115,5 @@ > + catch (ex) { } > + > + return { > + actor: this.actorID, > + nativeConsole: nativeConsole, I see that the nativeConsole flag is only used when the frontend retrieves the cached messages from the server. In that case, why not send this flag with the cachedMessages or the startedListeners response? This flag is not something that clients would want to know before they attach to the console. That way you'd get a simple actor ID grip again, which would simplify other places. @@ +132,5 @@ > + if (this.consoleAPIListener) { > + this.consoleAPIListener.destroy(); > + this.consoleAPIListener = null; > + } > + if (this._objectActorsPool) { You can drop the conditional here. I don't see how this._objectActorsPool can not be initialized by the time you get here, and removeActorPool can handle undefined/null arguments. @@ +182,5 @@ > + * > + * @param string aActorID > + * @return object > + */ > + getObjectActorByID: function WCA_getObjectActorByID(aActorID) You might want to drop the ByID if there is no other way to get an object actor. @@ +492,5 @@ > + * @private > + * @return Window > + * The XUL window that owns the content window. > + */ > + _xulWindow: function WCA__xulWindow() Nit: not that I care much, but maybe _chromeWindow makes more sense? @@ +521,5 @@ > + */ > +function WebConsoleObjectActor(aObj, aWebConsoleActor) > +{ > + this.obj = aObj; > + this.parentActor = aWebConsoleActor; Nit: how about s/parentActor/parent/g? @@ +557,5 @@ > + onInspectProperties: function WCOA_onInspectProperties() > + { > + // TODO: use LongStringActor for strings that are too long. Maybe we could > + // just substr() for now? We don't really need to allow very long strings > + // here, anyway. Thoughts? Yes, I think long strings will be a perfect fit here. @@ +568,5 @@ > + }; > + }, > + > + /** > + * Handle a protocol request to release a thread-lifetime grip. Nit: remove the "thread-lifetime" part. @@ +820,5 @@ > + _prepareMessageForRemote: function CAL__prepareMessageForRemote(aMessage) > + { > + let result = WebConsoleUtils.cloneObject(aMessage, true, > + function(aKey, aValue, aObject) { > + // We need to skip the arguments property from the original object. Can you explain why and add it to the comment? @@ +834,5 @@ > + case "group": > + case "groupCollapsed": > + case "time": > + case "timeEnd": > + result.arguments = aMessage.arguments; Don't you want to create a grip here, for the cases where improper arguments were used? @@ +837,5 @@ > + case "timeEnd": > + result.arguments = aMessage.arguments; > + break; > + case "groupEnd": > + result.arguments = []; Why not pass the actual arguments to the client?

In the interest of helping others understand the new protocol changes for the console API, here is a log of me playing with the patch. I typed console.log("foo") and then inspect(document.body). You can see all the new requests implemented in the second patch.

(In reply to Rob Campbell [:rc] (:robcee) from comment #16) > Comment on attachment 649744 [details] [diff] [review] > address latest comments from panos > > Review of attachment 649744 [details] [diff] [review]: > ----------------------------------------------------------------- > > clearing review for now. Looks good. Only real concern is the commented code > in HUDService.jsm and the duplicated console-type constants which is more of > a nit of an over-specification. I'll give this another pass when you think > this is ready to land. Thanks for the review! > ::: browser/devtools/webconsole/HUDService.jsm > @@ +1169,5 @@ > > + /** > > + * Tells if the connection was established. > > + * @type boolean > > + */ > > + connected: false, > > should these be private and prefixed with underscores? Not sure if owner is > referenced anywhere outside of WebConsoleConnectionProxy. I keep these public for later potential use. The |connected| property is used from the WebConsole object to check the connection state. If you want me to make certain props private, please reconfirm. > @@ +1235,5 @@ > > + aCallback && aCallback(); > > + > > + //let msg = [this.webConsoleClient.CACHED_MESSAGES.PageError]; > > + //this.webConsoleClient.getCachedMessages(msgs, > > + // this._onCachedMessages); > > presumably these can be removed in the final patch. If not, you need a TODO > comment and bug number for followup. Will cleanup. Follow up is the part 2 patch. > @@ +1274,5 @@ > > + > > + this.client = null; > > + this.webConsoleClient = null; > > + this.connected = false; > > + }, > > do you still need the reference to owner here? Does a client connection ever > get reused? It's never reused. I don't dereference the owner to prevent potential "crashes"/exceptions in any async listener. Lesson learned the hard way in previous async Web Console patches. Better to make sure I cleanup the proxy object. (if you want, I can dereference the owner) > ::: > browser/devtools/webconsole/test/ > browser_webconsole_bug_597103_deactivateHUDForContext_unfocused_window.js > @@ +111,2 @@ > > } > > > > no comment here other than it's nice to see arguments.callee going away. :) Yes. :) > ::: toolkit/devtools/debugger/dbg-client.jsm > @@ +560,5 @@ > > + CACHED_MESSAGES: { > > + PageError: "PageError", > > + ConsoleAPI: "ConsoleAPI", > > + }, > > + > > do we really need separate type containers for these? Will they ever differ? > Listeners and CachedMessages appear to be the same, so they could be > consolidated. This is something Panos also asked in previous comments. I will add the network and file activity listeners, which do not have any cached messages. So LISTENERS and CACHED_MESSAGES will not be the same. To simplify things I could just remove the constants and use "ConsoleAPI"/"PageError"/etc directly. Thoughts? > ::: toolkit/devtools/debugger/server/dbg-browser-actors.js > @@ +292,5 @@ > > } > > > > + // Shut down actors that belong to this tab's pool. > > + this._removeTabActorPool(); > > + > > I wish we could consolidate all of this detach-related code into a single > place, but don't have to do that here. There's some refactoring that needs > to be done around disconnection as Nick has discovered. Yeah... I found these changes as "ugly". I eagerly wait for the extensible debugger API. Will update the patch ASAP.

Updated to address Rob's review. Interdiff: https://github.com/mihaisucan/mozilla-patch-queue/commit/8aaddc81992ebe950f85d35af4e2950101f6951a https://github.com/mihaisucan/mozilla-patch-queue/commit/d7d35e285341b9123f90f5d744efbc972ed05bd4 I'm not sure if piling up the webconsole-actors.js script with PageErrorListener, ConsoleAPIObserver and JSTermHelpers is ideal. I'd like to move some of these things into WebConsoleUtils.jsm (or something new?). Please let me know if further changes are needed. Thank you!

(In reply to Panos Astithas [:past] from comment #19) > Comment on attachment 654728 [details] [diff] [review] > part 2: window.console messages and JS evaluation > > Review of attachment 654728 [details] [diff] [review]: > ----------------------------------------------------------------- > > Boy, was this long. But having almost all of the console working with the > remote protocol was totally worth it! Excellent job Mihai! Thank you! > ::: browser/devtools/webconsole/WebConsoleUtils.jsm > @@ +358,2 @@ > > } > > + catch (ex) { } > > In which cases can this throw? If we don't want to handle the errors, let's > add a comment in the empty catch clause. Good point. Will add a comment. This can throw if the constructor .name is a getter that throws, or if .name is an object with a broken toString() and so on. > @@ +511,5 @@ > > } > > } > > + catch (ex if (ex.name == "NS_ERROR_XPC_BAD_CONVERT_JS" || > > + ex.name == "NS_ERROR_XPC_BAD_OP_ON_WN_PROTO" || > > + ex.name == "TypeError")) { > > How come we ignore TypeError now? Add to the comment, please. TypeError comes from trying to get the property descriptor on non-objects like null. > @@ +645,5 @@ > > + // Sort string. > > + else if (a.name < b.name) { > > + return -1; > > + } > > + else if (a.name > b.name) { > > These don't look l10n-friendly. Followup material? Unfortunately, this was always the case. Sure, filed bug 787361. > @@ +685,5 @@ > > + > > + properties.push(this.inspectObjectProperty(aObject, name, aObjectWrapper)); > > + } > > + > > + return properties.sort(this.propertiesSort); > > How come you didn't use the same format as the debugger's > prototypeAndProperties request? Besides being a familiar format, it would > have ensured that when we are able to reuse the debugger's ObjectActors > here, there would have to be no changes to the client side. Also, in that > case you could reuse the parsing code from debugger-controller.js. > > Note that since this is a JSON format, expanding on a packet to add > console-related stuff, like "inspectable", is totally fine. I worked on that, actually. I started with the ObjectActor code, I copied and renamed it to WebConsoleObjectActor. Went the way of adding things I needed. However, when I needed to do something like inspectObject, I bumped into the main issue with the ObjectActor: I couldn't reuse any of the existing message types: - ownPropertyNames: gives only property names. - prototypeAndProperties: this was the closest to my needs and this is what I started hacking on. The problem is it gives only ownProperties and prototype is a separate object actor. - onPrototype, onProperty, onScope, onNameAndParameters ... etc are not even close. After analyzing what I needed it became obvious that even if the Web Console would use the thread actor and the object actor, I'd need a new message type. What I need is to get all of the enumerable properties on the object. Thoughts? Shall I make any changes here? > @@ +959,5 @@ > > + * The object you want to get the class name for. > > + * @return string > > + * The object class name. > > + */ > > + getObjectClassName: function WCF_getObjectClassName(aObject) > > A lot of these helpers would be significantly simplified if we were using > the Debugger API instead of a Sandbox, but that is definitely for another > day. Yes, it's kind of unfortunate I can't directly go the way of using the debugger API without causing page slowdowns. Anyway, it should be easy to later migrate this code to nicer stuff, to simplify the code. I will open a file-up bug for this. > ::: > browser/devtools/webconsole/test/ > browser_webconsole_bug_632347_iterators_generators.js > @@ +116,5 @@ > > > > ok(find("iter1: Iterator", false), > > "iter1 is correctly displayed in the Property Panel"); > > > > + ok(find("iter2: Iterator", false) || find("iter2: Object", false), > > How come it's not always the same? Hah, this is a debugging/testing remnant. Will fix. > ::: toolkit/devtools/debugger/dbg-client.jsm > @@ +198,5 @@ > > "interrupt": "interrupt", > > "listTabs": "listTabs", > > "nameAndParameters": "nameAndParameters", > > "ownPropertyNames": "ownPropertyNames", > > + "prototypeAndProperties": "prototypeAndProperties", > > This seems to have been accidentally moved out of place. Yep. > @@ +614,5 @@ > > + * > > + * @param string aActor > > + * The WebConsoleObjectActor ID to send the request to. > > + */ > > + releaseObject: function WCC_releaseObject(aActor) { > > Since the "release" request is used in the debugger as well, could you move > this definition to the DebuggerClient? There hasn't been a need for it in > the debugger frontend so far, but it may well come up in the future. > > I'd also prefer it if we called it "releaseActor" or plain "release" instead. Will do. > @@ +637,5 @@ > > + let packet = { > > + to: this._actor, > > + type: DebugProtocolTypes.evaluateJS, > > + requestId: aRequestId, > > + str: aString, > > String or text sound better than str. Will do. > ::: toolkit/devtools/debugger/server/dbg-browser-actors.js > @@ +293,5 @@ > > > > + if (this.consoleActor) { > > + this._tabPool && this._tabPool.removeActor(this.consoleActor.actorID); > > + this.consoleActor.disconnect(); > > + this.consoleActor = null; > > Instead of copying around the same code, how about an _exitConsole() or > similar? Will do. > ::: toolkit/devtools/debugger/server/dbg-webconsole-actors.js > @@ +18,5 @@ > > XPCOMUtils.defineLazyModuleGetter(this, "WebConsoleUtils", > > "resource:///modules/WebConsoleUtils.jsm"); > > > > +XPCOMUtils.defineLazyModuleGetter(this, "JSPropertyProvider", > > + "resource:///modules/WebConsoleUtils.jsm"); > > Creating references from toolkit/ to browser/ will make non-Firefox > embeddings, like B2G or Fennec, unable to use the web console. Can't you > move this stuff to toolkit/ or just define those helpers here? I looked > specifically for createValueGrip, which doesn't look like it's used > elsewhere. There may be others. Known issue with the patch. :( Two solutions: 1. use the extensibility API you work on and put the webconsole-actors script in browser/devtools. (IIANM, this should work) 2. move WebConsoleUtils.jsm into toolkit/devtools. This requires I also add a strings file because WCU has some strings... I'd like to pick solution 2. > @@ +51,5 @@ > > + > > + this._objectActorsPool = new ActorPool(this.conn); > > + this.conn.addActorPool(this._objectActorsPool); > > + > > + this._objectActors = new Map(); > > Not WeakMap? Weak maps can't hold any kind of object. The page script can give us any kind of object. > @@ +115,5 @@ > > + catch (ex) { } > > + > > + return { > > + actor: this.actorID, > > + nativeConsole: nativeConsole, > > I see that the nativeConsole flag is only used when the frontend retrieves > the cached messages from the server. In that case, why not send this flag > with the cachedMessages or the startedListeners response? This flag is not > something that clients would want to know before they attach to the console. > That way you'd get a simple actor ID grip again, which would simplify other > places. Putting the nativeConsole flag in the cachedMessages message is "shoehorning" - I don't want to require the client to get the cachedMessages just to check the nativeConsole flag. I can put this flag on the startedListeners response, but I'm not "too happy" - I don't have a better solution either. ;) > @@ +132,5 @@ > > + if (this.consoleAPIListener) { > > + this.consoleAPIListener.destroy(); > > + this.consoleAPIListener = null; > > + } > > + if (this._objectActorsPool) { > > You can drop the conditional here. I don't see how this._objectActorsPool > can not be initialized by the time you get here, and removeActorPool can > handle undefined/null arguments. Will do. > @@ +182,5 @@ > > + * > > + * @param string aActorID > > + * @return object > > + */ > > + getObjectActorByID: function WCA_getObjectActorByID(aActorID) > > You might want to drop the ByID if there is no other way to get an object > actor. I'd like to keep this, unless you have any strong feelings about this. Why: makes it clearer how you get the object actor (by what). > @@ +492,5 @@ > > + * @private > > + * @return Window > > + * The XUL window that owns the content window. > > + */ > > + _xulWindow: function WCA__xulWindow() > > Nit: not that I care much, but maybe _chromeWindow makes more sense? Sure. > @@ +521,5 @@ > > + */ > > +function WebConsoleObjectActor(aObj, aWebConsoleActor) > > +{ > > + this.obj = aObj; > > + this.parentActor = aWebConsoleActor; > > Nit: how about s/parentActor/parent/g? Will rename. > @@ +557,5 @@ > > + onInspectProperties: function WCOA_onInspectProperties() > > + { > > + // TODO: use LongStringActor for strings that are too long. Maybe we could > > + // just substr() for now? We don't really need to allow very long strings > > + // here, anyway. Thoughts? > > Yes, I think long strings will be a perfect fit here. Yep, but should I do it in this bug? I mean, the object inspector panel has no UI for handling long strings. At this point I'd like to file a follow-up bug about using LongStringActor in the object inspection. Until then, shall I limit the length of strings we send? (in this patch). I think the users of the object inspector, as-is now, would mind with, say, a hard limit of 1000 chars. (the immediate win here would be that we don't send too much data over the wire, in the remote connection case) > @@ +568,5 @@ > > + }; > > + }, > > + > > + /** > > + * Handle a protocol request to release a thread-lifetime grip. > > Nit: remove the "thread-lifetime" part. Argh. Did I mention I started with the ObjectActor? Hehe. > @@ +820,5 @@ > > + _prepareMessageForRemote: function CAL__prepareMessageForRemote(aMessage) > > + { > > + let result = WebConsoleUtils.cloneObject(aMessage, true, > > + function(aKey, aValue, aObject) { > > + // We need to skip the arguments property from the original object. > > Can you explain why and add it to the comment? Will do. > @@ +834,5 @@ > > + case "group": > > + case "groupCollapsed": > > + case "time": > > + case "timeEnd": > > + result.arguments = aMessage.arguments; > > Don't you want to create a grip here, for the cases where improper arguments > were used? These arguments come from the ConsoleAPI and these are not objects meant to be inspected from the page. If we get improper arguments, that's a bug in the ConsoleAPI (or we need to update this code). If I were to use value grips for these arguments, the Web Console UI would have to make additional requests before being able to show the messages - making things overly complex. For example, timeEnd gives an object which would be wrapped in an object grip. I'd need to get the properties, etc. Actually, we are inconsistent in the ConsoleAPI code: we should always pass all of the console API arguments. Things like the timeEnd object which tell timer name and duration should be additional properties on the console API message we send through the observer service. If this would be consistent across all methods, we could make our code here consistent as well, in the web console actor. Thoughts? Follow-up material? > @@ +837,5 @@ > > + case "timeEnd": > > + result.arguments = aMessage.arguments; > > + break; > > + case "groupEnd": > > + result.arguments = []; > > Why not pass the actual arguments to the client? In this case the Web Console UI doesn't use the arguments. For consistency, I will update the patch to pass the actual arguments. Thank you for the review!

(In reply to Mihai Sucan [:msucan] from comment #23) > (In reply to Panos Astithas [:past] from comment #19) > > @@ +685,5 @@ > > > + > > > + properties.push(this.inspectObjectProperty(aObject, name, aObjectWrapper)); > > > + } > > > + > > > + return properties.sort(this.propertiesSort); > > > > How come you didn't use the same format as the debugger's > > prototypeAndProperties request? Besides being a familiar format, it would > > have ensured that when we are able to reuse the debugger's ObjectActors > > here, there would have to be no changes to the client side. Also, in that > > case you could reuse the parsing code from debugger-controller.js. > > > > Note that since this is a JSON format, expanding on a packet to add > > console-related stuff, like "inspectable", is totally fine. > > I worked on that, actually. I started with the ObjectActor code, I copied > and renamed it to WebConsoleObjectActor. Went the way of adding things I > needed. However, when I needed to do something like inspectObject, I bumped > into the main issue with the ObjectActor: I couldn't reuse any of the > existing message types: > > - ownPropertyNames: gives only property names. > - prototypeAndProperties: this was the closest to my needs and this is what > I started hacking on. The problem is it gives only ownProperties and > prototype is a separate object actor. > - onPrototype, onProperty, onScope, onNameAndParameters ... etc are not even > close. > > After analyzing what I needed it became obvious that even if the Web Console > would use the thread actor and the object actor, I'd need a new message > type. What I need is to get all of the enumerable properties on the object. > > Thoughts? Shall I make any changes here? Right, I understand that a new allProperties/inspectProperties request makes more sense here, I'm not arguing against that. What I wonder about is how come you didn't reuse the general structure of the prototypeAndProperties response: things like using an object instead of an array for |properties| with the property names as keys, using proper object grips as values, etc. In case you are trying to fit everything the frontend might need in a single response, be aware that this trades snappiness for throughput. In other words, the frontend will have the data it needs ASAP, but it will have to wait until all of that data is transferred and parsed before displaying anything. With a response containing the traditional value grips, the frontend would be able to display properties incrementally, while retrieving the missing data. This also fits long string handling perfectly. Also, as another data point, in bug 786070 we are considering displaying the ownProperties before properties on the prototype chain in the debugger's variable view, since that would be the most pertinent information for the user, most of the time. If we consider the property panel as having the same requirements (which seems like a reasonable assumption), using prototypeAndProperties instead of allProperties, would obviously help with incremental display and snappiness as well. > > ::: toolkit/devtools/debugger/server/dbg-webconsole-actors.js > > @@ +18,5 @@ > > > XPCOMUtils.defineLazyModuleGetter(this, "WebConsoleUtils", > > > "resource:///modules/WebConsoleUtils.jsm"); > > > > > > +XPCOMUtils.defineLazyModuleGetter(this, "JSPropertyProvider", > > > + "resource:///modules/WebConsoleUtils.jsm"); > > > > Creating references from toolkit/ to browser/ will make non-Firefox > > embeddings, like B2G or Fennec, unable to use the web console. Can't you > > move this stuff to toolkit/ or just define those helpers here? I looked > > specifically for createValueGrip, which doesn't look like it's used > > elsewhere. There may be others. > > Known issue with the patch. :( > > Two solutions: > > 1. use the extensibility API you work on and put the webconsole-actors > script in browser/devtools. (IIANM, this should work) > > 2. move WebConsoleUtils.jsm into toolkit/devtools. This requires I also add > a strings file because WCU has some strings... > > I'd like to pick solution 2. Solution 1 would "work" in the sense that B2G and Fennec would not be broken, but wouldn't have a remotable console either, right? I agree that solution 2 would be better in the long term, and in fact I'm already adding a strings file in toolkit/ in bug 781515. > > @@ +115,5 @@ > > > + catch (ex) { } > > > + > > > + return { > > > + actor: this.actorID, > > > + nativeConsole: nativeConsole, > > > > I see that the nativeConsole flag is only used when the frontend retrieves > > the cached messages from the server. In that case, why not send this flag > > with the cachedMessages or the startedListeners response? This flag is not > > something that clients would want to know before they attach to the console. > > That way you'd get a simple actor ID grip again, which would simplify other > > places. > > Putting the nativeConsole flag in the cachedMessages message is > "shoehorning" - I don't want to require the client to get the cachedMessages > just to check the nativeConsole flag. > > I can put this flag on the startedListeners response, but I'm not "too > happy" - I don't have a better solution either. ;) You are already getting the cached messages before acting on the nativeConsole flag, so if it feels like shoehorning, you are already doing it ;-) The fact of the matter is that the only use of this flag is for displaying the warning message when the user tries to view console API output, and this can only happen after cached messages have been received. If you want to try to come up with an argument about how the flag is more semantically aligned with startedListeners or something else, I won't argue with you :-) > > @@ +182,5 @@ > > > + * > > > + * @param string aActorID > > > + * @return object > > > + */ > > > + getObjectActorByID: function WCA_getObjectActorByID(aActorID) > > > > You might want to drop the ByID if there is no other way to get an object > > actor. > > I'd like to keep this, unless you have any strong feelings about this. Why: > makes it clearer how you get the object actor (by what). I don't care much about it, other than to point out that there is no ambiguity when there is no more than one way to do things. > > @@ +557,5 @@ > > > + onInspectProperties: function WCOA_onInspectProperties() > > > + { > > > + // TODO: use LongStringActor for strings that are too long. Maybe we could > > > + // just substr() for now? We don't really need to allow very long strings > > > + // here, anyway. Thoughts? > > > > Yes, I think long strings will be a perfect fit here. > > Yep, but should I do it in this bug? I mean, the object inspector panel has > no UI for handling long strings. At this point I'd like to file a follow-up > bug about using LongStringActor in the object inspection. Until then, shall > I limit the length of strings we send? (in this patch). I think the users of > the object inspector, as-is now, would mind with, say, a hard limit of 1000 > chars. (the immediate win here would be that we don't send too much data > over the wire, in the remote connection case) Note that you don't necessarily have to add special UI for long strings, like an ellipsis button that fetches the remainder of the string, or whatever. Depending on the use case, a reasonable thing to do may be to display whatever is already received and fetch the remaining parts on mouseover, panel resize, etc. Having said that, I think using long strings in a followup is perfectly fine. This work is going to be hard to finish as it is, there is no point in trying to make it "perfect" from day 1. I wouldn't bother adding hard limits for now, either. It's not like we plan on waiting a couple of months before fixing it. > > @@ +834,5 @@ > > > + case "group": > > > + case "groupCollapsed": > > > + case "time": > > > + case "timeEnd": > > > + result.arguments = aMessage.arguments; > > > > Don't you want to create a grip here, for the cases where improper arguments > > were used? > > These arguments come from the ConsoleAPI and these are not objects meant to > be inspected from the page. If we get improper arguments, that's a bug in > the ConsoleAPI (or we need to update this code). If I were to use value > grips for these arguments, the Web Console UI would have to make additional > requests before being able to show the messages - making things overly > complex. > > For example, timeEnd gives an object which would be wrapped in an object > grip. I'd need to get the properties, etc. > > Actually, we are inconsistent in the ConsoleAPI code: we should always pass > all of the console API arguments. Things like the timeEnd object which tell > timer name and duration should be additional properties on the console API > message we send through the observer service. If this would be consistent > across all methods, we could make our code here consistent as well, in the > web console actor. > > Thoughts? Follow-up material? The reason I mentioned it is that questionable API usage like console.group({a:1}) is already supported in the current web console, although not as well as in Firebug or Chrome (we just stringify the object to get a label). Returning content objects as grips is very important in the debugger, for security reasons among others, so it would seem prudent to follow the same path in here as well.

(In reply to Panos Astithas [:past] from comment #24) > (In reply to Mihai Sucan [:msucan] from comment #23) > > I worked on that, actually. I started with the ObjectActor code, I copied > > and renamed it to WebConsoleObjectActor. Went the way of adding things I > > needed. However, when I needed to do something like inspectObject, I bumped > > into the main issue with the ObjectActor: I couldn't reuse any of the > > existing message types: > > > > - ownPropertyNames: gives only property names. > > - prototypeAndProperties: this was the closest to my needs and this is what > > I started hacking on. The problem is it gives only ownProperties and > > prototype is a separate object actor. > > - onPrototype, onProperty, onScope, onNameAndParameters ... etc are not even > > close. > > > > After analyzing what I needed it became obvious that even if the Web Console > > would use the thread actor and the object actor, I'd need a new message > > type. What I need is to get all of the enumerable properties on the object. > > > > Thoughts? Shall I make any changes here? > > Right, I understand that a new allProperties/inspectProperties request makes > more sense here, I'm not arguing against that. What I wonder about is how > come you didn't reuse the general structure of the prototypeAndProperties > response: things like using an object instead of an array for |properties| > with the property names as keys, using proper object grips as values, etc. I see. - I used an array for |properties| instead of an object because that worked best for the property panel. I change to an object if you want. Shall I? - "using proper object grips as values"? what do you mean? can you please explain? > In case you are trying to fit everything the frontend might need in a single > response, be aware that this trades snappiness for throughput. In other > words, the frontend will have the data it needs ASAP, but it will have to > wait until all of that data is transferred and parsed before displaying > anything. With a response containing the traditional value grips, the > frontend would be able to display properties incrementally, while retrieving > the missing data. This also fits long string handling perfectly. Good point. This sounds like a problem we'd have even with the ObjectActor from the script-actors.js. How about we send at max 100 (or whatever the number) property descriptors at once? Just like a LongStringActor, we should limit the number of properties we send at once. We could make so the client can request more. We could also look into trimming the property descriptor - thoughts? Like I could remove |writable|, |configurable|, |enumerable| and I could avoid sending grips for the getter and setter - I could just have booleans |hasGetter| and |hasSetter|. Lastly, should we look into fixing the issue at hand in this bug? Or shall we open another follow up? > Also, as another data point, in bug 786070 we are considering displaying the > ownProperties before properties on the prototype chain in the debugger's > variable view, since that would be the most pertinent information for the > user, most of the time. If we consider the property panel as having the same > requirements (which seems like a reasonable assumption), using > prototypeAndProperties instead of allProperties, would obviously help with > incremental display and snappiness as well. In this set of patches I'd like to keep the same behavior. Changing order might bring up discussion... Shall I open a follow up bug? > > Known issue with the patch. :( > > > > Two solutions: > > > > 1. use the extensibility API you work on and put the webconsole-actors > > script in browser/devtools. (IIANM, this should work) > > > > 2. move WebConsoleUtils.jsm into toolkit/devtools. This requires I also add > > a strings file because WCU has some strings... > > > > I'd like to pick solution 2. > > Solution 1 would "work" in the sense that B2G and Fennec would not be > broken, but wouldn't have a remotable console either, right? I agree that > solution 2 would be better in the long term, and in fact I'm already adding > a strings file in toolkit/ in bug 781515. Yeah, I already did solution 2. I avoided adding strings to toolkit. I had only one string! > > Putting the nativeConsole flag in the cachedMessages message is > > "shoehorning" - I don't want to require the client to get the cachedMessages > > just to check the nativeConsole flag. > > > > I can put this flag on the startedListeners response, but I'm not "too > > happy" - I don't have a better solution either. ;) > > You are already getting the cached messages before acting on the > nativeConsole flag, so if it feels like shoehorning, you are already doing > it ;-) > > The fact of the matter is that the only use of this flag is for displaying > the warning message when the user tries to view console API output, and this > can only happen after cached messages have been received. If you want to try > to come up with an argument about how the flag is more semantically aligned > with startedListeners or something else, I won't argue with you :-) You're right that the web console always gets the cached messages before acting on the nativeConsole flag. However, the web console will probably not be the only consumer of this new protocol API. So I decided to move the nativeConsole flag to the response of startListeners - one always needs to start listeners, and one of them is the console API listener. > > > > > > Yes, I think long strings will be a perfect fit here. > > > > Yep, but should I do it in this bug? I mean, the object inspector panel has > > no UI for handling long strings. At this point I'd like to file a follow-up > > bug about using LongStringActor in the object inspection. Until then, shall > > I limit the length of strings we send? (in this patch). I think the users of > > the object inspector, as-is now, would mind with, say, a hard limit of 1000 > > chars. (the immediate win here would be that we don't send too much data > > over the wire, in the remote connection case) > > Note that you don't necessarily have to add special UI for long strings, > like an ellipsis button that fetches the remainder of the string, or > whatever. Depending on the use case, a reasonable thing to do may be to > display whatever is already received and fetch the remaining parts on > mouseover, panel resize, etc. > > Having said that, I think using long strings in a followup is perfectly > fine. This work is going to be hard to finish as it is, there is no point in > trying to make it "perfect" from day 1. I wouldn't bother adding hard limits > for now, either. It's not like we plan on waiting a couple of months before > fixing it. Opened bug 787981. Thanks for your thoughts on this. > > > @@ +834,5 @@ > > > > + case "group": > > > > + case "groupCollapsed": > > > > + case "time": > > > > + case "timeEnd": > > > > + result.arguments = aMessage.arguments; > > > > > > Don't you want to create a grip here, for the cases where improper arguments > > > were used? > > > > These arguments come from the ConsoleAPI and these are not objects meant to > > be inspected from the page. If we get improper arguments, that's a bug in > > the ConsoleAPI (or we need to update this code). If I were to use value > > grips for these arguments, the Web Console UI would have to make additional > > requests before being able to show the messages - making things overly > > complex. > > > > For example, timeEnd gives an object which would be wrapped in an object > > grip. I'd need to get the properties, etc. > > > > Actually, we are inconsistent in the ConsoleAPI code: we should always pass > > all of the console API arguments. Things like the timeEnd object which tell > > timer name and duration should be additional properties on the console API > > message we send through the observer service. If this would be consistent > > across all methods, we could make our code here consistent as well, in the > > web console actor. > > > > Thoughts? Follow-up material? > > The reason I mentioned it is that questionable API usage like > console.group({a:1}) is already supported in the current web console, > although not as well as in Firebug or Chrome (we just stringify the object > to get a label). Returning content objects as grips is very important in the > debugger, for security reasons among others, so it would seem prudent to > follow the same path in here as well. You are correct, but the console API notification doesn't even send us the {a:1} object (in the case of group()). We just get it stringified. The debugger actor I'm adding can't send the object grip for {a:1}. I made some changes to make the code more robust, but I still don't use createValueGrip() for time/timeEnd which give the timer name and duration, to avoid complicating the code in the client. I also filed bug 787985 to fixup the Console API - I'd like there to also improve the way we handle the console API messages in the debugger code (always create value grips for all arguments given to any console API call). Going to attach an updated patch.

Rob, apologies for updating the patch while probably you are reviewing it. In comment 19 Panos rightly pointed out that we do Cu.import() in toolkit/ from browser/. I had planned to fix this issue once the debugger extensibility API is ready, but now I just went ahead and moved WebConsoleUtils.jsm into toolkit/devtools/webconsole. I also moved PageErrorListener into WCU.jsm so I can reuse it for the errors counter in the developer toolbar - HUDService-content.js will be removed and we probably want to avoid enabling the entire "machinery" of the Web Console and the remote debugging protocol just for the errors counter.

Updated the patch to address Panos's review comments. For interdiff please check the commits here: https://github.com/mihaisucan/mozilla-patch-queue/commits/ Please let me know if I missed anything. Thanks!

Panos: to see the performance issues I mentioned in comment 18 just load attachment 593077 [details] from bug 722685. Change the loop to 25000 or more and compare with these patches and without. Shall I open a follow up bug? Might depend on your testing if we can fix this here or in a follow up... Thanks!

Comment on attachment 657895 [details] [diff] [review] part 2: window.console messages and JS evaluation (v0.2) Review of attachment 657895 [details] [diff] [review]: ----------------------------------------------------------------- I'm OK with this as a first cut. We'll have plenty of opportunity to improve it in the future.

(In reply to Mihai Sucan [:msucan] from comment #25) > (In reply to Panos Astithas [:past] from comment #24) > > (In reply to Mihai Sucan [:msucan] from comment #23) > > > I worked on that, actually. I started with the ObjectActor code, I copied > > > and renamed it to WebConsoleObjectActor. Went the way of adding things I > > > needed. However, when I needed to do something like inspectObject, I bumped > > > into the main issue with the ObjectActor: I couldn't reuse any of the > > > existing message types: > > > > > > - ownPropertyNames: gives only property names. > > > - prototypeAndProperties: this was the closest to my needs and this is what > > > I started hacking on. The problem is it gives only ownProperties and > > > prototype is a separate object actor. > > > - onPrototype, onProperty, onScope, onNameAndParameters ... etc are not even > > > close. > > > > > > After analyzing what I needed it became obvious that even if the Web Console > > > would use the thread actor and the object actor, I'd need a new message > > > type. What I need is to get all of the enumerable properties on the object. > > > > > > Thoughts? Shall I make any changes here? > > > > Right, I understand that a new allProperties/inspectProperties request makes > > more sense here, I'm not arguing against that. What I wonder about is how > > come you didn't reuse the general structure of the prototypeAndProperties > > response: things like using an object instead of an array for |properties| > > with the property names as keys, using proper object grips as values, etc. > > I see. > > - I used an array for |properties| instead of an object because that worked > best for the property panel. I change to an object if you want. Shall I? > > - "using proper object grips as values"? what do you mean? can you please > explain? My main goal is reusability. If using an object and regular object grips (e.g. no displayString or functionName in the value property) helps you to reuse debugger frontend or backend code for parsing and generating the packets, then it sounds like a good thing to do. If, on the other hand, you can't get it to work the same way, you may as well leave it as it is now. > > In case you are trying to fit everything the frontend might need in a single > > response, be aware that this trades snappiness for throughput. In other > > words, the frontend will have the data it needs ASAP, but it will have to > > wait until all of that data is transferred and parsed before displaying > > anything. With a response containing the traditional value grips, the > > frontend would be able to display properties incrementally, while retrieving > > the missing data. This also fits long string handling perfectly. > > Good point. This sounds like a problem we'd have even with the ObjectActor > from the script-actors.js. How about we send at max 100 (or whatever the > number) property descriptors at once? Just like a LongStringActor, we should > limit the number of properties we send at once. We could make so the client > can request more. > > We could also look into trimming the property descriptor - thoughts? Like I > could remove |writable|, |configurable|, |enumerable| and I could avoid > sending grips for the getter and setter - I could just have booleans > |hasGetter| and |hasSetter|. > > Lastly, should we look into fixing the issue at hand in this bug? Or shall > we open another follow up? Optimizing without a clear target to optimize for is usually not a good idea. I'd like us to postpone this work until we have figured out the existing bottlenecks (if any). Also, moving further away from the packet formats used by the debugger should be the last of our options. A protocol has to be consistent if it is going to have any chances of adoption and success. > > Also, as another data point, in bug 786070 we are considering displaying the > > ownProperties before properties on the prototype chain in the debugger's > > variable view, since that would be the most pertinent information for the > > user, most of the time. If we consider the property panel as having the same > > requirements (which seems like a reasonable assumption), using > > prototypeAndProperties instead of allProperties, would obviously help with > > incremental display and snappiness as well. > > In this set of patches I'd like to keep the same behavior. Changing order > might bring up discussion... > > Shall I open a follow up bug? Certainly, I only mentioned this as something to inform our direction here. (In reply to Mihai Sucan [:msucan] from comment #28) > Panos: to see the performance issues I mentioned in comment 18 just load > attachment 593077 [details] from bug 722685. Change the loop to 25000 or > more and compare with these patches and without. Shall I open a follow up > bug? Might depend on your testing if we can fix this here or in a follow > up... I will test it ASAP, but a separate bug is warranted IMO, for all these performance-related issues. I don't think anyone expected to get the remote console as fast as the local one from the start :-)

This is the network logging part. This patch marks the complete transition of the Web Console to the remote debugging protocol. The message manager and HUDService-content.js script are no longer used. All tests pass. Looking forward for your reviews and feedback. Thank you!

As agreed with Rob, I'm updating this patch to address a problem with how it handles releasing of object actors.

Remove unused code and small fixes in several places. All tests pass, but dbg builds have memory leaks. Still need to fix that.

By the way, bug 785174 has landed: Debugger.Object.prototype.evalInGlobal has been implemented. I believe that's the right thing for the console to use to evaluate expressions in a content context.

Looking at the protocol log, we're sending a lot of traffic for autocompletion that isn't necessary; filed as bug 790016.

Comment on attachment 658607 [details] [diff] [review] part 3: network logging (v0.1) Review of attachment 658607 [details] [diff] [review]: ----------------------------------------------------------------- The only important thing is simplifying the networkActivity packets. The rest is good and can be improved with followup work. ::: toolkit/devtools/debugger/dbg-client.jsm @@ +685,5 @@ > + * An array with the preferences you want to retrieve. > + * @param function aOnResponse > + * Function to invoke when the response is received. > + */ > + getPreferences: function WCC_getPreferences(aPreferences, aOnResponse) { What is the use of getPreferences? The user always updates preferences through the frontend, right? In which case the frontent will be the ultimate source of authority on the preferences, eliminating the need to consult the server. ::: toolkit/devtools/debugger/server/dbg-webconsole-actors.js @@ +697,5 @@ > + { > + // TODO: we should use tabNavigated, but that lives on the TabActor and in > + // the Web Console we do not attach to the TabActor. Plus, the tabNavigated > + // packet is only sent at the end of a page load - for the Web Console we > + // need it as early as possible. Follow-up bug material? I think tabNavigated should cater to both the debugger and the console. As a matter of fact the browser tab actor already has a web progress listener, so modifying it to add a "state" property to tabNavigated shouldn't be too hard. I'm OK with it being done in a followup, too. ::: toolkit/devtools/webconsole/WebConsoleUtils.jsm @@ +2330,5 @@ > + let lowerName = aName.toLowerCase(); > + if (lowerName == "set-cookie") { > + setCookieHeader = aValue; > + } > + response.headers.push({ name: aName, value: aValue }); I think headers (like body) should be split into a separate actor. Keeping in the response only the things the console needs to display, will help reduce network overhead, and when the user chooses to inspect them, new requests could be fired.

Thanks Panos for your review. I'm going to update the patch ASAP. For everyone following the bug: Yesterday I wrote a document that explains the Web Console remoting patches: https://gist.github.com/3691691 Feedback is always welcome. Thanks!

Hi Mihai, I have a few small questions and comments. Take them with a grain of salt since I haven't dived in to the code yet. Is there a reason we can't put the webconsole actors/clients in `toolkit/devtools/webconsole` ? That would make more sense to me, personally, rather than having them all mixed in with debugger actors and clients. I like the extensions to the remote debugging protocol, and their structure looks good and all that. I have a few things I'd like to bring up, though. - Should we *always* start a console actor for each tab and send them in the listTabs message? Do we need to always start them so that we can show messages from before the console was open? If not, would it be ok to have another request/response pair where the client asks for a console actor for a given tab? - I feel that the setPreferences message should receive a response that indicates success or failure (even if we ignore the response on the client). AFAIK, we do not have any other requests that we send that never receive even an acknowledgement response. The protocol is based around request/response, and the unsolicited messages (like newScript) already feel a little out of place, I don't see why we should diverge from our request/response paradigm even more. - Are pageError/consoleAPICall/networkActivity/etc messages sent from the server to the client unsolicited? If we are moving away from the request/response paradigm more and more, we might need to rethink how clients and servers interact once again.

(In reply to Nick Fitzgerald [:fitzgen] from comment #38) > Is there a reason we can't put the webconsole actors/clients in > `toolkit/devtools/webconsole` > ? That would make more sense to me, personally, rather than having them all > mixed in with debugger actors and clients. Not sure if this is relevant, but when other apps like Fennec get webconsole support, we'd like the client actor code to live inn /mobile/android, not in /toolkit ot /browser.

(In reply to Mark Finkle (:mfinkle) from comment #39) > (In reply to Nick Fitzgerald [:fitzgen] from comment #38) > > > Is there a reason we can't put the webconsole actors/clients in > > `toolkit/devtools/webconsole` > > ? That would make more sense to me, personally, rather than having them all > > mixed in with debugger actors and clients. > > Not sure if this is relevant, but when other apps like Fennec get webconsole > support, we'd like the client actor code to live inn /mobile/android, not in > /toolkit ot /browser. The way we have structured the browser actors thus far has been to keep base objects in toolkit/ so that all products can use them, and put product-specific overrides to product directories (like mobile/android). That's also the plan for the console, but note that there is still some work to be done (frontend mostly) to expose a "remote console" option.

Hello Nick! Thanks for your comments, much appreciated. (In reply to Nick Fitzgerald [:fitzgen] from comment #38) > Hi Mihai, I have a few small questions and comments. Take them with a grain > of salt since I haven't dived in to the code yet. > > Is there a reason we can't put the webconsole actors/clients in > `toolkit/devtools/webconsole` > ? That would make more sense to me, personally, rather than having them all > mixed in with debugger actors and clients. Sure, I can move the web console actors and client in toolkit/devtools/webconsole. I didn't do this simply because of legacy - I initially started without moving Web Console things into toolkit. Panos, what do you think? Shall I make a WebConsoleClient.jsm in t/d/w? Also, shall I move webconsole-actors.js to t/d/w? I would say yes. > I like the extensions to the remote debugging protocol, and their structure > looks good and all that. I have a few things I'd like to bring up, though. Thanks! I like them, but at this point I'm unhappy with the networkActivity packet. I'll rework it. > - Should we *always* start a console actor for each tab and send them in the > listTabs message? Do we need to always start them so that we can show > messages from before the console was open? If not, would it be ok to have > another request/response pair where the client asks for a console actor for > a given tab? Making an instance of the WebConsoleActor does not start any kind of listeners and this is not needed for cached messages from before the console is open. This means: - we can make a separate request/response packets pair, if others agree. - but I would say we don't need that, simply because the consoleActor instance is cheap - no perf impact. Panos, any thoughts? > - I feel that the setPreferences message should receive a response that > indicates success or failure (even if we ignore the response on the client). > AFAIK, we do not have any other requests that we send that never receive > even an acknowledgement response. The protocol is based around > request/response, and the unsolicited messages (like newScript) already feel > a little out of place, I don't see why we should diverge from our > request/response paradigm even more. Sure, for completeness I can do that. Thanks for the suggestion! > - Are pageError/consoleAPICall/networkActivity/etc messages sent from the > server to the client unsolicited? If we are moving away from the > request/response paradigm more and more, we might need to rethink how > clients and servers interact once again. Yes, these are unsolicited packets. As for rethinking client-server interaction: I like we have both - unsolicited packets and request/response pairing. The request/response paradigm made things nicer than they were with the message manager (in the web console). Anyhow, this is a discussion for another day / outside of this bug. ;)

(In reply to Mihai Sucan [:msucan] from comment #41) > (In reply to Nick Fitzgerald [:fitzgen] from comment #38) > > Is there a reason we can't put the webconsole actors/clients in > > `toolkit/devtools/webconsole` > > ? That would make more sense to me, personally, rather than having them all > > mixed in with debugger actors and clients. > > Sure, I can move the web console actors and client in > toolkit/devtools/webconsole. I didn't do this simply because of legacy - I > initially started without moving Web Console things into toolkit. > > Panos, what do you think? Shall I make a WebConsoleClient.jsm in t/d/w? > Also, shall I move webconsole-actors.js to t/d/w? I would say yes. Sure, now that there are more than 1 file in toolkit/ I think it makes sense. > > - Should we *always* start a console actor for each tab and send them in the > > listTabs message? Do we need to always start them so that we can show > > messages from before the console was open? If not, would it be ok to have > > another request/response pair where the client asks for a console actor for > > a given tab? > > Making an instance of the WebConsoleActor does not start any kind of > listeners and this is not needed for cached messages from before the console > is open. This means: > > - we can make a separate request/response packets pair, if others agree. > - but I would say we don't need that, simply because the consoleActor > instance is cheap - no perf impact. > > Panos, any thoughts? I think the console actor doesn't do anything heavy to warrant a special request. Now that I looked at it again, only starting the web progress listener might qualify as "not completely insignificant". But even if it were heavy, we could simply use the API from bug 753401 which delays the actual actor construction when the first message addressed to it is received. > > - I feel that the setPreferences message should receive a response that > > indicates success or failure (even if we ignore the response on the client). > > AFAIK, we do not have any other requests that we send that never receive > > even an acknowledgement response. The protocol is based around > > request/response, and the unsolicited messages (like newScript) already feel > > a little out of place, I don't see why we should diverge from our > > request/response paradigm even more. > > Sure, for completeness I can do that. Thanks for the suggestion! From my reading of the patch it seemed that all requests return a response: onSetPreference returns an empty object which should have the actorID appended to it by the dispatcher. I agree that all client-initiated requests should have responses. > > - Are pageError/consoleAPICall/networkActivity/etc messages sent from the > > server to the client unsolicited? If we are moving away from the > > request/response paradigm more and more, we might need to rethink how > > clients and servers interact once again. > > Yes, these are unsolicited packets. > > As for rethinking client-server interaction: I like we have both - > unsolicited packets and request/response pairing. The request/response > paradigm made things nicer than they were with the message manager (in the > web console). Anyhow, this is a discussion for another day / outside of this > bug. ;) Agreed. Unsolicited messages are necessary, but perhaps we can improve the way they tie into the system.

(In reply to Mihai Sucan [:msucan] from comment #41) > > - Are pageError/consoleAPICall/networkActivity/etc messages sent from the > > server to the client unsolicited? If we are moving away from the > > request/response paradigm more and more, we might need to rethink how > > clients and servers interact once again. > > Yes, these are unsolicited packets. > > As for rethinking client-server interaction: I like we have both - > unsolicited packets and request/response pairing. The request/response > paradigm made things nicer than they were with the message manager (in the > web console). Anyhow, this is a discussion for another day / outside of this > bug. ;) Yes, of course :)

Updates: - rebased the patch. - move dbg-webconsole-actors.js from toolkit/devtools/debugger/server into toolkit/devtools/webconsole. - split the WebConsoleClient out from dbg-client.jsm into its own jsm in the webconsole toolkit folder. - fixes to filter unsolicited packets from unknown actors. - slimmer pageError packet. Changes are based on comments from Nick and Panos, things we agreed on. Thanks to both!

Updates: - rebased the patch. - made the new packets slimmer, based on agreement with Panos. - filter out unsolicited packets from unknown actors.

Updated to address Panos's review comments. I added a new NetworkEventActor... and many changes. I still need to file some follow up bugs. Also updated the document that explains these patch to reflect the latest changes: https://gist.github.com/3691691 Looking forward to feedback and reviews. Thank you all!

Comment on attachment 661910 [details] [diff] [review] part 4: cleanups (v0.2) (Pressed enter too soon!) This patch only needed a rebase.

Comment on attachment 661909 [details] [diff] [review] part 3: network logging (v0.2) Review of attachment 661909 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/webconsole/NetworkHelper.jsm @@ +174,5 @@ > */ > readPostTextFromPage: function NH_readPostTextFromPage(aDocShell, aCharset) > { > let webNav = aDocShell.QueryInterface(Ci.nsIWebNavigation); > + return this.readPostTextFromPageViaWebNav(webNav, aCharset); WhatALovelyMethodNameYouHaveThere :-)

Comment on attachment 661904 [details] [diff] [review] part 1: page errors (v0.6) Review of attachment 661904 [details] [diff] [review]: ----------------------------------------------------------------- solid. ::: browser/devtools/webconsole/webconsole.js @@ +3587,5 @@ > + */ > + owner: null, > + > + /** > + * Tells if the connection was established. "Is the connection established"? @@ +3613,5 @@ > + } > + }, > + > + /** > + * Initialize a debugger client and connects it to the debugger server. nit: ... connect it to the debugger... ::: toolkit/devtools/debugger/dbg-client.jsm @@ +271,5 @@ > + detachThread(); > + } > + }.bind(this); > + > + for each (let client in this._consoleClients) { could use for .. of here. I find that more readable. ::: browser/devtools/webconsole/WebConsoleUtils.jsm @@ +1038,5 @@ > + * The listener object must have a method: onPageError. This method is > + * invoked with one argument, the nsIScriptError, whenever a relevant > + * page error is received. > + */ > +function PageErrorListener(aWindow, aListener) this feels a *little* funny living in WebConsoleUtils.jsm. It feels a little too important to be stuck in a miscellaneous Utility module. @@ +1090,5 @@ > + return; > + } > + > + let errorWindow = > + WebConsoleUtils.getWindowByOuterId(aScriptError.outerWindowID, this.window); ... but, since it's making use of WCU methods like this one, maybe that's not such a bad place for it to be.

Green try results with all the patches: https://tbpl.mozilla.org/?tree=Try&rev=ca91ed2d98be Thanks to Panos for his fix in bug 792867.

Comment on attachment 661906 [details] [diff] [review] part 2: console API messages and JS eval (v0.4) Review of attachment 661906 [details] [diff] [review]: ----------------------------------------------------------------- not much I can say about this. Looks great. ::: browser/devtools/webconsole/webconsole.js @@ +2532,5 @@ > let errorMessage = aResponse.errorMessage; > + let result = aResponse.result; > + let inspectable = result && typeof result == "object" && result.inspectable; > + let helperResult = aResponse.helperResult; > + let helperRawOutput = !!(helperResult || {}).rawOutput; Would like to see an indication that you're expecting a boolean here. Maybe rename to helperHasRawOutput or similar. ::: toolkit/devtools/webconsole/WebConsoleUtils.jsm @@ +29,5 @@ > +// function() { ... > +const REGEX_MATCH_FUNCTION_NAME = /^\(?function\s+([^(\s]+)\s*\(/; > + > +// Match the function arguments from the result of toString() or toSource(). > +const REGEX_MATCH_FUNCTION_ARGS = /^\(?function\s*[^\s(]*\s*\((.+?)\)/; oof. sure would be nice to have the parse tree so we didn't have to do this ourselves. ;)

::: toolkit/devtools/webconsole/WebConsoleUtils.jsm @@ +29,5 @@ > +// function() { ... > +const REGEX_MATCH_FUNCTION_NAME = /^\(?function\s+([^(\s]+)\s*\(/; > + > +// Match the function arguments from the result of toString() or toSource(). > +const REGEX_MATCH_FUNCTION_ARGS = /^\(?function\s*[^\s(]*\s*\((.+?)\)/; If this is only used to get the function's name, wouldn't |func.name| work (and be faster)?

(In reply to Thaddee Tyl [:espadrine] from comment #53) > ::: toolkit/devtools/webconsole/WebConsoleUtils.jsm > @@ +29,5 @@ > > +// function() { ... > > +const REGEX_MATCH_FUNCTION_NAME = /^\(?function\s+([^(\s]+)\s*\(/; > > + > > +// Match the function arguments from the result of toString() or toSource(). > > +const REGEX_MATCH_FUNCTION_ARGS = /^\(?function\s*[^\s(]*\s*\((.+?)\)/; > > If this is only used to get the function's name, wouldn't |func.name| work > (and be faster)? I think you're right. Looking at getFunctionName we already have the function object and are stringifying it. Good catch!

but... looking again at getFunctionName, we first check for the existence of function.name and if present, return it. Then look for displayName and return that. If those aren't present, the function is stringified and then regexed. So it's a fallback of last resort.

Comment on attachment 661909 [details] [diff] [review] part 3: network logging (v0.2) Review of attachment 661909 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/webconsole/webconsole.js @@ +327,5 @@ > * @param boolean aValue > * The new value you want to set. > */ > set saveRequestAndResponseBodies(aValue) { > + let newValue = !!aValue; are you ever going to call a setter without a value? Can you? ::: toolkit/devtools/webconsole/WebConsoleUtils.jsm @@ +26,5 @@ > + > +XPCOMUtils.defineLazyServiceGetter(this, "gActivityDistributor", > + "@mozilla.org/network/http-activity-distributor;1", > + "nsIHttpActivityDistributor"); > + this file's getting big. Starting to feel like a dumping ground for Everything Web Console. :)

Comment on attachment 661910 [details] [diff] [review] part 4: cleanups (v0.2) Review of attachment 661910 [details] [diff] [review]: ----------------------------------------------------------------- okay. This is cleanuppy. ::: toolkit/devtools/webconsole/WebConsoleUtils.jsm @@ -53,5 @@ > - GENERATOR: 6, > - STRING: 7 > - }; > - > -var gObjectId = 0; ah. it got smaller. :)

let's land this puppy.

Rob, thanks for your reviews! Will update the patches ASAP. (In reply to Rob Campbell [:rc] (:robcee) from comment #50) > Comment on attachment 661904 [details] [diff] [review] > part 1: page errors (v0.6) > > Review of attachment 661904 [details] [diff] [review]: > ----------------------------------------------------------------- > > solid. > > ::: browser/devtools/webconsole/webconsole.js > @@ +3587,5 @@ > > + */ > > + owner: null, > > + > > + /** > > + * Tells if the connection was established. > > "Is the connection established"? Fixed. > @@ +3613,5 @@ > > + } > > + }, > > + > > + /** > > + * Initialize a debugger client and connects it to the debugger server. > > nit: ... connect it to the debugger... Fixed. > ::: toolkit/devtools/debugger/dbg-client.jsm > @@ +271,5 @@ > > + detachThread(); > > + } > > + }.bind(this); > > + > > + for each (let client in this._consoleClients) { > > could use for .. of here. I find that more readable. Fixed. > ::: browser/devtools/webconsole/WebConsoleUtils.jsm > @@ +1038,5 @@ > > + * The listener object must have a method: onPageError. This method is > > + * invoked with one argument, the nsIScriptError, whenever a relevant > > + * page error is received. > > + */ > > +function PageErrorListener(aWindow, aListener) > > this feels a *little* funny living in WebConsoleUtils.jsm. It feels a little > too important to be stuck in a miscellaneous Utility module. I was thinking of a new jsm, like WebConsoleListeners.jsm. > @@ +1090,5 @@ > > + return; > > + } > > + > > + let errorWindow = > > + WebConsoleUtils.getWindowByOuterId(aScriptError.outerWindowID, this.window); > > ... but, since it's making use of WCU methods like this one, maybe that's > not such a bad place for it to be. Yeah. (In reply to Rob Campbell [:rc] (:robcee) from comment #52) > Comment on attachment 661906 [details] [diff] [review] > part 2: console API messages and JS eval (v0.4) > > Review of attachment 661906 [details] [diff] [review]: > ----------------------------------------------------------------- > > not much I can say about this. Looks great. > > ::: browser/devtools/webconsole/webconsole.js > @@ +2532,5 @@ > > let errorMessage = aResponse.errorMessage; > > + let result = aResponse.result; > > + let inspectable = result && typeof result == "object" && result.inspectable; > > + let helperResult = aResponse.helperResult; > > + let helperRawOutput = !!(helperResult || {}).rawOutput; > > Would like to see an indication that you're expecting a boolean here. Maybe > rename to helperHasRawOutput or similar. Fixed. > ::: toolkit/devtools/webconsole/WebConsoleUtils.jsm > @@ +29,5 @@ > > +// function() { ... > > +const REGEX_MATCH_FUNCTION_NAME = /^\(?function\s+([^(\s]+)\s*\(/; > > + > > +// Match the function arguments from the result of toString() or toSource(). > > +const REGEX_MATCH_FUNCTION_ARGS = /^\(?function\s*[^\s(]*\s*\((.+?)\)/; > > oof. sure would be nice to have the parse tree so we didn't have to do this > ourselves. ;) Yes. I'd also expect to make all this code much nicer/simpler with the debugger API. (In reply to Rob Campbell [:rc] (:robcee) from comment #56) > Comment on attachment 661909 [details] [diff] [review] > part 3: network logging (v0.2) > > Review of attachment 661909 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/devtools/webconsole/webconsole.js > @@ +327,5 @@ > > * @param boolean aValue > > * The new value you want to set. > > */ > > set saveRequestAndResponseBodies(aValue) { > > + let newValue = !!aValue; > > are you ever going to call a setter without a value? Can you? No. That is for cases like saveRequestAndResponseBodies = "foo". > ::: toolkit/devtools/webconsole/WebConsoleUtils.jsm > @@ +26,5 @@ > > + > > +XPCOMUtils.defineLazyServiceGetter(this, "gActivityDistributor", > > + "@mozilla.org/network/http-activity-distributor;1", > > + "nsIHttpActivityDistributor"); > > + > > this file's getting big. Starting to feel like a dumping ground for > Everything Web Console. :) Right. Time for WebConsoleListeners.jsm? or other ideas?

(In reply to Mihai Sucan [:msucan] from comment #59) > Rob, thanks for your reviews! > > Will update the patches ASAP. > > > (In reply to Rob Campbell [:rc] (:robcee) from comment #50) > > Comment on attachment 661904 [details] [diff] [review] > > part 1: page errors (v0.6) > > > > Review of attachment 661904 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: toolkit/devtools/debugger/dbg-client.jsm > > @@ +271,5 @@ > > > + detachThread(); > > > + } > > > + }.bind(this); > > > + > > > + for each (let client in this._consoleClients) { > > > > could use for .. of here. I find that more readable. > > Fixed. Scratch that. for-of says _consoleClients cannot be iterated. I actively avoid using it for such random breakage.

Rebase, addressed Rob's review comments, less debugger patch thanks to Panos's work landing. I now use the new server extension API.

Rebase, minor fixes and addressed review comments. (marking the protocol log as obsolete, stuff did change since that log was saved)

Rebase and minor fixes.

Rebased.

New mochitest-plain tests in toolkit/devtools/webconsole. Untested: - LocationChange - FileActivity Found no obvious ways on how I could do these. Both capabilities are tested in the browser tests. Please let me know if any changes are needed. Thank you!

Try push: https://tbpl.mozilla.org/?tree=Try&rev=30b37aac5dbb For dependencies please see: https://raw.github.com/mihaisucan/mozilla-patch-queue/master/patches-webconsole/series (first is bug 793947) (second is bug 792925) (ignore bug-778766)

not tracking firefox 17.

Test fix for macos.

Small fixes based on try runs. Try results for all patches: https://tbpl.mozilla.org/?tree=Try&rev=211d7c18eb67 (green results until now)

Comment on attachment 665036 [details] [diff] [review] part 1: page errors (v0.7) Landed: https://hg.mozilla.org/integration/fx-team/rev/fd6633f5666e

Comment on attachment 665037 [details] [diff] [review] part 2: console API messages and JS eval (v0.5) Landed: https://hg.mozilla.org/integration/fx-team/rev/57755fd703c9

Comment on attachment 665829 [details] [diff] [review] [backedout] part 3: network logging (v0.4) Landed: https://hg.mozilla.org/integration/fx-team/rev/8c127abc3ad8

Comment on attachment 665040 [details] [diff] [review] [backedout] part 4: cleanups (v0.3) Landed: https://hg.mozilla.org/integration/fx-team/rev/9c70da27ec28

Comment on attachment 665831 [details] [diff] [review] [backedout] part 5: tests (v0.2) Landed: https://hg.mozilla.org/integration/fx-team/rev/89ab8685729d

Yay! Thank you Panos for reviews and debugger patches! Thank you Rob for your reviews!

Backed out from fx-team due to android failures and mochitest-2 dom/plugins/test failures. https://hg.mozilla.org/integration/fx-team/rev/3d2e44c2fd29 https://hg.mozilla.org/integration/fx-team/rev/b18888e94143 https://hg.mozilla.org/integration/fx-team/rev/954d1a3b410b https://hg.mozilla.org/integration/fx-team/rev/3a07399169df https://hg.mozilla.org/integration/fx-team/rev/b07d095ad2f4

To avoid the mochitest-2 oranges we switch the tests in part 5 from mochitest-plain to mochitest-chrome. However, tests only pass if we have a global console without filtering of messages per tab. This patch adds the global consoleActor. When you attach to the global consoleActor your client receives all messages from the browser, with no tab filtering. Behavior stays the same for the tab consoleActors: you only get messages for the specific tab.

Switched from mochitest-plain to mochitest-chrome. https://tbpl.mozilla.org/?tree=Try&rev=01699bdaed9d

Comment on attachment 668424 [details] [diff] [review] part 4.5: global console actor Review of attachment 668424 [details] [diff] [review]: ----------------------------------------------------------------- Remote *and* global console? This bug is the best! ::: dom/base/ConsoleAPIStorage.jsm @@ +81,4 @@ > */ > getEvents: function CS_getEvents(aId) > { > + if (aId != null) { Nit: !aId would be shorter, unless inner ID can be zero? ::: toolkit/devtools/webconsole/WebConsoleUtils.jsm @@ +531,5 @@ > + result.value = this.createValueGrip(aObject[aProperty], aObjectWrapper); > + } > + catch (ex) { > + // This can throw when the security restriction prevent us from reading > + // the value. Typo: "when security restrictions prevent us" @@ +2238,5 @@ > + let win = null; > + try { > + win = NetworkHelper.getWindowForRequest(aChannel); > + } > + catch (ex) { } Add a comment to explain the reason for eating the exception? ::: toolkit/devtools/webconsole/dbg-webconsole-actors.js @@ +136,5 @@ > /** > * The content window we work with. > * @type nsIDOMWindow > */ > + get window() this._browser ? this._browser.contentWindow : this._window, <3 @@ +332,5 @@ > startedListeners.push(listener); > break; > case "FileActivity": > + if (this._isGlobalActor) { > + // The ConsoleProgressListener cannot listen for global events. Why? Is this something we can fix in a followup?

(In reply to Panos Astithas [:past] from comment #79) > Comment on attachment 668424 [details] [diff] [review] > part 4.5: global console actor > > Review of attachment 668424 [details] [diff] [review]: > ----------------------------------------------------------------- > > Remote *and* global console? This bug is the best! Heh. /me no likes. I didn't want to have to do this here... > ::: dom/base/ConsoleAPIStorage.jsm > @@ +81,4 @@ > > */ > > getEvents: function CS_getEvents(aId) > > { > > + if (aId != null) { > > Nit: !aId would be shorter, unless inner ID can be zero? I just used what clearEvents() does. I didn't check if inner ID can be 0. > ::: toolkit/devtools/webconsole/WebConsoleUtils.jsm > @@ +531,5 @@ > > + result.value = this.createValueGrip(aObject[aProperty], aObjectWrapper); > > + } > > + catch (ex) { > > + // This can throw when the security restriction prevent us from reading > > + // the value. > > Typo: "when security restrictions prevent us" Good catch. > @@ +2238,5 @@ > > + let win = null; > > + try { > > + win = NetworkHelper.getWindowForRequest(aChannel); > > + } > > + catch (ex) { } > > Add a comment to explain the reason for eating the exception? Good point. This function throws on b2g. > ::: toolkit/devtools/webconsole/dbg-webconsole-actors.js > @@ +136,5 @@ > > /** > > * The content window we work with. > > * @type nsIDOMWindow > > */ > > + get window() this._browser ? this._browser.contentWindow : this._window, > > <3 > > @@ +332,5 @@ > > startedListeners.push(listener); > > break; > > case "FileActivity": > > + if (this._isGlobalActor) { > > + // The ConsoleProgressListener cannot listen for global events. > > Why? Is this something we can fix in a followup? This only works with additional changes that we do not need to do here. It would also need more testing on b2g. I will file a follow up bug. Thank you for the quick review!

Minor fixes: - forgot to put a bug number in a TODO. - attempt to fix an intermittent orange of the netpanel test on winxp.

Rebased the patch.

Addressed review comments.

Comment on attachment 665036 [details] [diff] [review] part 1: page errors (v0.7) https://hg.mozilla.org/integration/fx-team/rev/df7160707659

Comment on attachment 665037 [details] [diff] [review] part 2: console API messages and JS eval (v0.5) https://hg.mozilla.org/integration/fx-team/rev/fb9c303d0683

Comment on attachment 668754 [details] [diff] [review] part 3: network logging (v0.5) https://hg.mozilla.org/integration/fx-team/rev/acdaa722170d

Comment on attachment 668755 [details] [diff] [review] part 4: cleanups (v0.4) https://hg.mozilla.org/integration/fx-team/rev/fbe10b0e356b

Comment on attachment 668756 [details] [diff] [review] part 4.5: global console actor (v0.2) https://hg.mozilla.org/integration/fx-team/rev/211f9f0d9729

Comment on attachment 668426 [details] [diff] [review] part 5: tests (v0.3) https://hg.mozilla.org/integration/fx-team/rev/246011521d45

https://hg.mozilla.org/mozilla-central/rev/df7160707659 https://hg.mozilla.org/mozilla-central/rev/fb9c303d0683 https://hg.mozilla.org/mozilla-central/rev/acdaa722170d https://hg.mozilla.org/mozilla-central/rev/fbe10b0e356b https://hg.mozilla.org/mozilla-central/rev/211f9f0d9729 https://hg.mozilla.org/mozilla-central/rev/246011521d45

Comment on attachment 668426 [details] [diff] [review] part 5: tests (v0.3) >+ let container = top.document.createElementNS(XHTML_NS, "script"); >+ body.appendChild(container); >+ container.textContent = "document.documentElement.style.color = 'fooColor';"; >+ body.removeChild(container); >+ >+ container = top.document.createElementNS(XHTML_NS, "script"); >+ body.appendChild(container); >+ container.textContent = "document.doTheImpossible();"; >+ body.removeChild(container); This is unsafe. top only exists when running under the harness. Also, you don't expect the exception. I've no idea how you expect(!) that to work...

(In reply to neil@parkwaycc.co.uk from comment #91) > Comment on attachment 668426 [details] [diff] [review] > part 5: tests (v0.3) > > >+ let container = top.document.createElementNS(XHTML_NS, "script"); > >+ body.appendChild(container); > >+ container.textContent = "document.documentElement.style.color = 'fooColor';"; > >+ body.removeChild(container); > >+ > >+ container = top.document.createElementNS(XHTML_NS, "script"); > >+ body.appendChild(container); > >+ container.textContent = "document.doTheImpossible();"; > >+ body.removeChild(container); > This is unsafe. top only exists when running under the harness. Also, you > don't expect the exception. I've no idea how you expect(!) that to work... I expected, at the time of writing, that the code is unsafe. The reason I did it like that: the web console tab actor listens for cached error messages from the top level window only. Since the test harness runs the tests in iframes, I had to get out to the top window. When I added the global console actor I forgot to change the test to no longer use the unsafe approach - my bad. Thanks for spotting it!