Bug 199594

Summary: Web Inspector: Elements: always show all navigation bar items, but disable those that wouldn't work
Product: WebKit Reporter: Devin Rousso <drousso>
Component: Web InspectorAssignee: Devin Rousso <drousso>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, drousso, inspector-bugzilla-changes, ross.kirsling, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 199555    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch none

Description Devin Rousso 2019-07-08 15:32:55 PDT
Instead of only showing icons for functionality that is supported by the inspected target, we should always show all of the icons and just disable the ones that wouldn't work.
Comment 1 Devin Rousso 2019-07-11 11:53:43 PDT
Created attachment 373934 [details]
Patch
Comment 2 Ross Kirsling 2019-07-12 14:17:59 PDT
Comment on attachment 373934 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:36
> +        this._compositingBordersButtonNavigationItem.enabled = InspectorBackend.domains.Page;

Did you mean to drop the existing condition?

> this._compositingBordersButtonNavigationItem.enabled = InspectorBackend.domains.Page && !!InspectorBackend.domains.Page.getCompositingBordersVisible;
Comment 3 Devin Rousso 2019-07-12 14:36:46 PDT
Comment on attachment 373934 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:36
>> +        this._compositingBordersButtonNavigationItem.enabled = InspectorBackend.domains.Page;
> 
> Did you mean to drop the existing condition?

No, but I do need to add a `!!`.  The idea is that we only want to enable this button if the target has a `PageAgent`.  `Page.getCompositingBordersVisible` exists for all supported iOS inspection targets, so we don't need a more specific compatibility check than that.
Comment 4 Ross Kirsling 2019-07-12 14:38:59 PDT
(In reply to Devin Rousso from comment #3)
> Comment on attachment 373934 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373934&action=review
> 
> >> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:36
> >> +        this._compositingBordersButtonNavigationItem.enabled = InspectorBackend.domains.Page;
> > 
> > Did you mean to drop the existing condition?
> 
> No, but I do need to add a `!!`.  The idea is that we only want to enable
> this button if the target has a `PageAgent`. 
> `Page.getCompositingBordersVisible` exists for all supported iOS inspection
> targets, so we don't need a more specific compatibility check than that.

Ahh, I'm now seeing the correspondence with your // COMPATIBILITY comments. :)
Comment 5 Ross Kirsling 2019-07-12 14:40:52 PDT
Comment on attachment 373934 [details]
Patch

r=me with that tiny update then :)
Comment 6 Devin Rousso 2019-07-12 14:46:04 PDT
Created attachment 374037 [details]
Patch
Comment 7 WebKit Commit Bot 2019-07-12 16:09:58 PDT
Comment on attachment 374037 [details]
Patch

Clearing flags on attachment: 374037

Committed r247405: <https://trac.webkit.org/changeset/247405>
Comment 8 WebKit Commit Bot 2019-07-12 16:09:59 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2019-07-12 16:17:29 PDT
<rdar://problem/53032515>