Created policy QuicAllowed

Added QuicAllowed policy. Added unit and browser tests.

BUG=461739
Committed: https://crrev.com/5df83d489109f9df482abe00177e120513b4c3fa
Cr-Commit-Position: refs/heads/master@{#327723}

[peletskyi, 2015-03-12 16:48:01]

peletskyi@chromium.org changed reviewers:
+ atwilson@chromium.org

[Andrew T Wilson (Slow), 2015-03-12 17:23:34]

https://codereview.chromium.org/998383002/diff/1/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/998383002/diff/1/chrome/browser/io_thread.cc#...
chrome/browser/io_thread.cc:509: const base::Value* v = 
policy_service->GetPolicies(
remove extra space after =

https://codereview.chromium.org/998383002/diff/1/chrome/browser/io_thread.cc#...
chrome/browser/io_thread.cc:509: const base::Value* v = 
policy_service->GetPolicies(
Can this policy be changed on the fly, or only on startup?

https://codereview.chromium.org/998383002/diff/1/chrome/browser/io_thread.cc#...
chrome/browser/io_thread.cc:511:
std::string())).GetValue(policy::key::kQuicEnabled);
nit: indent is off

https://codereview.chromium.org/998383002/diff/1/chrome/browser/io_thread.cc#...
chrome/browser/io_thread.cc:1253: bool quic_enabled_by_policy) {
So, I think this should be called "quic_disabled_by_policy" and you should have
all the logic as if the original policy is "DisableQuic" - just change the code
that sets quic_disabled_by_policy to correctly map from the policy value to
this.

Otherwise it's too confusing, because an unset policy means
quic_enabled_by_policy == true, which is icky.

https://codereview.chromium.org/998383002/diff/1/chrome/browser/net/net_pref_...
File chrome/browser/net/net_pref_observer.cc (right):

https://codereview.chromium.org/998383002/diff/1/chrome/browser/net/net_pref_...
chrome/browser/net/net_pref_observer.cc:51: prefs::kQuicEnabled,
How is this pref used? You are reading directly from the policy service, I
thought? Is this just for disabling the UI?

https://codereview.chromium.org/998383002/diff/1/chrome/browser/policy/policy...
File chrome/browser/policy/policy_network_browsertest.cc (right):

https://codereview.chromium.org/998383002/diff/1/chrome/browser/policy/policy...
chrome/browser/policy/policy_network_browsertest.cc:47:
EXPECT_FALSE(quic_enabled);
Good test, but would be good to test the reverse case (kQuicEnabled=true) also.

https://codereview.chromium.org/998383002/diff/1/components/policy/resources/...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/998383002/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:7291: If this policy is set to
false usage of QUIC protocol is not possible.''',
Maybe describe the default?

[peletskyi, 2015-03-13 12:00:40]

PTAL

https://codereview.chromium.org/998383002/diff/1/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/998383002/diff/1/chrome/browser/io_thread.cc#...
chrome/browser/io_thread.cc:509: const base::Value* v = 
policy_service->GetPolicies(
On 2015/03/12 17:23:33, Andrew T Wilson wrote:
> remove extra space after =

Done.

https://codereview.chromium.org/998383002/diff/1/chrome/browser/io_thread.cc#...
chrome/browser/io_thread.cc:509: const base::Value* v = 
policy_service->GetPolicies(
On 2015/03/12 17:23:33, Andrew T Wilson wrote:
> remove extra space after =

Done.

https://codereview.chromium.org/998383002/diff/1/chrome/browser/io_thread.cc#...
chrome/browser/io_thread.cc:509: const base::Value* v = 
policy_service->GetPolicies(
On 2015/03/12 17:23:34, Andrew T Wilson wrote:
> Can this policy be changed on the fly, or only on startup?

Only on startup

https://codereview.chromium.org/998383002/diff/1/chrome/browser/io_thread.cc#...
chrome/browser/io_thread.cc:511:
std::string())).GetValue(policy::key::kQuicEnabled);
On 2015/03/12 17:23:33, Andrew T Wilson wrote:
> nit: indent is off

Done.

https://codereview.chromium.org/998383002/diff/1/chrome/browser/io_thread.cc#...
chrome/browser/io_thread.cc:1253: bool quic_enabled_by_policy) {
On 2015/03/12 17:23:33, Andrew T Wilson wrote:
> So, I think this should be called "quic_disabled_by_policy" and you should
have
> all the logic as if the original policy is "DisableQuic" - just change the
code
> that sets quic_disabled_by_policy to correctly map from the policy value to
> this.
> 
> Otherwise it's too confusing, because an unset policy means
> quic_enabled_by_policy == true, which is icky.

According to new name this variable is called "quic_allowed_by_policy_". I think
it is OK.

https://codereview.chromium.org/998383002/diff/1/chrome/browser/net/net_pref_...
File chrome/browser/net/net_pref_observer.cc (right):

https://codereview.chromium.org/998383002/diff/1/chrome/browser/net/net_pref_...
chrome/browser/net/net_pref_observer.cc:51: prefs::kQuicEnabled,
On 2015/03/12 17:23:34, Andrew T Wilson wrote:
> How is this pref used? You are reading directly from the policy service, I
> thought? Is this just for disabling the UI?

No use. This Pref was removed.

https://codereview.chromium.org/998383002/diff/1/chrome/browser/policy/policy...
File chrome/browser/policy/policy_network_browsertest.cc (right):

https://codereview.chromium.org/998383002/diff/1/chrome/browser/policy/policy...
chrome/browser/policy/policy_network_browsertest.cc:47:
EXPECT_FALSE(quic_enabled);
On 2015/03/12 17:23:34, Andrew T Wilson wrote:
> Good test, but would be good to test the reverse case (kQuicEnabled=true)
also.

From my point of view this test is enough. We wish to guarantee that QUIC is
disabled. In case if policy set to true, it does not mean that QUIC is enabled
(especially after being renamed to QuicAllowed)

https://codereview.chromium.org/998383002/diff/1/components/policy/resources/...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/998383002/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:7291: If this policy is set to
false usage of QUIC protocol is not possible.''',
On 2015/03/12 17:23:34, Andrew T Wilson wrote:
> Maybe describe the default?

Done.

[Andrew T Wilson (Slow), 2015-03-13 14:00:51]

LGTM with fixing up comments. If it's possible to make the code dynamically pick
up a change to the policy, that would be best, but you can also land that in a
separate CL.

https://codereview.chromium.org/998383002/diff/1/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/998383002/diff/1/chrome/browser/io_thread.cc#...
chrome/browser/io_thread.cc:509: const base::Value* v = 
policy_service->GetPolicies(
On 2015/03/13 12:00:40, peletskyi wrote:
> On 2015/03/12 17:23:34, Andrew T Wilson wrote:
> > Can this policy be changed on the fly, or only on startup?
> 
> Only on startup

OK, ideally all new policies would allow changing this dynamically. Is this
feasible in this case?

https://codereview.chromium.org/998383002/diff/80001/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/998383002/diff/80001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:1269: quic_allowed_by_policy) || 
ShouldEnableQuicForDataReductionProxy();
I don't think this is the right indent per
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

You need to indent to the (, not 4 characters.

https://codereview.chromium.org/998383002/diff/80001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/998383002/diff/80001/chrome/browser/io_thread...
chrome/browser/io_thread.h:481: bool is_quic_allowed_by_policy_;
name of this variable doesn't match the comment. can we change this to
"quic_disabled_by_policy_" also?

https://codereview.chromium.org/998383002/diff/80001/chrome/browser/policy/po...
File chrome/browser/policy/policy_network_browsertest.cc (right):

https://codereview.chromium.org/998383002/diff/80001/chrome/browser/policy/po...
chrome/browser/policy/policy_network_browsertest.cc:4: #include
"base/command_line.h"
blank line before #include

https://codereview.chromium.org/998383002/diff/80001/chrome/test/data/policy/...
File chrome/test/data/policy/policy_test_cases.json (right):

https://codereview.chromium.org/998383002/diff/80001/chrome/test/data/policy/...
chrome/test/data/policy/policy_test_cases.json:151: { "pref": "quic.enabled" }
Does this still have a pref mapping?

https://codereview.chromium.org/998383002/diff/80001/components/policy/resour...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/998383002/diff/80001/components/policy/resour...
components/policy/resources/policy_templates.json:7281: 'supported_on':
['chrome.*:43-'],
It is supported on chrome_os also.

[peletskyi, 2015-03-18 15:42:28]

peletskyi@chromium.org changed reviewers:
+ asvitkine@chromium.org, rsleevi@chromium.org

[peletskyi, 2015-03-18 15:42:30]

Hi all,
please review my changes.

https://codereview.chromium.org/998383002/diff/1/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/998383002/diff/1/chrome/browser/io_thread.cc#...
chrome/browser/io_thread.cc:509: const base::Value* v = 
policy_service->GetPolicies(
On 2015/03/13 14:00:51, Andrew T Wilson wrote:
> On 2015/03/13 12:00:40, peletskyi wrote:
> > On 2015/03/12 17:23:34, Andrew T Wilson wrote:
> > > Can this policy be changed on the fly, or only on startup?
> > 
> > Only on startup
> 
> OK, ideally all new policies would allow changing this dynamically. Is this
> feasible in this case?

It will be rather complicated. Currently I don't know any use case where QUIC is
enabled or disabled on the fly.

https://codereview.chromium.org/998383002/diff/80001/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/998383002/diff/80001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:1269: quic_allowed_by_policy) || 
ShouldEnableQuicForDataReductionProxy();
On 2015/03/13 14:00:51, Andrew T Wilson wrote:
> I don't think this is the right indent per
>
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...
> 
> You need to indent to the (, not 4 characters.

Done.

https://codereview.chromium.org/998383002/diff/80001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/998383002/diff/80001/chrome/browser/io_thread...
chrome/browser/io_thread.h:481: bool is_quic_allowed_by_policy_;
On 2015/03/13 14:00:51, Andrew T Wilson wrote:
> name of this variable doesn't match the comment. can we change this to
> "quic_disabled_by_policy_" also?

I've fixed comments. I think I would be better if this variable and policy will
have similar name and thus the same meaning.

https://codereview.chromium.org/998383002/diff/80001/chrome/browser/policy/po...
File chrome/browser/policy/policy_network_browsertest.cc (right):

https://codereview.chromium.org/998383002/diff/80001/chrome/browser/policy/po...
chrome/browser/policy/policy_network_browsertest.cc:4: #include
"base/command_line.h"
On 2015/03/13 14:00:51, Andrew T Wilson wrote:
> blank line before #include

Done.

https://codereview.chromium.org/998383002/diff/80001/chrome/test/data/policy/...
File chrome/test/data/policy/policy_test_cases.json (right):

https://codereview.chromium.org/998383002/diff/80001/chrome/test/data/policy/...
chrome/test/data/policy/policy_test_cases.json:151: { "pref": "quic.enabled" }
On 2015/03/13 14:00:51, Andrew T Wilson wrote:
> Does this still have a pref mapping?

Done.

https://codereview.chromium.org/998383002/diff/80001/components/policy/resour...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/998383002/diff/80001/components/policy/resour...
components/policy/resources/policy_templates.json:7281: 'supported_on':
['chrome.*:43-'],
On 2015/03/13 14:00:51, Andrew T Wilson wrote:
> It is supported on chrome_os also.

Done.

[Ryan Sleevi, 2015-03-18 15:51:25]

rsleevi@chromium.org changed reviewers:
+ rch@chromium.org
- rsleevi@chromium.org

[Ryan Hamilton, 2015-03-18 18:42:54]

A few comments on the io_thread changes.

Thanks for doing this!

https://codereview.chromium.org/998383002/diff/120001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/998383002/diff/120001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1241: globals_);
nit: this wrapping looks funny to me. I think the second line should be lined up
just after the ( from the previous line. Unless you wrap all the arguments in
which case it can start here.

https://codereview.chromium.org/998383002/diff/120001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1252: quic_allowed_by_policy);
ditto about wrapping.

https://codereview.chromium.org/998383002/diff/120001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1255: quic_trial_group, quic_allowed_by_policy);
ditto about wrapping.

https://codereview.chromium.org/998383002/diff/120001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/998383002/diff/120001/chrome/browser/io_threa...
chrome/browser/io_thread.h:335: 
Can you add tests for this new logic in io_thread_unittest.cc?

https://codereview.chromium.org/998383002/diff/120001/chrome/browser/io_threa...
chrome/browser/io_thread.h:480: bool is_quic_allowed_by_policy_;
heh, it's unfortunate that the polarity of this policy is reverse from SPDY. How
hard would it be to make the polarity match?

[peletskyi, 2015-03-20 17:54:47]

PTAL

https://codereview.chromium.org/998383002/diff/120001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/998383002/diff/120001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1241: globals_);
On 2015/03/18 18:42:54, Ryan Hamilton wrote:
> nit: this wrapping looks funny to me. I think the second line should be lined
up
> just after the ( from the previous line. Unless you wrap all the arguments in
> which case it can start here.

Done.

https://codereview.chromium.org/998383002/diff/120001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1252: quic_allowed_by_policy);
On 2015/03/18 18:42:54, Ryan Hamilton wrote:
> ditto about wrapping.

Done.

https://codereview.chromium.org/998383002/diff/120001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1255: quic_trial_group, quic_allowed_by_policy);
On 2015/03/18 18:42:54, Ryan Hamilton wrote:
> ditto about wrapping.

Done.

https://codereview.chromium.org/998383002/diff/120001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/998383002/diff/120001/chrome/browser/io_threa...
chrome/browser/io_thread.h:335: 
On 2015/03/18 18:42:54, Ryan Hamilton wrote:
> Can you add tests for this new logic in io_thread_unittest.cc?

Done.

https://codereview.chromium.org/998383002/diff/120001/chrome/browser/io_threa...
chrome/browser/io_thread.h:480: bool is_quic_allowed_by_policy_;
On 2015/03/18 18:42:54, Ryan Hamilton wrote:
> heh, it's unfortunate that the polarity of this policy is reverse from SPDY.
How
> hard would it be to make the polarity match?

Unfortunately one is not allowed not give name "QuicDisabled" to the policy. The
best name that matches the goals of this policy we could came up with is
"QuicAllowed". And this variable just reflects the value and the meaning of the
policy.

[Ryan Hamilton, 2015-03-20 18:21:58]

Modulo one formatting issue, the io_thread changes LGTM.

https://codereview.chromium.org/998383002/diff/160001/chrome/browser/io_threa...
File chrome/browser/io_thread_unittest.cc (right):

https://codereview.chromium.org/998383002/diff/160001/chrome/browser/io_threa...
chrome/browser/io_thread_unittest.cc:57: is_quic_allowed_by_policy_(true) {
Can you wrap the : command_line_(...) onto the next line, and line up
"is_quic.." with "command_line" (not with the ":", but with the "c")

[peletskyi, 2015-03-23 09:39:20]

The CQ bit was checked by peletskyi@chromium.org

[peletskyi, 2015-03-23 09:39:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
atwilson@chromium.org, rch@chromium.org
Link to the patchset: https://codereview.chromium.org/998383002/#ps180001
(title: "Formatting")

[commit-bot: I haz the power, 2015-03-23 09:39:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/998383002/180001

[commit-bot: I haz the power, 2015-03-23 09:47:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-23 09:47:41]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[peletskyi, 2015-03-23 10:03:17]

peletskyi@chromium.org changed reviewers:
+ isherman@chromium.org

[peletskyi, 2015-03-23 10:03:18]

Ilya can you take a look at these changes?

https://codereview.chromium.org/998383002/diff/160001/chrome/browser/io_threa...
File chrome/browser/io_thread_unittest.cc (right):

https://codereview.chromium.org/998383002/diff/160001/chrome/browser/io_threa...
chrome/browser/io_thread_unittest.cc:57: is_quic_allowed_by_policy_(true) {
On 2015/03/20 18:21:58, Ryan Hamilton wrote:
> Can you wrap the : command_line_(...) onto the next line, and line up
> "is_quic.." with "command_line" (not with the ":", but with the "c")

Done.

[Ilya Sherman, 2015-03-23 21:55:56]

histograms.xml lgtm

[peletskyi, 2015-03-24 12:48:05]

The CQ bit was checked by peletskyi@chromium.org

[peletskyi, 2015-03-24 12:48:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, rch@chromium.org, atwilson@chromium.org
Link to the patchset: https://codereview.chromium.org/998383002/#ps200001
(title: "Conflict resolution")

[commit-bot: I haz the power, 2015-03-24 12:48:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/998383002/200001

[commit-bot: I haz the power, 2015-03-24 13:57:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-24 13:57:20]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Alexei Svitkine (slow), 2015-03-30 15:08:41]

asvitkine@chromium.org changed reviewers:
- asvitkine@chromium.org

[peletskyi, 2015-04-07 17:02:14]

Drew, can you take a look at the modified
chrome/browser/policy/policy_network_browsertest.cc ?

[peletskyi, 2015-04-09 08:43:47]

Drew, frendly ping about chrome/browser/policy/policy_network_browsertest.cc

[Andrew T Wilson (Slow), 2015-04-09 09:02:04]

Mostly LG, but i think the test can be a bit more extensive with a few small
changes.

https://codereview.chromium.org/998383002/diff/260001/chrome/browser/policy/p...
File chrome/browser/policy/policy_network_browsertest.cc (right):

https://codereview.chromium.org/998383002/diff/260001/chrome/browser/policy/p...
chrome/browser/policy/policy_network_browsertest.cc:23: void
getContext(net::URLRequestContextGetter* getter,
nit: getContext->GetContext

https://codereview.chromium.org/998383002/diff/260001/chrome/browser/policy/p...
chrome/browser/policy/policy_network_browsertest.cc:24: const base::Closure&
done_callback) {
Should be indented to match the open paren above.

https://codereview.chromium.org/998383002/diff/260001/chrome/browser/policy/p...
chrome/browser/policy/policy_network_browsertest.cc:35: 
Should probably put this stuff in namespace policy since it lives in
browser/policy. Then you don't have to use policy:: below.

https://codereview.chromium.org/998383002/diff/260001/chrome/browser/policy/p...
chrome/browser/policy/policy_network_browsertest.cc:37: public:
indent this

https://codereview.chromium.org/998383002/diff/260001/chrome/browser/policy/p...
chrome/browser/policy/policy_network_browsertest.cc:39: protected:
blank line before protected:

https://codereview.chromium.org/998383002/diff/260001/chrome/browser/policy/p...
chrome/browser/policy/policy_network_browsertest.cc:67: base::Bind(getContext,
Maybe rename getContext to VerifyQuicEnabledStatus()?

If you pass in the expected value of quick_enabled, then you can easily write 3
tests: to check the behavior when no policy is set, when policy == true, and
when policy == false. You should probably do that.

[peletskyi, 2015-04-09 09:43:33]

PTAL

https://codereview.chromium.org/998383002/diff/260001/chrome/browser/policy/p...
File chrome/browser/policy/policy_network_browsertest.cc (right):

https://codereview.chromium.org/998383002/diff/260001/chrome/browser/policy/p...
chrome/browser/policy/policy_network_browsertest.cc:23: void
getContext(net::URLRequestContextGetter* getter,
On 2015/04/09 09:02:04, Andrew T Wilson wrote:
> nit: getContext->GetContext

Done.

https://codereview.chromium.org/998383002/diff/260001/chrome/browser/policy/p...
chrome/browser/policy/policy_network_browsertest.cc:24: const base::Closure&
done_callback) {
On 2015/04/09 09:02:04, Andrew T Wilson wrote:
> Should be indented to match the open paren above.

Done.

https://codereview.chromium.org/998383002/diff/260001/chrome/browser/policy/p...
chrome/browser/policy/policy_network_browsertest.cc:35: 
On 2015/04/09 09:02:04, Andrew T Wilson wrote:
> Should probably put this stuff in namespace policy since it lives in
> browser/policy. Then you don't have to use policy:: below.

Done.

https://codereview.chromium.org/998383002/diff/260001/chrome/browser/policy/p...
chrome/browser/policy/policy_network_browsertest.cc:37: public:
On 2015/04/09 09:02:04, Andrew T Wilson wrote:
> indent this

Done.

https://codereview.chromium.org/998383002/diff/260001/chrome/browser/policy/p...
chrome/browser/policy/policy_network_browsertest.cc:39: protected:
On 2015/04/09 09:02:04, Andrew T Wilson wrote:
> blank line before protected:

Done.

https://codereview.chromium.org/998383002/diff/260001/chrome/browser/policy/p...
chrome/browser/policy/policy_network_browsertest.cc:67: base::Bind(getContext,
On 2015/04/09 09:02:04, Andrew T Wilson wrote:
> Maybe rename getContext to VerifyQuicEnabledStatus()?
> 
> If you pass in the expected value of quick_enabled, then you can easily write
3
> tests: to check the behavior when no policy is set, when policy == true, and
> when policy == false. You should probably do that.

Rename - done.
About three tests. I think it is better to leave it as it is now. For the cases
QuicAllowed == true and QuicAllowed is not set we do not guarantee that Quic is
enabled. We just care when QuicAllowed == false, QUIC is disabled. The rest
cases are out of the scope.

[Andrew T Wilson (Slow), 2015-04-09 12:13:15]

https://codereview.chromium.org/998383002/diff/260001/chrome/browser/policy/p...
File chrome/browser/policy/policy_network_browsertest.cc (right):

https://codereview.chromium.org/998383002/diff/260001/chrome/browser/policy/p...
chrome/browser/policy/policy_network_browsertest.cc:67: base::Bind(getContext,
On 2015/04/09 09:43:33, peletskyi wrote:
> On 2015/04/09 09:02:04, Andrew T Wilson wrote:
> > Maybe rename getContext to VerifyQuicEnabledStatus()?
> > 
> > If you pass in the expected value of quick_enabled, then you can easily
write
> 3
> > tests: to check the behavior when no policy is set, when policy == true, and
> > when policy == false. You should probably do that.
> 
> Rename - done.
> About three tests. I think it is better to leave it as it is now. For the
cases
> QuicAllowed == true and QuicAllowed is not set we do not guarantee that Quic
is
> enabled. We just care when QuicAllowed == false, QUIC is disabled. The rest
> cases are out of the scope.

OK, let's imagine someone goes into io_thread.cc line 600-601 and makes this
change:

if (value)
  is_quic_allowed_by_policy_ = false;

So whenever this policy is set at all, quic is disabled. Would your test catch
this bug? Let's imagine that someone breaks the enable-quic flag or changes its
semantics so in your test quic is disabled by default - would your test catch
this?

I would disagree that testing these cases are out of scope, because the
documentation you have added to policy_templates.json explicitly says:

If this policy is set to true or not set usage of QUIC protocol in <ph
name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> is allowed.

But you don't verify that this behavior is actually happening via a test.

[peletskyi, 2015-04-10 08:26:34]

PTAL

https://codereview.chromium.org/998383002/diff/260001/chrome/browser/policy/p...
File chrome/browser/policy/policy_network_browsertest.cc (right):

https://codereview.chromium.org/998383002/diff/260001/chrome/browser/policy/p...
chrome/browser/policy/policy_network_browsertest.cc:67: base::Bind(getContext,
On 2015/04/09 12:13:15, Andrew T Wilson wrote:
> On 2015/04/09 09:43:33, peletskyi wrote:
> > On 2015/04/09 09:02:04, Andrew T Wilson wrote:
> > > Maybe rename getContext to VerifyQuicEnabledStatus()?
> > > 
> > > If you pass in the expected value of quick_enabled, then you can easily
> write
> > 3
> > > tests: to check the behavior when no policy is set, when policy == true,
and
> > > when policy == false. You should probably do that.
> > 
> > Rename - done.
> > About three tests. I think it is better to leave it as it is now. For the
> cases
> > QuicAllowed == true and QuicAllowed is not set we do not guarantee that Quic
> is
> > enabled. We just care when QuicAllowed == false, QUIC is disabled. The rest
> > cases are out of the scope.
> 
> OK, let's imagine someone goes into io_thread.cc line 600-601 and makes this
> change:
> 
> if (value)
>   is_quic_allowed_by_policy_ = false;
> 
> So whenever this policy is set at all, quic is disabled. Would your test catch
> this bug? Let's imagine that someone breaks the enable-quic flag or changes
its
> semantics so in your test quic is disabled by default - would your test catch
> this?
> 
> I would disagree that testing these cases are out of scope, because the
> documentation you have added to policy_templates.json explicitly says:
> 
> If this policy is set to true or not set usage of QUIC protocol in <ph
> name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> is allowed.
> 
> But you don't verify that this behavior is actually happening via a test.
> 
> 

Acknowledged.

[Andrew T Wilson (Slow), 2015-04-14 17:48:47]

https://codereview.chromium.org/998383002/diff/340001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/998383002/diff/340001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:575:
std::string())).GetValue(policy::key::kQuicAllowed);
This line is mis-indented (should be indented another 4 spaces)

https://codereview.chromium.org/998383002/diff/340001/chrome/browser/policy/p...
File chrome/browser/policy/policy_network_browsertest.cc (right):

https://codereview.chromium.org/998383002/diff/340001/chrome/browser/policy/p...
chrome/browser/policy/policy_network_browsertest.cc:29: EXPECT_EQ(quic_enabled,
quic_enabled_expected);
nit: this is the wrong order, order should be:

EXPECT_EQ(expected_value, actual_value)

https://codereview.chromium.org/998383002/diff/340001/chrome/browser/policy/p...
chrome/browser/policy/policy_network_browsertest.cc:32: done_callback);
indent to opening paren

https://codereview.chromium.org/998383002/diff/340001/chrome/browser/policy/p...
chrome/browser/policy/policy_network_browsertest.cc:39: // The tests are based
on the assumption that command line flag "enable-quic"
Maybe move this comment down to where you are appending that switch.

Also, since this switch is defined in chrome_switches.cc (kEnableQuic), you
should use that constant rather than using an ad-hoc string "enable-quic" - that
way if they remove that switch, they'll get a compilation error for this test.

https://codereview.chromium.org/998383002/diff/340001/chrome/browser/policy/p...
chrome/browser/policy/policy_network_browsertest.cc:58: virtual void
GetQuicAllowedPolicy(PolicyMap* values) = 0;
Why is this necessary? You can change the policy on the fly and it should pick
up the new value, right? So why not change the policy in each individual test to
the value you want, then test that quic is enabled/disabled appropriately?

[peletskyi, 2015-04-27 11:05:49]

peletskyi@chromium.org changed reviewers:
- isherman@chromium.org, rch@chromium.org

[peletskyi, 2015-04-27 11:05:50]

PTAL

https://codereview.chromium.org/998383002/diff/340001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/998383002/diff/340001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:575:
std::string())).GetValue(policy::key::kQuicAllowed);
On 2015/04/14 17:48:47, Andrew T Wilson wrote:
> This line is mis-indented (should be indented another 4 spaces)

Done.

https://codereview.chromium.org/998383002/diff/340001/chrome/browser/policy/p...
File chrome/browser/policy/policy_network_browsertest.cc (right):

https://codereview.chromium.org/998383002/diff/340001/chrome/browser/policy/p...
chrome/browser/policy/policy_network_browsertest.cc:29: EXPECT_EQ(quic_enabled,
quic_enabled_expected);
On 2015/04/14 17:48:47, Andrew T Wilson wrote:
> nit: this is the wrong order, order should be:
> 
> EXPECT_EQ(expected_value, actual_value)

Done.

https://codereview.chromium.org/998383002/diff/340001/chrome/browser/policy/p...
chrome/browser/policy/policy_network_browsertest.cc:32: done_callback);
On 2015/04/14 17:48:47, Andrew T Wilson wrote:
> indent to opening paren

Done.

https://codereview.chromium.org/998383002/diff/340001/chrome/browser/policy/p...
chrome/browser/policy/policy_network_browsertest.cc:39: // The tests are based
on the assumption that command line flag "enable-quic"
On 2015/04/14 17:48:47, Andrew T Wilson wrote:
> Maybe move this comment down to where you are appending that switch.
> 
> Also, since this switch is defined in chrome_switches.cc (kEnableQuic), you
> should use that constant rather than using an ad-hoc string "enable-quic" -
that
> way if they remove that switch, they'll get a compilation error for this test.
Insert kEnableQuic - done.
Move the comment: i think it is the best place for it because all the tests rely
on the assumption and command line flag is set just below.

https://codereview.chromium.org/998383002/diff/340001/chrome/browser/policy/p...
chrome/browser/policy/policy_network_browsertest.cc:58: virtual void
GetQuicAllowedPolicy(PolicyMap* values) = 0;
On 2015/04/14 17:48:47, Andrew T Wilson wrote:
> Why is this necessary? You can change the policy on the fly and it should pick
> up the new value, right? So why not change the policy in each individual test
to
> the value you want, then test that quic is enabled/disabled appropriately?

Unfortunately the policy value should be set on start.

[Andrew T Wilson (Slow), 2015-04-30 15:04:15]

LGTM with one comment.

https://codereview.chromium.org/998383002/diff/380001/chrome/browser/io_threa...
File chrome/browser/io_thread_unittest.cc (right):

https://codereview.chromium.org/998383002/diff/380001/chrome/browser/io_threa...
chrome/browser/io_thread_unittest.cc:465:
command_line_.AppendSwitch("enable-quic");
use kEnableQuic here?

[peletskyi, 2015-04-30 15:27:37]

The CQ bit was checked by peletskyi@chromium.org

[peletskyi, 2015-04-30 15:27:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, rch@chromium.org, atwilson@chromium.org
Link to the patchset: https://codereview.chromium.org/998383002/#ps400001
(title: " ")

[commit-bot: I haz the power, 2015-04-30 15:28:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/998383002/400001

[commit-bot: I haz the power, 2015-04-30 16:37:59]

Committed patchset #21 (id:400001)

[commit-bot: I haz the power, 2015-04-30 16:39:02]

Patchset 21 (id:??) landed as
https://crrev.com/5df83d489109f9df482abe00177e120513b4c3fa
Cr-Commit-Position: refs/heads/master@{#327723}