Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1441393002:
Add UMA to count the number of Google CAPTCHA pages shown and solved by users. (Closed)
|
Created:
5 years, 1 month ago by Matt Welsh Modified:
5 years, 1 month ago Reviewers:
Bryan McQuade, Alexei Svitkine (slow), Deprecated (see juliatuttle), Randy Smith (Not in Mondays), Charlie Harrison CC:
chromium-reviews, csharrison+watch_chromium.org, asvitkine+watch_chromium.org, loading-reviews+metrics_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd UMA to count the number of Google CAPTCHA pages shown and solved by users.
This is intended to track down problems where a large number of mobile
users in certain countries are apparently seeing CAPTCHA pages.
(http://b/23081866 for context.)
I realize this may not be the ideal place for this code - happy for suggestions as to where else this might go.
BUG=552470
Committed: https://crrev.com/71c7cacbdb9cd4fa161909a72a9157f574a2b775
Cr-Commit-Position: refs/heads/master@{#360011}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Changed to using histogram, updated test #
Total comments: 4
Patch Set 3 : Change histogram name and add helper function #
Total comments: 4
Patch Set 4 : Minor formatting changes. #Patch Set 5 : Refactor to use PageLoadMetricsObserver interface. #Patch Set 6 : Revert unnecessary code. #Patch Set 7 : Adding missing files (whoops) #
Total comments: 15
Patch Set 8 : Minor cleanups #
Total comments: 3
Patch Set 9 : Fix nit #Patch Set 10 : Rebasing #Patch Set 11 : Rebasing #Patch Set 12 : Small fix #Messages
Total messages: 56 (14 generated)
Description was changed from ========== Add UMA to count the number of Google CAPTCHA pages shown and solved by users. This is intended to track down problems where a large number of mobile users in certain countries are apparently seeing CAPTCHA pages. (http://b2.corp.google.com/23081866 for context.) BUG=552470 ========== to ========== Add UMA to count the number of Google CAPTCHA pages shown and solved by users. This is intended to track down problems where a large number of mobile users in certain countries are apparently seeing CAPTCHA pages. (http://b2.corp.google.com/23081866 for context.) I realize this may not be the ideal place for this code - happy for suggestions as to where else this might go. BUG=552470 ==========
mdw@chromium.org changed reviewers: + asvitkine@google.com, csharrison@chromium.org
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org - asvitkine@google.com
https://codereview.chromium.org/1441393002/diff/1/components/page_load_metric... File components/page_load_metrics/browser/metrics_web_contents_observer_unittest.cc (right): https://codereview.chromium.org/1441393002/diff/1/components/page_load_metric... components/page_load_metrics/browser/metrics_web_contents_observer_unittest.cc:683: { Nit: Can you make this a table driven test? https://codereview.chromium.org/1441393002/diff/1/tools/metrics/actions/actio... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/1441393002/diff/1/tools/metrics/actions/actio... tools/metrics/actions/actions.xml:3416: <action name="GoogleCaptchaShown"> I think it would be better to use an enum histogram for this, rather than user actions. One bucket for shown and the other for solved. (We have better support for histograms in our dashboards which allow more powerful analyses.)
PTAL! https://codereview.chromium.org/1441393002/diff/1/components/page_load_metric... File components/page_load_metrics/browser/metrics_web_contents_observer_unittest.cc (right): https://codereview.chromium.org/1441393002/diff/1/components/page_load_metric... components/page_load_metrics/browser/metrics_web_contents_observer_unittest.cc:683: { On 2015/11/13 17:45:12, Alexei Svitkine (slow) wrote: > Nit: Can you make this a table driven test? Done. https://codereview.chromium.org/1441393002/diff/1/tools/metrics/actions/actio... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/1441393002/diff/1/tools/metrics/actions/actio... tools/metrics/actions/actions.xml:3416: <action name="GoogleCaptchaShown"> On 2015/11/13 17:45:12, Alexei Svitkine (slow) wrote: > I think it would be better to use an enum histogram for this, rather than user > actions. One bucket for shown and the other for solved. > > (We have better support for histograms in our dashboards which allow more > powerful analyses.) Done.
lg, just a couple more things https://codereview.chromium.org/1441393002/diff/20001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1441393002/diff/20001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:455: UMA_HISTOGRAM_ENUMERATION(kUMAGoogleCaptchaHistogram, GOOGLE_CAPTCHA_SHOWN, Nit: Can you make a helper function to log this histogram? Each macro expands to quite a bit of code, so having a helper function avoids some bloat. Then, you can just inline the histogram name in the macro instead of a separate constant. https://codereview.chromium.org/1441393002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1441393002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:14179: +<histogram name="GoogleCaptcha.Event" enum="GoogleCaptchaEvent"> How about re-using the histogram name prefix that's used by the code where you're adding this - i.e. "PageLoad.Events". e.g. name it "PageLoad.Events.GoogleCaptcha"
Thanks for the quick turnaround :-) https://codereview.chromium.org/1441393002/diff/20001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1441393002/diff/20001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:455: UMA_HISTOGRAM_ENUMERATION(kUMAGoogleCaptchaHistogram, GOOGLE_CAPTCHA_SHOWN, On 2015/11/13 19:41:57, Alexei Svitkine (slow) wrote: > Nit: Can you make a helper function to log this histogram? Each macro expands to > quite a bit of code, so having a helper function avoids some bloat. Then, you > can just inline the histogram name in the macro instead of a separate constant. Done. https://codereview.chromium.org/1441393002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1441393002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:14179: +<histogram name="GoogleCaptcha.Event" enum="GoogleCaptchaEvent"> On 2015/11/13 19:41:57, Alexei Svitkine (slow) wrote: > How about re-using the histogram name prefix that's used by the code where > you're adding this - i.e. "PageLoad.Events". > > e.g. name it "PageLoad.Events.GoogleCaptcha" Done.
lgtm https://codereview.chromium.org/1441393002/diff/40001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1441393002/diff/40001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:456: if (IsGoogleCaptcha(navigation_handle->GetURL())) { Nit: No {}s https://codereview.chromium.org/1441393002/diff/40001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:504: if (IsGoogleCaptcha(navigation_handle->GetReferrer().url)) { Nit: No {}s
mdw@chromium.org changed reviewers: + rdsmith@chromium.org, ttuttle@chromium.org
https://codereview.chromium.org/1441393002/diff/40001/components/page_load_me... File components/page_load_metrics/browser/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/1441393002/diff/40001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:456: if (IsGoogleCaptcha(navigation_handle->GetURL())) { On 2015/11/13 20:17:12, Alexei Svitkine (slow) wrote: > Nit: No {}s Done. https://codereview.chromium.org/1441393002/diff/40001/components/page_load_me... components/page_load_metrics/browser/metrics_web_contents_observer.cc:504: if (IsGoogleCaptcha(navigation_handle->GetReferrer().url)) { On 2015/11/13 20:17:12, Alexei Svitkine (slow) wrote: > Nit: No {}s Done.
On 2015/11/13 19:41:57, Alexei Svitkine (slow) wrote: > lg, just a couple more things > > https://codereview.chromium.org/1441393002/diff/20001/components/page_load_me... > File components/page_load_metrics/browser/metrics_web_contents_observer.cc > (right): > > https://codereview.chromium.org/1441393002/diff/20001/components/page_load_me... > components/page_load_metrics/browser/metrics_web_contents_observer.cc:455: > UMA_HISTOGRAM_ENUMERATION(kUMAGoogleCaptchaHistogram, GOOGLE_CAPTCHA_SHOWN, > Nit: Can you make a helper function to log this histogram? Each macro expands to > quite a bit of code, so having a helper function avoids some bloat. Then, you > can just inline the histogram name in the macro instead of a separate constant. > > https://codereview.chromium.org/1441393002/diff/20001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/1441393002/diff/20001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:14179: +<histogram > name="GoogleCaptcha.Event" enum="GoogleCaptchaEvent"> > How about re-using the histogram name prefix that's used by the code where > you're adding this - i.e. "PageLoad.Events". > > e.g. name it "PageLoad.Events.GoogleCaptcha" This should probably be a PageLoadMetricsObserver. Doc is here: https://docs.google.com/document/d/1cYiUnz_2uQNaahf06Qr21-fjR3u98qb1aDHkRn4O-... Note that we don't support redirects, but if it is necessary for this metric, we can add this feature to PageLoadMetricsObserver pretty easily I think. Can you explain briefly why the redirect check is necessary? Also, will these navigations all be in the main frame?
On 2015/11/13 21:17:10, csharrison wrote: > On 2015/11/13 19:41:57, Alexei Svitkine (slow) wrote: > > lg, just a couple more things > > > > > https://codereview.chromium.org/1441393002/diff/20001/components/page_load_me... > > File components/page_load_metrics/browser/metrics_web_contents_observer.cc > > (right): > > > > > https://codereview.chromium.org/1441393002/diff/20001/components/page_load_me... > > components/page_load_metrics/browser/metrics_web_contents_observer.cc:455: > > UMA_HISTOGRAM_ENUMERATION(kUMAGoogleCaptchaHistogram, GOOGLE_CAPTCHA_SHOWN, > > Nit: Can you make a helper function to log this histogram? Each macro expands > to > > quite a bit of code, so having a helper function avoids some bloat. Then, you > > can just inline the histogram name in the macro instead of a separate > constant. > > > > > https://codereview.chromium.org/1441393002/diff/20001/tools/metrics/histogram... > > File tools/metrics/histograms/histograms.xml (right): > > > > > https://codereview.chromium.org/1441393002/diff/20001/tools/metrics/histogram... > > tools/metrics/histograms/histograms.xml:14179: +<histogram > > name="GoogleCaptcha.Event" enum="GoogleCaptchaEvent"> > > How about re-using the histogram name prefix that's used by the code where > > you're adding this - i.e. "PageLoad.Events". > > > > e.g. name it "PageLoad.Events.GoogleCaptcha" > > This should probably be a PageLoadMetricsObserver. Doc is here: > https://docs.google.com/document/d/1cYiUnz_2uQNaahf06Qr21-fjR3u98qb1aDHkRn4O-... > Note that we don't support redirects, but if it is necessary for this metric, we > can add this feature to PageLoadMetricsObserver pretty easily I think. Can you > explain briefly why the redirect check is necessary? > > Also, will these navigations all be in the main frame? Hey Charles, the redirection is crucial because that's how we detect that the user in fact solved the CAPTCHA (the CAPTCHA page redirects to the original target page once it's solved, but the Referer header does not always flow through). I don't believe these will all be in the main frame, though I'm not sure that it's important to track non-main-frame CAPTCHAs.
On 2015/11/13 21:17:10, csharrison wrote: > On 2015/11/13 19:41:57, Alexei Svitkine (slow) wrote: > > lg, just a couple more things > > > > > https://codereview.chromium.org/1441393002/diff/20001/components/page_load_me... > > File components/page_load_metrics/browser/metrics_web_contents_observer.cc > > (right): > > > > > https://codereview.chromium.org/1441393002/diff/20001/components/page_load_me... > > components/page_load_metrics/browser/metrics_web_contents_observer.cc:455: > > UMA_HISTOGRAM_ENUMERATION(kUMAGoogleCaptchaHistogram, GOOGLE_CAPTCHA_SHOWN, > > Nit: Can you make a helper function to log this histogram? Each macro expands > to > > quite a bit of code, so having a helper function avoids some bloat. Then, you > > can just inline the histogram name in the macro instead of a separate > constant. > > > > > https://codereview.chromium.org/1441393002/diff/20001/tools/metrics/histogram... > > File tools/metrics/histograms/histograms.xml (right): > > > > > https://codereview.chromium.org/1441393002/diff/20001/tools/metrics/histogram... > > tools/metrics/histograms/histograms.xml:14179: +<histogram > > name="GoogleCaptcha.Event" enum="GoogleCaptchaEvent"> > > How about re-using the histogram name prefix that's used by the code where > > you're adding this - i.e. "PageLoad.Events". > > > > e.g. name it "PageLoad.Events.GoogleCaptcha" > > This should probably be a PageLoadMetricsObserver. Doc is here: > https://docs.google.com/document/d/1cYiUnz_2uQNaahf06Qr21-fjR3u98qb1aDHkRn4O-... > Note that we don't support redirects, but if it is necessary for this metric, we > can add this feature to PageLoadMetricsObserver pretty easily I think. Can you > explain briefly why the redirect check is necessary? > > Also, will these navigations all be in the main frame? Thanks Matt! Glad you are able to track your metrics using page_load_metrics. +1 to doing this as a PageLoadMetricsObserver. You can add GoogleCaptchaPageLoadMetricsObserver in https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pag... and implement the OnCommit method to track this. To address the redirect case, I think it's reasonable to add an OnRedirect(NavigationHandle*); to PageLoadMetricsObserver. Charles WDYT? I could see the impl looking something like: void MetricsWebContentsObserver::DidRedirectNavigation( content::NavigationHandle* navigation_handle) { if (!navigation_handle->IsInMainFrame()) return; auto it = provisional_loads_.find(navigation_handle)); if (it == provisional_loads_.end()) return; it->second->Redirect(navigation_handle); } and then void PageLoadTracker::Redirect(content::NavigationHandle* navigation_handle) { FOR_EACH_OBSERVER(PageLoadMetricsObserver, *observers_, OnRedirect(navigation_handle)); } We should probably only track this for main frames, and not for iframes. Matt do you expect to need to track captchas in iframes? Tracking information for non-main frames is probably outside the scope of page_load_metrics. Matt, one possible issue with testing the referer URL is that if the navigation is going from https to http, we are unlikely to have set the full referer, and thus this test may fail. Is there a way for us to force a captcha so we can test this logic out and make sure it does the right thing in different scenarios?
On 2015/11/13 21:17:10, csharrison wrote: > On 2015/11/13 19:41:57, Alexei Svitkine (slow) wrote: > > lg, just a couple more things > > > > > https://codereview.chromium.org/1441393002/diff/20001/components/page_load_me... > > File components/page_load_metrics/browser/metrics_web_contents_observer.cc > > (right): > > > > > https://codereview.chromium.org/1441393002/diff/20001/components/page_load_me... > > components/page_load_metrics/browser/metrics_web_contents_observer.cc:455: > > UMA_HISTOGRAM_ENUMERATION(kUMAGoogleCaptchaHistogram, GOOGLE_CAPTCHA_SHOWN, > > Nit: Can you make a helper function to log this histogram? Each macro expands > to > > quite a bit of code, so having a helper function avoids some bloat. Then, you > > can just inline the histogram name in the macro instead of a separate > constant. > > > > > https://codereview.chromium.org/1441393002/diff/20001/tools/metrics/histogram... > > File tools/metrics/histograms/histograms.xml (right): > > > > > https://codereview.chromium.org/1441393002/diff/20001/tools/metrics/histogram... > > tools/metrics/histograms/histograms.xml:14179: +<histogram > > name="GoogleCaptcha.Event" enum="GoogleCaptchaEvent"> > > How about re-using the histogram name prefix that's used by the code where > > you're adding this - i.e. "PageLoad.Events". > > > > e.g. name it "PageLoad.Events.GoogleCaptcha" > > This should probably be a PageLoadMetricsObserver. Doc is here: > https://docs.google.com/document/d/1cYiUnz_2uQNaahf06Qr21-fjR3u98qb1aDHkRn4O-... > Note that we don't support redirects, but if it is necessary for this metric, we > can add this feature to PageLoadMetricsObserver pretty easily I think. Can you > explain briefly why the redirect check is necessary? > > Also, will these navigations all be in the main frame? Thanks Matt! Glad you are able to track your metrics using page_load_metrics. +1 to doing this as a PageLoadMetricsObserver. You can add GoogleCaptchaPageLoadMetricsObserver in https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pag... and implement the OnCommit method to track this. To address the redirect case, I think it's reasonable to add an OnRedirect(NavigationHandle*); to PageLoadMetricsObserver. Charles WDYT? I could see the impl looking something like: void MetricsWebContentsObserver::DidRedirectNavigation( content::NavigationHandle* navigation_handle) { if (!navigation_handle->IsInMainFrame()) return; auto it = provisional_loads_.find(navigation_handle)); if (it == provisional_loads_.end()) return; it->second->Redirect(navigation_handle); } and then void PageLoadTracker::Redirect(content::NavigationHandle* navigation_handle) { FOR_EACH_OBSERVER(PageLoadMetricsObserver, *observers_, OnRedirect(navigation_handle)); } We should probably only track this for main frames, and not for iframes. Matt do you expect to need to track captchas in iframes? Tracking information for non-main frames is probably outside the scope of page_load_metrics. Matt, one possible issue with testing the referer URL is that if the navigation is going from https to http, we are unlikely to have set the full referer, and thus this test may fail. Is there a way for us to force a captcha so we can test this logic out and make sure it does the right thing in different scenarios?
> +1 to doing this as a PageLoadMetricsObserver. Can you explain why, given that WebContentsObserver works just fine for this? I don't quite understand the rationale for the extra layer. If we decide to go this route I'm happy to add the OnRedirect capability to your interface, if you like (of course I will send it to you for review). > We should probably only track this for main frames, and not for iframes. Matt do > you expect to need to track captchas in iframes? Answered previously, but I don't particularly care about iframes for now. > Matt, one possible issue with testing the referer URL is that if the navigation > is going from https to http, we are unlikely to have set the full referer, and > thus this test may fail. I have tested it and it works. At least in the WebContentsObserver::DidRedirectNavigation, the Referrer is still set to the CAPTCHA page even if going from https (for the CAPTCHA) to http (for the target). Would that be the case if I move this code to PageLoadMetricsObserver?
On 2015/11/13 22:10:21, Bryan McQuade wrote: > On 2015/11/13 21:17:10, csharrison wrote: > > On 2015/11/13 19:41:57, Alexei Svitkine (slow) wrote: > > > lg, just a couple more things > > > > > > > > > https://codereview.chromium.org/1441393002/diff/20001/components/page_load_me... > > > File components/page_load_metrics/browser/metrics_web_contents_observer.cc > > > (right): > > > > > > > > > https://codereview.chromium.org/1441393002/diff/20001/components/page_load_me... > > > components/page_load_metrics/browser/metrics_web_contents_observer.cc:455: > > > UMA_HISTOGRAM_ENUMERATION(kUMAGoogleCaptchaHistogram, GOOGLE_CAPTCHA_SHOWN, > > > Nit: Can you make a helper function to log this histogram? Each macro > expands > > to > > > quite a bit of code, so having a helper function avoids some bloat. Then, > you > > > can just inline the histogram name in the macro instead of a separate > > constant. > > > > > > > > > https://codereview.chromium.org/1441393002/diff/20001/tools/metrics/histogram... > > > File tools/metrics/histograms/histograms.xml (right): > > > > > > > > > https://codereview.chromium.org/1441393002/diff/20001/tools/metrics/histogram... > > > tools/metrics/histograms/histograms.xml:14179: +<histogram > > > name="GoogleCaptcha.Event" enum="GoogleCaptchaEvent"> > > > How about re-using the histogram name prefix that's used by the code where > > > you're adding this - i.e. "PageLoad.Events". > > > > > > e.g. name it "PageLoad.Events.GoogleCaptcha" > > > > This should probably be a PageLoadMetricsObserver. Doc is here: > > > https://docs.google.com/document/d/1cYiUnz_2uQNaahf06Qr21-fjR3u98qb1aDHkRn4O-... > > Note that we don't support redirects, but if it is necessary for this metric, > we > > can add this feature to PageLoadMetricsObserver pretty easily I think. Can you > > explain briefly why the redirect check is necessary? > > > > Also, will these navigations all be in the main frame? > > Thanks Matt! Glad you are able to track your metrics using page_load_metrics. > > +1 to doing this as a PageLoadMetricsObserver. You can add > GoogleCaptchaPageLoadMetricsObserver in > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pag... > and implement the OnCommit method to track this. > > To address the redirect case, I think it's reasonable to add an > OnRedirect(NavigationHandle*); to PageLoadMetricsObserver. Charles WDYT? I could > see the impl looking something like: > > void MetricsWebContentsObserver::DidRedirectNavigation( > content::NavigationHandle* navigation_handle) { > if (!navigation_handle->IsInMainFrame()) > return; > > auto it = provisional_loads_.find(navigation_handle)); > if (it == provisional_loads_.end()) > return; > it->second->Redirect(navigation_handle); > } > > and then > > void PageLoadTracker::Redirect(content::NavigationHandle* navigation_handle) { > FOR_EACH_OBSERVER(PageLoadMetricsObserver, *observers_, > OnRedirect(navigation_handle)); > } > > We should probably only track this for main frames, and not for iframes. Matt do > you expect to need to track captchas in iframes? Tracking information for > non-main frames is probably outside the scope of page_load_metrics. > > Matt, one possible issue with testing the referer URL is that if the navigation > is going from https to http, we are unlikely to have set the full referer, and > thus this test may fail. Is there a way for us to force a captcha so we can test > this logic out and make sure it does the right thing in different scenarios? Bryan, that implementation looks good to me. Matt, if you don't mind restricting this to main frame loads, I can help you move this logic into an observer.
On 2015/11/13 22:27:52, csharrison wrote: > On 2015/11/13 22:10:21, Bryan McQuade wrote: > > On 2015/11/13 21:17:10, csharrison wrote: > > > On 2015/11/13 19:41:57, Alexei Svitkine (slow) wrote: > > > > lg, just a couple more things > > > > > > > > > > > > > > https://codereview.chromium.org/1441393002/diff/20001/components/page_load_me... > > > > File components/page_load_metrics/browser/metrics_web_contents_observer.cc > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1441393002/diff/20001/components/page_load_me... > > > > components/page_load_metrics/browser/metrics_web_contents_observer.cc:455: > > > > UMA_HISTOGRAM_ENUMERATION(kUMAGoogleCaptchaHistogram, > GOOGLE_CAPTCHA_SHOWN, > > > > Nit: Can you make a helper function to log this histogram? Each macro > > expands > > > to > > > > quite a bit of code, so having a helper function avoids some bloat. Then, > > you > > > > can just inline the histogram name in the macro instead of a separate > > > constant. > > > > > > > > > > > > > > https://codereview.chromium.org/1441393002/diff/20001/tools/metrics/histogram... > > > > File tools/metrics/histograms/histograms.xml (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1441393002/diff/20001/tools/metrics/histogram... > > > > tools/metrics/histograms/histograms.xml:14179: +<histogram > > > > name="GoogleCaptcha.Event" enum="GoogleCaptchaEvent"> > > > > How about re-using the histogram name prefix that's used by the code where > > > > you're adding this - i.e. "PageLoad.Events". > > > > > > > > e.g. name it "PageLoad.Events.GoogleCaptcha" > > > > > > This should probably be a PageLoadMetricsObserver. Doc is here: > > > > > > https://docs.google.com/document/d/1cYiUnz_2uQNaahf06Qr21-fjR3u98qb1aDHkRn4O-... > > > Note that we don't support redirects, but if it is necessary for this > metric, > > we > > > can add this feature to PageLoadMetricsObserver pretty easily I think. Can > you > > > explain briefly why the redirect check is necessary? > > > > > > Also, will these navigations all be in the main frame? > > > > Thanks Matt! Glad you are able to track your metrics using page_load_metrics. > > > > +1 to doing this as a PageLoadMetricsObserver. You can add > > GoogleCaptchaPageLoadMetricsObserver in > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pag... > > and implement the OnCommit method to track this. > > > > To address the redirect case, I think it's reasonable to add an > > OnRedirect(NavigationHandle*); to PageLoadMetricsObserver. Charles WDYT? I > could > > see the impl looking something like: > > > > void MetricsWebContentsObserver::DidRedirectNavigation( > > content::NavigationHandle* navigation_handle) { > > if (!navigation_handle->IsInMainFrame()) > > return; > > > > auto it = provisional_loads_.find(navigation_handle)); > > if (it == provisional_loads_.end()) > > return; > > it->second->Redirect(navigation_handle); > > } > > > > and then > > > > void PageLoadTracker::Redirect(content::NavigationHandle* navigation_handle) { > > FOR_EACH_OBSERVER(PageLoadMetricsObserver, *observers_, > > OnRedirect(navigation_handle)); > > } > > > > We should probably only track this for main frames, and not for iframes. Matt > do > > you expect to need to track captchas in iframes? Tracking information for > > non-main frames is probably outside the scope of page_load_metrics. > > > > Matt, one possible issue with testing the referer URL is that if the > navigation > > is going from https to http, we are unlikely to have set the full referer, and > > thus this test may fail. Is there a way for us to force a captcha so we can > test > > this logic out and make sure it does the right thing in different scenarios? > > Bryan, that implementation looks good to me. Matt, if you don't mind restricting > this to main frame loads, I can help you move this logic into an observer. Re: referrer sanitizing, this is determined here: https://code.google.com/p/chromium/codesearch#chromium/src/content/public/com... see: https://w3c.github.io/webappsec-referrer-policy/#determine-requests-referrer
On 2015/11/13 22:25:39, Matt Welsh wrote: > > +1 to doing this as a PageLoadMetricsObserver. > > Can you explain why, given that WebContentsObserver works just fine for this? I > don't quite understand the rationale for the extra layer. Sure. We have clients that want to track custom page load related metrics, such as https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pag... https://codereview.chromium.org/1303973009/diff/220001/chrome/browser/page_lo... Tarun will be adding one for datasaver and this new one for google captcha as well. We could add the logic for each of these to MetricsWebContentsObserver, but the concern is that MWCO becomes a large and complex file with a combination of the core logic to instrument the hooks needed for page load metrics collection, as well as a collection of various metrics that other teams are interested in. To keep the code more maintainable, we're trying to keep the core MetricsWebContentsObserver code focused on instrumenting the right hooks to track page load related metrics info, and putting the metrics logging code for each client in PageLoadMetricsObservers. We have an AI to possibly move our core metrics to a PageLoadMetricsObserver as well. > > If we decide to go this route I'm happy to add the OnRedirect capability to your > interface, if you like (of course I will send it to you for review). > > > We should probably only track this for main frames, and not for iframes. Matt > do > > you expect to need to track captchas in iframes? > > Answered previously, but I don't particularly care about iframes for now. > > > Matt, one possible issue with testing the referer URL is that if the > navigation > > is going from https to http, we are unlikely to have set the full referer, and > > thus this test may fail. > > I have tested it and it works. At least in the > WebContentsObserver::DidRedirectNavigation, the Referrer is still set to the > CAPTCHA page even if going from https (for the CAPTCHA) to http (for the > target). > Would that be the case if I move this code to PageLoadMetricsObserver? Yep it should work the same either way. The referer sanitizing code happens upstream of our code, so you will get the same result whether in MetricsWebContentsObserver or PageLoadMetricsObserver.
Thanks Charles -- I am taking a stab at refactoring this code to move it to a PageLoadMetricsObserver. Will send for review when ready.
OK, take a look. Happy to add additional tests for the new Redirect functionality if you agree with the design.
Matt: Are you looking for review specifically from me, or was I one of several net stack folks you pinged and you're good with review from any? If the latter, you seem to be in good hands :-}.
On 2015/11/14 00:05:56, rdsmith wrote: > Matt: Are you looking for review specifically from me, or was I one of several > net stack folks you pinged and you're good with review from any? If the latter, > you seem to be in good hands :-}. Randy, you're in OWNERS for components/page_load_metrics - I guess I need OWNERS approval at some point from someone?
On 2015/11/14 00:07:20, Matt Welsh wrote: > On 2015/11/14 00:05:56, rdsmith wrote: > > Matt: Are you looking for review specifically from me, or was I one of several > > net stack folks you pinged and you're good with review from any? If the > latter, > > you seem to be in good hands :-}. > > Randy, you're in OWNERS for components/page_load_metrics - I guess I need OWNERS > approval at some point from someone? I am? How amusing, I didn't realize. Charles knows the code better than I (he wrote it) so he can do the primary review, and I'll take a pass over after that. Thanks for the clarification/context.
On 2015/11/14 00:10:40, rdsmith wrote: > On 2015/11/14 00:07:20, Matt Welsh wrote: > > On 2015/11/14 00:05:56, rdsmith wrote: > > > Matt: Are you looking for review specifically from me, or was I one of > several > > > net stack folks you pinged and you're good with review from any? If the > > latter, > > > you seem to be in good hands :-}. > > > > Randy, you're in OWNERS for components/page_load_metrics - I guess I need > OWNERS > > approval at some point from someone? > > I am? How amusing, I didn't realize. Charles knows the code better than I (he > wrote it) so he can do the primary review, and I'll take a pass over after that. > > > Thanks for the clarification/context. Looks good Matt, but I think you forgot to add the observer files. Thanks for making this change. Randy, you were added as owner until Bryan/I can become committers. Hence all your code reviews for this project :) Sorry if that wasn't clear (and thanks!)
Sorry, added the missing files.
Looking good. https://codereview.chromium.org/1441393002/diff/120001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/google_captcha_observer.cc (right): https://codereview.chromium.org/1441393002/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/google_captcha_observer.cc:13: namespace { Nest the anonymous namespace within the google_captcha_observer namespace. https://codereview.chromium.org/1441393002/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/google_captcha_observer.cc:15: const char kHistogramNameGoogleCaptcha[] = "PageLoad.Events.GoogleCaptcha"; Can you follow the same naming style as other histogram clients? Look at FromGWS as an example. Here it should be "PageLoad.Clients.GoogleCaptcha.Events" This was originally meant to be as a histogram prefix, but I think it makes sense to overload the name for client specific events. Also, change the name to something like "kGoogleCaptchaEvents," to be more in line with constant redefinition in this pending CL: https://codereview.chromium.org/1417733011/ https://codereview.chromium.org/1441393002/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/google_captcha_observer.cc:58: } // namespace google_captcha_observer nit: two spaces between } and // (ditto other places where this occurs)
On 2015/11/14 00:35:09, csharrison wrote: > On 2015/11/14 00:10:40, rdsmith wrote: > > On 2015/11/14 00:07:20, Matt Welsh wrote: > > > On 2015/11/14 00:05:56, rdsmith wrote: > > > > Matt: Are you looking for review specifically from me, or was I one of > > several > > > > net stack folks you pinged and you're good with review from any? If the > > > latter, > > > > you seem to be in good hands :-}. > > > > > > Randy, you're in OWNERS for components/page_load_metrics - I guess I need > > OWNERS > > > approval at some point from someone? > > > > I am? How amusing, I didn't realize. Charles knows the code better than I > (he > > wrote it) so he can do the primary review, and I'll take a pass over after > that. > > > > > > Thanks for the clarification/context. > > Looks good Matt, but I think you forgot to add the observer files. Thanks for > making this change. > > Randy, you were added as owner until Bryan/I can become committers. Hence all > your code reviews for this project :) Sorry if that wasn't clear (and thanks!) Yeah, that was my guess when I saw the OWNERS file. And you've almost certainly told me before--I just forgot. No worries.
Looks fine except for the separate/shared file inconsistency mentioned below, but I'd like to make sure Charles and I have the same images about that before landing. https://codereview.chromium.org/1441393002/diff/120001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/page_load_metrics_observers_unittest.cc (right): https://codereview.chromium.org/1441393002/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/page_load_metrics_observers_unittest.cc:201: TEST_F(PageLoadMetricsObserverTest, IsGoogleCaptcha) { Charles: Did we have a conversation about all the different observers sharing a file for the unittests? It seems weird to me to have the actual code broken out into a separate file and the test code shared.
On 2015/11/14 01:45:49, rdsmith wrote: > Looks fine except for the separate/shared file inconsistency mentioned below, > but I'd like to make sure Charles and I have the same images about that before > landing. > > https://codereview.chromium.org/1441393002/diff/120001/chrome/browser/page_lo... > File > chrome/browser/page_load_metrics/observers/page_load_metrics_observers_unittest.cc > (right): > > https://codereview.chromium.org/1441393002/diff/120001/chrome/browser/page_lo... > chrome/browser/page_load_metrics/observers/page_load_metrics_observers_unittest.cc:201: > TEST_F(PageLoadMetricsObserverTest, IsGoogleCaptcha) { > Charles: Did we have a conversation about all the different observers sharing a > file for the unittests? It seems weird to me to have the actual code broken out > into a separate file and the test code shared. Yes and I think I was advocating single file. I dislike so many files for so little code. I would be willing to separate them into separate TestClasses if that needs to happen, but I don't think this one test needs a class to itself right now.
On 2015/11/14 01:59:49, csharrison wrote: > On 2015/11/14 01:45:49, rdsmith wrote: > > Looks fine except for the separate/shared file inconsistency mentioned below, > > but I'd like to make sure Charles and I have the same images about that before > > landing. > > > > > https://codereview.chromium.org/1441393002/diff/120001/chrome/browser/page_lo... > > File > > > chrome/browser/page_load_metrics/observers/page_load_metrics_observers_unittest.cc > > (right): > > > > > https://codereview.chromium.org/1441393002/diff/120001/chrome/browser/page_lo... > > > chrome/browser/page_load_metrics/observers/page_load_metrics_observers_unittest.cc:201: > > TEST_F(PageLoadMetricsObserverTest, IsGoogleCaptcha) { > > Charles: Did we have a conversation about all the different observers sharing > a > > file for the unittests? It seems weird to me to have the actual code broken > out > > into a separate file and the test code shared. > > Yes and I think I was advocating single file. I dislike so many files for so > little code. I would be willing to separate them into separate TestClasses if > that needs to happen, but I don't think this one test needs a class to itself > right now. Then can we move the interface and implementation of the classes into a shared file as well? I think having those be separate and the tests be shared is inconsistent with Chromium convention.
On 2015/11/14 02:08:18, rdsmith wrote: > On 2015/11/14 01:59:49, csharrison wrote: > > On 2015/11/14 01:45:49, rdsmith wrote: > > > Looks fine except for the separate/shared file inconsistency mentioned > below, > > > but I'd like to make sure Charles and I have the same images about that > before > > > landing. > > > > > > > > > https://codereview.chromium.org/1441393002/diff/120001/chrome/browser/page_lo... > > > File > > > > > > chrome/browser/page_load_metrics/observers/page_load_metrics_observers_unittest.cc > > > (right): > > > > > > > > > https://codereview.chromium.org/1441393002/diff/120001/chrome/browser/page_lo... > > > > > > chrome/browser/page_load_metrics/observers/page_load_metrics_observers_unittest.cc:201: > > > TEST_F(PageLoadMetricsObserverTest, IsGoogleCaptcha) { > > > Charles: Did we have a conversation about all the different observers > sharing > > a > > > file for the unittests? It seems weird to me to have the actual code broken > > out > > > into a separate file and the test code shared. > > > > Yes and I think I was advocating single file. I dislike so many files for so > > little code. I would be willing to separate them into separate TestClasses if > > that needs to happen, but I don't think this one test needs a class to itself > > right now. > > Then can we move the interface and implementation of the classes into a shared > file as well? I think having those be separate and the tests be shared is > inconsistent with Chromium convention. Fine by me. Filed https://crbug.com/555898 for now. I will make this change after these CLs get through.
Ok, thanks. LGTM.
still lgtm https://codereview.chromium.org/1441393002/diff/120001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/google_captcha_observer.h (right): https://codereview.chromium.org/1441393002/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/google_captcha_observer.h:11: namespace google_captcha_observer { I'll leave it up to owners of this, but I'm not sure it makes sense to have its own namespace for this class. It's more conventional to use the same namespace as the overall component. (Unless they owners prefer this style?) https://codereview.chromium.org/1441393002/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/google_captcha_observer.h:16: enum GoogleCaptchaEvent { Nit: This can probably live in the .cc if you're not using it elsewhere. https://codereview.chromium.org/1441393002/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/google_captcha_observer.h:38: DISALLOW_COPY_AND_ASSIGN(GoogleCaptchaObserver); Nit: Add an empty line above this.
Thanks for the comments, PTAL. https://codereview.chromium.org/1441393002/diff/120001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/google_captcha_observer.cc (right): https://codereview.chromium.org/1441393002/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/google_captcha_observer.cc:13: namespace { On 2015/11/14 01:19:55, csharrison wrote: > Nest the anonymous namespace within the google_captcha_observer namespace. Done. https://codereview.chromium.org/1441393002/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/google_captcha_observer.cc:15: const char kHistogramNameGoogleCaptcha[] = "PageLoad.Events.GoogleCaptcha"; On 2015/11/14 01:19:55, csharrison wrote: > Can you follow the same naming style as other histogram clients? Look at FromGWS > as an example. Here it should be "PageLoad.Clients.GoogleCaptcha.Events" > > This was originally meant to be as a histogram prefix, but I think it makes > sense to overload the name for client specific events. > > Also, change the name to something like "kGoogleCaptchaEvents," to be more in > line with constant redefinition in this pending CL: > https://codereview.chromium.org/1417733011/ Done. https://codereview.chromium.org/1441393002/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/google_captcha_observer.cc:58: } // namespace google_captcha_observer On 2015/11/14 01:19:55, csharrison wrote: > nit: two spaces between } and // (ditto other places where this occurs) Done. How do I autoformat my code to adhere to Chromium style? https://codereview.chromium.org/1441393002/diff/120001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/google_captcha_observer.h (right): https://codereview.chromium.org/1441393002/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/google_captcha_observer.h:11: namespace google_captcha_observer { On 2015/11/16 16:54:59, Alexei Svitkine (slow) wrote: > I'll leave it up to owners of this, but I'm not sure it makes sense to have its > own namespace for this class. It's more conventional to use the same namespace > as the overall component. (Unless they owners prefer this style?) So I'm happy to do whatever makes sense. There is no existing namespace for other classes in this directory -- from_gws_page_load_metrics_observer.cc uses the global namespace. Since I'm adding the IsGoogleCaptcha function, I feel that this should not be global and needs to be nested in some namespace, but I don't particularly care which -- Charles, do you have a suggestion? https://codereview.chromium.org/1441393002/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/google_captcha_observer.h:16: enum GoogleCaptchaEvent { On 2015/11/16 16:54:59, Alexei Svitkine (slow) wrote: > Nit: This can probably live in the .cc if you're not using it elsewhere. Done. https://codereview.chromium.org/1441393002/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/google_captcha_observer.h:38: DISALLOW_COPY_AND_ASSIGN(GoogleCaptchaObserver); On 2015/11/16 16:54:59, Alexei Svitkine (slow) wrote: > Nit: Add an empty line above this. Done. Is there a tool to format code according to Chrome style guidelines? (The google3 C++ formatting seems to be fairly different.)
bmcquade@chromium.org changed reviewers: + bmcquade@chromium.org
https://codereview.chromium.org/1441393002/diff/140001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/google_captcha_observer.h (right): https://codereview.chromium.org/1441393002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/google_captcha_observer.h:14: bool IsGoogleCaptcha(const GURL& url); re: removing the namespace, could we make this a static method on the GoogleCaptchaObserver class? and then you can put GoogleCaptchaObserver in the page_load_metrics namespace. FromGWSObserver should probably also be in page_load_metrics. I can fix that in a follow up change.
lgtm % nit. https://codereview.chromium.org/1441393002/diff/120001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/google_captcha_observer.cc (right): https://codereview.chromium.org/1441393002/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/google_captcha_observer.cc:58: } // namespace google_captcha_observer On 2015/11/16 18:30:53, Matt Welsh wrote: > On 2015/11/14 01:19:55, csharrison wrote: > > nit: two spaces between } and // (ditto other places where this occurs) > > Done. How do I autoformat my code to adhere to Chromium style? "git cl format" should do it. See https://code.google.com/p/chromium/codesearch#chromium/src/docs/clang_format.... https://codereview.chromium.org/1441393002/diff/120001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/google_captcha_observer.h (right): https://codereview.chromium.org/1441393002/diff/120001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/google_captcha_observer.h:11: namespace google_captcha_observer { On 2015/11/16 18:30:53, Matt Welsh wrote: > On 2015/11/16 16:54:59, Alexei Svitkine (slow) wrote: > > I'll leave it up to owners of this, but I'm not sure it makes sense to have > its > > own namespace for this class. It's more conventional to use the same namespace > > as the overall component. (Unless they owners prefer this style?) > > So I'm happy to do whatever makes sense. There is no existing namespace for > other classes in this directory -- from_gws_page_load_metrics_observer.cc uses > the global namespace. Since I'm adding the IsGoogleCaptcha function, I feel that > this should not be global and needs to be nested in some namespace, but I don't > particularly care which -- Charles, do you have a suggestion? I'm fine with the google_captcha_observer namespace. I may add another namespace to all the observers in some later refactor though. https://codereview.chromium.org/1441393002/diff/140001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/google_captcha_observer.h (right): https://codereview.chromium.org/1441393002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/google_captcha_observer.h:14: bool IsGoogleCaptcha(const GURL& url); On 2015/11/16 18:58:55, Bryan McQuade wrote: > re: removing the namespace, could we make this a static method on the > GoogleCaptchaObserver class? and then you can put GoogleCaptchaObserver in the > page_load_metrics namespace. FromGWSObserver should probably also be in > page_load_metrics. I can fix that in a follow up change. There was a discussion about this in the original observers CL that page_load_metrics namespace should only be used in the component. I would be happy to go against that advice, but we could also have a solution that adds a special namespace for observer / chrome. https://codereview.chromium.org/1441393002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/google_captcha_observer.h:33: } // namespace google_captcha_observer nit: space after }. I just tested git cl format and it doesn't detect this. Let's just add it here to be consistent.
> I'm fine with the google_captcha_observer namespace. I may add another namespace > to all the observers in some later refactor though. OK, leaving this as-is and you're welcome to refactor :-) > https://codereview.chromium.org/1441393002/diff/140001/chrome/browser/page_lo... > chrome/browser/page_load_metrics/observers/google_captcha_observer.h:33: } // > namespace google_captcha_observer > nit: space after }. I just tested git cl format and it doesn't detect this. > Let's just add it here to be consistent. Done.
The CQ bit was checked by mdw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, rdsmith@chromium.org, csharrison@chromium.org Link to the patchset: https://codereview.chromium.org/1441393002/#ps160001 (title: "Fix nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1441393002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1441393002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
The CQ bit was checked by mdw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, rdsmith@chromium.org, csharrison@chromium.org Link to the patchset: https://codereview.chromium.org/1441393002/#ps200001 (title: "Rebasing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1441393002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1441393002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, no build URL)
The CQ bit was checked by mdw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, rdsmith@chromium.org, csharrison@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1441393002/#ps220001 (title: "Small fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1441393002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1441393002/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/71c7cacbdb9cd4fa161909a72a9157f574a2b775 Cr-Commit-Position: refs/heads/master@{#360011}
Message was sent while issue was closed.
Description was changed from ========== Add UMA to count the number of Google CAPTCHA pages shown and solved by users. This is intended to track down problems where a large number of mobile users in certain countries are apparently seeing CAPTCHA pages. (http://b2.corp.google.com/23081866 for context.) I realize this may not be the ideal place for this code - happy for suggestions as to where else this might go. BUG=552470 Committed: https://crrev.com/71c7cacbdb9cd4fa161909a72a9157f574a2b775 Cr-Commit-Position: refs/heads/master@{#360011} ========== to ========== Add UMA to count the number of Google CAPTCHA pages shown and solved by users. This is intended to track down problems where a large number of mobile users in certain countries are apparently seeing CAPTCHA pages. (http://b/23081866 for context.) I realize this may not be the ideal place for this code - happy for suggestions as to where else this might go. BUG=552470 Committed: https://crrev.com/71c7cacbdb9cd4fa161909a72a9157f574a2b775 Cr-Commit-Position: refs/heads/master@{#360011} ========== |
