Experience sampling instrumentation for dangerous downloads warnings

Experience sampling instrumentation for dangerous downloads warnings

BUG=384635
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290076

[Chris Thompson, 2014-07-21 23:40:58]

felt@, here's the partial CL for instrumenting the downloads warnings. Could you
let me know what you think of this approach and if you have ideas for how this
might be done better? Thanks.

[Chris Thompson, 2014-07-24 20:35:08]

On 2014/07/21 23:40:58, Chris Thompson wrote:
> felt@, here's the partial CL for instrumenting the downloads warnings. Could
you
> let me know what you think of this approach and if you have ideas for how this
> might be done better? Thanks.

Friendly ping. Any thoughts on the design here? Otherwise I'll go ahead and port
this to the Cocoa version of the views as well.

[felt, 2014-07-25 17:36:50]

This seems like a reasonable approach.

You'll also want to add to:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/dow...

And to:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
(to the "saveDangerous" handler)

https://codereview.chromium.org/402293002/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/download/download_item_view.cc (right):

https://codereview.chromium.org/402293002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/download/download_item_view.cc:239:
sampling_event_->CreateUserDecisionEvent("deny");
Can you update the comment to say something like:

// Experience Sampling: If user took no action to remove the warning before it
disappeared, then the user
// effectively dismissed the download without keeping it.

[felt, 2014-07-25 21:23:21]

https://codereview.chromium.org/402293002/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/download/download_item_view.cc (right):

https://codereview.chromium.org/402293002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/download/download_item_view.cc:1138:
sampling_event_->CreateUserDecisionEvent("proceed");
P.S. remember that you defined constants for "proceed" and "deny"

[Chris Thompson, 2014-08-11 19:35:16]

I added instrumentation to the DownloadDangerPromptImpl.

DownloadDangerPromptImpl should cover the "saving malicious download" case, and
HandleDiscardDangerous should cause the download shelf item instrumentation to
trigger. I'll test this, but for now I'll skip adding extra instrumentation.

https://codereview.chromium.org/402293002/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/download/download_item_view.cc (right):

https://codereview.chromium.org/402293002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/download/download_item_view.cc:239:
sampling_event_->CreateUserDecisionEvent("deny");
On 2014/07/25 17:36:49, felt wrote:
> Can you update the comment to say something like:
> 
> // Experience Sampling: If user took no action to remove the warning before it
> disappeared, then the user
> // effectively dismissed the download without keeping it.

Done.

https://codereview.chromium.org/402293002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/download/download_item_view.cc:1138:
sampling_event_->CreateUserDecisionEvent("proceed");
On 2014/07/25 21:23:21, felt wrote:
> P.S. remember that you defined constants for "proceed" and "deny"

Done.

[felt, 2014-08-11 21:57:27]

this looks fine to me at a first pass, please fix nits and send to appropriate
reviewers

https://codereview.chromium.org/402293002/diff/80001/chrome/browser/download/...
File chrome/browser/download/download_danger_prompt.cc (right):

https://codereview.chromium.org/402293002/diff/80001/chrome/browser/download/...
chrome/browser/download/download_danger_prompt.cc:25: // Constants for the
Experience Sampling instrumentation.
Constant

https://codereview.chromium.org/402293002/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/download/download_item_controller.mm (right):

https://codereview.chromium.org/402293002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/download/download_item_controller.mm:153: std::string
event_name = downloadModel->MightBeMalcious() ?
MightBeMalicious()

[Chris Thompson, 2014-08-11 22:00:50]

https://codereview.chromium.org/402293002/diff/80001/chrome/browser/download/...
File chrome/browser/download/download_danger_prompt.cc (right):

https://codereview.chromium.org/402293002/diff/80001/chrome/browser/download/...
chrome/browser/download/download_danger_prompt.cc:25: // Constants for the
Experience Sampling instrumentation.
On 2014/08/11 21:57:27, felt wrote:
> Constant

Done.

https://codereview.chromium.org/402293002/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/download/download_item_controller.mm (right):

https://codereview.chromium.org/402293002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/download/download_item_controller.mm:153: std::string
event_name = downloadModel->MightBeMalcious() ?
On 2014/08/11 21:57:27, felt wrote:
> MightBeMalicious() 

Done.

[Chris Thompson, 2014-08-13 16:59:59]

For various OWNERS approval, could you PTAL?

