Move some proxy config code out of src/chrome

Move some proxy config code out of src/chrome

BUG=658015
Committed: https://crrev.com/100c620bb32532fce5683c052cdfebc141e18df3
Cr-Commit-Position: refs/heads/master@{#428257}

[stevenjb, 2016-10-24 23:57:23]

Patchset #3 (id:40001) has been deleted

[stevenjb, 2016-10-25 00:00:55]

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

[commit-bot: I haz the power, 2016-10-25 00:02:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[stevenjb, 2016-10-25 00:05:47]

stevenjb@chromium.org changed reviewers:
+ battre@chromium.org, mnissler@chromium.org

[stevenjb, 2016-10-25 00:05:48]

mnissler@ - This CL mostly just moves chromeos proxy code from src/chrome to
src/chromeos. Note, UIProxyConfig is not well named and isn't really UI code,
just some glue code that converts pref code between net::ProxyConfig, the Pref
Store, and a Dictionary compatible with the Network Config code.

[commit-bot: I haz the power, 2016-10-25 00:06:09]

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

[commit-bot: I haz the power, 2016-10-25 00:06:10]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[stevenjb, 2016-10-25 00:08:00]

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

[stevenjb, 2016-10-25 00:08:01]

+mmenke@ for chrome/browser/ui/webui/net_internals/

[stevenjb, 2016-10-25 00:23:44]

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

[commit-bot: I haz the power, 2016-10-25 00:24:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-25 00:28:35]

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

[commit-bot: I haz the power, 2016-10-25 00:28:36]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[battre, 2016-10-25 08:26:17]

lgtm

[mmenke, 2016-10-25 09:33:14]

https://codereview.chromium.org/2442313003/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (left):

https://codereview.chromium.org/2442313003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:89: #include
"chrome/browser/chromeos/net/onc_utils.h"
This header looks like it's needed for
chromeos::onc::ParseAndValidateOncForImport

[Mattias Nissler (ping if slow), 2016-10-25 09:53:17]

Code move looks fine, however I'm not sure whether it's OK to pull in the
user_manager dependency into chromeos/network?

Regarding UIProxyConfig naming, I have a faint memory of this being the model
object being used by the UI. The UI prefix was justified by the fact that it
contains data corresponding to the UI data model, which doesn't map cleanly to
ProxyConfig representations as used in net/

https://codereview.chromium.org/2442313003/diff/80001/chromeos/network/DEPS
File chromeos/network/DEPS (right):

https://codereview.chromium.org/2442313003/diff/80001/chromeos/network/DEPS#n...
chromeos/network/DEPS:7: "+components/user_manager",
Do you really want the user_manager dependency here (see also other comments)?

https://codereview.chromium.org/2442313003/diff/80001/chromeos/network/onc/on...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/2442313003/diff/80001/chromeos/network/onc/on...
chromeos/network/onc/onc_utils.cc:67: explicit UserStringSubstitution(const
user_manager::User* user)
Not sure whether this use of User justifies pulling in the UserManager
dependency. We could just as well pass in account name and email?

https://codereview.chromium.org/2442313003/diff/80001/chromeos/network/onc/on...
chromeos/network/onc/onc_utils.cc:1262:
user_manager::UserManager::Get()->GetActiveUser();
Ah, so here there are more references to UserManager. AFAICT, we only really
need username_hash for the vast majority of cases?

https://codereview.chromium.org/2442313003/diff/80001/chromeos/network/onc/on...
File chromeos/network/onc/onc_utils.h (right):

https://codereview.chromium.org/2442313003/diff/80001/chromeos/network/onc/on...
chromeos/network/onc/onc_utils.h:172: // |hashed_username|.
What does |hashed_username| refer to?

[stevenjb, 2016-10-25 17:26:22]

https://codereview.chromium.org/2442313003/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (left):

https://codereview.chromium.org/2442313003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:89: #include
"chrome/browser/chromeos/net/onc_utils.h"
On 2016/10/25 09:33:14, mmenke wrote:
> This header looks like it's needed for
> chromeos::onc::ParseAndValidateOncForImport

chrome/browser/chromeos/net/onc_utils.h has been merged into
chromeos/network/onc/onc_utils.h.

https://codereview.chromium.org/2442313003/diff/80001/chromeos/network/DEPS
File chromeos/network/DEPS (right):

https://codereview.chromium.org/2442313003/diff/80001/chromeos/network/DEPS#n...
chromeos/network/DEPS:7: "+components/user_manager",
On 2016/10/25 09:53:16, Mattias Nissler (ping if slow) wrote:
> Do you really want the user_manager dependency here (see also other comments)?

chromeos/ already depends on user_manager in login/ and cryptohome/, so this
doesn't add any real dependencies.

https://codereview.chromium.org/2442313003/diff/80001/chromeos/network/onc/on...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/2442313003/diff/80001/chromeos/network/onc/on...
chromeos/network/onc/onc_utils.cc:67: explicit UserStringSubstitution(const
user_manager::User* user)
On 2016/10/25 09:53:16, Mattias Nissler (ping if slow) wrote:
> Not sure whether this use of User justifies pulling in the UserManager
> dependency. We could just as well pass in account name and email?

As already noted, the UserManager dependency already exists. Also, that
dependency will become more lightweight soon with
https://codereview.chromium.org/2438063002/.

https://codereview.chromium.org/2442313003/diff/80001/chromeos/network/onc/on...
chromeos/network/onc/onc_utils.cc:1262:
user_manager::UserManager::Get()->GetActiveUser();
On 2016/10/25 09:53:16, Mattias Nissler (ping if slow) wrote:
> Ah, so here there are more references to UserManager. AFAICT, we only really
> need username_hash for the vast majority of cases?

It's possible that this could be re-factored to remove the UserManager
dependency but (again since it already exists) I would just as soon not do that
here.

https://codereview.chromium.org/2442313003/diff/80001/chromeos/network/onc/on...
File chromeos/network/onc/onc_utils.h (right):

https://codereview.chromium.org/2442313003/diff/80001/chromeos/network/onc/on...
chromeos/network/onc/onc_utils.h:172: // |hashed_username|.
On 2016/10/25 09:53:16, Mattias Nissler (ping if slow) wrote:
> What does |hashed_username| refer to?

Fixed.

[mmenke, 2016-10-25 17:28:28]

chrome/browser/ui/webui/net_internals/net_internals_ui.cc LGTM

https://codereview.chromium.org/2442313003/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (left):

https://codereview.chromium.org/2442313003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:89: #include
"chrome/browser/chromeos/net/onc_utils.h"
On 2016/10/25 17:26:22, stevenjb wrote:
> On 2016/10/25 09:33:14, mmenke wrote:
> > This header looks like it's needed for
> > chromeos::onc::ParseAndValidateOncForImport
> 
> chrome/browser/chromeos/net/onc_utils.h has been merged into
> chromeos/network/onc/onc_utils.h.

Sorry, the duplicate file name confused me.

[stevenjb, 2016-10-26 00:22:33]

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

[stevenjb, 2016-10-26 00:22:33]

+emaxx@, can you review this too? This is more re-factoring, but this actually
moves some significant bits from src/chrome to src/chromeos.

Note: components/user_manager is actually intended to be a lightweight helper
component, and src/chromeos already depends on it (it's just not in the top
level DEPS file which we try to keep pretty small). It currently depends on
components/session_manager (which is not so lightweight), but that dependency is
going away soon (see https://codereview.chromium.org/2438063002/).

[stevenjb, 2016-10-26 22:21:58]

Rebased after previous CL was committed.
emaxx@ - waiting on your review, thanks.

[stevenjb, 2016-10-26 22:22:23]

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

[commit-bot: I haz the power, 2016-10-26 22:23:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 22:37:57]

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

[commit-bot: I haz the power, 2016-10-26 22:37:58]

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

[emaxx, 2016-10-27 03:22:04]

lgtm

[stevenjb, 2016-10-27 18:53:17]

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

[commit-bot: I haz the power, 2016-10-27 18:53:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[stevenjb, 2016-10-27 18:57:14]

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

[stevenjb, 2016-10-27 18:57:15]

+atwilson@ - I needed to modify the components/policy GN config to avoid the
following circular dependency in the component build. Can you take a look at
components/policy/core/common/BUILD.gn and chrome/common/BUILD.gn? Thanks!

ERROR Dependency cycle:
  //ui/base:base ->
  //chromeos:chromeos ->
  //components/policy/core/common:common ->
* //components/policy:policy ->
  //components/policy/core/browser:internal ->
  //ui/base:base

* We should be able to depend on components/policy/core/common:common without
pulling in all of components/policy:policy.

[commit-bot: I haz the power, 2016-10-27 19:51:54]

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

[commit-bot: I haz the power, 2016-10-27 19:51:55]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[stevenjb, 2016-10-27 20:22:31]

Patchset #8 (id:160001) has been deleted

[stevenjb, 2016-10-27 20:22:42]

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

[commit-bot: I haz the power, 2016-10-27 20:23:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[stevenjb, 2016-10-27 20:23:33]

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

[stevenjb, 2016-10-27 20:23:34]

+xiyuan@ for chromeos/network/DEPS change to include user_manager. (Note: other
chromeos/ code already includes user_manager).

[xiyuan, 2016-10-27 20:35:56]

On 2016/10/27 20:23:34, stevenjb wrote:
> +xiyuan@ for chromeos/network/DEPS change to include user_manager. (Note:
other
> chromeos/ code already includes user_manager).

LGTM

Think this is okay.

[commit-bot: I haz the power, 2016-10-27 21:41:25]

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

[commit-bot: I haz the power, 2016-10-27 21:41:26]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[stevenjb, 2016-10-27 23:52:41]

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

[stevenjb, 2016-10-27 23:52:42]

+michaelpg@ FYI

[stevenjb, 2016-10-28 00:32:10]

stevenjb@chromium.org changed reviewers:
- atwilson@chromium.org

[stevenjb, 2016-10-28 00:32:11]

-atwilson@ - Nevermind, it turns out we don't actually need the policy
dependency (and the change I made broke win_clang for reasons I can't decipher).

[stevenjb, 2016-10-28 00:32:16]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2016-10-28 00:32:17]

The patchset sent to the CQ was uploaded after l-g-t-m from battre@chromium.org,
mmenke@chromium.org, emaxx@chromium.org, xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/2442313003/#ps200001
(title: "Remove unnecessary policy/core/common dependency.")

[commit-bot: I haz the power, 2016-10-28 00:32:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-28 00:47:35]

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

[commit-bot: I haz the power, 2016-10-28 00:47:38]

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

[stevenjb, 2016-10-28 01:01:14]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2016-10-28 01:01:15]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
emaxx@chromium.org, mmenke@chromium.org, battre@chromium.org
Link to the patchset: https://codereview.chromium.org/2442313003/#ps220001
(title: "Fix DEPS")

[commit-bot: I haz the power, 2016-10-28 01:01:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-28 02:28:27]

Committed patchset #10 (id:220001)

[commit-bot: I haz the power, 2016-10-28 02:30:21]

Description was changed from

==========
Move some proxy config code out of src/chrome

BUG=658015
==========

to

==========
Move some proxy config code out of src/chrome

BUG=658015

Committed: https://crrev.com/100c620bb32532fce5683c052cdfebc141e18df3
Cr-Commit-Position: refs/heads/master@{#428257}
==========

[commit-bot: I haz the power, 2016-10-28 02:30:23]

Patchset 10 (id:??) landed as
https://crrev.com/100c620bb32532fce5683c052cdfebc141e18df3
Cr-Commit-Position: refs/heads/master@{#428257}