Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
560553005:
Battery impact UMA (Closed)
|
Created:
6 years, 3 months ago by jeremy Modified:
6 years, 1 month ago CC:
chromium-reviews, Ilya Sherman, asvitkine+watch_chromium.org, tonyg, sullivan, oysteine, sadrul, timvolodine, qsr, blundell Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionReport UMA stats on battery discharge rate when Chrome is running.
Stats reported:
* Battery depletion percent over 5, 15 and 30 minute period.
* Battery discharge rate if battery was discharged for more than 30 minutes.
Reports are only generated after 30 minutes of uptime so as not to be affected by increased power draw at system startup.
In-progress stat collection is cancelled on sleep or when all renderers are closed.
BUG=418407
Committed: https://crrev.com/76a267615a7bc5fcb76f3a6c425c57eb99b66745
Cr-Commit-Position: refs/heads/master@{#304412}
Patch Set 1 #Patch Set 2 : Move to content/ #
Total comments: 6
Patch Set 3 : Fix review comments #
Total comments: 12
Patch Set 4 : WIP #Patch Set 5 : Now with unit tests #
Total comments: 57
Patch Set 6 : Fix review comments #Patch Set 7 : Minor style fixes. #
Total comments: 40
Patch Set 8 : Fix review comments #
Total comments: 35
Patch Set 9 : Fix review comments + Rebase #Patch Set 10 : Fix review comments #Patch Set 11 : Remove rate limiting + Move everything to main thread. #
Total comments: 32
Patch Set 12 : Fix review comments #Patch Set 13 : Additional style fixes #
Total comments: 13
Patch Set 14 : Fix review comments #Patch Set 15 : Remove debug code #
Total comments: 6
Patch Set 16 : Rate -> PercentPerHour #
Total comments: 8
Patch Set 17 : Fix review comments #
Total comments: 4
Patch Set 18 : Fix histograms.xml #Patch Set 19 : Fix unrelated change that snuck in. #Patch Set 20 : Pach for landing #Patch Set 21 : Fix component build. #
Total comments: 8
Patch Set 22 : Fix review comments #Messages
Total messages: 63 (11 generated)
jeremy@chromium.org changed reviewers: + sullivan@chromium.org, tonyg@chromium.org
Not ready yet, but would be grateful for high level feedback. The main piece that's missing is integration with ProcessPowerCollector.
Something else I've been thinking about - what should we do in the face of a device that goes to sleep? IMHO cancel the current measurement if the device wasn't awake for the entire duration of the measurement. Do you think this behavior makes sense for Android as well?
On 2014/09/15 08:08:55, jeremy wrote: > Something else I've been thinking about - what should we do in the face of a > device that goes to sleep? IMHO cancel the current measurement if the device > wasn't awake for the entire duration of the measurement. I agree we should cancel if Chrome wasn't awake the entire duration of the measurement. Along the same lines, we don't track whether Chrome is in the background or the foreground. Should we? > Do you think this behavior makes sense for Android as well? What would be different about Android? One thing I can think of is that sessions on Android are shorter. Should we have a smaller reporting time for Android?
tonyg@chromium.org changed reviewers: + oysteine@google.com
On 2014/09/15 16:26:58, sullivan wrote: > On 2014/09/15 08:08:55, jeremy wrote: > > Something else I've been thinking about - what should we do in the face of a > > device that goes to sleep? IMHO cancel the current measurement if the device > > wasn't awake for the entire duration of the measurement. > > I agree we should cancel if Chrome wasn't awake the entire duration of the > measurement. > > Along the same lines, we don't track whether Chrome is in the background or the > foreground. Should we? > > > Do you think this behavior makes sense for Android as well? > > What would be different about Android? > > One thing I can think of is that sessions on Android are shorter. Should we have > a smaller reporting time for Android? +1 to all of Annie's comments. The only other question on my mind is why you created a new monitor instead of piggy backing on the PerformanceMonitor? Fewer monitors means fewer wakeups because they can use the same timers to report their UMAs. It also helps centralize the collection of these performance related UMAs so that authors modifying them can consider them as a whole.
jeremy@chromium.org changed reviewers: + avi@chromium.org, derat@chromium.org - oysteine@google.com, sullivan@chromium.org, tonyg@chromium.org
derat/avi: Can you review this please? This still needs polish and tests but I'd really appreciate high level feedback at this point. Known missing pieces: * Polish and comments. * Unit tests. * Don't report stats if no renderers open initially.
(i just looked at a little of it before having the following question.) is there a bug or design doc for this? on chrome os, we already report similar metrics from the power manager daemon. also note that base::PowerMonitor doesn't work on chrome os: http://crbug.com/326534. (i'm not sure of it but assume that that's where the data is coming from here...) https://codereview.chromium.org/560553005/diff/20001/chrome/browser/chrome_br... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/560553005/diff/20001/chrome/browser/chrome_br... chrome/browser/chrome_browser_main.cc:1563: if (metrics->reporting_active()) nit: inline the g_browser_process->metrics_service() call here to save a line? https://codereview.chromium.org/560553005/diff/20001/content/browser/power_us... File content/browser/power_usage_monitor_impl.h (right): https://codereview.chromium.org/560553005/diff/20001/content/browser/power_us... content/browser/power_usage_monitor_impl.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. nit: we don't use (c) on new files https://codereview.chromium.org/560553005/diff/20001/content/browser/power_us... content/browser/power_usage_monitor_impl.h:112: base::hash_map<int, int> live_renderers_; any particular reason to use base::hash_map here instead of std::map? this is probably small, right?
Thanks for looking Daniel, I've filed a tracking bug, a detailed explanation is in the header comment in power_usage_monitor_impl.h . Happy to expand that based on any questions you may have. I looked at the power UMAs on CrOS, the closest thing there seems to be Power.BatteryDischargeRate (Sorry, but I can't seem to find where it's recorded - can you please send me a link). The UMAs I'm adding here are aimed at being cross platform and at measuring in cases where Chrome is doing work (they aren't recorded when there are no live renderers). The battery data comes from the BatteryStatusService and sleep/wake notifications come from base::PowerMonitor. https://codereview.chromium.org/560553005/diff/20001/chrome/browser/chrome_br... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/560553005/diff/20001/chrome/browser/chrome_br... chrome/browser/chrome_browser_main.cc:1563: if (metrics->reporting_active()) On 2014/09/22 16:10:20, Daniel Erat wrote: > nit: inline the g_browser_process->metrics_service() call here to save a line? Done. https://codereview.chromium.org/560553005/diff/20001/content/browser/power_us... File content/browser/power_usage_monitor_impl.h (right): https://codereview.chromium.org/560553005/diff/20001/content/browser/power_us... content/browser/power_usage_monitor_impl.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/09/22 16:10:20, Daniel Erat wrote: > nit: we don't use (c) on new files Done. https://codereview.chromium.org/560553005/diff/20001/content/browser/power_us... content/browser/power_usage_monitor_impl.h:112: base::hash_map<int, int> live_renderers_; I've changed this to base::hash_set since that's all I really need. Happy to change to std::map or whatever you think is best.
still just a partial review, but here are some more comments https://codereview.chromium.org/560553005/diff/40001/content/browser/power_us... File content/browser/power_usage_monitor_impl.cc (right): https://codereview.chromium.org/560553005/diff/40001/content/browser/power_us... content/browser/power_usage_monitor_impl.cc:59: PowerUsageMonitor::PowerUsageMonitor() : fix style here (please read the chromium and google c++ style guides) https://codereview.chromium.org/560553005/diff/40001/content/browser/power_us... content/browser/power_usage_monitor_impl.cc:134: BrowserThread::PostDelayedTask(BrowserThread::IO, can you move this into HistogramRecorder so it doesn't need to expose its weak pointers publicly? https://codereview.chromium.org/560553005/diff/40001/content/browser/power_us... content/browser/power_usage_monitor_impl.cc:151: if (discharge_time.InMinutes() > 30) need curly brackets here https://codereview.chromium.org/560553005/diff/40001/content/browser/power_us... content/browser/power_usage_monitor_impl.cc:195: base::Bind( fix weird indenting -- i recommend clang-format https://codereview.chromium.org/560553005/diff/40001/content/browser/power_us... content/browser/power_usage_monitor_impl.cc:210: if(type == NOTIFICATION_RENDERER_PROCESS_CREATED) { nit: add a space between 'if' and '(' here and everywhere else https://codereview.chromium.org/560553005/diff/40001/content/browser/power_us... content/browser/power_usage_monitor_impl.cc:220: FROM_HERE, fix indenting https://codereview.chromium.org/560553005/diff/40001/content/browser/power_us... File content/browser/power_usage_monitor_impl.h (right): https://codereview.chromium.org/560553005/diff/40001/content/browser/power_us... content/browser/power_usage_monitor_impl.h:36: class PowerUsageMonitor : base::PowerObserver, NotificationObserver { use public inheritance https://codereview.chromium.org/560553005/diff/40001/content/browser/power_us... content/browser/power_usage_monitor_impl.h:43: virtual void ReportBatteryLevelHistogram(base::TimeDelta discharge_time); why virtual? are you planning to create a derived version of this class? if so, a pure abstract class would probably be cleaner. (if you're planning to use gmock, i'd recommend against it. it's discouraged in chromium, particularly for concrete classes -- see e.g. https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/-KH_IP0rIWQ .) https://codereview.chromium.org/560553005/diff/40001/content/browser/power_us... content/browser/power_usage_monitor_impl.h:45: void CancelPendingHistgramReports(); add blank line after this https://codereview.chromium.org/560553005/diff/40001/content/browser/power_us... content/browser/power_usage_monitor_impl.h:50: // Used to cancel in progress timers. this should be private https://codereview.chromium.org/560553005/diff/40001/content/browser/power_us... content/browser/power_usage_monitor_impl.h:73: // POverridden from basde::PowerObserver: fix typos on this line https://codereview.chromium.org/560553005/diff/40001/content/browser/power_us... content/browser/power_usage_monitor_impl.h:112: base::hash_set<int> live_renderers_; hash_set is probably fine if you don't plan to ever iterate through this, but if there's a chance that you (or someone else) will do that later, std::set is probably a better choice. please also rename this to |live_renderer_ids_| so it's clearer what it holds.
All review comments fixed, ready for a thorough review, thanks!
On 2014/10/21 14:55:14, jeremy wrote: > All review comments fixed, ready for a thorough review, thanks! I see Tony already asked about why this doesn't piggyback on the existing performance_monitor; maybe that was answered off-review, but I'm curious as well :).
https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... File content/browser/power_usage_monitor_impl.cc (right): https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:115: // Called on IO thread. DCHECK it then. https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:140: base::TimeDelta::FromMinutes(kBatteryReportingIntervalMinutes[i]); for (auto reporting_interval : kBatteryReportingIntervalMinutes) { base::TimeDelta delay = base::TimeDelta::FromMinutes(reporting_interval); // ... https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... File content/browser/power_usage_monitor_impl.h (right): https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:35: // Threading: Most methods are called on the IO thread. This is not a helpful comment. Don't use descriptive language (e.g. "this is called on this thread") in an API. Use prescriptive language (e.g. "call this function on this thread"). Something like "Methods must be called on the IO thread unless otherwise noted" would be helpful. And then you'd have to mark the methods that require other thread uses. https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:86: virtual void OnSuspend() override; The new style is to drop the "virtual" and only have "override". I hope that at this point the clang plugin is updated to allow this. https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:117: size_t num_live_renderers_; It bothers me that we have num_live_renderers_ as a separate variable to live_renderer_ids_ even though in normal use it must always be equal to live_renderer_ids_.size(). Is there a way to remove this redundant variable?
i didn't look at the tests yet. https://codereview.chromium.org/560553005/diff/100001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/560553005/diff/100001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1558: content::StartPowerUsageMonitor(); is this the proper way to check whether metrics are being reported? i only see reporting_active() called in one place, SafeBrowsingUIManager::CanReportStats(), and it also checks that metrics_service() is non-NULL. is that check incorrect, or does this code also need it? can reporting be toggled on or off at runtime? https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... File content/browser/power_usage_monitor_impl.cc (right): https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:25: static PowerUsageMonitor* monitor_ = new PowerUsageMonitor(); why static? do you need to annotate this as being leaky? https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:30: : power_usage_monitor_(nullptr), weak_ptr_factory_(this) { nit: one per line since they don't all fit on the same line as the signature https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:81: if (base::PowerMonitor::Get()) why are you checking this here but not in Start()? what's the lifetime of this class relative to PowerMonitor? https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:87: base::PowerMonitor::Get()->AddObserver(this); if a PowerUsageMonitor gets constructed and destroyed without Start() being called, you have an unbalanced call to RemoveObserver(). please keep track of whether the object was initialized so the calls are balanced. https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:100: const base::TimeDelta delay = base::TimeDelta::FromSeconds(1); what's the significance of this 1-second delay? also, please just inline this in the PostDelayedTask() call below; it's not used anywhere else. https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:111: subscription_ = BatteryStatusService::GetInstance()->AddCallback(callback_); do you need to remove the callback in e.g. the d'tor? https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:128: first_histogram_report_timestamp_ = base::Time(); why isn't this using system_interface_->Now()? https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:141: system_interface_->ScheduleHistogramReport(delay); scheduling multiple future reports like this seems odd; it also means there are a bunch of tasks floating around with invalidated weak pointers whenever the power source changes. at the very least, couldn't this loop live in SystemInterface instead of PowerUsageMonitor? https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:146: // Called on IO thread. assert this instead of commenting it https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:170: // Called on IO thread. assert this instead of commenting it https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:173: double battery_level = status.level; what are the units of this level? WebBatteryStatus.h doesn't have any comments. if it's not uniform across all systems, i don't see how these metrics can be interpreted in a meaningful way. https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:180: if (num_live_renderers_ > 0) what's the significance of this check? when are there 0 live renderers? https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:204: void PowerUsageMonitor::OnSuspend() { is this guaranteed to get run before the system suspends? iirc, it isn't. will this cause problems? https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... File content/browser/power_usage_monitor_impl.h (right): https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:36: class PowerUsageMonitor : public base::PowerObserver, NotificationObserver { add 'public' to NotificationObserver too (not sure whether it'll go back to private by default, but better to be explicit) https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:43: void SetPowerUsageMonitor(PowerUsageMonitor* monitor) { set_power_usage_monitor https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:47: virtual void ScheduleHistogramReport(base::TimeDelta delay); make this class be a pure interface, please https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:48: virtual void CancelPendingHistgramReports(); s/Histgram/Histogram/ https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:57: PowerUsageMonitor* power_usage_monitor_; document that this isn't owned https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:80: void SetSystemInterfaceForTest(scoped_ptr<SystemInterface> recorder); s/recorder/interface/ https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:107: bool was_on_battery_power_; add a comment describing what this represents -- when was it on battery power? https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:108: double initial_battery_level_; initial when -- when the object was first created? https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:117: size_t num_live_renderers_; On 2014/10/21 15:12:50, Avi wrote: > It bothers me that we have num_live_renderers_ as a separate variable to > live_renderer_ids_ even though in normal use it must always be equal to > live_renderer_ids_.size(). Is there a way to remove this redundant variable? agreed; please remove this. https://codereview.chromium.org/560553005/diff/100001/content/public/browser/... File content/public/browser/power_usage_monitor.h (right): https://codereview.chromium.org/560553005/diff/100001/content/public/browser/... content/public/browser/power_usage_monitor.h:13: void CONTENT_EXPORT StartPowerUsageMonitor(); nit: "CONTENT_EXPORT void" is overwhelmingly more common
On 2014/10/21 15:09:05, Oystein wrote: > On 2014/10/21 14:55:14, jeremy wrote: > > All review comments fixed, ready for a thorough review, thanks! > > I see Tony already asked about why this doesn't piggyback on the existing > performance_monitor; maybe that was answered off-review, but I'm curious as well > :). Sorry, Tony and I discussed f2f - Unfortunately I don' think it's a good fit - performance_monitor monitors counters for separate processes, the code here tracks the battery level for the system as a whole. Other than in name, this CL is quite disjoin from what PerformanceMonitor does - listening on a bunch of system signals, etc.
Thanks for the quick review in the previous round! Everything should now be fixed, ready for another look. https://codereview.chromium.org/560553005/diff/100001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/560553005/diff/100001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1558: content::StartPowerUsageMonitor(); Avi: Do you know whether we need to null check metrics_service()? Regarding the null check: The check in SafeBrowsingUIManager::CanReportStats() was added in https://codereview.chromium.org/6023006/. Looking at the code it seems that in some cases we null check metrics_service() and in some cases we don't e.g. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... . Regarding the call being correct: When I looked into this my impression was that this was the supported way to do this. The reason I added this is because If stats reporting is disabled then there's no reason to instantiate the powermanager and if it's toggled at runtime then the UMAs we report just won't get sent. I don't think it's worth the added complexity of listening on the pref change and shutting down/starting the powermanager. https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... File content/browser/power_usage_monitor_impl.cc (right): https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:25: static PowerUsageMonitor* monitor_ = new PowerUsageMonitor(); I originally had this as a singleton but I wanted this to be testable. I've marked this as leaky. https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:30: : power_usage_monitor_(nullptr), weak_ptr_factory_(this) { On 2014/10/21 16:01:21, Daniel Erat wrote: > nit: one per line since they don't all fit on the same line as the signature Done. https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:81: if (base::PowerMonitor::Get()) PowerMonitor is started on Chrome startup and hangs around until exit, as mentioned above I originally implemented it as a singleton but then made changes so I could instantiate it freely in unit tests. When executed from a unit tests Start() is not called. https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:87: base::PowerMonitor::Get()->AddObserver(this); On 2014/10/21 16:01:21, Daniel Erat wrote: > if a PowerUsageMonitor gets constructed and destroyed without Start() being > called, you have an unbalanced call to RemoveObserver(). please keep track of > whether the object was initialized so the calls are balanced. Done. https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:100: const base::TimeDelta delay = base::TimeDelta::FromSeconds(1); Delay removed, it's not needed. https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:111: subscription_ = BatteryStatusService::GetInstance()->AddCallback(callback_); My understanding is that when we hit the destructor for subscription_ then the callback will be removed? https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:115: // Called on IO thread. Added, I didn't know about TestBrowserThreadBundle :o/ https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:128: first_histogram_report_timestamp_ = base::Time(); This is on purpose, I want a NULL time here. https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:140: base::TimeDelta::FromMinutes(kBatteryReportingIntervalMinutes[i]); On 2014/10/21 15:12:49, Avi wrote: > for (auto reporting_interval : kBatteryReportingIntervalMinutes) { > base::TimeDelta delay = base::TimeDelta::FromMinutes(reporting_interval); > // ... Done. https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:141: system_interface_->ScheduleHistogramReport(delay); When the power source changes or all renders die, all these tasks are cancelled. The way I envisioned SystemInterface I'm trying to keep it as thin as possible with all the logic here, on the other hand moving the loop in there seems fine - wdyt? https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:146: // Called on IO thread. On 2014/10/21 16:01:21, Daniel Erat wrote: > assert this instead of commenting it Done. https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:170: // Called on IO thread. On 2014/10/21 16:01:21, Daniel Erat wrote: > assert this instead of commenting it Done. https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:173: double battery_level = status.level; This is unitless, stated as a fraction between 0-1.0 - this object uses the mechanics that back our implementation of http://www.w3.org/TR/battery-status/ . This is not uniform across systems, I'm not sure how we could get something that is uniform without hardware support (Even MSR power registers on Intel CPUs are documented as not being comparable across systems). If we filter the UMA numbers for a specific machine model and OS we should be able to see power trends e.g. the number will go up for MacBook Pros running a given OS version if we regress power. https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:180: if (num_live_renderers_ > 0) Comment added. https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:204: void PowerUsageMonitor::OnSuspend() { Thanks for bringing this up! I don't know, on Mac and Win at least it appears it will always run but I'm not sure about other platforms. The PostTask() to Cancel can conceivably race the delayed tasks regardless. I've added a mitigation in ReportBatteryLevelHistogram() including an UMA that will hopefully tell us how often we're hitting this. https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... File content/browser/power_usage_monitor_impl.h (right): https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:35: // Threading: Most methods are called on the IO thread. Thanks, that is indeed clearer - I've removed the comment and added DCHECK()s to anotate. https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:36: class PowerUsageMonitor : public base::PowerObserver, NotificationObserver { On 2014/10/21 16:01:21, Daniel Erat wrote: > add 'public' to NotificationObserver too (not sure whether it'll go back to > private by default, but better to be explicit) Done. https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:43: void SetPowerUsageMonitor(PowerUsageMonitor* monitor) { On 2014/10/21 16:01:21, Daniel Erat wrote: > set_power_usage_monitor Done. https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:47: virtual void ScheduleHistogramReport(base::TimeDelta delay); On 2014/10/21 16:01:21, Daniel Erat wrote: > make this class be a pure interface, please Done. https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:48: virtual void CancelPendingHistgramReports(); On 2014/10/21 16:01:21, Daniel Erat wrote: > s/Histgram/Histogram/ Done. https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:57: PowerUsageMonitor* power_usage_monitor_; On 2014/10/21 16:01:21, Daniel Erat wrote: > document that this isn't owned Done. https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:80: void SetSystemInterfaceForTest(scoped_ptr<SystemInterface> recorder); On 2014/10/21 16:01:21, Daniel Erat wrote: > s/recorder/interface/ Done. https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:86: virtual void OnSuspend() override; On 2014/10/21 15:12:50, Avi wrote: > The new style is to drop the "virtual" and only have "override". I hope that at > this point the clang plugin is updated to allow this. Done. https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:107: bool was_on_battery_power_; On 2014/10/21 16:01:21, Daniel Erat wrote: > add a comment describing what this represents -- when was it on battery power? Done. https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:108: double initial_battery_level_; On 2014/10/21 16:01:21, Daniel Erat wrote: > initial when -- when the object was first created? Done. https://codereview.chromium.org/560553005/diff/100001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:117: size_t num_live_renderers_; Done. https://codereview.chromium.org/560553005/diff/100001/content/public/browser/... File content/public/browser/power_usage_monitor.h (right): https://codereview.chromium.org/560553005/diff/100001/content/public/browser/... content/public/browser/power_usage_monitor.h:13: void CONTENT_EXPORT StartPowerUsageMonitor(); On 2014/10/21 16:01:21, Daniel Erat wrote: > nit: "CONTENT_EXPORT void" is overwhelmingly more common Done.
https://codereview.chromium.org/560553005/diff/140001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/560553005/diff/140001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1557: #if !defined(OS_LINUX) // http://crbug.com/426393 i think that this incorrectly excludes chrome os, which does support base::PowerMonitor now. this should probably instead be: #if !defined(OS_LINUX) || defined(OS_CHROMEOS) https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... File content/browser/power_usage_monitor_impl.cc (right): https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:29: : power_usage_monitor_(owner), weak_ptr_factory_(this) {} nit: one per line https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:32: virtual void ScheduleHistogramReport(base::TimeDelta delay) override { omit 'virtual' when using 'override' (also below) https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:61: base::TimeDelta::FromSeconds(discharge_time.InSecondsF() * 1.5); what's the significance of 1.5 times the delay? why not just the delay plus 5 seconds or so? https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:71: 0, the docs for Histogram::FactoryGet() say that the minimum needs to be greater than 0 https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:84: // Used to cancel in progress timers. s/timers/delayed tasks/ https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:119: // Power Monitor used to get suspend/resume notifications. nit: s/used/is used/ https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:150: // Rate-limit power recording to once every 24 hours. this seems a bit strange in that histograms will be reported more frequently if chrome is restarted. https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:212: DischargeStarted(battery_level); i don't understand this. if this if condition is false, wouldn't you need to later call DischargeStarted() in response to NOTIFICATION_RENDERER_PROCESS_CREATED? or are you depending on OnBatteryStatusUpdate() getting called again at some point in the future after a renderer has been created? it'd be cleaner to call DischargeStarted() unconditionally whenever the battery starts discharging and push any extra logic about not measuring the power draw while there aren't any renderers into that method. https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:220: // Record timestamp of first histogram report. delete unnecessary comment and omit curly brackets https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:232: for (int i = 0; i < num_renderers; ++i) { nit: omit curly brackets https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... File content/browser/power_usage_monitor_impl.h (right): https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:59: void StartOnIOThread(); StartOnIOThread looks like it should be private instead of public https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:61: void OnBatteryStatusUpdate(const blink::WebBatteryStatus& status); i think that this should be private too https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:63: double DischargeAmount() { rename to discharge_amount since it's inlined https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:71: void SetNumLiveRenderersForTest(int num_renderers); maybe just SetRenderersForTest? https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:79: virtual void Observe(int type, omit 'virtual' when using 'override' https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:84: void ReportBatteryLevelHistogram(base::Time discharge_time); this looks unused; delete it https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:88: void CancelPendingHistogramRecording(); s/Recording/Reporting/? https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:114: base::Time first_histogram_report_timestamp_; s/timestamp/time/ (or s/time/timestamp/ in start_discharge_time_ to be consistent) https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:116: // IDs of live rendere processes. s/rendere/renderer/
Ready for another look, have you looked over the unit tests yet? Many thanks! https://codereview.chromium.org/560553005/diff/140001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/560553005/diff/140001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1557: #if !defined(OS_LINUX) // http://crbug.com/426393 On 2014/10/23 19:05:34, Daniel Erat wrote: > i think that this incorrectly excludes chrome os, which does support > base::PowerMonitor now. this should probably instead be: > > #if !defined(OS_LINUX) || defined(OS_CHROMEOS) Done. https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... File content/browser/power_usage_monitor_impl.cc (right): https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:29: : power_usage_monitor_(owner), weak_ptr_factory_(this) {} Sorry, clang-format did it :( https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:32: virtual void ScheduleHistogramReport(base::TimeDelta delay) override { On 2014/10/23 19:05:34, Daniel Erat wrote: > omit 'virtual' when using 'override' (also below) Done. https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:61: base::TimeDelta::FromSeconds(discharge_time.InSecondsF() * 1.5); Changed to 2 minutes, added comment. https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:71: 0, On 2014/10/23 19:05:34, Daniel Erat wrote: > the docs for Histogram::FactoryGet() say that the minimum needs to be greater > than 0 Done. https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:84: // Used to cancel in progress timers. On 2014/10/23 19:05:34, Daniel Erat wrote: > s/timers/delayed tasks/ Done. https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:119: // Power Monitor used to get suspend/resume notifications. On 2014/10/23 19:05:34, Daniel Erat wrote: > nit: s/used/is used/ Done. https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:150: // Rate-limit power recording to once every 24 hours. The reason I added rate-limiting is so as not to collect more samples for users who frequently connect/disconnect the power source. This needs to be mitigated if frequent Chrome restarts correlate with increased power draw, I think this is measurable so we can wait and mitigate if we do see that users who restart Chrome more often, report different power usage from users who report once a day. wdyt? https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:212: DischargeStarted(battery_level); Moved to DischargeStarted(), OnBatteryStatusUpdate() is called on a hearbeat so yes, I'm counting on it getting called again. https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:220: // Record timestamp of first histogram report. On 2014/10/23 19:05:34, Daniel Erat wrote: > delete unnecessary comment and omit curly brackets Done. https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:232: for (int i = 0; i < num_renderers; ++i) { On 2014/10/23 19:05:34, Daniel Erat wrote: > nit: omit curly brackets Done. https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... File content/browser/power_usage_monitor_impl.h (right): https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:59: void StartOnIOThread(); On 2014/10/23 19:05:35, Daniel Erat wrote: > StartOnIOThread looks like it should be private instead of public Done. https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:61: void OnBatteryStatusUpdate(const blink::WebBatteryStatus& status); On 2014/10/23 19:05:35, Daniel Erat wrote: > i think that this should be private too Done. https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:63: double DischargeAmount() { On 2014/10/23 19:05:35, Daniel Erat wrote: > rename to discharge_amount since it's inlined Done. https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:71: void SetNumLiveRenderersForTest(int num_renderers); On 2014/10/23 19:05:35, Daniel Erat wrote: > maybe just SetRenderersForTest? Done. https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:79: virtual void Observe(int type, On 2014/10/23 19:05:35, Daniel Erat wrote: > omit 'virtual' when using 'override' Done. https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:84: void ReportBatteryLevelHistogram(base::Time discharge_time); On 2014/10/23 19:05:35, Daniel Erat wrote: > this looks unused; delete it Done. https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:88: void CancelPendingHistogramRecording(); On 2014/10/23 19:05:35, Daniel Erat wrote: > s/Recording/Reporting/? Done. https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:114: base::Time first_histogram_report_timestamp_; On 2014/10/23 19:05:34, Daniel Erat wrote: > s/timestamp/time/ (or s/time/timestamp/ in start_discharge_time_ to be > consistent) Done. https://codereview.chromium.org/560553005/diff/140001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:116: // IDs of live rendere processes. On 2014/10/23 19:05:35, Daniel Erat wrote: > s/rendere/renderer/ Done.
(still haven't looked at tests) re this earlier reply: "This is not uniform across systems, I'm not sure how we could get something that is uniform without hardware support (Even MSR power registers on Intel CPUs are documented as not being comparable across systems). If we filter the UMA numbers for a specific machine model and OS we should be able to see power trends e.g. the number will go up for MacBook Pros running a given OS version if we regress power." have you confirmed that you'll actually be able to filter the numbers based on a specific machine model and os? https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... File content/browser/power_usage_monitor_impl.cc (right): https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:30: PowerUsageMonitorSystemInterface(PowerUsageMonitor* owner) add 'explicit' https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:156: if (live_renderer_ids_.size() == 0) live_renderer_ids_.empty() https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:157: return; it still seems weird that you intentionally return early without setting |on_battery_power_| to true here (and also below in the reported-recently case). i expect a variable with that name to reflect the current state of the system. https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:195: double discharge_rate = discharge_percent / discharge_time.InMinutes(); InMinutes() truncates, so i think you have a potential divide-by-zero here https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:197: if (discharge_time.InMinutes() > 30) add curly brackets since the statement part is two lines https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:227: void PowerUsageMonitor::BatteryLevelReported() { assert the thread here (looks like it should be IO) https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:267: } add NOTREACHED() for unexpected notification types https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:270: if (num_live_renderers == 0 && previous_num_live_renderers != 0) { just inline live_renderer_ids_.empty() here https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... File content/browser/power_usage_monitor_impl.h (right): https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:18: #include "third_party/WebKit/public/platform/WebBatteryStatus.h" it feels strange to have content/ depend on blink for the battery status, considering that blink depends on content/ to get it in the first place (from my reading of e.g. https://codereview.chromium.org/356873002). all of this seems like it ought to be coming from base/power_monitor/ in the first place instead of having yet another implementation in content/. (the base code would need to be updated to also include the battery level, though.) https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:49: virtual void ReportBatteryLevelHistogram( it doesn't make sense to me to declare a private or protected method in an interface. why not just let the implementations handle this themselves? https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:108: double initial_battery_level_; document the scale for this. looking at the docs for the api that blink is implementing, it seems like it's [0.0, 1.0], where 1.0 indicates a full battery.
whoops, meant to cc sadrul about the blink/content question.
https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... File content/browser/power_usage_monitor_impl.h (right): https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:18: #include "third_party/WebKit/public/platform/WebBatteryStatus.h" On 2014/10/27 16:15:58, Daniel Erat wrote: > it feels strange to have content/ depend on blink for the battery status, > considering that blink depends on content/ to get it in the first place (from my > reading of e.g. https://codereview.chromium.org/356873002). We have similar issues for events too, where we convert from ui::Event to blink::WebInputEvent in content/ before handing it over to blink. > all of this seems > like it ought to be coming from base/power_monitor/ in the first place instead > of having yet another implementation in content/. (the base code would need to > be updated to also include the battery level, though.) +timvolodine@
timvolodine@chromium.org changed reviewers: + timvolodine@chromium.org
https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... File content/browser/power_usage_monitor_impl.cc (right): https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:14: #include "base/time/time.h" no need to include this, it's already included in .h https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:82: 102, why is this 102? is this intentional? https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:112: callback_ = base::Bind(&PowerUsageMonitor::OnBatteryStatusUpdate, nit: why not put this in the member initialization list above? https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:132: remove empty line? https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:210: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); I think after "mojofication" of the battery API (https://codereview.chromium.org/685703002/) we are running callbacks on the main thread.. https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... File content/browser/power_usage_monitor_impl.h (right): https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:14: #include "content/browser/battery_status/battery_status_service.h" note that battery API has recently been moved to device/battery, see https://codereview.chromium.org/685703002/. https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:18: #include "third_party/WebKit/public/platform/WebBatteryStatus.h" On 2014/10/27 20:57:39, sadrul wrote: > On 2014/10/27 16:15:58, Daniel Erat wrote: > > it feels strange to have content/ depend on blink for the battery status, > > considering that blink depends on content/ to get it in the first place (from > my > > reading of e.g. https://codereview.chromium.org/356873002). > > We have similar issues for events too, where we convert from ui::Event to > blink::WebInputEvent in content/ before handing it over to blink. > > > all of this seems > > like it ought to be coming from base/power_monitor/ in the first place instead > > of having yet another implementation in content/. (the base code would need to > > be updated to also include the battery level, though.) > > +timvolodine@ as noted above after the move to device/battery we now have a BatteryStatus class there (i.e. from battery_status.mojom). in the longer term the WebBatteryStatus may be merged when blink and chromium repositories merge...
Ready for another look, thanks! > have you confirmed that you'll actually be able to filter the numbers based on > a specific machine model and os? Yes I checked, can be done on platforms we care about - for Mac issue #429487 needs fixing. https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... File content/browser/power_usage_monitor_impl.cc (right): https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:14: #include "base/time/time.h" On 2014/10/28 23:44:03, timvolodine wrote: > no need to include this, it's already included in .h Done. https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:30: PowerUsageMonitorSystemInterface(PowerUsageMonitor* owner) On 2014/10/27 16:15:58, Daniel Erat wrote: > add 'explicit' Done. https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:82: 102, On 2014/10/28 23:44:03, timvolodine wrote: > why is this 102? is this intentional? Done. https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:112: callback_ = base::Bind(&PowerUsageMonitor::OnBatteryStatusUpdate, On 2014/10/28 23:44:03, timvolodine wrote: > nit: why not put this in the member initialization list above? Done. https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:132: On 2014/10/28 23:44:03, timvolodine wrote: > remove empty line? Done. https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:156: if (live_renderer_ids_.size() == 0) On 2014/10/27 16:15:58, Daniel Erat wrote: > live_renderer_ids_.empty() Done. https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:157: return; This is intentional, the object doesn't change state when no renderers are open. Since everything is wired up to occurs on state transitions and I don't want those to happen when the object is disabled. Unless you feel strongly about this I'd rather leave this as is. https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:195: double discharge_rate = discharge_percent / discharge_time.InMinutes(); On 2014/10/27 16:15:58, Daniel Erat wrote: > InMinutes() truncates, so i think you have a potential divide-by-zero here Good catch, fixed. https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:197: if (discharge_time.InMinutes() > 30) On 2014/10/27 16:15:58, Daniel Erat wrote: > add curly brackets since the statement part is two lines Done. https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:210: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); Thanks! I don't have my laptop with me today, will test and make necessary modifications before next round. https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:227: void PowerUsageMonitor::BatteryLevelReported() { On 2014/10/27 16:15:58, Daniel Erat wrote: > assert the thread here (looks like it should be IO) Done. https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:267: } On 2014/10/27 16:15:58, Daniel Erat wrote: > add NOTREACHED() for unexpected notification types Done. https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:270: if (num_live_renderers == 0 && previous_num_live_renderers != 0) { On 2014/10/27 16:15:58, Daniel Erat wrote: > just inline live_renderer_ids_.empty() here Done. https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... File content/browser/power_usage_monitor_impl.h (right): https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:14: #include "content/browser/battery_status/battery_status_service.h" On 2014/10/28 23:44:03, timvolodine wrote: > note that battery API has recently been moved to device/battery, see > https://codereview.chromium.org/685703002/. Done. https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:49: virtual void ReportBatteryLevelHistogram( On 2014/10/27 16:15:58, Daniel Erat wrote: > it doesn't make sense to me to declare a private or protected method in an > interface. why not just let the implementations handle this themselves? Done. https://codereview.chromium.org/560553005/diff/160001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:108: double initial_battery_level_; On 2014/10/27 16:15:58, Daniel Erat wrote: > document the scale for this. looking at the docs for the api that blink is > implementing, it seems like it's [0.0, 1.0], where 1.0 indicates a full battery. Done.
On second thoughts, can you please hold off with another round of review - there are some more changes I'd like to make.
Ready for another round, changes: * Moved everything to the UI thread per API changes, this also simplified things. * Removed rate limiting in favor of filtering results on the server side. * Start monitoring after 30 minutes of uptime.
https://codereview.chromium.org/560553005/diff/220001/content/browser/power_u... File content/browser/power_usage_monitor_impl.cc (right): https://codereview.chromium.org/560553005/diff/220001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:11: #include "base/message_loop/message_loop.h" don't think you're using this now https://codereview.chromium.org/560553005/diff/220001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:32: virtual ~PowerUsageMonitorSystemInterface() {} override instead of virtual https://codereview.chromium.org/560553005/diff/220001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:117: int64 uptime_in_minutes = base::SysInfo::Uptime() / (60 * 1000); cleaner to just convert this to a TimeDelta immediately (it's ugly that Uptime() doesn't return this to begin with) :-/ const base::TimeDelta uptime = base::TimeDelta::FromMilliseconds( base::SysInfo::Uptime()); const base::TimeDelta min_uptime = base::TimeDelta::FromMinutes( kMinUptimeMinutes); if (uptime < min_uptime) { BrowserThread::PostDelayedTask( ..., min_uptime - uptime); } else { StartInternal(); } (and define kMinUptimeMinutes in the anon namespace at the top of the file) https://codereview.chromium.org/560553005/diff/220001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:131: void PowerUsageMonitor::StartInternal() { nit: CHECK(!started_); https://codereview.chromium.org/560553005/diff/220001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:179: if (discharge_time.InMinutes() > 30) { move the "30" to a constant at the top of the file, e.g. kMinDischargeMinutes https://codereview.chromium.org/560553005/diff/220001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:197: bool was_on_battery_power = on_battery_power_; i've mentioned it before, but i feel pretty strongly that you should be updating on_battery_power_ unconditionally right here. by potentially returning from DischargeStarted() without having set it to true, you end up with a member named on_battery_power_ that sometimes doesn't accurately reflect whether the system is on battery power or not. this is a bug waiting to happen later when someone changes this code and assumes (quite reasonably) that on_battery_power_ is always kept up-to-date. perhaps you could add an additional tracking_discharge_ or measuring_discharge_ member that gets updated in DischargeStarted() if you need to be able to defer measurement when there aren't any live renderers. https://codereview.chromium.org/560553005/diff/220001/content/browser/power_u... File content/browser/power_usage_monitor_impl.h (right): https://codereview.chromium.org/560553005/diff/220001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:31: // * Metrics are only collected once per day. this is outdated now, right? https://codereview.chromium.org/560553005/diff/220001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:50: virtual ~PowerUsageMonitor(); nit: override instead of virtual https://codereview.chromium.org/560553005/diff/220001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:57: double discharge_amount() { nit: move above Start() since this is inlined; also make it const https://codereview.chromium.org/560553005/diff/220001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:65: void OnPowerStateChange(bool on_battery_power) override {}; don't inline this and OnResume(): http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in.... https://codereview.chromium.org/560553005/diff/220001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:107: // Current battery level, 0 if on wall power. nit: document the range here (specifically the upper bound) https://codereview.chromium.org/560553005/diff/220001/content/browser/power_u... File content/browser/power_usage_monitor_impl_unittest.cc (right): https://codereview.chromium.org/560553005/diff/220001/content/browser/power_u... content/browser/power_usage_monitor_impl_unittest.cc:15: : num_pending_histogram_reports_(0), now_(base::Time::Now()) {} nit: one per line since they don't all fit on the same line as the signature also initialize now_ to something like base::Time::FromInternalValue(1000) instead of base::Time::Now() so you get repeatable test results https://codereview.chromium.org/560553005/diff/220001/content/browser/power_u... content/browser/power_usage_monitor_impl_unittest.cc:16: virtual ~SystemInterfaceForTest() {} use override instead of virtual here https://codereview.chromium.org/560553005/diff/220001/content/browser/power_u... content/browser/power_usage_monitor_impl_unittest.cc:18: int num_pending_histogram_reports() { return num_pending_histogram_reports_; } nit: make this method const https://codereview.chromium.org/560553005/diff/220001/content/browser/power_u... content/browser/power_usage_monitor_impl_unittest.cc:24: void AdvanceClockHours(int hours) { this doesn't get called; delete it https://codereview.chromium.org/560553005/diff/220001/content/browser/power_u... content/browser/power_usage_monitor_impl_unittest.cc:36: virtual base::Time Now() override { return now_; } remove 'virtual'
All fixed, ready for another look. Additional changes: * A couple of new unit tests. * Fixed scaling of dischargerate UMA, I hope the comments describe the rationale behind the scaling adequately, if not I'm happy to improve them. https://codereview.chromium.org/560553005/diff/220001/content/browser/power_u... File content/browser/power_usage_monitor_impl.cc (right): https://codereview.chromium.org/560553005/diff/220001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:11: #include "base/message_loop/message_loop.h" On 2014/11/04 17:04:02, Daniel Erat wrote: > don't think you're using this now Done. https://codereview.chromium.org/560553005/diff/220001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:32: virtual ~PowerUsageMonitorSystemInterface() {} On 2014/11/04 17:04:03, Daniel Erat wrote: > override instead of virtual Done. https://codereview.chromium.org/560553005/diff/220001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:117: int64 uptime_in_minutes = base::SysInfo::Uptime() / (60 * 1000); On 2014/11/04 17:04:03, Daniel Erat wrote: > cleaner to just convert this to a TimeDelta immediately (it's ugly that Uptime() > doesn't return this to begin with) :-/ > > const base::TimeDelta uptime = base::TimeDelta::FromMilliseconds( > base::SysInfo::Uptime()); > const base::TimeDelta min_uptime = base::TimeDelta::FromMinutes( > kMinUptimeMinutes); > if (uptime < min_uptime) { > BrowserThread::PostDelayedTask( > ..., > min_uptime - uptime); > } else { > StartInternal(); > } > > (and define kMinUptimeMinutes in the anon namespace at the top of the file) Done. https://codereview.chromium.org/560553005/diff/220001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:131: void PowerUsageMonitor::StartInternal() { On 2014/11/04 17:04:03, Daniel Erat wrote: > nit: CHECK(!started_); Done. https://codereview.chromium.org/560553005/diff/220001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:179: if (discharge_time.InMinutes() > 30) { On 2014/11/04 17:04:02, Daniel Erat wrote: > move the "30" to a constant at the top of the file, e.g. kMinDischargeMinutes Done. https://codereview.chromium.org/560553005/diff/220001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:197: bool was_on_battery_power = on_battery_power_; On 2014/11/04 17:04:02, Daniel Erat wrote: > i've mentioned it before, but i feel pretty strongly that you should be updating > on_battery_power_ unconditionally right here. by potentially returning from > DischargeStarted() without having set it to true, you end up with a member named > on_battery_power_ that sometimes doesn't accurately reflect whether the system > is on battery power or not. this is a bug waiting to happen later when someone > changes this code and assumes (quite reasonably) that on_battery_power_ is > always kept up-to-date. > > perhaps you could add an additional tracking_discharge_ or measuring_discharge_ > member that gets updated in DischargeStarted() if you need to be able to defer > measurement when there aren't any live renderers. Done. https://codereview.chromium.org/560553005/diff/220001/content/browser/power_u... File content/browser/power_usage_monitor_impl.h (right): https://codereview.chromium.org/560553005/diff/220001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:31: // * Metrics are only collected once per day. On 2014/11/04 17:04:03, Daniel Erat wrote: > this is outdated now, right? Done. https://codereview.chromium.org/560553005/diff/220001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:50: virtual ~PowerUsageMonitor(); On 2014/11/04 17:04:03, Daniel Erat wrote: > nit: override instead of virtual Done. https://codereview.chromium.org/560553005/diff/220001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:57: double discharge_amount() { On 2014/11/04 17:04:03, Daniel Erat wrote: > nit: move above Start() since this is inlined; also make it const Done. https://codereview.chromium.org/560553005/diff/220001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:65: void OnPowerStateChange(bool on_battery_power) override {}; On 2014/11/04 17:04:03, Daniel Erat wrote: > don't inline this and OnResume(): > http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in.... Done. https://codereview.chromium.org/560553005/diff/220001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:107: // Current battery level, 0 if on wall power. On 2014/11/04 17:04:03, Daniel Erat wrote: > nit: document the range here (specifically the upper bound) Done. https://codereview.chromium.org/560553005/diff/220001/content/browser/power_u... File content/browser/power_usage_monitor_impl_unittest.cc (right): https://codereview.chromium.org/560553005/diff/220001/content/browser/power_u... content/browser/power_usage_monitor_impl_unittest.cc:15: : num_pending_histogram_reports_(0), now_(base::Time::Now()) {} On 2014/11/04 17:04:03, Daniel Erat wrote: > nit: one per line since they don't all fit on the same line as the signature > > also initialize now_ to something like base::Time::FromInternalValue(1000) > instead of base::Time::Now() so you get repeatable test results Done. https://codereview.chromium.org/560553005/diff/220001/content/browser/power_u... content/browser/power_usage_monitor_impl_unittest.cc:16: virtual ~SystemInterfaceForTest() {} On 2014/11/04 17:04:03, Daniel Erat wrote: > use override instead of virtual here Done. https://codereview.chromium.org/560553005/diff/220001/content/browser/power_u... content/browser/power_usage_monitor_impl_unittest.cc:18: int num_pending_histogram_reports() { return num_pending_histogram_reports_; } On 2014/11/04 17:04:03, Daniel Erat wrote: > nit: make this method const Done. https://codereview.chromium.org/560553005/diff/220001/content/browser/power_u... content/browser/power_usage_monitor_impl_unittest.cc:24: void AdvanceClockHours(int hours) { On 2014/11/04 17:04:03, Daniel Erat wrote: > this doesn't get called; delete it Done. https://codereview.chromium.org/560553005/diff/220001/content/browser/power_u... content/browser/power_usage_monitor_impl_unittest.cc:36: virtual base::Time Now() override { return now_; } On 2014/11/04 17:04:03, Daniel Erat wrote: > remove 'virtual' Done.
https://codereview.chromium.org/560553005/diff/260001/content/browser/power_u... File content/browser/power_usage_monitor_impl.cc (right): https://codereview.chromium.org/560553005/diff/260001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:138: base::TimeDelta::FromMinutes(kMinUptimeMinutes - uptime.InMinutes()); just do base::TimeDelta::FromMinutes(kMinUptimeMinutes) - uptime, or put the min uptime in a base::TimeDelta earlier like i suggested in the previous round https://codereview.chromium.org/560553005/diff/260001/content/browser/power_u... File content/browser/power_usage_monitor_impl.h (right): https://codereview.chromium.org/560553005/diff/260001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:44: // Record the batter discharge rate over a period when the system is s/batter/battery/ https://codereview.chromium.org/560553005/diff/260001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:45: // on battery power. Possible values [0,100]. "rate" implies a numerator and denominator. what are they? please document these here -- the battery_level members look like they're in [0.0, 1.0]; how does this [0, 100] range relate to that?
Thanks Dan! These seem like small issues and I'm anxious to get this landed soon - are there any big things remaining or are these just the last few bits? Just checking because there's a lot of latency in reviews between us and I want to try to minimize the number of remaining rounds. [0,100] is just the level of precision at which the discharge rate UMA is reported - 0 being very slow discharge (slow discharge is defined as the battery depleting completely over 8 hours) and 100 being very fast discharge. The precision is arbitrary, I could have used [0,1000] instead or picked a longer time than 8 hours. I'll put in a proper comment and send out another round tomorrow. On Wed, Nov 5, 2014 at 7:17 PM, <derat@chromium.org> wrote: > > https://codereview.chromium.org/560553005/diff/260001/ > content/browser/power_usage_monitor_impl.cc > File content/browser/power_usage_monitor_impl.cc (right): > > https://codereview.chromium.org/560553005/diff/260001/ > content/browser/power_usage_monitor_impl.cc#newcode138 > content/browser/power_usage_monitor_impl.cc:138: > base::TimeDelta::FromMinutes(kMinUptimeMinutes - uptime.InMinutes()); > just do base::TimeDelta::FromMinutes(kMinUptimeMinutes) - uptime, or put > the min uptime in a base::TimeDelta earlier like i suggested in the > previous round > > https://codereview.chromium.org/560553005/diff/260001/ > content/browser/power_usage_monitor_impl.h > File content/browser/power_usage_monitor_impl.h (right): > > https://codereview.chromium.org/560553005/diff/260001/ > content/browser/power_usage_monitor_impl.h#newcode44 > content/browser/power_usage_monitor_impl.h:44: // Record the batter > discharge rate over a period when the system is > s/batter/battery/ > > https://codereview.chromium.org/560553005/diff/260001/ > content/browser/power_usage_monitor_impl.h#newcode45 > content/browser/power_usage_monitor_impl.h:45: // on battery power. > Possible values [0,100]. > "rate" implies a numerator and denominator. what are they? please > document these here -- the battery_level members look like they're in > [0.0, 1.0]; how does this [0, 100] range relate to that? > > https://codereview.chromium.org/560553005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
i don't think i had any other large comments; thanks for checking.
lgtm https://codereview.chromium.org/560553005/diff/260001/content/browser/power_u... File content/browser/power_usage_monitor_impl.cc (right): https://codereview.chromium.org/560553005/diff/260001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:150: CHECK(!started_); Why all the CHECKs rather than DCHECKs? That's not style. https://codereview.chromium.org/560553005/diff/260001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:203: // 1.0 = fully discharged in a short amt of time. s/amt/amount/ https://codereview.chromium.org/560553005/diff/260001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:245: type == NOTIFICATION_RENDERER_PROCESS_CLOSED) { You shouldn't need all of those. NOTIFICATION_RENDERER_PROCESS_CREATED is a render process launch, and NOTIFICATION_RENDERER_PROCESS_CLOSED is a render process end. NOTIFICATION_RENDERER_PROCESS_TERMINATED means that the render process *host* is being destroyed; do you need to track that if you already handle _CLOSED? https://codereview.chromium.org/560553005/diff/260001/content/browser/power_u... File content/browser/power_usage_monitor_impl_unittest.cc (right): https://codereview.chromium.org/560553005/diff/260001/content/browser/power_u... content/browser/power_usage_monitor_impl_unittest.cc:84: NOTIFICATION_RENDERER_PROCESS_TERMINATED, kDummyRenderProcessHostID); NOTIFICATION_RENDERER_PROCESS_CLOSED for the death of a renderer?
jeremy@chromium.org changed reviewers: + jochen@chromium.org
Jochen: Owner review for chrome/browser please. All fixed, ready for another look https://codereview.chromium.org/560553005/diff/260001/content/browser/power_u... File content/browser/power_usage_monitor_impl.cc (right): https://codereview.chromium.org/560553005/diff/260001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:138: base::TimeDelta::FromMinutes(kMinUptimeMinutes - uptime.InMinutes()); On 2014/11/05 17:17:20, Daniel Erat wrote: > just do base::TimeDelta::FromMinutes(kMinUptimeMinutes) - uptime, or put the min > uptime in a base::TimeDelta earlier like i suggested in the previous round Done. https://codereview.chromium.org/560553005/diff/260001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:150: CHECK(!started_); On 2014/11/05 20:56:28, Avi wrote: > Why all the CHECKs rather than DCHECKs? That's not style. Done. https://codereview.chromium.org/560553005/diff/260001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:203: // 1.0 = fully discharged in a short amt of time. On 2014/11/05 20:56:29, Avi wrote: > s/amt/amount/ Done. https://codereview.chromium.org/560553005/diff/260001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:245: type == NOTIFICATION_RENDERER_PROCESS_CLOSED) { On 2014/11/05 20:56:28, Avi wrote: > You shouldn't need all of those. > > NOTIFICATION_RENDERER_PROCESS_CREATED is a render process launch, and > NOTIFICATION_RENDERER_PROCESS_CLOSED is a render process end. > NOTIFICATION_RENDERER_PROCESS_TERMINATED means that the render process *host* is > being destroyed; do you need to track that if you already handle _CLOSED? Done. https://codereview.chromium.org/560553005/diff/260001/content/browser/power_u... File content/browser/power_usage_monitor_impl.h (right): https://codereview.chromium.org/560553005/diff/260001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:44: // Record the batter discharge rate over a period when the system is On 2014/11/05 17:17:20, Daniel Erat wrote: > s/batter/battery/ Done. https://codereview.chromium.org/560553005/diff/260001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:45: // on battery power. Possible values [0,100]. On 2014/11/05 17:17:20, Daniel Erat wrote: > "rate" implies a numerator and denominator. what are they? please document these > here -- the battery_level members look like they're in [0.0, 1.0]; how does this > [0, 100] range relate to that? Done.
derat@chromium.org changed reviewers: + mpearson@chromium.org
thanks, the code lgtm now. i'm adding mpearson@ as a reviewer since i still have some concerns about how this histogram will be interpreted and the sorts of insights that can be gained from looking at it, and would like to hear his opinions. https://codereview.chromium.org/560553005/diff/300001/content/browser/power_u... File content/browser/power_usage_monitor_impl.h (right): https://codereview.chromium.org/560553005/diff/300001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:52: // discern interesting changes in discharge rate. mark, any opinions about whether this is a meaningful thing to report as a histogram?
https://codereview.chromium.org/560553005/diff/300001/content/browser/power_u... File content/browser/power_usage_monitor_impl.h (right): https://codereview.chromium.org/560553005/diff/300001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:52: // discern interesting changes in discharge rate. On 2014/11/06 17:22:43, Daniel Erat wrote: > mark, any opinions about whether this is a meaningful thing to report as a > histogram? I like the idea. Here are my concerns: - When / how frequently is this emitted? If it's not emitted at a regular rate, then this will be hard to interpret (depending on what affects the rate at which it is emitted). - How arbitrary is the scale? Can it vary from device to device? What about platform to platform? In either case (especially the first), that can make this measure very hard to interpret.
i think that you also ought to add the histograms to tools/metrics/histograms/histograms.xml as part of this change. https://codereview.chromium.org/560553005/diff/300001/content/browser/power_u... File content/browser/power_usage_monitor_impl.h (right): https://codereview.chromium.org/560553005/diff/300001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:52: // discern interesting changes in discharge rate. On 2014/11/06 18:50:03, Mark P wrote: > On 2014/11/06 17:22:43, Daniel Erat wrote: > > mark, any opinions about whether this is a meaningful thing to report as a > > histogram? > > I like the idea. Here are my concerns: > - When / how frequently is this emitted? If it's not emitted at a regular rate, > then this will be hard to interpret (depending on what affects the rate at which > it is emitted). > - How arbitrary is the scale? Can it vary from device to device? What about > platform to platform? In either case (especially the first), that can make this > measure very hard to interpret. i asked about the device-to-device variance in an earlier patch set. the answer was "very", but it sounds like the intent is to use this to view changes across individual hardware platforms (e.g. specific macbooks or chromebooks). i think that most of my unease would go away if this histogram were renamed to something like Power.BatteryDischargePercentPerHour and scaled accordingly. i don't like the "arbitrary" definitions of slow vs. fast discharge.
https://codereview.chromium.org/560553005/diff/300001/content/browser/power_u... File content/browser/power_usage_monitor_impl.h (right): https://codereview.chromium.org/560553005/diff/300001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:24: // * Power.BatteryDischarge_{5,15,30} - delta between battery level when (and then Power.BatteryDischargePercent_... for these)
https://codereview.chromium.org/560553005/diff/300001/content/browser/power_u... File content/browser/power_usage_monitor_impl.h (right): https://codereview.chromium.org/560553005/diff/300001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:52: // discern interesting changes in discharge rate. On 2014/11/06 18:54:15, Daniel Erat wrote: > On 2014/11/06 18:50:03, Mark P wrote: > > On 2014/11/06 17:22:43, Daniel Erat wrote: > > > mark, any opinions about whether this is a meaningful thing to report as a > > > histogram? > > > > I like the idea. Here are my concerns: > > - When / how frequently is this emitted? If it's not emitted at a regular > rate, > > then this will be hard to interpret (depending on what affects the rate at > which > > it is emitted). > > - How arbitrary is the scale? Can it vary from device to device? What about > > platform to platform? In either case (especially the first), that can make > this > > measure very hard to interpret. > > i asked about the device-to-device variance in an earlier patch set. the answer > was "very", but it sounds like the intent is to use this to view changes across > individual hardware platforms (e.g. specific macbooks or chromebooks). Is hard the slicing by hardware fine-grained enough on the UMA dashboard, or would there be variety from device to device within the same hardware class at the granularity we're aware of? > > i think that most of my unease would go away if this histogram were renamed to > something like Power.BatteryDischargePercentPerHour and scaled accordingly. i > don't like the "arbitrary" definitions of slow vs. fast discharge.
jeremy@chromium.org changed reviewers: + thakis@chromium.org - jochen@chromium.org
Nico: Can you do an owner review of chrome_browser_main please? https://codereview.chromium.org/560553005/diff/300001/content/browser/power_u... File content/browser/power_usage_monitor_impl.h (right): https://codereview.chromium.org/560553005/diff/300001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:52: // discern interesting changes in discharge rate. * I changed this to report the percent of battery charge depleted per hour while the system is on battery power. * This is reported every time the power adapter is connected after 30 minutes or more on battery power. * I'd assume the number would be comparable across devices of the same hardware class: - Hardware class is reported directly on CrOS/OSX (crbug.com/429487) and on Android we can tell the device type by other means. - The UMA dashboard has a "Show Versions split by Chrome OS hardware class" checkbox which should allow us to split correctly for CrOS/OSX if not we'll need to either analyze via dremel or add the nobs we need to the dashboard.
a few more comments, but still lgtm after this https://codereview.chromium.org/560553005/diff/320001/content/browser/power_u... File content/browser/power_usage_monitor_impl.cc (right): https://codereview.chromium.org/560553005/diff/320001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:195: double discharge_hours = discharge_time.InMinutes() / 60.0; nit: delete extra space before '='. probably also better to use InSeconds() or InSecondsF() and divide by 3600.0 to avoid truncating e.g. 59 seconds to 0 minutes. https://codereview.chromium.org/560553005/diff/320001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:196: int percent_per_hour = (discharge_amount() / discharge_hours) * 100.0; i think you should round this value. i don't think that round() is available on all platforms (at least, i've had trouble with it on windows before), so you might need to just add 0.5 before truncating. does implicit conversion from double to int work everywhere, or do you need static_cast<int>? https://codereview.chromium.org/560553005/diff/320001/content/browser/power_u... File content/browser/power_usage_monitor_impl_unittest.cc (right): https://codereview.chromium.org/560553005/diff/320001/content/browser/power_u... content/browser/power_usage_monitor_impl_unittest.cc:104: ASSERT_EQ(system_interface_->discharge_percent_per_hour(), 0); reverse the order of the arguments here (and in a bunch of other calls); the _EQ() macros need to get (expected, actual) to produce correct failure messages. https://codereview.chromium.org/560553005/diff/320001/content/browser/power_u... content/browser/power_usage_monitor_impl_unittest.cc:113: TEST_F(PowerUsageMonitorTest, DischargePercReported) { nit: s/Perc/Percent/
thakis/mpearson: lgty? (Thanks Daniel!)
https://codereview.chromium.org/560553005/diff/320001/content/browser/power_u... File content/browser/power_usage_monitor_impl.cc (right): https://codereview.chromium.org/560553005/diff/320001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:195: double discharge_hours = discharge_time.InMinutes() / 60.0; On 2014/11/09 17:07:14, Daniel Erat wrote: > nit: delete extra space before '='. probably also better to use InSeconds() or > InSecondsF() and divide by 3600.0 to avoid truncating e.g. 59 seconds to 0 > minutes. Done. https://codereview.chromium.org/560553005/diff/320001/content/browser/power_u... content/browser/power_usage_monitor_impl.cc:196: int percent_per_hour = (discharge_amount() / discharge_hours) * 100.0; Changed to floor( + 0.5) https://codereview.chromium.org/560553005/diff/320001/content/browser/power_u... File content/browser/power_usage_monitor_impl_unittest.cc (right): https://codereview.chromium.org/560553005/diff/320001/content/browser/power_u... content/browser/power_usage_monitor_impl_unittest.cc:104: ASSERT_EQ(system_interface_->discharge_percent_per_hour(), 0); On 2014/11/09 17:07:14, Daniel Erat wrote: > reverse the order of the arguments here (and in a bunch of other calls); the > _EQ() macros need to get (expected, actual) to produce correct failure messages. Done. https://codereview.chromium.org/560553005/diff/320001/content/browser/power_u... content/browser/power_usage_monitor_impl_unittest.cc:113: TEST_F(PowerUsageMonitorTest, DischargePercReported) { On 2014/11/09 17:07:14, Daniel Erat wrote: > nit: s/Perc/Percent/ Done.
browser/chrome_browser_main.cc lgtm
Lots of comments in histograms.xml Sorry for the possible incoherence; I'm not entirely awake. https://codereview.chromium.org/560553005/diff/340001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/560553005/diff/340001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25193: + The amount of power used per hour. Reported every time the system is on Please make this description clearer. The comment says "amount of power". The units say "%". These are different things. Furthermore, if you meant to be talking about percent, what is the percent relative to? Starting battery power level? Full battery power level? https://codereview.chromium.org/560553005/diff/340001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25194: + battery power for more than 30 minutes. This measurement is tied tightly to If the system is on battery power for more than 30 minutes yet sleeping, is this recorded? Also, is this reported every 30 minutes, or only the first time the system is on battery power for more than thirty minutes? i.e., if I keep using my laptop after this this or another hour, do we report additional samples? Furthermore, if the latter, then how do you handle the second report (the one after being on battery for an hour)? It is the last thirty minutes normalized to be a "percent per hour" rate, or it is the actual percent per hour rate for the last hour? In short, these descriptions should be detailed. You need to import part of the comments you've answered in the code review thread and even in the changelist description into these descriptions. Many of these comment apply to below histograms too. https://codereview.chromium.org/560553005/diff/340001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25209: + <owner>Jeremy@chromium.org</owner> nit here and below: please don't captizalize usernames https://codereview.chromium.org/560553005/diff/340001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25251: + the measurement. This histogram doesn't seeming meaningful without a comparison point. If I see the number "500" here, how do I decide if that's a lot? Which histogram do I compare it to? Suppose the other histograms have counts (respectively of 5000, 2500, 2000, 1500)? I would think you need to break errors down by which histogram we were attempting to record and then failed at recording. Also, is the "delay" the time it takes to evaluate the power usage or the duration that the histogram is supposed to be recording over? Please clarify. If it's the later, is "much greater" relative to the size of the expected duration? It is, say, a deviation by 10% or an absolute deviation like say 10 minutes.
mpearson: Fixed histogram.xml description and removed cancelled histogram per offline discussion.
lgtm histograms.xml lgtm
The CQ bit was checked by jeremy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/560553005/400001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
jeremy@chromium.org changed reviewers: + ppi@chromium.org
ppi: Could you please confirm my change to content_tests.gypi in patchset #21 is correct? I'm basically mimicing what you did in https://codereview.chromium.org/592153002 so I can access BatteryStatus in content_unittests .
Ouch, I think we're working on conflicting things. Please take a look at the comments below. Wdyt? https://codereview.chromium.org/560553005/diff/420001/content/browser/power_u... File content/browser/power_usage_monitor_impl.h (right): https://codereview.chromium.org/560553005/diff/420001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:96: scoped_ptr<device::BatteryStatusService::BatteryUpdateSubscription> From the point of view of Mojo-fication of browser services, I'm worried about hooking this up directly to BatteryStatusService. BatteryStatusService is an implementation detail of one particular implementation of the abstract BatteryMonitor service. Right now we have only one implementation, so hooking this up here would work on all platforms, but we're working on having entirely independent implementation for Android. Once this change is done, we would stop recording UMA on Android. Ideally, instead of talking to BatteryStatusService, PowerUsageMonitor would connect to the battery monitor Mojo service as a client, just like the renderer does. This way we'd be isolated from any implementation details of the service. Could you explore going this way? I'd be happy to help with the Mojo-specific bits. https://codereview.chromium.org/560553005/diff/420001/content/browser/power_u... File content/browser/power_usage_monitor_impl_unittest.cc (right): https://codereview.chromium.org/560553005/diff/420001/content/browser/power_u... content/browser/power_usage_monitor_impl_unittest.cc:76: device::BatteryStatus battery_status; nit: include what you use, I think that would be "device/battery/battery_monitor.mojom.h". https://codereview.chromium.org/560553005/diff/420001/content/content_tests.gypi File content/content_tests.gypi (right): https://codereview.chromium.org/560553005/diff/420001/content/content_tests.g... content/content_tests.gypi:364: '../mojo/public/mojo_public.gyp:mojo_cpp_bindings', Does it compile without these two? It seems that you're using only the generated datatype in the tests, hence the ones with "battery.gyp" might be enough.
https://codereview.chromium.org/560553005/diff/420001/content/browser/power_u... File content/browser/power_usage_monitor_impl.h (right): https://codereview.chromium.org/560553005/diff/420001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:96: scoped_ptr<device::BatteryStatusService::BatteryUpdateSubscription> Thanks, wasn't aware of that. Unless you are strongly opposed, I'd like to land this as is and then look into porting this to use the Mojo service. The lead time on getting numbers from the stable channel for these UMAs is quite large so I'd like to get the clock ticking. If the underlying Mojo service is ready I can work with you next week to port the code to use the preferred interface. https://codereview.chromium.org/560553005/diff/420001/content/browser/power_u... File content/browser/power_usage_monitor_impl_unittest.cc (right): https://codereview.chromium.org/560553005/diff/420001/content/browser/power_u... content/browser/power_usage_monitor_impl_unittest.cc:76: device::BatteryStatus battery_status; On 2014/11/16 16:03:08, ppi wrote: > nit: include what you use, I think that would be > "device/battery/battery_monitor.mojom.h". Done. https://codereview.chromium.org/560553005/diff/420001/content/content_tests.gypi File content/content_tests.gypi (right): https://codereview.chromium.org/560553005/diff/420001/content/content_tests.g... content/content_tests.gypi:364: '../mojo/public/mojo_public.gyp:mojo_cpp_bindings', On 2014/11/16 16:03:08, ppi wrote: > Does it compile without these two? It seems that you're using only the generated > datatype in the tests, hence the ones with "battery.gyp" might be enough. No, this is the minimal set of dependencies needed for this to compile, I get the following error when removing these: Undefined symbols for architecture i386: "__ZN4mojo8internal10LogMessageC1EPKcii", referenced from: __ZNK4mojo16InlinedStructPtrIN6device13BatteryStatusEEptEv in libdevice_battery_mojo_bindings.a(device_battery_mojo_bindings.battery_status.mojom.o) "__ZN4mojo8internal10LogMessageD1Ev", referenced from: __ZNK4mojo16InlinedStructPtrIN6device13BatteryStatusEEptEv in libdevice_battery_mojo_bindings.a(device_battery_mojo_bindings.battery_status.mojom.o) "__ZN4mojo8internal20ValidateStructHeaderEPKvjjPNS0_13BoundsCheckerE", referenced from: __ZN6device8internal18BatteryStatus_Data8ValidateEPKvPN4mojo8internal13BoundsCheckerE in libdevice_battery_mojo_bindings.a(device_battery_mojo_bindings.battery_status.mojom.o)
lgtm https://codereview.chromium.org/560553005/diff/420001/content/browser/power_u... File content/browser/power_usage_monitor_impl.h (right): https://codereview.chromium.org/560553005/diff/420001/content/browser/power_u... content/browser/power_usage_monitor_impl.h:96: scoped_ptr<device::BatteryStatusService::BatteryUpdateSubscription> On 2014/11/17 07:50:48, jeremy wrote: > Thanks, wasn't aware of that. > > Unless you are strongly opposed, I'd like to land this as is and then look into > porting this to use the Mojo service. The lead time on getting numbers from the > stable channel for these UMAs is quite large so I'd like to get the clock > ticking. > If the underlying Mojo service is ready I can work with you next week to port > the code to use the preferred interface. Porting to Mojo in a separate CL sounds good, thanks. https://codereview.chromium.org/560553005/diff/420001/content/content_tests.gypi File content/content_tests.gypi (right): https://codereview.chromium.org/560553005/diff/420001/content/content_tests.g... content/content_tests.gypi:364: '../mojo/public/mojo_public.gyp:mojo_cpp_bindings', On 2014/11/17 07:50:48, jeremy wrote: > On 2014/11/16 16:03:08, ppi wrote: > > Does it compile without these two? It seems that you're using only the > generated > > datatype in the tests, hence the ones with "battery.gyp" might be enough. > > No, this is the minimal set of dependencies needed for this to compile, I get > the following error when removing these: > > Undefined symbols for architecture i386: > "__ZN4mojo8internal10LogMessageC1EPKcii", referenced from: > __ZNK4mojo16InlinedStructPtrIN6device13BatteryStatusEEptEv in > libdevice_battery_mojo_bindings.a(device_battery_mojo_bindings.battery_status.mojom.o) > "__ZN4mojo8internal10LogMessageD1Ev", referenced from: > __ZNK4mojo16InlinedStructPtrIN6device13BatteryStatusEEptEv in > libdevice_battery_mojo_bindings.a(device_battery_mojo_bindings.battery_status.mojom.o) > "__ZN4mojo8internal20ValidateStructHeaderEPKvjjPNS0_13BoundsCheckerE", > referenced from: > > __ZN6device8internal18BatteryStatus_Data8ValidateEPKvPN4mojo8internal13BoundsCheckerE > in > libdevice_battery_mojo_bindings.a(device_battery_mojo_bindings.battery_status.mojom.o) Yup, it seems that we need the environment for logging and cpp_bindings for validation, both of which due to the generated battery_status.mojom h/cc code that we're referencing in the test. Please go ahead.
The CQ bit was checked by jeremy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/560553005/440001
Message was sent while issue was closed.
Committed patchset #22 (id:440001)
Message was sent while issue was closed.
Patchset 22 (id:??) landed as https://crrev.com/76a267615a7bc5fcb76f3a6c425c57eb99b66745 Cr-Commit-Position: refs/heads/master@{#304412} |
