Bug 189041 - Web Inspector: generate CSSKeywordCompletions from backend values
Summary: Web Inspector: generate CSSKeywordCompletions from backend values
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-08-27 23:18 PDT by Devin Rousso
Modified: 2018-09-17 15:44 PDT (History)
9 users (show)

See Also:


Attachments
[Patch] WIP (43.95 KB, patch)
2018-08-27 23:25 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Patch] WIP (44.16 KB, patch)
2018-08-27 23:36 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Patch] WIP (48.23 KB, patch)
2018-08-28 00:26 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews116 for mac-sierra (3.45 MB, application/zip)
2018-08-28 02:24 PDT, EWS Watchlist
no flags Details
[Patch] WIP (48.10 KB, patch)
2018-08-28 10:14 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (76.81 KB, patch)
2018-08-30 23:11 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (79.50 KB, patch)
2018-09-14 09:17 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (77.87 KB, patch)
2018-09-17 14:00 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2018-08-27 23:18:42 PDT
<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.
Comment 1 Devin Rousso 2018-08-27 23:25:36 PDT
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).
Comment 2 EWS Watchlist 2018-08-27 23:27:48 PDT
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.
Comment 3 Devin Rousso 2018-08-27 23:36:31 PDT
Created attachment 348268 [details]
[Patch] WIP

Add missing includes.
Comment 4 EWS Watchlist 2018-08-27 23:38:59 PDT
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.
Comment 5 Devin Rousso 2018-08-28 00:26:40 PDT
Created attachment 348270 [details]
[Patch] WIP

Add basic test.
Comment 6 EWS Watchlist 2018-08-28 02:24:17 PDT Comment hidden (obsolete)
Comment 7 EWS Watchlist 2018-08-28 02:24:18 PDT Comment hidden (obsolete)
Comment 8 Devin Rousso 2018-08-28 10:14:11 PDT
Created attachment 348305 [details]
[Patch] WIP

Can't use AtomicString::ConstructFromLiteral since one of the keyword strings is 0 length "".
Comment 9 BJ Burg 2018-08-28 15:36:34 PDT
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.
Comment 10 Devin Rousso 2018-08-28 15:39:01 PDT
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.  😅
Comment 11 Devin Rousso 2018-08-30 23:11:03 PDT
Created attachment 348609 [details]
Patch

Added back removed properties for compatibility.  Include alias information for more thorough completion.
Comment 12 Devin Rousso 2018-08-30 23:23:22 PDT
(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.
Comment 13 Joseph Pecoraro 2018-09-11 18:36:09 PDT
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!
Comment 14 Joseph Pecoraro 2018-09-12 12:22:24 PDT
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`.
Comment 15 Devin Rousso 2018-09-12 14:44:09 PDT
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.
Comment 16 Devin Rousso 2018-09-14 09:17:18 PDT
Created attachment 349764 [details]
Patch
Comment 17 Joseph Pecoraro 2018-09-17 12:25:32 PDT
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.
Comment 18 Devin Rousso 2018-09-17 13:40:50 PDT
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?  🤔
Comment 19 Devin Rousso 2018-09-17 14:00:25 PDT
Created attachment 349937 [details]
Patch
Comment 20 WebKit Commit Bot 2018-09-17 15:43:18 PDT
Comment on attachment 349937 [details]
Patch

Clearing flags on attachment: 349937

Committed r236091: <https://trac.webkit.org/changeset/236091>
Comment 21 WebKit Commit Bot 2018-09-17 15:43:20 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Radar WebKit Bug Importer 2018-09-17 15:44:23 PDT
<rdar://problem/44537476>