Componentize ssl_config_service_manager_pref.cc

Componentize ssl_config_service_manager_pref.cc

. Removed not needed notifications.
. Created new switches and prefs for ssl_config.
. Using SingleThreadTaskRunner over BrowserThread to remove content dependencies.

BUG=517014
TBR=jochen

Committed: https://crrev.com/2849ceeb85e97574807c6f8b45f44a854fc81e5e
Cr-Commit-Position: refs/heads/master@{#355038}

[Abhishek, 2015-09-04 13:46:28]

abhishek.a21@samsung.com changed reviewers:
+ droger@chromium.org, r.kasibhatla@samsung.com

[Abhishek, 2015-09-04 13:46:29]

PTAL!

[droger, 2015-09-04 13:54:35]

Looks good! Only minor comments.

https://codereview.chromium.org/1320533007/diff/1/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/1320533007/diff/1/chrome/browser/BUILD.gn#new...
chrome/browser/BUILD.gn:154: "//components/ssl_config",
This is not needed.
iOS does not actually build this target.

https://codereview.chromium.org/1320533007/diff/1/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1320533007/diff/1/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:3155: '../components/components.gyp:ssl_config',
Same here: remove.

https://codereview.chromium.org/1320533007/diff/1/components/ssl_config/ssl_c...
File components/ssl_config/ssl_config_service_manager.h (right):

https://codereview.chromium.org/1320533007/diff/1/components/ssl_config/ssl_c...
components/ssl_config/ssl_config_service_manager.h:20: 
Can you put this in the ssl_config namespace?

Add here:
namespace ssl_config {

[Abhishek, 2015-09-07 09:18:09]

I've updated the patch.
PTAL #2!

https://codereview.chromium.org/1320533007/diff/1/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/1320533007/diff/1/chrome/browser/BUILD.gn#new...
chrome/browser/BUILD.gn:154: "//components/ssl_config",
On 2015/09/04 13:54:35, droger wrote:
> This is not needed.
> iOS does not actually build this target.

Done.

https://codereview.chromium.org/1320533007/diff/1/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1320533007/diff/1/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:3155: '../components/components.gyp:ssl_config',
On 2015/09/04 13:54:35, droger wrote:
> Same here: remove.

Done.

https://codereview.chromium.org/1320533007/diff/1/components/ssl_config/ssl_c...
File components/ssl_config/ssl_config_service_manager.h (right):

https://codereview.chromium.org/1320533007/diff/1/components/ssl_config/ssl_c...
components/ssl_config/ssl_config_service_manager.h:20: 
On 2015/09/04 13:54:35, droger wrote:
> Can you put this in the ssl_config namespace?
> 
> Add here:
> namespace ssl_config {

Done.

[droger, 2015-09-07 09:53:31]

droger@chromium.org changed reviewers:
+ blundell@chromium.org, rsleevi@chromium.org

[droger, 2015-09-07 09:53:32]

LGTM. Thanks.

+rsleevi as OWNER for chrome/browser/net
+sdefresne for components

We should add a OWNER file in the component.
Looking at the history of the file, I would suggest something like:

agl
davidben
rsleevi

https://codereview.chromium.org/1320533007/diff/40001/components/ssl_config/s...
File components/ssl_config/ssl_config_prefs.h (right):

https://codereview.chromium.org/1320533007/diff/40001/components/ssl_config/s...
components/ssl_config/ssl_config_prefs.h:11: // Local state prefs. Please add
Profile prefs above instead.
Remove this comment.

[sdefresne, 2015-09-07 10:08:35]

sdefresne@chromium.org changed reviewers:
+ sdefresne@chromium.org

[sdefresne, 2015-09-07 10:08:36]

components lgtm

[droger, 2015-09-07 13:06:37]

https://codereview.chromium.org/1320533007/diff/40001/components/ssl_config.gypi
File components/ssl_config.gypi (right):

https://codereview.chromium.org/1320533007/diff/40001/components/ssl_config.g...
components/ssl_config.gypi:10: 'type': '<(component)',
Change this to:
'type': 'static_library',

https://codereview.chromium.org/1320533007/diff/40001/components/ssl_config/B...
File components/ssl_config/BUILD.gn (right):

https://codereview.chromium.org/1320533007/diff/40001/components/ssl_config/B...
components/ssl_config/BUILD.gn:6: component("ssl_config") {
Change this to:
BUILD.gn:source_set("ssl_config") {

[Abhishek, 2015-09-07 13:16:21]

Thanks for updating me about its type!

I've updated it.

PTAL# 4!

https://codereview.chromium.org/1320533007/diff/40001/components/ssl_config.gypi
File components/ssl_config.gypi (right):

https://codereview.chromium.org/1320533007/diff/40001/components/ssl_config.g...
components/ssl_config.gypi:10: 'type': '<(component)',
On 2015/09/07 13:06:37, droger wrote:
> Change this to:
> 'type': 'static_library',

Done.

https://codereview.chromium.org/1320533007/diff/40001/components/ssl_config/B...
File components/ssl_config/BUILD.gn (right):

https://codereview.chromium.org/1320533007/diff/40001/components/ssl_config/B...
components/ssl_config/BUILD.gn:6: component("ssl_config") {
On 2015/09/07 13:06:37, droger wrote:
> Change this to:
> BUILD.gn:source_set("ssl_config") {

Done.

https://codereview.chromium.org/1320533007/diff/40001/components/ssl_config/s...
File components/ssl_config/ssl_config_prefs.h (right):

https://codereview.chromium.org/1320533007/diff/40001/components/ssl_config/s...
components/ssl_config/ssl_config_prefs.h:11: // Local state prefs. Please add
Profile prefs above instead.
On 2015/09/07 09:53:31, droger wrote:
> Remove this comment.

Done.

[droger, 2015-09-07 14:04:50]

The CQ bit was checked by droger@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-07 14:04:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1320533007/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1320533007/60001

[commit-bot: I haz the power, 2015-09-07 14:12:55]

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

[commit-bot: I haz the power, 2015-09-07 14:12:56]

Dry run: 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...)

[Ryan Sleevi, 2015-09-08 21:02:16]

https://codereview.chromium.org/1320533007/diff/60001/chrome/browser/net/ssl_...
File chrome/browser/net/ssl_config_service_manager_pref_unittest.cc (right):

https://codereview.chromium.org/1320533007/diff/60001/chrome/browser/net/ssl_...
chrome/browser/net/ssl_config_service_manager_pref_unittest.cc:3: // found in
the LICENSE file.
Why is this unit test still in //chrome/browser/net? It seems like it should
have entirely moved into the component. Did I miss something?

(To confirm from reading the bug)
- Moving this unittest is blocked on dealing with PrefServiceMockFactory, right?

If so, then droger@'s comment #8 applies - it would be better to deal with that
issue before, rather than after.

https://codereview.chromium.org/1320533007/diff/60001/chrome/chrome_browser_c...
File chrome/chrome_browser_chromeos.gypi (right):

https://codereview.chromium.org/1320533007/diff/60001/chrome/chrome_browser_c...
chrome/chrome_browser_chromeos.gypi:1110:
'../components/components.gyp:ssl_config',
sort

https://codereview.chromium.org/1320533007/diff/60001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/1320533007/diff/60001/chrome/chrome_tests.gyp...
chrome/chrome_tests.gypi:2056: '../components/components.gyp:ssl_config',
sort

https://codereview.chromium.org/1320533007/diff/60001/components/ssl_config/B...
File components/ssl_config/BUILD.gn (right):

https://codereview.chromium.org/1320533007/diff/60001/components/ssl_config/B...
components/ssl_config/BUILD.gn:19: "//components/content_settings/core/browser",
Why does this dep exist here, but not in the .gypi?

https://codereview.chromium.org/1320533007/diff/60001/components/ssl_config/s...
File components/ssl_config/ssl_config_service_manager.h (right):

https://codereview.chromium.org/1320533007/diff/60001/components/ssl_config/s...
components/ssl_config/ssl_config_service_manager.h:31:
scoped_refptr<base::SingleThreadTaskRunner> io_task_runner);
STYLE: Pass this by const-ref.

https://codereview.chromium.org/1320533007/diff/60001/components/ssl_config/s...
File components/ssl_config/ssl_config_service_manager_pref.cc (right):

https://codereview.chromium.org/1320533007/diff/60001/components/ssl_config/s...
components/ssl_config/ssl_config_service_manager_pref.cc:92:
SSLConfigServicePref(
STYLE: Add 'explicit'
STYLE: Pass by const-ref.

https://codereview.chromium.org/1320533007/diff/60001/components/ssl_config/s...
components/ssl_config/ssl_config_service_manager_pref.cc:137: explicit
SSLConfigServiceManagerPref(
STYLE: remove explicit, this is now a two-arg ctor.

https://codereview.chromium.org/1320533007/diff/60001/components/ssl_config/s...
components/ssl_config/ssl_config_service_manager_pref.cc:139:
scoped_refptr<base::SingleThreadTaskRunner> io_task_runner);
const-ref

https://codereview.chromium.org/1320533007/diff/60001/components/ssl_config/s...
components/ssl_config/ssl_config_service_manager_pref.cc:307:
scoped_refptr<base::SingleThreadTaskRunner> io_task_runner) {
const-ref

[Abhishek, 2015-09-10 13:32:54]

I've updated the patch as suggested.

Removed "c/b/p/pref_service_mock_factory.h" dependency from unittest. But Its
failing in one unittest.

After resolving failing unittest, I'll move it to components/ssl_config

PTAL #5!

https://codereview.chromium.org/1320533007/diff/60001/chrome/browser/net/ssl_...
File chrome/browser/net/ssl_config_service_manager_pref_unittest.cc (right):

https://codereview.chromium.org/1320533007/diff/60001/chrome/browser/net/ssl_...
chrome/browser/net/ssl_config_service_manager_pref_unittest.cc:3: // found in
the LICENSE file.
On 2015/09/08 21:02:16, Ryan Sleevi wrote:
> Why is this unit test still in //chrome/browser/net? It seems like it should
> have entirely moved into the component. Did I miss something?
> 
> (To confirm from reading the bug)
> - Moving this unittest is blocked on dealing with PrefServiceMockFactory,
right?
> 
> If so, then droger@'s comment #8 applies - it would be better to deal with
that
> issue before, rather than after.
I've tried removing the mentioned dependency, but one UI is failing.

https://codereview.chromium.org/1320533007/diff/60001/chrome/chrome_browser_c...
File chrome/chrome_browser_chromeos.gypi (right):

https://codereview.chromium.org/1320533007/diff/60001/chrome/chrome_browser_c...
chrome/chrome_browser_chromeos.gypi:1110:
'../components/components.gyp:ssl_config',
On 2015/09/08 21:02:16, Ryan Sleevi wrote:
> sort

Done.

https://codereview.chromium.org/1320533007/diff/60001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/1320533007/diff/60001/chrome/chrome_tests.gyp...
chrome/chrome_tests.gypi:2056: '../components/components.gyp:ssl_config',
On 2015/09/08 21:02:16, Ryan Sleevi wrote:
> sort

Done.

https://codereview.chromium.org/1320533007/diff/60001/components/ssl_config/B...
File components/ssl_config/BUILD.gn (right):

https://codereview.chromium.org/1320533007/diff/60001/components/ssl_config/B...
components/ssl_config/BUILD.gn:19: "//components/content_settings/core/browser",
On 2015/09/08 21:02:16, Ryan Sleevi wrote:
> Why does this dep exist here, but not in the .gypi?

Done.

https://codereview.chromium.org/1320533007/diff/60001/components/ssl_config/s...
File components/ssl_config/ssl_config_service_manager.h (right):

https://codereview.chromium.org/1320533007/diff/60001/components/ssl_config/s...
components/ssl_config/ssl_config_service_manager.h:31:
scoped_refptr<base::SingleThreadTaskRunner> io_task_runner);
On 2015/09/08 21:02:16, Ryan Sleevi wrote:
> STYLE: Pass this by const-ref.

Done.

https://codereview.chromium.org/1320533007/diff/60001/components/ssl_config/s...
File components/ssl_config/ssl_config_service_manager_pref.cc (right):

https://codereview.chromium.org/1320533007/diff/60001/components/ssl_config/s...
components/ssl_config/ssl_config_service_manager_pref.cc:92:
SSLConfigServicePref(
On 2015/09/08 21:02:16, Ryan Sleevi wrote:
> STYLE: Add 'explicit'
> STYLE: Pass by const-ref.

Done.

https://codereview.chromium.org/1320533007/diff/60001/components/ssl_config/s...
components/ssl_config/ssl_config_service_manager_pref.cc:137: explicit
SSLConfigServiceManagerPref(
On 2015/09/08 21:02:16, Ryan Sleevi wrote:
> STYLE: remove explicit, this is now a two-arg ctor.

Done.

https://codereview.chromium.org/1320533007/diff/60001/components/ssl_config/s...
components/ssl_config/ssl_config_service_manager_pref.cc:139:
scoped_refptr<base::SingleThreadTaskRunner> io_task_runner);
On 2015/09/08 21:02:16, Ryan Sleevi wrote:
> const-ref

Done.

https://codereview.chromium.org/1320533007/diff/60001/components/ssl_config/s...
components/ssl_config/ssl_config_service_manager_pref.cc:307:
scoped_refptr<base::SingleThreadTaskRunner> io_task_runner) {
On 2015/09/08 21:02:16, Ryan Sleevi wrote:
> const-ref

Done.

https://codereview.chromium.org/1320533007/diff/80001/chrome/browser/net/ssl_...
File chrome/browser/net/ssl_config_service_manager_pref_unittest.cc (right):

https://codereview.chromium.org/1320533007/diff/80001/chrome/browser/net/ssl_...
chrome/browser/net/ssl_config_service_manager_pref_unittest.cc:191:
EXPECT_FALSE(version_min_pref->IsUserModifiable());
Currently this test is failing here. I tried solving it but I don't know how to
solve it? Please help me!

https://codereview.chromium.org/1320533007/diff/80001/chrome/browser/net/ssl_...
chrome/browser/net/ssl_config_service_manager_pref_unittest.cc:195:
EXPECT_FALSE(version_max_pref->IsUserModifiable());
ditto

[droger, 2015-09-10 14:49:38]

https://codereview.chromium.org/1320533007/diff/80001/chrome/browser/net/ssl_...
File chrome/browser/net/ssl_config_service_manager_pref_unittest.cc (right):

https://codereview.chromium.org/1320533007/diff/80001/chrome/browser/net/ssl_...
chrome/browser/net/ssl_config_service_manager_pref_unittest.cc:191:
EXPECT_FALSE(version_min_pref->IsUserModifiable());
On 2015/09/10 13:32:54, Abhishek wrote:
> Currently this test is failing here. I tried solving it but I don't know how
to
> solve it? Please help me!

I discussed with sdefresne who is more familiar with this code.
It turns out that that what this is testing is:
1) the command line pref store is not user modifiable
2) setting a preference through a switch does not set it in the user pref store
3) the command line switch is correctly converted to a preference.

None of this is relevant to ssl_config_service_manager_pref (and we can see that
it is not even called here), and furthermore these behaviors are already tested
elsewhere.
1) is tested in PrefValueStoreTest.PrefValueUserModifiable
3) is already tested by CommandLinePrefStoreTest.SimpleStringPref

This leaves only 2), which is really some kind of integration test. I'm not sure
how much value there is in testing that. I would suggest splitting this
particular test to a different file, or maybe delete it if owners agree, because
there may be very little value in it.

