Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2739553003:
Make ComputedStyleBase abstract. (Closed)
DescriptionMake ComputedStyleBase abstract.
Currently ComputedStyleBase can be instantiated, and a ComputedStyle can
be deleted via a pointer to ComputedStyleBase. Both of these could cause
issues since ComputedStyleBase should be an abstract class.
This patch moves the ComputedStyleBase constructor and destructor to be
protected so that only ComputedStyle can call them.
BUG=628043
Review-Url: https://codereview.chromium.org/2739553003
Cr-Commit-Position: refs/heads/master@{#457579}
Committed: https://chromium.googlesource.com/chromium/src/+/ffd34d347dc1889ae692610c4d35cc2fc97ca9c0
Patch Set 1 #
Total comments: 2
Patch Set 2 : Make abstract #Patch Set 3 : Rebase #
Total comments: 1
Messages
Total messages: 37 (20 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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
shend@chromium.org changed reviewers: + meade@chromium.org
Hi Eddy, PTAL
https://codereview.chromium.org/2739553003/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl (left): https://codereview.chromium.org/2739553003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:30: ~ComputedStyleBase() {} I'm a bit surprised this isn't virtual. Since this is a base class, I think it's a good idea to keep this here but make it virtual, to avoid any issues later where if we add something into it or subclasses, where some destructors don't get run correctly.
meade@chromium.org changed reviewers: + sashab@chromium.org
+sasha for opinions on C++ idioms My opinion is that all base classes should have a virtual destructor to help avoid coding mistakes later (assuming inheritance is used normally - manual dispatch is the obv exception). wdyt?
https://codereview.chromium.org/2739553003/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl (left): https://codereview.chromium.org/2739553003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:30: ~ComputedStyleBase() {} On 2017/03/10 04:04:46, Eddy wrote: > I'm a bit surprised this isn't virtual. Since this is a base class, I think it's > a good idea to keep this here but make it virtual, to avoid any issues later > where if we add something into it or subclasses, where some destructors don't > get run correctly. Making it a virtual subclass will add vtables to ComputedStyle! Which is a very bad idea :) But yes, I would like to keep it as well, because even if it does nothing is documentation that this class is not virtual. So darren, if youd like to add a comment explaining why this isnt virtual, we can leave it here. Also I'm surprised this is a noop since if you delete it it gets moved into the private: section, and surely we have places the destroy a ComputedStyle? So this should compile fail.
On 2017/03/10 at 04:51:02, sashab wrote: > https://codereview.chromium.org/2739553003/diff/1/third_party/WebKit/Source/b... > File third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl (left): > > https://codereview.chromium.org/2739553003/diff/1/third_party/WebKit/Source/b... > third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:30: ~ComputedStyleBase() {} > On 2017/03/10 04:04:46, Eddy wrote: > > I'm a bit surprised this isn't virtual. Since this is a base class, I think it's > > a good idea to keep this here but make it virtual, to avoid any issues later > > where if we add something into it or subclasses, where some destructors don't > > get run correctly. > > Making it a virtual subclass will add vtables to ComputedStyle! Which is a very bad idea :) > > But yes, I would like to keep it as well, because even if it does nothing is documentation that this class is not virtual. So darren, if youd like to add a comment explaining why this isnt virtual, we can leave it here. > > Also I'm surprised this is a noop since if you delete it it gets moved into the private: section, and surely we have places the destroy a ComputedStyle? So this should compile fail. Yeah, my C++ knowledge in this area is a bit shaky, but it looks like CS should only inherit from CSB if CSB has a public virtual dtor or a protected nonvirtual dtor [1, 2]. So yes, this patch is bad. However, we definitely can't afford to make the CSB dtor virtual. To prevent accidental injuries, I'll move the dtor to be protected instead. WDYT? [1] http://stackoverflow.com/questions/2034916/is-it-okay-to-inherit-implementati... [2] http://www.gotw.ca/publications/mill18.htm
On 2017/03/10 05:45:39, shend wrote: > On 2017/03/10 at 04:51:02, sashab wrote: > > > https://codereview.chromium.org/2739553003/diff/1/third_party/WebKit/Source/b... > > File > third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl > (left): > > > > > https://codereview.chromium.org/2739553003/diff/1/third_party/WebKit/Source/b... > > third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:30: > ~ComputedStyleBase() {} > > On 2017/03/10 04:04:46, Eddy wrote: > > > I'm a bit surprised this isn't virtual. Since this is a base class, I think > it's > > > a good idea to keep this here but make it virtual, to avoid any issues later > > > where if we add something into it or subclasses, where some destructors > don't > > > get run correctly. > > > > Making it a virtual subclass will add vtables to ComputedStyle! Which is a > very bad idea :) > > > > But yes, I would like to keep it as well, because even if it does nothing is > documentation that this class is not virtual. So darren, if youd like to add a > comment explaining why this isnt virtual, we can leave it here. > > > > Also I'm surprised this is a noop since if you delete it it gets moved into > the private: section, and surely we have places the destroy a ComputedStyle? So > this should compile fail. > > Yeah, my C++ knowledge in this area is a bit shaky, but it looks like CS should > only inherit from CSB if CSB has a public virtual dtor or a protected nonvirtual > dtor [1, 2]. So yes, this patch is bad. However, we definitely can't afford to > make the CSB dtor virtual. To prevent accidental injuries, I'll move the dtor to > be protected instead. WDYT? > > [1] > http://stackoverflow.com/questions/2034916/is-it-okay-to-inherit-implementati... > [2] http://www.gotw.ca/publications/mill18.htm Is ComputedStyleBase abstract? You if it isn't, you shouldn't be able to make the destructor private or protected - the garbage collector needs to be able to call it.
On 2017/03/10 at 06:40:14, meade wrote: > On 2017/03/10 05:45:39, shend wrote: > > On 2017/03/10 at 04:51:02, sashab wrote: > > > > > https://codereview.chromium.org/2739553003/diff/1/third_party/WebKit/Source/b... > > > File > > third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl > > (left): > > > > > > > > https://codereview.chromium.org/2739553003/diff/1/third_party/WebKit/Source/b... > > > third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:30: > > ~ComputedStyleBase() {} > > > On 2017/03/10 04:04:46, Eddy wrote: > > > > I'm a bit surprised this isn't virtual. Since this is a base class, I think > > it's > > > > a good idea to keep this here but make it virtual, to avoid any issues later > > > > where if we add something into it or subclasses, where some destructors > > don't > > > > get run correctly. > > > > > > Making it a virtual subclass will add vtables to ComputedStyle! Which is a > > very bad idea :) > > > > > > But yes, I would like to keep it as well, because even if it does nothing is > > documentation that this class is not virtual. So darren, if youd like to add a > > comment explaining why this isnt virtual, we can leave it here. > > > > > > Also I'm surprised this is a noop since if you delete it it gets moved into > > the private: section, and surely we have places the destroy a ComputedStyle? So > > this should compile fail. > > > > Yeah, my C++ knowledge in this area is a bit shaky, but it looks like CS should > > only inherit from CSB if CSB has a public virtual dtor or a protected nonvirtual > > dtor [1, 2]. So yes, this patch is bad. However, we definitely can't afford to > > make the CSB dtor virtual. To prevent accidental injuries, I'll move the dtor to > > be protected instead. WDYT? > > > > [1] > > http://stackoverflow.com/questions/2034916/is-it-okay-to-inherit-implementati... > > [2] http://www.gotw.ca/publications/mill18.htm > > Is ComputedStyleBase abstract? You if it isn't, you shouldn't be able to make the destructor private or protected - the garbage collector needs to be able to call it. So CS is RefCounted, but CSB is not if that's what you're asking. Making it protected means that CS can call it, but external code can't.
On 2017/03/10 14:38:26, shend wrote: > On 2017/03/10 at 06:40:14, meade wrote: > > On 2017/03/10 05:45:39, shend wrote: > > > On 2017/03/10 at 04:51:02, sashab wrote: > > > > > > > > https://codereview.chromium.org/2739553003/diff/1/third_party/WebKit/Source/b... > > > > File > > > third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl > > > (left): > > > > > > > > > > > > https://codereview.chromium.org/2739553003/diff/1/third_party/WebKit/Source/b... > > > > > third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:30: > > > ~ComputedStyleBase() {} > > > > On 2017/03/10 04:04:46, Eddy wrote: > > > > > I'm a bit surprised this isn't virtual. Since this is a base class, I > think > > > it's > > > > > a good idea to keep this here but make it virtual, to avoid any issues > later > > > > > where if we add something into it or subclasses, where some destructors > > > don't > > > > > get run correctly. > > > > > > > > Making it a virtual subclass will add vtables to ComputedStyle! Which is a > > > very bad idea :) > > > > > > > > But yes, I would like to keep it as well, because even if it does nothing > is > > > documentation that this class is not virtual. So darren, if youd like to add > a > > > comment explaining why this isnt virtual, we can leave it here. > > > > > > > > Also I'm surprised this is a noop since if you delete it it gets moved > into > > > the private: section, and surely we have places the destroy a ComputedStyle? > So > > > this should compile fail. > > > > > > Yeah, my C++ knowledge in this area is a bit shaky, but it looks like CS > should > > > only inherit from CSB if CSB has a public virtual dtor or a protected > nonvirtual > > > dtor [1, 2]. So yes, this patch is bad. However, we definitely can't afford > to > > > make the CSB dtor virtual. To prevent accidental injuries, I'll move the > dtor to > > > be protected instead. WDYT? > > > > > > [1] > > > > http://stackoverflow.com/questions/2034916/is-it-okay-to-inherit-implementati... > > > [2] http://www.gotw.ca/publications/mill18.htm > > > > Is ComputedStyleBase abstract? You if it isn't, you shouldn't be able to make > the destructor private or protected - the garbage collector needs to be able to > call it. > > So CS is RefCounted, but CSB is not if that's what you're asking. Making it > protected means that CS can call it, but external code can't. Ok, so protected with a comment is probably what we want. I'm pretty sure you shouldn't be able to construct a ComputedStyleBase, so maybe the constructor should be protected too?
On 2017/03/13 at 06:40:42, meade wrote: > On 2017/03/10 14:38:26, shend wrote: > > On 2017/03/10 at 06:40:14, meade wrote: > > > On 2017/03/10 05:45:39, shend wrote: > > > > On 2017/03/10 at 04:51:02, sashab wrote: > > > > > > > > > > > https://codereview.chromium.org/2739553003/diff/1/third_party/WebKit/Source/b... > > > > > File > > > > third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl > > > > (left): > > > > > > > > > > > > > > > > https://codereview.chromium.org/2739553003/diff/1/third_party/WebKit/Source/b... > > > > > > > third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:30: > > > > ~ComputedStyleBase() {} > > > > > On 2017/03/10 04:04:46, Eddy wrote: > > > > > > I'm a bit surprised this isn't virtual. Since this is a base class, I > > think > > > > it's > > > > > > a good idea to keep this here but make it virtual, to avoid any issues > > later > > > > > > where if we add something into it or subclasses, where some destructors > > > > don't > > > > > > get run correctly. > > > > > > > > > > Making it a virtual subclass will add vtables to ComputedStyle! Which is a > > > > very bad idea :) > > > > > > > > > > But yes, I would like to keep it as well, because even if it does nothing > > is > > > > documentation that this class is not virtual. So darren, if youd like to add > > a > > > > comment explaining why this isnt virtual, we can leave it here. > > > > > > > > > > Also I'm surprised this is a noop since if you delete it it gets moved > > into > > > > the private: section, and surely we have places the destroy a ComputedStyle? > > So > > > > this should compile fail. > > > > > > > > Yeah, my C++ knowledge in this area is a bit shaky, but it looks like CS > > should > > > > only inherit from CSB if CSB has a public virtual dtor or a protected > > nonvirtual > > > > dtor [1, 2]. So yes, this patch is bad. However, we definitely can't afford > > to > > > > make the CSB dtor virtual. To prevent accidental injuries, I'll move the > > dtor to > > > > be protected instead. WDYT? > > > > > > > > [1] > > > > > > http://stackoverflow.com/questions/2034916/is-it-okay-to-inherit-implementati... > > > > [2] http://www.gotw.ca/publications/mill18.htm > > > > > > Is ComputedStyleBase abstract? You if it isn't, you shouldn't be able to make > > the destructor private or protected - the garbage collector needs to be able to > > call it. > > > > So CS is RefCounted, but CSB is not if that's what you're asking. Making it > > protected means that CS can call it, but external code can't. > > Ok, so protected with a comment is probably what we want. I'm pretty sure you shouldn't be able to construct a ComputedStyleBase, so maybe the constructor should be protected too? Ah, I might've misunderstood your previous comment. There's two types of protection that we can provide to CSB: 1. protected nonvirtual dtor: Prevents deletion of CS via a pointer to CSB, but you can still instantiate a CSB etc. 2. protected ctor: Can't instantiate a CSB, but can still delete a CS via a pointer to CSB. So if we want CSB to be abstract (which I agree with), then yes we would want to do both #1 and #2.
On 2017/03/13 06:55:02, shend wrote: > On 2017/03/13 at 06:40:42, meade wrote: > > On 2017/03/10 14:38:26, shend wrote: > > > On 2017/03/10 at 06:40:14, meade wrote: > > > > On 2017/03/10 05:45:39, shend wrote: > > > > > On 2017/03/10 at 04:51:02, sashab wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2739553003/diff/1/third_party/WebKit/Source/b... > > > > > > File > > > > > > third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl > > > > > (left): > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2739553003/diff/1/third_party/WebKit/Source/b... > > > > > > > > > > third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:30: > > > > > ~ComputedStyleBase() {} > > > > > > On 2017/03/10 04:04:46, Eddy wrote: > > > > > > > I'm a bit surprised this isn't virtual. Since this is a base class, > I > > > think > > > > > it's > > > > > > > a good idea to keep this here but make it virtual, to avoid any > issues > > > later > > > > > > > where if we add something into it or subclasses, where some > destructors > > > > > don't > > > > > > > get run correctly. > > > > > > > > > > > > Making it a virtual subclass will add vtables to ComputedStyle! Which > is a > > > > > very bad idea :) > > > > > > > > > > > > But yes, I would like to keep it as well, because even if it does > nothing > > > is > > > > > documentation that this class is not virtual. So darren, if youd like to > add > > > a > > > > > comment explaining why this isnt virtual, we can leave it here. > > > > > > > > > > > > Also I'm surprised this is a noop since if you delete it it gets moved > > > into > > > > > the private: section, and surely we have places the destroy a > ComputedStyle? > > > So > > > > > this should compile fail. > > > > > > > > > > Yeah, my C++ knowledge in this area is a bit shaky, but it looks like CS > > > should > > > > > only inherit from CSB if CSB has a public virtual dtor or a protected > > > nonvirtual > > > > > dtor [1, 2]. So yes, this patch is bad. However, we definitely can't > afford > > > to > > > > > make the CSB dtor virtual. To prevent accidental injuries, I'll move the > > > dtor to > > > > > be protected instead. WDYT? > > > > > > > > > > [1] > > > > > > > > > http://stackoverflow.com/questions/2034916/is-it-okay-to-inherit-implementati... > > > > > [2] http://www.gotw.ca/publications/mill18.htm > > > > > > > > Is ComputedStyleBase abstract? You if it isn't, you shouldn't be able to > make > > > the destructor private or protected - the garbage collector needs to be able > to > > > call it. > > > > > > So CS is RefCounted, but CSB is not if that's what you're asking. Making it > > > protected means that CS can call it, but external code can't. > > > > Ok, so protected with a comment is probably what we want. I'm pretty sure you > shouldn't be able to construct a ComputedStyleBase, so maybe the constructor > should be protected too? > > Ah, I might've misunderstood your previous comment. There's two types of > protection that we can provide to CSB: > 1. protected nonvirtual dtor: Prevents deletion of CS via a pointer to CSB, but > you can still instantiate a CSB etc. > 2. protected ctor: Can't instantiate a CSB, but can still delete a CS via a > pointer to CSB. > > So if we want CSB to be abstract (which I agree with), then yes we would want to > do both #1 and #2. Yeah, I think so too, but I'd double check with Sasha...
> There's two types of protection that we can provide to CSB: > 1. protected nonvirtual dtor: Prevents deletion of CS via a pointer to CSB, but > you can still instantiate a CSB etc. Why not make it private? :) Or does this not work because we need the constructor called from CS? > 2. protected ctor: Can't instantiate a CSB, but can still delete a CS via a > pointer to CSB. Yup, let's do it, add a comment explaining why.
On 2017/03/13 at 07:46:59, sashab wrote: > > There's two types of protection that we can provide to CSB: > > 1. protected nonvirtual dtor: Prevents deletion of CS via a pointer to CSB, but > > you can still instantiate a CSB etc. > Why not make it private? :) Or does this not work because we need the constructor called from CS? Yeah you need to be able to call it from CS :)
lgtm
Description was changed from ========== Remove ComputedStyleBase destructor. The ComputedStyleBase destructor is currently a noop. This patch removes it so that the generated code is cleaner. BUG=628043 ========== to ========== Make ComputedStyleBase abstract. Currently ComputedStyleBase can be instantiated, and a ComputedStyle can be deleted via a pointer to ComputedStyleBase. Both of these could cause issues since ComputedStyleBase should be an abstract class. This patch moves the ComputedStyleBase constructor and destructor to be protected so that only ComputedStyle can call them. BUG=628043 ==========
Description was changed from ========== Remove ComputedStyleBase destructor. The ComputedStyleBase destructor is currently a noop. This patch removes it so that the generated code is cleaner. BUG=628043 ========== to ========== Make ComputedStyleBase abstract. Currently ComputedStyleBase can be instantiated, and a ComputedStyle can be deleted via a pointer to ComputedStyleBase. Both of these could cause issues since ComputedStyleBase should be an abstract class. This patch moves the ComputedStyleBase constructor and destructor to be protected so that only ComputedStyle can call them. BUG=628043 ==========
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
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 Link to the patchset: https://codereview.chromium.org/2739553003/#ps40001 (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": 40001, "attempt_start_ts": 1489702004457840,
"parent_rev": "6a5fd5940dedb5f95405828787a5056b3d7c37d6", "commit_rev":
"ffd34d347dc1889ae692610c4d35cc2fc97ca9c0"}
Message was sent while issue was closed.
Description was changed from ========== Make ComputedStyleBase abstract. Currently ComputedStyleBase can be instantiated, and a ComputedStyle can be deleted via a pointer to ComputedStyleBase. Both of these could cause issues since ComputedStyleBase should be an abstract class. This patch moves the ComputedStyleBase constructor and destructor to be protected so that only ComputedStyle can call them. BUG=628043 ========== to ========== Make ComputedStyleBase abstract. Currently ComputedStyleBase can be instantiated, and a ComputedStyle can be deleted via a pointer to ComputedStyleBase. Both of these could cause issues since ComputedStyleBase should be an abstract class. This patch moves the ComputedStyleBase constructor and destructor to be protected so that only ComputedStyle can call them. BUG=628043 Review-Url: https://codereview.chromium.org/2739553003 Cr-Commit-Position: refs/heads/master@{#457579} Committed: https://chromium.googlesource.com/chromium/src/+/ffd34d347dc1889ae692610c4d35... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/ffd34d347dc1889ae692610c4d35...
Message was sent while issue was closed.
Thanks darren, but please add a comment explaining this. Also, in future, can we avoid words like "abstract" unless it has a specific meaning in C++? Eg abstract in C++ actually means at least one function is virtual - https://www.tutorialspoint.com/cplusplus/cpp_interfaces.htm (pretty sure this is just a reinterpretation of java though). Don't want someone looking at this patch and being like "oh, the title says abstract but the class isn't virtual, I guess Darren forgot to add virtual methods, I'll fix that" ;) https://codereview.chromium.org/2739553003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl (right): https://codereview.chromium.org/2739553003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:85: ~ComputedStyleBase() = default; I like this, but can we add a comment here like: // Make the ComputedStyleBase destructor protected so that you can only create one if it is a ComputedStyle.
Message was sent while issue was closed.
On 2017/03/16 at 23:55:31, sashab wrote: > Thanks darren, but please add a comment explaining this. > > Also, in future, can we avoid words like "abstract" unless it has a specific meaning in C++? Eg abstract in C++ actually means at least one function is virtual - https://www.tutorialspoint.com/cplusplus/cpp_interfaces.htm (pretty sure this is just a reinterpretation of java though). Don't want someone looking at this patch and being like "oh, the title says abstract but the class isn't virtual, I guess Darren forgot to add virtual methods, I'll fix that" ;) > > https://codereview.chromium.org/2739553003/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl (right): > > https://codereview.chromium.org/2739553003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:85: ~ComputedStyleBase() = default; > I like this, but can we add a comment here like: > // Make the ComputedStyleBase destructor protected so that you can only create one if it is a ComputedStyle. Oops, will do. |
