Add a few missing overrides found by a new clang warning.

Add a few missing overrides found by a new clang warning.

Namely, -Winconsistent-missing-override. No behavior change.

BUG=428099
Committed: https://crrev.com/fdb276fbe9b542f67c8453587659a3e673f44295
Cr-Commit-Position: refs/heads/master@{#302102}

[Nico, 2014-10-30 00:58:45]

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

[Nico, 2014-10-30 01:27:00]

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

[Nico, 2014-10-30 01:27:01]

https://codereview.chromium.org/682413003/diff/1/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility_win.h (right):

https://codereview.chromium.org/682413003/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_win.h:771:
InternalQueryInterface(void* this_ptr,
So this one is actually a static method and can't be overridden. dmazzoni: Is
that expected? Or should I delete this method?

[dmazzoni, 2014-10-30 15:38:17]

lgtm

Just checking, are all of the formatting changes generated by clang-format?

https://codereview.chromium.org/682413003/diff/1/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility_win.h (right):

https://codereview.chromium.org/682413003/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_win.h:771:
InternalQueryInterface(void* this_ptr,
On 2014/10/30 01:27:01, Nico wrote:
> So this one is actually a static method and can't be overridden. dmazzoni: Is
> that expected? Or should I delete this method?

Fascinating.

This had me confused for a bit because I'm absolutely positive that this method
does something useful, so I didn't understand why it wasn't overriding anything.

It turns out the BEGIN_COM_MAP macro actually expands to a bunch of code that
calls InternalQueryInterface - so it works whether it's static or not. So,
please don't delete it, but remove the "override" and add a comment about
BEGIN_COM_MAP so the next person isn't similarly confused.

[Nico, 2014-10-30 16:33:09]

On Thu, Oct 30, 2014 at 8:38 AM, <dmazzoni@chromium.org> wrote:

> lgtm
>
> Just checking, are all of the formatting changes generated by clang-format?


Yes.


>
> https://codereview.chromium.org/682413003/diff/1/content/
> browser/accessibility/browser_accessibility_win.h
> File content/browser/accessibility/browser_accessibility_win.h (right):
>
> https://codereview.chromium.org/682413003/diff/1/content/
> browser/accessibility/browser_accessibility_win.h#newcode771
> content/browser/accessibility/browser_accessibility_win.h:771:
> InternalQueryInterface(void* this_ptr,
> On 2014/10/30 01:27:01, Nico wrote:
>
>> So this one is actually a static method and can't be overridden.
>>
> dmazzoni: Is
>
>> that expected? Or should I delete this method?
>>
>
> Fascinating.
>
> This had me confused for a bit because I'm absolutely positive that this
> method does something useful, so I didn't understand why it wasn't
> overriding anything.
>
> It turns out the BEGIN_COM_MAP macro actually expands to a bunch of code
> that calls InternalQueryInterface - so it works whether it's static or
> not. So, please don't delete it, but remove the "override" and add a
> comment about BEGIN_COM_MAP so the next person isn't similarly confused.
>

Will do, thanks for explaining :-)

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2014-10-30 16:40:52]

https://codereview.chromium.org/682413003/diff/1/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility_win.h (right):

https://codereview.chromium.org/682413003/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_win.h:771:
InternalQueryInterface(void* this_ptr,
On 2014/10/30 15:38:17, dmazzoni wrote:
> On 2014/10/30 01:27:01, Nico wrote:
> > So this one is actually a static method and can't be overridden. dmazzoni:
Is
> > that expected? Or should I delete this method?
> 
> Fascinating.
> 
> This had me confused for a bit because I'm absolutely positive that this
method
> does something useful, so I didn't understand why it wasn't overriding
anything.
> 
> It turns out the BEGIN_COM_MAP macro actually expands to a bunch of code that
> calls InternalQueryInterface - so it works whether it's static or not. So,
> please don't delete it, but remove the "override" and add a comment about
> BEGIN_COM_MAP so the next person isn't similarly confused.

Done. Also made it static, since it looks like the macro will likely use this as
a static call (the superclass method is static, after all).

[Nico, 2014-10-30 16:40:59]

The CQ bit was checked by thakis@chromium.org

[commit-bot: I haz the power, 2014-10-30 16:41:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/682413003/20001

[dmazzoni, 2014-10-30 16:46:09]

https://codereview.chromium.org/682413003/diff/20001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility_win.cc (right):

https://codereview.chromium.org/682413003/diff/20001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_win.cc:2869: if (ia_role_ !=
ROLE_SYSTEM_GRAPHIC) {
ia_role_ is a member variable, so that won't work if it's static.

I believe everything accessed already has an accessor, so you should be able to
replace ia_role_ with this_ptr->ia_role(), for example.

[Nico, 2014-10-30 16:51:11]

https://codereview.chromium.org/682413003/diff/20001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility_win.cc (right):

https://codereview.chromium.org/682413003/diff/20001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_win.cc:2869: if (ia_role_ !=
ROLE_SYSTEM_GRAPHIC) {
On 2014/10/30 16:46:09, dmazzoni wrote:
> ia_role_ is a member variable, so that won't work if it's static.
> 
> I believe everything accessed already has an accessor, so you should be able
to
> replace ia_role_ with this_ptr->ia_role(), for example.
> 

Done.

[Nico, 2014-10-30 16:51:15]

The CQ bit was checked by thakis@chromium.org

[commit-bot: I haz the power, 2014-10-30 16:51:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/682413003/40001

[commit-bot: I haz the power, 2014-10-30 17:41:19]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-10-30 17:42:46]

Patchset 3 (id:??) landed as
https://crrev.com/fdb276fbe9b542f67c8453587659a3e673f44295
Cr-Commit-Position: refs/heads/master@{#302102}