Bug 165211

<rdar://problem/29443975>

Created attachment 295742 [details] [Image] New UI - not paused

Created attachment 295751 [details] [Image] New UI - paused

Created attachment 295752 [details] [Image] New UI - async call frames The hierarchical path navigation item for the call stack includes async call frames. Boundary frame menu items (requestAnimationFrame, setTimeout, etc) are not selectable (appear slightly dimmed), and have m-dashes in the title to set them apart. Selecting an async call frame using the menu shows the source code location, but does not change the active call frame.

Created attachment 296586 [details] [Video] UI changes Changes: - Debugging controls moved from sidebar to console toolbar - Expand / collapse toggle button added to left of console toolbar, replacing the triple-chevron expand button previously found on the right - Call frames shown in console toolbar

Created attachment 296589 [details] [Video] Console drawer behavior When debugging: - Pausing in the debugger automatically expands the console* - Collapsing the console hides the log but debugging controls remain visible - The console can be expanded by dragging up on the resizer - Resuming the debugger while the console is collapsed (debugging controls visible) will completely collapse the console - Switching to a tab that doesn't support the split console hides debugging controls * not shown in video

Created attachment 296592 [details] [Video] Async call frames behavior Async call frames can be selected from the console toolbar's Call Frames navigation item. As in the debugger sidebar, selecting an async frame doesn't change the active call frame. Async boundary call frames are also shown in the context menu, but are disabled (gray text, not selectable) and surround by em-dashes to approximate the style used in the sidebar.

Created attachment 296648 [details] Patch

*** Bug 144742 has been marked as a duplicate of this bug. ***

