Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2499243002:
Record time to navigation/tab-closed after HTTP-bad warning (Closed)
DescriptionRecord time to navigation/tab-closed after HTTP-bad warning
This CL adds UMA metrics to record the time after displaying a
password/credit-card field on an HTTP page until the user navigates or
closes the tab.
By comparing these metrics for users who see the omnibox warning and
those who only get a console warning, we hope to get some idea of how
the HTTP-bad omnibox warning affects user behavior.
BUG=663924
Committed: https://crrev.com/5c563b099036a96fc1e2b0ede70ec83d14012bc6
Cr-Commit-Position: refs/heads/master@{#433411}
Patch Set 1 #Patch Set 2 : fix test #
Total comments: 20
Patch Set 3 : add histograms.xml, elawrence comments #Patch Set 4 : rebase #Patch Set 5 : fix up rebase #
Total comments: 4
Patch Set 6 : elawrence comments #
Messages
Total messages: 41 (25 generated)
The CQ bit was checked by estark@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 checked by estark@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...
estark@chromium.org changed reviewers: + elawrence@chromium.org, lgarron@chromium.org
lgarron, elawrence, PTAL? Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/15 08:38:35, estark (slow thru Nov 18) wrote: > lgarron, elawrence, PTAL? Thanks! I have a few minor questions about the mechanics of this change, noted in comments. More generally, I'm interested in understanding what we expect the data collected will tell us. I worry that the "Time spent" metric seems likely to be a black box that we won't be able to make any sense of. A possibly more telling analysis would be "Pivoting based on whether the Omnibox warning is enabled or not, compare the submission rate of forms where SensitiveInput detection fires." That would more likely permit us to see whether there's a meaningful impact on submission rate based on whether the warning is shown.
https://codereview.chromium.org/2499243002/diff/20001/chrome/browser/ssl/chro... File chrome/browser/ssl/chrome_security_state_model_client.cc (right): https://codereview.chromium.org/2499243002/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client.cc:371: time_of_http_warning_on_current_navigation_ = base::Time::Now(); Do we really want to clear this? If a warning flashed in and then disappeared (e.g. on page load) but then later reappeared due to user-action, it seems like the more recent warning would be the more interesting one? https://codereview.chromium.org/2499243002/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client.cc:415: return; Is if (time_of_http_warning_on_current_navigation_.is_null() || !navigation_handle->IsInMainFrame() || navigation_handle->IsSamePage()) return; more readable? https://codereview.chromium.org/2499243002/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client.cc:417: // Record the time delta between when an HTTP warning was shown and I love comments. I worry that explaining what's happening here is not needed, but explaining WHY is. So maybe: // Record how quickly a user leaves a site after seeing a HTTPBad warning ? https://codereview.chromium.org/2499243002/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client.cc:424: time_of_http_warning_on_current_navigation_ = base::Time(); I don't understand why we reset this here? https://codereview.chromium.org/2499243002/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client.cc:437: void ChromeSecurityStateModelClient::WebContentsDestroyed() { This event only fires for the top-level frame, right? https://codereview.chromium.org/2499243002/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client.cc:445: UMA_HISTOGRAM_LONG_TIMES( "This histogram will only be recorded if the WebContents is destroyed before another navigation begins." How is that implemented? Why wouldn't this get logged as a part of teardown of the page after a navigation starts? https://codereview.chromium.org/2499243002/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client.cc:448: time_of_http_warning_on_current_navigation_ = base::Time(); I don't understand why we reset this here? https://codereview.chromium.org/2499243002/diff/20001/chrome/browser/ssl/chro... File chrome/browser/ssl/chrome_security_state_model_client_unittest.cc (right): https://codereview.chromium.org/2499243002/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client_unittest.cc:341: // Same as the test above, but ensuring that the histogram gets "test above" This comment feels fragile. Is it safe to assume ordering of the file will not change? https://codereview.chromium.org/2499243002/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client_unittest.cc:374: // Same as the test above, but ensuring that the histogram gets "test above" https://codereview.chromium.org/2499243002/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client_unittest.cc:376: // warning is not set. "warning is not set" Do we explicitly need to set the desired flag value, because presumably the command line default will eventually change?
The CQ bit was checked by estark@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...
(Discussion about the value of this metric ongoing in an email thread, will update this issue with a summary when it resolves) https://codereview.chromium.org/2499243002/diff/20001/chrome/browser/ssl/chro... File chrome/browser/ssl/chrome_security_state_model_client.cc (right): https://codereview.chromium.org/2499243002/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client.cc:371: time_of_http_warning_on_current_navigation_ = base::Time::Now(); On 2016/11/15 16:29:52, elawrence wrote: > Do we really want to clear this? If a warning flashed in and then disappeared > (e.g. on page load) but then later reappeared due to user-action, it seems like > the more recent warning would be the more interesting one? First of all, I remember being very convinced that this null check on line 370 is the right thing to do, but I don't remember why I thought that. :-/ The way the code is right now, whenever |logged_http_warning| is false, |time_of_http_warning| is null, which means line 370 should really be a DCHECK(time_of_http_warning_on_current_navigation.is_null()). Anyway, to your actual point... to do what you're suggesting (recording the time since the most recent HTTP-bad warning was shown), I think we'd have to keep some extra state around, like the values of |displayed_password_field_on_http| and |displayed_credit_card_field_on_http| from the most recent time VisibleSecurityStateChanged was called, to detect when the warning is appearing. Personally I'm having a hard enough time reasoning about when to set/clear the time without having even more state to think about, but it's probably doable if you feel strongly about it. https://codereview.chromium.org/2499243002/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client.cc:415: return; On 2016/11/15 16:29:52, elawrence wrote: > Is > > if (time_of_http_warning_on_current_navigation_.is_null() || > !navigation_handle->IsInMainFrame() || > navigation_handle->IsSamePage()) > return; > > more readable? Done. https://codereview.chromium.org/2499243002/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client.cc:417: // Record the time delta between when an HTTP warning was shown and On 2016/11/15 16:29:52, elawrence wrote: > I love comments. I worry that explaining what's happening here is not needed, > but explaining WHY is. So maybe: > > // Record how quickly a user leaves a site after seeing a HTTPBad warning > > ? Done. https://codereview.chromium.org/2499243002/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client.cc:424: time_of_http_warning_on_current_navigation_ = base::Time(); On 2016/11/15 16:29:52, elawrence wrote: > I don't understand why we reset this here? I find it easiest to reason about these histograms if we say that we record at most once per main-frame navigation. So to implement that, I'm setting the time at most once per navigation, recording the histogram only if the time is not null, and setting it to null after recording so that a time histogram doesn't get recorded again. I added a comment to try to explain this, lmk if it makes sense. https://codereview.chromium.org/2499243002/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client.cc:437: void ChromeSecurityStateModelClient::WebContentsDestroyed() { On 2016/11/15 16:29:53, elawrence wrote: > This event only fires for the top-level frame, right? Yeah, a WebContents usually is 1:1 with a tab. https://codereview.chromium.org/2499243002/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client.cc:445: UMA_HISTOGRAM_LONG_TIMES( On 2016/11/15 16:29:52, elawrence wrote: > "This histogram will only be recorded if the WebContents is destroyed before > another navigation begins." > > How is that implemented? Why wouldn't this get logged as a part of teardown of > the page after a navigation starts? If a navigation has begun before the WebContents is destroyed, then |time_of_http_warning| will be null (having been cleared on line 424) and we'll return on line 439. https://codereview.chromium.org/2499243002/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client.cc:448: time_of_http_warning_on_current_navigation_ = base::Time(); On 2016/11/15 16:29:52, elawrence wrote: > I don't understand why we reset this here? I guess this is unnecessary, removed. https://codereview.chromium.org/2499243002/diff/20001/chrome/browser/ssl/chro... File chrome/browser/ssl/chrome_security_state_model_client_unittest.cc (right): https://codereview.chromium.org/2499243002/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client_unittest.cc:341: // Same as the test above, but ensuring that the histogram gets On 2016/11/15 16:29:53, elawrence wrote: > "test above" > > This comment feels fragile. Is it safe to assume ordering of the file will not > change? Done. https://codereview.chromium.org/2499243002/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client_unittest.cc:374: // Same as the test above, but ensuring that the histogram gets On 2016/11/15 16:29:53, elawrence wrote: > "test above" Done. https://codereview.chromium.org/2499243002/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client_unittest.cc:376: // warning is not set. On 2016/11/15 16:29:53, elawrence wrote: > "warning is not set" > > Do we explicitly need to set the desired flag value, because presumably the > command line default will eventually change? Done.
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_...)
Ok, so from discussion from the email thread, the main value of this metric is to see if users get dramatically scared into immediately closing Chrome or leaving the site etc, though additional metrics might be useful (e.g. form submissions). Any additional objections or comments? (Also rebased on top of the ChromeSSMClient -> SecurityStateTabHelper refactor)
The CQ bit was checked by estark@chromium.org to run a CQ dry run
estark@chromium.org changed reviewers: + isherman@chromium.org
+isherman for histograms.xml
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/18 03:24:52, estark (slow thru Nov 18) wrote: > Ok, so from discussion from the email thread, the main value of this metric is > to see if users get dramatically scared into immediately closing Chrome or > leaving the site etc, though additional metrics might be useful (e.g. form > submissions). > > Any additional objections or comments? > > (Also rebased on top of the ChromeSSMClient -> SecurityStateTabHelper refactor) LGTM, although it looks like maybe patchset 3 was lost and patchset 4 was a rebase of patchset 2? I don't think there was anything critical in set #3 but it's not clear to me whether this was intentional?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) 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 estark@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...
On 2016/11/18 03:48:58, elawrence wrote: > On 2016/11/18 03:24:52, estark (slow thru Nov 18) wrote: > > Ok, so from discussion from the email thread, the main value of this metric is > > to see if users get dramatically scared into immediately closing Chrome or > > leaving the site etc, though additional metrics might be useful (e.g. form > > submissions). > > > > Any additional objections or comments? > > > > (Also rebased on top of the ChromeSSMClient -> SecurityStateTabHelper > refactor) > > LGTM, although it looks like maybe patchset 3 was lost and patchset 4 was a > rebase of patchset 2? I don't think there was anything critical in set #3 but > it's not clear to me whether this was intentional? Sorry about that, screwed up the rebase somehow. Should be fixed in patchset 5.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for updating! One possible typo, one question about future maintenance of the tests, otherwise still LGTM. https://codereview.chromium.org/2499243002/diff/80001/chrome/browser/ssl/secu... File chrome/browser/ssl/security_state_tab_helper.h (right): https://codereview.chromium.org/2499243002/diff/80001/chrome/browser/ssl/secu... chrome/browser/ssl/security_state_tab_helper.h:61: // at most once per main-frame navigation (the first that an HTTP Trivial: Missing word? "the first that" -> "the first time that" ? https://codereview.chromium.org/2499243002/diff/80001/chrome/browser/ssl/secu... File chrome/browser/ssl/security_state_tab_helper_unittest.cc (right): https://codereview.chromium.org/2499243002/diff/80001/chrome/browser/ssl/secu... chrome/browser/ssl/security_state_tab_helper_unittest.cc:123: // omnibox warning is not set. Do we need to explicitly write out the flag state we want for security_state::switches::kMarkHttpAs, to account for the fact that the default state of this flag will change later in 2017?
metrics lgtm, thanks
https://codereview.chromium.org/2499243002/diff/80001/chrome/browser/ssl/secu... File chrome/browser/ssl/security_state_tab_helper.h (right): https://codereview.chromium.org/2499243002/diff/80001/chrome/browser/ssl/secu... chrome/browser/ssl/security_state_tab_helper.h:61: // at most once per main-frame navigation (the first that an HTTP On 2016/11/18 17:43:30, elawrence wrote: > Trivial: Missing word? "the first that" -> "the first time that" ? Done. https://codereview.chromium.org/2499243002/diff/80001/chrome/browser/ssl/secu... File chrome/browser/ssl/security_state_tab_helper_unittest.cc (right): https://codereview.chromium.org/2499243002/diff/80001/chrome/browser/ssl/secu... chrome/browser/ssl/security_state_tab_helper_unittest.cc:123: // omnibox warning is not set. On 2016/11/18 17:43:30, elawrence wrote: > Do we need to explicitly write out the flag state we want for > security_state::switches::kMarkHttpAs, to account for the fact that the default > state of this flag will change later in 2017? Oops, sorry, lost that in the rebase too. :( Done.
The CQ bit was checked by estark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from elawrence@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2499243002/#ps100001 (title: "elawrence 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 estark@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.
Description was changed from ========== Record time to navigation/tab-closed after HTTP-bad warning This CL adds UMA metrics to record the time after displaying a password/credit-card field on an HTTP page until the user navigates or closes the tab. By comparing these metrics for users who see the omnibox warning and those who only get a console warning, we hope to get some idea of how the HTTP-bad omnibox warning affects user behavior. BUG=663924 ========== to ========== Record time to navigation/tab-closed after HTTP-bad warning This CL adds UMA metrics to record the time after displaying a password/credit-card field on an HTTP page until the user navigates or closes the tab. By comparing these metrics for users who see the omnibox warning and those who only get a console warning, we hope to get some idea of how the HTTP-bad omnibox warning affects user behavior. BUG=663924 Committed: https://crrev.com/5c563b099036a96fc1e2b0ede70ec83d14012bc6 Cr-Commit-Position: refs/heads/master@{#433411} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/5c563b099036a96fc1e2b0ede70ec83d14012bc6 Cr-Commit-Position: refs/heads/master@{#433411} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
