Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2338483002:
[Offline pages] Refactoring CreateTable method in OPMStoreSQL (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] Refactoring CreateTable method in OPMStoreSQL
BUG=645522
Committed: https://crrev.com/9b05a65dac0ca761608d8a47fcefe445044c3251
Cr-Commit-Position: refs/heads/master@{#418160}
Patch Set 1 #
Total comments: 6
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 16 (8 generated)
fgorski@chromium.org changed reviewers: + dimich@chromium.org
PTAL. Updates to the create table are in line with the comments from shess@ on the previous reviews.
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...
LGTM https://codereview.chromium.org/2338483002/diff/1/components/offline_pages/of... File components/offline_pages/offline_page_metadata_store_sql.cc (right): https://codereview.chromium.org/2338483002/diff/1/components/offline_pages/of... components/offline_pages/offline_page_metadata_store_sql.cc:36: " version INTEGER NOT NULL," Looks like 'version' should also be "Remove. Never used." https://codereview.chromium.org/2338483002/diff/1/components/offline_pages/of... components/offline_pages/offline_page_metadata_store_sql.cc:45: " user_initiated INTEGER," // this is actually a boolean Same here.
Addressed (by next patch) https://codereview.chromium.org/2338483002/diff/1/components/offline_pages/of... File components/offline_pages/offline_page_metadata_store_sql.cc (right): https://codereview.chromium.org/2338483002/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:14:31, Dmitry Titov wrote: > Looks like 'version' should also be "Remove. Never used." Acknowledged. As discussed, the upgrade patch will handle that. https://codereview.chromium.org/2338483002/diff/1/components/offline_pages/of... components/offline_pages/offline_page_metadata_store_sql.cc:45: " user_initiated INTEGER," // this is actually a boolean On 2016/09/12 23:14:31, Dmitry Titov wrote: > Same here. Acknowledged. As discussed, already handled by upgrade patch.
The CQ bit was checked by fgorski@chromium.org
The CQ bit was unchecked by fgorski@chromium.org
dewittj@chromium.org changed reviewers: + dewittj@chromium.org
drive-by! https://codereview.chromium.org/2338483002/diff/1/components/offline_pages/of... File components/offline_pages/offline_page_metadata_store_sql.cc (right): https://codereview.chromium.org/2338483002/diff/1/components/offline_pages/of... components/offline_pages/offline_page_metadata_store_sql.cc:41: // A note on this field: It will be NULL for now and is Please update this comment to refer to the version of Chrome that started using this value as a boolean.
Response to Justin's comment. https://codereview.chromium.org/2338483002/diff/1/components/offline_pages/of... File components/offline_pages/offline_page_metadata_store_sql.cc (right): https://codereview.chromium.org/2338483002/diff/1/components/offline_pages/of... components/offline_pages/offline_page_metadata_store_sql.cc:41: // A note on this field: It will be NULL for now and is On 2016/09/12 23:25:38, dewittj wrote: > Please update this comment to refer to the version of Chrome that started using > this value as a boolean. The field is being dropped, as it was never used.
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.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== [Offline pages] Refactoring CreateTable method in OPMStoreSQL BUG=645522 ========== to ========== [Offline pages] Refactoring CreateTable method in OPMStoreSQL BUG=645522 Committed: https://crrev.com/9b05a65dac0ca761608d8a47fcefe445044c3251 Cr-Commit-Position: refs/heads/master@{#418160} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/9b05a65dac0ca761608d8a47fcefe445044c3251 Cr-Commit-Position: refs/heads/master@{#418160} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
