Bug 165211 - Web Inspector: Debugger and Console improvements
Summary: Web Inspector: Debugger and Console improvements
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Matt Baker
URL:
Keywords: InRadar
: 144742 (view as bug list)
Depends on: 164047 165235
Blocks:
  Show dependency treegraph
 
Reported: 2016-11-30 12:21 PST by Matt Baker
Modified: 2017-06-19 13:58 PDT (History)
5 users (show)

See Also:


Attachments
[Image] New UI - not paused (281.79 KB, image/png)
2016-11-30 12:27 PST, Matt Baker
no flags Details
[Image] New UI - paused (150.98 KB, image/png)
2016-11-30 13:29 PST, Matt Baker
no flags Details
[Image] New UI - async call frames (109.15 KB, image/png)
2016-11-30 13:34 PST, Matt Baker
no flags Details
[Video] UI changes (1.31 MB, video/mp4)
2016-12-08 16:14 PST, Matt Baker
no flags Details
[Video] Console drawer behavior (1001.96 KB, video/mp4)
2016-12-08 16:23 PST, Matt Baker
no flags Details
[Video] Async call frames behavior (734.08 KB, video/mp4)
2016-12-08 16:46 PST, Matt Baker
no flags Details
Patch (87.88 KB, patch)
2016-12-09 02:41 PST, Matt Baker
joepeck: review-
Details | Formatted Diff | Diff
[Image] Debugger UI setting (experimental) (243.91 KB, image/png)
2017-06-19 13:58 PDT, Matt Baker
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Baker 2016-11-30 12:21:27 PST
Console:
- Create a ConsoleDrawer class for encapsulating split console logic currently in Main.js
- Replace icon in console drawer toolbar with a show/hide button (match style in Xcode)
- Remove "chevron" button from console drawer that shows console tab.
  - Seldom used, takes up toolbar space

Debugger:
- Make debugging possible with left-sidebar collapsed:
  - Move stepping controls to console drawer toolbar
  - Add call stack navigation item to console drawer toolbar
- Auto-expand console drawer when debugging starts
- Collapsing console drawer while debugging should not hide the toolbar (match style in Xcode)
Comment 1 Radar WebKit Bug Importer 2016-11-30 12:21:51 PST
<rdar://problem/29443975>
Comment 2 Matt Baker 2016-11-30 12:27:10 PST
Created attachment 295742 [details]
[Image] New UI - not paused
Comment 3 Matt Baker 2016-11-30 13:29:35 PST
Created attachment 295751 [details]
[Image] New UI - paused
Comment 4 Matt Baker 2016-11-30 13:34:38 PST
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.
Comment 5 Matt Baker 2016-12-08 16:14:54 PST
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
Comment 6 Matt Baker 2016-12-08 16:23:01 PST
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
Comment 7 Matt Baker 2016-12-08 16:46:29 PST
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.
Comment 8 Matt Baker 2016-12-09 02:41:16 PST
Created attachment 296648 [details]
Patch
Comment 9 Nikita Vasilyev 2016-12-11 20:48:48 PST
*** Bug 144742 has been marked as a duplicate of this bug. ***
Comment 10 Joseph Pecoraro 2016-12-12 14:37:30 PST
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.
Comment 11 Joseph Pecoraro 2016-12-12 14:39:06 PST
> 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!
Comment 12 Matt Baker 2017-06-19 13:58:39 PDT
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.