Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2588253002:
Enable download page action for error page (Closed)
|
Created:
4 years ago by jianli Modified:
3 years, 11 months ago CC:
chromium-reviews, asanka, cbentzel+watch_chromium.org, dewittj+watch_chromium.org, fgorski+watch_chromium.org, romax+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, agrieve+watch_chromium.org, dimich+watch_chromium.org, dcheng Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnable download page action for error page
If error page shows "DOWNLOAD PAGE LATER" button, we should also
enable download page action.
BUG=675225
Patch
Review-Url: https://codereview.chromium.org/2588253002
Cr-Commit-Position: refs/heads/master@{#443104}
Committed: https://chromium.googlesource.com/chromium/src/+/b0fe12b996f99d984f92983d188b7095b8c80134
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add comment to address feedback #Patch Set 3 : Fix trybots #
Total comments: 9
Patch Set 4 : Address feedback #Patch Set 5 : Fix trybots #
Total comments: 7
Patch Set 6 : Address more comment #Patch Set 7 : set false in non-error page commit #Patch Set 8 : set false in NetErrorTabHelper::DidFinishNavigation #
Total comments: 6
Patch Set 9 : Some small fixes #Patch Set 10 : Rebase #
Total comments: 4
Patch Set 11 : Address download feedback #Messages
Total messages: 66 (38 generated)
The CQ bit was checked by jianli@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...
jianli@chromium.org changed reviewers: + dcheng@chromium.org, dimich@chromium.org, mmenke@chromium.org, qinmin@chromium.org
dimich for offline pages qinmin for download mmenke for error pages dcheng for new IPC message
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2588253002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java (right): https://codereview.chromium.org/2588253002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:192: if (tab.isShowingErrorPage()) { Could you please add a comment here explaining what is the difference between both branches of this 'if'?
dcheng@chromium.org changed reviewers: + nasko@chromium.org
(handing IPC review off to nasko@)
The CQ bit was checked by jianli@chromium.org to run a CQ dry run
https://codereview.chromium.org/2588253002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java (right): https://codereview.chromium.org/2588253002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:192: if (tab.isShowingErrorPage()) { On 2016/12/20 04:20:34, Dmitry Titov wrote: > Could you please add a comment here explaining what is the difference between > both branches of this 'if'? Done.
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/2588253002/diff/40001/chrome/browser/net/net_... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/2588253002/diff/40001/chrome/browser/net/net_... chrome/browser/net/net_error_tab_helper.cc:147: SetIsShowingDownloadButtonInErrorPage) How can the browser process have a better idea of what's currently being displayed in the error page than the code that's actually displaying stuff in the error page? https://codereview.chromium.org/2588253002/diff/40001/components/error_page/r... File components/error_page/renderer/net_error_helper_core.cc (right): https://codereview.chromium.org/2588253002/diff/40001/components/error_page/r... components/error_page/renderer/net_error_helper_core.cc:587: delegate_->SetIsShowingDownloadButton(false); Bug: What we're showing only changes on commit, not when a load starts. Loads may be aborted. Due to the complexities of the state machine, I'd rather not add additional top-level variables related to state of the currently displayed error page. Any additional state should be added to ErrorPageInfo.
Just couple of nits. Otherwise the IPC itself looks good and I'll stamp the CL once all other reviewers are happy. https://codereview.chromium.org/2588253002/diff/40001/chrome/browser/net/net_... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/2588253002/diff/40001/chrome/browser/net/net_... chrome/browser/net/net_error_tab_helper.cc:147: SetIsShowingDownloadButtonInErrorPage) nit: Usually IPC handlers have an On prefix. https://codereview.chromium.org/2588253002/diff/40001/chrome/browser/net/net_... chrome/browser/net/net_error_tab_helper.cc:165: #endif // BUILDFLAG(ANDROID_JAVA_UI nit: Missing closing parenthesis.
The CQ bit was checked by jianli@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: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by jianli@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.
https://codereview.chromium.org/2588253002/diff/40001/chrome/browser/net/net_... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/2588253002/diff/40001/chrome/browser/net/net_... chrome/browser/net/net_error_tab_helper.cc:147: SetIsShowingDownloadButtonInErrorPage) On 2017/01/03 16:20:10, mmenke wrote: > How can the browser process have a better idea of what's currently being > displayed in the error page than the code that's actually displaying stuff in > the error page? I am not sure I fully understand this comment. The browser process has no idea about what is being displayed in the error page. This is the reason we send an IPC from the code in renderer process that does the actual display. https://codereview.chromium.org/2588253002/diff/40001/chrome/browser/net/net_... chrome/browser/net/net_error_tab_helper.cc:147: SetIsShowingDownloadButtonInErrorPage) On 2017/01/04 18:22:20, nasko wrote: > nit: Usually IPC handlers have an On prefix. Done. https://codereview.chromium.org/2588253002/diff/40001/chrome/browser/net/net_... chrome/browser/net/net_error_tab_helper.cc:165: #endif // BUILDFLAG(ANDROID_JAVA_UI On 2017/01/04 18:22:20, nasko wrote: > nit: Missing closing parenthesis. Done. https://codereview.chromium.org/2588253002/diff/40001/components/error_page/r... File components/error_page/renderer/net_error_helper_core.cc (right): https://codereview.chromium.org/2588253002/diff/40001/components/error_page/r... components/error_page/renderer/net_error_helper_core.cc:587: delegate_->SetIsShowingDownloadButton(false); On 2017/01/03 16:20:10, mmenke wrote: > Bug: What we're showing only changes on commit, not when a load starts. Loads > may be aborted. > > Due to the complexities of the state machine, I'd rather not add additional > top-level variables related to state of the currently displayed error page. Any > additional state should be added to ErrorPageInfo. Since we already have one in ErrorPageInfo, download_button_in_page, I think we can remove this one.
https://codereview.chromium.org/2588253002/diff/40001/chrome/browser/net/net_... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/2588253002/diff/40001/chrome/browser/net/net_... chrome/browser/net/net_error_tab_helper.cc:147: SetIsShowingDownloadButtonInErrorPage) On 2017/01/06 23:46:37, jianli wrote: > On 2017/01/03 16:20:10, mmenke wrote: > > How can the browser process have a better idea of what's currently being > > displayed in the error page than the code that's actually displaying stuff in > > the error page? > > I am not sure I fully understand this comment. The browser process has no idea > about what is being displayed in the error page. This is the reason we send an > IPC from the code in renderer process that does the actual display. Sorry, I was getting net_error_tab_helper confused with net_error_helper, and trying to figure out why net_error_helper was getting an IPC telling it that it was displaying an error page.
Sorry for not getting to this earlier today, busy day. https://codereview.chromium.org/2588253002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java (right): https://codereview.chromium.org/2588253002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:225: return bridge.isShowingDownloadButtonInErrorPage(tab.getWebContents()); How accurate is this required to be? It can return true for non-error pages, and returns the value of the last loaded error from load start through load commit until load finished. Tracking page state across processes can get finicky. https://codereview.chromium.org/2588253002/diff/80001/chrome/browser/net/net_... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/2588253002/diff/80001/chrome/browser/net/net_... chrome/browser/net/net_error_tab_helper.cc:234: void NetErrorTabHelper::OnSetIsShowingDownloadButtonInErrorPage(bool show) { "show" seems a bit short. Maybe just showing_button or is_showing_download_button. https://codereview.chromium.org/2588253002/diff/80001/chrome/browser/net/net_... File chrome/browser/net/net_error_tab_helper.h (right): https://codereview.chromium.org/2588253002/diff/80001/chrome/browser/net/net_... chrome/browser/net/net_error_tab_helper.h:127: // True if download button is being shown in the error page. Per comment in Java file, this isn't correct. Maybe at least clear it on commit?
https://codereview.chromium.org/2588253002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java (right): https://codereview.chromium.org/2588253002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:225: return bridge.isShowingDownloadButtonInErrorPage(tab.getWebContents()); On 2017/01/09 20:35:39, mmenke wrote: > How accurate is this required to be? It can return true for non-error pages, > and returns the value of the last loaded error from load start through load > commit until load finished. > > Tracking page state across processes can get finicky. isShowingDownloadButtonInErrorPage will only be checked after isShowingErrorPage returns true, which is only true after the error page navigation is committed. So it means: * If the page load is not committed, regardless error page or not, we will continue the flow to do other checks. * If the error page is committed, isShowingDownloadButtonInErrorPage info should already been set. We will return true/false accordingly. I think it is OK not to have the accurate info since the page saving layer should have its own checking logic. https://codereview.chromium.org/2588253002/diff/80001/chrome/browser/net/net_... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/2588253002/diff/80001/chrome/browser/net/net_... chrome/browser/net/net_error_tab_helper.cc:234: void NetErrorTabHelper::OnSetIsShowingDownloadButtonInErrorPage(bool show) { On 2017/01/09 20:35:39, mmenke wrote: > "show" seems a bit short. Maybe just showing_button or > is_showing_download_button. Done. https://codereview.chromium.org/2588253002/diff/80001/chrome/browser/net/net_... File chrome/browser/net/net_error_tab_helper.h (right): https://codereview.chromium.org/2588253002/diff/80001/chrome/browser/net/net_... chrome/browser/net/net_error_tab_helper.h:127: // True if download button is being shown in the error page. On 2017/01/09 20:35:39, mmenke wrote: > Per comment in Java file, this isn't correct. Maybe at least clear it on > commit? Done.
The CQ bit was checked by jianli@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/2588253002/diff/80001/chrome/browser/net/net_... File chrome/browser/net/net_error_tab_helper.h (right): https://codereview.chromium.org/2588253002/diff/80001/chrome/browser/net/net_... chrome/browser/net/net_error_tab_helper.h:127: // True if download button is being shown in the error page. On 2017/01/09 23:35:07, jianli wrote: > On 2017/01/09 20:35:39, mmenke wrote: > > Per comment in Java file, this isn't correct. Maybe at least clear it on > > commit? > > Done. Unless I'm missing something, you're still not setting this to false when a page commits?
The CQ bit was checked by jianli@chromium.org to run a CQ dry run
On 2017/01/09 23:37:28, mmenke wrote: > https://codereview.chromium.org/2588253002/diff/80001/chrome/browser/net/net_... > File chrome/browser/net/net_error_tab_helper.h (right): > > https://codereview.chromium.org/2588253002/diff/80001/chrome/browser/net/net_... > chrome/browser/net/net_error_tab_helper.h:127: // True if download button is > being shown in the error page. > On 2017/01/09 23:35:07, jianli wrote: > > On 2017/01/09 20:35:39, mmenke wrote: > > > Per comment in Java file, this isn't correct. Maybe at least clear it on > > > commit? > > > > Done. > > Unless I'm missing something, you're still not setting this to false when a page > commits? Got it. Added the setting at line 637 in NetErrorHelperCore::OnFinishLoad.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/01/10 00:20:22, jianli wrote: > On 2017/01/09 23:37:28, mmenke wrote: > > > https://codereview.chromium.org/2588253002/diff/80001/chrome/browser/net/net_... > > File chrome/browser/net/net_error_tab_helper.h (right): > > > > > https://codereview.chromium.org/2588253002/diff/80001/chrome/browser/net/net_... > > chrome/browser/net/net_error_tab_helper.h:127: // True if download button is > > being shown in the error page. > > On 2017/01/09 23:35:07, jianli wrote: > > > On 2017/01/09 20:35:39, mmenke wrote: > > > > Per comment in Java file, this isn't correct. Maybe at least clear it on > > > > commit? > > > > > > Done. > > > > Unless I'm missing something, you're still not setting this to false when a > page > > commits? > > Got it. Added the setting at line 637 in NetErrorHelperCore::OnFinishLoad. LGTM
On 2017/01/10 00:20:22, jianli wrote: > On 2017/01/09 23:37:28, mmenke wrote: > > > https://codereview.chromium.org/2588253002/diff/80001/chrome/browser/net/net_... > > File chrome/browser/net/net_error_tab_helper.h (right): > > > > > https://codereview.chromium.org/2588253002/diff/80001/chrome/browser/net/net_... > > chrome/browser/net/net_error_tab_helper.h:127: // True if download button is > > being shown in the error page. > > On 2017/01/09 23:35:07, jianli wrote: > > > On 2017/01/09 20:35:39, mmenke wrote: > > > > Per comment in Java file, this isn't correct. Maybe at least clear it on > > > > commit? > > > > > > Done. > > > > Unless I'm missing something, you're still not setting this to false when a > page > > commits? > > Got it. Added the setting at line 637 in NetErrorHelperCore::OnFinishLoad. With more thought, I change to set to false NetErrorTabHelper::DidFinishNavigation in order not to incur extra IPC cost.
The CQ bit was checked by jianli@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...
On 2017/01/10 00:23:35, jianli wrote: > On 2017/01/10 00:20:22, jianli wrote: > > On 2017/01/09 23:37:28, mmenke wrote: > > > > > > https://codereview.chromium.org/2588253002/diff/80001/chrome/browser/net/net_... > > > File chrome/browser/net/net_error_tab_helper.h (right): > > > > > > > > > https://codereview.chromium.org/2588253002/diff/80001/chrome/browser/net/net_... > > > chrome/browser/net/net_error_tab_helper.h:127: // True if download button is > > > being shown in the error page. > > > On 2017/01/09 23:35:07, jianli wrote: > > > > On 2017/01/09 20:35:39, mmenke wrote: > > > > > Per comment in Java file, this isn't correct. Maybe at least clear it > on > > > > > commit? > > > > > > > > Done. > > > > > > Unless I'm missing something, you're still not setting this to false when a > > page > > > commits? > > > > Got it. Added the setting at line 637 in NetErrorHelperCore::OnFinishLoad. > > With more thought, I change to set to false > NetErrorTabHelper::DidFinishNavigation in order not to incur extra IPC cost. SGTM. Just make sure to test it works (To make sure it's called before the IPC to set it to true when actually showing the error page), and I don't think there's a need for another round of review. That's the implementation I had in mind, but decided I could live with this one, too.
On 2017/01/10 00:25:37, mmenke wrote: > On 2017/01/10 00:23:35, jianli wrote: > > On 2017/01/10 00:20:22, jianli wrote: > > > On 2017/01/09 23:37:28, mmenke wrote: > > > > > > > > > > https://codereview.chromium.org/2588253002/diff/80001/chrome/browser/net/net_... > > > > File chrome/browser/net/net_error_tab_helper.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2588253002/diff/80001/chrome/browser/net/net_... > > > > chrome/browser/net/net_error_tab_helper.h:127: // True if download button > is > > > > being shown in the error page. > > > > On 2017/01/09 23:35:07, jianli wrote: > > > > > On 2017/01/09 20:35:39, mmenke wrote: > > > > > > Per comment in Java file, this isn't correct. Maybe at least clear it > > on > > > > > > commit? > > > > > > > > > > Done. > > > > > > > > Unless I'm missing something, you're still not setting this to false when > a > > > page > > > > commits? > > > > > > Got it. Added the setting at line 637 in NetErrorHelperCore::OnFinishLoad. > > > > With more thought, I change to set to false > > NetErrorTabHelper::DidFinishNavigation in order not to incur extra IPC cost. > > SGTM. Just make sure to test it works (To make sure it's called before the IPC > to set it to true when actually showing the error page), and I don't think > there's a need for another round of review. That's the implementation I had in > mind, but decided I could live with this one, too. Ah, where you put it makes that a moot point, anyways. LGTM
The CQ bit was checked by jianli@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...
Tested and it worked. Thanks so much for making sure everything worked. On Mon, Jan 9, 2017 at 4:26 PM, <mmenke@chromium.org> wrote: > On 2017/01/10 00:25:37, mmenke wrote: > > On 2017/01/10 00:23:35, jianli wrote: > > > On 2017/01/10 00:20:22, jianli wrote: > > > > On 2017/01/09 23:37:28, mmenke wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2588253002/diff/80001/ > chrome/browser/net/net_error_tab_helper.h > > > > > File chrome/browser/net/net_error_tab_helper.h (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2588253002/diff/80001/ > chrome/browser/net/net_error_tab_helper.h#newcode127 > > > > > chrome/browser/net/net_error_tab_helper.h:127: // True if download > button > > is > > > > > being shown in the error page. > > > > > On 2017/01/09 23:35:07, jianli wrote: > > > > > > On 2017/01/09 20:35:39, mmenke wrote: > > > > > > > Per comment in Java file, this isn't correct. Maybe at least > clear > it > > > on > > > > > > > commit? > > > > > > > > > > > > Done. > > > > > > > > > > Unless I'm missing something, you're still not setting this to > false > when > > a > > > > page > > > > > commits? > > > > > > > > Got it. Added the setting at line 637 in NetErrorHelperCore:: > OnFinishLoad. > > > > > > With more thought, I change to set to false > > > NetErrorTabHelper::DidFinishNavigation in order not to incur extra > IPC cost. > > > > SGTM. Just make sure to test it works (To make sure it's called before > the > IPC > > to set it to true when actually showing the error page), and I don't > think > > there's a need for another round of review. That's the implementation I > had > in > > mind, but decided I could live with this one, too. > > Ah, where you put it makes that a moot point, anyways. LGTM > > https://codereview.chromium.org/2588253002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
IPC LGTM, but a few nits here and there. https://codereview.chromium.org/2588253002/diff/140001/chrome/browser/net/net... File chrome/browser/net/net_error_tab_helper.h (right): https://codereview.chromium.org/2588253002/diff/140001/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.h:59: #endif // BUILDFLAG(ANDROID_JAVA_ nit: s/JAVA_/JAVA_UI)/ https://codereview.chromium.org/2588253002/diff/140001/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.h:129: #endif // BUILDFLAG(ANDROID_JAVA_UI nit: Still unmatched parenthesis here. https://codereview.chromium.org/2588253002/diff/140001/components/error_page/... File components/error_page/renderer/net_error_helper_core.h (right): https://codereview.chromium.org/2588253002/diff/140001/components/error_page/... components/error_page/renderer/net_error_helper_core.h:113: // inform that download button is being shown in the error page. nit: Capitalize "inform".
https://codereview.chromium.org/2588253002/diff/140001/chrome/browser/net/net... File chrome/browser/net/net_error_tab_helper.h (right): https://codereview.chromium.org/2588253002/diff/140001/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.h:59: #endif // BUILDFLAG(ANDROID_JAVA_ On 2017/01/10 17:40:22, nasko wrote: > nit: s/JAVA_/JAVA_UI)/ Done. https://codereview.chromium.org/2588253002/diff/140001/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.h:129: #endif // BUILDFLAG(ANDROID_JAVA_UI On 2017/01/10 17:40:22, nasko wrote: > nit: Still unmatched parenthesis here. Done. https://codereview.chromium.org/2588253002/diff/140001/components/error_page/... File components/error_page/renderer/net_error_helper_core.h (right): https://codereview.chromium.org/2588253002/diff/140001/components/error_page/... components/error_page/renderer/net_error_helper_core.h:113: // inform that download button is being shown in the error page. On 2017/01/10 17:40:22, nasko wrote: > nit: Capitalize "inform". Done.
The CQ bit was checked by jianli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dimich@chromium.org, nasko@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2588253002/#ps160001 (title: "Some small fixes")
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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by jianli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org, dimich@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2588253002/#ps180001 (title: "Rebase")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lgtm % comments https://codereview.chromium.org/2588253002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java (right): https://codereview.chromium.org/2588253002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:218: if (!OfflinePageBridge.canSavePage(tab.getUrl())) { nit: you can move return false to this line. https://codereview.chromium.org/2588253002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:225: return bridge.isShowingDownloadButtonInErrorPage(tab.getWebContents()); do we need to destroy the bridge here?
https://codereview.chromium.org/2588253002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java (right): https://codereview.chromium.org/2588253002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:218: if (!OfflinePageBridge.canSavePage(tab.getUrl())) { On 2017/01/11 23:36:15, qinmin wrote: > nit: you can move return false to this line. Done. https://codereview.chromium.org/2588253002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:225: return bridge.isShowingDownloadButtonInErrorPage(tab.getWebContents()); On 2017/01/11 23:36:15, qinmin wrote: > do we need to destroy the bridge here? destroy is not defined in OfflinePageBridge. Also the explicit destroy seems not to be necessary.
The CQ bit was checked by jianli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org, qinmin@chromium.org, dimich@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2588253002/#ps200001 (title: "Address download feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1484178482978700,
"parent_rev": "998d24774d5062f8d032fedbe8870b23ad16c519", "commit_rev":
"b0fe12b996f99d984f92983d188b7095b8c80134"}
Message was sent while issue was closed.
Description was changed from ========== Enable download page action for error page If error page shows "DOWNLOAD PAGE LATER" button, we should also enable download page action. BUG=675225 Patch ========== to ========== Enable download page action for error page If error page shows "DOWNLOAD PAGE LATER" button, we should also enable download page action. BUG=675225 Patch Review-Url: https://codereview.chromium.org/2588253002 Cr-Commit-Position: refs/heads/master@{#443104} Committed: https://chromium.googlesource.com/chromium/src/+/b0fe12b996f99d984f92983d188b... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/b0fe12b996f99d984f92983d188b... |
