Migrate old Shortcuts DB data to conform to new type values

chrome/browser/history/shortcuts_database.cc

Index: chrome/browser/history/shortcuts_database.cc
diff --git a/chrome/browser/history/shortcuts_database.cc b/chrome/browser/history/shortcuts_database.cc
index 51d6f09293fbffe58da621bfab05911739d5a9d8..a9851d73744940075a1dc50803bcb08e4eade199 100644
--- a/chrome/browser/history/shortcuts_database.cc
+++ b/chrome/browser/history/shortcuts_database.cc
@@ -196,6 +196,60 @@ bool ShortcutsDatabase::EnsureTable() {
         transaction.Commit();
   }
 
+  // With new suggestion types being added, following values have changed:

[Peter Kasting, 2014/02/05 21:59:39]

We really should do migration passes based on version numbers, so you can
quickly avoid ever running this migration more than once.  See e.g.
history_database.cc for how this can be done.

[Anuj, 2014/02/10 22:17:30]

There is no meta-table in Shortcuts DB. So just conditioned this on the presence
of MetaTable.

+  // SEARCH_OTHER_ENGINE -> SEARCH_SUGGEST_ENTITY
+  // EXTENSION_APP -> SEARCH_SUGGEST_INFINITE
+  // CONTACT -> SEARCH_SUGGEST_PERSONALIZED
+  // BOOKMARK_TITLE -> SEARCH_SUGGEST_PROFILE
+  sql::Transaction transaction(&db_);
+  transaction.Begin();
+  // Migrate old SEARCH_OTHER_ENGINE values to the new type value.
+  // The ENTITY suggestion values always have a description and always have a
+  // https google search url with gs_ssp parameter.  The old SEARCH_OTHER_ENGINE
+  // entries will definitely fail the url check, but empty description test is
+  // also sufficient and faster.

[Peter Kasting, 2014/02/05 21:59:39]

Is it actually necessary to do such sophisticated matching of URLs?  I mean,
it's not really wrong or forbidden to do so, I just figured it'd be easier to
simply map one set of types to another and not worry about the few that got
mis-filed.

[Anuj, 2014/02/10 22:17:30]

I am not sure. I feel if we are bothering to make the change, might as well do
it as a best effort. It is a one time modification.
I can express that as simpler LIKE queries if you prefer.

[Peter Kasting, 2014/02/10 22:29:46]

I'd probably write the "dumbest" possible code that simply transforms all rows
with type A to type B.  That's why I said earlier that it doesn't matter too
much if that transformation improperly marks some of these.

That results in code that's easier to test (which, incidentally, reminds me:
this CL needs a migration test using an old .sql file, see how we test e.g.
keywords table migration steps) and is also easier to read.

[Anuj, 2014/02/11 07:20:04]

Done.

+  db_.Execute(base::StringPrintf("UPDATE omni_box_shortcuts "
+        "SET type = %d "
+        "where type = %d AND "
+        "(description = '' OR "
+        "url NOT REGEXP '^https://www\\.google\\..*/search\\?.*&?gs_ssp=.*')",
+        static_cast<int>(AutocompleteMatchType::SEARCH_OTHER_ENGINE),

[Peter Kasting, 2014/02/05 21:59:39]

For both old and new type values, hardcode the numeric value instead of using
the enum value.

This is so that if we change the enum in the future, it doesn't break this
migration code, which wants to always assume certain values for the old and new
versions of the database.

[Anuj, 2014/02/10 22:17:30]

Done.

+        static_cast<int>(AutocompleteMatchType::SEARCH_SUGGEST_ENTITY))
+      .c_str()) &&

[Peter Kasting, 2014/02/05 21:59:39]

Nit: If you're going to chain statements with &&, the subsequent statements need
to be indented.

[Anuj, 2014/02/10 22:17:30]

They are not indented in the block above - lines  181-196. Shall I change that
too.

[Peter Kasting, 2014/02/10 22:29:46]

They are, in fact, indented.  They're part of the "return" that begins line 181,
which is why all statements on line 182+ are indented 4 from that return.

[Anuj, 2014/02/11 07:20:04]

Done.

+  // Migrate old EXTENSION_APP values to the new type value.
+  // The INFINITE suggestions are not launched. So it is safe to update all of
+  // them to EXTENSION_APP type wherever the url is not a google search.
+  db_.Execute(base::StringPrintf("UPDATE omni_box_shortcuts "
+        "SET type = %d "
+        "where type = %d AND "
+        "url NOT REGEXP '^https://www\\.google\\..*/search\\?.*')",
+        static_cast<int>(AutocompleteMatchType::EXTENSION_APP),
+        static_cast<int>(AutocompleteMatchType::SEARCH_SUGGEST_INFINITE))
+      .c_str()) &&
+  // Migrate old CONTACT values to the new type value.
+  // The PERSONALIZED suggestions are not launched. So it is safe to update all
+  // of them to CONTACT type wherever the url is not a google search.
+  db_.Execute(base::StringPrintf("UPDATE omni_box_shortcuts "
+        "SET type = %d "
+        "where type = %d AND "
+        "url NOT REGEXP '^https://www\\.google\\..*/search\\?.*')",
+        static_cast<int>(AutocompleteMatchType::CONTACT),
+        static_cast<int>(AutocompleteMatchType::SEARCH_SUGGEST_PERSONALIZED))
+      .c_str()) &&
+  // Migrate old BOOKMARK_TITLE values to the new type value.
+  // The PROFILE suggestion values always have a description and always have a
+  // https google search url with tbs parameter.  The old BOOKMARK_TITLE entries
+  // will definitely fail the url check, but empty description test is also
+  // sufficient and faster.
+  db_.Execute(base::StringPrintf("UPDATE omni_box_shortcuts "
+        "SET type = %d "
+        "where type = %d AND "
+        "(description = '' OR "
+        "url NOT REGEXP '^https://www\\.google\\..*/search\\?.*&?tbs=.*')",
+        static_cast<int>(AutocompleteMatchType::BOOKMARK_TITLE),
+        static_cast<int>(AutocompleteMatchType::SEARCH_SUGGEST_PROFILE))
+      .c_str()) &&
+  transaction.Commit();
   return true;

[Peter Kasting, 2014/02/05 21:59:39]

Seems like here we should be returning the result of the above boolean
statement.

[Anuj, 2014/02/10 22:17:30]

But we are okay about failing to do this modification. (because we are okay
about misfiling some). I don't want to cause any profile-version mismatch
errors.

[Peter Kasting, 2014/02/10 22:29:46]

If you fail to execute one of these statements, something went badly wrong and
the database could be in an inconsistent state.  You need to return false when
something fails.

There are a hundred or so other migration steps scattered around other tables in
the codebase.  Every one of them actually returns whether migration succeeded. 
Please don't do something different :)

[Anuj, 2014/02/11 07:20:04]

Done.

 }