Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2735283002:
Combine ComputedStyle default ctor with initial style ctor. (Closed)
DescriptionCombine ComputedStyle default ctor with initial style ctor.
ComputedStyle has two constructors. The default constructor initialises
all the fields and copies the DataRefs from initialStyle(). The other
constructor creates the initial style, which initialises all the fields
and also allocates the memory for each of the DataRefs.
Having two constructors makes it more difficult to maintain and think
about, so this patch removes the default constructor, which can be
implemented instead by copying from the initial style.
BUG=628043
Review-Url: https://codereview.chromium.org/2735283002
Cr-Commit-Position: refs/heads/master@{#456836}
Committed: https://chromium.googlesource.com/chromium/src/+/fc3b987e456edc71cb5b4286ce0fa90599468468
Patch Set 1 #
Total comments: 8
Patch Set 2 : Rebase #
Messages
Total messages: 31 (16 generated)
The CQ bit was checked by shend@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_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
shend@chromium.org changed reviewers: + nainar@chromium.org
Hi Naina, PTAL
lgtm. On a side note wonder why your test results are invalid? :/
On 2017/03/08 at 12:23:18, nainar wrote: > lgtm. > > On a side note wonder why your test results are invalid? :/ Running trybots for the fourth time seems to have fixed it ¯\_(ツ)_/¯
shend@chromium.org changed reviewers: + meade@chromium.org
Hey Eddy, PTAL
meade@chromium.org changed reviewers: + sashab@chromium.org
This lgtm, but I'd like to hear Sasha's opinion on the static_assert thing. Sasha, could you PTAL? Thanks! https://codereview.chromium.org/2735283002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right): https://codereview.chromium.org/2735283002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/style/ComputedStyle.cpp:148: "NonInheritedData should not grow"); Is there a problem with moving these static_asserts to the other constructor? I'm a bit confused about why they are here in the first place. We semm to use that SameSizeAsFoo idiom elsewhere - perhaps that is more appropriate here? Sasha might have a stronger opinion than me...
https://codereview.chromium.org/2735283002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right): https://codereview.chromium.org/2735283002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/style/ComputedStyle.cpp:148: "NonInheritedData should not grow"); On 2017/03/09 at 05:43:13, Eddy wrote: > Is there a problem with moving these static_asserts to the other constructor? I'm a bit confused about why they are here in the first place. We semm to use that SameSizeAsFoo idiom elsewhere - perhaps that is more appropriate here? Sasha might have a stronger opinion than me... Yeah I don't know why they're in the constructor. In ComputedStyleBase, we put them after the SameAsSize struct. I can move them to be consistent with ComputedStyleBase.
sashab@chromium.org changed reviewers: + esprehn@chromium.org
I'm surprised this patch actually works. This *seems* like a good cleanup, but there is a performance cost from passing initialStyle() and not using the default constructor. Also, initialization lists are definitely preferred over the .init() methods, which are confusing and don't give you nice compiler warnings when you skip fields (and nicely roll back if initialization fails). Also, it's not so bad having two constructors, since when we generate it, we can generate them both. Added esprehn@ for his opinion too. https://codereview.chromium.org/2735283002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/style/ComputedStyle.cpp (left): https://codereview.chromium.org/2735283002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/style/ComputedStyle.cpp:98: return adoptRef(new ComputedStyle()); Does this have a perf cost? https://codereview.chromium.org/2735283002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right): https://codereview.chromium.org/2735283002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/style/ComputedStyle.cpp:148: "NonInheritedData should not grow"); On 2017/03/09 14:59:54, shend wrote: > On 2017/03/09 at 05:43:13, Eddy wrote: > > Is there a problem with moving these static_asserts to the other constructor? > I'm a bit confused about why they are here in the first place. We semm to use > that SameSizeAsFoo idiom elsewhere - perhaps that is more appropriate here? > Sasha might have a stronger opinion than me... > > Yeah I don't know why they're in the constructor. In ComputedStyleBase, we put > them after the SameAsSize struct. I can move them to be consistent with > ComputedStyleBase. Hmm, you can check git blame, but this looks like an artifact left over from before we had the SameSizeAs structs. You can actually just delete these altogether - since ComputedStyle is going away, anything added will be added to ComputedStyleBase, which has its own size check. https://codereview.chromium.org/2735283002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/style/ComputedStyle.h (left): https://codereview.chromium.org/2735283002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/style/ComputedStyle.h:308: // TODO(sashab): Move these to the bottom of ComputedStyle. Leave this TODO here - this is referring to the fact that these methods/fields are private, so ComputedStyle has two "public" sections which is against our style guide. Maybe reword it to clarify this.
On 2017/03/09 at 23:48:22, sashab wrote: > I'm surprised this patch actually works. This *seems* like a good cleanup, but there is a performance cost from passing initialStyle() and not using the default constructor. Also, initialization lists are definitely preferred over the .init() methods, which are confusing and don't give you nice compiler warnings when you skip fields (and nicely roll back if initialization fails). > > Also, it's not so bad having two constructors, since when we generate it, we can generate them both. > > Added esprehn@ for his opinion too. > > https://codereview.chromium.org/2735283002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/style/ComputedStyle.cpp (left): > > https://codereview.chromium.org/2735283002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/style/ComputedStyle.cpp:98: return adoptRef(new ComputedStyle()); > Does this have a perf cost? > > https://codereview.chromium.org/2735283002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right): > > https://codereview.chromium.org/2735283002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/style/ComputedStyle.cpp:148: "NonInheritedData should not grow"); > On 2017/03/09 14:59:54, shend wrote: > > On 2017/03/09 at 05:43:13, Eddy wrote: > > > Is there a problem with moving these static_asserts to the other constructor? > > I'm a bit confused about why they are here in the first place. We semm to use > > that SameSizeAsFoo idiom elsewhere - perhaps that is more appropriate here? > > Sasha might have a stronger opinion than me... > > > > Yeah I don't know why they're in the constructor. In ComputedStyleBase, we put > > them after the SameAsSize struct. I can move them to be consistent with > > ComputedStyleBase. > > Hmm, you can check git blame, but this looks like an artifact left over from before we had the SameSizeAs structs. > > You can actually just delete these altogether - since ComputedStyle is going away, anything added will be added to ComputedStyleBase, which has its own size check. > > https://codereview.chromium.org/2735283002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/style/ComputedStyle.h (left): > > https://codereview.chromium.org/2735283002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/style/ComputedStyle.h:308: // TODO(sashab): Move these to the bottom of ComputedStyle. > Leave this TODO here - this is referring to the fact that these methods/fields are private, so ComputedStyle has two "public" sections which is against our style guide. Maybe reword it to clarify this. Hmm, I don't think there's much perf cost, since we're passing initialStyle by reference. I agree with the initialiser lists, and it's something I'd like to change on the DataRef side (like have an option to directly initialise my DataRefs), although again the perf cost should not be big since we only call the initial ctor once. You're right about the fact that we're gonna generate code eventually anyway, but if there's no perf cost, I think we should pick the cleaner looking solution.
Oh, I can't believe I missed this before, but I just saw that the initializer lists are literally calling parts of initialStyle(). So these are literally doing the same thing, haha. LGTM, but since we really like the initializer lists, surely there's a way to rewrite the copy constructor to use initializer lists? I'm pretty sure you can do that in C++.
On 2017/03/10 at 01:02:30, sashab wrote: > Oh, I can't believe I missed this before, but I just saw that the initializer lists are literally calling parts of initialStyle(). So these are literally doing the same thing, haha. > > LGTM, but since we really like the initializer lists, surely there's a way to rewrite the copy constructor to use initializer lists? I'm pretty sure you can do that in C++. I think the copy ctor already uses initializer lists: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/style/Com...
On 2017/03/10 at 01:02:30, sashab wrote: > Oh, I can't believe I missed this before, but I just saw that the initializer lists are literally calling parts of initialStyle(). So these are literally doing the same thing, haha. > > LGTM, but since we really like the initializer lists, surely there's a way to rewrite the copy constructor to use initializer lists? I'm pretty sure you can do that in C++. I think the copy ctor already uses initializer lists: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/style/Com...
Thanks for clarifying this patch offline. LGTM but leave that TODO in, also please add a TODO to remove mutableInitialStyle().
https://codereview.chromium.org/2735283002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/style/ComputedStyle.cpp (left): https://codereview.chromium.org/2735283002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/style/ComputedStyle.cpp:98: return adoptRef(new ComputedStyle()); On 2017/03/09 at 23:48:22, sashab wrote: > Does this have a perf cost? Shouldn't affect perf since initialStyle() returns a cached initial style. https://codereview.chromium.org/2735283002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right): https://codereview.chromium.org/2735283002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/style/ComputedStyle.cpp:148: "NonInheritedData should not grow"); On 2017/03/09 at 23:48:22, sashab wrote: > On 2017/03/09 14:59:54, shend wrote: > > On 2017/03/09 at 05:43:13, Eddy wrote: > > > Is there a problem with moving these static_asserts to the other constructor? > > I'm a bit confused about why they are here in the first place. We semm to use > > that SameSizeAsFoo idiom elsewhere - perhaps that is more appropriate here? > > Sasha might have a stronger opinion than me... > > > > Yeah I don't know why they're in the constructor. In ComputedStyleBase, we put > > them after the SameAsSize struct. I can move them to be consistent with > > ComputedStyleBase. > > Hmm, you can check git blame, but this looks like an artifact left over from before we had the SameSizeAs structs. > > You can actually just delete these altogether - since ComputedStyle is going away, anything added will be added to ComputedStyleBase, which has its own size check. Done. https://codereview.chromium.org/2735283002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/style/ComputedStyle.h (left): https://codereview.chromium.org/2735283002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/style/ComputedStyle.h:308: // TODO(sashab): Move these to the bottom of ComputedStyle. On 2017/03/09 at 23:48:22, sashab wrote: > Leave this TODO here - this is referring to the fact that these methods/fields are private, so ComputedStyle has two "public" sections which is against our style guide. Maybe reword it to clarify this. Done.
The CQ bit was checked by shend@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: This issue passed the CQ dry run.
The CQ bit was checked by shend@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from meade@chromium.org, nainar@chromium.org, sashab@chromium.org Link to the patchset: https://codereview.chromium.org/2735283002/#ps20001 (title: "Rebase")
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": 1489526057207650,
"parent_rev": "761f60864c37e7af6fe9dba49256a8b014860049", "commit_rev":
"fc3b987e456edc71cb5b4286ce0fa90599468468"}
Message was sent while issue was closed.
Description was changed from ========== Combine ComputedStyle default ctor with initial style ctor. ComputedStyle has two constructors. The default constructor initialises all the fields and copies the DataRefs from initialStyle(). The other constructor creates the initial style, which initialises all the fields and also allocates the memory for each of the DataRefs. Having two constructors makes it more difficult to maintain and think about, so this patch removes the default constructor, which can be implemented instead by copying from the initial style. BUG=628043 ========== to ========== Combine ComputedStyle default ctor with initial style ctor. ComputedStyle has two constructors. The default constructor initialises all the fields and copies the DataRefs from initialStyle(). The other constructor creates the initial style, which initialises all the fields and also allocates the memory for each of the DataRefs. Having two constructors makes it more difficult to maintain and think about, so this patch removes the default constructor, which can be implemented instead by copying from the initial style. BUG=628043 Review-Url: https://codereview.chromium.org/2735283002 Cr-Commit-Position: refs/heads/master@{#456836} Committed: https://chromium.googlesource.com/chromium/src/+/fc3b987e456edc71cb5b4286ce0f... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/fc3b987e456edc71cb5b4286ce0f... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
