implementation for new duplicate download UI

chrome/browser/download/chrome_download_manager_delegate.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/download/chrome_download_manager_delegate.h"
 
 #include <string>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ skipping 39 matching lines @@
 #include "components/prefs/pref_service.h"
 #include "content/public/browser/download_item.h"
 #include "content/public/browser/download_manager.h"
 #include "content/public/browser/notification_source.h"
 #include "content/public/browser/page_navigator.h"
 #include "net/base/filename_util.h"
 #include "net/base/mime_util.h"
 #include "ui/base/l10n/l10n_util.h"
 
 #if BUILDFLAG(ANDROID_JAVA_UI)
-#include "chrome/browser/android/download/chrome_download_manager_overwrite_infobar_delegate.h"
+#include "chrome/browser/android/download/chrome_duplicate_download_infobar_delegate.h"
 #include "chrome/browser/infobars/infobar_service.h"
 #endif
 
 #if defined(OS_CHROMEOS)
 #include "chrome/browser/chromeos/drive/download_handler.h"
 #include "chrome/browser/chromeos/drive/file_system_util.h"
 #endif
 
 #if defined(ENABLE_EXTENSIONS)
 #include "chrome/browser/extensions/api/downloads/downloads_api.h"
@@ skipping 539 matching lines @@
       conflict_action,
       callback);
 }
 
 void ChromeDownloadManagerDelegate::PromptUserForDownloadPath(
     DownloadItem* download,
     const base::FilePath& suggested_path,
     const DownloadTargetDeterminerDelegate::FileSelectedCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 #if BUILDFLAG(ANDROID_JAVA_UI)
-  chrome::android::ChromeDownloadManagerOverwriteInfoBarDelegate::Create(
-      InfoBarService::FromWebContents(download->GetWebContents()), download,
-      suggested_path, callback);
+  content::WebContents* web_contents = download->GetWebContents();

[asanka, 2016/11/10 20:49:39]

It's possible for the WebContents to be null here. E.g. a resumption attempt
encounters a filename conflict.

I see occasional crashes from the previous code path which also doesn't check if
the WebContents is null. But it's hard to tell whether those crashes were caused
by a missing WebContents.

[qinmin, 2016/11/10 21:37:08]

null WebContents caused by resumption attempt with a filename conflict is
handled in DownloadTargetDeterminer::ReserveVirtualPathDone(), instead of here. 

Anyway, did a null check here and call callback.Run(base::FilePath()) if
webcontents if it happens.

[asanka, 2016/11/10 21:57:13]

Sure. But ReserveVirtualPathDone() isn't where we initiate prompting. Even if
checking there works now, this function doesn't have any basis for assuming a
WebContents will still be there when it's called. Chances are eventually we'll
trip over these kinds of assumptions.

Either way, we are going to clean up some of these and add tests over at
https://codereview.chromium.org/2453633006 which also adds better signalling for
the type of error that caused the prompt.
Thanks. That hardcodes "Cancel" as the default action whenever a prompt can't be
displayed to the user. That's fine since that's what
DownloadTargetDeterminer::ReserveVirtualPathDone() currently does when there's
no WebContents.

+  chrome::android::ChromeDuplicateDownloadInfoBarDelegate::Create(
+      InfoBarService::FromWebContents(web_contents), download,
+      suggested_path, web_contents->GetBrowserContext()->IsOffTheRecord(),

[asanka, 2016/11/10 20:49:39]

Nit: IsOffTheRecord() is redundant. ChromeDuplicateDownloadInfoBarDelegate can
determine whether it's dealing with an off-the-record profile via the |download|
parameter and also via the |InfoBarService| parameter.

[qinmin, 2016/11/10 21:37:08]

Done.

+      callback);
 #else
   DownloadFilePicker::ShowFilePicker(download, suggested_path, callback);
 #endif
 }
 
 void ChromeDownloadManagerDelegate::DetermineLocalPath(
     DownloadItem* download,
     const base::FilePath& virtual_path,
     const DownloadTargetDeterminerDelegate::LocalPathCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
@@ skipping 174 matching lines @@
       path.MatchesExtension(FILE_PATH_LITERAL(".xht")) ||
       path.MatchesExtension(FILE_PATH_LITERAL(".xhtm")) ||
       path.MatchesExtension(FILE_PATH_LITERAL(".xhtml")) ||
       path.MatchesExtension(FILE_PATH_LITERAL(".xsl")) ||
       path.MatchesExtension(FILE_PATH_LITERAL(".xslt"))) {
     return true;
   }
 #endif
   return false;
 }