Bug 126669 – Web Inspector: cycle clicked breakpoints between enabled, auto-continue, and disabled

• Component: Web Inspector
• Status: RESOLVED FIXED

[Blaze Burg]

(when a breakpoint has actions)

[Radar WebKit Bug Importer]

<rdar://problem/15777071>

[Blaze Burg]

Created attachment 220771 [details]
patch

[Joseph Pecoraro]

Comment on attachment 220771 [details]
patch

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.

[Blaze Burg]

Created attachment 220797 [details]
patch

[Joseph Pecoraro]

Comment on attachment 220797 [details]
patch

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.HiddenStyleClassName);
>          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.

[Blaze Burg]

Comment on attachment 220797 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=220797&action=review

>> Source/WebInspectorUI/UserInterface/Breakpoint.js:223
>> +        }
> 
> 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

Not sure how I didn't catch this :|

>> Source/WebInspectorUI/UserInterface/Breakpoint.js:377
>>      },
> 
> Is this true? Will the popover ever need to change size here?

I think these are left over from the probes changes, will remove.

>> Source/WebInspectorUI/UserInterface/Breakpoint.js:395
>> +        checkboxElement.checked = !this.disabled;
> 
> Nit: I'd prefer we use the member variable internally instead of the getter.

OK

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

Inside removeAction, it toggles autoContinue if no more actions exist. I put this code here because the popover doesn't actually listen for any events, IIRC.

[Blaze Burg]

Created attachment 220853 [details]
v3

[Joseph Pecoraro]

Comment on attachment 220853 [details]
v3

View in context: https://bugs.webkit.org/attachment.cgi?id=220853&action=review

r=me

> Source/WebInspectorUI/ChangeLog:31
> +        (WebInspector.Breakpoint.prototype._popoverToggleEnabledCheckboxChanged):
> +        (WebInspector.Breakpoint.prototype._popoverToggleAutoContinueCheckboxChanged):
> +        Update the popover height since this could cause additional content to become visible.

Nit: Stale

> Source/WebInspectorUI/UserInterface/TextEditor.js:1167
> +            if (this._lineNumberWithMousedDownBreakpoint in this._breakpoints && this._columnNumberWithMousedDownBreakpoint in this._breakpoints[this._lineNumberWithMousedDownBreakpoint] && delegateImplementsBreakpointClicked)
> +                this._delegate.textEditorBreakpointClicked(this, this._lineNumberWithMousedDownBreakpoint, this._columnNumberWithMousedDownBreakpoint);
>          }

Do we need to update the TextEditor's this._breakpoints breakpoint info map which was being done before? Or does that eventually get triggered by breakpoint modification events (I suspect it does, because what if the breakpoint was showing in multiple files)?

[Blaze Burg]

Created attachment 220857 [details]
v3 fix changelog

[WebKit Commit Bot]

Comment on attachment 220857 [details]
v3 fix changelog

Rejecting attachment 220857 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 220857, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
0-ab3c-d52691b4dbfc ...
Currently at 161669 = 446d3e266be63cf09c7ddaf95dbb98ea2523e6cc
r161670 = 19009620d163404d4272ce198eaa8d5c7398af37
r161671 = 2a63fbe2cad2cf56d0f7a9329fc9c23e20c2f9f0
r161672 = 732907db7aa77fbc203f8eb4d4c08d70d5b0a738
r161673 = 8996b5b1ffbddee0dc255c150e842cc6bca22464
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: http://webkit-queues.appspot.com/results/6744813554630656

[Blaze Burg]

Created attachment 220898 [details]
v3 retry cq

[WebKit Commit Bot]

Comment on attachment 220898 [details]
v3 retry cq

Clearing flags on attachment: 220898

Committed r161687: <http://trac.webkit.org/changeset/161687>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.