[webkit-reviews] review denied: [Bug 126669] Web Inspector: cycle clicked breakpoints between enabled, auto-continue, and disabled : [Attachment 220797] 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 220797: patch
https://bugs.webkit.org/attachment.cgi?id=220797&action=review

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


r- for breaking toggling a generic breakpoint.

> Source/WebInspectorUI/UserInterface/Breakpoint.js:223
> +	   if (!this.disabled && this.autoContinue) {
> +	       this.disabled = true;
> +	       return;
> +	   }
> +
> +	   if (!this.disabled && this.actions.length) {
> +	       this.autoContinue = true;
> +	       return;
> +	   }

Nit: The !this.disabled checks are both unnecessary after the first if and
bail.

Looks like you're missing a base case. You have:

  if disabled -> enabled
  if autocontinue -> disabled
  if enabled and actions -> auto continue

But if you're enabled and don't have actions you should become disabled!

r- for this.

So this method should end with:

  this.disabled = true;

Scenario to test:

  1. Create breakpoint
  2. Toggle by clicking in gutter
    => should toggle enabled/disabled

> Source/WebInspectorUI/UserInterface/Breakpoint.js:365
> +	   this._popover.update();

Is this true? Will the popover ever need to change size here?

> Source/WebInspectorUI/UserInterface/Breakpoint.js:377
>      _popoverToggleAutoContinueCheckboxChanged: function(event)
>      {
>	   this.autoContinue = event.target.checked;
> +	   this._popover.update();
>      },

Is this true? Will the popover ever need to change size here?

> Source/WebInspectorUI/UserInterface/Breakpoint.js:395
> +	   checkboxElement.checked = !this.disabled;

Nit: I'd prefer we use the member variable internally instead of the getter.

> Source/WebInspectorUI/UserInterface/Breakpoint.js:446
> +	       optionsCheckbox.checked = this.autoContinue;

Nit: I'd prefer we use the member variable internally instead of the getter.

> Source/WebInspectorUI/UserInterface/Breakpoint.js:505
> +	  
this._popoverOptionsRowElement.classList.remove(WebInspector.Breakpoint.HiddenS
tyleClassName);
>	  
this._popoverActionsInsertBreakpointActionView(newBreakpointActionView, index);


Style: I would flip these two lines to match the order of events up above
(InsertBreakpointActionView, remove HiddenStyleClass, popover.update).

> Source/WebInspectorUI/UserInterface/Breakpoint.js:517
> +	       this._popoverOptionsCheckboxElement.checked = false;

I don't think that this will trigger an onchange event, so if you expect this
to set breakpoint.autoContinue = false, that won't be happening. I would
suggest you be explicit here:

    this.autoContinue = false;
    this._popoverOptionsCheckboxElement.checked = false;

You can test this scenario like so:

  1. Create breakpoint
  2. Edit breakpoint
  3. Add action
  4. Enable Automatic Continue
  5. Remove action
    => checkbox is set to false, but autoContinue hasn't changed
  6. Add Action
    => notice autoContinue checkbox is unchecked
  7. Dismiss popover
  8. Edit breakpoint
    => I would expect checkbox should be unchecked. With this patch I think it
will be checked.

Hmm, I just tested locally and it seem to work as expected, I don't know why
though.