GTTF: Add missing virtual destructors.

GTTF: Add missing virtual destructors.

R=jar
TBR=darin,satorux,fischman,jamesr,sky

BUG=45135
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180669

[jar (doing other things), 2013-01-28 17:02:57]

When adding a destructor (when there was none), why did you make it protected? 
Isn't the existing (default/non-virtual) destructor public?

https://codereview.chromium.org/12086018/diff/3002/cc/render_pass_sink.h
File cc/render_pass_sink.h (right):

https://codereview.chromium.org/12086018/diff/3002/cc/render_pass_sink.h#newc...
cc/render_pass_sink.h:18: protected:
nit: Add white space line before line 18.  .... if we're following Chrome style

[jamesr, 2013-01-28 19:20:43]

https://codereview.chromium.org/12086018/diff/3002/cc/layer_tree_host_impl.h
File cc/layer_tree_host_impl.h (right):

https://codereview.chromium.org/12086018/diff/3002/cc/layer_tree_host_impl.h#...
cc/layer_tree_host_impl.h:65: virtual ~LayerTreeHostImplClient() { };
no ;

https://codereview.chromium.org/12086018/diff/3002/cc/rate_limiter.h
File cc/rate_limiter.h (right):

https://codereview.chromium.org/12086018/diff/3002/cc/rate_limiter.h#newcode21
cc/rate_limiter.h:21: protected:
newline before 'protected'

https://codereview.chromium.org/12086018/diff/3002/cc/render_pass_sink.h
File cc/render_pass_sink.h (right):

https://codereview.chromium.org/12086018/diff/3002/cc/render_pass_sink.h#newc...
cc/render_pass_sink.h:19: virtual ~RenderPassSink() { }
not sure this one is really necessary. nobody takes ownership of a
RenderPassSink via the base class and the derived class doesn't need a virtual
destructor

[Paweł Hajdan Jr., 2013-01-28 19:55:48]

If you're fine with the "protected" usage, can I get a conditional stamp about
the style issues, which are straightforward to fix?

I'm just trying to cut review latency from GMT+1 time zone here - doing it just
for style nits doesn't seem worth it to me.

Please let me know if you have more questions about "protected" though. Let me
know if you'd want me to just make them public. It seems to me though that it's
better to use "protected" in case they are not intended to be deleted that way.

https://codereview.chromium.org/12086018/diff/3002/cc/render_pass_sink.h
File cc/render_pass_sink.h (right):

https://codereview.chromium.org/12086018/diff/3002/cc/render_pass_sink.h#newc...
cc/render_pass_sink.h:19: virtual ~RenderPassSink() { }
On 2013/01/28 19:20:43, jamesr wrote:
> not sure this one is really necessary. nobody takes ownership of a
> RenderPassSink via the base class and the derived class doesn't need a virtual
> destructor

That's why it's protected. This seems to be a convention used when no one
deletes the object via pointer to the base type. This will allow enabling a
warning in gcc about missing virtual dtors (see bug), and that warning triggers
here (same thing about ownership also applies to most cases in this CL, since
most of them are protected and I think just one is public).

[jamesr, 2013-01-28 20:13:45]

https://codereview.chromium.org/12086018/diff/3002/cc/render_pass_sink.h
File cc/render_pass_sink.h (right):

https://codereview.chromium.org/12086018/diff/3002/cc/render_pass_sink.h#newc...
cc/render_pass_sink.h:19: virtual ~RenderPassSink() { }
On 2013/01/28 19:55:49, Paweł Hajdan Jr. wrote:
> On 2013/01/28 19:20:43, jamesr wrote:
> > not sure this one is really necessary. nobody takes ownership of a
> > RenderPassSink via the base class and the derived class doesn't need a
virtual
> > destructor
> 
> That's why it's protected. This seems to be a convention used when no one
> deletes the object via pointer to the base type. This will allow enabling a
> warning in gcc about missing virtual dtors (see bug), and that warning
triggers
> here (same thing about ownership also applies to most cases in this CL, since
> most of them are protected and I think just one is public).

Protected and non-virtual seems like the right choice here

[Paweł Hajdan Jr., 2013-01-29 09:40:22]

https://codereview.chromium.org/12086018/diff/3002/cc/render_pass_sink.h
File cc/render_pass_sink.h (right):

https://codereview.chromium.org/12086018/diff/3002/cc/render_pass_sink.h#newc...
cc/render_pass_sink.h:19: virtual ~RenderPassSink() { }
On 2013/01/28 20:13:46, jamesr wrote:
> Protected and non-virtual seems like the right choice here

Alright, so we have agreed on protected. Good. :)

