New issue
Advanced search Search tips

Issue 879784 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 12
Cc:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug

Blocking:
issue 721403
issue 855620



Sign in to add a comment

Migrate components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc to using SimpleURLLoader

Project Member Reported by jam@chromium.org, Aug 31

Issue description

Blocks Android canary



 
Status: Available (was: Untriaged)
Blocking: 721403
Owner: cduvall@chromium.org
Cc: cduvall@chromium.org
Owner: ----
Oops, meant to CC myself.
Status: Started (was: Available)
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.
Owner: toniki...@chromium.org
Cc: tbansal@chromium.org
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?
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.
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.
Adding netlog integration to https://cs.chromium.org/chromium/src/services/network/network_service_proxy_delegate.cc can probably be done later (if needed).
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).
Eric is working on a better version removing netlog integration completely at http://crrev.com/c/1274613.
Blocking: 855620
Project Member

Comment 14 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
Project Member

Comment 16 by bugdroid1@chromium.org, 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