PyAuto automation hooks: blocking wifi connect, disconnect, and network scan.

ConnectToWifiNetwork() is now blocking.
Add DisconnectFromWifiNetwork(), which is also blocking.
Add NetworkScan(), which is also blocking.

Move chromeos automation observers into their own file.
Add better error message for when automation calls time out.


BUG=chromium-os:12727
TEST=Mostly manual. Automated tests will be set up in a lab with real wifi access points.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=79476

[stanleyw, 2011-03-24 23:16:03]

http://codereview.chromium.org/6732040/diff/3003/chrome/test/pyautolib/pyauto.py
File chrome/test/pyautolib/pyauto.py (right):

http://codereview.chromium.org/6732040/diff/3003/chrome/test/pyautolib/pyauto...
chrome/test/pyautolib/pyauto.py:2415: The second element contains an integer
error code in case of failure.
Rewrite as integer is too cumbersome to decode

http://codereview.chromium.org/6732040/diff/3003/chrome/test/pyautolib/pyauto...
chrome/test/pyautolib/pyauto.py:2429: return (False, result['error_code'])
result['error_code'] is too cumbersome to decode

[dennis_jeffrey, 2011-03-24 23:48:54]

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
File chrome/browser/automation/automation_provider_observers.h (right):

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers.h:707: : public
chromeos::NetworkLibrary::NetworkManagerObserver {
In other parts of this file, this line is generally indented 4 spaces underneath
the previous line, not 2 (see, e.g., line 100 above and line 813 below).

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers.h:727: : public
chromeos::NetworkLibrary::NetworkManagerObserver {
Same comment as line 707 above.

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
File chrome/browser/automation/automation_provider_observers_chromeos.cc
(right):

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers_chromeos.cc:5: #include
"chrome/browser/automation/automation_provider_observers.h"
Is there a reason why this include is separated from the others?

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers_chromeos.cc:43: 
Delete this blank line.

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers_chromeos.cc:76: 
Delete this blank line.

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers_chromeos.cc:97:
IPC::Message* reply_message)
Indent underneath the first parameter in the previous line.

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers_chromeos.cc:98: :
automation_(automation), reply_message_(reply_message) {
I think in other parts of the code, this line is generally indented 4 spaces,
not 2.

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers_chromeos.cc:99:
NetworkLibrary* network_library = CrosLibrary::Get()->GetNetworkLibrary();
Are we guaranteed that "network_library" will refer to a valid object, or do we
need to check to ensure this variable is non-NULL?

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers_chromeos.cc:104:
NetworkLibrary* network_library = CrosLibrary::Get()->GetNetworkLibrary();
Same comment as line 99 above.

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers_chromeos.cc:119: :
automation_(automation), reply_message_(reply_message), network_(network) {
Same comment as line 98 above.

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers_chromeos.cc:120:
NetworkLibrary* network_library = CrosLibrary::Get()->GetNetworkLibrary();
Same comment as line 99 above.

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers_chromeos.cc:125:
NetworkLibrary* network_library = CrosLibrary::Get()->GetNetworkLibrary();
Same comment as line 99 above.

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers_chromeos.cc:140: }
Is it possible for the conditions at lines 130 and 137 to both be false?  If so,
is it correct behavior to not do anything in that case?

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/te...
File chrome/browser/automation/testing_automation_provider_chromeos.cc (right):

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider_chromeos.cc:208:
NetworkLibrary* network_library = CrosLibrary::Get()->GetNetworkLibrary();
Are we guaranteed that this variable will be non-NULL before we dereference it
below?

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider_chromeos.cc:258:
reply.SendError("Invalid or missing args");
Put period at end of the string sentence (to be consistent with other error
strings in this file).

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider_chromeos.cc:267:
NetworkLibrary* network_library = CrosLibrary::Get()->GetNetworkLibrary();
Same comment as line 208 above.

http://codereview.chromium.org/6732040/diff/3003/chrome/test/functional/chrom...
File chrome/test/functional/chromeos_wifi.py (right):

http://codereview.chromium.org/6732040/diff/3003/chrome/test/functional/chrom...
chrome/test/functional/chromeos_wifi.py:22: """Basic check to sure that a
network scan doesn't throw errors."""
"sure" --> "ensure"

http://codereview.chromium.org/6732040/diff/3003/chrome/test/pyautolib/pyauto.py
File chrome/test/pyautolib/pyauto.py (right):

http://codereview.chromium.org/6732040/diff/3003/chrome/test/pyautolib/pyauto...
chrome/test/pyautolib/pyauto.py:2411: Blocks until connection suceeds or fails.
"suceeds" --> "succeeds"

http://codereview.chromium.org/6732040/diff/3003/chrome/test/pyautolib/pyauto...
chrome/test/pyautolib/pyauto.py:2434: password='', identity='', certpath=''):
Indent underneath first parameter in the previous line.

[stevenjb, 2011-03-25 01:12:22]

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
File chrome/browser/automation/automation_provider_observers_chromeos.cc
(right):

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers_chromeos.cc:24: delete
this;
Is deletion in the constructor a pattern we use elsewhere? This seems unsafe.
Usually logic like this should be done in an Init() function which can return
false on error, triggering cleanup by the owner.

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers_chromeos.cc:28:
NetworkManagerInitObserver::~NetworkManagerInitObserver() {}
Should call RemoveNetworkManagerObserver() here in case this gets destroyed
someplace other than from OnNetworkManagerChanged(), e.g. after a timeout.

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers_chromeos.cc:32:
obj->RemoveNetworkManagerObserver(this);
Do this in the destructor.

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers_chromeos.cc:99:
NetworkLibrary* network_library = CrosLibrary::Get()->GetNetworkLibrary();
GetNetworkLibrary() will always return a valid instance; it will return a stub
implementation if libcros is not loaded. We access this without checks like this
throughout the code.

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers_chromeos.cc:119: :
automation_(automation), reply_message_(reply_message), network_(network) {
I am not sure where this is called from, but we should not save off Network*'s.
After the recent refactoring, Network instances should only exist in
NetworkLibrary, and they may become invalid if a network disappears. Instead,
save off network->service_path(), and use
GetNetworkLibrary()->FindNetworkByServicePath() to receive a Network* (and be
sure to handle a NULL return value).

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers_chromeos.cc:130: if
(network_->failed()) {
See note above about saving Network *'s. Use:
Network* network =
CrosLibrary::Get()->GetNetworkLibrary()->FindNetworkByServicePath(service_path_);
if (!network) ...

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers_chromeos.cc:140: }
On 2011/03/24 23:48:54, dennis_jeffrey wrote:
> Is it possible for the conditions at lines 130 and 137 to both be false?  If
so,
> is it correct behavior to not do anything in that case?

This gets called any time anything in the network manager changes (e.g. changes
to a different network), so doing nothing here is correct. However, there should
probably be a timeout somewhere.

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/te...
File chrome/browser/automation/testing_automation_provider_chromeos.cc (right):

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider_chromeos.cc:247: new
NetworkConnectObserver(this, reply_message, wifi);
See my notes in NetworkConnectObserver, but we are probably better off passing
service_path to the constructor instead of wifi.

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider_chromeos.cc:249:
network_library->ConnectToWifiNetwork(service_path);
It is now valid and slightly more optimal to pass wifi here instead of
service_path (saves an extra lookup).

[dtu, 2011-03-25 22:22:11]

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
File chrome/browser/automation/automation_provider_observers.h (right):

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers.h:707: : public
chromeos::NetworkLibrary::NetworkManagerObserver {
On 2011/03/24 23:48:54, dennis_jeffrey wrote:
> In other parts of this file, this line is generally indented 4 spaces
underneath
> the previous line, not 2 (see, e.g., line 100 above and line 813 below).

Done.

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers.h:727: : public
chromeos::NetworkLibrary::NetworkManagerObserver {
On 2011/03/24 23:48:54, dennis_jeffrey wrote:
> Same comment as line 707 above.

Done.

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
File chrome/browser/automation/automation_provider_observers_chromeos.cc
(right):

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers_chromeos.cc:5: #include
"chrome/browser/automation/automation_provider_observers.h"
On 2011/03/24 23:48:54, dennis_jeffrey wrote:
> Is there a reason why this include is separated from the others?

The main purpose of this file is to implement methods in
automation_provider_observers.h.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers_chromeos.cc:24: delete
this;
On 2011/03/25 01:12:22, Steven Bennetts wrote:
> Is deletion in the constructor a pattern we use elsewhere? This seems unsafe.
> Usually logic like this should be done in an Init() function which can return
> false on error, triggering cleanup by the owner.

Done.

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers_chromeos.cc:28:
NetworkManagerInitObserver::~NetworkManagerInitObserver() {}
On 2011/03/25 01:12:22, Steven Bennetts wrote:
> Should call RemoveNetworkManagerObserver() here in case this gets destroyed
> someplace other than from OnNetworkManagerChanged(), e.g. after a timeout.

Done.

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers_chromeos.cc:32:
obj->RemoveNetworkManagerObserver(this);
On 2011/03/25 01:12:22, Steven Bennetts wrote:
> Do this in the destructor.

Done.

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers_chromeos.cc:43: 
On 2011/03/24 23:48:54, dennis_jeffrey wrote:
> Delete this blank line.

Done.

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers_chromeos.cc:76: 
On 2011/03/24 23:48:54, dennis_jeffrey wrote:
> Delete this blank line.

Done.

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers_chromeos.cc:97:
IPC::Message* reply_message)
On 2011/03/24 23:48:54, dennis_jeffrey wrote:
> Indent underneath the first parameter in the previous line.

Done.

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers_chromeos.cc:98: :
automation_(automation), reply_message_(reply_message) {
On 2011/03/24 23:48:54, dennis_jeffrey wrote:
> I think in other parts of the code, this line is generally indented 4 spaces,
> not 2.

Done.

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers_chromeos.cc:104:
NetworkLibrary* network_library = CrosLibrary::Get()->GetNetworkLibrary();
On 2011/03/24 23:48:54, dennis_jeffrey wrote:
> Same comment as line 99 above.

Yes, see stevenjb's comment on line 99 above.

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers_chromeos.cc:119: :
automation_(automation), reply_message_(reply_message), network_(network) {
On 2011/03/24 23:48:54, dennis_jeffrey wrote:
> Same comment as line 98 above.

Done.

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers_chromeos.cc:119: :
automation_(automation), reply_message_(reply_message), network_(network) {
On 2011/03/25 01:12:22, Steven Bennetts wrote:
> I am not sure where this is called from, but we should not save off
Network*'s.
> After the recent refactoring, Network instances should only exist in
> NetworkLibrary, and they may become invalid if a network disappears. Instead,
> save off network->service_path(), and use
> GetNetworkLibrary()->FindNetworkByServicePath() to receive a Network* (and be
> sure to handle a NULL return value).

Done.

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers_chromeos.cc:120:
NetworkLibrary* network_library = CrosLibrary::Get()->GetNetworkLibrary();
On 2011/03/24 23:48:54, dennis_jeffrey wrote:
> Same comment as line 99 above.

Same reply as line 99 above.

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers_chromeos.cc:125:
NetworkLibrary* network_library = CrosLibrary::Get()->GetNetworkLibrary();
On 2011/03/24 23:48:54, dennis_jeffrey wrote:
> Same comment as line 99 above.

Same reply as line 99 above.

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers_chromeos.cc:130: if
(network_->failed()) {
On 2011/03/25 01:12:22, Steven Bennetts wrote:
> See note above about saving Network *'s. Use:
> Network* network =
>
CrosLibrary::Get()->GetNetworkLibrary()->FindNetworkByServicePath(service_path_);
> if (!network) ...

Done.

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/te...
File chrome/browser/automation/testing_automation_provider_chromeos.cc (right):

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider_chromeos.cc:208:
NetworkLibrary* network_library = CrosLibrary::Get()->GetNetworkLibrary();
On 2011/03/24 23:48:54, dennis_jeffrey wrote:
> Are we guaranteed that this variable will be non-NULL before we dereference it
> below?

Yes, see stevenjb's comment.

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider_chromeos.cc:247: new
NetworkConnectObserver(this, reply_message, wifi);
On 2011/03/25 01:12:22, Steven Bennetts wrote:
> See my notes in NetworkConnectObserver, but we are probably better off passing
> service_path to the constructor instead of wifi.

Done.

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider_chromeos.cc:249:
network_library->ConnectToWifiNetwork(service_path);
On 2011/03/25 01:12:22, Steven Bennetts wrote:
> It is now valid and slightly more optimal to pass wifi here instead of
> service_path (saves an extra lookup).

Done.

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider_chromeos.cc:258:
reply.SendError("Invalid or missing args");
On 2011/03/24 23:48:54, dennis_jeffrey wrote:
> Put period at end of the string sentence (to be consistent with other error
> strings in this file).

Done.

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider_chromeos.cc:267:
NetworkLibrary* network_library = CrosLibrary::Get()->GetNetworkLibrary();
On 2011/03/24 23:48:54, dennis_jeffrey wrote:
> Same comment as line 208 above.

Same reply as line 208 above.

http://codereview.chromium.org/6732040/diff/3003/chrome/test/functional/chrom...
File chrome/test/functional/chromeos_wifi.py (right):

http://codereview.chromium.org/6732040/diff/3003/chrome/test/functional/chrom...
chrome/test/functional/chromeos_wifi.py:22: """Basic check to sure that a
network scan doesn't throw errors."""
On 2011/03/24 23:48:54, dennis_jeffrey wrote:
> "sure" --> "ensure"

Done.

http://codereview.chromium.org/6732040/diff/3003/chrome/test/pyautolib/pyauto.py
File chrome/test/pyautolib/pyauto.py (right):

http://codereview.chromium.org/6732040/diff/3003/chrome/test/pyautolib/pyauto...
chrome/test/pyautolib/pyauto.py:2411: Blocks until connection suceeds or fails.
On 2011/03/24 23:48:54, dennis_jeffrey wrote:
> "suceeds" --> "succeeds"

Done.

http://codereview.chromium.org/6732040/diff/3003/chrome/test/pyautolib/pyauto...
chrome/test/pyautolib/pyauto.py:2415: The second element contains an integer
error code in case of failure.
On 2011/03/24 23:16:03, stanleyw wrote:
> Rewrite as integer is too cumbersome to decode

Done. Using string error messages instead and comment updated.

http://codereview.chromium.org/6732040/diff/3003/chrome/test/pyautolib/pyauto...
chrome/test/pyautolib/pyauto.py:2429: return (False, result['error_code'])
On 2011/03/24 23:16:03, stanleyw wrote:
> result['error_code'] is too cumbersome to decode

It's decoded for you, a caller of this function will just see a tuple with
(success, error_code).

http://codereview.chromium.org/6732040/diff/3003/chrome/test/pyautolib/pyauto...
chrome/test/pyautolib/pyauto.py:2434: password='', identity='', certpath=''):
On 2011/03/24 23:48:54, dennis_jeffrey wrote:
> Indent underneath first parameter in the previous line.

Done.

[stanleyw, 2011-03-25 22:40:03]

http://codereview.chromium.org/6732040/diff/3003/chrome/test/pyautolib/pyauto.py
File chrome/test/pyautolib/pyauto.py (right):

http://codereview.chromium.org/6732040/diff/3003/chrome/test/pyautolib/pyauto...
chrome/test/pyautolib/pyauto.py:2415: The second element contains an integer
error code in case of failure.
What are the possible values of error strings? Or maybe specify where we can
find these. 
On 2011/03/25 22:22:11, Dave Tu wrote:
> On 2011/03/24 23:16:03, stanleyw wrote:
> > Rewrite as integer is too cumbersome to decode
> 
> Done. Using string error messages instead and comment updated.

[dennis_jeffrey, 2011-03-25 22:44:36]

LGTM

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
File chrome/browser/automation/automation_provider_observers_chromeos.cc
(right):

http://codereview.chromium.org/6732040/diff/3003/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers_chromeos.cc:5: #include
"chrome/browser/automation/automation_provider_observers.h"
On 2011/03/25 22:22:11, Dave Tu wrote:
> On 2011/03/24 23:48:54, dennis_jeffrey wrote:
> > Is there a reason why this include is separated from the others?
> 
> The main purpose of this file is to implement methods in
> automation_provider_observers.h.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...

Cool, thanks for the link.

[stevenjb, 2011-03-25 23:08:51]

C++ code LGTM.
Thanks for setting this up! I look forward to more and better automated
networking tests.

http://codereview.chromium.org/6732040/diff/14005/chrome/browser/automation/a...
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/6732040/diff/14005/chrome/browser/automation/a...
chrome/browser/automation/automation_provider.cc:183: delete observer;
return false or report failure here?

[stanleyw, 2011-03-25 23:28:02]

LGTM
On 2011/03/25 23:08:51, Steven Bennetts wrote:
> C++ code LGTM.
> Thanks for setting this up! I look forward to more and better automated
> networking tests.
> 
>
http://codereview.chromium.org/6732040/diff/14005/chrome/browser/automation/a...
> File chrome/browser/automation/automation_provider.cc (right):
> 
>
http://codereview.chromium.org/6732040/diff/14005/chrome/browser/automation/a...
> chrome/browser/automation/automation_provider.cc:183: delete observer;
> return false or report failure here?