Move creation of the various WebDatabaseTable types out of WebDatabase.

Move creation of the various WebDatabaseTable types out of WebDatabase.

This removes the last of its knowledge of the various table types,
which helps componentize it (see bug).

TBR=robertshield@chromium.org,tim@chromium.org
BUG=181277
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=189453

[Jói, 2013-03-12 04:07:24]

dhollowa: Main reviewer.
caitkp: FYI and for comment.

Cheers,
Jói

[dhollowa, 2013-03-12 21:29:37]

Great stuff.  Overall looks good.  Mainly just one implementation question.

https://codereview.chromium.org/12543034/diff/4001/chrome/browser/webdata/web...
File chrome/browser/webdata/web_database.h (right):

https://codereview.chromium.org/12543034/diff/4001/chrome/browser/webdata/web...
chrome/browser/webdata/web_database.h:76: TableMap tables_;
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.

https://codereview.chromium.org/12543034/diff/4001/chrome/browser/webdata/web...
File chrome/browser/webdata/web_database_migration_unittest.cc (right):

https://codereview.chromium.org/12543034/diff/4001/chrome/browser/webdata/web...
chrome/browser/webdata/web_database_migration_unittest.cc:174: // TODO(joi):
This whole unit test file needs to stay in //chrome
Is there a bug to track this?  Need one?

https://codereview.chromium.org/12543034/diff/4001/chrome/browser/webdata/web...
File chrome/browser/webdata/web_database_table.h (right):

https://codereview.chromium.org/12543034/diff/4001/chrome/browser/webdata/web...
chrome/browser/webdata/web_database_table.h:58: sql::Connection* db_;
A comment on these two members regarding ownership would be helpful.  I.e. who
owns them and lifetime?

[Jói, 2013-03-12 21:59:51]

https://codereview.chromium.org/12543034/diff/4001/chrome/browser/webdata/web...
File chrome/browser/webdata/web_database.h (right):

https://codereview.chromium.org/12543034/diff/4001/chrome/browser/webdata/web...
chrome/browser/webdata/web_database.h:76: TableMap tables_;
On 2013/03/12 21:29:38, dhollowa wrote:
> 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.

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).

https://codereview.chromium.org/12543034/diff/4001/chrome/browser/webdata/web...
File chrome/browser/webdata/web_database_migration_unittest.cc (right):

https://codereview.chromium.org/12543034/diff/4001/chrome/browser/webdata/web...
chrome/browser/webdata/web_database_migration_unittest.cc:174: // TODO(joi):
This whole unit test file needs to stay in //chrome
On 2013/03/12 21:29:38, dhollowa wrote:
> Is there a bug to track this?  Need one?

Just the overall "Componentize WebData" bug (http://crbug.com/181277) which I
think is sufficient as we go through TODOs in code related to the component
before we close out such bugs.

https://codereview.chromium.org/12543034/diff/4001/chrome/browser/webdata/web...
File chrome/browser/webdata/web_database_table.h (right):

https://codereview.chromium.org/12543034/diff/4001/chrome/browser/webdata/web...
chrome/browser/webdata/web_database_table.h:58: sql::Connection* db_;
On 2013/03/12 21:29:38, dhollowa wrote:
> A comment on these two members regarding ownership would be helpful.  I.e. who
> owns them and lifetime?

Done.

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

lgtm

https://codereview.chromium.org/12543034/diff/4001/chrome/browser/webdata/web...
File chrome/browser/webdata/web_database.h (right):

https://codereview.chromium.org/12543034/diff/4001/chrome/browser/webdata/web...
chrome/browser/webdata/web_database.h:76: TableMap tables_;
On 2013/03/12 21:59:53, Jói wrote:
> On 2013/03/12 21:29:38, dhollowa wrote:
> > 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.
> 
> 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).

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.

[Jói, 2013-03-13 03:08:11]

TBR=robertshield@chromium.org for trivial usage update in //chrome_frame.

TBR=tim@chromium.org for usage updates in sync.

This is to update usage of WebData-related code, already reviwed by
dhollowa@chromium.org who is the owner for //chrome/browser/webdata.

Cheers,
Jói

