Componentize proxy code from chrome/browser/net

Componentize proxy code from chrome/browser/net

. Moved preferences from pref_names to proxy_prefs
. Using base::ThreadChecker and base::SingleThreadedTaskRunner to remove
  BrowserThread dependency.
. Removed unused notification inclusions.

TBR=thakis

BUG=516714
Committed: https://crrev.com/71c612855d763ffbb195dd7b927dfeb91eee0326
Cr-Commit-Position: refs/heads/master@{#346369}

[Abhishek, 2015-08-17 13:00:42]

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

[Abhishek, 2015-08-17 13:00:42]

PTAL!

[droger, 2015-08-17 13:51:36]

Mostly LG. Minor comments below.

https://codereview.chromium.org/1296663003/diff/1/chrome/browser/net/pref_pro...
File chrome/browser/net/pref_proxy_config_tracker_impl_unittest.cc (right):

https://codereview.chromium.org/1296663003/diff/1/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_tracker_impl_unittest.cc:119:
content::TestBrowserThread io_thread_;
Can we remove these two threads and use ThreadTaskRunnerHandle::Get() instead
everywhere in this test?

https://codereview.chromium.org/1296663003/diff/1/components/proxy_config/pre...
File components/proxy_config/pref_proxy_config_tracker_impl.h (right):

https://codereview.chromium.org/1296663003/diff/1/components/proxy_config/pre...
components/proxy_config/pref_proxy_config_tracker_impl.h:35: class
ChromeProxyConfigService : public net::ProxyConfigService,
This class should probably be renamed, maybe as ProxyConfigServiceImpl.
This can probably be left for later though, to minimize the scope of this CL.

https://codereview.chromium.org/1296663003/diff/1/components/proxy_config/pre...
components/proxy_config/pref_proxy_config_tracker_impl.h:90: explicit
PrefProxyConfigTrackerImpl(
Nit: Remove "explicit"

https://codereview.chromium.org/1296663003/diff/1/components/proxy_config/pre...
components/proxy_config/pref_proxy_config_tracker_impl.h:92:
scoped_refptr<base::SingleThreadTaskRunner> task_runner);
Call this io_task_runner or network_task_runner.

https://codereview.chromium.org/1296663003/diff/1/components/proxy_config/pre...
components/proxy_config/pref_proxy_config_tracker_impl.h:170:
scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
Call this io_task_runner_ or network_task_runner_.

https://codereview.chromium.org/1296663003/diff/1/components/proxy_config/pro...
File components/proxy_config/proxy_prefs.cc (right):

https://codereview.chromium.org/1296663003/diff/1/components/proxy_config/pro...
components/proxy_config/proxy_prefs.cc:32: const char kProxy[] = "proxy";
Keep the original comment:
// Preference to store proxy settings.

https://codereview.chromium.org/1296663003/diff/1/components/proxy_config/pro...
File components/proxy_config/proxy_prefs.h (right):

https://codereview.chromium.org/1296663003/diff/1/components/proxy_config/pro...
components/proxy_config/proxy_prefs.h:63: PROXY_CONFIG_EXPORT extern const char
kProxy[];
For consistency this should be in the ProxyPrefs::prefs:: namespace.

Optionally move it to a separate file (proxy_config_pref_names.h), but maybe it
can stay here if the owner is OK with this. If you keep it here, maybe put it at
the top, right after the  namespace ProxyPrefs {.

namespace prefs {

PROXY_CONFIG_EXPORT extern const char kProxy[];

}  // namespace prefs

[Abhishek, 2015-08-19 08:30:49]

I've updated it as per the comments.

PTAL#3!

https://codereview.chromium.org/1296663003/diff/1/chrome/browser/net/pref_pro...
File chrome/browser/net/pref_proxy_config_tracker_impl_unittest.cc (right):

https://codereview.chromium.org/1296663003/diff/1/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_tracker_impl_unittest.cc:119:
content::TestBrowserThread io_thread_;
On 2015/08/17 13:51:35, droger wrote:
> Can we remove these two threads and use ThreadTaskRunnerHandle::Get() instead
> everywhere in this test?

Done.

https://codereview.chromium.org/1296663003/diff/1/components/proxy_config/pre...
File components/proxy_config/pref_proxy_config_tracker_impl.h (right):

https://codereview.chromium.org/1296663003/diff/1/components/proxy_config/pre...
components/proxy_config/pref_proxy_config_tracker_impl.h:35: class
ChromeProxyConfigService : public net::ProxyConfigService,
On 2015/08/17 13:51:36, droger wrote:
> This class should probably be renamed, maybe as ProxyConfigServiceImpl.
> This can probably be left for later though, to minimize the scope of this CL.

Done.

https://codereview.chromium.org/1296663003/diff/1/components/proxy_config/pre...
components/proxy_config/pref_proxy_config_tracker_impl.h:90: explicit
PrefProxyConfigTrackerImpl(
On 2015/08/17 13:51:35, droger wrote:
> Nit: Remove "explicit"

Done.

https://codereview.chromium.org/1296663003/diff/1/components/proxy_config/pre...
components/proxy_config/pref_proxy_config_tracker_impl.h:92:
scoped_refptr<base::SingleThreadTaskRunner> task_runner);
On 2015/08/17 13:51:36, droger wrote:
> Call this io_task_runner or network_task_runner.

Done.

https://codereview.chromium.org/1296663003/diff/1/components/proxy_config/pre...
components/proxy_config/pref_proxy_config_tracker_impl.h:170:
scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
On 2015/08/17 13:51:36, droger wrote:
> Call this io_task_runner_ or network_task_runner_.

Done.

https://codereview.chromium.org/1296663003/diff/1/components/proxy_config/pro...
File components/proxy_config/proxy_prefs.cc (right):

https://codereview.chromium.org/1296663003/diff/1/components/proxy_config/pro...
components/proxy_config/proxy_prefs.cc:32: const char kProxy[] = "proxy";
On 2015/08/17 13:51:36, droger wrote:
> Keep the original comment:
> // Preference to store proxy settings.

Done.

https://codereview.chromium.org/1296663003/diff/1/components/proxy_config/pro...
File components/proxy_config/proxy_prefs.h (right):

https://codereview.chromium.org/1296663003/diff/1/components/proxy_config/pro...
components/proxy_config/proxy_prefs.h:63: PROXY_CONFIG_EXPORT extern const char
kProxy[];
On 2015/08/17 13:51:36, droger wrote:
> For consistency this should be in the ProxyPrefs::prefs:: namespace.
> 
> Optionally move it to a separate file (proxy_config_pref_names.h), but maybe
it
> can stay here if the owner is OK with this. If you keep it here, maybe put it
at
> the top, right after the  namespace ProxyPrefs {.
> 
> namespace prefs {
> 
> PROXY_CONFIG_EXPORT extern const char kProxy[];
> 
> }  // namespace prefs

I think it would be good if we can move it to "proxy_config_pref_names.h".

I've kept it inside ProxyPrefs::prefs::kProxy namespace as suggested.

[droger, 2015-08-19 09:18:45]

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

[droger, 2015-08-19 09:18:46]

Very good. Still holding on the LG because of very minor issues..

+atwilson as OWNER for proxy code.

https://codereview.chromium.org/1296663003/diff/40001/components/proxy_config...
File components/proxy_config/pref_proxy_config_tracker_impl.h (right):

https://codereview.chromium.org/1296663003/diff/40001/components/proxy_config...
components/proxy_config/pref_proxy_config_tracker_impl.h:166:
ProxyConfigServiceImpl* chrome_proxy_config_service_;  // Weak ptr.
nit: rename this to:
proxy_config_service_impl_

https://codereview.chromium.org/1296663003/diff/40001/components/proxy_config...
File components/proxy_config/proxy_config_pref_names.cc (right):

https://codereview.chromium.org/1296663003/diff/40001/components/proxy_config...
components/proxy_config/proxy_config_pref_names.cc:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
Remove (c)

https://codereview.chromium.org/1296663003/diff/40001/components/proxy_config...
components/proxy_config/proxy_config_pref_names.cc:13: }  // namespace prefs
Same here: be consistent with blank lines.

https://codereview.chromium.org/1296663003/diff/40001/components/proxy_config...
File components/proxy_config/proxy_config_pref_names.h (right):

https://codereview.chromium.org/1296663003/diff/40001/components/proxy_config...
components/proxy_config/proxy_config_pref_names.h:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
Remove (c)

https://codereview.chromium.org/1296663003/diff/40001/components/proxy_config...
components/proxy_config/proxy_config_pref_names.h:14: // Preference to store
proxy settings.
Move this comment to the .cc file, to be consistent with other pref files.

https://codereview.chromium.org/1296663003/diff/40001/components/proxy_config...
components/proxy_config/proxy_config_pref_names.h:17: }  // namespace prefs
Add blank line here, or remove blank line 11, to be consistent.

https://codereview.chromium.org/1296663003/diff/40001/components/proxy_config...
components/proxy_config/proxy_config_pref_names.h:18: }  // namespace ProxyPrefs
Blank line before #endif.

