implementation for new duplicate download UI

implementation for new duplicate download UI

This change implements the new duplicate download UI.
It renames the DownloadOverwriteInfobarXXXX to DuplicateDownloadInfobarXXXX.
Chrome no longer show the overwrite button,
and always creating a new file if user click on the download button.

BUG=658239
Committed: https://crrev.com/577d7c791ae9a560d457bb08a8a959bd89eeec30
Cr-Commit-Position: refs/heads/master@{#431389}

[qinmin, 2016-11-04 17:55:31]

Description was changed from

==========
implementation for new duplicate download UI

This change implements the new duplicate download UI.
Chrome no longer show the overwrite button,
and always creating a new file if user click on the download button.

BUG=658239
==========

to

==========
implementation for new duplicate download UI

This change implements the new duplicate download UI.
It renames the DownloadOverwriteInfobarXXXX to DuplicateDownloadInfobarXXXX.
Chrome no longer show the overwrite button,
and always creating a new file if user click on the download button.

BUG=658239
==========

[qinmin, 2016-11-04 17:56:44]

qinmin@chromium.org changed reviewers:
+ dfalcantara@chromium.org, fgorski@chromium.org, pkasting@chromium.org

[qinmin, 2016-11-04 17:56:45]

PTAL.

dfalcantara@ for download infobar codereview.
fgorski@ for offline page changes
pkasting for OWNER stamp

[qinmin, 2016-11-04 17:59:29]

The CQ bit was checked by qinmin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-04 18:00:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Peter Kasting, 2016-11-04 21:01:12]

On 2016/11/04 17:56:45, qinmin wrote:
> pkasting for OWNER stamp

Sorry, but: OWNER stamp of what? chrome/browser/ui/BUILD.gn and
components/infobars/core/infobar_delegate.h?  (Since I'm clearly not an OWNER of
everything here)

[commit-bot: I haz the power, 2016-11-04 22:34:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-04 22:34:10]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[qinmin, 2016-11-05 00:01:49]

On 2016/11/04 21:01:12, Peter Kasting wrote:
> On 2016/11/04 17:56:45, qinmin wrote:
> > pkasting for OWNER stamp
> 
> Sorry, but: OWNER stamp of what? chrome/browser/ui/BUILD.gn and
> components/infobars/core/infobar_delegate.h?  (Since I'm clearly not an OWNER
of
> everything here)

components/infobars/ still list you as an OWNER there:
https://cs.chromium.org/chromium/src/components/infobars/OWNERS
so do chrome/browser/ui:
https://cs.chromium.org/chromium/src/chrome/browser/ui/OWNERS

[Peter Kasting, 2016-11-05 00:05:24]

On 2016/11/05 00:01:49, qinmin wrote:
> On 2016/11/04 21:01:12, Peter Kasting wrote:
> > On 2016/11/04 17:56:45, qinmin wrote:
> > > pkasting for OWNER stamp
> > 
> > Sorry, but: OWNER stamp of what? chrome/browser/ui/BUILD.gn and
> > components/infobars/core/infobar_delegate.h?  (Since I'm clearly not an
OWNER
> of
> > everything here)
> 
> components/infobars/ still list you as an OWNER there:
> https://cs.chromium.org/chromium/src/components/infobars/OWNERS
> so do chrome/browser/ui:
> https://cs.chromium.org/chromium/src/chrome/browser/ui/OWNERS

Right, I can do OWNER reviews for those directories.  I guess my comment was a
combination "not sure if you wanted my OWNER review for the android/
subdirectory of c/b/ui/" and "in the future, if adding someone to do an OWNER
review only of certain files, please do make the desired files clear, so they
don't start looking at the whole CL, realize they're not an OWNER of at least
part, and then wonder which parts they were actually meant to review."

[Peter Kasting, 2016-11-05 00:26:58]

LGTM

https://codereview.chromium.org/2478583004/diff/1/chrome/browser/ui/android/i...
File chrome/browser/ui/android/infobars/duplicate_download_infobar.cc (right):

https://codereview.chromium.org/2478583004/diff/1/chrome/browser/ui/android/i...
chrome/browser/ui/android/infobars/duplicate_download_infobar.cc:15: using
base::android::ScopedJavaLocalRef;
Nit: Half the uses of this below already fully-qualify it.  I would just
fully-qualify the others and remove this.

