[Webkit-unassigned] [Bug 230173] Web Inspector: Network: "Lock" icon next to domain name in table view should show security information

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=230173

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

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

Nice patch!  Some minor comments and then I think we're good to go :)

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.css:124
> +    cursor: pointer;

We don't normally make clickable icons into a pointer.  Is there a reason why we should in this case?

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:699
> +                let security = entry.resource.security;

I'm not sure we can assume that `entry.resource` is valid, so you may need to `entry.resource?.security || {}` instead.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:703
> +                if (!isEmptyObject(security)
> +                    && !isEmptyObject(security.certificate)
> +                    && !Object.values(security.certificate).every((value) => !value)) {

I think you can simplify this
```
    if (WI.NetworkManager.supportsShowCertificate() && Object.values(security?.certificate || {}).some((value) => value))
```

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:704
> +

Style: unnecessary whitespace

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:706
> +

ditto (:704)

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:707
> +                        if (WI.NetworkManager.supportsShowCertificate()) {

please move this to the outer `if` (see comment above) as we don't event want to add the `"click"` event listener if we can't do something with it

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:711
> +                            entry.resource.showCertificate()
> +                            .catch((error) => {
> +                                console.error(error);
> +                            });

you can simplify this to just be `entry.resource.showCertificate().catch(WI.reportInternalError)

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20211103/e0857864/attachment.htm>