[droger, 2015-08-19 09:19:01]

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

[droger, 2015-08-19 09:19:18]

Launching a tryjob for you.

[commit-bot: I haz the power, 2015-08-19 09:19:19]

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

[commit-bot: I haz the power, 2015-08-19 09:22:28]

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

[commit-bot: I haz the power, 2015-08-19 09:22:29]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[droger, 2015-08-19 09:30:53]

https://codereview.chromium.org/1296663003/diff/40001/components/proxy_config...
File components/proxy_config.gypi (right):

https://codereview.chromium.org/1296663003/diff/40001/components/proxy_config...
components/proxy_config.gypi:13: '../components.gyp:pref_registry',
This should be simply:
'pref_registry',

[Abhishek, 2015-08-19 10:10:29]

All comments are incorporated.

PTAL #4!

https://codereview.chromium.org/1296663003/diff/40001/components/proxy_config...
File components/proxy_config.gypi (right):

https://codereview.chromium.org/1296663003/diff/40001/components/proxy_config...
components/proxy_config.gypi:13: '../components.gyp:pref_registry',
On 2015/08/19 09:30:53, droger wrote:
> This should be simply:
> 'pref_registry',

Done.

https://codereview.chromium.org/1296663003/diff/40001/components/proxy_config...
File components/proxy_config/pref_proxy_config_tracker_impl.h (right):

https://codereview.chromium.org/1296663003/diff/40001/components/proxy_config...
components/proxy_config/pref_proxy_config_tracker_impl.h:166:
ProxyConfigServiceImpl* chrome_proxy_config_service_;  // Weak ptr.
On 2015/08/19 09:18:45, droger wrote:
> nit: rename this to:
> proxy_config_service_impl_

Done.

https://codereview.chromium.org/1296663003/diff/40001/components/proxy_config...
File components/proxy_config/proxy_config_pref_names.cc (right):

https://codereview.chromium.org/1296663003/diff/40001/components/proxy_config...
components/proxy_config/proxy_config_pref_names.cc:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
On 2015/08/19 09:18:45, droger wrote:
> Remove (c)

Done.

https://codereview.chromium.org/1296663003/diff/40001/components/proxy_config...
components/proxy_config/proxy_config_pref_names.cc:13: }  // namespace prefs
On 2015/08/19 09:18:45, droger wrote:
> Same here: be consistent with blank lines.

Done.

https://codereview.chromium.org/1296663003/diff/40001/components/proxy_config...
File components/proxy_config/proxy_config_pref_names.h (right):

