Last Comment Bug 679031 - Expose jsdbg2 Debugger object as an XPCOM component
: Expose jsdbg2 Debugger object as an XPCOM component
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: Jason Orendorff [:jorendorff]
:
Mentors:
: 677389 (view as bug list)
Depends on:
Blocks: 679165 679189 683894
  Show dependency treegraph
 
Reported: 2011-08-15 10:55 PDT by Jason Orendorff [:jorendorff]
Modified: 2011-12-02 11:07 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP 1 (20.57 KB, patch)
2011-08-15 11:21 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Review
WIP 2 (25.26 KB, patch)
2011-08-15 16:38 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Review
v3 (27.35 KB, patch)
2011-08-17 20:32 PDT, Jason Orendorff [:jorendorff]
benjamin: superreview+
Details | Diff | Review
v4 - same as v3 but rename js/components to js/ductwork (23.65 KB, patch)
2011-08-19 04:27 PDT, Jason Orendorff [:jorendorff]
ted: review+
jorendorff: superreview+
Details | Diff | Review

Description Jason Orendorff [:jorendorff] 2011-08-15 10:55:07 PDT
Dave Camp wrote code to do this. It works like so:

  var xpcDebugger = Cc["@mozilla.org/jsdebugger;1"].getService(Ci.nsIJSDebugger);
  xpcDebugger.addClass();

The addClass() method modifies the current global, adding a Debugger constructor. Then you can do stuff like:

  var dbg = new Debugger(anyContentWindow);
  dbg.onDebuggerStatement = function (frame) {
      alert("this is your debugger speaking");
  };
  anyContentWindow.setTimeout("debugger;", 0);

We have the code, with some tests even. We can expose this stuff to extension developers for FF8.
Comment 1 Jason Orendorff [:jorendorff] 2011-08-15 11:21:47 PDT
Created attachment 553224 [details] [diff] [review]
WIP 1

This is what Dave Camp hacked together. I got several useful comments on IRC about the approach, so I'm going to rearrange it completely and post a new patch. :)
Comment 2 Panos Astithas [:past] 2011-08-15 14:14:25 PDT
Note that I've updated these tests to work with the new Debugger API (onDebuggerStatement, etc.) in bug 676605. The fixes are still a WIP, but you can find them in my patch queue:

https://hg.mozilla.org/users/pastithas_mozilla.com/remote-debug-patches
Comment 3 Jason Orendorff [:jorendorff] 2011-08-15 16:36:46 PDT
*** Bug 677389 has been marked as a duplicate of this bug. ***
Comment 4 Jason Orendorff [:jorendorff] 2011-08-15 16:38:22 PDT
Created attachment 553305 [details] [diff] [review]
WIP 2

This one passes tests. The new .jsm was one of the IRC recommendations I got.

The other is to move the whole thing under the js/ directory. I think maybe js/components/{ctypes,debugger,reflect}, though I'll do the other two separately.
Comment 5 Jason Orendorff [:jorendorff] 2011-08-17 20:32:34 PDT
Created attachment 553992 [details] [diff] [review]
v3

I don't know who likes to give reviews and super-reviews on new components like this. Feel free to dodge, as long as there's someone standing behind you.
Comment 6 Benjamin Smedberg [:bsmedberg] 2011-08-18 08:47:56 PDT
Comment on attachment 553992 [details] [diff] [review]
v3

Please use straight js/debugger. components/ is an unfortunate artifact of directory history we should avoid.
Comment 7 Jason Orendorff [:jorendorff] 2011-08-18 12:38:21 PDT
I don't want to use js/debugger because that directory name looks like there might be something interesting in there, which would be totally untrue--it's just unloved glue code.

So following tense negotiations on IRC, bsmedberg and I have settled on
js/ductwork/debugger. Seriously.

Later I'll move toolkit/ctypes and toolkit/reflect under js/ductwork, and we can then happily forget about them until the revolution comes.
Comment 8 Jason Orendorff [:jorendorff] 2011-08-19 04:27:09 PDT
Created attachment 554366 [details] [diff] [review]
v4 - same as v3 but rename js/components to js/ductwork