[Abhishek, 2015-09-15 06:10:51]

@rsleevi: ping

[blundell, 2015-09-15 08:30:05]

On 2015/09/15 06:10:51, Abhishek wrote:
> @rsleevi: ping

Can you describe what the current status of the CL is? As of comment #17, it
seemed that there were things that needed to be fixed before Ryan did a
re-review. Have all unittests that needed to be componentized actually been
componentized and are passing? Thanks!

[droger, 2015-09-15 08:48:49]

On 2015/09/15 08:30:05, blundell wrote:
> On 2015/09/15 06:10:51, Abhishek wrote:
> > @rsleevi: ping
> 
> Can you describe what the current status of the CL is? As of comment #17, it
> seemed that there were things that needed to be fixed before Ryan did a
> re-review. Have all unittests that needed to be componentized actually been
> componentized and are passing? Thanks!

In comment #17 I suggest deleting the offending unittest if owners agree.
Ryan: can we delete that unittest or do you think it is worth splitting in a
separate file?

[Abhishek, 2015-09-15 09:07:34]

On 2015/09/15 08:48:49, droger wrote:
> On 2015/09/15 08:30:05, blundell wrote:
> > On 2015/09/15 06:10:51, Abhishek wrote:
> > > @rsleevi: ping
> > 
> > Can you describe what the current status of the CL is? 

