<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.
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).
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.
Created attachment 348268 [details] [Patch] WIP Add missing includes.
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.
Created attachment 348270 [details] [Patch] WIP Add basic test.
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
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
Created attachment 348305 [details] [Patch] WIP Can't use AtomicString::ConstructFromLiteral since one of the keyword strings is 0 length "".
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 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. 😅
Created attachment 348609 [details] Patch Added back removed properties for compatibility. Include alias information for more thorough completion.
(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 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 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 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.
Created attachment 349764 [details] Patch
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 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? 🤔
Created attachment 349937 [details] Patch
Comment on attachment 349937 [details] Patch Clearing flags on attachment: 349937 Committed r236091: <https://trac.webkit.org/changeset/236091>
All reviewed patches have been landed. Closing bug.
<rdar://problem/44537476>