Bug 182597

<rdar://problem/37341564>

Created attachment 333998 [details] WIP

Created attachment 334000 [details] WIP

(In reply to Matt Baker from comment #3) > Created attachment 334000 [details] > WIP Remaining issues: - Sidebar is still visible when the Inspector opens to the Canvas tab - Sidebar collapsed setting should be honored when re-enabling the sidebar - Might need to push logic into TabBrowser for keyboard shortcuts to work

Comment on attachment 334000 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=334000&action=review This looks good! (In reply to Matt Baker from comment #4) > Remaining issues: > - Sidebar is still visible when the Inspector opens to the Canvas tab This actually appears to be working just fine for me. When I open WebInspector with the Canvas tab previously selected, I don't see the sidebar at all (it's also disabled). > - Sidebar collapsed setting should be honored when re-enabling the sidebar The main issue here is that `set collapsed` fires `WI.Sidebar.Event.CollapsedStateDidChange`, which calls through to `WI.TabBrowser.prototype._sidebarCollapsedStateDidChange`, and in there the WI.Setting actually gets changed. We still want all the other functionality, but we just don't want that value to change. > - Might need to push logic into TabBrowser for keyboard shortcuts to work Yeah, the shortcut still works even if the button is disabled. :( > Source/WebInspectorUI/UserInterface/Views/ContentBrowserTabContentView.js:44 > + this._contentBrowser.addEventListener(WI.ContentBrowser.Event.CurrentRepresentedObjectsDidChange, this._currentRepresentedObjectsDidChange, this); I think we should follow the naming convention established on the following line and use `_contentBrowserCurrentRepresentedObjectsDidChange`.

Created attachment 334073 [details] WIP 2

(In reply to Matt Baker from comment #6) > Created attachment 334073 [details] > WIP 2 In this patch, the sidebar logic is shared between the TabBrowser and ContentBrowserTabContentView, mirroring the approach taken for DetailsSidebarPanels. This fixes: - Sidebar being shown when Inspector opens to the Canvas tab - Disabled sidebar being expandable using the keyboard shortcut Collapsed state management is improved, but not perfect.

Comment on attachment 334073 [details] WIP 2 Attachment 334073 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/6543448 New failing tests: http/tests/security/http-0.9/xhr-blocked.html

Created attachment 334083 [details] Archive of layout-test-results from ews103 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-sierra Platform: Mac OS X 10.12.6

Created attachment 334089 [details] Patch

Comment on attachment 334089 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=334089&action=review Other than the one issue, this looks great. I'd rather not r+ until it's fixed. > Source/WebInspectorUI/UserInterface/Views/TabBrowser.js:324 > + if (event.target === this._navigationSidebar && !tabContentView.managesNavigationSidebarPanel) By doing this, we don't save the collapsed state of the NavigationSidebar whenever we show/hide it manually via the button. This means that the collapsed state will never change from what it was before this change. I think we only want to apply this logic in the event that the sidebar is collapsed via code, not by the user, possibly by editing WI.toggleNavigationSidebar (and maybe WI.toggleDetailsSidebar too) to set a boolean or to manually set the setting value.

Created attachment 334182 [details] Patch

Comment on attachment 334182 [details] Patch r=me. Thanks for doing these iterations :)

Comment on attachment 334182 [details] Patch Clearing flags on attachment: 334182 Committed r228722: <https://trac.webkit.org/changeset/228722>

All reviewed patches have been landed. Closing bug.