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

Issue 7204024: Revert 89629 - Revert 89486 - Revert 87047 - Revert 86422 - Make DHCP WPAD on by default. (Closed)

Created:
9 years, 6 months ago by Chris Evans
Modified:
9 years, 6 months ago
Reviewers:
Chris Evans, Jói
CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org, Paweł Hajdan Jr.
Visibility:
Public.

Description

Revert 89629 - Revert 89486 - Revert 87047 - Revert 86422 - Make DHCP WPAD on by default. Reason: Turning back on for trunk to collect data as performance worries are addressed. BUG=18575, 84047 TEST=Run Chrome on Windows without any flags. Enable auto-detect in proxy configuration. Net log should show attempts to auto-detect via DHCP. Review URL: http://codereview.chromium.org/7167016 TBR=joi@chromium.org NOTE: I'll roll it back in if it turns out this wasn't the problem. Review URL: http://codereview.chromium.org/7200025 TBR=cevans@chromium.org Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=89633

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+16 lines, -14 lines) Patch
M chrome/browser/net/connection_tester.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/net/proxy_service_factory.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/common/chrome_switches.h View 2 chunks +1 line, -1 line 0 comments Download
M chrome/common/chrome_switches.cc View 2 chunks +3 lines, -5 lines 0 comments Download
M net/proxy/dhcp_proxy_script_fetcher_factory.h View 1 chunk +1 line, -1 line 0 comments Download
M net/proxy/dhcp_proxy_script_fetcher_factory.cc View 1 chunk +1 line, -3 lines 0 comments Download
M net/proxy/dhcp_proxy_script_fetcher_factory_unittest.cc View 2 chunks +6 lines, -0 lines 0 comments Download

Messages

Total messages: 2 (0 generated)
Chris Evans
9 years, 6 months ago (2011-06-20 00:28:17 UTC) #1
Jói
9 years, 6 months ago (2011-06-20 10:52:20 UTC) #2
LGTM
Thanks Chris.

(sent from phone, pardon brevity)
On Jun 19, 2011 8:28 PM, <cevans@chromium.org> wrote:
> Reviewers: Chris Evans,
>
> Description:
> Revert 89629 - Revert 89486 - Revert 87047 - Revert 86422 - Make DHCP WPAD

