[webkit-reviews] review granted: [Bug 235234] [Web Inspector] Graphics tab should display pseudo-elements for more than ::before and ::after : [Attachment 449283] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Antoine Quint
<graouts at webkit.org>'s request for review:
Bug 235234: [Web Inspector] Graphics tab should display pseudo-elements for
more than ::before and ::after
https://bugs.webkit.org/show_bug.cgi?id=235234

Attachment 449283: Patch

https://bugs.webkit.org/attachment.cgi?id=449283&action=review




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

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

r=me, nice work (and test)!

> Source/JavaScriptCore/inspector/protocol/CSS.json:270
> +	       "id": "Styleable",

Can we add a `"description": "..."` for this?

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:558
> +    return pushNodeToFrontend(element ? element : &styleable.element);

NIT: `element ?: &styleable.element`

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:660
> +Ref<Protocol::CSS::Styleable>
InspectorDOMAgent::pushStyleablePathToFrontend(Protocol::ErrorString
errorString, const Styleable& styleable)

NIT: It's a bit odd for the `InspectorDOMAgent` to be dealing with things from
the `CSS` protocol domain (especially since the `DOM` protocol domain is
basically a dependency of the `CSS` protocol domain).  I think I'd either move
this to `InspectorCSSAgent` (which would use
`m_instrumentingAgents.persistentDOMAgent()`) or move the `Styleable` protocol
object to the `DOM` domain (which should also change `WI.CSSStyleable` to
`WI.DOMStyleable` btw).  Regardless, in either case, please make sure the
naming is consistent in the frontend too :)

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:663
> +    auto nodeId = pushNodePathToFrontend(errorString, element ? element :
&styleable.element);

ditto (:558)

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:669
> +    if (styleable.pseudoId != PseudoId::None) {

NIT:  I wonder if instead of checking for `!= PseudoId::None` we should just
remove the `ASSERT_NOT_REACHED` in
`InspectorCSSAgent::protocolValueForPseudoId` ��

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:673
> +    }
> +    return protocolStyleable;

Style: I'd add a newline between these

> Source/WebInspectorUI/UserInterface/Base/DOMUtilities.js:60
> +WI.linkifyStyleable = function(styleable)

NIT: I'd add a `console.assert(styleable instanceof WI.CSSStyleable,
styleable);` just in case :)

> Source/WebInspectorUI/UserInterface/Base/DOMUtilities.js:71
> +

oops?

> Source/WebInspectorUI/UserInterface/Models/Animation.js:43
> +	   // COMPATIBILITY (iOS 15): effectTarget changed from DOM.NodeId to
CSS.Styleable.

This should really be next to `requestEffectTarget` since that's actually where
the difference is.  The callback given to it also needs to handle both
`CSS.Styleable` and `DOM.NodeId`.

> Source/WebInspectorUI/UserInterface/Models/Animation.js:213
> +	   target.AnimationAgent.requestEffectTarget(this._animationId, (error,
styleable) => {
> +	       this._effectTarget = !error ?
WI.CSSStyleable.fromPayload(styleable) : null;

ditto (:43)
```
    target.AnimationAgent.requestEffectTarget(this._animationId, (error,
styleable) => {
	// COMPATIBILITY (iOS 15): effectTarget changed from DOM.NodeId to
CSS.Styleable.
	if (!isNaN(styleable))
	    styleable = {nodeId: styleable};

	...
    });
```

You can confirm that this works by using your locally ToT build to inspect an
older iOS simulator (which shouldn't have these changes).

> Source/WebInspectorUI/UserInterface/Models/CSSStyleable.js:1
> +WI.CSSStyleable = class CSSStyleable

I think this needs the copyright header ��

> Source/WebInspectorUI/UserInterface/Models/CSSStyleable.js:8
> +	   if (pseudoId)
> +	      
console.assert(Object.values(WI.CSSManager.PseudoSelectorNames).includes(pseudo
Id), pseudoId);

We strip `console.assert` from production builds, so this will cause some
issues.  I'd do this instead:
```
    console.assert(node instanceof WI.DOMNode, node);
    console.assert(!pseudoId ||
Object.values(WI.CSSManager.PseudoSelectorNames).includes(pseudoId), pseudoId);
```

>
Source/WebInspectorUI/UserInterface/Views/AnimationCollectionContentView.js:102
> +	       styleable.node.highlight();

I wonder if we should make a `WI.CSSStyleable.prototype.highlight` and add a
new `CSS.highlightStyleable` protocol command so that we can narrow down to the
pseudo element (which we should already support).  This can probably be a
followup tho :)

> Source/WebInspectorUI/UserInterface/Views/AnimationContentView.js:166
> +	       this._animationTargetDOMNode = styleable.node;

I wonder if we should make this `_animationTarget` and create a
`WI.appendContextMenuItemsForStyleable` so that we refer to the right node. 
This can probably be a followup tho :)