[webkit-reviews] review denied: [Bug 126669] Web Inspector: cycle clicked breakpoints between enabled, auto-continue, and disabled : [Attachment 220771] patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Brian Burg <bburg at apple.com>'s
request for review:
Bug 126669: Web Inspector: cycle clicked breakpoints between enabled,
auto-continue, and disabled
https://bugs.webkit.org/show_bug.cgi?id=126669

Attachment 220771: patch
https://bugs.webkit.org/attachment.cgi?id=220771&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=220771&action=review


Via IRC discussion I think keeping 2 booleans is better then the 3 modes.

> Source/WebInspectorUI/UserInterface/Breakpoint.js:55
> +    if (modeOrDisabledFlag === true)
> +	   this._mode = WebInspector.Breakpoint.Mode.Disabled;
> +    else
> +	   this._mode = modeOrDisabledFlag ||
WebInspector.Breakpoint.Mode.Enabled;

This constructor is probably the grossest in our frontend. The
"modeOrDisabledFlag" gets pretty confusing here. I'd prefer if we handled the
boolean / non-boolean cases disjointedly:

    if (typeof modeOrDisabledFlag === "boolean")
	this._mode = modeOrDisabledFlag ? Disabled : Enabled;
    else
	this._mode = modeOrDisabledFlag || Enabled;

This removes the easy to miss significance of "=== true" and cascading logic if
that parameter is false.

> Source/WebInspectorUI/UserInterface/Breakpoint.js:216
> +	   if (this._mode === WebInspector.Breakpoint.Mode.Enabled)
> +	       return (this._actions.length > 0) ?
WebInspector.Breakpoint.Mode.AutoContinue :
WebInspector.Breakpoint.Mode.Disabled;
> +	   else if (this._mode === WebInspector.Breakpoint.Mode.AutoContinue)
> +	       return WebInspector.Breakpoint.Mode.Disabled;
> +	   else if (this._mode === WebInspector.Breakpoint.Mode.Disabled)
> +	       return WebInspector.Breakpoint.Mode.Enabled;

Style: We prefer if return; if return; if return; over if return; else if
return; else if return;
Style: Unnecessary parenthesis around ternary condition on line 212. We prefer
dropping it is most cases.

This could just be a switch statement, with:

    default:
	console.assert(false, "Missing mode handling");

> Source/WebInspectorUI/UserInterface/Breakpoint.js:250
> +	       contextMenu.appendItem(WebInspector.UIString("Cancel
Auto-Continue"), setBreakpointMode.bind(this,
WebInspector.Breakpoint.Mode.Enabled));

I think in the UI String we should spell it out. "Cancel Automatic Continue"

> Source/WebInspectorUI/UserInterface/Breakpoint.js:253
> +	    else if (this.mode === WebInspector.Breakpoint.Mode.Disabled)

Style: Uneven leading indent on line 253.

> Source/WebInspectorUI/UserInterface/Breakpoint.js:257
> +	       contextMenu.appendItem(WebInspector.UIString("Set to
Auto-Continue"), setBreakpointMode.bind(this, this.nextMode));

I think in the UI String we should spell it out. "Set to Automatically
Continue"

> Source/WebInspectorUI/UserInterface/Breakpoint.js:371
> -	   this.autoContinue = event.target.checked;
> +	   this.mode = event.target.checked ?
WebInspector.Breakpoint.Mode.AutoContinue :
WebInspector.Breakpoint.Mode.Enabled;
> +	   this._popover.update();

This can conflict with the enabled/disabled checkbox.

Scenario:

  1. open an edit popover for a disabled breakpoint
  2. check automatically continue checkbox
    => the Breakpoint model object has enabled, but the popover still show it
as disabled

I don't think there is a good editing experience here. 2 checkboxes makes more
sense then a <select> / radio for 3 states. For this reason, I think we should
avoid the 3 mode, and instead keep 2 booleans.

We can still keep tri-state clicks Enabled -> AutoContinue -> Disabled. But
only enable that behavior happen if Breakpoint.hasActions().

> Source/WebInspectorUI/UserInterface/Breakpoint.js:527
> +	   delete this._popoverOptionsRowElement;
> +	   delete this._popoverOptionsCheckboxElement;

Heh, good catch =).

> Source/WebInspectorUI/UserInterface/TextEditor.css:71
> +.text-editor > .CodeMirror .breakpoint-auto-continue
.CodeMirror-linenumber::before {
> +    opacity: 0.6;
> +}

Testing this locally it is very difficult to tell if a breakpoint is disabled
or auto-continued. We will need a better UI. Can be done in a follow-up.