Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2830573002:
Use generated getters/setters in ComputedStyle instead of directly accessing fields (Closed)
DescriptionUse generated getters/setters in ComputedStyle instead of directly accessing fields
Use generated getters/setters from
https://codereview.chromium.org/2786883002 in ComputedStyle instead of directly
accessing fields.
It also marks all generated fields as private to ensure no direct usage.
BUG=628043
Review-Url: https://codereview.chromium.org/2830573002
Cr-Commit-Position: refs/heads/master@{#465973}
Committed: https://chromium.googlesource.com/chromium/src/+/c8b65a9c8a99a53561f8fd337f98e0bf5ac98f96
Patch Set 1 #Patch Set 2 : Make all generated fields private #
Total comments: 4
Patch Set 3 : Changes suggested by shend@ #
Total comments: 6
Patch Set 4 : nits #
Total comments: 12
Patch Set 5 : suzyh@'s comments #Patch Set 6 : Comments #
Total comments: 2
Patch Set 7 : Grammatical nit #Patch Set 8 : Grammatical nit #
Depends on Patchset: Messages
Total messages: 44 (27 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...
nainar@chromium.org changed reviewers: + shend@chromium.org
Darren, PTAL? Thanks!
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 ========== Use generated getters/setters in COmputedStyle instead of directly accessing fields Use generated getters/setters from https://codereview.chromium.org/2786883002 in ComputedStyle instead of directly accessing fields. BUG=628043 ========== to ========== Use generated getters/setters in COmputedStyle instead of directly accessing fields Use generated getters/setters from https://codereview.chromium.org/2786883002 in ComputedStyle instead of directly accessing fields. It also marks all generated fields as private to ensure no direct usage. BUG=628043 ==========
https://codereview.chromium.org/2830573002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right): https://codereview.chromium.org/2830573002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.cpp:348: SetVerticalAlignInternal(other.VerticalAlignInternal()); This is an interesting case. Both the previous and new code work. I think we need to make a decision about which getter/setter to call when there is a public getter that's the same as the internal getter. IMO, we should only use internal getters/setters as a last resort, but I don't mind as long as there is a consistent rule. https://codereview.chromium.org/2830573002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.cpp:2468: SetEmptyStateInternal(other.EmptyStateInternal()); Here is the problem with using internal getters. I don't think this code does the same thing. Previously, we would have called SetUnique from the SetEmptyState, but now we don't. So internal setters don't maintain class invariants, which could subtly break code. Surprisingly, tests don't break...
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...
Made the changes you asked for, PTAL? https://codereview.chromium.org/2830573002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right): https://codereview.chromium.org/2830573002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.cpp:348: SetVerticalAlignInternal(other.VerticalAlignInternal()); On 2017/04/19 at 06:17:30, shend wrote: > This is an interesting case. Both the previous and new code work. > I think we need to make a decision about which getter/setter to call when there is a public getter that's the same as the internal getter. > > IMO, we should only use internal getters/setters as a last resort, but I don't mind as long as there is a consistent rule. Replied in the next comment.Also done. https://codereview.chromium.org/2830573002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.cpp:2468: SetEmptyStateInternal(other.EmptyStateInternal()); Yup, I see what you mean. Using public getters is best when they are available. Made the change Also yeah at lack of test coverage.
lgtm with nits :) https://codereview.chromium.org/2830573002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right): https://codereview.chromium.org/2830573002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.cpp:483: VerticalAlignInternal() == Use public getter. https://codereview.chromium.org/2830573002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.cpp:854: if (box_->vertical_align_ != other.box_->vertical_align_) Use public getter. https://codereview.chromium.org/2830573002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.cpp:861: if (VerticalAlignInternal() != other.VerticalAlignInternal() || Use the public getter.
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: + suzyh@chromium.org
Sending for OWNERS with shend@'s changes incorporated. https://codereview.chromium.org/2830573002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right): https://codereview.chromium.org/2830573002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.cpp:483: VerticalAlignInternal() == Done. https://codereview.chromium.org/2830573002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.cpp:854: if (box_->vertical_align_ != other.box_->vertical_align_) Done. https://codereview.chromium.org/2830573002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.cpp:861: if (VerticalAlignInternal() != other.VerticalAlignInternal() || Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2830573002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right): https://codereview.chromium.org/2830573002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.cpp:483: VerticalAlign() == other.VerticalAlign() && // Not generated in Was this an auto-formatting change? https://codereview.chromium.org/2830573002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.cpp:2025: // using m_hasSimpleUnderlineInternal. Was this intentional? https://codereview.chromium.org/2830573002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/ComputedStyle.h (right): https://codereview.chromium.org/2830573002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.h:3697: return PseudoBitsInternal(); It looks like pseudo_bits_ is a PseudoId, not a bool. Now that there is an extra layer of indirection here, is there value in making the conversion explicit? https://codereview.chromium.org/2830573002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.h:3710: PseudoBitsInternal() | 1 << (pseudo - kFirstPublicPseudoId))); This is getting nasty. Consider adding a TODO to make the operations on these enums easier to grok.
Suzy, Made the changes you asked for, PTAL? Thanks! https://codereview.chromium.org/2830573002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right): https://codereview.chromium.org/2830573002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.cpp:483: VerticalAlign() == other.VerticalAlign() && // Not generated in Yup. https://codereview.chromium.org/2830573002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.cpp:2025: // using m_hasSimpleUnderlineInternal. Nope, reverted. https://codereview.chromium.org/2830573002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/ComputedStyle.h (right): https://codereview.chromium.org/2830573002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.h:3697: return PseudoBitsInternal(); Do you mean cast PseudoBitsInternal() to a bool? https://codereview.chromium.org/2830573002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.h:3710: PseudoBitsInternal() | 1 << (pseudo - kFirstPublicPseudoId))); Added a comment explaining what the code represents here.
Plus a typo in the CL headline: COmputedStyle (extra capitalisation). https://codereview.chromium.org/2830573002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/ComputedStyle.h (right): https://codereview.chromium.org/2830573002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.h:3697: return PseudoBitsInternal(); On 2017/04/20 at 03:06:16, nainar wrote: > Do you mean cast PseudoBitsInternal() to a bool? I'm actually not sure what the right way to do it is. I suppose you could also do return PseudoBitsInternal() != kPseudoIdNone ? https://codereview.chromium.org/2830573002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.h:3710: PseudoBitsInternal() | 1 << (pseudo - kFirstPublicPseudoId))); On 2017/04/20 at 03:06:16, nainar wrote: > Added a comment explaining what the code represents here. Hmm nope that's not really much more helpful. Before your patch this code was bit-twiddling black magic. Now it is obscured bit-twiddling black magic with a comment that may get out of date with the code. I think remove the comment again and just add a TODO to fix it up later. (I don't want to block this patch on fixing it.)
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...
Made the changes you asked for, PTAL? https://codereview.chromium.org/2830573002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/ComputedStyle.h (right): https://codereview.chromium.org/2830573002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.h:3697: return PseudoBitsInternal(); That works. Done https://codereview.chromium.org/2830573002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.h:3710: PseudoBitsInternal() | 1 << (pseudo - kFirstPublicPseudoId))); Done.
Description was changed from ========== Use generated getters/setters in COmputedStyle instead of directly accessing fields Use generated getters/setters from https://codereview.chromium.org/2786883002 in ComputedStyle instead of directly accessing fields. It also marks all generated fields as private to ensure no direct usage. BUG=628043 ========== to ========== Use generated getters/setters in ComputedStyle instead of directly accessing fields Use generated getters/setters from https://codereview.chromium.org/2786883002 in ComputedStyle instead of directly accessing fields. It also marks all generated fields as private to ensure no direct usage. BUG=628043 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm with one grammar nit https://codereview.chromium.org/2830573002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/style/ComputedStyle.h (right): https://codereview.chromium.org/2830573002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/style/ComputedStyle.h:3705: // TODO: Fix up this code it is hard to understand Grammar nit: "code it" -> "code; it"
Nit fixed. Landing now. https://codereview.chromium.org/2830573002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/style/ComputedStyle.h (right): https://codereview.chromium.org/2830573002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/style/ComputedStyle.h:3705: // TODO: Fix up this code it is hard to understand Done.
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, suzyh@chromium.org Link to the patchset: https://codereview.chromium.org/2830573002/#ps120001 (title: "Grammatical nit")
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
Try jobs failed on following builders: linux_chromium_tsan_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 nainar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from suzyh@chromium.org, shend@chromium.org Link to the patchset: https://codereview.chromium.org/2830573002/#ps140001 (title: "Grammatical nit")
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
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_...)
The CQ bit was checked by nainar@chromium.org
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": 140001, "attempt_start_ts": 1492685790112640,
"parent_rev": "95f34789951a95ea306db1b8e9b568696a3cacaa", "commit_rev":
"c8b65a9c8a99a53561f8fd337f98e0bf5ac98f96"}
Message was sent while issue was closed.
Description was changed from ========== Use generated getters/setters in ComputedStyle instead of directly accessing fields Use generated getters/setters from https://codereview.chromium.org/2786883002 in ComputedStyle instead of directly accessing fields. It also marks all generated fields as private to ensure no direct usage. BUG=628043 ========== to ========== Use generated getters/setters in ComputedStyle instead of directly accessing fields Use generated getters/setters from https://codereview.chromium.org/2786883002 in ComputedStyle instead of directly accessing fields. It also marks all generated fields as private to ensure no direct usage. BUG=628043 Review-Url: https://codereview.chromium.org/2830573002 Cr-Commit-Position: refs/heads/master@{#465973} Committed: https://chromium.googlesource.com/chromium/src/+/c8b65a9c8a99a53561f8fd337f98... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/c8b65a9c8a99a53561f8fd337f98... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
