Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2336813002:
[Offline pages] OPMStoreSQL: Upgrade to M55, Upgrade refactoring, conversion refactoring (Closed)
|
Created:
4 years, 3 months ago by fgorski Modified:
4 years, 3 months ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, dimich+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Offline pages] OPMStoreSQL: Upgrade to M55, Upgrade refactoring, conversion refactoring
Removes the following fields:
* offline URL
* version
* status
* user initiated
Updates the Upgrade routine; updates conversions between DB row and offline page item.
BUG=645522
Committed: https://crrev.com/430911d0e02f663a5df76e470cd8e0ca4b3852f2
Cr-Commit-Position: refs/heads/master@{#418397}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Removing the version field #
Total comments: 10
Patch Set 3 : Addressing CR feedback #
Total comments: 4
Messages
Total messages: 27 (13 generated)
PTAL
fgorski@chromium.org changed reviewers: + dimich@chromium.org
PTAL
https://codereview.chromium.org/2336813002/diff/1/components/offline_pages/of... File components/offline_pages/offline_page_metadata_store_sql.cc (right): https://codereview.chromium.org/2336813002/diff/1/components/offline_pages/of... components/offline_pages/offline_page_metadata_store_sql.cc:36: " version INTEGER NOT NULL," could you add this to the kill list as well? Since you are removing others as well.
The CQ bit was checked by fgorski@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Offline pages] OPMStoreSQL: Upgrade to M55, Upgrade refactoring, conversion refactoring BUG=645522 ========== to ========== [Offline pages] OPMStoreSQL: Upgrade to M55, Upgrade refactoring, conversion refactoring Removes the following fields: * offline URL * version * status * user initiated Updates the Upgrade routine; updates conversions between DB row and offline page item. BUG=645522 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
fgorski@chromium.org changed reviewers: + dewittj@chromium.org
Addressed. https://codereview.chromium.org/2336813002/diff/1/components/offline_pages/of... File components/offline_pages/offline_page_metadata_store_sql.cc (right): https://codereview.chromium.org/2336813002/diff/1/components/offline_pages/of... components/offline_pages/offline_page_metadata_store_sql.cc:36: " version INTEGER NOT NULL," On 2016/09/12 23:25:41, Dmitry Titov wrote: > could you add this to the kill list as well? Since you are removing others as > well. Done.
looks good, just a few comments https://codereview.chromium.org/2336813002/diff/20001/components/offline_page... File components/offline_pages/offline_page_metadata_store_impl_unittest.cc (right): https://codereview.chromium.org/2336813002/diff/20001/components/offline_page... components/offline_pages/offline_page_metadata_store_impl_unittest.cc:424: // TODO(romax): Move this to sql_unittest. I'm sure he will be happy about more todos :) https://codereview.chromium.org/2336813002/diff/20001/components/offline_page... File components/offline_pages/offline_page_metadata_store_sql.cc (right): https://codereview.chromium.org/2336813002/diff/20001/components/offline_page... components/offline_pages/offline_page_metadata_store_sql.cc:105: if (!db->Execute( Now that there are 3 copies of this function, it seems like it's time to refactor this a little to reduce duplication. https://codereview.chromium.org/2336813002/diff/20001/components/offline_page... components/offline_pages/offline_page_metadata_store_sql.cc:137: // and add appropriate method to cover changes from M55. I would like to see somewhere a log of the changes in each version, so that the upgrade checks are somewhat legible. If I am reading the code right: * Initial version lasted through M52 * 53 we added expiration_time * 54 we added title * 55 we removed version, status, user_initiated, offline_url. https://codereview.chromium.org/2336813002/diff/20001/components/offline_page... components/offline_pages/offline_page_metadata_store_sql.cc:161: base::FilePath GetPathFromString(const std::string& path_string) { Maybe name this GetPathFromUTF8String (and similar below), this makes it obvious what the point is.
https://codereview.chromium.org/2336813002/diff/20001/components/offline_page... File components/offline_pages/offline_page_metadata_store_impl_unittest.cc (right): https://codereview.chromium.org/2336813002/diff/20001/components/offline_page... components/offline_pages/offline_page_metadata_store_impl_unittest.cc:424: // TODO(romax): Move this to sql_unittest. On 2016/09/13 18:10:07, dewittj wrote: > I'm sure he will be happy about more todos :) :S https://codereview.chromium.org/2336813002/diff/20001/components/offline_page... File components/offline_pages/offline_page_metadata_store_sql.cc (right): https://codereview.chromium.org/2336813002/diff/20001/components/offline_page... components/offline_pages/offline_page_metadata_store_sql.cc:137: // and add appropriate method to cover changes from M55. On 2016/09/13 18:10:08, dewittj wrote: > I would like to see somewhere a log of the changes in each version, so that the > upgrade checks are somewhat legible. If I am reading the code right: > * Initial version lasted through M52 > * 53 we added expiration_time > * 54 we added title > * 55 we removed version, status, user_initiated, offline_url. I wish these comments can be with those upgrade functions. And originally I was thinking maybe it's better to keep track of the table version with the table so that we don't have to check the schema to match with a version. Also I suggest to use upgrade from X to X+1 and do that for M53ToM54, M54ToM55 while using a method creating M53 schema as a base. Otherwise it seems to me everytime we update the schema, we have to change (# of versions - 1) methods while updating the code?
On 2016/09/13 18:32:14, romax wrote: > https://codereview.chromium.org/2336813002/diff/20001/components/offline_page... > File components/offline_pages/offline_page_metadata_store_impl_unittest.cc > (right): > > https://codereview.chromium.org/2336813002/diff/20001/components/offline_page... > components/offline_pages/offline_page_metadata_store_impl_unittest.cc:424: // > TODO(romax): Move this to sql_unittest. > On 2016/09/13 18:10:07, dewittj wrote: > > I'm sure he will be happy about more todos :) > > :S > > https://codereview.chromium.org/2336813002/diff/20001/components/offline_page... > File components/offline_pages/offline_page_metadata_store_sql.cc (right): > > https://codereview.chromium.org/2336813002/diff/20001/components/offline_page... > components/offline_pages/offline_page_metadata_store_sql.cc:137: // and add > appropriate method to cover changes from M55. > On 2016/09/13 18:10:08, dewittj wrote: > > I would like to see somewhere a log of the changes in each version, so that > the > > upgrade checks are somewhat legible. If I am reading the code right: > > * Initial version lasted through M52 > > * 53 we added expiration_time > > * 54 we added title > > * 55 we removed version, status, user_initiated, offline_url. > > I wish these comments can be with those upgrade functions. And originally I was > thinking maybe it's better to keep track of the table version with the table so > that we don't have to check the schema to match with a version. Also I suggest > to use upgrade from X to X+1 and do that for M53ToM54, M54ToM55 while using a > method creating M53 schema as a base. Otherwise it seems to me everytime we > update the schema, we have to change (# of versions - 1) methods while updating > the code? The flip side is that with N upgrade methods you will have to perform N different ALTER TABLE + INSERT INTO *. Have you ever gotten a new android phone that did the N, N+1, N+2... upgrade dance? So for now I prefer this method..
On 2016/09/13 18:34:42, dewittj wrote: > On 2016/09/13 18:32:14, romax wrote: > > > https://codereview.chromium.org/2336813002/diff/20001/components/offline_page... > > File components/offline_pages/offline_page_metadata_store_impl_unittest.cc > > (right): > > > > > https://codereview.chromium.org/2336813002/diff/20001/components/offline_page... > > components/offline_pages/offline_page_metadata_store_impl_unittest.cc:424: // > > TODO(romax): Move this to sql_unittest. > > On 2016/09/13 18:10:07, dewittj wrote: > > > I'm sure he will be happy about more todos :) > > > > :S > > > > > https://codereview.chromium.org/2336813002/diff/20001/components/offline_page... > > File components/offline_pages/offline_page_metadata_store_sql.cc (right): > > > > > https://codereview.chromium.org/2336813002/diff/20001/components/offline_page... > > components/offline_pages/offline_page_metadata_store_sql.cc:137: // and add > > appropriate method to cover changes from M55. > > On 2016/09/13 18:10:08, dewittj wrote: > > > I would like to see somewhere a log of the changes in each version, so that > > the > > > upgrade checks are somewhat legible. If I am reading the code right: > > > * Initial version lasted through M52 > > > * 53 we added expiration_time > > > * 54 we added title > > > * 55 we removed version, status, user_initiated, offline_url. > > > > I wish these comments can be with those upgrade functions. And originally I > was > > thinking maybe it's better to keep track of the table version with the table > so > > that we don't have to check the schema to match with a version. Also I suggest > > to use upgrade from X to X+1 and do that for M53ToM54, M54ToM55 while using a > > method creating M53 schema as a base. Otherwise it seems to me everytime we > > update the schema, we have to change (# of versions - 1) methods while > updating > > the code? > > The flip side is that with N upgrade methods you will have to perform N > different ALTER TABLE + INSERT INTO *. Have you ever gotten a new android phone > that did the N, N+1, N+2... upgrade dance? So for now I prefer this method.. haven't thought about the other side... thanks for explanation
The CQ bit was checked by fgorski@chromium.org to run a CQ dry run
Addressed. https://codereview.chromium.org/2336813002/diff/20001/components/offline_page... File components/offline_pages/offline_page_metadata_store_impl_unittest.cc (right): https://codereview.chromium.org/2336813002/diff/20001/components/offline_page... components/offline_pages/offline_page_metadata_store_impl_unittest.cc:424: // TODO(romax): Move this to sql_unittest. On 2016/09/13 18:32:14, romax wrote: > On 2016/09/13 18:10:07, dewittj wrote: > > I'm sure he will be happy about more todos :) > > :S Acknowledged. https://codereview.chromium.org/2336813002/diff/20001/components/offline_page... File components/offline_pages/offline_page_metadata_store_sql.cc (right): https://codereview.chromium.org/2336813002/diff/20001/components/offline_page... components/offline_pages/offline_page_metadata_store_sql.cc:105: if (!db->Execute( On 2016/09/13 18:10:07, dewittj wrote: > Now that there are 3 copies of this function, it seems like it's time to > refactor this a little to reduce duplication. Done. https://codereview.chromium.org/2336813002/diff/20001/components/offline_page... components/offline_pages/offline_page_metadata_store_sql.cc:137: // and add appropriate method to cover changes from M55. On 2016/09/13 18:10:08, dewittj wrote: > I would like to see somewhere a log of the changes in each version, so that the > upgrade checks are somewhat legible. If I am reading the code right: > * Initial version lasted through M52 > * 53 we added expiration_time > * 54 we added title > * 55 we removed version, status, user_initiated, offline_url. Done. https://codereview.chromium.org/2336813002/diff/20001/components/offline_page... components/offline_pages/offline_page_metadata_store_sql.cc:161: base::FilePath GetPathFromString(const std::string& path_string) { On 2016/09/13 18:10:07, dewittj wrote: > Maybe name this GetPathFromUTF8String (and similar below), this makes it obvious > what the point is. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm, thanks! https://codereview.chromium.org/2336813002/diff/40001/components/offline_page... File components/offline_pages/offline_page_metadata_store_sql.cc (right): https://codereview.chromium.org/2336813002/diff/40001/components/offline_page... components/offline_pages/offline_page_metadata_store_sql.cc:278: void AddOrUpdateOfflinePageSync( nit: I'd avoid moving these functions just because, screws up blaming. (not a strong opinion so feel free to ignore) https://codereview.chromium.org/2336813002/diff/40001/components/offline_page... components/offline_pages/offline_page_metadata_store_sql.cc:283: // TODO(bburns): add UMA metrics here (and for levelDB). But if you do move the functions, it might be wise to retarget the TODO :)
Responded per offline chat. https://codereview.chromium.org/2336813002/diff/40001/components/offline_page... File components/offline_pages/offline_page_metadata_store_sql.cc (right): https://codereview.chromium.org/2336813002/diff/40001/components/offline_page... components/offline_pages/offline_page_metadata_store_sql.cc:278: void AddOrUpdateOfflinePageSync( On 2016/09/13 21:43:51, dewittj wrote: > nit: I'd avoid moving these functions just because, screws up blaming. (not a > strong opinion so feel free to ignore) Move had to happen to clean up the interface and blaming bburns@ for anything wouldn't work much anyway ;) https://codereview.chromium.org/2336813002/diff/40001/components/offline_page... components/offline_pages/offline_page_metadata_store_sql.cc:283: // TODO(bburns): add UMA metrics here (and for levelDB). On 2016/09/13 21:43:51, dewittj wrote: > But if you do move the functions, it might be wise to retarget the TODO :) I'll get to that later.
The CQ bit was unchecked by fgorski@chromium.org
The CQ bit was checked by fgorski@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Offline pages] OPMStoreSQL: Upgrade to M55, Upgrade refactoring, conversion refactoring Removes the following fields: * offline URL * version * status * user initiated Updates the Upgrade routine; updates conversions between DB row and offline page item. BUG=645522 ========== to ========== [Offline pages] OPMStoreSQL: Upgrade to M55, Upgrade refactoring, conversion refactoring Removes the following fields: * offline URL * version * status * user initiated Updates the Upgrade routine; updates conversions between DB row and offline page item. BUG=645522 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [Offline pages] OPMStoreSQL: Upgrade to M55, Upgrade refactoring, conversion refactoring Removes the following fields: * offline URL * version * status * user initiated Updates the Upgrade routine; updates conversions between DB row and offline page item. BUG=645522 ========== to ========== [Offline pages] OPMStoreSQL: Upgrade to M55, Upgrade refactoring, conversion refactoring Removes the following fields: * offline URL * version * status * user initiated Updates the Upgrade routine; updates conversions between DB row and offline page item. BUG=645522 Committed: https://crrev.com/430911d0e02f663a5df76e470cd8e0ca4b3852f2 Cr-Commit-Position: refs/heads/master@{#418397} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/430911d0e02f663a5df76e470cd8e0ca4b3852f2 Cr-Commit-Position: refs/heads/master@{#418397} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
