[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
bugzilla-daemon at webkit.org
Fri Feb 15 20:02:36 PST 2019
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.
More information about the webkit-reviews
mailing list