Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2889353002:
Generate diffs for properties that are generated in ComputedStyle (Closed)
DescriptionGenerate diffs for properties that are generated in ComputedStyle
This patch generates the diffing functions for properties:
1. that are stored directly on ComputedStyle
2. that were left behind when previously generating diffs for
properties that are on generated groups.
Diff: https://gist.github.com/nainar/7ee584319877b3da59a04b80a1690908/revisions
BUG=710938
Review-Url: https://codereview.chromium.org/2889353002
Cr-Commit-Position: refs/heads/master@{#474227}
Committed: https://chromium.googlesource.com/chromium/src/+/dd5069631f086ea22e7c43d9a1f85b0a83fc87df
Patch Set 1 #Patch Set 2 : Only check not inherited fields and fix fonts #
Total comments: 2
Patch Set 3 : Moved GetPosition() to ScrollAnchorDisablingPropertyChanged() #
Total comments: 9
Patch Set 4 : alancutter@ suggestions #
Messages
Total messages: 31 (20 generated)
Description was changed from ========== Generate diffs for properties that are generated in ComputedStyle This patch generates the diffing functions for properties: 1. that are stored directly on ComputedStyle 2. that were left behind when previously generating diffs for properties that are on generated groups. Diff: https://gist.github.com/ee75c8958823e2678e01bc67c281ba52/revisions BUG=710938 ========== to ========== Generate diffs for properties that are generated in ComputedStyle This patch generates the diffing functions for properties: 1. that are stored directly on ComputedStyle 2. that were left behind when previously generating diffs for properties that are on generated groups. Diff: https://gist.github.com/nainar/a44aaa08fb38508286d34e70163fa74d/revisions BUG=710938 ==========
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...
nainar@chromium.org changed reviewers: + shend@chromium.org
shend@, PTAL? Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2889353002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5 (right): https://codereview.chromium.org/2889353002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5:53: { These should be in DiffNeedsFullLayoutAndPaintInvalidation I think.
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...
shend@, Made the changes. PTAL? Thanks! https://codereview.chromium.org/2889353002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5 (right): https://codereview.chromium.org/2889353002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5:53: { On 2017/05/22 at 02:48:24, shend wrote: > These should be in DiffNeedsFullLayoutAndPaintInvalidation I think. Moved GetPosition() to ScrollAnchorDisablingPropertyChanged()
Description was changed from ========== Generate diffs for properties that are generated in ComputedStyle This patch generates the diffing functions for properties: 1. that are stored directly on ComputedStyle 2. that were left behind when previously generating diffs for properties that are on generated groups. Diff: https://gist.github.com/nainar/a44aaa08fb38508286d34e70163fa74d/revisions BUG=710938 ========== to ========== Generate diffs for properties that are generated in ComputedStyle This patch generates the diffing functions for properties: 1. that are stored directly on ComputedStyle 2. that were left behind when previously generating diffs for properties that are on generated groups. Diff: https://gist.github.com/nainar/7ee584319877b3da59a04b80a1690908/revisions BUG=710938 ==========
lgtm
nainar@chromium.org changed reviewers: + alancutter@chromium.org
alancutter@, PTAL? Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2889353002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): https://codereview.chromium.org/2889353002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:210: if field.is_property: I don't fully understand this. You add field_dependencies like "StyleType". Is that field considered a property? How does its corresponding method get included? https://codereview.chromium.org/2889353002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5 (right): https://codereview.chromium.org/2889353002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5:114: }, Let's sort these alphabetically. https://codereview.chromium.org/2889353002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right): https://codereview.chromium.org/2889353002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.cpp:922: if (Resize() != other.Resize()) Why not generate this part as well?
alancutter@ responded to your comments. https://codereview.chromium.org/2889353002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): https://codereview.chromium.org/2889353002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:210: if field.is_property: On 2017/05/24 at 03:31:17, alancutter wrote: > I don't fully understand this. You add field_dependencies like "StyleType". Is that field considered a property? How does its corresponding method get included? field.is_property is a badly named field that indicates that this field isn't inherited. It contains both properties and other things like StyleType stored on ComputedStyle. Could change this to read !field.is_inherited_flag if you think that would make it easy to read https://codereview.chromium.org/2889353002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5 (right): https://codereview.chromium.org/2889353002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5:114: }, On 2017/05/24 at 03:31:17, alancutter wrote: > Let's sort these alphabetically. We want them sorted in order in which they appeared in the diff function. https://codereview.chromium.org/2889353002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right): https://codereview.chromium.org/2889353002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.cpp:922: if (Resize() != other.Resize()) On 2017/05/24 at 03:31:17, alancutter wrote: > Why not generate this part as well? The property this depends on is not generated as of yet
lgtm https://codereview.chromium.org/2889353002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): https://codereview.chromium.org/2889353002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:210: if field.is_property: On 2017/05/24 at 06:07:21, nainar wrote: > On 2017/05/24 at 03:31:17, alancutter wrote: > > I don't fully understand this. You add field_dependencies like "StyleType". Is that field considered a property? How does its corresponding method get included? > > field.is_property is a badly named field that indicates that this field isn't inherited. It contains both properties and other things like StyleType stored on ComputedStyle. > > Could change this to read !field.is_inherited_flag if you think that would make it easy to read Yes please, that sounds like it would be much better. Can be done in separate patch. https://codereview.chromium.org/2889353002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5 (right): https://codereview.chromium.org/2889353002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5:114: }, On 2017/05/24 at 06:07:21, nainar wrote: > On 2017/05/24 at 03:31:17, alancutter wrote: > > Let's sort these alphabetically. > > We want them sorted in order in which they appeared in the diff function. Fair point, that's probably important.
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...
alancutter@, Made the changes you asked for. https://codereview.chromium.org/2889353002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): https://codereview.chromium.org/2889353002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:210: if field.is_property: On 2017/05/24 at 07:25:36, alancutter wrote: > On 2017/05/24 at 06:07:21, nainar wrote: > > On 2017/05/24 at 03:31:17, alancutter wrote: > > > I don't fully understand this. You add field_dependencies like "StyleType". Is that field considered a property? How does its corresponding method get included? > > > > field.is_property is a badly named field that indicates that this field isn't inherited. It contains both properties and other things like StyleType stored on ComputedStyle. > > > > Could change this to read !field.is_inherited_flag if you think that would make it easy to read > > Yes please, that sounds like it would be much better. Can be done in separate patch. Done in this patch.
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 nainar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alancutter@chromium.org, shend@chromium.org Link to the patchset: https://codereview.chromium.org/2889353002/#ps60001 (title: "alancutter@ suggestions")
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": 1495618977140630,
"parent_rev": "d457f863f78941ed376b463ee39476b1a1852797", "commit_rev":
"dd5069631f086ea22e7c43d9a1f85b0a83fc87df"}
Message was sent while issue was closed.
Description was changed from ========== Generate diffs for properties that are generated in ComputedStyle This patch generates the diffing functions for properties: 1. that are stored directly on ComputedStyle 2. that were left behind when previously generating diffs for properties that are on generated groups. Diff: https://gist.github.com/nainar/7ee584319877b3da59a04b80a1690908/revisions BUG=710938 ========== to ========== Generate diffs for properties that are generated in ComputedStyle This patch generates the diffing functions for properties: 1. that are stored directly on ComputedStyle 2. that were left behind when previously generating diffs for properties that are on generated groups. Diff: https://gist.github.com/nainar/7ee584319877b3da59a04b80a1690908/revisions BUG=710938 Review-Url: https://codereview.chromium.org/2889353002 Cr-Commit-Position: refs/heads/master@{#474227} Committed: https://chromium.googlesource.com/chromium/src/+/dd5069631f086ea22e7c43d9a1f8... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/dd5069631f086ea22e7c43d9a1f8... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
