Experience sampling instrumentation for dangerous downloads warnings

chrome/browser/ui/views/download/download_item_view.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 //
 // A ChromeView that implements one download on the Download shelf.
 // Each DownloadItemView contains an application icon, a text label
 // indicating the download's file name, a text label indicating the
 // download's status (such as the number of bytes downloaded so far)
 // and a button for canceling an in progress download, or opening
 // the completed download.
@@ skipping 20 matching lines @@
 #include "content/public/browser/download_manager.h"
 #include "ui/gfx/animation/animation_delegate.h"
 #include "ui/gfx/font_list.h"
 #include "ui/views/context_menu_controller.h"
 #include "ui/views/controls/button/button.h"
 #include "ui/views/view.h"
 
 class DownloadShelfView;
 class DownloadShelfContextMenuView;
 
+namespace extensions {
+class ExperienceSamplingEvent;
+}
+
 namespace gfx {
 class Image;
 class ImageSkia;
 class SlideAnimation;
 }
 
 namespace views {
 class Label;
 class LabelButton;
 }
@@ skipping 281 matching lines @@
   scoped_ptr<DownloadShelfContextMenuView> context_menu_;
 
   // The name of this view as reported to assistive technology.
   base::string16 accessible_name_;
 
   // The icon loaded in the download shelf is based on the file path of the
   // item.  Store the path used, so that we can detect a change in the path
   // and reload the icon.
   base::FilePath last_download_item_path_;
 
+  // ExperienceSampling: This maintains event state.

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

This comment doesn't really add much value... explain more for the uninformed?

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

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

+  scoped_ptr<extensions::ExperienceSamplingEvent> sampling_event_;

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

Why isn't all this done at the cross-platform DownloadItemModelData level?
(that might also remove the need for DownloadDangerPromptImpl changes?)

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

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]

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.

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

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.

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

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.).

+
   DISALLOW_COPY_AND_ASSIGN(DownloadItemView);
 };
 
 #endif  // CHROME_BROWSER_UI_VIEWS_DOWNLOAD_DOWNLOAD_ITEM_VIEW_H__