Bug 33217 – Web Inspector: there should be a way to "deactivate" or "skip" all breakpoints while debugging.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Pavel Feldman]

It is called "Deactivate" in Xcode and "Skip all breakpoins" in Eclipse. This toggle state applies to all the breakpoints out there.

[Timothy Hatcher]

Created attachment 48935 [details]
Glyph image

[Alexander Pavlov (apavlov)]

Created attachment 49003 [details]
[PATCH] Proposed solution

[Alexander Pavlov (apavlov)]

Created attachment 49004 [details]
[IMAGE] Activated and deactivated breakpoints + Toggle button in the Scripts panel

The tooltip for the button says either "Deactivate all breakpoints." or "Activate all breakpoints."

[Alexander Pavlov (apavlov)]

Created attachment 49005 [details]
[PATCH] Proposed solution with English.lproj

[Pavel Feldman]

Comment on attachment 49005 [details]
[PATCH] Proposed solution with English.lproj

> +void JavaScriptDebugServer::setBreakpointsActivated(bool activated)

I'd use (de)activateBreakpoints pair of methods here. Or setBreakpointsActive. I personally don't like Activated, leaving to Timothy.
> +        if (WebInspector.panels.scripts)
> +            WebInspector.panels.scripts.addEventListener("breakpointsActivationChanged", this._breakpointsActivationChanged, this);

ScriptsPanel has a pointer to the active view, so you don't need listener-based indirection here. r- is for this.

> +    // Breakpoints should be activated by default, so emulate a click to toggle on.
> +    this._deactivateClicked();

This sounds confusing. By default should be activated, so let us call "deactivate".

[Timothy Hatcher]

Comment on attachment 49005 [details]
[PATCH] Proposed solution with English.lproj


> +.deactivate-breakpoints.toggled-on .glyph {
> +    background-color: rgb(1, 142, 217) !important;
> +}

You don't need !important here since this rule has a more specific selector and is later in the file.

Also the button looks like I can't click it when breakpoints are deactivated. I wonder if we should have it be normal grey when deactivated and blue when activated? Otherwise the light blue looks more like it is just a disabled button.

[Timothy Hatcher]

I know you were trying to matc the breakpoint colors for the two states. But the more I look at the button the more it looks disabled, since it is next to other disabled (light grey) buttons.

I think using the normal toggled-on blue and the darker grey colors would help.

[Pavel Feldman]

(In reply to comment #7)
> I know you were trying to matc the breakpoint colors for the two states. But
> the more I look at the button the more it looks disabled, since it is next to
> other disabled (light grey) buttons.
> 
> I think using the normal toggled-on blue and the darker grey colors would help.

Yeah, sorry, I pulled Alexander into this. How about strike-through circle around breakpoint?

[Timothy Hatcher]

Created attachment 49016 [details]
Disabled Glyph

[Timothy Hatcher]

Created attachment 49017 [details]
Wider Disabled Glyph

[Timothy Hatcher]

Created attachment 49018 [details]
Simplier Disabled Glyph

[Timothy Hatcher]

Created attachment 49019 [details]
Strike Disabled Glyph

[Timothy Hatcher]

On IRC we decided to use a different glyph for the deactivated state. Give the Strike Disabled Glyph a try.

And there would be no color, just the dark grey.

[Alexander Pavlov (apavlov)]

Created attachment 49064 [details]
[PATCH] Comments addressed, fixed glyphs, added Chromium backend support

[Alexander Pavlov (apavlov)]

Comment on attachment 49004 [details]
[IMAGE] Activated and deactivated breakpoints + Toggle button in the Scripts panel

The "disabled breakpoints" button glyph is not a "dimmed blue" but rather "Strike Disabled Glyph"

[Pavel Feldman]

Comment on attachment 49064 [details]
[PATCH] Comments addressed, fixed glyphs, added Chromium backend support

Two thoughts here:
1) It would be much cleaner (and would require no repaints) if you added some opacity 0.5 style that would be applicable to line numbers with breakpoints.
2) Not related to this change, but worth fixing: _drawBreakpointImagesIfNeeded and _drawProgramCounterImageIfNeeded should be static. We no longer have iframes, so it is sufficient to create those only once.

[Alexander Pavlov (apavlov)]

Created attachment 49084 [details]
[PATCH] Fixed breakpoint double-disabling via CSS opacity rather than repainting

[Timothy Hatcher]

Comment on attachment 49084 [details]
[PATCH] Fixed breakpoint double-disabling via CSS opacity rather than repainting

Should we activate breakpoints if the user adds a new one? Xcode does this, and it saves you from needing to remember to activate them (otherwise you wonder why the break isn't hit.)

[Eric Seidel (no email)]

Comment on attachment 49084 [details]
[PATCH] Fixed breakpoint double-disabling via CSS opacity rather than repainting

Purple EWS bubbles mean that this patch did not apply, thus it can't be cq+'d

[Pavel Feldman]

Comment on attachment 49084 [details]
[PATCH] Fixed breakpoint double-disabling via CSS opacity rather than repainting

I see no problems other than the one mentioned by Timothy (we should enable breakpoints upon adding the new one). This can be done in subsequent change (and should anyway be a trivial change).

[Alexander Pavlov (apavlov)]

Created attachment 49198 [details]
[PATCH] Merge in ScriptDebugServer changes, auto-activate BPs once a BP is added

[Alexander Pavlov (apavlov)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
        M       WebCore/ChangeLog
        M       WebCore/English.lproj/localizedStrings.js
        M       WebCore/bindings/js/ScriptDebugServer.cpp
        M       WebCore/bindings/js/ScriptDebugServer.h
        M       WebCore/bindings/v8/ScriptDebugServer.h
        M       WebCore/inspector/InspectorBackend.cpp
        M       WebCore/inspector/InspectorBackend.h
        M       WebCore/inspector/InspectorBackend.idl
        A       WebCore/inspector/front-end/Images/deactivateBreakpointsButtonGlyph.png
        A       WebCore/inspector/front-end/Images/deactivateBreakpointsDisabledButtonGlyph.png
        M       WebCore/inspector/front-end/InspectorBackendStub.js
        M       WebCore/inspector/front-end/ScriptsPanel.js
        M       WebCore/inspector/front-end/inspector.css
        M       WebCore/inspector/front-end/textViewer.css
        M       WebKit/chromium/ChangeLog
        M       WebKit/chromium/src/js/DebuggerAgent.js
        M       WebKit/chromium/src/js/InspectorControllerImpl.js
Committed r55077