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

Issue 998383002: Created policy QuicAllowed (Closed)

Created:
5 years, 9 months ago by peletskyi
Modified:
5 years, 7 months ago
CC:
chromium-reviews, cbentzel+watch_chromium.org, asvitkine+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Added QuicAllowed policy. Added unit and browser tests. BUG=461739 Committed: https://crrev.com/5df83d489109f9df482abe00177e120513b4c3fa Cr-Commit-Position: refs/heads/master@{#327723}

Patch Set 1 #

Total comments: 17

Patch Set 2 : Policy renamed to QuicAllowed #

Patch Set 3 : Removed prefs #

Patch Set 4 : #

Patch Set 5 : #

Total comments: 10

Patch Set 6 : Fixed Drew's remarks. #

Patch Set 7 : #

Total comments: 10

Patch Set 8 : Unit test added + Format issues solved #

Patch Set 9 : #

Total comments: 2

Patch Set 10 : Formatting #

Patch Set 11 : Conflict resolution #

Patch Set 12 : Fixed problem with the test in debug mode and rebase #

Patch Set 13 : Merge #

Patch Set 14 : #

Total comments: 14

Patch Set 15 : #

Patch Set 16 : #

Patch Set 17 : Added additional tests #

Patch Set 18 : Rebase #

Total comments: 10

Patch Set 19 : Rebase and nits fix #

Patch Set 20 : Made browser tests codebase better #

Total comments: 1