I'd resolved all the comments, those were registered on patch set #4.

Please find the unit test report with my latest patchset #5 from local build
[----------] Global test environment tear-down
[==========] 6 tests from 1 test case ran. (345 ms total)
[  PASSED  ] 5 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] SSLConfigServiceManagerPrefTest.CommandLinePrefs

As of comment #17, it
> > seemed that there were things that needed to be fixed before Ryan did a
> > re-review. Have all unittests that needed to be componentized actually been
> > componentized and are passing? Thanks!
> 
> In comment #17 I suggest deleting the offending unittest if owners agree.
> Ryan: can we delete that unittest or do you think it is worth splitting in a
> separate file?

[droger, 2015-09-24 09:44:32]

rsleevi: ping

[Ryan Sleevi, 2015-09-24 20:59:56]

Thanks for the ping. I'm still not sure componentization is the right thing
long-term, but to the extent it's still relevant for iOS, I guess we can
abstract more out of //chrome

Regarding the test, I'm not really keen to lose it. It was a result of a real
bug, and a request from the Chrome enterprise team to make sure we don't
introduce similar issues in future TLS-related policy flags.

https://codereview.chromium.org/1320533007/diff/80001/chrome/browser/net/ssl_...
File chrome/browser/net/ssl_config_service_manager_pref_unittest.cc (right):

