Bug 189041 – Web Inspector: generate CSSKeywordCompletions from backend values

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

<https://webkit.org/b/189001> demonstrated that we need to do a better job of either keeping `CSSKeywordCompletions.js` up to date, or create some sort of autogeneration system so that we don't need to manually change that file for every CSS keyword change.

[Devin Rousso]

Created attachment 348267 [details]
[Patch] WIP

This is a good start to getting many values from the backend (including some we didn\'t even have before).

[EWS Watchlist]

Attachment 348267 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Devin Rousso]

Created attachment 348268 [details]
[Patch] WIP

Add missing includes.

[EWS Watchlist]

Attachment 348268 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Devin Rousso]

Created attachment 348270 [details]
[Patch] WIP

Add basic test.

[EWS Watchlist]

Comment on attachment 348270 [details]
[Patch] WIP

Attachment 348270 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9007892

New failing tests:
fast/media/mq-pointer.html
fast/media/mq-hover-invalid.html
fast/media/mq-color-gamut.html
fast/media/mq-pointer-invalid.html
fast/media/mq-any-pointer-invalid.html
fast/media/mq-any-hover-invalid.html

[EWS Watchlist]

Created attachment 348280 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Devin Rousso]

Created attachment 348305 [details]
[Patch] WIP

Can't use AtomicString::ConstructFromLiteral since one of the keyword strings is 0 length "".

[Blaze Burg]

Comment on attachment 348305 [details]
[Patch] WIP

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

> Source/WebCore/css/makevalues.pl:78
> +#include <wtf/ASCIICType.h>

I believe these imports should go between HashTools and string.h

> Source/WebCore/css/makevalues.pl:134
> +    if (id <= 0 || id > lastCSSValueKeyword)

If id is an unsigned short, how could id <= 0 ever evaluate to true? I don't think we expect rollover to be involved.

[Devin Rousso]

Comment on attachment 348305 [details]
[Patch] WIP

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

>> Source/WebCore/css/makevalues.pl:78
>> +#include <wtf/ASCIICType.h>
> 
> I believe these imports should go between HashTools and string.h

I was following what "makeprop.pl" does.  I'll edit this (and that) for consistency.

>> Source/WebCore/css/makevalues.pl:134
>> +    if (id <= 0 || id > lastCSSValueKeyword)
> 
> If id is an unsigned short, how could id <= 0 ever evaluate to true? I don't think we expect rollover to be involved.

Good point.  I didn't really think much into this, as I was more concerned with breaking something.  😅

[Devin Rousso]

Created attachment 348609 [details]
Patch

Added back removed properties for compatibility.  Include alias information for more thorough completion.

[Devin Rousso]

