AcceleratorProvider: Make GetAcceleratorForCommandId const.

AcceleratorProvider: Make GetAcceleratorForCommandId const.

Same for SimpleMenuModel and ButtonMenuItemModel.

There is no reason for it to be non-const. Updated all subclasses.

SimpleMenuModel::GetIndexOfCommandID was also made const.

TBR=derat@chromium.org,avi@chromium.org,hajimehoshi@chromium.org,mkwst@chromium.org
BUG=627344
Committed: https://crrev.com/272470e81fb7c7b47298e32009e2bae2001eb480
Cr-Commit-Position: refs/heads/master@{#405004}

[Matt Giuca, 2016-07-12 04:37:17]

Description was changed from

==========
AcceleratorProvider: Make GetAcceleratorForCommandId const.

Same for SimpleMenuModel and ButtonMenuItemModel.

There is no reason for it to be non-const. Updated all subclasses.

SimpleMenuModel::GetIndexOfCommandID was also made const.

BUG=
==========

to

==========
AcceleratorProvider: Make GetAcceleratorForCommandId const.

Same for SimpleMenuModel and ButtonMenuItemModel.

There is no reason for it to be non-const. Updated all subclasses.

SimpleMenuModel::GetIndexOfCommandID was also made const.

BUG=627344
==========

[Matt Giuca, 2016-07-12 09:12:11]

mgiuca@chromium.org changed reviewers:
+ thakis@chromium.org

[Matt Giuca, 2016-07-12 09:12:13]

Nico: Please review the main change in ui/base/accelerators/accelerator.h,
ui/base/models/simple_menu_model.h and ui/base/models/button_menu_item_model.h.

The rest of the changes are mechanical (adding const to all the overrides) and I
will TBR.

Thanks!

[Nico, 2016-07-12 17:58:38]

lgtm

To thin that not too long ago a change like this could've silently introduced
lots of subtle bugs...progress!

[Nico, 2016-07-12 17:58:46]

To think, too

[Matt Giuca, 2016-07-13 00:59:45]

Description was changed from

==========
AcceleratorProvider: Make GetAcceleratorForCommandId const.

Same for SimpleMenuModel and ButtonMenuItemModel.

There is no reason for it to be non-const. Updated all subclasses.

SimpleMenuModel::GetIndexOfCommandID was also made const.

BUG=627344
==========

to

==========
AcceleratorProvider: Make GetAcceleratorForCommandId const.

Same for SimpleMenuModel and ButtonMenuItemModel.

There is no reason for it to be non-const. Updated all subclasses.

SimpleMenuModel::GetIndexOfCommandID was also made const.

TBR=derat@chromium.org,avi@chromium.org,hajimehoshi@chromium.org,peter@chromi...
BUG=627344
==========

[Matt Giuca, 2016-07-13 01:00:30]

Description was changed from

==========
AcceleratorProvider: Make GetAcceleratorForCommandId const.

Same for SimpleMenuModel and ButtonMenuItemModel.

There is no reason for it to be non-const. Updated all subclasses.

SimpleMenuModel::GetIndexOfCommandID was also made const.

TBR=derat@chromium.org,avi@chromium.org,hajimehoshi@chromium.org,peter@chromi...
BUG=627344
==========

to

==========
AcceleratorProvider: Make GetAcceleratorForCommandId const.

Same for SimpleMenuModel and ButtonMenuItemModel.

There is no reason for it to be non-const. Updated all subclasses.

SimpleMenuModel::GetIndexOfCommandID was also made const.

TBR=derat@chromium.org,avi@chromium.org,hajimehoshi@chromium.org,mkwst@chromi...
BUG=627344
==========

[Matt Giuca, 2016-07-13 01:01:51]

mgiuca@chromium.org changed reviewers:
+ avi@chromium.org, derat@chromium.org, hajimehoshi@chromium.org,
mkwst@chromium.org

[Matt Giuca, 2016-07-13 01:01:52]

Adding TBRs for the rest of the mechanical changes (just adding const):

derat@chromium.org: ash/*
avi@chromium.org: components/renderer_context_menu/*
hajimehoshi@chromium.org: components/translate/*
mkwst@chromium.org: content/shell/*

[Matt Giuca, 2016-07-13 01:07:57]

On 2016/07/12 17:58:38, Nico wrote:
> lgtm
> 
> To thin that not too long ago a change like this could've silently introduced
> lots of subtle bugs...progress!

Really? Do you mean before we mandated "override"? Well, the error message I get
when I've missed an override is:
../../ui/views/controls/textfield/textfield.h:275:65: error: non-virtual member
function marked 'override' hides virtual member function
                                  ui::Accelerator* accelerator) override;
                                                                ^
../../ui/base/models/simple_menu_model.h:40:18: note: hidden overloaded virtual
function 'ui::SimpleMenuModel::Delegate::GetAcceleratorForCommandId' declared
here: different qualifiers (const vs none)
    virtual bool GetAcceleratorForCommandId(
                 ^
../../ui/views/controls/message_box_view.cc:194:25: error: allocating an object
of abstract class type 'views::Textfield'
    prompt_field_ = new Textfield;
                        ^
../../ui/base/models/simple_menu_model.h:40:18: note: unimplemented pure virtual
method 'GetAcceleratorForCommandId' in 'Textfield'
    virtual bool GetAcceleratorForCommandId(
                 ^
2 errors generated.

So the first error wouldn't have been seen a couple of years ago, but the second
would always have been an error (so as long as the subclasses are actually being
instantiated somewhere, we would still have seen it).

[Matt Giuca, 2016-07-13 01:08:37]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2016-07-13 01:11:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2016-07-13 01:26:19]

On 2016/07/13 01:07:57, Matt Giuca wrote:
> On 2016/07/12 17:58:38, Nico wrote:
> > lgtm
> > 
> > To thin that not too long ago a change like this could've silently
introduced
> > lots of subtle bugs...progress!
> 
> Really? Do you mean before we mandated "override"? Well, the error message I
get
> when I've missed an override is:
> ../../ui/views/controls/textfield/textfield.h:275:65: error: non-virtual
member
> function marked 'override' hides virtual member function
>                                   ui::Accelerator* accelerator) override;
>                                                                 ^
> ../../ui/base/models/simple_menu_model.h:40:18: note: hidden overloaded
virtual
> function 'ui::SimpleMenuModel::Delegate::GetAcceleratorForCommandId' declared
> here: different qualifiers (const vs none)
>     virtual bool GetAcceleratorForCommandId(
>                  ^
> ../../ui/views/controls/message_box_view.cc:194:25: error: allocating an
object
> of abstract class type 'views::Textfield'
>     prompt_field_ = new Textfield;
>                         ^
> ../../ui/base/models/simple_menu_model.h:40:18: note: unimplemented pure
virtual
> method 'GetAcceleratorForCommandId' in 'Textfield'
>     virtual bool GetAcceleratorForCommandId(
>                  ^
> 2 errors generated.
> 
> So the first error wouldn't have been seen a couple of years ago, but the
second
> would always have been an error (so as long as the subclasses are actually
being
> instantiated somewhere, we would still have seen it).

from the peanut gallery: it probably "depends" - a class could inherit (i.e. a
second time) from a non-abstract base and try to re-override the implemented
const method with a non-const method. Unless compiling with -Woverloaded-virtual
(pretty sure we don't), that would have once been valid, but could lead to
different behaviour depending on the static type at the call site.

[Daniel Erat, 2016-07-13 01:45:06]

lgtm for ash

[Avi (use Gerrit), 2016-07-13 02:15:25]

lgtm

[commit-bot: I haz the power, 2016-07-13 04:13:48]

Description was changed from

==========
AcceleratorProvider: Make GetAcceleratorForCommandId const.

Same for SimpleMenuModel and ButtonMenuItemModel.

There is no reason for it to be non-const. Updated all subclasses.

SimpleMenuModel::GetIndexOfCommandID was also made const.

TBR=derat@chromium.org,avi@chromium.org,hajimehoshi@chromium.org,mkwst@chromi...
BUG=627344
==========

to

==========
AcceleratorProvider: Make GetAcceleratorForCommandId const.

Same for SimpleMenuModel and ButtonMenuItemModel.

There is no reason for it to be non-const. Updated all subclasses.

SimpleMenuModel::GetIndexOfCommandID was also made const.

TBR=derat@chromium.org,avi@chromium.org,hajimehoshi@chromium.org,mkwst@chromi...
BUG=627344
==========

[commit-bot: I haz the power, 2016-07-13 04:13:50]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-07-13 04:14:23]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 04:16:57]

Description was changed from

==========
AcceleratorProvider: Make GetAcceleratorForCommandId const.

Same for SimpleMenuModel and ButtonMenuItemModel.

There is no reason for it to be non-const. Updated all subclasses.

SimpleMenuModel::GetIndexOfCommandID was also made const.

TBR=derat@chromium.org,avi@chromium.org,hajimehoshi@chromium.org,mkwst@chromi...
BUG=627344
==========

to

==========
AcceleratorProvider: Make GetAcceleratorForCommandId const.

Same for SimpleMenuModel and ButtonMenuItemModel.

There is no reason for it to be non-const. Updated all subclasses.

SimpleMenuModel::GetIndexOfCommandID was also made const.

TBR=derat@chromium.org,avi@chromium.org,hajimehoshi@chromium.org,mkwst@chromi...
BUG=627344

Committed: https://crrev.com/272470e81fb7c7b47298e32009e2bae2001eb480
Cr-Commit-Position: refs/heads/master@{#405004}
==========

[commit-bot: I haz the power, 2016-07-13 04:16:59]

Patchset 6 (id:??) landed as
https://crrev.com/272470e81fb7c7b47298e32009e2bae2001eb480
Cr-Commit-Position: refs/heads/master@{#405004}