[cros] update screen re-factoring pt.1

Disjoints update screen controller part (UpdateScreen) from it's views part (UpdateView) via UpdateScreenActor interface.

BUG=chromium-os:11632
TEST=browser_tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86270

[altimofeev, 2011-05-19 12:53:30]

PTAL

[Paweł Hajdan Jr., 2011-05-19 14:23:42]

Drive-by with a testing comment (chrome/test/OWNERS).

http://codereview.chromium.org/7039042/diff/6004/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/update_screen_browsertest.cc (right):

http://codereview.chromium.org/7039042/diff/6004/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/update_screen_browsertest.cc:89:
static_cast<UpdateLibrary::Observer*>(update_screen_)->
This is worrying - UpdateScreen inherits from UpdateLibrary::Observer. If
casting to the parent class results in a different behavior, it's a maintenance
problem. Otherwise you shouldn't need a cast.

[altimofeev, 2011-05-19 14:58:40]

http://codereview.chromium.org/7039042/diff/6004/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/update_screen_browsertest.cc (right):

http://codereview.chromium.org/7039042/diff/6004/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/update_screen_browsertest.cc:89:
static_cast<UpdateLibrary::Observer*>(update_screen_)->
On 2011/05/19 14:23:42, Paweł Hajdan Jr. wrote:
> This is worrying - UpdateScreen inherits from UpdateLibrary::Observer. If
> casting to the parent class results in a different behavior, it's a
maintenance
> problem. Otherwise you shouldn't need a cast.

I don't want this method to be used by anyone, except Notification service,
which treats the class as an Observer. Is it forbidden in our style guide?

[Paweł Hajdan Jr., 2011-05-19 16:22:07]

http://codereview.chromium.org/7039042/diff/6004/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/update_screen_browsertest.cc (right):

http://codereview.chromium.org/7039042/diff/6004/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/update_screen_browsertest.cc:89:
static_cast<UpdateLibrary::Observer*>(update_screen_)->
On 2011/05/19 14:58:41, altimofeev wrote:
> On 2011/05/19 14:23:42, Paweł Hajdan Jr. wrote:
> > This is worrying - UpdateScreen inherits from UpdateLibrary::Observer. If
> > casting to the parent class results in a different behavior, it's a
> maintenance
> > problem. Otherwise you shouldn't need a cast.
> 
> I don't want this method to be used by anyone, except Notification service,
> which treats the class as an Observer. Is it forbidden in our style guide?

I think we generally don't bother with making interface-inherited methods
private. But that's something for the original reviewer to decide.

For the test, if you still want to keep that method private, please friend the
test class instead of using a cast. "friend UpdateScreenTest" should work fine.

[altimofeev, 2011-05-20 09:22:41]

http://codereview.chromium.org/7039042/diff/6004/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/update_screen_browsertest.cc (right):

http://codereview.chromium.org/7039042/diff/6004/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/update_screen_browsertest.cc:89:
static_cast<UpdateLibrary::Observer*>(update_screen_)->
On 2011/05/19 16:22:07, Paweł Hajdan Jr. wrote:
> On 2011/05/19 14:58:41, altimofeev wrote:
> > On 2011/05/19 14:23:42, Paweł Hajdan Jr. wrote:
> > > This is worrying - UpdateScreen inherits from UpdateLibrary::Observer. If
> > > casting to the parent class results in a different behavior, it's a
> > maintenance
> > > problem. Otherwise you shouldn't need a cast.
> > 
> > I don't want this method to be used by anyone, except Notification service,
> > which treats the class as an Observer. Is it forbidden in our style guide?
> 
> I think we generally don't bother with making interface-inherited methods
> private. But that's something for the original reviewer to decide.
> 
> For the test, if you still want to keep that method private, please friend the
> test class instead of using a cast. "friend UpdateScreenTest" should work
fine.

Returned back to the public variant. Sorry for disturbing.

[Paweł Hajdan Jr., 2011-05-20 09:24:24]

No problem, code I commented in the drive-by LGTM. Thanks!

[Nikita (slow), 2011-05-20 09:28:23]

This CL is a part of 11632, please add BUG field.

[whywhat, 2011-05-20 09:29:50]

