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

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> 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 350476: Patch

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




--- Comment #31 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 350476
  --> https://bugs.webkit.org/attachment.cgi?id=350476
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.C
licked, (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.