Support 'break on exception' feature

Support 'break on exception' feature

Committed: http://code.google.com/p/chromedevtools/source/detail?r=394

[apavlov, 2010-09-17 10:35:20]

http://codereview.chromium.org/3402011/diff/2001/3003
File
plugins/org.chromium.debug.core/src/org/chromium/debug/core/model/WorkspaceBridge.java
(right):

http://codereview.chromium.org/3402011/diff/2001/3003#newcode108
plugins/org.chromium.debug.core/src/org/chromium/debug/core/model/WorkspaceBridge.java:108:
Boolean getBreakExceptionState(JavascriptVm.ExceptionCatchType catchType);
I suggest that you get rid of this "tri-state variable" anti-pattern and use an
enum instead...

http://codereview.chromium.org/3402011/diff/2001/3006
File
plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/actions/ExceptionBreakpoints.java
(right):

http://codereview.chromium.org/3402011/diff/2001/3006#newcode61
plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/actions/ExceptionBreakpoints.java:61:
@Override boolean getCurrentCheckedState() {
are we now writing annotations on the same line as the method sig?

http://codereview.chromium.org/3402011/diff/2001/3007
File
plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/actions/SynchronizeBreakpoints.java
(right):

http://codereview.chromium.org/3402011/diff/2001/3007#newcode122
plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/actions/SynchronizeBreakpoints.java:122:
public static DebugTargetImpl getDebugTarget(Object element) {
this should go as "getDebugTargetImpl" since it involves an explicit instanceof
DebugTargetImpl check.

http://codereview.chromium.org/3402011/diff/2001/3008
File plugins/org.chromium.sdk/src/org/chromium/sdk/JavascriptVm.java (right):

http://codereview.chromium.org/3402011/diff/2001/3008#newcode123
plugins/org.chromium.sdk/src/org/chromium/sdk/JavascriptVm.java:123: * A generic
callback used in operations that simply set value of some variable on remote.
simply -> /dev/null

suggested:
A generic callback used in operations that set a remote variable value.

http://codereview.chromium.org/3402011/diff/2001/3008#newcode127
plugins/org.chromium.sdk/src/org/chromium/sdk/JavascriptVm.java:127: * Method is
called on the variable being successfully updated.
I don't understand what this sentence means. This method is defined on
SetValueCallback, not a variable type.

http://codereview.chromium.org/3402011/diff/2001/3008#newcode136
plugins/org.chromium.sdk/src/org/chromium/sdk/JavascriptVm.java:136: * @param
enabled new value or null if only current value is in question
what does "only current value is in question" mean?

http://codereview.chromium.org/3402011/diff/2001/3008#newcode146
plugins/org.chromium.sdk/src/org/chromium/sdk/JavascriptVm.java:146: *
Asynchronously enables or disables pausing on exception. All exception
pausing -> breaking
for consistency?

http://codereview.chromium.org/3402011/diff/2001/3008#newcode148
plugins/org.chromium.sdk/src/org/chromium/sdk/JavascriptVm.java:148: * @param
enabled new value or null if only current value is in question
same as above

http://codereview.chromium.org/3402011/diff/2001/3011
File
plugins/org.chromium.sdk/src/org/chromium/sdk/internal/protocol/FlagsBody.java
(right):

http://codereview.chromium.org/3402011/diff/2001/3011#newcode13
plugins/org.chromium.sdk/src/org/chromium/sdk/internal/protocol/FlagsBody.java:13:
public interface FlagsBody extends JsonSubtype<CommandResponseBody> {
Flags support should probably go as a separate change.

http://codereview.chromium.org/3402011/diff/2001/3012
File
plugins/org.chromium.sdk/src/org/chromium/sdk/internal/tools/v8/BreakpointManager.java
(right):

http://codereview.chromium.org/3402011/diff/2001/3012#newcode200
plugins/org.chromium.sdk/src/org/chromium/sdk/internal/tools/v8/BreakpointManager.java:200:
switch (catchType) {
given that you have the "default" branch below, should we null-check the
parameter?

http://codereview.chromium.org/3402011/diff/2001/3012#newcode201
plugins/org.chromium.sdk/src/org/chromium/sdk/internal/tools/v8/BreakpointManager.java:201:
case CAUGHT:
"case"s should be indented

http://codereview.chromium.org/3402011/diff/2001/3012#newcode231
plugins/org.chromium.sdk/src/org/chromium/sdk/internal/tools/v8/BreakpointManager.java:231:
for (int i = 0; i < flagList.size(); i++) {
why doesn't for (FlagInfo info : flagList) work for you? It saves a lot of code
:)

On a related note, we usually write ++i

http://codereview.chromium.org/3402011/diff/2001/3012#newcode234
plugins/org.chromium.sdk/src/org/chromium/sdk/internal/tools/v8/BreakpointManager.java:234:
break findCorrectFlag;
why do you need the label?!

http://codereview.chromium.org/3402011/diff/2001/3012#newcode245
plugins/org.chromium.sdk/src/org/chromium/sdk/internal/tools/v8/BreakpointManager.java:245:
resValue = (Boolean)value;
space after cast

[Peter Rybin, 2010-09-17 20:57:14]

http://codereview.chromium.org/3402011/diff/2001/3003
File
plugins/org.chromium.debug.core/src/org/chromium/debug/core/model/WorkspaceBridge.java
(right):

http://codereview.chromium.org/3402011/diff/2001/3003#newcode108
plugins/org.chromium.debug.core/src/org/chromium/debug/core/model/WorkspaceBridge.java:108:
Boolean getBreakExceptionState(JavascriptVm.ExceptionCatchType catchType);
On 2010/09/17 10:35:20, apavlov wrote:
> I suggest that you get rid of this "tri-state variable" anti-pattern and use
an
> enum instead...
What do you mean by 3-state? catchType or return value?

http://codereview.chromium.org/3402011/diff/2001/3006
File
plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/actions/ExceptionBreakpoints.java
(right):

http://codereview.chromium.org/3402011/diff/2001/3006#newcode61
plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/actions/ExceptionBreakpoints.java:61:
@Override boolean getCurrentCheckedState() {
On 2010/09/17 10:35:20, apavlov wrote:
> are we now writing annotations on the same line as the method sig?
I don't know. Is it a problem?
This 'override' awfully takes another line, which is too much for a short
method.

http://codereview.chromium.org/3402011/diff/2001/3007
File
plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/actions/SynchronizeBreakpoints.java
(right):

http://codereview.chromium.org/3402011/diff/2001/3007#newcode122
plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/actions/SynchronizeBreakpoints.java:122:
public static DebugTargetImpl getDebugTarget(Object element) {
On 2010/09/17 10:35:20, apavlov wrote:
> this should go as "getDebugTargetImpl" since it involves an explicit
instanceof
> DebugTargetImpl check.

Done.

http://codereview.chromium.org/3402011/diff/2001/3008
File plugins/org.chromium.sdk/src/org/chromium/sdk/JavascriptVm.java (right):

http://codereview.chromium.org/3402011/diff/2001/3008#newcode123
plugins/org.chromium.sdk/src/org/chromium/sdk/JavascriptVm.java:123: * A generic
callback used in operations that simply set value of some variable on remote.
On 2010/09/17 10:35:20, apavlov wrote:
> simply -> /dev/null
> 
> suggested:
> A generic callback used in operations that set a remote variable value.

Done.

http://codereview.chromium.org/3402011/diff/2001/3008#newcode127
plugins/org.chromium.sdk/src/org/chromium/sdk/JavascriptVm.java:127: * Method is
called on the variable being successfully updated.
On 2010/09/17 10:35:20, apavlov wrote:
> I don't understand what this sentence means. This method is defined on
> SetValueCallback, not a variable type.

Done.

http://codereview.chromium.org/3402011/diff/2001/3008#newcode136
plugins/org.chromium.sdk/src/org/chromium/sdk/JavascriptVm.java:136: * @param
enabled new value or null if only current value is in question
On 2010/09/17 10:35:20, apavlov wrote:
> what does "only current value is in question" mean?

Done.

http://codereview.chromium.org/3402011/diff/2001/3008#newcode146
plugins/org.chromium.sdk/src/org/chromium/sdk/JavascriptVm.java:146: *
Asynchronously enables or disables pausing on exception. All exception
On 2010/09/17 10:35:20, apavlov wrote:
> pausing -> breaking
> for consistency?

Done.

http://codereview.chromium.org/3402011/diff/2001/3008#newcode148
plugins/org.chromium.sdk/src/org/chromium/sdk/JavascriptVm.java:148: * @param
enabled new value or null if only current value is in question
On 2010/09/17 10:35:20, apavlov wrote:
> same as above

Done.

http://codereview.chromium.org/3402011/diff/2001/3011
File
plugins/org.chromium.sdk/src/org/chromium/sdk/internal/protocol/FlagsBody.java
(right):

http://codereview.chromium.org/3402011/diff/2001/3011#newcode13
plugins/org.chromium.sdk/src/org/chromium/sdk/internal/protocol/FlagsBody.java:13:
public interface FlagsBody extends JsonSubtype<CommandResponseBody> {
On 2010/09/17 10:35:20, apavlov wrote:
> Flags support should probably go as a separate change.

You mean you want this change split? Cause you never really asked separate
change lists.

http://codereview.chromium.org/3402011/diff/2001/3012
File
plugins/org.chromium.sdk/src/org/chromium/sdk/internal/tools/v8/BreakpointManager.java
(right):

http://codereview.chromium.org/3402011/diff/2001/3012#newcode200
plugins/org.chromium.sdk/src/org/chromium/sdk/internal/tools/v8/BreakpointManager.java:200:
switch (catchType) {
On 2010/09/17 10:35:20, apavlov wrote:
> given that you have the "default" branch below, should we null-check the
> parameter?
I expect switch to fail with NPE anyway. Don't you think it's enough?

http://codereview.chromium.org/3402011/diff/2001/3012#newcode201
plugins/org.chromium.sdk/src/org/chromium/sdk/internal/tools/v8/BreakpointManager.java:201:
case CAUGHT:
On 2010/09/17 10:35:20, apavlov wrote:
> "case"s should be indented

Done.

http://codereview.chromium.org/3402011/diff/2001/3012#newcode231
plugins/org.chromium.sdk/src/org/chromium/sdk/internal/tools/v8/BreakpointManager.java:231:
for (int i = 0; i < flagList.size(); i++) {
On 2010/09/17 10:35:20, apavlov wrote:
> why doesn't for (FlagInfo info : flagList) work for you? It saves a lot of
code
> :)
> 
> On a related note, we usually write ++i

I'm trying to avoid initializing a variable with 'null' default value. Could you
re-write this code without 'null' or an additional variable?

http://codereview.chromium.org/3402011/diff/2001/3012#newcode234
plugins/org.chromium.sdk/src/org/chromium/sdk/internal/tools/v8/BreakpointManager.java:234:
break findCorrectFlag;
On 2010/09/17 10:35:20, apavlov wrote:
> why do you need the label?!

because I don't break a cycle, I break whole block.
the block is written as never normally finishing (exception in the end).
Besides the label gives a self-explanatory name to a block.

http://codereview.chromium.org/3402011/diff/2001/3012#newcode245
plugins/org.chromium.sdk/src/org/chromium/sdk/internal/tools/v8/BreakpointManager.java:245:
resValue = (Boolean)value;
On 2010/09/17 10:35:20, apavlov wrote:
> space after cast

Done.

[Peter Rybin, 2010-09-17 21:19:39]

http://codereview.chromium.org/3402011/diff/2001/3012
File
plugins/org.chromium.sdk/src/org/chromium/sdk/internal/tools/v8/BreakpointManager.java
(right):

http://codereview.chromium.org/3402011/diff/2001/3012#newcode231
plugins/org.chromium.sdk/src/org/chromium/sdk/internal/tools/v8/BreakpointManager.java:231:
for (int i = 0; i < flagList.size(); i++) {
On 2010/09/17 10:35:20, apavlov wrote:
> why doesn't for (FlagInfo info : flagList) work for you? It saves a lot of
code
> :)
> 
> On a related note, we usually write ++i

Does it make sense for Java to use this awkward syntax?
Besides, I checked the code before I joined the project, only 11 increments out
of 30 are prefixed in our codebase.