Bug 203377 - Web Inspector: CONSOLE ERROR Shown panel style-rules must be visible
Summary: Web Inspector: CONSOLE ERROR Shown panel style-rules must be visible
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Yury Semikhatsky
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-10-24 11:23 PDT by Yury Semikhatsky
Modified: 2019-10-31 10:19 PDT (History)
4 users (show)

See Also:


Attachments
Patch (3.32 KB, patch)
2019-10-24 15:29 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch for landing (4.10 KB, patch)
2019-10-31 09:34 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yury Semikhatsky 2019-10-24 11:23:38 PDT
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
Comment 1 Yury Semikhatsky 2019-10-24 11:28:48 PDT
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
Comment 2 Yury Semikhatsky 2019-10-24 15:29:48 PDT
Created attachment 381847 [details]
Patch
Comment 3 Devin Rousso 2019-10-30 14:10:07 PDT
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.
Comment 4 Yury Semikhatsky 2019-10-30 14:52:12 PDT
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.
Comment 5 Devin Rousso 2019-10-30 22:31:37 PDT
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?
Comment 6 Yury Semikhatsky 2019-10-31 09:34:54 PDT
Created attachment 382467 [details]
Patch for landing
Comment 7 Yury Semikhatsky 2019-10-31 09:35:29 PDT
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.
Comment 8 WebKit Commit Bot 2019-10-31 10:18:26 PDT
Comment on attachment 382467 [details]
Patch for landing

Clearing flags on attachment: 382467

Committed r251853: <https://trac.webkit.org/changeset/251853>
Comment 9 WebKit Commit Bot 2019-10-31 10:18:28 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2019-10-31 10:19:13 PDT
<rdar://problem/56785760>