https://codereview.chromium.org/1320533007/diff/80001/chrome/browser/net/ssl_...
chrome/browser/net/ssl_config_service_manager_pref_unittest.cc:191:
EXPECT_FALSE(version_min_pref->IsUserModifiable());
On 2015/09/10 14:49:38, droger wrote:
> This leaves only 2), which is really some kind of integration test. I'm not
sure
> how much value there is in testing that. I would suggest splitting this
> particular test to a different file, or maybe delete it if owners agree,
because
> there may be very little value in it.

Well, it was a regression test based on a real bug, so I'm kind of inclined to
keep it, since we know the testing of the other components wasn't sufficient in
the past :)

[Abhishek, 2015-09-28 05:30:33]

I'll upload a new patch soon.

https://codereview.chromium.org/1320533007/diff/80001/chrome/browser/net/ssl_...
File chrome/browser/net/ssl_config_service_manager_pref_unittest.cc (right):

https://codereview.chromium.org/1320533007/diff/80001/chrome/browser/net/ssl_...
chrome/browser/net/ssl_config_service_manager_pref_unittest.cc:191:
EXPECT_FALSE(version_min_pref->IsUserModifiable());
On 2015/09/24 20:59:56, Ryan Sleevi wrote:
> On 2015/09/10 14:49:38, droger wrote:
> > This leaves only 2), which is really some kind of integration test. I'm not
> sure
> > how much value there is in testing that. I would suggest splitting this
> > particular test to a different file, or maybe delete it if owners agree,
> because
> > there may be very little value in it.
> 
> Well, it was a regression test based on a real bug, so I'm kind of inclined to
> keep it, since we know the testing of the other components wasn't sufficient
in
> the past :)