Patch Set 21 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+210 lines, -15 lines) Patch
M chrome/browser/io_thread.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +8 lines, -2 lines 0 comments Download
M chrome/browser/io_thread.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 6 chunks +21 lines, -8 lines 0 comments Download
M chrome/browser/io_thread_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 5 chunks +23 lines, -4 lines 0 comments Download
A chrome/browser/policy/policy_network_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +134 lines, -0 lines 0 comments Download
M chrome/chrome_tests.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +1 line, -0 lines 0 comments Download
M chrome/test/data/policy/policy_test_cases.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +5 lines, -0 lines 0 comments Download
M components/policy/resources/policy_templates.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 2 chunks +17 lines, -1 line 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 39 (14 generated)
Andrew T Wilson (Slow)
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#newcode509 chrome/browser/io_thread.cc:509: const base::Value* v = policy_service->GetPolicies( remove extra space after ...
5 years, 9 months ago (2015-03-12 17:23:34 UTC) #2
peletskyi
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#newcode509 chrome/browser/io_thread.cc:509: const base::Value* v = policy_service->GetPolicies( On 2015/03/12 17:23:33, ...
5 years, 9 months ago (2015-03-13 12:00:40 UTC) #3
Andrew T Wilson (Slow)
LGTM with fixing up comments. If it's possible to make the code dynamically pick up ...
5 years, 9 months ago (2015-03-13 14:00:51 UTC) #4
peletskyi
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#newcode509 chrome/browser/io_thread.cc:509: const base::Value* v ...
5 years, 9 months ago (2015-03-18 15:42:30 UTC) #6
Ryan Hamilton
A few comments on the io_thread changes. Thanks for doing this! https://codereview.chromium.org/998383002/diff/120001/chrome/browser/io_thread.cc File chrome/browser/io_thread.cc (right): ...
5 years, 9 months ago (2015-03-18 18:42:54 UTC) #8
peletskyi
PTAL https://codereview.chromium.org/998383002/diff/120001/chrome/browser/io_thread.cc File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/998383002/diff/120001/chrome/browser/io_thread.cc#newcode1241 chrome/browser/io_thread.cc:1241: globals_); On 2015/03/18 18:42:54, Ryan Hamilton wrote: > ...
5 years, 9 months ago (2015-03-20 17:54:47 UTC) #9
Ryan Hamilton
Modulo one formatting issue, the io_thread changes LGTM. https://codereview.chromium.org/998383002/diff/160001/chrome/browser/io_thread_unittest.cc File chrome/browser/io_thread_unittest.cc (right): https://codereview.chromium.org/998383002/diff/160001/chrome/browser/io_thread_unittest.cc#newcode57 chrome/browser/io_thread_unittest.cc:57: is_quic_allowed_by_policy_(true) ...
5 years, 9 months ago (2015-03-20 18:21:58 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/998383002/180001
5 years, 9 months ago (2015-03-23 09:39:50 UTC) #13
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/51268)
5 years, 9 months ago (2015-03-23 09:47:41 UTC) #15
peletskyi
Ilya can you take a look at these changes? https://codereview.chromium.org/998383002/diff/160001/chrome/browser/io_thread_unittest.cc File chrome/browser/io_thread_unittest.cc (right): https://codereview.chromium.org/998383002/diff/160001/chrome/browser/io_thread_unittest.cc#newcode57 chrome/browser/io_thread_unittest.cc:57: ...
5 years, 9 months ago (2015-03-23 10:03:18 UTC) #17
Ilya Sherman
histograms.xml lgtm
5 years, 9 months ago (2015-03-23 21:55:56 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/998383002/200001
5 years, 9 months ago (2015-03-24 12:48:13 UTC) #21
commit-bot: I haz the power
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_chromeos_rel_ng/builds/39099)
5 years, 9 months ago (2015-03-24 13:57:20 UTC) #23
peletskyi
Drew, can you take a look at the modified chrome/browser/policy/policy_network_browsertest.cc ?
5 years, 8 months ago (2015-04-07 17:02:14 UTC) #25
peletskyi
Drew, frendly ping about chrome/browser/policy/policy_network_browsertest.cc
5 years, 8 months ago (2015-04-09 08:43:47 UTC) #26
Andrew T Wilson (Slow)
Mostly LG, but i think the test can be a bit more extensive with a ...
5 years, 8 months ago (2015-04-09 09:02:04 UTC) #27
peletskyi
PTAL https://codereview.chromium.org/998383002/diff/260001/chrome/browser/policy/policy_network_browsertest.cc File chrome/browser/policy/policy_network_browsertest.cc (right): https://codereview.chromium.org/998383002/diff/260001/chrome/browser/policy/policy_network_browsertest.cc#newcode23 chrome/browser/policy/policy_network_browsertest.cc:23: void getContext(net::URLRequestContextGetter* getter, On 2015/04/09 09:02:04, Andrew T ...
5 years, 8 months ago (2015-04-09 09:43:33 UTC) #28
Andrew T Wilson (Slow)
https://codereview.chromium.org/998383002/diff/260001/chrome/browser/policy/policy_network_browsertest.cc File chrome/browser/policy/policy_network_browsertest.cc (right): https://codereview.chromium.org/998383002/diff/260001/chrome/browser/policy/policy_network_browsertest.cc#newcode67 chrome/browser/policy/policy_network_browsertest.cc:67: base::Bind(getContext, On 2015/04/09 09:43:33, peletskyi wrote: > On 2015/04/09 ...
5 years, 8 months ago (2015-04-09 12:13:15 UTC) #29
peletskyi
PTAL https://codereview.chromium.org/998383002/diff/260001/chrome/browser/policy/policy_network_browsertest.cc File chrome/browser/policy/policy_network_browsertest.cc (right): https://codereview.chromium.org/998383002/diff/260001/chrome/browser/policy/policy_network_browsertest.cc#newcode67 chrome/browser/policy/policy_network_browsertest.cc:67: base::Bind(getContext, On 2015/04/09 12:13:15, Andrew T Wilson wrote: ...
5 years, 8 months ago (2015-04-10 08:26:34 UTC) #30
Andrew T Wilson (Slow)
https://codereview.chromium.org/998383002/diff/340001/chrome/browser/io_thread.cc File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/998383002/diff/340001/chrome/browser/io_thread.cc#newcode575 chrome/browser/io_thread.cc:575: std::string())).GetValue(policy::key::kQuicAllowed); This line is mis-indented (should be indented another ...
5 years, 8 months ago (2015-04-14 17:48:47 UTC) #31
peletskyi
PTAL https://codereview.chromium.org/998383002/diff/340001/chrome/browser/io_thread.cc File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/998383002/diff/340001/chrome/browser/io_thread.cc#newcode575 chrome/browser/io_thread.cc:575: std::string())).GetValue(policy::key::kQuicAllowed); On 2015/04/14 17:48:47, Andrew T Wilson wrote: ...
5 years, 8 months ago (2015-04-27 11:05:50 UTC) #33
Andrew T Wilson (Slow)
LGTM with one comment. https://codereview.chromium.org/998383002/diff/380001/chrome/browser/io_thread_unittest.cc File chrome/browser/io_thread_unittest.cc (right): https://codereview.chromium.org/998383002/diff/380001/chrome/browser/io_thread_unittest.cc#newcode465 chrome/browser/io_thread_unittest.cc:465: command_line_.AppendSwitch("enable-quic"); use kEnableQuic here?
5 years, 7 months ago (2015-04-30 15:04:15 UTC) #34
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/998383002/400001
5 years, 7 months ago (2015-04-30 15:28:05 UTC) #37
commit-bot: I haz the power
Committed patchset #21 (id:400001)
5 years, 7 months ago (2015-04-30 16:37:59 UTC) #38
commit-bot: I haz the power
5 years, 7 months ago (2015-04-30 16:39:02 UTC) #39
Message was sent while issue was closed.
Patchset 21 (id:??) landed as
https://crrev.com/5df83d489109f9df482abe00177e120513b4c3fa
Cr-Commit-Position: refs/heads/master@{#327723}

Powered by Google App Engine
This is Rietveld 408576698