|
|
Created:
8 years, 3 months ago by ramant (doing other things) Modified:
8 years, 3 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org, jar (doing other things), willchan no longer on Chromium Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionSPDY - If spdy.disable is not enabled in policy file, then don't update if SPDY is enabled or not. Leave the current state (as modified by the command line argument or as defined by the default value).
Will submit a separate CL not to use command line arguments that disable SPDY if Policy file specifies spdy.disable pref.
R=mmenke@chromium.org, joaodasilva@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.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156863
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 3
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #Messages
Total messages: 22 (0 generated)
http://codereview.chromium.org/10911198/diff/12001/chrome/browser/net/net_pre... File chrome/browser/net/net_pref_observer.cc (right): http://codereview.chromium.org/10911198/diff/12001/chrome/browser/net/net_pre... chrome/browser/net/net_pref_observer.cc:51: net::HttpStreamFactory::set_spdy_enabled(!*spdy_disabled_); This removes the ability of modifying the policy value to re-enable SPDY, when it's disabled by a means other than the command line. Am I correct that this is already broken, anyways, since we never re-populate HttpStreamFactory::next_protos_, after deleting it when spdy is disabled?
http://codereview.chromium.org/10911198/diff/12001/chrome/browser/net/net_pre... File chrome/browser/net/net_pref_observer.cc (right): http://codereview.chromium.org/10911198/diff/12001/chrome/browser/net/net_pre... chrome/browser/net/net_pref_observer.cc:51: net::HttpStreamFactory::set_spdy_enabled(!*spdy_disabled_); On 2012/09/12 01:31:17, Matt Menke wrote: > This removes the ability of modifying the policy value to re-enable SPDY, when > it's disabled by a means other than the command line. Correct. My proposal is for the user to run without the command line option (--use-spdy=off) for policy to take effect. As you had mentioned "So disabling takes precedence over enabling, in all cases". > > Am I correct that this is already broken, anyways, since we never re-populate > HttpStreamFactory::next_protos_, after deleting it when spdy is disabled? In http_stream_facttory_impl_job.cc, we don't set _using_spdy to true unless spdy is enabled (unless a SPDY connection was created before property from prefs is read. That is cached spdy connections). Tested the code, if spdy is disabled in policy file, then we don't use SPDY sessions.
http://codereview.chromium.org/10911198/diff/12001/chrome/browser/net/net_pre... File chrome/browser/net/net_pref_observer.cc (right): http://codereview.chromium.org/10911198/diff/12001/chrome/browser/net/net_pre... chrome/browser/net/net_pref_observer.cc:51: net::HttpStreamFactory::set_spdy_enabled(!*spdy_disabled_); On 2012/09/12 01:51:52, ramant wrote: > On 2012/09/12 01:31:17, Matt Menke wrote: > > This removes the ability of modifying the policy value to re-enable SPDY, when > > it's disabled by a means other than the command line. > > Correct. My proposal is for the user to run without the command line option > (--use-spdy=off) for policy to take effect. > > As you had mentioned "So disabling takes precedence over enabling, in all > cases". > > > > Am I correct that this is already broken, anyways, since we never re-populate > > HttpStreamFactory::next_protos_, after deleting it when spdy is disabled? > > In http_stream_facttory_impl_job.cc, we don't set _using_spdy to true unless > spdy is enabled (unless a SPDY connection was created before property from prefs > is read. That is cached spdy connections). > > Tested the code, if spdy is disabled in policy file, then we don't use SPDY > sessions. > I was just concerned about the preference being disabled and then enabled. Doesn't look like that can happen, unless we reload the policy file if it changes, and looks like it wouldn't actually enable spdy again anyways (Since spdy_enabled_ would be true, but next_protos_ would be still be NULL).
Oh, and LGTM.
On 2012/09/12 02:26:09, Matt Menke wrote: > Oh, and LGTM. Thanks very much mmenke.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rtenneti@chromium.org/10911198/12001
Presubmit check for 10911198-12001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit ERRORS ** Missing LGTM from an OWNER for files in these directories: chrome/browser/policy Was the presubmit check useful? Please send feedback & hate mail to maruel@chromium.org!
joaodasilva@: can I get OWNERs rubber stamp for policy_browsertest.cc changes? thanks much.
This is not the way that the command line interacts with policies; usually policies always override the command line. The right way to fix this is to add the command line flag to the CommandLinePrefStore. See comment #17 on the bug for more details. Please don't land this as is. It should be fairly simple to add the spdy flag to the CommandLinePrefStore and get the right result. I'd happily review that :-) Thanks!
Hi Matt Menke, Joao da Silva, Made changes per Joao da Silva suggestion. Could you take a look at this change? thanks raman
I initially thought --use-spdy was an on/off flag but I've now realized it can configure other aspects of the protocol. The right thing to do is to completely ignore that flag if the spdy policy is in place (either on or off). Here's what can be done: in chrome_browser_main.cc, pass browser_process_->policy_service() to InitializeNetworkOptions. At InitializeNetworkOptions(), skip the spdy command line flags if policy_service->GetPolicies(policy::POLICY_DOMAIN_CHROME, std::string()).Get(policy::key::kDisableSpdy) is not NULL. This test can be placed in a new function named HasSpdyPolicy() to make it more clear what it means. The only flag from InitializeNetworkOptions() that shouldn't depend on this is kEnableFileCookies. The rationale for this is that --use-spdy is a flag for tests but can be used by users to disable spdy in some cases, and bypass the administrator's configuration. That should never happen. About the initial issue: The problem is that InitializeNetworkOptions() disables spdy, but after the profile is created the NetPrefObserver enables it again. This means that currently --use-spdy=off doesn't really work. Since prefs::kDisableSpdy is only set by policies, a simple fix is to add if (spdy_disabled_->IsManaged()) net::HttpStreamFactory::set_spdy_enabled(!*spdy_disabled_); at NetPrefObserver::ApplySettings().
lgtm. Please update the issue description before landing.
On 2012/09/14 08:52:40, Joao da Silva wrote: > lgtm. Please update the issue description before landing. This still doesn't make the group policy override the command line flags, does it? spdy_enabled_ will be true, but next_proto_ will still be NULL.
On 2012/09/14 13:12:47, Matt Menke wrote: > On 2012/09/14 08:52:40, Joao da Silva wrote: > > lgtm. Please update the issue description before landing. > > This still doesn't make the group policy override the command line flags, does > it? spdy_enabled_ will be true, but next_proto_ will still be NULL. Err...spdy_disabled_ will be false, rather (With the command line flag to disable it, but pref to enable it)
On Fri, Sep 14, 2012 at 3:14 PM, <mmenke@chromium.org> wrote: > On 2012/09/14 13:12:47, Matt Menke wrote: > >> On 2012/09/14 08:52:40, Joao da Silva wrote: >> > lgtm. Please update the issue description before landing. >> > > This still doesn't make the group policy override the command line flags, >> does >> it? spdy_enabled_ will be true, but next_proto_ will still be NULL. >> > > Err...spdy_disabled_ will be false, rather (With the command line flag to > disable it, but pref to enable it) > Yes, that's the 2nd issue and can be solved in another CL. The issue described in the bug is solved by this patch. If both are solved in one CL I'm cool with it, and I'm also cool with solving each problem in it's own CL :-) > > https://chromiumcodereview.**appspot.com/10911198/<https://chromiumcodereview... >
On 2012/09/14 13:18:22, Joao da Silva wrote: > On Fri, Sep 14, 2012 at 3:14 PM, <mailto:mmenke@chromium.org> wrote: > > > On 2012/09/14 13:12:47, Matt Menke wrote: > > > >> On 2012/09/14 08:52:40, Joao da Silva wrote: > >> > lgtm. Please update the issue description before landing. > >> > > > > This still doesn't make the group policy override the command line flags, > >> does > >> it? spdy_enabled_ will be true, but next_proto_ will still be NULL. > >> > > > > Err...spdy_disabled_ will be false, rather (With the command line flag to > > disable it, but pref to enable it) > > > > Yes, that's the 2nd issue and can be solved in another CL. The issue > described in the bug is solved by this patch. If both are solved in one CL > I'm cool with it, and I'm also cool with solving each problem in it's own > CL :-) Ok, then. LGTM.
On 2012/09/14 14:06:09, Matt Menke wrote: > On 2012/09/14 13:18:22, Joao da Silva wrote: > > On Fri, Sep 14, 2012 at 3:14 PM, <mailto:mmenke@chromium.org> wrote: > > > > > On 2012/09/14 13:12:47, Matt Menke wrote: > > > > > >> On 2012/09/14 08:52:40, Joao da Silva wrote: > > >> > lgtm. Please update the issue description before landing. > > >> > > > > > > This still doesn't make the group policy override the command line flags, > > >> does > > >> it? spdy_enabled_ will be true, but next_proto_ will still be NULL. > > >> > > > > > > Err...spdy_disabled_ will be false, rather (With the command line flag to > > > disable it, but pref to enable it) > > > > > > > Yes, that's the 2nd issue and can be solved in another CL. The issue > > described in the bug is solved by this patch. If both are solved in one CL > > I'm cool with it, and I'm also cool with solving each problem in it's own > > CL :-) > > Ok, then. LGTM. I have another change in which chrome_browser_main.cc includes policy_constants and doesn't call http_network_layer code if policy defines SpdyEnabled. It changes dependencies slightly. In my ninja builds on windows, policy code wasn't built (didn't have out/Debug/gen/policy/policy/policy_constants.*). Once I ran policy target explicitly, I have policy_constants.*. Working on verifying that this change doesn't break ninja builds). As you both have approved this CL, will submit changes to chrome_browser_main.cc in a separate CL. thanks much for your patience.
On Fri, Sep 14, 2012 at 5:59 PM, <rtenneti@chromium.org> wrote: > On 2012/09/14 14:06:09, Matt Menke wrote: > >> On 2012/09/14 13:18:22, Joao da Silva wrote: >> > On Fri, Sep 14, 2012 at 3:14 PM, <mailto:mmenke@chromium.org> wrote: >> > >> > > On 2012/09/14 13:12:47, Matt Menke wrote: >> > > >> > >> On 2012/09/14 08:52:40, Joao da Silva wrote: >> > >> > lgtm. Please update the issue description before landing. >> > >> >> > > >> > > This still doesn't make the group policy override the command line >> flags, >> > >> does >> > >> it? spdy_enabled_ will be true, but next_proto_ will still be NULL. >> > >> >> > > >> > > Err...spdy_disabled_ will be false, rather (With the command line >> flag to >> > > disable it, but pref to enable it) >> > > >> > >> > Yes, that's the 2nd issue and can be solved in another CL. The issue >> > described in the bug is solved by this patch. If both are solved in one >> CL >> > I'm cool with it, and I'm also cool with solving each problem in it's >> own >> > CL :-) >> > > Ok, then. LGTM. >> > > I have another change in which chrome_browser_main.cc includes > policy_constants > and doesn't call http_network_layer code if policy defines SpdyEnabled. It > changes dependencies slightly. In my ninja builds on windows, policy code > wasn't > built (didn't have out/Debug/gen/policy/policy/**policy_constants.*). > Once I ran > policy target explicitly, I have policy_constants.*. > > Working on verifying that this change doesn't break ninja builds). > It shouldn't. The browser target (and many others) depend on the policy target already, which is always built on every desktop platform. Only android and the linux_redux bots have policy disabled; to avoid breaking those make sure the policy-specific parts are wrapped in #if defined(ENABLE_CONFIGURATION_POLICY) ... #endif > > As you both have approved this CL, will submit changes to > chrome_browser_main.cc > in a separate CL. > > thanks much for your patience. > > https://chromiumcodereview.**appspot.com/10911198/<https://chromiumcodereview... >
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rtenneti@chromium.org/10911198/12005
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rtenneti@chromium.org/10911198/12005
Change committed as 156863 |