Bug 183420 – Web Inspector: [META] Merge Resources and Debugger into a single Sources tab

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Nikita Vasilyev]

At a high level:
— The resources tree stays in the left sidebar;
— Most debugger controls move to the right sidebar (Pause/Resume buttons, Call stack, and Variables);
— The breakpoints list move to a separate panel of the left sidebar.

<rdar://problem/35298960>

[Devin Rousso]

Created attachment 355499 [details]
[Image] After Patch is applied (A)

[Devin Rousso]

Created attachment 355501 [details]
[Image] After Patch is applied (B)

[Devin Rousso]

So I've been working on this for the past few days (mainly because I'm so fed up with having to switch between Resources and Debugger whenever I want to interact with breakpoints, especially after switching files with the open resource dialog), and I've created two versions of this:

(In reply to Devin Rousso from comment #1)
> Created attachment 355499 [details]
> [Image] After Patch is applied (A)
Moves the debugger action buttons to the console drawer (matches Xcode), and the drawer is automatically shown whenever the debugger pauses.

(In reply to Devin Rousso from comment #2)
> Created attachment 355501 [details]
> [Image] After Patch is applied (B)
Moves the resource type scope bar to be lower inside the navigation sidebar (similar to the canvas sidebar recording scope bar), meaning that it only applies filtering to the resources list.

I personally prefer (B), as that seems to be a more "familiar" approach the layout of the Resources and Debugger tab navigation sidebars (while testing (A) I often found myself subconsciously moving my mouse to click on the debugger action buttons in the top left, even though they had been moved to the console drawer).  In the future, we may want to (also) add the debugger action buttons to the console drawer (or even the console tab) as a way for the developer to interact when paused.

[Devin Rousso]

Created attachment 355521 [details]
Patch

This is the patch for version (B).

[EWS Watchlist]

Comment on attachment 355521 [details]
Patch

Attachment 355521 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10122580

New failing tests:
imported/w3c/web-platform-tests/webrtc/simplecall.https.html

[EWS Watchlist]

Created attachment 355539 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Devin Rousso]

Created attachment 355739 [details]
Patch

[Joseph Pecoraro]

> > [Image] After Patch is applied (A)
>
> Moves the debugger action buttons to the console drawer (matches Xcode), and
> the drawer is automatically shown whenever the debugger pauses.

Where are the debugger controls when the console drawer is collapsed? I recall Matt Baker had a version that added a navigation bar above the quick console with the debugger controls, which felt pretty good.

[Devin Rousso]

(In reply to Joseph Pecoraro from comment #8)
> > > [Image] After Patch is applied (A)
> >
> > Moves the debugger action buttons to the console drawer (matches Xcode), and
> > the drawer is automatically shown whenever the debugger pauses.
> 
> Where are the debugger controls when the console drawer is collapsed? I
> recall Matt Baker had a version that added a navigation bar above the quick
> console with the debugger controls, which felt pretty good.
When the console is collapsed, the controls aren't visible, but the console is automatically expanded whenever the debugger pauses.  If the user chooses to collapse the console while paused, the controls would no longer be visible (this is similar to the sidebar being collapsed while paused).

I focused on building version (B) as I felt it was the least "change" from what is currently expected from Resources/Debugger, and I felt that the initial change should be to make sure it works (we can rework the UI after the initial change).

[Devin Rousso]

Created attachment 358159 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 358159 [details]
Patch

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

For the brief amount of time I played with this it felt fine. I like that the debugger controls haven't moved right now, since that would be the most jarring. But overall this is great. It needs to be rebaselined for the XHRBreakpoint -> URLBreakpoint rename. And a few small questions, but otherwise this looks good given it is behind an experimental flag.

> Source/WebInspectorUI/UserInterface/Base/Main.js:1042
> +}

Style: Missing semicolon.

> Source/WebInspectorUI/UserInterface/Base/Main.js:1492
> +    if (WI.settings.experimentalEnableSourcesTab.value)
> +        WI.showSourcesTab({showScopeChainSidebar: WI.settings.showScopeChainOnPause.value});
> +    else
> +        WI.showDebuggerTab({showScopeChainSidebar: WI.settings.showScopeChainOnPause.value});

You could just make `showDebuggerTab` redirect to `showSourcesTab` when the setting is set.

> Source/WebInspectorUI/UserInterface/Protocol/InspectorFrontendAPI.js:106
> +        if (WI.settings.experimentalEnableSourcesTab.value)
> +            WI.showSourcesTab();
> +        else
> +            WI.showResourcesTab();
> +

Same you could make `showResourcesTab` redirect to `showSourcesTab` when the setting is enabled.

> Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:754
> +WI.NavigationSidebarPanel.IgnoreCookieRestoration = Symbol("ignore-cookie-restoration");

This deserves a comment. Why are we choosing to ignore restoration?

> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:276
> +        if (WI.SourcesNavigationSidebarPanel.shouldPlaceResourcesAtTopLevel()) {

We may want to test this with a JSContext with named resources (ask me about JSContextTester)

> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:299
> +        if (WI.networkManager.mainFrame)
> +            this._updateMainFrameTreeElement(WI.networkManager.mainFrame);
> +
> +        for (let target of WI.targets)
> +            this._addTarget(target);
> +
> +        this._updateCallStackTreeOutline();
> +
> +        if (WI.networkManager.mainFrame)
> +            this._addResourcesRecursivelyForFrame(WI.networkManager.mainFrame);

Two instances of updating things from the mainFrame, very weird. This whole initial population has aways been somewhat problematic for us, but I suspect this is just mirroring the existing sidebar panels.

> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:499
> +            // FIXME: handle breakloints/issues

So is the filter going to filter both sidebar tree outlines?

> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:1697
> +WI.SourcesNavigationSidebarPanel.DebuggerPausedStyleClassName = "paused";
> +WI.SourcesNavigationSidebarPanel.ExceptionIconStyleClassName = "breakpoint-exception-icon";
> +WI.SourcesNavigationSidebarPanel.AssertionIconStyleClassName = "breakpoint-assertion-icon";
> +WI.SourcesNavigationSidebarPanel.PausedBreakpointIconStyleClassName = "breakpoint-paused-icon";

We should be able to eliminate these now and inline them, unless they are used outside of this class.

[Devin Rousso]

Comment on attachment 358159 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Base/Main.js:1492
>> +        WI.showDebuggerTab({showScopeChainSidebar: WI.settings.showScopeChainOnPause.value});
> 
> You could just make `showDebuggerTab` redirect to `showSourcesTab` when the setting is set.

I find that that would make it harder to remove later, not to mention that it might cause issues if we changed `showDebuggerTab` to do something different.

>> Source/WebInspectorUI/UserInterface/Protocol/InspectorFrontendAPI.js:106
>> +
> 
> Same you could make `showResourcesTab` redirect to `showSourcesTab` when the setting is enabled.

Ditto.

>> Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:754
>> +WI.NavigationSidebarPanel.IgnoreCookieRestoration = Symbol("ignore-cookie-restoration");
> 
> This deserves a comment. Why are we choosing to ignore restoration?

This ensures that the breakpoints list isn't used when restoring selection (e.g. after a reload).  The main source of selection will come from the list of sources.

>> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:499
>> +            // FIXME: handle breakloints/issues
> 
> So is the filter going to filter both sidebar tree outlines?

Text-based filtering will affect both, but the scope bar will only affect the resources list (we early return on >474).

[Devin Rousso]

Created attachment 362223 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 362223 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:37
> +        let createNavigationItem = (constructor, {identifier, defaultToolTip, alternateToolTip, defaultImage, alternateImage, imageWidth, imageHeight}) => {
> +            let args = [identifier];

I don't find this clearer.

  • I'd rather be able to search the codebase for 'new WI.ToggleButtonNavigationItem' and be able to find the debuggerPauseResumeButtonItem.
  • This is brittle if the order of arguments, or if a new argument gets inserted in the middle of one of these NavigationItem constructors

> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:268
> +        WI.timelineManager.addEventListener(WI.TimelineManager.Event.CapturingWillStart, this._handleTimelineCapturingWillStart, this);
> +        WI.timelineManager.addEventListener(WI.TimelineManager.Event.CapturingStopped, this._handleTimelineCapturingStopped, this);

These _handle prefixes are so much harder to read for me, they just become a wash of handles =(

> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:580
> +        // FIXME: When WI.mainTarget changes?
> +        if (target === WI.mainTarget)
> +            this._mainTargetTreeElement = treeElement;

I think this behaves correctly.

During a Page Transition, (when WI.pageTarget changes, which will change WI.mainTarget) you'll get a new `WI.TargetManager.Event.TargetAdded` event with the new WI.pageTarget, and this condition will be true for the new main target.

A possible user scenario would be:
1. Inspect apple.com
2. Navigate to webkit.org
3. Pause in the debugger
  => Make sure the call frames shows the compact view and not expanded view when there are multiple threads/workers

> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:622
> +    _addScript(script)

(I made it to this point and then got hungry)

[Joseph Pecoraro]

Comment on attachment 362223 [details]
Patch

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

rs=me

>> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:622
>> +    _addScript(script)
> 
> (I made it to this point and then got hungry)

I recognize this code is mostly migrating from Resources/Debugger. I'd love a fresh approach to building up the sidebar reliably from the top level Targets, but this is good for now!

> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:695
> +        let targetTreeElement = new WI.WorkerTreeElement(target);
> +        this._workerTargetTreeElementMap.set(target, targetTreeElement);

I think it is fine to name this `this._workerTargeTreeElementMap` now, but in concept this should really be for all targets so might go back to being `this._targetTreeElementMap` in the future. Better to name is specifically now so we can catch when we change it easily.

> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:1442
> +    _handleDebuggerScriptsCleared(event)

This seems like it is doing a lot of work, removing resources 1 at a time and reordering everything... If debugger scripts cleared isn't it basically removing all scripts / resources?!

Room for optimizations in the future!

> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:1448
> +        for (var i = this._breakpointsTreeOutline.children.length - 1; i >= 0; --i) {
> +            var treeElement = this._breakpointsTreeOutline.children[i];

Style: Final `var`s to remove!

[Devin Rousso]

Comment on attachment 362223 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:37
>> +            let args = [identifier];
> 
> I don't find this clearer.
> 
>   • I'd rather be able to search the codebase for 'new WI.ToggleButtonNavigationItem' and be able to find the debuggerPauseResumeButtonItem.
>   • This is brittle if the order of arguments, or if a new argument gets inserted in the middle of one of these NavigationItem constructors

I was trying to avoid super long constructor lines (or a lot of local variables).  I'll rework it a bit to make it nicer.

>> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:268
>> +        WI.timelineManager.addEventListener(WI.TimelineManager.Event.CapturingStopped, this._handleTimelineCapturingStopped, this);
> 
> These _handle prefixes are so much harder to read for me, they just become a wash of handles =(

I much prefer `_handle*`.  I find it makes things immediately clear (down below, in the actual function definitions) what things are expecting events and what things aren't.

>>> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:622
>>> +    _addScript(script)
>> 
>> (I made it to this point and then got hungry)
> 
> I recognize this code is mostly migrating from Resources/Debugger. I'd love a fresh approach to building up the sidebar reliably from the top level Targets, but this is good for now!

My plan for this was to get something working, and then go back and iterate/fix on issues that may arise.

> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:1452
> +            this._breakpointsTreeOutline.removeChildAtIndex(i, suppressOnDeselect, suppressSelectSibling);

Rather than remove by index, we could also just call `removeChild` with `treeElement` (currently this is a small optimization, but it makes it slightly harder to read/understand).

[Devin Rousso]

Created attachment 362875 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 362875 [details]
Patch

Clearing flags on attachment: 362875

Committed r242049: <https://trac.webkit.org/changeset/242049>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.