https://codereview.chromium.org/1296663003/diff/40001/components/proxy_config...
components/proxy_config/proxy_config_pref_names.h:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
On 2015/08/19 09:18:46, droger wrote:
> Remove (c)

Done.

https://codereview.chromium.org/1296663003/diff/40001/components/proxy_config...
components/proxy_config/proxy_config_pref_names.h:14: // Preference to store
proxy settings.
On 2015/08/19 09:18:45, droger wrote:
> Move this comment to the .cc file, to be consistent with other pref files.

Done.

https://codereview.chromium.org/1296663003/diff/40001/components/proxy_config...
components/proxy_config/proxy_config_pref_names.h:17: }  // namespace prefs
On 2015/08/19 09:18:46, droger wrote:
> Add blank line here, or remove blank line 11, to be consistent.

Done.

https://codereview.chromium.org/1296663003/diff/40001/components/proxy_config...
components/proxy_config/proxy_config_pref_names.h:18: }  // namespace ProxyPrefs
On 2015/08/19 09:18:46, droger wrote:
> Blank line before #endif.

Done.

[Abhishek, 2015-08-19 10:10:38]

The CQ bit was checked by abhishek.a21@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2015-08-19 10:10:51]

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

[commit-bot: I haz the power, 2015-08-19 10:10:52]

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

[commit-bot: I haz the power, 2015-08-19 10:10:53]

Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are
accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Abhishek, 2015-08-19 10:12:59]

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

[droger, 2015-08-19 10:42:29]

lgtm

https://codereview.chromium.org/1296663003/diff/60001/components/proxy_config...
File components/proxy_config.gypi (right):

https://codereview.chromium.org/1296663003/diff/60001/components/proxy_config...
components/proxy_config.gypi:14: 'components.gyp:pref_registry',
Drop the components.gyp, just
'pref_registry',

[Abhishek, 2015-08-19 13:31:41]

Thanks for your review!

I've updated it.

I'm trying to solve buldbot failure issues.

https://codereview.chromium.org/1296663003/diff/60001/components/proxy_config...
File components/proxy_config.gypi (right):

https://codereview.chromium.org/1296663003/diff/60001/components/proxy_config...
components/proxy_config.gypi:14: 'components.gyp:pref_registry',
On 2015/08/19 10:42:29, droger wrote:
> Drop the components.gyp, just
> 'pref_registry',

Done.

[Andrew T Wilson (Slow), 2015-08-19 15:03:43]

I don't think I'm an OWNER of any proxy code - specifically which directories do
you need my OWNERS approval for?

[droger, 2015-08-19 15:15:23]

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

[droger, 2015-08-19 15:15:24]

On 2015/08/19 15:03:43, Andrew T Wilson wrote:
> I don't think I'm an OWNER of any proxy code - specifically which directories
do
> you need my OWNERS approval for?

Sorry I made a mistake reading the owner file. Thanks for the heads up.
FYI: you are listed in chrome/browser/net/OWNERS as owning proxy_policy_handler,
but this is not what is touched in this CL.

Adding stevenjb instead.

[stevenjb, 2015-08-19 15:38:28]

stevenjb@chromium.org changed reviewers:
+ pneubeck@chromium.org

[stevenjb, 2015-08-19 15:38:29]

I'll take a look, but pneubeck@ should probably look at this also, he's more
familiar with chromeos proxy code than I am.

[stevenjb, 2015-08-19 15:44:46]

Actually, this is pretty mechanical, pnebueck@ - FYI.
LGTM

https://codereview.chromium.org/1296663003/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/extension_message_bubble_controller_unittest.cc
(right):

https://codereview.chromium.org/1296663003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_message_bubble_controller_unittest.cc:19:
#include "chrome/common/pref_names.h"
Still needed?

[droger, 2015-08-19 15:55:57]

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

[droger, 2015-08-19 15:55:58]

+mmenke for chrome/browser/net

[mmenke, 2015-08-19 16:03:11]

mmenke@chromium.org changed reviewers:
+ eroman@chromium.org

[mmenke, 2015-08-19 16:03:12]

[+eroman]  Deferring to eroman, who knows the proxy stuff better than I do.  Not
a huge fan of the pattern of splitting up tests and the code they actually test,
since it means we're using code on a platform that we aren't actually testing it
on.

[droger, 2015-08-19 16:29:23]

On 2015/08/19 16:03:12, mmenke wrote:
> [+eroman]  Deferring to eroman, who knows the proxy stuff better than I do. 
Not
> a huge fan of the pattern of splitting up tests and the code they actually
test,
> since it means we're using code on a platform that we aren't actually testing
it
> on.

Good point.The unittest should be moved too.
Most of them seem very easy to move.

The tests about the command line, I'm not completely sure if they require some
Chrome-specific behavior or not.
Maybe we can use base::TestingPrefServiceSyncable instead of the
PrefServiceMockFactory? This needs some investigation.

[droger, 2015-08-19 16:39:33]

On 2015/08/19 16:29:23, droger wrote:
> On 2015/08/19 16:03:12, mmenke wrote:
> > [+eroman]  Deferring to eroman, who knows the proxy stuff better than I do. 
> Not
> > a huge fan of the pattern of splitting up tests and the code they actually
> test,
> > since it means we're using code on a platform that we aren't actually
testing
> it
> > on.
> 
> Good point.The unittest should be moved too.
> Most of them seem very easy to move.
> 
> The tests about the command line, I'm not completely sure if they require some
> Chrome-specific behavior or not.
> Maybe we can use base::TestingPrefServiceSyncable instead of the
> PrefServiceMockFactory? This needs some investigation.

It seems that the functionality relies on CommandLinePrefStore, which is in
Chrome.
So I think PrefProxyConfigTrackerImplCommandLineTest should stay in //chrome,
and the rest can be moved to the component.

[blundell, 2015-08-20 08:29:10]

