Bug 33217 - Web Inspector: there should be a way to "deactivate" or "skip" all breakpoints while debugging.
Summary: Web Inspector: there should be a way to "deactivate" or "skip" all breakpoint...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alexander Pavlov (apavlov)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-05 10:40 PST by Pavel Feldman
Modified: 2010-02-26 02:49 PST (History)
4 users (show)

See Also:


Attachments
Glyph image (197 bytes, image/png)
2010-02-17 13:59 PST, Timothy Hatcher
no flags Details
[PATCH] Proposed solution (15.78 KB, patch)
2010-02-18 05:49 PST, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
[IMAGE] Activated and deactivated breakpoints + Toggle button in the Scripts panel (156.71 KB, image/png)
2010-02-18 05:58 PST, Alexander Pavlov (apavlov)
no flags Details
[PATCH] Proposed solution with English.lproj (16.17 KB, patch)
2010-02-18 06:00 PST, Alexander Pavlov (apavlov)
pfeldman: review-
Details | Formatted Diff | Diff
Disabled Glyph (257 bytes, image/png)
2010-02-18 08:30 PST, Timothy Hatcher
no flags Details
Wider Disabled Glyph (258 bytes, image/png)
2010-02-18 08:35 PST, Timothy Hatcher
no flags Details
Simplier Disabled Glyph (263 bytes, image/png)
2010-02-18 08:36 PST, Timothy Hatcher
no flags Details
Strike Disabled Glyph (387 bytes, image/png)
2010-02-18 08:41 PST, Timothy Hatcher
no flags Details
[PATCH] Comments addressed, fixed glyphs, added Chromium backend support (20.84 KB, patch)
2010-02-19 03:45 PST, Alexander Pavlov (apavlov)
pfeldman: review-
Details | Formatted Diff | Diff
[PATCH] Fixed breakpoint double-disabling via CSS opacity rather than repainting (15.96 KB, patch)
2010-02-19 09:17 PST, Alexander Pavlov (apavlov)
pfeldman: review+
eric: commit-queue-
Details | Formatted Diff | Diff
[PATCH] Merge in ScriptDebugServer changes, auto-activate BPs once a BP is added (16.28 KB, patch)
2010-02-22 03:50 PST, Alexander Pavlov (apavlov)
pfeldman: review+
apavlov: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Feldman 2010-01-05 10:40:19 PST
It is called "Deactivate" in Xcode and "Skip all breakpoins" in Eclipse. This toggle state applies to all the breakpoints out there.
Comment 1 Timothy Hatcher 2010-02-17 13:59:21 PST
Created attachment 48935 [details]
Glyph image
Comment 2 Alexander Pavlov (apavlov) 2010-02-18 05:49:07 PST
Created attachment 49003 [details]
[PATCH] Proposed solution
Comment 3 Alexander Pavlov (apavlov) 2010-02-18 05:58:46 PST
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."
Comment 4 Alexander Pavlov (apavlov) 2010-02-18 06:00:10 PST
Created attachment 49005 [details]
[PATCH] Proposed solution with English.lproj
Comment 5 Pavel Feldman 2010-02-18 07:17:22 PST
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".
Comment 6 Timothy Hatcher 2010-02-18 08:10:36 PST
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.
Comment 7 Timothy Hatcher 2010-02-18 08:13:45 PST
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.
Comment 8 Pavel Feldman 2010-02-18 08:19:33 PST
(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?
Comment 9 Timothy Hatcher 2010-02-18 08:30:02 PST
Created attachment 49016 [details]
Disabled Glyph
Comment 10 Timothy Hatcher 2010-02-18 08:35:49 PST
Created attachment 49017 [details]
Wider Disabled Glyph
Comment 11 Timothy Hatcher 2010-02-18 08:36:08 PST
Created attachment 49018 [details]
Simplier Disabled Glyph
Comment 12 Timothy Hatcher 2010-02-18 08:41:34 PST
Created attachment 49019 [details]
Strike Disabled Glyph
Comment 13 Timothy Hatcher 2010-02-18 08:44:42 PST
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.
Comment 14 Alexander Pavlov (apavlov) 2010-02-19 03:45:14 PST
Created attachment 49064 [details]
[PATCH] Comments addressed, fixed glyphs, added Chromium backend support
Comment 15 Alexander Pavlov (apavlov) 2010-02-19 03:47:35 PST
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"
Comment 16 Pavel Feldman 2010-02-19 05:33:39 PST
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.
Comment 17 Alexander Pavlov (apavlov) 2010-02-19 09:17:35 PST
Created attachment 49084 [details]
[PATCH] Fixed breakpoint double-disabling via CSS opacity rather than repainting
Comment 18 Timothy Hatcher 2010-02-19 09:30:23 PST
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.)
Comment 19 Eric Seidel (no email) 2010-02-19 13:38:53 PST
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
Comment 20 Pavel Feldman 2010-02-19 13:52:09 PST
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).
Comment 21 Alexander Pavlov (apavlov) 2010-02-22 03:50:45 PST
Created attachment 49198 [details]
[PATCH] Merge in ScriptDebugServer changes, auto-activate BPs once a BP is added
Comment 22 Alexander Pavlov (apavlov) 2010-02-22 04:15:42 PST
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