Remove unnecessary DRPConfiguratorTestUtils and add more DRPConfig tests

Remove unnecessary DRPConfiguratorTestUtils and add more DRPConfig tests

The TestDRPConfigurator served no purpose, so this CL removes it. This
CL also changes some of the DRPConfig tests around the secure proxy
check to test that HTTPS proxies are removed from the effective proxy
lists. This CL also contains miscellaneous cleanup of DRPConfig that
doesn't affect the functionality of that class, only the performance and
readability.

BUG=613021
Committed: https://crrev.com/be89059015aa60b12762b5858be2ad98203b7319
Cr-Commit-Position: refs/heads/master@{#394929}

[sclittle, 2016-05-19 00:22:29]

Description was changed from

==========
Remove unnecessary DRPConfiguratorTestUtils and add more DRPConfig tests

The TestDRPConfigurator served no purpose, so this CL removes it. This
CL also changes some of the DRPConfig tests around the secure proxy
check to test that HTTPS proxies are removed from the effective proxy
lists. This CL also contains miscellaneous cleanup of DRPConfig that
don't affect the functionality of that class, only the performance and
readability.

BUG=
==========

to

==========
Remove unnecessary DRPConfiguratorTestUtils and add more DRPConfig tests

The TestDRPConfigurator served no purpose, so this CL removes it. This
CL also changes some of the DRPConfig tests around the secure proxy
check to test that HTTPS proxies are removed from the effective proxy
lists. This CL also contains miscellaneous cleanup of DRPConfig that
don't affect the functionality of that class, only the performance and
readability.

BUG=613021
==========

[sclittle, 2016-05-19 00:22:48]

sclittle@chromium.org changed reviewers:
+ tbansal@chromium.org

[sclittle, 2016-05-19 00:31:24]

Description was changed from

==========
Remove unnecessary DRPConfiguratorTestUtils and add more DRPConfig tests

The TestDRPConfigurator served no purpose, so this CL removes it. This
CL also changes some of the DRPConfig tests around the secure proxy
check to test that HTTPS proxies are removed from the effective proxy
lists. This CL also contains miscellaneous cleanup of DRPConfig that
don't affect the functionality of that class, only the performance and
readability.

BUG=613021
==========

to

==========
Remove unnecessary DRPConfiguratorTestUtils and add more DRPConfig tests

The TestDRPConfigurator served no purpose, so this CL removes it. This
CL also changes some of the DRPConfig tests around the secure proxy
check to test that HTTPS proxies are removed from the effective proxy
lists. This CL also contains miscellaneous cleanup of DRPConfig that
doesn't affect the functionality of that class, only the performance and
readability.

BUG=613021
==========

[tbansal1, 2016-05-19 01:07:54]

lgtm %nits.

https://codereview.chromium.org/1989393004/diff/20001/components/data_reducti...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
(right):

https://codereview.chromium.org/1989393004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:104:
base::StringPiece prefix,
pass by ref?

https://codereview.chromium.org/1989393004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:360:
proxy_info->is_fallback = (proxy_it != proxy_list.begin());
I wonder if |is_fallback| is useful now given that the config server is now
sending 3 proxies in some cases (they are sending http proxy followed by https
proxy!).

https://codereview.chromium.org/1989393004/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc
(right):

https://codereview.chromium.org/1989393004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:65:
std::string GetRetryMapKeyFromOrigin(const std::string& origin) {
I am confused. What is this method doing? Returning whatever is passed?

https://codereview.chromium.org/1989393004/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h
(right):

https://codereview.chromium.org/1989393004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h:436:
// Uses a |TestDataReductionProxyConfigurator| to record proxy configuration
I think you want to delete this comment, instead of the one below it.

[sclittle, 2016-05-19 02:25:22]

https://codereview.chromium.org/1989393004/diff/20001/components/data_reducti...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
(right):

https://codereview.chromium.org/1989393004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:104:
base::StringPiece prefix,
On 2016/05/19 01:07:54, tbansal1 wrote:
> pass by ref?

It's faster to pass by value, see
https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin...

https://codereview.chromium.org/1989393004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:360:
proxy_info->is_fallback = (proxy_it != proxy_list.begin());
On 2016/05/19 01:07:54, tbansal1 wrote:
> I wonder if |is_fallback| is useful now given that the config server is now
> sending 3 proxies in some cases (they are sending http proxy followed by https
> proxy!).

I think |is_fallback| is still used for reporting certain histograms, e.g. for
recording bypasses, but you're right that we should be careful not to assume
that primary=HTTPS and fallback=HTTP.

https://codereview.chromium.org/1989393004/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc
(right):

https://codereview.chromium.org/1989393004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:65:
std::string GetRetryMapKeyFromOrigin(const std::string& origin) {
On 2016/05/19 01:07:54, tbansal1 wrote:
> I am confused. What is this method doing? Returning whatever is passed?

I just moved this method from below in the d_r_p namespace to up here into an
anonymous namespace. I don't know why it exists or what it's supposed to do, but
I'll just leave it as-is for now.

https://codereview.chromium.org/1989393004/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h
(right):

https://codereview.chromium.org/1989393004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h:436:
// Uses a |TestDataReductionProxyConfigurator| to record proxy configuration
On 2016/05/19 01:07:54, tbansal1 wrote:
> I think you want to delete this comment, instead of the one below it.

Done.

[sclittle, 2016-05-19 02:25:31]

The CQ bit was checked by sclittle@chromium.org

[sclittle, 2016-05-19 02:25:31]

The patchset sent to the CQ was uploaded after l-g-t-m from tbansal@chromium.org
Link to the patchset: https://codereview.chromium.org/1989393004/#ps40001
(title: "Fixed comment")

[commit-bot: I haz the power, 2016-05-19 02:25:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1989393004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1989393004/40001

[commit-bot: I haz the power, 2016-05-19 03:09:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-19 03:09:20]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sclittle, 2016-05-19 20:51:08]

The CQ bit was checked by sclittle@chromium.org

[sclittle, 2016-05-19 20:51:09]

The patchset sent to the CQ was uploaded after l-g-t-m from tbansal@chromium.org
Link to the patchset: https://codereview.chromium.org/1989393004/#ps60001
(title: "Fix failing DCHECK")

[commit-bot: I haz the power, 2016-05-19 20:51:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1989393004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1989393004/60001

[commit-bot: I haz the power, 2016-05-20 00:35:23]

Description was changed from

==========
Remove unnecessary DRPConfiguratorTestUtils and add more DRPConfig tests

The TestDRPConfigurator served no purpose, so this CL removes it. This
CL also changes some of the DRPConfig tests around the secure proxy
check to test that HTTPS proxies are removed from the effective proxy
lists. This CL also contains miscellaneous cleanup of DRPConfig that
doesn't affect the functionality of that class, only the performance and
readability.

BUG=613021
==========

to

==========
Remove unnecessary DRPConfiguratorTestUtils and add more DRPConfig tests

The TestDRPConfigurator served no purpose, so this CL removes it. This
CL also changes some of the DRPConfig tests around the secure proxy
check to test that HTTPS proxies are removed from the effective proxy
lists. This CL also contains miscellaneous cleanup of DRPConfig that
doesn't affect the functionality of that class, only the performance and
readability.

BUG=613021
==========

[commit-bot: I haz the power, 2016-05-20 00:35:24]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-20 00:38:01]

Description was changed from

==========
Remove unnecessary DRPConfiguratorTestUtils and add more DRPConfig tests

The TestDRPConfigurator served no purpose, so this CL removes it. This
CL also changes some of the DRPConfig tests around the secure proxy
check to test that HTTPS proxies are removed from the effective proxy
lists. This CL also contains miscellaneous cleanup of DRPConfig that
doesn't affect the functionality of that class, only the performance and
readability.

BUG=613021
==========

to

==========
Remove unnecessary DRPConfiguratorTestUtils and add more DRPConfig tests

The TestDRPConfigurator served no purpose, so this CL removes it. This
CL also changes some of the DRPConfig tests around the secure proxy
check to test that HTTPS proxies are removed from the effective proxy
lists. This CL also contains miscellaneous cleanup of DRPConfig that
doesn't affect the functionality of that class, only the performance and
readability.

BUG=613021

Committed: https://crrev.com/be89059015aa60b12762b5858be2ad98203b7319
Cr-Commit-Position: refs/heads/master@{#394929}
==========

[commit-bot: I haz the power, 2016-05-20 00:38:03]

Patchset 4 (id:??) landed as
https://crrev.com/be89059015aa60b12762b5858be2ad98203b7319
Cr-Commit-Position: refs/heads/master@{#394929}