Bug 232868 - Use isValidCustomIdentifier in consumeWillChange
Summary: Use isValidCustomIdentifier in consumeWillChange
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Nguyen (:ntim)
URL:
Keywords: InRadar
Depends on:
Blocks: 224816
  Show dependency treegraph
 
Reported: 2021-11-08 22:50 PST by Tim Nguyen (:ntim)
Modified: 2021-11-09 02:34 PST (History)
10 users (show)

See Also:


Attachments
Patch (2.14 KB, patch)
2021-11-08 22:55 PST, Tim Nguyen (:ntim)
koivisto: review+
Details | Formatted Diff | Diff
Patch (4.79 KB, patch)
2021-11-09 00:44 PST, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff
Patch (14.21 KB, patch)
2021-11-09 01:02 PST, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Nguyen (:ntim) 2021-11-08 22:50:03 PST
This is what the spec says: https://drafts.csswg.org/css-will-change/#will-change

> The <custom-ident> production used here excludes the keywords will-change, none, all, auto, scroll-position, and contents, in addition to the keywords normally excluded from <custom-ident>.
Comment 1 Tim Nguyen (:ntim) 2021-11-08 22:55:08 PST
Created attachment 443655 [details]
Patch
Comment 2 Antti Koivisto 2021-11-08 23:39:50 PST
Comment on attachment 443655 [details]
Patch

Can you either include the relevant WPT merge with the patch or merge the WPT first? Landing behaviour changes without tests is not nice.
Comment 3 Tim Nguyen (:ntim) 2021-11-09 00:44:53 PST
Created attachment 443657 [details]
Patch
Comment 4 EWS Watchlist 2021-11-09 00:45:55 PST
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Comment 5 Tim Nguyen (:ntim) 2021-11-09 01:02:40 PST
Created attachment 443659 [details]
Patch
Comment 6 EWS 2021-11-09 01:31:08 PST
Committed r285487 (244011@main): <https://commits.webkit.org/244011@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 443659 [details].
Comment 7 Radar WebKit Bug Importer 2021-11-09 01:32:24 PST
<rdar://problem/85193689>
Comment 8 Antti Koivisto 2021-11-09 02:16:19 PST
Comment on attachment 443655 [details]
Patch

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

> Source/WebCore/css/parser/CSSPropertyParser.cpp:517
> +                if (!isValidCustomIdentifier(id))
> +                    return nullptr;
>                  // Append properties we don't recognize, but that are legal, as strings.
>                  values->append(consumeCustomIdent(range).releaseNonNull());

consumeCustomIdent should ASSERT(isValidCustomIdentifier) to catch all these.
Comment 9 Antti Koivisto 2021-11-09 02:34:39 PST
Comment on attachment 443659 [details]
Patch

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

> Source/WebCore/css/parser/CSSPropertyParser.cpp:517
> +                if (!isValidCustomIdentifier(id))
> +                    return nullptr;
>                  // Append properties we don't recognize, but that are legal, as strings.
>                  values->append(consumeCustomIdent(range).releaseNonNull());

Actually since consumeCustomIdent already check isValidCustomIdentifier() a better fix here would be to just null chack consumeCustomIdent result