Implement networkingPrivate.setWifiTDLSEnabledState

Implement networkingPrivate.setWifiTDLSEnabledState

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

[stevenjb, 2014-02-06 22:29:18]

asargent@ - for extension_function_histogram_value.h
pwstew@ - FYI, but you may want to look at the logic in
network_device_handler_impl.cc

[Paul Stewart, 2014-02-06 22:58:40]

https://codereview.chromium.org/156353002/diff/80001/chromeos/network/network...
File chromeos/network/network_device_handler_impl.cc (right):

https://codereview.chromium.org/156353002/diff/80001/chromeos/network/network...
chromeos/network/network_device_handler_impl.cc:170: request_delay =
base::TimeDelta::FromMilliseconds(kReRequestDelayMs);
I'm not sure I understand, but:

 - Does this just retry ad-infinitum?  Two issues if that's true:
   - If the error is other than kErrorResultInProgress, you stand to just sit
here forever.
   - If the error is kErrorResultInProgress you still stand to sit here forever
if the IP address never resolves.

https://codereview.chromium.org/156353002/diff/80001/chromeos/network/network...
chromeos/network/network_device_handler_impl.cc:172: next_operation = 
shill::kTDLSStatusOperation;
What is request_delay set to at this point?  Does the compiler warn against
uninitialized variables below?

[Paul Stewart, 2014-02-06 23:01:42]

https://codereview.chromium.org/156353002/diff/80001/chromeos/network/network...
File chromeos/network/network_device_handler_impl.cc (right):

https://codereview.chromium.org/156353002/diff/80001/chromeos/network/network...
chromeos/network/network_device_handler_impl.cc:172: next_operation = 
shill::kTDLSStatusOperation;
On 2014/02/06 22:58:40, Paul Stewart wrote:
> What is request_delay set to at this point?  Does the compiler warn against
> uninitialized variables below?

Just spotted this was an object, which probably is set to some zero value.  You
may not want this value to be zero since the TDLS setup in the driver / firmware
won't necessarily be instant.  Since you return a result to your callback on the
first return of operation != shill::kTDLSSetupOperation, I'd suggest a small (5
second?) delay here.

[tbarzic, 2014-02-07 23:17:40]

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

https://codereview.chromium.org/156353002/diff/80001/chrome/common/extensions...
chrome/common/extensions/api/networking_private.json:369: "description": "If
true, enable TDLS."
this suggests that enabled==false case is no-op, but the existence of
kTDLSTearDownOperation suggests the opposite :)

[asargent_no_longer_on_chrome, 2014-02-10 18:23:25]

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

https://codereview.chromium.org/156353002/diff/80001/chrome/common/extensions...
chrome/common/extensions/api/networking_private.json:364: "description": "The IP
or MAC address for which to enable TDLS."
I am relatively unfamiliar with TDLS, but from my cursory reading it sounds liek
a way to establish device-to-device direct connections between 2 devices without
having to send traffic through a wireless router/access point. Assuming that's
correct, does the |ip_or_mac_address| being referred to here represent a peer
we're willing to connect to (eg TDLS can be enabled/disabled on an individual
basis for each peer)? Or does it represent the ip/mac address of one of our own
adapters, and serves as a master control for whether TDLS will be used to
connect to any peer for this adapter?

Please update the comment here to be specific about this.

https://codereview.chromium.org/156353002/diff/80001/chrome/common/extensions...
chrome/common/extensions/api/networking_private.json:380: "description": "A
callback function that receives a string with the current TDLS state."
What are the expected values of |state|? If this is just used to report the
success of the function call, then you can omit it and just communicate
success/failure with a callback that has no parameters with failure indicated
via a non-null runtime.lastError (which is auto-populated from the boolean you
pass to SendResponse in your NetworkingPrivateSetWifiTDLSEnabledStateFunction
functions, and in error cases sets runtime.lastError.message to the string you
set error_ to). 

If the result of the function is more complicated than just success failure and
you do need the string return value, then you should include in the comment each
of the possible values it can take and a brief explanation of the meaning of
each (if it isn't obvious from the value).

[stevenjb, 2014-02-12 23:22:49]

PTAL

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

https://codereview.chromium.org/156353002/diff/80001/chrome/common/extensions...
chrome/common/extensions/api/networking_private.json:364: "description": "The IP
or MAC address for which to enable TDLS."
On 2014/02/10 18:23:26, Antony Sargent wrote:
> I am relatively unfamiliar with TDLS, but from my cursory reading it sounds
liek
> a way to establish device-to-device direct connections between 2 devices
without
> having to send traffic through a wireless router/access point. Assuming that's
> correct, does the |ip_or_mac_address| being referred to here represent a peer
> we're willing to connect to (eg TDLS can be enabled/disabled on an individual
> basis for each peer)? Or does it represent the ip/mac address of one of our
own
> adapters, and serves as a master control for whether TDLS will be used to
> connect to any peer for this adapter?
> 
> Please update the comment here to be specific about this.

