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

chrome/browser/history/history_extension_api.h

Index: chrome/browser/history/history_extension_api.h
diff --git a/chrome/browser/history/history_extension_api.h b/chrome/browser/history/history_extension_api.h
index e66ea76882ed063d6a6119c07cd09246fffe799f..d1333136f5d384ba1bc3553d6f72569e91d1ae5a 100644
--- a/chrome/browser/history/history_extension_api.h
+++ b/chrome/browser/history/history_extension_api.h
@@ -48,7 +48,8 @@ class HistoryExtensionEventRouter : public content::NotificationObserver {
 
 // Base class for history function APIs.
 class HistoryFunction : public AsyncExtensionFunction {
- public:
+ protected:

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

Did you intend to make Run/RunIMpl protected?

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

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.

+  virtual ~HistoryFunction() {}
   virtual void Run() OVERRIDE;
   virtual bool RunImpl() = 0;
 
@@ -61,7 +62,6 @@ class HistoryFunction : public AsyncExtensionFunction {
 class HistoryFunctionWithCallback : public HistoryFunction {
  public:
   HistoryFunctionWithCallback();
-  virtual ~HistoryFunctionWithCallback();
 
   // Return true if the async call was completed, false otherwise.
   virtual bool RunAsyncImpl() = 0;
@@ -70,10 +70,12 @@ class HistoryFunctionWithCallback : public HistoryFunction {
   // This method calls Release().
   virtual void SendAsyncResponse();
 
+ protected:
+  virtual ~HistoryFunctionWithCallback();
+
   // Override HistoryFunction::RunImpl.
   virtual bool RunImpl() OVERRIDE;
 
- protected:
   // The consumer for the HistoryService callbacks.
   CancelableRequestConsumer cancelable_consumer_;
 
@@ -94,6 +96,9 @@ class GetVisitsHistoryFunction : public HistoryFunctionWithCallback {
                      bool success,
                      const history::URLRow* url_row,
                      history::VisitVector* visits);
+
+ protected:
+  virtual ~GetVisitsHistoryFunction() {}
 };
 
 class SearchHistoryFunction : public HistoryFunctionWithCallback {
@@ -104,12 +109,18 @@ class SearchHistoryFunction : public HistoryFunctionWithCallback {
   // Callback for the history function to provide results.
   void SearchComplete(HistoryService::Handle request_handle,
                       history::QueryResults* results);
+
+ protected:
+  virtual ~SearchHistoryFunction() {}
 };
 
 class AddUrlHistoryFunction : public HistoryFunction {
  public:
   virtual bool RunImpl() OVERRIDE;
   DECLARE_EXTENSION_FUNCTION_NAME("history.addUrl");
+
+ protected:
+  virtual ~AddUrlHistoryFunction() {}
 };
 
 class DeleteAllHistoryFunction : public HistoryFunctionWithCallback {
@@ -119,6 +130,9 @@ class DeleteAllHistoryFunction : public HistoryFunctionWithCallback {
 
   // Callback for the history service to acknowledge deletion.
   void DeleteComplete();
+
+ protected:
+  virtual ~DeleteAllHistoryFunction() {}
 };
 
 
@@ -126,6 +140,9 @@ class DeleteUrlHistoryFunction : public HistoryFunction {
  public:
   virtual bool RunImpl() OVERRIDE;
   DECLARE_EXTENSION_FUNCTION_NAME("history.deleteUrl");
+
+ protected:
+  virtual ~DeleteUrlHistoryFunction() {}
 };
 
 class DeleteRangeHistoryFunction : public HistoryFunctionWithCallback {
@@ -135,6 +152,9 @@ class DeleteRangeHistoryFunction : public HistoryFunctionWithCallback {
 
   // Callback for the history service to acknowledge deletion.
   void DeleteComplete();
+
+ protected:
+  virtual ~DeleteRangeHistoryFunction() {}
 };
 
 #endif  // CHROME_BROWSER_HISTORY_HISTORY_EXTENSION_API_H_