[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