[webkit-reviews] review denied: [Bug 182406] Web Inspector: Hide DOM and XHR breakpoint sections when they are empty : [Attachment 348395] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has denied Devin Rousso <drousso at apple.com>'s
request for review:
Bug 182406: Web Inspector: Hide DOM and XHR breakpoint sections when they are
empty
https://bugs.webkit.org/show_bug.cgi?id=182406

Attachment 348395: Patch

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




--- Comment #5 from Brian Burg <bburg at apple.com> ---
Comment on attachment 348395
  --> https://bugs.webkit.org/attachment.cgi?id=348395
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.