Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
580043002:
[Android] Prompt with infobar on filename conflict (Closed)
|
Created:
6 years, 3 months ago by Changwan Ryu Modified:
5 years, 9 months ago CC:
chromium-reviews, benjhayden+dwatch_chromium.org, asanka, klobag.chromium Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Android] Prompt with infobar on filename conflict
Previously on Android, we silently uniquified when there is a filename
conflict. Now we're changing the behavior to prompt the user with
an infobar.
BUG=333085, 415711
TEST=1) Download same file twice, choose 'replace file', and checked that
file is overwritten.
2) Download same file twice, choose 'create new file', and checked
a new file is created.
3) Download same file twice, dismiss the infobar and checked download
did not happen.
4) Download file A twice, check the infobar, download file B twice,
and checked that two infobars have showed up, and checked that
both files can still be overwritten and new files can be created.
5) Download same file twice, check that infobar shows up, and navigate
to another link and checked that infobar is still there and you can
initiate the download.
Committed: https://crrev.com/fd2eb7523de2f1997d10ca2782745d487f8dff06
Cr-Commit-Position: refs/heads/master@{#319177}
Patch Set 1 #Patch Set 2 : #
Total comments: 2
Patch Set 3 : addressed ted's comments and added dismiss listener #
Total comments: 14
Patch Set 4 : addressed ted's and min's comments #Patch Set 5 : call onFileNotSelected() when uniquification fails #Patch Set 6 : switching to infobar implementation #Patch Set 7 : rebased #Patch Set 8 : #Patch Set 9 : full implementation #
Total comments: 10
Patch Set 10 : adding comments and a null check #Patch Set 11 : addressed ted's comments #
Total comments: 16
Patch Set 12 : addressed ted's comments #
Total comments: 50
Patch Set 13 : addressed pkasting@'s comments #
Total comments: 10
Patch Set 14 : addressed comments and added 'create new file' behavior #
Total comments: 4
Patch Set 15 : rebased and changed to sticky infobar #Patch Set 16 : also handles use cases from ChromeDownloadDelegate #
Total comments: 28
Patch Set 17 : addressed pkasting@'s comments #
Total comments: 14
Patch Set 18 : fixed nits #
Total comments: 15
Patch Set 19 : addressed qinmin's and tedchoc's comments #Patch Set 20 : avoided Java-only infobar #
Total comments: 44
Patch Set 21 : used Java DownloadInfo and fixed nits #
Total comments: 18
Patch Set 22 : #
Total comments: 4
Patch Set 23 : changed function order according to headers #Messages
Total messages: 67 (7 generated)
changwan@chromium.org changed reviewers: + asanka@chromium.org, qinmin@chromium.org, tedchoc@chromium.org
qinmin@chromium.org: Please review changes in select_file_dialog_android.* asanka@chromium.org: Please review changes in download_target_determiner.cc miguelg@chromium.org: Please review changes in SelectFileDialog.java, android_ui_strings.grd Thanks
On 2014/09/18 09:54:46, Changwan Ryu wrote: > mailto:qinmin@chromium.org: Please review changes in > select_file_dialog_android.* > > mailto:asanka@chromium.org: Please review changes in > download_target_determiner.cc > > mailto:miguelg@chromium.org: Please review changes in I mean tedchoc@, sorry. > SelectFileDialog.java, android_ui_strings.grd > > Thanks
On 2014/09/18 09:55:27, Changwan Ryu wrote: > On 2014/09/18 09:54:46, Changwan Ryu wrote: > > mailto:qinmin@chromium.org: Please review changes in > > select_file_dialog_android.* > > > > mailto:asanka@chromium.org: Please review changes in > > download_target_determiner.cc > > > > mailto:miguelg@chromium.org: Please review changes in > I mean tedchoc@, sorry. > > SelectFileDialog.java, android_ui_strings.grd > > > > Thanks Can you upload a screenshot of the dialog to the bug?
selectFileDialog is for file input, so I am not sure whether it is the right place to put the warning dialog here. Basically a simple infobar prompt is fine, since we already have those for dangerous download.
On 2014/09/18 22:25:12, qinmin wrote: > selectFileDialog is for file input, so I am not sure whether it is the right > place to put the warning dialog here. Basically a simple infobar prompt is fine, > since we already have those for dangerous download. Since select_file_dialog, its native corresponding unit, is responsible for handling all the types including SELECT_SAVEAS_FILE, I thought that it would be natural to handle it inside SelectFileDialog.java, or at least triggered from there. This logic probably needs to be applied whenever type is SELECT_SAVEAS_FILE. If it's not the case, could you point me where such logic would fit better? Thanks.
On 2014/09/18 18:19:58, Ted C wrote: > On 2014/09/18 09:55:27, Changwan Ryu wrote: > > On 2014/09/18 09:54:46, Changwan Ryu wrote: > > > mailto:qinmin@chromium.org: Please review changes in > > > select_file_dialog_android.* > > > > > > mailto:asanka@chromium.org: Please review changes in > > > download_target_determiner.cc > > > > > > mailto:miguelg@chromium.org: Please review changes in > > I mean tedchoc@, sorry. > > > SelectFileDialog.java, android_ui_strings.grd > > > > > > Thanks > > Can you upload a screenshot of the dialog to the bug? Screenshot uploaded on crbug.com/415711 (private). Thanks.
(with the caveat that I don't know this code well, so I will defer to Min about the file location stuff) https://codereview.chromium.org/580043002/diff/20001/ui/android/java/src/org/... File ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java (right): https://codereview.chromium.org/580043002/diff/20001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java:108: showOverwriteDialog(weakRefActivity.get(), window, defaultPath); I would rather you add a separate method triggered from native if you need to show this overwrite dialog. Then it could just take window and default path instead of trying to conflate the two.
https://codereview.chromium.org/580043002/diff/20001/ui/android/java/src/org/... File ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java (right): https://codereview.chromium.org/580043002/diff/20001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java:108: showOverwriteDialog(weakRefActivity.get(), window, defaultPath); On 2014/09/19 00:42:25, Ted C wrote: > I would rather you add a separate method triggered from native if you need to > show this overwrite dialog. > > Then it could just take window and default path instead of trying to conflate > the two. Done.
this looks fine to me, but need to make sure you satisfy Min's request https://codereview.chromium.org/580043002/diff/40001/ui/android/java/src/org/... File ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java (right): https://codereview.chromium.org/580043002/diff/40001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java:71: public void showOverwriteDialog(final String path, final WindowAndroid window) { I think this is only called from native right? It should be private in that case. https://codereview.chromium.org/580043002/diff/40001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java:87: path); this should fit on the prior line now. https://codereview.chromium.org/580043002/diff/40001/ui/android/java/strings/... File ui/android/java/strings/android_ui_strings.grd (right): https://codereview.chromium.org/580043002/diff/40001/ui/android/java/strings/... ui/android/java/strings/android_ui_strings.grd:154: Would you like to overwrite? Choosing 'cancel' will create a unique file name instead. Hopefully 'cancel' will be translated in a reasonable way in other languages. It might be better to say "Canceling[Declining] will create a unique file name instead." Then you're not as tied to the string on the button translating the same as in this sentence. https://codereview.chromium.org/580043002/diff/40001/ui/shell_dialogs/select_... File ui/shell_dialogs/select_file_dialog_android.cc (right): https://codereview.chromium.org/580043002/diff/40001/ui/shell_dialogs/select_... ui/shell_dialogs/select_file_dialog_android.cc:128: ScopedJavaLocalRef<jstring> jdefault_path = this block is indented 2 too many
https://codereview.chromium.org/580043002/diff/40001/ui/android/java/src/org/... File ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java (right): https://codereview.chromium.org/580043002/diff/40001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java:121: remove this line https://codereview.chromium.org/580043002/diff/40001/ui/shell_dialogs/select_... File ui/shell_dialogs/select_file_dialog_android.h (right): https://codereview.chromium.org/580043002/diff/40001/ui/shell_dialogs/select_... ui/shell_dialogs/select_file_dialog_android.h:34: ScopedJavaLocalRef<jstring> GetUniqueFilePath(JNIEnv* env, this doesn't needs to be a member function?
PTAL https://codereview.chromium.org/580043002/diff/40001/ui/android/java/src/org/... File ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java (right): https://codereview.chromium.org/580043002/diff/40001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java:71: public void showOverwriteDialog(final String path, final WindowAndroid window) { On 2014/09/19 22:31:40, Ted C wrote: > I think this is only called from native right? It should be private in that > case. Done. https://codereview.chromium.org/580043002/diff/40001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java:87: path); On 2014/09/19 22:31:40, Ted C wrote: > this should fit on the prior line now. Done. https://codereview.chromium.org/580043002/diff/40001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java:121: On 2014/09/20 05:06:58, qinmin wrote: > remove this line Done. https://codereview.chromium.org/580043002/diff/40001/ui/android/java/strings/... File ui/android/java/strings/android_ui_strings.grd (right): https://codereview.chromium.org/580043002/diff/40001/ui/android/java/strings/... ui/android/java/strings/android_ui_strings.grd:154: Would you like to overwrite? Choosing 'cancel' will create a unique file name instead. On 2014/09/19 22:31:40, Ted C wrote: > Hopefully 'cancel' will be translated in a reasonable way in other languages. > > It might be better to say "Canceling[Declining] will create a unique file name > instead." > > Then you're not as tied to the string on the button translating the same as in > this sentence. Fixed as 'canceling' https://codereview.chromium.org/580043002/diff/40001/ui/shell_dialogs/select_... File ui/shell_dialogs/select_file_dialog_android.cc (right): https://codereview.chromium.org/580043002/diff/40001/ui/shell_dialogs/select_... ui/shell_dialogs/select_file_dialog_android.cc:128: ScopedJavaLocalRef<jstring> jdefault_path = On 2014/09/19 22:31:40, Ted C wrote: > this block is indented 2 too many Done. https://codereview.chromium.org/580043002/diff/40001/ui/shell_dialogs/select_... ui/shell_dialogs/select_file_dialog_android.cc:128: ScopedJavaLocalRef<jstring> jdefault_path = On 2014/09/19 22:31:40, Ted C wrote: > this block is indented 2 too many Done. https://codereview.chromium.org/580043002/diff/40001/ui/shell_dialogs/select_... File ui/shell_dialogs/select_file_dialog_android.h (right): https://codereview.chromium.org/580043002/diff/40001/ui/shell_dialogs/select_... ui/shell_dialogs/select_file_dialog_android.h:34: ScopedJavaLocalRef<jstring> GetUniqueFilePath(JNIEnv* env, On 2014/09/20 05:06:58, qinmin wrote: > this doesn't needs to be a member function? Hmm.. This is accessed by Java. Is there any way I can get a static non-member function accessed by Java? Please let me know.
RBRSTMP LGTM for /download/ change.
lgtm % comment https://codereview.chromium.org/580043002/diff/40001/ui/shell_dialogs/select_... File ui/shell_dialogs/select_file_dialog_android.h (right): https://codereview.chromium.org/580043002/diff/40001/ui/shell_dialogs/select_... ui/shell_dialogs/select_file_dialog_android.h:34: ScopedJavaLocalRef<jstring> GetUniqueFilePath(JNIEnv* env, On 2014/09/22 14:02:38, Changwan Ryu wrote: > On 2014/09/20 05:06:58, qinmin wrote: > > this doesn't needs to be a member function? > > Hmm.. This is accessed by Java. Is there any way I can get a static non-member > function accessed by Java? Please let me know. In java side, if you declare you native call without the native pointer, it will call the static function inside this class. For example, you can have nativeGetUniqueFilepath(String path) in java. And then declare static ScopedJavaLocalRef<jstring> GetUniqueFilePath(JNIEnv* env, jclass, jstring) in the cc file.
On 2014/10/02 17:02:48, qinmin wrote: > lgtm % comment > > https://codereview.chromium.org/580043002/diff/40001/ui/shell_dialogs/select_... > File ui/shell_dialogs/select_file_dialog_android.h (right): > > https://codereview.chromium.org/580043002/diff/40001/ui/shell_dialogs/select_... > ui/shell_dialogs/select_file_dialog_android.h:34: ScopedJavaLocalRef<jstring> > GetUniqueFilePath(JNIEnv* env, > On 2014/09/22 14:02:38, Changwan Ryu wrote: > > On 2014/09/20 05:06:58, qinmin wrote: > > > this doesn't needs to be a member function? > > > > Hmm.. This is accessed by Java. Is there any way I can get a static non-member > > function accessed by Java? Please let me know. > > In java side, if you declare you native call without the native pointer, it will > call the static function inside this class. > For example, you can have > nativeGetUniqueFilepath(String path) in java. > And then declare static ScopedJavaLocalRef<jstring> GetUniqueFilePath(JNIEnv* > env, jclass, jstring) in the cc file. Do we want to land this based on the updates in: crbug.com/415711 It does look like we want to use an infobar instead of a dialog. Do you want to land this incrementally or should we just use an infobar now?
On 2014/10/02 20:28:25, Ted C wrote: > On 2014/10/02 17:02:48, qinmin wrote: > > lgtm % comment > > > > > https://codereview.chromium.org/580043002/diff/40001/ui/shell_dialogs/select_... > > File ui/shell_dialogs/select_file_dialog_android.h (right): > > > > > https://codereview.chromium.org/580043002/diff/40001/ui/shell_dialogs/select_... > > ui/shell_dialogs/select_file_dialog_android.h:34: ScopedJavaLocalRef<jstring> > > GetUniqueFilePath(JNIEnv* env, > > On 2014/09/22 14:02:38, Changwan Ryu wrote: > > > On 2014/09/20 05:06:58, qinmin wrote: > > > > this doesn't needs to be a member function? > > > > > > Hmm.. This is accessed by Java. Is there any way I can get a static > non-member > > > function accessed by Java? Please let me know. > > > > In java side, if you declare you native call without the native pointer, it > will > > call the static function inside this class. > > For example, you can have > > nativeGetUniqueFilepath(String path) in java. > > And then declare static ScopedJavaLocalRef<jstring> GetUniqueFilePath(JNIEnv* > > env, jclass, jstring) in the cc file. > > Do we want to land this based on the updates in: crbug.com/415711 > > It does look like we want to use an infobar instead of a dialog. Do you want to > land this incrementally or should we just use an infobar now? I'll be working on an infobar change. Thanks.
On 2014/10/02 17:02:48, qinmin wrote: > lgtm % comment > > https://codereview.chromium.org/580043002/diff/40001/ui/shell_dialogs/select_... > File ui/shell_dialogs/select_file_dialog_android.h (right): > > https://codereview.chromium.org/580043002/diff/40001/ui/shell_dialogs/select_... > ui/shell_dialogs/select_file_dialog_android.h:34: ScopedJavaLocalRef<jstring> > GetUniqueFilePath(JNIEnv* env, > On 2014/09/22 14:02:38, Changwan Ryu wrote: > > On 2014/09/20 05:06:58, qinmin wrote: > > > this doesn't needs to be a member function? > > > > Hmm.. This is accessed by Java. Is there any way I can get a static non-member > > function accessed by Java? Please let me know. > > In java side, if you declare you native call without the native pointer, it will > call the static function inside this class. > For example, you can have > nativeGetUniqueFilepath(String path) in java. > And then declare static ScopedJavaLocalRef<jstring> GetUniqueFilePath(JNIEnv* > env, jclass, jstring) in the cc file. Thanks for the tip.
On 2014/10/08 05:48:42, Changwan Ryu wrote: > On 2014/10/02 17:02:48, qinmin wrote: > > lgtm % comment > > > > > https://codereview.chromium.org/580043002/diff/40001/ui/shell_dialogs/select_... > > File ui/shell_dialogs/select_file_dialog_android.h (right): > > > > > https://codereview.chromium.org/580043002/diff/40001/ui/shell_dialogs/select_... > > ui/shell_dialogs/select_file_dialog_android.h:34: ScopedJavaLocalRef<jstring> > > GetUniqueFilePath(JNIEnv* env, > > On 2014/09/22 14:02:38, Changwan Ryu wrote: > > > On 2014/09/20 05:06:58, qinmin wrote: > > > > this doesn't needs to be a member function? > > > > > > Hmm.. This is accessed by Java. Is there any way I can get a static > non-member > > > function accessed by Java? Please let me know. > > > > In java side, if you declare you native call without the native pointer, it > will > > call the static function inside this class. > > For example, you can have > > nativeGetUniqueFilepath(String path) in java. > > And then declare static ScopedJavaLocalRef<jstring> GetUniqueFilePath(JNIEnv* > > env, jclass, jstring) in the cc file. > > Thanks for the tip. I've uploaded a new CL to switch to infobar implementation. Could you take another look?
Wanted to ask the big question about whether this should be using the confirm infobars before I dig in too much more. https://codereview.chromium.org/580043002/diff/170001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java (right): https://codereview.chromium.org/580043002/diff/170001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java:21: * Java version of the translate infobar this needs to be updated https://codereview.chromium.org/580043002/diff/170001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java:23: public class DownloadOverwriteInfoBar extends InfoBar { This looks like a ConfirmInfoBar at least from the visuals. Any reason we can't use that? https://codereview.chromium.org/580043002/diff/170001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/580043002/diff/170001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:191: YES, REPLACE Is full upper case consistent with our other infobars? I thought we used lowercase in grds and just had our xml files force it to uppercase (but I'm fine with being consistent). https://codereview.chromium.org/580043002/diff/170001/chrome/browser/android/... File chrome/browser/android/download_overwrite_infobar_delegate.h (right): https://codereview.chromium.org/580043002/diff/170001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. no (c) https://codereview.chromium.org/580043002/diff/170001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:19: class DownloadOverwriteInfoBarDelegate : public infobars::InfoBarDelegate { Similar question about whether we should be using ConfirmInfoBarDelegate here instead.
https://codereview.chromium.org/580043002/diff/170001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java (right): https://codereview.chromium.org/580043002/diff/170001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java:21: * Java version of the translate infobar On 2014/10/14 01:25:37, Ted C wrote: > this needs to be updated Done. https://codereview.chromium.org/580043002/diff/170001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java:23: public class DownloadOverwriteInfoBar extends InfoBar { On 2014/10/14 01:25:37, Ted C wrote: > This looks like a ConfirmInfoBar at least from the visuals. Any reason we can't > use that? The main reason for deviating from ConfirmInfoBar is as follows: 1) We need to use bold style and in-text link to meet the design requirement from the original bug. This cannot be handled by message text of ConfirmInfoBar. 2) The link should launch an intent instead of opening a new tab. https://codereview.chromium.org/580043002/diff/170001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/580043002/diff/170001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:191: YES, REPLACE On 2014/10/14 01:25:37, Ted C wrote: > Is full upper case consistent with our other infobars? I thought we used > lowercase in grds and just had our xml files force it to uppercase (but I'm fine > with being consistent). Done. https://codereview.chromium.org/580043002/diff/170001/chrome/browser/android/... File chrome/browser/android/download_overwrite_infobar_delegate.h (right): https://codereview.chromium.org/580043002/diff/170001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/10/14 01:25:37, Ted C wrote: > no (c) Done. https://codereview.chromium.org/580043002/diff/170001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:19: class DownloadOverwriteInfoBarDelegate : public infobars::InfoBarDelegate { On 2014/10/14 01:25:37, Ted C wrote: > Similar question about whether we should be using ConfirmInfoBarDelegate here > instead. As explained elsewhere, this requires some unique design elements such as bold file name and link for directory names (which should launch an intent). This could not be represented by GetMessageText(). And coming up with a very generic solution (such as what SpannableString provides) here seems hard and unnecessary.
changwan@chromium.org changed reviewers: + newt@chromium.org
overall, this looks reasonable to me. Sad we can't use a confirm inforbar, but formatting the string and including the intent redirector does seem to be pretty unique to this type. https://codereview.chromium.org/580043002/diff/210001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java (right): https://codereview.chromium.org/580043002/diff/210001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java:52: return isPrimaryButton ? InfoBar.ACTION_TYPE_OK : InfoBar.ACTION_TYPE_CANCEL; I would just inline this in the onButtonClicked method. https://codereview.chromium.org/580043002/diff/210001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java:72: return context.getString(R.string.download_overwrite_infobar_yes_replace_button); I would inline these two get*ButtonText methods as well. It makes this class more verbose than it needs to be. https://codereview.chromium.org/580043002/diff/210001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java:93: formattedDirName.setSpan(new ClickableSpan() { If the intent is null, then I don't think you should add the ClickableSpan at all. https://codereview.chromium.org/580043002/diff/210001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java:100: context.startActivity(dirNameIntent); Before calling this, you should see if the intent will resolve to an activity otherwise this could crash. I would do it before adding the span and in here for safety sake (don't add the span if there are no resolvers and in the callback in case you were to exit chrome, remove the resolve, come back and click this link). https://codereview.chromium.org/580043002/diff/210001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/580043002/diff/210001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:188: Do you want to replace the existing <ph name="FILE_NAME">^1<ex>specialfile.pdf</ex></ph> in your <ph name="DIRECTORY_NAME">^2<ex>Downloads</ex></ph>? should there be a "directory" after this placeholder? https://codereview.chromium.org/580043002/diff/210001/chrome/browser/android/... File chrome/browser/android/download_overwrite_infobar_delegate.cc (right): https://codereview.chromium.org/580043002/diff/210001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.cc:22: infobars::InfoBar* DownloadOverwriteInfoBarDelegate::Create( the ordering of the .cc should match exactly the .h (.ie. this came before the destructor in the header). https://codereview.chromium.org/580043002/diff/210001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.cc:56: const FileSelectedCallback& callback) : the : goes on the next line (aligning with the const above) https://codereview.chromium.org/580043002/diff/210001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/download_overwrite_infobar.cc (right): https://codereview.chromium.org/580043002/diff/210001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/download_overwrite_infobar.cc:30: DownloadOverwriteInfoBar::~DownloadOverwriteInfoBar() { same method ordering comment as the other file
https://codereview.chromium.org/580043002/diff/210001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java (right): https://codereview.chromium.org/580043002/diff/210001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java:52: return isPrimaryButton ? InfoBar.ACTION_TYPE_OK : InfoBar.ACTION_TYPE_CANCEL; On 2014/10/15 00:19:44, Ted C wrote: > I would just inline this in the onButtonClicked method. Done. https://codereview.chromium.org/580043002/diff/210001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java:72: return context.getString(R.string.download_overwrite_infobar_yes_replace_button); On 2014/10/15 00:19:44, Ted C wrote: > I would inline these two get*ButtonText methods as well. It makes this class > more verbose than it needs to be. Done. https://codereview.chromium.org/580043002/diff/210001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java:93: formattedDirName.setSpan(new ClickableSpan() { On 2014/10/15 00:19:44, Ted C wrote: > If the intent is null, then I don't think you should add the ClickableSpan at > all. Done. https://codereview.chromium.org/580043002/diff/210001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java:100: context.startActivity(dirNameIntent); On 2014/10/15 00:19:44, Ted C wrote: > Before calling this, you should see if the intent will resolve to an activity > otherwise this could crash. > > I would do it before adding the span and in here for safety sake (don't add the > span if there are no resolvers and in the callback in case you were to exit > chrome, remove the resolve, come back and click this link). Done. https://codereview.chromium.org/580043002/diff/210001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/580043002/diff/210001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:188: Do you want to replace the existing <ph name="FILE_NAME">^1<ex>specialfile.pdf</ex></ph> in your <ph name="DIRECTORY_NAME">^2<ex>Downloads</ex></ph>? On 2014/10/15 00:19:44, Ted C wrote: > should there be a "directory" after this placeholder? Done. https://codereview.chromium.org/580043002/diff/210001/chrome/browser/android/... File chrome/browser/android/download_overwrite_infobar_delegate.cc (right): https://codereview.chromium.org/580043002/diff/210001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.cc:22: infobars::InfoBar* DownloadOverwriteInfoBarDelegate::Create( On 2014/10/15 00:19:44, Ted C wrote: > the ordering of the .cc should match exactly the .h (.ie. this came before the > destructor in the header). Done. https://codereview.chromium.org/580043002/diff/210001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.cc:56: const FileSelectedCallback& callback) : On 2014/10/15 00:19:44, Ted C wrote: > the : goes on the next line (aligning with the const above) Done. https://codereview.chromium.org/580043002/diff/210001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/download_overwrite_infobar.cc (right): https://codereview.chromium.org/580043002/diff/210001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/download_overwrite_infobar.cc:30: DownloadOverwriteInfoBar::~DownloadOverwriteInfoBar() { On 2014/10/15 00:19:44, Ted C wrote: > same method ordering comment as the other file Done.
changwan@chromium.org changed reviewers: + pkasting@chromium.org
pkasting@chromium.org: Please review changes in infobars. Thanks.
unlock code 71028 On Tuesday, October 21, 2014 2:55:00 PM UTC-5, chan...@chromium.org wrote: > > pkas...@chromium.org <javascript:>: Please review changes in infobars. > Thanks. > > > > https://codereview.chromium.org/580043002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
You shouldn't be using infobars for this kind of user interaction. Please consult the UI leads for how to do this correctly and loop me in to any existing discussion you may have had about how to do this -- the bug linked here says nothing about infobars. I'm sorry to have to tell you this after you've done all the work to write this change.
On 2014/10/21 19:58:43, Peter Kasting wrote: > You shouldn't be using infobars for this kind of user interaction. Please > consult the UI leads for how to do this correctly and loop me in to any existing > discussion you may have had about how to do this -- the bug linked here says > nothing about infobars. There is a bug linked from the one in the patch that does talk about infobars https://code.google.com/p/chromium/issues/detail?id=415711 In particular rolfe@ in #12 says: """ Got UI Review consensus to show an infobar when the user clicks on the same download link twice """ > > I'm sorry to have to tell you this after you've done all the work to write this > change.
On 2014/10/21 20:02:29, Ted C wrote: > On 2014/10/21 19:58:43, Peter Kasting wrote: > > You shouldn't be using infobars for this kind of user interaction. Please > > consult the UI leads for how to do this correctly and loop me in to any > existing > > discussion you may have had about how to do this -- the bug linked here says > > nothing about infobars. > > There is a bug linked from the one in the patch that does talk about infobars > https://code.google.com/p/chromium/issues/detail?id=415711 > > In particular rolfe@ in #12 says: > """ > Got UI Review consensus to show an infobar when the user clicks on the same > download link twice > """ Thanks, I've replied on that bug.
https://codereview.chromium.org/580043002/diff/230001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/580043002/diff/230001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:194: No thanks "No thanks" is vague. As a user, I don't know if that means "stop download" or "save as some other name". How about we use "No, rename" (assuming that's accurate)? Also give the translators context about what these choices mean regardless. https://codereview.chromium.org/580043002/diff/230001/chrome/browser/android/... File chrome/browser/android/download_overwrite_infobar_delegate.cc (right): https://codereview.chromium.org/580043002/diff/230001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.cc:31: return NULL; 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!) https://codereview.chromium.org/580043002/diff/230001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.cc:33: if (!web_contents) When can this actually be NULL? https://codereview.chromium.org/580043002/diff/230001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.cc:39: DownloadOverwriteInfoBarDelegate* const delegate = 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. https://codereview.chromium.org/580043002/diff/230001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.cc:42: scoped_ptr<DownloadOverwriteInfoBarDelegate>(delegate)).release(); 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 https://codereview.chromium.org/580043002/diff/230001/chrome/browser/android/... File chrome/browser/android/download_overwrite_infobar_delegate.h (right): https://codereview.chromium.org/580043002/diff/230001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:20: // use by ChromeDownloadManagerDelegate. Nit: Maybe instead of a comment that's a bunch of class names, say in English what the purpose of this infobar is and where it appears? https://codereview.chromium.org/580043002/diff/230001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:25: // redefined to avoid a template-related compile error. What error? I'm confused why this needs to be here. https://codereview.chromium.org/580043002/diff/230001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:28: virtual ~DownloadOverwriteInfoBarDelegate(); Nit: Blank line above and below this so each type of method gets its own section https://codereview.chromium.org/580043002/diff/230001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:28: virtual ~DownloadOverwriteInfoBarDelegate(); Nit: "override" rather than "virtual" https://codereview.chromium.org/580043002/diff/230001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:35: virtual DownloadOverwriteInfoBarDelegate* Nit: No "virtual" when "override" is present https://codereview.chromium.org/580043002/diff/230001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:38: bool Accept(); Nit: Say what Accept() and Cancel() do. Perhaps you should rename these to be more descriptive, since you're not bound by the genericism of ConfirmInfoBarDelegate. https://codereview.chromium.org/580043002/diff/230001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:45: DownloadOverwriteInfoBarDelegate( Why is this protected rather than private? No one subclasses this class AFAICT. https://codereview.chromium.org/580043002/diff/230001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:51: int pending_id_; Nit: Document these. https://codereview.chromium.org/580043002/diff/230001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/download_overwrite_infobar.cc (right): https://codereview.chromium.org/580043002/diff/230001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/download_overwrite_infobar.cc:14: using base::android::ConvertUTF8ToJavaString; Nit: I believe at least this statement can be omitted without making the lines below overflow the 80-column limit. In general, avoid using directives unless they significantly improve the rest of the file. https://codereview.chromium.org/580043002/diff/230001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/download_overwrite_infobar.cc:20: return scoped_ptr<infobars::InfoBar>( Nit: Can use make_scoped_ptr, I think? https://codereview.chromium.org/580043002/diff/230001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/download_overwrite_infobar.cc:29: : InfoBarAndroid(delegate.PassAs<infobars::InfoBarDelegate>()), Nit: Can just use Pass() now https://codereview.chromium.org/580043002/diff/230001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/download_overwrite_infobar.cc:49: int action, const std::string& action_value) { Nit: More typical wrapping: void DownloadOverwriteInfoBar::ProcessButton(int action, const std::string& action_value) { https://codereview.chromium.org/580043002/diff/230001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/download_overwrite_infobar.cc:54: if (action == InfoBarAndroid::ACTION_OK) { Nit: {} unnecessary https://codereview.chromium.org/580043002/diff/230001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/download_overwrite_infobar.cc:59: DCHECK_EQ(InfoBarAndroid::ACTION_NONE, action); When is this arm reached? On clicking the close button? https://codereview.chromium.org/580043002/diff/230001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/download_overwrite_infobar.cc:66: return delegate()->AsDownloadOverwriteInfoBarDelegate(); Just static-cast here, since you know the type of the infobar delegate. This eliminates the need for AsDownloadOverwriteInfoBarDelegate() and thus the need to add knowledge of this type (even under ifdefs) to cross-platform files like infobar_delegate.*. (Since this is only called twice, you could even inline this into the two callers, but I'm fine either way, whatever you prefer.) https://codereview.chromium.org/580043002/diff/230001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/download_overwrite_infobar.h (right): https://codereview.chromium.org/580043002/diff/230001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/download_overwrite_infobar.h:24: virtual ~DownloadOverwriteInfoBar(); Nit: "override" rather than "virtual" https://codereview.chromium.org/580043002/diff/230001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/download_overwrite_infobar.h:31: virtual base::android::ScopedJavaLocalRef<jobject> CreateRenderInfoBar( Nit: "virtual" not necessary with "override" (2 places)
https://codereview.chromium.org/580043002/diff/230001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/580043002/diff/230001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:194: No thanks On 2014/10/24 01:32:22, Peter Kasting wrote: > "No thanks" is vague. As a user, I don't know if that means "stop download" or > "save as some other name". How about we use "No, rename" (assuming that's > accurate)? Also give the translators context about what these choices mean > regardless. This means 'stop download'. We decided not to rename / uniquify. Asked UX on the crbug. https://codereview.chromium.org/580043002/diff/230001/chrome/browser/android/... File chrome/browser/android/download_overwrite_infobar_delegate.cc (right): https://codereview.chromium.org/580043002/diff/230001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.cc:31: return NULL; On 2014/10/24 01:32:22, Peter Kasting wrote: > 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!) Removed DCHECK. Infobar won't be created in this case. https://codereview.chromium.org/580043002/diff/230001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.cc:33: if (!web_contents) On 2014/10/24 01:32:22, Peter Kasting wrote: > When can this actually be NULL? I'm not sure, but there were similar checks in other places, so I could not cross out the possibility of it being NULL. https://codereview.chromium.org/580043002/diff/230001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.cc:39: DownloadOverwriteInfoBarDelegate* const delegate = On 2014/10/24 01:32:22, Peter Kasting wrote: > 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. Done. Thanks for the detailed explanation. https://codereview.chromium.org/580043002/diff/230001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.cc:42: scoped_ptr<DownloadOverwriteInfoBarDelegate>(delegate)).release(); On 2014/10/24 01:32:22, Peter Kasting wrote: > 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 Done. https://codereview.chromium.org/580043002/diff/230001/chrome/browser/android/... File chrome/browser/android/download_overwrite_infobar_delegate.h (right): https://codereview.chromium.org/580043002/diff/230001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:20: // use by ChromeDownloadManagerDelegate. On 2014/10/24 01:32:22, Peter Kasting wrote: > Nit: Maybe instead of a comment that's a bunch of class names, say in English > what the purpose of this infobar is and where it appears? Done. https://codereview.chromium.org/580043002/diff/230001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:25: // redefined to avoid a template-related compile error. On 2014/10/24 01:32:22, Peter Kasting wrote: > What error? I'm confused why this needs to be here. Hmm... Removed as the compiler error isn't reproduced. https://codereview.chromium.org/580043002/diff/230001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:28: virtual ~DownloadOverwriteInfoBarDelegate(); On 2014/10/24 01:32:22, Peter Kasting wrote: > Nit: Blank line above and below this so each type of method gets its own section Done. https://codereview.chromium.org/580043002/diff/230001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:28: virtual ~DownloadOverwriteInfoBarDelegate(); On 2014/10/24 01:32:22, Peter Kasting wrote: > Nit: "override" rather than "virtual" Done. https://codereview.chromium.org/580043002/diff/230001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:35: virtual DownloadOverwriteInfoBarDelegate* On 2014/10/24 01:32:22, Peter Kasting wrote: > Nit: No "virtual" when "override" is present Done. https://codereview.chromium.org/580043002/diff/230001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:38: bool Accept(); On 2014/10/24 01:32:22, Peter Kasting wrote: > Nit: Say what Accept() and Cancel() do. > > Perhaps you should rename these to be more descriptive, since you're not bound > by the genericism of ConfirmInfoBarDelegate. Done. https://codereview.chromium.org/580043002/diff/230001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:45: DownloadOverwriteInfoBarDelegate( On 2014/10/24 01:32:22, Peter Kasting wrote: > Why is this protected rather than private? No one subclasses this class AFAICT. Done. https://codereview.chromium.org/580043002/diff/230001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:51: int pending_id_; On 2014/10/24 01:32:22, Peter Kasting wrote: > Nit: Document these. Done. https://codereview.chromium.org/580043002/diff/230001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/download_overwrite_infobar.cc (right): https://codereview.chromium.org/580043002/diff/230001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/download_overwrite_infobar.cc:14: using base::android::ConvertUTF8ToJavaString; On 2014/10/24 01:32:23, Peter Kasting wrote: > Nit: I believe at least this statement can be omitted without making the lines > below overflow the 80-column limit. > > In general, avoid using directives unless they significantly improve the rest of > the file. Done. https://codereview.chromium.org/580043002/diff/230001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/download_overwrite_infobar.cc:20: return scoped_ptr<infobars::InfoBar>( On 2014/10/24 01:32:22, Peter Kasting wrote: > Nit: Can use make_scoped_ptr, I think? Done. https://codereview.chromium.org/580043002/diff/230001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/download_overwrite_infobar.cc:29: : InfoBarAndroid(delegate.PassAs<infobars::InfoBarDelegate>()), On 2014/10/24 01:32:22, Peter Kasting wrote: > Nit: Can just use Pass() now Done. https://codereview.chromium.org/580043002/diff/230001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/download_overwrite_infobar.cc:49: int action, const std::string& action_value) { On 2014/10/24 01:32:22, Peter Kasting wrote: > Nit: More typical wrapping: > > void DownloadOverwriteInfoBar::ProcessButton(int action, > const std::string& action_value) { Done. https://codereview.chromium.org/580043002/diff/230001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/download_overwrite_infobar.cc:54: if (action == InfoBarAndroid::ACTION_OK) { On 2014/10/24 01:32:22, Peter Kasting wrote: > Nit: {} unnecessary Done. https://codereview.chromium.org/580043002/diff/230001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/download_overwrite_infobar.cc:59: DCHECK_EQ(InfoBarAndroid::ACTION_NONE, action); On 2014/10/24 01:32:22, Peter Kasting wrote: > When is this arm reached? On clicking the close button? Actually it shouldn't be reached. Changed as DCHECK(false). https://codereview.chromium.org/580043002/diff/230001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/download_overwrite_infobar.cc:66: return delegate()->AsDownloadOverwriteInfoBarDelegate(); On 2014/10/24 01:32:23, Peter Kasting wrote: > Just static-cast here, since you know the type of the infobar delegate. This > eliminates the need for AsDownloadOverwriteInfoBarDelegate() and thus the need > to add knowledge of this type (even under ifdefs) to cross-platform files like > infobar_delegate.*. > > (Since this is only called twice, you could even inline this into the two > callers, but I'm fine either way, whatever you prefer.) Fixed to static-cast. https://codereview.chromium.org/580043002/diff/230001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/download_overwrite_infobar.h (right): https://codereview.chromium.org/580043002/diff/230001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/download_overwrite_infobar.h:24: virtual ~DownloadOverwriteInfoBar(); On 2014/10/24 01:32:23, Peter Kasting wrote: > Nit: "override" rather than "virtual" Done. https://codereview.chromium.org/580043002/diff/230001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/download_overwrite_infobar.h:31: virtual base::android::ScopedJavaLocalRef<jobject> CreateRenderInfoBar( On 2014/10/24 01:32:23, Peter Kasting wrote: > Nit: "virtual" not necessary with "override" (2 places) Done.
few last little comments https://codereview.chromium.org/580043002/diff/230001/chrome/browser/android/... File chrome/browser/android/download_overwrite_infobar_delegate.cc (right): https://codereview.chromium.org/580043002/diff/230001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.cc:33: if (!web_contents) On 2014/10/27 06:40:04, Changwan Ryu wrote: > On 2014/10/24 01:32:22, Peter Kasting wrote: > > When can this actually be NULL? > > I'm not sure, but there were similar checks in other places, so I could not > cross out the possibility of it being NULL. 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. https://codereview.chromium.org/580043002/diff/250001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/580043002/diff/250001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:193: <message name="IDS_DOWNLOAD_OVERWRITE_INFOBAR_NO_THANKS_BUTTON" desc="Label for choosing 'no' in the prompt for replacing a file. [CHAR-LIMIT=32]"> I believe that Peter (correct me if I'm wrong) was asking for description clarification even if the message didn't change per UX. i.e. Here you would add, "If this option is selected, the existing file will not be replaced and no additional download will occur." https://codereview.chromium.org/580043002/diff/250001/chrome/browser/android/... File chrome/browser/android/download_overwrite_infobar_delegate.cc (right): https://codereview.chromium.org/580043002/diff/250001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.cc:55: DownloadOverwriteInfoBarDelegate::DownloadOverwriteInfoBarDelegate( this doesn't match the header order, the GetFileName()... are all public, so this should move to the bottom. https://codereview.chromium.org/580043002/diff/250001/chrome/browser/android/... File chrome/browser/android/download_overwrite_infobar_delegate.h (right): https://codereview.chromium.org/580043002/diff/250001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:20: // A class to implement an infobar that asks if it is ok to overwrite an exist an exist[ing] https://codereview.chromium.org/580043002/diff/250001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:22: // when we are downloading a file but there exists another file with the same I think this might be easier to read: Due to limited disk space on Android, two options are presented to the user when downloading a file whose name conflicts with an already present file: 1.) Overwrite the file. 2.) Do not download. The existing one is a bit confusing, but I'm not 100% happy with this either. https://codereview.chromium.org/580043002/diff/250001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/download_overwrite_infobar.cc (right): https://codereview.chromium.org/580043002/diff/250001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/download_overwrite_infobar.cc:42: env, java_delegate_.obj(), reinterpret_cast<intptr_t>(this), I think if the parameters don't all fit on a single line in C++, they all need to go on their own line. env, java_delegate_.obj(), ... j_dir_full_path.obj());
https://codereview.chromium.org/580043002/diff/230001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/580043002/diff/230001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:194: No thanks On 2014/10/27 06:40:04, Changwan Ryu wrote: > On 2014/10/24 01:32:22, Peter Kasting wrote: > > "No thanks" is vague. As a user, I don't know if that means "stop download" > or > > "save as some other name". How about we use "No, rename" (assuming that's > > accurate)? Also give the translators context about what these choices mean > > regardless. > > This means 'stop download'. We decided not to rename / uniquify. Asked UX on the > crbug. Making this stop the download seems bad, because then there's no way to download two copies of the file. The user can already stop the download by closing this infobar, it doesn't make sense to also make clicking this button stop things. https://codereview.chromium.org/580043002/diff/230001/chrome/browser/android/... File chrome/browser/android/download_overwrite_infobar_delegate.cc (right): https://codereview.chromium.org/580043002/diff/230001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.cc:33: if (!web_contents) On 2014/10/27 06:40:04, Changwan Ryu wrote: > On 2014/10/24 01:32:22, Peter Kasting wrote: > > When can this actually be NULL? > > I'm not sure, but there were similar checks in other places, so I could not > cross out the possibility of it being NULL. 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.
https://codereview.chromium.org/580043002/diff/230001/chrome/browser/android/... File chrome/browser/android/download_overwrite_infobar_delegate.cc (right): https://codereview.chromium.org/580043002/diff/230001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.cc:33: if (!web_contents) On 2014/10/27 15:30:11, Ted C wrote: > On 2014/10/27 06:40:04, Changwan Ryu wrote: > > On 2014/10/24 01:32:22, Peter Kasting wrote: > > > When can this actually be NULL? > > > > I'm not sure, but there were similar checks in other places, so I could not > > cross out the possibility of it being NULL. > > 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. 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. https://codereview.chromium.org/580043002/diff/250001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/580043002/diff/250001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:193: <message name="IDS_DOWNLOAD_OVERWRITE_INFOBAR_NO_THANKS_BUTTON" desc="Label for choosing 'no' in the prompt for replacing a file. [CHAR-LIMIT=32]"> On 2014/10/27 15:30:11, Ted C wrote: > I believe that Peter (correct me if I'm wrong) was asking for description > clarification even if the message didn't change per UX. > > i.e. Here you would add, "If this option is selected, the existing file will not > be replaced and no additional download will occur." It turned out that 'no thanks' means 'create new file'. With the updated UI string, the message is more self-explanatory, so I don't think I need to add additional description. Please correct me if it still needs one. https://codereview.chromium.org/580043002/diff/250001/chrome/browser/android/... File chrome/browser/android/download_overwrite_infobar_delegate.cc (right): https://codereview.chromium.org/580043002/diff/250001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.cc:55: DownloadOverwriteInfoBarDelegate::DownloadOverwriteInfoBarDelegate( On 2014/10/27 15:30:11, Ted C wrote: > this doesn't match the header order, the GetFileName()... are all public, so > this should move to the bottom. Done. https://codereview.chromium.org/580043002/diff/250001/chrome/browser/android/... File chrome/browser/android/download_overwrite_infobar_delegate.h (right): https://codereview.chromium.org/580043002/diff/250001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:20: // A class to implement an infobar that asks if it is ok to overwrite an exist On 2014/10/27 15:30:12, Ted C wrote: > an exist[ing] Done. https://codereview.chromium.org/580043002/diff/250001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:22: // when we are downloading a file but there exists another file with the same On 2014/10/27 15:30:11, Ted C wrote: > I think this might be easier to read: > > Due to limited disk space on Android, two options are presented to the user > when downloading a file whose name conflicts with an already present file: > 1.) Overwrite the file. > 2.) Do not download. > > The existing one is a bit confusing, but I'm not 100% happy with this either. Done. Please let me know if the comment still needs fixing. https://codereview.chromium.org/580043002/diff/250001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/download_overwrite_infobar.cc (right): https://codereview.chromium.org/580043002/diff/250001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/download_overwrite_infobar.cc:42: env, java_delegate_.obj(), reinterpret_cast<intptr_t>(this), On 2014/10/27 15:30:12, Ted C wrote: > I think if the parameters don't all fit on a single line in C++, they all need > to go on their own line. > > env, > java_delegate_.obj(), > ... > j_dir_full_path.obj()); Done.
PTAL https://codereview.chromium.org/580043002/diff/230001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/580043002/diff/230001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:194: No thanks On 2014/10/27 17:49:27, Peter Kasting wrote: > On 2014/10/27 06:40:04, Changwan Ryu wrote: > > On 2014/10/24 01:32:22, Peter Kasting wrote: > > > "No thanks" is vague. As a user, I don't know if that means "stop download" > > or > > > "save as some other name". How about we use "No, rename" (assuming that's > > > accurate)? Also give the translators context about what these choices mean > > > regardless. > > > > This means 'stop download'. We decided not to rename / uniquify. Asked UX on > the > > crbug. > > Making this stop the download seems bad, because then there's no way to download > two copies of the file. The user can already stop the download by closing this > infobar, it doesn't make sense to also make clicking this button stop things. Fixed to create a new file. Thanks for pointing it out.
lgtm https://codereview.chromium.org/580043002/diff/270001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java (right): https://codereview.chromium.org/580043002/diff/270001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java:38: public static final int ACTION_TYPE_OVERWRITE = 5; add a comment above these two like the translate/confirm ones above https://codereview.chromium.org/580043002/diff/270001/chrome/browser/android/... File chrome/browser/android/download_overwrite_infobar_delegate.cc (right): https://codereview.chromium.org/580043002/diff/270001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.cc:32: if (!download) At a minimum, you should be able to remove this as it is not consistent with the other file.
Thanks for keeping on top of things on the bug... it seems like people are still sort of going around about what they actually want the infobar behavior here to be. Let's get to the point where everything seems to be agreed-upon there, then ping me again on this review to take a look at the ultimate implementation. https://codereview.chromium.org/580043002/diff/230001/chrome/browser/android/... File chrome/browser/android/download_overwrite_infobar_delegate.cc (right): https://codereview.chromium.org/580043002/diff/230001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.cc:33: if (!web_contents) On 2014/10/28 04:59:43, Changwan Ryu wrote: > On 2014/10/27 15:30:11, Ted C wrote: > > On 2014/10/27 06:40:04, Changwan Ryu wrote: > > > On 2014/10/24 01:32:22, Peter Kasting wrote: > > > > When can this actually be NULL? > > > > > > I'm not sure, but there were similar checks in other places, so I could not > > > cross out the possibility of it being NULL. > > > > 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. > > 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. 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. > 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.) Then it doesn't sound like that's a case you'd need to worry about here. > I'd like to keep it. 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.
On 2014/10/29 04:08:03, Peter Kasting wrote: > Thanks for keeping on top of things on the bug... it seems like people are still > sort of going around about what they actually want the infobar behavior here to > be. Let's get to the point where everything seems to be agreed-upon there, then > ping me again on this review to take a look at the ultimate implementation. > > https://codereview.chromium.org/580043002/diff/230001/chrome/browser/android/... > File chrome/browser/android/download_overwrite_infobar_delegate.cc (right): > > https://codereview.chromium.org/580043002/diff/230001/chrome/browser/android/... > chrome/browser/android/download_overwrite_infobar_delegate.cc:33: if > (!web_contents) > On 2014/10/28 04:59:43, Changwan Ryu wrote: > > On 2014/10/27 15:30:11, Ted C wrote: > > > On 2014/10/27 06:40:04, Changwan Ryu wrote: > > > > On 2014/10/24 01:32:22, Peter Kasting wrote: > > > > > When can this actually be NULL? > > > > > > > > I'm not sure, but there were similar checks in other places, so I could > not > > > > cross out the possibility of it being NULL. > > > > > > 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. > > > > 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. > > 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. > > > 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.) > > Then it doesn't sound like that's a case you'd need to worry about here. > > > I'd like to keep it. > > 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. Any updates on this change?
On 2015/02/03 23:05:31, qinmin wrote: > On 2014/10/29 04:08:03, Peter Kasting wrote: > > Thanks for keeping on top of things on the bug... it seems like people are > still > > sort of going around about what they actually want the infobar behavior here > to > > be. Let's get to the point where everything seems to be agreed-upon there, > then > > ping me again on this review to take a look at the ultimate implementation. > > > > > https://codereview.chromium.org/580043002/diff/230001/chrome/browser/android/... > > File chrome/browser/android/download_overwrite_infobar_delegate.cc (right): > > > > > https://codereview.chromium.org/580043002/diff/230001/chrome/browser/android/... > > chrome/browser/android/download_overwrite_infobar_delegate.cc:33: if > > (!web_contents) > > On 2014/10/28 04:59:43, Changwan Ryu wrote: > > > On 2014/10/27 15:30:11, Ted C wrote: > > > > On 2014/10/27 06:40:04, Changwan Ryu wrote: > > > > > On 2014/10/24 01:32:22, Peter Kasting wrote: > > > > > > When can this actually be NULL? > > > > > > > > > > I'm not sure, but there were similar checks in other places, so I could > > not > > > > > cross out the possibility of it being NULL. > > > > > > > > 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. > > > > > > 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. > > > > 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. > > > > > 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.) > > > > Then it doesn't sound like that's a case you'd need to worry about here. > > > > > I'd like to keep it. > > > > 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. > > Any updates on this change? Apologies for the long break on this. Let me resume on it.
PTAL https://codereview.chromium.org/580043002/diff/270001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java (right): https://codereview.chromium.org/580043002/diff/270001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java:38: public static final int ACTION_TYPE_OVERWRITE = 5; On 2014/10/29 00:15:12, Ted C wrote: > add a comment above these two like the translate/confirm ones above Done. https://codereview.chromium.org/580043002/diff/270001/chrome/browser/android/... File chrome/browser/android/download_overwrite_infobar_delegate.cc (right): https://codereview.chromium.org/580043002/diff/270001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.cc:32: if (!download) On 2014/10/29 00:15:12, Ted C wrote: > At a minimum, you should be able to remove this as it is not > consistent with the other file. Done.
https://codereview.chromium.org/580043002/diff/310001/chrome/browser/android/... File chrome/browser/android/download_overwrite_infobar_delegate.cc (right): https://codereview.chromium.org/580043002/diff/310001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.cc:30: content::DownloadItem* download, This argument is only used to calculate the InfoBarService* and to pass to the constructor. Since the constructor doesn't use it (see below), just pass in the InfoBarService* directly. https://codereview.chromium.org/580043002/diff/310001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.cc:40: scoped_ptr<infobars::InfoBar> infobar = Nit: Inline this into the next statement to avoid the need to call Pass() there. https://codereview.chromium.org/580043002/diff/310001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.cc:47: bool DownloadOverwriteInfoBarDelegate::AcceptOverwrite() { If these two button-handler functions will always return true, just return void and do whatever you'd do with "return true" on the caller side unconditionally. https://codereview.chromium.org/580043002/diff/310001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.cc:59: if (uniquifier > 0) { What if uniquifer <= 0? Then it seems like you pass an empty path to the callback. That seems bad? https://codereview.chromium.org/580043002/diff/310001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.cc:97: content::DownloadItem* download, This argument seems to be unused. https://codereview.chromium.org/580043002/diff/310001/chrome/browser/android/... File chrome/browser/android/download_overwrite_infobar_delegate.h (right): https://codereview.chromium.org/580043002/diff/310001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:20: // A class to implement an infobar that asks if it is ok to overwrite an Nit: Remove "A class to implement" https://codereview.chromium.org/580043002/diff/310001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:28: // Also, you can dismiss the infobar. Nit: "The user can dismiss the infobar to abort the download."? https://codereview.chromium.org/580043002/diff/310001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:30: // Note that this infobar does not expire when the user navigates away. Nit: "when the user navigates away" -> "if the user subsequently navigates, since such navigations won't automatically cancel the underlying download"? https://codereview.chromium.org/580043002/diff/310001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:35: static infobars::InfoBar* Create( Looks like you don't use the return value, so return void. https://codereview.chromium.org/580043002/diff/310001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:42: bool AcceptOverwrite(); Nit: Consider calling this OverwriteExistingFile(), putting it with CreateNewFile(), and commenting the pair "These methods are called when the user selects the corresponding choice." https://codereview.chromium.org/580043002/diff/310001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:44: static void CreateNewFileInternal( Nit: Add a comment explaining what this does, and make it private. https://codereview.chromium.org/580043002/diff/310001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:55: // Implements InfoBarDelegate Nit: "infobars::InfoBarDelegate:" https://codereview.chromium.org/580043002/diff/310001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:56: void InfoBarDismissed() override; Nit: Swap the order of these two functions (to match the base class order), and make both private https://codereview.chromium.org/580043002/diff/310001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/download_overwrite_infobar.h (right): https://codereview.chromium.org/580043002/diff/310001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/download_overwrite_infobar.h:20: class DownloadOverwriteInfoBar : public InfoBarAndroid { Instead of adding this class, the Java-side class, and the new constants in InfoBarAndroid's ActionType, can't we just use a ConfirmInfoBar? That already allows you to display a custom message and two buttons with custom text, which is all you seem to need here. You wouldn't have snazzy styling of the file and directory names within the message, but does that really matter? (If it does, perhaps ConfirmInfoBar should be able to pass along some styling info to the Java side?)
Thanks for the careful review. PTAL. https://codereview.chromium.org/580043002/diff/310001/chrome/browser/android/... File chrome/browser/android/download_overwrite_infobar_delegate.cc (right): https://codereview.chromium.org/580043002/diff/310001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.cc:30: content::DownloadItem* download, On 2015/02/18 00:51:31, Peter Kasting wrote: > This argument is only used to calculate the InfoBarService* and to pass to the > constructor. Since the constructor doesn't use it (see below), just pass in the > InfoBarService* directly. Done. https://codereview.chromium.org/580043002/diff/310001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.cc:40: scoped_ptr<infobars::InfoBar> infobar = On 2015/02/18 00:51:31, Peter Kasting wrote: > Nit: Inline this into the next statement to avoid the need to call Pass() there. Done. https://codereview.chromium.org/580043002/diff/310001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.cc:47: bool DownloadOverwriteInfoBarDelegate::AcceptOverwrite() { On 2015/02/18 00:51:31, Peter Kasting wrote: > If these two button-handler functions will always return true, just return void > and do whatever you'd do with "return true" on the caller side unconditionally. Done. https://codereview.chromium.org/580043002/diff/310001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.cc:59: if (uniquifier > 0) { On 2015/02/18 00:51:31, Peter Kasting wrote: > What if uniquifer <= 0? Then it seems like you pass an empty path to the > callback. That seems bad? Fixed to overwrite the existing file. https://codereview.chromium.org/580043002/diff/310001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.cc:97: content::DownloadItem* download, On 2015/02/18 00:51:31, Peter Kasting wrote: > This argument seems to be unused. Removed https://codereview.chromium.org/580043002/diff/310001/chrome/browser/android/... File chrome/browser/android/download_overwrite_infobar_delegate.h (right): https://codereview.chromium.org/580043002/diff/310001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:20: // A class to implement an infobar that asks if it is ok to overwrite an On 2015/02/18 00:51:31, Peter Kasting wrote: > Nit: Remove "A class to implement" Done. https://codereview.chromium.org/580043002/diff/310001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:28: // Also, you can dismiss the infobar. On 2015/02/18 00:51:31, Peter Kasting wrote: > Nit: "The user can dismiss the infobar to abort the download."? Done. https://codereview.chromium.org/580043002/diff/310001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:30: // Note that this infobar does not expire when the user navigates away. On 2015/02/18 00:51:31, Peter Kasting wrote: > Nit: "when the user navigates away" -> "if the user subsequently navigates, > since such navigations won't automatically cancel the underlying download"? Done. https://codereview.chromium.org/580043002/diff/310001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:35: static infobars::InfoBar* Create( On 2015/02/18 00:51:31, Peter Kasting wrote: > Looks like you don't use the return value, so return void. Done. https://codereview.chromium.org/580043002/diff/310001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:42: bool AcceptOverwrite(); On 2015/02/18 00:51:31, Peter Kasting wrote: > Nit: Consider calling this OverwriteExistingFile(), putting it with > CreateNewFile(), and commenting the pair "These methods are called when the user > selects the corresponding choice." Done. https://codereview.chromium.org/580043002/diff/310001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:44: static void CreateNewFileInternal( On 2015/02/18 00:51:31, Peter Kasting wrote: > Nit: Add a comment explaining what this does, and make it private. Done. https://codereview.chromium.org/580043002/diff/310001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:55: // Implements InfoBarDelegate On 2015/02/18 00:51:31, Peter Kasting wrote: > Nit: "infobars::InfoBarDelegate:" Done. https://codereview.chromium.org/580043002/diff/310001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:56: void InfoBarDismissed() override; On 2015/02/18 00:51:31, Peter Kasting wrote: > Nit: Swap the order of these two functions (to match the base class order), and > make both private Done. https://codereview.chromium.org/580043002/diff/310001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/download_overwrite_infobar.h (right): https://codereview.chromium.org/580043002/diff/310001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/download_overwrite_infobar.h:20: class DownloadOverwriteInfoBar : public InfoBarAndroid { On 2015/02/18 00:51:31, Peter Kasting wrote: > Instead of adding this class, the Java-side class, and the new constants in > InfoBarAndroid's ActionType, can't we just use a ConfirmInfoBar? That already > allows you to display a custom message and two buttons with custom text, which > is all you seem to need here. You wouldn't have snazzy styling of the file and > directory names within the message, but does that really matter? (If it does, > perhaps ConfirmInfoBar should be able to pass along some styling info to the > Java side?) I discussed this with tedchoc@ and newt@. Because we do not provide a style generator and intent link generator in the native side, we decided to make a new info bar instead.
LGTM https://codereview.chromium.org/580043002/diff/330001/chrome/browser/android/... File chrome/browser/android/download_overwrite_infobar_delegate.cc (right): https://codereview.chromium.org/580043002/diff/330001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.cc:19: #include "ui/base/l10n/l10n_util.h" Many of these #includes (e.g. utf_string_conversions.h, theme_resources.h, l10n_util.h) seem unnecessary, please prune to the minimum list. (Do this in other files as well.) https://codereview.chromium.org/580043002/diff/330001/chrome/browser/android/... File chrome/browser/android/download_overwrite_infobar_delegate.h (right): https://codereview.chromium.org/580043002/diff/330001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:48: std::string GetDirFullPath() const; Instead of these three out-of-line accessors, just do: base::FilePath suggested_download_path() const { return suggested_download_path_; } After all, your caller wants to call all three right together anyway, so just let the caller pull the pieces out of the filepath that it desires. https://codereview.chromium.org/580043002/diff/330001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:55: // Implements infobars::InfoBarDelegate Nit: Remove "Implements " and add trailing colon https://codereview.chromium.org/580043002/diff/330001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:59: // Create a new file in content::BrowserThread::FILE thread, and when it's Nit: Use descriptive ("creates") verbs, not imperative ("create") (2 places) https://codereview.chromium.org/580043002/diff/330001/chrome/browser/download... File chrome/browser/download/chrome_download_manager_delegate.cc (right): https://codereview.chromium.org/580043002/diff/330001/chrome/browser/download... chrome/browser/download/chrome_download_manager_delegate.cc:583: if (!web_contents) When can this conditional be true? Please add comments (e.g. "May be NULL when testing") or remove if it can't actually be true. https://codereview.chromium.org/580043002/diff/330001/chrome/browser/download... chrome/browser/download/chrome_download_manager_delegate.cc:587: InfoBarService::FromWebContents(web_contents); Nit: Just inline this into the call below. https://codereview.chromium.org/580043002/diff/330001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/download_overwrite_infobar.cc (right): https://codereview.chromium.org/580043002/diff/330001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/download_overwrite_infobar.cc:56: DCHECK(false); Just to be sure, this function never gets reached when the user clicks the infobar's close button, right? (Otherwise your DCHECK is wrong.)
New patchsets have been uploaded after l-g-t-m from pkasting@chromium.org
https://codereview.chromium.org/580043002/diff/330001/chrome/browser/android/... File chrome/browser/android/download_overwrite_infobar_delegate.cc (right): https://codereview.chromium.org/580043002/diff/330001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.cc:19: #include "ui/base/l10n/l10n_util.h" On 2015/02/18 21:32:28, Peter Kasting wrote: > Many of these #includes (e.g. utf_string_conversions.h, theme_resources.h, > l10n_util.h) seem unnecessary, please prune to the minimum list. (Do this in > other files as well.) Done. https://codereview.chromium.org/580043002/diff/330001/chrome/browser/android/... File chrome/browser/android/download_overwrite_infobar_delegate.h (right): https://codereview.chromium.org/580043002/diff/330001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:48: std::string GetDirFullPath() const; On 2015/02/18 21:32:28, Peter Kasting wrote: > Instead of these three out-of-line accessors, just do: > > base::FilePath suggested_download_path() const { > return suggested_download_path_; > } > > After all, your caller wants to call all three right together anyway, so just > let the caller pull the pieces out of the filepath that it desires. Done. https://codereview.chromium.org/580043002/diff/330001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:55: // Implements infobars::InfoBarDelegate On 2015/02/18 21:32:28, Peter Kasting wrote: > Nit: Remove "Implements " and add trailing colon Done. https://codereview.chromium.org/580043002/diff/330001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.h:59: // Create a new file in content::BrowserThread::FILE thread, and when it's On 2015/02/18 21:32:28, Peter Kasting wrote: > Nit: Use descriptive ("creates") verbs, not imperative ("create") (2 places) Done. https://codereview.chromium.org/580043002/diff/330001/chrome/browser/download... File chrome/browser/download/chrome_download_manager_delegate.cc (right): https://codereview.chromium.org/580043002/diff/330001/chrome/browser/download... chrome/browser/download/chrome_download_manager_delegate.cc:583: if (!web_contents) On 2015/02/18 21:32:29, Peter Kasting wrote: > When can this conditional be true? Please add comments (e.g. "May be NULL when > testing") or remove if it can't actually be true. Removed https://codereview.chromium.org/580043002/diff/330001/chrome/browser/download... chrome/browser/download/chrome_download_manager_delegate.cc:587: InfoBarService::FromWebContents(web_contents); On 2015/02/18 21:32:29, Peter Kasting wrote: > Nit: Just inline this into the call below. Done. https://codereview.chromium.org/580043002/diff/330001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/download_overwrite_infobar.cc (right): https://codereview.chromium.org/580043002/diff/330001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/download_overwrite_infobar.cc:56: DCHECK(false); On 2015/02/18 21:32:29, Peter Kasting wrote: > Just to be sure, this function never gets reached when the user clicks the > infobar's close button, right? (Otherwise your DCHECK is wrong.) Correct. We have a separate function to handle close button - InfoBarDismissed().
lgtm % nits https://codereview.chromium.org/580043002/diff/350001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java (right): https://codereview.chromium.org/580043002/diff/350001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java:310: private boolean launchInfoBarIfFileExists(final DownloadInfo info) { java doc https://codereview.chromium.org/580043002/diff/350001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java (right): https://codereview.chromium.org/580043002/diff/350001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java:89: private static Intent getIntentForDirectoryLaunch(String dirFullPath) { javadoc here and below https://codereview.chromium.org/580043002/diff/350001/chrome/browser/android/... File chrome/browser/android/download_overwrite_infobar_delegate.cc (right): https://codereview.chromium.org/580043002/diff/350001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.cc:20: } nit: move this to the previous line https://codereview.chromium.org/580043002/diff/350001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/download_overwrite_infobar.cc (right): https://codereview.chromium.org/580043002/diff/350001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/download_overwrite_infobar.cc:24: } nit: you can move this to the line above
https://codereview.chromium.org/580043002/diff/350001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java (right): https://codereview.chromium.org/580043002/diff/350001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java:323: InfoBar infoBar = new DownloadOverwriteInfoBar(0, new DownloadOverwriteInfoBar.Callback() { We are really trying to avoid any java only infobar additions as we'd like to slowly migrate to only a single path for infobars in the future. Is there no way this can be converted to use a native delegate like the other usage? Maybe with more params when constructing the delegate? https://codereview.chromium.org/580043002/diff/350001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java (right): https://codereview.chromium.org/580043002/diff/350001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java:33: public interface Callback { FWIW, I think you should always more verbosely qualify internal Callbacks because it becomes very hard to understand them outside the class (i.e. DownloadOverwriteCallback). With that said, I "hope" there is a way we can do this without the callback and from native. https://codereview.chromium.org/580043002/diff/350001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java:78: : InfoBar.ACTION_TYPE_CREATE_NEW_FILE; this is C++ style indenting and it should be 8 from the start of the previous line.
New patchsets have been uploaded after l-g-t-m from qinmin@chromium.org
https://codereview.chromium.org/580043002/diff/350001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java (right): https://codereview.chromium.org/580043002/diff/350001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java:310: private boolean launchInfoBarIfFileExists(final DownloadInfo info) { On 2015/02/20 01:00:40, qinmin wrote: > java doc Done. https://codereview.chromium.org/580043002/diff/350001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java:323: InfoBar infoBar = new DownloadOverwriteInfoBar(0, new DownloadOverwriteInfoBar.Callback() { On 2015/02/20 01:48:08, Ted C wrote: > We are really trying to avoid any java only infobar additions as we'd like to > slowly migrate to only a single path for infobars in the future. > > Is there no way this can be converted to use a native delegate like the other > usage? > > Maybe with more params when constructing the delegate? Hmm... I don't have a clear idea how this should be done, but it sounds like a big change. Basically, since we are allowing multiple infobars, we should keep track of Java DownloadInfo objects or something similar in the ChromeDownloadDelegate so that native callbacks can be correctly hooked up with Java side callbacks. Then we need to make sure that we remove those objects when infobars are dismissed. Finally, native ChromeDownloadDelegate should be changed so that it can have the same life cycle with the Java counterpart, and enable the native to call the Java functions. If you're ok, I'd like to leave it as a TODO and try it in a separate CL. https://codereview.chromium.org/580043002/diff/350001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java (right): https://codereview.chromium.org/580043002/diff/350001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java:33: public interface Callback { On 2015/02/20 01:48:08, Ted C wrote: > FWIW, I think you should always more verbosely qualify internal Callbacks > because it becomes very hard to understand them outside the class (i.e. > DownloadOverwriteCallback). > > With that said, I "hope" there is a way we can do this without the callback and > from native. Done. https://codereview.chromium.org/580043002/diff/350001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java:78: : InfoBar.ACTION_TYPE_CREATE_NEW_FILE; On 2015/02/20 01:48:08, Ted C wrote: > this is C++ style indenting and it should be 8 from the start of the previous > line. Done. https://codereview.chromium.org/580043002/diff/350001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java:89: private static Intent getIntentForDirectoryLaunch(String dirFullPath) { On 2015/02/20 01:00:40, qinmin wrote: > javadoc here and below Done. https://codereview.chromium.org/580043002/diff/350001/chrome/browser/android/... File chrome/browser/android/download_overwrite_infobar_delegate.cc (right): https://codereview.chromium.org/580043002/diff/350001/chrome/browser/android/... chrome/browser/android/download_overwrite_infobar_delegate.cc:20: } On 2015/02/20 01:00:40, qinmin wrote: > nit: move this to the previous line This is the result of git cl format. It seems that in .h files {} in one line is more prevalent, while { (new line) } is more prevalent in .cc files. https://codereview.chromium.org/580043002/diff/350001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/download_overwrite_infobar.cc (right): https://codereview.chromium.org/580043002/diff/350001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/download_overwrite_infobar.cc:24: } On 2015/02/20 01:00:40, qinmin wrote: > nit: you can move this to the line above This is the result of git cl format. It seems that in .h files {} in one line is more prevalent, while { (new line) } is more prevalent in .cc files.
I'm fine with a TODO, but I don't see where the complexity is coming from. https://codereview.chromium.org/580043002/diff/350001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java (right): https://codereview.chromium.org/580043002/diff/350001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java:323: InfoBar infoBar = new DownloadOverwriteInfoBar(0, new DownloadOverwriteInfoBar.Callback() { On 2015/02/23 06:50:37, Changwan Ryu wrote: > On 2015/02/20 01:48:08, Ted C wrote: > > We are really trying to avoid any java only infobar additions as we'd like to > > slowly migrate to only a single path for infobars in the future. > > > > Is there no way this can be converted to use a native delegate like the other > > usage? > > > > Maybe with more params when constructing the delegate? > > Hmm... I don't have a clear idea how this should be done, but it sounds like a > big change. > > Basically, since we are allowing multiple infobars, we should keep track of Java > DownloadInfo objects or something similar in the ChromeDownloadDelegate so that > native callbacks can be correctly hooked up with Java side callbacks. > > Then we need to make sure that we remove those objects when infobars are > dismissed. > > Finally, native ChromeDownloadDelegate should be changed so that it can have the > same life cycle with the Java counterpart, and enable the native to call the > Java functions. > > If you're ok, I'd like to leave it as a TODO and try it in a separate CL. Couldn't you just have a different native DownloadOverwriteInfobarDelegate::Create method that takes a DownloadInfo object? You have access to the Tab->WebContents in this class so it should be relatively easy to add another native infobar the same way you do it from chrome_download_manager_delegate.cc. I don't want this to be something that is "tried", I want it to be done. So I would need pretty strong convincing arguments why it is really had to pipe it through the same code path.
On 2015/02/23 18:48:15, Ted C wrote: > I'm fine with a TODO, but I don't see where the complexity is coming from. > > https://codereview.chromium.org/580043002/diff/350001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java > (right): > > https://codereview.chromium.org/580043002/diff/350001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java:323: > InfoBar infoBar = new DownloadOverwriteInfoBar(0, new > DownloadOverwriteInfoBar.Callback() { > On 2015/02/23 06:50:37, Changwan Ryu wrote: > > On 2015/02/20 01:48:08, Ted C wrote: > > > We are really trying to avoid any java only infobar additions as we'd like > to > > > slowly migrate to only a single path for infobars in the future. > > > > > > Is there no way this can be converted to use a native delegate like the > other > > > usage? > > > > > > Maybe with more params when constructing the delegate? > > > > Hmm... I don't have a clear idea how this should be done, but it sounds like a > > big change. > > > > Basically, since we are allowing multiple infobars, we should keep track of > Java > > DownloadInfo objects or something similar in the ChromeDownloadDelegate so > that > > native callbacks can be correctly hooked up with Java side callbacks. > > > > Then we need to make sure that we remove those objects when infobars are > > dismissed. > > > > Finally, native ChromeDownloadDelegate should be changed so that it can have > the > > same life cycle with the Java counterpart, and enable the native to call the > > Java functions. > > > > If you're ok, I'd like to leave it as a TODO and try it in a separate CL. > > Couldn't you just have a different native > DownloadOverwriteInfobarDelegate::Create method that takes a DownloadInfo > object? > > You have access to the Tab->WebContents in this class so it should be relatively > easy to add another native infobar the same way you do it from > chrome_download_manager_delegate.cc. > > I don't want this to be something that is "tried", I want it to be done. So I > would need pretty strong convincing arguments why it is really had to pipe it > through the same code path. Sure, I've uploaded a new patch to avoid Java-only infobar usage. In doing so, I've moved delegate files under download/, and made the class abstract and derived two classes from it. Also added a callback class to keep track of Java ChromeDownloadDelegate. Please take another look.
I hope this is what Ted wanted. I haven't been paying attention to that subthread. https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... File chrome/browser/android/download/chrome_download_delegate.cc (right): https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... chrome/browser/android/download/chrome_download_delegate.cc:5: #include <chrome/browser/android/download/download_overwrite_infobar_delegate_java_initiated.h> Both the location of this include and its use of angle brackets are something outside my experience. https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... File chrome/browser/android/download/download_overwrite_info.h (right): https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... chrome/browser/android/download/download_overwrite_info.h:18: DownloadOverwriteInfo(std::string file_name, const &s? Can we store a FilePath instead of the (file_name, dir_name, dir_full_path) triple? And a GURL for the URL? https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... chrome/browser/android/download/download_overwrite_info.h:29: bool is_GET_request); Nit: Don't capitalize GET here (or elsewhere) https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... chrome/browser/android/download/download_overwrite_info.h:47: // Default copy constructor is used for passing this struct by value. Nit: No need for this comment, allowing copies of data-only structs is standard in Google style https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... File chrome/browser/android/download/download_overwrite_infobar_delegate.h (right): https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... chrome/browser/android/download/download_overwrite_infobar_delegate.h:39: virtual std::string GetFileName() const = 0; This should be a single FilePath accessor (which should be doable if you make DownloadOverwriteInfo use a FilePath instead of three strings). https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... File chrome/browser/android/download/download_overwrite_infobar_delegate_java_initiated.h (right): https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... chrome/browser/android/download/download_overwrite_infobar_delegate_java_initiated.h:25: typedef base::Callback<void(const DownloadOverwriteInfo&)> Typedefs go above members https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... chrome/browser/android/download/download_overwrite_infobar_delegate_java_initiated.h:26: FileSelectedFromDownloadOverwriteInfoCallback; Nit: This name seems a bit verbose, can you shorten it? What about e.g. FileSelectedCallback? https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... chrome/browser/android/download/download_overwrite_infobar_delegate_java_initiated.h:33: void OverwriteExistingFile() override; Nit: "// DownloadOverwriteInfoBarDelegate:" How come these are public, while the overrides below are private? Can they all be private, assuming the caller accesses these functions polymorphically through a pointer to the base class? https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... chrome/browser/android/download/download_overwrite_infobar_delegate_java_initiated.h:44: // infobars::InfoBarDelegateForDownloadOverwriteInfo: This is not a class name? And if you meant to write "infobars::InfoBarDelegate:", that also wouldn't be right, since you don't directly inherit from that -- this should also be "// DownloadOverwriteInfoBarDelegate:" https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... File chrome/browser/android/download/download_overwrite_infobar_delegate_native_initiated.cc (right): https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... chrome/browser/android/download/download_overwrite_infobar_delegate_native_initiated.cc:45: const { Nit: Arguable, but I think this wrapping would be better: std::string DownloadOverwriteInfoBarDelegateNativeInitiated::GetFileName() const { Basically, const and override should go on the same line as the argument list close paren unless there's no other way. https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... chrome/browser/android/download/download_overwrite_infobar_delegate_native_initiated.cc:70: return false; Seems like method implementations like this which are common to both your subclasses should just be implemented in the parent class. https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... File chrome/browser/android/download/download_overwrite_infobar_delegate_native_initiated.h (right): https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... chrome/browser/android/download/download_overwrite_infobar_delegate_native_initiated.h:37: std::string GetFileName() const override; Same comments as other header https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... chrome/browser/android/download/download_overwrite_infobar_delegate_native_initiated.h:46: // infobars::InfoBarDelegate: See comment in other header re: you no longer inherit directly from this class https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... chrome/browser/android/download/download_overwrite_infobar_delegate_native_initiated.h:52: // done, calls |callback| in the UI thread. Nit: Clearer: Called on the FILE thread to create a new file. Calls |callback| on the UI thread when finished.
<sidebar> When I say it's fine to leave something as a TODO, I do mean that. Often you'll find it easier to iterate on multiple changes if you get a baseline checked in.</sidebar> https://codereview.chromium.org/580043002/diff/390001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java (right): https://codereview.chromium.org/580043002/diff/390001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java:33: public interface DownloadOverwriteCallback { is this used anymore? https://codereview.chromium.org/580043002/diff/390001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java:71: if (mNativeInfoBarPtr != 0) nativeOnCloseButtonClicked(mNativeInfoBarPtr); Isn't it always a native info bar, so these checks are no longer needed? https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... File chrome/browser/android/download/chrome_download_delegate.cc (right): https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... chrome/browser/android/download/chrome_download_delegate.cc:89: Java_ChromeDownloadDelegate_enqueueDownloadManagerRequestFromNative( and any reason the delegate of the new class can't do this internally instead of having to pass a callback? Just wondering why it can't be a standalone delegate. https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... chrome/browser/android/download/chrome_download_delegate.cc:121: chrome::android::DownloadOverwriteInfo download_overwrite_info( Any reason we don't just pass down the DownloadInfo java object and just use that if need be? https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... File chrome/browser/android/download/download_overwrite_info.h (right): https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... chrome/browser/android/download/download_overwrite_info.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. no need (c) here or anywhere else. https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... File chrome/browser/android/download/download_overwrite_infobar_delegate_java_initiated.cc (right): https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... chrome/browser/android/download/download_overwrite_infobar_delegate_java_initiated.cc:48: std::string DownloadOverwriteInfoBarDelegateJavaInitiated::GetDirFullPath() add a blank line above this. https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... File chrome/browser/android/download/download_overwrite_infobar_delegate_java_initiated.h (right): https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... chrome/browser/android/download/download_overwrite_infobar_delegate_java_initiated.h:20: class DownloadOverwriteInfoBarDelegateJavaInitiated The names of these two files is odd to me. Is the real issue of whether it's triggered from java vs native? If so, I would argue as to why we have two code paths to trigger this (or why we couldn't combine the logic entirely). Or is the issue that one goes through the android download manager vs one that goes through chrome download manager. If so, I think we want the name to reflect their usage instead of their origin of trigger. https://codereview.chromium.org/580043002/diff/390001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/download_overwrite_infobar.cc (right): https://codereview.chromium.org/580043002/diff/390001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/download_overwrite_infobar.cc:33: java_delegate_.Reset(Java_DownloadOverwriteInfoBarDelegate_create(env)); I find it quite odd that the infobar creates the java delegate instead of the C++ delegate doing it. Looking at AppBannerInfoBarDelegate, it creates and owns the java delegate itself, which I think is closer to the model we want to follow.
https://codereview.chromium.org/580043002/diff/390001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java (right): https://codereview.chromium.org/580043002/diff/390001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java:33: public interface DownloadOverwriteCallback { On 2015/02/26 00:57:55, Ted C wrote: > is this used anymore? Removed https://codereview.chromium.org/580043002/diff/390001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java:71: if (mNativeInfoBarPtr != 0) nativeOnCloseButtonClicked(mNativeInfoBarPtr); On 2015/02/26 00:57:55, Ted C wrote: > Isn't it always a native info bar, so these checks are no longer needed? Removed https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... File chrome/browser/android/download/chrome_download_delegate.cc (right): https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... chrome/browser/android/download/chrome_download_delegate.cc:5: #include <chrome/browser/android/download/download_overwrite_infobar_delegate_java_initiated.h> On 2015/02/25 06:24:57, Peter Kasting wrote: > Both the location of this include and its use of angle brackets are something > outside my experience. Must be the result of eclipse auto refactoring. Done. https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... chrome/browser/android/download/chrome_download_delegate.cc:89: Java_ChromeDownloadDelegate_enqueueDownloadManagerRequestFromNative( On 2015/02/26 00:57:55, Ted C wrote: > and any reason the delegate of the new class can't do this internally instead of > having to pass a callback? > > Just wondering why it can't be a standalone delegate. Hmm... I thought I needed it to keep reference to the Java object. After giving it more thought I removed this callback and passed JavaObjectWeakGlobalRef (for ChromeDownloadDelegate) and ScopedJavaGlobalRef (for DownloadInfo). https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... chrome/browser/android/download/chrome_download_delegate.cc:121: chrome::android::DownloadOverwriteInfo download_overwrite_info( On 2015/02/26 00:57:55, Ted C wrote: > Any reason we don't just pass down the DownloadInfo java object and just use > that if need be? Done. We can pass downloadinfo by wrapping it inside ScopedJavaGlobalRef. https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... File chrome/browser/android/download/download_overwrite_info.h (right): https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... chrome/browser/android/download/download_overwrite_info.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2015/02/26 00:57:56, Ted C wrote: > no need (c) here or anywhere else. Removed https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... chrome/browser/android/download/download_overwrite_info.h:18: DownloadOverwriteInfo(std::string file_name, On 2015/02/25 06:24:57, Peter Kasting wrote: > const &s? > > Can we store a FilePath instead of the (file_name, dir_name, dir_full_path) > triple? And a GURL for the URL? For the other code path that uses AndroidDownloadManager on the Java side, I think it's better to keep it as is. Removed URL by passing Java downloadinfo directly. https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... chrome/browser/android/download/download_overwrite_info.h:29: bool is_GET_request); On 2015/02/25 06:24:57, Peter Kasting wrote: > Nit: Don't capitalize GET here (or elsewhere) Removed by passing Java DownloadInfo directly. https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... chrome/browser/android/download/download_overwrite_info.h:47: // Default copy constructor is used for passing this struct by value. On 2015/02/25 06:24:57, Peter Kasting wrote: > Nit: No need for this comment, allowing copies of data-only structs is standard > in Google style Class removed. https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... File chrome/browser/android/download/download_overwrite_infobar_delegate.h (right): https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... chrome/browser/android/download/download_overwrite_infobar_delegate.h:39: virtual std::string GetFileName() const = 0; On 2015/02/25 06:24:57, Peter Kasting wrote: > This should be a single FilePath accessor (which should be doable if you make > DownloadOverwriteInfo use a FilePath instead of three strings). Hmm... Since these values are from Java side in the case of AndroidDownloadManager, and we pass the values directly to the Java for both scenarios, I don't see much value in converting it into FilePath. https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... File chrome/browser/android/download/download_overwrite_infobar_delegate_java_initiated.cc (right): https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... chrome/browser/android/download/download_overwrite_infobar_delegate_java_initiated.cc:48: std::string DownloadOverwriteInfoBarDelegateJavaInitiated::GetDirFullPath() On 2015/02/26 00:57:56, Ted C wrote: > add a blank line above this. Done. https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... File chrome/browser/android/download/download_overwrite_infobar_delegate_java_initiated.h (right): https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... chrome/browser/android/download/download_overwrite_infobar_delegate_java_initiated.h:20: class DownloadOverwriteInfoBarDelegateJavaInitiated On 2015/02/26 00:57:56, Ted C wrote: > The names of these two files is odd to me. Is the real issue of whether it's > triggered from java vs native? If so, I would argue as to why we have two code > paths to trigger this (or why we couldn't combine the logic entirely). > > Or is the issue that one goes through the android download manager vs one that > goes through chrome download manager. If so, I think we want the name to > reflect their usage instead of their origin of trigger. Done. https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... chrome/browser/android/download/download_overwrite_infobar_delegate_java_initiated.h:25: typedef base::Callback<void(const DownloadOverwriteInfo&)> On 2015/02/25 06:24:57, Peter Kasting wrote: > Typedefs go above members I see. Removed now since the callback use case is now gone. https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... chrome/browser/android/download/download_overwrite_infobar_delegate_java_initiated.h:26: FileSelectedFromDownloadOverwriteInfoCallback; On 2015/02/25 06:24:57, Peter Kasting wrote: > Nit: This name seems a bit verbose, can you shorten it? What about e.g. > FileSelectedCallback? Ditto https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... chrome/browser/android/download/download_overwrite_infobar_delegate_java_initiated.h:33: void OverwriteExistingFile() override; On 2015/02/25 06:24:57, Peter Kasting wrote: > Nit: "// DownloadOverwriteInfoBarDelegate:" > > How come these are public, while the overrides below are private? Can they all > be private, assuming the caller accesses these functions polymorphically through > a pointer to the base class? Done. https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... chrome/browser/android/download/download_overwrite_infobar_delegate_java_initiated.h:44: // infobars::InfoBarDelegateForDownloadOverwriteInfo: On 2015/02/25 06:24:57, Peter Kasting wrote: > This is not a class name? And if you meant to write > "infobars::InfoBarDelegate:", that also wouldn't be right, since you don't > directly inherit from that -- this should also be "// > DownloadOverwriteInfoBarDelegate:" Done. https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... File chrome/browser/android/download/download_overwrite_infobar_delegate_native_initiated.cc (right): https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... chrome/browser/android/download/download_overwrite_infobar_delegate_native_initiated.cc:45: const { On 2015/02/25 06:24:57, Peter Kasting wrote: > Nit: Arguable, but I think this wrapping would be better: > > std::string > DownloadOverwriteInfoBarDelegateNativeInitiated::GetFileName() const { > > Basically, const and override should go on the same line as the argument list > close paren unless there's no other way. Hmm... I tried it but git cl format reverses the change probably because only 'const' part went to the next line. I'll leave it as is for consistency. https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... chrome/browser/android/download/download_overwrite_infobar_delegate_native_initiated.cc:70: return false; On 2015/02/25 06:24:57, Peter Kasting wrote: > Seems like method implementations like this which are common to both your > subclasses should just be implemented in the parent class. Done. https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... File chrome/browser/android/download/download_overwrite_infobar_delegate_native_initiated.h (right): https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... chrome/browser/android/download/download_overwrite_infobar_delegate_native_initiated.h:37: std::string GetFileName() const override; On 2015/02/25 06:24:57, Peter Kasting wrote: > Same comments as other header Done. https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... chrome/browser/android/download/download_overwrite_infobar_delegate_native_initiated.h:46: // infobars::InfoBarDelegate: On 2015/02/25 06:24:57, Peter Kasting wrote: > See comment in other header re: you no longer inherit directly from this class Done. https://codereview.chromium.org/580043002/diff/390001/chrome/browser/android/... chrome/browser/android/download/download_overwrite_infobar_delegate_native_initiated.h:52: // done, calls |callback| in the UI thread. On 2015/02/25 06:24:57, Peter Kasting wrote: > Nit: Clearer: > > Called on the FILE thread to create a new file. Calls |callback| on the UI > thread when finished. Done. https://codereview.chromium.org/580043002/diff/390001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/download_overwrite_infobar.cc (right): https://codereview.chromium.org/580043002/diff/390001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/download_overwrite_infobar.cc:33: java_delegate_.Reset(Java_DownloadOverwriteInfoBarDelegate_create(env)); On 2015/02/26 00:57:56, Ted C wrote: > I find it quite odd that the infobar creates the java delegate instead of the > C++ delegate doing it. > > Looking at AppBannerInfoBarDelegate, it creates and owns the java delegate > itself, which I think is closer to the model we want to follow. Done.
https://codereview.chromium.org/580043002/diff/410001/chrome/browser/android/... File chrome/browser/android/download/download_overwrite_infobar_delegate.h (right): https://codereview.chromium.org/580043002/diff/410001/chrome/browser/android/... chrome/browser/android/download/download_overwrite_infobar_delegate.h:14: using base::android::ScopedJavaGlobalRef; This shouldn't be here (nor the #include it references). https://codereview.chromium.org/580043002/diff/410001/chrome/browser/android/... chrome/browser/android/download/download_overwrite_infobar_delegate.h:41: // Get the file name to be downloaded. Nit: "Gets", not "Get" (several places) https://codereview.chromium.org/580043002/diff/410001/chrome/browser/android/... File chrome/browser/android/download/download_overwrite_infobar_delegate_for_android_download_manager.cc (right): https://codereview.chromium.org/580043002/diff/410001/chrome/browser/android/... chrome/browser/android/download/download_overwrite_infobar_delegate_for_android_download_manager.cc:67: chrome_download_delegate_ = chrome_download_delegate; Nit: Can't this be initialized in the initializer list? https://codereview.chromium.org/580043002/diff/410001/chrome/browser/android/... File chrome/browser/android/download/download_overwrite_infobar_delegate_for_android_download_manager.h (right): https://codereview.chromium.org/580043002/diff/410001/chrome/browser/android/... chrome/browser/android/download/download_overwrite_infobar_delegate_for_android_download_manager.h:49: void InfoBarDismissed() override; Why do you override InfoBarDismissed() when your no-op implementation doesn't differ from the base class'? https://codereview.chromium.org/580043002/diff/410001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/download_overwrite_infobar.cc (right): https://codereview.chromium.org/580043002/diff/410001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/download_overwrite_infobar.cc:59: DCHECK(false); You can shorten both this code and the code in DownloadOverwriteInfoBarDelegateForAndroidDownloadManager by collapsing the OverwriteExistingFile() and CreateNewFile() functions into a single function that takes a bool, e.g. OnUserAction(bool overwrite_existing_file). Then the implementation of that function (for Android at least) is just: ChromeDownloadDelegate::EnqueueDownloadManagerRequest( base::android::AttachCurrentThread(), chrome_download_delegate_, overwrite_existing_file, download_info_); And the code for this block can be: DCHECK((action == InfoBarAndroid::ACTION_OVERWRITE) || (action == InfoBarAndroid::ACTION_CREATE_NEW_FILE)); GetDelegate()->OnUserAction(action == InfoBarAndroid::ACTION_OVERWRITE);
Final few comments. Wish I had a better suggestion for the naming...but at least I tried? https://codereview.chromium.org/580043002/diff/410001/chrome/browser/android/... File chrome/browser/android/download/chrome_download_delegate.cc (right): https://codereview.chromium.org/580043002/diff/410001/chrome/browser/android/... chrome/browser/android/download/chrome_download_delegate.cc:54: JNIEnv* env, Unless you specifically need the calling env, we typically just use AttachCurrentThread to get it. https://codereview.chromium.org/580043002/diff/410001/chrome/browser/android/... chrome/browser/android/download/chrome_download_delegate.cc:89: JavaObjectWeakGlobalRef(env, delegate), scoped_download_info); why weak global ref? Do you really want the delegate to go away if it has no other refs? https://codereview.chromium.org/580043002/diff/410001/chrome/browser/android/... File chrome/browser/android/download/chrome_download_delegate.h (right): https://codereview.chromium.org/580043002/diff/410001/chrome/browser/android/... chrome/browser/android/download/chrome_download_delegate.h:16: JavaObjectWeakGlobalRef chrome_download_delegate, For passing between functions, I prefer to see either jobject or ScopedJavaLocalRef. Globals should only be known within a class that needs to keep ownership. This applies to the rest of this change. https://codereview.chromium.org/580043002/diff/410001/chrome/browser/android/... File chrome/browser/android/download/download_overwrite_infobar_delegate_for_android_download_manager.h (right): https://codereview.chromium.org/580043002/diff/410001/chrome/browser/android/... chrome/browser/android/download/download_overwrite_infobar_delegate_for_android_download_manager.h:23: class DownloadOverwriteInfoBarDelegateForAndroidDownloadManager Although I do like this for completeness, the length is really, really unfortunate. Would the following convey the same meaning: AndroidDownloadManagerOverwriteInfoBarDelegate ChromeDownloadManagerOverwriteInfoBarDelegate We do lose the easy ability to grep the prefix for both, but it is slightly shorter.
> https://codereview.chromium.org/580043002/diff/410001/chrome/browser/android/... > File > chrome/browser/android/download/download_overwrite_infobar_delegate_for_android_download_manager.h > (right): > > https://codereview.chromium.org/580043002/diff/410001/chrome/browser/android/... > chrome/browser/android/download/download_overwrite_infobar_delegate_for_android_download_manager.h:23: > class DownloadOverwriteInfoBarDelegateForAndroidDownloadManager > Although I do like this for completeness, the length is really, really > unfortunate. > > Would the following convey the same meaning: > AndroidDownloadManagerOverwriteInfoBarDelegate > ChromeDownloadManagerOverwriteInfoBarDelegate > > We do lose the easy ability to grep the prefix for both, but it is slightly > shorter. Or even remove "Overwrite" from that. {Anroid,Chrome,}DownloadManagerInfoBarDelegate. I don't know why not having a common prefix is a problem as long as the two have a common substring.
On 2015/03/03 01:43:10, Peter Kasting wrote: > > > https://codereview.chromium.org/580043002/diff/410001/chrome/browser/android/... > > File > > > chrome/browser/android/download/download_overwrite_infobar_delegate_for_android_download_manager.h > > (right): > > > > > https://codereview.chromium.org/580043002/diff/410001/chrome/browser/android/... > > > chrome/browser/android/download/download_overwrite_infobar_delegate_for_android_download_manager.h:23: > > class DownloadOverwriteInfoBarDelegateForAndroidDownloadManager > > Although I do like this for completeness, the length is really, really > > unfortunate. > > > > Would the following convey the same meaning: > > AndroidDownloadManagerOverwriteInfoBarDelegate > > ChromeDownloadManagerOverwriteInfoBarDelegate > > > > We do lose the easy ability to grep the prefix for both, but it is slightly > > shorter. > > Or even remove "Overwrite" from that. > {Anroid,Chrome,}DownloadManagerInfoBarDelegate. > > I don't know why not having a common prefix is a problem as long as the two have > a common substring. Personal preference, I like to see them sitting happily next to each other in my IDE. But something I'm willing to sacrifice at any potential benefit.
I've changed the names to
{Android,Chrome}DownloadManagerOverwriteInfoBarDelegate.
I also considered removing 'overwrite' in it, but I think it's not as clear
because there can be other infobars for download managers such as dangerous
download infobar.
https://codereview.chromium.org/580043002/diff/410001/chrome/browser/android/...
File chrome/browser/android/download/chrome_download_delegate.cc (right):
https://codereview.chromium.org/580043002/diff/410001/chrome/browser/android/...
chrome/browser/android/download/chrome_download_delegate.cc:54: JNIEnv* env,
On 2015/03/03 01:14:44, Ted C wrote:
> Unless you specifically need the calling env, we typically just use
> AttachCurrentThread to get it.
Done.
https://codereview.chromium.org/580043002/diff/410001/chrome/browser/android/...
chrome/browser/android/download/chrome_download_delegate.cc:89:
JavaObjectWeakGlobalRef(env, delegate), scoped_download_info);
On 2015/03/03 01:14:44, Ted C wrote:
> why weak global ref? Do you really want the delegate to go away if it has no
> other refs?
Changed to ScopedJavaGlobalRef
https://codereview.chromium.org/580043002/diff/410001/chrome/browser/android/...
File chrome/browser/android/download/chrome_download_delegate.h (right):
https://codereview.chromium.org/580043002/diff/410001/chrome/browser/android/...
chrome/browser/android/download/chrome_download_delegate.h:16:
JavaObjectWeakGlobalRef chrome_download_delegate,
On 2015/03/03 01:14:44, Ted C wrote:
> For passing between functions, I prefer to see either jobject or
> ScopedJavaLocalRef. Globals should only be known within a class that needs to
> keep ownership.
>
> This applies to the rest of this change.
Done.
https://codereview.chromium.org/580043002/diff/410001/chrome/browser/android/...
File chrome/browser/android/download/download_overwrite_infobar_delegate.h
(right):
https://codereview.chromium.org/580043002/diff/410001/chrome/browser/android/...
chrome/browser/android/download/download_overwrite_infobar_delegate.h:14: using
base::android::ScopedJavaGlobalRef;
On 2015/03/02 21:05:15, Peter Kasting wrote:
> This shouldn't be here (nor the #include it references).
Done.
https://codereview.chromium.org/580043002/diff/410001/chrome/browser/android/...
chrome/browser/android/download/download_overwrite_infobar_delegate.h:41: // Get
the file name to be downloaded.
On 2015/03/02 21:05:15, Peter Kasting wrote:
> Nit: "Gets", not "Get" (several places)
Done.
https://codereview.chromium.org/580043002/diff/410001/chrome/browser/android/...
File
chrome/browser/android/download/download_overwrite_infobar_delegate_for_android_download_manager.cc
(right):
https://codereview.chromium.org/580043002/diff/410001/chrome/browser/android/...
chrome/browser/android/download/download_overwrite_infobar_delegate_for_android_download_manager.cc:67:
chrome_download_delegate_ = chrome_download_delegate;
On 2015/03/02 21:05:15, Peter Kasting wrote:
> Nit: Can't this be initialized in the initializer list?
Not applicable as I've changed it to scopedjavaglobalref
https://codereview.chromium.org/580043002/diff/410001/chrome/browser/android/...
File
chrome/browser/android/download/download_overwrite_infobar_delegate_for_android_download_manager.h
(right):
https://codereview.chromium.org/580043002/diff/410001/chrome/browser/android/...
chrome/browser/android/download/download_overwrite_infobar_delegate_for_android_download_manager.h:23:
class DownloadOverwriteInfoBarDelegateForAndroidDownloadManager
On 2015/03/03 01:14:44, Ted C wrote:
> Although I do like this for completeness, the length is really, really
> unfortunate.
>
> Would the following convey the same meaning:
> AndroidDownloadManagerOverwriteInfoBarDelegate
> ChromeDownloadManagerOverwriteInfoBarDelegate
>
> We do lose the easy ability to grep the prefix for both, but it is slightly
> shorter.
Changed as suggested.
https://codereview.chromium.org/580043002/diff/410001/chrome/browser/android/...
chrome/browser/android/download/download_overwrite_infobar_delegate_for_android_download_manager.h:49:
void InfoBarDismissed() override;
On 2015/03/02 21:05:15, Peter Kasting wrote:
> Why do you override InfoBarDismissed() when your no-op implementation doesn't
> differ from the base class'?
Done.
https://codereview.chromium.org/580043002/diff/410001/chrome/browser/ui/andro...
File chrome/browser/ui/android/infobars/download_overwrite_infobar.cc (right):
https://codereview.chromium.org/580043002/diff/410001/chrome/browser/ui/andro...
chrome/browser/ui/android/infobars/download_overwrite_infobar.cc:59:
DCHECK(false);
On 2015/03/02 21:05:15, Peter Kasting wrote:
> You can shorten both this code and the code in
> DownloadOverwriteInfoBarDelegateForAndroidDownloadManager by collapsing the
> OverwriteExistingFile() and CreateNewFile() functions into a single function
> that takes a bool, e.g. OnUserAction(bool overwrite_existing_file). Then the
> implementation of that function (for Android at least) is just:
>
> ChromeDownloadDelegate::EnqueueDownloadManagerRequest(
> base::android::AttachCurrentThread(), chrome_download_delegate_,
> overwrite_existing_file, download_info_);
>
> And the code for this block can be:
>
> DCHECK((action == InfoBarAndroid::ACTION_OVERWRITE) ||
> (action == InfoBarAndroid::ACTION_CREATE_NEW_FILE));
> GetDelegate()->OnUserAction(action == InfoBarAndroid::ACTION_OVERWRITE);
Hmm... I understand that it will shorten code length in the
AndroidDownloadManager code flow, but I feel that it is not as clear as the
current implementation.
For instance, it is not very clear that the user action was 'create new file'
when overwrite_existing_file was false, so I'll need to add additional javadoc
to describe it. So I prefer how it's currently implemented.
lgtm Thanks for pushing through with this! https://codereview.chromium.org/580043002/diff/430001/chrome/browser/android/... File chrome/browser/android/download/android_download_manager_overwrite_infobar_delegate.cc (right): https://codereview.chromium.org/580043002/diff/430001/chrome/browser/android/... chrome/browser/android/download/android_download_manager_overwrite_infobar_delegate.cc:51: AndroidDownloadManagerOverwriteInfoBarDelegate:: This should go right under Create to match the header. https://codereview.chromium.org/580043002/diff/430001/chrome/browser/android/... File chrome/browser/android/download/chrome_download_manager_overwrite_infobar_delegate.cc (right): https://codereview.chromium.org/580043002/diff/430001/chrome/browser/android/... chrome/browser/android/download/chrome_download_manager_overwrite_infobar_delegate.cc:47: ChromeDownloadManagerOverwriteInfoBarDelegate:: same ordering comment as the other file.
https://codereview.chromium.org/580043002/diff/430001/chrome/browser/android/... File chrome/browser/android/download/android_download_manager_overwrite_infobar_delegate.cc (right): https://codereview.chromium.org/580043002/diff/430001/chrome/browser/android/... chrome/browser/android/download/android_download_manager_overwrite_infobar_delegate.cc:51: AndroidDownloadManagerOverwriteInfoBarDelegate:: On 2015/03/04 16:24:24, Ted C wrote: > This should go right under Create to match the header. Done. https://codereview.chromium.org/580043002/diff/430001/chrome/browser/android/... File chrome/browser/android/download/chrome_download_manager_overwrite_infobar_delegate.cc (right): https://codereview.chromium.org/580043002/diff/430001/chrome/browser/android/... chrome/browser/android/download/chrome_download_manager_overwrite_infobar_delegate.cc:47: ChromeDownloadManagerOverwriteInfoBarDelegate:: On 2015/03/04 16:24:24, Ted C wrote: > same ordering comment as the other file. Done.
The CQ bit was checked by changwan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org, pkasting@chromium.org, qinmin@chromium.org, tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/580043002/#ps450001 (title: "changed function order according to headers")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/580043002/450001
Message was sent while issue was closed.
Committed patchset #23 (id:450001)
Message was sent while issue was closed.
Patchset 23 (id:??) landed as https://crrev.com/fd2eb7523de2f1997d10ca2782745d487f8dff06 Cr-Commit-Position: refs/heads/master@{#319177} |
