Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2388303007:
[Offline Pages] Adds UMA for result of attempts to immediately start background loading. (Closed)
|
Created:
4 years, 2 months ago by dougarnett Modified:
4 years, 2 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, asvitkine+watch_chromium.org, dimich+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Offline Pages] Adds UMA for result of attempts to immediately start background loading.
Some cases are benign - no network connection or already busy - but we want to be able
to separate out cases in M55 due to running on svelte device or if prerenderer did not accept
the request (eg, due to RECENTLY_VISITED check).
BUG=652486
Committed: https://crrev.com/e82ebcb155fee93de819feb7a4d1f9342befe0a5
Cr-Commit-Position: refs/heads/master@{#423770}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Swapped order of two enum vals per fgorski #Patch Set 3 : Splitting StartProcessIfConnected into two methods per petewil feedback #Patch Set 4 : Merge #
Total comments: 8
Patch Set 5 : Addressed mpearson feedback #
Total comments: 5
Patch Set 6 : Two comment improvements #
Messages
Total messages: 37 (20 generated)
The CQ bit was checked by dougarnett@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...
dougarnett@chromium.org changed reviewers: + fgorski@chromium.org, holte@google.com, petewil@chromium.org
Trying to get this in for M55 per talo@ request.
overall idea and code LGTM. I'd make a few tweaks thou... https://codereview.chromium.org/2388303007/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2388303007/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:29: BUSY, Can you reorder these two? Almost feels like it should be: 0 - I Succeeded 1-5.. Here is why I didn't WDYT? https://codereview.chromium.org/2388303007/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:42: static_cast<int>(immediate_start_status), holte@: should be using int vs. int32_t here? I have another patch where I have the same dilemma. https://codereview.chromium.org/2388303007/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:400: RecordImmediateStart(OfflinerImmediateStartStatus::NOT_STARTED_ON_SVELTE); Just a thought: If you make this check above, you are effectively counting svelte devices... https://codereview.chromium.org/2388303007/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:424: if (StartProcessing(device_conditions, base::Bind(&EmptySchedulerCallback))) { nit: braces not needed
mostly looks good, one refactoring suggestion. https://codereview.chromium.org/2388303007/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2388303007/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:391: void RequestCoordinator::StartProcessingIfConnected() { Would it be better to have StartProcessingIfConnected just return the immediate start status in a return value, and do the UMA from there? If we could, it would make the code easier to read. It might also make sense to do the same for StartProcessing.
On 2016/10/05 21:51:53, Pete Williamson wrote: > mostly looks good, one refactoring suggestion. > > https://codereview.chromium.org/2388303007/diff/1/components/offline_pages/ba... > File components/offline_pages/background/request_coordinator.cc (right): > > https://codereview.chromium.org/2388303007/diff/1/components/offline_pages/ba... > components/offline_pages/background/request_coordinator.cc:391: void > RequestCoordinator::StartProcessingIfConnected() { > Would it be better to have StartProcessingIfConnected just return the immediate > start status in a return value, and do the UMA from there? If we could, it > would make the code easier to read. > > It might also make sense to do the same for StartProcessing. Sounds OK, but you cannot count svelte devices, unless you never expect to get busy there.
https://codereview.chromium.org/2388303007/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2388303007/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:29: BUSY, On 2016/10/05 21:46:13, fgorski wrote: > Can you reorder these two? > > Almost feels like it should be: > 0 - I Succeeded > 1-5.. Here is why I didn't > > WDYT? Done. https://codereview.chromium.org/2388303007/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:42: static_cast<int>(immediate_start_status), On 2016/10/05 21:46:13, fgorski wrote: > holte@: should be using int vs. int32_t here? I have another patch where I have > the same dilemma. Pete just had CL that used int so followed that example. https://codereview.chromium.org/2388303007/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:391: void RequestCoordinator::StartProcessingIfConnected() { On 2016/10/05 21:51:53, Pete Williamson wrote: > Would it be better to have StartProcessingIfConnected just return the immediate > start status in a return value, and do the UMA from there? If we could, it > would make the code easier to read. > > It might also make sense to do the same for StartProcessing. It is called in two places - so capture UMA in both or just for Adds not Resumes? Or create a wrapping method? Improved later to get this in 55? https://codereview.chromium.org/2388303007/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:400: RecordImmediateStart(OfflinerImmediateStartStatus::NOT_STARTED_ON_SVELTE); On 2016/10/05 21:46:13, fgorski wrote: > Just a thought: If you make this check above, you are effectively counting > svelte devices... Happens anyway as far as unique devices goes on first Add (won't be busy).
https://codereview.chromium.org/2388303007/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2388303007/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:391: void RequestCoordinator::StartProcessingIfConnected() { On 2016/10/05 22:28:14, dougarnett wrote: > On 2016/10/05 21:51:53, Pete Williamson wrote: > > Would it be better to have StartProcessingIfConnected just return the > immediate > > start status in a return value, and do the UMA from there? If we could, it > > would make the code easier to read. > > > > It might also make sense to do the same for StartProcessing. > > It is called in two places - so capture UMA in both or just for Adds not > Resumes? Or create a wrapping method? Improved later to get this in 55? I'm happy if you want to improve this later in M55 instead of now, it's fine by me to create a TODO. I'm also open to any arguments if you think the code is better as is. I'd capture UMA for both, or creating a wrapping method is also viable.
The CQ bit was checked by dougarnett@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by dougarnett@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...
Pete, PTAL at wrapper change https://codereview.chromium.org/2388303007/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2388303007/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:391: void RequestCoordinator::StartProcessingIfConnected() { On 2016/10/05 22:45:03, Pete Williamson wrote: > On 2016/10/05 22:28:14, dougarnett wrote: > > On 2016/10/05 21:51:53, Pete Williamson wrote: > > > Would it be better to have StartProcessingIfConnected just return the > > immediate > > > start status in a return value, and do the UMA from there? If we could, it > > > would make the code easier to read. > > > > > > It might also make sense to do the same for StartProcessing. > > > > It is called in two places - so capture UMA in both or just for Adds not > > Resumes? Or create a wrapping method? Improved later to get this in 55? > > I'm happy if you want to improve this later in M55 instead of now, it's fine by > me to create a TODO. I'm also open to any arguments if you think the code is > better as is. I'd capture UMA for both, or creating a wrapping method is also > viable. I tried wrapper - see if you like it better.
much nicer, thanks! lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dougarnett@chromium.org changed reviewers: + mpearson@chromium.org
Mark, do you have some cycles to give histograms review here?
https://codereview.chromium.org/2388303007/diff/60001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2388303007/diff/60001/components/offline_page... components/offline_pages/background/request_coordinator.cc:377: static_cast<int>(immediate_start_status), Do you need static_casts here? I've never seen anyone else need them. https://codereview.chromium.org/2388303007/diff/60001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2388303007/diff/60001/components/offline_page... components/offline_pages/background/request_coordinator.h:169: enum class OfflinerImmediateStartStatus { Please follow the advice here about enum histograms: https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... https://codereview.chromium.org/2388303007/diff/60001/components/offline_page... components/offline_pages/background/request_coordinator.h:174: NO_EFFECTIVE_CONNECTION, What's the difference between this one and the one above? This probably deserved a comment here and in the histograms.xml enum. https://codereview.chromium.org/2388303007/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2388303007/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:38363: + Status of attempts to immediately starting background loading. Please clearly state when the histogram is recorded (started background loading may be technically correct but not useful to anyone who doesn't know the intimate details of the offline pages code).
The CQ bit was checked by dougarnett@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...
dougarnett@chromium.org changed reviewers: - holte@google.com
https://codereview.chromium.org/2388303007/diff/60001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2388303007/diff/60001/components/offline_page... components/offline_pages/background/request_coordinator.cc:377: static_cast<int>(immediate_start_status), On 2016/10/06 18:54:41, Mark P wrote: > Do you need static_casts here? I've never seen anyone else need them. Needed for scoped enums ("enum class"). Switched to unscoped enum instead. https://codereview.chromium.org/2388303007/diff/60001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2388303007/diff/60001/components/offline_page... components/offline_pages/background/request_coordinator.h:169: enum class OfflinerImmediateStartStatus { On 2016/10/06 18:54:41, Mark P wrote: > Please follow the advice here about enum histograms: > https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... switched to unscoped enum with explicit values https://codereview.chromium.org/2388303007/diff/60001/components/offline_page... components/offline_pages/background/request_coordinator.h:174: NO_EFFECTIVE_CONNECTION, On 2016/10/06 18:54:41, Mark P wrote: > What's the difference between this one and the one above? This probably > deserved a comment here and in the histograms.xml enum. Done. https://codereview.chromium.org/2388303007/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2388303007/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:38363: + Status of attempts to immediately starting background loading. On 2016/10/06 18:54:42, Mark P wrote: > Please clearly state when the histogram is recorded (started background loading > may be technically correct but not useful to anyone who doesn't know the > intimate details of the offline pages code). Please check new wording.
histograms.xml lgtm modulo comments below --mark https://codereview.chromium.org/2388303007/diff/80001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2388303007/diff/80001/components/offline_page... components/offline_pages/background/request_coordinator.h:172: enum OfflinerImmediateStartStatus { On 2016/10/06 23:05:00, dougarnett wrote: > On 2016/10/06 18:54:41, Mark P wrote: > > Please follow the advice here about enum histograms: > > > https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... > > switched to unscoped enum with explicit values Can you please add the warning at the top as well? https://codereview.chromium.org/2388303007/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2388303007/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:38413: + Status of attempts to immediately start offlining a page in the background nit: attempts -> attempt (each status is for a single attempt)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dougarnett@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org, petewil@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/2388303007/#ps100001 (title: "Two comment improvements")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2388303007/diff/80001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2388303007/diff/80001/components/offline_page... components/offline_pages/background/request_coordinator.h:172: enum OfflinerImmediateStartStatus { On 2016/10/06 23:11:36, Mark P wrote: > On 2016/10/06 23:05:00, dougarnett wrote: > > On 2016/10/06 18:54:41, Mark P wrote: > > > Please follow the advice here about enum histograms: > > > > > > https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... > > > > switched to unscoped enum with explicit values > > Can you please add the warning at the top as well? Done. https://codereview.chromium.org/2388303007/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2388303007/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:38413: + Status of attempts to immediately start offlining a page in the background On 2016/10/06 23:11:36, Mark P wrote: > nit: attempts -> attempt > (each status is for a single attempt) Done.
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Adds UMA for result of attempts to immediately start background loading. Some cases are benign - no network connection or already busy - but we want to be able to separate out cases in M55 due to running on svelte device or if prerenderer did not accept the request (eg, due to RECENTLY_VISITED check). BUG=652486 ========== to ========== [Offline Pages] Adds UMA for result of attempts to immediately start background loading. Some cases are benign - no network connection or already busy - but we want to be able to separate out cases in M55 due to running on svelte device or if prerenderer did not accept the request (eg, due to RECENTLY_VISITED check). BUG=652486 Committed: https://crrev.com/e82ebcb155fee93de819feb7a4d1f9342befe0a5 Cr-Commit-Position: refs/heads/master@{#423770} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/e82ebcb155fee93de819feb7a4d1f9342befe0a5 Cr-Commit-Position: refs/heads/master@{#423770}
Message was sent while issue was closed.
https://codereview.chromium.org/2388303007/diff/80001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2388303007/diff/80001/components/offline_page... components/offline_pages/background/request_coordinator.h:172: enum OfflinerImmediateStartStatus { On 2016/10/07 00:18:56, dougarnett wrote: > On 2016/10/06 23:11:36, Mark P wrote: > > On 2016/10/06 23:05:00, dougarnett wrote: > > > On 2016/10/06 18:54:41, Mark P wrote: > > > > Please follow the advice here about enum histograms: > > > > > > > > > > https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... > > > > > > switched to unscoped enum with explicit values > > > > Can you please add the warning at the top as well? > > Done. Next time you update this file, can you please update the warning, following the guideline here: https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... In particular, your warning misses the information that values shouldn't be changed or reused. You just state that they need to stay in sync with histograms.xml. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
