Bug 182406 – Web Inspector: Hide DOM and XHR breakpoint sections when they are empty

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Matt Baker]

Summary:
Hide DOM and XHR breakpoint sections when they are empty. The DOM Breakpoints section has no value when empty, since breakpoints are added from the Elements tab. XHR breakpoints are a little trickier, since the "All Requests" global breakpoint needs a home.

[Radar WebKit Bug Importer]

<rdar://problem/37131512>

[Nikita Vasilyev]

> XHR breakpoints are a little trickier, since the "All Requests" global breakpoint needs a home.

Any ideas where would it go?

I think it's fine to keep XHR breakpoint section as-is,
but I'm open to new ideas.

[Devin Rousso]

Created attachment 348395 [details]
Patch

[Devin Rousso]

Created attachment 348396 [details]
[Image] After Patch is applied

[Blaze Burg]

Comment on attachment 348395 [details]
Patch

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

Overall, I think this is a positive change. I have some code arrangement suggestions and nits for you to think about.

There are also several design issues that need to be resolved prior to granting review:
- With this change, all other sidebar panel filter bars are mispositioned. Only Debugger sidebar panel needs the extra space.
- The "show only resources with errors" filter seems to have disappeared with the patch applied.
- The + button is not properly inverted for Dark Mode. I think we do this correctly in other places, take a look.

Lastly some functionality changes I'd like, but perhaps we need to discuss in team meeting:
- The + is hard to find. I'd expect it to be present on the right side of the section header for the Breakpoints section, because that section is always available.
- Instead of the text "No Breakpoints" it would be better to hyperlink the text "Add Breakpoint" or present a button with the same.
- The above suggestion also applies when the popover appears, anchored to the text No Breakpoints. It would be clearer if the popover was anchored to "Add Breakpoint" or "New Breakpoint".

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:402
> +localizedStrings["Event Breakpoint..."] = "Event Breakpoint...";

Nit: use an ellipsis codepoint

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1078
> +localizedStrings["XHR Breakpoint..."] = "XHR Breakpoint...";

Nit: use an ellipsis codepoint

> Source/WebInspectorUI/UserInterface/Controllers/BreakpointPopoverController.js:39
> +    appendContextMenuItems(contextMenu, breakpoint, breakpointDisplayElement, breakpointTreeElement)

I haven't read this code before. My thoughts:

- Can't we locate the display element and tree element from a breakpoint?
- I don't like passing nullable DOM elements as positional things. If it's truly optional, make the last argument an options object.
- This would be better named 'appendContextMenuItemsForBreakpoint(breakpoint, contextMenu, options={})'

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:170
> +        console.assert(this.isBreakpointRemovable(breakpoint));

Please include `breakpoint` as the last positional argument so that it gets logged when this assertion fails.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:235
> +        console.assert(this.isBreakpointRemovable(breakpoint));

Ditto.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:297
> +        console.assert(this.isBreakpointRemovable(breakpoint));

Ditto.

> Source/WebInspectorUI/UserInterface/Controllers/XHRBreakpointTreeController.js:-37
> -        this._allReqestsBreakpointTreeElement = new WI.XHRBreakpointTreeElement(WI.domDebuggerManager.allRequestsBreakpoint, WI.DebuggerSidebarPanel.AssertionIconStyleClassName, WI.UIString("All Requests"));

Lol, typo removal.

> Source/WebInspectorUI/UserInterface/Views/BreakpointTreeElement.js:86
> +            this.__deletedViaDeleteKeyboardShortcut = true;

Nit: use a symbol?