Thanks for updating it!

Sure, I'll keep it and break it into another file as suggested by droger.

[Abhishek, 2015-10-08 13:16:57]

@rsleevi: 
I've broken the unittest for CommandLinePref. Now all the unittest are passing.

Please take a look at updated patchset# 6.

[droger, 2015-10-08 13:20:04]

Looks great.
Could you move ssl_config_service_manager_pref_unittest.cc to
	components/ssl_config?

[Abhishek, 2015-10-08 16:03:11]

On 2015/10/08 13:20:04, droger wrote:
> Looks great.
> Could you move ssl_config_service_manager_pref_unittest.cc to
> 	components/ssl_config?

Sure, I'll make changes and upload patch.

[Abhishek, 2015-10-09 05:48:30]

PTAL# 7.
I've moved unittest to components/ssl_config.

[droger, 2015-10-09 12:59:47]

Thanks.

LGTM with the comments below addressed.

https://codereview.chromium.org/1320533007/diff/120001/components/ssl_config/...
File components/ssl_config/DEPS (right):

https://codereview.chromium.org/1320533007/diff/120001/components/ssl_config/...
components/ssl_config/DEPS:4: "+components/syncable_prefs",
syncable_prefs is probably not needed.

https://codereview.chromium.org/1320533007/diff/120001/components/ssl_config/...
File components/ssl_config/ssl_config_service_manager_pref_unittest.cc (right):

