Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2933303002:
Generate diffs for all fields on StyleRareNonInheritedData (Closed)
|
Created:
3 years, 6 months ago by nainar Modified:
3 years, 6 months ago Reviewers:
shend CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, blink-reviews-style_chromium.org, chromium-reviews, dglazkov+blink, rwlbuis Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionGenerate diffs for all fields on StyleRareNonInheritedData
This patch generates diffs for all fields directly stored on
StyleRareNonInheritedData. A future patch will generate diffs for the
groups stored on StyleRareNonInheritedDatat and fields stored on those
groups.
Please note to acheive this some tests had to be turned into functions
so that they could be tested as predicates.
Diff: https://gist.github.com/nainar/024af3fe4f7e834f0342c4000aaaa57f/revisions
BUG=710938
Review-Url: https://codereview.chromium.org/2933303002
Cr-Commit-Position: refs/heads/master@{#479313}
Committed: https://chromium.googlesource.com/chromium/src/+/c4efddc762cbbfb2bce13cfcb49a1e9a589de079
Patch Set 1 #
Total comments: 10
Patch Set 2 : shend@ suggestions #
Total comments: 6
Patch Set 3 : nits #Patch Set 4 : fix failing tests #
Dependent Patchsets: Messages
Total messages: 25 (17 generated)
nainar@chromium.org changed reviewers: + shend@chromium.org
shend@, PTAL? Thanks!
https://codereview.chromium.org/2933303002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right): https://codereview.chromium.org/2933303002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/style/ComputedStyle.cpp:593: // We only need do layout for opacity changes if adding or losing opacity Would generate this but what about the comment? https://codereview.chromium.org/2933303002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/style/ComputedStyle.cpp:775: bool has_clip = HasOutOfFlowPosition() && !visual_data_->has_auto_clip_; Doing this in a separate CL. Question should I move to this to a separate predicate or just remove references to the group here.
https://codereview.chromium.org/2933303002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5 (right): https://codereview.chromium.org/2933303002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5:194: "max-height", "VerticalAlignLength", "box-sizing", "align-content", nit: indent these so that they look like they're continuing from the previous line. https://codereview.chromium.org/2933303002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5:216: "-webkit-user-drag", "object-fit", "object-position"], nit: same here. https://codereview.chromium.org/2933303002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right): https://codereview.chromium.org/2933303002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/style/ComputedStyle.cpp:593: // We only need do layout for opacity changes if adding or losing opacity On 2017/06/13 at 07:55:42, nainar wrote: > Would generate this but what about the comment? Add to JSON :) https://codereview.chromium.org/2933303002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/style/ComputedStyle.cpp:775: bool has_clip = HasOutOfFlowPosition() && !visual_data_->has_auto_clip_; On 2017/06/13 at 07:55:42, nainar wrote: > Doing this in a separate CL. Question should I move to this to a separate predicate or just remove references to the group here. Make predicate and generate diff :)
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@, PTAL? Thanks! https://codereview.chromium.org/2933303002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5 (right): https://codereview.chromium.org/2933303002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5:194: "max-height", "VerticalAlignLength", "box-sizing", "align-content", On 2017/06/13 at 08:15:53, shend wrote: > nit: indent these so that they look like they're continuing from the previous line. Done. https://codereview.chromium.org/2933303002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/ComputedStyleDiffFunctions.json5:216: "-webkit-user-drag", "object-fit", "object-position"], On 2017/06/13 at 08:15:53, shend wrote: > nit: same here. Done. https://codereview.chromium.org/2933303002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right): https://codereview.chromium.org/2933303002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/style/ComputedStyle.cpp:593: // We only need do layout for opacity changes if adding or losing opacity On 2017/06/13 at 08:15:53, shend wrote: > On 2017/06/13 at 07:55:42, nainar wrote: > > Would generate this but what about the comment? > > Add to JSON :) Moved to predicate function and generating test for that predicate https://codereview.chromium.org/2933303002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/style/ComputedStyle.cpp:775: bool has_clip = HasOutOfFlowPosition() && !visual_data_->has_auto_clip_; On 2017/06/13 at 08:15:53, shend wrote: > On 2017/06/13 at 07:55:42, nainar wrote: > > Doing this in a separate CL. Question should I move to this to a separate predicate or just remove references to the group here. > > Make predicate and generate diff :) Will do this in a separate CL - since its on VisualData
lgtm with nits. https://codereview.chromium.org/2933303002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right): https://codereview.chromium.org/2933303002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.cpp:637: return true; nit: Could just be "return ComputedStyleBase..." https://codereview.chromium.org/2933303002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.cpp:643: if (ComputedStyleBase::DiffNeedsPaintInvalidationSubtree(*this, other)) nit: same here. https://codereview.chromium.org/2933303002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.cpp:706: if (ComputedStyleBase::DiffNeedsVisualRectUpdate(*this, other)) nit: Same here.
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...
Description was changed from ========== Generate diffs for all fields on StyleRareNonInheritedData This patch generates diffs for all fields directly stored on StyleRareNonInheritedData. A future patch will generate diffs for the groups stored on StyleRareNonInheritedDatat and fields stored on those groups. Please note to acheive this some tests had to be turned into functions so that they could be tested as predicates. Diff:https://gist.github.com/c956fede5040bc966c1aa78897cfa6a4/revisions BUG=710938 ========== to ========== Generate diffs for all fields on StyleRareNonInheritedData This patch generates diffs for all fields directly stored on StyleRareNonInheritedData. A future patch will generate diffs for the groups stored on StyleRareNonInheritedDatat and fields stored on those groups. Please note to acheive this some tests had to be turned into functions so that they could be tested as predicates. Diff:https://gist.github.com/nainar/024af3fe4f7e834f0342c4000aaaa57f/revisions BUG=710938 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== Generate diffs for all fields on StyleRareNonInheritedData This patch generates diffs for all fields directly stored on StyleRareNonInheritedData. A future patch will generate diffs for the groups stored on StyleRareNonInheritedDatat and fields stored on those groups. Please note to acheive this some tests had to be turned into functions so that they could be tested as predicates. Diff:https://gist.github.com/nainar/024af3fe4f7e834f0342c4000aaaa57f/revisions BUG=710938 ========== to ========== Generate diffs for all fields on StyleRareNonInheritedData This patch generates diffs for all fields directly stored on StyleRareNonInheritedData. A future patch will generate diffs for the groups stored on StyleRareNonInheritedDatat and fields stored on those groups. Please note to acheive this some tests had to be turned into functions so that they could be tested as predicates. Diff: https://gist.github.com/nainar/024af3fe4f7e834f0342c4000aaaa57f/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...
Published a new CL. Weh I took out code into the predicates I didnt flip the != to == https://codereview.chromium.org/2933303002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right): https://codereview.chromium.org/2933303002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.cpp:637: return true; On 2017/06/14 at 00:33:35, shend wrote: > nit: Could just be "return ComputedStyleBase..." Done https://codereview.chromium.org/2933303002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.cpp:643: if (ComputedStyleBase::DiffNeedsPaintInvalidationSubtree(*this, other)) On 2017/06/14 at 00:33:35, shend wrote: > nit: same here. Done https://codereview.chromium.org/2933303002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.cpp:706: if (ComputedStyleBase::DiffNeedsVisualRectUpdate(*this, other)) On 2017/06/14 at 00:33:34, shend wrote: > nit: Same here. Done
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 shend@chromium.org Link to the patchset: https://codereview.chromium.org/2933303002/#ps60001 (title: "fix failing tests")
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": 1497424100346210,
"parent_rev": "2f71c9d3211c9a05c8a3d9046717ed785352f592", "commit_rev":
"c4efddc762cbbfb2bce13cfcb49a1e9a589de079"}
Message was sent while issue was closed.
Description was changed from ========== Generate diffs for all fields on StyleRareNonInheritedData This patch generates diffs for all fields directly stored on StyleRareNonInheritedData. A future patch will generate diffs for the groups stored on StyleRareNonInheritedDatat and fields stored on those groups. Please note to acheive this some tests had to be turned into functions so that they could be tested as predicates. Diff: https://gist.github.com/nainar/024af3fe4f7e834f0342c4000aaaa57f/revisions BUG=710938 ========== to ========== Generate diffs for all fields on StyleRareNonInheritedData This patch generates diffs for all fields directly stored on StyleRareNonInheritedData. A future patch will generate diffs for the groups stored on StyleRareNonInheritedDatat and fields stored on those groups. Please note to acheive this some tests had to be turned into functions so that they could be tested as predicates. Diff: https://gist.github.com/nainar/024af3fe4f7e834f0342c4000aaaa57f/revisions BUG=710938 Review-Url: https://codereview.chromium.org/2933303002 Cr-Commit-Position: refs/heads/master@{#479313} Committed: https://chromium.googlesource.com/chromium/src/+/c4efddc762cbbfb2bce13cfcb49a... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/c4efddc762cbbfb2bce13cfcb49a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
