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

Issue 2384423003: [Offline pages] Resetting offline page metadata store if initial load fails (Closed)

Created:
4 years, 2 months ago by fgorski
Modified:
4 years, 1 month ago
CC:
asvitkine+watch_chromium.org, chili+watch_chromium.org, chromium-reviews, dewittj+watch_chromium.org, dimich+watch_chromium.org, fgorski+watch_chromium.org, petewil+watch_chromium.org, romax+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

This issue is obsolete. https://codereview.chromium.org/2497703002 solved the reset problem. [Offline pages] Resetting offline page metadata store if initial load fails * Updates the reset functionality of OPMStoreSQL * Updates tests * Makes OPM attempt a reset if the initial read fails * Adds relevant tests for reset. BUG=645522

Patch Set 1 #

Total comments: 9

Patch Set 2 : Rebased and comments addressed #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+205 lines, -117 lines) Patch
M components/offline_pages/background/request_queue_store_sql.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M components/offline_pages/offline_page_metadata_store.h View 2 chunks +3 lines, -15 lines 0 comments Download
M components/offline_pages/offline_page_metadata_store_impl_unittest.cc View 4 chunks +3 lines, -14 lines 0 comments Download
M components/offline_pages/offline_page_metadata_store_sql.h View 2 chunks +2 lines, -4 lines 0 comments Download
M components/offline_pages/offline_page_metadata_store_sql.cc View 10 chunks +40 lines, -54 lines 0 comments Download
M components/offline_pages/offline_page_model_impl.h View 1 2 chunks +12 lines, -3 lines 0 comments Download
M components/offline_pages/offline_page_model_impl.cc View 1 4 chunks +57 lines, -10 lines 1 comment Download
M components/offline_pages/offline_page_model_impl_unittest.cc View 1 1 chunk +22 lines, -0 lines 0 comments Download
M components/offline_pages/offline_page_test_store.h View 3 chunks +6 lines, -1 line 0 comments Download
M components/offline_pages/offline_page_test_store.cc View 3 chunks +29 lines, -10 lines 0 comments Download
M components/offline_pages/offline_store_types.h View 1 1 chunk +11 lines, -5 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 3 chunks +19 lines, -0 lines 0 comments Download

Messages

Total messages: 26 (14 generated)
fgorski
PTAL.
4 years, 2 months ago (2016-10-05 17:09:58 UTC) #6
fgorski
Adding Justin who shown interest in the morning.
4 years, 2 months ago (2016-10-06 22:54:10 UTC) #9
fgorski
Adding jochen@ for histograms
4 years, 2 months ago (2016-10-06 22:58:07 UTC) #11
jochen (gone - plz use gerrit)
I can only review UseCounter additions to histograms.xml, sorry, please add a metrics OWNER instead
4 years, 2 months ago (2016-10-10 12:47:33 UTC) #12
Dmitry Titov
I'd like to chat (offline) about overall approach. I'd like to clarify some corner cases...
4 years, 2 months ago (2016-10-10 16:38:59 UTC) #13
fgorski
Replacing jochen@ with Mark. (Tools failed me.) Please review histograms.
4 years, 2 months ago (2016-10-10 21:03:38 UTC) #15
Mark P
I am away today and yesterday; I will review this tomorrow. -m
4 years, 2 months ago (2016-10-11 23:08:10 UTC) #16
Mark P
https://codereview.chromium.org/2384423003/diff/1/components/offline_pages/offline_page_model_impl.cc File components/offline_pages/offline_page_model_impl.cc (right): https://codereview.chromium.org/2384423003/diff/1/components/offline_pages/offline_page_model_impl.cc#newcode831 components/offline_pages/offline_page_model_impl.cc:831: UMA_HISTOGRAM_COUNTS("OfflinePages.SavedPageCount", I notice this moved from offline_page_metadata_store_sql.cc. Is it ...
4 years, 2 months ago (2016-10-12 16:04:13 UTC) #17
Mark P
https://codereview.chromium.org/2384423003/diff/1/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2384423003/diff/1/tools/metrics/histograms/histograms.xml#newcode38808 tools/metrics/histograms/histograms.xml:38808: + store. It feels a bit odd to me ...
4 years, 2 months ago (2016-10-12 16:06:31 UTC) #18
fgorski
OK. I was able to address one directly, we can discuss 2 vs 1 UMAs. ...
4 years, 2 months ago (2016-10-12 20:44:09 UTC) #21
Mark P
histograms.xml lgtm with one ongoing concern --mark https://codereview.chromium.org/2384423003/diff/1/components/offline_pages/offline_page_model_impl.cc File components/offline_pages/offline_page_model_impl.cc (right): https://codereview.chromium.org/2384423003/diff/1/components/offline_pages/offline_page_model_impl.cc#newcode831 components/offline_pages/offline_page_model_impl.cc:831: UMA_HISTOGRAM_COUNTS("OfflinePages.SavedPageCount", On ...
4 years, 2 months ago (2016-10-12 22:12:15 UTC) #24
Dmitry Titov
4 years, 1 month ago (2016-11-15 20:23:46 UTC) #25
https://codereview.chromium.org/2384423003/diff/20001/components/offline_page...
File components/offline_pages/offline_page_model_impl.cc (right):

https://codereview.chromium.org/2384423003/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_impl.cc:848:
"OfflinePages.StoreState", static_cast<int32_t>(store_->state()),
There is also another point of logging same UMA in OnGetOfflinePagesDoneForInit,
which will double-count successes I think. Probably should only log here if
(!reset_success)? Or do you want a separate UMA for successes of the reset?

Powered by Google App Engine
This is Rietveld 408576698