Migrate components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc to using SimpleURLLoader |
|||||||||
Issue descriptionBlocks Android canary
,
Sep 25
,
Oct 3
,
Oct 3
Oops, meant to CC myself.
,
Oct 10
1st WIP CL here: https://chromium-review.googlesource.com/c/chromium/src/+/1274405. Remark, I added a small dependency from components/data_reduction_proxy to content/test, and iOS bots complained, although there is not plans to enable/enable DRP on iOS.
,
Oct 10
,
Oct 10
I believe iOS is having an issue because components/net_log depends on components/data_reduction_proxy and net_log is used in iOS: https://cs.chromium.org/chromium/src/components/net_log/BUILD.gn?l=17&rcl=03c68b4c484214eaa1465d05e39be15ea6341d75 It looks like the only line that uses DRP is here: https://cs.chromium.org/chromium/src/components/net_log/chrome_net_log.cc?l=83&rcl=c87bfede7bd30b0913284b3af03bbe114af7fd9d Since DRP isn't used on iOS, it should be able to depend on content. Maybe we can split the DataReductionProxyEventStore::AddConstants static function (which is the only one used in net_log) into a separate build target. Tarun, what are your thoughts on this?
,
Oct 10
Thanks, cduvall. I was able to remove the dependence on content/, and there is one left bot/test failing: DataReductionProxyConfigServiceClientTest.FetchConfigOnForeground I have an android/simulator build finishing and should be able to get it going shortly after. Then the CL will be ready for review. PS: I offer to help with untangling ios+DRP in a follow up too.
,
Oct 10
I think we can remove the whole DataReductionProxyEventStore class since DRP should no longer be reporting to netlog directly. We can probably add netlog statements from https://cs.chromium.org/chromium/src/services/network/network_service_proxy_delegate.cc if we need some netlog integration for the proxy.
,
Oct 10
Adding netlog integration to https://cs.chromium.org/chromium/src/services/network/network_service_proxy_delegate.cc can probably be done later (if needed).
,
Oct 11
It looks like getting rid of DataReductionProxyEventStore will be a bit more complicated because of some of the uses in https://cs.chromium.org/chromium/src/chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc. I put up http://crrev.com/c/1275134 to reduce the dep for now so we can depend on //content in the future (I needed this for a change I was working on too).
,
Oct 11
Eric is working on a better version removing netlog integration completely at http://crrev.com/c/1274613.
,
Oct 11
,
Oct 11
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a9b01b75f20b0659199c7b496de9c5156e020fce commit a9b01b75f20b0659199c7b496de9c5156e020fce Author: Antonio Gomes <tonikitoo@igalia.com> Date: Thu Oct 11 21:06:43 2018 Migrate components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc to using SimpleURLLoader URLFetcher will stop working with advent of Network Service, and SimpleURLLoader is the replacement API for most clients. This CL migrates Android's WarmupURLFetcher and the respective unittests away from URLFetcher. When migrating the unittests, it was opted to implement a queue of mocked responses mechanism, to mimic the current behavior (using net::MockClientSocketFactory and net::MockRead objects). The CL also removed the helper method GetURLFetcherForConfig, merging it into RetrieveRemoteConfig. Basically, although it made sense with URLFetcher's set up and flow of method calls, it is not the case with SimpleURLLoader. BUG= 879784 Change-Id: I0c38042fcd37f5848dca49ff8322e514f818cdba Reviewed-on: https://chromium-review.googlesource.com/c/1274405 Reviewed-by: Matt Menke <mmenke@chromium.org> Reviewed-by: Tarun Bansal <tbansal@chromium.org> Commit-Queue: Antonio Gomes <tonikitoo@igalia.com> Cr-Commit-Position: refs/heads/master@{#598936} [modify] https://crrev.com/a9b01b75f20b0659199c7b496de9c5156e020fce/components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc [modify] https://crrev.com/a9b01b75f20b0659199c7b496de9c5156e020fce/components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.h [modify] https://crrev.com/a9b01b75f20b0659199c7b496de9c5156e020fce/components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc [modify] https://crrev.com/a9b01b75f20b0659199c7b496de9c5156e020fce/components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc [modify] https://crrev.com/a9b01b75f20b0659199c7b496de9c5156e020fce/components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.cc [modify] https://crrev.com/a9b01b75f20b0659199c7b496de9c5156e020fce/components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h
,
Oct 12
,
Oct 16
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cdd8a239edc662ee156464512b73933960576c54 commit cdd8a239edc662ee156464512b73933960576c54 Author: Antonio Gomes <tonikitoo@igalia.com> Date: Tue Oct 16 21:43:18 2018 [DRP] Remove BasicHTTPURLRequestContextGetter In [1], DataReductionProxyConfigServiceClient [2] switched from using URLFetcher to SimpleURLLoader for making URL requests. During the discussion phase with tbansal and cduvall, it was mentioned that the URLRequestContext specialization used by DataReductionProxyConfigServiceClient was originally needed to obey two criterias: (i) Use of SPDY and QUIC protocols is explicitly disabled; (ii) Use of any proxies is also disabled. Today, (i) isn't needed and (ii) can be obtained with a specific load flag: net::LOAD_BYPASS_PROXY. Hence, the URLRequestContext specialization (namely BasicHTTPURLRequestContextGetter) became unneeded, since (ii) can be easily respected with the default network context. This CL removes the left over BasicHTTPURLRequestContextGetter and associated code. [1] https://crrev.com/c/1274405 [2] components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc Bug= 879784 Change-Id: Ib46fecbbe03c57f0fc9375cb8986da62d77bb525 Reviewed-on: https://chromium-review.googlesource.com/c/1284169 Reviewed-by: rajendrant <rajendrant@chromium.org> Reviewed-by: Tarun Bansal <tbansal@chromium.org> Commit-Queue: Antonio Gomes <tonikitoo@igalia.com> Cr-Commit-Position: refs/heads/master@{#600133} [modify] https://crrev.com/cdd8a239edc662ee156464512b73933960576c54/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc [modify] https://crrev.com/cdd8a239edc662ee156464512b73933960576c54/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h [modify] https://crrev.com/cdd8a239edc662ee156464512b73933960576c54/components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc [modify] https://crrev.com/cdd8a239edc662ee156464512b73933960576c54/components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc [modify] https://crrev.com/cdd8a239edc662ee156464512b73933960576c54/components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h [modify] https://crrev.com/cdd8a239edc662ee156464512b73933960576c54/components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc [modify] https://crrev.com/cdd8a239edc662ee156464512b73933960576c54/components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.cc [modify] https://crrev.com/cdd8a239edc662ee156464512b73933960576c54/components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by dougt@chromium.org
, Aug 31