[webkit-reviews] review granted: [Bug 203377] Web Inspector: CONSOLE ERROR Shown panel style-rules must be visible : [Attachment 381847] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Yury Semikhatsky
<yurys at chromium.org>'s request for review:
Bug 203377: Web Inspector: CONSOLE ERROR Shown panel style-rules must be
visible
https://bugs.webkit.org/show_bug.cgi?id=203377

Attachment 381847: Patch

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




--- Comment #5 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 381847
  --> https://bugs.webkit.org/attachment.cgi?id=381847
Patch

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

r=me, I think this is fine.  I tried moving around between various tabs and
changing state when certain panels weren't visible and it didn't seem to cause
any regressions.

>>>
Source/WebInspectorUI/UserInterface/Views/ContentBrowserTabContentView.js:212
>>> +	     if (!currentSidebarPanels.length)
>> 
>> This change shouldn't actually modify any logic.  `currentSidebarPanels`
should be a reference to the same array instance as
`WI.detailsSidebar.sidebarPanels`.  As such, please revert this.
> 
> Yes, it doesn't change any logic. Its a leftover from me try to fix the logic
in this method first. A mix of currentSidebarPanels.length and
WI.detailsSidebar.sidebarPanels.length in this method is somewhat confusing
that's why I updated that for consistency. So let me know if you'd still prefer
to revert it.

I think I'd prefer it as it was (and even to have the other cases of
`currentSidebarPanels` be removed/renamed), as there's no immediate indication
that `currentSidebarPanels` is ever modified.  Since we're calling a bunch of
`add*`/`remove*` methods on `WI.detailsSidebar`, it makes more sense that we'd
check it there.  Plus, using `currentSidebarPanels` because it's the same
object is an implementation detail that could theoretically change in the
future, and we'd never know about it causing issues here with the change.

>>> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:139
>>> +		 if (!this.collapsed) {
>> 
>> This part looks fine, but I'd like to test it myself before I say yes/no.
> 
> SG.

Should we also be doing this for the `hidden()` call above?