[commit-bot: I haz the power, 2013-03-13 03:11:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/12543034/19001

[commit-bot: I haz the power, 2013-03-13 03:11:24]

Failed to apply patch for chrome/browser/webdata/autofill_table.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/webdata/autofill_table.cc
  Hunk #2 FAILED at 320.
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/webdata/autofill_table.cc.rej

Patch:       chrome/browser/webdata/autofill_table.cc
Index: chrome/browser/webdata/autofill_table.cc
diff --git a/chrome/browser/webdata/autofill_table.cc
b/chrome/browser/webdata/autofill_table.cc
index
279ecfc6ad042be2675f1a51dc8fba40f30bb244..b4950d8a2a65d2fc4566f6ccad3ad2990dac3473
100644
--- a/chrome/browser/webdata/autofill_table.cc
+++ b/chrome/browser/webdata/autofill_table.cc
@@ -20,6 +20,7 @@
 #include "chrome/browser/password_manager/encryptor.h"
 #include "chrome/browser/webdata/autofill_change.h"
 #include "chrome/browser/webdata/autofill_entry.h"
+#include "chrome/browser/webdata/web_database.h"
 #include "components/autofill/browser/autofill_country.h"
 #include "components/autofill/browser/autofill_profile.h"
 #include "components/autofill/browser/autofill_type.h"
@@ -319,29 +320,87 @@ bool RemoveAutofillProfilePieces(const std::string& guid,
sql::Connection* db) {
   return s3.Run();
 }
 
+WebDatabaseTable::TypeKey GetKey() {
+  return reinterpret_cast<void*>(&AutofillTable::FromWebDatabase);
+}
+
 }  // namespace
 
 // The maximum length allowed for form data.
 const size_t AutofillTable::kMaxDataLength = 1024;
 
-AutofillTable::AutofillTable(sql::Connection* db, sql::MetaTable* meta_table)
-    : WebDatabaseTable(db, meta_table) {
+AutofillTable::AutofillTable() {
 }
 
 AutofillTable::~AutofillTable() {
 }
 
-bool AutofillTable::Init() {
- return (InitMainTable() && InitCreditCardsTable() && InitDatesTable() &&
-     InitProfilesTable() && InitProfileNamesTable() &&
-     InitProfileEmailsTable() && InitProfilePhonesTable() &&
-     InitProfileTrashTable());
+AutofillTable* AutofillTable::FromWebDatabase(WebDatabase* db) {
+  return static_cast<AutofillTable*>(db->GetTable(GetKey()));
+}
+
+WebDatabaseTable::TypeKey AutofillTable::GetTypeKey() const {
+  return GetKey();
+}
+
+bool AutofillTable::Init(sql::Connection* db, sql::MetaTable* meta_table) {
+  WebDatabaseTable::Init(db, meta_table);
+  return (InitMainTable() && InitCreditCardsTable() && InitDatesTable() &&
+          InitProfilesTable() && InitProfileNamesTable() &&
+          InitProfileEmailsTable() && InitProfilePhonesTable() &&
+          InitProfileTrashTable());
 }
 
 bool AutofillTable::IsSyncable() {
   return true;
 }
 
+bool AutofillTable::MigrateToVersion(int version,
+                                     const std::string& app_locale,
+                                     bool* update_compatible_version) {
+  // Migrate if necessary.
+  switch (version) {
+    case 22:
+      return ClearAutofillEmptyValueElements();
+    case 23:
+      return MigrateToVersion23AddCardNumberEncryptedColumn();
+    case 24:
+      return MigrateToVersion24CleanupOversizedStringFields();
+    case 27:
+      *update_compatible_version = true;
+      return MigrateToVersion27UpdateLegacyCreditCards();
+    case 30:
+      *update_compatible_version = true;
+      return MigrateToVersion30AddDateModifed();
+    case 31:
+      *update_compatible_version = true;
+      return MigrateToVersion31AddGUIDToCreditCardsAndProfiles();
+    case 32:
+      *update_compatible_version = true;
+      return MigrateToVersion32UpdateProfilesAndCreditCards();
+    case 33:
+      *update_compatible_version = true;
+      return MigrateToVersion33ProfilesBasedOnFirstName();
+    case 34:
+      *update_compatible_version = true;
+      return MigrateToVersion34ProfilesBasedOnCountryCode(app_locale);
+    case 35:
+      *update_compatible_version = true;
+      return MigrateToVersion35GreatBritainCountryCodes();
+    // Combine migrations 36 and 37.  This is due to enhancements to the merge
+    // step when migrating profiles.  The original migration from 35 to 36 did
+    // not merge profiles with identical addresses, but the migration from 36
to
+    // 37 does.  The step from 35 to 36 should only happen on the Chrome 12 dev
+    // channel.  Chrome 12 beta and release users will jump from 35 to 37
+    // directly getting the full benefits of the multi-valued merge as well as
+    // the culling of bad data.
+    case 37:
+      *update_compatible_version = true;
+      return MigrateToVersion37MergeAndCullOlderProfiles();
+  }
+  return true;
+}
+
 bool AutofillTable::AddFormFieldValues(
     const std::vector<FormFieldData>& elements,
     std::vector<AutofillChange>* changes) {

[commit-bot: I haz the power, 2013-03-13 03:17:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/12543034/19002

[commit-bot: I haz the power, 2013-03-13 04:59:30]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-03-20 19:32:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/12543034/69001

[commit-bot: I haz the power, 2013-03-20 23:21:03]

Change committed as 189453