Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2876353002:
Generate diffing functions for generated subgroup BoxData in ComputedStyle (Closed)
DescriptionGenerate diffing functions for generated subgroup BoxData in ComputedStyle
This patch generates the diffing functions for StyleBoxData which is now
a generated subgroup of ComputedStyle.
Please note that this patch doesn't generate the diffing for
VerticalAlign as it is accessed through both BoxData (when a length) and
a separate getter on ComputedStyle (when a keyword.) This will be
generated once we can generate diffs using functions.
Please note: No added callsite for ComputedStyleBase::ScrollAnchorDisablingPropertyChanged has been added
since it is already being called. We have just appended to that JSON
entry.
Diff: https://gist.github.com/5701f6d9a1be397677cbde5ad7033259/revisions
BUG=710938
Review-Url: https://codereview.chromium.org/2876353002
Cr-Commit-Position: refs/heads/master@{#471683}
Committed: https://chromium.googlesource.com/chromium/src/+/9937b524e266fe058f637e442f96efba14b35c1f
Patch Set 1 #
Total comments: 2
Patch Set 2 : Formatting #
Total comments: 4
Depends on Patchset: Messages
Total messages: 24 (15 generated)
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 diffing functions for generated subgroup BoxData in ComputedStyle This patch generates the diffing functions for StyleBoxData which is now a generated subgroup of ComputedStyle. Please note that this patch doesn't generate the diffing for VerticalAlign as it is accessed through both BoxData (when a length) and a separate getter on ComputedStyle (when a keyword.) This will be generated once we can generate diffs using functions. BUG=710938 ========== to ========== Generate diffing functions for generated subgroup BoxData in ComputedStyle This patch generates the diffing functions for StyleBoxData which is now a generated subgroup of ComputedStyle. Please note that this patch doesn't generate the diffing for VerticalAlign as it is accessed through both BoxData (when a length) and a separate getter on ComputedStyle (when a keyword.) This will be generated once we can generate diffs using functions. Diff: https://gist.github.com/5701f6d9a1be397677cbde5ad7033259/revisions Known Issue: the formatting of the diff isn't correct. Will work on this in a separate CL. BUG=710938 ==========
nainar@chromium.org changed reviewers: + shend@chromium.org
shend@, PTAL? Thanks!
lgtm once the ordering is fixed. https://codereview.chromium.org/2876353002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/style/ComputedStyle.cpp (left): https://codereview.chromium.org/2876353002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/style/ComputedStyle.cpp:821: if (box_data_->width_ != other.box_data_->width_ || Hmm, why do the generated code have a different order?
nainar@chromium.org changed reviewers: + meade@chromium.org
meade@, PTAL? Thanks! https://codereview.chromium.org/2876353002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/style/ComputedStyle.cpp (left): https://codereview.chromium.org/2876353002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/style/ComputedStyle.cpp:821: if (box_data_->width_ != other.box_data_->width_ || On 2017/05/15 at 01:40:22, shend wrote: > Hmm, why do the generated code have a different order? This is because the order is determined by the order in which they are stored on the group. Ordering shouldn't matter.
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
lgtm with nit https://codereview.chromium.org/2876353002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/ComputedStyle.cpp (left): https://codereview.chromium.org/2876353002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.cpp:561: if (box_data_.Get() != other.box_data_.Get()) { This doesn't need to be replaced with anything? :o Would be worth putting a comment in the description for why?
https://codereview.chromium.org/2876353002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/ComputedStyle.cpp (left): https://codereview.chromium.org/2876353002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.cpp:561: if (box_data_.Get() != other.box_data_.Get()) { On 2017/05/15 at 04:07:45, meade_UTC10 wrote: > This doesn't need to be replaced with anything? :o Would be worth putting a comment in the description for why? That's because we are already calling ComputedStyleBase::ScrollAnchorDisablingPropertyChanged we just added the box properties to that list. Do you think that needs to be clarified in the CL description? The call site is directly below the removed code.
https://codereview.chromium.org/2876353002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/ComputedStyle.cpp (left): https://codereview.chromium.org/2876353002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.cpp:561: if (box_data_.Get() != other.box_data_.Get()) { On 2017/05/15 04:10:49, nainar wrote: > On 2017/05/15 at 04:07:45, meade_UTC10 wrote: > > This doesn't need to be replaced with anything? :o Would be worth putting a > comment in the description for why? > > That's because we are already calling > ComputedStyleBase::ScrollAnchorDisablingPropertyChanged we just added the box > properties to that list. > > Do you think that needs to be clarified in the CL description? The call site is > directly below the removed code. It's not obvious to me that this is equivalent - if you just look at this file, it looks like you're only adding a new call in ComputedStyle::DiffNeedsFullLayout. A 1-sentence note to say that you moved the box-data properties to the generated ScrollAnchorDisablingPropertyChanged should be sufficient.
Description was changed from ========== Generate diffing functions for generated subgroup BoxData in ComputedStyle This patch generates the diffing functions for StyleBoxData which is now a generated subgroup of ComputedStyle. Please note that this patch doesn't generate the diffing for VerticalAlign as it is accessed through both BoxData (when a length) and a separate getter on ComputedStyle (when a keyword.) This will be generated once we can generate diffs using functions. Diff: https://gist.github.com/5701f6d9a1be397677cbde5ad7033259/revisions Known Issue: the formatting of the diff isn't correct. Will work on this in a separate CL. BUG=710938 ========== to ========== Generate diffing functions for generated subgroup BoxData in ComputedStyle This patch generates the diffing functions for StyleBoxData which is now a generated subgroup of ComputedStyle. Please note that this patch doesn't generate the diffing for VerticalAlign as it is accessed through both BoxData (when a length) and a separate getter on ComputedStyle (when a keyword.) This will be generated once we can generate diffs using functions. Please note: No added callsite for ComputedStyleBase::ScrollAnchorDisablingPropertyChanged has been added since it is already being called. We have just appended to that JSON entry. Diff: https://gist.github.com/5701f6d9a1be397677cbde5ad7033259/revisions Known Issue: the formatting of the diff isn't correct. Will work on this in a separate CL. BUG=710938 ==========
Description was changed from ========== Generate diffing functions for generated subgroup BoxData in ComputedStyle This patch generates the diffing functions for StyleBoxData which is now a generated subgroup of ComputedStyle. Please note that this patch doesn't generate the diffing for VerticalAlign as it is accessed through both BoxData (when a length) and a separate getter on ComputedStyle (when a keyword.) This will be generated once we can generate diffs using functions. Please note: No added callsite for ComputedStyleBase::ScrollAnchorDisablingPropertyChanged has been added since it is already being called. We have just appended to that JSON entry. Diff: https://gist.github.com/5701f6d9a1be397677cbde5ad7033259/revisions Known Issue: the formatting of the diff isn't correct. Will work on this in a separate CL. BUG=710938 ========== to ========== Generate diffing functions for generated subgroup BoxData in ComputedStyle This patch generates the diffing functions for StyleBoxData which is now a generated subgroup of ComputedStyle. Please note that this patch doesn't generate the diffing for VerticalAlign as it is accessed through both BoxData (when a length) and a separate getter on ComputedStyle (when a keyword.) This will be generated once we can generate diffs using functions. Please note: No added callsite for ComputedStyleBase::ScrollAnchorDisablingPropertyChanged has been added since it is already being called. We have just appended to that JSON entry. Diff: https://gist.github.com/5701f6d9a1be397677cbde5ad7033259/revisions BUG=710938 ==========
https://codereview.chromium.org/2876353002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/ComputedStyle.cpp (left): https://codereview.chromium.org/2876353002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.cpp:561: if (box_data_.Get() != other.box_data_.Get()) { On 2017/05/15 at 04:25:45, meade_UTC10 wrote: > On 2017/05/15 04:10:49, nainar wrote: > > On 2017/05/15 at 04:07:45, meade_UTC10 wrote: > > > This doesn't need to be replaced with anything? :o Would be worth putting a > > comment in the description for why? > > > > That's because we are already calling > > ComputedStyleBase::ScrollAnchorDisablingPropertyChanged we just added the box > > properties to that list. > > > > Do you think that needs to be clarified in the CL description? The call site is > > directly below the removed code. > > It's not obvious to me that this is equivalent - if you just look at this file, it looks like you're only adding a new call in ComputedStyle::DiffNeedsFullLayout. A 1-sentence note to say that you moved the box-data properties to the generated ScrollAnchorDisablingPropertyChanged should be sufficient. Sounds good. Added to the CL description. Thank you for the due diligence
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/2876353002/#ps20001 (title: "Formatting")
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": 20001, "attempt_start_ts": 1494829567704460,
"parent_rev": "9f9fcf0ea74f3184b59b09dc5a3ef9f684e3af64", "commit_rev":
"9937b524e266fe058f637e442f96efba14b35c1f"}
Message was sent while issue was closed.
Description was changed from ========== Generate diffing functions for generated subgroup BoxData in ComputedStyle This patch generates the diffing functions for StyleBoxData which is now a generated subgroup of ComputedStyle. Please note that this patch doesn't generate the diffing for VerticalAlign as it is accessed through both BoxData (when a length) and a separate getter on ComputedStyle (when a keyword.) This will be generated once we can generate diffs using functions. Please note: No added callsite for ComputedStyleBase::ScrollAnchorDisablingPropertyChanged has been added since it is already being called. We have just appended to that JSON entry. Diff: https://gist.github.com/5701f6d9a1be397677cbde5ad7033259/revisions BUG=710938 ========== to ========== Generate diffing functions for generated subgroup BoxData in ComputedStyle This patch generates the diffing functions for StyleBoxData which is now a generated subgroup of ComputedStyle. Please note that this patch doesn't generate the diffing for VerticalAlign as it is accessed through both BoxData (when a length) and a separate getter on ComputedStyle (when a keyword.) This will be generated once we can generate diffs using functions. Please note: No added callsite for ComputedStyleBase::ScrollAnchorDisablingPropertyChanged has been added since it is already being called. We have just appended to that JSON entry. Diff: https://gist.github.com/5701f6d9a1be397677cbde5ad7033259/revisions BUG=710938 Review-Url: https://codereview.chromium.org/2876353002 Cr-Commit-Position: refs/heads/master@{#471683} Committed: https://chromium.googlesource.com/chromium/src/+/9937b524e266fe058f637e442f96... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/9937b524e266fe058f637e442f96... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