https://codereview.chromium.org/1320533007/diff/120001/components/ssl_config/...
components/ssl_config/ssl_config_service_manager_pref_unittest.cc:19: #include
"components/syncable_prefs/testing_pref_service_syncable.h"
You can remove the includes of components/syncable_prefs, they are not needed.

[Abhishek, 2015-10-12 08:40:14]

@droger, rsleevi:

PTAL# 9

https://codereview.chromium.org/1320533007/diff/120001/components/ssl_config/...
File components/ssl_config/DEPS (right):

https://codereview.chromium.org/1320533007/diff/120001/components/ssl_config/...
components/ssl_config/DEPS:4: "+components/syncable_prefs",
On 2015/10/09 12:59:47, droger wrote:
> syncable_prefs is probably not needed.

Done.

https://codereview.chromium.org/1320533007/diff/120001/components/ssl_config/...
File components/ssl_config/ssl_config_service_manager_pref_unittest.cc (right):

https://codereview.chromium.org/1320533007/diff/120001/components/ssl_config/...
components/ssl_config/ssl_config_service_manager_pref_unittest.cc:19: #include
"components/syncable_prefs/testing_pref_service_syncable.h"
On 2015/10/09 12:59:47, droger wrote:
> You can remove the includes of components/syncable_prefs, they are not needed.

Done.

[droger, 2015-10-12 08:44:43]

droger@chromium.org changed reviewers:
+ battre@chromium.org

[droger, 2015-10-12 08:44:44]

Thanks.

+battre as OWNER for command_line_pref_store_ssl_manager_unittest.cc.
This is a test moved out of the ssl_manager_unittest. Moving to
//chrome/browser/prefs because it seems to be testing basic behavior of pref
classes.

[battre, 2015-10-12 08:53:28]

I don't think that this belongs to //chrome/browser/prefs.

IMO, the core part being tested there is whether you can instantiate a
SSLConfigServiceManager/SSLConfigService, and whether they can deal with the
preferences configured in the commandline preferences.

Everything up to line 46 is just code to set up an environment. We would not
need a new test for that code.

[droger, 2015-10-12 13:27:27]

