[webkit-reviews] review granted: [Bug 28622] Caught exceptions still pause the debugger : [Attachment 47064] [PATCH] Tri-State Pause on Exceptions Button

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at hatcher.name> has granted Brian Weinstein
<bweinstein at apple.com>'s request for review:
Bug 28622: Caught exceptions still pause the debugger
https://bugs.webkit.org/show_bug.cgi?id=28622

Attachment 47064: [PATCH] Tri-State Pause on Exceptions Button
https://bugs.webkit.org/attachment.cgi?id=47064&action=review

------- Additional Comments from Timothy Hatcher <timothy at hatcher.name>

> -    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.