On 2015/08/19 16:03:12, mmenke wrote:
> [+eroman]  Deferring to eroman, who knows the proxy stuff better than I do. 
Not
> a huge fan of the pattern of splitting up tests and the code they actually
test,
> since it means we're using code on a platform that we aren't actually testing
it
> on.

What we often do during componentization is to tackle the componentization of
the unittests in a second phase as a separation of concerns. Note that for iOS
in particular accomplishing the first phase is a net win, as before iOS was
using the code downstream but not even the production code was being built
upstream on iOS.

I agree with you that the state of having unittests in a different module than
the production code should always be a temporary one.

[mmenke, 2015-08-20 11:27:52]

On 2015/08/20 08:29:10, blundell wrote:
> On 2015/08/19 16:03:12, mmenke wrote:
> > [+eroman]  Deferring to eroman, who knows the proxy stuff better than I do. 
> Not
> > a huge fan of the pattern of splitting up tests and the code they actually
> test,
> > since it means we're using code on a platform that we aren't actually
testing
> it
> > on.
> 
> What we often do during componentization is to tackle the componentization of
> the unittests in a second phase as a separation of concerns. Note that for iOS
> in particular accomplishing the first phase is a net win, as before iOS was
> using the code downstream but not even the production code was being built
> upstream on iOS.
> 
> I agree with you that the state of having unittests in a different module than
> the production code should always be a temporary one.

There are a number of cases I've seen where this never seems to happen - tests
often remain outside of the corresponding component.

[blundell, 2015-08-20 13:15:01]

On 2015/08/20 11:27:52, mmenke wrote:
> On 2015/08/20 08:29:10, blundell wrote:
> > On 2015/08/19 16:03:12, mmenke wrote:
> > > [+eroman]  Deferring to eroman, who knows the proxy stuff better than I
do. 
> > Not
> > > a huge fan of the pattern of splitting up tests and the code they actually
> > test,
> > > since it means we're using code on a platform that we aren't actually
> testing
> > it
> > > on.
> > 
> > What we often do during componentization is to tackle the componentization
of
> > the unittests in a second phase as a separation of concerns. Note that for
iOS
> > in particular accomplishing the first phase is a net win, as before iOS was
> > using the code downstream but not even the production code was being built
> > upstream on iOS.
> > 
> > I agree with you that the state of having unittests in a different module
than
> > the production code should always be a temporary one.
> 
> There are a number of cases I've seen where this never seems to happen - tests
> often remain outside of the corresponding component.

I agree that that's bad. I'm aware of one feature whose componentization I'm
responsible for where that's currently the case --
//chrome/browser{autocomplete, ui/omnibox}, whose componentization I finished
earlier this summer. I have those tests targeted for componentization. Let me
know of any others that you're aware of, and if I/we performed the
componentizations, I'll make sure that componentizing the tests is on our radar.

[Abhishek, 2015-08-20 14:14:51]

I've updated as commented.

I'm checking other bots errors.
But some bots error are not coming in my local build like:
--
PrefProxyConfigTrackerImpl::PrefConfigToNetConfig(ProxyConfigDictionary const&,
net::ProxyConfig*): error: undefined reference to 'GURL::GURL(std::string
const&)'
--

I think I'm missing some target name or flag mismatch.

If any pointer about it, that will be a great help!

https://codereview.chromium.org/1296663003/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/extension_message_bubble_controller_unittest.cc
(right):

https://codereview.chromium.org/1296663003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_message_bubble_controller_unittest.cc:19:
#include "chrome/common/pref_names.h"
On 2015/08/19 15:44:46, stevenjb wrote:
> Still needed?

Done.

[droger, 2015-08-20 14:25:35]

On 2015/08/20 14:14:51, Abhishek wrote:
> I've updated as commented.
> 
> I'm checking other bots errors.
> But some bots error are not coming in my local build like:
> --
> PrefProxyConfigTrackerImpl::PrefConfigToNetConfig(ProxyConfigDictionary
const&,
> net::ProxyConfig*): error: undefined reference to 'GURL::GURL(std::string
> const&)'
> --
> 
> I think I'm missing some target name or flag mismatch.
> 
> If any pointer about it, that will be a great help!

Missing dependency on //url in components/proxy_config/BUILD.gn

[droger, 2015-08-20 14:26:24]

On 2015/08/20 14:25:35, droger wrote:
> Missing dependency on //url in components/proxy_config/BUILD.gn
And components/proxy_config.gypi too.

[Abhishek, 2015-08-20 14:37:58]

On 2015/08/20 14:26:24, droger wrote:
> On 2015/08/20 14:25:35, droger wrote:
> > Missing dependency on //url in components/proxy_config/BUILD.gn
> And components/proxy_config.gypi too.

Thanks a lot!

I've updated it.

[mmenke, 2015-08-20 15:29:48]

On 2015/08/20 13:15:01, blundell wrote:
> On 2015/08/20 11:27:52, mmenke wrote:
> > On 2015/08/20 08:29:10, blundell wrote:
> > > On 2015/08/19 16:03:12, mmenke wrote:
> > > > [+eroman]  Deferring to eroman, who knows the proxy stuff better than I
> do. 
> > > Not
> > > > a huge fan of the pattern of splitting up tests and the code they
actually
> > > test,
> > > > since it means we're using code on a platform that we aren't actually
> > testing
> > > it
> > > > on.
> > > 
> > > What we often do during componentization is to tackle the componentization
> of
> > > the unittests in a second phase as a separation of concerns. Note that for
> iOS
> > > in particular accomplishing the first phase is a net win, as before iOS
was
> > > using the code downstream but not even the production code was being built
> > > upstream on iOS.
> > > 
> > > I agree with you that the state of having unittests in a different module
> than
> > > the production code should always be a temporary one.
> > 
> > There are a number of cases I've seen where this never seems to happen -
tests
> > often remain outside of the corresponding component.
> 
> I agree that that's bad. I'm aware of one feature whose componentization I'm
> responsible for where that's currently the case --
> //chrome/browser{autocomplete, ui/omnibox}, whose componentization I finished
> earlier this summer. I have those tests targeted for componentization. Let me
> know of any others that you're aware of, and if I/we performed the
> componentizations, I'll make sure that componentizing the tests is on our
radar.

