Summary: | Caught exceptions still pause the debugger | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kevin Lindeman <klindeman> | ||||||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Brian Weinstein <bweinstein> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Enhancement | CC: | eric, oliver, pfeldman, timothy, yurys | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | All | ||||||||||||||||
OS: | All | ||||||||||||||||
Attachments: |
|
Description
Kevin Lindeman
2009-08-21 13:13:21 PDT
Created attachment 38390 [details]
proposed patch
This patch gets us the proposed behavior. It waits to see until function exit if an exception still remains - one won't remain if we caught it.
I am not sure what the exception() callback should do if we don't pause anymore.
Comment on attachment 38390 [details]
proposed patch
No ChagneLog. No test.
This is a good idea. Would be great to land this with a ChangeLog and a manual test in WebCore/manual-tests/inspector. Alright, I will work on that. Created attachment 46965 [details]
[PATCH] Fix
Don't break on exceptions that are caught in the same function.
I disagree, in all debuggers for other languages, pause on exceptions means pause on all exceptions, regardless as to whether there is a handler. Pause on exceptions is used specifically for the case where you want to pause on an exception they may actually be caught. Rather than looking at things like dashcode, people should be looking at real debuggers such as VS, Eclipse, etc, where (if memory serves) th debugger stops on uncaught exceptions by default, and pause on exceptions actually means pause on all exceptions whether or not there is a handler. I can se the use of both options. I think we should have a tri-state button. States being: don't pause on any exceptions, pause on uncaught exceptions, and pause on all exceptions. Comment on attachment 46965 [details]
[PATCH] Fix
This patch as-is will likely change behaviour in Dashcode or other clients. They might rely on the exeption function always being called.
Since those clients are protected from the C++ layer, I propose we pass another bool to the function: hasHandler.
Then the Debugger can decided if it cares about the exception, and it lets up have a setting. It will also prevent breaking other clients. (You will need to update WebScriptDebugger.mm in WebKit to prevent breaking the build when you add the new argument.)
Created attachment 46967 [details]
[PATCH] Add a hasHandler parameter to JavaScriptDebugServer::exception
This patch has no change in behavior, but enables us to differentiate between Break on All Exceptions and Break on Uncaught Exceptions.
Comment on attachment 46967 [details]
[PATCH] Add a hasHandler parameter to JavaScriptDebugServer::exception
I think you need to "UNUSED_PARAM(hasHandler);" in WebScriptDebugger too. The rest of the code looks correct, though.
FWIW, I agree with Oliver about this feature: DashCode's behavior is odd. Maybe we could add a tri-state button. But have any real users requested one?
Comment on attachment 46967 [details]
[PATCH] Add a hasHandler parameter to JavaScriptDebugServer::exception
You don't need UNUSED_PARAM in WebKit.
Either this or bug 33820 seems to have broken a webgl test. At least thats' what the bots say: http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r53517%20(9580)/fast/canvas/webgl/texImage2DImageDataTest-pretty-diff.html The first failure started between r53514 and r53517 inclusive. I highly doubt it was this change, which has nothing to do with layout or rendering. (In reply to comment #13) > Either this or bug 33820 seems to have broken a webgl test. At least thats' > what the bots say: > http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r53517%20(9580)/fast/canvas/webgl/texImage2DImageDataTest-pretty-diff.html > > The first failure started between r53514 and r53517 inclusive. This added a parameter that is unused. It wouldn't have broken any tests I don't think. Comment on attachment 46967 [details] [PATCH] Add a hasHandler parameter to JavaScriptDebugServer::exception Landed in r53516. Created attachment 47064 [details]
[PATCH] Tri-State Pause on Exceptions Button
Comment on attachment 47064 [details] [PATCH] Tri-State Pause on Exceptions Button > - bool pauseOnExceptions(); > - void setPauseOnExceptions(bool pause); > + int pauseOnExceptions(); > + void setPauseOnExceptions(int pause); I think we should add "State" to these function names. setPauseOnExceptionsState. Why an int here and not long? When the idl uses long. > - , m_pauseOnExceptions(false) > + , m_pauseOnExceptions(DontPauseOnExceptions) Add "State" to m_pauseOnExceptions: m_pauseOnExceptionsState. > + enum PauseOnExceptionsCondition { > + DontPauseOnExceptions, > + PauseOnAllExceptions, > + PauseOnUncaughtExceptions > + }; I think State is better than Condition. > + PauseOnExceptionsCondition pauseOnExceptions() const { return m_pauseOnExceptions; } > + void setPauseOnExceptions(PauseOnExceptionsCondition); Add State to the function names. > +WebInspector.ScriptsPanel.PauseOnExceptionsCondition = { State. > + this.pauseOnExceptionButton.title = WebInspector.UIString("Don't pause on exceptions / Click to Pause on all exceptions."); > + else if (InspectorBackend.pauseOnExceptions() == WebInspector.ScriptsPanel.PauseOnExceptionsCondition.PauseOnAllExceptions) > + this.pauseOnExceptionButton.title = WebInspector.UIString("Pause on all exceptions / Click to Pause on uncaught exceptions."); > + else if (InspectorBackend.pauseOnExceptions() == WebInspector.ScriptsPanel.PauseOnExceptionsCondition.PauseOnUncaughtExceptions) > + this.pauseOnExceptionButton.title = WebInspector.UIString("Pause on uncaught exceptions / Click to Not pause on exceptions."); Replace the "/" with a \n. I meant to tell you that on IRC. > + this.numStates = numStates; > + if (!numStates) > + this.numStates = 2; I don't think "num" helps. Just call it "states". > + if (numStates == 2) > + this._toggled = false; > + else > + this._toggled = 0; I think _state would be a better name for _toggled now. Overall looks good. Attach another patch if you want me to look over any tweaks. Created attachment 47083 [details]
[PATCH] Tri-State Button + Tim's Comments
This renames the functions to pauseOnExceptionsState, and takes care of other feedback from Tim. Also waiting on feedback from Pavel before landing.
Comment on attachment 47083 [details] [PATCH] Tri-State Button + Tim's Comments > get toggled() > set toggled(x) You should renamed these too. But that will touch more files. Maybe add a get/set state, and make toggled use that only for states == 2? And throw otherwise. Created attachment 47085 [details]
[PATCH] Cleaned up toggled/state Getter/Setter code
Should be a final pass, waiting for Pavel to take a look before landing. Sorry about the numerous iterations.
Comment on attachment 47085 [details] [PATCH] Cleaned up toggled/state Getter/Setter code > + throw("Only used toggled when there are 2 states, otherwise, use state"); > + throw("Only used toggled when there are 2 states, otherwise, use state"); Typo: "used". (In reply to comment #22) > (From update of attachment 47085 [details]) > > + throw("Only used toggled when there are 2 states, otherwise, use state"); > > + throw("Only used toggled when there are 2 states, otherwise, use state"); > > Typo: "used". Fixed. Thanks! (In reply to comment #21) > Created an attachment (id=47085) [details] > [PATCH] Cleaned up toggled/state Getter/Setter code > > Should be a final pass, waiting for Pavel to take a look before landing. Sorry > about the numerous iterations. We are now fixing Chromium so that it works with this patch. Will land this one shortly! (In reply to comment #24) > (In reply to comment #21) > > Created an attachment (id=47085) [details] [details] > > [PATCH] Cleaned up toggled/state Getter/Setter code > > > > Should be a final pass, waiting for Pavel to take a look before landing. Sorry > > about the numerous iterations. > > We are now fixing Chromium so that it works with this patch. Will land this one > shortly! Let me know when you land it, I can land my part when it's ready. Thanks for following up on this. Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/inspector/InspectorBackend.cpp M WebCore/inspector/InspectorBackend.h M WebCore/inspector/InspectorBackend.idl M WebCore/inspector/JavaScriptDebugServer.cpp M WebCore/inspector/JavaScriptDebugServer.h M WebCore/inspector/front-end/InspectorBackendStub.js M WebCore/inspector/front-end/ScriptsPanel.js M WebCore/inspector/front-end/StatusBarButton.js M WebCore/inspector/front-end/inspector.css Committed r53696 |