Add PeripheralBatteryObserver

chrome/browser/chromeos/power/peripheral_battery_observer.cc

+// Copyright (c) 2013 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/chromeos/power/peripheral_battery_observer.h"
+
+#include "ash/shell.h"
+#include "base/bind.h"
+#include "base/stringprintf.h"
+#include "base/utf_string_conversions.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/notifications/notification.h"
+#include "chrome/browser/notifications/notification_ui_manager.h"
+#include "chrome/browser/profiles/profile_manager.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "device/bluetooth/bluetooth_adapter_factory.h"
+#include "device/bluetooth/bluetooth_device.h"
+#include "grit/ash_strings.h"
+#include "grit/theme_resources.h"
+#include "ui/base/l10n/l10n_util.h"
+#include "ui/base/resource/resource_bundle.h"
+#include "ui/gfx/image/image.h"
+
+namespace chromeos {
+
+namespace {
+
+const int kLowBatteryLevel = 15;

[Daniel Erat, 2013/04/06 01:52:34]

add comments describing what these mean.  for example, i would've guessed from
this variable's name that the battery is considered low when it's <= this level.
 looking at the code, it appears that you're doing < instead.  (i'd recommend
switching to <= below, but please add a comment regardless.)

[Yufeng Shen (Slow to review), 2013/04/09 00:45:08]

Done.

+const int kNotificationInterval = 60;

[Daniel Erat, 2013/04/06 01:52:34]

include units in this constant's name, e.g. kNotificationIntervalSec

[Yufeng Shen (Slow to review), 2013/04/09 00:45:08]

Done.

+
+class PeripheralBatteryNotificationDelegate : public NotificationDelegate {

[Daniel Erat, 2013/04/06 01:52:34]

is this class really necessary?  it looks like it doesn't do anything except
store an id.  it'd be worth asking some notifications people if there's a
cleaner way to do this.

[Yufeng Shen (Slow to review), 2013/04/09 00:45:08]

offline talk to dewittj@ about making NotificationDelegate's pure virtual
functions
to be non-virtual with default implementation, and he suggested not to do so
since
they are going to change the prototype pretty soon and I can come back to fix
the
implementation later.

+ public:
+

[Daniel Erat, 2013/04/06 01:52:34]

delete blank line

[Yufeng Shen (Slow to review), 2013/04/09 00:45:08]

Done.

+  explicit PeripheralBatteryNotificationDelegate(const std::string& id)
+      : id_(id) {}
+
+  // Overridden from NotificationDelegate:
+  virtual void Display() OVERRIDE {}
+  virtual void Error() OVERRIDE {}
+  virtual void Close(bool by_user) OVERRIDE {}
+  virtual void Click() OVERRIDE {}
+  virtual std::string id() const OVERRIDE { return id_; }
+  virtual content::RenderViewHost* GetRenderViewHost() const OVERRIDE {
+    return NULL;
+  }
+
+ private:
+  const std::string id_;
+
+  DISALLOW_COPY_AND_ASSIGN(PeripheralBatteryNotificationDelegate);
+};
+
+base::TimeDelta TimeNow() {
+  return base::TimeDelta::FromInternalValue(

[Daniel Erat, 2013/04/06 01:52:34]

why are you converting from one class's internal value to another class's
internal value?  TimeDelta represents the difference between two TimeTicks; what
you're doing here doesn't make sense.  don't use
ToInternalValue/FromInternalValue for anything except
serialization/deserialization.

[Yufeng Shen (Slow to review), 2013/04/09 00:45:08]

Removed. 

+      base::TimeTicks::Now().ToInternalValue());
+}
+
+}  // namespace
+
+PeripheralBatteryObserver::PeripheralBatteryObserver()
+    : weakptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(
+          new base::WeakPtrFactory<PeripheralBatteryObserver>(this))) {
+  DBusThreadManager::Get()->GetPowerManagerClient()->AddObserver(this);
+  device::BluetoothAdapterFactory::GetAdapter(
+      base::Bind(&PeripheralBatteryObserver::InitializeOnBluetoothReady,
+                 weakptr_factory_->GetWeakPtr()));
+}
+
+PeripheralBatteryObserver::~PeripheralBatteryObserver() {
+  if (bluetooth_adapter_.get())
+    bluetooth_adapter_->RemoveObserver(this);
+  DBusThreadManager::Get()->GetPowerManagerClient()->RemoveObserver(this);
+}
+
+void PeripheralBatteryObserver::InitializeOnBluetoothReady(
+    scoped_refptr<device::BluetoothAdapter> adapter) {
+  bluetooth_adapter_ = adapter;
+  CHECK(bluetooth_adapter_);
+  bluetooth_adapter_->AddObserver(this);
+}
+
+void PeripheralBatteryObserver::PeripheralBatteryStatusReceived(
+    const std::string& path, const std::string& name, int level) {

[Daniel Erat, 2013/04/06 01:52:34]

one parameter per line

[Yufeng Shen (Slow to review), 2013/04/09 00:45:08]

Done.

+  if (level < -1 || level > 100) {
+    LOG(ERROR) << "Invalid battery level " << level
+               << " for device " << name << " at path " << path;
+    return;
+  }
+  // If unknown battery level received, cancel any existing notification.
+  if (level == -1)
+    g_browser_process->notification_ui_manager()->CancelById(path);

[Daniel Erat, 2013/04/06 01:52:34]

can you return immediately here?  is there any reason to continue, and possibly
create a new BatteryInfo, for an unknown level?

[Yufeng Shen (Slow to review), 2013/04/09 00:45:08]

Done.

+
+  // Post the notification in 2 cases:
+  // 1. It's the first time the battery level is received, and it is
+  //    below kLowBatteryLevel.
+  // 2. The battery level is in record and it drops below kLowBatteryLevel.
+  if (batteries_.find(path) == batteries_.end()) {
+    BatteryInfo battery(path, name, level, base::TimeDelta(), TimeNow());
+    if (level >= 0 && level < kLowBatteryLevel)
+      PostNotification(&battery);
+    batteries_[path] = battery;
+  } else {
+    BatteryInfo* battery = &batteries_[path];
+    battery->set_name(name);
+    int old_level = battery->level();
+    battery->set_level(level);
+    battery->set_update_timestamp(TimeNow());
+    if (old_level >= kLowBatteryLevel && level >= 0 && level < kLowBatteryLevel)
+      PostNotification(battery);
+  }
+}
+
+void PeripheralBatteryObserver::DeviceChanged(device::BluetoothAdapter* adapter,
+                                              device::BluetoothDevice* device) {
+  if (!device->IsPaired())
+    RemoveBattery(UTF16ToUTF8(device->GetName()));

[Daniel Erat, 2013/04/06 01:52:34]

is it guaranteed that the names that you get here will match the names coming
from powerd in the battery level signals?

[Yufeng Shen (Slow to review), 2013/04/09 00:45:08]

So the name can actually be set by the user so it is not guaranteed to
be unique. I changed it to use the bluetooth device's address as the key
add/remove battery information.

+}
+
+void PeripheralBatteryObserver::DeviceRemoved(device::BluetoothAdapter* adapter,
+                                              device::BluetoothDevice* device) {
+  RemoveBattery(UTF16ToUTF8(device->GetName()));
+}
+
+void PeripheralBatteryObserver::RemoveBattery(const std::string& name) {
+  std::map<std::string, BatteryInfo>::iterator it = batteries_.begin();
+  for (; it != batteries_.end(); it++) {
+    if (name == it->second.name()) {

[Daniel Erat, 2013/04/06 01:52:34]

could you just key the map by device name instead of by path so you don't need
to iterate through the whole map here?

[Yufeng Shen (Slow to review), 2013/04/09 00:45:08]

Changed to use the device's bluetooth address as the key.

+      g_browser_process->notification_ui_manager()->CancelById(it->first);
+      batteries_.erase(it);
+      break;
+    }
+  }
+}
+
+void PeripheralBatteryObserver::PostNotification(BatteryInfo* battery) {

[Daniel Erat, 2013/04/06 01:52:34]

this should take a const BatteryInfo& instead of a pointer

[Yufeng Shen (Slow to review), 2013/04/09 00:45:08]

Done.

+  // Only post notification if kNotificationInterval seconds have passed
+  // since last notification showed, avoiding the case the battery level

[Daniel Erat, 2013/04/06 01:52:34]

nit: s/case the/case where the/

[Yufeng Shen (Slow to review), 2013/04/09 00:45:08]

Done.

+  // oscillates around the threshold level.
+  if (TimeNow() - battery->last_notification_timestamp() <
+      base::TimeDelta::FromSeconds(kNotificationInterval))
+    return;
+
+  NotificationUIManager* notification_manager =
+      g_browser_process->notification_ui_manager();
+
+  std::string string_id = battery->path();
+  std::string string_text = base::StringPrintf("Battery Low (%d%%)",

[Daniel Erat, 2013/04/06 01:52:34]

this needs to be a translatable string resource

[Yufeng Shen (Slow to review), 2013/04/09 00:45:08]

Done.

+                                               battery->level());
+
+  if (notification_manager->DoesIdExist(string_id)) {
+    if (!notification_manager->CancelById(string_id)) {

[Jun Mukai, 2013/04/06 01:11:56]

You do not need to cancel it by yourself. NotificationUIManager will handle the
replacement.

[Yufeng Shen (Slow to review), 2013/04/09 00:45:08]

great.

+      LOG(ERROR) << "Can't cancel notification for ID " << string_id;
+      return;
+    }
+  }
+
+  Notification notification(

[Jun Mukai, 2013/04/06 01:11:56]

Please add my TODO here:
TODO(mukai): add SYSTEM priority and use here

[Yufeng Shen (Slow to review), 2013/04/09 00:45:08]

Done.

+      GURL(),
+      ui::ResourceBundle::GetSharedInstance().GetImageNamed(
+          IDR_NOTIFICATION_PERIPHERAL_BATTERY_LOW),
+      UTF8ToUTF16(battery->name()),
+      UTF8ToUTF16(string_text),
+      WebKit::WebTextDirectionDefault,
+      string16(),
+      UTF8ToUTF16(string_id),
+      new PeripheralBatteryNotificationDelegate(string_id));
+
+  notification_manager->Add(notification,
+                            ProfileManager::GetDefaultProfileOrOffTheRecord());
+
+  battery->set_notification_timestamp(TimeNow());
+}
+
+}  // namespace chromeos