Here are the ones I've seen:

chrome/browser/policy/url_blacklist_manager_unittest.cc
chrome/browser/content_settings/content_settings_default_provider_unittest.cc
chrome/browser/extensions has a couple (extension_protocols_unittest.cc and
chrome_info_map_unittest.cc for isntance, there are probably others -
implementations of corresponding classes are in extensions/browsers.  I assume
these have nothing to do with the componentization for iOS effort).

I'm sure I've seen them in one or two other places - there was even a
browsertest somewhere that seemed like perhaps it should be with a component.

[blundell, 2015-08-25 08:41:43]

On 2015/08/20 15:29:48, mmenke wrote:
> On 2015/08/20 13:15:01, blundell wrote:
> > On 2015/08/20 11:27:52, mmenke wrote:
> > > On 2015/08/20 08:29:10, blundell wrote:
> > > > On 2015/08/19 16:03:12, mmenke wrote:
> > > > > [+eroman]  Deferring to eroman, who knows the proxy stuff better than
I
> > do. 
> > > > Not
> > > > > a huge fan of the pattern of splitting up tests and the code they
> actually
> > > > test,
> > > > > since it means we're using code on a platform that we aren't actually
> > > testing
> > > > it
> > > > > on.
> > > > 
> > > > What we often do during componentization is to tackle the
componentization
> > of
> > > > the unittests in a second phase as a separation of concerns. Note that
for
> > iOS
> > > > in particular accomplishing the first phase is a net win, as before iOS
> was
> > > > using the code downstream but not even the production code was being
built
> > > > upstream on iOS.
> > > > 
> > > > I agree with you that the state of having unittests in a different
module
> > than
> > > > the production code should always be a temporary one.
> > > 
> > > There are a number of cases I've seen where this never seems to happen -
> tests
> > > often remain outside of the corresponding component.
> > 
> > I agree that that's bad. I'm aware of one feature whose componentization I'm
> > responsible for where that's currently the case --
> > //chrome/browser{autocomplete, ui/omnibox}, whose componentization I
finished
> > earlier this summer. I have those tests targeted for componentization. Let
me
> > know of any others that you're aware of, and if I/we performed the
> > componentizations, I'll make sure that componentizing the tests is on our
> radar.
> 
> Here are the ones I've seen:
> 

Thanks!

> chrome/browser/policy/url_blacklist_manager_unittest.cc

I looked at this one and it actually had a comment in the .cc file explaining
the
reason why it hadn't been componentized. That reason has since become obsolete;
I'll componentize it.

> chrome/browser/content_settings/content_settings_default_provider_unittest.cc

I'll reach out to the engineer who supervised that componentization to see
what's
going on there.

> chrome/browser/extensions has a couple (extension_protocols_unittest.cc and
> chrome_info_map_unittest.cc for isntance, there are probably others -
> implementations of corresponding classes are in extensions/browsers.  I assume
> these have nothing to do with the componentization for iOS effort).
>

Correct.
 
> I'm sure I've seen them in one or two other places - there was even a
> browsertest somewhere that seemed like perhaps it should be with a component.

FWIW, I find reasoning about the proper location of browsertests trickier than
unittests. Often they end up going through flows that exercises the //chrome
side
of components (e.g., the //chrome implementation of a component's client
interface).

[blundell, 2015-08-25 08:43:04]

On 2015/08/25 08:41:43, blundell wrote:
> On 2015/08/20 15:29:48, mmenke wrote:
> > On 2015/08/20 13:15:01, blundell wrote:
> > > On 2015/08/20 11:27:52, mmenke wrote:
> > > > On 2015/08/20 08:29:10, blundell wrote:
> > > > > On 2015/08/19 16:03:12, mmenke wrote:
> > > > > > [+eroman]  Deferring to eroman, who knows the proxy stuff better
than
> I
> > > do. 
> > > > > Not
> > > > > > a huge fan of the pattern of splitting up tests and the code they
> > actually
> > > > > test,
> > > > > > since it means we're using code on a platform that we aren't
actually
> > > > testing
> > > > > it
> > > > > > on.
> > > > > 
> > > > > What we often do during componentization is to tackle the
> componentization
> > > of
> > > > > the unittests in a second phase as a separation of concerns. Note that
> for
> > > iOS
> > > > > in particular accomplishing the first phase is a net win, as before
iOS
> > was
> > > > > using the code downstream but not even the production code was being
> built
> > > > > upstream on iOS.
> > > > > 
> > > > > I agree with you that the state of having unittests in a different
> module
> > > than
> > > > > the production code should always be a temporary one.
> > > > 
> > > > There are a number of cases I've seen where this never seems to happen -
> > tests
> > > > often remain outside of the corresponding component.
> > > 
> > > I agree that that's bad. I'm aware of one feature whose componentization
I'm
> > > responsible for where that's currently the case --
> > > //chrome/browser{autocomplete, ui/omnibox}, whose componentization I
> finished
> > > earlier this summer. I have those tests targeted for componentization. Let
> me
> > > know of any others that you're aware of, and if I/we performed the
> > > componentizations, I'll make sure that componentizing the tests is on our
> > radar.
> > 
> > Here are the ones I've seen:
> > 
> 
> Thanks!
> 
> > chrome/browser/policy/url_blacklist_manager_unittest.cc
> 
> I looked at this one and it actually had a comment in the .cc file explaining
> the
> reason why it hadn't been componentized. That reason has since become
obsolete;
> I'll componentize it.
> 
> >
chrome/browser/content_settings/content_settings_default_provider_unittest.cc
> 
> I'll reach out to the engineer who supervised that componentization to see
> what's
> going on there.
> 
> > chrome/browser/extensions has a couple (extension_protocols_unittest.cc and
> > chrome_info_map_unittest.cc for isntance, there are probably others -
> > implementations of corresponding classes are in extensions/browsers.  I
assume
> > these have nothing to do with the componentization for iOS effort).
> >
> 
> Correct.
>  
> > I'm sure I've seen them in one or two other places - there was even a
> > browsertest somewhere that seemed like perhaps it should be with a
component.
> 
> FWIW, I find reasoning about the proper location of browsertests trickier than
> unittests. Often they end up going through flows that exercises the //chrome
> side
> of components (e.g., the //chrome implementation of a component's client
> interface).

