Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
436683002:
Battery Status API: implementation for Linux. (Closed)
|
Created:
6 years, 4 months ago by timvolodine Modified:
6 years, 4 months ago Reviewers:
stevenjb, hashimoto, mlamouri (slow - plz ping), Elliot Glaysher, Michael van Ouwerkerk, keybuk, brettw, satorux1 CC:
chromium-reviews, darin-cc_chromium.org, jam, stevenjb Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionBattery Status API: implementation for Linux.
Implementation of the Battery Status API for the Linux platform.
Implementation uses DBus to talk to org.freedesktop.UPower service
to obtain battery information.
BUG=122593
TEST=http://jsbin.com/battery-status-test (manual)
TBR=brettw@chromium.org
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289372
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289431
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 23
Patch Set 3 : fixed comments #
Total comments: 28
Patch Set 4 : addressed many comments #Patch Set 5 : fix BUILD.gn #
Total comments: 89
Patch Set 6 : hashimoto's comments #Patch Set 7 : more comments #Patch Set 8 : cleanup and comments in notification thread destructor #
Total comments: 19
Patch Set 9 : comments #Patch Set 10 : fix chromeOS build #
Messages
Total messages: 35 (0 generated)
https://codereview.chromium.org/436683002/diff/20001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_linux.cc (right): https://codereview.chromium.org/436683002/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:125: // TODO(timvolodine): add support for multiple batteries. Currently we Please file a bug for this. It would be nice to have UMA logging as well, which would be suitable as a TODO. https://codereview.chromium.org/436683002/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:206: void BatteryChanged(dbus::Signal* signal) { The signal param initially confused me. How about a comment along the lines of /* unused as the only connected signal is for battery changes */ https://codereview.chromium.org/436683002/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:206: void BatteryChanged(dbus::Signal* signal) { Maybe a unit test would be appropriate here. I think you could extract a static method from BatteryChanged that is easy to test, something like: blink::WebBatteryStatus GetStatus(base::DictionaryValue data);
https://codereview.chromium.org/436683002/diff/20001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_linux.cc (right): https://codereview.chromium.org/436683002/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:17: #include "third_party/cros_system_api/dbus/service_constants.h" I think you should remove that #include. Isn't it for Chrome OS? It sounds wrong to use Chrome OS stuff in battery_status_manager_linux.cc. https://codereview.chromium.org/436683002/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:26: const char kUPowerDeviceSignalChanged[] = "Changed"; I think you can use "DeviceChanged" instead of "Changed". In which case, you could check at the beginning of the callback whether the change is related to the device you are currently tracking (ie. |battery_proxy_|). https://codereview.chromium.org/436683002/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:155: ShutdownDBus(); nit: could you rename this ShutdownDBusConnection() or something clearer. The name is slightly confusing. https://codereview.chromium.org/436683002/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:214: status.charging = state != 2; // true if not discharging Could you have an enum that mirror the possible values of state (http://upower.freedesktop.org/docs/Device.html#Device:State). It would make things way easier to understand and help spot bugs. Here, it's hard to judge whether you are doing the right thing. https://codereview.chromium.org/436683002/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:217: status.level = round(percentage * 10) / 1000.f; Why not *0.01.f or /10.f? https://codereview.chromium.org/436683002/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:222: status.chargingTime = Are we using the right default value in Blink if this is not set? https://codereview.chromium.org/436683002/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:232: status.dischargingTime = time_to_empty; ditto https://codereview.chromium.org/436683002/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:271: return false; That's not really something we expect to happen, right? Shouldn't we at least LOG this? Maybe LOG(ERROR)?
thanks for the comments, replies below. https://codereview.chromium.org/436683002/diff/20001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_linux.cc (right): https://codereview.chromium.org/436683002/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:17: #include "third_party/cros_system_api/dbus/service_constants.h" On 2014/08/01 17:14:14, Mounir Lamouri wrote: > I think you should remove that #include. Isn't it for Chrome OS? It sounds wrong > to use Chrome OS stuff in battery_status_manager_linux.cc. Done. https://codereview.chromium.org/436683002/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:26: const char kUPowerDeviceSignalChanged[] = "Changed"; On 2014/08/01 17:14:13, Mounir Lamouri wrote: > I think you can use "DeviceChanged" instead of "Changed". In which case, you > could check at the beginning of the callback whether the change is related to > the device you are currently tracking (ie. |battery_proxy_|). UPower.Device only has the Changed() signal, in line 140 we explicitly register for a specific battery: "battery_proxy_->ConnectToSignal(".. https://codereview.chromium.org/436683002/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:125: // TODO(timvolodine): add support for multiple batteries. Currently we On 2014/08/01 15:24:39, Michael van Ouwerkerk wrote: > Please file a bug for this. It would be nice to have UMA logging as well, which > would be suitable as a TODO. Done. https://codereview.chromium.org/436683002/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:155: ShutdownDBus(); On 2014/08/01 17:14:13, Mounir Lamouri wrote: > nit: could you rename this ShutdownDBusConnection() or something clearer. The > name is slightly confusing. Done. https://codereview.chromium.org/436683002/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:206: void BatteryChanged(dbus::Signal* signal) { On 2014/08/01 15:24:39, Michael van Ouwerkerk wrote: > The signal param initially confused me. How about a comment along the lines of > /* unused as the only connected signal is for battery changes */ Done. https://codereview.chromium.org/436683002/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:206: void BatteryChanged(dbus::Signal* signal) { On 2014/08/01 15:24:39, Michael van Ouwerkerk wrote: > Maybe a unit test would be appropriate here. I think you could extract a static > method from BatteryChanged that is easy to test, something like: > blink::WebBatteryStatus GetStatus(base::DictionaryValue data); I've extracted a method for testing (ComputeWebBatteryStatus) and added unittests. I had to add a header file as well in order to expose that method for unit testing. https://codereview.chromium.org/436683002/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:214: status.charging = state != 2; // true if not discharging On 2014/08/01 17:14:14, Mounir Lamouri wrote: > Could you have an enum that mirror the possible values of state > (http://upower.freedesktop.org/docs/Device.html#Device:State). It would make > things way easier to understand and help spot bugs. Here, it's hard to judge > whether you are doing the right thing. Done. https://codereview.chromium.org/436683002/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:217: status.level = round(percentage * 10) / 1000.f; On 2014/08/01 17:14:13, Mounir Lamouri wrote: > Why not *0.01.f or /10.f? dbus reports the value with lost of fractional digits, here the rounding to 3 significant digits is happening. https://codereview.chromium.org/436683002/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:222: status.chargingTime = On 2014/08/01 17:14:13, Mounir Lamouri wrote: > Are we using the right default value in Blink if this is not set? here we explicitly set it to either the time or +inf, because the default is 0. https://codereview.chromium.org/436683002/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:232: status.dischargingTime = time_to_empty; On 2014/08/01 17:14:13, Mounir Lamouri wrote: > ditto dischargingTime is +inf by default so we are fine here (as explained in the comment) https://codereview.chromium.org/436683002/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:271: return false; On 2014/08/01 17:14:14, Mounir Lamouri wrote: > That's not really something we expect to happen, right? Shouldn't we at least > LOG this? Maybe LOG(ERROR)? right, added LOG(ERROR).
lgtm with my comments applied. But please, get someone else to review this. Someone who knows a bit about dbus and thread usage in chromium would be a good pick. https://codereview.chromium.org/436683002/diff/20001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_linux.cc (right): https://codereview.chromium.org/436683002/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:26: const char kUPowerDeviceSignalChanged[] = "Changed"; On 2014/08/06 16:37:42, timvolodine wrote: > On 2014/08/01 17:14:13, Mounir Lamouri wrote: > > I think you can use "DeviceChanged" instead of "Changed". In which case, you > > could check at the beginning of the callback whether the change is related to > > the device you are currently tracking (ie. |battery_proxy_|). > > UPower.Device only has the Changed() signal, in line 140 we explicitly register > for a specific battery: "battery_proxy_->ConnectToSignal(".. My bad, I misread. I didn't see that you were registering to Device.Changed. https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_linux.cc (right): https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:124: if (is_present && type == 2 /* battery */) { Could you add an enum for that too? Sorry for not catching it before. https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:215: callback_.Run(status); This block needs some empty lines. It's quite dense. https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:283: status.level = round(percentage * 10) / 1000.f; Could you add a test with "Percentage" = 13.37 and check that it is rounded to "0.113". https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_linux.h (right): https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.h:29: const base::DictionaryValue& dictionary, blink::WebBatteryStatus& status); Could you add a comment describing what this is doing. https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_linux_unittest.cc (right): https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux_unittest.cc:70: dictionary.SetDouble("Percentage", 90); Could you add a similar test with no TimeToEmpty. https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux_unittest.cc:78: Could you add some tests for STATE_UNKWNOWN and STATE_EMPTY? https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux_unittest.cc:80: Also, it would be greate to add some empty lines in those big blocks. It's a bit painful to read. You could do things like: base::DictionaryValue dictionary; dictionary.SetDouble("State", UPOWER_DEVICE_STATE_CHARGING); dictionary.SetDouble("TimeToFull", 100.f); dictionary.SetDouble("Percentage", 1); blink::WebBatteryStatus status; ComputeWebBatteryStatus(dictionary, status); EXPECT_EQ(true, status.charging); EXPECT_EQ(100, status.chargingTime); EXPECT_EQ(std::numeric_limits<double>::infinity(), status.dischargingTime); EXPECT_EQ(.01, status.level);
+ adding Elliot as Linux expert to have a look at DBus and linux specifics.
I don't believe I've ever used our dbus implementation; I don't think I have anything useful to say. You may want to seek a reviewer who has written a bunch of code using our dbus implementation.
+ Scott as one of the dbus/ owners. Scott, this patch uses dbus could you have glance?
+ cc: stevenjb@ as another owner of dbus.
https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_linux.cc (right): https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:30: // intended for unit32, int64, uint64, double. s/unit32/uint32/ https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_linux.h (right): https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.h:28: CONTENT_EXPORT void ComputeWebBatteryStatus( Why does this need CONTENT_EXPORT?
+satorux@, +hashimoto@ who are much more familiar with the low level DBus module than I am. On Chrome OS we have a DBusThreadManager class to handle all DBus traffic on a single thread. I wonder if we should be doing something similar on linux, now that we have several components creating their own dbus::Bus instances (geolocation, media_transfer_protocol, now this)? satoru, hashimoto, thoughts? https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_linux.cc (right): https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:36: return dictionary.GetDouble(property_name, &value) ? value : default_value; static_cast<T>(value) https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:58: if (response) { We should probably return NULL on failure to read the property and handle that in the callers. Also, early exit if !response. https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:124: if (is_present && type == 2 /* battery */) { On 2014/08/07 16:17:04, Mounir Lamouri wrote: > Could you add an enum for that too? Sorry for not catching it before. nit: if (!is_present || type != TYPE_BATTERY) continue; https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:246: } This bit should probably be moved to a helper method, e.g. if (!NotifierThreadStarted()) return false;
https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_linux.h (right): https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.h:28: CONTENT_EXPORT void ComputeWebBatteryStatus( On 2014/08/08 14:54:59, Michael van Ouwerkerk wrote: > Why does this need CONTENT_EXPORT? To be used in the unit test I presume.
https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_linux.cc (right): https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:30: // intended for unit32, int64, uint64, double. On 2014/08/08 14:54:59, Michael van Ouwerkerk wrote: > s/unit32/uint32/ Done. https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:36: return dictionary.GetDouble(property_name, &value) ? value : default_value; On 2014/08/08 16:18:16, stevenjb wrote: > static_cast<T>(value) Done. https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:58: if (response) { On 2014/08/08 16:18:16, stevenjb wrote: > We should probably return NULL on failure to read the property and handle that > in the callers. Also, early exit if !response. the idea here is to return an empty dictionary, which does not cause special cases like NULL later. early exit does not look possible because of another if later on. https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:124: if (is_present && type == 2 /* battery */) { On 2014/08/07 16:17:04, Mounir Lamouri wrote: > Could you add an enum for that too? Sorry for not catching it before. Done. https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:124: if (is_present && type == 2 /* battery */) { On 2014/08/08 16:18:16, stevenjb wrote: > On 2014/08/07 16:17:04, Mounir Lamouri wrote: > > Could you add an enum for that too? Sorry for not catching it before. > > nit: if (!is_present || type != TYPE_BATTERY) continue; Done. https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:215: callback_.Run(status); On 2014/08/07 16:17:04, Mounir Lamouri wrote: > This block needs some empty lines. It's quite dense. Done. https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:246: } On 2014/08/08 16:18:16, stevenjb wrote: > This bit should probably be moved to a helper method, e.g. > if (!NotifierThreadStarted()) > return false; Done. https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:283: status.level = round(percentage * 10) / 1000.f; On 2014/08/07 16:17:04, Mounir Lamouri wrote: > Could you add a test with "Percentage" = 13.37 and check that it is rounded to > "0.113". Done. https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_linux.h (right): https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.h:28: CONTENT_EXPORT void ComputeWebBatteryStatus( On 2014/08/08 17:31:00, Mounir Lamouri wrote: > On 2014/08/08 14:54:59, Michael van Ouwerkerk wrote: > > Why does this need CONTENT_EXPORT? > > To be used in the unit test I presume. yes, unittests are a separate library so need to export this otherwise won't link. https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.h:28: CONTENT_EXPORT void ComputeWebBatteryStatus( On 2014/08/08 14:54:59, Michael van Ouwerkerk wrote: > Why does this need CONTENT_EXPORT? https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.h:29: const base::DictionaryValue& dictionary, blink::WebBatteryStatus& status); On 2014/08/07 16:17:04, Mounir Lamouri wrote: > Could you add a comment describing what this is doing. Done. https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_linux_unittest.cc (right): https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux_unittest.cc:70: dictionary.SetDouble("Percentage", 90); On 2014/08/07 16:17:05, Mounir Lamouri wrote: > Could you add a similar test with no TimeToEmpty. Done. https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux_unittest.cc:78: On 2014/08/07 16:17:05, Mounir Lamouri wrote: > Could you add some tests for STATE_UNKWNOWN and STATE_EMPTY? Done. https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux_unittest.cc:80: On 2014/08/07 16:17:05, Mounir Lamouri wrote: > Also, it would be greate to add some empty lines in those big blocks. It's a bit > painful to read. You could do things like: > > base::DictionaryValue dictionary; > dictionary.SetDouble("State", UPOWER_DEVICE_STATE_CHARGING); > dictionary.SetDouble("TimeToFull", 100.f); > dictionary.SetDouble("Percentage", 1); > > blink::WebBatteryStatus status; > ComputeWebBatteryStatus(dictionary, status); > > EXPECT_EQ(true, status.charging); > EXPECT_EQ(100, status.chargingTime); > EXPECT_EQ(std::numeric_limits<double>::infinity(), status.dischargingTime); > EXPECT_EQ(.01, status.level); Done.
lgtm
https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_linux.cc (right): https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:44: // intended for uint32, int64, uint64, double. I see no uint64 in this file. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:46: T GetProperty(const base::DictionaryValue& dictionary, I don't see any merit of having this template. It even makes it hard to locate a potential bug where precision is lost because int64 is converted to double by PopDataAsValue(). Please use GetDouble() and GetBoolean() directly in the caller sites. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:49: double value; Please don't leave a variable uninitialized. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:66: kDBusPropertiesGetAll); Why don't you use dbus/property.h? https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:82: return scoped_ptr<base::DictionaryValue>(new base::DictionaryValue()); Why not returning a null scoped_ptr? https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:86: std::vector<dbus::ObjectPath>& paths) { Please don't use non-const references. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:90: dbus::MethodCall method_call(kUPowerServiceName, "EnumerateDevices"); "EnumerateDevices" should be a constant. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:92: dbus::ObjectProxy::TIMEOUT_USE_DEFAULT)); nit: Arguments should be aligned. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:99: paths.clear(); This line is not needed. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:101: Please add comments for each class which describe the roles of the classes. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:102: class BatteryStatusNotificationThread : public base::Thread { No need to inherit base::Thread. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:108: battery_proxy_(0) {} Use NULL. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:132: kUPowerServiceName, device_path); ObjectProxy won't be destroyed until the bus gets destroyed or RemoveObjectProxy is called. It might be better to call RemoveObjectProxy() for unused proxies if device_paths.size() can be a large number. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:179: nit: No need to have a blank line. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:181: return std::string(base::PlatformThread::GetName()).compare( Use operator==. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:182: kBatteryWatcherThreadName) == 0; Why don't you directly compare the name of the thread? https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:198: base::Bind(&dbus::Bus::ShutdownAndBlock, Doesn't this result in a case where BatteryChanged() is called after this object gets destryoed? https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:200: system_bus_ = 0; Use NULL for pointers. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:210: signal_name.compare(kUPowerDeviceSignalChanged) != 0) { Use operator!= https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:218: BatteryChanged(0); Use NULL for pointers. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:221: // default values. Why do we need to do this? https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:239: dbus::ObjectProxy* battery_proxy_; // owned by dbus "owned by dbus" is ambiguos. It's owned by the bus. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:263: base::Unretained(notifier_thread_.get()))); Doesn't this result in a crash if StartListening() is run after the notifier thread is destryoed? https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:276: bool NotifierThreadStarted() { "NotifierThreadStarted" sounds like a getter which returns the thread's status, or a handler which handles the thread initialization errors while what it actually does is to start the thread. Please give it a more descriptive name. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:309: status.level = round(percentage * 10) / 1000.f; Why should we intentionally degrade precision? https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:313: int64 time_to_full = GetProperty<int64>(dictionary, "TimeToFull", 0); Why do we need to convert a double value stored in the dictionary to int64 and later convert it again into double? https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_linux.h (right): https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.h:7: #include "content/browser/battery_status/battery_status_manager.h" nit: No need to include this. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.h:31: const base::DictionaryValue& dictionary, blink::WebBatteryStatus& status); Our style guide prohibits use of non-const references.
On 2014/08/08 16:18:16, stevenjb wrote: > +satorux@, +hashimoto@ who are much more familiar with the low level DBus module > than I am. > > On Chrome OS we have a DBusThreadManager class to handle all DBus traffic on a > single thread. I wonder if we should be doing something similar on linux, now > that we have several components creating their own dbus::Bus instances > (geolocation, media_transfer_protocol, now this)? satoru, hashimoto, thoughts? Seems there are more: - chrome/browser/password_manager/native_backend_kwallet_x.cc - content/browser/power_save_blocker_x11.cc - device/media_transfer_protocol/media_transfer_protocol_manager.cc (this file has many #if defined(OS_CHROMEOS) to switch DBusThreadManager and a Bus owned by it.) I think it might make the code simpler to have something like DBusThreadManager on Linux. Though I'm not sure how hard it would be to implement the actual change and how many users can benefit from it. > > https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... > File content/browser/battery_status/battery_status_manager_linux.cc (right): > > https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... > content/browser/battery_status/battery_status_manager_linux.cc:36: return > dictionary.GetDouble(property_name, &value) ? value : default_value; > static_cast<T>(value) > > https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... > content/browser/battery_status/battery_status_manager_linux.cc:58: if (response) > { > We should probably return NULL on failure to read the property and handle that > in the callers. Also, early exit if !response. > > https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... > content/browser/battery_status/battery_status_manager_linux.cc:124: if > (is_present && type == 2 /* battery */) { > On 2014/08/07 16:17:04, Mounir Lamouri wrote: > > Could you add an enum for that too? Sorry for not catching it before. > > nit: if (!is_present || type != TYPE_BATTERY) continue; > > https://codereview.chromium.org/436683002/diff/40001/content/browser/battery_... > content/browser/battery_status/battery_status_manager_linux.cc:246: } > This bit should probably be moved to a helper method, e.g. > if (!NotifierThreadStarted()) > return false;
https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_linux.cc (right): https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:44: // intended for uint32, int64, uint64, double. On 2014/08/11 08:19:15, hashimoto wrote: > I see no uint64 in this file. Acknowledged. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:46: T GetProperty(const base::DictionaryValue& dictionary, On 2014/08/11 08:19:15, hashimoto wrote: > I don't see any merit of having this template. > It even makes it hard to locate a potential bug where precision is lost because > int64 is converted to double by PopDataAsValue(). > Please use GetDouble() and GetBoolean() directly in the caller sites. Done. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:49: double value; On 2014/08/11 08:19:14, hashimoto wrote: > Please don't leave a variable uninitialized. Done. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:66: kDBusPropertiesGetAll); On 2014/08/11 08:19:15, hashimoto wrote: > Why don't you use dbus/property.h? Done. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:82: return scoped_ptr<base::DictionaryValue>(new base::DictionaryValue()); On 2014/08/11 08:19:14, hashimoto wrote: > Why not returning a null scoped_ptr? upon failure it returns an empty dictionary which will later correspond to default values provided by the API (i.e. no information can be provided). That way we avoid extra null checks. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:86: std::vector<dbus::ObjectPath>& paths) { On 2014/08/11 08:19:15, hashimoto wrote: > Please don't use non-const references. Done. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:90: dbus::MethodCall method_call(kUPowerServiceName, "EnumerateDevices"); On 2014/08/11 08:19:15, hashimoto wrote: > "EnumerateDevices" should be a constant. Done. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:92: dbus::ObjectProxy::TIMEOUT_USE_DEFAULT)); On 2014/08/11 08:19:14, hashimoto wrote: > nit: Arguments should be aligned. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... Done. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:99: paths.clear(); On 2014/08/11 08:19:15, hashimoto wrote: > This line is not needed. Done. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:101: On 2014/08/11 08:19:14, hashimoto wrote: > Please add comments for each class which describe the roles of the classes. Done. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:102: class BatteryStatusNotificationThread : public base::Thread { On 2014/08/11 08:19:15, hashimoto wrote: > No need to inherit base::Thread. ? what do you mean, it's a thread. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:108: battery_proxy_(0) {} On 2014/08/11 08:19:14, hashimoto wrote: > Use NULL. Done. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:132: kUPowerServiceName, device_path); On 2014/08/11 08:19:14, hashimoto wrote: > ObjectProxy won't be destroyed until the bus gets destroyed or RemoveObjectProxy > is called. > It might be better to call RemoveObjectProxy() for unused proxies if > device_paths.size() can be a large number. Done. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:179: On 2014/08/11 08:19:14, hashimoto wrote: > nit: No need to have a blank line. Done. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:181: return std::string(base::PlatformThread::GetName()).compare( On 2014/08/11 08:19:15, hashimoto wrote: > Use operator==. Acknowledged. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:182: kBatteryWatcherThreadName) == 0; On 2014/08/11 08:19:14, hashimoto wrote: > Why don't you directly compare the name of the thread? PlatformThread::GetName() == kBatteryWatcherThreadName doesn't work. apparently the names are interned by thread_id_name_manager.cc. switched to strcomp now. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:198: base::Bind(&dbus::Bus::ShutdownAndBlock, On 2014/08/11 08:19:15, hashimoto wrote: > Doesn't this result in a case where BatteryChanged() is called after this object > gets destryoed? I've added a check in BatteryChanged to prevent the case when BatteryChanged is called after StopListening has been called. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:200: system_bus_ = 0; On 2014/08/11 08:19:15, hashimoto wrote: > Use NULL for pointers. Done. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:210: signal_name.compare(kUPowerDeviceSignalChanged) != 0) { On 2014/08/11 08:19:15, hashimoto wrote: > Use operator!= Done. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:218: BatteryChanged(0); On 2014/08/11 08:19:15, hashimoto wrote: > Use NULL for pointers. Done. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:221: // default values. On 2014/08/11 08:19:15, hashimoto wrote: > Why do we need to do this? this means we cannot get battery updates, so need to propagate default values to blink (i.e. meaning we cannot provide battery information according to specification) https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:239: dbus::ObjectProxy* battery_proxy_; // owned by dbus On 2014/08/11 08:19:14, hashimoto wrote: > "owned by dbus" is ambiguos. > It's owned by the bus. Done. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:263: base::Unretained(notifier_thread_.get()))); On 2014/08/11 08:19:14, hashimoto wrote: > Doesn't this result in a crash if StartListening() is run after the notifier > thread is destryoed? this cannot happen, currently the thread is destroyed during shutdown. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:276: bool NotifierThreadStarted() { On 2014/08/11 08:19:15, hashimoto wrote: > "NotifierThreadStarted" sounds like a getter which returns the thread's status, > or a handler which handles the thread initialization errors while what it > actually does is to start the thread. > Please give it a more descriptive name. Done. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:309: status.level = round(percentage * 10) / 1000.f; On 2014/08/11 08:19:14, hashimoto wrote: > Why should we intentionally degrade precision? this is to avoid fingerprinting and not to trigger level changes in blink more often than needed. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:313: int64 time_to_full = GetProperty<int64>(dictionary, "TimeToFull", 0); On 2014/08/11 08:19:15, hashimoto wrote: > Why do we need to convert a double value stored in the dictionary to int64 and > later convert it again into double? Done. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_linux.h (right): https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.h:7: #include "content/browser/battery_status/battery_status_manager.h" On 2014/08/11 08:19:15, hashimoto wrote: > nit: No need to include this. this is needed due to BatteryStatusManager::Create defined in battery_status_manager_linux.cc https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.h:31: const base::DictionaryValue& dictionary, blink::WebBatteryStatus& status); On 2014/08/11 08:19:16, hashimoto wrote: > Our style guide prohibits use of non-const references. Done. made it return a scoped_ptr.
+ brettw@ : for content/browser/BUILD.gn
https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_linux.cc (right): https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:66: kDBusPropertiesGetAll); On 2014/08/11 16:02:59, timvolodine wrote: > On 2014/08/11 08:19:15, hashimoto wrote: > > Why don't you use dbus/property.h? > > Done. Sorry for not being clear, why don't you use dbus::PropertySet defined in property.h? https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:102: class BatteryStatusNotificationThread : public base::Thread { On 2014/08/11 16:02:59, timvolodine wrote: > On 2014/08/11 08:19:15, hashimoto wrote: > > No need to inherit base::Thread. > > ? what do you mean, it's a thread. Composition is preferred to inheritance. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Inheri... You can do the same thing by having base::Thread as a member. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:182: kBatteryWatcherThreadName) == 0; On 2014/08/11 16:03:00, timvolodine wrote: > On 2014/08/11 08:19:14, hashimoto wrote: > > Why don't you directly compare the name of the thread? > > PlatformThread::GetName() == kBatteryWatcherThreadName doesn't work. apparently > the names are interned by thread_id_name_manager.cc. switched to strcomp now. Sorry, not then name, the ID. Can't we do the same thing as MessageLoopProxyImpl::RunsTasksOnCurrentThread()? https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop/... Or, how about calling task_runner()->RunsTasksOnCurrentThread()? https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:198: base::Bind(&dbus::Bus::ShutdownAndBlock, On 2014/08/11 16:02:59, timvolodine wrote: > On 2014/08/11 08:19:15, hashimoto wrote: > > Doesn't this result in a case where BatteryChanged() is called after this > object > > gets destryoed? > > I've added a check in BatteryChanged to prevent the case when BatteryChanged is > called after StopListening has been called. I'm not sure if that |system_bus_| check can be helpful. It should result in a crash with SIGSEGV accessing any member of this object after the dtor gets called. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:221: // default values. On 2014/08/11 16:03:00, timvolodine wrote: > On 2014/08/11 08:19:15, hashimoto wrote: > > Why do we need to do this? > > this means we cannot get battery updates, so need to propagate default values to > blink (i.e. meaning we cannot provide battery information according to > specification) It seems the Chrome OS implmentation is not doing the same thing when PowerManager fails to connect to the signal. Is this really needed? https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:263: base::Unretained(notifier_thread_.get()))); On 2014/08/11 16:03:00, timvolodine wrote: > On 2014/08/11 08:19:14, hashimoto wrote: > > Doesn't this result in a crash if StartListening() is run after the notifier > > thread is destryoed? > > this cannot happen, currently the thread is destroyed during shutdown. Are you sure that StartListening() and StopListening() tasks are not run after notifier_thread_ gets destroyed considering all possible scenarios? https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:309: status.level = round(percentage * 10) / 1000.f; On 2014/08/11 16:02:59, timvolodine wrote: > On 2014/08/11 08:19:14, hashimoto wrote: > > Why should we intentionally degrade precision? > > this is to avoid fingerprinting and not to trigger level changes in blink more > often than needed. I'm not sure what "fingerprinting" means in this context. Could you add a comment which describes why this is needed and why it should be 3 digits of precision (not 2, 4, 5 or whatever)? Also, it seems existing implementations for Mac and Chrome OS are not doing this. Don't we need this for those platforms too? If needed, how about moving this precision degrade code to the caller site (e.g. Blink code)? https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_linux.h (right): https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.h:7: #include "content/browser/battery_status/battery_status_manager.h" On 2014/08/11 16:03:00, timvolodine wrote: > On 2014/08/11 08:19:15, hashimoto wrote: > > nit: No need to include this. > > this is needed due to BatteryStatusManager::Create defined in > battery_status_manager_linux.cc Should be included in battery_status_manager_linux.cc, not in the header. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.h:31: const base::DictionaryValue& dictionary, blink::WebBatteryStatus& status); On 2014/08/11 16:03:00, timvolodine wrote: > On 2014/08/11 08:19:16, hashimoto wrote: > > Our style guide prohibits use of non-const references. > > Done. made it return a scoped_ptr. WebBatteryStatus is a copyable class. Why does this function return a scoped_ptr, instead of WebBatteryStatus?
https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_linux.cc (right): https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:66: kDBusPropertiesGetAll); On 2014/08/12 10:01:42, hashimoto wrote: > On 2014/08/11 16:02:59, timvolodine wrote: > > On 2014/08/11 08:19:15, hashimoto wrote: > > > Why don't you use dbus/property.h? > > > > Done. > > Sorry for not being clear, > why don't you use dbus::PropertySet defined in property.h? I'll look into that, not sure if this will simplify current code though. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:102: class BatteryStatusNotificationThread : public base::Thread { On 2014/08/12 10:01:42, hashimoto wrote: > On 2014/08/11 16:02:59, timvolodine wrote: > > On 2014/08/11 08:19:15, hashimoto wrote: > > > No need to inherit base::Thread. > > > > ? what do you mean, it's a thread. > Composition is preferred to inheritance. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Inheri... > You can do the same thing by having base::Thread as a member. oh I see, in this case I don't think it'll make things much better as I would need to expose some start method in this class which then would be called from a different thread.. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:182: kBatteryWatcherThreadName) == 0; On 2014/08/12 10:01:42, hashimoto wrote: > On 2014/08/11 16:03:00, timvolodine wrote: > > On 2014/08/11 08:19:14, hashimoto wrote: > > > Why don't you directly compare the name of the thread? > > > > PlatformThread::GetName() == kBatteryWatcherThreadName doesn't work. > apparently > > the names are interned by thread_id_name_manager.cc. switched to strcomp now. > > Sorry, not then name, the ID. > Can't we do the same thing as MessageLoopProxyImpl::RunsTasksOnCurrentThread()? > https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop/... > > Or, how about calling task_runner()->RunsTasksOnCurrentThread()? yes, RunsTasksOnCurrentThread appears to work. Done. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:198: base::Bind(&dbus::Bus::ShutdownAndBlock, On 2014/08/12 10:01:42, hashimoto wrote: > On 2014/08/11 16:02:59, timvolodine wrote: > > On 2014/08/11 08:19:15, hashimoto wrote: > > > Doesn't this result in a case where BatteryChanged() is called after this > > object > > > gets destryoed? > > > > I've added a check in BatteryChanged to prevent the case when BatteryChanged > is > > called after StopListening has been called. > > I'm not sure if that |system_bus_| check can be helpful. > It should result in a crash with SIGSEGV accessing any member of this object > after the dtor gets called. I am not sure I understand your concern. The BatteryStatusNotificationThread currently gets destroyed only on shutdown. At destruction time it makes sure to call ShutdownDBusConnection() and then calls Stop() method of the Thread class which makes sure that the message loop is drained and the bus is properly disconnected. I don't see how we can get any callbacks after the BatteryStatusNotificationThread is destroyed. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:221: // default values. On 2014/08/12 10:01:42, hashimoto wrote: > On 2014/08/11 16:03:00, timvolodine wrote: > > On 2014/08/11 08:19:15, hashimoto wrote: > > > Why do we need to do this? > > > > this means we cannot get battery updates, so need to propagate default values > to > > blink (i.e. meaning we cannot provide battery information according to > > specification) > > It seems the Chrome OS implmentation is not doing the same thing when > PowerManager fails to connect to the signal. > Is this really needed? I believe this is needed to cover the following (unlikely) situation: imagine we can get current battery status, but fail to connect to the signal. We have two options: 1. either execute callback with the current status and do nothing, or 2. execute the callback with default values (meaning cannot provide data according to spec). I think option 2 is better as option 1 would result in stale data after some time which would be misleading. ChromeOS probably follows option 1 and we should see if this can be fixed. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:263: base::Unretained(notifier_thread_.get()))); On 2014/08/12 10:01:42, hashimoto wrote: > On 2014/08/11 16:03:00, timvolodine wrote: > > On 2014/08/11 08:19:14, hashimoto wrote: > > > Doesn't this result in a crash if StartListening() is run after the notifier > > > thread is destryoed? > > > > this cannot happen, currently the thread is destroyed during shutdown. > > Are you sure that StartListening() and StopListening() tasks are not run after > notifier_thread_ gets destroyed considering all possible scenarios? notifier_thead_ currently get destroyed at shutdown by the chrome main thread after the IO thread has been stopped already. {Start,Stop}Listening() come in on IO thread so not possible to receive them after notifier_thread_ destruction. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:309: status.level = round(percentage * 10) / 1000.f; On 2014/08/12 10:01:42, hashimoto wrote: > On 2014/08/11 16:02:59, timvolodine wrote: > > On 2014/08/11 08:19:14, hashimoto wrote: > > > Why should we intentionally degrade precision? > > > > this is to avoid fingerprinting and not to trigger level changes in blink more > > often than needed. > > I'm not sure what "fingerprinting" means in this context. > Could you add a comment which describes why this is needed and why it should be > 3 digits of precision (not 2, 4, 5 or whatever)? > > Also, it seems existing implementations for Mac and Chrome OS are not doing > this. > Don't we need this for those platforms too? > If needed, how about moving this precision degrade code to the caller site (e.g. > Blink code)? Mac and Android do not need this rounding, don't know about chrome OS. Interesting suggestion about moving to blink, added some comments + TODO for this. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_linux.h (right): https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.h:7: #include "content/browser/battery_status/battery_status_manager.h" On 2014/08/12 10:01:43, hashimoto wrote: > On 2014/08/11 16:03:00, timvolodine wrote: > > On 2014/08/11 08:19:15, hashimoto wrote: > > > nit: No need to include this. > > > > this is needed due to BatteryStatusManager::Create defined in > > battery_status_manager_linux.cc > > Should be included in battery_status_manager_linux.cc, not in the header. Done. although it looks a bit weird to include two of them in .cc. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.h:31: const base::DictionaryValue& dictionary, blink::WebBatteryStatus& status); On 2014/08/12 10:01:43, hashimoto wrote: > On 2014/08/11 16:03:00, timvolodine wrote: > > On 2014/08/11 08:19:16, hashimoto wrote: > > > Our style guide prohibits use of non-const references. > > > > Done. made it return a scoped_ptr. > > WebBatteryStatus is a copyable class. > Why does this function return a scoped_ptr, instead of WebBatteryStatus? I was actually trying to avoid a copy, but the compiler should optimize the return value anyway.. so changed to return by value.
https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_linux.cc (right): https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:82: return scoped_ptr<base::DictionaryValue>(new base::DictionaryValue()); On 2014/08/11 16:02:59, timvolodine wrote: > On 2014/08/11 08:19:14, hashimoto wrote: > > Why not returning a null scoped_ptr? > > upon failure it returns an empty dictionary which will later correspond to > default values provided by the API (i.e. no information can be provided). That > way we avoid extra null checks. Why should we perform totally redandunt allocation to save one NULL check? https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:102: class BatteryStatusNotificationThread : public base::Thread { On 2014/08/12 19:27:53, timvolodine wrote: > On 2014/08/12 10:01:42, hashimoto wrote: > > On 2014/08/11 16:02:59, timvolodine wrote: > > > On 2014/08/11 08:19:15, hashimoto wrote: > > > > No need to inherit base::Thread. > > > > > > ? what do you mean, it's a thread. > > Composition is preferred to inheritance. > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Inheri... > > You can do the same thing by having base::Thread as a member. > > oh I see, in this case I don't think it'll make things much better as I would > need to expose some start method in this class which then would be called from a > different thread.. Please don't violate the coding style and degrade the readability of the code to save a few key strokes. Also, you can pass a pointer to a SingleThreadTaskRunner (or appropriate subclass of TaskRunner) to this class while owning base::Thread in the manager. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:198: base::Bind(&dbus::Bus::ShutdownAndBlock, On 2014/08/12 19:27:53, timvolodine wrote: > On 2014/08/12 10:01:42, hashimoto wrote: > > On 2014/08/11 16:02:59, timvolodine wrote: > > > On 2014/08/11 08:19:15, hashimoto wrote: > > > > Doesn't this result in a case where BatteryChanged() is called after this > > > object > > > > gets destryoed? > > > > > > I've added a check in BatteryChanged to prevent the case when BatteryChanged > > is > > > called after StopListening has been called. > > > > I'm not sure if that |system_bus_| check can be helpful. > > It should result in a crash with SIGSEGV accessing any member of this object > > after the dtor gets called. > > I am not sure I understand your concern. The BatteryStatusNotificationThread > currently gets destroyed only on shutdown. At destruction time it makes sure to > call ShutdownDBusConnection() and then calls Stop() method of the Thread class > which makes sure that the message loop is drained and the bus is properly > disconnected. I don't see how we can get any callbacks after the > BatteryStatusNotificationThread is destroyed. I was concerned about a case where this object is destroyed but the bus is still alive. But since you changed the dtor and ShutdownDBusConnection(), it seems it cannot happen. (sorry for dismissing this) https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:221: // default values. On 2014/08/12 19:27:53, timvolodine wrote: > On 2014/08/12 10:01:42, hashimoto wrote: > > On 2014/08/11 16:03:00, timvolodine wrote: > > > On 2014/08/11 08:19:15, hashimoto wrote: > > > > Why do we need to do this? > > > > > > this means we cannot get battery updates, so need to propagate default > values > > to > > > blink (i.e. meaning we cannot provide battery information according to > > > specification) > > > > It seems the Chrome OS implmentation is not doing the same thing when > > PowerManager fails to connect to the signal. > > Is this really needed? > > I believe this is needed to cover the following (unlikely) situation: imagine we > can get current battery status, but fail to connect to the signal. We have two > options: > 1. either execute callback with the current status and do nothing, or > 2. execute the callback with default values (meaning cannot provide data > according to spec). > > I think option 2 is better as option 1 would result in stale data after some > time which would be misleading. ChromeOS probably follows option 1 and we should > see if this can be fixed. When something goes wrong with D-Bus, it seems the current Chrome OS implementation does nothing because PowerChanged() never gets called in error cases. Also, it's more natural to do nothing than running the observer callback with the default value reserved to indicate an error. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:309: status.level = round(percentage * 10) / 1000.f; On 2014/08/12 19:27:52, timvolodine wrote: > On 2014/08/12 10:01:42, hashimoto wrote: > > On 2014/08/11 16:02:59, timvolodine wrote: > > > On 2014/08/11 08:19:14, hashimoto wrote: > > > > Why should we intentionally degrade precision? > > > > > > this is to avoid fingerprinting and not to trigger level changes in blink > more > > > often than needed. > > > > I'm not sure what "fingerprinting" means in this context. > > Could you add a comment which describes why this is needed and why it should > be > > 3 digits of precision (not 2, 4, 5 or whatever)? > > > > Also, it seems existing implementations for Mac and Chrome OS are not doing > > this. > > Don't we need this for those platforms too? > > If needed, how about moving this precision degrade code to the caller site > (e.g. > > Blink code)? > > Mac and Android do not need this rounding, don't know about chrome OS. Chrome OS is much more important than Linux. Please implement the same logic or resolve the TODO in short term if we should really care about this problem. > Interesting suggestion about moving to blink, added some comments + TODO for > this. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_linux.h (right): https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.h:31: const base::DictionaryValue& dictionary, blink::WebBatteryStatus& status); On 2014/08/12 19:27:53, timvolodine wrote: > On 2014/08/12 10:01:43, hashimoto wrote: > > On 2014/08/11 16:03:00, timvolodine wrote: > > > On 2014/08/11 08:19:16, hashimoto wrote: > > > > Our style guide prohibits use of non-const references. > > > > > > Done. made it return a scoped_ptr. > > > > WebBatteryStatus is a copyable class. > > Why does this function return a scoped_ptr, instead of WebBatteryStatus? > > I was actually trying to avoid a copy, but the compiler should optimize the > return value anyway.. so changed to return by value. Why do you care about a copy here when you allocate an unused DictionaryValue to save a few key strokes for a NULL check? Allocation should be more expensive. https://codereview.chromium.org/436683002/diff/140001/content/browser/battery... File content/browser/battery_status/battery_status_manager_linux.cc (right): https://codereview.chromium.org/436683002/diff/140001/content/browser/battery... content/browser/battery_status/battery_status_manager_linux.cc:194: options.connection_type = dbus::Bus::PRIVATE; BTW, why don't you set options.dbus_task_runner? This way you don't have to implement thread related logic after all. Also, it makes it easy to introduce DBusThreadManager-ish object to Linux Chrome. (please refer to stevenjb@'s comment). https://codereview.chromium.org/436683002/diff/140001/content/browser/battery... content/browser/battery_status/battery_status_manager_linux.cc:208: system_bus_)); What is the purpose of this? Do we need to process tasks at this moment? Aren't we here because no one is interested in this bus? Also, can we be sure that no additional task is posted during the period between this PostTask() and when ShutdownAndBlock() actually gets run? https://codereview.chromium.org/436683002/diff/140001/content/browser/battery... content/browser/battery_status/battery_status_manager_linux.cc:245: callback_.Run(status); Isn't this callback supposed to be called on the IO thread? https://codereview.chromium.org/436683002/diff/140001/content/browser/battery... content/browser/battery_status/battery_status_manager_linux.cc:270: if (!StartNotifierThreadIfNecessary()) Does this code correctly handle a case where StartListeningBatterChange() is called after StopListeningBatteryChange()? https://codereview.chromium.org/436683002/diff/140001/content/browser/battery... content/browser/battery_status/battery_status_manager_linux.cc:352: default: { Don't we need to treat this case as an error? https://codereview.chromium.org/436683002/diff/140001/content/browser/battery... File content/browser/battery_status/battery_status_manager_linux_unittest.cc (right): https://codereview.chromium.org/436683002/diff/140001/content/browser/battery... content/browser/battery_status/battery_status_manager_linux_unittest.cc:33: EXPECT_EQ(true, status.charging); nit: You can use EXPECT_TRUE/FALSE for booleans.
https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_linux.cc (right): https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:82: return scoped_ptr<base::DictionaryValue>(new base::DictionaryValue()); On 2014/08/13 11:02:20, hashimoto wrote: > On 2014/08/11 16:02:59, timvolodine wrote: > > On 2014/08/11 08:19:14, hashimoto wrote: > > > Why not returning a null scoped_ptr? > > > > upon failure it returns an empty dictionary which will later correspond to > > default values provided by the API (i.e. no information can be provided). That > > way we avoid extra null checks. > > Why should we perform totally redandunt allocation to save one NULL check? ok made it to return null. Done. it's actually more that 1 null check but still ok. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:198: base::Bind(&dbus::Bus::ShutdownAndBlock, On 2014/08/13 11:02:20, hashimoto wrote: > On 2014/08/12 19:27:53, timvolodine wrote: > > On 2014/08/12 10:01:42, hashimoto wrote: > > > On 2014/08/11 16:02:59, timvolodine wrote: > > > > On 2014/08/11 08:19:15, hashimoto wrote: > > > > > Doesn't this result in a case where BatteryChanged() is called after > this > > > > object > > > > > gets destryoed? > > > > > > > > I've added a check in BatteryChanged to prevent the case when > BatteryChanged > > > is > > > > called after StopListening has been called. > > > > > > I'm not sure if that |system_bus_| check can be helpful. > > > It should result in a crash with SIGSEGV accessing any member of this object > > > after the dtor gets called. > > > > I am not sure I understand your concern. The BatteryStatusNotificationThread > > currently gets destroyed only on shutdown. At destruction time it makes sure > to > > call ShutdownDBusConnection() and then calls Stop() method of the Thread class > > which makes sure that the message loop is drained and the bus is properly > > disconnected. I don't see how we can get any callbacks after the > > BatteryStatusNotificationThread is destroyed. > > I was concerned about a case where this object is destroyed but the bus is still > alive. > But since you changed the dtor and ShutdownDBusConnection(), it seems it cannot > happen. (sorry for dismissing this) Done. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:221: // default values. On 2014/08/13 11:02:20, hashimoto wrote: > On 2014/08/12 19:27:53, timvolodine wrote: > > On 2014/08/12 10:01:42, hashimoto wrote: > > > On 2014/08/11 16:03:00, timvolodine wrote: > > > > On 2014/08/11 08:19:15, hashimoto wrote: > > > > > Why do we need to do this? > > > > > > > > this means we cannot get battery updates, so need to propagate default > > values > > > to > > > > blink (i.e. meaning we cannot provide battery information according to > > > > specification) > > > > > > It seems the Chrome OS implmentation is not doing the same thing when > > > PowerManager fails to connect to the signal. > > > Is this really needed? > > > > I believe this is needed to cover the following (unlikely) situation: imagine > we > > can get current battery status, but fail to connect to the signal. We have two > > options: > > 1. either execute callback with the current status and do nothing, or > > 2. execute the callback with default values (meaning cannot provide data > > according to spec). > > > > I think option 2 is better as option 1 would result in stale data after some > > time which would be misleading. ChromeOS probably follows option 1 and we > should > > see if this can be fixed. > > When something goes wrong with D-Bus, it seems the current Chrome OS > implementation does nothing because PowerChanged() never gets called in error > cases. > Also, it's more natural to do nothing than running the observer callback with > the default value reserved to indicate an error. I think ChromeOS actually forces one callback on start: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/ba... The specification says that we should provide default values if they cannot be provided by the system. There is no error case, but we need to send something otherwise the promise in blink will never resolve. Also when we cannot update battery information the values will be stale after some time. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:309: status.level = round(percentage * 10) / 1000.f; On 2014/08/13 11:02:20, hashimoto wrote: > On 2014/08/12 19:27:52, timvolodine wrote: > > On 2014/08/12 10:01:42, hashimoto wrote: > > > On 2014/08/11 16:02:59, timvolodine wrote: > > > > On 2014/08/11 08:19:14, hashimoto wrote: > > > > > Why should we intentionally degrade precision? > > > > > > > > this is to avoid fingerprinting and not to trigger level changes in blink > > more > > > > often than needed. > > > > > > I'm not sure what "fingerprinting" means in this context. > > > Could you add a comment which describes why this is needed and why it should > > be > > > 3 digits of precision (not 2, 4, 5 or whatever)? > > > > > > Also, it seems existing implementations for Mac and Chrome OS are not doing > > > this. > > > Don't we need this for those platforms too? > > > If needed, how about moving this precision degrade code to the caller site > > (e.g. > > > Blink code)? > > > > Mac and Android do not need this rounding, don't know about chrome OS. > Chrome OS is much more important than Linux. > Please implement the same logic or resolve the TODO in short term if we should > really care about this problem. > > Interesting suggestion about moving to blink, added some comments + TODO for > > this. I'll check with the chromeOS guys on this. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_linux.h (right): https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.h:31: const base::DictionaryValue& dictionary, blink::WebBatteryStatus& status); On 2014/08/13 11:02:20, hashimoto wrote: > On 2014/08/12 19:27:53, timvolodine wrote: > > On 2014/08/12 10:01:43, hashimoto wrote: > > > On 2014/08/11 16:03:00, timvolodine wrote: > > > > On 2014/08/11 08:19:16, hashimoto wrote: > > > > > Our style guide prohibits use of non-const references. > > > > > > > > Done. made it return a scoped_ptr. > > > > > > WebBatteryStatus is a copyable class. > > > Why does this function return a scoped_ptr, instead of WebBatteryStatus? > > > > I was actually trying to avoid a copy, but the compiler should optimize the > > return value anyway.. so changed to return by value. > > Why do you care about a copy here when you allocate an unused DictionaryValue to > save a few key strokes for a NULL check? > Allocation should be more expensive. well, the empty dictionary is a special case (i.e. an error). but I've replaced the empty dict with NULL now. https://codereview.chromium.org/436683002/diff/140001/content/browser/battery... File content/browser/battery_status/battery_status_manager_linux.cc (right): https://codereview.chromium.org/436683002/diff/140001/content/browser/battery... content/browser/battery_status/battery_status_manager_linux.cc:194: options.connection_type = dbus::Bus::PRIVATE; On 2014/08/13 11:02:21, hashimoto wrote: > BTW, why don't you set options.dbus_task_runner? > This way you don't have to implement thread related logic after all. > > Also, it makes it easy to introduce DBusThreadManager-ish object to Linux > Chrome. (please refer to stevenjb@'s comment). currently dbus runs on this thread anyway not sure what would I set the dbus_task_runner to.. Yes, ideally we would have DBusThreadManager for linux, like chromeOS does, I suppose that would make dbus implementations simpler. https://codereview.chromium.org/436683002/diff/140001/content/browser/battery... content/browser/battery_status/battery_status_manager_linux.cc:208: system_bus_)); On 2014/08/13 11:02:21, hashimoto wrote: > What is the purpose of this? > > Do we need to process tasks at this moment? > Aren't we here because no one is interested in this bus? > > Also, can we be sure that no additional task is posted during the period between > this PostTask() and when ShutdownAndBlock() actually gets run? this is needed because StartListening can posttask on this thread (i.e. battery_proxy_->ConnectToSignal). so if we have StartListening() followed by StopListening() the latter should not close dbus immediately but after all residual tasks of StartListening(). https://codereview.chromium.org/436683002/diff/140001/content/browser/battery... content/browser/battery_status/battery_status_manager_linux.cc:245: callback_.Run(status); On 2014/08/13 11:02:20, hashimoto wrote: > Isn't this callback supposed to be called on the IO thread? yes it is, the callback can be executed on any thread, it will get dispatched to the IO thread by the battery_status_manager_service. https://codereview.chromium.org/436683002/diff/140001/content/browser/battery... content/browser/battery_status/battery_status_manager_linux.cc:270: if (!StartNotifierThreadIfNecessary()) On 2014/08/13 11:02:20, hashimoto wrote: > Does this code correctly handle a case where StartListeningBatterChange() is > called after StopListeningBatteryChange()? yeah good catch the notifier_thread_ in StopListeningBatteryChange should be non-null. Done. https://codereview.chromium.org/436683002/diff/140001/content/browser/battery... content/browser/battery_status/battery_status_manager_linux.cc:352: default: { On 2014/08/13 11:02:20, hashimoto wrote: > Don't we need to treat this case as an error? no, in other cases we just report what we can and leave the default values for chargingTime/dischargingTime (in accordance to the spec). https://codereview.chromium.org/436683002/diff/140001/content/browser/battery... File content/browser/battery_status/battery_status_manager_linux_unittest.cc (right): https://codereview.chromium.org/436683002/diff/140001/content/browser/battery... content/browser/battery_status/battery_status_manager_linux_unittest.cc:33: EXPECT_EQ(true, status.charging); On 2014/08/13 11:02:21, hashimoto wrote: > nit: You can use EXPECT_TRUE/FALSE for booleans. Done.
+ TBR=brettw@ : for content/browser/BUILD.gn
The CQ bit was checked by timvolodine@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/timvolodine@chromium.org/436683002/160001
The CQ bit was unchecked by timvolodine@chromium.org
The CQ bit was checked by timvolodine@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/timvolodine@chromium.org/436683002/160001
Message was sent while issue was closed.
Committed patchset #9 (160001) as 289372
The CQ bit was checked by timvolodine@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/timvolodine@chromium.org/436683002/180001
Message was sent while issue was closed.
Committed patchset #10 (180001) as 289431
Message was sent while issue was closed.
Leaving my not lgtm for the record. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_linux.cc (right): https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:66: kDBusPropertiesGetAll); On 2014/08/12 19:27:53, timvolodine wrote: > On 2014/08/12 10:01:42, hashimoto wrote: > > On 2014/08/11 16:02:59, timvolodine wrote: > > > On 2014/08/11 08:19:15, hashimoto wrote: > > > > Why don't you use dbus/property.h? > > > > > > Done. > > > > Sorry for not being clear, > > why don't you use dbus::PropertySet defined in property.h? > > I'll look into that, not sure if this will simplify current code though. ping? https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:221: // default values. On 2014/08/13 16:58:08, timvolodine wrote: > On 2014/08/13 11:02:20, hashimoto wrote: > > On 2014/08/12 19:27:53, timvolodine wrote: > > > On 2014/08/12 10:01:42, hashimoto wrote: > > > > On 2014/08/11 16:03:00, timvolodine wrote: > > > > > On 2014/08/11 08:19:15, hashimoto wrote: > > > > > > Why do we need to do this? > > > > > > > > > > this means we cannot get battery updates, so need to propagate default > > > values > > > > to > > > > > blink (i.e. meaning we cannot provide battery information according to > > > > > specification) > > > > > > > > It seems the Chrome OS implmentation is not doing the same thing when > > > > PowerManager fails to connect to the signal. > > > > Is this really needed? > > > > > > I believe this is needed to cover the following (unlikely) situation: > imagine > > we > > > can get current battery status, but fail to connect to the signal. We have > two > > > options: > > > 1. either execute callback with the current status and do nothing, or > > > 2. execute the callback with default values (meaning cannot provide data > > > according to spec). > > > > > > I think option 2 is better as option 1 would result in stale data after some > > > time which would be misleading. ChromeOS probably follows option 1 and we > > should > > > see if this can be fixed. > > > > When something goes wrong with D-Bus, it seems the current Chrome OS > > implementation does nothing because PowerChanged() never gets called in error > > cases. > > Also, it's more natural to do nothing than running the observer callback with > > the default value reserved to indicate an error. > > I think ChromeOS actually forces one callback on start: > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/ba... That RequestStatusUpdate() results in nothing if an error happens. > > The specification says that we should provide default values if they cannot be > provided by the system. There is no error case, but we need to send something > otherwise the promise in blink will never resolve. Also when we cannot update > battery information the values will be stale after some time. https://codereview.chromium.org/436683002/diff/140001/content/browser/battery... File content/browser/battery_status/battery_status_manager_linux.cc (right): https://codereview.chromium.org/436683002/diff/140001/content/browser/battery... content/browser/battery_status/battery_status_manager_linux.cc:194: options.connection_type = dbus::Bus::PRIVATE; On 2014/08/13 16:58:09, timvolodine wrote: > On 2014/08/13 11:02:21, hashimoto wrote: > > BTW, why don't you set options.dbus_task_runner? > > This way you don't have to implement thread related logic after all. > > > > Also, it makes it easy to introduce DBusThreadManager-ish object to Linux > > Chrome. (please refer to stevenjb@'s comment). > > currently dbus runs on this thread anyway not sure what would I set the > dbus_task_runner to.. Yes, ideally we would have DBusThreadManager for linux, > like chromeOS does, I suppose that would make dbus implementations simpler. What I meant was owning the bus on the IO thread and let the bus handle all D-Bus related business on another thread. This way every code you write would be running on the IO thread. https://codereview.chromium.org/436683002/diff/140001/content/browser/battery... content/browser/battery_status/battery_status_manager_linux.cc:208: system_bus_)); On 2014/08/13 16:58:09, timvolodine wrote: > On 2014/08/13 11:02:21, hashimoto wrote: > > What is the purpose of this? > > > > Do we need to process tasks at this moment? > > Aren't we here because no one is interested in this bus? > > > > Also, can we be sure that no additional task is posted during the period > between > > this PostTask() and when ShutdownAndBlock() actually gets run? > > this is needed because StartListening can posttask on this thread (i.e. > battery_proxy_->ConnectToSignal). so if we have StartListening() followed by > StopListening() the latter should not close dbus immediately but after all > residual tasks of StartListening(). Why do we need to process signals when we are shutting down? https://codereview.chromium.org/436683002/diff/140001/content/browser/battery... content/browser/battery_status/battery_status_manager_linux.cc:270: if (!StartNotifierThreadIfNecessary()) On 2014/08/13 16:58:09, timvolodine wrote: > On 2014/08/13 11:02:20, hashimoto wrote: > > Does this code correctly handle a case where StartListeningBatterChange() is > > called after StopListeningBatteryChange()? > > yeah good catch the notifier_thread_ in StopListeningBatteryChange should be > non-null. Done. No, that's not what I meant. Can we start listening again after StopListningBatteryChange() gets called?
Message was sent while issue was closed.
hashimoto@: Is there a specific reason for your !lgtm? If that's because of the PropertySet, I could look into that in a future refactoring. PropertySet is currently only used in chromeOS (bluethooth and rfc) and it looks like would require some significant changes to this patch. btw, do you have any ideas on how hard would it be to port chromeos::PowerManagerClient to linux? I am not very familiar with chromeos, but it looks like there is lots of functional similarity. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_linux.cc (right): https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:221: // default values. On 2014/08/14 04:17:04, hashimoto wrote: > On 2014/08/13 16:58:08, timvolodine wrote: > > On 2014/08/13 11:02:20, hashimoto wrote: > > > On 2014/08/12 19:27:53, timvolodine wrote: > > > > On 2014/08/12 10:01:42, hashimoto wrote: > > > > > On 2014/08/11 16:03:00, timvolodine wrote: > > > > > > On 2014/08/11 08:19:15, hashimoto wrote: > > > > > > > Why do we need to do this? > > > > > > > > > > > > this means we cannot get battery updates, so need to propagate default > > > > values > > > > > to > > > > > > blink (i.e. meaning we cannot provide battery information according to > > > > > > specification) > > > > > > > > > > It seems the Chrome OS implmentation is not doing the same thing when > > > > > PowerManager fails to connect to the signal. > > > > > Is this really needed? > > > > > > > > I believe this is needed to cover the following (unlikely) situation: > > imagine > > > we > > > > can get current battery status, but fail to connect to the signal. We have > > two > > > > options: > > > > 1. either execute callback with the current status and do nothing, or > > > > 2. execute the callback with default values (meaning cannot provide data > > > > according to spec). > > > > > > > > I think option 2 is better as option 1 would result in stale data after > some > > > > time which would be misleading. ChromeOS probably follows option 1 and we > > > should > > > > see if this can be fixed. > > > > > > When something goes wrong with D-Bus, it seems the current Chrome OS > > > implementation does nothing because PowerChanged() never gets called in > error > > > cases. > > > Also, it's more natural to do nothing than running the observer callback > with > > > the default value reserved to indicate an error. > > > > I think ChromeOS actually forces one callback on start: > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/ba... > That RequestStatusUpdate() results in nothing if an error happens. What I meant is that callback SignalConnected() is ignored in power_manager_client.cc (it simply logs a warning if it fails). So I was referring to the (hypothetical) case where the SignalConnected fails but the properties can be read from dbus in which case RequestStatusUpdate will propagate to observers. https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/dbus/powe... https://codereview.chromium.org/436683002/diff/140001/content/browser/battery... File content/browser/battery_status/battery_status_manager_linux.cc (right): https://codereview.chromium.org/436683002/diff/140001/content/browser/battery... content/browser/battery_status/battery_status_manager_linux.cc:194: options.connection_type = dbus::Bus::PRIVATE; On 2014/08/14 04:17:05, hashimoto wrote: > On 2014/08/13 16:58:09, timvolodine wrote: > > On 2014/08/13 11:02:21, hashimoto wrote: > > > BTW, why don't you set options.dbus_task_runner? > > > This way you don't have to implement thread related logic after all. > > > > > > Also, it makes it easy to introduce DBusThreadManager-ish object to Linux > > > Chrome. (please refer to stevenjb@'s comment). > > > > currently dbus runs on this thread anyway not sure what would I set the > > dbus_task_runner to.. Yes, ideally we would have DBusThreadManager for linux, > > like chromeOS does, I suppose that would make dbus implementations simpler. > > What I meant was owning the bus on the IO thread and let the bus handle all > D-Bus related business on another thread. > This way every code you write would be running on the IO thread. yes that's another possibility, but since we don't have a dbus thread on linux we need to run a separate thread anyway. Furthermore if on IO thread we cannot do blocking calls, which would complicate the implementation because we would need to fetch the information asynchronously. Note that current Geolocation implementation for linux does a similar thing. https://codereview.chromium.org/436683002/diff/140001/content/browser/battery... content/browser/battery_status/battery_status_manager_linux.cc:208: system_bus_)); On 2014/08/14 04:17:05, hashimoto wrote: > On 2014/08/13 16:58:09, timvolodine wrote: > > On 2014/08/13 11:02:21, hashimoto wrote: > > > What is the purpose of this? > > > > > > Do we need to process tasks at this moment? > > > Aren't we here because no one is interested in this bus? > > > > > > Also, can we be sure that no additional task is posted during the period > > between > > > this PostTask() and when ShutdownAndBlock() actually gets run? > > > > this is needed because StartListening can posttask on this thread (i.e. > > battery_proxy_->ConnectToSignal). so if we have StartListening() followed by > > StopListening() the latter should not close dbus immediately but after all > > residual tasks of StartListening(). > > Why do we need to process signals when we are shutting down? StartListening() posts a ObjectProxy::ConnectToSignalInternal on the thread which needs a life dbus connection. We can only shutdown it after that task has executed. https://codereview.chromium.org/436683002/diff/140001/content/browser/battery... content/browser/battery_status/battery_status_manager_linux.cc:270: if (!StartNotifierThreadIfNecessary()) On 2014/08/14 04:17:05, hashimoto wrote: > On 2014/08/13 16:58:09, timvolodine wrote: > > On 2014/08/13 11:02:20, hashimoto wrote: > > > Does this code correctly handle a case where StartListeningBatterChange() is > > > called after StopListeningBatteryChange()? > > > > yeah good catch the notifier_thread_ in StopListeningBatteryChange should be > > non-null. Done. > No, that's not what I meant. > > Can we start listening again after StopListningBatteryChange() gets called? yes.
Message was sent while issue was closed.
PowerManagerClient was made to communicate with Chrome OS's PowerManager. I don't think we can port it to Linux. https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_linux.cc (right): https://codereview.chromium.org/436683002/diff/80001/content/browser/battery_... content/browser/battery_status/battery_status_manager_linux.cc:221: // default values. On 2014/08/14 22:02:45, timvolodine wrote: > On 2014/08/14 04:17:04, hashimoto wrote: > > On 2014/08/13 16:58:08, timvolodine wrote: > > > On 2014/08/13 11:02:20, hashimoto wrote: > > > > On 2014/08/12 19:27:53, timvolodine wrote: > > > > > On 2014/08/12 10:01:42, hashimoto wrote: > > > > > > On 2014/08/11 16:03:00, timvolodine wrote: > > > > > > > On 2014/08/11 08:19:15, hashimoto wrote: > > > > > > > > Why do we need to do this? > > > > > > > > > > > > > > this means we cannot get battery updates, so need to propagate > default > > > > > values > > > > > > to > > > > > > > blink (i.e. meaning we cannot provide battery information according > to > > > > > > > specification) > > > > > > > > > > > > It seems the Chrome OS implmentation is not doing the same thing when > > > > > > PowerManager fails to connect to the signal. > > > > > > Is this really needed? > > > > > > > > > > I believe this is needed to cover the following (unlikely) situation: > > > imagine > > > > we > > > > > can get current battery status, but fail to connect to the signal. We > have > > > two > > > > > options: > > > > > 1. either execute callback with the current status and do nothing, or > > > > > 2. execute the callback with default values (meaning cannot provide data > > > > > according to spec). > > > > > > > > > > I think option 2 is better as option 1 would result in stale data after > > some > > > > > time which would be misleading. ChromeOS probably follows option 1 and > we > > > > should > > > > > see if this can be fixed. > > > > > > > > When something goes wrong with D-Bus, it seems the current Chrome OS > > > > implementation does nothing because PowerChanged() never gets called in > > error > > > > cases. > > > > Also, it's more natural to do nothing than running the observer callback > > with > > > > the default value reserved to indicate an error. > > > > > > I think ChromeOS actually forces one callback on start: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/ba... > > That RequestStatusUpdate() results in nothing if an error happens. > > What I meant is that callback SignalConnected() is ignored in > power_manager_client.cc (it simply logs a warning if it fails). So I was > referring to the (hypothetical) case where the SignalConnected fails but the > properties can be read from dbus in which case RequestStatusUpdate will > propagate to observers. > https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/dbus/powe... You shouldn't expect the service to return responses for method calls when SignalConnected() fails. https://codereview.chromium.org/436683002/diff/140001/content/browser/battery... File content/browser/battery_status/battery_status_manager_linux.cc (right): https://codereview.chromium.org/436683002/diff/140001/content/browser/battery... content/browser/battery_status/battery_status_manager_linux.cc:194: options.connection_type = dbus::Bus::PRIVATE; On 2014/08/14 22:02:45, timvolodine wrote: > On 2014/08/14 04:17:05, hashimoto wrote: > > On 2014/08/13 16:58:09, timvolodine wrote: > > > On 2014/08/13 11:02:21, hashimoto wrote: > > > > BTW, why don't you set options.dbus_task_runner? > > > > This way you don't have to implement thread related logic after all. > > > > > > > > Also, it makes it easy to introduce DBusThreadManager-ish object to Linux > > > > Chrome. (please refer to stevenjb@'s comment). > > > > > > currently dbus runs on this thread anyway not sure what would I set the > > > dbus_task_runner to.. Yes, ideally we would have DBusThreadManager for > linux, > > > like chromeOS does, I suppose that would make dbus implementations simpler. > > > > What I meant was owning the bus on the IO thread and let the bus handle all > > D-Bus related business on another thread. > > This way every code you write would be running on the IO thread. > > yes that's another possibility, but since we don't have a dbus thread on linux > we need to run a separate thread anyway. Furthermore if on IO thread we cannot > do blocking calls, which would complicate the implementation because we would > need to fetch the information asynchronously. Note that current Geolocation > implementation for linux does a similar thing. By using it you don't have to manually post tasks to the thread. Adding a few callbacks to handle asynchronous steps isn't anything. On Linux, developers tend to write their own code to setup dbus::Bus only because there is no DBusThreadManager. Having many classes which set up their own dbus::Bus is not desirable as Steven said. It's better to follow MediaTransferProtocolManager's example. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