https://codereview.chromium.org/2478583004/diff/1/chrome/browser/ui/android/i...
chrome/browser/ui/android/infobars/duplicate_download_infobar.cc:50: if (action
== InfoBarAndroid::ACTION_OK) {
Nit: A simpler to write this code might be:

  const bool is_ok = action == InfoBarAndroid::ACTION_OK;
  DCHECK(is_ok || (action == InfoBarAndroid::ACTION_CANCEL));
  if (is_ok ? delegate->Download() : delegate->Cancel())
    RemoveSelf();

https://codereview.chromium.org/2478583004/diff/1/chrome/browser/ui/android/i...
chrome/browser/ui/android/infobars/duplicate_download_infobar.cc:57:
CHECK(false);
Nit: Prefer DCHECK to CHECK in cases where you're not temporarily tracking down
bugs, and the consequence is not a security failure.  See
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
.

https://codereview.chromium.org/2478583004/diff/1/components/infobars/core/in...
File components/infobars/core/infobar_delegate.h (right):

https://codereview.chromium.org/2478583004/diff/1/components/infobars/core/in...
components/infobars/core/infobar_delegate.h:78:
CHROME_DUPLICATE_DOWNLOAD_INFOBAR_DELEGATE = 4,
Is changing these OK, in terms of getting histogram data from the old and new
things mixed together?

[fgorski, 2016-11-07 17:52:45]

offline_pages stuff lgtm

https://codereview.chromium.org/2478583004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java
(right):

https://codereview.chromium.org/2478583004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java:286:
native void nativeStartDownload(
nit: single line

[gone, 2016-11-07 19:45:55]

https://codereview.chromium.org/2478583004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java
(right):

https://codereview.chromium.org/2478583004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java:88:
boolean fileExists = doesFileAlreadyExists(fullDirPath, fileName);
nit: Can you rename this function?  It's not grammatically correct.

doesFileExist
checkFileExists
fileAlreadyExists

https://codereview.chromium.org/2478583004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/DuplicateDownloadInfoBar.java
(right):

https://codereview.chromium.org/2478583004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DuplicateDownloadInfoBar.java:30:
public class DuplicateDownloadInfoBar extends InfoBar {
This should be extending ConfirmInfoBar.

https://codereview.chromium.org/2478583004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DuplicateDownloadInfoBar.java:64:
public void onClick(View view) {
Can you just call DownloadUtils.openFile()?  I'd rather not duplicate this logic
again.

https://codereview.chromium.org/2478583004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DuplicateDownloadInfoBar.java:122:
private static String getMimeTypeFromFile(Uri fileUri) {
FromUri because you're passing in a Uri.

https://codereview.chromium.org/2478583004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DuplicateDownloadInfoBar.java:143:
public void onButtonClicked(boolean isPrimaryButton) {
Once you inherit from ConfirmInfoBar, this comes free.

https://codereview.chromium.org/2478583004/diff/1/chrome/android/java/strings...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2478583004/diff/1/chrome/android/java/strings...
chrome/android/java/strings/android_chrome_strings.grd:1330: Cancel
Reuse the existing IDS_CANCEL at the beginning of the file instead of adding a
new one.

https://codereview.chromium.org/2478583004/diff/1/chrome/android/java/strings...
chrome/android/java/strings/android_chrome_strings.grd:1332: <message
name="IDS_DUPLICATE_DOWNLOAD_INFOBAR_DOWNLOAD_BUTTON" desc="Label for choosing
'Download' in the prompt for a duplicate download. [CHAR-LIMIT=32]">
This description doesn't help translators figure out what the string should say.
 Maybe:

"Label for the button that confirms that a user wants to download the file
again."

https://codereview.chromium.org/2478583004/diff/1/chrome/browser/android/down...
File
chrome/browser/android/download/android_download_manager_duplicate_infobar_delegate.h
(right):

https://codereview.chromium.org/2478583004/diff/1/chrome/browser/android/down...
chrome/browser/android/download/android_download_manager_duplicate_infobar_delegate.h:17:

Add a comment describing what the infobar is actually used for.

https://codereview.chromium.org/2478583004/diff/1/chrome/browser/android/down...
File chrome/browser/android/download/duplicate_download_infobar_delegate.h
(right):

https://codereview.chromium.org/2478583004/diff/1/chrome/browser/android/down...
chrome/browser/android/download/duplicate_download_infobar_delegate.h:16: //
already a duplicate file on the sdcard. If user chooses to proceed,
the sdcard -> in storage?

https://codereview.chromium.org/2478583004/diff/1/chrome/browser/android/down...
chrome/browser/android/download/duplicate_download_infobar_delegate.h:20: class
DuplicateDownloadInfoBarDelegate : public infobars::InfoBarDelegate {
Make this a ConfirmInfoBarDelegate subclass, then hook Download and Cancel into
ConfirmInfoBarDelegate::Accept() and ConfirmInfoBarDelegate::Cancel().

https://codereview.chromium.org/2478583004/diff/1/chrome/browser/android/down...
chrome/browser/android/download/duplicate_download_infobar_delegate.h:40: bool
ShouldExpire(const NavigationDetails& details) const override;
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/2478583004/diff/1/chrome/browser/ui/android/i...
File chrome/browser/ui/android/infobars/duplicate_download_infobar.h (right):

https://codereview.chromium.org/2478583004/diff/1/chrome/browser/ui/android/i...
chrome/browser/ui/android/infobars/duplicate_download_infobar.h:20: class
DuplicateDownloadInfoBar : public InfoBarAndroid {
This should extend ConfimrInfoBar instead.  It's prompting the user for
something.

https://codereview.chromium.org/2478583004/diff/1/components/infobars/core/in...
File components/infobars/core/infobar_delegate.h (right):

https://codereview.chromium.org/2478583004/diff/1/components/infobars/core/in...
components/infobars/core/infobar_delegate.h:78:
CHROME_DUPLICATE_DOWNLOAD_INFOBAR_DELEGATE = 4,
On 2016/11/05 00:26:57, Peter Kasting wrote:
> Is changing these OK, in terms of getting histogram data from the old and new
> things mixed together?

AFAICT they're expanding the scope of the overwrite infobar to catch more
downloads.  It should be fine to reuse the old IDs.

[gone, 2016-11-07 19:45:56]

https://codereview.chromium.org/2478583004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java
(right):

https://codereview.chromium.org/2478583004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java:88:
boolean fileExists = doesFileAlreadyExists(fullDirPath, fileName);
nit: Can you rename this function?  It's not grammatically correct.

doesFileExist
checkFileExists
fileAlreadyExists

https://codereview.chromium.org/2478583004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/DuplicateDownloadInfoBar.java
(right):

https://codereview.chromium.org/2478583004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DuplicateDownloadInfoBar.java:30:
public class DuplicateDownloadInfoBar extends InfoBar {
This should be extending ConfirmInfoBar.

https://codereview.chromium.org/2478583004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DuplicateDownloadInfoBar.java:64:
public void onClick(View view) {
Can you just call DownloadUtils.openFile()?  I'd rather not duplicate this logic
again.

https://codereview.chromium.org/2478583004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DuplicateDownloadInfoBar.java:122:
private static String getMimeTypeFromFile(Uri fileUri) {
FromUri because you're passing in a Uri.

https://codereview.chromium.org/2478583004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DuplicateDownloadInfoBar.java:143:
public void onButtonClicked(boolean isPrimaryButton) {
Once you inherit from ConfirmInfoBar, this comes free.

https://codereview.chromium.org/2478583004/diff/1/chrome/android/java/strings...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2478583004/diff/1/chrome/android/java/strings...
chrome/android/java/strings/android_chrome_strings.grd:1330: Cancel
Reuse the existing IDS_CANCEL at the beginning of the file instead of adding a
new one.

https://codereview.chromium.org/2478583004/diff/1/chrome/android/java/strings...
chrome/android/java/strings/android_chrome_strings.grd:1332: <message
name="IDS_DUPLICATE_DOWNLOAD_INFOBAR_DOWNLOAD_BUTTON" desc="Label for choosing
'Download' in the prompt for a duplicate download. [CHAR-LIMIT=32]">
This description doesn't help translators figure out what the string should say.
 Maybe:

"Label for the button that confirms that a user wants to download the file
again."

https://codereview.chromium.org/2478583004/diff/1/chrome/browser/android/down...
File
chrome/browser/android/download/android_download_manager_duplicate_infobar_delegate.h
(right):

https://codereview.chromium.org/2478583004/diff/1/chrome/browser/android/down...
chrome/browser/android/download/android_download_manager_duplicate_infobar_delegate.h:17:

Add a comment describing what the infobar is actually used for.

https://codereview.chromium.org/2478583004/diff/1/chrome/browser/android/down...
File chrome/browser/android/download/duplicate_download_infobar_delegate.h
(right):

https://codereview.chromium.org/2478583004/diff/1/chrome/browser/android/down...
chrome/browser/android/download/duplicate_download_infobar_delegate.h:16: //
already a duplicate file on the sdcard. If user chooses to proceed,
the sdcard -> in storage?

https://codereview.chromium.org/2478583004/diff/1/chrome/browser/android/down...
chrome/browser/android/download/duplicate_download_infobar_delegate.h:20: class
DuplicateDownloadInfoBarDelegate : public infobars::InfoBarDelegate {
Make this a ConfirmInfoBarDelegate subclass, then hook Download and Cancel into
ConfirmInfoBarDelegate::Accept() and ConfirmInfoBarDelegate::Cancel().

https://codereview.chromium.org/2478583004/diff/1/chrome/browser/android/down...
chrome/browser/android/download/duplicate_download_infobar_delegate.h:40: bool
ShouldExpire(const NavigationDetails& details) const override;
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/2478583004/diff/1/chrome/browser/ui/android/i...
File chrome/browser/ui/android/infobars/duplicate_download_infobar.h (right):

https://codereview.chromium.org/2478583004/diff/1/chrome/browser/ui/android/i...
chrome/browser/ui/android/infobars/duplicate_download_infobar.h:20: class
DuplicateDownloadInfoBar : public InfoBarAndroid {
This should extend ConfimrInfoBar instead.  It's prompting the user for
something.

https://codereview.chromium.org/2478583004/diff/1/components/infobars/core/in...
File components/infobars/core/infobar_delegate.h (right):

https://codereview.chromium.org/2478583004/diff/1/components/infobars/core/in...
components/infobars/core/infobar_delegate.h:78:
CHROME_DUPLICATE_DOWNLOAD_INFOBAR_DELEGATE = 4,
On 2016/11/05 00:26:57, Peter Kasting wrote:
> Is changing these OK, in terms of getting histogram data from the old and new
> things mixed together?

AFAICT they're expanding the scope of the overwrite infobar to catch more
downloads.  It should be fine to reuse the old IDs.

[qinmin, 2016-11-08 00:30:51]

https://codereview.chromium.org/2478583004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java
(right):

https://codereview.chromium.org/2478583004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java:88:
boolean fileExists = doesFileAlreadyExists(fullDirPath, fileName);
On 2016/11/07 19:45:55, dfalcantara (check my queue) wrote:
> nit: Can you rename this function?  It's not grammatically correct.
> 
> doesFileExist
> checkFileExists
> fileAlreadyExists

Done. Changed to checkFileExists

https://codereview.chromium.org/2478583004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/DuplicateDownloadInfoBar.java
(right):

https://codereview.chromium.org/2478583004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DuplicateDownloadInfoBar.java:30:
public class DuplicateDownloadInfoBar extends InfoBar {
On 2016/11/07 19:45:55, dfalcantara (check my queue) wrote:
> This should be extending ConfirmInfoBar.

Done.

https://codereview.chromium.org/2478583004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DuplicateDownloadInfoBar.java:64:
public void onClick(View view) {
On 2016/11/07 19:45:55, dfalcantara (check my queue) wrote:
> Can you just call DownloadUtils.openFile()?  I'd rather not duplicate this
logic
> again.

Thanks, I didn't saw this function in my previous checkout, so it has been added
recently.

https://codereview.chromium.org/2478583004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DuplicateDownloadInfoBar.java:122:
private static String getMimeTypeFromFile(Uri fileUri) {
On 2016/11/07 19:45:55, dfalcantara (check my queue) wrote:
> FromUri because you're passing in a Uri.

Done.

https://codereview.chromium.org/2478583004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DuplicateDownloadInfoBar.java:143:
public void onButtonClicked(boolean isPrimaryButton) {
On 2016/11/07 19:45:55, dfalcantara (check my queue) wrote:
> Once you inherit from ConfirmInfoBar, this comes free.

Done.

https://codereview.chromium.org/2478583004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java
(right):

https://codereview.chromium.org/2478583004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java:286:
native void nativeStartDownload(
On 2016/11/07 17:52:45, fgorski wrote:
> nit: single line

Done.

https://codereview.chromium.org/2478583004/diff/1/chrome/android/java/strings...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2478583004/diff/1/chrome/android/java/strings...
chrome/android/java/strings/android_chrome_strings.grd:1330: Cancel
On 2016/11/07 19:45:55, dfalcantara (check my queue) wrote:
> Reuse the existing IDS_CANCEL at the beginning of the file instead of adding a
> new one.

Done.

https://codereview.chromium.org/2478583004/diff/1/chrome/android/java/strings...
chrome/android/java/strings/android_chrome_strings.grd:1332: <message
name="IDS_DUPLICATE_DOWNLOAD_INFOBAR_DOWNLOAD_BUTTON" desc="Label for choosing
'Download' in the prompt for a duplicate download. [CHAR-LIMIT=32]">
On 2016/11/07 19:45:55, dfalcantara (check my queue) wrote:
> This description doesn't help translators figure out what the string should
say.
>  Maybe:
> 
> "Label for the button that confirms that a user wants to download the file
> again."

Done.

https://codereview.chromium.org/2478583004/diff/1/chrome/browser/android/down...
File
chrome/browser/android/download/android_download_manager_duplicate_infobar_delegate.h
(right):

https://codereview.chromium.org/2478583004/diff/1/chrome/browser/android/down...
chrome/browser/android/download/android_download_manager_duplicate_infobar_delegate.h:17:

On 2016/11/07 19:45:55, dfalcantara (check my queue) wrote:
> Add a comment describing what the infobar is actually used for.

Done.

https://codereview.chromium.org/2478583004/diff/1/chrome/browser/android/down...
File chrome/browser/android/download/duplicate_download_infobar_delegate.h
(right):

https://codereview.chromium.org/2478583004/diff/1/chrome/browser/android/down...
chrome/browser/android/download/duplicate_download_infobar_delegate.h:16: //
already a duplicate file on the sdcard. If user chooses to proceed,
On 2016/11/07 19:45:55, dfalcantara (check my queue) wrote:
> the sdcard -> in storage?

Done.

https://codereview.chromium.org/2478583004/diff/1/chrome/browser/android/down...
chrome/browser/android/download/duplicate_download_infobar_delegate.h:20: class
DuplicateDownloadInfoBarDelegate : public infobars::InfoBarDelegate {
On 2016/11/07 19:45:55, dfalcantara (check my queue) wrote:
> Make this a ConfirmInfoBarDelegate subclass, then hook Download and Cancel
into
> ConfirmInfoBarDelegate::Accept() and ConfirmInfoBarDelegate::Cancel().

Done.

https://codereview.chromium.org/2478583004/diff/1/chrome/browser/android/down...
chrome/browser/android/download/duplicate_download_infobar_delegate.h:40: bool
ShouldExpire(const NavigationDetails& details) const override;
On 2016/11/07 19:45:55, dfalcantara (check my queue) wrote:
> DISALLOW_COPY_AND_ASSIGN

This is a pure virtual class, so DISALLOW_COPY_AND_ASSIGN is not needed.

https://codereview.chromium.org/2478583004/diff/1/chrome/browser/ui/android/i...
File chrome/browser/ui/android/infobars/duplicate_download_infobar.cc (right):

https://codereview.chromium.org/2478583004/diff/1/chrome/browser/ui/android/i...
chrome/browser/ui/android/infobars/duplicate_download_infobar.cc:15: using
base::android::ScopedJavaLocalRef;
On 2016/11/05 00:26:57, Peter Kasting wrote:
> Nit: Half the uses of this below already fully-qualify it.  I would just
> fully-qualify the others and remove this.

Done.

https://codereview.chromium.org/2478583004/diff/1/chrome/browser/ui/android/i...
chrome/browser/ui/android/infobars/duplicate_download_infobar.cc:50: if (action
== InfoBarAndroid::ACTION_OK) {
On 2016/11/05 00:26:57, Peter Kasting wrote:
> Nit: A simpler to write this code might be:
> 
>   const bool is_ok = action == InfoBarAndroid::ACTION_OK;
>   DCHECK(is_ok || (action == InfoBarAndroid::ACTION_CANCEL));
>   if (is_ok ? delegate->Download() : delegate->Cancel())
>     RemoveSelf();

Done.

https://codereview.chromium.org/2478583004/diff/1/chrome/browser/ui/android/i...
chrome/browser/ui/android/infobars/duplicate_download_infobar.cc:57:
CHECK(false);
On 2016/11/05 00:26:57, Peter Kasting wrote:
> Nit: Prefer DCHECK to CHECK in cases where you're not temporarily tracking
down
> bugs, and the consequence is not a security failure.  See
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
> .

Done.

https://codereview.chromium.org/2478583004/diff/1/chrome/browser/ui/android/i...
File chrome/browser/ui/android/infobars/duplicate_download_infobar.h (right):

https://codereview.chromium.org/2478583004/diff/1/chrome/browser/ui/android/i...
chrome/browser/ui/android/infobars/duplicate_download_infobar.h:20: class
DuplicateDownloadInfoBar : public InfoBarAndroid {
On 2016/11/07 19:45:56, dfalcantara (check my queue) wrote:
> This should extend ConfimrInfoBar instead.  It's prompting the user for
> something.

Done.

https://codereview.chromium.org/2478583004/diff/1/components/infobars/core/in...
File components/infobars/core/infobar_delegate.h (right):

https://codereview.chromium.org/2478583004/diff/1/components/infobars/core/in...
components/infobars/core/infobar_delegate.h:78:
CHROME_DUPLICATE_DOWNLOAD_INFOBAR_DELEGATE = 4,
On 2016/11/07 19:45:56, dfalcantara (check my queue) wrote:
> On 2016/11/05 00:26:57, Peter Kasting wrote:
> > Is changing these OK, in terms of getting histogram data from the old and
new
> > things mixed together?
> 
> AFAICT they're expanding the scope of the overwrite infobar to catch more
> downloads.  It should be fine to reuse the old IDs.

These enums represents how the infobar is generated, from Java side or from
native sode. And since this CL doesn't change the way infobar is generated, so
reusing them should be ok.

[gone, 2016-11-08 01:41:51]

lgtm

https://codereview.chromium.org/2478583004/diff/20001/chrome/browser/android/...
File chrome/browser/android/download/duplicate_download_infobar_delegate.h
(right):

https://codereview.chromium.org/2478583004/diff/20001/chrome/browser/android/...
chrome/browser/android/download/duplicate_download_infobar_delegate.h:30:
base::string16 GetMessageText() const override;
Group together GetMessageText and ShouldExpier?  I think they both override
InfoBarDelegate functions.

[qinmin, 2016-11-08 19:19:05]

https://codereview.chromium.org/2478583004/diff/20001/chrome/browser/android/...
File chrome/browser/android/download/duplicate_download_infobar_delegate.h
(right):

https://codereview.chromium.org/2478583004/diff/20001/chrome/browser/android/...
chrome/browser/android/download/duplicate_download_infobar_delegate.h:30:
base::string16 GetMessageText() const override;
On 2016/11/08 01:41:50, dfalcantara (check my queue) wrote:
> Group together GetMessageText and ShouldExpier?  I think they both override
> InfoBarDelegate functions.

Done.

[qinmin, 2016-11-08 19:19:13]

The CQ bit was checked by qinmin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-08 19:19:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-08 21:38:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-08 21:38:50]

Dry run: This issue passed the CQ dry run.

[qinmin, 2016-11-08 21:59:27]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2016-11-08 21:59:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, fgorski@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2478583004/#ps40001
(title: "grouping inherited methods")

[commit-bot: I haz the power, 2016-11-08 22:00:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-08 22:12:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-08 22:12:47]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[qinmin, 2016-11-09 18:51:42]

qinmin@chromium.org changed reviewers:
+ svaldez@chromium.org

[qinmin, 2016-11-09 18:51:43]

+svaldez for chrome/browser/download OWNER

[qinmin, 2016-11-10 19:42:51]

ping, svaldez@, would you give OWNER stamp for
chrome/browser/download/chrome_download_manager_delegate.cc ?
Thanks

[qinmin, 2016-11-10 19:42:53]

ping, svaldez@, would you give OWNER stamp for
chrome/browser/download/chrome_download_manager_delegate.cc ?
Thanks

[asanka, 2016-11-10 20:49:38]

asanka@chromium.org changed reviewers:
+ asanka@chromium.org

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

https://codereview.chromium.org/2478583004/diff/40001/chrome/browser/download...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/2478583004/diff/40001/chrome/browser/download...
chrome/browser/download/chrome_download_manager_delegate.cc:620:
content::WebContents* web_contents = download->GetWebContents();
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.

https://codereview.chromium.org/2478583004/diff/40001/chrome/browser/download...
chrome/browser/download/chrome_download_manager_delegate.cc:623: suggested_path,
web_contents->GetBrowserContext()->IsOffTheRecord(),
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:09]

https://codereview.chromium.org/2478583004/diff/40001/chrome/browser/download...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/2478583004/diff/40001/chrome/browser/download...
chrome/browser/download/chrome_download_manager_delegate.cc:620:
content::WebContents* web_contents = download->GetWebContents();
On 2016/11/10 20:49:39, asanka wrote:
> 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.

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.

https://codereview.chromium.org/2478583004/diff/40001/chrome/browser/download...
chrome/browser/download/chrome_download_manager_delegate.cc:623: suggested_path,
web_contents->GetBrowserContext()->IsOffTheRecord(),
On 2016/11/10 20:49:39, asanka wrote:
> 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.

Done.

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

lgtm

https://codereview.chromium.org/2478583004/diff/40001/chrome/browser/download...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/2478583004/diff/40001/chrome/browser/download...
chrome/browser/download/chrome_download_manager_delegate.cc:620:
content::WebContents* web_contents = download->GetWebContents();
On 2016/11/10 at 21:37:08, qinmin wrote:
> On 2016/11/10 20:49:39, asanka wrote:
> > 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.
> 
> null WebContents caused by resumption attempt with a filename conflict is
handled in DownloadTargetDeterminer::ReserveVirtualPathDone(), instead of here. 


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.

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

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.

[qinmin, 2016-11-10 22:09:33]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2016-11-10 22:09:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, dfalcantara@chromium.org, fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/2478583004/#ps60001
(title: "do null check on webcontents")

[commit-bot: I haz the power, 2016-11-10 22:10:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-10 23:11:00]

Description was changed from

==========
implementation for new duplicate download UI

This change implements the new duplicate download UI.
It renames the DownloadOverwriteInfobarXXXX to DuplicateDownloadInfobarXXXX.
Chrome no longer show the overwrite button,
and always creating a new file if user click on the download button.

BUG=658239
==========

to

==========
implementation for new duplicate download UI

This change implements the new duplicate download UI.
It renames the DownloadOverwriteInfobarXXXX to DuplicateDownloadInfobarXXXX.
Chrome no longer show the overwrite button,
and always creating a new file if user click on the download button.

BUG=658239
==========

[commit-bot: I haz the power, 2016-11-10 23:11:02]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-10 23:28:42]

Description was changed from

==========
implementation for new duplicate download UI

This change implements the new duplicate download UI.
It renames the DownloadOverwriteInfobarXXXX to DuplicateDownloadInfobarXXXX.
Chrome no longer show the overwrite button,
and always creating a new file if user click on the download button.

BUG=658239
==========

to

==========
implementation for new duplicate download UI

This change implements the new duplicate download UI.
It renames the DownloadOverwriteInfobarXXXX to DuplicateDownloadInfobarXXXX.
Chrome no longer show the overwrite button,
and always creating a new file if user click on the download button.

BUG=658239

Committed: https://crrev.com/577d7c791ae9a560d457bb08a8a959bd89eeec30
Cr-Commit-Position: refs/heads/master@{#431389}
==========

[commit-bot: I haz the power, 2016-11-10 23:28:43]

Patchset 4 (id:??) landed as
https://crrev.com/577d7c791ae9a560d457bb08a8a959bd89eeec30
Cr-Commit-Position: refs/heads/master@{#431389}