[webkit-reviews] review granted: [Bug 189041] Web Inspector: generate CSSKeywordCompletions from backend values : [Attachment 349764] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Sep 17 12:25:32 PDT 2018


Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 189041: Web Inspector: generate CSSKeywordCompletions from backend values
https://bugs.webkit.org/show_bug.cgi?id=189041

Attachment 349764: Patch

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




--- Comment #17 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 349764
  --> https://bugs.webkit.org/attachment.cgi?id=349764
Patch

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

r=me but a few comments worth addressing below. And this needs a follow-up
email re CSS domain changes

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:128
>		       // Skip numbers, like the ones defined for font-weight.
> -		       if (!isNaN(Number(keywords[i])))
> +		       if (keywords[i] ===
WI.CSSKeywordCompletions.AllPropertyNamesPlaceholder ||
!isNaN(Number(keywords[i])))
>			   continue;

Huh, I wonder if there is a faster way. For example just: 
/^\d+$/.test(keyword) instead of the isNaN(Number(...)) dance. Not really
important, just an observation.

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:138
> -WI.CSSKeywordCompletions.AllPropertyNamesPlaceholder = "__all-properties__";
> +WI.CSSKeywordCompletions.AllPropertyNamesPlaceholder =
Symbol("all-properties");

Any reason to make this a Symbol? I just think that takes extra construction
over a String. I'm fine either way.

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:140
> +// Populated by CSS.getSupportedCSSProperties

Nit: End sentence with a period.

> LayoutTests/accessibility/aria-checkbox-sends-notification-expected.txt:2
> +#PID UNRESPONSIVE - com.apple.WebKit.WebContent.Development (pid 13864)
> +FAIL: Timed out waiting for notifyDone to be called

This seems wrong. You probably don't want to include this.

> LayoutTests/inspector/css/getSupportedCSSProperties.html:36
> +		   for (var i = 0; i < entries.length; ++i) {

Nit: `let`

> LayoutTests/inspector/css/getSupportedCSSProperties.html:46
> +		       if (expectedProperty.hasAliases) {
> +			   if (entry.aliases.length)
> +			       ProtocolTest.log(`"${expectedProperty.name}" has
aliases`);

Might as well include the aliases. I don't suppose they will change often or
differ between ports.

> LayoutTests/inspector/css/getSupportedCSSProperties.html:53
> +		       if (expectedProperty.hasLonghands) {
> +			   if (entry.longhands.length)
> +			       ProtocolTest.log(`"${expectedProperty.name}" has
longhands`);

Ditto, include the longhands would be nice.

> LayoutTests/inspector/css/getSupportedCSSProperties.html:67
> +		       if (expectedProperty.value) {
> +			   if (!entry.values)
> +			       ProtocolTest.log(`"${expectedProperty.name}" has
NO values`);

Same thing. Including the values for some stable but unlikely to change
property, would be nicer than just yes/no. It could be that the values being
sent are incorrect / confusing, but that wouldn't be caught by this test which
is just partially checking things.


More information about the webkit-reviews mailing list