Comment on attachment 296648 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=296648&action=review Patch looks pretty good. After playing with it I have some high level comments that I think warrant another version of the patch. It is weird that the debugger controls are available everywhere except the Console tab. Especially given the Console tab's navigation bar looks like the ConsoleDrawer's bar, so I'd expect the debugger controls to be there too. Putting then in the Console tab would mean the debugger controls really are available everywhere. I would expect the debugger controls to be available everywhere the Quick Console is. That means all tabs, including the Timeline, Network, and NewTab tabs. In fact on those tabs right now there is a display issue where there is no top border on the quick console. When clicking the stepping controls the Debugger sidebar flashes its selection. The selected call frame goes gray (unselected) and then blue again after stepping. This is very distracting. This does match Xcode, but it is a regression from our existing behavior. When pausing, if the ConsoleDrawer was closed, the ConsoleDrawer opens unexpectedly. Why not just keep it in the minimal size stacked on top of the Quick Console? That is what I would expect, otherwise I will have to be constantly or re-closing or adjusting the size of my ConsoleDrawer because it keeps opening without me wanting it open. I think the initial version has to do something about the navigation bar causing ellipsis text of the call frames. Otherwise, 99% of the time I have cut off text in the Console for these new elements. This makes the UI feel broken. > Source/WebInspectorUI/ChangeLog:39 > + * UserInterface/Images/HideConsoleDrawer.svg: Added. > + * UserInterface/Images/ShowConsoleDrawer.svg: Added. > + Console drawer "toggle" images. Styled after corresponding Xcode button. What is the story for our linux ports? Images/gtk > Source/WebInspectorUI/UserInterface/Base/Main.js:1312 > + this.showSplitConsole(); I don't think we should be opening the console. Just showing the controls would be enough. > Source/WebInspectorUI/UserInterface/Base/Main.js:1580 > + this.hideSplitConsole(); This should still be showing the debugger controls even if the current tab doesn't support a split content browser. > Source/WebInspectorUI/UserInterface/Controllers/CallStackTreeOutlineController.js:46 > + } This needs to initialize its initial state (create TargetTreeElements for current list of targets and set active call frame tree element) or otherwise assert that this object is never created while we are paused and that there is only one target. Maybe this can affect if you close and open the debugger tab while paused? Just adding these asserts would be enough to catch potential issues: console.assert(WebInspector.targets.size === 1); console.assert(!WebInspector.debuggerManager.paused); > Source/WebInspectorUI/UserInterface/Controllers/CallStackTreeOutlineController.js:74 > + this._activeCallFrameTreeElement.select(); Why is this select() being added? Is this causing the distracting deselect/select in the sidebar when stepping? > Source/WebInspectorUI/UserInterface/Controllers/CallStackTreeOutlineController.js:93 > +WebInspector.CallStackTreeOutlineController.ControllerSymbol = Symbol("controller"); Lets name this slightly better so that if we see it in the Console we know it is the "call-stack-controller" or something. > Source/WebInspectorUI/UserInterface/Images/HideConsoleDrawer.svg:5 > + <rect stroke="currentColor" fill="none" x="1.5" y="2.5" width="13" height="11"/> > + <path fill="none" stroke="currentColor" d="M 4.5 6.5 L 8 10 L 11.5 6.5 Z"/> Style: Lets keep the order of fill and stroke the same for both of these. It'll make it easier to read. > Source/WebInspectorUI/UserInterface/Views/ConsoleDrawer.css:70 > +@media (-webkit-min-device-pixel-ratio: 2) { > + .console-drawer > .navigation-bar .item.divider { > + width: 1px; > + } > +} Why differ from the normal 0.5px here? > Source/WebInspectorUI/UserInterface/Views/ConsoleDrawer.js:52 > + if (WebInspector.debuggerManager.paused) > + this._debuggerDidPause(null); Style: I'd prefer we register all event listeners and then update initial view style at the end. I think that more closely matches our usual patterns. Especially in constructors (setup everything, update display). It avoids the possibility that later somebody might update _debuggerDidPause or _timelineCapturingWillStart in such a way that uses members that aren't initialized yet (like callStackTreeOutlineController. In this case, I don't think we can ever create a ConsoleDrawer when the Debugger is paused. Since this is a singleton created when the frontend is opened. > Source/WebInspectorUI/UserInterface/Views/ConsoleDrawer.js:58 > + if (WebInspector.timelineManager.isCapturing() && WebInspector.debuggerManager.breakpointsDisabledTemporarily) > + this._timelineCapturingWillStart(null); Likewise, I don't think this is possible. > Source/WebInspectorUI/UserInterface/Views/ConsoleDrawer.js:60 > + this._callStackTreeOutlineController = new WebInspector.CallStackTreeOutlineController(this._callStackTreeOutline); Style: Move this up > Source/WebInspectorUI/UserInterface/Views/ConsoleDrawer.js:91 > + if (!paused) > + this.element.classList.add("hidden"); Should this go inside of hidden? Is it possible for someone else to call hidden other than this? > Source/WebInspectorUI/UserInterface/Views/ConsoleDrawer.js:99 > + get height() { return this.element.offsetHeight; } Style: Lets not one line this. It is not trivial, it causes a forced layout. > Source/WebInspectorUI/UserInterface/Views/ConsoleDrawer.js:101 > + shown() Style: These should go in the Protected section. They call super. > Source/WebInspectorUI/UserInterface/Views/ConsoleDrawer.js:166 > + if (oldHeight !== this.element.offsetHeight) > + this.dispatchEventToListeners(WebInspector.ConsoleDrawer.Event.Resized); If you move this to `_updateDrawerHeight` you could simplify dispatching this event. You could avoid a possible forced layout here checking offsetHeight after editing it. > Source/WebInspectorUI/UserInterface/Views/ConsoleDrawer.js:189 > + this._debuggerPauseResumeButtonItem.enabled = true; > + this._debuggerPauseResumeButtonItem.toggled = true; > + this._debuggerStepOverButtonItem.enabled = true; > + this._debuggerStepIntoButtonItem.enabled = true; This is missing `this._debuggerStepOutButtonItem.enabled = true;` > Source/WebInspectorUI/UserInterface/Views/ConsoleDrawer.js:315 > + if (event.button !== 0) > + return; I don't think the button matters for the drag, only the position of the mouse. We already won't get into this code unless the proper button is down. > Source/WebInspectorUI/UserInterface/Views/ConsoleDrawer.js:322 > + this.dispatchEventToListeners(WebInspector.ConsoleDrawer.Event.Resized); If this was inside of of _updateDrawerHeight you would not need to dispatch it here, which would be good because if the user is already at the Max or Min then we don't need to be dispatching this event because the UI is not changing. > Source/WebInspectorUI/UserInterface/Views/ConsoleDrawer.js:348 > + }; Style: Remove this semicolon. > Source/WebInspectorUI/UserInterface/Views/ConsoleDrawer.js:374 > + Resized: "console-drawer-resized" Style: Add the trailing comma. It makes future diffs easier to read. > Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:-183 > + this._callStackTreeOutline.addEventListener(WebInspector.TreeOutline.Event.ElementAdded, this._treeElementAddedOrRemoved, this); > + this._callStackTreeOutline.addEventListener(WebInspector.TreeOutline.Event.ElementRemoved, this._treeElementAddedOrRemoved, this); > > - this._mainTargetTreeElement = new WebInspector.ThreadTreeElement(WebInspector.mainTarget); > - this._callStackTreeOutline.appendChild(this._mainTargetTreeElement); > - > - this._updateCallStackTreeOutline(); I think calling `updateCallStackTreeOutline` on every modification seems excessive. It only cares if we go between 1 to non-1 targets, and never cares about call frame changes. I think it could be much cheaper by just listening for target added / removed events and keeping this original _updateCallStackTreeOutline to set the proper initial state. > Source/WebInspectorUI/UserInterface/Views/DividerNavigationItem.js:31 > - constructor(identifier) > + constructor() > { > - super(identifier); > + super(); > } Style: Looks like you can remove the constructor. It is the default constructor. > Source/WebInspectorUI/UserInterface/Views/GeneralTreeElementPathComponent.js:34 > - generalTreeElement.addEventListener(WebInspector.GeneralTreeElement.Event.MainTitleDidChange, this._mainTitleDidChange, this); > + generalTreeElement.addEventListener(WebInspector.GeneralTreeElement.Event.MainTitleDidChange, () => { this._updateDisplayName(); }); And reason for this change? The original code looks identical if not simpler since it doesn't create a new function per-object. > Source/WebInspectorUI/UserInterface/Views/HierarchicalPathComponent.js:245 > + get selectable() { return this._selectable; } > + set selectable(flag) { this._selectable = flag; } Lets put these next to other flags (hidden, collapsed) > Source/WebInspectorUI/UserInterface/Views/HierarchicalPathComponent.js:270 > + if (!component.selectable) > + optionElement.disabled = "disabled"; Style: A boolean assignment works fine here. `optionElement.disabled = !component.selectable;` > Source/WebInspectorUI/UserInterface/Views/PathComponentIcons.css:108 > +.thread .icon { > + content: url(../Images/Thread.svg); > +} Style: Lets not put this between the snapshot ones and the FIXME for snapshot on Linux. Putting this after program-icon seems fine. Something I did that is a bit unusual is the icon is 16x16 but I sized it 15x15 in the sidebar. Otherwise it looks too large alongside the square icons. It still looks too large to my eyes. Making it 15x15 here looks better to me. Although it should probably also be moved down 0.5px on Retina. I'll let you determine what is best. > Source/WebInspectorUI/UserInterface/Views/QuickConsole.css:49 > +.quick-console:not(.showing-log).paused { > + border-top: none; > +} This seems to be causing no border to show up on tabs that "disallow" split console. I think this can go away and we would just have a min-size drawer in those cases. > Source/WebInspectorUI/UserInterface/Views/QuickConsole.js:82 > + WebInspector.ConsoleDrawer.addEventListener(WebInspector.ConsoleDrawer.Event.CollapsedStateChanged, this._updateStyles, this); Maybe put this even on the WebInspector.consoleDrawer instance? We don't normally use the class unless there is a good reason. > Source/WebInspectorUI/UserInterface/Views/QuickConsole.js:352 > + else if (WebInspector.consoleDrawer) Is this really needed? When is consoleDrawer null here? And if it is, can we just initialize ConsoleDrawer before QuickConsole? > Source/WebInspectorUI/UserInterface/Views/QuickConsole.js:361 > WebInspector.QuickConsole.ToolbarSingleLineHeight = 21; > WebInspector.QuickConsole.ToolbarPromptPadding = 4; > WebInspector.QuickConsole.ToolbarTopBorder = 1; We can remove these, they appear to be unused. > Source/WebInspectorUI/UserInterface/Views/TreeElement.js:374 > + if (!this._collapsible) > + return; Does collapsible need to be checked in expand? Seems we should always allow expanding something even if it is not collapsible.

> It is weird that the debugger controls are available everywhere except the > Console tab. Especially given the Console tab's navigation bar looks like > the ConsoleDrawer's bar, so I'd expect the debugger controls to be there > too. Putting then in the Console tab would mean the debugger controls really > are available everywhere. We could actually discuss different behaviors for where the debugger controls should go in the Console tab. It even could stay stuck to the QuickConsole!

Created attachment 313329 [details] [Image] Debugger UI setting (experimental) This patch introduces a drastic change to the debugger UI. It should go behind an experimental flag of now.