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();
 }
 
+bool DownloadItemView::ShouldAllowDownloadFeedback() {
+  DCHECK(shelf_);
+  return model_.MightBeMalicious() && model_.ShouldAllowDownloadFeedback() &&
+         !shelf_->browser()->profile()->IsOffTheRecord();
+}
+
+void DownloadItemView::MaybeSubmitDownloadToFeedbackService(
+    DownloadCommands::Command download_command) {
+  PrefService* prefs = shelf_->browser()->profile()->GetPrefs();
+  if (!safe_browsing::ExtendedReportingPrefExists(*prefs)) {

[Peter Kasting, 2016/10/24 21:39:07]

Nit: Remove ! and reverse conditional arms, so the "else" doesn't mentally read
like a double-negative.

[Jialiu Lin, 2016/10/25 21:07:09]

Done.

+    // Show dialog, because the dialog hasn't been shown before.
+    DownloadFeedbackDialogView::Show(
+        shelf_->get_parent()->GetNativeWindow(), shelf_->browser()->profile(),
+        shelf_->GetNavigator(),
+        base::Bind(&DownloadItemView::PossiblySubmitDownloadToFeedbackService,
+                   weak_ptr_factory_.GetWeakPtr(), download_command));
+  } else {
+    PossiblySubmitDownloadToFeedbackService(
+        download_command, safe_browsing::IsExtendedReportingEnabled(*prefs));
+  }
+}
+
 // 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 295 matching lines @@
       sampling_event_.reset();
     }
     // This will change the state and notify us.
     download()->ValidateDangerousDownload();
     return;
   }
 
   // WARNING: all end states after this point delete |this|.
   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;
+  if (ShouldAllowDownloadFeedback()) {

[Peter Kasting, 2016/10/24 21:39:07]

Nit: No {}

[Jialiu Lin, 2016/10/25 21:07:09]

Done.

+    MaybeSubmitDownloadToFeedbackService(DownloadCommands::DISCARD);
+  } else {
+    download()->Remove();
   }
-  download()->Remove();
 }
 
 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 164 matching lines @@
   download_protection_service->feedback_service()->BeginFeedbackForDownload(
       download());
   // 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())
+void DownloadItemView::PossiblySubmitDownloadToFeedbackService(
+    DownloadCommands::Command download_command_id,
+    bool enabled) {
+  if ((!enabled || !SubmitDownloadToFeedbackService()) &&

[Peter Kasting, 2016/10/24 21:39:07]

Doesn't this result in deleting the download when submission succeeds, even if
the command was KEEP?  Is that a problem?

[asanka, 2016/10/25 20:51:13]

Yes. The StealDangerousDownload logic is meant to acquire a dangerous download
after the user has chosen to discard it.

Even if we didn't delete the file, we can't run the upload logic after accepting
a dangerous download. :-( We expose the file to external consumers at that
point. The upload logic may interfere with applications that are trying to
access the download file and prevent the user from opening the file
successfully. 

[Jialiu Lin, 2016/10/25 21:07:09]

oops, Thanks for catching this. Seems changes need to be made to more files.

+      download_command_id == DownloadCommands::DISCARD)

[Peter Kasting, 2016/10/24 21:39:07]

Nit: This might be clearer if written like:

  if (feedback_enabled && SubmitDownloadToFeedbackService())
    return;  // WARNING: |this| is now deleted.

  if (download_command_id == DownloadCommands::DISCARD)
    download()->Remove();

[Jialiu Lin, 2016/10/25 21:07:09]

Done.

     download()->Remove();
   // 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)),
@@ skipping 30 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);
 }