Now why non-virtual? The deriving class should have a virtual dtor anyway,
otherwise it's asking for trouble. You may still get "correct" code by making
sure you always invoke the right dtor by using the same pointer type as the
actual class allocated, but this seems pretty fragile. Refactoring, new derived
class, many things looking innocent can invalidate such assumptions.

A virtual dtor here forces any derived class to also have a virtual dtor. This
makes it much less error-prone. The class has a vtable already anyway.

Do you still think it's important to make this dtor non-virtual? If so, why? For
me the ease of misuse is a much bigger issue here, but I'm open for arguments
why a non-virtual dtor would be better. I can definitely learn more about C++.

[jar (doing other things), 2013-01-29 17:58:22]

Although you may be able to argue in some special cases "protected" makes sense,
when I do cleanups, I try not to make semantic changes without much closer
scrutiny.

Given that most of the code currently has public (default) destructors, I wasn't
clear why you wanted to change that status (generally).  You might have a
special reason here and there... but I didn't see a general reason.

https://codereview.chromium.org/12086018/diff/3002/cc/render_pass_sink.h
File cc/render_pass_sink.h (right):

https://codereview.chromium.org/12086018/diff/3002/cc/render_pass_sink.h#newc...
cc/render_pass_sink.h:19: virtual ~RenderPassSink() { }
Simple rule of thumb: If you have any virtual functions, you should have a
virtual destructor.  No one should argue (and I don't think anyone is).

On 2013/01/29 09:40:22, Paweł Hajdan Jr. wrote:
> On 2013/01/28 20:13:46, jamesr wrote:
> > Protected and non-virtual seems like the right choice here
> 
> Alright, so we have agreed on protected. Good. :)
> 
> Now why non-virtual? The deriving class should have a virtual dtor anyway,
> otherwise it's asking for trouble. You may still get "correct" code by making
> sure you always invoke the right dtor by using the same pointer type as the
> actual class allocated, but this seems pretty fragile. Refactoring, new
derived
> class, many things looking innocent can invalidate such assumptions.
> 
> A virtual dtor here forces any derived class to also have a virtual dtor. This
> makes it much less error-prone. The class has a vtable already anyway.
> 
> Do you still think it's important to make this dtor non-virtual? If so, why?
For
> me the ease of misuse is a much bigger issue here, but I'm open for arguments
> why a non-virtual dtor would be better. I can definitely learn more about C++.

[jamesr, 2013-01-29 18:52:20]

https://codereview.chromium.org/12086018/diff/3002/cc/render_pass_sink.h
File cc/render_pass_sink.h (right):

https://codereview.chromium.org/12086018/diff/3002/cc/render_pass_sink.h#newc...
cc/render_pass_sink.h:19: virtual ~RenderPassSink() { }
On 2013/01/29 09:40:22, Paweł Hajdan Jr. wrote:
> On 2013/01/28 20:13:46, jamesr wrote:
> > Protected and non-virtual seems like the right choice here
> 
> Alright, so we have agreed on protected. Good. :)

Actually, I think this file is perfectly fine as-is.  What's the motivation for
changing it at all?

[Paweł Hajdan Jr., 2013-01-30 10:00:27]

PTAL

https://codereview.chromium.org/12086018/diff/3002/cc/layer_tree_host_impl.h
File cc/layer_tree_host_impl.h (right):

https://codereview.chromium.org/12086018/diff/3002/cc/layer_tree_host_impl.h#...
cc/layer_tree_host_impl.h:65: virtual ~LayerTreeHostImplClient() { };
On 2013/01/28 19:20:43, jamesr wrote:
> no ;

Done.

https://codereview.chromium.org/12086018/diff/3002/cc/render_pass_sink.h
File cc/render_pass_sink.h (right):

https://codereview.chromium.org/12086018/diff/3002/cc/render_pass_sink.h#newc...
cc/render_pass_sink.h:19: virtual ~RenderPassSink() { }
On 2013/01/29 18:52:20, jamesr wrote:
> On 2013/01/29 09:40:22, Paweł Hajdan Jr. wrote:
> > On 2013/01/28 20:13:46, jamesr wrote:
> > > Protected and non-virtual seems like the right choice here
> > 
> > Alright, so we have agreed on protected. Good. :)
> 
> Actually, I think this file is perfectly fine as-is.  What's the motivation
for
> changing it at all?

Please see the bug and earlier comments. I removed it from this CL for now.

[jar (doing other things), 2013-02-01 03:41:07]

LGTM with nits below for patch 2 in base and net

https://codereview.chromium.org/12086018/diff/102/cc/picture_layer_tiling.h
File cc/picture_layer_tiling.h (right):

