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

chrome/browser/automation/automation_provider_observers_chromeos.cc

Index: chrome/browser/automation/automation_provider_observers_chromeos.cc
diff --git a/chrome/browser/automation/automation_provider_observers_chromeos.cc b/chrome/browser/automation/automation_provider_observers_chromeos.cc
new file mode 100644
index 0000000000000000000000000000000000000000..9e103e73fcf387b1c50e4791414282fdf14ba4bd
--- /dev/null
+++ b/chrome/browser/automation/automation_provider_observers_chromeos.cc
@@ -0,0 +1,142 @@
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/automation/automation_provider_observers.h"

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

Is there a reason why this include is separated from the others?

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

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...

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

Cool, thanks for the link.

+
+#include "base/values.h"
+#include "chrome/browser/automation/automation_provider.h"
+#include "chrome/browser/chromeos/cros/cros_library.h"
+#include "chrome/browser/chromeos/login/authentication_notification_details.h"
+#include "content/common/notification_service.h"
+
+using chromeos::CrosLibrary;
+using chromeos::NetworkLibrary;
+
+NetworkManagerInitObserver::NetworkManagerInitObserver(
+    AutomationProvider* automation)
+    : automation_(automation->AsWeakPtr()) {
+  if (CrosLibrary::Get()->EnsureLoaded()) {
+    CrosLibrary::Get()->GetNetworkLibrary()->
+        AddNetworkManagerObserver(this);
+  } else {
+    automation_->OnNetworkLibraryInit();
+    delete this;

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

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.

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

Done.

+  }
+}
+
+NetworkManagerInitObserver::~NetworkManagerInitObserver() {}

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

Should call RemoveNetworkManagerObserver() here in case this gets destroyed
someplace other than from OnNetworkManagerChanged(), e.g. after a timeout.

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

Done.

+
+void NetworkManagerInitObserver::OnNetworkManagerChanged(NetworkLibrary* obj) {
+  if (!obj->wifi_scanning()) {
+    obj->RemoveNetworkManagerObserver(this);

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

Do this in the destructor.

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

Done.

+    automation_->OnNetworkLibraryInit();
+    delete this;
+  }
+}
+
+LoginManagerObserver::LoginManagerObserver(
+    AutomationProvider* automation,
+    IPC::Message* reply_message)
+    : automation_(automation->AsWeakPtr()),
+      reply_message_(reply_message) {
+

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

Delete this blank line.

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

Done.

+  registrar_.Add(this, NotificationType::LOGIN_USER_CHANGED,
+                 NotificationService::AllSources());
+}
+
+LoginManagerObserver::~LoginManagerObserver() {}
+
+void LoginManagerObserver::Observe(NotificationType type,
+                                   const NotificationSource& source,
+                                   const NotificationDetails& details) {
+  DCHECK(type == NotificationType::LOGIN_USER_CHANGED);
+
+  if (!automation_) {
+    delete this;
+    return;
+  }
+
+  AutomationJSONReply reply(automation_, reply_message_.release());
+  Details<AuthenticationNotificationDetails> auth_details(details);
+  if (auth_details->success())
+    reply.SendSuccess(NULL);
+  else
+    reply.SendError("Login failure.");
+  delete this;
+}
+
+ScreenLockUnlockObserver::ScreenLockUnlockObserver(
+    AutomationProvider* automation,
+    IPC::Message* reply_message,
+    bool lock_screen)
+    : automation_(automation),
+      reply_message_(reply_message),
+      lock_screen_(lock_screen) {
+

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

Delete this blank line.

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

Done.

+  registrar_.Add(this, NotificationType::SCREEN_LOCK_STATE_CHANGED,
+                 NotificationService::AllSources());
+}
+
+ScreenLockUnlockObserver::~ScreenLockUnlockObserver() {}
+
+void ScreenLockUnlockObserver::Observe(NotificationType type,
+                                       const NotificationSource& source,
+                                       const NotificationDetails& details) {
+  DCHECK(type == NotificationType::SCREEN_LOCK_STATE_CHANGED);
+  AutomationJSONReply reply(automation_, reply_message_);
+  bool is_screen_locked = *Details<bool>(details).ptr();
+  if (lock_screen_ == is_screen_locked)
+    reply.SendSuccess(NULL);
+  else
+    reply.SendError("Screen lock failure.");
+  delete this;
+}
+
+NetworkScanObserver::NetworkScanObserver(AutomationProvider* automation,
+                         IPC::Message* reply_message)

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

Indent underneath the first parameter in the previous line.

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

Done.

+  : automation_(automation), reply_message_(reply_message) {

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

I think in other parts of the code, this line is generally indented 4 spaces,
not 2.

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

Done.

+  NetworkLibrary* network_library = CrosLibrary::Get()->GetNetworkLibrary();

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

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?

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

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.

+  network_library->AddNetworkManagerObserver(this);
+}
+
+NetworkScanObserver::~NetworkScanObserver() {
+  NetworkLibrary* network_library = CrosLibrary::Get()->GetNetworkLibrary();

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

Same comment as line 99 above.

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

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

+  network_library->RemoveNetworkManagerObserver(this);
+}
+
+void NetworkScanObserver::OnNetworkManagerChanged(NetworkLibrary* obj) {
+  if (obj->wifi_scanning())
+    return;
+
+  AutomationJSONReply(automation_, reply_message_).SendSuccess(NULL);
+  delete this;
+}
+
+NetworkConnectObserver::NetworkConnectObserver(AutomationProvider* automation,
+                                               IPC::Message* reply_message,
+                                               chromeos::Network* network)
+  : automation_(automation), reply_message_(reply_message), network_(network) {

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

Same comment as line 98 above.

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

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).

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

Done.

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

Done.

+  NetworkLibrary* network_library = CrosLibrary::Get()->GetNetworkLibrary();

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

Same comment as line 99 above.

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

Same reply as line 99 above.

+  network_library->AddNetworkManagerObserver(this);
+}
+
+NetworkConnectObserver::~NetworkConnectObserver() {
+  NetworkLibrary* network_library = CrosLibrary::Get()->GetNetworkLibrary();

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

Same comment as line 99 above.

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

Same reply as line 99 above.

+  network_library->RemoveNetworkManagerObserver(this);
+}
+
+void NetworkConnectObserver::OnNetworkManagerChanged(NetworkLibrary* obj) {
+  if (network_->failed()) {

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

See note above about saving Network *'s. Use:
Network* network =
CrosLibrary::Get()->GetNetworkLibrary()->FindNetworkByServicePath(service_path_);
if (!network) ...

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

Done.

+    scoped_ptr<DictionaryValue> return_value(new DictionaryValue);
+    return_value->SetInteger("error_code", network_->error());
+    AutomationJSONReply reply(automation_, reply_message_);
+    reply.SendSuccess(return_value.get());
+    delete this;
+  }
+  if (network_->connected()) {
+    AutomationJSONReply(automation_, reply_message_).SendSuccess(NULL);
+    delete this;
+  }

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

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?

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

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.

+}
+