Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2105173002:
Add enum-based UMA reporting to automatic redirect logic of Offline Pages. (Closed)
|
Created:
4 years, 5 months ago by Dmitry Titov Modified:
4 years, 5 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 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd enum-based UMA reporting to automatic redirect logic of Offline Pages.
Removes 3 other counter-based UMA histograms that are now replaced with one.
BUG=622424
Committed: https://crrev.com/853787e334f8a353abbb44189c872b5a40f2330e
Cr-Commit-Position: refs/heads/master@{#403114}
Patch Set 1 #
Total comments: 29
Patch Set 2 : addressed feedback #
Total comments: 20
Patch Set 3 : addressed feedback #
Total comments: 21
Patch Set 4 : Mark's comments addressed #
Total comments: 4
Patch Set 5 : Jian's comments addressed #Patch Set 6 : Addressed last Mark's comments. #
Messages
Total messages: 29 (7 generated)
dimich@chromium.org changed reviewers: + dewittj@chromium.org, fgorski@chromium.org, jianli@chromium.org
PTAL dewittj: overall jianli: UMA-specific (I'm replacing 3 histograms!) fgorski: optional, in case I'm missing something big
https://codereview.chromium.org/2105173002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/offline_page_tab_helper.cc (right): https://codereview.chromium.org/2105173002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:89: // TODO(dimich): Not sure this is needed. Clarify and remove. maybe add a tracking bug reference here? https://codereview.chromium.org/2105173002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:101: RedirectResult::DISCONNECTED_NETWORK); It feels like there are actually two enums here: one representing the conditions under which the redirect was generated, and one representing the result of the redirect. Then you wouldn't have the error-prone switch statement when |page_not_found == true| in ReportUMARedirectResult. https://codereview.chromium.org/2105173002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:176: return; Do we want to UMA the redirect loop? https://codereview.chromium.org/2105173002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:267: switch (result) { This logic is a bit hard to read, it feels like the results should be manipulated in SelectBestPageForRedirectToOffline, rather than here. https://codereview.chromium.org/2105173002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/offline_page_tab_helper.h (right): https://codereview.chromium.org/2105173002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_tab_helper.h:45: enum RedirectResult { nit: enum class https://codereview.chromium.org/2105173002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_tab_helper.h:47: DISCONNECTED_NETWORK_NOT_FOUND, // Redirect failed, no snapshot the wording of the NOT_FOUND enum values is a little confusing. Something like PAGE_NOT_FOUND_ON_DISCONNECTED_NETWORK reads a little more clearly to me.
https://codereview.chromium.org/2105173002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/offline_page_tab_helper.cc (right): https://codereview.chromium.org/2105173002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:158: // TODO(dimich): why is this? Clarify and possibly redirect as well. This is because doing redirect here does not work for going back/forward. https://codereview.chromium.org/2105173002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:159: if (result == RedirectResult::FLAKY_NETWORK_FORWARD_BACK) { Can we rewrite line 148-164 to the following: if (ui::PageTransitionTypeIncludingQualifiersIs(...)) { // Don't actually want to redirect on a forward/back nav since it // doesn't work. ReportUMARedirectResult(RedirectResult::FLAKY_NETWORK_FORWARD_BACK); return; } GetPagesForRedirectToOffline(navigated_url, RedirectResult::FLAKY_NETWORK); https://codereview.chromium.org/2105173002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/offline_page_tab_helper.h (right): https://codereview.chromium.org/2105173002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_tab_helper.h:45: enum RedirectResult { using enum class? https://codereview.chromium.org/2105173002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_tab_helper.h:46: DISCONNECTED_NETWORK, // Redirect to offline, success Since now this enum is about redirect result, it is hard to get a sense of success from DISCONNECTED_NETWORK value. How about renaming it to SHOW_OFFLINE_ON_NO_NETWORK https://codereview.chromium.org/2105173002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_tab_helper.h:95: // Returns true is a given URL is in redirect chain already. update comment https://codereview.chromium.org/2105173002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_tab_helper.h:96: bool IsInRedirectLoop(const GURL& to_url); nit: can we add const modifier https://codereview.chromium.org/2105173002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_tab_helper.h:97: void ReportUMARedirectResult(RedirectResult result, nit: ReportRedirectResultUMA or simply ReportRedirectResult https://codereview.chromium.org/2105173002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2105173002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:35866: - <owner>jianli@chromium.org</owner> You should mark this as obsolete instead of removing it.
PTAL https://codereview.chromium.org/2105173002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/offline_page_tab_helper.cc (right): https://codereview.chromium.org/2105173002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:89: // TODO(dimich): Not sure this is needed. Clarify and remove. On 2016/06/29 00:16:31, dewittj wrote: > maybe add a tracking bug reference here? Done. https://codereview.chromium.org/2105173002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:101: RedirectResult::DISCONNECTED_NETWORK); On 2016/06/29 00:16:31, dewittj wrote: > It feels like there are actually two enums here: one representing the conditions > under which the redirect was generated, and one representing the result of the > redirect. Then you wouldn't have the error-prone switch statement when > |page_not_found == true| in ReportUMARedirectResult. I removed that switch but still want to have a denormalized flat enum. Even though some outcomes are clearly a combination of things, it provides for much more understandable and useful UMA histogram at the end. For example, if it was normalized into 2 histograms (if this is what you mean) the connection between results would be lost and it would be impossible to tell in which case pages are not being found etc. Also less Dremel to get useful numbers. I've changed wording and removed one unused value so how it is clearer now. https://codereview.chromium.org/2105173002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:158: // TODO(dimich): why is this? Clarify and possibly redirect as well. On 2016/06/29 00:19:30, jianli wrote: > This is because doing redirect here does not work for going back/forward. I don't understand *why* it doesn't work so I'll leave the TODO for now - it is mostly a TODO to understand this :) https://codereview.chromium.org/2105173002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:159: if (result == RedirectResult::FLAKY_NETWORK_FORWARD_BACK) { On 2016/06/29 00:19:29, jianli wrote: > Can we rewrite line 148-164 to the following: > if (ui::PageTransitionTypeIncludingQualifiersIs(...)) { > // Don't actually want to redirect on a forward/back nav since it > // doesn't work. > ReportUMARedirectResult(RedirectResult::FLAKY_NETWORK_FORWARD_BACK); > return; > } > > GetPagesForRedirectToOffline(navigated_url, RedirectResult::FLAKY_NETWORK); Done. https://codereview.chromium.org/2105173002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:176: return; On 2016/06/29 00:16:31, dewittj wrote: > Do we want to UMA the redirect loop? Done. I think it might be a fringe condition, but with FLAKY_NETWORKS it can be more... https://codereview.chromium.org/2105173002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:267: switch (result) { On 2016/06/29 00:16:31, dewittj wrote: > This logic is a bit hard to read, it feels like the results should be > manipulated in SelectBestPageForRedirectToOffline, rather than here. Done. https://codereview.chromium.org/2105173002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/offline_page_tab_helper.h (right): https://codereview.chromium.org/2105173002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_tab_helper.h:45: enum RedirectResult { On 2016/06/29 00:16:32, dewittj wrote: > nit: enum class Done. Adds more static_cast<int>() in the test code but it's ok. https://codereview.chromium.org/2105173002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_tab_helper.h:45: enum RedirectResult { On 2016/06/29 00:16:32, dewittj wrote: > nit: enum class Done. https://codereview.chromium.org/2105173002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_tab_helper.h:46: DISCONNECTED_NETWORK, // Redirect to offline, success On 2016/06/29 00:19:30, jianli wrote: > Since now this enum is about redirect result, it is hard to get a sense of > success from DISCONNECTED_NETWORK value. How about renaming it to > SHOW_OFFLINE_ON_NO_NETWORK Done. https://codereview.chromium.org/2105173002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_tab_helper.h:47: DISCONNECTED_NETWORK_NOT_FOUND, // Redirect failed, no snapshot On 2016/06/29 00:16:32, dewittj wrote: > the wording of the NOT_FOUND enum values is a little confusing. Something like > PAGE_NOT_FOUND_ON_DISCONNECTED_NETWORK reads a little more clearly to me. Done. https://codereview.chromium.org/2105173002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_tab_helper.h:95: // Returns true is a given URL is in redirect chain already. On 2016/06/29 00:19:30, jianli wrote: > update comment Done. https://codereview.chromium.org/2105173002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_tab_helper.h:96: bool IsInRedirectLoop(const GURL& to_url); On 2016/06/29 00:19:30, jianli wrote: > nit: can we add const modifier Done. https://codereview.chromium.org/2105173002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_tab_helper.h:97: void ReportUMARedirectResult(RedirectResult result, On 2016/06/29 00:19:30, jianli wrote: > nit: ReportRedirectResultUMA or simply ReportRedirectResult Done. https://codereview.chromium.org/2105173002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2105173002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:35866: - <owner>jianli@chromium.org</owner> On 2016/06/29 00:19:30, jianli wrote: > You should mark this as obsolete instead of removing it. Done.
lgtm with nits and comments. https://codereview.chromium.org/2105173002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_tab_helper.cc (right): https://codereview.chromium.org/2105173002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:89: // TODO(dimich): Not sure this is needed. Clarify and remove. Bug 624216. https://bugs.chromium.org/p/chromium/issues/detail?id=601217 It might be useful, but perhaps it is not. https://codereview.chromium.org/2105173002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:154: ReportRedirectResultUMA(RedirectResult::IGNORED_FLAKY_NETWORK_FORWARD_BACK); Cool find. https://codereview.chromium.org/2105173002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:225: result = RedirectResult::PAGE_NOT_FOUND_ON_FLAKY_NETWORK; nit: I am mildly bothered by overwriting input parameter, and I know you are returning right after that... but on some level... How about this? ReportRedirectResultUMA( result == RedirectResult::REDIRECTED_ON_FLAKY_NETWORK ? RedirectResult::PAGE_NOT_FOUND_ON_FLAKY_NETWORK : RedirectResult::PAGE_NOT_FOUND_ON_DISCONNECTED_NETWORK) return; I know it looks like monstrosity, but you know... still bothers me less :) https://codereview.chromium.org/2105173002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:261: // Avoids looping between online and offline redirections. Update comment: Detects looping... https://codereview.chromium.org/2105173002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_tab_helper.h (right): https://codereview.chromium.org/2105173002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.h:45: enum class RedirectResult { nit: Could you move this above the desctructor? It is a type definition and I think should be at the beginning https://codereview.chromium.org/2105173002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.h:90: void TryRedirectToOffline(RedirectResult result, nit: please reorder parameters for consistency, if you don't mind. https://codereview.chromium.org/2105173002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2105173002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:83811: + <int value="4" label="Ignored flaky network f/b"/> expand f/b https://codereview.chromium.org/2105173002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:83812: + <int value="5" label="Connected network"/> Redirected on connected network https://codereview.chromium.org/2105173002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:83813: + <int value="6" label="No Android Tab found"/> No tab ID found? https://codereview.chromium.org/2105173002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:83814: + <int value="7" label="Show net error page"/> Showing error page with no redirection path ?
lgtm mostly https://codereview.chromium.org/2105173002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/offline_page_tab_helper.cc (right): https://codereview.chromium.org/2105173002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:101: RedirectResult::DISCONNECTED_NETWORK); On 2016/06/29 01:51:31, Dmitry Titov wrote: > On 2016/06/29 00:16:31, dewittj wrote: > > It feels like there are actually two enums here: one representing the > conditions > > under which the redirect was generated, and one representing the result of the > > redirect. Then you wouldn't have the error-prone switch statement when > > |page_not_found == true| in ReportUMARedirectResult. > > I removed that switch but still want to have a denormalized flat enum. Even > though some outcomes are clearly a combination of things, it provides for much > more understandable and useful UMA histogram at the end. For example, if it was > normalized into 2 histograms (if this is what you mean) the connection between > results would be lost and it would be impossible to tell in which case pages are > not being found etc. Also less Dremel to get useful numbers. > > I've changed wording and removed one unused value so how it is clearer now. I was actually requesting two enums in code: one for program flow control here, and a separate one for use with the histogram; the point being that we don't have a logical "result" yet, we have a state machine state which will be used to modify execution paths further down the line.
On 2016/06/29 17:36:07, dewittj wrote: > lgtm mostly > > https://codereview.chromium.org/2105173002/diff/1/chrome/browser/android/offl... > File chrome/browser/android/offline_pages/offline_page_tab_helper.cc (right): > > https://codereview.chromium.org/2105173002/diff/1/chrome/browser/android/offl... > chrome/browser/android/offline_pages/offline_page_tab_helper.cc:101: > RedirectResult::DISCONNECTED_NETWORK); > On 2016/06/29 01:51:31, Dmitry Titov wrote: > > On 2016/06/29 00:16:31, dewittj wrote: > > > It feels like there are actually two enums here: one representing the > > conditions > > > under which the redirect was generated, and one representing the result of > the > > > redirect. Then you wouldn't have the error-prone switch statement when > > > |page_not_found == true| in ReportUMARedirectResult. > > > > I removed that switch but still want to have a denormalized flat enum. Even > > though some outcomes are clearly a combination of things, it provides for much > > more understandable and useful UMA histogram at the end. For example, if it > was > > normalized into 2 histograms (if this is what you mean) the connection between > > results would be lost and it would be impossible to tell in which case pages > are > > not being found etc. Also less Dremel to get useful numbers. > > > > I've changed wording and removed one unused value so how it is clearer now. > > I was actually requesting two enums in code: one for program flow control here, > and a separate one for use with the histogram; the point being that we don't > have a logical "result" yet, we have a state machine state which will be used to > modify execution paths further down the line. Thanks for explanation. I agree and instead I removed the enum from flow control altogether, it is only for UMA purposes. Added comment to .h file to this effect, to reflect that it is used to accumulate result to send it to UMA at the end. So at the end we only need one UMA enum.
PTAL +mpearson for histograms.xml https://codereview.chromium.org/2105173002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_tab_helper.cc (right): https://codereview.chromium.org/2105173002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:89: // TODO(dimich): Not sure this is needed. Clarify and remove. Bug 624216. On 2016/06/29 17:01:15, fgorski wrote: > https://bugs.chromium.org/p/chromium/issues/detail?id=601217 > > It might be useful, but perhaps it is not. Thanks! Added reference to the tracking bug to link them. https://codereview.chromium.org/2105173002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:154: ReportRedirectResultUMA(RedirectResult::IGNORED_FLAKY_NETWORK_FORWARD_BACK); On 2016/06/29 17:01:16, fgorski wrote: > Cool find. Acknowledged. https://codereview.chromium.org/2105173002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:225: result = RedirectResult::PAGE_NOT_FOUND_ON_FLAKY_NETWORK; On 2016/06/29 17:01:15, fgorski wrote: > nit: I am mildly bothered by overwriting input parameter, and I know you are > returning right after that... but on some level... > > How about this? > ReportRedirectResultUMA( > result == RedirectResult::REDIRECTED_ON_FLAKY_NETWORK ? > RedirectResult::PAGE_NOT_FOUND_ON_FLAKY_NETWORK : > RedirectResult::PAGE_NOT_FOUND_ON_DISCONNECTED_NETWORK) > return; > > I know it looks like monstrosity, but you know... still bothers me less :) Done. https://codereview.chromium.org/2105173002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:261: // Avoids looping between online and offline redirections. On 2016/06/29 17:01:15, fgorski wrote: > Update comment: Detects looping... Done. https://codereview.chromium.org/2105173002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_tab_helper.h (right): https://codereview.chromium.org/2105173002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.h:45: enum class RedirectResult { On 2016/06/29 17:01:16, fgorski wrote: > nit: Could you move this above the desctructor? It is a type definition and I > think should be at the beginning Done. https://codereview.chromium.org/2105173002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.h:90: void TryRedirectToOffline(RedirectResult result, On 2016/06/29 17:01:16, fgorski wrote: > nit: please reorder parameters for consistency, if you don't mind. Done. https://codereview.chromium.org/2105173002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2105173002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:83811: + <int value="4" label="Ignored flaky network f/b"/> On 2016/06/29 17:01:16, fgorski wrote: > expand f/b Done. https://codereview.chromium.org/2105173002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:83812: + <int value="5" label="Connected network"/> On 2016/06/29 17:01:16, fgorski wrote: > Redirected on connected network Done. https://codereview.chromium.org/2105173002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:83813: + <int value="6" label="No Android Tab found"/> On 2016/06/29 17:01:16, fgorski wrote: > No tab ID found? Done. https://codereview.chromium.org/2105173002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:83814: + <int value="7" label="Show net error page"/> On 2016/06/29 17:01:16, fgorski wrote: > Showing error page with no redirection path ? Done, used "Unknown net error, showing error page"
dimich@chromium.org changed reviewers: + mpearson@chromium.org
https://codereview.chromium.org/2105173002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_tab_helper.h (right): https://codereview.chromium.org/2105173002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.h:39: // errors (like no OfflinePageModel etc) are not included. Please add something in here that because this is used for UMA reporting, these values should not be changed or reused; new values should be ended immediately before the MAX value. Also, for ease in aligning this with histograms.xml, please do =0, =1, =2, etc. on each of these (possibly except MAX, up to you). https://codereview.chromium.org/2105173002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.h:39: // errors (like no OfflinePageModel etc) are not included. If "fringe errors are not included" yet "one of these outcomes will happen", what do you emit for fringe errors? https://codereview.chromium.org/2105173002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.h:41: enum class RedirectResult { nit: why a class? https://codereview.chromium.org/2105173002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.h:47: REDIRECTED_ON_CONNECTED_NETWORK, No PAGE_NOT_FOUND_ON_CONNECTED_NETWORK? https://codereview.chromium.org/2105173002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2105173002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:35868: + Result of automatic redirect to offline version of the page or back. It would be nice to say roughly when this is emitted. I somewhat get the impression from the enum comment that you might be able to say: "Emitted to exactly once when an offline page is attempted to be shown." https://codereview.chromium.org/2105173002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:83813: + <int value="6" label="No Tab Id found"/> While I can understand the other labels, I don't understand this one. Is there a clearer way to explain it?
lgtm https://codereview.chromium.org/2105173002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_tab_helper.cc (right): https://codereview.chromium.org/2105173002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:166: if (!offline_page) It seems that we can return PAGE_NOT_FOUND_ON_CONNECTED_NETWORK here. https://codereview.chromium.org/2105173002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:183: RedirectResult result, const GURL& online_url) { Passing RedirectResult at this time seems to be confusing since we indeed have not concluded with the final result yet. Can we just pass a boolean flag to indicate if the network is disconnected or flaky? https://codereview.chromium.org/2105173002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_tab_helper.h (right): https://codereview.chromium.org/2105173002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.h:99: bool IsInRedirectLoop(const GURL& to_url) const; nit: add empty line
https://codereview.chromium.org/2105173002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_tab_helper.h (right): https://codereview.chromium.org/2105173002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.h:39: // errors (like no OfflinePageModel etc) are not included. On 2016/06/29 19:18:27, Mark P wrote: > If "fringe errors are not included" yet "one of these outcomes will happen", > what do you emit for fringe errors? Nothing is emitted for those. These are the cases that theoretically can happen but won't in practice - for example, we pull the pointer to a KeyedService (which should always be there) but the code still checks it for NULL and does an early return. Those are likely never happen or at least they are not interesting for evaluating redirect usage/effectiveness. https://codereview.chromium.org/2105173002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.h:39: // errors (like no OfflinePageModel etc) are not included. On 2016/06/29 19:18:27, Mark P wrote: > Please add something in here that because this is used for UMA reporting, these > values should not be changed or reused; new values should be ended immediately > before the MAX value. > > Also, for ease in aligning this with histograms.xml, please do =0, =1, =2, etc. > on each of these (possibly except MAX, up to you). Done. Moved and re-phrased the comment that was near REDIRECT_RESULT_MAX... https://codereview.chromium.org/2105173002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.h:41: enum class RedirectResult { On 2016/06/29 19:18:27, Mark P wrote: > nit: why a class? We seem to be getting a suggestion to use enum classes rather than an old enum almost universally lately... For new code for sure. While coding style does not mandate usage of one over another, they are more 'advanced' (no implicit conversions, scoped names). Is there a reason not to use them? Here is a link I use to look for allowed C++ features: https://chromium-cpp.appspot.com/ https://codereview.chromium.org/2105173002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.h:47: REDIRECTED_ON_CONNECTED_NETWORK, On 2016/06/29 19:18:27, Mark P wrote: > No PAGE_NOT_FOUND_ON_CONNECTED_NETWORK? Right, not applicable to connected network. "Page not found" in this context always means offline page (a snapshot saved locally) and we don't do lookup for it in connected case. This type of redirect happens when we were showing the offline snapshot and then user reloads when network is connected - this redirect always goes to online page, which might be also 'not found' but it's a different 'not found' that we are not interested in here... https://codereview.chromium.org/2105173002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2105173002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:35868: + Result of automatic redirect to offline version of the page or back. On 2016/06/29 19:18:27, Mark P wrote: > It would be nice to say roughly when this is emitted. I somewhat get the > impression from the enum comment that you might be able to say: > "Emitted to exactly once when an offline page is attempted to be shown." > Done. https://codereview.chromium.org/2105173002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:83813: + <int value="6" label="No Tab Id found"/> On 2016/06/29 19:18:27, Mark P wrote: > While I can understand the other labels, I don't understand this one. Is there > a clearer way to explain it? Done. We use Android Tab persistent ID to guard for privacy, so in addition to tests we'd like to know if this will ever be not 0. This is the only reason to have it here reported, otherwise it'd be in 'fringe' category.
Jians comments addressed https://codereview.chromium.org/2105173002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_tab_helper.cc (right): https://codereview.chromium.org/2105173002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:166: if (!offline_page) On 2016/06/29 20:20:16, jianli wrote: > It seems that we can return PAGE_NOT_FOUND_ON_CONNECTED_NETWORK here. This case includes all regular navigations (when online). This case is not interesting for the feature and will dwarf the useful results in histogram because it is the much more frequent case. Added comment. https://codereview.chromium.org/2105173002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:183: RedirectResult result, const GURL& online_url) { On 2016/06/29 20:20:16, jianli wrote: > Passing RedirectResult at this time seems to be confusing since we indeed have > not concluded with the final result yet. Can we just pass a boolean flag to > indicate if the network is disconnected or flaky? I have a commentin .h file that explains that the result is accumulated along the codepath. passing booleans and other enums to combine into some final result at the end is possible but produces much more complex code. I considered this as well. https://codereview.chromium.org/2105173002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_tab_helper.h (right): https://codereview.chromium.org/2105173002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.h:99: bool IsInRedirectLoop(const GURL& to_url) const; On 2016/06/29 20:20:16, jianli wrote: > nit: add empty line Done.
Mark, it's ready for you when you are. See the comments/updates above.
Thanks for the detailed answers. histograms.xml lgtm baring several minor comments below. --mark https://codereview.chromium.org/2105173002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_tab_helper.h (right): https://codereview.chromium.org/2105173002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.h:39: // errors (like no OfflinePageModel etc) are not included. On 2016/06/29 20:27:42, Dmitry Titov wrote: > On 2016/06/29 19:18:27, Mark P wrote: > > Please add something in here that because this is used for UMA reporting, > these > > values should not be changed or reused; new values should be ended immediately > > before the MAX value. > > > > Also, for ease in aligning this with histograms.xml, please do =0, =1, =2, > etc. > > on each of these (possibly except MAX, up to you). > > Done. > Moved and re-phrased the comment that was near REDIRECT_RESULT_MAX... Apologies, somehow I missed that comment. https://codereview.chromium.org/2105173002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.h:41: enum class RedirectResult { On 2016/06/29 20:27:42, Dmitry Titov wrote: > On 2016/06/29 19:18:27, Mark P wrote: > > nit: why a class? > > We seem to be getting a suggestion to use enum classes rather than an old enum > almost universally lately... For new code for sure. While coding style does not > mandate usage of one over another, they are more 'advanced' (no implicit > conversions, scoped names). Is there a reason not to use them? > Here is a link I use to look for allowed C++ features: > https://chromium-cpp.appspot.com/ Thanks for the link and explanation. class sgtm. https://codereview.chromium.org/2105173002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.h:47: REDIRECTED_ON_CONNECTED_NETWORK, On 2016/06/29 20:27:42, Dmitry Titov wrote: > On 2016/06/29 19:18:27, Mark P wrote: > > No PAGE_NOT_FOUND_ON_CONNECTED_NETWORK? > > Right, not applicable to connected network. "Page not found" in this context > always means offline page (a snapshot saved locally) and we don't do lookup for > it in connected case. This type of redirect happens when we were showing the > offline snapshot and then user reloads when network is connected - this redirect > always goes to online page, which might be also 'not found' but it's a different > 'not found' that we are not interested in here... Acknowledged. https://codereview.chromium.org/2105173002/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_tab_helper.h (right): https://codereview.chromium.org/2105173002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.h:38: // redirect intent and result. One of these outcomes will happen. The fringe nit: One of -> Generally one of (because otherwise the next sentence directly contradicts this one) https://codereview.chromium.org/2105173002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.h:39: // errors (like no OfflinePageModel etc) are not reported due to their low nit: comma before "etc", period after
Thanks for review! https://codereview.chromium.org/2105173002/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_tab_helper.h (right): https://codereview.chromium.org/2105173002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.h:38: // redirect intent and result. One of these outcomes will happen. The fringe On 2016/06/30 05:09:02, Mark P wrote: > nit: One of -> Generally one of > (because otherwise the next sentence directly contradicts this one) Done. https://codereview.chromium.org/2105173002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.h:39: // errors (like no OfflinePageModel etc) are not reported due to their low On 2016/06/30 05:09:02, Mark P wrote: > nit: comma before "etc", period after Done.
The CQ bit was checked by dimich@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dewittj@chromium.org, fgorski@chromium.org, jianli@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/2105173002/#ps100001 (title: "Addressed last Mark's comments.")
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
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dimich@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 #6 (id:100001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Add enum-based UMA reporting to automatic redirect logic of Offline Pages. Removes 3 other counter-based UMA histograms that are now replaced with one. BUG=622424 ========== to ========== Add enum-based UMA reporting to automatic redirect logic of Offline Pages. Removes 3 other counter-based UMA histograms that are now replaced with one. BUG=622424 Committed: https://crrev.com/853787e334f8a353abbb44189c872b5a40f2330e Cr-Commit-Position: refs/heads/master@{#403114} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/853787e334f8a353abbb44189c872b5a40f2330e Cr-Commit-Position: refs/heads/master@{#403114} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
