Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11975015:
This adds a private extension API to use for simple networking (Closed)
|
Created:
7 years, 11 months ago by Greg Spencer (Chromium) Modified:
7 years, 10 months ago CC:
chromium-reviews, nkostylev+watch_chromium.org, sail+watch_chromium.org, Aaron Boodman, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, zel, mazda Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionThis adds a private extension API to use for simple networking
requests (connect/disconnect/visible networks, etc).
(TBR'ing owners for chrome/ because of simple addition of
files to chrome_browser_chromeos.gypi)
TBR=sky@chromium.org
BUG=chromium:168713
TEST=ran new browser api test
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180525
Patch Set 1 #Patch Set 2 : Fixed self review nits #
Total comments: 21
Patch Set 3 : Review changes #Patch Set 4 : Fix permission_set_unittest #
Total comments: 24
Patch Set 5 : Fixing clang problems #Patch Set 6 : Fix clang problems #Patch Set 7 : Converting to ONC #
Total comments: 14
Patch Set 8 : Uploading after setting tracking branch #Patch Set 9 : Review changes #
Total comments: 10
Patch Set 10 : Review changes #Patch Set 11 : Upload after merge #
Total comments: 42
Patch Set 12 : Converted to schema compiler #
Total comments: 7
Patch Set 13 : Review changes #
Total comments: 1
Patch Set 14 : Simplify failure case #
Total comments: 18
Patch Set 15 : Review changes #
Total comments: 5
Patch Set 16 : Review changes #Patch Set 17 : upload after merge #Patch Set 18 : upload after another merge #Messages
Total messages: 36 (0 generated)
This is the first CL for this API. It's missing the property change notification events. I'll add that in a later CL. Please take a look at the referenced bug for the design doc.
https://codereview.chromium.org/11975015/diff/2001/chrome/browser/chromeos/ex... File chrome/browser/chromeos/extensions/networking_private_api.cc (right): https://codereview.chromium.org/11975015/diff/2001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.cc:39: if (properties.GetString(flimflam::kNameProperty, &name)) { nit: braces are not needed https://codereview.chromium.org/11975015/diff/2001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.cc:72: return false; nit: indent with two spaces https://codereview.chromium.org/11975015/diff/2001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.cc:115: return false; nit: indent with two spaces https://codereview.chromium.org/11975015/diff/2001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.cc:199: return false; nit: indent with two spaces https://codereview.chromium.org/11975015/diff/2001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.cc:202: dbus::ObjectPath(service_path), nit: indent with four spaces https://codereview.chromium.org/11975015/diff/2001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.cc:227: return false; nit: indent with two spaces https://codereview.chromium.org/11975015/diff/2001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.cc:230: dbus::ObjectPath(service_path), nit: indent with four spaces https://codereview.chromium.org/11975015/diff/2001/chrome/browser/chromeos/ex... File chrome/browser/chromeos/extensions/networking_private_api.h (right): https://codereview.chromium.org/11975015/diff/2001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.h:63: ~ScopedDecrementer(); nit: add an empty line. https://codereview.chromium.org/11975015/diff/2001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.h:133: } // chromeos // namespace chromeos https://codereview.chromium.org/11975015/diff/2001/chrome/browser/chromeos/ex... File chrome/browser/chromeos/extensions/networking_private_apitest.cc (right): https://codereview.chromium.org/11975015/diff/2001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_apitest.cc:7: #include "base/stl_util.h" Several headers seem unnecessary.
https://codereview.chromium.org/11975015/diff/2001/chrome/browser/chromeos/ex... File chrome/browser/chromeos/extensions/networking_private_api.cc (right): https://codereview.chromium.org/11975015/diff/2001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.cc:39: if (properties.GetString(flimflam::kNameProperty, &name)) { On 2013/01/16 22:08:52, mazda wrote: > nit: braces are not needed Done. https://codereview.chromium.org/11975015/diff/2001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.cc:72: return false; On 2013/01/16 22:08:52, mazda wrote: > nit: indent with two spaces Done. https://codereview.chromium.org/11975015/diff/2001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.cc:72: return false; On 2013/01/16 22:08:52, mazda wrote: > nit: indent with two spaces Done. https://codereview.chromium.org/11975015/diff/2001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.cc:115: return false; On 2013/01/16 22:08:52, mazda wrote: > nit: indent with two spaces Done. https://codereview.chromium.org/11975015/diff/2001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.cc:199: return false; On 2013/01/16 22:08:52, mazda wrote: > nit: indent with two spaces Done. https://codereview.chromium.org/11975015/diff/2001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.cc:202: dbus::ObjectPath(service_path), On 2013/01/16 22:08:52, mazda wrote: > nit: indent with four spaces Done. https://codereview.chromium.org/11975015/diff/2001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.cc:227: return false; On 2013/01/16 22:08:52, mazda wrote: > nit: indent with two spaces Done. https://codereview.chromium.org/11975015/diff/2001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.cc:230: dbus::ObjectPath(service_path), On 2013/01/16 22:08:52, mazda wrote: > nit: indent with four spaces Done. https://codereview.chromium.org/11975015/diff/2001/chrome/browser/chromeos/ex... File chrome/browser/chromeos/extensions/networking_private_api.h (right): https://codereview.chromium.org/11975015/diff/2001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.h:63: ~ScopedDecrementer(); On 2013/01/16 22:08:52, mazda wrote: > nit: add an empty line. Done. https://codereview.chromium.org/11975015/diff/2001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.h:133: } // chromeos On 2013/01/16 22:08:52, mazda wrote: > // namespace chromeos Done. https://codereview.chromium.org/11975015/diff/2001/chrome/browser/chromeos/ex... File chrome/browser/chromeos/extensions/networking_private_apitest.cc (right): https://codereview.chromium.org/11975015/diff/2001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_apitest.cc:7: #include "base/stl_util.h" On 2013/01/16 22:08:52, mazda wrote: > Several headers seem unnecessary. Done.
https://codereview.chromium.org/11975015/diff/3021/chrome/browser/chromeos/ex... File chrome/browser/chromeos/extensions/networking_private_api.cc (right): https://codereview.chromium.org/11975015/diff/3021/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.cc:30: // interested in passing on to JavaScript. I thought we were going to use ONC properties for all JS dictionaries, to avoid lots of slightly different property names? Also, it seems like if the only property we need here not already in NetworkState is WifiBSsid, we could just use NetworkStateHandler? Eventually we're going to need something just like this that translates every Service property for JS. It would be nice to only fetch services from the Manager in only two places instead of three. https://codereview.chromium.org/11975015/diff/3021/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.cc:46: if (state == flimflam::kStateReady || state == flimflam::kStateOnline) { This should use NetworkState::StateIsConnected() https://codereview.chromium.org/11975015/diff/3021/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.cc:50: state == flimflam::kStateConfiguration) { This should use NetworkState::StateIsConnecting() https://codereview.chromium.org/11975015/diff/3021/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.cc:51: filtered_result->SetString(kStatus, "connecting"); This should also be a const defined above https://codereview.chromium.org/11975015/diff/3021/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.cc:53: filtered_result->SetString(kStatus, "notConnected"); ditto https://codereview.chromium.org/11975015/diff/3021/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.cc:66: NetworkingGetPropertiesFunction::~NetworkingGetPropertiesFunction() {} nit: I generally see/prefer } on a second line outside of inlined declarations https://codereview.chromium.org/11975015/diff/3021/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.cc:95: : parent_(parent), result_list_(result_list) { align, separate line per member https://codereview.chromium.org/11975015/diff/3021/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.cc:99: result_list_->count--; pre-decrement https://codereview.chromium.org/11975015/diff/3021/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.cc:213: SendResponse(true); Note: this does not mean that we connected, just that the DBus message succeeded. We should either document that, or wait for the State change before sending a response. https://codereview.chromium.org/11975015/diff/3021/chrome/browser/chromeos/ex... File chrome/browser/chromeos/extensions/networking_private_api.h (right): https://codereview.chromium.org/11975015/diff/3021/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.h:56: int count; Since this is a class, we should use member_ and accessors. Also I think we require an explicit non-inlined destructor? + DISALLOW... https://codereview.chromium.org/11975015/diff/3021/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.h:77: const base::DictionaryValue& result); nit: WS https://codereview.chromium.org/11975015/diff/3021/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.h:96: const std::string& errorMessage); nit: WS https://codereview.chromium.org/11975015/diff/3021/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.h:115: const std::string& errorMessage); nit: WS https://codereview.chromium.org/11975015/diff/3021/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.h:129: static NetworkingPrivateAPI* Get(Profile* profile); nit: WS
https://codereview.chromium.org/11975015/diff/3021/chrome/browser/chromeos/ex... File chrome/browser/chromeos/extensions/networking_private_api.cc (right): https://codereview.chromium.org/11975015/diff/3021/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.cc:30: // interested in passing on to JavaScript. On 2013/01/17 00:04:30, stevenjb (chromium) wrote: > I thought we were going to use ONC properties for all JS dictionaries, to avoid > lots of slightly different property names? > > Also, it seems like if the only property we need here not already in > NetworkState is WifiBSsid, we could just use NetworkStateHandler? > > Eventually we're going to need something just like this that translates every > Service property for JS. It would be nice to only fetch services from the > Manager in only two places instead of three. Good point on the ONC, I'll switch it to that format. I forgot to write out in the description for this change that in order to prevent the other project from being dependent upon CONFUSE in order to ship, I'm avoiding using the stuff that we have behind a flag. As soon as we move it out from behind the flag I was going to make the internals of this class change to use the "new stuff" but for now just use Shill directly. Or perhaps just make the things it executes conditional on the flag for now.
That's fair, if this is targeting 26, but lets at least go ahead and use the ONC constants, and add a TODO (and NetworkState static methods are still OK to use at least).
OK, so this now uses the ONC translation infrastructure for converting from Shill->ONC for the properties, and then filters out any properties we don't care about. At the moment this patch incorporates a dependent change to the ONC translator that was necessary to allow this to work (but it's needed anyhow for future work). So you can ignore any files here that are in https://codereview.chromium.org/11962048/ (which is mostly the chromeos/network/onc files) I'll merge it after that patch lands. https://codereview.chromium.org/11975015/diff/3021/chrome/browser/chromeos/ex... File chrome/browser/chromeos/extensions/networking_private_api.cc (right): https://codereview.chromium.org/11975015/diff/3021/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.cc:66: NetworkingGetPropertiesFunction::~NetworkingGetPropertiesFunction() {} On 2013/01/17 00:04:30, stevenjb (chromium) wrote: > nit: I generally see/prefer } on a second line outside of inlined declarations Done. https://codereview.chromium.org/11975015/diff/3021/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.cc:95: : parent_(parent), result_list_(result_list) { On 2013/01/17 00:04:30, stevenjb (chromium) wrote: > align, separate line per member Done. https://codereview.chromium.org/11975015/diff/3021/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.cc:99: result_list_->count--; On 2013/01/17 00:04:30, stevenjb (chromium) wrote: > pre-decrement Done. https://codereview.chromium.org/11975015/diff/3021/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.cc:213: SendResponse(true); On 2013/01/17 00:04:30, stevenjb (chromium) wrote: > Note: this does not mean that we connected, just that the DBus message > succeeded. We should either document that, or wait for the State change before > sending a response. I documented it. The Javascript can wait for a property change event if it wants to. https://codereview.chromium.org/11975015/diff/3021/chrome/browser/chromeos/ex... File chrome/browser/chromeos/extensions/networking_private_api.h (right): https://codereview.chromium.org/11975015/diff/3021/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.h:56: int count; On 2013/01/17 00:04:30, stevenjb (chromium) wrote: > Since this is a class, we should use member_ and accessors. Also I think we > require an explicit non-inlined destructor? + DISALLOW... Yeah, OK. It really is just a struct with a constructor, but it is ref counted, so I guess I'll switch to "class" mode and add the accessors and the other bits. https://codereview.chromium.org/11975015/diff/3021/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.h:77: const base::DictionaryValue& result); On 2013/01/17 00:04:30, stevenjb (chromium) wrote: > nit: WS Done. https://codereview.chromium.org/11975015/diff/3021/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.h:96: const std::string& errorMessage); On 2013/01/17 00:04:30, stevenjb (chromium) wrote: > nit: WS Done. https://codereview.chromium.org/11975015/diff/3021/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.h:115: const std::string& errorMessage); On 2013/01/17 00:04:30, stevenjb (chromium) wrote: > nit: WS Done. https://codereview.chromium.org/11975015/diff/3021/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/networking_private_api.h:129: static NetworkingPrivateAPI* Get(Profile* profile); On 2013/01/17 00:04:30, stevenjb (chromium) wrote: > nit: WS Done.
Mostly nits, but I also don't know the extension stuff well enough. Btw. if you set git branch --set-upstream dependent-branch start-point git cl should only upload the difference to start-point. https://codereview.chromium.org/11975015/diff/22001/chrome/browser/chromeos/e... File chrome/browser/chromeos/extensions/networking_private_api.cc (right): https://codereview.chromium.org/11975015/diff/22001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:35: const char* desired_fields[] = { may add static but could even be static const char* const desired_fields[] https://codereview.chromium.org/11975015/diff/22001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:148: for (size_t i = 0; i < available_services->GetSize(); ++i) { As you don't need the index, I'd prefer using ListValue::iterator https://codereview.chromium.org/11975015/diff/22001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:172: ScopedDecrementer decrementer(this, result_list); The only usage now and in the future seems to be this place. Wouldn't a if (...error conditions...) { } else { } decrement count_ if (..count_ == 0) { send response } be less complex? https://codereview.chromium.org/11975015/diff/22001/chrome/browser/chromeos/e... File chrome/browser/chromeos/extensions/networking_private_api.h (right): https://codereview.chromium.org/11975015/diff/22001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.h:73: class ScopedDecrementer { The forward declaration is enough, this can move to the cc. https://codereview.chromium.org/11975015/diff/22001/chrome/browser/chromeos/e... File chrome/browser/chromeos/extensions/networking_private_api_factory.cc (right): https://codereview.chromium.org/11975015/diff/22001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api_factory.cc:42: // Explicitly and always allow this service in guest login mode. See whitespace https://codereview.chromium.org/11975015/diff/22001/chrome/browser/chromeos/e... File chrome/browser/chromeos/extensions/networking_private_api_factory.h (right): https://codereview.chromium.org/11975015/diff/22001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api_factory.h:43: }; not strictly necessary, but maybe good for documentation: DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/11975015/diff/22001/chrome/common/extensions/... File chrome/common/extensions/api/networking_private.json (right): https://codereview.chromium.org/11975015/diff/22001/chrome/common/extensions/... chrome/common/extensions/api/networking_private.json:7: "namespace":"networkingPrivate", I just found this page about IDL for new extension APIs: http://dev.chromium.org/developers/design-documents/extensions/proposed-chang... but I've no idea if that applies to this API.
Thanks for the pointer about git branch --set-upstream! I've been wondering if there was a way to do exactly that for a while now. https://codereview.chromium.org/11975015/diff/22001/chrome/browser/chromeos/e... File chrome/browser/chromeos/extensions/networking_private_api.cc (right): https://codereview.chromium.org/11975015/diff/22001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:35: const char* desired_fields[] = { On 2013/01/18 13:22:36, pneubeck wrote: > may add static > but could even be > static const char* const desired_fields[] Done. https://codereview.chromium.org/11975015/diff/22001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:148: for (size_t i = 0; i < available_services->GetSize(); ++i) { On 2013/01/18 13:22:36, pneubeck wrote: > As you don't need the index, I'd prefer using ListValue::iterator Done. https://codereview.chromium.org/11975015/diff/22001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:172: ScopedDecrementer decrementer(this, result_list); On 2013/01/18 13:22:36, pneubeck wrote: > The only usage now and in the future seems to be this place. > Wouldn't [...] be less complex? Yes, I suppose that's simpler. When I wrote the decrementer, there were other paths that used it, and I wanted to make sure it always happened because if a path fails to decrement the count properly, then the results never get sent, so this seemed more complex, but less error prone. I've removed the decrementer and switched to what you suggested. https://codereview.chromium.org/11975015/diff/22001/chrome/browser/chromeos/e... File chrome/browser/chromeos/extensions/networking_private_api.h (right): https://codereview.chromium.org/11975015/diff/22001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.h:73: class ScopedDecrementer { On 2013/01/18 13:22:36, pneubeck wrote: > The forward declaration is enough, this can move to the cc. Done, and for ResultList too. https://codereview.chromium.org/11975015/diff/22001/chrome/browser/chromeos/e... File chrome/browser/chromeos/extensions/networking_private_api_factory.cc (right): https://codereview.chromium.org/11975015/diff/22001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api_factory.cc:42: // Explicitly and always allow this service in guest login mode. See On 2013/01/18 13:22:36, pneubeck wrote: > whitespace Done. https://codereview.chromium.org/11975015/diff/22001/chrome/browser/chromeos/e... File chrome/browser/chromeos/extensions/networking_private_api_factory.h (right): https://codereview.chromium.org/11975015/diff/22001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api_factory.h:43: }; On 2013/01/18 13:22:36, pneubeck wrote: > not strictly necessary, but maybe good for documentation: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/11975015/diff/22001/chrome/common/extensions/... File chrome/common/extensions/api/networking_private.json (right): https://codereview.chromium.org/11975015/diff/22001/chrome/common/extensions/... chrome/common/extensions/api/networking_private.json:7: "namespace":"networkingPrivate", On 2013/01/18 13:22:36, pneubeck wrote: > I just found this page about IDL for new extension APIs: > > http://dev.chromium.org/developers/design-documents/extensions/proposed-chang... > > but I've no idea if that applies to this API. Interesting. I haven't seen that. For now, since this works, I'm going to go with the "old" way. I may convert to a .idl file in the future if it makes sense. Since this is not a public API, I'm not sure it matters.
lgtm, but I am also only passing familiar with the extensions/ code, so it would be good to get an lg from someone on that team.
ACK https://codereview.chromium.org/11975015/diff/27001/chrome/browser/chromeos/e... File chrome/browser/chromeos/extensions/networking_private_api.cc (right): https://codereview.chromium.org/11975015/diff/27001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:176: // All of the results were malformed, so we fail. 'results' is misleading -> 'service paths' https://codereview.chromium.org/11975015/diff/27001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:191: if (call_status == DBUS_METHOD_CALL_SUCCESS) { why not if ( .. && .. && .. ) instead of nesting? Because of short-circuit execution, actually all conditions can be ANDed. https://codereview.chromium.org/11975015/diff/27001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:195: if (TranslateStringToONC(onc::kNetworkTypeTable, shill_type, &onc_type)) { With my suggestion about adding a OncValueSignature for network type, this code will instead look like: scoped_ptr<base::Value> onc_type_value( TranslateShillServiceToONCValue(onc::kNetworkTypeSignature, result)) if (onc_type_value && onc_type_value->GetAsString(&onc_type)) ... I'll upload a CL to add this translation function TranslateShillServiceToONCValue, that works on non-dictionary values. https://codereview.chromium.org/11975015/diff/27001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:197: network_type == onc::network_type::kAllTypes) { kAllTypes should be in the list of ONC constants, see comment in the other CL.
https://codereview.chromium.org/11975015/diff/27001/chrome/browser/chromeos/e... File chrome/browser/chromeos/extensions/networking_private_api.cc (right): https://codereview.chromium.org/11975015/diff/27001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:195: if (TranslateStringToONC(onc::kNetworkTypeTable, shill_type, &onc_type)) { On 2013/01/21 09:28:51, pneubeck wrote: > With my suggestion about adding a OncValueSignature for network type, this code > will instead look like: > > scoped_ptr<base::Value> onc_type_value( > TranslateShillServiceToONCValue(onc::kNetworkTypeSignature, result)) > if (onc_type_value && > onc_type_value->GetAsString(&onc_type)) > ... > > I'll upload a CL to add this translation function > TranslateShillServiceToONCValue, that works on non-dictionary values. I thought more about this and tried to implement something. But OncValueSignature is obviously the wrong thing to use (it doesn't have a shill_property_name). There are actually several problems to provide such a per field translation, as most fields are currently translated relative to the enclosing object. So, I think the best thing to do is: Translate the complete enclosing object (here Network) and access whatever you need. Thereby, you remove the dependency on flimflam::kTypeProperty. I don't think that optimizing here is worth the effort.
https://codereview.chromium.org/11975015/diff/27001/chrome/browser/chromeos/e... File chrome/browser/chromeos/extensions/networking_private_api.cc (right): https://codereview.chromium.org/11975015/diff/27001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:176: // All of the results were malformed, so we fail. On 2013/01/21 09:28:51, pneubeck wrote: > 'results' is misleading -> 'service paths' Done. https://codereview.chromium.org/11975015/diff/27001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:191: if (call_status == DBUS_METHOD_CALL_SUCCESS) { On 2013/01/21 09:28:51, pneubeck wrote: > why not > if ( .. && .. && .. ) > instead of nesting? > Because of short-circuit execution, actually all conditions can be ANDed. Done. https://codereview.chromium.org/11975015/diff/27001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:195: if (TranslateStringToONC(onc::kNetworkTypeTable, shill_type, &onc_type)) { On 2013/01/21 12:12:09, pneubeck wrote: > So, I think the best thing to do is: > Translate the complete enclosing object (here Network) and access whatever you > need. Thereby, you remove the dependency on flimflam::kTypeProperty. > > I don't think that optimizing here is worth the effort. OK, I'll take that path. I'll remove the exports for the translation routines and constants. I'll probably leave the routines (but private to the ONC module). https://codereview.chromium.org/11975015/diff/27001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:197: network_type == onc::network_type::kAllTypes) { On 2013/01/21 09:28:51, pneubeck wrote: > kAllTypes should be in the list of ONC constants, see comment in the other CL. Hmm. Maybe I misunderstood your other comment. From that comment, I thought you wanted kAllTypes to be defined in this file, but this comment seems to say otherwise. Can you explain again?
Still ACK. https://codereview.chromium.org/11975015/diff/27001/chrome/browser/chromeos/e... File chrome/browser/chromeos/extensions/networking_private_api.cc (right): https://codereview.chromium.org/11975015/diff/27001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:197: network_type == onc::network_type::kAllTypes) { On 2013/01/22 19:44:29, Greg Spencer (Chromium) wrote: > On 2013/01/21 09:28:51, pneubeck wrote: > > kAllTypes should be in the list of ONC constants, see comment in the other CL. > > Hmm. Maybe I misunderstood your other comment. From that comment, I thought > you wanted kAllTypes to be defined in this file, but this comment seems to say > otherwise. Can you explain again? Sorry, misstyped. I meant "shouldn't". But I get your argument and, we may keep it for now. We really have to find a good way to validate different ONC versions.
Adding davemoore for OWNERS review of chromeos/dbus/shill_service_client.cc and chrome/browser/profiles/profile_dependency_manager.cc Adding kalman for OWNERS review of: chrome/browser/chromeos/extensions/* chrome/browser/extensions/* chrome/common/extensions/* (The private extension API has already been reviewed and received approval.)
https://codereview.chromium.org/11975015/diff/45002/chrome/browser/chromeos/e... File chrome/browser/chromeos/extensions/networking_private_api.cc (right): https://codereview.chromium.org/11975015/diff/45002/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:65: if (!args_->GetString(0, &service_path)) Do you know about the JSON schema compiler? Ideally you should use it, but maybe there's a reason you haven't. The compiler generates all the Value boilerplate here, as well as making the resource file, extension_api.cc, and RegisterFunction boilerplate unnecessary. List networkingPrivate.json in api.gyp and set the necessary compiler_options (see chrome/common/extensions/api/file_browser_handler_internal.json), then here you'd have scoped_ptr<api::GetProperties::Params> params = api::GetProperties::Params::Create(*args_); EXTENSION_FUNCTION_VALIDATE(params); DBusThreadManager::Get()->GetShillServiceClient()->GetProperties( dbus::ObjectPath(params->networkGuid), base::Bind(&NetworkingGetPropertiesFunction::ResultCallback, this)); return true; https://codereview.chromium.org/11975015/diff/45002/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:70: &NetworkingGetPropertiesFunction::ResultCallback, this)); nit: line arguments vertically (like above) https://codereview.chromium.org/11975015/diff/45002/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:80: SetResult(filtered_result.release()); And here, you'd make CreateFilteredResult return the generate type data, like scoped_ptr<api::Properties> CreateFilteredResult(...) {} then use like result_ = api::GetProperties::Result::Create( CreateFilteredResult(...).release()); or whatever it is. Something along those lines that makes sense structurally. This comment and the one above apply to all API bindings, and should make this stuff a lot simpler. By the way, if in doubt you will be able to see the code that got generate for your API in out/Debug/gen/chrome/common/extensions/api/networking_private.h https://codereview.chromium.org/11975015/diff/45002/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:87: : public base::RefCounted<ResultList> { does this really need to be refcounted? https://codereview.chromium.org/11975015/diff/45002/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:230: const std::string& errorName, error_name not errorName (same below etc) https://codereview.chromium.org/11975015/diff/45002/chrome/common/extensions/... File chrome/common/extensions/api/networking_private.json (right): https://codereview.chromium.org/11975015/diff/45002/chrome/common/extensions/... chrome/common/extensions/api/networking_private.json:26: "additionalProperties": { "type": "any" }, Why isn't it possible to enumerate the properties? https://codereview.chromium.org/11975015/diff/45002/chrome/common/extensions/... chrome/common/extensions/api/networking_private.json:52: "additionalProperties": { "type": "any"} } Ditto. In fact will this be the same data as returned by getProperties? In which case it should be defined as a type. https://codereview.chromium.org/11975015/diff/45002/chrome/common/extensions/... chrome/common/extensions/api/networking_private.json:59: "name": "requestConnect", "request" seems tautological, since an API call is effectively a request. Just "connect"? https://codereview.chromium.org/11975015/diff/45002/chrome/common/extensions/... chrome/common/extensions/api/networking_private.json:77: "name": "requestDisconnect", ditto https://codereview.chromium.org/11975015/diff/45002/chrome/test/data/extensio... File chrome/test/data/extensions/api_test/networking/background.js (right): https://codereview.chromium.org/11975015/diff/45002/chrome/test/data/extensio... chrome/test/data/extensions/api_test/networking/background.js:7: }); Is there a reason the test can't be entirely run from the background page? https://codereview.chromium.org/11975015/diff/45002/chrome/test/data/extensio... File chrome/test/data/extensions/api_test/networking/test.js (right): https://codereview.chromium.org/11975015/diff/45002/chrome/test/data/extensio... chrome/test/data/extensions/api_test/networking/test.js:5: var EXTENSION_ID = 'epcifkihnkjgphfkloaaleeakhpmgdmn'; There's actually a property chrome.runtime.id that contains the extension ID automatically. https://codereview.chromium.org/11975015/diff/45002/chrome/test/data/extensio... chrome/test/data/extensions/api_test/networking/test.js:12: function(result) { For asynchronous callbacks you need to wrap them in callbackPass() to keep the test running - otherwise the next test will run, which might interfere with things - and the test might think that it's finished when in fact it hasn't. You also don't need the chrome.test.succeed() nor the chrome.runtime.lastError check in that case, they will happen automatically. https://codereview.chromium.org/11975015/diff/45002/chrome/test/data/extensio... chrome/test/data/extensions/api_test/networking/test.js:15: console.log("Visible Networks: " + JSON.stringify(result)); Remove logging before submitting https://codereview.chromium.org/11975015/diff/45002/chrome/test/data/extensio... chrome/test/data/extensions/api_test/networking/test.js:16: chrome.test.assertEq(4, result.length); assertEq works with objects, so chrome.test.assertEq([ { "Name": "eth0" }, { "Name": "wifi1" }, ... ], result); would be easier to read. Incidentally, usually object keys are camelCase, so I'd expect these to be "name", "connectionState", "type", etc. These comments apply to all the other tests too. https://codereview.chromium.org/11975015/diff/45002/chrome/test/data/extensio... chrome/test/data/extensions/api_test/networking/test.js:93: chrome.runtime.lastError.message); For these, use callbackFail - it's the same as callbackPass but accepts a second argument, the expected failure message.
https://codereview.chromium.org/11975015/diff/45002/chrome/browser/chromeos/e... File chrome/browser/chromeos/extensions/networking_private_api.cc (right): https://codereview.chromium.org/11975015/diff/45002/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:65: if (!args_->GetString(0, &service_path)) On 2013/01/23 22:37:52, kalman wrote: > Do you know about the JSON schema compiler? Ideally you should use it, but maybe > there's a reason you haven't. Well, I didn't know about it until I had already coded it this way. Looking at the docs, it seemed as though either way was acceptable, but as using the schema compiler is the "newer" way, I've rewritten it to use that. I looked at moving it to WebIDL, but that didn't seem to have all the capabilities I needed. https://codereview.chromium.org/11975015/diff/45002/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:70: &NetworkingGetPropertiesFunction::ResultCallback, this)); On 2013/01/23 22:37:52, kalman wrote: > nit: line arguments vertically (like above) Done. https://codereview.chromium.org/11975015/diff/45002/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:87: : public base::RefCounted<ResultList> { On 2013/01/23 22:37:52, kalman wrote: > does this really need to be refcounted? Yes, it's kind of the reason this class exists: it's assembling the results of multiple async calls to Shill, and so each callback needs a reference to the list, and reference counting is the easiest way to assure that they all disappear automatically when the callback objects go away. https://codereview.chromium.org/11975015/diff/45002/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:230: const std::string& errorName, On 2013/01/23 22:37:52, kalman wrote: > error_name not errorName (same below etc) Done. https://codereview.chromium.org/11975015/diff/45002/chrome/common/extensions/... File chrome/common/extensions/api/networking_private.json (right): https://codereview.chromium.org/11975015/diff/45002/chrome/common/extensions/... chrome/common/extensions/api/networking_private.json:26: "additionalProperties": { "type": "any" }, On 2013/01/23 22:37:52, kalman wrote: > Why isn't it possible to enumerate the properties? It is possible, but will have to be kept in sync with the ONC spec. Currently, this API only supports five properties, but it will very shortly be more like 40, and that seems prohibitive to maintain. I have a hard time making a "correctness" argument for keeping it unenumerated however, so if you want I can enumerate the five properties for now, and when it gets hard to maintain we can switch it back to this... https://codereview.chromium.org/11975015/diff/45002/chrome/common/extensions/... chrome/common/extensions/api/networking_private.json:52: "additionalProperties": { "type": "any"} } On 2013/01/23 22:37:52, kalman wrote: > Ditto. In fact will this be the same data as returned by getProperties? In which > case it should be defined as a type. Good point. I defined it as a type. https://codereview.chromium.org/11975015/diff/45002/chrome/common/extensions/... chrome/common/extensions/api/networking_private.json:59: "name": "requestConnect", On 2013/01/23 22:37:52, kalman wrote: > "request" seems tautological, since an API call is effectively a request. Just > "connect"? Well, but the reason it's a "request" is because when this call finishes successfully, it's not connected to a network, it's just successfully requested a connection to a network from Shill. If it were just "connect", then one might expect that the network is connected when this function calls it's closure, and that's not correct. https://codereview.chromium.org/11975015/diff/45002/chrome/test/data/extensio... File chrome/test/data/extensions/api_test/networking/background.js (right): https://codereview.chromium.org/11975015/diff/45002/chrome/test/data/extensio... chrome/test/data/extensions/api_test/networking/background.js:7: }); On 2013/01/23 22:37:52, kalman wrote: > Is there a reason the test can't be entirely run from the background page? No. Done.
https://codereview.chromium.org/11975015/diff/45002/chrome/test/data/extensio... File chrome/test/data/extensions/api_test/networking/background.js (right): https://codereview.chromium.org/11975015/diff/45002/chrome/test/data/extensio... chrome/test/data/extensions/api_test/networking/background.js:7: }); On 2013/01/31 16:40:36, Greg Spencer (Chromium) wrote: > On 2013/01/23 22:37:52, kalman wrote: > > Is there a reason the test can't be entirely run from the background page? > > No. Done. [Whoops, forgot to change this comment before publishing.] Actually, I tried this, and the browser tests just hangs, so I guess that's not going to work.
https://codereview.chromium.org/11975015/diff/45002/chrome/browser/chromeos/e... File chrome/browser/chromeos/extensions/networking_private_api.cc (right): https://codereview.chromium.org/11975015/diff/45002/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:65: if (!args_->GetString(0, &service_path)) On 2013/01/31 16:40:36, Greg Spencer (Chromium) wrote: > On 2013/01/23 22:37:52, kalman wrote: > > Do you know about the JSON schema compiler? Ideally you should use it, but > maybe > > there's a reason you haven't. > > Well, I didn't know about it until I had already coded it this way. Looking at > the docs, it seemed as though either way was acceptable, but as using the schema > compiler is the "newer" way, I've rewritten it to use that. I looked at moving > it to WebIDL, but that didn't seem to have all the capabilities I needed. NOTE(chrome-extensions-team): we need to update the docs. https://codereview.chromium.org/11975015/diff/45002/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:87: : public base::RefCounted<ResultList> { On 2013/01/31 16:40:36, Greg Spencer (Chromium) wrote: > On 2013/01/23 22:37:52, kalman wrote: > > does this really need to be refcounted? > > Yes, it's kind of the reason this class exists: it's assembling the results of > multiple async calls to Shill, and so each callback needs a reference to the > list, and reference counting is the easiest way to assure that they all > disappear automatically when the callback objects go away. Ok, cool, you're right. However I think I asked the wrong question here: the problem as you say is that there are multiple async calls. However, I think a cleaner approach to solving this problem is making it a single object's responsibility to mediate this rather than a combination of two classes collaborating (ResultList and NetworkingGetVisibleNetworksFunction). I imagine it would look something like: class ResultList : public base::RefCounted<ResultList> { public: ResultList(services, shill_service_client, object_arg_thingy, callback) : callback_(callback) { for (service : services) { shill_service_client->GetProperties( object_arg_thingy, base::Bind(&OnGetPropertiesResponse, this)); } } private: friend class base::RefCounted<ResultList>; void OnGetPropertiesResult(...) { DCHECK(BrowserThread::CurrentlyOn(UI)) // or whatever result_.Append(...); } ~ResultList() { callback_.Run(result_); } result_; }; the class could probably do with a different name then. And yeah, the |callback| arg would basically be just SetResult and a SendResponse. https://codereview.chromium.org/11975015/diff/45002/chrome/common/extensions/... File chrome/common/extensions/api/networking_private.json (right): https://codereview.chromium.org/11975015/diff/45002/chrome/common/extensions/... chrome/common/extensions/api/networking_private.json:26: "additionalProperties": { "type": "any" }, On 2013/01/31 16:40:36, Greg Spencer (Chromium) wrote: > On 2013/01/23 22:37:52, kalman wrote: > > Why isn't it possible to enumerate the properties? > > It is possible, but will have to be kept in sync with the ONC spec. Currently, > this API only supports five properties, but it will very shortly be more like > 40, and that seems prohibitive to maintain. > > I have a hard time making a "correctness" argument for keeping it unenumerated > however, so if you want I can enumerate the five properties for now, and when it > gets hard to maintain we can switch it back to this... Yeah, I have a strong preference to making this an enum - if it's a matter of keeping it in sync with an enum elsewhere, making the compiler check it could look something like // (declare make NetworkConfig an enum type) const char* GetNetworkConfigName(api::networking_private::NetworkConfig name) { switch (name) { case api::networking_private::NETWORK_CONFIG_WIFI: return onc::network_constants::kWiFi; ... // No default case so that compiler picks up problems. } NOTREACHED(); return ""; } At least then you would only need to update a single place (the JSON) and a comment to that effect in onc_constants.h seems like it would suffice. If for some reason that isn't practical I'd still like to try making it an enum then as you suggest switch it to "additionalProperties:" {"type": "string"} if necessary. https://codereview.chromium.org/11975015/diff/45002/chrome/common/extensions/... chrome/common/extensions/api/networking_private.json:59: "name": "requestConnect", On 2013/01/31 16:40:36, Greg Spencer (Chromium) wrote: > On 2013/01/23 22:37:52, kalman wrote: > > "request" seems tautological, since an API call is effectively a request. Just > > "connect"? > > Well, but the reason it's a "request" is because when this call finishes > successfully, it's not connected to a network, it's just successfully requested > a connection to a network from Shill. If it were just "connect", then one might > expect that the network is connected when this function calls it's closure, and > that's not correct. Ok - so to make sure I understand - there are two events that will happen here. (1) it requests a connection, (2) the connection is (or isn't) established. Currently this API call returns when (1), but would it make sense to return when it's (2)? If not, should there be a way to notify (e.g. an event)? "requestConnect" still sounds to me like it could be denied, but the only way it will fail is if there's a hard error, so I'd prefer name like "startConnect" or "beginConnect" or something (same for disconnect). https://codereview.chromium.org/11975015/diff/45002/chrome/test/data/extensio... File chrome/test/data/extensions/api_test/networking/background.js (right): https://codereview.chromium.org/11975015/diff/45002/chrome/test/data/extensio... chrome/test/data/extensions/api_test/networking/background.js:7: }); On 2013/01/31 16:43:01, Greg Spencer (Chromium) wrote: > On 2013/01/31 16:40:36, Greg Spencer (Chromium) wrote: > > On 2013/01/23 22:37:52, kalman wrote: > > > Is there a reason the test can't be entirely run from the background page? > > > > No. Done. > > [Whoops, forgot to change this comment before publishing.] > > Actually, I tried this, and the browser tests just hangs, so I guess that's not > going to work. I think it will work, it's a matter of making the test pass... either that or this API really doesn't work from background pages, and that would be a problem. https://codereview.chromium.org/11975015/diff/47001/chrome/browser/chromeos/e... File chrome/browser/chromeos/extensions/networking_private_api.cc (right): https://codereview.chromium.org/11975015/diff/47001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:26: const char kInvalidResponseError[] = "Error.invalidResponse"; These errors are printed in the extension's console so it's nice to make them descriptive. It's also fine to use StringPrintf or something to include some more info about the error, if you think that'd be useful for debugging extensions. https://codereview.chromium.org/11975015/diff/47001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:62: // NetworkingPrivateGetPropertiesFunction I find these big comments distracting. Of course, I may be in the minority. https://codereview.chromium.org/11975015/diff/47001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:124: list_.push_back(linked_ptr<api::NetworkProperties>(value)); implement these methods inline? https://codereview.chromium.org/11975015/diff/47001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:211: // TODO(gspencer): For now the "guid" we send back is going to look too much indent https://codereview.chromium.org/11975015/diff/47001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:283: const std::string& errorMessage) { errorName/errorMessage -> error_name/error_message https://codereview.chromium.org/11975015/diff/47001/chrome/browser/chromeos/e... File chrome/browser/chromeos/extensions/networking_private_api.h (right): https://codereview.chromium.org/11975015/diff/47001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.h:87: const std::string& errorMessage); error_name, error_message, same below in Disconnect. https://codereview.chromium.org/11975015/diff/47001/chrome/test/data/extensio... File chrome/test/data/extensions/api_test/networking/test.js (right): https://codereview.chromium.org/11975015/diff/47001/chrome/test/data/extensio... chrome/test/data/extensions/api_test/networking/test.js:12: function(result) { I started making comments here but realised they were the exact same comments I made last round.
https://codereview.chromium.org/11975015/diff/45002/chrome/browser/chromeos/e... File chrome/browser/chromeos/extensions/networking_private_api.cc (right): https://codereview.chromium.org/11975015/diff/45002/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:87: : public base::RefCounted<ResultList> { On 2013/02/01 19:09:42, kalman wrote: > And yeah, the |callback| arg would basically be just SetResult and a > SendResponse. OK, I like this a little better: at least we can get rid of the count, and use the refcount instead. But why does the ResultList need to be the thing that calls GetProperties? Seems like once all the callbacks disappear, all of the references will be gone, and it just needs to call a callback, so the ResultList could just take the callback as an argument, and the rest of the code can stay the same... I've done it that way (because it's easier to do), let me know if that's sufficient. Also, realize that this implementation is calling Shill directly because we didn't want to make this API addition dependent upon other network code changes (to avoid making this task dependent on the network code changes being ready for 26), so this implementation is likely to be short-lived. As soon as the network code changes are in (and they almost are), then this will use the new NetworkStateHandler API instead of calling Shill directly, and all of these shenanigans with the collection of async results will go away. https://codereview.chromium.org/11975015/diff/45002/chrome/common/extensions/... File chrome/common/extensions/api/networking_private.json (right): https://codereview.chromium.org/11975015/diff/45002/chrome/common/extensions/... chrome/common/extensions/api/networking_private.json:26: "additionalProperties": { "type": "any" }, On 2013/02/01 19:09:42, kalman wrote: > At least then you would only need to update a single place (the JSON) and a > comment to that effect in onc_constants.h seems like it would suffice. > > If for some reason that isn't practical I'd still like to try making it an enum > then as you suggest switch it to "additionalProperties:" {"type": "string"} if > necessary. No, it's not really practical because we already have constants for these values in chromeos/network/onc/onc_constants.h, and that can't be dependent upon a generated header from the schema compiler. I started to change this to call out the (for now) five properties that can be present, but quickly realized that it's not that simple: the WiFi value is an object that can have other nested objects, etc, and there a a lot of other objects like that which will be present in the future. It's really too bad that this JSON schema doesn't allow us to have objects with an empty set of required properties (making all properties optional), so that I could stop at specifying the top level properties and just specify that WiFi is an object with no required properties. The "additional properties" thing is a hack, and makes it hard with nested values, since I'd have to traverse the result and "explode" all of the "additional_properties" elements in the results so that they are presented to the JavaScript as correct ONC dictionaries. I *could* still make the fields explicit, but I think it's an awful lot of pain for very little gain. As an example, a WiFi object looks something like this (and there are several more optional fields that aren't included in this example): "WiFi": { "AutoConnect": false, "EAP": { "ClientCertPattern": { "EnrollmentURI": [ "http://fetch-my-certificate.com" ], "IssuerCARef": [ "{6ed8dce9-64c8-d568-d225d7e467e37828}" ] }, "ClientCertType": "Pattern", "Outer": "EAP-TLS", "ServerCARef": "{6ed8dce9-64c8-d568-d225d7e467e37828}", "UseSystemCAs": true }, "HiddenSSID": false, "SSID": "MyTTLSNetwork", "Security": "WPA-EAP" } https://codereview.chromium.org/11975015/diff/45002/chrome/common/extensions/... chrome/common/extensions/api/networking_private.json:59: "name": "requestConnect", On 2013/02/01 19:09:42, kalman wrote: > On 2013/01/31 16:40:36, Greg Spencer (Chromium) wrote: > > On 2013/01/23 22:37:52, kalman wrote: > > > "request" seems tautological, since an API call is effectively a request. > Just > > > "connect"? > > > > Well, but the reason it's a "request" is because when this call finishes > > successfully, it's not connected to a network, it's just successfully > requested > > a connection to a network from Shill. If it were just "connect", then one > might > > expect that the network is connected when this function calls it's closure, > and > > that's not correct. > > Ok - so to make sure I understand - there are two events that will happen here. > (1) it requests a connection, (2) the connection is (or isn't) established. > > Currently this API call returns when (1), but would it make sense to return when > it's (2)? If not, should there be a way to notify (e.g. an event)? > > "requestConnect" still sounds to me like it could be denied, but the only way it > will fail is if there's a hard error, so I'd prefer name like "startConnect" or > "beginConnect" or something (same for disconnect). There is an event coming in a later CL that will propagate network changes, including connection state. I've changed the names to startConnect and startDisconnect. https://codereview.chromium.org/11975015/diff/45002/chrome/test/data/extensio... File chrome/test/data/extensions/api_test/networking/background.js (right): https://codereview.chromium.org/11975015/diff/45002/chrome/test/data/extensio... chrome/test/data/extensions/api_test/networking/background.js:7: }); On 2013/02/01 19:09:42, kalman wrote: > On 2013/01/31 16:43:01, Greg Spencer (Chromium) wrote: > > On 2013/01/31 16:40:36, Greg Spencer (Chromium) wrote: > > > On 2013/01/23 22:37:52, kalman wrote: > > > > Is there a reason the test can't be entirely run from the background page? > > > > > > No. Done. > > > > [Whoops, forgot to change this comment before publishing.] > > > > Actually, I tried this, and the browser tests just hangs, so I guess that's > not > > going to work. > > I think it will work, it's a matter of making the test pass... either that or > this API really doesn't work from background pages, and that would be a problem. Huh, well I tried it again (before making your other changes in test.js), and it worked just fine this time. Not sure what I did incorrectly last time. I ran it several times to make sure it wasn't flaky (which it shouldn't be: it's using a stub). https://codereview.chromium.org/11975015/diff/45002/chrome/test/data/extensio... File chrome/test/data/extensions/api_test/networking/test.js (right): https://codereview.chromium.org/11975015/diff/45002/chrome/test/data/extensio... chrome/test/data/extensions/api_test/networking/test.js:5: var EXTENSION_ID = 'epcifkihnkjgphfkloaaleeakhpmgdmn'; On 2013/01/23 22:37:52, kalman wrote: > There's actually a property chrome.runtime.id that contains the extension ID > automatically. Done. https://codereview.chromium.org/11975015/diff/45002/chrome/test/data/extensio... chrome/test/data/extensions/api_test/networking/test.js:12: function(result) { On 2013/01/23 22:37:52, kalman wrote: > For asynchronous callbacks you need to wrap them in callbackPass() to keep the > test running - otherwise the next test will run, which might interfere with > things - and the test might think that it's finished when in fact it hasn't. > > You also don't need the chrome.test.succeed() nor the chrome.runtime.lastError > check in that case, they will happen automatically. Done. https://codereview.chromium.org/11975015/diff/45002/chrome/test/data/extensio... chrome/test/data/extensions/api_test/networking/test.js:15: console.log("Visible Networks: " + JSON.stringify(result)); On 2013/01/23 22:37:52, kalman wrote: > Remove logging before submitting Done. https://codereview.chromium.org/11975015/diff/45002/chrome/test/data/extensio... chrome/test/data/extensions/api_test/networking/test.js:16: chrome.test.assertEq(4, result.length); On 2013/01/23 22:37:52, kalman wrote: > assertEq works with objects, so > > chrome.test.assertEq([ > { "Name": "eth0" }, > { "Name": "wifi1" }, > ... > ], result); > > would be easier to read. Done. The only bad thing about doing it this way is that if something fails you don't immediately see in the results what field was missing/wrong, and have to diff the objects visually, but the code is more readable. > Incidentally, usually object keys are camelCase, so I'd expect these to be > "name", "connectionState", "type", etc. > > These comments apply to all the other tests too. These values are using ONC naming conventions, which is why it's not camel case: it wouldn't be valid ONC then. https://codereview.chromium.org/11975015/diff/45002/chrome/test/data/extensio... chrome/test/data/extensions/api_test/networking/test.js:93: chrome.runtime.lastError.message); On 2013/01/23 22:37:52, kalman wrote: > For these, use callbackFail - it's the same as callbackPass but accepts a second > argument, the expected failure message. Done.
https://codereview.chromium.org/11975015/diff/45002/chrome/browser/chromeos/e... File chrome/browser/chromeos/extensions/networking_private_api.cc (right): https://codereview.chromium.org/11975015/diff/45002/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:87: : public base::RefCounted<ResultList> { On 2013/02/01 23:35:55, Greg Spencer (Chromium) wrote: > On 2013/02/01 19:09:42, kalman wrote: > > And yeah, the |callback| arg would basically be just SetResult and a > > SendResponse. > > OK, I like this a little better: at least we can get rid of the count, and use > the refcount instead. > > But why does the ResultList need to be the thing that calls GetProperties? > Seems like once all the callbacks disappear, all of the references will be gone, > and it just needs to call a callback, so the ResultList could just take the > callback as an argument, and the rest of the code can stay the same... > > I've done it that way (because it's easier to do), let me know if that's > sufficient. Well the reason is like I said, at the moment it's the responsibility of two classes to gather the results and the control flow bounces back and forth. I would find it much easier to follow if all of the logic and all of the data were in a single place - here that means within ResultList. There would be no methods exposed on ResultList at all, everything private. Law of demeter etc etc. You could probably even make this class declared within an anonymous namespace and not have a presence in the header file at all. In fact you might be able to do it now anyway. > > Also, realize that this implementation is calling Shill directly because we > didn't want to make this API addition dependent upon other network code changes > (to avoid making this task dependent on the network code changes being ready for > 26), so this implementation is likely to be short-lived. As soon as the network > code changes are in (and they almost are), then this will use the new > NetworkStateHandler API instead of calling Shill directly, and all of these > shenanigans with the collection of async results will go away. If it really is all going to go away, and ResultList really will be entirely unnecessary, then it seems ok leave well enough alone and move on. However, I've been bitten almost every time by somebody claiming they'll fix something because things are about to change, and then it never happens. But alright. Change my assumptions. https://codereview.chromium.org/11975015/diff/45002/chrome/common/extensions/... File chrome/common/extensions/api/networking_private.json (right): https://codereview.chromium.org/11975015/diff/45002/chrome/common/extensions/... chrome/common/extensions/api/networking_private.json:26: "additionalProperties": { "type": "any" }, On 2013/02/01 23:35:55, Greg Spencer (Chromium) wrote: > On 2013/02/01 19:09:42, kalman wrote: > > At least then you would only need to update a single place (the JSON) and a > > comment to that effect in onc_constants.h seems like it would suffice. > > > > If for some reason that isn't practical I'd still like to try making it an > enum > > then as you suggest switch it to "additionalProperties:" {"type": "string"} if > > necessary. > > No, it's not really practical because we already have constants for these values > in chromeos/network/onc/onc_constants.h, and that can't be dependent upon a > generated header from the schema compiler. > > I started to change this to call out the (for now) five properties that can be > present, but quickly realized that it's not that simple: the WiFi value is an > object that can have other nested objects, etc, and there a a lot of other > objects like that which will be present in the future. > > It's really too bad that this JSON schema doesn't allow us to have objects with > an empty set of required properties (making all properties optional), so that I > could stop at specifying the top level properties and just specify that WiFi is > an object with no required properties. > > The "additional properties" thing is a hack, and makes it hard with nested > values, since I'd have to traverse the result and "explode" all of the > "additional_properties" elements in the results so that they are presented to > the JavaScript as correct ONC dictionaries. > > I *could* still make the fields explicit, but I think it's an awful lot of pain > for very little gain. > > As an example, a WiFi object looks something like this (and there are several > more optional fields that aren't included in this example): > > "WiFi": { > "AutoConnect": false, > "EAP": { > "ClientCertPattern": { > "EnrollmentURI": [ > "http://fetch-my-certificate.com" > ], > "IssuerCARef": [ > "{6ed8dce9-64c8-d568-d225d7e467e37828}" > ] > }, > "ClientCertType": "Pattern", > "Outer": "EAP-TLS", > "ServerCARef": "{6ed8dce9-64c8-d568-d225d7e467e37828}", > "UseSystemCAs": true > }, > "HiddenSSID": false, > "SSID": "MyTTLSNetwork", > "Security": "WPA-EAP" > } Ah, I didn't realise these were rich structures, I thought they were just strings. My apologies. https://codereview.chromium.org/11975015/diff/45002/chrome/test/data/extensio... File chrome/test/data/extensions/api_test/networking/test.js (right): https://codereview.chromium.org/11975015/diff/45002/chrome/test/data/extensio... chrome/test/data/extensions/api_test/networking/test.js:16: chrome.test.assertEq(4, result.length); On 2013/02/01 23:35:55, Greg Spencer (Chromium) wrote: > On 2013/01/23 22:37:52, kalman wrote: > > assertEq works with objects, so > > > > chrome.test.assertEq([ > > { "Name": "eth0" }, > > { "Name": "wifi1" }, > > ... > > ], result); > > > > would be easier to read. > > Done. The only bad thing about doing it this way is that if something fails you > don't immediately see in the results what field was missing/wrong, and have to > diff the objects visually, but the code is more readable. > If this becomes a problem it's something we can fix within assertEq. https://codereview.chromium.org/11975015/diff/61001/chrome/browser/chromeos/e... File chrome/browser/chromeos/extensions/networking_private_api.cc (right): https://codereview.chromium.org/11975015/diff/61001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:126: } Like I said, it's odd visually to me that one of the methods is implemented within the class declaration and the rest aren't. I like implementing them all in the class declaration; but I don't mind as long as it's consistent. https://codereview.chromium.org/11975015/diff/57003/chrome/browser/chromeos/e... File chrome/browser/chromeos/extensions/networking_private_api.cc (right): https://codereview.chromium.org/11975015/diff/57003/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:92: // NetworkingPrivateGetVisibleNetworksFunction::ResultList if you put the method bodies all within the class declaration, you could delete this banner... https://codereview.chromium.org/11975015/diff/57003/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:209: // TODO(gspencer): For now the "GUID" we send back is going to look This is indented 6 spaces. It should only be indented 2. https://codereview.chromium.org/11975015/diff/57003/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:241: const std::string& errorName, errorName -> error_name errorMessage -> error_message https://codereview.chromium.org/11975015/diff/57003/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:279: const std::string& errorName, ditto ????????? https://codereview.chromium.org/11975015/diff/57003/chrome/common/extensions/... File chrome/common/extensions/api/networking_private.json (right): https://codereview.chromium.org/11975015/diff/57003/chrome/common/extensions/... chrome/common/extensions/api/networking_private.json:11: "nodoc": "true", nodoc is unnecessary since you haven't added an HTML file for it. https://codereview.chromium.org/11975015/diff/57003/chrome/test/data/extensio... File chrome/test/data/extensions/api_test/networking/test.js (right): https://codereview.chromium.org/11975015/diff/57003/chrome/test/data/extensio... chrome/test/data/extensions/api_test/networking/test.js:5: var EXTENSION_ID = chrome.runtime.id; this alias doesn't seem like it's saving much also it's not being used https://codereview.chromium.org/11975015/diff/57003/chrome/test/data/extensio... chrome/test/data/extensions/api_test/networking/test.js:6: var FILE_CONTENTS = 'hello from test extension.'; neither is this https://codereview.chromium.org/11975015/diff/57003/chrome/test/data/extensio... chrome/test/data/extensions/api_test/networking/test.js:89: assertTrue(!chrome.runtime.lastError); not necessary https://codereview.chromium.org/11975015/diff/57003/chrome/test/data/extensio... chrome/test/data/extensions/api_test/networking/test.js:96: assertTrue(!chrome.runtime.lastError); not necessary
https://codereview.chromium.org/11975015/diff/45002/chrome/browser/chromeos/e... File chrome/browser/chromeos/extensions/networking_private_api.cc (right): https://codereview.chromium.org/11975015/diff/45002/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:87: : public base::RefCounted<ResultList> { It wasn't too hard to do, so I went ahead and did it. [I couldn't have removed the declaration from the header file before because the class was used as an argument to some of the callbacks] https://codereview.chromium.org/11975015/diff/57003/chrome/browser/chromeos/e... File chrome/browser/chromeos/extensions/networking_private_api.cc (right): https://codereview.chromium.org/11975015/diff/57003/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:92: // NetworkingPrivateGetVisibleNetworksFunction::ResultList On 2013/02/02 00:11:32, kalman wrote: > if you put the method bodies all within the class declaration, you could delete > this banner... Done. https://codereview.chromium.org/11975015/diff/57003/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:209: // TODO(gspencer): For now the "GUID" we send back is going to look On 2013/02/02 00:11:32, kalman wrote: > This is indented 6 spaces. It should only be indented 2. Whoops. Fixed. https://codereview.chromium.org/11975015/diff/57003/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:241: const std::string& errorName, On 2013/02/02 00:11:32, kalman wrote: > errorName -> error_name > errorMessage -> error_message I thought I did this already?? Maybe I just did it in the header... https://codereview.chromium.org/11975015/diff/57003/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.cc:279: const std::string& errorName, On 2013/02/02 00:11:32, kalman wrote: > ditto > > ????????? Done. https://codereview.chromium.org/11975015/diff/57003/chrome/common/extensions/... File chrome/common/extensions/api/networking_private.json (right): https://codereview.chromium.org/11975015/diff/57003/chrome/common/extensions/... chrome/common/extensions/api/networking_private.json:11: "nodoc": "true", On 2013/02/02 00:11:32, kalman wrote: > nodoc is unnecessary since you haven't added an HTML file for it. Done. https://codereview.chromium.org/11975015/diff/57003/chrome/test/data/extensio... File chrome/test/data/extensions/api_test/networking/test.js (right): https://codereview.chromium.org/11975015/diff/57003/chrome/test/data/extensio... chrome/test/data/extensions/api_test/networking/test.js:5: var EXTENSION_ID = chrome.runtime.id; Removed. https://codereview.chromium.org/11975015/diff/57003/chrome/test/data/extensio... chrome/test/data/extensions/api_test/networking/test.js:6: var FILE_CONTENTS = 'hello from test extension.'; On 2013/02/02 00:11:32, kalman wrote: > neither is this Removed. https://codereview.chromium.org/11975015/diff/57003/chrome/test/data/extensio... chrome/test/data/extensions/api_test/networking/test.js:89: assertTrue(!chrome.runtime.lastError); On 2013/02/02 00:11:32, kalman wrote: > not necessary Done. https://codereview.chromium.org/11975015/diff/57003/chrome/test/data/extensio... chrome/test/data/extensions/api_test/networking/test.js:96: assertTrue(!chrome.runtime.lastError); On 2013/02/02 00:11:32, kalman wrote: > not necessary Done.
lgtm https://codereview.chromium.org/11975015/diff/68001/chrome/browser/chromeos/e... File chrome/browser/chromeos/extensions/networking_private_api.h (right): https://codereview.chromium.org/11975015/diff/68001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.h:79: void ConnectionStartFailed(const std::string& errorName, error_name https://codereview.chromium.org/11975015/diff/68001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.h:80: const std::string& errorMessage); error_message https://codereview.chromium.org/11975015/diff/68001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.h:104: void DisconnectionStartFailed(const std::string& errorName, error_name https://codereview.chromium.org/11975015/diff/68001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.h:105: const std::string& errorMessage); error_message
https://codereview.chromium.org/11975015/diff/68001/chrome/browser/chromeos/e... File chrome/browser/chromeos/extensions/networking_private_api.h (right): https://codereview.chromium.org/11975015/diff/68001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/networking_private_api.h:79: void ConnectionStartFailed(const std::string& errorName, On 2013/02/02 01:08:17, kalman wrote: > error_name Gah! Hopefully I've got all of these now. I think I changed it once, and then accidentally reverted it.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gspencer@chromium.org/11975015/76002
Presubmit check for 11975015-76002 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome
Presubmit checks took 3.0s to calculate.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gspencer@chromium.org/11975015/76002
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gspencer@chromium.org/11975015/76002
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gspencer@chromium.org/11975015/76002
Failed to apply patch for
chrome/browser/extensions/extension_function_histogram_value.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file chrome/browser/extensions/extension_function_histogram_value.h
Hunk #1 FAILED at 463.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/extensions/extension_function_histogram_value.h.rej
Patch: chrome/browser/extensions/extension_function_histogram_value.h
Index: chrome/browser/extensions/extension_function_histogram_value.h
diff --git a/chrome/browser/extensions/extension_function_histogram_value.h
b/chrome/browser/extensions/extension_function_histogram_value.h
index
5023c49be36034861d36bb483a183da8c5bbfd5c..ce1c5a15a6e32fb5e8e6ea594b4ff2d7bc240973
100644
--- a/chrome/browser/extensions/extension_function_histogram_value.h
+++ b/chrome/browser/extensions/extension_function_histogram_value.h
@@ -463,6 +463,10 @@ enum HistogramValue {
SESSIONRESTORE_RESTORE,
MANAGEMENT_UNINSTALLSELF,
ECHOPRIVATE_GETOOBETIMESTAMP,
+ NETWORKINGPRIVATE_GETPROPERTIES,
+ NETWORKINGPRIVATE_GETVISIBLENETWORKS,
+ NETWORKINGPRIVATE_STARTCONNECT,
+ NETWORKINGPRIVATE_STARTDISCONNECT,
ENUM_BOUNDARY // Last entry: Add new entries above.
};
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gspencer@chromium.org/11975015/89001
Message was sent while issue was closed.
Change committed as 180525 |