For whoever knows the answer to this question:

What is the status of this CL? Is the next step that there are unittests to
componentize as well?

[droger, 2015-08-25 08:49:06]

> What is the status of this CL? Is the next step that there are unittests to
> componentize as well?

I'd vote for componentizing the tests in a follow up. I can help or do it with
high priority if required.
Not everything can be componentized right now though, so either we wait until
the command line pref store is componentized (which is planned) or we only
componentize the test partially.

[Abhishek, 2015-08-25 09:01:25]

On 2015/08/25 08:49:06, droger wrote:
> > What is the status of this CL? Is the next step that there are unittests to
> > componentize as well?
Currently I'm trying to solve build error's. Which are coming on multiple
platforms but not on my local's(android, linux, chormeos) :/

> 
> I'd vote for componentizing the tests in a follow up. I can help or do it with
> high priority if required.
> Not everything can be componentized right now though, so either we wait until
> the command line pref store is componentized (which is planned) or we only
> componentize the test partially.

I too agree with droger for componentizing the
test(pref_proxy_config_tracker_impl_unittest) related to this patch in follow up
CL.

[droger, 2015-08-25 09:21:29]

On 2015/08/25 09:01:25, Abhishek wrote:
> On 2015/08/25 08:49:06, droger wrote:
> > > What is the status of this CL? Is the next step that there are unittests
to
> > > componentize as well?
> Currently I'm trying to solve build error's. Which are coming on multiple
> platforms but not on my local's(android, linux, chormeos) :/

I am not sure, but I am suspecting that missing PROXY_CONFIG_EXPORT may cause
the link failures.
This is defined in components/proxy_config/proxy_config_export.h.

[blundell, 2015-08-25 09:25:57]

On 2015/08/25 09:21:29, droger wrote:
> On 2015/08/25 09:01:25, Abhishek wrote:
> > On 2015/08/25 08:49:06, droger wrote:
> > > > What is the status of this CL? Is the next step that there are unittests
> to
> > > > componentize as well?
> > Currently I'm trying to solve build error's. Which are coming on multiple
> > platforms but not on my local's(android, linux, chormeos) :/
> 
> I am not sure, but I am suspecting that missing PROXY_CONFIG_EXPORT may cause
> the link failures.
> This is defined in components/proxy_config/proxy_config_export.h.

Yep, that looks like it. If you want to repro locally, add 'component':
'shared_library' to your ~/.gyp/include.gypi, rerun gyp, and rebuild.

[Abhishek, 2015-08-25 13:45:16]

On 2015/08/25 09:25:57, blundell wrote:
> On 2015/08/25 09:21:29, droger wrote:
> > On 2015/08/25 09:01:25, Abhishek wrote:
> > > On 2015/08/25 08:49:06, droger wrote:
> > > > > What is the status of this CL? Is the next step that there are
unittests
> > to
> > > > > componentize as well?
> > > Currently I'm trying to solve build error's. Which are coming on multiple
> > > platforms but not on my local's(android, linux, chormeos) :/
> > 
> > I am not sure, but I am suspecting that missing PROXY_CONFIG_EXPORT may
cause
> > the link failures.
> > This is defined in components/proxy_config/proxy_config_export.h.
> 
> Yep, that looks like it. If you want to repro locally, add 'component':
> 'shared_library' to your ~/.gyp/include.gypi, rerun gyp, and rebuild.

Thanks!

I'll upload updated patch soon.

[Abhishek, 2015-08-26 09:48:59]

PTAL #10!

https://codereview.chromium.org/1296663003/diff/180001/components/proxy_confi...
File components/proxy_config/pref_proxy_config_tracker_impl.cc (right):

https://codereview.chromium.org/1296663003/diff/180001/components/proxy_confi...
components/proxy_config/pref_proxy_config_tracker_impl.cc:139:
DCHECK(thread_checker_.CalledOnValidThread());
Tests are crashing at this place.

http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...

Do I need to remove this CHECK?
Or
Need to convert in io_task_runner?

[droger, 2015-08-26 10:11:22]

https://codereview.chromium.org/1296663003/diff/180001/components/proxy_confi...
File components/proxy_config/pref_proxy_config_tracker_impl.cc (right):

https://codereview.chromium.org/1296663003/diff/180001/components/proxy_confi...
components/proxy_config/pref_proxy_config_tracker_impl.cc:139:
DCHECK(thread_checker_.CalledOnValidThread());
On 2015/08/26 09:48:59, Abhishek wrote:
> Tests are crashing at this place.

It looks like it happens because the object is created on the UI thread, and
thus the thread_checker_ is attached to the UI thread, not the IO thread.

If that works, I would suggest calling
thread_checker_.DetachFromThread() in the ProxyConfigServiceImpl contstructor,
with a comment:

  // ProxyConfigServiceImpl is created on the UI thread, but used on the network
  // thread.
  thread_checker_.DetachFromThread();


Another option would be to pass the io_task_runner_ to ProxyConfigServiceImpl
and then do the checks using io_task_runner_BelongsToCurrentThread().

[Abhishek, 2015-08-26 13:44:36]

