Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1692693003:
add a flag to enable/disable download interception on android (Closed)
Descriptionadd a flag to enable/disable download interception on android
Currently some of the download are passed to Android DownloadManager.
We might change that behavior in the future.
This CL adds a flag to allow us to toggle that behavior.
Chrome flag and finch flag "EnableDownloadInterception" are added to allow locally/remotely set/unset the flag
BUG=545640
Committed: https://crrev.com/d888e48bbc95627822d5fc10913e245a33e2db77
Cr-Commit-Position: refs/heads/master@{#377724}
Patch Set 1 #Patch Set 2 : allow finch trial to turn on off the flag #
Total comments: 6
Patch Set 3 : allow server trial to be overriden #
Total comments: 8
Patch Set 4 : nits #
Total comments: 4
Patch Set 5 : use base::Feature instead of switches #Patch Set 6 : remove unused variable #
Messages
Total messages: 33 (12 generated)
qinmin@chromium.org changed reviewers: + asanka@chromium.org, sky@chromium.org, svaldez@chromium.org
@PTAL, really tiny change.
What does the flag default to? I think we want to default to intercepting the download to match current behavior, and then use the flag to disable interception?
On 2016/02/11 21:32:28, svaldez wrote: > What does the flag default to? I think we want to default to intercepting the > download to match current behavior, and then use the flag to disable > interception? Also, do you intend to manually set/reset the flag? Or would you like to control this behavior via a server-side experiment?
On 2016/02/11 21:47:56, asanka wrote: > On 2016/02/11 21:32:28, svaldez wrote: > > What does the flag default to? I think we want to default to intercepting the > > download to match current behavior, and then use the flag to disable > > interception? > > Also, do you intend to manually set/reset the flag? Or would you like to control > this behavior via a server-side experiment? Good questions, I have another CL to set/unset the flag through finch and chrome://flags. I have merged that CL to this change now, updated the CL description to match the new change
Description was changed from ========== add a flag to enable/disable download interception on android Currently some of the download are passed to Android DownloadManager. We might change that behavior in the future. This CL adds a flag to allow us to toggle that behavior. BUG=545640 ========== to ========== add a flag to enable/disable download interception on android Currently some of the download are passed to Android DownloadManager. We might change that behavior in the future. This CL adds a flag to allow us to toggle that behavior. Chrome flag and finch flag "EnableDownloadInterception" are added to allow locally/remotely set/unset the flag BUG=545640 ==========
Have a look at https://code.google.com/p/chromium/codesearch#chromium/src/base/feature_list.h If you are controlling a flag via a server-side experiment, the should be a way to override the flag in either direction. https://codereview.chromium.org/1692693003/diff/20001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1692693003/diff/20001/chrome/app/generated_re... chrome/app/generated_resources.grd:5694: + <message name="IDS_FLAGS_DOWNLOAD_INTERCEPTION_NAME" desc="Title for the flag to enable the download interception feature."> Conditionalize so that this is only defined for Android. https://codereview.chromium.org/1692693003/diff/20001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/1692693003/diff/20001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:279: base::CompareCase::INSENSITIVE_ASCII); Let's move these out to a separate file. https://codereview.chromium.org/1692693003/diff/20001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:434: switches::kEnableDownloadInterception)) { Why is disabling the default? Also, if the feature was enabled by the field trial, the user can't override it.
https://codereview.chromium.org/1692693003/diff/20001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1692693003/diff/20001/chrome/app/generated_re... chrome/app/generated_resources.grd:5694: + <message name="IDS_FLAGS_DOWNLOAD_INTERCEPTION_NAME" desc="Title for the flag to enable the download interception feature."> On 2016/02/11 22:49:20, asanka wrote: > Conditionalize so that this is only defined for Android. Done. https://codereview.chromium.org/1692693003/diff/20001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/1692693003/diff/20001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:279: base::CompareCase::INSENSITIVE_ASCII); On 2016/02/11 22:49:20, asanka wrote: > Let's move these out to a separate file. Done. Moved to InterceptDownloadResourceThrottle class. https://codereview.chromium.org/1692693003/diff/20001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:434: switches::kEnableDownloadInterception)) { On 2016/02/11 22:49:20, asanka wrote: > Why is disabling the default? > > Also, if the feature was enabled by the field trial, the user can't override it. Done. Now the remote server will disable download interception, and locally we can override that. The default behavior is to always intercept.
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/1692693003/diff/60001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1692693003/diff/60001/chrome/app/generated_re... chrome/app/generated_resources.grd:5695: + <message name="IDS_FLAGS_DOWNLOAD_INTERCEPTION_NAME" desc="Title for the flag to enable the download interception feature."> These flag messages should be slightly more user-friendly instead of referencing "Download Interception" which users have no idea what that means. Even something like "Use Native Android Download Manager" instead might make more sense as a flag name. https://codereview.chromium.org/1692693003/diff/60001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/1692693003/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:421: if (chrome::InterceptDownloadResourceThrottle:: Consider getting rid of the check here, and instead having InterceptDownloadResourceThrottle check the IsDownloadInterceptionEnabled and do nothing if so. This lets us keep our metrics from InterceptDownloadResourceThrottle. https://codereview.chromium.org/1692693003/diff/60001/chrome/common/chrome_sw... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/1692693003/diff/60001/chrome/common/chrome_sw... chrome/common/chrome_switches.cc:1128: // Enables android DownloadManager to intercept downloads. nit: Enables Android https://codereview.chromium.org/1692693003/diff/60001/chrome/common/chrome_sw... chrome/common/chrome_switches.cc:1131: // Disables android DownloadManager to intercept downloads. same
https://codereview.chromium.org/1692693003/diff/60001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1692693003/diff/60001/chrome/app/generated_re... chrome/app/generated_resources.grd:5695: + <message name="IDS_FLAGS_DOWNLOAD_INTERCEPTION_NAME" desc="Title for the flag to enable the download interception feature."> On 2016/02/12 15:21:19, svaldez wrote: > These flag messages should be slightly more user-friendly instead of referencing > "Download Interception" which users have no idea what that means. Even something > like "Use Native Android Download Manager" instead might make more sense as a > flag name. Done. Changed this to "Use native download manager when applicable." https://codereview.chromium.org/1692693003/diff/60001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/1692693003/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:421: if (chrome::InterceptDownloadResourceThrottle:: On 2016/02/12 15:21:19, svaldez wrote: > Consider getting rid of the check here, and instead having > InterceptDownloadResourceThrottle check the IsDownloadInterceptionEnabled and do > nothing if so. > > This lets us keep our metrics from InterceptDownloadResourceThrottle. Done. However, there is a very tiny performance hit as we always need to go through the throttle for each download. https://codereview.chromium.org/1692693003/diff/60001/chrome/common/chrome_sw... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/1692693003/diff/60001/chrome/common/chrome_sw... chrome/common/chrome_switches.cc:1128: // Enables android DownloadManager to intercept downloads. On 2016/02/12 15:21:19, svaldez wrote: > nit: Enables Android Done. https://codereview.chromium.org/1692693003/diff/60001/chrome/common/chrome_sw... chrome/common/chrome_switches.cc:1131: // Disables android DownloadManager to intercept downloads. On 2016/02/12 15:21:20, svaldez wrote: > same Done.
ping, asanka@, would you mind take another look?
LGTM % nits. But I'd strongly prefer using base::Feature for controlling this rather than adding pairs of switches. https://codereview.chromium.org/1692693003/diff/80001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1692693003/diff/80001/chrome/app/generated_re... chrome/app/generated_resources.grd:5699: + Enable download interception by the native download manager. I don't think users would understand what 'download interception' means. The name above is clearer about what this flag does. https://codereview.chromium.org/1692693003/diff/80001/chrome/browser/android/... File chrome/browser/android/intercept_download_resource_throttle.cc (right): https://codereview.chromium.org/1692693003/diff/80001/chrome/browser/android/... chrome/browser/android/intercept_download_resource_throttle.cc:65: base::FieldTrialList::FindFullName(kDisableDownloadInterception), See base/feature_list.h . Rather than add the enable/disable flags, you should consider using the Features API. This way you'll be able to control the behavior both on the command line and via a field trial.
What is the reason not to use base::Feature for controlling this? On Mon, Feb 22, 2016 at 6:55 PM, <asanka@chromium.org> wrote: > LGTM % nits. But I'd strongly prefer using base::Feature for controlling > this > rather than adding pairs of switches. > > > https://codereview.chromium.org/1692693003/diff/80001/chrome/app/generated_re... > File chrome/app/generated_resources.grd (right): > > https://codereview.chromium.org/1692693003/diff/80001/chrome/app/generated_re... > chrome/app/generated_resources.grd:5699: + Enable download > interception by the native download manager. > I don't think users would understand what 'download interception' means. > The name above is clearer about what this flag does. > > https://codereview.chromium.org/1692693003/diff/80001/chrome/browser/android/... > File chrome/browser/android/intercept_download_resource_throttle.cc > (right): > > https://codereview.chromium.org/1692693003/diff/80001/chrome/browser/android/... > chrome/browser/android/intercept_download_resource_throttle.cc:65: > base::FieldTrialList::FindFullName(kDisableDownloadInterception), > See base/feature_list.h . Rather than add the enable/disable flags, you > should consider using the Features API. This way you'll be able to > control the behavior both on the command line and via a field trial. > > https://codereview.chromium.org/1692693003/ -- 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.
Switched to use base::Feature, PTAL again https://codereview.chromium.org/1692693003/diff/80001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1692693003/diff/80001/chrome/app/generated_re... chrome/app/generated_resources.grd:5699: + Enable download interception by the native download manager. On 2016/02/23 02:55:30, asanka wrote: > I don't think users would understand what 'download interception' means. The > name above is clearer about what this flag does. Done. Changed the description to "Allow downloads to be handled by the system download manager when applicable." and changed the flags name to "IDS_FLAGS_ENABLE_SYSTEM_DOWNLOAD_MANAGER_XXX" https://codereview.chromium.org/1692693003/diff/80001/chrome/browser/android/... File chrome/browser/android/intercept_download_resource_throttle.cc (right): https://codereview.chromium.org/1692693003/diff/80001/chrome/browser/android/... chrome/browser/android/intercept_download_resource_throttle.cc:65: base::FieldTrialList::FindFullName(kDisableDownloadInterception), On 2016/02/23 02:55:30, asanka wrote: > See base/feature_list.h . Rather than add the enable/disable flags, you should > consider using the Features API. This way you'll be able to control the behavior > both on the command line and via a field trial. Done.
ping, sky@, would you please take another look as I switched to from switches to base::Feature?
LGTM
The CQ bit was checked by qinmin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org Link to the patchset: https://codereview.chromium.org/1692693003/#ps100001 (title: "use base::Feature instead of switches")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1692693003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1692693003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by qinmin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1692693003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1692693003/120001
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 qinmin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1692693003/#ps120001 (title: "remove unused variable")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1692693003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1692693003/120001
Message was sent while issue was closed.
Description was changed from ========== add a flag to enable/disable download interception on android Currently some of the download are passed to Android DownloadManager. We might change that behavior in the future. This CL adds a flag to allow us to toggle that behavior. Chrome flag and finch flag "EnableDownloadInterception" are added to allow locally/remotely set/unset the flag BUG=545640 ========== to ========== add a flag to enable/disable download interception on android Currently some of the download are passed to Android DownloadManager. We might change that behavior in the future. This CL adds a flag to allow us to toggle that behavior. Chrome flag and finch flag "EnableDownloadInterception" are added to allow locally/remotely set/unset the flag BUG=545640 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== add a flag to enable/disable download interception on android Currently some of the download are passed to Android DownloadManager. We might change that behavior in the future. This CL adds a flag to allow us to toggle that behavior. Chrome flag and finch flag "EnableDownloadInterception" are added to allow locally/remotely set/unset the flag BUG=545640 ========== to ========== add a flag to enable/disable download interception on android Currently some of the download are passed to Android DownloadManager. We might change that behavior in the future. This CL adds a flag to allow us to toggle that behavior. Chrome flag and finch flag "EnableDownloadInterception" are added to allow locally/remotely set/unset the flag BUG=545640 Committed: https://crrev.com/d888e48bbc95627822d5fc10913e245a33e2db77 Cr-Commit-Position: refs/heads/master@{#377724} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/d888e48bbc95627822d5fc10913e245a33e2db77 Cr-Commit-Position: refs/heads/master@{#377724} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
