Bug 203377 – Web Inspector: CONSOLE ERROR Shown panel style-rules must be visible

• Component: Web Inspector
• Status: RESOLVED FIXED

[Yury Semikhatsky]

The following error is printed to the console when opening Web Inspector:

resource:///org/webkit/inspector/UserInterface/Views/GeneralStyleDetailsSidebarPanel.js:66:23: CONSOLE ERROR Shown panel style-rules must be visible. false

[Yury Semikhatsky]

shown@resource:///org/webkit/inspector/UserInterface/Views/GeneralStyleDetailsSidebarPanel.js:66:131
selectedSidebarPanel@resource:///org/webkit/inspector/UserInterface/Views/Sidebar.js:139:45
showDetailsSidebarPanels@resource:///org/webkit/inspector/UserInterface/Views/ContentBrowserTabContentView.js:200:38
_contentBrowserCurrentRepresentedObjectsDidChange@resource:///org/webkit/inspector/UserInterface/Views/ContentBrowserTabContentView.js:331:38
dispatch@resource:///org/webkit/inspector/UserInterface/Base/Object.js:165:30
dispatchEventToListeners@resource:///org/webkit/inspector/UserInterface/Base/Object.js:172:17
resource:///org/webkit/inspector/UserInterface/Views/ContentBrowser.js:96:42
_execute@resource:///org/webkit/inspector/UserInterface/Base/Debouncer.js:132:29
resource:///org/webkit/inspector/UserInterface/Base/Debouncer.js:75:26

[Yury Semikhatsky]

Created attachment 381847 [details]
Patch

[Devin Rousso]

Comment on attachment 381847 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/ContentBrowserTabContentView.js:212
> -        if (!WI.detailsSidebar.sidebarPanels.length)
> +        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.

> 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.

[Yury Semikhatsky]

Comment on attachment 381847 [details]
Patch

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

>> 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.

>> 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.

[Devin Rousso]

Comment on attachment 381847 [details]
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?

[Yury Semikhatsky]

Created attachment 382467 [details]
Patch for landing

[Yury Semikhatsky]

Comment on attachment 381847 [details]
Patch

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

>>>> 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.

Agreed. Updated the code to not use currentSidebarPanels at all and access WI.detailsSidebar.sidebarPanels directly instead.

>>>> 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?

I considered that but it could lead to a situation when hidden() is not called at all if the Sidebar is collapsed and _selectedSidebarPanel changes.

[WebKit Commit Bot]

Comment on attachment 382467 [details]
Patch for landing

Clearing flags on attachment: 382467

Committed r251853: <https://trac.webkit.org/changeset/251853>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/56785760>