Adding policy support to the new network configuration stack.

Adding policy support to the new network configuration stack.

Adapts in particular the ManagedNetworkConfigurationHandler, the
networkingPrivate extension API and the network configuration extension.

BUG=223869
TBR=thestig@chromium.org (for chrome_browser_chromeos.gypi)

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195267

[pneubeck (no reviews), 2013-03-22 17:01:39]

Hi Greg,
just a pre-pre-preview FYI :-)

Most action is in managed_network_configuration_handler.cc
It can already apply policies, but the current Shill API is still holding me
back.

[Greg Spencer (Chromium), 2013-04-10 01:14:15]

https://codereview.chromium.org/12676017/diff/8003/chromeos/network/managed_n...
File chromeos/network/managed_network_configuration_handler.cc (right):

https://codereview.chromium.org/12676017/diff/8003/chromeos/network/managed_n...
chromeos/network/managed_network_configuration_handler.cc:121: void DoNothing()
{
You can use base::DoNothing() instead of this.

https://codereview.chromium.org/12676017/diff/8003/chromeos/network/managed_n...
chromeos/network/managed_network_configuration_handler.cc:374: // properties
_might_ be user configured.
Ugh.  Doesn't the UIData have a version number?

https://codereview.chromium.org/12676017/diff/8003/chromeos/network/managed_n...
chromeos/network/managed_network_configuration_handler.cc:409: void
TranslatePropertiesAndRunCallback(
Consolidate this into the anonymous namespace at the top.

https://codereview.chromium.org/12676017/diff/8003/chromeos/network/managed_n...
chromeos/network/managed_network_configuration_handler.cc:872: (new
PolicyApplicator(weak_ptr_factory_.GetWeakPtr(),
Maybe a local variable instead of just newwing it up?  I know they're all owned
and managed by the callbacks created with Bind, but at least a note about
lifetime/ownership is probably in order here.

https://codereview.chromium.org/12676017/diff/8003/chromeos/network/network_u...
File chromeos/network/network_ui_data.h (right):

https://codereview.chromium.org/12676017/diff/8003/chromeos/network/network_u...
chromeos/network/network_ui_data.h:87: // Key for storing thecertificate type.
thecertificate --> the certificate

https://codereview.chromium.org/12676017/diff/8003/chromeos/network/onc/onc_m...
File chromeos/network/onc/onc_merger.cc (right):

https://codereview.chromium.org/12676017/diff/8003/chromeos/network/onc/onc_m...
chromeos/network/onc/onc_merger.cc:339: // Prevent that credentials are
forwarded in cleartext to UI. User/shared
Prevent that credentials are --> Prevent credentials from being

https://codereview.chromium.org/12676017/diff/8003/chromeos/network/onc/onc_m...
chromeos/network/onc/onc_merger.cc:401: const OncValueSignature* signature_;
DISALLOW_COPY_AND_ASSIGN?  And for other classes here too?

[pneubeck (no reviews), 2013-04-11 12:14:51]

Hi Steven, could you take a look at the dbus/*client changes?

Greg will look at the more UI/JS related parts.
Julian does an overall look at the structure and policy application.

Thanks!

[pastarmovj, 2013-04-11 14:46:29]

Almost there :)

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/networking_private_api.cc (right):

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/networking_private_api.cc:151: // The
|network_guid| parameter is storing the service path.
Is there any other reason to create a temp here other than documenting the code
(which could be done inplace as well below)?

This comment applies for NetworkingPrivateGetManagedPropertiesFunction::RunImpl
as well.

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/networking_private_api.h (right):

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/networking_private_api.h:55: // Callbacks if
talking to ManagedNetworkConfigurationHandler.
This is a little vague - are those callbacks called by the MNCH?

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/network_configuration_updater2.cc (right):

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater2.cc:60: <<
chromeos::onc::GetSourceAsString(onc_source);
Align.

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater2.cc:73: if
(policy_value == NULL) {
!policy_value is preferred.

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater2.cc:91: if
(onc_dict.get() == NULL) {
!onc_dict works for scoped_ptr.

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/network_configuration_updater2.h (right):

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater2.h:27: class
NetworkConfigurationUpdater2 {
Please make NetworkConfigurationUpdater2 and etworkConfigurationUpdater have a
common base class and then make the BrowserPolicyConnector return one or the
other based on the CommandLine switch to avoid having to duplicate the check all
over the code.

Also make sure to adjust includes accordingly wherever needed.

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/resources/...
File
chrome/browser/resources/chromeos/network_configuration/js/network_config.js
(right):

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/resources/...
chrome/browser/resources/chromeos/network_configuration/js/network_config.js:67:
if (Object.keys(result).length)
This is inconsistent. Either use curlies around single line if/else-s or not.
I'd prefer not if it is fine with the JS style guide - couldn't fine anything
about that.

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/resources/...
File
chrome/browser/resources/chromeos/network_configuration/js/network_status.js
(right):

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/resources/...
chrome/browser/resources/chromeos/network_configuration/js/network_status.js:625:
console.log('NetworkStatus: Network ' + id + ' added.');
Is this for you for debugging or is it intended to stay in the final code?

https://codereview.chromium.org/12676017/diff/23001/chromeos/dbus/shill_manag...
File chromeos/dbus/shill_manager_client.cc (right):

https://codereview.chromium.org/12676017/diff/23001/chromeos/dbus/shill_manag...
chromeos/dbus/shill_manager_client.cc:34: // DEPRECATED: Keep this only for
backward compatibility with NetworkLibrary.
Put a todo here to remove that once not needed and make a bug for that
referenced from the TODO - that way this will not be code rotting here forever.

https://codereview.chromium.org/12676017/diff/23001/chromeos/dbus/shill_manag...
chromeos/dbus/shill_manager_client.cc:175:
DCHECK(AreServicePropertiesValid2(properties));
DCHECK will only verify in debug builds - does your code below handle this
situation correctly - if not you should either CHECK or exit early here.

https://codereview.chromium.org/12676017/diff/23001/chromeos/dbus/shill_manag...
File chromeos/dbus/shill_manager_client.h (right):

https://codereview.chromium.org/12676017/diff/23001/chromeos/dbus/shill_manag...
chromeos/dbus/shill_manager_client.h:14: #include
"chromeos/dbus/dbus_method_call_status.h"
I think forward declarations should be ok since you only use references to those
types here right?

https://codereview.chromium.org/12676017/diff/23001/chromeos/dbus/shill_profi...
File chromeos/dbus/shill_profile_client.cc (right):

https://codereview.chromium.org/12676017/diff/23001/chromeos/dbus/shill_profi...
chromeos/dbus/shill_profile_client.cc:53: return NULL;
Maybe put a NOTREACHED() here too to make sure this code is not called at all.

https://codereview.chromium.org/12676017/diff/23001/chromeos/dbus/shill_profi...
File chromeos/dbus/shill_profile_client_stub.h (right):

https://codereview.chromium.org/12676017/diff/23001/chromeos/dbus/shill_profi...
chromeos/dbus/shill_profile_client_stub.h:54: // Note: This should remain the
last member so it'll be destroyed and
This comment is wrong for this class!

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
File chromeos/network/managed_network_configuration_handler.cc (right):

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:64: PROFILE_NONE,  //
Not in any profile.
nit: Please align the comments for easier readability.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:72: const char
kFakeCredential[] = "FAKE_CREDENTIAL";
It's highly unlikely but this can never be used as a password so it should be a
value that is not a valid password either. What about an empty string? You can
modify it on the UI side if you want to show something else whenever this field
is empty.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:138: void
LogError(const std::string& error) {
Why would you do that?

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:151:
onc_object->GetWithoutPathExpansion(field_name, &value);
Why not use it.value() here?

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:264: if (policy_type
!= network_type)
Please be consistent in the file if you use or not use curlies around single
line statements. The style guide allows both but mixing them in one file is not
really nice.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:286: it !=
policies.end();
combine those two lines.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:349:
flimflam::kProfileProperty,
Combine those two lines better.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:464: false,  // Ignore
missing fields.
Almost aligned here.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:598:
remaining_policies_.swap(*modified_policies);
Hmm is this safe in all cases?

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:665: &old_guid)) {
align.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:666: LOG(WARNING) <<
"Entry " << entry << " of profile " << profile_path_
align.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:712: if (new_policy) {
Combine those ifs they are the same.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:834: LOG(ERROR) <<
"ONC from " << onc::GetSourceAsString(onc_source)
It there a chance you are leaking here infromation to the logs that is privacy
sensitive?

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:892: (new
PolicyApplicator(weak_ptr_factory_.GetWeakPtr(),
That a little bit too function-programming-y for C++. Please create a pointer
variable and then call Run on it to make this look more C++.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
File chromeos/network/managed_network_configuration_handler_unittest.cc (right):

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler_unittest.cc:56:
ManagedNetworkConfigurationHandlerTest() {
move {} on a single line.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler_unittest.cc:140:
SetPolicy(onc::ONC_SOURCE_USER_POLICY, "policy/policy_wifi1.onc");
Is this a file? You need to include any files needed by unit tests in the
isolates lists. I haven't done this but i know it is important if you want your
test to run on the sharded test bots. :)

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/network_...
File chromeos/network/network_configuration_handler.cc (right):

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/network_...
chromeos/network/network_configuration_handler.cc:202: if (type ==
flimflam::kTypeWifi) {
Why is wify so special here? What about other tpyes of networks like mobile or
WiMax?

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/network_...
File chromeos/network/network_ui_data.h (right):

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/network_...
chromeos/network/network_ui_data.h:87: // Key for storing thecertificate type.
the<space>cer...

[stevenjb, 2013-04-11 18:20:07]

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/networking_private_api.h (right):

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/networking_private_api.h:55: // Callbacks if
talking to ManagedNetworkConfigurationHandler.
On 2013/04/11 14:46:29, pastarmovj wrote:
> This is a little vague - are those callbacks called by the MNCH? 
How about:
Callbacks for ManagedNetworkConfigurationHandler::GetManagedProperties

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/networking_private_api.h:59:
scoped_ptr<base::DictionaryValue> error_data);
nit: blank line

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/networking_private_apitest.cc (right):

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/networking_private_apitest.cc:158:
std::string uidata_blob =
nit: const std::string

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/networking_private_apitest.cc:166: "}";
nit: maybe use less space by not putting {} on their own lines? WS in the blob
shouldn't matter, should it?

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/networking_private_apitest.cc:187:
std::string user_policy_blob =
Same nits as above.

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/network_configuration_updater2.cc (right):

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater2.cc:67: }
nit: No {}

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater2.cc:82: } else {
No else after return.

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/network_configuration_updater2.h (right):

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater2.h:22: // profiles.
Requests the NetworkLibrary to apply the ONC of the network
This version doesn't use NetworkLibrary, right?

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater2.h:27: class
NetworkConfigurationUpdater2 {
On 2013/04/11 14:46:29, pastarmovj wrote:
> Please make NetworkConfigurationUpdater2 and etworkConfigurationUpdater have a
> common base class and then make the BrowserPolicyConnector return one or the
> other based on the CommandLine switch to avoid having to duplicate the check
all
> over the code.
> 
> Also make sure to adjust includes accordingly wherever needed.

Agreed that we should use a pure virtual base class. Then we can use a static,
Create() method which does the command line check and constructs the correct
implementation. 

I also do not like "2" as a name differentiator. Lets move the existing
implementation to something like NetworkConfigurationUpdaterImplCros, and the
new one just NetworkConfigurationUpdaterImpl.

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/extensions...
File chrome/browser/extensions/api/test/test_api.cc (right):

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/extensions...
chrome/browser/extensions/api/test/test_api.cc:73: LOG(ERROR) << message;
Revert to VLOG before committing.

https://codereview.chromium.org/12676017/diff/23001/chromeos/dbus/shill_manag...
File chromeos/dbus/shill_manager_client.cc (right):

https://codereview.chromium.org/12676017/diff/23001/chromeos/dbus/shill_manag...
chromeos/dbus/shill_manager_client.cc:25: bool AreServicePropertiesValid2(const
base::DictionaryValue& properties) {
2 is a poor differentiator. Maybe ForProfile or WithMode?

https://codereview.chromium.org/12676017/diff/23001/chromeos/dbus/shill_profi...
File chromeos/dbus/shill_profile_client.cc (right):

https://codereview.chromium.org/12676017/diff/23001/chromeos/dbus/shill_profi...
chromeos/dbus/shill_profile_client.cc:53: return NULL;
On 2013/04/11 14:46:29, pastarmovj wrote:
> Maybe put a NOTREACHED() here too to make sure this code is not called at all.

Actually, I think it's better (easier to debug) if the code  crashes where the
test interface is accessed.

https://codereview.chromium.org/12676017/diff/23001/chromeos/dbus/shill_profi...
File chromeos/dbus/shill_profile_client.h (right):

https://codereview.chromium.org/12676017/diff/23001/chromeos/dbus/shill_profi...
chromeos/dbus/shill_profile_client.h:52: virtual bool AddService(const
std::string& service_path) = 0;
nit: blank line

https://codereview.chromium.org/12676017/diff/23001/chromeos/dbus/shill_profi...
chromeos/dbus/shill_profile_client.h:54: ~TestInterface() {}
virtual

https://codereview.chromium.org/12676017/diff/23001/chromeos/dbus/shill_profi...
chromeos/dbus/shill_profile_client.h:96: virtual TestInterface*
GetTestInterface() = 0;
blank line

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
File chromeos/network/managed_network_configuration_handler.cc (right):

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:103: }
nit: no {}

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:135: const
std::string& error_message) {
Do we really want to ignore these error messages instead of logging them, at
least as WARNING? We shouldn't normally be getting errors, should we?

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:138: void
LogError(const std::string& error) {
This appears to be unused.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:151:
onc_object->GetWithoutPathExpansion(field_name, &value);
On 2013/04/11 14:46:29, pastarmovj wrote:
> Why not use it.value() here?
value() returns a const&, and RemoveFakeCredentials may modify
value/nested_object. Maybe add a comment to that effect.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:157: if
(value->GetAsDictionary(&nested_object)) {
This is kind of confusing. While more verbose, I would prefer to see:

if (value->IsType(TYPE_DICTIONARY)) {
  base::DictionaryValue* nested_object;
  DCHECK(value->GetAsDictionary(&nested_object));
  ...
} else if (value->IsType(TYPE_STRING) {
  ...
}

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:192: user_settings); 
// shared settings
nit: Name 'settings' since may be user or shared?

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:228: }
nit: no {}

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:252: // Returns if
|policy| matches |onc_network_part|. This is should be the only
s/Returns/Returns true/

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:264: if (policy_type
!= network_type)
On 2013/04/11 14:46:29, pastarmovj wrote:
> Please be consistent in the file if you use or not use curlies around single
> line statements. The style guide allows both but mixing them in one file is
not
> really nice.
Also while the style guide allows either, historically Chrome prefers not using
{} for single line statements in the generally interest of using less vertical
space.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:272: &policy_ssid);
nit: one line?

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:282: const
std::map<std::string, const base::DictionaryValue*> &policies,
Use ManagedNetworkConfigurationHandler::PolicyMap

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:286: it !=
policies.end();
On 2013/04/11 14:46:29, pastarmovj wrote:
> combine those two lines.
In fact, I would suggest placing both line 286 and 287 on 285.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:407: namespace {
nit: I'd combine this with the anon namesapce at the top of the file.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:482: return;
nit: Put error clause with early exit above warning clause, with no else.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:598:
remaining_policies_.swap(*modified_policies);
On 2013/04/11 14:46:29, pastarmovj wrote:
> Hmm is this safe in all cases?
This is confusing with a member and a scoped_ptr. I think it would be less
confusing with a raw * here and a a non * local in the calling function.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:606: this),
nit: combine the above 3 lines?

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:624: &entries)) {
nit: combine lines

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:854: new
std::set<std::string>());
As noted above, this should just be 'std::set<std::string> modified_policies.
Also, declare it just above 'if (network_configurations)', i.e. jut before it is
needed. It's confusing here since it has nothing to do with the next statement.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:904: }
nit: no {}

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
File chromeos/network/managed_network_configuration_handler.h (right):

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.h:121: // Only to be
called by NetworkConfigurationUpdater.
Also describe what the method does.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.h:142:
scoped_ptr<PolicyMap> device_policies_by_guid_;
This is pretty confusing; it's a pointer to a map of pointers.

Do we actually need to pass ownership of these maps elsewhere, or could we just
make these maps, and convert:

user_policies_by_guid_ = GetFoo(..)
to
SetFoo(..., &user_policies_by_guid_);

And

!user_policies_by_guid_ to user_policies_by_guid_.empty()?

Or do we need to differentiate between an empty map and an unset one?

There is also the question of who owns the Dictionary*'s in the maps. A scoped
pointer implies that destroying the member will clean up any contents, but it
appears that is not the case. If we need to differentiate between empty and
NULL, then I would use raw *'s here, not scoped_ptr.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
File chromeos/network/managed_network_configuration_handler_unittest.cc (right):

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler_unittest.cc:36: namespace
{
nit: blank line after namespace

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler_unittest.cc:42: }
nit: blank line before, and  // namespace

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler_unittest.cc:50: }
Put in anon namespace

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler_unittest.cc:56:
ManagedNetworkConfigurationHandlerTest() {
On 2013/04/11 14:46:29, pastarmovj wrote:
> move {} on a single line.
Or be consistent and put } on separate line for destructor (I prefer that when
the rest of the class is implemented inline).

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler_unittest.cc:109: }
nit: no {}

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler_unittest.cc:122:
MessageLoop message_loop_;
DISALLOW...

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/network_...
File chromeos/network/network_configuration_handler.cc (right):

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/network_...
chromeos/network/network_configuration_handler.cc:202: if (type ==
flimflam::kTypeWifi) {
On 2013/04/11 14:46:29, pastarmovj wrote:
> Why is wify so special here? What about other tpyes of networks like mobile or
> WiMax?
+1. Can't we have proxy config for a network in the user profile?

Regardless, shouldn't we check on whether the profile property exists and is non
empty as to whether or not to call ConfigureServiceForProfile?

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/network_...
File chromeos/network/network_ui_data.h (right):

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/network_...
chromeos/network/network_ui_data.h:64: base::DictionaryValue* user_settings() {
const and return const* (or provide both)

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/network_...
chromeos/network/network_ui_data.h:70: const std::string& policy_guid() {
const

[pneubeck (no reviews), 2013-04-15 12:16:23]

Thanks for all the comments!
I addressed most issues. In a few cases, I have a counter question.

The isolation of the unit test is still to be done.

https://codereview.chromium.org/12676017/diff/8003/chromeos/network/managed_n...
File chromeos/network/managed_network_configuration_handler.cc (right):

https://codereview.chromium.org/12676017/diff/8003/chromeos/network/managed_n...
chromeos/network/managed_network_configuration_handler.cc:121: void DoNothing()
{
On 2013/04/10 01:14:15, Greg Spencer (Chromium) wrote:
> You can use base::DoNothing() instead of this.

Done.

https://codereview.chromium.org/12676017/diff/8003/chromeos/network/managed_n...
chromeos/network/managed_network_configuration_handler.cc:374: // properties
_might_ be user configured.
On 2013/04/10 01:14:15, Greg Spencer (Chromium) wrote:
> Ugh.  Doesn't the UIData have a version number?

No. But that wouldn't help much either: The problem is that up to now, we didn't
explicitly designate user settings.

E.g. Autoconnect: We can always read that value and get true/false. We can only
guess that the user changed the value, if it's not set to the default anymore.

https://codereview.chromium.org/12676017/diff/8003/chromeos/network/managed_n...
chromeos/network/managed_network_configuration_handler.cc:409: void
TranslatePropertiesAndRunCallback(
On 2013/04/10 01:14:15, Greg Spencer (Chromium) wrote:
> Consolidate this into the anonymous namespace at the top.

Done.

https://codereview.chromium.org/12676017/diff/8003/chromeos/network/managed_n...
chromeos/network/managed_network_configuration_handler.cc:872: (new
PolicyApplicator(weak_ptr_factory_.GetWeakPtr(),
On 2013/04/10 01:14:15, Greg Spencer (Chromium) wrote:
> Maybe a local variable instead of just newwing it up?  I know they're all
owned
> and managed by the callbacks created with Bind, but at least a note about
> lifetime/ownership is probably in order here.

Done.

https://codereview.chromium.org/12676017/diff/8003/chromeos/network/network_u...
File chromeos/network/network_ui_data.h (right):

https://codereview.chromium.org/12676017/diff/8003/chromeos/network/network_u...
chromeos/network/network_ui_data.h:87: // Key for storing thecertificate type.
On 2013/04/10 01:14:15, Greg Spencer (Chromium) wrote:
> thecertificate --> the certificate

Done.

https://codereview.chromium.org/12676017/diff/8003/chromeos/network/onc/onc_m...
File chromeos/network/onc/onc_merger.cc (right):

https://codereview.chromium.org/12676017/diff/8003/chromeos/network/onc/onc_m...
chromeos/network/onc/onc_merger.cc:339: // Prevent that credentials are
forwarded in cleartext to UI. User/shared
On 2013/04/10 01:14:15, Greg Spencer (Chromium) wrote:
> Prevent that credentials are --> Prevent credentials from being

Done.

https://codereview.chromium.org/12676017/diff/8003/chromeos/network/onc/onc_m...
chromeos/network/onc/onc_merger.cc:401: const OncValueSignature* signature_;
On 2013/04/10 01:14:15, Greg Spencer (Chromium) wrote:
> DISALLOW_COPY_AND_ASSIGN?  And for other classes here too?

Done.

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/networking_private_api.cc (right):

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/networking_private_api.cc:151: // The
|network_guid| parameter is storing the service path.
On 2013/04/11 14:46:29, pastarmovj wrote:
> Is there any other reason to create a temp here other than documenting the
code
> (which could be done inplace as well below)?
> 
> This comment applies for
NetworkingPrivateGetManagedPropertiesFunction::RunImpl
> as well.

No other reason. If just found it harder to miss if it's a separate variable.

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/networking_private_api.h (right):

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/networking_private_api.h:55: // Callbacks if
talking to ManagedNetworkConfigurationHandler.
On 2013/04/11 14:46:29, pastarmovj wrote:
> This is a little vague - are those callbacks called by the MNCH? 

Done.

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/networking_private_api.h:55: // Callbacks if
talking to ManagedNetworkConfigurationHandler.
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> On 2013/04/11 14:46:29, pastarmovj wrote:
> > This is a little vague - are those callbacks called by the MNCH? 
> How about:
> Callbacks for ManagedNetworkConfigurationHandler::GetManagedProperties

Done.

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/networking_private_api.h:59:
scoped_ptr<base::DictionaryValue> error_data);
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> nit: blank line

Done.

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/networking_private_apitest.cc (right):

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/networking_private_apitest.cc:158:
std::string uidata_blob =
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> nit: const std::string

Done.

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/networking_private_apitest.cc:166: "}";
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> nit: maybe use less space by not putting {} on their own lines? WS in the blob
> shouldn't matter, should it?
Whitespace and linebreaks don't matter. But I'd prefer readability over
size-reduction in test files.

Removed linebreaks for opening braces.

Actually, I don't like these escaped strings at all. Perhaps we can change this
to a complete initialization from test files in the future.

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/networking_private_apitest.cc:187:
std::string user_policy_blob =
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> Same nits as above.

Done.

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/network_configuration_updater2.cc (right):

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater2.cc:60: <<
chromeos::onc::GetSourceAsString(onc_source);
On 2013/04/11 14:46:29, pastarmovj wrote:
> Align.

Done.

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater2.cc:67: }
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> nit: No {}

Done.

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater2.cc:73: if
(policy_value == NULL) {
On 2013/04/11 14:46:29, pastarmovj wrote:
> !policy_value is preferred.

Done.

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater2.cc:82: } else {
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> No else after return.

Done.

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater2.cc:91: if
(onc_dict.get() == NULL) {
On 2013/04/11 14:46:29, pastarmovj wrote:
> !onc_dict works for scoped_ptr.

Done.

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/network_configuration_updater2.h (right):

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater2.h:22: // profiles.
Requests the NetworkLibrary to apply the ONC of the network
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> This version doesn't use NetworkLibrary, right?
I will write write 100 times "You shall not copy and paste." ;-)

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater2.h:27: class
NetworkConfigurationUpdater2 {
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> On 2013/04/11 14:46:29, pastarmovj wrote:
> > Please make NetworkConfigurationUpdater2 and etworkConfigurationUpdater have
a
> > common base class and then make the BrowserPolicyConnector return one or the
> > other based on the CommandLine switch to avoid having to duplicate the check
> all
> > over the code.
> > 
> > Also make sure to adjust includes accordingly wherever needed.
> 
> Agreed that we should use a pure virtual base class. Then we can use a static,
> Create() method which does the command line check and constructs the correct
> implementation. 
> 
> I also do not like "2" as a name differentiator. Lets move the existing
> implementation to something like NetworkConfigurationUpdaterImplCros, and the
> new one just NetworkConfigurationUpdaterImpl.

Done. The only thing I didn't rename is the test file and test fixture class, as
the names word get really lengthy.

The separation in Interface and Impl is only temporary, so it should be ok?

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater2.h:27: class
NetworkConfigurationUpdater2 {
On 2013/04/11 14:46:29, pastarmovj wrote:
> Please make NetworkConfigurationUpdater2 and etworkConfigurationUpdater have a
> common base class and then make the BrowserPolicyConnector return one or the
> other based on the CommandLine switch to avoid having to duplicate the check
all
> over the code.
> 
> Also make sure to adjust includes accordingly wherever needed.

Done.

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/extensions...
File chrome/browser/extensions/api/test/test_api.cc (right):

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/extensions...
chrome/browser/extensions/api/test/test_api.cc:73: LOG(ERROR) << message;
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> Revert to VLOG before committing.

Done.

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/resources/...
File
chrome/browser/resources/chromeos/network_configuration/js/network_config.js
(right):

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/resources/...
chrome/browser/resources/chromeos/network_configuration/js/network_config.js:67:
if (Object.keys(result).length)
On 2013/04/11 14:46:29, pastarmovj wrote:
> This is inconsistent. Either use curlies around single line if/else-s or not.
> I'd prefer not if it is fine with the JS style guide - couldn't fine anything
> about that.

Done.

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/resources/...
File
chrome/browser/resources/chromeos/network_configuration/js/network_status.js
(right):

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/resources/...
chrome/browser/resources/chromeos/network_configuration/js/network_status.js:625:
console.log('NetworkStatus: Network ' + id + ' added.');
On 2013/04/11 14:46:29, pastarmovj wrote:
> Is this for you for debugging or is it intended to stay in the final code?
It's for debugging. I don't know, do we have some logging standard for JS?

https://codereview.chromium.org/12676017/diff/23001/chromeos/dbus/shill_manag...
File chromeos/dbus/shill_manager_client.cc (right):

https://codereview.chromium.org/12676017/diff/23001/chromeos/dbus/shill_manag...
chromeos/dbus/shill_manager_client.cc:25: bool AreServicePropertiesValid2(const
base::DictionaryValue& properties) {
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> 2 is a poor differentiator. Maybe ForProfile or WithMode?

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/dbus/shill_manag...
chromeos/dbus/shill_manager_client.cc:34: // DEPRECATED: Keep this only for
backward compatibility with NetworkLibrary.
On 2013/04/11 14:46:29, pastarmovj wrote:
> Put a todo here to remove that once not needed and make a bug for that
> referenced from the TODO - that way this will not be code rotting here
forever.

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/dbus/shill_manag...
chromeos/dbus/shill_manager_client.cc:175:
DCHECK(AreServicePropertiesValid2(properties));
On 2013/04/11 14:46:29, pastarmovj wrote:
> DCHECK will only verify in debug builds - does your code below handle this
> situation correctly - if not you should either CHECK or exit early here.

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/dbus/shill_manag...
File chromeos/dbus/shill_manager_client.h (right):

https://codereview.chromium.org/12676017/diff/23001/chromeos/dbus/shill_manag...
chromeos/dbus/shill_manager_client.h:14: #include
"chromeos/dbus/dbus_method_call_status.h"
On 2013/04/11 14:46:29, pastarmovj wrote:
> I think forward declarations should be ok since you only use references to
those
> types here right?

dbus_method_call_status.h contains only typedefs and an enum.
ObjectPath however could be removed.

https://codereview.chromium.org/12676017/diff/23001/chromeos/dbus/shill_profi...
File chromeos/dbus/shill_profile_client.cc (right):

https://codereview.chromium.org/12676017/diff/23001/chromeos/dbus/shill_profi...
chromeos/dbus/shill_profile_client.cc:53: return NULL;
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> On 2013/04/11 14:46:29, pastarmovj wrote:
> > Maybe put a NOTREACHED() here too to make sure this code is not called at
all.
> 
> Actually, I think it's better (easier to debug) if the code  crashes where the
> test interface is accessed.
> 

NOTREACHED will still need the NULL return. CHECK(false) would be the
alternative.

I prefer it as it is.

https://codereview.chromium.org/12676017/diff/23001/chromeos/dbus/shill_profi...
chromeos/dbus/shill_profile_client.cc:53: return NULL;
On 2013/04/11 14:46:29, pastarmovj wrote:
> Maybe put a NOTREACHED() here too to make sure this code is not called at all.

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/dbus/shill_profi...
File chromeos/dbus/shill_profile_client.h (right):

https://codereview.chromium.org/12676017/diff/23001/chromeos/dbus/shill_profi...
chromeos/dbus/shill_profile_client.h:52: virtual bool AddService(const
std::string& service_path) = 0;
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> nit: blank line

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/dbus/shill_profi...
chromeos/dbus/shill_profile_client.h:54: ~TestInterface() {}
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> virtual

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/dbus/shill_profi...
chromeos/dbus/shill_profile_client.h:96: virtual TestInterface*
GetTestInterface() = 0;
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> blank line

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/dbus/shill_profi...
File chromeos/dbus/shill_profile_client_stub.h (right):

https://codereview.chromium.org/12676017/diff/23001/chromeos/dbus/shill_profi...
chromeos/dbus/shill_profile_client_stub.h:54: // Note: This should remain the
last member so it'll be destroyed and
On 2013/04/11 14:46:29, pastarmovj wrote:
> This comment is wrong for this class!
Ah thanks for the reminder. I wanted to remove the factory but forgot it after a
while.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
File chromeos/network/managed_network_configuration_handler.cc (right):

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:64: PROFILE_NONE,  //
Not in any profile.
On 2013/04/11 14:46:29, pastarmovj wrote:
> nit: Please align the comments for easier readability.

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:72: const char
kFakeCredential[] = "FAKE_CREDENTIAL";
On 2013/04/11 14:46:29, pastarmovj wrote:
> It's highly unlikely but this can never be used as a password so it should be
a
> value that is not a valid password either. What about an empty string? You can
> modify it on the UI side if you want to show something else whenever this
field
> is empty.

If find it to probable, that at some other place we interprete the empty string
already differently (unset) and debugging is more tricky with the empty string.

Therefore, I changed it to what I originally intended but forgot: Postfix a
random string. Now it's too unlikely to be set by a user, so we can ignore that.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:103: }
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> nit: no {}

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:135: const
std::string& error_message) {
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> Do we really want to ignore these error messages instead of logging them, at
> least as WARNING? We shouldn't normally be getting errors, should we?

Normally, we shouldn't get errors.
However, if they occur something might be really broken, so I'd go with
log(error)s.
In order to help anything for debugging, I also log the location of the
instantiation of the callback.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:138: void
LogError(const std::string& error) {
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> This appears to be unused.

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:138: void
LogError(const std::string& error) {
On 2013/04/11 14:46:29, pastarmovj wrote:
> Why would you do that?

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:151:
onc_object->GetWithoutPathExpansion(field_name, &value);
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> On 2013/04/11 14:46:29, pastarmovj wrote:
> > Why not use it.value() here?
> value() returns a const&, and RemoveFakeCredentials may modify
> value/nested_object. Maybe add a comment to that effect.
> 
> 

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:151:
onc_object->GetWithoutPathExpansion(field_name, &value);
On 2013/04/11 14:46:29, pastarmovj wrote:
> Why not use it.value() here?

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:157: if
(value->GetAsDictionary(&nested_object)) {
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> This is kind of confusing. While more verbose, I would prefer to see:
> 
> if (value->IsType(TYPE_DICTIONARY)) {
>   base::DictionaryValue* nested_object;
>   DCHECK(value->GetAsDictionary(&nested_object));
>   ...
> } else if (value->IsType(TYPE_STRING) {
>   ...
> }
> 

This pattern is rather common and typesafe. Do you find it confusing because of
the prepended variable declarations? But you will find this problem with every
usage of out-parameters.

What I don't like about your alternative (besides the redundancy) is that the
relation of TYPE_DICTIONARY to GetAsDictionary cannot be checked by the
compiler.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:192: user_settings); 
// shared settings
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> nit: Name 'settings' since may be user or shared?
Ugh. Yes, the current naming is confusing without context.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:228: }
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> nit: no {}

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:252: // Returns if
|policy| matches |onc_network_part|. This is should be the only
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> s/Returns/Returns true/

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:264: if (policy_type
!= network_type)
On 2013/04/11 14:46:29, pastarmovj wrote:
> Please be consistent in the file if you use or not use curlies around single
> line statements. The style guide allows both but mixing them in one file is
not
> really nice.

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:264: if (policy_type
!= network_type)
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> On 2013/04/11 14:46:29, pastarmovj wrote:
> > Please be consistent in the file if you use or not use curlies around single
> > line statements. The style guide allows both but mixing them in one file is
> not
> > really nice.
> Also while the style guide allows either, historically Chrome prefers not
using
> {} for single line statements in the generally interest of using less vertical
> space.
> 

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:272: &policy_ssid);
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> nit: one line?

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:282: const
std::map<std::string, const base::DictionaryValue*> &policies,
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> Use ManagedNetworkConfigurationHandler::PolicyMap

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:286: it !=
policies.end();
On 2013/04/11 14:46:29, pastarmovj wrote:
> combine those two lines.

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:286: it !=
policies.end();
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> On 2013/04/11 14:46:29, pastarmovj wrote:
> > combine those two lines.
> In fact, I would suggest placing both line 286 and 287 on 285.

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:349:
flimflam::kProfileProperty,
On 2013/04/11 14:46:29, pastarmovj wrote:
> Combine those two lines better.

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:407: namespace {
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> nit: I'd combine this with the anon namesapce at the top of the file.

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:464: false,  // Ignore
missing fields.
On 2013/04/11 14:46:29, pastarmovj wrote:
> Almost aligned here.
?
You want the comments to be aligned?

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:482: return;
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> nit: Put error clause with early exit above warning clause, with no else.

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:598:
remaining_policies_.swap(*modified_policies);
On 2013/04/11 14:46:29, pastarmovj wrote:
> Hmm is this safe in all cases?
Not if modified_policies is NULL, in which case it should segfault.
Do you prefer a DCHECK?

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:598:
remaining_policies_.swap(*modified_policies);
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> On 2013/04/11 14:46:29, pastarmovj wrote:
> > Hmm is this safe in all cases?
> This is confusing with a member and a scoped_ptr. I think it would be less
> confusing with a raw * here and a a non * local in the calling function.

The scoped_ptr indicates that ownership is passed to this function. Thus, the
caller must not use the pointer after the call and modification of that argument
doesn't have to be documented.

If I use a raw pointer (i.e. an out-parameter), it implies that I will modify
the argument and its effect is visible to the caller. Thus, documentation about
this effect is required.

Do you still prefer the latter?

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:606: this),
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> nit: combine the above 3 lines?

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:624: &entries)) {
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> nit: combine lines

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:665: &old_guid)) {
On 2013/04/11 14:46:29, pastarmovj wrote:
> align.

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:666: LOG(WARNING) <<
"Entry " << entry << " of profile " << profile_path_
On 2013/04/11 14:46:29, pastarmovj wrote:
> align.

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:712: if (new_policy) {
On 2013/04/11 14:46:29, pastarmovj wrote:
> Combine those ifs they are the same.

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:834: LOG(ERROR) <<
"ONC from " << onc::GetSourceAsString(onc_source)
On 2013/04/11 14:46:29, pastarmovj wrote:
> It there a chance you are leaking here infromation to the logs that is privacy
> sensitive?

Good point.
In most places I only log things like onc_source, profile_path, entry_path, ...
which are unproblematic.

The only critical log is in CreateShillConfiguration, line 247.
It _shouldn't_ contain any credentials as they are masked before and it's a
VLOG. So it should be ok as well.

I will also check other existing code and see if it needs masking.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:854: new
std::set<std::string>());
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> As noted above, this should just be 'std::set<std::string> modified_policies.
> Also, declare it just above 'if (network_configurations)', i.e. jut before it
is
> needed. It's confusing here since it has nothing to do with the next
statement.

Cannot move it much further, as it must stay in front of the if.
Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:892: (new
PolicyApplicator(weak_ptr_factory_.GetWeakPtr(),
On 2013/04/11 14:46:29, pastarmovj wrote:
> That a little bit too function-programming-y for C++. Please create a pointer
> variable and then call Run on it to make this look more C++.

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:904: }
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> nit: no {}

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
File chromeos/network/managed_network_configuration_handler.h (right):

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.h:121: // Only to be
called by NetworkConfigurationUpdater.
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> Also describe what the method does.

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.h:142:
scoped_ptr<PolicyMap> device_policies_by_guid_;
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> This is pretty confusing; it's a pointer to a map of pointers.
> 
> Do we actually need to pass ownership of these maps elsewhere, or could we
just
> make these maps, and convert:
> 
> user_policies_by_guid_ = GetFoo(..)
> to
> SetFoo(..., &user_policies_by_guid_);
> 
> And
> 
> !user_policies_by_guid_ to user_policies_by_guid_.empty()?
> 
> Or do we need to differentiate between an empty map and an unset one?
Yes, that must be differentiated.
In the previous patch I used a separate boolean "...is_initialized". Which
Julian thought not to be ideal.
What I like about the current usage of "pointer to map" is that every user of
the map is reminded by the type that the pointer might be NULL.
What I don't like is the bloated syntax in some of the functions.


> There is also the question of who owns the Dictionary*'s in the maps. A scoped
> pointer implies that destroying the member will clean up any contents, but it
> appears that is not the case. If we need to differentiate between empty and
> NULL, then I would use raw *'s here, not scoped_ptr.

As in other cases of map<*> usages of the code base, I added a clarifying
comment about the ownership.
Alternatively, I could change the Map to DictionaryValue, which however drops
the type information about it's content (the entries will be of type Value
instead of DictionaryValue).


I think, we'll have to come up with a compromise here.
Reverting to the booleans "...initialized_" as in the previous patch? At least
the semantics are more obvious then and don't infer with ownership.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
File chromeos/network/managed_network_configuration_handler_unittest.cc (right):

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler_unittest.cc:36: namespace
{
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> nit: blank line after namespace

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler_unittest.cc:42: }
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> nit: blank line before, and  // namespace

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler_unittest.cc:50: }
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> Put in anon namespace

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler_unittest.cc:56:
ManagedNetworkConfigurationHandlerTest() {
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> On 2013/04/11 14:46:29, pastarmovj wrote:
> > move {} on a single line.
> Or be consistent and put } on separate line for destructor (I prefer that when
> the rest of the class is implemented inline).
> 

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler_unittest.cc:56:
ManagedNetworkConfigurationHandlerTest() {
On 2013/04/11 14:46:29, pastarmovj wrote:
> move {} on a single line.

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler_unittest.cc:109: }
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> nit: no {}

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler_unittest.cc:122:
MessageLoop message_loop_;
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> DISALLOW...

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/network_...
File chromeos/network/network_configuration_handler.cc (right):

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/network_...
chromeos/network/network_configuration_handler.cc:202: if (type ==
flimflam::kTypeWifi) {
On 2013/04/11 14:46:29, pastarmovj wrote:
> Why is wify so special here? What about other tpyes of networks like mobile or
> WiMax?
Added a clarifying comment.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/network_...
chromeos/network/network_configuration_handler.cc:202: if (type ==
flimflam::kTypeWifi) {
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> On 2013/04/11 14:46:29, pastarmovj wrote:
> > Why is wify so special here? What about other tpyes of networks like mobile
or
> > WiMax?
> +1. Can't we have proxy config for a network in the user profile?
Not sure what this code has to do with proxy, but I will going to implement
proxy handling in the next few weeks.
> 
> Regardless, shouldn't we check on whether the profile property exists and is
non
> empty as to whether or not to call ConfigureServiceForProfile?
>

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/network_...
File chromeos/network/network_ui_data.h (right):

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/network_...
chromeos/network/network_ui_data.h:64: base::DictionaryValue* user_settings() {
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> const and return const* (or provide both)

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/network_...
chromeos/network/network_ui_data.h:70: const std::string& policy_guid() {
On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> const

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/network_...
chromeos/network/network_ui_data.h:87: // Key for storing thecertificate type.
On 2013/04/11 14:46:29, pastarmovj wrote:
> the<space>cer...

Done.

[stevenjb, 2013-04-15 17:38:54]

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/networking_private_apitest.cc (right):

https://codereview.chromium.org/12676017/diff/23001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/networking_private_apitest.cc:166: "}";
On 2013/04/15 12:16:24, pneubeck wrote:
> On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> > nit: maybe use less space by not putting {} on their own lines? WS in the
blob
> > shouldn't matter, should it?
> Whitespace and linebreaks don't matter. But I'd prefer readability over
> size-reduction in test files.
> 
> Removed linebreaks for opening braces.
> 
> Actually, I don't like these escaped strings at all. Perhaps we can change
this
> to a complete initialization from test files in the future.
+1

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
File chromeos/network/managed_network_configuration_handler.cc (right):

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:157: if
(value->GetAsDictionary(&nested_object)) {
Well, I don't feel strongly enough about it to waste much time with nits, but
the primary source of confusion for me was the declaration of both
'nested_object' and 'string_value' at the top level. Only on or the other will
get used, and they both effectively refer to the same data.

Maybe instead of as an if/else, have each if() return at the end and declare
string_value above its if clause? A brief comment above each clause would also
help. (e.g. "If the value is a Dictionary..." and "If the value is a String...")

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:282: const
std::map<std::string, const base::DictionaryValue*> &policies,
On 2013/04/15 12:16:24, pneubeck wrote:
> On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> > Use ManagedNetworkConfigurationHandler::PolicyMap
> 
> Done.
Not done?

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:598:
remaining_policies_.swap(*modified_policies);
With swap(), I do prefer it changed with the comment.

Passing ownership with a scoped_ptr would make more sense if remaining_policies_
were a scoped_ptr istelf (and we should definitely use a typedef for the set<>
in that case).

Alternately copying a const& would be fine and safe, I don't think the overhead
is sufficient.

But, all that said, I think this is a good case for using a * and a comment like
you have.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
File chromeos/network/managed_network_configuration_handler.h (right):

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.h:142:
scoped_ptr<PolicyMap> device_policies_by_guid_;
How about raw *'s here then, instead of scoped_ptr? scoped_ptr is great when a
member fully cleans itself up, but that is not the case here.

Alternatively, make PolicyMap a class with a destructor that does the cleanup
instead of a typedef.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/network_...
File chromeos/network/network_configuration_handler.cc (right):

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/network_...
chromeos/network/network_configuration_handler.cc:202: if (type ==
flimflam::kTypeWifi) {
I mentioned 'proxy' only as an example of a non-wifi service property that might
be different in the user profile than the shared profile. Sounds like you're on
top of addressing this for non wifi networks, so this is fine with the comment
for now.

[pneubeck (no reviews), 2013-04-16 07:50:06]

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
File chromeos/network/managed_network_configuration_handler.cc (right):

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:157: if
(value->GetAsDictionary(&nested_object)) {
On 2013/04/15 17:38:54, stevenjb (chromium) wrote:
> Well, I don't feel strongly enough about it to waste much time with nits, but
> the primary source of confusion for me was the declaration of both
> 'nested_object' and 'string_value' at the top level. Only on or the other will
> get used, and they both effectively refer to the same data.
> 
> Maybe instead of as an if/else, have each if() return at the end and declare
> string_value above its if clause? A brief comment above each clause would also
> help. (e.g. "If the value is a Dictionary..." and "If the value is a
String...")

Done.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:282: const
std::map<std::string, const base::DictionaryValue*> &policies,
On 2013/04/15 17:38:54, stevenjb (chromium) wrote:
> On 2013/04/15 12:16:24, pneubeck wrote:
> > On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> > > Use ManagedNetworkConfigurationHandler::PolicyMap
> > 
> > Done.
> Not done?

Yeah, I had previously replaced all within ManagedNetworkConfigurationHandler.
Done.

[pastarmovj, 2013-04-16 11:31:17]

Almost there :)

https://codereview.chromium.org/12676017/diff/23001/chromeos/dbus/shill_profi...
File chromeos/dbus/shill_profile_client.cc (right):

https://codereview.chromium.org/12676017/diff/23001/chromeos/dbus/shill_profi...
chromeos/dbus/shill_profile_client.cc:53: return NULL;
On 2013/04/15 12:16:24, pneubeck wrote:
> On 2013/04/11 18:20:08, stevenjb (chromium) wrote:
> > On 2013/04/11 14:46:29, pastarmovj wrote:
> > > Maybe put a NOTREACHED() here too to make sure this code is not called at
> all.
> > 
> > Actually, I think it's better (easier to debug) if the code  crashes where
the
> > test interface is accessed.
> > 
> 
> NOTREACHED will still need the NULL return. CHECK(false) would be the
> alternative.
> 
> I prefer it as it is.

I am not sure if it will be easier to debug since the crash might occur very far
away from the point that calls GetTestInterface. I suggest to keep the return
NULL but add a NOTREACHED for speeding up the crash in debug builds. It will not
add any extra code to the release build.

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
File chromeos/network/managed_network_configuration_handler.cc (right):

https://codereview.chromium.org/12676017/diff/23001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:464: false,  // Ignore
missing fields.
On 2013/04/15 12:16:24, pneubeck wrote:
> On 2013/04/11 14:46:29, pastarmovj wrote:
> > Almost aligned here.
> ?
> You want the comments to be aligned?

Yes, but you can leave it as is if you wish.

https://codereview.chromium.org/12676017/diff/58002/chrome/browser/policy/bro...
File chrome/browser/policy/browser_policy_connector.cc (right):

https://codereview.chromium.org/12676017/diff/58002/chrome/browser/policy/bro...
chrome/browser/policy/browser_policy_connector.cc:409:
network_configuration_updater_.reset(new NetworkConfigurationUpdaterImpl(
nit: I think it will be more easily readable if you split this line before new
and join with the rest below.

https://codereview.chromium.org/12676017/diff/58002/chromeos/dbus/shill_manag...
File chromeos/dbus/shill_manager_client.cc (right):

https://codereview.chromium.org/12676017/diff/58002/chromeos/dbus/shill_manag...
chromeos/dbus/shill_manager_client.cc:196: LOG(ERROR) <<
kIncompleteServicePropertiesMessage;
You can make this NOTREACHED - it is like a DCHECK(false) so it will both print
your message and crash in debug builds like your initial intention was.

https://codereview.chromium.org/12676017/diff/58002/chromeos/test/data/networ...
File chromeos/test/data/network/policy/shill_managed_wifi1.json (right):

https://codereview.chromium.org/12676017/diff/58002/chromeos/test/data/networ...
chromeos/test/data/network/policy/shill_managed_wifi1.json:12: "UIData":
"{\"onc_source\":\"user_policy\",\"user_settings\":{\"WiFi\":{\"Passphrase\":\"FAKE_CREDENTIAL\"}}}"
Don't you need to adjust the tests to your new FAKE_CRED... string now?

[pneubeck (no reviews), 2013-04-16 14:56:28]

https://codereview.chromium.org/12676017/diff/58002/chrome/browser/policy/bro...
File chrome/browser/policy/browser_policy_connector.cc (right):

https://codereview.chromium.org/12676017/diff/58002/chrome/browser/policy/bro...
chrome/browser/policy/browser_policy_connector.cc:409:
network_configuration_updater_.reset(new NetworkConfigurationUpdaterImpl(
On 2013/04/16 11:31:18, pastarmovj wrote:
> nit: I think it will be more easily readable if you split this line before new
> and join with the rest below.

Done.

https://codereview.chromium.org/12676017/diff/58002/chromeos/dbus/shill_manag...
File chromeos/dbus/shill_manager_client.cc (right):

https://codereview.chromium.org/12676017/diff/58002/chromeos/dbus/shill_manag...
chromeos/dbus/shill_manager_client.cc:196: LOG(ERROR) <<
kIncompleteServicePropertiesMessage;
On 2013/04/16 11:31:18, pastarmovj wrote:
> You can make this NOTREACHED - it is like a DCHECK(false) so it will both
print
> your message and crash in debug builds like your initial intention was.

Done.

https://codereview.chromium.org/12676017/diff/58002/chromeos/test/data/networ...
File chromeos/test/data/network/policy/shill_managed_wifi1.json (right):

https://codereview.chromium.org/12676017/diff/58002/chromeos/test/data/networ...
chromeos/test/data/network/policy/shill_managed_wifi1.json:12: "UIData":
"{\"onc_source\":\"user_policy\",\"user_settings\":{\"WiFi\":{\"Passphrase\":\"FAKE_CREDENTIAL\"}}}"
On 2013/04/16 11:31:18, pastarmovj wrote:
> Don't you need to adjust the tests to your new FAKE_CRED... string now?

Done.

[pastarmovj, 2013-04-16 15:06:29]

LGTM.

[stevenjb, 2013-04-16 16:02:39]

lgtm

https://codereview.chromium.org/12676017/diff/73064/chromeos/network/managed_...
File chromeos/network/managed_network_configuration_handler.cc (right):

https://codereview.chromium.org/12676017/diff/73064/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:165: // If |value| is
a string and a credential, ...
Maybe 'If |value| is a string, check if it is a fake credential'?

https://codereview.chromium.org/12676017/diff/73064/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:170: if (string_value
== kFakeCredential) {
redundant

[stevenjb, 2013-04-16 16:02:41]

lgtm

[pneubeck (no reviews), 2013-04-17 07:59:07]

Hi Benjamin,
please owner-review the *extension* files. Thanks.

[not at google - send to devlin, 2013-04-17 17:56:19]

https://codereview.chromium.org/12676017/diff/88001/chrome/common/extensions/...
File chrome/common/extensions/api/networking_private.json (right):

https://codereview.chromium.org/12676017/diff/88001/chrome/common/extensions/...
chrome/common/extensions/api/networking_private.json:75: "description": "Gets
the merged properties of the network with id networkGuid from the sources: User
settings, shared settings, user policy, device policy and the currently active
settings.",
I don't understand the comment. Is it saying that this function returns the
object union of getProperties() and ... ok I'm lost.

https://codereview.chromium.org/12676017/diff/88001/chrome/common/extensions/...
chrome/common/extensions/api/networking_private.json:88: "$ref":
"ManagedNetworkProperties",
If it's only used in once place then no need to $ref it. However, depending on
answer to above question... I may have comments.

[not at google - send to devlin, 2013-04-18 16:03:01]

API review style comment:

Can you merge getManagedProperties with getProperties? Make getProperties take
an optional second "options" argument that, at least for now, would just include
a boolean "managed" property.

That would make the documentation and extra type issue moot.

[pneubeck (no reviews), 2013-04-18 20:22:20]

On 2013/04/18 16:03:01, kalman wrote:
> API review style comment:
> 
> Can you merge getManagedProperties with getProperties? Make getProperties take
> an optional second "options" argument that, at least for now, would just
include
> a boolean "managed" property.
> 
> That would make the documentation and extra type issue moot.

Could you explain what is improved by your proposed variant? The reason I added
the separate type is, that the type of the return values of GetManagedProperties
and GetProperties differs. For examples, how the returned objects can look like,
see the examples in test.js (the primitive values are replaced by objects)

I know, that currently we have only the placeholder types in the json-schema.
But the reason for that is that we don't have an automatic conversion from the
ONC description to json-schema yet (or the other way round). ONC itself is
strictly typed.

[not at google - send to devlin, 2013-04-18 20:24:08]

Ok. I just don't understand the API call, but if it's only ever used internally,
nbd. lgtm.

[commit-bot: I haz the power, 2013-04-19 14:48:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/12676017/64013

[commit-bot: I haz the power, 2013-04-19 15:15:43]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-19 15:45:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/12676017/101007

[commit-bot: I haz the power, 2013-04-19 17:54:46]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-04-19 18:29:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/12676017/101007

[commit-bot: I haz the power, 2013-04-19 20:35:09]

Change committed as 195267

[Lei Zhang, 2013-04-21 23:55:41]

gypi file lgtm.