Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(28)

Issue 10696127: Force WebDatabase version to max(version, compatible version) before migration. (Closed)

Created:
6 years, 9 months ago by Ivan Korotkov
Modified:
6 years, 9 months ago
Reviewers:
Peter Kasting, sky, dhollowa
CC:
chromium-reviews
Visibility:
Public.

Description

Force WebDatabase version to max(version, compatible version) before migration. BUG=135342 TEST=WebDatabaseMigrationTest.* Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=145665

Patch Set 1 #

Total comments: 2

Patch Set 2 : Style fix #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+47 lines, -5 lines) Patch
M chrome/browser/webdata/web_database.cc View 1 1 chunk +8 lines, -1 line 3 comments Download
M chrome/browser/webdata/web_database_migration_unittest.cc View 1 chunk +38 lines, -0 lines 0 comments Download
A + chrome/test/data/web_database/version_45_compatible.sql View 2 chunks +1 line, -4 lines 0 comments Download

Messages

Total messages: 8 (0 generated)
Ivan Korotkov
Hi Peter and Scott, I guess the easiest way around the AVG hack is to ...
6 years, 9 months ago (2012-07-06 16:05:36 UTC) #1
Peter Kasting
LGTM as far as this goes, but I think we should also make changes to ...
6 years, 9 months ago (2012-07-06 17:05:12 UTC) #2
Ivan Korotkov
http://codereview.chromium.org/10696127/diff/1/chrome/browser/webdata/web_database.cc File chrome/browser/webdata/web_database.cc (right): http://codereview.chromium.org/10696127/diff/1/chrome/browser/webdata/web_database.cc#newcode161 chrome/browser/webdata/web_database.cc:161: // Ensure that version is not smaller than the ...
6 years, 9 months ago (2012-07-09 10:37:38 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ivankr@chromium.org/10696127/1004
6 years, 9 months ago (2012-07-09 10:39:16 UTC) #4
commit-bot: I haz the power
Change committed as 145665
6 years, 9 months ago (2012-07-09 14:36:35 UTC) #5
dhollowa
https://chromiumcodereview.appspot.com/10696127/diff/1004/chrome/browser/webdata/web_database.cc File chrome/browser/webdata/web_database.cc (right): https://chromiumcodereview.appspot.com/10696127/diff/1004/chrome/browser/webdata/web_database.cc#newcode166 chrome/browser/webdata/web_database.cc:166: if (current_version < meta_table_.GetCompatibleVersionNumber()) Shouldn't this be: if (meta_table_.GetVersionNumber() ...
6 years, 9 months ago (2012-07-09 20:26:06 UTC) #6
Ivan Korotkov
https://chromiumcodereview.appspot.com/10696127/diff/1004/chrome/browser/webdata/web_database.cc File chrome/browser/webdata/web_database.cc (right): https://chromiumcodereview.appspot.com/10696127/diff/1004/chrome/browser/webdata/web_database.cc#newcode166 chrome/browser/webdata/web_database.cc:166: if (current_version < meta_table_.GetCompatibleVersionNumber()) On 2012/07/09 20:26:06, dhollowa wrote: ...
6 years, 9 months ago (2012-07-09 20:51:04 UTC) #7
Peter Kasting
6 years, 9 months ago (2012-07-09 21:22:01 UTC) #8
https://chromiumcodereview.appspot.com/10696127/diff/1004/chrome/browser/webd...
File chrome/browser/webdata/web_database.cc (right):

https://chromiumcodereview.appspot.com/10696127/diff/1004/chrome/browser/webd...
chrome/browser/webdata/web_database.cc:166: if (current_version <
meta_table_.GetCompatibleVersionNumber())
On 2012/07/09 20:51:04, Ivan Korotkov wrote:
> On 2012/07/09 20:26:06, dhollowa wrote:
> > Shouldn't this be:
> >   if (meta_table_.GetVersionNumber() <
> meta_table_.GetCompatibleVersionNumber())
> > ...
> 
> Ouch, you must be right. The test passed because there was a migration to
verson
> 46 which still incremented the version field.

I'm sorry for suggesting this incorrect code to Ivan in the first place!

Ivan, you have LGTM in advance from me to fix/land/merge this.  (Feel free to
TBR.)

Powered by Google App Engine
This is Rietveld 408576698