Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2400213003:
[Offline Pages] Fix several infobar bugs. (Closed)
|
Created:
4 years, 2 months ago by dewittj Modified:
4 years, 2 months ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, dimich+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Offline Pages] Fix several infobar bugs.
* Overwrite button now deletes all page downloads with the same URL before saving the new page
* Folder name is now called "Downloads"
* Folder link now opens "Downloads home"
BUG=653936
Committed: https://crrev.com/6a97d07086a724662167d73893ab9d2ab00605d1
Cr-Commit-Position: refs/heads/master@{#424569}
Patch Set 1 #Patch Set 2 : git cl web #
Total comments: 19
Patch Set 3 : fgorski & dimich comments. #Patch Set 4 : Comments. #
Total comments: 8
Patch Set 5 : Address dfalcantara's & qinmin's feedback. #
Messages
Total messages: 42 (27 generated)
The CQ bit was checked by dewittj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dewittj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Offline Pages] Implement overwrite button for offline infobar. Existing button does not actually overwrite anything. BUG=653936 ========== to ========== [Offline Pages] Fix several infobar bugs. * Overwrite button now deletes all page downloads with the same URL before saving the new page * Folder name is now called "Downloads" * Folder link now opens "Downloads home" BUG=653936 ==========
dewittj@chromium.org changed reviewers: + fgorski@chromium.org
Because of merge, I had to use an existing string for the downloads name. This is the string from the menu, which only exists in Java. So I plumbed it all the way through to the infobar delegate. LMK if you think there's a better way. Also the "integration" with DownloadOverwriteInfoBar.java is hacky; not sure if I should go for a more fully-engineered solution here.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mostly looks good. https://codereview.chromium.org/2400213003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java (right): https://codereview.chromium.org/2400213003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java:65: if (!"".equals(mDirFullPath)) intent = getIntentForDirectoryLaunch(mDirFullPath); why that way? Is passing null ok? How about: if (mDirFullPath != null && !mDirFullPath.isEmpty()) ? https://codereview.chromium.org/2400213003/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc (right): https://codereview.chromium.org/2400213003/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:11: #include "base/debug/stack_trace.h" Do we need this or was it for debugging? https://codereview.chromium.org/2400213003/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:70: OfflinePageModel* GetOfflinePageModelFromJavaTab( General comment about the code in this file. I think we should move it down to download_ui_adapter (if that is possible). I didn't think about that with the previous patch and I don't expect changes in this one, beyond adding a TODO. https://codereview.chromium.org/2400213003/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:137: notification_bridge.NotifyDownloadProgress(item); Good catch. https://codereview.chromium.org/2400213003/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:150: void OnGetPagesForDeleteDone(const GURL& query_url, nit: How about naming it something related to what is happening in the method and not when? https://codereview.chromium.org/2400213003/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:166: offline_ids, base::Bind(&OnDeletePagesForInfoBar, query_url, j_tab_ref)); nit: In the spirit of the above nit, why not simply &SavePageIfNavigatedToURL as continuation? https://codereview.chromium.org/2400213003/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:179: if (!offline_page_model) we should probably discuss with PMs the fact that if any step here fails, we don't give any message to the user. https://codereview.chromium.org/2400213003/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:181: please add a dcheck that the action is OVERWRITE at this point. https://codereview.chromium.org/2400213003/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/downloads/offline_page_infobar_delegate.h (right): https://codereview.chromium.org/2400213003/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_infobar_delegate.h:21: enum class Action { CONFIRM, OVERWRITE }; s/CONFIRM/CREATE_NEW/ Logically both are confirmation that you will save the page. Also please add the documentation.
dimich@chromium.org changed reviewers: + dimich@chromium.org
https://codereview.chromium.org/2400213003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java (right): https://codereview.chromium.org/2400213003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java:65: if (!"".equals(mDirFullPath)) intent = getIntentForDirectoryLaunch(mDirFullPath); On 2016/10/10 18:08:20, fgorski wrote: > why that way? Is passing null ok? > > How about: if (mDirFullPath != null && !mDirFullPath.isEmpty()) ? +1 :)
https://codereview.chromium.org/2400213003/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.h (right): https://codereview.chromium.org/2400213003/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.h:65: const base::android::JavaParamRef<jstring>& j_downloads_string); on Java side you had a better name for it, download_label. It is already clear from the type that it is a string...
The CQ bit was checked by dewittj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2400213003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java (right): https://codereview.chromium.org/2400213003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java:65: if (!"".equals(mDirFullPath)) intent = getIntentForDirectoryLaunch(mDirFullPath); On 2016/10/10 18:08:20, fgorski wrote: > why that way? Is passing null ok? > > How about: if (mDirFullPath != null && !mDirFullPath.isEmpty()) ? Added a private opensDownloadMnaager() https://codereview.chromium.org/2400213003/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc (right): https://codereview.chromium.org/2400213003/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:11: #include "base/debug/stack_trace.h" On 2016/10/10 18:08:20, fgorski wrote: > Do we need this or was it for debugging? Removed. https://codereview.chromium.org/2400213003/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:70: OfflinePageModel* GetOfflinePageModelFromJavaTab( On 2016/10/10 18:08:20, fgorski wrote: > General comment about the code in this file. I think we should move it down to > download_ui_adapter (if that is possible). I didn't think about that with the > previous patch and I don't expect changes in this one, beyond adding a TODO. Acknowledged. https://codereview.chromium.org/2400213003/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:137: notification_bridge.NotifyDownloadProgress(item); On 2016/10/10 18:08:21, fgorski wrote: > Good catch. Acknowledged. https://codereview.chromium.org/2400213003/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:150: void OnGetPagesForDeleteDone(const GURL& query_url, On 2016/10/10 18:08:20, fgorski wrote: > nit: How about naming it something related to what is happening in the method > and not when? how about DeletePagesForOverwrite? https://codereview.chromium.org/2400213003/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:166: offline_ids, base::Bind(&OnDeletePagesForInfoBar, query_url, j_tab_ref)); On 2016/10/10 18:08:21, fgorski wrote: > nit: In the spirit of the above nit, why not simply &SavePageIfNavigatedToURL as > continuation? Wanted that, but there is an extra parameter and I didn't know how to ignore that in a bound function. https://codereview.chromium.org/2400213003/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:179: if (!offline_page_model) On 2016/10/10 18:08:21, fgorski wrote: > we should probably discuss with PMs the fact that if any step here fails, we > don't give any message to the user. I don't really expect this to be possible, since we are already going for the original profile. Can you think of a case where this would be encountered? If so I totally agree. https://codereview.chromium.org/2400213003/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:181: On 2016/10/10 18:08:20, fgorski wrote: > please add a dcheck that the action is OVERWRITE at this point. Done (used switch for better compile-time checking too.)
The CQ bit was checked by dewittj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2400213003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java (right): https://codereview.chromium.org/2400213003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java:73: if (!opensDownloadManager()) intent = getIntentForDirectoryLaunch(mDirFullPath); You may need {}. I think someone mentioned to me in review in the past that one-oliners on java side are reserved for return statements only. Leaving this comment for reviewer to check.
dewittj@chromium.org changed reviewers: + dfalcantara@chromium.org
Dan, please take a look - whole patch is relevant since we're piggy-backing onto the download overwrite infobar. Thanks!
lgtm but you should swing this by Min; he owns that infobar. https://codereview.chromium.org/2400213003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java (right): https://codereview.chromium.org/2400213003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java:46: * will be opened. indent this under "The" https://codereview.chromium.org/2400213003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java:73: if (!opensDownloadManager()) intent = getIntentForDirectoryLaunch(mDirFullPath); On 2016/10/10 21:30:46, fgorski wrote: > You may need {}. I think someone mentioned to me in review in the past that > one-oliners on java side are reserved for return statements only. Leaving this > comment for reviewer to check. This should be fine; we've done it in the past to set variables that are null, for example. https://codereview.chromium.org/2400213003/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc (right): https://codereview.chromium.org/2400213003/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:386: std::string downloads = ConvertJavaStringToUTF8(env, j_downloads_label); downloads_label? "downloads" is ambiguous.
dewittj@chromium.org changed reviewers: + qinmin@chromium.org
+Min as infobar owner. PTAL, thanks.
lgtm % nit https://codereview.chromium.org/2400213003/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc (right): https://codereview.chromium.org/2400213003/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:210: const std::string& downloads, nit: s/downloads/downloads_label/ ?
The CQ bit was checked by dewittj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2400213003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java (right): https://codereview.chromium.org/2400213003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/DownloadOverwriteInfoBar.java:46: * will be opened. On 2016/10/11 20:01:18, dfalcantara wrote: > indent this under "The" Done. https://codereview.chromium.org/2400213003/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc (right): https://codereview.chromium.org/2400213003/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:210: const std::string& downloads, On 2016/10/11 21:05:06, qinmin wrote: > nit: s/downloads/downloads_label/ ? Done. https://codereview.chromium.org/2400213003/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:386: std::string downloads = ConvertJavaStringToUTF8(env, j_downloads_label); On 2016/10/11 20:01:18, dfalcantara wrote: > downloads_label? "downloads" is ambiguous. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dewittj@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org, qinmin@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2400213003/#ps80001 (title: "Address dfalcantara's & qinmin's feedback.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Fix several infobar bugs. * Overwrite button now deletes all page downloads with the same URL before saving the new page * Folder name is now called "Downloads" * Folder link now opens "Downloads home" BUG=653936 ========== to ========== [Offline Pages] Fix several infobar bugs. * Overwrite button now deletes all page downloads with the same URL before saving the new page * Folder name is now called "Downloads" * Folder link now opens "Downloads home" BUG=653936 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Fix several infobar bugs. * Overwrite button now deletes all page downloads with the same URL before saving the new page * Folder name is now called "Downloads" * Folder link now opens "Downloads home" BUG=653936 ========== to ========== [Offline Pages] Fix several infobar bugs. * Overwrite button now deletes all page downloads with the same URL before saving the new page * Folder name is now called "Downloads" * Folder link now opens "Downloads home" BUG=653936 Committed: https://crrev.com/6a97d07086a724662167d73893ab9d2ab00605d1 Cr-Commit-Position: refs/heads/master@{#424569} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/6a97d07086a724662167d73893ab9d2ab00605d1 Cr-Commit-Position: refs/heads/master@{#424569} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