PTAL #11!

I've updated it as suggested and added it.

https://codereview.chromium.org/1296663003/diff/180001/components/proxy_confi...
File components/proxy_config/pref_proxy_config_tracker_impl.cc (right):

https://codereview.chromium.org/1296663003/diff/180001/components/proxy_confi...
components/proxy_config/pref_proxy_config_tracker_impl.cc:139:
DCHECK(thread_checker_.CalledOnValidThread());
On 2015/08/26 10:11:22, droger wrote:
> On 2015/08/26 09:48:59, Abhishek wrote:
> > Tests are crashing at this place.
> 
> It looks like it happens because the object is created on the UI thread, and
> thus the thread_checker_ is attached to the UI thread, not the IO thread.
> 
> If that works, I would suggest calling
> thread_checker_.DetachFromThread() in the ProxyConfigServiceImpl contstructor,
> with a comment:
> 
>   // ProxyConfigServiceImpl is created on the UI thread, but used on the
network
>   // thread.
>   thread_checker_.DetachFromThread();
> 
I've changed it as suggested here.

> 
> Another option would be to pass the io_task_runner_ to ProxyConfigServiceImpl
> and then do the checks using io_task_runner_BelongsToCurrentThread().

[mmenke, 2015-08-26 15:04:00]

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

[eroman, 2015-08-27 01:16:33]

The translation looks correct.

LGTM as long as someone is going to follow-up for componentizing the 
unit-tests.

https://codereview.chromium.org/1296663003/diff/200001/components/proxy_confi...
File components/proxy_config/proxy_config_pref_names.h (right):

https://codereview.chromium.org/1296663003/diff/200001/components/proxy_confi...
components/proxy_config/proxy_config_pref_names.h:5: #ifndef
COMPONETS_PROXY_CONFIG_PROXY_CONFIG_PREF_NAMES_H_
typo: COMPONENTS_....