https://codereview.chromium.org/12086018/diff/102/cc/picture_layer_tiling.h#n...
cc/picture_layer_tiling.h:30: PictureLayerTiling* tiling,
You probably want to leave funky format alone (and do as the Romans do in this
file), since this is seemingly third party, and you'll want merges to be as
smoother as possible.  I ignored other formatting issues in sibling files.

https://codereview.chromium.org/12086018/diff/102/chrome/browser/extensions/a...
File chrome/browser/extensions/app_notify_channel_setup.h (right):

https://codereview.chromium.org/12086018/diff/102/chrome/browser/extensions/a...
chrome/browser/extensions/app_notify_channel_setup.h:75: virtual
~InterceptorForTests() { }
nit: IMO, better is public, unless you have a STRONG reason, so as to not change
existing semantics.

[Paweł Hajdan Jr., 2013-02-01 10:12:42]

https://codereview.chromium.org/12086018/diff/102/cc/picture_layer_tiling.h
File cc/picture_layer_tiling.h (right):

https://codereview.chromium.org/12086018/diff/102/cc/picture_layer_tiling.h#n...
cc/picture_layer_tiling.h:30: PictureLayerTiling* tiling,
On 2013/02/01 03:41:07, jar wrote:
> You probably want to leave funky format alone (and do as the Romans do in this
> file), since this is seemingly third party, and you'll want merges to be as
> smoother as possible.  I ignored other formatting issues in sibling files.

Done. The formatting of this class is not even consistent with the rest of the
file though.

Now if this is third party, it should live in third_party and not here.

https://codereview.chromium.org/12086018/diff/102/chrome/browser/extensions/a...
File chrome/browser/extensions/app_notify_channel_setup.h (right):

https://codereview.chromium.org/12086018/diff/102/chrome/browser/extensions/a...
chrome/browser/extensions/app_notify_channel_setup.h:75: virtual
~InterceptorForTests() { }
On 2013/02/01 03:41:07, jar wrote:
> nit: IMO, better is public, unless you have a STRONG reason, so as to not
change
> existing semantics.

Done, sorry I just forgot to change it to public.

[jamesr, 2013-02-01 19:30:04]

The interfaces I've looked at should all be protected.  I would not change
interfaces that are not well understood by you or the reviewer.

https://codereview.chromium.org/12086018/diff/9052/cc/layer_animation_event_o...
File cc/layer_animation_event_observer.h (right):

https://codereview.chromium.org/12086018/diff/9052/cc/layer_animation_event_o...
cc/layer_animation_event_observer.h:12: virtual ~LayerAnimationEventObserver() {
}
make this protected. client/observer type interfaces typically don't include
ownership, and that's definitely the case here

https://codereview.chromium.org/12086018/diff/9052/cc/layer_tree_host_impl.h
File cc/layer_tree_host_impl.h (right):

https://codereview.chromium.org/12086018/diff/9052/cc/layer_tree_host_impl.h#...
cc/layer_tree_host_impl.h:46: virtual ~LayerTreeHostImplClient() { }
protected, please

https://codereview.chromium.org/12086018/diff/9052/cc/picture_layer_tiling.h
File cc/picture_layer_tiling.h (right):

https://codereview.chromium.org/12086018/diff/9052/cc/picture_layer_tiling.h#...
cc/picture_layer_tiling.h:25: virtual ~PictureLayerTilingClient() { }
protected

https://codereview.chromium.org/12086018/diff/9052/cc/rate_limiter.h
File cc/rate_limiter.h (right):

https://codereview.chromium.org/12086018/diff/9052/cc/rate_limiter.h#newcode20
cc/rate_limiter.h:20: virtual ~RateLimiterClient() { }
protected

https://codereview.chromium.org/12086018/diff/9052/cc/video_frame_provider.h
File cc/video_frame_provider.h (right):

https://codereview.chromium.org/12086018/diff/9052/cc/video_frame_provider.h#...
cc/video_frame_provider.h:27: virtual ~Client() { }
protected

[jar (doing other things), 2013-02-01 20:05:55]

I'm fine with changes to protected destructors where JamesR has spent the time
looking closely, and *wants* the change (in addition to just the visible virtual
destructor).  His concern was that the explicit virtual destructor would be read
as a statement-of-intent by the class, rather than (my reading) of a
style-guide-compliance to preclude really painful bugs.

JamesR also suggested that it would be cool if a compiler would automatically
*add* virtual public destructors, when it needed to add a destructor, and could
see virtual appearing in the class or its base.  That would leave everyone happy
(no explicit/confusing interface, an no need for this robotic code insertion).

