Remove ordering dependencies from CSSValueKeywords.h |
|||||||||
Issue descriptionAs part of generating ComputedStyle, we need to remove any ordering dependencies from keywords in CSSValueKeywords.h. Most of these can be seen as comments like "enum ordering is defined by CSSValueKeywords.in" or "The order here must match the order of the EBorderStyle enum in ComputedStyleConstants.h". This basically involves finding all inequality checks on CSSValueIDs and replacing them with equality checks only. This can be done by making CSSValueID an enum class, which causes errors on inequality checks. We should also introduce some common functionality for checking the valid keywords for a particular property, i.e. isValidKeywordForProperty<CSSPropertyID>(CSSValueID).
,
Nov 21 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6d786157c078b244a5f6c3cfe5dffe986c2d5b83 commit 6d786157c078b244a5f6c3cfe5dffe986c2d5b83 Author: sashab <sashab@chromium.org> Date: Mon Nov 21 07:07:18 2016 Removed ordering dependencies for ETextAlign Removed ordering dependencies for ETextAlign from CSSValueKeywords.h. Randomly re-ordered the keywords before running tests to ensure no ordering dependencies still exist, then reverted to original order for committing. BUG= 665272 Review-Url: https://codereview.chromium.org/2519813002 Cr-Commit-Position: refs/heads/master@{#433490} [modify] https://crrev.com/6d786157c078b244a5f6c3cfe5dffe986c2d5b83/third_party/WebKit/Source/core/css/CSSPrimitiveValueMappings.h [modify] https://crrev.com/6d786157c078b244a5f6c3cfe5dffe986c2d5b83/third_party/WebKit/Source/core/css/CSSValueKeywords.in
,
Nov 28 2016
Actually, CSSParserFastPaths.cpp still relies on those ordering requirements, so CSSValueKeywords.in needs to be updated to reflect that if the first/last value is changed CSSParserFastPaths needs to be changed too. The order of the enum doesn't have to match, though.
,
Nov 30 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/02bf8ffd9e09a701ca30410df5e1b71d37b0dd09 commit 02bf8ffd9e09a701ca30410df5e1b71d37b0dd09 Author: sashab <sashab@chromium.org> Date: Wed Nov 30 09:08:08 2016 Removed ordering dependencies for EListStyleType Removed ordering dependencies for EListStyleType from CSSValueKeywords.h. This was already done in crrev.com/2367293002, so this patch just updates the comments to show it is no longer needed. Also added a comment to clarify that the fast paths still rely on the ordering, so not to update those. BUG= 665272 Review-Url: https://codereview.chromium.org/2535663002 Cr-Commit-Position: refs/heads/master@{#435192} [modify] https://crrev.com/02bf8ffd9e09a701ca30410df5e1b71d37b0dd09/third_party/WebKit/Source/core/css/CSSValueKeywords.in [modify] https://crrev.com/02bf8ffd9e09a701ca30410df5e1b71d37b0dd09/third_party/WebKit/Source/core/style/ComputedStyleConstants.h
,
Dec 1 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/889989a027d7a812a86db6e0824008b1e36fc9b4 commit 889989a027d7a812a86db6e0824008b1e36fc9b4 Author: sashab <sashab@chromium.org> Date: Thu Dec 01 03:12:31 2016 Added ordering warnings back for CSSValueKeywords.in Added ordering warnings back for CSSValueKeywords.in, since the order in this file must match the order in CSSParserFastPaths until the fast paths file is generated. BUG= 665272 Review-Url: https://codereview.chromium.org/2537393002 Cr-Commit-Position: refs/heads/master@{#435541} [modify] https://crrev.com/889989a027d7a812a86db6e0824008b1e36fc9b4/third_party/WebKit/Source/core/css/CSSValueKeywords.in
,
Dec 7 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a9bca4b5fe68c25941b4e89b19227e99d6d4df7e commit a9bca4b5fe68c25941b4e89b19227e99d6d4df7e Author: sashab <sashab@chromium.org> Date: Wed Dec 07 04:48:41 2016 Removed ordering dependencies for EDisplay Removed ordering dependencies for EDisplay from CSSValueKeywords.h. This involved updating ComputedStyleConstants.h to to not use arithmetic operations to convert from CSSValue to the EDisplay enum, and removed the comment requiring the enums match the order in CSSValueKeywords.in. Also added a comment to clarify that the fast paths still rely on the ordering, so not to update them unless the fast paths are updated too. BUG= 665272 Review-Url: https://codereview.chromium.org/2551013002 Cr-Commit-Position: refs/heads/master@{#436863} [modify] https://crrev.com/a9bca4b5fe68c25941b4e89b19227e99d6d4df7e/third_party/WebKit/Source/core/css/CSSPrimitiveValueMappings.h [modify] https://crrev.com/a9bca4b5fe68c25941b4e89b19227e99d6d4df7e/third_party/WebKit/Source/core/css/CSSValueKeywords.in [modify] https://crrev.com/a9bca4b5fe68c25941b4e89b19227e99d6d4df7e/third_party/WebKit/Source/core/style/ComputedStyleConstants.h
,
Jan 31 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1aa5a2b59d912d6781ec85942708efebe2e19314 commit 1aa5a2b59d912d6781ec85942708efebe2e19314 Author: shend <shend@chromium.org> Date: Tue Jan 31 04:03:15 2017 Use switch statement instead of bit masking for EPosition. Currently, LayoutObject converts EPosition objects into an internal representation using bit masking. However, as part of generating ComputedStyle, there will no longer be a guarantee on the EPosition enumerator values, so the bit masking will not work. This patch converts the bit masking to an explicit switch statements that converts EPosition to the internal LayoutObject representation. This is prework for converting EPosition to an enum class and generating it. BUG= 665272 Review-Url: https://codereview.chromium.org/2664713002 Cr-Commit-Position: refs/heads/master@{#447186} [modify] https://crrev.com/1aa5a2b59d912d6781ec85942708efebe2e19314/third_party/WebKit/Source/core/layout/LayoutObject.h
,
Jan 31 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b2ba6289f11d85440727609ac8430e7804933066 commit b2ba6289f11d85440727609ac8430e7804933066 Author: shend <shend@chromium.org> Date: Tue Jan 31 04:41:41 2017 Replace bitwise & on EClear enum with equality. Currently EClear is defined so that ClearLeft = 1, ClearRight = 2, and ClearBoth = 3. Thus, we can use EClear like a bit flag and check if a clear value X is either left or both by writing X & ClearLeft. However, when we genereate EClear as part of generating ComputedStyle, there's no guarantee on the ordering of the enum values, so we can no longer depend on this trick. This patch simply replaces the bitwise operation with an equality expression which doesn't depend on the ordering of enum values. This is prework for converting EClear to an enum class. BUG= 665272 Review-Url: https://codereview.chromium.org/2652103002 Cr-Commit-Position: refs/heads/master@{#447190} [modify] https://crrev.com/b2ba6289f11d85440727609ac8430e7804933066/third_party/WebKit/Source/core/layout/LayoutBlock.cpp [modify] https://crrev.com/b2ba6289f11d85440727609ac8430e7804933066/third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp
,
Jan 31 2017
Transitioning to meade@ as new owner.
,
Feb 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d7bb23484bdfdc5f4141b7bf9de7815e28d1f1b5 commit d7bb23484bdfdc5f4141b7bf9de7815e28d1f1b5 Author: shend <shend@chromium.org> Date: Mon Feb 06 00:03:07 2017 Remove explicit enum values in EPosition. Currently, the enum values of EPosition are specified explicitly (namely EPosition::FixedPosition is set to 6 instead of the default of 4). This was used for bit masking in LayoutObject, but no longer. This patch removes the explicit enum values since we no longer depend on them. LayoutObject's dependence on explicit enum values was removed in: https://codereview.chromium.org/2664713002. Note that TextAutoSizer::computeFingerprint is indirectly affected by this patch because it assumes that EPosition uses 3 bits. However, EPosition still uses 3 bits after this patch, so no change is required here. This is prework for converting EPosition to an enum class and generating it. BUG= 665272 Review-Url: https://codereview.chromium.org/2667533002 Cr-Commit-Position: refs/heads/master@{#448192} [modify] https://crrev.com/d7bb23484bdfdc5f4141b7bf9de7815e28d1f1b5/third_party/WebKit/Source/core/style/ComputedStyleConstants.h
,
Feb 15 2017
,
Feb 15 2017
,
Feb 15 2017
Darren, you're working on this! Have a bug :)
,
Feb 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5bf6de7534c19b51e66607786263b90deba99b26 commit 5bf6de7534c19b51e66607786263b90deba99b26 Author: shend <shend@chromium.org> Date: Tue Feb 28 18:11:44 2017 Generate mappings between CSSValueID and ComputedStyle enums. To convert between CSSValueIDs and ComputedStyle enums, we currently use handwritten switch statements. This patch generates, for every 'keyword' field in CSSProperties.json5, a pair of mapping functions that convert between a CSSValueID and the corresponding enum of the field. This makes it easier to add new CSS properties and reduces the risk of typos in the handwritten mapping functions. As we generate more CSS properties, we can remove more of the handwritten switch statements. For special cases that we can't generate (e.g. when the mapping is not bijective), we can overload/ specialize the mapping functions in C++. The generated mappings file CSSValueMappings.h is at: https://gist.github.com/anonymous/93e797f4e99c3e8aba829139f612abe0 The names for the mapping functions comes from: https://codereview.chromium.org/2366243002/ BUG= 665272 Review-Url: https://codereview.chromium.org/2702173002 Cr-Commit-Position: refs/heads/master@{#453643} [modify] https://crrev.com/5bf6de7534c19b51e66607786263b90deba99b26/third_party/WebKit/Source/build/scripts/make_computed_style_base.py [add] https://crrev.com/5bf6de7534c19b51e66607786263b90deba99b26/third_party/WebKit/Source/build/scripts/templates/CSSValueIDMappingsGenerated.h.tmpl [modify] https://crrev.com/5bf6de7534c19b51e66607786263b90deba99b26/third_party/WebKit/Source/core/BUILD.gn [modify] https://crrev.com/5bf6de7534c19b51e66607786263b90deba99b26/third_party/WebKit/Source/core/css/BUILD.gn [modify] https://crrev.com/5bf6de7534c19b51e66607786263b90deba99b26/third_party/WebKit/Source/core/css/CSSIdentifierValue.h [modify] https://crrev.com/5bf6de7534c19b51e66607786263b90deba99b26/third_party/WebKit/Source/core/css/CSSPrimitiveValueMappings.h [add] https://crrev.com/5bf6de7534c19b51e66607786263b90deba99b26/third_party/WebKit/Source/core/css/CSSValueIDMappings.h
,
Mar 1 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7f0d7601fd45a6bea9b279bd43c984300b6ff683 commit 7f0d7601fd45a6bea9b279bd43c984300b6ff683 Author: shend <shend@chromium.org> Date: Wed Mar 01 15:35:20 2017 Remove enum ordering dependency in StyleBuilderCustom. StyleBuilderCustom makes an assumption that the values of EListStyleType (which is generated) is ordered in a particular way. This might lead to subtle breakages if the generated order changes. As part of removing all ordering dependencies, this patch uses the generated CSSValueID to platform enum mapping instead, which makes no assumptions about the order of the enums. BUG= 665272 Review-Url: https://codereview.chromium.org/2694333003 Cr-Commit-Position: refs/heads/master@{#453937} [modify] https://crrev.com/7f0d7601fd45a6bea9b279bd43c984300b6ff683/third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp
,
Mar 14 2017
,
Apr 11 2017
,
Apr 18 2017
,
Jun 30 2017
shend@, Please provide an update here. Is this something the intern will be looking into?
,
Jul 3 2017
Not really, it's mainly a laborious task of finding code that depend on the order and change them to if statements or something.
,
Sep 28 2017
Keyword enum values are now sorted by the generator and nothing seems to be broken so there's probably not anything that depends on enum ordering anymore. |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by bugdroid1@chromium.org
, Nov 21 2016