[Android] Prompt with infobar on filename conflict

chrome/browser/android/download_overwrite_infobar_delegate.cc

+// Copyright 2014 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/android/download_overwrite_infobar_delegate.h"
+
+#include "base/memory/scoped_ptr.h"
+#include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/infobars/infobar_service.h"
+#include "chrome/browser/ui/android/infobars/download_overwrite_infobar.h"
+#include "chrome/grit/generated_resources.h"
+#include "components/infobars/core/infobar.h"
+#include "content/public/browser/download_item.h"
+#include "content/public/browser/web_contents.h"
+#include "grit/theme_resources.h"
+#include "ui/base/l10n/l10n_util.h"
+
+namespace chrome {
+namespace android {
+
+DownloadOverwriteInfoBarDelegate::~DownloadOverwriteInfoBarDelegate() {
+}
+
+// static
+infobars::InfoBar* DownloadOverwriteInfoBarDelegate::Create(
+    content::DownloadItem* download,
+    const base::FilePath& suggested_path,
+    const FileSelectedCallback& callback) {
+  DCHECK(download);
+  if (!download)
+    return NULL;

[Peter Kasting, 2014/10/24 01:32:22]

Don't handle DCHECK failure.  Either download can be NULL (so you shouldn't
DCHECK) or it can't (so you shouldn't have this conditional).  (And don't err on
the side of safety if it really can't be NULL, either!)

[Changwan Ryu, 2014/10/27 06:40:04]

Removed DCHECK. Infobar won't be created in this case.

+  content::WebContents* web_contents = download->GetWebContents();
+  if (!web_contents)

[Peter Kasting, 2014/10/24 01:32:22]

When can this actually be NULL?

[Changwan Ryu, 2014/10/27 06:40:04]

I'm not sure, but there were similar checks in other places, so I could not
cross out the possibility of it being NULL.

[Ted C, 2014/10/27 15:30:11]

DownloadFilePicker (download_file_picker.cc) doesn't have NULL checks for item
passed in and I believe that is the desktop equivalent of what this delegate is.
 I think you can safely remove the NULL checks here to be consistent.

[Peter Kasting, 2014/10/27 17:49:27]

Please find out for certain.  We should never have code in the codebase that we
don't understand/know for certain is necessary.  The uncertainty tends to
propagate, as it is doing here.

[Changwan Ryu, 2014/10/28 04:59:43]

Ok, I digged a bit.

download_file_picker.cc has the following lines:

  WebContents* web_contents = item->GetWebContents();
  select_file_dialog_ = ui::SelectFileDialog::Create(
      this, new ChromeSelectFilePolicy(web_contents));

...

  gfx::NativeWindow owning_window = web_contents ?
      platform_util::GetTopLevel(web_contents->GetNativeView()) : NULL;

It does not have explicit NULL check but still takes into consideration the
possibility of web_contents being NULL.

Also, in download_item_impl.cc, GetWebContents() can return NULL and there are
if statements that check nullity of GetWebContents() (though comments say it
should only happen when it's created from history import.)

I'd like to keep it.

[Peter Kasting, 2014/10/29 04:08:03]

If it has no comments about when/why this can be NULL, you probably have to
assume the person who wrote that code was as uncertain as you are.  I don't
think that's a reliable signal.
Then it doesn't sound like that's a case you'd need to worry about here.
I don't want it unless you have a concrete scenario where you can demonstrate
it's NULL.

Either leave it out entirely, or, if you're really concerned that this can truly
be NULL put in a CHECK that it's non-NULL that you guarantee you'll convert (to
nothing or a conditional, as appropriate) within, say, a month of it landing on
trunk, based on whether it actually fires.  If it then needs to stay, it needs
to gain a comment about precisely when it can be NULL.

I realize that just putting in a NULL-check is easy and safe, but I'm being
harder-nosed about this because that's precisely what has resulted in a lot of
what turned out to be incorrect NULL-checks in the past that have led to code
being harder to read and refactor.  I want to make sure we never check for cases
that can't happen.

+    return NULL;
+
+  InfoBarService* infobar_service =
+      InfoBarService::FromWebContents(web_contents);
+
+  DownloadOverwriteInfoBarDelegate* const delegate =

[Peter Kasting, 2014/10/24 01:32:22]

Nit: Foo* const, while fine with me, is unusual in Chrome code.  I would avoid
it, and really I'd probably do that by inlining these lines into one longer
statement for consistency with how most other infobars' Create() methods work.

[Changwan Ryu, 2014/10/27 06:40:04]

Done. Thanks for the detailed explanation.

+      new DownloadOverwriteInfoBarDelegate(download, suggested_path, callback);
+  infobars::InfoBar* infobar = DownloadOverwriteInfoBar::CreateInfoBar(
+      scoped_ptr<DownloadOverwriteInfoBarDelegate>(delegate)).release();

[Peter Kasting, 2014/10/24 01:32:22]

Why are you releasing this just to make a scoped_ptr out of it again on the next
line?  Just Pass() it directly.

Nit: Can use make_scoped_ptr() for brevity

[Changwan Ryu, 2014/10/27 06:40:04]

Done.

+  return infobar_service->AddInfoBar(scoped_ptr<infobars::InfoBar>(infobar));
+}
+
+DownloadOverwriteInfoBarDelegate*
+    DownloadOverwriteInfoBarDelegate::AsDownloadOverwriteInfoBarDelegate() {
+  return this;
+}
+
+bool DownloadOverwriteInfoBarDelegate::Accept() {
+  callback_.Run(suggested_path_);
+  return true;
+}
+
+bool DownloadOverwriteInfoBarDelegate::Cancel() {
+  callback_.Run(base::FilePath());
+  return true;
+}
+
+DownloadOverwriteInfoBarDelegate::DownloadOverwriteInfoBarDelegate(
+    content::DownloadItem* download,
+    const base::FilePath& suggested_path,
+    const FileSelectedCallback& callback)
+    : pending_id_(download->GetId()),
+      suggested_path_(suggested_path),
+      callback_(callback) {
+}
+
+std::string DownloadOverwriteInfoBarDelegate::GetFileName() const {
+  return suggested_path_.BaseName().value();
+}
+
+std::string DownloadOverwriteInfoBarDelegate::GetDirName() const {
+  return suggested_path_.DirName().BaseName().value();
+}
+
+std::string DownloadOverwriteInfoBarDelegate::GetDirFullPath() const {
+  return suggested_path_.DirName().value();
+}
+
+}  // namespace android
+}  // namespace chrome