Ignore --use-spdy=off command line option if spdy.disabled pref is (issue 10916341)

[rten...@chromium.org]

Reviewers: Joao da Silva,

Message:
Hi Joao,
Finished implementing the 2nd part of the fix you had suggested for
controlling
spdy.enabled preference. Would appreciate your comments.

thanks
raman

Description:
Ignore --use-spdy=off command line option if spdy.disabled pref is
managed by the policy.

Implement the following priority order for spdy.disabled pref and
--use-spdy command line options.

1. If there is a policy for spdy.disabled, use its value
2. If there is a command line for --use-spdy, use its value
3. If there is a user pref for spdy.disabled, use its value
4. Otherwise use the default value of the pref for spdy.pref (which will
always enable SPDY).

Part of priortity order was fixed earlier in
https://chromiumcodereview.appspot.com/10911198

R=joaod...@chromium.org
BUG=83895
TEST=browser unit tests. Specify --disable-spdy command line switch
and in chrome://net-internals SPDY should say it is off.

Please review this at https://chromiumcodereview.appspot.com/10916341/

SVN Base: svn://chrome-svn/chrome/trunk/src/

Affected files:
M chrome/browser/chrome_browser_main.cc


Index: chrome/browser/chrome_browser_main.cc
===================================================================
--- chrome/browser/chrome_browser_main.cc (revision 157062)
+++ chrome/browser/chrome_browser_main.cc (working copy)
@@ -59,6 +59,7 @@
#include "chrome/browser/performance_monitor/performance_monitor.h"
#include "chrome/browser/performance_monitor/startup_timer.h"
#include "chrome/browser/plugin_prefs.h"
+#include "chrome/browser/policy/policy_service.h"
#include "chrome/browser/prefs/pref_service.h"
#include "chrome/browser/prefs/pref_value_store.h"
#include "chrome/browser/prefs/scoped_user_pref_update.h"
@@ -116,6 +117,7 @@
#include "net/spdy/spdy_session_pool.h"
#include "net/url_request/url_request.h"
#include "net/websockets/websocket_job.h"
+#include "policy/policy_constants.h"
#include "ui/base/l10n/l10n_util.h"
#include "ui/base/layout.h"
#include "ui/base/resource/resource_bundle.h"
@@ -201,13 +203,24 @@
}
}

-void InitializeNetworkOptions(const CommandLine& parsed_command_line) {
+void InitializeNetworkOptions(const CommandLine& parsed_command_line,
+ policy::PolicyService* policy_service) {
if (parsed_command_line.HasSwitch(switches::kEnableFileCookies)) {
// Enable cookie storage for file:// URLs. Must do this before the
first
// Profile (and therefore the first CookieMonster) is created.
net::CookieMonster::EnableFileScheme();
}

+#if defined(ENABLE_CONFIGURATION_POLICY)
+ bool has_spdy_policy = policy_service->GetPolicies(
+ policy::POLICY_DOMAIN_CHROME,
+ std::string()).Get(policy::key::kDisableSpdy) != NULL;
+ // If "spdy.disabled" peference is controlled via policy, then skip
use-spdy
+ // command line flags.
+ if (has_spdy_policy)
+ return;
+#endif // ENABLE_CONFIGURATION_POLICY
+
if (parsed_command_line.HasSwitch(switches::kEnableIPPooling))
net::SpdySessionPool::enable_ip_pooling(true);

@@ -839,7 +852,8 @@
chrome::VersionInfo::GetVersionStringModifier());
#endif

- InitializeNetworkOptions(parsed_command_line());
+ InitializeNetworkOptions(parsed_command_line(),
+ browser_process_->policy_service());

// Initialize tracking synchronizer system.
tracking_synchronizer_ = new
chrome_browser_metrics::TrackingSynchronizer();

[joaod...@chromium.org]

lgtm, thanks for doing this!


http://codereview.chromium.org/10916341/diff/1/chrome/browser/chrome_browser_main.cc
File chrome/browser/chrome_browser_main.cc (right):

http://codereview.chromium.org/10916341/diff/1/chrome/browser/chrome_browser_main.cc#newcode120
chrome/browser/chrome_browser_main.cc:120: #include
"policy/policy_constants.h"
policy_service.h is always available, but this header only exists if
ENABLE_CONFIGURATION_POLICY is defined. Please #include it below within
#ifdefs (the linux_redux and android try bots should catch this).

http://codereview.chromium.org/10916341/diff/1/chrome/browser/chrome_browser_main.cc#newcode218
chrome/browser/chrome_browser_main.cc:218: // If "spdy.disabled"

peference is controlled via policy, then skip use-spdy

Nit: "peference" typo

http://codereview.chromium.org/10916341/

[rten...@chromium.org]

Many many thanks Joao da Silva,
raman

http://codereview.chromium.org/10916341/diff/1/chrome/browser/chrome_browser_main.cc
File chrome/browser/chrome_browser_main.cc (right):

http://codereview.chromium.org/10916341/diff/1/chrome/browser/chrome_browser_main.cc#newcode120
chrome/browser/chrome_browser_main.cc:120: #include
"policy/policy_constants.h"

On 2012/09/18 11:35:56, Joao da Silva wrote:
> policy_service.h is always available, but this header only exists if
> ENABLE_CONFIGURATION_POLICY is defined. Please #include it below
within #ifdefs
> (the linux_redux and android try bots should catch this).

Done.

http://codereview.chromium.org/10916341/diff/1/chrome/browser/chrome_browser_main.cc#newcode218
chrome/browser/chrome_browser_main.cc:218: // If "spdy.disabled"
peference is controlled via policy, then skip use-spdy

On 2012/09/18 11:35:56, Joao da Silva wrote:
> Nit: "peference" typo

Done.

http://codereview.chromium.org/10916341/

[commi...@chromium.org]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rten...@chromium.org/10916341/10001


https://chromiumcodereview.appspot.com/10916341/

[commi...@chromium.org]

Presubmit check for 10916341-10001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome



https://chromiumcodereview.appspot.com/10916341/

[rten...@chromium.org]

Hi thestig@,
Can I get OWNERs approval (or rubber stamp) please?

thanks much
raman

https://chromiumcodereview.appspot.com/10916341/

[the...@chromium.org]

lgtm



https://chromiumcodereview.appspot.com/10916341/

[commi...@chromium.org]