Closed
Bug 614557
Opened 14 years ago
Closed 13 years ago
NS_ERROR_NOT_IMPLEMENTED when opening venkman
Categories
(Other Applications Graveyard :: Venkman JS Debugger, defect)
Other Applications Graveyard
Venkman JS Debugger
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: Fallen, Assigned: mnyromyr)
References
Details
(Keywords: regression)
Attachments
(1 file, 4 obsolete files)
10.29 KB,
patch
|
mnyromyr
:
review+
|
Details | Diff | Splinter Review |
Since bug 595243 landed, venkman is not usable anymore. While I understand that the focus is on firefox and therefore firebug/chromebug, this does not work for other Mozilla Apps. Please at least fix up venkman so it works again.
Reporter | ||
Comment 1•14 years ago
|
||
More info, sorry: When opening venkman, a dialog shows up that it could not start the debugger service, since jsdIDebuggerSerive::On() now throws NS_ERROR_NOT_IMPLEMENTED. It seems now that instead a debugMode needs to be enabled, see bug 595243. After clicking ok, the debugger window opens, but everything is empty except the toolbar and menu.
you need to use .asyncOn(...) http://mxr.mozilla.org/mozilla-central/ident?i=asyncOn
Reporter | ||
Comment 3•14 years ago
|
||
This fixes the initial startup, but still some files are not shown. I guess thats a different issues though.
Reporter | ||
Updated•14 years ago
|
Attachment #493000 -
Flags: review? → review?(gijskruitbosch+bugs)
this isn't backwards compat. What you should do is something like: if ("asyncOn" in jsd) { jsd.asynOn(this); } else { jsd.on(); this.onDebuggerActivated(); } and stick the rest of your code into this.onDebuggerActivated.
Reporter | ||
Comment 5•14 years ago
|
||
You're right, I guess I should keep it backwards compatible. This patch takes care and also aligns the style to whats in the files already. Note there is no explicit this object there, but I put the code into an extra function.
Attachment #493000 -
Attachment is obsolete: true
Attachment #493111 -
Flags: review?(gijskruitbosch+bugs)
Attachment #493000 -
Flags: review?(gijskruitbosch+bugs)
+++ b/resources/content/tests/eval.xul Wed Nov 24 22:47:02 2010 +0100 arguably this should also support the old way :)
Updated•14 years ago
|
Comment 10•13 years ago
|
||
(In reply to comment #7) > +++ b/resources/content/tests/eval.xul Wed Nov 24 22:47:02 2010 +0100 > > arguably this should also support the old way :) Philipp, may I suggest fixing the test and requesting review from timeless? Gijs seems to be busy.
Reporter | ||
Comment 11•13 years ago
|
||
Ah yes, sorry. Totally forgot about this bug. Thanks for the note!
Attachment #493111 -
Attachment is obsolete: true
Attachment #506340 -
Flags: review?
Attachment #493111 -
Flags: review?(gijskruitbosch+bugs)
Reporter | ||
Updated•13 years ago
|
Attachment #506340 -
Flags: review? → review?(timeless)
Comment 12•13 years ago
|
||
Comment on attachment 506340 [details] [diff] [review] Fix - v3 (In reply to comment #11) > Created attachment 506340 [details] [diff] [review] > Fix - v3 > > Ah yes, sorry. Totally forgot about this bug. Thanks for the note! Two points. First, AFAICT there are two instances where you changed to an async pattern (not counting the test) and one has actual code in the callback listener (the dump statement is less important...). What if the first bit of code turns on JSD (on startup), will the callback still fire once you call asyncOn a second time (from venkman-debugger.js) ? The code in that callback does need to be executed. Please test/verify this, and if not, make sure the code runs if jsds.isOn (it may be better to just run the code directly if jsds.isOn(), and only run .on()/.asyncOn() otherwise). Second, initDebugger is expected to be synchronous. If you check the caller, there is some (but not all!) code there (in venkman-static.js) that expects the debugger is initialized (from the looks of it, at least the (delayed) initPost, the venkman-started hook, etc.). It would be best to use a callback structure (pass a function to initDebugger, have onDebuggerActivated call it) to make sure this code doesn't fire until the debugger is actually active. With the callback and ensuring the hooks get added even if JSDS is turned on at startup, r=me.
Attachment #506340 -
Flags: review?(timeless) → review+
Assignee | ||
Comment 13•13 years ago
|
||
(Stealing this with Fallen's permission.) (In reply to comment #12) > What if the first bit of code turns on JSD (on startup), will the callback > still fire once you call asyncOn a second time (from venkman-debugger.js)? No, it won't. Fixed. > Second, initDebugger is expected to be synchronous. ... > It would be best to use a callback structure Fixed. Venkman still dumps some JS errors on startup, but those don't seem to be related with this bug.
Assignee: philipp → mnyromyr
Attachment #506340 -
Attachment is obsolete: true
Attachment #512915 -
Flags: review?(gijskruitbosch+bugs)
Comment 14•13 years ago
|
||
Comment on attachment 512915 [details] [diff] [review] v4: sync asyncOn Drive-by review: >+ // (initAtStartup was removed by changeset 46998/bug 568691/2010-06-10) Changeset numbers are local and unportable. Could try linking to a rev on hg.mozilla.org though. >+ jsds.asyncOn({ onDebuggerActivated: onDebuggerActivated }); jsdIActivationCallback has recently been made a [function], so you can just pass onDebuggerActivated directly. >+ jsds.asyncOn({ onDebuggerActivated: function() {} }); You can also pass null for no callback. (Maybe it should be optional?) >- console.executionHook = { onExecute: jsdExecutionHook }; >+ console.executionHook = { onExecute: jsdExecutionHook }; [Is a -w diff possible at all?]
Comment 15•13 years ago
|
||
Comment on attachment 512915 [details] [diff] [review] v4: sync asyncOn With the nits noted by Neil, r=me.
Attachment #512915 -
Flags: review?(gijskruitbosch+bugs) → review+
Assignee | ||
Comment 16•13 years ago
|
||
This got pushed as <http://hg.mozilla.org/venkman/rev/65c7be10defd>, with Neil's nits fixed (I dropped the changeset part completely).
Attachment #512915 -
Attachment is obsolete: true
Attachment #513265 -
Flags: review+
Assignee | ||
Updated•13 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 17•13 years ago
|
||
Karsten, thanks for taking care!
Comment 18•13 years ago
|
||
Checked out latest source, built XPI, and tested in Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110217 Firefox/4.0b12pre ID:20110217123839. Works like a charm. Thanks Karsten for fixing that! Marking as verified fixed.
Status: RESOLVED → VERIFIED
Comment 19•13 years ago
|
||
could someone please update and release the extension on AMO..
Assignee | ||
Comment 20•13 years ago
|
||
Note that this patch does only work with Firefox trunk builds, but *not* with FF4 beta builds up to beta 11! (The function parameter change was only recently 2011-02-14 in bug 633833, well after beta 11 was released.)
Assignee | ||
Comment 21•13 years ago
|
||
s/this patch/Venkman with this patch/
Comment 24•13 years ago
|
||
So, the dupe count increases. Someone with appropriate rights should update the maxVersion on AMO as a minimum, or better still release a fixed version with maxVersion bumps to 4.0.* (Firefox) and 2.1.* (SeaMonkey).
Comment 25•13 years ago
|
||
I've set the max version for JavaScript Debugger 0.9.88.1 to Firefox 4.0b6 (i.e. beta 6) which I think is the last one that worked; if not, just say and I can adjust it, but it at least isn't claiming 4.0 final compatibility now.
Updated•6 years ago
|
Product: Other Applications → Other Applications Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•