Done.

https://codereview.chromium.org/156353002/diff/80001/chrome/common/extensions...
chrome/common/extensions/api/networking_private.json:369: "description": "If
true, enable TDLS."
On 2014/02/07 23:17:41, tbarzic wrote:
> this suggests that enabled==false case is no-op, but the existence of
> kTDLSTearDownOperation suggests the opposite :)

Done.

https://codereview.chromium.org/156353002/diff/80001/chrome/common/extensions...
chrome/common/extensions/api/networking_private.json:380: "description": "A
callback function that receives a string with the current TDLS state."
On 2014/02/10 18:23:26, Antony Sargent wrote:
> What are the expected values of |state|? If this is just used to report the
> success of the function call, then you can omit it and just communicate
> success/failure with a callback that has no parameters with failure indicated
> via a non-null runtime.lastError (which is auto-populated from the boolean you
> pass to SendResponse in your NetworkingPrivateSetWifiTDLSEnabledStateFunction
> functions, and in error cases sets runtime.lastError.message to the string you
> set error_ to). 
> 
> If the result of the function is more complicated than just success failure
and
> you do need the string return value, then you should include in the comment
each
> of the possible values it can take and a brief explanation of the meaning of
> each (if it isn't obvious from the value). 

Listed possible values; meaning should be clear within the context of someone
using the function.

Note: It has been requested that this be split up into SetEnabled / GetState, so
I will do that in a follow up and list the states in GetState.

https://codereview.chromium.org/156353002/diff/80001/chromeos/network/network...
File chromeos/network/network_device_handler_impl.cc (right):

https://codereview.chromium.org/156353002/diff/80001/chromeos/network/network...
chromeos/network/network_device_handler_impl.cc:170: request_delay =
base::TimeDelta::FromMilliseconds(kReRequestDelayMs);
On 2014/02/06 22:58:40, Paul Stewart wrote:
> I'm not sure I understand, but:
> 
>  - Does this just retry ad-infinitum?  Two issues if that's true:
>    - If the error is other than kErrorResultInProgress, you stand to just sit
> here forever.
>    - If the error is kErrorResultInProgress you still stand to sit here
forever
> if the IP address never resolves.
> 
You're right, we definitely need a timeout here. I'm going to start with 5s. If
it takes longer than that, the Extension can decide to retry or fail.

https://codereview.chromium.org/156353002/diff/80001/chromeos/network/network...
chromeos/network/network_device_handler_impl.cc:172: next_operation = 
shill::kTDLSStatusOperation;
On 2014/02/06 23:01:43, Paul Stewart wrote:
> On 2014/02/06 22:58:40, Paul Stewart wrote:
> > What is request_delay set to at this point?  Does the compiler warn against
> > uninitialized variables below?
> 
> Just spotted this was an object, which probably is set to some zero value. 
You
> may not want this value to be zero since the TDLS setup in the driver /
firmware
> won't necessarily be instant.  Since you return a result to your callback on
the
> first return of operation != shill::kTDLSSetupOperation, I'd suggest a small
(5
> second?) delay here.
I will set the delay to 500ms so that there is an opportunity for early feedback
to the user. I added a getWifiTDLSStatus method so that if the status is still
other than 'Connected' or 'Nonexistent' the extension can decide to poll the
status or not.

[tbarzic, 2014-02-13 18:36:32]

lgtm once the failing test if fixed.

btw. you may want to reupload..side-by-side diffs are screwed up.

https://codereview.chromium.org/156353002/diff/230001/chromeos/network/networ...
File chromeos/network/network_device_handler.h (right):

https://codereview.chromium.org/156353002/diff/230001/chromeos/network/networ...
chromeos/network/network_device_handler.h:43: 
it looks like the errors should be lexicographically sorted

https://codereview.chromium.org/156353002/diff/230001/chromeos/network/networ...
File chromeos/network/network_device_handler_impl.h (right):

https://codereview.chromium.org/156353002/diff/230001/chromeos/network/networ...
chromeos/network/network_device_handler_impl.h:100: 
indent

[Paul Stewart, 2014-02-13 19:46:05]

https://codereview.chromium.org/156353002/diff/490001/chromeos/network/networ...
File chromeos/network/network_device_handler_impl.cc (right):

https://codereview.chromium.org/156353002/diff/490001/chromeos/network/networ...
chromeos/network/network_device_handler_impl.cc:175: } else if (result ==
shill::kErrorResultInProgress &&
I don't think this will work the way you think it will.  In the situation where
the PerformTDLSOperation returns the "shill::kErrorResultInProgress") error
result, I believe network_handler::ShillErrorCallbackFunction() will be called
as opposed this method, since that is what you have bound as an error callback. 
In fact, "shill::kErrorResultInProgress" will probably be the |dbus_error_name|
argument to that method.  I think you need to bind something other than
network_handler::ShillErrorCallbackFunction() and have your retry logic there.

