Standardize usage of virtual/override/final in chrome/browser/ui/

Standardize usage of virtual/override/final in chrome/browser/ui/

This patch was automatically generated by applying clang fixit hints
generated by the plugin to the source tree.

BUG=417463
TBR=msw@chromium.org

Committed: https://crrev.com/5dd5ff67b66b15edbb08201cd4c4590861567e4e
Cr-Commit-Position: refs/heads/master@{#300476}

[dcheng, 2014-10-21 10:07:22]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-10-21 10:09:51]

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

[commit-bot: I haz the power, 2014-10-21 11:54:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-21 11:54:33]

Try jobs failed on following builders:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dcheng, 2014-10-21 11:58:10]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-10-21 12:01:31]

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

[commit-bot: I haz the power, 2014-10-21 12:42:58]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2014-10-21 13:10:13]

Patchset 1 (id:??) landed as
https://crrev.com/5dd5ff67b66b15edbb08201cd4c4590861567e4e
Cr-Commit-Position: refs/heads/master@{#300476}

[msw, 2014-10-21 19:10:03]

This CL implements multiple new C++ policies that I'm not entirely familiar
with, and aren't called out in the CL description or the bug. Each category of
keyword changes probably should have been landed separately. Did you run a
script to make these changes? Do I actually need to review over 4,000 changed
lines of code?

[dcheng, 2014-10-21 19:17:55]

On 2014/10/21 at 19:10:03, msw wrote:
> This CL implements multiple new C++ policies that I'm not entirely familiar
with, and aren't called out in the CL description or the bug. Each category of
keyword changes probably should have been landed separately.

Can you suggest a way I could have separated the keyword changes? Overall, there
are three kind of changes in this patch:
- Convert destructors to use override where appropriate.
- Remove redundant virtuals in the presence of override.
- Remove redundant virtuals and overrides in the presence of final.

However, I don't see a lot of value in separating these, but maybe I'm missing
something.

> Did you run a script to make these changes?

Sorry, I should have made the description clearer for people who aren't familiar
with clang. The process was largely automatic, but driven by a human. Clang
diagnostics have the ability to emit fixit hints, which are suggested edits when
clang (or the clang plugin in this case) is 100% sure it knows how to fix the
problem. These fixits can be automatically applied. Essentially, it boiled down
to repeating the following two steps until compilation succeeded:
ninja -C out/Debug > fixits  # Dumps a bunch of fix it hints into fixits
python tools/clang/scripts/apply-fixits.py < fixits  # Apply the fixits. In
theory, clang can do this itself, but in practice, it's difficult to use.

> Do I actually need to review over 4,000 changed lines of code?

I spot checked files here and there. The original diff was 40k lines changed.
It's hard to review something that big, and splitting it up past a certain point
gets pretty painful =/
The tooling for this should be quite precise and accurate (the functionality in
the plugin is explicitly tested), so I don't think you need to manually review
all changed lines.

[msw, 2014-10-21 19:36:11]

Thanks for the explanation, this seems a bit more reasonable now. I did a quick
spot check, but didn't find any changes involving the "final" keyword (and
apparently the patch set is too large to view/download a complete diff that I
could search over), shrug. Overall, I just have two questions about the pattern
of changes I saw, it seems plausibly correct, but this is the first I've
actually seen of these new policies.

https://codereview.chromium.org/671653002/diff/1/chrome/browser/ui/android/ta...
File chrome/browser/ui/android/tab_model/tab_model.h (right):

https://codereview.chromium.org/671653002/diff/1/chrome/browser/ui/android/ta...
chrome/browser/ui/android/tab_model/tab_model.h:59: ~TabModel() override;
Why does this seem to operate contrary to the Google C++ style guide?
"Make your destructor virtual if necessary. If your class has virtual methods,
its destructor should be virtual."
Is the actual "virtual" keyword no longer necessary to mark the destructor
virtual if the "override" keyword is used?

https://codereview.chromium.org/671653002/diff/1/chrome/browser/ui/android/ta...
chrome/browser/ui/android/tab_model/tab_model.h:69: void Observe(int type,
I guess this is implementing the following policy? "For clarity, use exactly one
of override, final, or virtual when declaring an override."

[dcheng, 2014-10-21 20:14:53]

Thanks for your feedback. I could have done a better job explaining these
changes in the description, so this will make it easier for future reviewers...
though it's too late for this one =(

Apologies for the large patch. It gets pretty unwieldy to split up patches past
a certain directory depth, but c/b/ui is huge

https://codereview.chromium.org/671653002/diff/1/chrome/browser/ui/android/ta...
File chrome/browser/ui/android/tab_model/tab_model.h (right):

https://codereview.chromium.org/671653002/diff/1/chrome/browser/ui/android/ta...
chrome/browser/ui/android/tab_model/tab_model.h:59: ~TabModel() override;
On 2014/10/21 19:36:11, msw wrote:
> Why does this seem to operate contrary to the Google C++ style guide?
> "Make your destructor virtual if necessary. If your class has virtual methods,
> its destructor should be virtual."
> Is the actual "virtual" keyword no longer necessary to mark the destructor
> virtual if the "override" keyword is used?

From http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Inheritance
Explicitly annotate overrides of virtual functions or virtual destructors with
an override or (less frequently) final specifier.

You're not the only one that's expressed surprise at this, and I raised this
issue on chromium-dev. However, the wording seems unambiguous and the general
consensus seems to be that we should follow the Google style.

https://codereview.chromium.org/671653002/diff/1/chrome/browser/ui/android/ta...
chrome/browser/ui/android/tab_model/tab_model.h:69: void Observe(int type,
On 2014/10/21 19:36:11, msw wrote:
> I guess this is implementing the following policy? "For clarity, use exactly
one
> of override, final, or virtual when declaring an override."

Yep.

[msw, 2014-10-21 20:17:03]

okay, rubber stamp lgtm