|
|
DescriptionAllow (non)property fields to be hardcoded in make_computed_style_base.
Currently we divide ComputedStyle fields into two groups: properties
and nonproperties. Properties represent actual CSS properties and are
read from CSSProperties.json5. Nonproperties represent fields that do
not correspond to a CSS property and are read from a hardcoded list in
the generator script.
However, some fields act like properties, but do not correspond to
CSS properties (e.g. margin is a combination of four properties, but it
itself is not a CSS property). These need to be stored in the hardcoded
list. But since everything in the hardcoded list is considered a
nonproperty, this will not generate the right code.
Hence, this patch changes the hardcoded list to be rather a list of
"extra" fields which can be property or nonproperty fields. To
distinguish between properties and nonproperties, we introduce a
has_custom_compare_and_copy parameter. If true, the field is a
nonproperty.
This is prework for replacing the concept of nonproperties with
has_custom_compare_and_copy entirely.
This patch does not affect existing behaviour.
BUG=628043
Review-Url: https://codereview.chromium.org/2811273003
Cr-Commit-Position: refs/heads/master@{#466166}
Committed: https://chromium.googlesource.com/chromium/src/+/abefb698e5e6ad81853ffdd0560c9efa4740464d
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Rebase #Patch Set 4 : has_custom_compare_and_copy #Patch Set 5 : Rebase #Patch Set 6 : Rebase #
Total comments: 2
Patch Set 7 : Rebase #
Total comments: 2
Patch Set 8 : Address comments #Patch Set 9 : Rebase #Patch Set 10 : Rebase #Messages
Total messages: 37 (30 generated)
Description was changed from ========== Allow nonproperty fields to in CSSProperties.json5. Currently we divide ComputedStyle fields into two groups: properties and nonproperties. Properties represent actual CSS properties and are read from CSSProperties.json5. Nonproperties represent fields that do not correspond to a CSS property and are read from a hardcoded list in the generator script. However, some fields act like properties, but do not correspond to CSS properties (e.g. margin is a combination of four properties, but it itself is not a CSS property). These need to be stored in the hardcoded list. But since everything in the hardcoded list is considered a nonproperty, this will not work. Hence, this patch changes the hardcoded list to be rather a list of "extra" fields which can be property or nonproperty fields. To distinguish between properties and nonproperties, we use the is_property parameter. This allows us to add in extra fields that behave like properties but does not directly correspond to a CSS property. BUG=628043 ========== to ========== Allow (non)property fields to be hardcoded in make_computed_style_base. Currently we divide ComputedStyle fields into two groups: properties and nonproperties. Properties represent actual CSS properties and are read from CSSProperties.json5. Nonproperties represent fields that do not correspond to a CSS property and are read from a hardcoded list in the generator script. However, some fields act like properties, but do not correspond to CSS properties (e.g. margin is a combination of four properties, but it itself is not a CSS property). These need to be stored in the hardcoded list. But since everything in the hardcoded list is considered a nonproperty, this will not generate the right code. Hence, this patch changes the hardcoded list to be rather a list of "extra" fields which can be property or nonproperty fields. To distinguish between properties and nonproperties, we use the is_property parameter. This allows us to add in extra fields that behave like properties by specifying is_property=True. BUG=628043 ==========
shend@chromium.org changed reviewers: + nainar@chromium.org
Hi Bugs, PTAL
Not Bugs but lgtm :P
shend@chromium.org changed reviewers: + alancutter@chromium.org
Hi Alan, PTAL
Description was changed from ========== Allow (non)property fields to be hardcoded in make_computed_style_base. Currently we divide ComputedStyle fields into two groups: properties and nonproperties. Properties represent actual CSS properties and are read from CSSProperties.json5. Nonproperties represent fields that do not correspond to a CSS property and are read from a hardcoded list in the generator script. However, some fields act like properties, but do not correspond to CSS properties (e.g. margin is a combination of four properties, but it itself is not a CSS property). These need to be stored in the hardcoded list. But since everything in the hardcoded list is considered a nonproperty, this will not generate the right code. Hence, this patch changes the hardcoded list to be rather a list of "extra" fields which can be property or nonproperty fields. To distinguish between properties and nonproperties, we use the is_property parameter. This allows us to add in extra fields that behave like properties by specifying is_property=True. BUG=628043 ========== to ========== Allow (non)property fields to be hardcoded in make_computed_style_base. Currently we divide ComputedStyle fields into two groups: properties and nonproperties. Properties represent actual CSS properties and are read from CSSProperties.json5. Nonproperties represent fields that do not correspond to a CSS property and are read from a hardcoded list in the generator script. However, some fields act like properties, but do not correspond to CSS properties (e.g. margin is a combination of four properties, but it itself is not a CSS property). These need to be stored in the hardcoded list. But since everything in the hardcoded list is considered a nonproperty, this will not generate the right code. Hence, this patch changes the hardcoded list to be rather a list of "extra" fields which can be property or nonproperty fields. To distinguish between properties and nonproperties, we introduce a has_custom_compare_and_copy parameter. If true, the field is a nonproperty. This is prework for replacing the concept of nonproperties with has_custom_compare_and_copy entirely. This patch does not affect existing behaviour. BUG=628043 ==========
The CQ bit was checked by shend@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by shend@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by shend@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by shend@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2811273003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): https://codereview.chromium.org/2811273003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:321: property_['has_custom_compare_and_copy'] = False # All CSS properties that are generated do not have custom comparison and copy logic. https://codereview.chromium.org/2811273003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): https://codereview.chromium.org/2811273003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:327: extra_fields = [defaultdict(lambda: None, item) for item in EXTRA_FIELDS] I still have concerns over this from https://codereview.chromium.org/2811253002. Treating this bit as separate for the review.
https://codereview.chromium.org/2811273003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): https://codereview.chromium.org/2811273003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:321: property_['has_custom_compare_and_copy'] = False On 2017/04/19 at 04:40:15, alancutter wrote: > # All CSS properties that are generated do not have custom comparison and copy logic. Done. https://codereview.chromium.org/2811273003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): https://codereview.chromium.org/2811273003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:327: extra_fields = [defaultdict(lambda: None, item) for item in EXTRA_FIELDS] On 2017/04/19 at 04:40:15, alancutter wrote: > I still have concerns over this from https://codereview.chromium.org/2811253002. > Treating this bit as separate for the review. Fixed in parent patch.
The CQ bit was checked by shend@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by shend@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by shend@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nainar@chromium.org, alancutter@chromium.org Link to the patchset: https://codereview.chromium.org/2811273003/#ps180001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 180001, "attempt_start_ts": 1492727405611570, "parent_rev": "014aec7ea81e1725b6e7fc9ae7d8654d8ee8f6a7", "commit_rev": "abefb698e5e6ad81853ffdd0560c9efa4740464d"}
Message was sent while issue was closed.
Description was changed from ========== Allow (non)property fields to be hardcoded in make_computed_style_base. Currently we divide ComputedStyle fields into two groups: properties and nonproperties. Properties represent actual CSS properties and are read from CSSProperties.json5. Nonproperties represent fields that do not correspond to a CSS property and are read from a hardcoded list in the generator script. However, some fields act like properties, but do not correspond to CSS properties (e.g. margin is a combination of four properties, but it itself is not a CSS property). These need to be stored in the hardcoded list. But since everything in the hardcoded list is considered a nonproperty, this will not generate the right code. Hence, this patch changes the hardcoded list to be rather a list of "extra" fields which can be property or nonproperty fields. To distinguish between properties and nonproperties, we introduce a has_custom_compare_and_copy parameter. If true, the field is a nonproperty. This is prework for replacing the concept of nonproperties with has_custom_compare_and_copy entirely. This patch does not affect existing behaviour. BUG=628043 ========== to ========== Allow (non)property fields to be hardcoded in make_computed_style_base. Currently we divide ComputedStyle fields into two groups: properties and nonproperties. Properties represent actual CSS properties and are read from CSSProperties.json5. Nonproperties represent fields that do not correspond to a CSS property and are read from a hardcoded list in the generator script. However, some fields act like properties, but do not correspond to CSS properties (e.g. margin is a combination of four properties, but it itself is not a CSS property). These need to be stored in the hardcoded list. But since everything in the hardcoded list is considered a nonproperty, this will not generate the right code. Hence, this patch changes the hardcoded list to be rather a list of "extra" fields which can be property or nonproperty fields. To distinguish between properties and nonproperties, we introduce a has_custom_compare_and_copy parameter. If true, the field is a nonproperty. This is prework for replacing the concept of nonproperties with has_custom_compare_and_copy entirely. This patch does not affect existing behaviour. BUG=628043 Review-Url: https://codereview.chromium.org/2811273003 Cr-Commit-Position: refs/heads/master@{#466166} Committed: https://chromium.googlesource.com/chromium/src/+/abefb698e5e6ad81853ffdd0560c... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/abefb698e5e6ad81853ffdd0560c... |