Closed
Bug 723391
Opened 12 years ago
Closed 12 years ago
Add a simple remote JS console to b2g
Categories
(Firefox OS Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
DeveloperPhone
People
(Reporter: kanru, Assigned: kanru)
References
Details
Attachments
(1 file, 2 obsolete files)
4.04 KB,
patch
|
vingtetun
:
review+
|
Details | Diff | Splinter Review |
It would be handy to have a REPL for experimenting APIs interactively.
Did we have a bug on file for this? I don't remember. I remember both you guys had started hacking on something like this :).
Assignee | ||
Comment 2•12 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #1) > Did we have a bug on file for this? I don't remember. > > I remember both you guys had started hacking on something like this :). This one is a short-turn hack :)
Assignee | ||
Comment 3•12 years ago
|
||
Attachment #593727 -
Flags: review?(jones.chris.g)
Comment 4•12 years ago
|
||
Mark Finkle has a couple of add-ons to add remote debugging to Fennec : look for firefly-probe and firefly-shell here: http://people.mozilla.com/~mfinkle/fennec/addons/ More complex, but also uses a sandbox instead of just eval() which is a bit scary.
Assignee | ||
Comment 5•12 years ago
|
||
(In reply to Fabrice Desré [:fabrice] from comment #4) > Mark Finkle has a couple of add-ons to add remote debugging to Fennec : look > for firefly-probe and firefly-shell here: > http://people.mozilla.com/~mfinkle/fennec/addons/ > > More complex, but also uses a sandbox instead of just eval() which is a bit > scary. Not that scary since it's meant to touch chrome code :)
Comment on attachment 593727 [details] [diff] [review] Add a simple remote JS console to b2g You guys should figure out how to combine efforts. This patch looks mostly ok on first skim.
Attachment #593727 -
Flags: review?(jones.chris.g) → review?(21)
Comment 7•12 years ago
|
||
Comment on attachment 593727 [details] [diff] [review] Add a simple remote JS console to b2g Review of attachment 593727 [details] [diff] [review]: ----------------------------------------------------------------- Neat. I commented on some Gecko coding style nits. ::: b2g/chrome/content/shell.js @@ +347,5 @@ > }); > } > }; > > +(function Repl() { Why the anonymous function? There's no harm in putting an object or function in the global scope. @@ +360,5 @@ > + .createInstance(Ci.nsIScriptableInputStream); > + sin.init(input); > + try { > + let val = eval(sin.read(sin.available())); > + let ret = val ? val.toString()+'\n' : "undefined\n"; Coding style: spaces between all operators. An easier way to convert `val` into a string would be to simply do `"" + val`. @@ +364,5 @@ > + let ret = val ? val.toString()+'\n' : "undefined\n"; > + output.write(ret, ret.length); > + } catch (e) { > + output.write(e, e.length); > + } This convolutes several possible exceptions. sin.available() can throw if the socket has been closed, same for sin.read(). It's doubtful if it still makes sense to write anything to output in that case. @@ +366,5 @@ > + } catch (e) { > + output.write(e, e.length); > + } > + output.write(prompt, prompt.length); > + let tm = Cc["@mozilla.org/thread-manager;1"].getService(); Please use Services.tm. @@ +372,5 @@ > + } > + } > + let listener = { > + onSocketAccepted: function(serverSocket, clientSocket) { > + dump("Accepted connection on "+clientSocket.host + '\n'); Coding style: spaces between all operators. @@ +374,5 @@ > + let listener = { > + onSocketAccepted: function(serverSocket, clientSocket) { > + dump("Accepted connection on "+clientSocket.host + '\n'); > + let input = clientSocket.openInputStream(0, 0, 0) > + .QueryInterface(Ci.nsIAsyncInputStream); Please align the `.` operators. @@ +377,5 @@ > + let input = clientSocket.openInputStream(0, 0, 0) > + .QueryInterface(Ci.nsIAsyncInputStream); > + output = clientSocket.openOutputStream(Ci.nsITransport.OPEN_BLOCKING, 0, 0); > + output.write(prompt, prompt.length); > + let tm = Cc["@mozilla.org/thread-manager;1"].getService(); Services.tm @@ +382,5 @@ > + input.asyncWait(reader, 0, 0, tm.mainThread); > + } > + } > + let serverPort = Services.prefs.getIntPref("b2g.remote-js.port"); > + if (serverPort) { This will pretty much always be `true`. Otherwise we won't have gotten here. `Services.prefs.getIntPref()` throws if the pref isn't present.
Comment 8•12 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #1) > Did we have a bug on file for this? I don't remember. > > I remember both you guys had started hacking on something like this :). My plan for the console is to have a remote web console that have the same set of features than the current web console of Firefox. I have start hacking on https://github.com/vingtetun/remote-web-console and at the end I was having something with a full support of of console.[log/warn/debug/stack/...] but I think it's not enough and my addon is just a hack on top of the HUDService. But the good thing about this addon is the fact that I have been able to see what's missing in the platform for having a real remote web console and so... I'm pushing to see bug 673148 resolved. Once this bug is resolved it will be pretty simple to hack on top of it and to have a remote web console by using: - a frame script on the server side (the desktop firefox) to listen for the console commands and forward them via the network to a client - some code in shell.js that listen on the network and forward the received message to the content process. (In reply to Fabrice Desré [:fabrice] from comment #4) > Mark Finkle has a couple of add-ons to add remote debugging to Fennec : look > for firefly-probe and firefly-shell here: > http://people.mozilla.com/~mfinkle/fennec/addons/ > > More complex, but also uses a sandbox instead of just eval() which is a bit > scary. I have started to do that too - I have looked at the code too - but at the end I feel it was not enough and I would rather prefer sonmething that is shipped with the product. Also Jetpack (and possibly PDF.js in the future) needs a web console that support <browser remote="true"> so if there is a remote web console in the mozilla codebase, Jetpack, B2G *and* Fennec will be a winner here! :)
Comment 9•12 years ago
|
||
(In reply to Kan-Ru Chen [:kanru] from comment #2) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #1) > > Did we have a bug on file for this? I don't remember. > > > > I remember both you guys had started hacking on something like this :). > > This one is a short-turn hack :) I will try to update my extension code to have something with more features.
Depends on: async-webconsole
What's the plan for getting something landed? Simple is fine if it doesn't lead in the wrong direction. Vivien how do you feel about taking this patch for now?
Assignee | ||
Comment 11•12 years ago
|
||
(In reply to Philipp von Weitershausen [:philikon] from comment #7) > Comment on attachment 593727 [details] [diff] [review] > Add a simple remote JS console to b2g > > Review of attachment 593727 [details] [diff] [review]: > ----------------------------------------------------------------- > > Neat. I commented on some Gecko coding style nits. > > ::: b2g/chrome/content/shell.js > @@ +347,5 @@ > > }); > > } > > }; > > > > +(function Repl() { > > Why the anonymous function? There's no harm in putting an object or function > in the global scope. No particular reason, just so that it will run once and only once in the beginning.
Comment 12•12 years ago
|
||
(In reply to Kan-Ru Chen [:kanru] from comment #11) > > Why the anonymous function? There's no harm in putting an object or function > > in the global scope. > > No particular reason, just so that it will run once and only once in the > beginning. Sure, I understand that. I was just trying to point out that it's a pretty uncommon (and also unnecessary) patterns in Gecko code. ;)
Assignee | ||
Comment 13•12 years ago
|
||
Attachment #593727 -
Attachment is obsolete: true
Attachment #593727 -
Flags: review?(21)
Comment 14•12 years ago
|
||
(In reply to Kan-Ru Chen [:kanru] from comment #13) > Created attachment 596618 [details] [diff] [review] > Add a simple remote JS console to b2g. v2 The other solution I as hoping for takes more time than planned. I will r? this patch.
Comment 15•12 years ago
|
||
Comment on attachment 596618 [details] [diff] [review] Add a simple remote JS console to b2g. v2 >--- a/b2g/chrome/content/shell.js >+(function Repl() { >+ if (!Services.prefs.getBoolPref("b2g.remote-js.enabled")) { >+ return; >+ } >+ const prompt = "JS> "; >+ let output; >+ let reader = { >+ onInputStreamReady : function(input) { Can you add a name to your anonymous function, it's helpful when you are using a profiler/debugger. Instead of seeing 'anonymous' you have the name of the function instead. >+ let sin = Cc['@mozilla.org/scriptableinputstream;1'] >+ .createInstance(Ci.nsIScriptableInputStream); >+ sin.init(input); >+ try { >+ let val = eval(sin.read(sin.available())); >+ let ret = val ? val + '\n' : "undefined\n"; You code will return 'undefined' if val == 0 or if val == null >+ output.write(ret, ret.length); >+ // TODO: check if socket has been closed >+ } catch (e) { >+ if (e.result === Cr.NS_BASE_STREAM_CLOSED || >+ (typeof e === "object" && e.result === Cr.NS_BASE_STREAM_CLOSED)) { >+ return; >+ } >+ let message = typeof e === "object" ? e.message + '\n' : e + '\n'; >+ output.write(message, message.length); >+ } >+ output.write(prompt, prompt.length); >+ input.asyncWait(reader, 0, 0, Services.tm.mainThread); >+ } >+ } >+ let listener = { >+ onSocketAccepted: function(serverSocket, clientSocket) { Add a name to your function. >+ dump('Accepted connection on ' + clientSocket.host + '\n'); >+ let input = clientSocket.openInputStream(0, 0, 0) >+ .QueryInterface(Ci.nsIAsyncInputStream); >+ output = clientSocket.openOutputStream(Ci.nsITransport.OPEN_BLOCKING, 0, 0); Use the flag name in clientsocket,openInputStream for the first parameter (similar to what you have done in clientSocket.openOutputStream) >+ output.write(prompt, prompt.length); >+ input.asyncWait(reader, 0, 0, Services.tm.mainThread); >+ } >+ } >+ let serverPort = Services.prefs.getIntPref("b2g.remote-js.port"); >+ let serverSocket = Cc['@mozilla.org/network/server-socket;1'] >+ .createInstance(Ci.nsIServerSocket); >+ serverSocket.init(serverPort, true, 5); Just curious, if you allow connection only on the loopback interface, do you |adb shell| first? Also is there any reason to use 5 for the backlog instead of -1? >+ dump('Opened socket on ' + serverSocket.port + '\n'); >+ serverSocket.asyncListen(listener); >+})(); Also use single quote everywhere instead of a mix of single/double quotes (to make gjslint happy)
Comment 16•12 years ago
|
||
(In reply to Vivien Nicolas (:vingtetun) from comment #15) > >+ dump('Opened socket on ' + serverSocket.port + '\n'); > >+ serverSocket.asyncListen(listener); > >+})(); > > Also use single quote everywhere instead of a mix of single/double quotes > (to make gjslint happy) Not in Gecko code, please. Gecko's coding style prefers double quotes over single quotes (https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#Literals). (You're not going to have much luck linting Gecko's JS code anyway, I suspect. And the fact that gslint doesn't seem to be configurable in that respect and just for this tool gaia violates various Mozilla coding style rules makes me sad.)
Comment 17•12 years ago
|
||
(In reply to Philipp von Weitershausen [:philikon] from comment #16) > (In reply to Vivien Nicolas (:vingtetun) from comment #15) > > >+ dump('Opened socket on ' + serverSocket.port + '\n'); > > >+ serverSocket.asyncListen(listener); > > >+})(); > > > > Also use single quote everywhere instead of a mix of single/double quotes > > (to make gjslint happy) > > Not in Gecko code, please. Gecko's coding style prefers double quotes over > single quotes > (https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#Literals). > (You're not going to have much luck linting Gecko's JS code anyway, I > suspect. And the fact that gslint doesn't seem to be configurable in that > respect and just for this tool gaia violates various Mozilla coding style > rules makes me sad.) The code has first been checked-in with single quotes. To stay consistent let's use those for now. If that's a problem and you want to do some bikeshedding about the coding guidelines, please open an other bug.
Assignee | ||
Comment 18•12 years ago
|
||
(In reply to Vivien Nicolas (:vingtetun) from comment #15) > >+ let sin = Cc['@mozilla.org/scriptableinputstream;1'] > >+ .createInstance(Ci.nsIScriptableInputStream); > >+ sin.init(input); > >+ try { > >+ let val = eval(sin.read(sin.available())); > >+ let ret = val ? val + '\n' : "undefined\n"; > > You code will return 'undefined' if val == 0 or if val == null Doh, I have fixed this in another copy of this patch. I must forgot to sync it back. > >+ output.write(prompt, prompt.length); > >+ input.asyncWait(reader, 0, 0, Services.tm.mainThread); > >+ } > >+ } > >+ let serverPort = Services.prefs.getIntPref("b2g.remote-js.port"); > >+ let serverSocket = Cc['@mozilla.org/network/server-socket;1'] > >+ .createInstance(Ci.nsIServerSocket); > >+ serverSocket.init(serverPort, true, 5); > > Just curious, if you allow connection only on the loopback interface, do you > |adb shell| first? I |adb forward| first, then I connect to it locally. > Also is there any reason to use 5 for the backlog instead of -1? Hum.. I can't remember the reason.. I think I should use -1 here. > >+ dump('Opened socket on ' + serverSocket.port + '\n'); > >+ serverSocket.asyncListen(listener); > >+})(); > > Also use single quote everywhere instead of a mix of single/double quotes > (to make gjslint happy) Since the rest of the file uses single quote, I will match the style here.
Assignee | ||
Comment 19•12 years ago
|
||
Attachment #596618 -
Attachment is obsolete: true
Attachment #596910 -
Flags: review?(21)
Attachment #596910 -
Flags: review?(21) → review+
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 21•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fd3817a300ef
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•