RefCounted types should not have public destructors, chrome/browser/ part 1

RefCounted types should not have public destructors, chrome/browser/ part 1

BUG=123295
TEST=none


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

[Ryan Sleevi, 2012-04-13 08:54:57]

eroman: chrome/browser/net
sky: chrome/browser/history
cpu: chrome/browser/component_updater

[sky, 2012-04-13 15:50:52]

http://codereview.chromium.org/10071032/diff/1/chrome/browser/history/history...
File chrome/browser/history/history_browsertest.cc (right):

http://codereview.chromium.org/10071032/diff/1/chrome/browser/history/history...
chrome/browser/history/history_browsertest.cc:45: virtual ~WaitForHistoryTask()
{}
There are subclasses of this class. Won't this need to be protected?

http://codereview.chromium.org/10071032/diff/1/chrome/browser/history/history...
File chrome/browser/history/history_extension_api.h (right):

http://codereview.chromium.org/10071032/diff/1/chrome/browser/history/history...
chrome/browser/history/history_extension_api.h:51: protected:
Did you intend to make Run/RunIMpl protected?

http://codereview.chromium.org/10071032/diff/1/chrome/browser/history/in_memo...
File chrome/browser/history/in_memory_url_index.h (right):

http://codereview.chromium.org/10071032/diff/1/chrome/browser/history/in_memo...
chrome/browser/history/in_memory_url_index.h:173: virtual
~RebuildPrivateDataFromHistoryDBTask();
Move implementation to match new position.

http://codereview.chromium.org/10071032/diff/1/chrome/browser/history/shortcu...
File chrome/browser/history/shortcuts_backend.h (right):

http://codereview.chromium.org/10071032/diff/1/chrome/browser/history/shortcu...
chrome/browser/history/shortcuts_backend.h:131: virtual ~ShortcutsBackend();
Move implementation to match new position.

http://codereview.chromium.org/10071032/diff/1/chrome/browser/history/shortcu...
File chrome/browser/history/shortcuts_database.h (right):

http://codereview.chromium.org/10071032/diff/1/chrome/browser/history/shortcu...
chrome/browser/history/shortcuts_database.h:70: virtual ~ShortcutsDatabase();
Move implementation to match.

[cpu_(ooo_6.6-7.5), 2012-04-13 17:41:40]

lgtm on the component updater

[eroman, 2012-04-13 18:47:25]

http://codereview.chromium.org/10071032/diff/1/chrome/browser/net/sqlite_pers...
File chrome/browser/net/sqlite_persistent_cookie_store.h (right):

http://codereview.chromium.org/10071032/diff/1/chrome/browser/net/sqlite_pers...
chrome/browser/net/sqlite_persistent_cookie_store.h:52: virtual
~SQLitePersistentCookieStore();
Is it intentional that these lack implementations?

[Ryan Sleevi, 2012-04-13 18:52:34]

http://codereview.chromium.org/10071032/diff/1/chrome/browser/net/sqlite_pers...
File chrome/browser/net/sqlite_persistent_cookie_store.h (right):

http://codereview.chromium.org/10071032/diff/1/chrome/browser/net/sqlite_pers...
chrome/browser/net/sqlite_persistent_cookie_store.h:52: virtual
~SQLitePersistentCookieStore();
On 2012/04/13 18:47:25, eroman wrote:
> Is it intentional that these lack implementations?

The implementation is in the .cc file. All that changed was the placement.

[Ryan Sleevi, 2012-04-13 19:00:25]

http://codereview.chromium.org/10071032/diff/1/chrome/browser/history/history...
File chrome/browser/history/history_browsertest.cc (right):

http://codereview.chromium.org/10071032/diff/1/chrome/browser/history/history...
chrome/browser/history/history_browsertest.cc:45: virtual ~WaitForHistoryTask()
{}
On 2012/04/13 15:50:52, sky wrote:
> There are subclasses of this class. Won't this need to be protected?

There are no subclasses. This is just in this unittest

http://codereview.chromium.org/10071032/diff/1/chrome/browser/history/history...
File chrome/browser/history/history_extension_api.h (right):

http://codereview.chromium.org/10071032/diff/1/chrome/browser/history/history...
chrome/browser/history/history_extension_api.h:51: protected:
On 2012/04/13 15:50:52, sky wrote:
> Did you intend to make Run/RunIMpl protected?

Run() no, RunImpl() yes.

Similar things can be said about RunAsyncImpl/SendAsyncResponse below, which
should be protected, and not public.

Extensions (as a whole - there's ~60-70 files that are various parts of
extensions) are the sort of egregious offenders today of mixing and matching
public -> protected/private, and private/protected -> public (unnecessarily). Or
just declaring things public "because" (see, for example, lines 95+110+132+154 -
all of which are private implementation details)

Since public -> protected/private does nothing, it's something we can easily
warn about in a Clang plugin and correct. I anticipate such a change to be
yet-another-400-file change.

[eroman, 2012-04-13 19:26:38]

LGTM for browser/net/*

http://codereview.chromium.org/10071032/diff/1/chrome/browser/net/sqlite_pers...
File chrome/browser/net/sqlite_persistent_cookie_store.h (right):

http://codereview.chromium.org/10071032/diff/1/chrome/browser/net/sqlite_pers...
chrome/browser/net/sqlite_persistent_cookie_store.h:52: virtual
~SQLitePersistentCookieStore();
On 2012/04/13 18:52:35, Ryan Sleevi wrote:
> On 2012/04/13 18:47:25, eroman wrote:
> > Is it intentional that these lack implementations?
> 
> The implementation is in the .cc file. All that changed was the placement.

oops, misread that!

[commit-bot: I haz the power, 2012-04-25 05:28:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/10071032/9001

[commit-bot: I haz the power, 2012-04-25 05:28:19]

Presubmit check for 10071032-9001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/history

Presubmit checks took 2.8s to calculate.

[Ryan Sleevi, 2012-04-25 05:29:23]

sky: Looks like I never collected my LGTM stamp from you :)

[sky, 2012-04-27 03:38:48]

LGTM

[commit-bot: I haz the power, 2012-04-27 03:40:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/10071032/9001

[commit-bot: I haz the power, 2012-04-27 05:07:20]

Change committed as 134231