Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(201)

Issue 2339693002: [Offline pages] Splitting Add and Update methods in OPMStore (Closed)

Created:
4 years, 3 months ago by fgorski
Modified:
4 years, 3 months ago
Reviewers:
Dmitry Titov, dewittj
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] Splitting Add and Update methods in OPMStore In OfflinePageMetadataStore the following changes: * AddOfflinePage (for single item) is added * UpdateOfflinePages (for multiple items) is added * AddOrUpdateOfflinePage is removed. Resulting changes: * OfflinePageMetadataStoreSQL and OfflinePageTestStore are updated accordingly. * Tests are updated. * OfflinePageModelImpl usage of above functions is updated. BUG=645522 Committed: https://crrev.com/9305bb6b94a0bf9ed2823037bd2c3314a793ecae Cr-Commit-Position: refs/heads/master@{#418587}

Patch Set 1 #

Total comments: 4

Patch Set 2 : Addressing feedback #

Unified diffs Side-by-side diffs Delta from patch set Stats (+162 lines, -68 lines) Patch
M components/offline_pages/offline_page_metadata_store.h View 2 chunks +16 lines, -4 lines 0 comments Download
M components/offline_pages/offline_page_metadata_store_impl_unittest.cc View 11 chunks +38 lines, -26 lines 0 comments Download
M components/offline_pages/offline_page_metadata_store_sql.h View 1 1 chunk +4 lines, -2 lines 0 comments Download
M components/offline_pages/offline_page_metadata_store_sql.cc View 1 2 chunks +44 lines, -9 lines 0 comments Download
M components/offline_pages/offline_page_model_impl.h View 2 chunks +2 lines, -2 lines 0 comments Download
M components/offline_pages/offline_page_model_impl.cc View 1 7 chunks +29 lines, -18 lines 0 comments Download
M components/offline_pages/offline_page_test_store.h View 1 chunk +4 lines, -2 lines 0 comments Download
M components/offline_pages/offline_page_test_store.cc View 1 chunk +25 lines, -5 lines 0 comments Download

Messages

Total messages: 18 (10 generated)
fgorski
PTAL. There are implementation details I want to change about Add and of course signature ...
4 years, 3 months ago (2016-09-13 21:50:26 UTC) #4
dewittj
this seems like a fine patch. I think it lgtm, though I wouldn't mind seeing ...
4 years, 3 months ago (2016-09-14 00:24:12 UTC) #7
fgorski
Addressed https://codereview.chromium.org/2339693002/diff/1/components/offline_pages/offline_page_model_impl.cc File components/offline_pages/offline_page_model_impl.cc (right): https://codereview.chromium.org/2339693002/diff/1/components/offline_pages/offline_page_model_impl.cc#newcode335 components/offline_pages/offline_page_model_impl.cc:335: std::vector<OfflinePageItem> items; On 2016/09/14 00:24:11, dewittj wrote: > ...
4 years, 3 months ago (2016-09-14 03:54:22 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2339693002/20001
4 years, 3 months ago (2016-09-14 03:55:06 UTC) #11
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/292678)
4 years, 3 months ago (2016-09-14 04:51:26 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2339693002/20001
4 years, 3 months ago (2016-09-14 16:03:57 UTC) #15
commit-bot: I haz the power
Committed patchset #2 (id:20001)
4 years, 3 months ago (2016-09-14 16:44:38 UTC) #16
commit-bot: I haz the power
4 years, 3 months ago (2016-09-14 16:46:16 UTC) #18
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/9305bb6b94a0bf9ed2823037bd2c3314a793ecae
Cr-Commit-Position: refs/heads/master@{#418587}

Powered by Google App Engine
This is Rietveld 408576698