Bug 199594 - Web Inspector: Elements: always show all navigation bar items, but disable those that wouldn't work
Summary: Web Inspector: Elements: always show all navigation bar items, but disable th...
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: Devin Rousso
URL:
Keywords: InRadar
Depends on: 199555
Blocks:
  Show dependency tree / graph
 
Reported: 2019-07-08 15:32 PDT by Devin Rousso
Modified: 2019-07-12 16:17 PDT (History)
5 users (show)

See Also:

Attachments
Patch (16.35 KB, patch)
2019-07-11 11:53 PDT, Devin Rousso 		no flags Details | Formatted Diff | Diff
Patch (16.35 KB, patch)
2019-07-12 14:46 PDT, Devin Rousso 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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>