Download Feedback Service should upload all eligible downloads

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

 // 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.
 
 #include "chrome/browser/ui/views/download/download_item_view.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <vector>
@@ skipping 239 matching lines @@
     const ui::ThemeProvider* theme) {
   return theme ? theme->GetColor(ThemeProperties::COLOR_BOOKMARK_TEXT)
                : gfx::kPlaceholderColor;
 }
 
 void DownloadItemView::OnExtractIconComplete(gfx::Image* icon_bitmap) {
   if (icon_bitmap)
     shelf_->SchedulePaint();
 }
 
+void DownloadItemView::MaybeSubmitDownloadToFeedbackService(
+    DownloadCommands::Command download_command) {
+  PrefService* prefs = shelf_->browser()->profile()->GetPrefs();
+  if (model_.MightBeMalicious() && model_.ShouldAllowDownloadFeedback() &&
+      !shelf_->browser()->profile()->IsOffTheRecord()) {
+    if (safe_browsing::ExtendedReportingPrefExists(*prefs)) {
+      SubmitDownloadWhenFeedbackServiceEnabled(
+          download_command, safe_browsing::IsExtendedReportingEnabled(*prefs));
+    } else {
+      // Show dialog, because the dialog hasn't been shown before.
+      DownloadFeedbackDialogView::Show(
+          shelf_->get_parent()->GetNativeWindow(), shelf_->browser()->profile(),
+          shelf_->GetNavigator(),
+          base::Bind(
+              &DownloadItemView::SubmitDownloadWhenFeedbackServiceEnabled,
+              weak_ptr_factory_.GetWeakPtr(), download_command));
+    }
+  } else {
+    DownloadCommands command(download());
+    command.ExecuteCommand(download_command);
+  }
+}
+
 // DownloadObserver interface.
 
 // Update the progress graphic on the icon and our text status label
 // to reflect our current bytes downloaded, time remaining.
 void DownloadItemView::OnDownloadUpdated(DownloadItem* download_item) {
   DCHECK_EQ(download(), download_item);
 
   if (!model_.ShouldShowInShelf()) {
     shelf_->RemoveDownloadView(this);  // This will delete us!
     return;
@@ skipping 292 matching lines @@
     if (sampling_event_) {
       sampling_event_->CreateUserDecisionEvent(
           ExperienceSamplingEvent::kProceed);
       sampling_event_.reset();
     }
     // This will change the state and notify us.
     download()->ValidateDangerousDownload();
     return;
   }
 
   // WARNING: all end states after this point delete |this|.

[Peter Kasting, 2016/11/01 23:48:32]

I think this lies; when MaybeSubmit...() calls
DownloadFeedbackDialogView::Show(), that doesn't delete |this|, does it?

In either case I'd move this to be at the end of the function like the comment
in SubmitDownloadWhenFeedbackServiceEnabled(), but be careful that the wording
is accurate.

[Jialiu Lin, 2016/11/03 20:16:50]

Done.

   DCHECK_EQ(discard_button_, sender);
   UMA_HISTOGRAM_LONG_TIMES("clickjacking.discard_download", warning_duration);
-  Profile* profile = shelf_->browser()->profile();
-  if (!model_.IsMalicious() && model_.ShouldAllowDownloadFeedback() &&
-      !profile->IsOffTheRecord()) {
-    if (!safe_browsing::ExtendedReportingPrefExists(*profile->GetPrefs())) {
-      // Show dialog, because the dialog hasn't been shown before.
-      DownloadFeedbackDialogView::Show(
-          shelf_->get_parent()->GetNativeWindow(), profile,
-          shelf_->GetNavigator(),
-          base::Bind(&DownloadItemView::PossiblySubmitDownloadToFeedbackService,
-                     weak_ptr_factory_.GetWeakPtr()));
-    } else {
-      PossiblySubmitDownloadToFeedbackService(
-          safe_browsing::IsExtendedReportingEnabled(*profile->GetPrefs()));
-    }
-    return;
-  }
-  download()->Remove();
+  MaybeSubmitDownloadToFeedbackService(DownloadCommands::DISCARD);
 }
 
 SkColor DownloadItemView::GetVectorIconBaseColor() const {
   return GetTextColor();
 }
 
 void DownloadItemView::AnimationProgressed(const gfx::Animation* animation) {
   // We don't care if what animation (body button/drop button/complete),
   // is calling back, as they all have to go through the same paint call.
   SchedulePaint();
@@ skipping 144 matching lines @@
   UMA_HISTOGRAM_LONG_TIMES("clickjacking.open_download",
                            base::Time::Now() - creation_time_);
 
   UpdateAccessibleName();
 
   // Calling download()->OpenDownload may delete this, so this must be
   // the last thing we do.
   download()->OpenDownload();
 }
 