[stevenjb, 2014-02-13 20:58:27]

PTAL

https://codereview.chromium.org/156353002/diff/490001/chromeos/network/networ...
File chromeos/network/network_device_handler_impl.cc (right):

https://codereview.chromium.org/156353002/diff/490001/chromeos/network/networ...
chromeos/network/network_device_handler_impl.cc:175: } else if (result ==
shill::kErrorResultInProgress &&
On 2014/02/13 19:46:06, Paul Stewart wrote:
> I don't think this will work the way you think it will.  In the situation
where
> the PerformTDLSOperation returns the "shill::kErrorResultInProgress") error
> result, I believe network_handler::ShillErrorCallbackFunction() will be called
> as opposed this method, since that is what you have bound as an error
callback. 
> In fact, "shill::kErrorResultInProgress" will probably be the
|dbus_error_name|
> argument to that method.  I think you need to bind something other than
> network_handler::ShillErrorCallbackFunction() and have your retry logic there.

Ugh, right, I should have realized that. Will a successful call with operation
== "Setup" ever have a non empty return value? (We should probably document the
return values in device-api.txt and which operations they apply to, I assume
only "Status"?)

[Paul Stewart, 2014-02-13 21:06:29]

https://codereview.chromium.org/156353002/diff/490001/chromeos/network/networ...
File chromeos/network/network_device_handler_impl.cc (right):

https://codereview.chromium.org/156353002/diff/490001/chromeos/network/networ...
chromeos/network/network_device_handler_impl.cc:175: } else if (result ==
shill::kErrorResultInProgress &&
The only time PerformTDLSOperation (conventionally) returns a non-empty string
is in response to a "Status" call.  All the other functions either return an
empty string or return in error.  I should definitely document return values.

[Paul Stewart, 2014-02-13 21:12:10]

https://codereview.chromium.org/156353002/diff/580001/chromeos/network/networ...
File chromeos/network/network_device_handler_impl.cc (right):

https://codereview.chromium.org/156353002/diff/580001/chromeos/network/networ...
chromeos/network/network_device_handler_impl.cc:156: TDLSOperationParams params,
This looks like it could be const.

[Paul Stewart, 2014-02-13 21:13:35]

https://codereview.chromium.org/156353002/diff/580001/chromeos/network/networ...
File chromeos/network/network_device_handler_impl.cc (right):

https://codereview.chromium.org/156353002/diff/580001/chromeos/network/networ...
chromeos/network/network_device_handler_impl.cc:186: device_path, params,
callback, error_callback),
You probably mean "status_params" here instead of "params" in the arguments
above.

[stevenjb, 2014-02-13 21:48:12]

PTAL

https://codereview.chromium.org/156353002/diff/580001/chromeos/network/networ...
File chromeos/network/network_device_handler_impl.cc (right):

https://codereview.chromium.org/156353002/diff/580001/chromeos/network/networ...
chromeos/network/network_device_handler_impl.cc:156: TDLSOperationParams params,
On 2014/02/13 21:12:11, Paul Stewart wrote:
> This looks like it could be const.
Done, passed as reference everywhere, did an explicit copy for retry.

https://codereview.chromium.org/156353002/diff/580001/chromeos/network/networ...
chromeos/network/network_device_handler_impl.cc:186: device_path, params,
callback, error_callback),
On 2014/02/13 21:13:36, Paul Stewart wrote:
> You probably mean "status_params" here instead of "params" in the arguments
> above.

Gah! Thanks. Added a test for repeat logic.

[Paul Stewart, 2014-02-13 21:58:45]

https://codereview.chromium.org/156353002/diff/680001/chromeos/network/networ...
File chromeos/network/network_device_handler_impl.cc (right):

https://codereview.chromium.org/156353002/diff/680001/chromeos/network/networ...
chromeos/network/network_device_handler_impl.cc:186: device_path, status_params,
callback, error_callback),
This won't go well.  You're passing a reference to a stack-allocated object to
Bind.  That won't go well since by the time CallPerformTDLSOperation is called,
the referenced object will be off the stack.

