[webkit-reviews] review granted: [Bug 191820] Web Inspector: Add support for forcing color scheme appearance in DOM tree : [Attachment 355305] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Timothy Hatcher
<timothy at apple.com>'s request for review:
Bug 191820: Web Inspector: Add support for forcing color scheme appearance in
DOM tree
https://bugs.webkit.org/show_bug.cgi?id=191820

Attachment 355305: Patch

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




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

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

r=me, super awesome job!

It'd be great for WebInspector development if there was a way to do a similar
thing for WebInspector within WebInspector, just like how we can force the
layout direction.  You could do this for dark mode via System Preferences or by
inspector^2, but that's a bit more annoying :P

> Source/JavaScriptCore/inspector/protocol/Page.json:206
> +		   { "name": "appearance", "$ref": "Appearance", "description":
"Appearance name to force. Empty string disables the override." }

Considering that we are using a specified type `Appearance`, perhaps we should
add a `Default` to the list of values.	If this value was a _real_ enum, an
"empty string" would be invalid, so even though that works here because it's
just a `String`, it'd be more "correct" (somewhat similar to
`WI.LayoutDirection`).	You'd also be able to use this in the frontend by
passing actual specific values instead of empty strings.

> Source/JavaScriptCore/inspector/protocol/Page.json:312
> +	       "name": "defaultAppearanceDidChange",

NIT: I hate it when events have "Did" in the name, as it's almost always
unnecessary.  I'd prefer this be called `defaultAppearanceChanged`.

> Source/JavaScriptCore/inspector/protocol/Page.json:315
> +		   { "name": "appearance", "$ref": "Appearance", "description":
"Name of the appearance that is active (not considering any forced
appearance.)" }

NIT: the "." should go after the ")"

> Source/WebCore/inspector/agents/InspectorPageAgent.cpp:821
> +void InspectorPageAgent::setForcedAppearance(ErrorString&, const String&
appearance)

See Page.json:206 for using the actual protocol type instead of a `String`

> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:151
> +	   PageAgent.setForcedAppearance(protocolName).then(() => {

Should we do any error logging?  I'd also prefer a callback if there's no
reason to use the promise (e.g. returning it for chaining).

> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:159
> +	   return window.PageAgent && !!PageAgent.setForcedAppearance &&
this._defaultAppearance;

I think the new approach for feature checking is to use
`InspectorBackend.domains.Page.setForcedAppearance`.

> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:644
> +    DefaultAppearanceDidChange: "css-manager-default-appearance-did-change",
> +    ForcedAppearanceDidChange: "css-manager-forced-appearance-did-change",

See Page.json:312

> LayoutTests/inspector/css/force-page-appearance.html:30
> +	       InspectorTest.expectEqual(getProperty("color").rawValue, "rgb(0,
0, 0)");

These checks could use comments, as otherwise it's a bit confusing to read the
expected value and understand what's going on.