chrome: Extract CommandUpdaterDelegate into its own header file.

chrome: Extract CommandUpdaterDelegate into its own header file.

R=sky@chromium.org


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

[tfarina, 2012-11-28 23:23:32]

Please, take a look.

[tfarina, 2012-11-30 00:11:25]

Hi Scott, could you review this to me?

Thanks,

[tfarina, 2012-12-05 23:56:11]

Hi Scott, could you review this to me when you get some spare time?

Thanks!

[sky, 2012-12-06 01:38:08]

https://codereview.chromium.org/11308259/diff/11001/chrome/browser/command_up...
File chrome/browser/command_updater.h (right):

https://codereview.chromium.org/11308259/diff/11001/chrome/browser/command_up...
chrome/browser/command_updater.h:28: ~CommandUpdater();
In general if there are subclasses we make the destructor virtual. While we some
times deviate from this, in general we don't.

[tfarina, 2012-12-06 01:41:40]

https://codereview.chromium.org/11308259/diff/11001/chrome/browser/command_up...
File chrome/browser/command_updater.h (right):

https://codereview.chromium.org/11308259/diff/11001/chrome/browser/command_up...
chrome/browser/command_updater.h:28: ~CommandUpdater();
On 2012/12/06 01:38:08, sky wrote:
> In general if there are subclasses we make the destructor virtual. While we
some
> times deviate from this, in general we don't.
If I understand you, and you are concerned about that I made the destructor of
CommandUpdater non-virtual was because it doesn't have virtual methods and
NOBODY inherits from CommandUpdater.

The one inherited is CommandUpdaterDelegate and that interface has a virtual
destructor :)

Was that your concern?

[sky, 2012-12-06 01:53:43]

The test has a class the subclasses CommandObserver, right?

  -Scott

On Wed, Dec 5, 2012 at 5:41 PM,  <tfarina@chromium.org> wrote:
>
>
https://codereview.chromium.org/11308259/diff/11001/chrome/browser/command_up...
> File chrome/browser/command_updater.h (right):
>
>
https://codereview.chromium.org/11308259/diff/11001/chrome/browser/command_up...
> chrome/browser/command_updater.h:28: ~CommandUpdater();
> On 2012/12/06 01:38:08, sky wrote:
>>
>> In general if there are subclasses we make the destructor virtual.
>
> While we some
>>
>> times deviate from this, in general we don't.
>
> If I understand you, and you are concerned about that I made the
> destructor of CommandUpdater non-virtual was because it doesn't have
> virtual methods and NOBODY inherits from CommandUpdater.
>
> The one inherited is CommandUpdaterDelegate and that interface has a
> virtual destructor :)
>
> Was that your concern?
>
> https://codereview.chromium.org/11308259/

[tfarina, 2012-12-06 01:56:28]

On Wed, Dec 5, 2012 at 11:53 PM, Scott Violet <sky@chromium.org> wrote:
> The test has a class the subclasses CommandObserver, right?
>
Yup.

We have FakeCommandUpdaterDelegate which IS-A CommandUpdaterDelegate and
              FakeCommandObserver            which IS-A CommandObserver.

CommandUpdater does not have nested classes anymore nor is inherited anywhere.

-- 
Thiago

[sky, 2012-12-06 16:32:59]

LGTM

[commit-bot: I haz the power, 2012-12-06 16:51:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tfarina@chromium.org/11308259/11001

[commit-bot: I haz the power, 2012-12-06 21:27:14]

Change committed as 171592