LGTM

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/update_screen.cc (right):

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen.cc:13: #include
"chrome/browser/chromeos/login/update_screen_views_actor.h"
Shouldn't be views_update_screen_actor.h to have consistency with existing
classes: we use Views or DOMUI as prefix

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen.cc:72:
is_all_updates_critical_(true),  // See http://crosbug.com/10068
Comments in the header state it's false by default - could you update the
comment too (with bug number too).

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen.cc:249: if (!is_shown_)
I think is_shown_ could be smth like IsVisible method of the actor. Why do we
need MakeSureScreenIsShown anyway? As we have actor now, maybe it should show
itself in SetProgress/SetCurtain if it's not shown. I doesn't look like the
screen should own that.

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/update_screen.h (right):

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen.h:22: // Controller for the update
screen. It does not depends on the specific
nit: depends -> depend

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen.h:23: // realization of the screen
showing (Views of WebUI based), it delegates it to
nit: realization -> implementation, it delegates it -> the dependency is moved

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen.h:36: // Checks for updates and
updates if needed. Made virtual to simplify mocking.
nit: second "updates" -> performs an update

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen.h:39: // Force udpate cancel. Made
virtual to simplify mocking.
nit: Force cancel update (udpate -> update and cancel is a verb)

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen.h:48: void
SetAllUpdatesCritical(bool is_critical);
If it's only for tests, why not make it private and use FRIEND_TEST?

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen.h:91: // Is all updates critical? If
true, update deadlines are ignored.
nit: Is -> Are

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen.h:92: bool is_all_updates_critical_;
optional: there too. or maybe rename to ignore_update_deadlines?

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/update_screen_actor.h (right):

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen_actor.h:15: virtual void Show() = 0;
Comments about methods?

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen_actor.h:22: }  // namespace
UpdateScreen
UpdateScreen -> chromeos

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/update_screen_browsertest.cc (right):

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen_browsertest.cc:89:
static_cast<UpdateLibrary::Observer*>(update_screen_)->
Can't you call the method if you make the class a friend of the test class?

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/update_screen_views_actor.h (right):

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen_views_actor.h:9: #include
"chrome/browser/chromeos/login/wizard_screen.h"
Sort includes

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen_views_actor.h:23: virtual
~UpdateScreenViewsActor() {}
I think you don't need this dtor here.

http://codereview.chromium.org/7039042/diff/11001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/update_screen.cc (right):

http://codereview.chromium.org/7039042/diff/11001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen.cc:76: actor_(new
UpdateScreenViewsActor(delegate,
Discuss: how do we set up a different actor here? Should we have a simple
factory class that returns different actor for each screen depending on
command-line switch?

[altimofeev, 2011-05-20 12:00:32]

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/update_screen.cc (right):

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen.cc:13: #include
"chrome/browser/chromeos/login/update_screen_views_actor.h"
On 2011/05/20 09:29:50, whywhat wrote:
> Shouldn't be views_update_screen_actor.h to have consistency with existing
> classes: we use Views or DOMUI as prefix

Done.

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen.cc:72:
is_all_updates_critical_(true),  // See http://crosbug.com/10068
On 2011/05/20 09:29:50, whywhat wrote:
> Comments in the header state it's false by default - could you update the
> comment too (with bug number too).

Done.

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen.cc:249: if (!is_shown_)
On 2011/05/20 09:29:50, whywhat wrote:
> I think is_shown_ could be smth like IsVisible method of the actor. Why do we
> need MakeSureScreenIsShown anyway? As we have actor now, maybe it should show
> itself in SetProgress/SetCurtain if it's not shown. I doesn't look like the
> screen should own that.

I thought about it.
First of all, currently, WizardController do some stuff before showing the
screen. Probably, after re-factoring it will not, but now it does. Also, my
desire is to make actor as simple as possible. Or using other words, move all
common logic to the controller (screen) part. What do you think?

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/update_screen.h (right):

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen.h:22: // Controller for the update
screen. It does not depends on the specific
On 2011/05/20 09:29:50, whywhat wrote:
> nit: depends -> depend

Done.

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen.h:23: // realization of the screen
showing (Views of WebUI based), it delegates it to
On 2011/05/20 09:29:50, whywhat wrote:
> nit: realization -> implementation, it delegates it -> the dependency is moved

Done.

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen.h:36: // Checks for updates and
updates if needed. Made virtual to simplify mocking.
On 2011/05/20 09:29:50, whywhat wrote:
> nit: second "updates" -> performs an update

Done.

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen.h:39: // Force udpate cancel. Made
virtual to simplify mocking.
On 2011/05/20 09:29:50, whywhat wrote:
> nit: Force cancel update (udpate -> update and cancel is a verb)
Done.
nit: cancel is a noun too.

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen.h:48: void
SetAllUpdatesCritical(bool is_critical);
On 2011/05/20 09:29:50, whywhat wrote:
> If it's only for tests, why not make it private and use FRIEND_TEST?

Then I can completely remove it - done.

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen.h:91: // Is all updates critical? If
true, update deadlines are ignored.
On 2011/05/20 09:29:50, whywhat wrote:
> nit: Is -> Are

Done.

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen.h:92: bool is_all_updates_critical_;
On 2011/05/20 09:29:50, whywhat wrote:
> optional: there too. or maybe rename to ignore_update_deadlines?

Done.

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/update_screen_actor.h (right):

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen_actor.h:15: virtual void Show() = 0;
On 2011/05/20 09:29:50, whywhat wrote:
> Comments about methods?

Done.

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen_actor.h:22: }  // namespace
UpdateScreen
On 2011/05/20 09:29:51, whywhat wrote:
> UpdateScreen -> chromeos

