Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
134853004:
Migrate old Shortcuts DB data to conform to new type values (Closed)
|
Created:
6 years, 10 months ago by Anuj Modified:
6 years, 10 months ago CC:
chromium-reviews, browser-components-watch_chromium.org, David Black Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMigrate old Shortcuts DB data to conform to new type values
All new suggestion types will be a Google Search URL with https. In some cases,
url may contain additional identifying parameter.
BUG=339270
TBR=brettw@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251442
Patch Set 1 #
Total comments: 16
Patch Set 2 : Addressed Peter's comments - 1 #
Total comments: 8
Patch Set 3 : Addressed Peter's comments - 2 #
Total comments: 14
Patch Set 4 : Addressed comments #Patch Set 5 : Rebased #Patch Set 6 : Trying to work around the copy as well as modification of v1.sql #
Messages
Total messages: 37 (0 generated)
PTAL
https://codereview.chromium.org/134853004/diff/1/chrome/browser/history/short... File chrome/browser/history/shortcuts_database.cc (right): https://codereview.chromium.org/134853004/diff/1/chrome/browser/history/short... chrome/browser/history/shortcuts_database.cc:199: // With new suggestion types being added, following values have changed: 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. https://codereview.chromium.org/134853004/diff/1/chrome/browser/history/short... chrome/browser/history/shortcuts_database.cc:210: // also sufficient and faster. 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. https://codereview.chromium.org/134853004/diff/1/chrome/browser/history/short... chrome/browser/history/shortcuts_database.cc:216: static_cast<int>(AutocompleteMatchType::SEARCH_OTHER_ENGINE), 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. https://codereview.chromium.org/134853004/diff/1/chrome/browser/history/short... chrome/browser/history/shortcuts_database.cc:218: .c_str()) && Nit: If you're going to chain statements with &&, the subsequent statements need to be indented. https://codereview.chromium.org/134853004/diff/1/chrome/browser/history/short... chrome/browser/history/shortcuts_database.cc:253: return true; Seems like here we should be returning the result of the above boolean statement.
FYI, I plan to do a once-over on this changelist too, after Peter's happy with it. I doubt I'll have comments, but I would like the opportunity to say something before you submit it. At the moment, it generally looks okay to me, though, like Peter, I'd like to avoid ugly URL matching if possible. --mark
https://codereview.chromium.org/134853004/diff/1/chrome/browser/history/short... File chrome/browser/history/shortcuts_database.cc (right): https://codereview.chromium.org/134853004/diff/1/chrome/browser/history/short... chrome/browser/history/shortcuts_database.cc:199: // With new suggestion types being added, following values have changed: On 2014/02/05 21:59:39, Peter Kasting wrote: > 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. There is no meta-table in Shortcuts DB. So just conditioned this on the presence of MetaTable. https://codereview.chromium.org/134853004/diff/1/chrome/browser/history/short... chrome/browser/history/shortcuts_database.cc:210: // also sufficient and faster. On 2014/02/05 21:59:39, Peter Kasting wrote: > 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. 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. https://codereview.chromium.org/134853004/diff/1/chrome/browser/history/short... chrome/browser/history/shortcuts_database.cc:216: static_cast<int>(AutocompleteMatchType::SEARCH_OTHER_ENGINE), On 2014/02/05 21:59:39, Peter Kasting wrote: > 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. Done. https://codereview.chromium.org/134853004/diff/1/chrome/browser/history/short... chrome/browser/history/shortcuts_database.cc:218: .c_str()) && On 2014/02/05 21:59:39, Peter Kasting wrote: > Nit: If you're going to chain statements with &&, the subsequent statements need > to be indented. They are not indented in the block above - lines 181-196. Shall I change that too. https://codereview.chromium.org/134853004/diff/1/chrome/browser/history/short... chrome/browser/history/shortcuts_database.cc:253: return true; On 2014/02/05 21:59:39, Peter Kasting wrote: > Seems like here we should be returning the result of the above boolean > statement. 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.
https://codereview.chromium.org/134853004/diff/60001/chrome/browser/history/s... File chrome/browser/history/shortcuts_database.cc (right): https://codereview.chromium.org/134853004/diff/60001/chrome/browser/history/s... chrome/browser/history/shortcuts_database.cc:23: const int kCurrentVersionNumber = 28; Not sure what version numbers I should use here.
https://codereview.chromium.org/134853004/diff/1/chrome/browser/history/short... File chrome/browser/history/shortcuts_database.cc (right): https://codereview.chromium.org/134853004/diff/1/chrome/browser/history/short... chrome/browser/history/shortcuts_database.cc:210: // also sufficient and faster. On 2014/02/10 22:17:30, Anuj wrote: > On 2014/02/05 21:59:39, Peter Kasting wrote: > > 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. > > 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. 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. https://codereview.chromium.org/134853004/diff/1/chrome/browser/history/short... chrome/browser/history/shortcuts_database.cc:218: .c_str()) && On 2014/02/10 22:17:30, Anuj wrote: > On 2014/02/05 21:59:39, Peter Kasting wrote: > > Nit: If you're going to chain statements with &&, the subsequent statements > need > > to be indented. > > They are not indented in the block above - lines 181-196. Shall I change that > too. 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. https://codereview.chromium.org/134853004/diff/1/chrome/browser/history/short... chrome/browser/history/shortcuts_database.cc:253: return true; On 2014/02/10 22:17:30, Anuj wrote: > On 2014/02/05 21:59:39, Peter Kasting wrote: > > Seems like here we should be returning the result of the above boolean > > statement. > > 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. 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 :) https://codereview.chromium.org/134853004/diff/60001/chrome/browser/history/s... File chrome/browser/history/shortcuts_database.cc (right): https://codereview.chromium.org/134853004/diff/60001/chrome/browser/history/s... chrome/browser/history/shortcuts_database.cc:23: const int kCurrentVersionNumber = 28; On 2014/02/10 22:18:44, Anuj wrote: > Not sure what version numbers I should use here. If you didn't have a table before, use version 1 (for both). https://codereview.chromium.org/134853004/diff/60001/chrome/browser/history/s... chrome/browser/history/shortcuts_database.cc:168: int ShortcutsDatabase::GetCurrentVersion() { It doesn't seem like we need this as its own function, at least as this patch is coded? https://codereview.chromium.org/134853004/diff/60001/chrome/browser/history/s... chrome/browser/history/shortcuts_database.cc:211: if (sql::MetaTable::DoesTableExist(&db_)) { Don't you mean !exist? https://codereview.chromium.org/134853004/diff/60001/chrome/browser/history/s... chrome/browser/history/shortcuts_database.cc:213: // With new suggestion types being added, following values have changed: This comment is more confusing than helpful. We didn't change the meaning of SEARCH_OTHER_ENGINE to SEARCH_SUGGEST_ENTITY, we just renumbered the enum values. If you want to write a comment saying things like "SEARCH_OTHER_ENGINE changed from 9 to 13", that'd be OK, but I think if we make the migration steps simple, they'll become self-documenting so such a comment won't be necessary.
https://codereview.chromium.org/134853004/diff/1/chrome/browser/history/short... File chrome/browser/history/shortcuts_database.cc (right): https://codereview.chromium.org/134853004/diff/1/chrome/browser/history/short... chrome/browser/history/shortcuts_database.cc:210: // also sufficient and faster. On 2014/02/10 22:29:46, Peter Kasting wrote: > On 2014/02/10 22:17:30, Anuj wrote: > > On 2014/02/05 21:59:39, Peter Kasting wrote: > > > 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. > > > > 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. > > 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. Done. https://codereview.chromium.org/134853004/diff/1/chrome/browser/history/short... chrome/browser/history/shortcuts_database.cc:218: .c_str()) && On 2014/02/10 22:29:46, Peter Kasting wrote: > On 2014/02/10 22:17:30, Anuj wrote: > > On 2014/02/05 21:59:39, Peter Kasting wrote: > > > Nit: If you're going to chain statements with &&, the subsequent statements > > need > > > to be indented. > > > > They are not indented in the block above - lines 181-196. Shall I change that > > too. > > 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. Done. https://codereview.chromium.org/134853004/diff/1/chrome/browser/history/short... chrome/browser/history/shortcuts_database.cc:253: return true; On 2014/02/10 22:29:46, Peter Kasting wrote: > On 2014/02/10 22:17:30, Anuj wrote: > > On 2014/02/05 21:59:39, Peter Kasting wrote: > > > Seems like here we should be returning the result of the above boolean > > > statement. > > > > 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. > > 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 :) Done. https://codereview.chromium.org/134853004/diff/60001/chrome/browser/history/s... File chrome/browser/history/shortcuts_database.cc (right): https://codereview.chromium.org/134853004/diff/60001/chrome/browser/history/s... chrome/browser/history/shortcuts_database.cc:168: int ShortcutsDatabase::GetCurrentVersion() { On 2014/02/10 22:29:47, Peter Kasting wrote: > It doesn't seem like we need this as its own function, at least as this patch is > coded? Done. https://codereview.chromium.org/134853004/diff/60001/chrome/browser/history/s... chrome/browser/history/shortcuts_database.cc:211: if (sql::MetaTable::DoesTableExist(&db_)) { On 2014/02/10 22:29:47, Peter Kasting wrote: > Don't you mean !exist? Done. https://codereview.chromium.org/134853004/diff/60001/chrome/browser/history/s... chrome/browser/history/shortcuts_database.cc:213: // With new suggestion types being added, following values have changed: On 2014/02/10 22:29:47, Peter Kasting wrote: > This comment is more confusing than helpful. We didn't change the meaning of > SEARCH_OTHER_ENGINE to SEARCH_SUGGEST_ENTITY, we just renumbered the enum > values. If you want to write a comment saying things like "SEARCH_OTHER_ENGINE > changed from 9 to 13", that'd be OK, but I think if we make the migration steps > simple, they'll become self-documenting so such a comment won't be necessary. Done.
LGTM https://codereview.chromium.org/134853004/diff/130001/chrome/browser/history/... File chrome/browser/history/shortcuts_database.cc (right): https://codereview.chromium.org/134853004/diff/130001/chrome/browser/history/... chrome/browser/history/shortcuts_database.cc:22: // or database without *too* many bad effects. Nit: or -> our https://codereview.chromium.org/134853004/diff/130001/chrome/browser/history/... chrome/browser/history/shortcuts_database.cc:187: transaction.Begin(); Change this block to something like: if (!transaction.Begin() || !db_.Execute() ...) return false; This way, we'll go ahead and create the meta table correctly for such databases. This will also check the return value of Begin(), which we erroneously weren't doing before. https://codereview.chromium.org/134853004/diff/130001/chrome/browser/history/... chrome/browser/history/shortcuts_database.cc:209: transaction.Begin(); Nit: Include this call in the return statement You may want to also structure this like a "if (!...) return false;" that falls through to the subsequent code (which is today just a "return true") in order to be less likely to break if we add more migration steps in the future. https://codereview.chromium.org/134853004/diff/130001/chrome/browser/history/... File chrome/browser/history/shortcuts_database_unittest.cc (right): https://codereview.chromium.org/134853004/diff/130001/chrome/browser/history/... chrome/browser/history/shortcuts_database_unittest.cc:302: TEST(ShortcutsDatabaseMigrationTest, MigrateV2ToV3) { Nit: Hmm, I see we're using "v2" and "v3" here because of how we used version numbers in the previous test, but it's really confusing that they don't match the meta table numbers. We should probably rename the above test something like "MigrateTableAddFillIntoEdit" and rename its .sql file to use "no_fill_into_edit" in place of "v1"; then we can call this test "MigrateV0ToV1" and name its .sql file similarly. The other option would be to start the meta table with version 3, but that would also be confusing since there never would have been a version 1 or 2 and we wouldn't be able to look for those versions to check for databases that needed the older migration step. https://codereview.chromium.org/134853004/diff/130001/chrome/browser/history/... chrome/browser/history/shortcuts_database_unittest.cc:319: // Check the values in each of the new columns. Nit: Maybe "Check that all the old type values got converted to new values"? https://codereview.chromium.org/134853004/diff/130001/chrome/browser/history/... chrome/browser/history/shortcuts_database_unittest.cc:325: while (statement.Step()) { Nit: {} not necessary https://codereview.chromium.org/134853004/diff/130001/chrome/test/data/Histor... File chrome/test/data/History/Shortcuts.v2.sql (right): https://codereview.chromium.org/134853004/diff/130001/chrome/test/data/Histor... chrome/test/data/History/Shortcuts.v2.sql:9: -- BOOKMARK_TITLE (type = 12) Do we need to test migrations for types 10 and 11?
>>> FYI, I plan to do a once-over on this changelist too, after Peter's happy with it. I doubt I'll have comments, but I would like the opportunity to say something before you submit it. At the moment, it generally looks okay to me, though, like Peter, I'd like to avoid ugly URL matching if possible. >>> I did a once-over for this change, and it looks fine to me. No need to loop me back into this change after you finish resolving Peter's last few comments. thanks, mark
Sending it to CQ https://codereview.chromium.org/134853004/diff/130001/chrome/browser/history/... File chrome/browser/history/shortcuts_database.cc (right): https://codereview.chromium.org/134853004/diff/130001/chrome/browser/history/... chrome/browser/history/shortcuts_database.cc:22: // or database without *too* many bad effects. On 2014/02/12 00:27:50, Peter Kasting wrote: > Nit: or -> our Done. https://codereview.chromium.org/134853004/diff/130001/chrome/browser/history/... chrome/browser/history/shortcuts_database.cc:187: transaction.Begin(); On 2014/02/12 00:27:50, Peter Kasting wrote: > Change this block to something like: > > if (!transaction.Begin() || !db_.Execute() ...) > return false; > > This way, we'll go ahead and create the meta table correctly for such databases. > > This will also check the return value of Begin(), which we erroneously weren't > doing before. Done. I will let compiler deal with Mr. De Morgan. https://codereview.chromium.org/134853004/diff/130001/chrome/browser/history/... chrome/browser/history/shortcuts_database.cc:209: transaction.Begin(); On 2014/02/12 00:27:50, Peter Kasting wrote: > Nit: Include this call in the return statement > > You may want to also structure this like a "if (!...) return false;" that falls > through to the subsequent code (which is today just a "return true") in order to > be less likely to break if we add more migration steps in the future. Done. https://codereview.chromium.org/134853004/diff/130001/chrome/browser/history/... File chrome/browser/history/shortcuts_database_unittest.cc (right): https://codereview.chromium.org/134853004/diff/130001/chrome/browser/history/... chrome/browser/history/shortcuts_database_unittest.cc:302: TEST(ShortcutsDatabaseMigrationTest, MigrateV2ToV3) { On 2014/02/12 00:27:50, Peter Kasting wrote: > Nit: Hmm, I see we're using "v2" and "v3" here because of how we used version > numbers in the previous test, but it's really confusing that they don't match > the meta table numbers. > > We should probably rename the above test something like > "MigrateTableAddFillIntoEdit" and rename its .sql file to use > "no_fill_into_edit" in place of "v1"; then we can call this test "MigrateV0ToV1" > and name its .sql file similarly. > > The other option would be to start the meta table with version 3, but that would > also be confusing since there never would have been a version 1 or 2 and we > wouldn't be able to look for those versions to check for databases that needed > the older migration step. I think we are caught in a no good-names situation. Made the changes you suggested. https://codereview.chromium.org/134853004/diff/130001/chrome/browser/history/... chrome/browser/history/shortcuts_database_unittest.cc:319: // Check the values in each of the new columns. On 2014/02/12 00:27:50, Peter Kasting wrote: > Nit: Maybe "Check that all the old type values got converted to new values"? Done. https://codereview.chromium.org/134853004/diff/130001/chrome/browser/history/... chrome/browser/history/shortcuts_database_unittest.cc:325: while (statement.Step()) { On 2014/02/12 00:27:50, Peter Kasting wrote: > Nit: {} not necessary Done. https://codereview.chromium.org/134853004/diff/130001/chrome/test/data/Histor... File chrome/test/data/History/Shortcuts.v2.sql (right): https://codereview.chromium.org/134853004/diff/130001/chrome/test/data/Histor... chrome/test/data/History/Shortcuts.v2.sql:9: -- BOOKMARK_TITLE (type = 12) On 2014/02/12 00:27:50, Peter Kasting wrote: > Do we need to test migrations for types 10 and 11? 10 and 11 are ExtensionApp and Contacts. I did not see those providers being registered anywhere, and could not trigger those suggestions. Moreover, the nature of change is such that we really don't need to test all types.
The CQ bit was checked by skanuj@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skanuj@chromium.org/134853004/220001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for chrome/test/data/History/Shortcuts.v1.sql:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file chrome/test/data/History/Shortcuts.v1.sql
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/test/data/History/Shortcuts.v1.sql.rej
Patch: chrome/test/data/History/Shortcuts.v1.sql
Index: chrome/test/data/History/Shortcuts.v1.sql
diff --git a/chrome/test/data/History/Shortcuts.v1.sql
b/chrome/test/data/History/Shortcuts.v1.sql
index
94f0366dd4d21dfdb2307b1eede7b6745c3d3482..71df745e81dc6553e23dd0ae9d8d75f95bfd94f3
100644
--- a/chrome/test/data/History/Shortcuts.v1.sql
+++ b/chrome/test/data/History/Shortcuts.v1.sql
@@ -1,9 +1,12 @@
-PRAGMA foreign_keys=OFF;
-BEGIN TRANSACTION;
-CREATE TABLE omni_box_shortcuts ( id VARCHAR PRIMARY KEY, text VARCHAR, url
VARCHAR, contents VARCHAR, contents_class VARCHAR, description VARCHAR,
description_class VARCHAR, last_access_time INTEGER, number_of_hits INTEGER);
-INSERT INTO "omni_box_shortcuts"
VALUES('34A7401D-0DC5-4A8F-A6B5-3FA4FF786C42','sha','http://shacknews.com/','shacknews.com','0,1','Video
Game News and Features - Video Game News, Videos, and File Downloads for PC and
Console Games at Shacknews.com','0,0',13024194201806179,3);
-INSERT INTO "omni_box_shortcuts"
VALUES('9EA31BB8-8528-4AE0-A6B1-FD06458DFC31','shacknews','http://shacknews.com/','shacknews.com','0,1','Video
Game News and Features - Video Game News, Videos, and File Downloads for PC and
Console Games at Shacknews.com','0,0',13024456716613368,4);
-INSERT INTO "omni_box_shortcuts"
VALUES('CD853DC4-7C4E-4E64-903D-A4BBCAA410F9','shacknews.com','http://shacknews.com/','shacknews.com','0,1','Video
Game News and Features - Video Game News, Videos, and File Downloads for PC and
Console Games at Shacknews.com','0,0',13024196901413541,1);
-INSERT INTO "omni_box_shortcuts"
VALUES('377314F7-E3AB-4264-8F3F-3404BE48DADB','echo
echo','chrome-extension://cedabbhfglmiikkmdgcpjdkocfcmbkee/?q=echo','Run Echo
command: echo','0,0','Echo','0,4',13025401559133998,2);
-INSERT INTO "omni_box_shortcuts"
VALUES('BCE200CA-01E9-4A2F-B5EC-2D561DDFBE41','echo','chrome-extension://cedabbhfglmiikkmdgcpjdkocfcmbkee/?q=frobber','Run
Echo command: frobber','0,0','Echo','0,4',13025413423801769,2);
-COMMIT;
+PRAGMA foreign_keys=OFF;
+BEGIN TRANSACTION;
+CREATE TABLE omni_box_shortcuts ( id VARCHAR PRIMARY KEY, text VARCHAR, url
VARCHAR, contents VARCHAR, contents_class VARCHAR, description VARCHAR,
description_class VARCHAR, last_access_time INTEGER, number_of_hits INTEGER,
fill_into_edit VARCHAR, transition INTEGER, type INTEGER, keyword VARCHAR);
+INSERT INTO "omni_box_shortcuts"
VALUES('34A7401D-0DC5-4A8F-A6B5-3FA4FF786C42','sha','http://shacknews.com/','shacknews.com','0,1','Video
Game News and Features - Video Game News, Videos, and File Downloads for PC and
Console Games at
Shacknews.com','0,0',13024194201806179,3,'http://shacknews.com/',1,2,NULL);
+INSERT INTO "omni_box_shortcuts"
VALUES('9EA31BB8-8528-4AE0-A6B1-FD06458DFC31','shacknews','http://shacknews.com/','shacknews.com','0,1','Video
Game News and Features - Video Game News, Videos, and File Downloads for PC and
Console Games at
Shacknews.com','0,0',13024456716613368,4,'http://shacknews.com/',1,2,NULL);
+INSERT INTO "omni_box_shortcuts"
VALUES('CD853DC4-7C4E-4E64-903D-A4BBCAA410F9','shacknews.com','http://shacknews.com/','shacknews.com','0,1','Video
Game News and Features - Video Game News, Videos, and File Downloads for PC and
Console Games at
Shacknews.com','0,0',13024196901413541,1,'http://shacknews.com/',1,2,NULL);
+-- SEARCH_OTHER_ENGINE (type = 9)
+INSERT INTO "omni_box_shortcuts"
VALUES('B07F4744-F008-3C87-37E4-9D284AD9A697','yahoo.com other
keyword','yahoo.com other
keyword','http://search.yahoo.com/search?ei=UTF-8&fr=crmas&p=other+keyword','other
keyword','0,0','Yahoo! Search','0,4',9,9,'yahoo.com',13036572535614255,1);
+-- BOOKMARK_TITLE (type = 12)
+INSERT INTO "omni_box_shortcuts"
VALUES('9CBB669D-B7F0-5AFF-D808-EFFF7028B159','rdt','www.reddit.com/','http://www.reddit.com/','www.reddit.com/','0,1','rdt','0,0',1,12,'',13036574229966419,1);
+COMMIT;
+
The CQ bit was checked by skanuj@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skanuj@chromium.org/134853004/300001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for chrome/test/data/History/Shortcuts.v1.sql:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file chrome/test/data/History/Shortcuts.v1.sql
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/test/data/History/Shortcuts.v1.sql.rej
Patch: chrome/test/data/History/Shortcuts.v1.sql
Index: chrome/test/data/History/Shortcuts.v1.sql
diff --git a/chrome/test/data/History/Shortcuts.v1.sql
b/chrome/test/data/History/Shortcuts.v1.sql
index
94f0366dd4d21dfdb2307b1eede7b6745c3d3482..71df745e81dc6553e23dd0ae9d8d75f95bfd94f3
100644
--- a/chrome/test/data/History/Shortcuts.v1.sql
+++ b/chrome/test/data/History/Shortcuts.v1.sql
@@ -1,9 +1,12 @@
-PRAGMA foreign_keys=OFF;
-BEGIN TRANSACTION;
-CREATE TABLE omni_box_shortcuts ( id VARCHAR PRIMARY KEY, text VARCHAR, url
VARCHAR, contents VARCHAR, contents_class VARCHAR, description VARCHAR,
description_class VARCHAR, last_access_time INTEGER, number_of_hits INTEGER);
-INSERT INTO "omni_box_shortcuts"
VALUES('34A7401D-0DC5-4A8F-A6B5-3FA4FF786C42','sha','http://shacknews.com/','shacknews.com','0,1','Video
Game News and Features - Video Game News, Videos, and File Downloads for PC and
Console Games at Shacknews.com','0,0',13024194201806179,3);
-INSERT INTO "omni_box_shortcuts"
VALUES('9EA31BB8-8528-4AE0-A6B1-FD06458DFC31','shacknews','http://shacknews.com/','shacknews.com','0,1','Video
Game News and Features - Video Game News, Videos, and File Downloads for PC and
Console Games at Shacknews.com','0,0',13024456716613368,4);
-INSERT INTO "omni_box_shortcuts"
VALUES('CD853DC4-7C4E-4E64-903D-A4BBCAA410F9','shacknews.com','http://shacknews.com/','shacknews.com','0,1','Video
Game News and Features - Video Game News, Videos, and File Downloads for PC and
Console Games at Shacknews.com','0,0',13024196901413541,1);
-INSERT INTO "omni_box_shortcuts"
VALUES('377314F7-E3AB-4264-8F3F-3404BE48DADB','echo
echo','chrome-extension://cedabbhfglmiikkmdgcpjdkocfcmbkee/?q=echo','Run Echo
command: echo','0,0','Echo','0,4',13025401559133998,2);
-INSERT INTO "omni_box_shortcuts"
VALUES('BCE200CA-01E9-4A2F-B5EC-2D561DDFBE41','echo','chrome-extension://cedabbhfglmiikkmdgcpjdkocfcmbkee/?q=frobber','Run
Echo command: frobber','0,0','Echo','0,4',13025413423801769,2);
-COMMIT;
+PRAGMA foreign_keys=OFF;
+BEGIN TRANSACTION;
+CREATE TABLE omni_box_shortcuts ( id VARCHAR PRIMARY KEY, text VARCHAR, url
VARCHAR, contents VARCHAR, contents_class VARCHAR, description VARCHAR,
description_class VARCHAR, last_access_time INTEGER, number_of_hits INTEGER,
fill_into_edit VARCHAR, transition INTEGER, type INTEGER, keyword VARCHAR);
+INSERT INTO "omni_box_shortcuts"
VALUES('34A7401D-0DC5-4A8F-A6B5-3FA4FF786C42','sha','http://shacknews.com/','shacknews.com','0,1','Video
Game News and Features - Video Game News, Videos, and File Downloads for PC and
Console Games at
Shacknews.com','0,0',13024194201806179,3,'http://shacknews.com/',1,2,NULL);
+INSERT INTO "omni_box_shortcuts"
VALUES('9EA31BB8-8528-4AE0-A6B1-FD06458DFC31','shacknews','http://shacknews.com/','shacknews.com','0,1','Video
Game News and Features - Video Game News, Videos, and File Downloads for PC and
Console Games at
Shacknews.com','0,0',13024456716613368,4,'http://shacknews.com/',1,2,NULL);
+INSERT INTO "omni_box_shortcuts"
VALUES('CD853DC4-7C4E-4E64-903D-A4BBCAA410F9','shacknews.com','http://shacknews.com/','shacknews.com','0,1','Video
Game News and Features - Video Game News, Videos, and File Downloads for PC and
Console Games at
Shacknews.com','0,0',13024196901413541,1,'http://shacknews.com/',1,2,NULL);
+-- SEARCH_OTHER_ENGINE (type = 9)
+INSERT INTO "omni_box_shortcuts"
VALUES('B07F4744-F008-3C87-37E4-9D284AD9A697','yahoo.com other
keyword','yahoo.com other
keyword','http://search.yahoo.com/search?ei=UTF-8&fr=crmas&p=other+keyword','other
keyword','0,0','Yahoo! Search','0,4',9,9,'yahoo.com',13036572535614255,1);
+-- BOOKMARK_TITLE (type = 12)
+INSERT INTO "omni_box_shortcuts"
VALUES('9CBB669D-B7F0-5AFF-D808-EFFF7028B159','rdt','www.reddit.com/','http://www.reddit.com/','www.reddit.com/','0,1','rdt','0,0',1,12,'',13036574229966419,1);
+COMMIT;
+
For some reason, git patch keeps failing. I think it is because git sees Shortcuts.v1.sql -> Shortcuts.v0.sql copy and Shortcuts.v1.sql edit. Since original v1.sql also has types column the migration works fine against that too. So I am submitting this with MigrationV0ToV1 running against V(-1). I will follow up with a patch to to edit Shortcuts.V1.sql
The CQ bit was checked by skanuj@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skanuj@chromium.org/134853004/440001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on mac_rel for step(s) app_list_unittests, cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git, chromedriver_unittests, components_unittests, content_browsertests, content_unittests, crypto_unittests, google_apis_unittests, gpu_unittests, ipc_tests, jingle_unittests, media_unittests, message_center_unittests, nacl_integration, ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests, sync_unit_tests, telemetry_perf_unittests, telemetry_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
The CQ bit was checked by skanuj@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skanuj@chromium.org/134853004/440001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skanuj@chromium.org/134853004/440001
The CQ bit was unchecked by commit-bot@chromium.org
The commit queue went berserk retrying too often for a seemingly flaky test on builder android_dbg: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
The CQ bit was checked by skanuj@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skanuj@chromium.org/134853004/440001
The CQ bit was unchecked by commit-bot@chromium.org
The commit queue went berserk retrying too often for a seemingly flaky test on builder android_dbg: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
The CQ bit was checked by skanuj@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skanuj@chromium.org/134853004/440001
Message was sent while issue was closed.
Change committed as 251442
Message was sent while issue was closed.
On 2014/02/14 23:57:24, I haz the power (commit-bot) wrote: > Change committed as 251442 http://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%...
Message was sent while issue was closed.
On 2014/02/15 01:17:06, hubbe wrote: > On 2014/02/14 23:57:24, I haz the power (commit-bot) wrote: > > Change committed as 251442 > > http://build.chromium.org/p/chromium.linux/builders/Android%2520Tests%2520%25... This test used to be named TEST(ShortcutsDatabaseMigrationTest, MigrateV1ToV2) Is there some blacklist of the tests to be not run on Android? Because I don't think that step should fail, except for a bad merge/sync on the client. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