-bool DownloadItemView::SubmitDownloadToFeedbackService() {
+bool DownloadItemView::SubmitDownloadToFeedbackService(
+    DownloadCommands::Command download_command) {
 #if defined(FULL_SAFE_BROWSING)
   safe_browsing::SafeBrowsingService* sb_service =
       g_browser_process->safe_browsing_service();
   if (!sb_service)
     return false;
   safe_browsing::DownloadProtectionService* download_protection_service =
       sb_service->download_protection_service();
   if (!download_protection_service)
     return false;
   download_protection_service->feedback_service()->BeginFeedbackForDownload(
-      download());
+      download(), download_command);
   // WARNING: we are deleted at this point.  Don't access 'this'.
   return true;
 #else
   NOTREACHED();
   return false;
 #endif
 }
 
-void DownloadItemView::PossiblySubmitDownloadToFeedbackService(bool enabled) {
-  if (!enabled || !SubmitDownloadToFeedbackService())
-    download()->Remove();
+void DownloadItemView::SubmitDownloadWhenFeedbackServiceEnabled(
+    DownloadCommands::Command download_command,
+    bool feedback_enabled) {
+  if (feedback_enabled && SubmitDownloadToFeedbackService(download_command))
+    return;
+
+  DownloadCommands command(download());
+  command.ExecuteCommand(download_command);
   // WARNING: 'this' is deleted at this point. Don't access 'this'.
 }
 
 void DownloadItemView::LoadIcon() {
   IconManager* im = g_browser_process->icon_manager();
   last_download_item_path_ = download()->GetTargetFilePath();
   im->LoadIcon(last_download_item_path_, IconLoader::SMALL,
                base::Bind(&DownloadItemView::OnExtractIconComplete,
                           base::Unretained(this)),
                &cancelable_task_tracker_);
@@ skipping 29 matching lines @@
   // mouse press event we block RootView in mouse dispatching. This also
   // appears to cause RootView to get a mouse pressed BEFORE the mouse
   // release is seen, which means RootView sends us another mouse press no
   // matter where the user pressed. To force RootView to recalculate the
   // mouse target during the mouse press we explicitly set the mouse handler
   // to null.
   static_cast<views::internal::RootView*>(GetWidget()->GetRootView())
       ->SetMouseHandler(nullptr);
 
   if (!context_menu_.get())
-    context_menu_.reset(new DownloadShelfContextMenuView(download()));
+    context_menu_.reset(new DownloadShelfContextMenuView(this));
   context_menu_->Run(GetWidget()->GetTopLevelWidget(), rect, source_type,
                      base::Bind(&DownloadItemView::ReleaseDropdown,
                                 weak_ptr_factory_.GetWeakPtr()));
 }
 
 void DownloadItemView::HandlePressEvent(const ui::LocatedEvent& event,
                                         bool active_event) {
   // The event should not activate us in dangerous/malicious mode.
   if (IsShowingWarningDialog())
     return;
@@ skipping 268 matching lines @@
     SchedulePaint();
 }
 
 SkColor DownloadItemView::GetTextColor() const {
   return GetTextColorForThemeProvider(GetThemeProvider());
 }
 
 SkColor DownloadItemView::GetDimmedTextColor() const {
   return SkColorSetA(GetTextColor(), 0xC7);
 }