Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
101963004:
[chromeos] New PowerManagerClient observer to collect power data. (Closed)
|
Created:
7 years ago by Siva Chandra Modified:
7 years ago CC:
chromium-reviews, oshima+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[chromeos] New PowerManagerClient observer to collect power data.
The observer currently only collects power supply data. This CL
should be treated as the first in a series to address the
marked issue.
BUG=312956
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=241445
Patch Set 1 #
Total comments: 3
Patch Set 2 : Address the various comments about location etc. #
Total comments: 20
Patch Set 3 : Address comments in patch set 2 #
Total comments: 20
Patch Set 4 : Address comments in patch set 3 #Patch Set 5 : Fix a comment #Patch Set 6 : Remove a now unused global constant #
Total comments: 36
Patch Set 7 : Fix another round of comments #
Total comments: 47
Patch Set 8 : Address nits #
Total comments: 8
Patch Set 9 : address stevenjb's nits #
Total comments: 9
Patch Set 10 : #
Total comments: 10
Patch Set 11 : Fit more misses #
Total comments: 2
Patch Set 12 : Last nit #Patch Set 13 : Add CHROMEOS_EXPORT #Patch Set 14 : Add log statements #Patch Set 15 : #Patch Set 16 : #Patch Set 17 : rebase #Patch Set 18 : Should work! #Patch Set 19 : #
Messages
Total messages: 35 (0 generated)
Just as an early, high-level comment, this code probably shouldn't live in chromeos/dbus/. That directory holds code that communicates using D-Bus. Your new class just observes PowerManagerClient; as such, it should be able to live anywhere. Steven, any suggestions? If the collected data is eventually just going to appear at chrome://power, my first guess is that you'd want to add a new content::WebUIController implementation in chrome/browser/ui/webui/chromeos that also implements PowerManagerClient::Observer and passes the collected power information to your WebUI page.
On 2013/12/04 00:20:14, Daniel Erat wrote: > Just as an early, high-level comment, this code probably shouldn't live in > chromeos/dbus/. That directory holds code that communicates using D-Bus. Your > new class just observes PowerManagerClient; as such, it should be able to live > anywhere. > > Steven, any suggestions? If the collected data is eventually just going to > appear at chrome://power, my first guess is that you'd want to add a new > content::WebUIController implementation in chrome/browser/ui/webui/chromeos that > also implements PowerManagerClient::Observer and passes the collected power > information to your WebUI page. I agree. There should be a comment in power_data_collector.h that describes it's purpose, but unless there are plans to make this generally useful in multiple UI components (i.e. from both src/ash and src/chrome), it seems like something that could just be implemented as part of whatever consumes it (e.g. c/b/ui/webui/chromeos/power_ui.cc). Regardless it should be owned by the UI component, not by DBusThreadManager. Also: we should consider moving PowerManagerClient::Observer out of PowerManagerClient and into its own power_manager_client_obsver.h file. This will reduce header dependencies for observers (and simplify each header).
https://codereview.chromium.org/101963004/diff/1/chromeos/dbus/power_data_col... File chromeos/dbus/power_data_collector.cc (right): https://codereview.chromium.org/101963004/diff/1/chromeos/dbus/power_data_col... chromeos/dbus/power_data_collector.cc:12: : pm_client_(pm_client) { This should just set pm_client_(DBusThreadManager::Get()->GetPowerManagerClient()) https://codereview.chromium.org/101963004/diff/1/chromeos/dbus/power_data_col... chromeos/dbus/power_data_collector.cc:14: pm_client_->AddObserver(this); Probably want to call pm_client_->RequestStatusUpdate(); https://codereview.chromium.org/101963004/diff/1/chromeos/dbus/power_data_col... chromeos/dbus/power_data_collector.cc:29: PowerDataCollector::~PowerDataCollector() { Should follow constructor
On 2013/12/04 00:48:00, stevenjb wrote: > On 2013/12/04 00:20:14, Daniel Erat wrote: > > Just as an early, high-level comment, this code probably shouldn't live in > > chromeos/dbus/. That directory holds code that communicates using D-Bus. Your > > new class just observes PowerManagerClient; as such, it should be able to live > > anywhere. > > > > Steven, any suggestions? If the collected data is eventually just going to > > appear at chrome://power, my first guess is that you'd want to add a new > > content::WebUIController implementation in chrome/browser/ui/webui/chromeos > that > > also implements PowerManagerClient::Observer and passes the collected power > > information to your WebUI page. > > I agree. There should be a comment in power_data_collector.h that describes it's > purpose, but unless there are plans to make this generally useful in multiple UI > components (i.e. from both src/ash and src/chrome), it seems like something that > could just be implemented as part of whatever consumes it (e.g. > c/b/ui/webui/chromeos/power_ui.cc). Regardless it should be owned by the UI > component, not by DBusThreadManager. I don't have an opinion on where this code should live but just wanted to point out that we will likely want to include this data as part of feedback reports and automated test runs. > > Also: we should consider moving PowerManagerClient::Observer out of > PowerManagerClient and into its own power_manager_client_obsver.h file. This > will reduce header dependencies for observers (and simplify each header).
On 2013/12/04 18:31:34, Sameer Nanda wrote: > On 2013/12/04 00:48:00, stevenjb wrote: > > On 2013/12/04 00:20:14, Daniel Erat wrote: > > > Just as an early, high-level comment, this code probably shouldn't live in > > > chromeos/dbus/. That directory holds code that communicates using D-Bus. > Your > > > new class just observes PowerManagerClient; as such, it should be able to > live > > > anywhere. > > > > > > Steven, any suggestions? If the collected data is eventually just going to > > > appear at chrome://power, my first guess is that you'd want to add a new > > > content::WebUIController implementation in chrome/browser/ui/webui/chromeos > > that > > > also implements PowerManagerClient::Observer and passes the collected power > > > information to your WebUI page. > > > > I agree. There should be a comment in power_data_collector.h that describes > it's > > purpose, but unless there are plans to make this generally useful in multiple > UI > > components (i.e. from both src/ash and src/chrome), it seems like something > that > > could just be implemented as part of whatever consumes it (e.g. > > c/b/ui/webui/chromeos/power_ui.cc). Regardless it should be owned by the UI > > component, not by DBusThreadManager. > I don't have an opinion on where this code should live but just wanted to point > out that we will likely want to include this data as part of feedback reports > and automated test runs. Hmm. How would it get dumped in test runs? Do we currently have data from within Chrome dumped as part of these? Note that the power supply data collected here (and more) should already be present in powerd's log, which is included in feedback reports. My main point was just that the new code should probably live somewhere under chrome/browser/ instead of in chromeos/dbus/. If it's going to be used for things besides a WebUI page in the near future, I agree that c/b/ui/webui isn't the right place for it. > > > > > Also: we should consider moving PowerManagerClient::Observer out of > > PowerManagerClient and into its own power_manager_client_obsver.h file. This > > will reduce header dependencies for observers (and simplify each header).
On 2013/12/04 18:39:04, Daniel Erat wrote: > On 2013/12/04 18:31:34, Sameer Nanda wrote: > > On 2013/12/04 00:48:00, stevenjb wrote: > > > On 2013/12/04 00:20:14, Daniel Erat wrote: > > > > Just as an early, high-level comment, this code probably shouldn't live in > > > > chromeos/dbus/. That directory holds code that communicates using D-Bus. > > Your > > > > new class just observes PowerManagerClient; as such, it should be able to > > live > > > > anywhere. > > > > > > > > Steven, any suggestions? If the collected data is eventually just going to > > > > appear at chrome://power, my first guess is that you'd want to add a new > > > > content::WebUIController implementation in > chrome/browser/ui/webui/chromeos > > > that > > > > also implements PowerManagerClient::Observer and passes the collected > power > > > > information to your WebUI page. > > > > > > I agree. There should be a comment in power_data_collector.h that describes > > it's > > > purpose, but unless there are plans to make this generally useful in > multiple > > UI > > > components (i.e. from both src/ash and src/chrome), it seems like something > > that > > > could just be implemented as part of whatever consumes it (e.g. > > > c/b/ui/webui/chromeos/power_ui.cc). Regardless it should be owned by the UI > > > component, not by DBusThreadManager. > > I don't have an opinion on where this code should live but just wanted to > point > > out that we will likely want to include this data as part of feedback reports > > and automated test runs. > > Hmm. How would it get dumped in test runs? Maybe via a dbus message sent to Chrome to dump the data to a log/HTML file? > Do we currently have data from within > Chrome dumped as part of these? Not directly, but there are a whole bunch of log files that get collected as part of test run. > > Note that the power supply data collected here (and more) should already be > present in powerd's log, which is included in feedback reports. Correct. The tests collect the power data directly as well. > > My main point was just that the new code should probably live somewhere under > chrome/browser/ instead of in chromeos/dbus/. If it's going to be used for > things besides a WebUI page in the near future, I agree that c/b/ui/webui isn't > the right place for it. I am not that familiar with Chrome best practices so up to you folks on the location. But yes, I do see other uses of this data aside from displaying it in the UI. > > > > > > > > > Also: we should consider moving PowerManagerClient::Observer out of > > > PowerManagerClient and into its own power_manager_client_obsver.h file. This > > > will reduce header dependencies for observers (and simplify each header).
On 2013/12/04 19:30:13, Sameer Nanda wrote: > On 2013/12/04 18:39:04, Daniel Erat wrote: > > On 2013/12/04 18:31:34, Sameer Nanda wrote: > > > On 2013/12/04 00:48:00, stevenjb wrote: > > > > On 2013/12/04 00:20:14, Daniel Erat wrote: > > > > > Just as an early, high-level comment, this code probably shouldn't live > in > > > > > chromeos/dbus/. That directory holds code that communicates using D-Bus. > > > Your > > > > > new class just observes PowerManagerClient; as such, it should be able > to > > > live > > > > > anywhere. > > > > > > > > > > Steven, any suggestions? If the collected data is eventually just going > to > > > > > appear at chrome://power, my first guess is that you'd want to add a new > > > > > content::WebUIController implementation in > > chrome/browser/ui/webui/chromeos > > > > that > > > > > also implements PowerManagerClient::Observer and passes the collected > > power > > > > > information to your WebUI page. > > > > > > > > I agree. There should be a comment in power_data_collector.h that > describes > > > it's > > > > purpose, but unless there are plans to make this generally useful in > > multiple > > > UI > > > > components (i.e. from both src/ash and src/chrome), it seems like > something > > > that > > > > could just be implemented as part of whatever consumes it (e.g. > > > > c/b/ui/webui/chromeos/power_ui.cc). Regardless it should be owned by the > UI > > > > component, not by DBusThreadManager. > > > I don't have an opinion on where this code should live but just wanted to > > point > > > out that we will likely want to include this data as part of feedback > reports > > > and automated test runs. > > > > Hmm. How would it get dumped in test runs? > Maybe via a dbus message sent to Chrome to dump the data to a log/HTML file? > > > Do we currently have data from within > > Chrome dumped as part of these? > Not directly, but there are a whole bunch of log files that get collected as > part of test run. > > > > > Note that the power supply data collected here (and more) should already be > > present in powerd's log, which is included in feedback reports. > Correct. The tests collect the power data directly as well. > > > > > My main point was just that the new code should probably live somewhere under > > chrome/browser/ instead of in chromeos/dbus/. If it's going to be used for > > things besides a WebUI page in the near future, I agree that c/b/ui/webui > isn't > > the right place for it. > > I am not that familiar with Chrome best practices so up to you folks on the > location. But yes, I do see other uses of this data aside from displaying it in > the UI. > > > > > > > > > > > > > > Also: we should consider moving PowerManagerClient::Observer out of > > > > PowerManagerClient and into its own power_manager_client_obsver.h file. > This > > > > will reduce header dependencies for observers (and simplify each header). Often I find it better to implement something like this with the class that initially needs it, then extract it to a common location when another need arises. That said, if we're pretty certain we will need it in more than one place, then this should go in c/b/chromeos/power/.
On 2013/12/04 19:35:33, stevenjb wrote: > On 2013/12/04 19:30:13, Sameer Nanda wrote: > > On 2013/12/04 18:39:04, Daniel Erat wrote: > > > On 2013/12/04 18:31:34, Sameer Nanda wrote: > > > > On 2013/12/04 00:48:00, stevenjb wrote: > > > > > On 2013/12/04 00:20:14, Daniel Erat wrote: > > > > > > Just as an early, high-level comment, this code probably shouldn't > live > > in > > > > > > chromeos/dbus/. That directory holds code that communicates using > D-Bus. > > > > Your > > > > > > new class just observes PowerManagerClient; as such, it should be able > > to > > > > live > > > > > > anywhere. > > > > > > > > > > > > Steven, any suggestions? If the collected data is eventually just > going > > to > > > > > > appear at chrome://power, my first guess is that you'd want to add a > new > > > > > > content::WebUIController implementation in > > > chrome/browser/ui/webui/chromeos > > > > > that > > > > > > also implements PowerManagerClient::Observer and passes the collected > > > power > > > > > > information to your WebUI page. > > > > > > > > > > I agree. There should be a comment in power_data_collector.h that > > describes > > > > it's > > > > > purpose, but unless there are plans to make this generally useful in > > > multiple > > > > UI > > > > > components (i.e. from both src/ash and src/chrome), it seems like > > something > > > > that > > > > > could just be implemented as part of whatever consumes it (e.g. > > > > > c/b/ui/webui/chromeos/power_ui.cc). Regardless it should be owned by the > > UI > > > > > component, not by DBusThreadManager. > > > > I don't have an opinion on where this code should live but just wanted to > > > point > > > > out that we will likely want to include this data as part of feedback > > reports > > > > and automated test runs. > > > > > > Hmm. How would it get dumped in test runs? > > Maybe via a dbus message sent to Chrome to dump the data to a log/HTML file? > > > > > Do we currently have data from within > > > Chrome dumped as part of these? > > Not directly, but there are a whole bunch of log files that get collected as > > part of test run. > > > > > > > > Note that the power supply data collected here (and more) should already be > > > present in powerd's log, which is included in feedback reports. > > Correct. The tests collect the power data directly as well. > > > > > > > > My main point was just that the new code should probably live somewhere > under > > > chrome/browser/ instead of in chromeos/dbus/. If it's going to be used for > > > things besides a WebUI page in the near future, I agree that c/b/ui/webui > > isn't > > > the right place for it. > > > > I am not that familiar with Chrome best practices so up to you folks on the > > location. But yes, I do see other uses of this data aside from displaying it > in > > the UI. > > > > > > > > > > > > > > > > > > > Also: we should consider moving PowerManagerClient::Observer out of > > > > > PowerManagerClient and into its own power_manager_client_obsver.h file. > > This > > > > > will reduce header dependencies for observers (and simplify each > header). > > Often I find it better to implement something like this with the class that > initially needs it, then extract it to a common location when another need > arises. That said, if we're pretty certain we will need it in more than one > place, then this should go in c/b/chromeos/power/. I agree with regard to putting this in the place where it's used at first and moving the code to a more general location if/when the need arises.
On 2013/12/04 19:35:33, stevenjb wrote: > On 2013/12/04 19:30:13, Sameer Nanda wrote: > > On 2013/12/04 18:39:04, Daniel Erat wrote: > > > On 2013/12/04 18:31:34, Sameer Nanda wrote: > > > > On 2013/12/04 00:48:00, stevenjb wrote: > > > > > On 2013/12/04 00:20:14, Daniel Erat wrote: > > > > > > Just as an early, high-level comment, this code probably shouldn't > live > > in > > > > > > chromeos/dbus/. That directory holds code that communicates using > D-Bus. > > > > Your > > > > > > new class just observes PowerManagerClient; as such, it should be able > > to > > > > live > > > > > > anywhere. > > > > > > > > > > > > Steven, any suggestions? If the collected data is eventually just > going > > to > > > > > > appear at chrome://power, my first guess is that you'd want to add a > new > > > > > > content::WebUIController implementation in > > > chrome/browser/ui/webui/chromeos > > > > > that > > > > > > also implements PowerManagerClient::Observer and passes the collected > > > power > > > > > > information to your WebUI page. > > > > > > > > > > I agree. There should be a comment in power_data_collector.h that > > describes > > > > it's > > > > > purpose, but unless there are plans to make this generally useful in > > > multiple > > > > UI > > > > > components (i.e. from both src/ash and src/chrome), it seems like > > something > > > > that > > > > > could just be implemented as part of whatever consumes it (e.g. > > > > > c/b/ui/webui/chromeos/power_ui.cc). Regardless it should be owned by the > > UI > > > > > component, not by DBusThreadManager. > > > > I don't have an opinion on where this code should live but just wanted to > > > point > > > > out that we will likely want to include this data as part of feedback > > reports > > > > and automated test runs. > > > > > > Hmm. How would it get dumped in test runs? > > Maybe via a dbus message sent to Chrome to dump the data to a log/HTML file? > > > > > Do we currently have data from within > > > Chrome dumped as part of these? > > Not directly, but there are a whole bunch of log files that get collected as > > part of test run. > > > > > > > > Note that the power supply data collected here (and more) should already be > > > present in powerd's log, which is included in feedback reports. > > Correct. The tests collect the power data directly as well. > > > > > > > > My main point was just that the new code should probably live somewhere > under > > > chrome/browser/ instead of in chromeos/dbus/. If it's going to be used for > > > things besides a WebUI page in the near future, I agree that c/b/ui/webui > > isn't > > > the right place for it. > > > > I am not that familiar with Chrome best practices so up to you folks on the > > location. But yes, I do see other uses of this data aside from displaying it > in > > the UI. > > > > > > > > > > > > > > > > > > > Also: we should consider moving PowerManagerClient::Observer out of > > > > > PowerManagerClient and into its own power_manager_client_obsver.h file. > > This > > > > > will reduce header dependencies for observers (and simplify each > header). > > Often I find it better to implement something like this with the class that > initially needs it, then extract it to a common location when another need > arises. That said, if we're pretty certain we will need it in more than one > place, then this should go in c/b/chromeos/power/. Or, actually, since this doesn't have any src/chrome dependencies, we should probably place it in src/chromeos/power (new directory). I don't think it needs to be global, i.e. it could be owned by whichever UI component depends on it.
Please look at patch set 2. Gist of changes: 1. I have moved the files to chromeos/power (new dir). 2. We want the power data to be collected right from startup. Hence, we cannot have the data collector instance be owned by the UI component. I have implemented it such that a single global instance of power data collector exists. 3. Since the UI component displaying the power data would read while the data collector is collecting the data, we will need a synchronization mechanism. I have implemented this. Kindly bare as this is my first chromium C++ CL and I am not sure I am fully familiar yet with the idioms.
just some high-level comments; haven't looked at much more than the header https://chromiumcodereview.appspot.com/101963004/diff/20001/chromeos/power/po... File chromeos/power/power_data_collector.cc (right): https://chromiumcodereview.appspot.com/101963004/diff/20001/chromeos/power/po... chromeos/power/power_data_collector.cc:45: base::Time now = base::Time::NowFromSystemTime(); use base::TimeTicks (CLOCK_MONOTONIC), not base::Time (wall time) https://chromiumcodereview.appspot.com/101963004/diff/20001/chromeos/power/po... File chromeos/power/power_data_collector.h (right): https://chromiumcodereview.appspot.com/101963004/diff/20001/chromeos/power/po... chromeos/power/power_data_collector.h:5: #ifndef CHROMEOS_DBUS_POWER_DATA_COLLECTOR_H_ nit: update these to reflect the new path (don't forget the #endif at the end of the file) https://chromiumcodereview.appspot.com/101963004/diff/20001/chromeos/power/po... chromeos/power/power_data_collector.h:34: int64 time; why not just use base::TimeTicks here? the internal value is there for serialization https://chromiumcodereview.appspot.com/101963004/diff/20001/chromeos/power/po... chromeos/power/power_data_collector.h:36: // true if connected to externel power at the time of the snapshot. nit: s/externel/external/ https://chromiumcodereview.appspot.com/101963004/diff/20001/chromeos/power/po... chromeos/power/power_data_collector.h:51: // Call this when processing the power supply data got from nit: s/got/received/ https://chromiumcodereview.appspot.com/101963004/diff/20001/chromeos/power/po... chromeos/power/power_data_collector.h:53: static void LockPowerSupplyData(void); this lock/unlock mechanism seems error-prone. why is it necessary? are you planning to access this data from outside of the UI thread? are you planning to hang onto the reference for an extended period of time? if the answer to both of those questions is "no", i don't see why the lock is necessary. https://chromiumcodereview.appspot.com/101963004/diff/20001/chromeos/power/po... chromeos/power/power_data_collector.h:58: static std::vector<PowerSupplySnapshot> &GetPowerSupplyData(void); instead of having a bunch of static methods, i think that the typical pattern is to have a single static Get() method that returns the singleton and implement everything else as instance methods https://chromiumcodereview.appspot.com/101963004/diff/20001/chromeos/power/po... chromeos/power/power_data_collector.h:83: virtual void PowerChanged(const power_manager::PowerSupplyProperties& prop); list virtual methods after other methods, add "// PowerManagerClient::Observer implementation:" comment, and use OVERRIDE (see any other chrome classes for an example) https://chromiumcodereview.appspot.com/101963004/diff/20001/chromeos/power/po... chromeos/power/power_data_collector.h:85: void Shutdown_Instance(void); google c++ naming style would be ShutdownInstance
PTaL https://chromiumcodereview.appspot.com/101963004/diff/20001/chromeos/power/po... File chromeos/power/power_data_collector.cc (right): https://chromiumcodereview.appspot.com/101963004/diff/20001/chromeos/power/po... chromeos/power/power_data_collector.cc:45: base::Time now = base::Time::NowFromSystemTime(); On 2013/12/11 01:06:52, Daniel Erat wrote: > use base::TimeTicks (CLOCK_MONOTONIC), not base::Time (wall time) Done. https://chromiumcodereview.appspot.com/101963004/diff/20001/chromeos/power/po... File chromeos/power/power_data_collector.h (right): https://chromiumcodereview.appspot.com/101963004/diff/20001/chromeos/power/po... chromeos/power/power_data_collector.h:5: #ifndef CHROMEOS_DBUS_POWER_DATA_COLLECTOR_H_ On 2013/12/11 01:06:52, Daniel Erat wrote: > nit: update these to reflect the new path (don't forget the #endif at the end of > the file) Done. https://chromiumcodereview.appspot.com/101963004/diff/20001/chromeos/power/po... chromeos/power/power_data_collector.h:34: int64 time; On 2013/12/11 01:06:52, Daniel Erat wrote: > why not just use base::TimeTicks here? the internal value is there for > serialization Done. How does one convert time ticks to human understandable time deltas? https://chromiumcodereview.appspot.com/101963004/diff/20001/chromeos/power/po... chromeos/power/power_data_collector.h:36: // true if connected to externel power at the time of the snapshot. On 2013/12/11 01:06:52, Daniel Erat wrote: > nit: s/externel/external/ Done. https://chromiumcodereview.appspot.com/101963004/diff/20001/chromeos/power/po... chromeos/power/power_data_collector.h:51: // Call this when processing the power supply data got from On 2013/12/11 01:06:52, Daniel Erat wrote: > nit: s/got/received/ Done. https://chromiumcodereview.appspot.com/101963004/diff/20001/chromeos/power/po... chromeos/power/power_data_collector.h:53: static void LockPowerSupplyData(void); On 2013/12/11 01:06:52, Daniel Erat wrote: > this lock/unlock mechanism seems error-prone. Why is it error-prone? > are you > planning to access this data from outside of the UI thread? Correct me if I am wrong in my understanding here. My understanding here is that UI thread (which is a process actually) has a number of threads running. So, in our case, we want the UI thread displaying the about:power page be able to read the data being being accumulated by the power data collector. So, potentially, a read from the UI thread could be happening while there is a write happening via the DBusThreadManager. > are you planning to > hang onto the reference for an extended period of time? What is "extended period of time". In my opinion, if there is a possibility of the vector's size being updated while someone is reading it, then we should have synchronization. https://chromiumcodereview.appspot.com/101963004/diff/20001/chromeos/power/po... chromeos/power/power_data_collector.h:58: static std::vector<PowerSupplySnapshot> &GetPowerSupplyData(void); On 2013/12/11 01:06:52, Daniel Erat wrote: > instead of having a bunch of static methods, i think that the typical pattern is > to have a single static Get() method that returns the singleton and implement > everything else as instance methods Done. https://chromiumcodereview.appspot.com/101963004/diff/20001/chromeos/power/po... chromeos/power/power_data_collector.h:83: virtual void PowerChanged(const power_manager::PowerSupplyProperties& prop); On 2013/12/11 01:06:52, Daniel Erat wrote: > list virtual methods after other methods, add "// PowerManagerClient::Observer > implementation:" comment, and use OVERRIDE (see any other chrome classes for an > example) Done. https://chromiumcodereview.appspot.com/101963004/diff/20001/chromeos/power/po... chromeos/power/power_data_collector.h:85: void Shutdown_Instance(void); On 2013/12/11 01:06:52, Daniel Erat wrote: > google c++ naming style would be ShutdownInstance Done.
https://chromiumcodereview.appspot.com/101963004/diff/40001/chrome/browser/ch... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://chromiumcodereview.appspot.com/101963004/diff/40001/chrome/browser/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:409: PowerDataCollector::Initialize(); This should be initialized and shut down as part of DBusServices. (Which would maybe better be named "DBusRelatedServices"). https://chromiumcodereview.appspot.com/101963004/diff/40001/chromeos/power/DEPS File chromeos/power/DEPS (right): https://chromiumcodereview.appspot.com/101963004/diff/40001/chromeos/power/DE... chromeos/power/DEPS:2: "+content/public/browser", src/chromeos should not depend on content. (I know that low_memory_listener.cc does, but there's an issue to fix that). https://chromiumcodereview.appspot.com/101963004/diff/40001/chromeos/power/po... File chromeos/power/power_data_collector.cc (right): https://chromiumcodereview.appspot.com/101963004/diff/40001/chromeos/power/po... chromeos/power/power_data_collector.cc:8: #include "content/public/browser/browser_thread.h" We shouldn't use BrowserThread in src/chromeos, but I don't think you need it anyway. https://chromiumcodereview.appspot.com/101963004/diff/40001/chromeos/power/po... chromeos/power/power_data_collector.cc:48: prop.battery_percent()}; } on new line https://chromiumcodereview.appspot.com/101963004/diff/40001/chromeos/power/po... chromeos/power/power_data_collector.cc:54: &PowerDataCollector::AppendPowerSupplySnapshot, this, snapshot)); It should be safe to do this immediately, no need for locking. PowerChanged will be called on the UI thread. https://chromiumcodereview.appspot.com/101963004/diff/40001/chromeos/power/po... File chromeos/power/power_data_collector.h (right): https://chromiumcodereview.appspot.com/101963004/diff/40001/chromeos/power/po... chromeos/power/power_data_collector.h:28: // shutdown. This paragraph could be simplified, e.g.: "This class is implemented as a global singleton, initialized after DBusThreadManager which it depends on." (The details are already sufficiently commented with each method) https://chromiumcodereview.appspot.com/101963004/diff/40001/chromeos/power/po... chromeos/power/power_data_collector.h:34: // Generated by TimeTicks::Now. Describe what this represents (the time of the snapshot) not the implementation. https://chromiumcodereview.appspot.com/101963004/diff/40001/chromeos/power/po... chromeos/power/power_data_collector.h:37: // true if connected to external power at the time of the snapshot. True https://chromiumcodereview.appspot.com/101963004/diff/40001/chromeos/power/po... chromeos/power/power_data_collector.h:62: std::vector<PowerSupplySnapshot> &GetPowerSupplyData(void); I don't think we should introduce a lock just to avoid a copy, but also I don't expect you will actually need to access this from anything except the UI thread. (The PowerManagerClient::Observer will all be called on the UI thread).
https://chromiumcodereview.appspot.com/101963004/diff/20001/chromeos/power/po... File chromeos/power/power_data_collector.h (right): https://chromiumcodereview.appspot.com/101963004/diff/20001/chromeos/power/po... chromeos/power/power_data_collector.h:34: int64 time; On 2013/12/11 20:32:02, Siva Chandra wrote: > On 2013/12/11 01:06:52, Daniel Erat wrote: > > why not just use base::TimeTicks here? the internal value is there for > > serialization > > Done. > > How does one convert time ticks to human understandable time deltas? base::TimeDelta is a class that represents an interval of time (e.g. 5 seconds, 1 hour, etc.). you can add and subtract TimeDeltas from Times and TimeTicks. base::TimeTicks represents a point in time since the system booted. it's based on the kernel's mononotically-increasing clock, so it won't e.g. go backwards if the system clock gets adjusted. base::Time represents a point in time based on the system clock (a.k.a. "wall time"). it can go backwards. in general, you should use TimeTicks for everything. you can get a TimeDelta by subtracting one TimeTicks from another TimeTicks and then convert that to a human-understandable interval of time. https://chromiumcodereview.appspot.com/101963004/diff/20001/chromeos/power/po... chromeos/power/power_data_collector.h:53: static void LockPowerSupplyData(void); On 2013/12/11 20:32:02, Siva Chandra wrote: > On 2013/12/11 01:06:52, Daniel Erat wrote: > > this lock/unlock mechanism seems error-prone. > > Why is it error-prone? it is easy for a caller to acquire the lock and later forget to release it is. it is easy to accidentally introduce deadlocks. > > are you > > planning to access this data from outside of the UI thread? > > Correct me if I am wrong in my understanding here. > > My understanding here is that UI thread (which is a process actually) has a > number of threads running. So, in our case, we want the UI thread displaying the > about:power page be able to read the data being being accumulated by the power > data collector. So, potentially, a read from the UI thread could be happening > while there is a write happening via the DBusThreadManager. there is a browser process with a UI thread (and some other threads). DBusThreadManager notifies observers on the UI thread (so that is where PowerChanged() will run). the code that IPCs the data to javascript will also be running on the UI thread (so that is where GetPowerSupplyData() will run). all of these things run as tasks on the same message loop; there's no need for synchronization here. > > are you planning to > > hang onto the reference for an extended period of time? > > What is "extended period of time". In my opinion, if there is a possibility of > the vector's size being updated while someone is reading it, then we should have > synchronization. there is no possibility of this happening unless whatever is going to call GetPowerSupplyData() needs to hang on to the data and continue accessing it after control returns to the message loop (which you shouldn't need to do).
PTAL. Addressed all comments except one for which I have a question. https://chromiumcodereview.appspot.com/101963004/diff/40001/chrome/browser/ch... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://chromiumcodereview.appspot.com/101963004/diff/40001/chrome/browser/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:409: PowerDataCollector::Initialize(); On 2013/12/11 20:54:37, stevenjb wrote: > This should be initialized and shut down as part of DBusServices. (Which would > maybe better be named "DBusRelatedServices"). > While I do not have any thing against it, I have a point to make. Only a part of PowerDataCollector listens to PowerManagerClient. Should this warrant us to treat as it as a DBusRelatedService? https://chromiumcodereview.appspot.com/101963004/diff/40001/chromeos/power/DEPS File chromeos/power/DEPS (right): https://chromiumcodereview.appspot.com/101963004/diff/40001/chromeos/power/DE... chromeos/power/DEPS:2: "+content/public/browser", On 2013/12/11 20:54:37, stevenjb wrote: > src/chromeos should not depend on content. (I know that low_memory_listener.cc > does, but there's an issue to fix that). Removed. https://chromiumcodereview.appspot.com/101963004/diff/40001/chromeos/power/po... File chromeos/power/power_data_collector.cc (right): https://chromiumcodereview.appspot.com/101963004/diff/40001/chromeos/power/po... chromeos/power/power_data_collector.cc:8: #include "content/public/browser/browser_thread.h" On 2013/12/11 20:54:37, stevenjb wrote: > We shouldn't use BrowserThread in src/chromeos, but I don't think you need it > anyway. Removed. https://chromiumcodereview.appspot.com/101963004/diff/40001/chromeos/power/po... chromeos/power/power_data_collector.cc:48: prop.battery_percent()}; On 2013/12/11 20:54:37, stevenjb wrote: > } on new line Done. https://chromiumcodereview.appspot.com/101963004/diff/40001/chromeos/power/po... chromeos/power/power_data_collector.cc:54: &PowerDataCollector::AppendPowerSupplySnapshot, this, snapshot)); On 2013/12/11 20:54:37, stevenjb wrote: > It should be safe to do this immediately, no need for locking. PowerChanged will > be called on the UI thread. Done. https://chromiumcodereview.appspot.com/101963004/diff/40001/chromeos/power/po... File chromeos/power/power_data_collector.h (right): https://chromiumcodereview.appspot.com/101963004/diff/40001/chromeos/power/po... chromeos/power/power_data_collector.h:28: // shutdown. On 2013/12/11 20:54:37, stevenjb wrote: > This paragraph could be simplified, e.g.: "This class is implemented as a global > singleton, initialized after DBusThreadManager which it depends on." > > (The details are already sufficiently commented with each method) > Done. https://chromiumcodereview.appspot.com/101963004/diff/40001/chromeos/power/po... chromeos/power/power_data_collector.h:34: // Generated by TimeTicks::Now. On 2013/12/11 20:54:37, stevenjb wrote: > Describe what this represents (the time of the snapshot) not the implementation. Done. https://chromiumcodereview.appspot.com/101963004/diff/40001/chromeos/power/po... chromeos/power/power_data_collector.h:37: // true if connected to external power at the time of the snapshot. On 2013/12/11 20:54:37, stevenjb wrote: > True Done. https://chromiumcodereview.appspot.com/101963004/diff/40001/chromeos/power/po... chromeos/power/power_data_collector.h:62: std::vector<PowerSupplySnapshot> &GetPowerSupplyData(void); On 2013/12/11 20:54:37, stevenjb wrote: > I don't think we should introduce a lock just to avoid a copy, but also I don't > expect you will actually need to access this from anything except the UI thread. > (The PowerManagerClient::Observer will all be called on the UI thread). Done.
https://chromiumcodereview.appspot.com/101963004/diff/100001/chromeos/power/p... File chromeos/power/power_data_collector.cc (right): https://chromiumcodereview.appspot.com/101963004/diff/100001/chromeos/power/p... chromeos/power/power_data_collector.cc:8: #include "chromeos/power/power_data_collector.h" move this include to the top of the list and separate it from the subsequent includes with a blank line (to match the rest of chrome) https://chromiumcodereview.appspot.com/101963004/diff/100001/chromeos/power/p... chromeos/power/power_data_collector.cc:13: static PowerDataCollector* power_data_collector; put this in a nested anonymous namespace instead of using 'static' and explicitly initialize it to NULL https://chromiumcodereview.appspot.com/101963004/diff/100001/chromeos/power/p... chromeos/power/power_data_collector.cc:17: AddRef(); delete this https://chromiumcodereview.appspot.com/101963004/diff/100001/chromeos/power/p... chromeos/power/power_data_collector.cc:20: DBusThreadManager::Get()->GetPowerManagerClient()->AddObserver(this); i don't think you need this |testing| flag; can't you just use the mock or stub DBusThreadManager when running on tests? https://chromiumcodereview.appspot.com/101963004/diff/100001/chromeos/power/p... chromeos/power/power_data_collector.cc:24: void PowerDataCollector::ShutdownInstance() { (mentioned earlier: put this in the destructor instead) https://chromiumcodereview.appspot.com/101963004/diff/100001/chromeos/power/p... chromeos/power/power_data_collector.cc:34: Release(); delete this https://chromiumcodereview.appspot.com/101963004/diff/100001/chromeos/power/p... chromeos/power/power_data_collector.cc:39: PowerSupplySnapshot snapshot = { nit: this would be a bit clearer: PowerSupplySnapshot snapshot; snapshot.time = base::TimeTicks::Now(); etc. https://chromiumcodereview.appspot.com/101963004/diff/100001/chromeos/power/p... chromeos/power/power_data_collector.cc:54: // Do no allow intializing twice. nit: s/no/not/ (but i don't think that this comment is necessary; i'd probably just delete it and remove the blank lines between the three statements in this method) https://chromiumcodereview.appspot.com/101963004/diff/100001/chromeos/power/p... chromeos/power/power_data_collector.cc:60: void PowerDataCollector::InitializeTesting() { remove this (see above) https://chromiumcodereview.appspot.com/101963004/diff/100001/chromeos/power/p... chromeos/power/power_data_collector.cc:76: // NULL it to protect from improper usage after shutdown. nit: delete unnecessary comments and blank lines here too https://chromiumcodereview.appspot.com/101963004/diff/100001/chromeos/power/p... chromeos/power/power_data_collector.cc:85: return power_data_collector->power_supply_; (see earlier comment about just making this be an inlined accessor, but note that in general, you'd want to just return |power_supply_| here instead of having an instance method return data from a global instance instead of from itself) https://chromiumcodereview.appspot.com/101963004/diff/100001/chromeos/power/p... File chromeos/power/power_data_collector.h (right): https://chromiumcodereview.appspot.com/101963004/diff/100001/chromeos/power/p... chromeos/power/power_data_collector.h:11: #include "base/synchronization/lock.h" nit: remove this https://chromiumcodereview.appspot.com/101963004/diff/100001/chromeos/power/p... chromeos/power/power_data_collector.h:28: public base::RefCountedThreadSafe<PowerDataCollector> { you don't need this, i don't think (as this will only be created, accessed, and destroyed from the UI thread, and there won't be any references to it floating around, just a global pointer) https://chromiumcodereview.appspot.com/101963004/diff/100001/chromeos/power/p... chromeos/power/power_data_collector.h:30: struct PowerSupplySnapshot { this creates a default c'tor that leaves |external_power| and |battery_charge| uninitialized. please explicitly add a no-args c'tor that initializes those fields to sane values https://chromiumcodereview.appspot.com/101963004/diff/100001/chromeos/power/p... chromeos/power/power_data_collector.h:50: static PowerDataCollector *Get(); nit: move '*' to left side of space to match the rest of chrome (i.e. PowerDataCollector* Get()) https://chromiumcodereview.appspot.com/101963004/diff/100001/chromeos/power/p... chromeos/power/power_data_collector.h:53: std::vector<PowerSupplySnapshot> &GetPowerSupplyData(void); nit: make this be a const reference and move '&' to the left of the space note that you can just make it be an inlined accessor, though (i.e. rename the member variable to |power_supply_snapshots_| and add an inlined const std::vector<PowerSupplySnapshot>& power_supply_snapshots() const method) https://chromiumcodereview.appspot.com/101963004/diff/100001/chromeos/power/p... chromeos/power/power_data_collector.h:65: virtual ~PowerDataCollector() {} move the destructor implementation to the .cc file instead of leaving it in the header https://chromiumcodereview.appspot.com/101963004/diff/100001/chromeos/power/p... chromeos/power/power_data_collector.h:71: void ShutdownInstance(void); get rid of this method and just do cleanup in the d'tor
PTAL at patch set 7. https://chromiumcodereview.appspot.com/101963004/diff/100001/chromeos/power/p... File chromeos/power/power_data_collector.cc (right): https://chromiumcodereview.appspot.com/101963004/diff/100001/chromeos/power/p... chromeos/power/power_data_collector.cc:8: #include "chromeos/power/power_data_collector.h" On 2013/12/11 22:26:47, Daniel Erat wrote: > move this include to the top of the list and separate it from the subsequent > includes with a blank line (to match the rest of chrome) Done. https://chromiumcodereview.appspot.com/101963004/diff/100001/chromeos/power/p... chromeos/power/power_data_collector.cc:13: static PowerDataCollector* power_data_collector; On 2013/12/11 22:26:47, Daniel Erat wrote: > put this in a nested anonymous namespace instead of using 'static' and > explicitly initialize it to NULL Done. https://chromiumcodereview.appspot.com/101963004/diff/100001/chromeos/power/p... chromeos/power/power_data_collector.cc:17: AddRef(); On 2013/12/11 22:26:47, Daniel Erat wrote: > delete this Done. https://chromiumcodereview.appspot.com/101963004/diff/100001/chromeos/power/p... chromeos/power/power_data_collector.cc:20: DBusThreadManager::Get()->GetPowerManagerClient()->AddObserver(this); On 2013/12/11 22:26:47, Daniel Erat wrote: > i don't think you need this |testing| flag; can't you just use the mock or stub > DBusThreadManager when running on tests? Done. https://chromiumcodereview.appspot.com/101963004/diff/100001/chromeos/power/p... chromeos/power/power_data_collector.cc:24: void PowerDataCollector::ShutdownInstance() { On 2013/12/11 22:26:47, Daniel Erat wrote: > (mentioned earlier: put this in the destructor instead) Done. https://chromiumcodereview.appspot.com/101963004/diff/100001/chromeos/power/p... chromeos/power/power_data_collector.cc:34: Release(); On 2013/12/11 22:26:47, Daniel Erat wrote: > delete this Done. https://chromiumcodereview.appspot.com/101963004/diff/100001/chromeos/power/p... chromeos/power/power_data_collector.cc:39: PowerSupplySnapshot snapshot = { On 2013/12/11 22:26:47, Daniel Erat wrote: > nit: this would be a bit clearer: > > PowerSupplySnapshot snapshot; > snapshot.time = base::TimeTicks::Now(); > etc. Done. https://chromiumcodereview.appspot.com/101963004/diff/100001/chromeos/power/p... chromeos/power/power_data_collector.cc:54: // Do no allow intializing twice. On 2013/12/11 22:26:47, Daniel Erat wrote: > nit: s/no/not/ > > (but i don't think that this comment is necessary; i'd probably just delete it > and remove the blank lines between the three statements in this method) Done. https://chromiumcodereview.appspot.com/101963004/diff/100001/chromeos/power/p... chromeos/power/power_data_collector.cc:60: void PowerDataCollector::InitializeTesting() { On 2013/12/11 22:26:47, Daniel Erat wrote: > remove this (see above) Done. https://chromiumcodereview.appspot.com/101963004/diff/100001/chromeos/power/p... chromeos/power/power_data_collector.cc:76: // NULL it to protect from improper usage after shutdown. On 2013/12/11 22:26:47, Daniel Erat wrote: > nit: delete unnecessary comments and blank lines here too Done. https://chromiumcodereview.appspot.com/101963004/diff/100001/chromeos/power/p... chromeos/power/power_data_collector.cc:85: return power_data_collector->power_supply_; On 2013/12/11 22:26:47, Daniel Erat wrote: > (see earlier comment about just making this be an inlined accessor, but note > that in general, you'd want to just return |power_supply_| here instead of > having an instance method return data from a global instance instead of from > itself) My bad. Fixed. https://chromiumcodereview.appspot.com/101963004/diff/100001/chromeos/power/p... File chromeos/power/power_data_collector.h (right): https://chromiumcodereview.appspot.com/101963004/diff/100001/chromeos/power/p... chromeos/power/power_data_collector.h:11: #include "base/synchronization/lock.h" On 2013/12/11 22:26:47, Daniel Erat wrote: > nit: remove this Done. I realize now that I did not remove a lot of unnecessary pieces. https://chromiumcodereview.appspot.com/101963004/diff/100001/chromeos/power/p... chromeos/power/power_data_collector.h:28: public base::RefCountedThreadSafe<PowerDataCollector> { On 2013/12/11 22:26:47, Daniel Erat wrote: > you don't need this, i don't think (as this will only be created, accessed, and > destroyed from the UI thread, and there won't be any references to it floating > around, just a global pointer) Done. https://chromiumcodereview.appspot.com/101963004/diff/100001/chromeos/power/p... chromeos/power/power_data_collector.h:30: struct PowerSupplySnapshot { On 2013/12/11 22:26:47, Daniel Erat wrote: > this creates a default c'tor that leaves |external_power| and |battery_charge| > uninitialized. please explicitly add a no-args c'tor that initializes those > fields to sane values Done. https://chromiumcodereview.appspot.com/101963004/diff/100001/chromeos/power/p... chromeos/power/power_data_collector.h:50: static PowerDataCollector *Get(); On 2013/12/11 22:26:47, Daniel Erat wrote: > nit: move '*' to left side of space to match the rest of chrome (i.e. > PowerDataCollector* Get()) Done. https://chromiumcodereview.appspot.com/101963004/diff/100001/chromeos/power/p... chromeos/power/power_data_collector.h:53: std::vector<PowerSupplySnapshot> &GetPowerSupplyData(void); On 2013/12/11 22:26:47, Daniel Erat wrote: > nit: make this be a const reference and move '&' to the left of the space > > note that you can just make it be an inlined accessor, though (i.e. rename the > member variable to |power_supply_snapshots_| and add an inlined const > std::vector<PowerSupplySnapshot>& power_supply_snapshots() const method) Done. https://chromiumcodereview.appspot.com/101963004/diff/100001/chromeos/power/p... chromeos/power/power_data_collector.h:65: virtual ~PowerDataCollector() {} On 2013/12/11 22:26:47, Daniel Erat wrote: > move the destructor implementation to the .cc file instead of leaving it in the > header Done. https://chromiumcodereview.appspot.com/101963004/diff/100001/chromeos/power/p... chromeos/power/power_data_collector.h:71: void ShutdownInstance(void); On 2013/12/11 22:26:47, Daniel Erat wrote: > get rid of this method and just do cleanup in the d'tor Done.
this generally looks okay to me now; just small comments this time https://chromiumcodereview.appspot.com/101963004/diff/120001/chrome/browser/c... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://chromiumcodereview.appspot.com/101963004/diff/120001/chrome/browser/c... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:344: PowerDataCollector::Shutdown(); (i'll let steven comment on whether there's a better place for this) https://chromiumcodereview.appspot.com/101963004/diff/120001/chrome/browser/c... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:409: PowerDataCollector::Initialize(); same here https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... File chromeos/power/power_data_collector.cc (right): https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector.cc:16: nit: i'd either remove this blank line or add another blank line after "namespace {" to match it https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector.cc:17: } // Anonymous namespace nit: s/Anonymous namespace/namespace/ https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector.cc:24: DBusThreadManager *dbus_manager = DBusThreadManager::Get(); nit: move '*' to left of space https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector.cc:32: snapshot.time = base::TimeTicks::Now(); you don't need it yet, but if/when you have tests that depend on the timestamps of the recorded data, you'll probably either want to replace this call with one to a class that lets you set the time for tests (i'm not sure if chrome has something like this already; probably) or just add a |now_for_testing_| member and a set_now_for_testing() setter that tests can use, and then change this code to something like: snapshot.time = now_for_testing_.null() ? base::TimeTicks::Now() : now_for_testing_; https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector.cc:48: PowerDataCollector *PowerDataCollector::Get() { nit: move '*' to left of space https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector.cc:57: // NULL it to protect from improper usage after shutdown. nit: remove unnecessary comment https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector.cc:62: : time(base::TimeTicks::Now()), external_power(false), battery_charge(0) { nit: one parameter per line in initialization list if they don't all fit on the same line as the c'tor signature (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr...) https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... File chromeos/power/power_data_collector.h (right): https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector.h:26: : public PowerManagerClient::Observer { nit: just move this up to the end of the previous line https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector.h:38: double battery_charge; nit: rename this to |battery_percent| as in PowerSupplyProperties to make it obvious that it's a percentage and not actually the charge (i.e. Ah) https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector.h:41: public: nit: remove this; there's already a public access label earlier https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector.h:52: // Returns a reference to a vector of power supply data. nit: remove unnecessary comment. accessors also usually appear near the top of the section, immediately after the c'tor and d'tor. since this class's c'tor and d'tor are private, i'd put it between the PowerSupplySnapshot definition and Initialize() https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector.h:53: const std::vector<PowerSupplySnapshot>& power_supply_data(void) const { remove void argument https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector.h:58: friend class base::RefCountedThreadSafe<PowerDataCollector>; remove this https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector.h:62: explicit PowerDataCollector(); remove 'explicit' https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector.h:68: static void InitializeTesting(); nit: remove this https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector.h:72: const power_manager::PowerSupplyProperties& prop) OVERRIDE; probably safest to include base/compiler_specific.h as well (that's where OVERRIDE is defined, iirc) https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector.h:77: DISALLOW_COPY_AND_ASSIGN(PowerDataCollector); i'd include base/basictypes.h as well for this macro https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... File chromeos/power/power_data_collector_unittest.cc (right): https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector_unittest.cc:9: #include "testing/gmock/include/gmock/gmock.h" don't think you need this include https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector_unittest.cc:32: PowerDataCollector* power_data_collector = PowerDataCollector::Get(); nit: to reduce duplicated code when you add additional tests later, i'd just add a protected |collector_| member to PowerDataCollectorTest() and initialize it in SetUp() after calling PowerDataCollector::Initialize() https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector_unittest.cc:41: EXPECT_EQ(1, data_size); nit: do: ASSERT_EQ(static_cast<size_t>(1), data1.size()); instead so you won't e.g. segfault later if data1 is empty https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector_unittest.cc:42: EXPECT_EQ(20.00, data1[0].battery_charge); nit: use EXPECT_DOUBLE_EQ() when comparing floating-point values, and also just do: EXPECT_DOUBLE_EQ(prop1.battery_percent(), data1[0].battery_charge); instead of duplicating the literal from above https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector_unittest.cc:51: data_size = data2.size(); (comments from above apply here too)
Addressed all nits. PTAL at patch set 8. https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... File chromeos/power/power_data_collector.cc (right): https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector.cc:16: On 2013/12/12 01:31:52, Daniel Erat wrote: > nit: i'd either remove this blank line or add another blank line after > "namespace {" to match it Done. https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector.cc:17: } // Anonymous namespace On 2013/12/12 01:31:52, Daniel Erat wrote: > nit: s/Anonymous namespace/namespace/ Done. https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector.cc:24: DBusThreadManager *dbus_manager = DBusThreadManager::Get(); On 2013/12/12 01:31:52, Daniel Erat wrote: > nit: move '*' to left of space Done. https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector.cc:32: snapshot.time = base::TimeTicks::Now(); On 2013/12/12 01:31:52, Daniel Erat wrote: > you don't need it yet, but if/when you have tests that depend on the timestamps > of the recorded data, you'll probably either want to replace this call with one > to a class that lets you set the time for tests (i'm not sure if chrome has > something like this already; probably) or just add a |now_for_testing_| member > and a set_now_for_testing() setter that tests can use, and then change this code > to something like: > > snapshot.time = now_for_testing_.null() ? > base::TimeTicks::Now() : > now_for_testing_; Thanks. Will keep this in mind. Your and Steven's comments used |<var name>| when commenting. What does it mean? https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector.cc:48: PowerDataCollector *PowerDataCollector::Get() { On 2013/12/12 01:31:52, Daniel Erat wrote: > nit: move '*' to left of space Done. https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector.cc:62: : time(base::TimeTicks::Now()), external_power(false), battery_charge(0) { On 2013/12/12 01:31:52, Daniel Erat wrote: > nit: one parameter per line in initialization list if they don't all fit on the > same line as the c'tor signature > (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr...) Done. https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... File chromeos/power/power_data_collector.h (right): https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector.h:26: : public PowerManagerClient::Observer { On 2013/12/12 01:31:52, Daniel Erat wrote: > nit: just move this up to the end of the previous line Done. https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector.h:38: double battery_charge; On 2013/12/12 01:31:52, Daniel Erat wrote: > nit: rename this to |battery_percent| as in PowerSupplyProperties to make it > obvious that it's a percentage and not actually the charge (i.e. Ah) Done. https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector.h:41: public: On 2013/12/12 01:31:52, Daniel Erat wrote: > nit: remove this; there's already a public access label earlier Done. https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector.h:52: // Returns a reference to a vector of power supply data. On 2013/12/12 01:31:52, Daniel Erat wrote: > nit: remove unnecessary comment. accessors also usually appear near the top of > the section, immediately after the c'tor and d'tor. since this class's c'tor and > d'tor are private, i'd put it between the PowerSupplySnapshot definition and > Initialize() Done. https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector.h:53: const std::vector<PowerSupplySnapshot>& power_supply_data(void) const { On 2013/12/12 01:31:52, Daniel Erat wrote: > remove void argument Done. https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector.h:58: friend class base::RefCountedThreadSafe<PowerDataCollector>; On 2013/12/12 01:31:52, Daniel Erat wrote: > remove this Done. https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector.h:62: explicit PowerDataCollector(); On 2013/12/12 01:31:52, Daniel Erat wrote: > remove 'explicit' Done. https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector.h:68: static void InitializeTesting(); On 2013/12/12 01:31:52, Daniel Erat wrote: > nit: remove this Done. https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector.h:72: const power_manager::PowerSupplyProperties& prop) OVERRIDE; On 2013/12/12 01:31:52, Daniel Erat wrote: > probably safest to include base/compiler_specific.h as well (that's where > OVERRIDE is defined, iirc) Done. https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector.h:77: DISALLOW_COPY_AND_ASSIGN(PowerDataCollector); On 2013/12/12 01:31:52, Daniel Erat wrote: > i'd include base/basictypes.h as well for this macro Done. https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector.h:77: DISALLOW_COPY_AND_ASSIGN(PowerDataCollector); On 2013/12/12 01:31:52, Daniel Erat wrote: > i'd include base/basictypes.h as well for this macro Done. https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... File chromeos/power/power_data_collector_unittest.cc (right): https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector_unittest.cc:9: #include "testing/gmock/include/gmock/gmock.h" On 2013/12/12 01:31:52, Daniel Erat wrote: > don't think you need this include Done. https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector_unittest.cc:32: PowerDataCollector* power_data_collector = PowerDataCollector::Get(); On 2013/12/12 01:31:52, Daniel Erat wrote: > nit: to reduce duplicated code when you add additional tests later, i'd just add > a protected |collector_| member to PowerDataCollectorTest() and initialize it in > SetUp() after calling PowerDataCollector::Initialize() Done. https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector_unittest.cc:41: EXPECT_EQ(1, data_size); On 2013/12/12 01:31:52, Daniel Erat wrote: > nit: do: > > ASSERT_EQ(static_cast<size_t>(1), data1.size()); > > instead so you won't e.g. segfault later if data1 is empty Done. https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector_unittest.cc:42: EXPECT_EQ(20.00, data1[0].battery_charge); On 2013/12/12 01:31:52, Daniel Erat wrote: > nit: use EXPECT_DOUBLE_EQ() when comparing floating-point values, and also just > do: > > EXPECT_DOUBLE_EQ(prop1.battery_percent(), data1[0].battery_charge); > > instead of duplicating the literal from above Done. https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector_unittest.cc:51: data_size = data2.size(); On 2013/12/12 01:31:52, Daniel Erat wrote: > (comments from above apply here too) Done.
https://chromiumcodereview.appspot.com/101963004/diff/40001/chrome/browser/ch... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://chromiumcodereview.appspot.com/101963004/diff/40001/chrome/browser/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:409: PowerDataCollector::Initialize(); On 2013/12/11 21:30:56, Siva Chandra wrote: > On 2013/12/11 20:54:37, stevenjb wrote: > > This should be initialized and shut down as part of DBusServices. (Which would > > maybe better be named "DBusRelatedServices"). > > > > While I do not have any thing against it, I have a point to make. Only a part of > PowerDataCollector listens to PowerManagerClient. Should this warrant us to > treat as it as a DBusRelatedService? Yes. The class is dependent on DBusManager, it should be nitialized and shut down as part of DBusServices. It makes it easier to keep track of, and ensures that DBusManager won't be destroyed before PowerDataCollector is. https://chromiumcodereview.appspot.com/101963004/diff/140001/chromeos/power/p... File chromeos/power/power_data_collector.cc (right): https://chromiumcodereview.appspot.com/101963004/diff/140001/chromeos/power/p... chromeos/power/power_data_collector.cc:16: PowerDataCollector* power_data_collector = NULL; nit: globals (even in an anon namespace) are generally prefixed with g_. https://chromiumcodereview.appspot.com/101963004/diff/140001/chromeos/power/p... File chromeos/power/power_data_collector.h (right): https://chromiumcodereview.appspot.com/101963004/diff/140001/chromeos/power/p... chromeos/power/power_data_collector.h:32: // Time the snapshot was captured. nit: Time when, or Time at which https://chromiumcodereview.appspot.com/101963004/diff/140001/chromeos/power/p... File chromeos/power/power_data_collector_unittest.cc (right): https://chromiumcodereview.appspot.com/101963004/diff/140001/chromeos/power/p... chromeos/power/power_data_collector_unittest.cc:15: PowerDataCollectorTest() {} : power_data_collector_(NULL) https://chromiumcodereview.appspot.com/101963004/diff/140001/chromeos/power/p... chromeos/power/power_data_collector_unittest.cc:26: DBusThreadManager::Shutdown(); power_data_collector_ = NULL; (With only one test this doesn't really matter, but still good practice / future-proofing).
PTAL at patch set 9. https://chromiumcodereview.appspot.com/101963004/diff/40001/chrome/browser/ch... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://chromiumcodereview.appspot.com/101963004/diff/40001/chrome/browser/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:409: PowerDataCollector::Initialize(); On 2013/12/13 22:28:45, stevenjb wrote: > On 2013/12/11 21:30:56, Siva Chandra wrote: > > On 2013/12/11 20:54:37, stevenjb wrote: > > > This should be initialized and shut down as part of DBusServices. (Which > would > > > maybe better be named "DBusRelatedServices"). > > > > > > > While I do not have any thing against it, I have a point to make. Only a part > of > > PowerDataCollector listens to PowerManagerClient. Should this warrant us to > > treat as it as a DBusRelatedService? > > Yes. The class is dependent on DBusManager, it should be nitialized and shut > down as part of DBusServices. It makes it easier to keep track of, and ensures > that DBusManager won't be destroyed before PowerDataCollector is. Done. https://chromiumcodereview.appspot.com/101963004/diff/140001/chromeos/power/p... File chromeos/power/power_data_collector.cc (right): https://chromiumcodereview.appspot.com/101963004/diff/140001/chromeos/power/p... chromeos/power/power_data_collector.cc:16: PowerDataCollector* power_data_collector = NULL; On 2013/12/13 22:28:45, stevenjb wrote: > nit: globals (even in an anon namespace) are generally prefixed with g_. Done. https://chromiumcodereview.appspot.com/101963004/diff/140001/chromeos/power/p... File chromeos/power/power_data_collector.h (right): https://chromiumcodereview.appspot.com/101963004/diff/140001/chromeos/power/p... chromeos/power/power_data_collector.h:32: // Time the snapshot was captured. On 2013/12/13 22:28:45, stevenjb wrote: > nit: Time when, or Time at which > Done. https://chromiumcodereview.appspot.com/101963004/diff/140001/chromeos/power/p... File chromeos/power/power_data_collector_unittest.cc (right): https://chromiumcodereview.appspot.com/101963004/diff/140001/chromeos/power/p... chromeos/power/power_data_collector_unittest.cc:15: PowerDataCollectorTest() {} On 2013/12/13 22:28:45, stevenjb wrote: > : power_data_collector_(NULL) Done. https://chromiumcodereview.appspot.com/101963004/diff/140001/chromeos/power/p... chromeos/power/power_data_collector_unittest.cc:26: DBusThreadManager::Shutdown(); On 2013/12/13 22:28:45, stevenjb wrote: > power_data_collector_ = NULL; > (With only one test this doesn't really matter, but still good practice / > future-proofing). Done.
LGTM
https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... File chromeos/power/power_data_collector.cc (right): https://chromiumcodereview.appspot.com/101963004/diff/120001/chromeos/power/p... chromeos/power/power_data_collector.cc:32: snapshot.time = base::TimeTicks::Now(); On 2013/12/13 22:10:39, Siva Chandra wrote: > On 2013/12/12 01:31:52, Daniel Erat wrote: > > you don't need it yet, but if/when you have tests that depend on the > timestamps > > of the recorded data, you'll probably either want to replace this call with > one > > to a class that lets you set the time for tests (i'm not sure if chrome has > > something like this already; probably) or just add a |now_for_testing_| member > > and a set_now_for_testing() setter that tests can use, and then change this > code > > to something like: > > > > snapshot.time = now_for_testing_.null() ? > > base::TimeTicks::Now() : > > now_for_testing_; > > Thanks. Will keep this in mind. Your and Steven's comments used |<var name>| > when commenting. What does it mean? this is the style usually used in chrome for referring to variable names within comments. it makes them easier to understand when the variable name is a regular word, e.g. "Toggles |active|." rather than "Toggles active." https://chromiumcodereview.appspot.com/101963004/diff/150001/chromeos/power/p... File chromeos/power/power_data_collector.cc (right): https://chromiumcodereview.appspot.com/101963004/diff/150001/chromeos/power/p... chromeos/power/power_data_collector.cc:20: PowerDataCollector::PowerDataCollector() { please reorder the methods in the .cc file to match the order in the header https://chromiumcodereview.appspot.com/101963004/diff/150001/chromeos/power/p... chromeos/power/power_data_collector.cc:41: void PowerDataCollector::Initialize() { nit: add a "// static" comment on the line above each static method (this is common in chrome) https://chromiumcodereview.appspot.com/101963004/diff/150001/chromeos/power/p... File chromeos/power/power_data_collector.h (right): https://chromiumcodereview.appspot.com/101963004/diff/150001/chromeos/power/p... chromeos/power/power_data_collector.h:65: virtual void PowerChanged( if you make this public, then you can get rid of the friend stuff above and remove the gtest_prod_util.h include, right? https://chromiumcodereview.appspot.com/101963004/diff/150001/chromeos/power/p... chromeos/power/power_data_collector.h:68: private: remove this extra 'private' label
PTAL https://chromiumcodereview.appspot.com/101963004/diff/150001/chromeos/power/p... File chromeos/power/power_data_collector.cc (right): https://chromiumcodereview.appspot.com/101963004/diff/150001/chromeos/power/p... chromeos/power/power_data_collector.cc:20: PowerDataCollector::PowerDataCollector() { On 2013/12/13 23:57:09, Daniel Erat wrote: > please reorder the methods in the .cc file to match the order in the header Done. https://chromiumcodereview.appspot.com/101963004/diff/150001/chromeos/power/p... chromeos/power/power_data_collector.cc:41: void PowerDataCollector::Initialize() { On 2013/12/13 23:57:09, Daniel Erat wrote: > nit: add a "// static" comment on the line above each static method (this is > common in chrome) Done. https://chromiumcodereview.appspot.com/101963004/diff/150001/chromeos/power/p... File chromeos/power/power_data_collector.h (right): https://chromiumcodereview.appspot.com/101963004/diff/150001/chromeos/power/p... chromeos/power/power_data_collector.h:65: virtual void PowerChanged( On 2013/12/13 23:57:09, Daniel Erat wrote: > if you make this public, then you can get rid of the friend stuff above and > remove the gtest_prod_util.h include, right? Yes. But are you telling me to remove? I felt it appropriate to keep the overriden method to be private as it is not supposed to be called by the "clients". https://chromiumcodereview.appspot.com/101963004/diff/150001/chromeos/power/p... chromeos/power/power_data_collector.h:68: private: On 2013/12/13 23:57:09, Daniel Erat wrote: > remove this extra 'private' label Done.
https://chromiumcodereview.appspot.com/101963004/diff/150001/chromeos/power/p... File chromeos/power/power_data_collector.h (right): https://chromiumcodereview.appspot.com/101963004/diff/150001/chromeos/power/p... chromeos/power/power_data_collector.h:65: virtual void PowerChanged( On 2013/12/14 00:35:50, Siva Chandra wrote: > On 2013/12/13 23:57:09, Daniel Erat wrote: > > if you make this public, then you can get rid of the friend stuff above and > > remove the gtest_prod_util.h include, right? > > Yes. But are you telling me to remove? > > I felt it appropriate to keep the overriden method to be private as it is not > supposed to be called by the "clients". your call. in general, i've seen FRIEND_TEST_ALL_PREFIXES() often lead to a lot of duplicated testing code that touches a class's internals, making it painful to change the implementation later. i think that the risk of anyone thinking it's a good idea to call a public PowerChanged() method from anywhere besides PowerManagerClient is awfully slim. https://chromiumcodereview.appspot.com/101963004/diff/170001/chromeos/power/p... File chromeos/power/power_data_collector_unittest.cc (right): https://chromiumcodereview.appspot.com/101963004/diff/170001/chromeos/power/p... chromeos/power/power_data_collector_unittest.cc:31: PowerDataCollector *power_data_collector_; nit: move '*' to left of space https://chromiumcodereview.appspot.com/101963004/diff/170001/chromeos/power/p... chromeos/power/power_data_collector_unittest.cc:43: unsigned int data_size = data1.size(); think i had a previous comment about how |data_size| seems unnecessary; just compare against data1.size() on the next line https://chromiumcodereview.appspot.com/101963004/diff/170001/chromeos/power/p... chromeos/power/power_data_collector_unittest.cc:44: EXPECT_EQ(static_cast<size_t>(1), data_size); i had a previous comment about this too: use ASSERT_EQ() here instead so the test doesn't segfault if data1 is empty https://chromiumcodereview.appspot.com/101963004/diff/170001/chromeos/power/p... chromeos/power/power_data_collector_unittest.cc:54: data_size = data2.size(); remove this too https://chromiumcodereview.appspot.com/101963004/diff/170001/chromeos/power/p... chromeos/power/power_data_collector_unittest.cc:55: EXPECT_EQ(static_cast<size_t>(2), data_size); ASSERT_EQ()
Hope I addressed all this time. PTAL. https://chromiumcodereview.appspot.com/101963004/diff/170001/chromeos/power/p... File chromeos/power/power_data_collector_unittest.cc (right): https://chromiumcodereview.appspot.com/101963004/diff/170001/chromeos/power/p... chromeos/power/power_data_collector_unittest.cc:31: PowerDataCollector *power_data_collector_; On 2013/12/14 01:00:31, Daniel Erat wrote: > nit: move '*' to left of space Done. https://chromiumcodereview.appspot.com/101963004/diff/170001/chromeos/power/p... chromeos/power/power_data_collector_unittest.cc:43: unsigned int data_size = data1.size(); On 2013/12/14 01:00:31, Daniel Erat wrote: > think i had a previous comment about how |data_size| seems unnecessary; just > compare against data1.size() on the next line Done. https://chromiumcodereview.appspot.com/101963004/diff/170001/chromeos/power/p... chromeos/power/power_data_collector_unittest.cc:44: EXPECT_EQ(static_cast<size_t>(1), data_size); On 2013/12/14 01:00:31, Daniel Erat wrote: > i had a previous comment about this too: use ASSERT_EQ() here instead so the > test doesn't segfault if data1 is empty Done. https://chromiumcodereview.appspot.com/101963004/diff/170001/chromeos/power/p... chromeos/power/power_data_collector_unittest.cc:54: data_size = data2.size(); On 2013/12/14 01:00:31, Daniel Erat wrote: > remove this too Done. https://chromiumcodereview.appspot.com/101963004/diff/170001/chromeos/power/p... chromeos/power/power_data_collector_unittest.cc:55: EXPECT_EQ(static_cast<size_t>(2), data_size); On 2013/12/14 01:00:31, Daniel Erat wrote: > ASSERT_EQ() Done.
lgtm https://chromiumcodereview.appspot.com/101963004/diff/190001/chromeos/power/p... File chromeos/power/power_data_collector.h (right): https://chromiumcodereview.appspot.com/101963004/diff/190001/chromeos/power/p... chromeos/power/power_data_collector.h:12: #include "base/gtest_prod_util.h" nit: i think you don't need this now
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sivachandra@chromium.org/101963004/210001
Checked the commit box after fixing the nit. https://chromiumcodereview.appspot.com/101963004/diff/190001/chromeos/power/p... File chromeos/power/power_data_collector.h (right): https://chromiumcodereview.appspot.com/101963004/diff/190001/chromeos/power/p... chromeos/power/power_data_collector.h:12: #include "base/gtest_prod_util.h" On 2013/12/14 14:29:10, Daniel Erat wrote: > nit: i think you don't need this now Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sivachandra@chromium.org/101963004/230001
Retried try job too often on linux_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sivachandra@chromium.org/101963004/350001
Message was sent while issue was closed.
Change committed as 241445 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
