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

chrome/browser/history/history_extension_api.h

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_HISTORY_HISTORY_EXTENSION_API_H_
 #define CHROME_BROWSER_HISTORY_HISTORY_EXTENSION_API_H_
 #pragma once
 
 #include <string>
 
@@ skipping 30 matching lines @@
 
   // Used for tracking registrations to history service notifications.
   content::NotificationRegistrar registrar_;
 
   DISALLOW_COPY_AND_ASSIGN(HistoryExtensionEventRouter);
 };
 
 
 // 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;
 
   bool GetUrlFromValue(base::Value* value, GURL* url);
   bool GetTimeFromValue(base::Value* value, base::Time* time);
 };
 
 // Base class for history funciton APIs which require async interaction with
 // chrome services and the extension thread.
 class HistoryFunctionWithCallback : public HistoryFunction {
  public:
   HistoryFunctionWithCallback();
-  virtual ~HistoryFunctionWithCallback();
 
   // Return true if the async call was completed, false otherwise.
   virtual bool RunAsyncImpl() = 0;
 
   // Call this method to report the results of the async method to the caller.
   // 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_;
 
  private:
   // The actual call to SendResponse.  This is required since the semantics for
   // CancelableRequestConsumerT require it to be accessed after the call.
   void SendResponseToCallback();
 };
 
 class GetVisitsHistoryFunction : public HistoryFunctionWithCallback {
  public:
   // Override HistoryFunction.
   virtual bool RunAsyncImpl() OVERRIDE;
   DECLARE_EXTENSION_FUNCTION_NAME("history.getVisits");
 
   // Callback for the history function to provide results.
   void QueryComplete(HistoryService::Handle request_service,
                      bool success,
                      const history::URLRow* url_row,
                      history::VisitVector* visits);
+
+ protected:
+  virtual ~GetVisitsHistoryFunction() {}
 };
 
 class SearchHistoryFunction : public HistoryFunctionWithCallback {
  public:
   virtual bool RunAsyncImpl() OVERRIDE;
   DECLARE_EXTENSION_FUNCTION_NAME("history.search");
 
   // 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 {
  public:
   virtual bool RunAsyncImpl() OVERRIDE;
   DECLARE_EXTENSION_FUNCTION_NAME("history.deleteAll");
 
   // Callback for the history service to acknowledge deletion.
   void DeleteComplete();
+
+ protected:
+  virtual ~DeleteAllHistoryFunction() {}
 };
 
 
 class DeleteUrlHistoryFunction : public HistoryFunction {
  public:
   virtual bool RunImpl() OVERRIDE;
   DECLARE_EXTENSION_FUNCTION_NAME("history.deleteUrl");
+
+ protected:
+  virtual ~DeleteUrlHistoryFunction() {}
 };
 
 class DeleteRangeHistoryFunction : public HistoryFunctionWithCallback {
  public:
   virtual bool RunAsyncImpl() OVERRIDE;
   DECLARE_EXTENSION_FUNCTION_NAME("history.deleteRange");
 
   // Callback for the history service to acknowledge deletion.
   void DeleteComplete();
+
+ protected:
+  virtual ~DeleteRangeHistoryFunction() {}
 };
 
 #endif  // CHROME_BROWSER_HISTORY_HISTORY_EXTENSION_API_H_