> Source/WebInspectorUI/UserInterface/Views/DOMBreakpointTreeElement.js:119
> +        contextMenu.appendItem(WI.UIString("Delete Breakpoint"), () => {

This could be on one line, I think.

> Source/WebInspectorUI/UserInterface/Views/DOMBreakpointTreeElement.js:143
> +    _removeBreakpoint()

To maintain MVC separation, I'd prefer this is done by calling a method of the controller of the relevant tree outline. I think this is DOMBreakpointTreeController. Ideally the tree element doesn't care about the two different code paths.

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.css:38
> +body[dir=ltr]  .sidebar > .panel.navigation.debugger > .options > .add-breakpoint {

Nit: extra space after [dir=ltr]

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.css:42
> +body[dir=rtl]  .sidebar > .panel.navigation.debugger > .options > .add-breakpoint {

Ditto

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:140
> +        let breakpointsGroup = new WI.DetailsSectionGroup([this._breakpointsRow]);

It might be time to move the normal and "special" breakpoints into their own tree controller class. DebuggerSidebarPanel is getting pretty large and unwieldy, so any way we can abstract the details of the sections without complicating things would be good IMO.

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:1356
> +    _xhrBreakpointAddedOrRemoved(event)

Along the lines above, all of the layout logic for context menu item handlers should probably live in the relevant controller class. It would be great if the DebuggerSidebarPanel didn't have to keep track of all rows, sections, etc. It could just pull the section or whatever from the controller and append it to its own tree.

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:1393
> +        contextMenu.appendCheckboxItem(WI.UIString("All Exceptions"), () => {

I am more divided about how to divvy up this method. The current approach needs to dig into the tree outlines and maintain elements in instance variables. I'd like to move away from that per above comments.

One approach is to have each breakpoint tree controller add whatever context menu items it wants to the Add Breakpoint menu. A less significant change would be for the method here to call API of the relevant controllers rather than munging DOM elements itself but I prefer the other approach that abstracts away the handlers completely.

[Matt Baker]

(In reply to Brian Burg from comment #5)
> Comment on attachment 348395 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=348395&action=review
> 
> Overall, I think this is a positive change. I have some code arrangement
> suggestions and nits for you to think about.
> 
> There are also several design issues that need to be resolved prior to
> granting review:
> - With this change, all other sidebar panel filter bars are mispositioned.
> Only Debugger sidebar panel needs the extra space.
> - The "show only resources with errors" filter seems to have disappeared
> with the patch applied.

I'm not sure how useful this was. If we'd like to retain the ability to filter on issues, the button can be placed within the filter text field, right-aligned (in LTR). This is where Xcode places toggle filters.

> - The + button is not properly inverted for Dark Mode. I think we do this
> correctly in other places, take a look.
> 
> Lastly some functionality changes I'd like, but perhaps we need to discuss
> in team meeting:
> - The + is hard to find. I'd expect it to be present on the right side of
> the section header for the Breakpoints section, because that section is
> always available.

The header isn't always visible, since it can be scrolled out of view when there are lots of tree items. Personally, I like the current placement.

[Devin Rousso]

Comment on attachment 348395 [details]
Patch

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

> - The "show only resources with errors" filter seems to have disappeared
> with the patch applied.
This is something I had talked about with Matt. I'm trying to follow what Xcode does as far as their breakpoint sidebar, and they have no mention of this. I will add it back.

> - The + button is not properly inverted for Dark Mode. I think we do this
> correctly in other places, take a look.
That's right! I need to start thinking about this too :P

> - The + is hard to find. I'd expect it to be present on the right side of
> the section header for the Breakpoints section, because that section is
> always available.
This matches the location of the "+" in Xcode, not to mention some of the other "+" that we have in WebInspector (e.g. styles sidebar).


> - Instead of the text "No Breakpoints" it would be better to hyperlink the
> text "Add Breakpoint" or present a button with the same.
Ooo I like this idea!

> - The above suggestion also applies when the popover appears, anchored to
> the text No Breakpoints. It would be clearer if the popover was anchored to
> "Add Breakpoint" or "New Breakpoint".
👍

>> Source/WebInspectorUI/UserInterface/Controllers/BreakpointPopoverController.js:39
>> +    appendContextMenuItems(contextMenu, breakpoint, breakpointDisplayElement, breakpointTreeElement)
> 
> I haven't read this code before. My thoughts:
> 
> - Can't we locate the display element and tree element from a breakpoint?
> - I don't like passing nullable DOM elements as positional things. If it's truly optional, make the last argument an options object.
> - This would be better named 'appendContextMenuItemsForBreakpoint(breakpoint, contextMenu, options={})'

If given an arbitrary breakpoint, I don't think we have any way of getting a `WI.TreeElement` from it without knowing it's "owner" `WI.TreeOutline`.  That seems like a layering violation to me.

With that having been said, I think I can make this change work without needing the nullable `breakpointTreeElement`.

>> Source/WebInspectorUI/UserInterface/Views/DOMBreakpointTreeElement.js:119
>> +        contextMenu.appendItem(WI.UIString("Delete Breakpoint"), () => {
> 
> This could be on one line, I think.

I personally don't like putting these types of functions on one line.  I prefer only putting things on one line when we actually want to use the return value, as there could be unintended side-effects in the future if the function changes to have a return value or the caller changes to expect one.

>> Source/WebInspectorUI/UserInterface/Views/DOMBreakpointTreeElement.js:143
>> +    _removeBreakpoint()
> 
> To maintain MVC separation, I'd prefer this is done by calling a method of the controller of the relevant tree outline. I think this is DOMBreakpointTreeController. Ideally the tree element doesn't care about the two different code paths.

I don't think the `WI.TreeELement` has any access to the controller, so I'm not sure what you mean by this.  I agree that it would be nicer to "centralize" this, but I don't really see how.

[Blaze Burg]

Comment on attachment 348395 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Controllers/BreakpointPopoverController.js:39
>>> +    appendContextMenuItems(contextMenu, breakpoint, breakpointDisplayElement, breakpointTreeElement)
>> 
>> I haven't read this code before. My thoughts:
>> 
>> - Can't we locate the display element and tree element from a breakpoint?
>> - I don't like passing nullable DOM elements as positional things. If it's truly optional, make the last argument an options object.
>> - This would be better named 'appendContextMenuItemsForBreakpoint(breakpoint, contextMenu, options={})'
> 
> If given an arbitrary breakpoint, I don't think we have any way of getting a `WI.TreeElement` from it without knowing it's "owner" `WI.TreeOutline`.  That seems like a layering violation to me.
> 
> With that having been said, I think I can make this change work without needing the nullable `breakpointTreeElement`.

It's possible! This should do the trick...

WI.tabBar.selectedTabBarItem.representedObject.navigationSidebarPanel.treeElementForRepresentedObject(breakpoint)

That said, this is just a popover controller, so it's probably best to just name it "anchorElement".

> Source/WebInspectorUI/UserInterface/Controllers/BreakpointPopoverController.js:62
> +                breakpointTreeElement.parent.removeChild(breakpointTreeElement);

I don't like how this is digging around into the tree element's DOM. Can we have the breakpoint controller listen for breakpoint disabled event then adjust the DOM accordingly?

>>> Source/WebInspectorUI/UserInterface/Views/DOMBreakpointTreeElement.js:119
>>> +        contextMenu.appendItem(WI.UIString("Delete Breakpoint"), () => {
>> 
>> This could be on one line, I think.
> 
> I personally don't like putting these types of functions on one line.  I prefer only putting things on one line when we actually want to use the return value, as there could be unintended side-effects in the future if the function changes to have a return value or the caller changes to expect one.

OK

>>> Source/WebInspectorUI/UserInterface/Views/DOMBreakpointTreeElement.js:143
>>> +    _removeBreakpoint()
>> 
>> To maintain MVC separation, I'd prefer this is done by calling a method of the controller of the relevant tree outline. I think this is DOMBreakpointTreeController. Ideally the tree element doesn't care about the two different code paths.
> 
> I don't think the `WI.TreeELement` has any access to the controller, so I'm not sure what you mean by this.  I agree that it would be nicer to "centralize" this, but I don't really see how.

To do this, you can provide a delegate object to each tree element, then call this.delegate.breakpointControllerRemoveBreakpoint(breakpoint). Then the various *BreakpointTreeController objects can implement that differently as needed.

[Devin Rousso]

Comment on attachment 348395 [details]
Patch

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

>>>> Source/WebInspectorUI/UserInterface/Controllers/BreakpointPopoverController.js:39
>>>> +    appendContextMenuItems(contextMenu, breakpoint, breakpointDisplayElement, breakpointTreeElement)
>>> 
>>> I haven't read this code before. My thoughts:
>>> 
>>> - Can't we locate the display element and tree element from a breakpoint?
>>> - I don't like passing nullable DOM elements as positional things. If it's truly optional, make the last argument an options object.
>>> - This would be better named 'appendContextMenuItemsForBreakpoint(breakpoint, contextMenu, options={})'
>> 
>> If given an arbitrary breakpoint, I don't think we have any way of getting a `WI.TreeElement` from it without knowing it's "owner" `WI.TreeOutline`.  That seems like a layering violation to me.
>> 
>> With that having been said, I think I can make this change work without needing the nullable `breakpointTreeElement`.
> 
> It's possible! This should do the trick...
> 
> WI.tabBar.selectedTabBarItem.representedObject.navigationSidebarPanel.treeElementForRepresentedObject(breakpoint)
> 
> That said, this is just a popover controller, so it's probably best to just name it "anchorElement".

Wow that seems like a HUGE layer violation 😅

>> Source/WebInspectorUI/UserInterface/Controllers/BreakpointPopoverController.js:62
>> +                breakpointTreeElement.parent.removeChild(breakpointTreeElement);
> 
> I don't like how this is digging around into the tree element's DOM. Can we have the breakpoint controller listen for breakpoint disabled event then adjust the DOM accordingly?

The issue is that we don't want to remove the breakpoint when it's just disabled.  The current approach already works if the user just wants to disable the breakpoint, so we want to keep that functionality and just add the ability to delete.

[Devin Rousso]

Created attachment 348469 [details]
Patch

I tried creating a `WI.BreakpointTreeController`, but the logic of `WI.DebuggerSidebarPanel` is so tightly coupled that it would probably be too large of a patch.

[Devin Rousso]

Created attachment 348470 [details]
[Image] After Patch is applied

[EWS Watchlist]

Comment on attachment 348469 [details]
Patch

Attachment 348469 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9030623

New failing tests:
inspector/debugger/breakpoints-disabled.html
inspector/debugger/pause-reason.html
inspector/worker/debugger-pause.html
inspector/debugger/break-on-uncaught-exception-throw-in-promise.html
inspector/debugger/setPauseOnAssertions.html
inspector/debugger/break-on-uncaught-exception.html

[EWS Watchlist]

Created attachment 348475 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 348469 [details]
Patch

Attachment 348469 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9030725

New failing tests:
inspector/debugger/breakpoints-disabled.html
inspector/debugger/pause-reason.html
inspector/worker/debugger-pause.html
inspector/debugger/break-on-uncaught-exception-throw-in-promise.html
inspector/debugger/setPauseOnAssertions.html
inspector/debugger/break-on-uncaught-exception.html

[EWS Watchlist]

Created attachment 348476 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 348469 [details]
Patch

Attachment 348469 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9030729

New failing tests:
inspector/debugger/breakpoints-disabled.html
inspector/debugger/pause-reason.html
inspector/worker/debugger-pause.html
inspector/debugger/break-on-uncaught-exception-throw-in-promise.html
inspector/debugger/setPauseOnAssertions.html
inspector/debugger/break-on-uncaught-exception.html

[EWS Watchlist]

Created attachment 348480 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Devin Rousso]

Created attachment 348481 [details]
Patch

[Blaze Burg]

(In reply to Devin Rousso from comment #7)
> Comment on attachment 348395 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=348395&action=review
> 
> > - The "show only resources with errors" filter seems to have disappeared
> > with the patch applied.
> This is something I had talked about with Matt. I'm trying to follow what
> Xcode does as far as their breakpoint sidebar, and they have no mention of
> this. I will add it back.
> 
> > - The + button is not properly inverted for Dark Mode. I think we do this
> > correctly in other places, take a look.
> That's right! I need to start thinking about this too :P
> 
> > - The + is hard to find. I'd expect it to be present on the right side of
> > the section header for the Breakpoints section, because that section is
> > always available.
> This matches the location of the "+" in Xcode, not to mention some of the
> other "+" that we have in WebInspector (e.g. styles sidebar).

I'm aware Xcode does it that way. I believe they have made the wrong tradeoff. Putting at the bottom edge makes it have a larger hit target, but makes it less discoverable. If the + is at the bottom of the sidebar, a user has to see "No Breakpoints", get confused, and hopefully continue reading all the way down to the bottom of the sidebar, and click on a + that's not labelled or associated with anything. In the common case it will be located closer to a list of resources than an empty list of breakpoints.

[Devin Rousso]

(In reply to Brian Burg from comment #19)
> I'm aware Xcode does it that way. I believe they have made the wrong
> tradeoff. Putting at the bottom edge makes it have a larger hit target, but
> makes it less discoverable. If the + is at the bottom of the sidebar, a user
> has to see "No Breakpoints", get confused, and hopefully continue reading
> all the way down to the bottom of the sidebar, and click on a + that's not
> labelled or associated with anything. In the common case it will be located
> closer to a list of resources than an empty list of breakpoints.
I think the solution in attachment 348481 [details] is a good compromise.  It makes use of the same `WI.createNavigationItemHelp` that the Canvas tab uses to let users know about importing when there are no canvases.  Putting the "+" in the header of the "Breakpoints" section doesn't make a huge amount of sense to me since it's possible for the user to collapse that section.  If we decide to unify all of these views into the "Sources" tab then we might want some sort of "function bar" at the top of the sidebar for things of this nature.

[Joseph Pecoraro]

I don't like the idea of removing the top level exception breakpoints. I think these should always be available, and never be hidden:

    [E] All Exceptions
    [E] Uncaught Exceptions

I feel they are too useful to be hidden behind a potentially hard to find (+) button.

[Matt Baker]

(In reply to Joseph Pecoraro from comment #21)
> I don't like the idea of removing the top level exception breakpoints. I
> think these should always be available, and never be hidden:
> 
>     [E] All Exceptions
>     [E] Uncaught Exceptions
> 
> I feel they are too useful to be hidden behind a potentially hard to find
> (+) button.

Strongly agree.

[Devin Rousso]

(In reply to Joseph Pecoraro from comment #21)
> I don't like the idea of removing the top level exception breakpoints. I
> think these should always be available, and never be hidden:
> 
>     [E] All Exceptions
>     [E] Uncaught Exceptions
> 
> I feel they are too useful to be hidden behind a potentially hard to find
> (+) button.
My objection to this is that it then makes it confusing as to what's deletable and what isn't, especially since there's no indicator.  While I do agree with the utility of the uncaught exceptions breakpoint, I don't think the all exceptions breakpoint is as useful.  Both of these (and the assertions breakpoint) are added by default in this change, and will stay visible until the user decides to delete them.

[Joseph Pecoraro]

> My objection to this is that it then makes it confusing as to what's
> deletable and what isn't, especially since there's no indicator.  While I do
> agree with the utility of the uncaught exceptions breakpoint, I don't think
> the all exceptions breakpoint is as useful.  Both of these (and the
> assertions breakpoint) are added by default in this change, and will stay
> visible until the user decides to delete them.

I don't think it will be confusing that they are not deletable. That has been the status quo in Safari for years. I think it would be more confusing for users to not know the feature exists, or if they know it exists have to go through 2-3 clicks to get them back and enabled. Especially given how frequently I toggle these breakpoints during debugging sessions.

I have found All Exceptions to be useful multiple times in the past. Certainly All Uncaught Exceptions is the most useful, but still All Exceptions has value. The Assertions breakpoints I agree is far less likely to be used and I like the idea of that being excluded by default.

I don't think confusion around delete-ability is reason to remove them. For comparison we just made the same change with tabs. We removed the ability to "close tabs" because it was easy to do accidentally and when users hit that it they then had to figure out how to get them back, which was not immediately obvious to some users.

I still feel strongly that we should keep these global breakpoints always visible.

[Matt Baker]

(In reply to Joseph Pecoraro from comment #24)
> > My objection to this is that it then makes it confusing as to what's
> > deletable and what isn't, especially since there's no indicator.  While I do
> > agree with the utility of the uncaught exceptions breakpoint, I don't think
> > the all exceptions breakpoint is as useful.  Both of these (and the
> > assertions breakpoint) are added by default in this change, and will stay
> > visible until the user decides to delete them.
> 
> I don't think it will be confusing that they are not deletable. That has
> been the status quo in Safari for years. I think it would be more confusing
> for users to not know the feature exists, or if they know it exists have to
> go through 2-3 clicks to get them back and enabled. Especially given how
> frequently I toggle these breakpoints during debugging sessions.
> 
> I have found All Exceptions to be useful multiple times in the past.
> Certainly All Uncaught Exceptions is the most useful, but still All
> Exceptions has value. The Assertions breakpoints I agree is far less likely
> to be used and I like the idea of that being excluded by default.

FWIW, I use the Assertions breakpoint the most by far, because assertions are much more common than exceptions in Inspector code. I think all three have value.

[Joseph Pecoraro]

> > I have found All Exceptions to be useful multiple times in the past.
> > Certainly All Uncaught Exceptions is the most useful, but still All
> > Exceptions has value. The Assertions breakpoints I agree is far less likely
> > to be used and I like the idea of that being excluded by default.
> 
> FWIW, I use the Assertions breakpoint the most by far, because assertions
> are much more common than exceptions in Inspector code. I think all three
> have value.

Interesting. I use it as well when developing on Web Inspector but I don't think most web developers take advantage of `console.assert` like we do. Also, when I debug any production web page assertions are typically stripped (as we do in our own production) so I'd expect it to have less value outside of a pure development page.

[Blaze Burg]

My 2¢:

- I think all breakpoints should be deletable. I would love to hide the special breakpoints if I don't use them.
- I think the default set of breakpoints should include Pause on Uncaught Exception showing but disabled, so there is no change in the default state from shipped versions. Whether the breakpoint is actually deleted or just removed from UI doesn't matter.
- We have no real user data, so this is really just everyone saying what *they* do. I think we all understand that our usage is easiest to understand, and probably not representative of other users.

[Devin Rousso]

(In reply to Brian Burg from comment #27)
> - I think all breakpoints should be deletable. I would love to hide the
> special breakpoints if I don't use them.
I feel the same way (I never use "All Exceptions").

> - I think the default set of breakpoints should include Pause on Uncaught
> Exception showing but disabled, so there is no change in the default state
> from shipped versions. Whether the breakpoint is actually deleted or just
> removed from UI doesn't matter.
This is exactly how this patch works.  Just to clarify, if the user disables any of the "global" breakpoints, they don't disappear (they stay visible but are disabled).  That only happens when the user explicitly deletes the breakpoint.

[Devin Rousso]

Created attachment 350475 [details]
Patch

Change the All Exceptions and Uncaught Exceptions breakpoints to always be visible

[Devin Rousso]

Created attachment 350476 [details]
Patch

Whoops.  Uploaded from the wrong directory...without tests :|

[Joseph Pecoraro]

Comment on attachment 350476 [details]
Patch

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

r- because I'm encountering an issue with keyboard navigation.

Steps to reproduce:
1. Inspect webkit.org
2. Open a JavaScript resource
3. Set two breakpoints
4. Select All Exceptions in the debugger sidebar
5. Press down arrow many times
  => Never able to select the last breakpoint in the resource

> Source/WebInspectorUI/ChangeLog:18
> +         - Assertion Failures

I'd remove this one by default. I think its useful to us more than most developers.

> Source/WebInspectorUI/UserInterface/Controllers/XHRBreakpointTreeController.js:-58
> -        WI.Frame.removeEventListener(null, null, this);

Heh, nice.

> Source/WebInspectorUI/UserInterface/Controllers/XHRBreakpointTreeController.js:93
> +        let setting = null;
> +        if (treeElement.representedObject === WI.domDebuggerManager.allRequestsBreakpoint)
> +            setting = WI.settings.showAllRequestsBreakpoint;
> +
> +        if (setting)
> +            setting.value = !!treeElement.parent;

Seems this would read easier as:

    let breakpoint = treeElement.representedObject;
    if (breakpoint === WI.domDebuggerManager.allRequestsBreakpoint)
        WI.settings.showAllRequestsBreakpoint.value = !!treeElement.parent;

> Source/WebInspectorUI/UserInterface/Views/BreakpointDetailsSection.js:54
> +    showIfNotEmpty()
> +    {
> +        let hasBreakpoints = !!this._treeOutline.children.length;
> +
> +        if (!this._preventHide)
> +            this.element.hidden = !hasBreakpoints;

A function named "showIfNotEmpty" seems like it can hide?

Maybe this should be named something like `updateVisibility()`?

> Source/WebInspectorUI/UserInterface/Views/BreakpointTreeElement.js:85
> +        if (!WI.debuggerManager.isBreakpointRemovable(this._breakpoint)) {
> +            InspectorFrontendHost.beep();
> +            return true;
> +        }

Did this change behavior in any way? I was already hearing a beep when hitting delete on a non-removable breakpoint.

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.css:33
> +    content: url(../Images/Plus15.svg);

This icon looks a bit too dark compared to the other icons in the bottom left. It also might be a tad large. Compare to Xcode.

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:158
> +        let createBreakpointNavigationItem = new WI.ButtonNavigationItem("create-breakpoint", WI.UIString("Create a breakpoint"), "Images/Plus15.svg", 14, 14);
> +        createBreakpointNavigationItem.buttonStyle = WI.ButtonNavigationItem.Style.Image;
> +        createBreakpointNavigationItem.addEventListener(WI.ButtonNavigationItem.Event.Clicked, (event) => {
> +            this._handleCreateBreakpointClicked(event.data.nativeEvent);
> +        });
> +
> +        this._breakpointsSection = new WI.BreakpointDetailsSection("breakpoints", WI.UIString("Breakpoints"), this._breakpointsContentTreeOutline, {
> +            preventHide: true,
> +            emptyMessage: WI.createNavigationItemHelp(WI.UIString("Press %s to create a breakpoint."), createBreakpointNavigationItem),
> +        });

This is never reachable anymore since it will never be empty. Seems we could strip out a bunch of code.

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:278
> +        WI.Target.removeEventListener(null, null, this);

Nice!

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:1298
> +        let setting = null;
> +        if (treeElement.representedObject === WI.debuggerManager.assertionFailuresBreakpoint)
> +            setting = WI.settings.showAssertionFailuresBreakpoint;
>  
> -        this._domBreakpointsSection.collapsed = false;
> +        if (setting)
> +            setting.value = !!treeElement.parent;

Same here, this seems unnecessarily.

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:1364
>              if (breakpoint)
>                  WI.domDebuggerManager.addEventBreakpoint(breakpoint);
> +            else
> +                this._eventBreakpointsSection.showIfNotEmpty();

It seems weird to call this conditionally. It seems an `updateVisibility` would work here unconditionally.

> Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.css:41
> +.sidebar > .panel.navigation > .options > * {
> +    width: 100%;
> +}

It is weird to have to do this.

[Devin Rousso]

Created attachment 350840 [details]
Patch

[Devin Rousso]

Created attachment 350841 [details]
[Image] After Patch is applied

[EWS Watchlist]

Comment on attachment 350840 [details]
Patch

Attachment 350840 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9353124

New failing tests:
fast/workers/worker-exception-during-navigation.html

[EWS Watchlist]

Created attachment 350850 [details]
Archive of layout-test-results from ews203 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews203  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Joseph Pecoraro]

Comment on attachment 350840 [details]
Patch

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

r-me

> Source/WebInspectorUI/UserInterface/Views/BreakpointTreeElement.js:83
>          if (!WI.debuggerManager.isBreakpointRemovable(this._breakpoint))
> -            return false;
> +            return true;

What does this mean?

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.css:-117
> -.sidebar > .panel.navigation.debugger .details-section.dom-breakpoints .item.dom-node .icon {
> -    content: url(../Images/DOMElement.svg);
> -}

This was lost. DOM Node Breakpoints have no icon with this patch.

Steps to Reproduce:
1. Inspect this page
2. Show Elements Tab
3. Right Click <body> and "Break on..." > Subtree Modified
4. Show Debugger Tab
  => DOM Node breakpoint missing icon.

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:153
> +        let createBreakpointButton = new WI.ButtonNavigationItem("create-breakpoint", WI.UIString("Create Breakpoint"), "Images/Plus13.svg", 13, 13);

This button should be a tad gray instead of black. File a follow-up?

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:273
> +        if (this._eventBreakpointTreeController) {
> +            this._eventBreakpointTreeController.disconnect();
> +            this._eventBreakpointTreeController = null;
> +        }
> +
> +        if (this._xhrBreakpointTreeController) {
> +            this._xhrBreakpointTreeController.disconnect();
> +            this._xhrBreakpointTreeController = null;
> +        }

Dead code, remove.

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:534
> +            var pauseReasonBreakpointTreeElement = this._pauseReasonTreeOutline.getCachedTreeElement(breakpoint);

Style: let

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:539
> +        var breakpointTreeElement = this._breakpointsContentTreeOutline.getCachedTreeElement(breakpoint);

Style: let

> Source/WebInspectorUI/UserInterface/Views/EventBreakpointTreeElement.js:39
> -        let subtitle = null;
> -        if (linkifyNode && breakpoint.eventListener)
> -            subtitle = WI.linkifyNodeReference(breakpoint.eventListener.node);
> -
> +        const subtitle = null;

Where did this go?

[Devin Rousso]

Comment on attachment 350840 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/BreakpointTreeElement.js:83
>> +            return true;
> 
> What does this mean?

`WI.TreeOutline.prototype._treeKeyDown` uses the return value to determine whether `ondelete` was "handled".  In this case, we don't want to defer to the `WI.TreeOutline`'s `ondelete`, since `WI.DebuggerSidebarPanel` expects the `WI.TreeElement` to be a resource/script, not a breakpoint.  Furthermore, as a matter of technicality, we have "handled" the event here, but we've just decided to do nothing.

>> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.css:-117
>> -}
> 
> This was lost. DOM Node Breakpoints have no icon with this patch.
> 
> Steps to Reproduce:
> 1. Inspect this page
> 2. Show Elements Tab
> 3. Right Click <body> and "Break on..." > Subtree Modified
> 4. Show Debugger Tab
>   => DOM Node breakpoint missing icon.

Ah whoops.  Looks like webkit-patch didn't include any of the files I created for this patch :(

>> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:153
>> +        let createBreakpointButton = new WI.ButtonNavigationItem("create-breakpoint", WI.UIString("Create Breakpoint"), "Images/Plus13.svg", 13, 13);
> 
> This button should be a tad gray instead of black. File a follow-up?

Can you provide an example of where elsewhere this is the case?  I can fix it here.

>> Source/WebInspectorUI/UserInterface/Views/EventBreakpointTreeElement.js:39
>> +        const subtitle = null;
> 
> Where did this go?

One change made in this patch is that event breakpoints that are specific to a node are now listed under that node, meaning there's no reason to have a separate linkified reference for that specific breakpoint.  You can see this for the "click" event listener under <button> in <https://bugs.webkit.org/attachment.cgi?id=350841>.

[Devin Rousso]

Created attachment 350945 [details]
Patch

[Devin Rousso]

Created attachment 350946 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 350946 [details]
Patch

Clearing flags on attachment: 350946

Committed r236540: <https://trac.webkit.org/changeset/236540>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.