On 2013/02/01 19:30:04, jamesr wrote:
> The interfaces I've looked at should all be protected.  I would not change
> interfaces that are not well understood by you or the reviewer.
> 
>
https://codereview.chromium.org/12086018/diff/9052/cc/layer_animation_event_o...
> File cc/layer_animation_event_observer.h (right):
> 
>
https://codereview.chromium.org/12086018/diff/9052/cc/layer_animation_event_o...
> cc/layer_animation_event_observer.h:12: virtual ~LayerAnimationEventObserver()
{
> }
> make this protected. client/observer type interfaces typically don't include
> ownership, and that's definitely the case here
> 
> https://codereview.chromium.org/12086018/diff/9052/cc/layer_tree_host_impl.h
> File cc/layer_tree_host_impl.h (right):
> 
>
https://codereview.chromium.org/12086018/diff/9052/cc/layer_tree_host_impl.h#...
> cc/layer_tree_host_impl.h:46: virtual ~LayerTreeHostImplClient() { }
> protected, please
> 
> https://codereview.chromium.org/12086018/diff/9052/cc/picture_layer_tiling.h
> File cc/picture_layer_tiling.h (right):
> 
>
https://codereview.chromium.org/12086018/diff/9052/cc/picture_layer_tiling.h#...
> cc/picture_layer_tiling.h:25: virtual ~PictureLayerTilingClient() { }
> protected
> 
> https://codereview.chromium.org/12086018/diff/9052/cc/rate_limiter.h
> File cc/rate_limiter.h (right):
> 
> https://codereview.chromium.org/12086018/diff/9052/cc/rate_limiter.h#newcode20
> cc/rate_limiter.h:20: virtual ~RateLimiterClient() { }
> protected
> 
> https://codereview.chromium.org/12086018/diff/9052/cc/video_frame_provider.h
> File cc/video_frame_provider.h (right):
> 
>
https://codereview.chromium.org/12086018/diff/9052/cc/video_frame_provider.h#...
> cc/video_frame_provider.h:27: virtual ~Client() { }
> protected

[Paweł Hajdan Jr., 2013-02-05 10:30:35]

TBR

Trivial changes, feel free to take a look at your areas. I'm happy to flip the
dtors from public to protected if you want, or do any other change as needed.

[jamesr, 2013-02-05 21:33:55]

On 2013/02/05 10:30:35, Paweł Hajdan Jr. wrote:
> TBR
> 
> Trivial changes, feel free to take a look at your areas. I'm happy to flip the
> dtors from public to protected if you want, or do any other change as needed.

I don't think it's a good idea to change the interfaces for other people's
classes without letting them look at the change.  The cc/ ones look good, but
they didn't in the first patch.  TBR should be used for build fixes and things
of that nature, not to skip reviews.

[Paweł Hajdan Jr., 2013-02-06 18:16:49]

On 2013/02/05 21:33:55, jamesr wrote:
> I don't think it's a good idea to change the interfaces for other people's
> classes without letting them look at the change.  The cc/ ones look good, but
> they didn't in the first patch.  TBR should be used for build fixes and things
> of that nature, not to skip reviews.

1. This is now reverted anyway - https://codereview.chromium.org/12224027/

2. My understanding of http://www.chromium.org/developers/owners-files, and
especially "For widespread or refactoring changes that are mechanical or global
in nature, it is acceptable to TBR the OWNERS required for submission, as long
as your changes have had the appropriate level of review or general approval."
is that it's fine here - I got review from Jim, and the change is pretty much
global and mechanical.

[satorux1, 2013-02-06 22:33:23]

On 2013/02/06 18:16:49, Paweł Hajdan Jr. wrote:
> On 2013/02/05 21:33:55, jamesr wrote:
> > I don't think it's a good idea to change the interfaces for other people's
> > classes without letting them look at the change.  The cc/ ones look good,
but
> > they didn't in the first patch.  TBR should be used for build fixes and
things
> > of that nature, not to skip reviews.
> 
> 1. This is now reverted anyway - https://codereview.chromium.org/12224027/
> 
> 2. My understanding of http://www.chromium.org/developers/owners-files, and
> especially "For widespread or refactoring changes that are mechanical or
global
> in nature, it is acceptable to TBR the OWNERS required for submission, as long
> as your changes have had the appropriate level of review or general approval."
> is that it's fine here - I got review from Jim, and the change is pretty much
> global and mechanical.

I don't feel strongly about this patch, but I'm rather concerned about TBR for
global cleanup patches in general (on the contrary I love these patches, but
just TBR is a concern) because these patches could make it hard to merge
important fixes to release branches.

FWIW, I expressed the same concern in the past in a similar occasion:

> https://codereview.chromium.org/12018035/
> 
> It's not always safe to submit clean up patches, especially after a release
> branch is cut,  as these patches could make it difficult to merge important
> fixes to the branch. It's safer to get code review from owners in the standard
> way than doing TBR.