dhollowa: Main reviewer.
caitkp: FYI and for comment.
Cheers,
Jói

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?

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 (https://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.

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

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

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

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

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

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