(In reply to Devin Rousso from comment #11)
> Created attachment 348609 [details]
> Patch
> 
> Added back removed properties for compatibility.  Include alias information
> for more thorough completion.
This patch, even though it's r?, doesn't technically fulfill the bug title.  Right now, there are no code-dependent lists (meaning that new keywords cannot be added without modifying it) tying CSS property names and keyword values.  The closes to something like that is `CSSParserFastPaths::isValidKeywordPropertyAndValue`, but that is limited to CSS properties that have *only* keyword values (e.g. `margin-*` wouldn't work because it has both "auto" and "1px").  Ideally, we'd have a way to generate the keywords for every CSS property, regardless of whether it has other valid values.  This is a good step in the right direction.  As such, I think it's reviewable/committable.

[Joseph Pecoraro]

Comment on attachment 348609 [details]
Patch

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

I haven't completed reviewing this but adding my comments so far.

> Source/WebInspectorUI/ChangeLog:8
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * UserInterface/Models/CSSCompletions.js:

Could use some high level descriptions in the ChangeLog.

> Source/WebCore/css/makeprop.pl:411
> +    if (!$nameToAliases{$name}) {
> +        next;
> +    }

Might read better as:

    next if !$nameToAliases{$name};

But I don't know if we avoid that in our perl style.

> Source/WebCore/css/makeprop.pl:413
> +    print GPERF "        return { \"" . join("\", \"", @{$nameToAliases{$name}}) . "\" };\n";

Maybe this should use _s to get benefits of ASCIILiterals:

    print GPERF "        return { \"" . join("\"_s, \"", @{$nameToAliases{$name}}) . "\"_s };\n";

Should convert:

    return { "one", "two" };

To:

    return { "one"_s, "two"_s };

> Source/WebCore/css/makeprop.pl:421
> +

Nit: Extra newline.

> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:749
> +            for (int j = firstCSSValueKeyword; j < lastCSSValueKeyword; ++j) {

Seems this comparison should be inclusive, since "last" sounds inclusive:

    j <= lastCSSValueKeyword

> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:752
> +                if (CSSParserFastPaths::isValidKeywordPropertyAndValue(propertyID, valueID, HTMLStandardMode))
> +                    values->addItem(getValueNameString(valueID));

Cool! I can't help but think this might be missing some stuff that we may have had previously, but this is most certainly a step in the right direction!

[Joseph Pecoraro]

Comment on attachment 348609 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:142
> +WI.CSSKeywordCompletions.PropertyNameForAlias = {};
>  
> -    // iOS Properties
> -    "-webkit-overflow-scrolling", "-webkit-touch-callout", "-webkit-tap-highlight-color"
> -].keySet();
> +WI.CSSKeywordCompletions.LonghandNamesForShorthandProperty = {};

Nit: Would be nice to include a comment that these are populated from CSS.getSupportedCSSProperties

Also seems these two objects should probably be a `new Map`.

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:145
> +    // Compatibility (iOS 12): `inherited` didn't exist on `CSSPropertyInfo`

I'd put this comment above the variable definition, not in the list.

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:305
>  ].keySet();

I wonder if `keySet` would be better implemented as `new Set` now. I think they are efficient and inline-able by JITs.

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:307
>  WI.CSSKeywordCompletions._propertyKeywordMap = {

What changes were made to this list and how were the changes made?

> LayoutTests/inspector/css/getSupportedCSSProperties.html:24
> +            for (let expectedProperty of expectedProperties) {

r- here because this should be testing all of the new backend additions. This seems to cover `inherited` but doesn't cover `aliases`.

[Devin Rousso]

Comment on attachment 348609 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:145
>> +    // Compatibility (iOS 12): `inherited` didn't exist on `CSSPropertyInfo`
> 
> I'd put this comment above the variable definition, not in the list.

I kept it in the list because the compatibility part is the default values of the set, not the set itself.  If we drop support for the older backends in the future, we'd only want to delete the strings, not `WI.CSSKeywordCompletions.InheritedProperties`.  I'll change it to be above.

>> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:307
>>  WI.CSSKeywordCompletions._propertyKeywordMap = {
> 
> What changes were made to this list and how were the changes made?

The properties that were moved to the bottom of this list were the ones that were successfully generated by the new backend code in this patch.  As an example, the `CSSAgent.getSupportedCSSProperties` payload now has "auto" and "fixed" for "table-layout", meaning that we no longer need that value here in the list (it's only kept for compatibility reasons).

One fun thing of note was that this actually caught some completion values that are no longer supported, like the "region" values for the "break-*" properties.

[Devin Rousso]

Created attachment 349764 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 349764 [details]
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.

[Devin Rousso]

Comment on attachment 349764 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:138
>> +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.

My only reason to use a string is that if in the future a property name gets added called "__all-properties__" this won't break, but that's not a very good reason, so I'll change it  back.

>> LayoutTests/accessibility/aria-checkbox-sends-notification-expected.txt:2
>> +FAIL: Timed out waiting for notifyDone to be called
> 
> This seems wrong. You probably don't want to include this.

Oh weird.  How'd that happen?  🤔

[Devin Rousso]

Created attachment 349937 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 349937 [details]
Patch

Clearing flags on attachment: 349937

Committed r236091: <https://trac.webkit.org/changeset/236091>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44537476>