Move creation of the various WebDatabaseTable types out of WebDatabase.

chrome/browser/webdata/web_database.h

 // Copyright (c) 2012 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_WEBDATA_WEB_DATABASE_H_
 #define CHROME_BROWSER_WEBDATA_WEB_DATABASE_H_
 
+#include <map>
+
 #include "base/memory/scoped_ptr.h"
-#include "base/memory/scoped_vector.h"
+#include "chrome/browser/webdata/web_database_table.h"
 #include "sql/connection.h"
 #include "sql/init_status.h"
 #include "sql/meta_table.h"
 
-class AutofillTable;
-class KeywordTable;
-class LoginsTable;
-class TokenServiceTable;
-class WebAppsTable;
-class WebDatabaseTable;
-class WebIntentsTable;
-
 namespace base {
 class FilePath;
 }
 
 namespace content {
 class NotificationService;
 }
 
 // This class manages a SQLite database that stores various web page meta data.
 class WebDatabase {
  public:
   enum State {
     COMMIT_NOT_NEEDED,
     COMMIT_NEEDED
   };
   // Exposed publicly so the keyword table can access it.
   static const int kCurrentVersionNumber;
 
   WebDatabase();
   virtual ~WebDatabase();
 
+  // Adds a database table. Ownership remains with the caller, which
+  // must ensure that the lifetime of |table| exceeds this object's
+  // lifetime. Must only be called before Init.
+  void AddTable(WebDatabaseTable* table);
+
+  // Retrieves a table based on its |key|.
+  WebDatabaseTable* GetTable(WebDatabaseTable::TypeKey key);
+
   // Initialize the database given a name. The name defines where the SQLite
   // file is. If this returns an error code, no other method should be called.
   // Requires the |app_locale| to be passed as a parameter as the locale can
   // only safely be queried on the UI thread.
-  sql::InitStatus Init(const base::FilePath& db_name,
-                       const std::string& app_locale);
+  //
+  // Before calling this method, you must call AddTable for any
+  // WebDatabaseTable objects that are supposed to participate in
+  // managing the database.
+  sql::InitStatus Init(
+      const base::FilePath& db_name,
+      const std::string& app_locale);
 
   // Transactions management
   void BeginTransaction();
   void CommitTransaction();
 
-  virtual AutofillTable* GetAutofillTable();
-  virtual KeywordTable* GetKeywordTable();
-  virtual LoginsTable* GetLoginsTable();
-  virtual TokenServiceTable* GetTokenServiceTable();
-  virtual WebAppsTable* GetWebAppsTable();
-
   // Exposed for testing only.
   sql::Connection* GetSQLConnection();
 
  private:
   // Used by |Init()| to migration database schema from older versions to
   // current version.  Requires the |app_locale| to be passed as a parameter as
   // the locale can only safely be queried on the UI thread.
   sql::InitStatus MigrateOldVersionsAsNeeded(const std::string& app_locale);
 
   sql::Connection db_;
   sql::MetaTable meta_table_;
 
-  // TODO(joi): All of the typed pointers are going in a future
-  // change, as we remove knowledge of the specific types from this
-  // class.
-  AutofillTable* autofill_table_;
-  KeywordTable* keyword_table_;
-  LoginsTable* logins_table_;
-  TokenServiceTable* token_service_table_;
-  WebAppsTable* web_apps_table_;
-  // TODO(thakis): Add a migration to delete this table, then remove this.
-  WebIntentsTable* web_intents_table_;
-
-  // Owns all the different database tables that have been added to
-  // this object.
-  ScopedVector<WebDatabaseTable> tables_;
+  // Map of all the different tables that have been added to this
+  // object. Non-owning.
+  typedef std::map<WebDatabaseTable::TypeKey, WebDatabaseTable*> TableMap;
+  TableMap tables_;

[dhollowa, 2013/03/12 21:29:38]

Did you consider using base::SupportsUserData for this?  It has some potential
advantages:
  1. Shared code instead of home-rolled, existing patterns with keys etc.
  2. It has support for richer lifetime semantics with the base::UserDataAdaptor
that allows for scoped_refptr semantics for shared lifetimes/ownership across
threads.

[Jói, 2013/03/12 21:59:53]

I thought about it, and that's where the static-as-key trick comes from.

I discarded it for one main reason, which is it seems intended for data the
class it hangs off of doesn't need to know about, so for a class that does know
about, and need to use, these things being hung off it, you're better off doing
something more specific.  If we were to use SupportsUserData here, we'd need to
nail down the types you're allowed to hang off the object (so we couldn't really
have it just inherit from SupportsUserData directly).

The other reason was ownership. SupportsUserData assumes ownership of the Data
items, although as you say you can decide the ownership semantics by having the
SupportsUserData::Data items point to something else, use UserDataAdaptor, etc.

I was ambivalent, and am willing to think more about this if you feel this would
be a better path. One small change that we'd need in //base is the ability to
enumerate over the SupportsUserData::Data items, but this is a small change.
After thinking about this for a few minutes, I'm starting to feel if we do
reuse, what we want maybe isn't SupportsUserData but something related
(something that possibly could be a superclass of SupportsUserData).

[dhollowa, 2013/03/12 23:34:37]

Ya, the ownership semantics and iteration are significant enough a departure
that I'm fine with it as-written.

I guess if we encounter a similar need elsewhere we can promote up as the need
occurs.

 
   scoped_ptr<content::NotificationService> notification_service_;
 
   DISALLOW_COPY_AND_ASSIGN(WebDatabase);
 };
 
 #endif  // CHROME_BROWSER_WEBDATA_WEB_DATABASE_H_