> on by
> default.
>
> Reason: Turning back on for trunk to collect data as performance worries
are
> addressed.
>
> BUG=18575,84047
> TEST=Run Chrome on Windows without any flags. Enable auto-detect in proxy
> configuration. Net log should show attempts to auto-detect via DHCP.
>
> Review URL: http://codereview.chromium.org/7167016
>
> TBR=joi@chromium.org
>
> NOTE: I'll roll it back in if it turns out this wasn't the problem.
> Review URL: http://codereview.chromium.org/7200025
>
> TBR=cevans@chromium.org
>
> Please review this at http://codereview.chromium.org/7204024/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src/
>
> Affected files:
> M chrome/browser/net/connection_tester.cc
> M chrome/browser/net/proxy_service_factory.cc
> M chrome/common/chrome_switches.h
> M chrome/common/chrome_switches.cc
> M net/proxy/dhcp_proxy_script_fetcher_factory.h
> M net/proxy/dhcp_proxy_script_fetcher_factory.cc
> M net/proxy/dhcp_proxy_script_fetcher_factory_unittest.cc
>
>
> Index: chrome/browser/net/connection_tester.cc
> ===================================================================
> --- chrome/browser/net/connection_tester.cc (revision 89632)
> +++ chrome/browser/net/connection_tester.cc (working copy)
> @@ -187,8 +187,8 @@
>
> net::DhcpProxyScriptFetcherFactory dhcp_factory;
> if (CommandLine::ForCurrentProcess()->HasSwitch(
> - switches::kEnableDhcpWpad)) {
> - dhcp_factory.set_enabled(true);
> + switches::kDisableDhcpWpad)) {
> + dhcp_factory.set_enabled(false);
> }
>
> proxy_service->reset(net::ProxyService::CreateUsingV8ProxyResolver(
> Index: chrome/browser/net/proxy_service_factory.cc
> ===================================================================
> --- chrome/browser/net/proxy_service_factory.cc (revision 89632)
> +++ chrome/browser/net/proxy_service_factory.cc (working copy)
> @@ -86,8 +86,8 @@
> net::ProxyService* proxy_service;
> if (use_v8) {
> net::DhcpProxyScriptFetcherFactory dhcp_factory;
> - if (command_line.HasSwitch(switches::kEnableDhcpWpad)) {
> - dhcp_factory.set_enabled(true);
> + if (command_line.HasSwitch(switches::kDisableDhcpWpad)) {
> + dhcp_factory.set_enabled(false);
> }
>
> proxy_service = net::ProxyService::CreateUsingV8ProxyResolver(
> Index: chrome/common/chrome_switches.cc
> ===================================================================
> --- chrome/common/chrome_switches.cc (revision 89632)
> +++ chrome/common/chrome_switches.cc (working copy)
> @@ -223,6 +223,9 @@
> // Disables the custom JumpList on Windows 7.
> const char kDisableCustomJumpList[] = "disable-custom-jumplist";
>
> +// Disables retrieval of PAC URLs from DHCP as per the WPAD standard.
> +const char kDisableDhcpWpad[] = "disable-dhcp-wpad";
> +
> // Disable extensions.
> const char kDisableExtensions[] = "disable-extensions";
>
> @@ -398,11 +401,6 @@
> // Enables web developers to create apps for Chrome without using crx
> packages.
> const char kEnableCrxlessWebApps[] = "enable-crxless-web-apps";
>
> -// Enables retrieval of PAC URLs from DHCP as per the WPAD standard. Note
> -// that this feature is not supported on all platforms, and using the
flag
> -// is a no-op on such platforms.
> -const char kEnableDhcpWpad[] = "enable-dhcp-wpad";
> -
> // Enable DNS side checking of certificates. Still experimental, should
> only
> // be used by developers at the current time.
> const char kEnableDNSCertProvenanceChecking[] =
> Index: chrome/common/chrome_switches.h
> ===================================================================
> --- chrome/common/chrome_switches.h (revision 89632)
> +++ chrome/common/chrome_switches.h (working copy)
> @@ -71,6 +71,7 @@
> extern const char kDisableClientSidePhishingDetection[];
> extern const char kDisableConnectBackupJobs[];
> extern const char kDisableCustomJumpList[];
> +extern const char kDisableDhcpWpad[];
> extern const char kDisableExtensionsFileAccessCheck[];
> extern const char kDisableExtensions[];
> extern const char kDisableFlashSandbox[];
> @@ -118,7 +119,6 @@
> extern const char kEnableCompositeToTexture[];
> extern const char kEnableConnectBackupJobs[];
> extern const char kEnableCrxlessWebApps[];
> -extern const char kEnableDhcpWpad[];
> extern const char kEnableDNSCertProvenanceChecking[];
> extern const char kEnableExperimentalExtensionApis[];
> extern const char kEnableExtensionTimelineApi[];
> Index: net/proxy/dhcp_proxy_script_fetcher_factory.cc
> ===================================================================
> --- net/proxy/dhcp_proxy_script_fetcher_factory.cc (revision 89632)
> +++ net/proxy/dhcp_proxy_script_fetcher_factory.cc (working copy)
> @@ -15,9 +15,7 @@
>
> DhcpProxyScriptFetcherFactory::DhcpProxyScriptFetcherFactory()
> : feature_enabled_(false) {
> - // TODO(joi): Change this default, and the comment on |set_enabled()|,
> - // when the time is right.
> - set_enabled(false);
> + set_enabled(true);
> }
>
> DhcpProxyScriptFetcher* DhcpProxyScriptFetcherFactory::Create(
> Index: net/proxy/dhcp_proxy_script_fetcher_factory.h
> ===================================================================
> --- net/proxy/dhcp_proxy_script_fetcher_factory.h (revision 89632)
> +++ net/proxy/dhcp_proxy_script_fetcher_factory.h (working copy)
> @@ -47,7 +47,7 @@
> // Attempts to enable/disable the DHCP WPAD feature. Does nothing
> // if |IsSupported()| returns false.
> //
> - // The current default is |enabled() == false|.
> + // The default is |enabled() == true|.
> void set_enabled(bool enabled);
>
> // Returns true if the DHCP WPAD feature is enabled. Always returns
> Index: net/proxy/dhcp_proxy_script_fetcher_factory_unittest.cc
> ===================================================================
> --- net/proxy/dhcp_proxy_script_fetcher_factory_unittest.cc (revision
89632)
> +++ net/proxy/dhcp_proxy_script_fetcher_factory_unittest.cc (working copy)
> @@ -9,8 +9,10 @@
>
> namespace net {
> namespace {
> +
> TEST(DhcpProxyScriptFetcherFactoryTest, DoNothingWhenDisabled) {
> DhcpProxyScriptFetcherFactory factory;
> + factory.set_enabled(false);
> scoped_ptr<DhcpProxyScriptFetcher> fetcher(factory.Create(NULL));
> EXPECT_EQ("", fetcher->GetFetcherName());
> }
> @@ -36,7 +38,11 @@
>
> TEST(DhcpProxyScriptFetcherFactoryTest, SetEnabled) {
> DhcpProxyScriptFetcherFactory factory;
> +#if defined(OS_WIN)
> + EXPECT_TRUE(factory.enabled());
> +#else
> EXPECT_FALSE(factory.enabled());
> +#endif // defined(OS_WIN)
>
> factory.set_enabled(false);
> EXPECT_FALSE(factory.enabled());
>
>

Powered by Google App Engine
This is Rietveld 408576698