Carrying forward bsmedberg's sr+.
Comment 9 Jason Orendorff [:jorendorff] 2011-08-19 04:50:02 PDT
Actually it's not quite the same. v4 removes enter/exitNestedEventLoop and eventLoopNestLevel, at dcamp's encouragement.
Comment 10 Ted Mielczarek [:ted.mielczarek] 2011-08-25 11:39:13 PDT
Comment on attachment 554366 [details] [diff] [review]
v4 - same as v3 but rename js/components to js/ductwork

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

::: js/ductwork/Makefile.in
@@ +19,5 @@
> +# the Initial Developer. All Rights Reserved.
> +#
> +# Contributor(s):
> +#  Brian Ryner <bryner@brianryner.com>
> +#  Benjamin Smedberg <benjamin@smedbergs.us>

I'm sure you copy-pasted these from somewhere, but you can probably remove the contributors from these new files you're adding, since they're pretty much all yours. Also, change copyright dates to 2011 while you're copy-pasting?

::: js/ductwork/debugger/Makefile.in
@@ +55,5 @@
> +IS_COMPONENT = 1
> +
> +CPPSRCS = \
> +    nsJSDebugger.cpp \
> +    $(NULL)

Two-space indent after line continuations, please.

@@ +64,5 @@
> +    $(NULL)
> +
> +EXTRA_JS_MODULES = \
> +	jsdebugger.jsm \
> +	$(NULL)

Two-space, as above, and please don't use actual tabs in Makefiles except before rule commands where they're required.

@@ +72,5 @@
> +	$(NULL)
> +
> +ifdef ENABLE_TESTS
> +    DIRS += tests
> +endif

It's not really typical to indent inside conditionals in Makefiles, but I don't have strong feelings against it if you like it.

::: toolkit/components/ctypes/ctypes.jsm
@@ +52,5 @@
>  
> +// Initialize the Debugger object. You do not need to do this yourself.
> +const init = Components.classes["@mozilla.org/jsdebugger;1"].createInstance(Components.interfaces.nsIJSDebugger);
> +init.addClass();
> +Debugger.util = init;

Is there a reason this has a two-step init? I assume addClass adds Debugger, but why can't it setup its own .util?

Not super-worried about this, this is all awful ductwork that we really ought to rip out and replace with something better.

::: js/ductwork/debugger/nsJSDebugger.cpp
@@ +1,1 @@
> +/* -*-  Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2; -*- */

Dare to dream and leave off the ns prefix?

::: js/ductwork/debugger/tests/Makefile.in
@@ +44,5 @@
> +include $(DEPTH)/config/autoconf.mk
> +
> +MODULE          = test_debugger
> +
> +XPCSHELL_TESTS = unit

You could feasibly move this up a level and just have XPCSHELL_TESTS = tests in js/ductwork/debugger/Makefile.in, which would save us a make invocation.
Comment 11 Jason Orendorff [:jorendorff] 2011-08-25 14:10:49 PDT
OK, I made all those changes, except that

(In reply to Ted Mielczarek [:ted, :luser] from comment #10)
> Is there a reason this has a two-step init? I assume addClass adds Debugger,
> but why can't it setup its own .util?

I ripped this out in v4.

I'll land this tonight.
Comment 12 Jason Orendorff [:jorendorff] 2011-08-25 15:29:50 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/28dd46b9ee31
Comment 13 Jason Orendorff [:jorendorff] 2011-08-25 20:56:44 PDT
I backed it out. The test fails! Which is pretty annoying--it doesn't fail here. I'll look at it tomorrow morning.
Comment 14 Jason Orendorff [:jorendorff] 2011-08-31 08:52:39 PDT
Ted figured it out for me. It was failing only after 'make package', due to a missing manifest entry for an xpt file.

So I added that, confirmed with Try server, and pushed.
http://hg.mozilla.org/integration/mozilla-inbound/rev/a0fb16f50677
Comment 15 Ed Morley [:emorley] 2011-09-01 01:40:50 PDT
http://hg.mozilla.org/mozilla-central/rev/a0fb16f50677
Comment 16 Eric Shepherd [:sheppy] 2011-12-02 11:07:01 PST
Added a doc for the IJSDebugger interface, which strongly recommends not using it.

https://developer.mozilla.org/en/XPCOM_Interface_Reference/IJSDebugger

Note You need to log in before you can comment on or make changes to this bug.