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

Showing 1-8 of 8 messages
Ignore --use-spdy=off command line option if spdy.disabled pref is (issue 10916341) rten...@chromium.org 9/17/12 10:33 AM
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();


Re: Ignore --use-spdy=off command line option if spdy.disabled pref is (issue 10916341) joaod...@chromium.org 9/18/12 4:35 AM
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/
Re: Ignore --use-spdy=off command line option if spdy.disabled pref is (issue 10916341) rten...@chromium.org 9/18/12 2:16 PM
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/
Re: Ignore --use-spdy=off command line option if spdy.disabled pref is (issue 10916341) commi...@chromium.org 9/18/12 2:16 PM
Re: Ignore --use-spdy=off command line option if spdy.disabled pref is (issue 10916341) commi...@chromium.org 9/18/12 2:16 PM
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/
Re: Ignore --use-spdy=off command line option if spdy.disabled pref is (issue 10916341) rten...@chromium.org 9/18/12 2:18 PM
Hi thestig@,
   Can I get OWNERs approval (or rubber stamp) please?

thanks much
raman

https://chromiumcodereview.appspot.com/10916341/
Re: Ignore --use-spdy=off command line option if spdy.disabled pref is (issue 10916341) the...@chromium.org 9/18/12 3:02 PM
Re: Ignore --use-spdy=off command line option if spdy.disabled pref is (issue 10916341) commi...@chromium.org 9/18/12 3:03 PM