[webkit-reviews] review denied: [Bug 183420] Web Inspector: [META] Merge Resources and Debugger into a single Sources tab : [Attachment 358159] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Devin Rousso
<drousso at apple.com>'s request for review:
Bug 183420: Web Inspector: [META] Merge Resources and Debugger into a single
Sources tab
https://bugs.webkit.org/show_bug.cgi?id=183420

Attachment 358159: Patch

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




--- Comment #11 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 358159
  --> https://bugs.webkit.org/attachment.cgi?id=358159
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.