Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2451293002:
Add a histogram for each time the HTTP Bad UI is shown (Closed)
DescriptionAdd a histogram for each time the HTTP Bad UI is shown
Log a sample each time a HTTP Bad UI warning is shown in either the console or
the omnibox.
BUG=647567
Committed: https://crrev.com/c346a85602ff22ead6e8ce22f370267e3f192230
Cr-Commit-Position: refs/heads/master@{#428441}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Correct style and nits #
Total comments: 5
Patch Set 3 : More nits #
Total comments: 1
Patch Set 4 : DISALLOW_COPY_AND_ASSIGN for test fixture #
Total comments: 4
Patch Set 5 : Update histogram description text and enum #Patch Set 6 : Update VisibleSSLStateChange to VisibleSecurityStateChange #
Messages
Total messages: 36 (19 generated)
elawrence@chromium.org changed reviewers: + estark@chromium.org
PTAL, thanks!
The CQ bit was checked by elawrence@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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Thanks, looks good overall, just some style stuff + nits inline. https://codereview.chromium.org/2451293002/diff/1/chrome/browser/ssl/chrome_s... File chrome/browser/ssl/chrome_security_state_model_client.cc (right): https://codereview.chromium.org/2451293002/diff/1/chrome/browser/ssl/chrome_s... chrome/browser/ssl/chrome_security_state_model_client.cc:347: UMA_HISTOGRAM_BOOLEAN("Security.HTTPBad.UserWarnedAboutSensitiveInput", I'm not 100% sure, but I think, because of https://cs.chromium.org/chromium/src/base/metrics/histogram_macros.h?q=UMA_HI..., you're not supposed to use different strings to record the same histogram? When you add a histograms.xml owner they'll tell you for sure. It might be simpler in any case to do: bool histogram_sample = false; switch (security_info.security_level) { // set histogram_sample in here } UMA_HISTOGRAM_BOOLEAN("Security.HTTPBad.UserWarnedAboutSensitiveInput", histogram_sample); https://codereview.chromium.org/2451293002/diff/1/chrome/browser/ssl/chrome_s... File chrome/browser/ssl/chrome_security_state_model_client_unittest.cc (right): https://codereview.chromium.org/2451293002/diff/1/chrome/browser/ssl/chrome_s... chrome/browser/ssl/chrome_security_state_model_client_unittest.cc:266: void signalPassword() { style: should be signal_password https://codereview.chromium.org/2451293002/diff/1/chrome/browser/ssl/chrome_s... chrome/browser/ssl/chrome_security_state_model_client_unittest.cc:271: void navigateToHTTP() { NavigateAndCommit(GURL("http://example.test")); } style: should be navigate_to_http https://codereview.chromium.org/2451293002/diff/1/chrome/browser/ssl/chrome_s... chrome/browser/ssl/chrome_security_state_model_client_unittest.cc:273: ChromeSecurityStateModelClient* client; style: should be client_ Also non-private data members are technically style guide violations, though you'll occasionally see tests with protected members. It really should be private with a protected client() accessor method. https://codereview.chromium.org/2451293002/diff/1/chrome/browser/ssl/chrome_s... chrome/browser/ssl/chrome_security_state_model_client_unittest.cc:274: const char* kHTTPBadHistogram = No need for this to be a class member, I'd just put it in the anonymous namespace at the top of the file (line 22) https://codereview.chromium.org/2451293002/diff/1/chrome/browser/ssl/chrome_s... chrome/browser/ssl/chrome_security_state_model_client_unittest.cc:282: // Show Warning Chip nit: end comments with periods, here and below. It's in the style guide: https://google.github.io/styleguide/cppguide.html#Punctuation,_Spelling_and_G.... which I reference only to prove that it's not my own personal pedantry :P https://codereview.chromium.org/2451293002/diff/1/chrome/browser/ssl/chrome_s... chrome/browser/ssl/chrome_security_state_model_client_unittest.cc:289: histograms.ExpectUniqueSample(kHTTPBadHistogram, 1, 1); I think `true` instead of `1` for the second arg would be clearer. (1 made me think it's an enum value.) https://codereview.chromium.org/2451293002/diff/1/chrome/browser/ssl/chrome_s... chrome/browser/ssl/chrome_security_state_model_client_unittest.cc:312: histograms.ExpectUniqueSample(kHTTPBadHistogram, 0, 1); same comment about false instead of 0 https://codereview.chromium.org/2451293002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2451293002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:56101: + Counts the number of times the "Not secure" warning was shown in "the number of times" => "the number of main-frame navigations on which" (to emphasize that it's recorded at most once per main-frame nav)
The CQ bit was checked by elawrence@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...
thanks! https://codereview.chromium.org/2451293002/diff/1/chrome/browser/ssl/chrome_s... File chrome/browser/ssl/chrome_security_state_model_client_unittest.cc (right): https://codereview.chromium.org/2451293002/diff/1/chrome/browser/ssl/chrome_s... chrome/browser/ssl/chrome_security_state_model_client_unittest.cc:266: void signalPassword() { On 2016/10/26 20:30:56, estark wrote: > style: should be signal_password Done. https://codereview.chromium.org/2451293002/diff/1/chrome/browser/ssl/chrome_s... chrome/browser/ssl/chrome_security_state_model_client_unittest.cc:271: void navigateToHTTP() { NavigateAndCommit(GURL("http://example.test")); } On 2016/10/26 20:30:56, estark wrote: > style: should be navigate_to_http Done. https://codereview.chromium.org/2451293002/diff/1/chrome/browser/ssl/chrome_s... chrome/browser/ssl/chrome_security_state_model_client_unittest.cc:273: ChromeSecurityStateModelClient* client; On 2016/10/26 20:30:56, estark wrote: > style: should be client_ > > Also non-private data members are technically style guide violations, though > you'll occasionally see tests with protected members. It really should be > private with a protected client() accessor method. Done. https://codereview.chromium.org/2451293002/diff/1/chrome/browser/ssl/chrome_s... chrome/browser/ssl/chrome_security_state_model_client_unittest.cc:274: const char* kHTTPBadHistogram = On 2016/10/26 20:30:56, estark wrote: > No need for this to be a class member, I'd just put it in the anonymous > namespace at the top of the file (line 22) Done. https://codereview.chromium.org/2451293002/diff/1/chrome/browser/ssl/chrome_s... chrome/browser/ssl/chrome_security_state_model_client_unittest.cc:282: // Show Warning Chip On 2016/10/26 20:30:56, estark wrote: > nit: end comments with periods, here and below. It's in the style guide: > https://google.github.io/styleguide/cppguide.html#Punctuation,_Spelling_and_G.... > which I reference only to prove that it's not my own personal pedantry :P Done. https://codereview.chromium.org/2451293002/diff/1/chrome/browser/ssl/chrome_s... chrome/browser/ssl/chrome_security_state_model_client_unittest.cc:289: histograms.ExpectUniqueSample(kHTTPBadHistogram, 1, 1); On 2016/10/26 20:30:56, estark wrote: > I think `true` instead of `1` for the second arg would be clearer. (1 made me > think it's an enum value.) Done. https://codereview.chromium.org/2451293002/diff/1/chrome/browser/ssl/chrome_s... chrome/browser/ssl/chrome_security_state_model_client_unittest.cc:312: histograms.ExpectUniqueSample(kHTTPBadHistogram, 0, 1); On 2016/10/26 20:30:56, estark wrote: > same comment about false instead of 0 Done.
elawrence@chromium.org changed reviewers: + ericwilligers@chromium.org
elawrence@chromium.org changed required reviewers: + ericwilligers@chromium.org
ericwilligers@: Please review histograms.xml. estark@: PTAL at my updates. Thanks!
lgtm with a couple nits and notes https://codereview.chromium.org/2451293002/diff/20001/chrome/browser/ssl/chro... File chrome/browser/ssl/chrome_security_state_model_client.cc (right): https://codereview.chromium.org/2451293002/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client.cc:351: case security_state::SecurityStateModel::DANGEROUS: Two notes about this (no changes needed): 1. One could argue that this change belongs in a separate CL because it changes functionality other than the main thing you're doing in this CL (which is adding the histogram). Since it's so simple, I think it's fine to leave it here, just making a note of it. 2. One could also argue that there should be a browser test for a console message on DANGEROUS, along the lines of https://cs.chromium.org/chromium/src/chrome/browser/ssl/chrome_security_state.... However... those tests are so long and ugly that I can't bear the thought of adding another one. So I filed a bug for this missing test (https://crbug.com/659801), blocking on a change that will allow these tests to be much simpler (https://crbug.com/658099) https://codereview.chromium.org/2451293002/diff/20001/chrome/browser/ssl/chro... File chrome/browser/ssl/chrome_security_state_model_client_unittest.cc (right): https://codereview.chromium.org/2451293002/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client_unittest.cc:12: #include "content/public/browser/navigation_entry.h" nit: are this and ssl_status.h needed? https://codereview.chromium.org/2451293002/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client_unittest.cc:270: void signal_password() { nit: blank line above this
https://codereview.chromium.org/2451293002/diff/20001/chrome/browser/ssl/chro... File chrome/browser/ssl/chrome_security_state_model_client_unittest.cc (right): https://codereview.chromium.org/2451293002/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client_unittest.cc:12: #include "content/public/browser/navigation_entry.h" On 2016/10/26 21:29:46, estark wrote: > nit: are this and ssl_status.h needed? Nope; stale from when I was fumbling about earlier. https://codereview.chromium.org/2451293002/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client_unittest.cc:270: void signal_password() { On 2016/10/26 21:29:46, estark wrote: > nit: blank line above this Done.
https://codereview.chromium.org/2451293002/diff/40001/chrome/browser/ssl/chro... File chrome/browser/ssl/chrome_security_state_model_client_unittest.cc (right): https://codereview.chromium.org/2451293002/diff/40001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client_unittest.cc:277: ChromeSecurityStateModelClient* client_; Sorry, one more nit I just noticed, this should have DISALLOW_COPY_AND_ASSIGN(ChromeSSMClientHistogramTest) at the end of the private section.
Description was changed from ========== Add a histogram for each time the HTTP Bad UI is shown Log a sample each time a HTTP Bad UI warning is shown in either the console or the omnibox. BUG=647567 ========== to ========== Add a histogram for each time the HTTP Bad UI is shown Log a sample each time a HTTP Bad UI warning is shown in either the console or the omnibox. BUG=647567 ==========
ericwilligers@chromium.org changed reviewers: + rkaplow@chromium.org - ericwilligers@chromium.org
ericwilligers@chromium.org changed required reviewers: - ericwilligers@chromium.org
On 2016/10/26 21:23:38, elawrence wrote: > ericwilligers@: Please review histograms.xml. This isn't a trivial UseCounter histogram change generated by running a update_use_counter script. Added rkaplow@ from tools/metrics/OWNERS for the histogram review.
Added DISALLOW_COPY_AND_ASSIGN to test fixture.
https://codereview.chromium.org/2451293002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2451293002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:56104: +<histogram name="Security.HTTPBad.UserWarnedAboutSensitiveInput"> should have enum="Boolean" https://codereview.chromium.org/2451293002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:56108: + Counts the number of main-frame navigations on which a "Not I wouldn't describe this as "counts the number of times" since it is a boolean. I would describe it given a single instance of an event.
Thanks, rkaplow@! PTAL at the updated version? https://codereview.chromium.org/2451293002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2451293002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:56104: +<histogram name="Security.HTTPBad.UserWarnedAboutSensitiveInput"> On 2016/10/28 01:07:31, rkaplow wrote: > should have enum="Boolean" Can I use "BooleanShown" to match what we're really tracking? https://codereview.chromium.org/2451293002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:56108: + Counts the number of main-frame navigations on which a "Not On 2016/10/28 01:07:31, rkaplow wrote: > I wouldn't describe this as "counts the number of times" since it is a boolean. > I would describe it given a single instance of an event. Done.
lgtm yep, BooleanShown is fine
The CQ bit was checked by elawrence@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org Link to the patchset: https://codereview.chromium.org/2451293002/#ps80001 (title: "Update histogram description text and enum")
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_compile_dbg_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 elawrence@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org, estark@chromium.org Link to the patchset: https://codereview.chromium.org/2451293002/#ps100001 (title: "Update VisibleSSLStateChange to VisibleSecurityStateChange")
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.
Description was changed from ========== Add a histogram for each time the HTTP Bad UI is shown Log a sample each time a HTTP Bad UI warning is shown in either the console or the omnibox. BUG=647567 ========== to ========== Add a histogram for each time the HTTP Bad UI is shown Log a sample each time a HTTP Bad UI warning is shown in either the console or the omnibox. BUG=647567 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Add a histogram for each time the HTTP Bad UI is shown Log a sample each time a HTTP Bad UI warning is shown in either the console or the omnibox. BUG=647567 ========== to ========== Add a histogram for each time the HTTP Bad UI is shown Log a sample each time a HTTP Bad UI warning is shown in either the console or the omnibox. BUG=647567 Committed: https://crrev.com/c346a85602ff22ead6e8ce22f370267e3f192230 Cr-Commit-Position: refs/heads/master@{#428441} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/c346a85602ff22ead6e8ce22f370267e3f192230 Cr-Commit-Position: refs/heads/master@{#428441} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
