Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(191)

Issue 2787253002: chromeos: Simplify D-Bus proxy provider (but not tests). (Closed)

Created:
3 years, 8 months ago by Daniel Erat
Modified:
3 years, 8 months ago
Reviewers:
James Cook, eroman
CC:
chromium-reviews, tbansal+watch-data-reduction-proxy_chromium.org, oshima+watch_chromium.org, hashimoto+watch_chromium.org, davemoore+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

chromeos: Simplify D-Bus proxy provider (but not tests). Move some complexity out of ProxyResolutionServiceProvider::Delegate and add a lot of complexity to its unit tests by constructing an alternate net::ProxyService. BUG=446115, 703217, 706665 Review-Url: https://codereview.chromium.org/2787253002 Cr-Commit-Position: refs/heads/master@{#461258} Committed: https://chromium.googlesource.com/chromium/src/+/4a50453a68e5577dd9a54510c3810d112b35a763

Patch Set 1 #

Total comments: 7

Patch Set 2 : remove error stuff from tests #

Unified diffs Side-by-side diffs Delta from patch set Stats (+110 lines, -101 lines) Patch
M chrome/browser/chromeos/dbus/chrome_proxy_resolution_service_provider_delegate.h View 1 chunk +0 lines, -4 lines 0 comments Download
M chrome/browser/chromeos/dbus/chrome_proxy_resolution_service_provider_delegate.cc View 2 chunks +0 lines, -13 lines 0 comments Download
M chromeos/dbus/services/proxy_resolution_service_provider.h View 3 chunks +0 lines, -11 lines 0 comments Download
M chromeos/dbus/services/proxy_resolution_service_provider.cc View 2 chunks +4 lines, -3 lines 0 comments Download
M chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc View 1 10 chunks +106 lines, -70 lines 0 comments Download

Messages

Total messages: 17 (6 generated)
Daniel Erat
james, i'm curious about what you'll think about this. :-P eric, would you mind taking ...
3 years, 8 months ago (2017-03-31 02:39:44 UTC) #2
Daniel Erat
On 2017/03/31 02:39:44, Daniel Erat wrote: > james, i'm curious about what you'll think about ...
3 years, 8 months ago (2017-03-31 03:22:51 UTC) #4
James Cook
LGTM with one question. I like pushing the complexity onto the test code. https://codereview.chromium.org/2787253002/diff/1/chromeos/dbus/services/proxy_resolution_service_provider.cc File ...
3 years, 8 months ago (2017-03-31 17:33:10 UTC) #5
Daniel Erat
https://codereview.chromium.org/2787253002/diff/1/chromeos/dbus/services/proxy_resolution_service_provider.cc File chromeos/dbus/services/proxy_resolution_service_provider.cc (right): https://codereview.chromium.org/2787253002/diff/1/chromeos/dbus/services/proxy_resolution_service_provider.cc#newcode202 chromeos/dbus/services/proxy_resolution_service_provider.cc:202: const int result = proxy_service->ResolveProxy( On 2017/03/31 17:33:10, James ...
3 years, 8 months ago (2017-03-31 17:46:38 UTC) #6
Daniel Erat
i've returned the test code for making ProxyResolver return custom errors, along with the test ...
3 years, 8 months ago (2017-03-31 21:33:26 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2787253002/20001
3 years, 8 months ago (2017-03-31 21:34:20 UTC) #10
commit-bot: I haz the power
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/4a50453a68e5577dd9a54510c3810d112b35a763
3 years, 8 months ago (2017-03-31 22:45:11 UTC) #13
eroman
> eric, would you mind taking a quick look at the test file to see ...
3 years, 8 months ago (2017-04-04 21:06:10 UTC) #14
Daniel Erat
thanks for taking a look! https://codereview.chromium.org/2787253002/diff/1/chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc File chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc (right): https://codereview.chromium.org/2787253002/diff/1/chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc#newcode308 chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc:308: // about how to ...
3 years, 8 months ago (2017-04-05 02:34:14 UTC) #15
eroman
Sure, feel free to follow-up with me (can show me using a separate CL where ...
3 years, 8 months ago (2017-04-05 18:27:40 UTC) #16
Daniel Erat
3 years, 8 months ago (2017-04-06 00:39:54 UTC) #17
Message was sent while issue was closed.
On 2017/04/05 18:27:40, eroman wrote:
> Sure, feel free to follow-up with me (can show me using a separate CL where
you
> see the problem).
> 
> I think what you need to change is:
> 
> Replace:
> 
> net::ProxyConfig config = net::ProxyConfig::CreateAutoDetect();
> 
> With:
> 
> ProxyConfig config;
> config.set_pac_url(GURL("http://www.example.com");
> config.set_pac_mandatory(true);

thanks! that's what i was missing, i think -- i was trying to set pac_url and
pac_mandatory after calling CreateAutoDetect(). when i instead set them on an
empty config that doesn't have auto_detect set and then return an error from
ProxyResolver::GetProxyForURL(), i get an ultimate response of
net::ERR_MANDATORY_PROXY_CONFIGURATION_FAILED (along with "DIRECT" as the PAC
string).

Powered by Google App Engine
This is Rietveld 408576698