Part 1 of repairing regressions to my old clang check plugins so Nico can

Part 1 of repairing regressions to my old clang check plugins so Nico can
deploy the clang plugins to the waterfall/trybots.

BUG=none
TEST=compiles

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

[Nico, 2011-01-27 03:55:17]

LG

A few questions (…and too much rambling).

http://codereview.chromium.org/6366019/diff/2001/chrome/browser/browsing_data...
File chrome/browser/browsing_data_indexed_db_helper.h (left):

http://codereview.chromium.org/6366019/diff/2001/chrome/browser/browsing_data...
chrome/browser/browsing_data_indexed_db_helper.h:52: return protocol ==
chrome::kFileScheme;
does your plugin warn on this? This looks pretty harmless to me.

http://codereview.chromium.org/6366019/diff/2001/chrome/browser/debugger/insp...
File chrome/browser/debugger/inspectable_tab_proxy.h (right):

http://codereview.chromium.org/6366019/diff/2001/chrome/browser/debugger/insp...
chrome/browser/debugger/inspectable_tab_proxy.h:83: virtual void
FrameNavigating(const std::string& url);
OVERRIDE? :-P

(requires base/compiler_specific.h)

(if so, also a few more times later)

http://codereview.chromium.org/6366019/diff/2001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/web_navigation_observer.h (left):

http://codereview.chromium.org/6366019/diff/2001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/web_navigation_observer.h:16: public:
no constructor/destructor here?

http://codereview.chromium.org/6366019/diff/2001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/web_navigation_observer.h:30: virtual bool
OnMessageReceived(const IPC::Message& message) { return false; }
this looks harmless too?

http://codereview.chromium.org/6366019/diff/2001/chrome/common/child_process_...
File chrome/common/child_process_host.h (left):

http://codereview.chromium.org/6366019/diff/2001/chrome/common/child_process_...
chrome/common/child_process_host.h:83: virtual bool OnMessageReceived(const
IPC::Message& msg) { return false; }
these two. i will stop repeating this question.

http://codereview.chromium.org/6366019/diff/2001/chrome/common/net/gaia/gaia_...
File chrome/common/net/gaia/gaia_auth_consumer.h (left):

http://codereview.chromium.org/6366019/diff/2001/chrome/common/net/gaia/gaia_...
chrome/common/net/gaia/gaia_auth_consumer.h:18: inline ClientLoginResult() :
two_factor(false) {}
lol inline

but since it's mentioned explicitly, maybe add whoever added this as reviewer,
just in case

http://codereview.chromium.org/6366019/diff/2001/chrome/renderer/autofill_hel...
File chrome/renderer/autofill_helper.h (right):

http://codereview.chromium.org/6366019/diff/2001/chrome/renderer/autofill_hel...
chrome/renderer/autofill_helper.h:36: virtual ~AutoFillHelper();
we don't usually say |virtual| for destructors in non-base classes, do we?

http://codereview.chromium.org/6366019/diff/2001/chrome/renderer/device_orien...
File chrome/renderer/device_orientation_dispatcher.h (right):

http://codereview.chromium.org/6366019/diff/2001/chrome/renderer/device_orien...
chrome/renderer/device_orientation_dispatcher.h:20: explicit
DeviceOrientationDispatcher(RenderView* render_view);
+1

http://codereview.chromium.org/6366019/diff/2001/chrome/service/remoting/chro...
File chrome/service/remoting/chromoting_host_manager.h (right):

http://codereview.chromium.org/6366019/diff/2001/chrome/service/remoting/chro...
chrome/service/remoting/chromoting_host_manager.h:63: friend class
base::RefCountedThreadSafe<ChromotingHostManager>;
! +1

[Elliot Glaysher, 2011-01-27 18:48:05]

FYI: chron (re: gaia_auth_consumer.h)

http://codereview.chromium.org/6366019/diff/2001/chrome/browser/browsing_data...
File chrome/browser/browsing_data_indexed_db_helper.h (left):

http://codereview.chromium.org/6366019/diff/2001/chrome/browser/browsing_data...
chrome/browser/browsing_data_indexed_db_helper.h:52: return protocol ==
chrome::kFileScheme;
On 2011/01/27 03:55:17, Nico wrote:
> does your plugin warn on this? This looks pretty harmless to me.

No, it didn't. I just did it for consistency with the other classes I did this
to. I agree that this is harmless. Undone.

http://codereview.chromium.org/6366019/diff/2001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/web_navigation_observer.h (left):

http://codereview.chromium.org/6366019/diff/2001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/web_navigation_observer.h:16: public:
On 2011/01/27 03:55:17, Nico wrote:
> no constructor/destructor here?

Correct. This is a base class whose base class already has declared virtualness
of the dtor. No member vars on this class, so as long as the subclasses have
their c/dtors ool this should be good.

http://codereview.chromium.org/6366019/diff/2001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/web_navigation_observer.h:30: virtual bool
OnMessageReceived(const IPC::Message& message) { return false; }
On 2011/01/27 03:55:17, Nico wrote:
> this looks harmless too?

See this code review for discussion between me and Peter:
http://codereview.chromium.org/5741001/

After doing that bit of measurement, I've adopted a policy of no non-empty
virtual methods (since most compilers have a common case for that) and check for
that in the plugin. There are probably a few cases where you are correct but I'm
now doing this for consistency so I can automate the check.  (though this is
probably not one of them since the derived class hierarchy here is wide instead
of deep.)

http://codereview.chromium.org/6366019/diff/2001/chrome/renderer/autofill_hel...
File chrome/renderer/autofill_helper.h (right):

http://codereview.chromium.org/6366019/diff/2001/chrome/renderer/autofill_hel...
chrome/renderer/autofill_helper.h:36: virtual ~AutoFillHelper();
On 2011/01/27 03:55:17, Nico wrote:
> we don't usually say |virtual| for destructors in non-base classes, do we?

Not only do we usually do this, but one of my checks is to warn on methods that
are virtual but aren't declared as virtual.

[Elliot Glaysher, 2011-01-27 18:48:33]

Actually add chron for the FYI (See one comment up)