Done.

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/update_screen_browsertest.cc (right):

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen_browsertest.cc:89:
static_cast<UpdateLibrary::Observer*>(update_screen_)->
On 2011/05/20 09:29:51, whywhat wrote:
> Can't you call the method if you make the class a friend of the test class?

Not applicable anymore.

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/update_screen_views_actor.h (right):

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen_views_actor.h:9: #include
"chrome/browser/chromeos/login/wizard_screen.h"
On 2011/05/20 09:29:51, whywhat wrote:
> Sort includes

Done.

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen_views_actor.h:23: virtual
~UpdateScreenViewsActor() {}
On 2011/05/20 09:29:51, whywhat wrote:
> I think you don't need this dtor here.

Why?

[Nikita (slow), 2011-05-20 12:09:46]

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/update_screen.cc (right):

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen.cc:13: #include
"chrome/browser/chromeos/login/update_screen_views_actor.h"
On 2011/05/20 09:29:50, whywhat wrote:
> Shouldn't be views_update_screen_actor.h to have consistency with existing
> classes: we use Views or DOMUI as prefix

DOMUI is now named as WebUI.

[whywhat, 2011-05-21 19:26:58]

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/update_screen.cc (right):

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen.cc:249: if (!is_shown_)
On 2011/05/20 12:00:32, altimofeev wrote:
> On 2011/05/20 09:29:50, whywhat wrote:
> > I think is_shown_ could be smth like IsVisible method of the actor. Why do
we
> > need MakeSureScreenIsShown anyway? As we have actor now, maybe it should
show
> > itself in SetProgress/SetCurtain if it's not shown. I doesn't look like the
> > screen should own that.
> 
> I thought about it.
> First of all, currently, WizardController do some stuff before showing the
> screen. Probably, after re-factoring it will not, but now it does. Also, my
> desire is to make actor as simple as possible. Or using other words, move all
> common logic to the controller (screen) part. What do you think?

From code simplicity point of view it's better to perform this check in one/two
methods of actor than each time before we call one of these methods. If this is
needed for WebUIUpdateScreenActor, we can always make UpdateScreenActor a base
class with this check in it or a common wrapper. Since we'll eventually have
only one implementation, having check in two actors is not a big problem
comparing to calling MakeSureScreenIsShown in each case branch. BTW, can we call
it before the switch once?

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/update_screen_views_actor.h (right):

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen_views_actor.h:23: virtual
~UpdateScreenViewsActor() {}
On 2011/05/20 12:00:32, altimofeev wrote:
> On 2011/05/20 09:29:51, whywhat wrote:
> > I think you don't need this dtor here.
> 
> Why?

Empty dtor is automatically generated and it is virtual since the dtor of the
base class is virtual.

[altimofeev, 2011-05-23 08:13:58]

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/update_screen.cc (right):

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen.cc:249: if (!is_shown_)
On 2011/05/21 19:26:58, whywhat wrote:
> On 2011/05/20 12:00:32, altimofeev wrote:
> > On 2011/05/20 09:29:50, whywhat wrote:
> > > I think is_shown_ could be smth like IsVisible method of the actor. Why do
> we
> > > need MakeSureScreenIsShown anyway? As we have actor now, maybe it should
> show
> > > itself in SetProgress/SetCurtain if it's not shown. I doesn't look like
the
> > > screen should own that.
> > 
> > I thought about it.
> > First of all, currently, WizardController do some stuff before showing the
> > screen. Probably, after re-factoring it will not, but now it does. Also, my
> > desire is to make actor as simple as possible. Or using other words, move
all
> > common logic to the controller (screen) part. What do you think?
> 
> From code simplicity point of view it's better to perform this check in
one/two
> methods of actor than each time before we call one of these methods. If this
is
> needed for WebUIUpdateScreenActor, we can always make UpdateScreenActor a base
> class with this check in it or a common wrapper. Since we'll eventually have
> only one implementation, having check in two actors is not a big problem
> comparing to calling MakeSureScreenIsShown in each case branch. BTW, can we
call
> it before the switch once?

