Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1238043005:
Use new API of DataReductionProxyService. Fixes CroNet build break. (Closed)
|
Created:
5 years, 5 months ago by Not at Google. Contact bengr Modified:
5 years, 5 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, bengr Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse new API of DataReductionProxyService. Fixes CroNet build break.
Committed: https://crrev.com/ef3d9b3a8377ea053159d477731d6a44dfd40bcc
Cr-Commit-Position: refs/heads/master@{#339671}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Sync to head #
Messages
Total messages: 11 (3 generated)
kundaji@chromium.org changed reviewers: + mef@chromium.org, pauljensen@chromium.org
https://codereview.chromium.org/1238043005/diff/1/components/cronet/android/c... File components/cronet/android/cronet_data_reduction_proxy.cc (right): https://codereview.chromium.org/1238043005/diff/1/components/cronet/android/c... components/cronet/android/cronet_data_reduction_proxy.cc:119: task_runner_, task_runner_, task_runner_, base::TimeDelta())); Is this going to be running blocking tasks on the networking thread? Can we pass nullptr for them to be sure they aren't used inappropriately? I don't know what these other task runners are used for; the comment on DataReductionProxyService() was not updated to describe these new task runners.
Thanks for the review! https://codereview.chromium.org/1238043005/diff/1/components/cronet/android/c... File components/cronet/android/cronet_data_reduction_proxy.cc (right): https://codereview.chromium.org/1238043005/diff/1/components/cronet/android/c... components/cronet/android/cronet_data_reduction_proxy.cc:119: task_runner_, task_runner_, task_runner_, base::TimeDelta())); On 2015/07/20 11:41:10, pauljensen wrote: > Is this going to be running blocking tasks on the networking thread? Can we > pass nullptr for them to be sure they aren't used inappropriately? I don't know > what these other task runners are used for; the comment on > DataReductionProxyService() was not updated to describe these new task runners. No, this will not run blocking tasks. This cl does 2 things: 1) Move initialization of |compression_stats| to be within |DataReductionProxyService| instead of in this file. So the task runner which was being passed into |DRPCompressionStats| is now being passed to |DRPService|. 2) Provide a persistent store along with a task runner for db operations. |DataStore| is a no-op implementation of the store, and so does not perform any blocking operations. Is passing nullptr useful considering that blocking IO operations on networking thread will cause AssertIOAllowed() to fail? Allowing the possibility of null task runners will require defensive code which does not seem ideal. I'll update the comment on DataReductionProxyService() in a different cl.
lgtm to get the build fixed https://codereview.chromium.org/1238043005/diff/1/components/cronet/android/c... File components/cronet/android/cronet_data_reduction_proxy.cc (right): https://codereview.chromium.org/1238043005/diff/1/components/cronet/android/c... components/cronet/android/cronet_data_reduction_proxy.cc:119: task_runner_, task_runner_, task_runner_, base::TimeDelta())); On 2015/07/20 17:25:42, kundaji wrote: > On 2015/07/20 11:41:10, pauljensen wrote: > > Is this going to be running blocking tasks on the networking thread? Can we > > pass nullptr for them to be sure they aren't used inappropriately? I don't > know > > what these other task runners are used for; the comment on > > DataReductionProxyService() was not updated to describe these new task > runners. > > No, this will not run blocking tasks. > > This cl does 2 things: > > 1) Move initialization of |compression_stats| to be within > |DataReductionProxyService| instead of in this file. So the task runner which > was being passed into |DRPCompressionStats| is now being passed to |DRPService|. > > 2) Provide a persistent store along with a task runner for db operations. > |DataStore| is a no-op implementation of the store, and so does not perform any > blocking operations. Should this be using Cronet's persistent storage directory? > > Is passing nullptr useful considering that blocking IO operations on networking > thread will cause AssertIOAllowed() to fail? Allowing the possibility of null > task runners will require defensive code which does not seem ideal. I don't know if AssertIOAllowed() is a no-op on debug builds. If you're sure we won't be performing blocking operations on the network thread it's fine. > > I'll update the comment on DataReductionProxyService() in a different cl.
The CQ bit was checked by kundaji@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pauljensen@chromium.org Link to the patchset: https://codereview.chromium.org/1238043005/#ps20001 (title: "Sync to head")
https://codereview.chromium.org/1238043005/diff/1/components/cronet/android/c... File components/cronet/android/cronet_data_reduction_proxy.cc (right): https://codereview.chromium.org/1238043005/diff/1/components/cronet/android/c... components/cronet/android/cronet_data_reduction_proxy.cc:119: task_runner_, task_runner_, task_runner_, base::TimeDelta())); On 2015/07/21 12:17:52, pauljensen wrote: > On 2015/07/20 17:25:42, kundaji wrote: > > On 2015/07/20 11:41:10, pauljensen wrote: > > > Is this going to be running blocking tasks on the networking thread? Can we > > > pass nullptr for them to be sure they aren't used inappropriately? I don't > > know > > > what these other task runners are used for; the comment on > > > DataReductionProxyService() was not updated to describe these new task > > runners. > > > > No, this will not run blocking tasks. > > > > This cl does 2 things: > > > > 1) Move initialization of |compression_stats| to be within > > |DataReductionProxyService| instead of in this file. So the task runner which > > was being passed into |DRPCompressionStats| is now being passed to > |DRPService|. > > > > 2) Provide a persistent store along with a task runner for db operations. > > |DataStore| is a no-op implementation of the store, and so does not perform > any > > blocking operations. > > Should this be using Cronet's persistent storage directory? The store is used to persist detailed data usage information such as total data usage, breakdown by sites and connection type, etc. If this functionality would be useful, then a persistent store could be provided. If so, we should pass in the file thread task runner. > > > > > Is passing nullptr useful considering that blocking IO operations on > networking > > thread will cause AssertIOAllowed() to fail? Allowing the possibility of null > > task runners will require defensive code which does not seem ideal. > > I don't know if AssertIOAllowed() is a no-op on debug builds. If you're sure we > won't be performing blocking operations on the network thread it's fine. I had tested that AssertIOAllowed() failed if I attempted to open a file on IO thread in Chrome build on my desktop, so it is not a no-op in Chrome atleast. > > > > > I'll update the comment on DataReductionProxyService() in a different cl. >
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1238043005/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ef3d9b3a8377ea053159d477731d6a44dfd40bcc Cr-Commit-Position: refs/heads/master@{#339671} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
