Bug 183420 - Web Inspector: [META] Merge Resources and Debugger into a single Sources tab
Summary: Web Inspector: [META] Merge Resources and Debugger into a single Sources tab
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on: 183316 183317
Blocks:
  Show dependency treegraph
 
Reported: 2018-03-07 15:50 PST by Nikita Vasilyev
Modified: 2019-02-25 11:09 PST (History)
6 users (show)

See Also:


Attachments
[Image] After Patch is applied (A) (1.32 MB, image/png)
2018-11-22 23:56 PST, Devin Rousso
no flags Details
[Image] After Patch is applied (B) (1.52 MB, image/png)
2018-11-22 23:56 PST, Devin Rousso
no flags Details
Patch (123.40 KB, patch)
2018-11-23 09:54 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (2.48 MB, application/zip)
2018-11-23 16:06 PST, Build Bot
no flags Details
Patch (124.86 KB, patch)
2018-11-27 09:05 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (124.79 KB, patch)
2018-12-31 18:04 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (123.50 KB, patch)
2019-02-16 17:20 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (123.30 KB, patch)
2019-02-24 19:46 PST, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2018-03-07 15:50:47 PST
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>
Comment 1 Devin Rousso 2018-11-22 23:56:01 PST
Created attachment 355499 [details]
[Image] After Patch is applied (A)
Comment 2 Devin Rousso 2018-11-22 23:56:24 PST
Created attachment 355501 [details]
[Image] After Patch is applied (B)
Comment 3 Devin Rousso 2018-11-23 00:04:01 PST
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.
Comment 4 Devin Rousso 2018-11-23 09:54:57 PST
Created attachment 355521 [details]
Patch

This is the patch for version (B).
Comment 5 Build Bot 2018-11-23 16:06:30 PST Comment hidden (obsolete)
Comment 6 Build Bot 2018-11-23 16:06:32 PST Comment hidden (obsolete)
Comment 7 Devin Rousso 2018-11-27 09:05:46 PST
Created attachment 355739 [details]
Patch
Comment 8 Joseph Pecoraro 2018-11-27 12:08:20 PST
> > [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.
Comment 9 Devin Rousso 2018-11-27 12:11:14 PST
(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).
Comment 10 Devin Rousso 2018-12-31 18:04:12 PST
Created attachment 358159 [details]
Patch
Comment 11 Joseph Pecoraro 2019-02-15 20:02:36 PST
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.
Comment 12 Devin Rousso 2019-02-16 17:19:15 PST
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).
Comment 13 Devin Rousso 2019-02-16 17:20:02 PST
Created attachment 362223 [details]
Patch
Comment 14 Joseph Pecoraro 2019-02-21 12:50:49 PST
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)
Comment 15 Joseph Pecoraro 2019-02-21 15:18:58 PST
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!
Comment 16 Devin Rousso 2019-02-21 16:33:21 PST
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).
Comment 17 Devin Rousso 2019-02-24 19:46:48 PST
Created attachment 362875 [details]
Patch
Comment 18 WebKit Commit Bot 2019-02-25 11:09:11 PST
Comment on attachment 362875 [details]
Patch

Clearing flags on attachment: 362875

Committed r242049: <https://trac.webkit.org/changeset/242049>
Comment 19 WebKit Commit Bot 2019-02-25 11:09:13 PST
All reviewed patches have been landed.  Closing bug.