[stevenjb, 2014-02-13 22:38:45]

https://codereview.chromium.org/156353002/diff/680001/chromeos/network/networ...
File chromeos/network/network_device_handler_impl.cc (right):

https://codereview.chromium.org/156353002/diff/680001/chromeos/network/networ...
chromeos/network/network_device_handler_impl.cc:186: device_path, status_params,
callback, error_callback),
On 2014/02/13 21:58:46, Paul Stewart wrote:
> This won't go well.  You're passing a reference to a stack-allocated object to
> Bind.  That won't go well since by the time CallPerformTDLSOperation is
called,
> the referenced object will be off the stack.
base::Bind handles that, it's some super subtle/clever C++ code, and reference
args must be const. Otherwise we'd have the same problem with device_path and
callback which are also objects passed by reference that may originate on the
stack somewhere. (Basically passing by reference here means we do one copy
instead of two).

[Paul Stewart, 2014-02-13 23:05:30]

On 2014/02/13 22:38:45, stevenjb wrote:
>
https://codereview.chromium.org/156353002/diff/680001/chromeos/network/networ...
> File chromeos/network/network_device_handler_impl.cc (right):
> 
>
https://codereview.chromium.org/156353002/diff/680001/chromeos/network/networ...
> chromeos/network/network_device_handler_impl.cc:186: device_path,
status_params,
> callback, error_callback),
> On 2014/02/13 21:58:46, Paul Stewart wrote:
> > This won't go well.  You're passing a reference to a stack-allocated object
to
> > Bind.  That won't go well since by the time CallPerformTDLSOperation is
> called,
> > the referenced object will be off the stack.
> base::Bind handles that, it's some super subtle/clever C++ code, and reference
> args must be const. Otherwise we'd have the same problem with device_path and
> callback which are also objects passed by reference that may originate on the
> stack somewhere. (Basically passing by reference here means we do one copy
> instead of two).

LGTM if that's how that works.

[stevenjb, 2014-02-14 19:09:32]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-02-14 19:10:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/156353002/970001

[commit-bot: I haz the power, 2014-02-14 19:33:32]

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

[commit-bot: I haz the power, 2014-02-14 19:33:33]

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

[asargent_no_longer_on_chrome, 2014-02-14 20:35:08]

lgtm

https://codereview.chromium.org/156353002/diff/970001/chrome/common/extension...
File chrome/common/extensions/api/networking_private.json (right):

https://codereview.chromium.org/156353002/diff/970001/chrome/common/extension...
chrome/common/extensions/api/networking_private.json:402: "description": "A
callback function that receives a string with the current TDLS status which can
be 'Connected', 'Disabled', 'Disconnected', 'Nonexistent', or 'Unknown'"
I just remembered that you can formally declare the possible values by doing:

"enum": ["Connected", "Disabled", ... etc. ]

here, which has the advantage that you'll get checking of these values for free
and they will also appear in the docs autogeneration.

See chrome/common/extensions/api/management.json /
https://developer.chrome.com/extensions/management.html#type-ExtensionInfo for
one example.

[stevenjb, 2014-02-14 20:46:59]

https://codereview.chromium.org/156353002/diff/970001/chrome/common/extension...
File chrome/common/extensions/api/networking_private.json (right):

https://codereview.chromium.org/156353002/diff/970001/chrome/common/extension...
chrome/common/extensions/api/networking_private.json:402: "description": "A
callback function that receives a string with the current TDLS status which can
be 'Connected', 'Disabled', 'Disconnected', 'Nonexistent', or 'Unknown'"
On 2014/02/14 20:35:08, Antony Sargent wrote:
> I just remembered that you can formally declare the possible values by doing:
> 
> "enum": ["Connected", "Disabled", ... etc. ]
> 
> here, which has the advantage that you'll get checking of these values for
free
> and they will also appear in the docs autogeneration.
> 
> See chrome/common/extensions/api/management.json /
> https://developer.chrome.com/extensions/management.html#type-ExtensionInfo for
> one example. 
> 
> 
Is that:
"type": "enum": ["Connected", "Disabled", ... etc. ]
?

If we change that later will it affect JS code that expects a string? That list
may not be 100% set in stone yet, so I am cautious about using a fixed enum now.

[stevenjb, 2014-02-14 21:00:46]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-02-14 21:07:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/156353002/970001

[commit-bot: I haz the power, 2014-02-14 22:59:52]

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

[commit-bot: I haz the power, 2014-02-14 22:59:53]

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

[stevenjb, 2014-02-14 23:20:34]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-02-14 23:22:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/156353002/1330001

[commit-bot: I haz the power, 2014-02-15 03:48:53]

Change committed as 251501