msw@ for chrome/browser/ui/views/*
groby@ for chrome/browser/ui/cocoa/*
asanka@ for chrome/browser/downloads/*

Thanks!

[msw, 2014-08-13 20:11:38]

I have no idea why extensions would care about whether users respond to
dangerous download prompts and don't understand the justification or nuances of
this entire Experience Sampling feature. But why aren't we just exposing some
UMA as needed? It seems odd to add an entirely new (and duplicate) user metrics
reporting system. (imagine features needing to add UMA and Experience
Sampling...). I really hope UMA folks have taken a look at this whole concept,
and security has done a review for reporting metrics to extension.

https://codereview.chromium.org/402293002/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view.cc (right):

https://codereview.chromium.org/402293002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view.cc:98: const char
kEventNameMalicious[] = "download_warning_malicious";
These string constants should be shared between Cocoa and Views.
Are we doing the same thing for Android and iOS? Ditto there then.

https://codereview.chromium.org/402293002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view.cc:240: // Experience
Sampling: If the user took no action to remove the warning
What's the use for this info? Wouldn't allow/deny/ignore be more informative?

https://codereview.chromium.org/402293002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view.cc:1142: //
ExperienceSampling: User proceeded through the warning.
Use "Experience Sampling" or "ExperienceSampling" consistently everywhere.

https://codereview.chromium.org/402293002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view.cc:1192: :
kEventNameDangerous;
This formatting is incorrect, use "git cl format"
https://code.google.com/p/chromium/wiki/ClangFormat
I'd probably just combine this line with the one above.

https://codereview.chromium.org/402293002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view.cc:1193:
sampling_event_.reset(new ExperienceSamplingEvent(
Yikes, I really hope this has undergone security review...

https://codereview.chromium.org/402293002/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view.h (right):

https://codereview.chromium.org/402293002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view.h:346: //
ExperienceSampling: This maintains event state.
This comment doesn't really add much value... explain more for the uninformed?

https://codereview.chromium.org/402293002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view.h:347:
scoped_ptr<extensions::ExperienceSamplingEvent> sampling_event_;
Why isn't all this done at the cross-platform DownloadItemModelData level?
(that might also remove the need for DownloadDangerPromptImpl changes?)

[Alexei Svitkine (slow), 2014-08-13 20:39:24]

I haven't heard of this. Is there a design doc somewhere?


On Wed, Aug 13, 2014 at 4:11 PM, <msw@chromium.org> wrote:

> I have no idea why extensions would care about whether users respond to
> dangerous download prompts and don't understand the justification or
> nuances of
> this entire Experience Sampling feature. But why aren't we just exposing
> some
> UMA as needed? It seems odd to add an entirely new (and duplicate) user
> metrics
> reporting system. (imagine features needing to add UMA and Experience
> Sampling...). I really hope UMA folks have taken a look at this whole
> concept,
> and security has done a review for reporting metrics to extension.
>
>
> https://codereview.chromium.org/402293002/diff/120001/
> chrome/browser/ui/views/download/download_item_view.cc
> File chrome/browser/ui/views/download/download_item_view.cc (right):
>
> https://codereview.chromium.org/402293002/diff/120001/
> chrome/browser/ui/views/download/download_item_view.cc#newcode98
> chrome/browser/ui/views/download/download_item_view.cc:98: const char
> kEventNameMalicious[] = "download_warning_malicious";
> These string constants should be shared between Cocoa and Views.
> Are we doing the same thing for Android and iOS? Ditto there then.
>
> https://codereview.chromium.org/402293002/diff/120001/
> chrome/browser/ui/views/download/download_item_view.cc#newcode240
> chrome/browser/ui/views/download/download_item_view.cc:240: //
> Experience Sampling: If the user took no action to remove the warning
> What's the use for this info? Wouldn't allow/deny/ignore be more
> informative?
>
> https://codereview.chromium.org/402293002/diff/120001/
> chrome/browser/ui/views/download/download_item_view.cc#newcode1142
> chrome/browser/ui/views/download/download_item_view.cc:1142: //
> ExperienceSampling: User proceeded through the warning.
> Use "Experience Sampling" or "ExperienceSampling" consistently
> everywhere.
>
> https://codereview.chromium.org/402293002/diff/120001/
> chrome/browser/ui/views/download/download_item_view.cc#newcode1192
> chrome/browser/ui/views/download/download_item_view.cc:1192: :
> kEventNameDangerous;
> This formatting is incorrect, use "git cl format"
> https://code.google.com/p/chromium/wiki/ClangFormat
> I'd probably just combine this line with the one above.
>
> https://codereview.chromium.org/402293002/diff/120001/
> chrome/browser/ui/views/download/download_item_view.cc#newcode1193
> chrome/browser/ui/views/download/download_item_view.cc:1193:
> sampling_event_.reset(new ExperienceSamplingEvent(
> Yikes, I really hope this has undergone security review...
>
> https://codereview.chromium.org/402293002/diff/120001/
> chrome/browser/ui/views/download/download_item_view.h
> File chrome/browser/ui/views/download/download_item_view.h (right):
>
> https://codereview.chromium.org/402293002/diff/120001/
> chrome/browser/ui/views/download/download_item_view.h#newcode346
> chrome/browser/ui/views/download/download_item_view.h:346: //
> ExperienceSampling: This maintains event state.
> This comment doesn't really add much value... explain more for the
> uninformed?
>
> https://codereview.chromium.org/402293002/diff/120001/
> chrome/browser/ui/views/download/download_item_view.h#newcode347
> chrome/browser/ui/views/download/download_item_view.h:347:
> scoped_ptr<extensions::ExperienceSamplingEvent> sampling_event_;
> Why isn't all this done at the cross-platform DownloadItemModelData
> level?
> (that might also remove the need for DownloadDangerPromptImpl changes?)
>
> https://codereview.chromium.org/402293002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[groby-ooo-7-16, 2014-08-13 20:40:14]

Travelling today, will address tomorrow

[asanka, 2014-08-13 20:59:30]

https://codereview.chromium.org/402293002/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view.h (right):

https://codereview.chromium.org/402293002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view.h:347:
scoped_ptr<extensions::ExperienceSamplingEvent> sampling_event_;
Yeah. chrome://downloads also provides a UI surface. It's possible for a user to
open the downloads page from the shelf and then accept/reject the download
there.

What's the lifetime of the sampling_event_ supposed to be?

[Chris Thompson, 2014-08-13 21:18:00]

This API is to allow the Chrome Security UX team to perform experience sampling
of users -- that is, to show surveys to users immediately after they see and
make a decision about security UI in Chrome. It is a whitelist-only private API
-- it will not be made available to other extensions.

Merely adding more UMA metrics is not sufficient -- we can't trigger surveys
with UMA. By exposing these events to a whitelisted extension, we can show
surveys and gather much more in-depth information about their decision process
and comprehension. In the long run, this may be merged more closely with
UMA/Finch.

This API has undergone extensions API and security review. felt@ has been giving
overall review of the related CLs and giving security review.

The overall design doc is
https://docs.google.com/a/google.com/document/d/1CEc4-Pg9vJhMYw8g55E1WMf9JjKH...

The API proposal doc is
https://docs.google.com/a/chromium.org/document/d/1JxPMH5Q9TibQgtnmU_DbgeHwJ4...

https://codereview.chromium.org/402293002/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view.cc (right):

https://codereview.chromium.org/402293002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view.cc:98: const char
kEventNameMalicious[] = "download_warning_malicious";
On 2014/08/13 20:11:37, msw wrote:
> These string constants should be shared between Cocoa and Views.
> Are we doing the same thing for Android and iOS? Ditto there then.

Done. Moved these to experience_sampling.h.

https://codereview.chromium.org/402293002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view.cc:240: // Experience
Sampling: If the user took no action to remove the warning
On 2014/08/13 20:11:37, msw wrote:
> What's the use for this info? Wouldn't allow/deny/ignore be more informative?

Changed this to "ignore".

https://codereview.chromium.org/402293002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view.cc:1142: //
ExperienceSampling: User proceeded through the warning.
On 2014/08/13 20:11:37, msw wrote:
> Use "Experience Sampling" or "ExperienceSampling" consistently everywhere.

Done. s/Experience Sampling/ExperienceSampling/g

https://codereview.chromium.org/402293002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view.cc:1192: :
kEventNameDangerous;
On 2014/08/13 20:11:37, msw wrote:
> This formatting is incorrect, use "git cl format"
> https://code.google.com/p/chromium/wiki/ClangFormat
> I'd probably just combine this line with the one above.

Done. Missed this in an edit that shortened the line.

https://codereview.chromium.org/402293002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view.cc:1193:
sampling_event_.reset(new ExperienceSamplingEvent(
On 2014/08/13 20:11:37, msw wrote:
> Yikes, I really hope this has undergone security review...

Do you have particular concerns about this use of scoped_ptr here?

https://codereview.chromium.org/402293002/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view.h (right):

https://codereview.chromium.org/402293002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view.h:346: //
ExperienceSampling: This maintains event state.
On 2014/08/13 20:11:38, msw wrote:
> This comment doesn't really add much value... explain more for the uninformed?

Good point. I've rewritten the comment to explain what it's tracking.

https://codereview.chromium.org/402293002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view.h:347:
scoped_ptr<extensions::ExperienceSamplingEvent> sampling_event_;
On 2014/08/13 20:11:38, msw wrote:
> Why isn't all this done at the cross-platform DownloadItemModelData level?
> (that might also remove the need for DownloadDangerPromptImpl changes?)

Simpler, cross-platform implementation would be ideal, but there does not seem
to be a good way to track the UI state in that class. We need to be able to
detect when the dangerous/malicious download warning UI is shown, and when the
user responds to it. DownloadItem and DownloadItem::ValidateDangerousDownload
would work, but they're in content/, so we can't instrument them for extensions.

https://codereview.chromium.org/402293002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view.h:347:
scoped_ptr<extensions::ExperienceSamplingEvent> sampling_event_;
On 2014/08/13 20:59:30, asanka wrote:
> Yeah. chrome://downloads also provides a UI surface. It's possible for a user
to
> open the downloads page from the shelf and then accept/reject the download
> there.
> 
> What's the lifetime of the sampling_event_ supposed to be?

sampling_event_ should live until the user makes a decision about the warning
(dismisses the download or proceeds all the way through). I could make this more
explicit by null-ing out the scoped_ptr after CreateUserDecisionEvent() is
called.

If the user makes no decision about the UI, the sampling_event_'s lifetime is
tied to the download_item.

Note that we're separately instrumenting the native malicious download warning
prompt, although the results of that prompt (if the user proceeds through) could
also affect the DownloadItemView.

[msw, 2014-08-14 01:48:33]

I hope others will assess the overall ExperienceSampling feature plan/design,
that's not quite up my alley.

https://codereview.chromium.org/402293002/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view.cc (right):

https://codereview.chromium.org/402293002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view.cc:1193:
sampling_event_.reset(new ExperienceSamplingEvent(
On 2014/08/13 21:18:00, Chris Thompson wrote:
> On 2014/08/13 20:11:37, msw wrote:
> > Yikes, I really hope this has undergone security review...
> 
> Do you have particular concerns about this use of scoped_ptr here?

No, my point was more about reporting url, referrer, and browser context info to
some extension. You whitelisting explanation addressed my security concerns.

https://codereview.chromium.org/402293002/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view.h (right):

https://codereview.chromium.org/402293002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view.h:347:
scoped_ptr<extensions::ExperienceSamplingEvent> sampling_event_;
On 2014/08/13 21:18:00, Chris Thompson wrote:
> On 2014/08/13 20:11:38, msw wrote:
> > Why isn't all this done at the cross-platform DownloadItemModelData level?
> > (that might also remove the need for DownloadDangerPromptImpl changes?)
> 
> Simpler, cross-platform implementation would be ideal, but there does not seem
> to be a good way to track the UI state in that class. We need to be able to
> detect when the dangerous/malicious download warning UI is shown, and when the
> user responds to it. DownloadItem and DownloadItem::ValidateDangerousDownload
> would work, but they're in content/, so we can't instrument them for
extensions.

I wonder if it'd take just a couple new DownloadItemModelData::OnXYZ functions
to support your cross-platform impl, but I'm not adamant that this be done, so
long as you cover all the UI cases that are needed (including
chrome://downloads, Android, iOS, etc.).

https://codereview.chromium.org/402293002/diff/140001/chrome/browser/extensio...
File
chrome/browser/extensions/api/experience_sampling_private/experience_sampling.cc
(right):

https://codereview.chromium.org/402293002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/experience_sampling_private/experience_sampling.cc:17:
// String constants for user decision events.
nit: Move these comments to the header with the decls.

https://codereview.chromium.org/402293002/diff/140001/chrome/browser/extensio...
File
chrome/browser/extensions/api/experience_sampling_private/experience_sampling.h
(right):

https://codereview.chromium.org/402293002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/experience_sampling_private/experience_sampling.h:28:
static const char kEventNameMaliciousDownload[];
nit: Maybe add a blank line between these and the response type strings above?

https://codereview.chromium.org/402293002/diff/140001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/download/download_item_controller.mm (right):

https://codereview.chromium.org/402293002/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/download/download_item_controller.mm:155: ?
ExperienceSamplingEvent::kEventNameMaliciousDownload
Wow, is this what git cl format does? I'm surprised.

https://codereview.chromium.org/402293002/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view.cc (right):

https://codereview.chromium.org/402293002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view.cc:1139:
sampling_event_->CreateUserDecisionEvent(ExperienceSamplingEvent::kProceed);
Can the user explicitly deny in Views like Cocoa's dismissMaliciousDownload?

[Chris Thompson, 2014-08-14 15:43:44]

https://codereview.chromium.org/402293002/diff/140001/chrome/browser/extensio...
File
chrome/browser/extensions/api/experience_sampling_private/experience_sampling.cc
(right):

https://codereview.chromium.org/402293002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/experience_sampling_private/experience_sampling.cc:17:
// String constants for user decision events.
On 2014/08/14 01:48:33, msw wrote:
> nit: Move these comments to the header with the decls.

Done.

https://codereview.chromium.org/402293002/diff/140001/chrome/browser/extensio...
File
chrome/browser/extensions/api/experience_sampling_private/experience_sampling.h
(right):

https://codereview.chromium.org/402293002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/experience_sampling_private/experience_sampling.h:28:
static const char kEventNameMaliciousDownload[];
On 2014/08/14 01:48:33, msw wrote:
> nit: Maybe add a blank line between these and the response type strings above?

Done.

https://codereview.chromium.org/402293002/diff/140001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/download/download_item_controller.mm (right):

https://codereview.chromium.org/402293002/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/download/download_item_controller.mm:155: ?
ExperienceSamplingEvent::kEventNameMaliciousDownload
On 2014/08/14 01:48:33, msw wrote:
> Wow, is this what git cl format does? I'm surprised.

Yep. The line is just long enough to make any other line-break go past 80char.

https://codereview.chromium.org/402293002/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view.cc (right):

https://codereview.chromium.org/402293002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view.cc:1139:
sampling_event_->CreateUserDecisionEvent(ExperienceSamplingEvent::kProceed);
On 2014/08/14 01:48:33, msw wrote:
> Can the user explicitly deny in Views like Cocoa's dismissMaliciousDownload?

The dismiss button is handled in DownloadItemView::ButtonPressed.

[Chris Thompson, 2014-08-14 17:19:16]

https://codereview.chromium.org/402293002/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view.cc (right):

https://codereview.chromium.org/402293002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view.cc:1139:
sampling_event_->CreateUserDecisionEvent(ExperienceSamplingEvent::kProceed);
On 2014/08/14 15:43:44, Chris Thompson wrote:
> On 2014/08/14 01:48:33, msw wrote:
> > Can the user explicitly deny in Views like Cocoa's dismissMaliciousDownload?
> 
> The dismiss button is handled in DownloadItemView::ButtonPressed.

I've also reworked the instrumentation here a little bit to be more like the
Cocoa version, and added instrumentation to the ButtonPressed method.

[msw, 2014-08-14 17:56:44]

chrome/browser/ui/views/* lgtm with nit qs.

https://codereview.chromium.org/402293002/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view.cc (right):

https://codereview.chromium.org/402293002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view.cc:1139:
sampling_event_->CreateUserDecisionEvent(ExperienceSamplingEvent::kProceed);
On 2014/08/14 17:19:16, Chris Thompson wrote:
> On 2014/08/14 15:43:44, Chris Thompson wrote:
> > On 2014/08/14 01:48:33, msw wrote:
> > > Can the user explicitly deny in Views like Cocoa's
dismissMaliciousDownload?
> > 
> > The dismiss button is handled in DownloadItemView::ButtonPressed.
> 
> I've also reworked the instrumentation here a little bit to be more like the
> Cocoa version, and added instrumentation to the ButtonPressed method.

Cool, the the latest patch set looks more complete.

https://codereview.chromium.org/402293002/diff/160001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view.cc (right):

https://codereview.chromium.org/402293002/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view.cc:562: if
(sampling_event_.get()) {
nit q: Should this be a DCHECK instead?

https://codereview.chromium.org/402293002/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view.cc:577: if
(sampling_event_.get()) {
ditto nit q: Should this be a DCHECK instead?

https://codereview.chromium.org/402293002/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view.cc:1150: if
(sampling_event_.get()) {
ditto nit q: Should this be a DCHECK instead?

[Chris Thompson, 2014-08-14 20:00:03]

https://codereview.chromium.org/402293002/diff/160001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view.cc (right):

https://codereview.chromium.org/402293002/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view.cc:562: if
(sampling_event_.get()) {
On 2014/08/14 17:56:44, msw wrote:
> nit q: Should this be a DCHECK instead?

Given the possible flows of dangerous downloads UI, I'd rather not assume that
sampling_event_ can't get NULL here. In a way, this is gating on the first path
that triggers a decision event wins, but that doesn't necessarily prevent other
paths that could also trigger the event from happening (or I'm not sure enough
about all possible execution paths).

[msw, 2014-08-14 20:46:42]

https://codereview.chromium.org/402293002/diff/160001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view.cc (right):

https://codereview.chromium.org/402293002/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view.cc:562: if
(sampling_event_.get()) {
On 2014/08/14 20:00:03, Chris Thompson wrote:
> On 2014/08/14 17:56:44, msw wrote:
> > nit q: Should this be a DCHECK instead?
> 
> Given the possible flows of dangerous downloads UI, I'd rather not assume that
> sampling_event_ can't get NULL here. In a way, this is gating on the first
path
> that triggers a decision event wins, but that doesn't necessarily prevent
other
> paths that could also trigger the event from happening (or I'm not sure enough
> about all possible execution paths).

Acknowledged; it's fine as-is, but DCHECKs could expose unexpected  execution
paths, which could yield experience sampling (and actual UI) improvements.

[felt, 2014-08-14 22:23:02]

chrome/browser/extensions/api/experience_sampling_private/experience_sampling.{h|cc}
lgtm

[groby-ooo-7-16, 2014-08-14 23:02:19]

c/b/ui/cocoa lgtm

[asanka, 2014-08-15 15:53:32]

/download/ lgtm. I'll leave it up to you how you would like to interpret CANCEL
vs. DISMISS. If you have a preferred way to record "user wasn't compelled to
interact with the warning" that may be how you want to treat DISMISS.

https://codereview.chromium.org/402293002/diff/200001/chrome/browser/download...
File chrome/browser/download/download_danger_prompt.cc (right):

https://codereview.chromium.org/402293002/diff/200001/chrome/browser/download...
chrome/browser/download/download_danger_prompt.cc:232:
sampling_event_->CreateUserDecisionEvent(ExperienceSamplingEvent::kDeny);
DISMISS means that the user navigated away from the page (or closed the tab)
without taking any explicit action on the warning. We leave the download in the
'unaccepted' state until they either close the browser or accepts the danger
state via some other means. I'm not sure it should be treated the same as CANCEL
which explicitly cancels and removes the download.

[Chris Thompson, 2014-08-15 16:05:28]

https://codereview.chromium.org/402293002/diff/200001/chrome/browser/download...
File chrome/browser/download/download_danger_prompt.cc (right):

https://codereview.chromium.org/402293002/diff/200001/chrome/browser/download...
chrome/browser/download/download_danger_prompt.cc:232:
sampling_event_->CreateUserDecisionEvent(ExperienceSamplingEvent::kDeny);
On 2014/08/15 15:53:32, asanka wrote:
> DISMISS means that the user navigated away from the page (or closed the tab)
> without taking any explicit action on the warning. We leave the download in
the
> 'unaccepted' state until they either close the browser or accepts the danger
> state via some other means. I'm not sure it should be treated the same as
CANCEL
> which explicitly cancels and removes the download.

My manual testing before showed that "Cancel" in the danger prompt popup just
closes the dialog (the download is still shown in the WebUI and download shelf),
so I treated Cancel and Close the same.

[Chris Thompson, 2014-08-15 17:34:06]

The CQ bit was checked by cthomp@chromium.org

[commit-bot: I haz the power, 2014-08-15 17:35:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/cthomp@chromium.org/402293002/220001

[commit-bot: I haz the power, 2014-08-16 01:10:26]

Committed patchset #12 (220001) as 290076