Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2902433002:
Generates predicates to test in diff functions in ComputedStyle (Closed)
DescriptionGenerates predicates to test in diff functions in ComputedStyle
This patch adds the ability to generate the predicates that encapsulate
diffing logic when generating diff functions in ComputedStyle.
We specify the predicates to test as a list of predicates to test in
ComputedStyleDiffFunctions.json5.
A diff has been generated for two predicates for properties stored on
StyleInheritedData. Further generation will be done in a separate patch.
Diff: https://gist.github.com/nainar/e8956f13f3569d2195183513ffa11600/revisions
BUG=710938
Review-Url: https://codereview.chromium.org/2902433002
Cr-Commit-Position: refs/heads/master@{#474567}
Committed: https://chromium.googlesource.com/chromium/src/+/2ee241f419323d7dd924c8bf4b4506189b0319ab
Patch Set 1 #
Total comments: 7
Patch Set 2 : alancutter@'s suggestions #
Total comments: 4
Patch Set 3 : Rebase #Patch Set 4 : format changes #
Messages
Total messages: 33 (22 generated)
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
The CQ bit was unchecked by nainar@chromium.org
nainar@chromium.org changed reviewers: + shend@chromium.org
shend@, PTAL? Thanks! https://codereview.chromium.org/2902433002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5 (right): https://codereview.chromium.org/2902433002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5:60: predicate: "TextShadowDataEquivalent(other)", could add the other as an argument bit in field.tmpl but then will have to pass this in without brackets. so :/
lgtm https://codereview.chromium.org/2902433002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5 (right): https://codereview.chromium.org/2902433002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5:60: predicate: "TextShadowDataEquivalent(other)", On 2017/05/22 at 03:45:29, nainar wrote: > could add the other as an argument bit in field.tmpl but then will have to pass this in without brackets. so :/ I would prefer not writing "other" in here, but it may not be possible if predicates can take arguments.
nainar@google.com changed reviewers: + alancutter@chromium.org, nainar@google.com
alancutter@, PTAL? Thanks! https://codereview.chromium.org/2902433002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5 (right): https://codereview.chromium.org/2902433002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5:60: predicate: "TextShadowDataEquivalent(other)", On 2017/05/22 at 03:51:05, shend wrote: > On 2017/05/22 at 03:45:29, nainar wrote: > > could add the other as an argument bit in field.tmpl but then will have to pass this in without brackets. so :/ > > I would prefer not writing "other" in here, but it may not be possible if predicates can take arguments. Yeah - I think leaving it here for the MVP and then seeing what way to go once we have moved everything over is the best call to make at this stage.
nainar@chromium.org changed reviewers: - nainar@google.com
rune@opera.com changed reviewers: + rune@opera.com
rs lgtm
The CQ bit was checked by nainar@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: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) 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_...)
https://codereview.chromium.org/2902433002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5 (right): https://codereview.chromium.org/2902433002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5:24: // which lists the properties this predicate test depends on. List the config fields with bullet points or a skeleton example to make it easier to find quickly. Mention the availability of the "other" variable.
lgtm https://codereview.chromium.org/2902433002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): https://codereview.chromium.org/2902433002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:210: field_dependencies = _list_field_dependencies(methods_to_diff) + _list_field_dependencies(predicates_to_test) The + can go inside the brackets instead if you want to make this line a bit shorter.
alancutter@, Made the changes you asked for. Waiting on shend@ to land his CL before landing this. https://codereview.chromium.org/2902433002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): https://codereview.chromium.org/2902433002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:210: field_dependencies = _list_field_dependencies(methods_to_diff) + _list_field_dependencies(predicates_to_test) On 2017/05/24 at 07:43:18, alancutter wrote: > The + can go inside the brackets instead if you want to make this line a bit shorter. Ooohh yeah good catch. Done. https://codereview.chromium.org/2902433002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5 (right): https://codereview.chromium.org/2902433002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5:24: // which lists the properties this predicate test depends on. On 2017/05/24 at 07:29:36, alancutter wrote: > List the config fields with bullet points or a skeleton example to make it easier to find quickly. > Mention the availability of the "other" variable. Done.
The CQ bit was checked by nainar@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) 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 nainar@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...
https://codereview.chromium.org/2902433002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5 (right): https://codereview.chromium.org/2902433002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5:24: // The fields are: No need for this line, just end the previous line with :. https://codereview.chromium.org/2902433002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5:26: // 2. field_dependencies -lists the properties this predicate test depends on. Missing space. Maybe use : instead of - to match what it'll actually look like.
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
alancutter@, Done. https://codereview.chromium.org/2902433002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5 (right): https://codereview.chromium.org/2902433002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5:24: // The fields are: Done. https://codereview.chromium.org/2902433002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5:26: // 2. field_dependencies -lists the properties this predicate test depends on. Done.
The CQ bit was unchecked by nainar@chromium.org
The CQ bit was checked by nainar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alancutter@chromium.org, rune@opera.com, shend@chromium.org Link to the patchset: https://codereview.chromium.org/2902433002/#ps60001 (title: "format changes")
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": 60001, "attempt_start_ts": 1495677872314790,
"parent_rev": "c96775196dce1f2705df5f7b3b2571c383759733", "commit_rev":
"2ee241f419323d7dd924c8bf4b4506189b0319ab"}
Message was sent while issue was closed.
Description was changed from ========== Generates predicates to test in diff functions in ComputedStyle This patch adds the ability to generate the predicates that encapsulate diffing logic when generating diff functions in ComputedStyle. We specify the predicates to test as a list of predicates to test in ComputedStyleDiffFunctions.json5. A diff has been generated for two predicates for properties stored on StyleInheritedData. Further generation will be done in a separate patch. Diff: https://gist.github.com/nainar/e8956f13f3569d2195183513ffa11600/revisions BUG=710938 ========== to ========== Generates predicates to test in diff functions in ComputedStyle This patch adds the ability to generate the predicates that encapsulate diffing logic when generating diff functions in ComputedStyle. We specify the predicates to test as a list of predicates to test in ComputedStyleDiffFunctions.json5. A diff has been generated for two predicates for properties stored on StyleInheritedData. Further generation will be done in a separate patch. Diff: https://gist.github.com/nainar/e8956f13f3569d2195183513ffa11600/revisions BUG=710938 Review-Url: https://codereview.chromium.org/2902433002 Cr-Commit-Position: refs/heads/master@{#474567} Committed: https://chromium.googlesource.com/chromium/src/+/2ee241f419323d7dd924c8bf4b45... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/2ee241f419323d7dd924c8bf4b45... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