https://codereview.chromium.org/1296663003/diff/200001/components/proxy_confi...
components/proxy_config/proxy_config_pref_names.h:10: namespace ProxyPrefs {
Isn't Chromium style to use lower-case namespace?
Perhaps this should be proxy_config (to match the component name), or
proxy_prefs

https://codereview.chromium.org/1296663003/diff/200001/components/proxy_confi...
components/proxy_config/proxy_config_pref_names.h:11: namespace prefs {
why keep the prefs inner namespace? Although I see other components doing
similarly, so maybe fine for consistency.

[Abhishek, 2015-08-27 06:38:16]

I've updated the patch and changed namespace to proxy_config.

PTAL #12!

https://codereview.chromium.org/1296663003/diff/200001/components/proxy_confi...
File components/proxy_config/proxy_config_pref_names.h (right):

https://codereview.chromium.org/1296663003/diff/200001/components/proxy_confi...
components/proxy_config/proxy_config_pref_names.h:5: #ifndef
COMPONETS_PROXY_CONFIG_PROXY_CONFIG_PREF_NAMES_H_
On 2015/08/27 01:16:33, eroman wrote:
> typo: COMPONENTS_....

Done.

https://codereview.chromium.org/1296663003/diff/200001/components/proxy_confi...
components/proxy_config/proxy_config_pref_names.h:10: namespace ProxyPrefs {
On 2015/08/27 01:16:33, eroman wrote:
> Isn't Chromium style to use lower-case namespace?
> Perhaps this should be proxy_config (to match the component name), or
I feel it would be good if we keep it "proxy_config" as you mentioned to match
component name.

I'll make the necessary changes.

> proxy_prefs

https://codereview.chromium.org/1296663003/diff/200001/components/proxy_confi...
components/proxy_config/proxy_config_pref_names.h:11: namespace prefs {
On 2015/08/27 01:16:33, eroman wrote:
> why keep the prefs inner namespace? Although I see other components doing
> similarly, so maybe fine for consistency.

Yeah! I also think the same. For consistency it would be good.

[eroman, 2015-08-27 21:51:48]

patchset 12, LGTM

[Abhishek, 2015-08-28 05:51:49]

abhishek.a21@samsung.com changed reviewers:
+ anthonyvd@chromium.org, avi@chromium.org, battre@chromium.org,
cbentzel@chromium.org, finnur@chromium.org, jochen@chromium.org,
phajdan.jr@chromium.org

[Abhishek, 2015-08-28 05:51:50]

+battre c/b/prefs/
+finnur c/b/extensions/
+anthonyvd c/b/profiles/
+phajdan.jr c/test/
+avi c/b/renderer_context_menu/
+cbentzel c/browser/io_thread.cc
+jochen c/browser/BUILD.gn

PTAL #12!

[battre, 2015-08-28 06:40:38]

lgtm

[blundell, 2015-08-28 08:41:21]

On 2015/08/28 05:51:50, Abhishek wrote:
> +battre c/b/prefs/
> +finnur c/b/extensions/
> +anthonyvd c/b/profiles/
> +phajdan.jr c/test/
> +avi c/b/renderer_context_menu/
> +cbentzel c/browser/io_thread.cc
> +jochen c/browser/BUILD.gn
> 
> PTAL #12!


All of the //chrome changes outside of //chrome/browser/net fall under the
umbrella of "refactoring changes that are mechanical in nature"
(https://www.chromium.org/developers/owners-files#TOC-When-to-use-To-Be-Review...).
Rather than having all the individual OWNERS review, I suggest you TBR thakis@
as //chrome OWNER for them.

I looked at all of those changes, and they lgtm

[Abhishek, 2015-08-28 08:58:04]

On 2015/08/28 08:41:21, blundell wrote:
> On 2015/08/28 05:51:50, Abhishek wrote:
> > +battre c/b/prefs/
> > +finnur c/b/extensions/
> > +anthonyvd c/b/profiles/
> > +phajdan.jr c/test/
> > +avi c/b/renderer_context_menu/
> > +cbentzel c/browser/io_thread.cc
> > +jochen c/browser/BUILD.gn
> > 
> > PTAL #12!
> 
> 
> All of the //chrome changes outside of //chrome/browser/net fall under the
> umbrella of "refactoring changes that are mechanical in nature"
>
(https://www.chromium.org/developers/owners-files#TOC-When-to-use-To-Be-Review...).
> Rather than having all the individual OWNERS review, I suggest you TBR thakis@
> as //chrome OWNER for them.
> 
> I looked at all of those changes, and they lgtm

Thanks!

[Abhishek, 2015-08-28 08:58:35]

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

[Abhishek, 2015-08-28 08:58:47]

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

[Abhishek, 2015-08-28 08:58:48]

The patchset sent to the CQ was uploaded after l-g-t-m from droger@chromium.org,
stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/1296663003/#ps220001
(title: "changed ProxyPrefs -> proxy_config")

[commit-bot: I haz the power, 2015-08-28 08:59:03]

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

[blundell, 2015-08-28 09:09:55]

blundell@chromium.org changed reviewers:
+ blundell@chromium.org
- anthonyvd@chromium.org, atwilson@chromium.org, avi@chromium.org,
cbentzel@chromium.org, finnur@chromium.org, jochen@chromium.org,
phajdan.jr@chromium.org

[blundell, 2015-08-28 09:09:56]

-all the now-superfluous reviewers

TBR=thakis for //chrome outside of //chrome/browser/net

(When you TBR someone, always make sure to let them know what you're actually
TBR'ing them for so they know what to look at)

[commit-bot: I haz the power, 2015-08-28 09:13:44]

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

[commit-bot: I haz the power, 2015-08-28 09:13:46]

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

[Abhishek, 2015-08-28 11:49:45]

@eroman / blundell:
Could you please confirm namespace change?

https://codereview.chromium.org/1296663003/diff/220001/components/proxy_confi...
File components/proxy_config/proxy_config_pref_names.h (right):

https://codereview.chromium.org/1296663003/diff/220001/components/proxy_confi...
components/proxy_config/proxy_config_pref_names.h:10: namespace proxy_config {
I feel this namespace change is not exported to chromeos or not visible to it.

[blundell, 2015-08-28 11:56:35]

On 2015/08/28 11:49:45, Abhishek wrote:
> @eroman / blundell:
> Could you please confirm namespace change?
> 
>
https://codereview.chromium.org/1296663003/diff/220001/components/proxy_confi...
> File components/proxy_config/proxy_config_pref_names.h (right):
> 
>
https://codereview.chromium.org/1296663003/diff/220001/components/proxy_confi...
> components/proxy_config/proxy_config_pref_names.h:10: namespace proxy_config {
> I feel this namespace change is not exported to chromeos or not visible to it.

The problem is occurring because there is a chromeos::proxy_config namespace. If
already in the chromeos namespace, then proxy_config::prefs gets interpreted as
chromeos::proxy_config::prefs. The way to fix it is to use ::proxy_config::prefs
in that context to tell the compiler that you're actually talking about a
top-level proxy_configs namespace, not a nested proxy_config namespace.

It looks like you also need to disable warning 4267 on Windows. Look at e.g.
what //components/keyed_service.gypi does (grep for "4267") and what
//components/keyed_service/core/BUILD.gn does (grep for
"no_size_t_to_int_warning"), and copy them over to your gypi/GN files.

[Abhishek, 2015-08-28 12:15:13]

On 2015/08/28 11:56:35, blundell wrote:
> On 2015/08/28 11:49:45, Abhishek wrote:
> > @eroman / blundell:
> > Could you please confirm namespace change?
> > 
> >
>
https://codereview.chromium.org/1296663003/diff/220001/components/proxy_confi...
> > File components/proxy_config/proxy_config_pref_names.h (right):
> > 
> >
>
https://codereview.chromium.org/1296663003/diff/220001/components/proxy_confi...
> > components/proxy_config/proxy_config_pref_names.h:10: namespace proxy_config
{
> > I feel this namespace change is not exported to chromeos or not visible to
it.
> 
> The problem is occurring because there is a chromeos::proxy_config namespace.
If
> already in the chromeos namespace, then proxy_config::prefs gets interpreted
as
> chromeos::proxy_config::prefs. The way to fix it is to use
::proxy_config::prefs
> in that context to tell the compiler that you're actually talking about a
> top-level proxy_configs namespace, not a nested proxy_config namespace.
> 
> It looks like you also need to disable warning 4267 on Windows. Look at e.g.
> what //components/keyed_service.gypi does (grep for "4267") and what
> //components/keyed_service/core/BUILD.gn does (grep for
> "no_size_t_to_int_warning"), and copy them over to your gypi/GN files.

Thanks for your quick response!

I'll check and apply all the changes and upload a patch soon.

[Avi (use Gerrit), 2015-08-28 14:44:01]

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

[Avi (use Gerrit), 2015-08-28 14:44:02]

c/b/renderer_context_menu/ lgtm

[Abhishek, 2015-08-31 10:13:30]

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

[Abhishek, 2015-08-31 10:13:30]

The patchset sent to the CQ was uploaded after l-g-t-m from droger@chromium.org,
stevenjb@chromium.org, avi@chromium.org, eroman@chromium.org,
battre@chromium.org, blundell@chromium.org
Link to the patchset: https://codereview.chromium.org/1296663003/#ps260001
(title: "updating for win p/f")

[commit-bot: I haz the power, 2015-08-31 10:13:41]

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

[commit-bot: I haz the power, 2015-08-31 10:48:30]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2015-08-31 10:49:05]

Patchset 14 (id:??) landed as
https://crrev.com/71c612855d763ffbb195dd7b927dfeb91eee0326
Cr-Commit-Position: refs/heads/master@{#346369}