Web Inspector: no discoverable way to dismiss the split console

[BJ Burg]

You have to randomly hit ESC, or click the Show console tab icon and switch back, to get rid of the split console. Maybe we could make the Show Console Tab have a chevron pointing down next to it, or we could add a "hide debug area" style button like Xcode on the left edge of the split console navigation bar.

[Radar WebKit Bug Importer]

<rdar://problem/30023436>

[Matt Baker]

This is covered by https://bugs.webkit.org/show_bug.cgi?id=165211, which adds a expand/collapse button (styled after Xcode's debugging console).

[Matt Baker]

I'll break this change out from the WIP patch I have for the above bug.

[Matt Baker]

Created attachment 313430 [details]
Patch

[Nikita Vasilyev]

Screenshot?

[Matt Baker]

Created attachment 313438 [details]
[Image] Dismiss button

This is a toggle, so it has a "Show Console" state as well. This is unused now, but I'd like to leave it in for future use.

[Nikita Vasilyev]

Comment on attachment 313430 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Images/HideConsoleDrawer.svg:2
> +<!-- Copyright © 2016 Apple Inc. All rights reserved. -->

2017.

> Source/WebInspectorUI/UserInterface/Images/ShowConsoleDrawer.svg:2
> +<!-- Copyright © 2016 Apple Inc. All rights reserved. -->

2017.

[Build Bot]

Comment on attachment 313430 [details]
Patch

Attachment 313430 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3968205

New failing tests:
inspector/canvas/create-canvas-contexts.html

[Build Bot]

Created attachment 313447 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

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

[Devin Rousso]

Comment on attachment 313430 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Base/Main.js:-302
> -    this._consoleTreeElement = new WebInspector.LogTreeElement(this._consoleRepresentedObject);

IIRC, LogTreeElement was created to help with restoring the console tab when reopening the inspector (honestly could be wrong here).  Does this still work?

> Source/WebInspectorUI/UserInterface/Base/Main.js:1597
> +    this._showingSplitConsoleSetting.value = !this.consoleDrawer.collapsed;

NIT: you can use the helper from above:

    this._showingSplitConsoleSetting.value = WebInspector.isShowingSplitConsole();

> Source/WebInspectorUI/UserInterface/Views/ConsoleDrawer.js:30
> +        super(element, null, true, false);

NIT: I think it would be nice to have these constant arguments be named:

    const delegate = null;
    const disableBackForward = true;
    const disableFindBanner = false;
    super(element, delegate, disableBackForward, disableFindBanner);

> Source/WebInspectorUI/UserInterface/Views/ConsoleDrawer.js:42
> +        this.collapsed = true;

Instead of having "_showingSplitConsoleSetting" be on WebInspector, why not move it to this class and ditch much of the event logic for updating it's value.  It would also help avoid unnecessary className churn, since you assume it to be collapsed here and it might not be via the setting.

> Source/WebInspectorUI/UserInterface/Views/ConsoleDrawer.js:69
> +        this.dispatchEventToListeners(WebInspector.ConsoleDrawer.Event.Resized);

I don't really think this makes sense.  Personally, I see the drawer collapsing as more of a "disappear" than a "resized to 0".  I would rather the listeners for CollapsedStateChange understand that collapsing means it changed size than have this event be fired.

> Source/WebInspectorUI/UserInterface/Views/ConsoleDrawer.js:87
> +    layout()

NIT: please call super.layout() just in case we need to do something with ContentBrowser.prototype.layout or View.prototype.layout in the future.

> Source/WebInspectorUI/UserInterface/Views/ConsoleDrawer.js:95
> +        let oldHeight = this.element.offsetHeight;

NIT: you can just call this.height

    let oldHeight = this.height;

> Source/WebInspectorUI/UserInterface/Views/ConsoleDrawer.js:146
> +        if (height === this.element.style.height)

Would it be better to compare to this.height?

    if (height === this.height)

> Source/WebInspectorUI/UserInterface/Views/ConsoleDrawer.js:149
> +        this.element.style.height = height + "px";

NIT: template string

    this.element.style.height = `${height}px`;

> Source/WebInspectorUI/UserInterface/Views/QuickConsole.js:342
> +        let showingLog = WebInspector.isShowingConsoleTab() ? true : !WebInspector.consoleDrawer.collapsed;
> +        this.element.classList.toggle("showing-log", showingLog);

I think this can be simplified:

    this.element.classList.toggle("showing-log", WebInspector.isShowingConsoleTab() || WebInspector.isShowingSplitConsole());

[Matt Baker]

Comment on attachment 313430 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Base/Main.js:-302
>> -    this._consoleTreeElement = new WebInspector.LogTreeElement(this._consoleRepresentedObject);
> 
> IIRC, LogTreeElement was created to help with restoring the console tab when reopening the inspector (honestly could be wrong here).  Does this still work?

It works, Main.js restores the selected tab from `this._selectedTabIndexSetting`.

>> Source/WebInspectorUI/UserInterface/Views/ConsoleDrawer.js:42
>> +        this.collapsed = true;
> 
> Instead of having "_showingSplitConsoleSetting" be on WebInspector, why not move it to this class and ditch much of the event logic for updating it's value.  It would also help avoid unnecessary className churn, since you assume it to be collapsed here and it might not be via the setting.

WebInspector manages the console's collapsed state, since the selected tab may not support the split console. We could revisit this in the future.

>> Source/WebInspectorUI/UserInterface/Views/ConsoleDrawer.js:149
>> +        this.element.style.height = height + "px";
> 
> NIT: template string
> 
>     this.element.style.height = `${height}px`;

I don't think there is much of a benefit for such simple string concatenations.

[Matt Baker]

Created attachment 313548 [details]
Patch

[Devin Rousso]

Comment on attachment 313548 [details]
Patch

r=me.  Awesome work!

[WebKit Commit Bot]

Comment on attachment 313548 [details]
Patch

Clearing flags on attachment: 313548

Committed r218839: <http://trac.webkit.org/changeset/218839>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.