On 2015/10/12 08:53:28, battre wrote:
> I don't think that this belongs to //chrome/browser/prefs.
> 
> IMO, the core part being tested there is whether you can instantiate a
> SSLConfigServiceManager/SSLConfigService, and whether they can deal with the
> preferences configured in the commandline preferences.

I think the core of the test is about wether that the preferences are not
user-modifiable, and are not leaked to the preference store.
The only //chrome code that is included in this unittest is
chrome/browser/prefs/command_line_pref_store.h.

I would be OK moving back this test to //chrome/browser/net though, on the
grounds that this test is somewhat related to network code.
Is this what you recommend?

> 
> Everything up to line 46 is just code to set up an environment. We would not
> need a new test for that code.

[battre, 2015-10-12 15:29:55]

On 2015/10/12 13:27:27, droger wrote:
> On 2015/10/12 08:53:28, battre wrote:
> > I don't think that this belongs to //chrome/browser/prefs.
> > 
> > IMO, the core part being tested there is whether you can instantiate a
> > SSLConfigServiceManager/SSLConfigService, and whether they can deal with the
> > preferences configured in the commandline preferences.
> 
> I think the core of the test is about wether that the preferences are not
> user-modifiable, and are not leaked to the preference store.
> The only //chrome code that is included in this unittest is
> chrome/browser/prefs/command_line_pref_store.h.
> 
> I would be OK moving back this test to //chrome/browser/net though, on the
> grounds that this test is somewhat related to network code.
> Is this what you recommend?

Actually, I looked at this again, and having the test in //chrome/browser/prefs
LGTM.

> 
> > 
> > Everything up to line 46 is just code to set up an environment. We would not
> > need a new test for that code.

[droger, 2015-10-12 16:24:11]

The CQ bit was checked by droger@chromium.org to run a CQ dry run

[droger, 2015-10-12 16:24:42]

rsleevi: ping

[commit-bot: I haz the power, 2015-10-12 16:25:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1320533007/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1320533007/160001

[commit-bot: I haz the power, 2015-10-12 16:27:40]

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

[commit-bot: I haz the power, 2015-10-12 16:27:41]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Abhishek, 2015-10-13 14:59:06]

@rsleevi: PTAL patchset# 10!

[Abhishek, 2015-10-19 18:11:14]

@rsleevi: ping

[Ryan Sleevi, 2015-10-19 18:32:10]

lgtm

[Abhishek, 2015-10-20 10:21:23]

Description was changed from

==========
Componentize ssl_config_service_manager_pref.cc

. Removed not needed notifications.
. Created new switches and prefs for ssl_config.
. Using SingleThreadTaskRunner over BrowserThread to remove content
dependencies.

BUG=517014
==========

to

==========
Componentize ssl_config_service_manager_pref.cc

. Removed not needed notifications.
. Created new switches and prefs for ssl_config.
. Using SingleThreadTaskRunner over BrowserThread to remove content
dependencies.

BUG=517014

TBR=jochen
==========

[Abhishek, 2015-10-20 10:21:23]

abhishek.a21@samsung.com changed reviewers:
+ jochen@chromium.org

[Abhishek, 2015-10-20 10:23:50]

TBR'ing jochen for the remaining changes.

Now /c/b/policy, /c/b/profiles and /c/b/chromeos/mobile/ contains namespace
changes.

[Abhishek, 2015-10-20 10:24:22]

The CQ bit was checked by abhishek.a21@samsung.com

[Abhishek, 2015-10-20 10:24:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
sdefresne@chromium.org, droger@chromium.org, battre@chromium.org
Link to the patchset: https://codereview.chromium.org/1320533007/#ps220001
(title: "Resolve -Wnewline-eof mac_chromium bot failure")

[commit-bot: I haz the power, 2015-10-20 10:25:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1320533007/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1320533007/220001

[commit-bot: I haz the power, 2015-10-20 11:27:37]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-10-20 11:28:37]

Patchset 12 (id:??) landed as
https://crrev.com/2849ceeb85e97574807c6f8b45f44a854fc81e5e
Cr-Commit-Position: refs/heads/master@{#355038}