To understand why MakeSureScreenIsShown can't be called before switch I refresh
the current logic: 
- WizardController initiates update checking.
- WizardController schedules delayed screen showing.
- if UpdateCheck returns with nothing to do, screen is not shown at all
(scheduled show is canceled).
- MakeSureScreenIsShown actually checks that screen has already been shown by
WizardController. Note, it speaks to WizardController to show the screen, not
Actor.

About simplicity - I don't get what methods are you talking about. Currently,
there is one method in UpdateScreen (MakeSureScreenIsShown), which interacts
with WizardController.

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/update_screen_views_actor.h (right):

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen_views_actor.h:23: virtual
~UpdateScreenViewsActor() {}
On 2011/05/21 19:26:58, whywhat wrote:
> On 2011/05/20 12:00:32, altimofeev wrote:
> > On 2011/05/20 09:29:51, whywhat wrote:
> > > I think you don't need this dtor here.
> > 
> > Why?
> 
> Empty dtor is automatically generated and it is virtual since the dtor of the
> base class is virtual.

I understand it. But I think that a rule of always having explicit virtual dtor
in classes with virtual methods is a good thing, even supported by our style
guide?

[whywhat, 2011-05-23 08:46:28]

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/update_screen.cc (right):

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen.cc:249: if (!is_shown_)
On 2011/05/23 08:13:58, altimofeev wrote:
> On 2011/05/21 19:26:58, whywhat wrote:
> > On 2011/05/20 12:00:32, altimofeev wrote:
> > > On 2011/05/20 09:29:50, whywhat wrote:
> > > > I think is_shown_ could be smth like IsVisible method of the actor. Why
do
> > we
> > > > need MakeSureScreenIsShown anyway? As we have actor now, maybe it should
> > show
> > > > itself in SetProgress/SetCurtain if it's not shown. I doesn't look like
> the
> > > > screen should own that.
> > > 
> > > I thought about it.
> > > First of all, currently, WizardController do some stuff before showing the
> > > screen. Probably, after re-factoring it will not, but now it does. Also,
my
> > > desire is to make actor as simple as possible. Or using other words, move
> all
> > > common logic to the controller (screen) part. What do you think?
> > 
> > From code simplicity point of view it's better to perform this check in
> one/two
> > methods of actor than each time before we call one of these methods. If this
> is
> > needed for WebUIUpdateScreenActor, we can always make UpdateScreenActor a
base
> > class with this check in it or a common wrapper. Since we'll eventually have
> > only one implementation, having check in two actors is not a big problem
> > comparing to calling MakeSureScreenIsShown in each case branch. BTW, can we
> call
> > it before the switch once?
> 
> To understand why MakeSureScreenIsShown can't be called before switch I
refresh
> the current logic: 
> - WizardController initiates update checking.
> - WizardController schedules delayed screen showing.
> - if UpdateCheck returns with nothing to do, screen is not shown at all
> (scheduled show is canceled).
> - MakeSureScreenIsShown actually checks that screen has already been shown by
> WizardController. Note, it speaks to WizardController to show the screen, not
> Actor.
> 
> About simplicity - I don't get what methods are you talking about. Currently,
> there is one method in UpdateScreen (MakeSureScreenIsShown), which interacts
> with WizardController. 
> 

Ok, thanks for the explanation! Guess this logic distribution worth a separate
look later.

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/update_screen_views_actor.h (right):

http://codereview.chromium.org/7039042/diff/10001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen_views_actor.h:23: virtual
~UpdateScreenViewsActor() {}
On 2011/05/23 08:13:58, altimofeev wrote:
> On 2011/05/21 19:26:58, whywhat wrote:
> > On 2011/05/20 12:00:32, altimofeev wrote:
> > > On 2011/05/20 09:29:51, whywhat wrote:
> > > > I think you don't need this dtor here.
> > > 
> > > Why?
> > 
> > Empty dtor is automatically generated and it is virtual since the dtor of
the
> > base class is virtual.
> 
> I understand it. But I think that a rule of always having explicit virtual
dtor
> in classes with virtual methods is a good thing, even supported by our style
> guide?

OK