Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
140583003:
Chrome power profiler service (Closed)
|
Created:
6 years, 11 months ago by Pan Modified:
6 years, 9 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionThis CL implemented part of chrome power profiler, including the service, data provider and observer base.
BUG=337138
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=254116
Patch Set 1 #
Total comments: 20
Patch Set 2 : #
Total comments: 20
Patch Set 3 : #
Total comments: 12
Patch Set 4 : remove an invalid comment. #Patch Set 5 : #
Total comments: 8
Patch Set 6 : #Patch Set 7 : #
Total comments: 2
Patch Set 8 : #
Total comments: 2
Patch Set 9 : #
Total comments: 2
Patch Set 10 : #Patch Set 11 : #
Total comments: 18
Patch Set 12 : #Patch Set 13 : Remove devtools_protocol_constants, which was replaced by generators. #
Total comments: 5
Patch Set 14 : #
Total comments: 53
Patch Set 15 : #Patch Set 16 : Move complex destructor impl to .cc #
Total comments: 36
Patch Set 17 : #Patch Set 18 : remove timeout from tests, patch for landing #
Total comments: 19
Patch Set 19 : #
Total comments: 1
Patch Set 20 : patch for landing #
Total comments: 11
Patch Set 21 : #Patch Set 22 : Remove PowerDataProviderFactory #
Total comments: 2
Patch Set 23 : patch for landing #Patch Set 24 : #Patch Set 25 : #Messages
Total messages: 83 (0 generated)
Hi, split the first patch from https://codereview.chromium.org/106223002/, ready to review now. thanks Pan
https://codereview.chromium.org/140583003/diff/1/content/browser/power_profil... File content/browser/power_profiler/power_profiler_service.cc (right): https://codereview.chromium.org/140583003/diff/1/content/browser/power_profil... content/browser/power_profiler/power_profiler_service.cc:41: bool PowerProfilerService::AddObserver(PowerProfilerObserver* obs) { I don't think you need a return value here. If someone is interested to know if using this makes sense, it does have the IsAvailable method. https://codereview.chromium.org/140583003/diff/1/content/browser/power_profil... content/browser/power_profiler/power_profiler_service.cc:66: GetHigherResolution(resolution)); I don't really like that each observer needs to implement the same comparaison code. Can you have it return a value instead? You will have to iterate yourself over the list and get the maximal value. https://codereview.chromium.org/140583003/diff/1/content/browser/power_profil... content/browser/power_profiler/power_profiler_service.cc:72: if (query_power_timer_.IsRunning()) You might want to check on your status, instead of finding that out from the timer status) https://codereview.chromium.org/140583003/diff/1/content/browser/power_profil... content/browser/power_profiler/power_profiler_service.cc:79: DCHECK (!query_power_timer_.IsRunning()); Maybe also check for your status. https://codereview.chromium.org/140583003/diff/1/content/browser/power_profil... content/browser/power_profiler/power_profiler_service.cc:93: if (PROFILING == status_) { Why is that a test instead of a DCHECK? Are you expecting stop to be called when you are not running? https://codereview.chromium.org/140583003/diff/1/content/browser/power_profil... content/browser/power_profiler/power_profiler_service.cc:109: void PowerProfilerService::ProcessData() { you could DCHECK that you are on your task_runner. Also maybe rename this method QueryData instead of ProcessData. And even: rename OnTimer QueryData rename ProcessData QueryDataOnTaskRunner and in Start, instead of writing the PostTask, you can call QueryData https://codereview.chromium.org/140583003/diff/1/content/public/browser/power... File content/public/browser/power_data_provider.h (right): https://codereview.chromium.org/140583003/diff/1/content/public/browser/power... content/public/browser/power_data_provider.h:24: base::TimeTicks time; Comment on those 2 values, especially the |value| one. What is it, what is the unit. https://codereview.chromium.org/140583003/diff/1/content/public/browser/power... File content/public/browser/power_profiler_observer.h (right): https://codereview.chromium.org/140583003/diff/1/content/public/browser/power... content/public/browser/power_profiler_observer.h:26: ~PowerProfilerObserver(); Where is the implementation of those 2 methods? You probably want those inline here. Moreover, please the destructor needs to be virtual. https://codereview.chromium.org/140583003/diff/1/content/public/browser/power... File content/public/browser/power_profiler_service.h (right): https://codereview.chromium.org/140583003/diff/1/content/public/browser/power... content/public/browser/power_profiler_service.h:29: void UpdateResolution(); Why is this method public? My guess is that it is for the case where an observer would need to change its resolution? As this is not needed yet, just put this in the private section. When it is needed, I think removing the observer and re-adding it would be better than exposing those kind of internal details. https://codereview.chromium.org/140583003/diff/1/content/public/browser/power... content/public/browser/power_profiler_service.h:42: PowerProfilerService(); You probably want a second constructor that takes a PowerDataProvider (maybe private), so that you can test this class. And following this could you also write a test for this class.
https://codereview.chromium.org/140583003/diff/1/content/browser/power_profil... File content/browser/power_profiler/power_profiler_service.cc (right): https://codereview.chromium.org/140583003/diff/1/content/browser/power_profil... content/browser/power_profiler/power_profiler_service.cc:41: bool PowerProfilerService::AddObserver(PowerProfilerObserver* obs) { On 2014/01/23 12:50:06, qsr wrote: > I don't think you need a return value here. If someone is interested to know if > using this makes sense, it does have the IsAvailable method. Done. https://codereview.chromium.org/140583003/diff/1/content/browser/power_profil... content/browser/power_profiler/power_profiler_service.cc:66: GetHigherResolution(resolution)); On 2014/01/23 12:50:06, qsr wrote: > I don't really like that each observer needs to implement the same comparaison > code. Can you have it return a value instead? You will have to iterate yourself > over the list and get the maximal value. Done. https://codereview.chromium.org/140583003/diff/1/content/browser/power_profil... content/browser/power_profiler/power_profiler_service.cc:72: if (query_power_timer_.IsRunning()) On 2014/01/23 12:50:06, qsr wrote: > You might want to check on your status, instead of finding that out from the > timer status) Done. https://codereview.chromium.org/140583003/diff/1/content/browser/power_profil... content/browser/power_profiler/power_profiler_service.cc:79: DCHECK (!query_power_timer_.IsRunning()); On 2014/01/23 12:50:06, qsr wrote: > Maybe also check for your status. Done. https://codereview.chromium.org/140583003/diff/1/content/browser/power_profil... content/browser/power_profiler/power_profiler_service.cc:93: if (PROFILING == status_) { On 2014/01/23 12:50:06, qsr wrote: > Why is that a test instead of a DCHECK? Are you expecting stop to be called when > you are not running? Done. https://codereview.chromium.org/140583003/diff/1/content/browser/power_profil... content/browser/power_profiler/power_profiler_service.cc:109: void PowerProfilerService::ProcessData() { On 2014/01/23 12:50:06, qsr wrote: > you could DCHECK that you are on your task_runner. Also maybe rename this > method QueryData instead of ProcessData. And even: > > rename OnTimer QueryData > rename ProcessData QueryDataOnTaskRunner > > and in Start, instead of writing the PostTask, you can call QueryData Done. https://codereview.chromium.org/140583003/diff/1/content/public/browser/power... File content/public/browser/power_data_provider.h (right): https://codereview.chromium.org/140583003/diff/1/content/public/browser/power... content/public/browser/power_data_provider.h:24: base::TimeTicks time; On 2014/01/23 12:50:06, qsr wrote: > Comment on those 2 values, especially the |value| one. What is it, what is the > unit. Done. https://codereview.chromium.org/140583003/diff/1/content/public/browser/power... File content/public/browser/power_profiler_observer.h (right): https://codereview.chromium.org/140583003/diff/1/content/public/browser/power... content/public/browser/power_profiler_observer.h:26: ~PowerProfilerObserver(); On 2014/01/23 12:50:06, qsr wrote: > Where is the implementation of those 2 methods? You probably want those inline > here. Moreover, please the destructor needs to be virtual. Done. https://codereview.chromium.org/140583003/diff/1/content/public/browser/power... File content/public/browser/power_profiler_service.h (right): https://codereview.chromium.org/140583003/diff/1/content/public/browser/power... content/public/browser/power_profiler_service.h:29: void UpdateResolution(); On 2014/01/23 12:50:06, qsr wrote: > Why is this method public? My guess is that it is for the case where an observer > would need to change its resolution? As this is not needed yet, just put this in > the private section. When it is needed, I think removing the observer and > re-adding it would be better than exposing those kind of internal details. right, done https://codereview.chromium.org/140583003/diff/1/content/public/browser/power... content/public/browser/power_profiler_service.h:42: PowerProfilerService(); On 2014/01/23 12:50:06, qsr wrote: > You probably want a second constructor that takes a PowerDataProvider (maybe > private), so that you can test this class. > > And following this could you also write a test for this class. thanks for your help, unit test is prepared, please review.
https://codereview.chromium.org/140583003/diff/120001/content/browser/power_p... File content/browser/power_profiler/power_profiler_service.cc (right): https://codereview.chromium.org/140583003/diff/120001/content/browser/power_p... content/browser/power_profiler/power_profiler_service.cc:16: , data_provider_(PowerDataProviderFactory::Create()) { comma should be on the previous line. https://codereview.chromium.org/140583003/diff/120001/content/browser/power_p... content/browser/power_profiler/power_profiler_service.cc:27: scoped_ptr<PowerDataProvider> provider) I sent you a patch off-review to show exactly what I mean, but you should also pass the task_runner here, as it would help you for test (making them single threaded). https://codereview.chromium.org/140583003/diff/120001/content/browser/power_p... content/browser/power_profiler/power_profiler_service.cc:45: delays_[PowerProfilerObserver::NORMAL] '=' should be on the previous line, more generally you should cut after the operators, not before. https://codereview.chromium.org/140583003/diff/120001/content/browser/power_p... content/browser/power_profiler/power_profiler_service.cc:71: void PowerProfilerService::UpdateResolution() { I've been thinking about resolution again, and I finally found out why I dislike the way it is done here: Suppose I have on observer that needs to be called infrequently, but would do some significant work on each call, let's say around ~50 ms of work. I will define it with a low resolution, but that would not work, because if another observer is registered with a high resolution, I will be called every 50ms and mainly pegged the CPU. So in the current state, even if I ask for a low resolution, I still need to apply my own mechanism to only do my work every few seconds. So if you really want resolution, you need to ensure that a low resolution observer is not called on a high frequency. Another issue is that the max frequency you can attain does not depend on the observer, but in fact depends on the provider. Some provider might only be able to give you result every couple of seconds, so having this run every 50ms is not useful at all. At this point, I would advise to do one of 2 things: 1) Forget about resolution for the moment. Let the service decide of the resolution itself, and right now just have a single 50ms resolution. We will be able to change that to depend on the provider later on when the necessity happens. 2) Have multiple queues for each resolution, and make sure you only call each observer at the right rate. Given that we have a single observer and a single provider right now, I would strongly choose 1 over 2. We will be able to update this for 2 later on if found necessary. If instead you go for 2, then you will also have to update the tests to check you are doing the right thing. https://codereview.chromium.org/140583003/diff/120001/content/browser/power_p... content/browser/power_profiler/power_profiler_service.cc:87: if (PROFILING == status_) Add {} around statement as it is more than one line long. Here and below. https://codereview.chromium.org/140583003/diff/120001/content/browser/power_p... File content/browser/power_profiler/power_profiler_service_unittest.cc (right): https://codereview.chromium.org/140583003/diff/120001/content/browser/power_p... content/browser/power_profiler/power_profiler_service_unittest.cc:14: class PowerProfilerServiceTest : public testing::Test { See off review patch for suggestion on this: mainly do not use another thread, as you can do everything on a single thread by passing a task runner to the ProfilerService. Also, move all the test classes inside this file in a anonymous namespace. Those other classes won't be used by any other class. https://codereview.chromium.org/140583003/diff/120001/content/public/browser/... File content/public/browser/power_data_provider.h (right): https://codereview.chromium.org/140583003/diff/120001/content/public/browser/... content/public/browser/power_data_provider.h:15: enum Type { You should remove Type. Right now you have a single type, so it doesn't give anything. Moreover, your API for the provider doesn't handle multiple type (you are secifying you are getting back a single PowerEvent for each call to the provider), nor does the definition of the |value| field (do I need to keep the last value for each type to be able to get anything, or something else?) So, right now, remove this, and when we have a provider that returns multiple type of power event, we will refactor this to handle it. https://codereview.chromium.org/140583003/diff/120001/content/public/browser/... content/public/browser/power_data_provider.h:26: // Power value between last event to this one, in watt, What does Power value mean? Power consumption might be a better comment. Also specify what the sign means as I guess negative means the device is charging. https://codereview.chromium.org/140583003/diff/120001/content/public/browser/... content/public/browser/power_data_provider.h:34: PowerDataProvider() { } No spaces inside {}, here and everywhere on this CL. https://codereview.chromium.org/140583003/diff/120001/content/test/test_power... File content/test/test_power_data_provider.h (right): https://codereview.chromium.org/140583003/diff/120001/content/test/test_power... content/test/test_power_data_provider.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. No (c) here and all other new files. Also all those test classes should be defined in an anonymous namespace on the Service test itself.
https://codereview.chromium.org/140583003/diff/120001/content/browser/power_p... File content/browser/power_profiler/power_profiler_service.cc (right): https://codereview.chromium.org/140583003/diff/120001/content/browser/power_p... content/browser/power_profiler/power_profiler_service.cc:71: void PowerProfilerService::UpdateResolution() { On 2014/01/27 10:01:47, qsr wrote: > I've been thinking about resolution again, and I finally found out why I dislike > the way it is done here: > > Suppose I have on observer that needs to be called infrequently, but would do > some significant work on each call, let's say around ~50 ms of work. I will > define it with a low resolution, but that would not work, because if another > observer is registered with a high resolution, I will be called every 50ms and > mainly pegged the CPU. So in the current state, even if I ask for a low > resolution, I still need to apply my own mechanism to only do my work every few > seconds. So if you really want resolution, you need to ensure that a low > resolution observer is not called on a high frequency. > > Another issue is that the max frequency you can attain does not depend on the > observer, but in fact depends on the provider. Some provider might only be able > to give you result every couple of seconds, so having this run every 50ms is not > useful at all. > > At this point, I would advise to do one of 2 things: > 1) Forget about resolution for the moment. Let the service decide of the > resolution itself, and right now just have a single 50ms resolution. We will be > able to change that to depend on the provider later on when the necessity > happens. > 2) Have multiple queues for each resolution, and make sure you only call each > observer at the right rate. > > Given that we have a single observer and a single provider right now, I would > strongly choose 1 over 2. We will be able to update this for 2 later on if found > necessary. > > If instead you go for 2, then you will also have to update the tests to check > you are doing the right thing. I've thought it should works similar to Android sensor API, observer request a resolution, while it is a hint to service, the real service should be same or better. I agree with the resolution depends on provider in future, so request is designed to "HIGH" "NORMAL" "LOW" , although actually it is a 50, 200, 4000 internally. Given we don't have too many cases with diff resolution requirement right now, I agree to remove resolution related code in this CL. https://codereview.chromium.org/140583003/diff/120001/content/public/browser/... File content/public/browser/power_data_provider.h (right): https://codereview.chromium.org/140583003/diff/120001/content/public/browser/... content/public/browser/power_data_provider.h:15: enum Type { On 2014/01/27 10:01:47, qsr wrote: > You should remove Type. Right now you have a single type, so it doesn't give > anything. Moreover, your API for the provider doesn't handle multiple type (you > are secifying you are getting back a single PowerEvent for each call to the > provider), nor does the definition of the |value| field (do I need to keep the > last value for each type to be able to get anything, or something else?) > > So, right now, remove this, and when we have a provider that returns multiple > type of power event, we will refactor this to handle it. Yes, it is a little weird to have only one type in the enum. Previously, I left 3 types there, and GetData() aims to fill all 3 type event, but seems not all of them are useful, so only SOC_PACKAGE is provided. I agree to remove Type, while, a little concern is that, the values that provider support is always associated with a type. E.g, currently, SOC_PACKAGE power contains power consumes in SoC, including CPU, GPU etc, but it doesn't contains modules out of SoC, such as screen, so the values doesn't mean "battery". So if we remove the type here, I suggest a comment in the service to say, it is SoC_Package power, when other type added, resume Type, make sense? thanks Pan https://codereview.chromium.org/140583003/diff/120001/content/public/browser/... content/public/browser/power_data_provider.h:26: // Power value between last event to this one, in watt, On 2014/01/27 10:01:47, qsr wrote: > What does Power value mean? Power consumption might be a better comment. Also > specify what the sign means as I guess negative means the device is charging. For SOC_PACKAGE power, it means power consumed in SoC, so it is always a positive one. Scenario of events is: e1 {t1,v1}; e2 {t2, v2}; e3 {t3, v3}; once an observer registered, it got e1 immediately, after a delay_, it is e2, then e3. v2 means the power(watt) between t1 and t2, v3 means power between v2 and t3. For v1, it may mean power between "last time you read" and t1, may mean nothing, so just forget it. I will document this.
https://codereview.chromium.org/140583003/diff/120001/content/browser/power_p... File content/browser/power_profiler/power_profiler_service.cc (right): https://codereview.chromium.org/140583003/diff/120001/content/browser/power_p... content/browser/power_profiler/power_profiler_service.cc:16: , data_provider_(PowerDataProviderFactory::Create()) { On 2014/01/27 10:01:47, qsr wrote: > comma should be on the previous line. thanks, done. https://codereview.chromium.org/140583003/diff/120001/content/browser/power_p... content/browser/power_profiler/power_profiler_service.cc:27: scoped_ptr<PowerDataProvider> provider) On 2014/01/27 10:01:47, qsr wrote: > I sent you a patch off-review to show exactly what I mean, but you should also > pass the task_runner here, as it would help you for test (making them single > threaded). thanks, I missed the one to run a nested loop, it is really helpful. https://codereview.chromium.org/140583003/diff/120001/content/browser/power_p... content/browser/power_profiler/power_profiler_service.cc:45: delays_[PowerProfilerObserver::NORMAL] On 2014/01/27 10:01:47, qsr wrote: > '=' should be on the previous line, more generally you should cut after the > operators, not before. thanks, done https://codereview.chromium.org/140583003/diff/120001/content/browser/power_p... content/browser/power_profiler/power_profiler_service.cc:87: if (PROFILING == status_) On 2014/01/27 10:01:47, qsr wrote: > Add {} around statement as it is more than one line long. Here and below. Done. https://codereview.chromium.org/140583003/diff/120001/content/browser/power_p... File content/browser/power_profiler/power_profiler_service_unittest.cc (right): https://codereview.chromium.org/140583003/diff/120001/content/browser/power_p... content/browser/power_profiler/power_profiler_service_unittest.cc:14: class PowerProfilerServiceTest : public testing::Test { On 2014/01/27 10:01:47, qsr wrote: > See off review patch for suggestion on this: mainly do not use another thread, > as you can do everything on a single thread by passing a task runner to the > ProfilerService. > > Also, move all the test classes inside this file in a anonymous namespace. Those > other classes won't be used by any other class. Done. https://codereview.chromium.org/140583003/diff/120001/content/public/browser/... File content/public/browser/power_data_provider.h (right): https://codereview.chromium.org/140583003/diff/120001/content/public/browser/... content/public/browser/power_data_provider.h:34: PowerDataProvider() { } On 2014/01/27 10:01:47, qsr wrote: > No spaces inside {}, here and everywhere on this CL. Done. https://codereview.chromium.org/140583003/diff/120001/content/test/test_power... File content/test/test_power_data_provider.h (right): https://codereview.chromium.org/140583003/diff/120001/content/test/test_power... content/test/test_power_data_provider.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/01/27 10:01:47, qsr wrote: > No (c) here and all other new files. Also all those test classes should be > defined in an anonymous namespace on the Service test itself. Done.
LGTM with nits. You will now need a LGTM from the right owners. https://codereview.chromium.org/140583003/diff/190001/content/browser/power_p... File content/browser/power_profiler/power_profiler_service.cc (right): https://codereview.chromium.org/140583003/diff/190001/content/browser/power_p... content/browser/power_profiler/power_profiler_service.cc:15: base::TimeDelta::FromMilliseconds(50); This is a static initializer and is not allowed in chromium code. You will need to only defined a constant there, and define the TimeDelta as a non static member of your class. As a bonus, you could add this as a parameter of the test constructor, and have your test run more quickly. https://codereview.chromium.org/140583003/diff/190001/content/browser/power_p... content/browser/power_profiler/power_profiler_service.cc:25: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); You should move the DCHECK before doing anything else. https://codereview.chromium.org/140583003/diff/190001/content/browser/power_p... content/browser/power_profiler/power_profiler_service.cc:86: base::Unretained(this))); base::Unretained should be aligned with &PowerProfilerService: https://codereview.chromium.org/140583003/diff/190001/content/browser/power_p... content/browser/power_profiler/power_profiler_service.cc:100: if(data_provider_->GetData(&event)) { add a space between if and ( https://codereview.chromium.org/140583003/diff/190001/content/public/browser/... File content/public/browser/power_profiler_observer.h (right): https://codereview.chromium.org/140583003/diff/190001/content/public/browser/... content/public/browser/power_profiler_observer.h:9: #include "base/memory/ref_counted.h" You can remove this include. https://codereview.chromium.org/140583003/diff/190001/content/public/browser/... content/public/browser/power_profiler_observer.h:11: #include "content/public/browser/power_data_provider.h" You can also remove this include by just forward declare the PowerEvent struct.
@qsr, thanks for your great help. @darin, @jam, I saw you are the owners of content, could you please help review this CL? thanks Pan https://codereview.chromium.org/140583003/diff/190001/content/browser/power_p... File content/browser/power_profiler/power_profiler_service.cc (right): https://codereview.chromium.org/140583003/diff/190001/content/browser/power_p... content/browser/power_profiler/power_profiler_service.cc:15: base::TimeDelta::FromMilliseconds(50); On 2014/01/28 13:44:15, qsr wrote: > This is a static initializer and is not allowed in chromium code. You will need > to only defined a constant there, and define the TimeDelta as a non static > member of your class. > > As a bonus, you could add this as a parameter of the test constructor, and have > your test run more quickly. thanks, Done. https://codereview.chromium.org/140583003/diff/190001/content/browser/power_p... content/browser/power_profiler/power_profiler_service.cc:25: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); On 2014/01/28 13:44:15, qsr wrote: > You should move the DCHECK before doing anything else. Done. https://codereview.chromium.org/140583003/diff/190001/content/browser/power_p... content/browser/power_profiler/power_profiler_service.cc:86: base::Unretained(this))); On 2014/01/28 13:44:15, qsr wrote: > base::Unretained should be aligned with &PowerProfilerService: Done. https://codereview.chromium.org/140583003/diff/190001/content/browser/power_p... content/browser/power_profiler/power_profiler_service.cc:100: if(data_provider_->GetData(&event)) { On 2014/01/28 13:44:15, qsr wrote: > add a space between if and ( Done. https://codereview.chromium.org/140583003/diff/190001/content/public/browser/... File content/public/browser/power_profiler_observer.h (right): https://codereview.chromium.org/140583003/diff/190001/content/public/browser/... content/public/browser/power_profiler_observer.h:9: #include "base/memory/ref_counted.h" On 2014/01/28 13:44:15, qsr wrote: > You can remove this include. Done. https://codereview.chromium.org/140583003/diff/190001/content/public/browser/... content/public/browser/power_profiler_observer.h:11: #include "content/public/browser/power_data_provider.h" On 2014/01/28 13:44:15, qsr wrote: > You can also remove this include by just forward declare the PowerEvent struct. Done.
Please read http://www.chromium.org/developers/content-module to see what goes into content, and http://www.chromium.org/developers/content-module/content-api to see how an API looks for stuff in content/public. My first question: why is this in content? The callers appear to be in src/chrome, and this isn't part of the web platform. Let's figure out where this should go first, and then we can look closer at the details of the code.
+playmobil
Would appreciate some context on what data the hardware provides and the planned scope of this API :o) Also, I think this is five kinds of awesome! https://codereview.chromium.org/140583003/diff/250001/content/public/browser/... File content/public/browser/power_data_provider.h (right): https://codereview.chromium.org/140583003/diff/250001/content/public/browser/... content/public/browser/power_data_provider.h:16: struct PowerEvent { So this API only provides power usage for the SoC as a whole? Is GPU power draw also something we want to report? Are we interested in reporting idle wakeups, c-state changes, etc as part of this API? Sorry, just trying to understand the scope of this API :) https://codereview.chromium.org/140583003/diff/250001/content/public/browser/... content/public/browser/power_data_provider.h:24: // profiling. Does the hardware measure cumulative power usage or instantaneous power usage?
On 2014/01/28 15:51:51, jam wrote: > Please read http://www.chromium.org/developers/content-module to see what goes > into content, and http://www.chromium.org/developers/content-module/content-api > to see how an API looks for stuff in content/public. > > My first question: why is this in content? The callers appear to be in > src/chrome, and this isn't part of the web platform. > > Let's figure out where this should go first, and then we can look closer at the > details of the code. thanks @jam, devtools is one of the caller, the API can be used for power metrics report in tests on kinds of targets and platform, although this part of work is not done yet. So I think content is the right place. Pan
https://codereview.chromium.org/140583003/diff/250001/content/public/browser/... File content/public/browser/power_data_provider.h (right): https://codereview.chromium.org/140583003/diff/250001/content/public/browser/... content/public/browser/power_data_provider.h:16: struct PowerEvent { On 2014/01/28 19:45:43, jeremy wrote: > So this API only provides power usage for the SoC as a whole? > Is GPU power draw also something we want to report? > Are we interested in reporting idle wakeups, c-state changes, etc as part of > this API? > > Sorry, just trying to understand the scope of this API :) @jeremy, you are welcome, this CL is a start, GPU power type can be there once there is a need, we also can extend it to provide wakeups and state changes. https://codereview.chromium.org/140583003/diff/250001/content/public/browser/... content/public/browser/power_data_provider.h:24: // profiling. On 2014/01/28 19:45:43, jeremy wrote: > Does the hardware measure cumulative power usage or instantaneous power usage? it is a 32bit MSR on hardware, register updated per 1 millisecond by new consumed energy. see 14.7.3 if you are insterested :) http://www.intel.com/content/dam/www/public/us/en/documents/manuals/64-ia-32-...
On 2014/01/29 00:29:22, Pan wrote: > https://codereview.chromium.org/140583003/diff/250001/content/public/browser/... > File content/public/browser/power_data_provider.h (right): > > https://codereview.chromium.org/140583003/diff/250001/content/public/browser/... > content/public/browser/power_data_provider.h:16: struct PowerEvent { > On 2014/01/28 19:45:43, jeremy wrote: > > So this API only provides power usage for the SoC as a whole? > > Is GPU power draw also something we want to report? > > Are we interested in reporting idle wakeups, c-state changes, etc as part of > > this API? > > > > Sorry, just trying to understand the scope of this API :) > > @jeremy, you are welcome, this CL is a start, GPU power type can be there once > there is a need, we also can extend it to provide wakeups and state changes. > > https://codereview.chromium.org/140583003/diff/250001/content/public/browser/... > content/public/browser/power_data_provider.h:24: // profiling. > On 2014/01/28 19:45:43, jeremy wrote: > > Does the hardware measure cumulative power usage or instantaneous power usage? > > it is a 32bit MSR on hardware, register updated per 1 millisecond by new > consumed energy. > see 14.7.3 if you are insterested :) > http://www.intel.com/content/dam/www/public/us/en/documents/manuals/64-ia-32-... This is the implementation from power gadget. At this level you are designing an API for all architecture, so the definition should not rely on a specific implementation.
https://chromiumcodereview.appspot.com/140583003/diff/250001/content/public/b... File content/public/browser/power_profiler_observer.h (right): https://chromiumcodereview.appspot.com/140583003/diff/250001/content/public/b... content/public/browser/power_profiler_observer.h:20: virtual void OnPowerEvent(const PowerEvent&) = 0; You must forward declare PowerEvent, otherwise this cannot compile by itself. https://chromiumcodereview.appspot.com/140583003/diff/250001/content/public/b... File content/public/browser/power_profiler_service.h (right): https://chromiumcodereview.appspot.com/140583003/diff/250001/content/public/b... content/public/browser/power_profiler_service.h:45: base::TimeDelta delay); const base::TimeDelta&
On 2014/01/28 19:45:42, jeremy wrote: > Would appreciate some context on what data the hardware provides and the planned > scope of this API :o) > > Also, I think this is five kinds of awesome! > > https://codereview.chromium.org/140583003/diff/250001/content/public/browser/... > File content/public/browser/power_data_provider.h (right): > > https://codereview.chromium.org/140583003/diff/250001/content/public/browser/... > content/public/browser/power_data_provider.h:16: struct PowerEvent { > So this API only provides power usage for the SoC as a whole? > Is GPU power draw also something we want to report? > Are we interested in reporting idle wakeups, c-state changes, etc as part of > this API? The only implementation that was ready when designing this was only sending SoC information, so we simplify the API. As you did look into the OSX API, if you have way to retrieve more information, then we should improve this API to be able to report it. > > Sorry, just trying to understand the scope of this API :) > > https://codereview.chromium.org/140583003/diff/250001/content/public/browser/... > content/public/browser/power_data_provider.h:24: // profiling. > Does the hardware measure cumulative power usage or instantaneous power usage?
I assume that Maverick's powermetrics utility is providing data using the same mechanism you are using? powermetrics provides very granular power usage data for every part of the package. I discussed with tony and I think we'd like to have the api support providing the most granular power metrics we can from the start esp. the GPU power if it's readily available. Perhaps add an enum to the power data which signifies which component it's for?
On 2014/01/29 20:55:22, jeremy wrote: > I assume that Maverick's powermetrics utility is providing data using the same > mechanism you are using? powermetrics provides very granular power usage data > for every part of the package. > > I discussed with tony and I think we'd like to have the api support providing > the most granular power metrics we can from the start esp. the GPU power if it's > readily available. > > Perhaps add an enum to the power data which signifies which component it's for? Hi @jeremy, We have kinds of tools that provide granular power metrics(not Intel PowerGadget), we can bring them in if API is exposed and if it is suitable. It may take some time, so I suggest we support granular metrics in future CL. As qsr's reply, event type is simplified because currently it is for sending SoC power information. type to support GPU can be added when we bring in Power Gadget Library. One thing to note is that GPU power usage is only available on part of platforms. In addition, due to Chinese New Year, I will be out of office in next 1~2 weeks, with limited mail check, thanks for your help and patience. Pan thanks Pan
https://codereview.chromium.org/140583003/diff/250001/content/public/browser/... File content/public/browser/power_profiler_observer.h (right): https://codereview.chromium.org/140583003/diff/250001/content/public/browser/... content/public/browser/power_profiler_observer.h:20: virtual void OnPowerEvent(const PowerEvent&) = 0; On 2014/01/29 14:26:52, qsr wrote: > You must forward declare PowerEvent, otherwise this cannot compile by itself. Done. https://codereview.chromium.org/140583003/diff/250001/content/public/browser/... File content/public/browser/power_profiler_service.h (right): https://codereview.chromium.org/140583003/diff/250001/content/public/browser/... content/public/browser/power_profiler_service.h:45: base::TimeDelta delay); On 2014/01/29 14:26:52, qsr wrote: > const base::TimeDelta& Done.
On 2014/01/28 23:52:37, Pan wrote: > On 2014/01/28 15:51:51, jam wrote: > > Please read http://www.chromium.org/developers/content-module to see what goes > > into content, and > http://www.chromium.org/developers/content-module/content-api > > to see how an API looks for stuff in content/public. > > > > My first question: why is this in content? The callers appear to be in > > src/chrome, and this isn't part of the web platform. > > > > Let's figure out where this should go first, and then we can look closer at > the > > details of the code. > > thanks @jam, > devtools is one of the caller, the API can be used for power metrics report in > tests > on kinds of targets and platform, although this part of work is not done yet. So > I > think content is the right place. > > Pan It's hard to know what you're thinking of in the future. Are you saying tests will be in the content layer, or written against the content layer? It's really not obvious to me why this should be in content.
On 2014/01/31 17:43:36, jam wrote: > On 2014/01/28 23:52:37, Pan wrote: > > On 2014/01/28 15:51:51, jam wrote: > > > Please read http://www.chromium.org/developers/content-module to see what > goes > > > into content, and > > http://www.chromium.org/developers/content-module/content-api > > > to see how an API looks for stuff in content/public. > > > > > > My first question: why is this in content? The callers appear to be in > > > src/chrome, and this isn't part of the web platform. > > > > > > Let's figure out where this should go first, and then we can look closer at > > the > > > details of the code. > > > > thanks @jam, > > devtools is one of the caller, the API can be used for power metrics report in > > tests > > on kinds of targets and platform, although this part of work is not done yet. > So > > I > > think content is the right place. > > > > Pan > > It's hard to know what you're thinking of in the future. Are you saying tests > will be in the content layer, or written against the content layer? > > It's really not obvious to me why this should be in content. We would like this to be in content, because this is the API on which we will build power reporting in dev tools. And we would like to have this reporting on the content shell. This is the same reason we have content/browser/devtools/*
> We would like this to be in content, because this is the API on which we will > build power reporting in dev tools. And we would like to have this reporting on > the content shell. This is the same reason we have content/browser/devtools/* I don't think this is a fair argument. content/browser/devtools/ implements minimal necessary infrastructure for wiring devtools instrumentation to its clients. It is a framework, not the actual instrumentation. // It does implement some of the remote debugging protocol commands (navigation, history, quota management), but that's because they need to be implemented on the browser level due to security constraints + they already operate content terms and are used by all content/ embedders. We could expose content/ API for registering remote debugging protocol handlers that are external to content/. The question is whether we expect this instrumentation to apply to all (most) content/ embedders or not.
On 2014/02/03 14:50:19, pfeldman wrote: > > We would like this to be in content, because this is the API on which we will > > build power reporting in dev tools. And we would like to have this reporting > on > > the content shell. This is the same reason we have content/browser/devtools/* > > I don't think this is a fair argument. content/browser/devtools/ implements > minimal necessary infrastructure for wiring devtools instrumentation to its > clients. It is a framework, not the actual instrumentation. > // It does implement some of the remote debugging protocol commands (navigation, > history, quota management), but that's because they need to be implemented on > the browser level due to security constraints + they already operate content > terms and are used by all content/ embedders. Hum sorry, I misinterpreted the role of devtools_system_info_handler.cc. > > We could expose content/ API for registering remote debugging protocol handlers > that are external to content/. The question is whether we expect this > instrumentation to apply to all (most) content/ embedders or not. We were hoping to be able to get power usage information for the content shell. But I thought that most of the dev tools were available on the content shell, and it seems I was wrong, so we will probably start by putting this in chrome, and revisit later on if necessary.
> We were hoping to be able to get power usage information for the content shell. > But I thought that most of the dev tools were available on the content shell, Pretty much all the instrumentation is in Blink, embedded by content. Hence, available to content shell. > and it seems I was wrong, so we will probably start by putting this in chrome, > and revisit later on if necessary. This will be the first instrumentation outside content/, so it will need some additional hooks in the API. It is not clear to me whether content/ should expose (a) APIs for external devtools protocol handlers or (b) allow pluggable power info providers If we think this will make it into Chrome for Android in some form and would like to stick with the same remote debugging API for it, we should do (b).
On 2014/02/03 16:33:29, pfeldman wrote: > > We were hoping to be able to get power usage information for the content > shell. > > But I thought that most of the dev tools were available on the content shell, > > Pretty much all the instrumentation is in Blink, embedded by content. Hence, > available to content shell. > > > and it seems I was wrong, so we will probably start by putting this in chrome, > > and revisit later on if necessary. > > This will be the first instrumentation outside content/, so it will need some > additional hooks in the API. It is not clear to me whether content/ should > expose > (a) APIs for external devtools protocol handlers or > (b) allow pluggable power info providers > > If we think this will make it into Chrome for Android in some form and would > like to stick with the same remote debugging API for it, we should do (b). Sorry, I am missing a point there. What do you mean by pluggable power info provider? This CL already allow platform specific power info provider (and even having multiple provider for a given platform with a priority depending on what is accessible), this is the reason for the PowerDataProviderFactory. After that, if all intrumentation is in Blink, embedded by content, that would be an argument for the power info to be also available in content.
> Sorry, I am missing a point there. What do you mean by pluggable power info > provider? This CL already allow platform specific power info provider (and even > having multiple provider for a given platform with a priority depending on what > is accessible), this is the reason for the PowerDataProviderFactory. > > After that, if all intrumentation is in Blink, embedded by content, that would > be an argument for the power info to be also available in content. Sorry, yes, (b) is pretty much this patch. Except for I would bake power_profiler_service.cc right into the devtools handler (devtools_power_handler that is similar to devtools_tracing_handler). We don't anticipate non-devtools clients of this service in content anyways, do we? Simple handler with the start/stop protocol that emits events with power data in json-serialized form would suffice.
On 2014/02/03 16:51:40, pfeldman wrote: > > Sorry, I am missing a point there. What do you mean by pluggable power info > > provider? This CL already allow platform specific power info provider (and > even > > having multiple provider for a given platform with a priority depending on > what > > is accessible), this is the reason for the PowerDataProviderFactory. > > > > After that, if all intrumentation is in Blink, embedded by content, that > would > > be an argument for the power info to be also available in content. > > Sorry, yes, (b) is pretty much this patch. Except for I would bake > power_profiler_service.cc right into the devtools handler > (devtools_power_handler that is similar to devtools_tracing_handler). We don't > anticipate non-devtools clients of this service in content anyways, do we? > Simple handler with the start/stop protocol that emits events with power data in > json-serialized form would suffice. thanks @pfeldman and @qsr, power related is removed from content API, and devtools_power_handler added. I guess we may have multiple devtools_power_handler instances, if it is true, dose it make sense to keep the singleton PowerProfilerService? thanks Pan
https://codereview.chromium.org/140583003/diff/380001/content/browser/power_p... File content/browser/power_profiler/power_data_provider.h (right): https://codereview.chromium.org/140583003/diff/380001/content/browser/power_p... content/browser/power_profiler/power_data_provider.h:25: double value; See jeremy@ message, we probably want an enum there. The question then is whether we want to received one data point at a time, or multiple data point on a single event.
https://codereview.chromium.org/140583003/diff/380001/content/browser/power_p... File content/browser/power_profiler/power_data_provider.h (right): https://codereview.chromium.org/140583003/diff/380001/content/browser/power_p... content/browser/power_profiler/power_data_provider.h:25: double value; On 2014/02/10 14:02:49, qsr wrote: > See jeremy@ message, we probably want an enum there. The question then is > whether we want to received one data point at a time, or multiple data point on > a single event. done. IMHO, we should provide per data here, and allow its client to filter/buffer the events, any idea? Pan
On 2014/02/11 02:03:22, Pan wrote: > https://codereview.chromium.org/140583003/diff/380001/content/browser/power_p... > File content/browser/power_profiler/power_data_provider.h (right): > > https://codereview.chromium.org/140583003/diff/380001/content/browser/power_p... > content/browser/power_profiler/power_data_provider.h:25: double value; > On 2014/02/10 14:02:49, qsr wrote: > > See jeremy@ message, we probably want an enum there. The question then is > > whether we want to received one data point at a time, or multiple data point > on > > a single event. > > done. I'm not sure this change is sufficient. How can the provider provide more than one type of event, given that you are polling it. I don't have a problem with only polling for an event at a time, but then you need to send the type to the provider. And for this to make sense, the provider have to be able to tell you what type it is susceptible to send back. Another solution is to have the provider send a set of event, and then you can dispatch those one at a time to listeners. > IMHO, we should provide per data here, and allow its client > to filter/buffer the events, any idea? > > Pan
On 2014/02/11 09:04:21, qsr wrote: > On 2014/02/11 02:03:22, Pan wrote: > > > https://codereview.chromium.org/140583003/diff/380001/content/browser/power_p... > > File content/browser/power_profiler/power_data_provider.h (right): > > > > > https://codereview.chromium.org/140583003/diff/380001/content/browser/power_p... > > content/browser/power_profiler/power_data_provider.h:25: double value; > > On 2014/02/10 14:02:49, qsr wrote: > > > See jeremy@ message, we probably want an enum there. The question then is > > > whether we want to received one data point at a time, or multiple data point > > on > > > a single event. > > > > done. > > I'm not sure this change is sufficient. How can the provider provide more than > one type of event, given that you are polling it. > > I don't have a problem with only polling for an event at a time, but then you > need to send the type to the provider. And for this to make sense, the provider > have to be able to tell you what type it is susceptible to send back. > > Another solution is to have the provider send a set of event, and then you can > dispatch those one at a time to listeners. To support multi types, the first solution is good, but seems there won't be too many power types. So I prefer 2nd one, GetData() in Provider fill all type events in an array and notify, then observer could choose its own type in OnPowerEvent(), there won't be too much as overhead, especially given only one type for now, right? Pan
On 2014/02/11 09:53:49, Pan wrote: > On 2014/02/11 09:04:21, qsr wrote: > > On 2014/02/11 02:03:22, Pan wrote: > > > > > > https://codereview.chromium.org/140583003/diff/380001/content/browser/power_p... > > > File content/browser/power_profiler/power_data_provider.h (right): > > > > > > > > > https://codereview.chromium.org/140583003/diff/380001/content/browser/power_p... > > > content/browser/power_profiler/power_data_provider.h:25: double value; > > > On 2014/02/10 14:02:49, qsr wrote: > > > > See jeremy@ message, we probably want an enum there. The question then is > > > > whether we want to received one data point at a time, or multiple data > point > > > on > > > > a single event. > > > > > > done. > > > > I'm not sure this change is sufficient. How can the provider provide more > than > > one type of event, given that you are polling it. > > > > I don't have a problem with only polling for an event at a time, but then you > > need to send the type to the provider. And for this to make sense, the > provider > > have to be able to tell you what type it is susceptible to send back. > > > > Another solution is to have the provider send a set of event, and then you > can > > dispatch those one at a time to listeners. > > To support multi types, the first solution is good, but seems there won't be too > many power types. > So I prefer 2nd one, GetData() in Provider fill all type events in an array and > notify, then observer could choose its own type in OnPowerEvent(), there won't > be too much as overhead, especially given only one type for now, right? Yes, that would work too, but you were saying you still wanted one type per event. With multiple type per event, you can start with an array of measurement, one per type, but you need a way to distinguish when a type is not present. You will either need a marker value for this, or another way to inspect with types are present. > > Pan
On 2014/02/11 10:20:00, qsr wrote: > On 2014/02/11 09:53:49, Pan wrote: > > On 2014/02/11 09:04:21, qsr wrote: > > > On 2014/02/11 02:03:22, Pan wrote: > > > > > > > > > > https://codereview.chromium.org/140583003/diff/380001/content/browser/power_p... > > > > File content/browser/power_profiler/power_data_provider.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/140583003/diff/380001/content/browser/power_p... > > > > content/browser/power_profiler/power_data_provider.h:25: double value; > > > > On 2014/02/10 14:02:49, qsr wrote: > > > > > See jeremy@ message, we probably want an enum there. The question then > is > > > > > whether we want to received one data point at a time, or multiple data > > point > > > > on > > > > > a single event. > > > > > > > > done. > > > > > > I'm not sure this change is sufficient. How can the provider provide more > > than > > > one type of event, given that you are polling it. > > > > > > I don't have a problem with only polling for an event at a time, but then > you > > > need to send the type to the provider. And for this to make sense, the > > provider > > > have to be able to tell you what type it is susceptible to send back. > > > > > > Another solution is to have the provider send a set of event, and then you > > can > > > dispatch those one at a time to listeners. > > > > To support multi types, the first solution is good, but seems there won't be > too > > many power types. > > So I prefer 2nd one, GetData() in Provider fill all type events in an array > and > > notify, then observer could choose its own type in OnPowerEvent(), there won't > > be too much as overhead, especially given only one type for now, right? > > Yes, that would work too, but you were saying you still wanted one type per > event. With multiple type per event, you can start with an array of measurement, > one per type, but you need a way to distinguish when a type is not present. You > will either need a marker value for this, or another way to inspect with types > are present. thanks, I mean provider fills events into array(it should be vector) due to its capability. E.g. a SoC and GPU provider fetches the [{type:SOC_PACKAGE, time:t1, value:1.0}, {type:GPU, time:t1, value:0.2}], while SoC only provider fetches [{type:SOC_PACKAGE, time:t2, value:1.0}]. "time" are duplicated in multi-type case, but I think it is acceptable. ok?
> thanks, I mean provider fills events into array(it should be vector) due to
its
> capability.
> E.g. a SoC and GPU provider fetches the [{type:SOC_PACKAGE, time:t1,
value:1.0},
> {type:GPU, time:t1, value:0.2}],
> while SoC only provider fetches [{type:SOC_PACKAGE, time:t2, value:1.0}].
> "time" are duplicated in multi-type case, but I think it is acceptable. ok?
patch set 9 should address what I mean, please review, thanks
Pan
https://codereview.chromium.org/140583003/diff/590001/content/browser/devtool... File content/browser/devtools/devtools_power_handler.cc (right): https://codereview.chromium.org/140583003/diff/590001/content/browser/devtool... content/browser/devtools/devtools_power_handler.cc:36: if (iter->type != PowerEvent::SOC_PACKAGE) Why only SOC_PACKAGE? Some implementation won't be able to get soc package, and will only be able to get the information on the device level. Why are you only considering soc here?
https://codereview.chromium.org/140583003/diff/590001/content/browser/devtool... File content/browser/devtools/devtools_power_handler.cc (right): https://codereview.chromium.org/140583003/diff/590001/content/browser/devtool... content/browser/devtools/devtools_power_handler.cc:36: if (iter->type != PowerEvent::SOC_PACKAGE) On 2014/02/11 15:51:13, qsr wrote: > Why only SOC_PACKAGE? Some implementation won't be able to get soc package, and > will only be able to get the information on the device level. Why are you only > considering soc here? oh, sorry, I was thinking add other types here when new types comes. Now, a kPowerTypeNames is defined with PowerEvent, we can just update there in future.
devtools part looks reasonable to me. https://codereview.chromium.org/140583003/diff/450001/content/browser/devtool... File content/browser/devtools/devtools_protocol_constants.cc (right): https://codereview.chromium.org/140583003/diff/450001/content/browser/devtool... content/browser/devtools/devtools_protocol_constants.cc:232: const char kName[] = "Power.dataReceived"; dataReceived is not protocol-user-friendly. I'd rather call it dataAvailable
https://codereview.chromium.org/140583003/diff/450001/content/browser/devtool... File content/browser/devtools/devtools_protocol_constants.cc (right): https://codereview.chromium.org/140583003/diff/450001/content/browser/devtool... content/browser/devtools/devtools_protocol_constants.cc:232: const char kName[] = "Power.dataReceived"; On 2014/02/12 09:08:51, pfeldman wrote: > dataReceived is not protocol-user-friendly. I'd rather call it dataAvailable done. thanks!
Hi @jeremy,@qsr, Any more comments? thanks for your help. Pan
LGTM with nits. https://codereview.chromium.org/140583003/diff/760001/content/browser/devtool... File content/browser/devtools/devtools_power_handler.cc (right): https://codereview.chromium.org/140583003/diff/760001/content/browser/devtool... content/browser/devtools/devtools_power_handler.cc:41: convertMonotonicTimeToWallTime(iter->time).ToDoubleT() * 1000.0); Any reason for this to be in ms, instead of second? Especially when webkit itself is using seconds? https://codereview.chromium.org/140583003/diff/760001/content/browser/devtool... content/browser/devtools/devtools_power_handler.cc:72: scoped_refptr<DevToolsProtocol::Command> command) { This fits on a singe line. https://codereview.chromium.org/140583003/diff/760001/content/browser/devtool... File content/browser/devtools/devtools_power_handler.h (right): https://codereview.chromium.org/140583003/diff/760001/content/browser/devtool... content/browser/devtools/devtools_power_handler.h:29: base::Time convertMonotonicTimeToWallTime(const base::TimeTicks&); This probably can be an anonymous inlined function in the implementation file. In the end this is just a couple of addition/substraction. https://codereview.chromium.org/140583003/diff/760001/content/browser/power_p... File content/browser/power_profiler/power_data_provider.h (right): https://codereview.chromium.org/140583003/diff/760001/content/browser/power_p... content/browser/power_profiler/power_data_provider.h:19: // modules out of SoC such as screen is not included. Can you add a type of the whole device consumption. Will be useful for mobile devices. https://codereview.chromium.org/140583003/diff/760001/content/browser/power_p... File content/browser/power_profiler/power_data_provider_factory.cc (right): https://codereview.chromium.org/140583003/diff/760001/content/browser/power_p... content/browser/power_profiler/power_data_provider_factory.cc:3: // found in the LICENSE file. If you have the implementation in a separate file, you might want to also have a specific header. https://codereview.chromium.org/140583003/diff/760001/content/browser/power_p... File content/browser/power_profiler/power_profiler_observer.h (right): https://codereview.chromium.org/140583003/diff/760001/content/browser/power_p... content/browser/power_profiler/power_profiler_observer.h:24: // This method should be called on UI thread s/should/will be https://codereview.chromium.org/140583003/diff/760001/content/browser/power_p... File content/browser/power_profiler/power_profiler_service.h (right): https://codereview.chromium.org/140583003/diff/760001/content/browser/power_p... content/browser/power_profiler/power_profiler_service.h:43: explicit PowerProfilerService(scoped_ptr<PowerDataProvider> provider, explicit is only useful if the constructor has a single argument. https://codereview.chromium.org/140583003/diff/760001/content/browser/power_p... File content/browser/power_profiler/power_type_names.cc (right): https://codereview.chromium.org/140583003/diff/760001/content/browser/power_p... content/browser/power_profiler/power_type_names.cc:5: #include "content/browser/power_profiler/power_data_provider.h" Rename this file content/browser/power_profiler/power_data_provider.cc so that the implementation corresponds to the declaration. https://codereview.chromium.org/140583003/diff/760001/content/browser/power_p... content/browser/power_profiler/power_type_names.cc:10: "SoC_Package" Can you write a compile time assertion that the number of element in this array is the same as the number of enum declared in PowerType?
https://codereview.chromium.org/140583003/diff/760001/content/browser/devtool... File content/browser/devtools/devtools_power_handler.cc (right): https://codereview.chromium.org/140583003/diff/760001/content/browser/devtool... content/browser/devtools/devtools_power_handler.cc:41: convertMonotonicTimeToWallTime(iter->time).ToDoubleT() * 1000.0); On 2014/02/13 16:40:13, qsr wrote: > Any reason for this to be in ms, instead of second? Especially when webkit > itself is using seconds? I don't have strong preference, the reason I used ms here is to align with other events that InspectorTimelineAgent send out(startTime and endTime is in ms). anyway, we should described it in protocol. https://codereview.chromium.org/140583003/diff/760001/content/browser/devtool... content/browser/devtools/devtools_power_handler.cc:72: scoped_refptr<DevToolsProtocol::Command> command) { On 2014/02/13 16:40:13, qsr wrote: > This fits on a singe line. Done. https://codereview.chromium.org/140583003/diff/760001/content/browser/devtool... File content/browser/devtools/devtools_power_handler.h (right): https://codereview.chromium.org/140583003/diff/760001/content/browser/devtool... content/browser/devtools/devtools_power_handler.h:29: base::Time convertMonotonicTimeToWallTime(const base::TimeTicks&); On 2014/02/13 16:40:13, qsr wrote: > This probably can be an anonymous inlined function in the implementation file. > In the end this is just a couple of addition/substraction. done, thanks https://codereview.chromium.org/140583003/diff/760001/content/browser/power_p... File content/browser/power_profiler/power_data_provider.h (right): https://codereview.chromium.org/140583003/diff/760001/content/browser/power_p... content/browser/power_profiler/power_data_provider.h:19: // modules out of SoC such as screen is not included. On 2014/02/13 16:40:13, qsr wrote: > Can you add a type of the whole device consumption. Will be useful for mobile > devices. Done. https://codereview.chromium.org/140583003/diff/760001/content/browser/power_p... File content/browser/power_profiler/power_data_provider_factory.cc (right): https://codereview.chromium.org/140583003/diff/760001/content/browser/power_p... content/browser/power_profiler/power_data_provider_factory.cc:3: // found in the LICENSE file. On 2014/02/13 16:40:13, qsr wrote: > If you have the implementation in a separate file, you might want to also have a > specific header. Done. https://codereview.chromium.org/140583003/diff/760001/content/browser/power_p... File content/browser/power_profiler/power_profiler_observer.h (right): https://codereview.chromium.org/140583003/diff/760001/content/browser/power_p... content/browser/power_profiler/power_profiler_observer.h:24: // This method should be called on UI thread On 2014/02/13 16:40:13, qsr wrote: > s/should/will be Done. https://codereview.chromium.org/140583003/diff/760001/content/browser/power_p... File content/browser/power_profiler/power_profiler_service.h (right): https://codereview.chromium.org/140583003/diff/760001/content/browser/power_p... content/browser/power_profiler/power_profiler_service.h:43: explicit PowerProfilerService(scoped_ptr<PowerDataProvider> provider, On 2014/02/13 16:40:13, qsr wrote: > explicit is only useful if the constructor has a single argument. right, I forgot to remove it, thanks. https://codereview.chromium.org/140583003/diff/760001/content/browser/power_p... File content/browser/power_profiler/power_type_names.cc (right): https://codereview.chromium.org/140583003/diff/760001/content/browser/power_p... content/browser/power_profiler/power_type_names.cc:5: #include "content/browser/power_profiler/power_data_provider.h" On 2014/02/13 16:40:13, qsr wrote: > Rename this file content/browser/power_profiler/power_data_provider.cc so that > the implementation corresponds to the declaration. Done. https://codereview.chromium.org/140583003/diff/760001/content/browser/power_p... content/browser/power_profiler/power_type_names.cc:10: "SoC_Package" On 2014/02/13 16:40:13, qsr wrote: > Can you write a compile time assertion that the number of element in this array > is the same as the number of enum declared in PowerType? Done.
https://codereview.chromium.org/140583003/diff/940001/content/browser/devtool... File content/browser/devtools/devtools_power_handler.cc (right): https://codereview.chromium.org/140583003/diff/940001/content/browser/devtool... content/browser/devtools/devtools_power_handler.cc:51: params->Set(devtools::Power::dataAvailable::kParamValue, event_list); Never format events manually. https://codereview.chromium.org/140583003/diff/940001/content/browser/devtool... content/browser/devtools/devtools_power_handler.cc:56: SendRawMessage(json_string); Please use SendNotification instead. https://codereview.chromium.org/140583003/diff/940001/content/browser/devtool... File content/browser/devtools/devtools_power_handler.h (right): https://codereview.chromium.org/140583003/diff/940001/content/browser/devtool... content/browser/devtools/devtools_power_handler.h:14: class DevToolsPowerHandler Please define schema for your new domain in browser_protocol.json
@pfeldman and @jeremy, If no more comments, could you please grant a LGTM? :) thanks Pan https://codereview.chromium.org/140583003/diff/940001/content/browser/devtool... File content/browser/devtools/devtools_power_handler.cc (right): https://codereview.chromium.org/140583003/diff/940001/content/browser/devtool... content/browser/devtools/devtools_power_handler.cc:51: params->Set(devtools::Power::dataAvailable::kParamValue, event_list); On 2014/02/14 08:11:09, pfeldman wrote: > Never format events manually. thanks, done https://codereview.chromium.org/140583003/diff/940001/content/browser/devtool... content/browser/devtools/devtools_power_handler.cc:56: SendRawMessage(json_string); On 2014/02/14 08:11:09, pfeldman wrote: > Please use SendNotification instead. thanks, done
devtools lgtm
Nice! Some remarks, mostly small things around polish. Please make sure that: * Constants are on RHS in comparisons. * 2 spaces before // in line comment. * First letter of comment is capitalized with '.' at end, no undue capitalization. * Unittest looks like it may be flaky due to timeout, I might be wrong in which case I'll shut up :) https://codereview.chromium.org/140583003/diff/1120001/content/browser/devtoo... File content/browser/devtools/devtools_power_handler.h (right): https://codereview.chromium.org/140583003/diff/1120001/content/browser/devtoo... content/browser/devtools/devtools_power_handler.h:14: class DevToolsPowerHandler Can you please add a class level comment and one for the OnPowerEvent() method. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... File content/browser/power_profiler/power_data_provider.h (right): https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_data_provider.h:18: // modules out of SoC such as screen is not included. nit: modules which aren't part of the SoC such as the screen are not included. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_data_provider.h:24: // Count the number of known PowerEvent. nit: // Keep this at the end. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_data_provider.h:34: // Suppose event1 is the first event observer received, then event2, event3. nit: the observer https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_data_provider.h:35: // Then v2 is average power from t1 to t2, v3 is the average power from t2 to nit: the average https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_data_provider.h:45: // a class used to GET power usage. nit: s/a/A/ nit: lowercase get ? https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_data_provider.h:52: // due to the type it supports. nit: 'Returns' , s/due/for/ https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... File content/browser/power_profiler/power_profiler_observer.h (right): https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_profiler_observer.h:18: // A class used to monitor the power usage and tell profiler the power data nit: . at end of comment. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_profiler_observer.h:24: // This method will be called on UI thread nit: the UI thread. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... File content/browser/power_profiler/power_profiler_service.cc (right): https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_profiler_service.cc:17: delay_(base::TimeDelta::FromMilliseconds(50)), Where does this value come from? Recommend initializing from a file constant with a comment on where this number came from. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_profiler_service.cc:24: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); IMHO this should be at the start of the function. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_profiler_service.cc:49: void PowerProfilerService::AddObserver(PowerProfilerObserver* obs) { obs -> observer https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_profiler_service.cc:50: if (UNINITIALIZED == status_) status_ == UNINITALIZED constant on rhs in comparisons below as well. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_profiler_service.cc:69: // send out power events immediately. Send https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_profiler_service.cc:79: // stop timer, set status to INITIALIZED I think this comment is redundant. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_profiler_service.cc:99: // Get Data and Notify . at end of comment. Data and Notify shouldn't be capitalized. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... File content/browser/power_profiler/power_profiler_service.h (right): https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_profiler_service.h:19: // a class used to monitor the power usage and tell profiler the power data. nit: s/a/A/ https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_profiler_service.h:32: UNINITIALIZED, // Uninitialized. coding style is 2 space separation for line comments, here and elsewhere in the cl. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_profiler_service.h:44: scoped_refptr<base::TaskRunner> task_runner, I think the indentation here is off, I'd line these up with the upper 'scoped' but other reviewers opinions may differ. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_profiler_service.h:51: void QueryData(); Function comments for these. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_profiler_service.h:53: // Notify works in UI thread. // Executes on the UI thread. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_profiler_service.h:62: const base::TimeDelta delay_; From the comment, it sounds like this should be renamed to sample_period_ ? https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... File content/browser/power_profiler/power_profiler_service_unittest.cc (right): https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_profiler_service_unittest.cc:15: const int kEvents = 3; kNumEvents, kNumObservers ? https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_profiler_service_unittest.cc:82: // No one received PowerEvent now. // No PowerEvents received. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_profiler_service_unittest.cc:117: TEST_F(PowerProfilerServiceTest, AvailableService) { Could you add a comment on what this is supposed to test. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_profiler_service_unittest.cc:120: RunLoopForDelay(base::TimeDelta::FromMilliseconds(20)); Is there a better way to wait here? Using a timeout can be flaky?...
thanks @jeremy, your help is very appreciated! :) Pan https://codereview.chromium.org/140583003/diff/1120001/content/browser/devtoo... File content/browser/devtools/devtools_power_handler.h (right): https://codereview.chromium.org/140583003/diff/1120001/content/browser/devtoo... content/browser/devtools/devtools_power_handler.h:14: class DevToolsPowerHandler On 2014/02/16 14:09:19, jeremy wrote: > Can you please add a class level comment and one for the OnPowerEvent() method. Done. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... File content/browser/power_profiler/power_data_provider.h (right): https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_data_provider.h:18: // modules out of SoC such as screen is not included. On 2014/02/16 14:09:19, jeremy wrote: > nit: modules which aren't part of the SoC such as the screen are not included. Done. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_data_provider.h:24: // Count the number of known PowerEvent. On 2014/02/16 14:09:19, jeremy wrote: > nit: // Keep this at the end. Done. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_data_provider.h:34: // Suppose event1 is the first event observer received, then event2, event3. On 2014/02/16 14:09:19, jeremy wrote: > nit: the observer Done. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_data_provider.h:35: // Then v2 is average power from t1 to t2, v3 is the average power from t2 to On 2014/02/16 14:09:19, jeremy wrote: > nit: the average Done. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_data_provider.h:45: // a class used to GET power usage. On 2014/02/16 14:09:19, jeremy wrote: > nit: s/a/A/ > nit: lowercase get ? Done. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_data_provider.h:52: // due to the type it supports. On 2014/02/16 14:09:19, jeremy wrote: > nit: 'Returns' , s/due/for/ Done. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... File content/browser/power_profiler/power_profiler_observer.h (right): https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_profiler_observer.h:18: // A class used to monitor the power usage and tell profiler the power data On 2014/02/16 14:09:19, jeremy wrote: > nit: . at end of comment. Done. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_profiler_observer.h:24: // This method will be called on UI thread On 2014/02/16 14:09:19, jeremy wrote: > nit: the UI thread. Done. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... File content/browser/power_profiler/power_profiler_service.cc (right): https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_profiler_service.cc:17: delay_(base::TimeDelta::FromMilliseconds(50)), On 2014/02/16 14:09:19, jeremy wrote: > Where does this value come from? > > Recommend initializing from a file constant with a comment on where this number > came from. This value is recommended by Intel Power Gadget, considering both poll overhead and resolution request per app. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_profiler_service.cc:24: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); On 2014/02/16 14:09:19, jeremy wrote: > IMHO this should be at the start of the function. Done. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_profiler_service.cc:49: void PowerProfilerService::AddObserver(PowerProfilerObserver* obs) { On 2014/02/16 14:09:19, jeremy wrote: > obs -> observer Done. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_profiler_service.cc:50: if (UNINITIALIZED == status_) On 2014/02/16 14:09:19, jeremy wrote: > status_ == UNINITALIZED > > constant on rhs in comparisons below as well. Done. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_profiler_service.cc:69: // send out power events immediately. On 2014/02/16 14:09:19, jeremy wrote: > Send Done. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_profiler_service.cc:79: // stop timer, set status to INITIALIZED On 2014/02/16 14:09:19, jeremy wrote: > I think this comment is redundant. Done. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_profiler_service.cc:99: // Get Data and Notify On 2014/02/16 14:09:19, jeremy wrote: > . at end of comment. > > Data and Notify shouldn't be capitalized. Done. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... File content/browser/power_profiler/power_profiler_service.h (right): https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_profiler_service.h:19: // a class used to monitor the power usage and tell profiler the power data. On 2014/02/16 14:09:19, jeremy wrote: > nit: s/a/A/ Done. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_profiler_service.h:32: UNINITIALIZED, // Uninitialized. On 2014/02/16 14:09:19, jeremy wrote: > coding style is 2 space separation for line comments, here and elsewhere in the > cl. Done. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_profiler_service.h:44: scoped_refptr<base::TaskRunner> task_runner, On 2014/02/16 14:09:19, jeremy wrote: > I think the indentation here is off, I'd line these up with the upper 'scoped' > but other reviewers opinions may differ. Done. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_profiler_service.h:51: void QueryData(); On 2014/02/16 14:09:19, jeremy wrote: > Function comments for these. Done. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_profiler_service.h:53: // Notify works in UI thread. On 2014/02/16 14:09:19, jeremy wrote: > // Executes on the UI thread. Done. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_profiler_service.h:62: const base::TimeDelta delay_; On 2014/02/16 14:09:19, jeremy wrote: > From the comment, it sounds like this should be renamed to sample_period_ ? Done. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... File content/browser/power_profiler/power_profiler_service_unittest.cc (right): https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_profiler_service_unittest.cc:15: const int kEvents = 3; On 2014/02/16 14:09:19, jeremy wrote: > kNumEvents, kNumObservers ? Done. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_profiler_service_unittest.cc:82: // No one received PowerEvent now. On 2014/02/16 14:09:19, jeremy wrote: > // No PowerEvents received. Done. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_profiler_service_unittest.cc:117: TEST_F(PowerProfilerServiceTest, AvailableService) { On 2014/02/16 14:09:19, jeremy wrote: > Could you add a comment on what this is supposed to test. Done. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_profiler_service_unittest.cc:120: RunLoopForDelay(base::TimeDelta::FromMilliseconds(20)); On 2014/02/16 14:09:19, jeremy wrote: > Is there a better way to wait here? Using a timeout can be flaky?... All run in a single thread, I don't see a better way here, we can make the wait time long enough for all events dispatched, and add a comment here.
Much better, just a bit more comment cleanup. The test timeout is the only real remaining functional issue AFAIC. https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... File content/browser/power_profiler/power_profiler_service_unittest.cc (right): https://codereview.chromium.org/140583003/diff/1120001/content/browser/power_... content/browser/power_profiler/power_profiler_service_unittest.cc:120: RunLoopForDelay(base::TimeDelta::FromMilliseconds(20)); Flaky tests are a major problem, the pattern of waiting for a certain timeout is not a good one. We know how many events the observer is expected to see so can you just spin the message loop until it receives that number of events and then terminate the current loop. if that doesn't work please reach out to phajdan.jr@chromium.org who knows a lot about writing stable tests, he can point you in the right direction. Whatever he thinks is best I'm fine with too... https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... File content/browser/power_profiler/power_data_provider.h (right): https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... content/browser/power_profiler/power_data_provider.h:24: // Count the number of known PowerEvent. Keep this at the end. Just the second sentence is enough, the first part is clear from context. https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... content/browser/power_profiler/power_data_provider.h:32: // Power value between last event to this one, in watt. to -> and watt -> watts https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... File content/browser/power_profiler/power_profiler_observer.h (right): https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... content/browser/power_profiler/power_profiler_observer.h:18: // A class used to monitor the power information. the power information -> power usage data. https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... File content/browser/power_profiler/power_profiler_service.cc (right): https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... content/browser/power_profiler/power_profiler_service.cc:15: // Default sampling period, recommended by Intel Power Gadget. recommended -> as recommended. Would also be great if you could provide a link to where this is mentioned. https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... content/browser/power_profiler/power_profiler_service.cc:16: const int kDefaultSamplePeriod = 50; kDefaultSamplePeriodMS https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... content/browser/power_profiler/power_profiler_service.cc:41: sample_period_(delay), delay -> sample_period https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... content/browser/power_profiler/power_profiler_service.cc:107: return; Should this be a DCHECK ? https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... File content/browser/power_profiler/power_profiler_service.h (right): https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... content/browser/power_profiler/power_profiler_service.h:32: UNINITIALIZED, // Uninitialized. comment is redundant. https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... content/browser/power_profiler/power_profiler_service.h:33: INITIALIZED, // Initialized, not in profiling. not in profiling -> profiling has not started. https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... content/browser/power_profiler/power_profiler_service.h:34: PROFILING, // In profiling. I think you can remove this comment. https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... content/browser/power_profiler/power_profiler_service.h:50: // Query power data by PowerDataProvider, executes on the WorkerPool thread. by -> from https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... content/browser/power_profiler/power_profiler_service.h:57: // Notify executes on the UI thread. Notify executes -> Executes https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... content/browser/power_profiler/power_profiler_service.h:65: // Interval of reading the power data by provider. // Sampling period of power data measurement. https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... File content/browser/power_profiler/power_profiler_service_unittest.cc (right): https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... content/browser/power_profiler/power_profiler_service_unittest.cc:18: class TestPowerDataProvider : public PowerDataProvider { // Provide a set number of power events. https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... content/browser/power_profiler/power_profiler_service_unittest.cc:20: TestPowerDataProvider(int count) : event_number_(count) {} num_events_to_send ? https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... content/browser/power_profiler/power_profiler_service_unittest.cc:39: int event_number_; num_events_to_send_ ? https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... content/browser/power_profiler/power_profiler_service_unittest.cc:81: service_->AddObserver(&observers_[index]); Any reason to add more than 1 observer? The observer system is already used in many places and FOR_EACH_OBSERVER should just work ? Not that it really bothers me, but just trying to figure out if there's something more going on here? https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... content/browser/power_profiler/power_profiler_service_unittest.cc:111: // UI thread thread.
thanks @jeremy, For the timeout, I see similar usages in other tests, for example: content/browser/renderer_host/render_widget_host_unittest.cc, it wait for unresponsive timer fire. for this test, there is no heavy task, so it seems there is very little risk to cause flaky, make sense? Pan https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... File content/browser/power_profiler/power_data_provider.h (right): https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... content/browser/power_profiler/power_data_provider.h:24: // Count the number of known PowerEvent. Keep this at the end. On 2014/02/17 06:31:13, jeremy wrote: > Just the second sentence is enough, the first part is clear from context. Done. https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... content/browser/power_profiler/power_data_provider.h:32: // Power value between last event to this one, in watt. On 2014/02/17 06:31:13, jeremy wrote: > to -> and > watt -> watts Done. https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... File content/browser/power_profiler/power_profiler_observer.h (right): https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... content/browser/power_profiler/power_profiler_observer.h:18: // A class used to monitor the power information. On 2014/02/17 06:31:13, jeremy wrote: > the power information -> power usage data. Done. https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... File content/browser/power_profiler/power_profiler_service.cc (right): https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... content/browser/power_profiler/power_profiler_service.cc:15: // Default sampling period, recommended by Intel Power Gadget. On 2014/02/17 06:31:13, jeremy wrote: > recommended -> as recommended. > > Would also be great if you could provide a link to where this is mentioned. Done. https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... content/browser/power_profiler/power_profiler_service.cc:16: const int kDefaultSamplePeriod = 50; On 2014/02/17 06:31:13, jeremy wrote: > kDefaultSamplePeriodMS Done. https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... content/browser/power_profiler/power_profiler_service.cc:41: sample_period_(delay), On 2014/02/17 06:31:13, jeremy wrote: > delay -> sample_period Done. https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... content/browser/power_profiler/power_profiler_service.cc:107: return; On 2014/02/17 06:31:13, jeremy wrote: > Should this be a DCHECK ? right, thanks. https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... File content/browser/power_profiler/power_profiler_service.h (right): https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... content/browser/power_profiler/power_profiler_service.h:32: UNINITIALIZED, // Uninitialized. On 2014/02/17 06:31:13, jeremy wrote: > comment is redundant. Done. https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... content/browser/power_profiler/power_profiler_service.h:33: INITIALIZED, // Initialized, not in profiling. On 2014/02/17 06:31:13, jeremy wrote: > not in profiling -> profiling has not started. Done. https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... content/browser/power_profiler/power_profiler_service.h:34: PROFILING, // In profiling. On 2014/02/17 06:31:13, jeremy wrote: > I think you can remove this comment. Done. https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... content/browser/power_profiler/power_profiler_service.h:50: // Query power data by PowerDataProvider, executes on the WorkerPool thread. On 2014/02/17 06:31:13, jeremy wrote: > by -> from Done. https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... content/browser/power_profiler/power_profiler_service.h:57: // Notify executes on the UI thread. On 2014/02/17 06:31:13, jeremy wrote: > Notify executes -> Executes Done. https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... content/browser/power_profiler/power_profiler_service.h:65: // Interval of reading the power data by provider. On 2014/02/17 06:31:13, jeremy wrote: > // Sampling period of power data measurement. Done. https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... File content/browser/power_profiler/power_profiler_service_unittest.cc (right): https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... content/browser/power_profiler/power_profiler_service_unittest.cc:18: class TestPowerDataProvider : public PowerDataProvider { On 2014/02/17 06:31:13, jeremy wrote: > // Provide a set number of power events. Done. https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... content/browser/power_profiler/power_profiler_service_unittest.cc:20: TestPowerDataProvider(int count) : event_number_(count) {} On 2014/02/17 06:31:13, jeremy wrote: > num_events_to_send ? Done. https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... content/browser/power_profiler/power_profiler_service_unittest.cc:39: int event_number_; On 2014/02/17 06:31:13, jeremy wrote: > num_events_to_send_ ? Done. https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... content/browser/power_profiler/power_profiler_service_unittest.cc:81: service_->AddObserver(&observers_[index]); On 2014/02/17 06:31:13, jeremy wrote: > Any reason to add more than 1 observer? > The observer system is already used in many places and FOR_EACH_OBSERVER should > just work ? > > Not that it really bothers me, but just trying to figure out if there's > something more going on here? thanks, the motivation is simple, to test AddObserver can accept several observers, and notify all, not aims to test FOR_EACH_OBSERVER. https://codereview.chromium.org/140583003/diff/1260001/content/browser/power_... content/browser/power_profiler/power_profiler_service_unittest.cc:111: // UI thread On 2014/02/17 06:31:13, jeremy wrote: > thread. Done.
Can you please explain why quitting the message loop from inside the observer or counting the number of events received and quitting then will not work? As mentioned previously, timeouts in tests are bad and cause flake we should find an alternate approach. On Mon, Feb 17, 2014 at 9:53 AM, <pan.deng@intel.com> wrote: > thanks @jeremy, > For the timeout, I see similar usages in other tests, for example: > content/browser/renderer_host/render_widget_host_unittest.cc, it wait for > unresponsive timer fire. > for this test, there is no heavy task, so it seems there is very little > risk to > cause flaky, make sense? > > Pan > > > > https://codereview.chromium.org/140583003/diff/1260001/ > content/browser/power_profiler/power_data_provider.h > File content/browser/power_profiler/power_data_provider.h (right): > > https://codereview.chromium.org/140583003/diff/1260001/ > content/browser/power_profiler/power_data_provider.h#newcode24 > content/browser/power_profiler/power_data_provider.h:24: // Count the > number of known PowerEvent. Keep this at the end. > On 2014/02/17 06:31:13, jeremy wrote: > >> Just the second sentence is enough, the first part is clear from >> > context. > > Done. > > > https://codereview.chromium.org/140583003/diff/1260001/ > content/browser/power_profiler/power_data_provider.h#newcode32 > content/browser/power_profiler/power_data_provider.h:32: // Power value > between last event to this one, in watt. > On 2014/02/17 06:31:13, jeremy wrote: > >> to -> and >> watt -> watts >> > > Done. > > > https://codereview.chromium.org/140583003/diff/1260001/ > content/browser/power_profiler/power_profiler_observer.h > File content/browser/power_profiler/power_profiler_observer.h (right): > > https://codereview.chromium.org/140583003/diff/1260001/ > content/browser/power_profiler/power_profiler_observer.h#newcode18 > content/browser/power_profiler/power_profiler_observer.h:18: // A class > used to monitor the power information. > On 2014/02/17 06:31:13, jeremy wrote: > >> the power information -> power usage data. >> > > Done. > > > https://codereview.chromium.org/140583003/diff/1260001/ > content/browser/power_profiler/power_profiler_service.cc > File content/browser/power_profiler/power_profiler_service.cc (right): > > https://codereview.chromium.org/140583003/diff/1260001/ > content/browser/power_profiler/power_profiler_service.cc#newcode15 > content/browser/power_profiler/power_profiler_service.cc:15: // Default > sampling period, recommended by Intel Power Gadget. > On 2014/02/17 06:31:13, jeremy wrote: > >> recommended -> as recommended. >> > > Would also be great if you could provide a link to where this is >> > mentioned. > > Done. > > > https://codereview.chromium.org/140583003/diff/1260001/ > content/browser/power_profiler/power_profiler_service.cc#newcode16 > content/browser/power_profiler/power_profiler_service.cc:16: const int > kDefaultSamplePeriod = 50; > On 2014/02/17 06:31:13, jeremy wrote: > >> kDefaultSamplePeriodMS >> > > Done. > > > https://codereview.chromium.org/140583003/diff/1260001/ > content/browser/power_profiler/power_profiler_service.cc#newcode41 > content/browser/power_profiler/power_profiler_service.cc:41: > sample_period_(delay), > On 2014/02/17 06:31:13, jeremy wrote: > >> delay -> sample_period >> > > Done. > > > https://codereview.chromium.org/140583003/diff/1260001/ > content/browser/power_profiler/power_profiler_service.cc#newcode107 > content/browser/power_profiler/power_profiler_service.cc:107: return; > On 2014/02/17 06:31:13, jeremy wrote: > >> Should this be a DCHECK ? >> > > right, thanks. > > > https://codereview.chromium.org/140583003/diff/1260001/ > content/browser/power_profiler/power_profiler_service.h > File content/browser/power_profiler/power_profiler_service.h (right): > > https://codereview.chromium.org/140583003/diff/1260001/ > content/browser/power_profiler/power_profiler_service.h#newcode32 > content/browser/power_profiler/power_profiler_service.h:32: > UNINITIALIZED, // Uninitialized. > On 2014/02/17 06:31:13, jeremy wrote: > >> comment is redundant. >> > > Done. > > > https://codereview.chromium.org/140583003/diff/1260001/ > content/browser/power_profiler/power_profiler_service.h#newcode33 > content/browser/power_profiler/power_profiler_service.h:33: INITIALIZED, > // Initialized, not in profiling. > On 2014/02/17 06:31:13, jeremy wrote: > >> not in profiling -> profiling has not started. >> > > Done. > > > https://codereview.chromium.org/140583003/diff/1260001/ > content/browser/power_profiler/power_profiler_service.h#newcode34 > content/browser/power_profiler/power_profiler_service.h:34: PROFILING, > // In profiling. > On 2014/02/17 06:31:13, jeremy wrote: > >> I think you can remove this comment. >> > > Done. > > > https://codereview.chromium.org/140583003/diff/1260001/ > content/browser/power_profiler/power_profiler_service.h#newcode50 > content/browser/power_profiler/power_profiler_service.h:50: // Query > power data by PowerDataProvider, executes on the WorkerPool thread. > On 2014/02/17 06:31:13, jeremy wrote: > >> by -> from >> > > Done. > > > https://codereview.chromium.org/140583003/diff/1260001/ > content/browser/power_profiler/power_profiler_service.h#newcode57 > content/browser/power_profiler/power_profiler_service.h:57: // Notify > executes on the UI thread. > On 2014/02/17 06:31:13, jeremy wrote: > >> Notify executes -> Executes >> > > Done. > > > https://codereview.chromium.org/140583003/diff/1260001/ > content/browser/power_profiler/power_profiler_service.h#newcode65 > content/browser/power_profiler/power_profiler_service.h:65: // Interval > of reading the power data by provider. > On 2014/02/17 06:31:13, jeremy wrote: > >> // Sampling period of power data measurement. >> > > Done. > > > https://codereview.chromium.org/140583003/diff/1260001/ > content/browser/power_profiler/power_profiler_service_unittest.cc > File content/browser/power_profiler/power_profiler_service_unittest.cc > (right): > > https://codereview.chromium.org/140583003/diff/1260001/ > content/browser/power_profiler/power_profiler_ > service_unittest.cc#newcode18 > content/browser/power_profiler/power_profiler_service_unittest.cc:18: > class TestPowerDataProvider : public PowerDataProvider { > On 2014/02/17 06:31:13, jeremy wrote: > >> // Provide a set number of power events. >> > > Done. > > > https://codereview.chromium.org/140583003/diff/1260001/ > content/browser/power_profiler/power_profiler_ > service_unittest.cc#newcode20 > content/browser/power_profiler/power_profiler_service_unittest.cc:20: > TestPowerDataProvider(int count) : event_number_(count) {} > On 2014/02/17 06:31:13, jeremy wrote: > >> num_events_to_send ? >> > > Done. > > > https://codereview.chromium.org/140583003/diff/1260001/ > content/browser/power_profiler/power_profiler_ > service_unittest.cc#newcode39 > content/browser/power_profiler/power_profiler_service_unittest.cc:39: > int event_number_; > On 2014/02/17 06:31:13, jeremy wrote: > >> num_events_to_send_ ? >> > > Done. > > > https://codereview.chromium.org/140583003/diff/1260001/ > content/browser/power_profiler/power_profiler_ > service_unittest.cc#newcode81 > content/browser/power_profiler/power_profiler_service_unittest.cc:81: > service_->AddObserver(&observers_[index]); > On 2014/02/17 06:31:13, jeremy wrote: > >> Any reason to add more than 1 observer? >> The observer system is already used in many places and >> > FOR_EACH_OBSERVER should > >> just work ? >> > > Not that it really bothers me, but just trying to figure out if >> > there's > >> something more going on here? >> > thanks, the motivation is simple, to test AddObserver can accept several > observers, and notify all, not aims to test FOR_EACH_OBSERVER. > > > https://codereview.chromium.org/140583003/diff/1260001/ > content/browser/power_profiler/power_profiler_ > service_unittest.cc#newcode111 > content/browser/power_profiler/power_profiler_service_unittest.cc:111: > // UI thread > On 2014/02/17 06:31:13, jeremy wrote: > >> thread. >> > > Done. > > https://codereview.chromium.org/140583003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/02/17 11:12:50, jeremy wrote: > Can you please explain why quitting the message loop from inside the > observer or counting the number of events received and quitting then will > not work? > > As mentioned previously, timeouts in tests are bad and cause flake we > should find an alternate approach. I felt a little weird in that way, now check count and quit from observer. thanks! Pan
https://codereview.chromium.org/140583003/diff/1610001/content/browser/power_... File content/browser/power_profiler/power_profiler_service_unittest.cc (right): https://codereview.chromium.org/140583003/diff/1610001/content/browser/power_... content/browser/power_profiler/power_profiler_service_unittest.cc:85: base::TimeDelta::FromMilliseconds(5)); If you do not check anything related to time, make that even smaller to have the test as quick as possible. FromMicroseconds(1) for example.
https://codereview.chromium.org/140583003/diff/1610001/content/browser/power_... File content/browser/power_profiler/power_profiler_service_unittest.cc (right): https://codereview.chromium.org/140583003/diff/1610001/content/browser/power_... content/browser/power_profiler/power_profiler_service_unittest.cc:61: static int all_receive_counter; Much better! The only issue is the use of a class static here, this would be a problem for example if we added additional tests to this file or if tests are ever run in parallel. I would prefer that this counter be a local variable in the test whose address you pass to the observers. you could then change the code in OnPowerEvent to: (*total_num_events_received)++; if (*total_num_events_received >= kNumEvents * kNumObservers) { // All expected events received, exiting. base::MessageLoop::current()->Quit(); }
Some additional comments for things I missed before, sorry about that, hope we can get this past review quickly... https://codereview.chromium.org/140583003/diff/1610001/content/browser/power_... File content/browser/power_profiler/power_data_provider.h (right): https://codereview.chromium.org/140583003/diff/1610001/content/browser/power_... content/browser/power_profiler/power_data_provider.h:30: base::TimeTicks time; // Time that power data was read. 2 spaces before // https://codereview.chromium.org/140583003/diff/1610001/content/browser/power_... content/browser/power_profiler/power_data_provider.h:51: // Returns a vector of power events, one per type, for the type it supports. for the types it supports? https://codereview.chromium.org/140583003/diff/1610001/content/browser/power_... File content/browser/power_profiler/power_profiler_service.cc (right): https://codereview.chromium.org/140583003/diff/1610001/content/browser/power_... content/browser/power_profiler/power_profiler_service.cc:16: // http://software.intel.com/en-us/blogs/2013/10/03/using-the-intel-power-gadget... Excellent, thanks! I might even say that it's section 3.1 of that URL but your call... https://codereview.chromium.org/140583003/diff/1610001/content/browser/power_... content/browser/power_profiler/power_profiler_service.cc:17: const int kDefaultSamplePeriodMS = 50; Sorry, this should end with Ms not MS, my mistake. https://codereview.chromium.org/140583003/diff/1610001/content/browser/power_... content/browser/power_profiler/power_profiler_service.cc:29: if (data_provider_.get()) { I don't think you need this if() , that would mean that make_scoped_ptr() failed and it appears we don't check that in other places in the code. Looking at this further, do you really need the UNINITIALIZED status? https://codereview.chromium.org/140583003/diff/1610001/content/browser/power_... content/browser/power_profiler/power_profiler_service.cc:63: return; Should these ifs be DCHECKS or removed? My concern is with masking programming errors... https://codereview.chromium.org/140583003/diff/1610001/content/browser/power_... content/browser/power_profiler/power_profiler_service.cc:106: DCHECK(task_runner_->RunsTasksOnCurrentThread() && status_ == PROFILING); Would recommend breaking into 2 DCHECK() statements so if this fails we know exactly what went wrong.
https://codereview.chromium.org/140583003/diff/1610001/content/browser/power_... File content/browser/power_profiler/power_profiler_service.cc (right): https://codereview.chromium.org/140583003/diff/1610001/content/browser/power_... content/browser/power_profiler/power_profiler_service.cc:29: if (data_provider_.get()) { On 2014/02/18 11:35:33, jeremy wrote: > I don't think you need this if() , that would mean that make_scoped_ptr() failed > and it appears we don't check that in other places in the code. > > Looking at this further, do you really need the UNINITIALIZED status? This is in fact needed. The factory is not guaranteed to return a provider, if it cannot create one for the current platform.
Thanks Ben, in that case I'd add a comment (e.g. it wasn't clear to me under what conditions a provider could fail) and an early return in this case rather than wrapping the function contents in an if() block. On Tue, Feb 18, 2014 at 1:38 PM, <qsr@chromium.org> wrote: > > https://codereview.chromium.org/140583003/diff/1610001/ > content/browser/power_profiler/power_profiler_service.cc > File content/browser/power_profiler/power_profiler_service.cc (right): > > https://codereview.chromium.org/140583003/diff/1610001/ > content/browser/power_profiler/power_profiler_service.cc#newcode29 > content/browser/power_profiler/power_profiler_service.cc:29: if > (data_provider_.get()) { > On 2014/02/18 11:35:33, jeremy wrote: > >> I don't think you need this if() , that would mean that >> > make_scoped_ptr() failed > >> and it appears we don't check that in other places in the code. >> > > Looking at this further, do you really need the UNINITIALIZED status? >> > > This is in fact needed. The factory is not guaranteed to return a > provider, if it cannot create one for the current platform. > > https://codereview.chromium.org/140583003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/140583003/diff/1610001/content/browser/power_... File content/browser/power_profiler/power_data_provider.h (right): https://codereview.chromium.org/140583003/diff/1610001/content/browser/power_... content/browser/power_profiler/power_data_provider.h:30: base::TimeTicks time; // Time that power data was read. On 2014/02/18 11:35:33, jeremy wrote: > 2 spaces before // Done. https://codereview.chromium.org/140583003/diff/1610001/content/browser/power_... content/browser/power_profiler/power_data_provider.h:51: // Returns a vector of power events, one per type, for the type it supports. On 2014/02/18 11:35:33, jeremy wrote: > for the types it supports? Done. https://codereview.chromium.org/140583003/diff/1610001/content/browser/power_... File content/browser/power_profiler/power_profiler_service.cc (right): https://codereview.chromium.org/140583003/diff/1610001/content/browser/power_... content/browser/power_profiler/power_profiler_service.cc:16: // http://software.intel.com/en-us/blogs/2013/10/03/using-the-intel-power-gadget... On 2014/02/18 11:35:33, jeremy wrote: > Excellent, thanks! I might even say that it's section 3.1 of that URL but your > call... Done. https://codereview.chromium.org/140583003/diff/1610001/content/browser/power_... content/browser/power_profiler/power_profiler_service.cc:17: const int kDefaultSamplePeriodMS = 50; On 2014/02/18 11:35:33, jeremy wrote: > Sorry, this should end with Ms not MS, my mistake. Done. https://codereview.chromium.org/140583003/diff/1610001/content/browser/power_... content/browser/power_profiler/power_profiler_service.cc:29: if (data_provider_.get()) { On 2014/02/18 11:38:09, qsr wrote: > On 2014/02/18 11:35:33, jeremy wrote: > > I don't think you need this if() , that would mean that make_scoped_ptr() > failed > > and it appears we don't check that in other places in the code. > > > > Looking at this further, do you really need the UNINITIALIZED status? > > This is in fact needed. The factory is not guaranteed to return a provider, if > it cannot create one for the current platform. right, needed here. https://codereview.chromium.org/140583003/diff/1610001/content/browser/power_... content/browser/power_profiler/power_profiler_service.cc:63: return; On 2014/02/18 11:35:33, jeremy wrote: > Should these ifs be DCHECKS or removed? My concern is with masking programming > errors... status_ should be UNINTIALIZED if no provider. https://codereview.chromium.org/140583003/diff/1610001/content/browser/power_... content/browser/power_profiler/power_profiler_service.cc:106: DCHECK(task_runner_->RunsTasksOnCurrentThread() && status_ == PROFILING); On 2014/02/18 11:35:33, jeremy wrote: > Would recommend breaking into 2 DCHECK() statements so if this fails we know > exactly what went wrong. Done. https://codereview.chromium.org/140583003/diff/1610001/content/browser/power_... File content/browser/power_profiler/power_profiler_service_unittest.cc (right): https://codereview.chromium.org/140583003/diff/1610001/content/browser/power_... content/browser/power_profiler/power_profiler_service_unittest.cc:61: static int all_receive_counter; On 2014/02/18 10:28:57, jeremy wrote: > Much better! > > The only issue is the use of a class static here, this would be a problem for > example if we added additional tests to this file or if tests are ever run in > parallel. > > I would prefer that this counter be a local variable in the test whose address > you pass to the observers. > > you could then change the code in OnPowerEvent to: > > (*total_num_events_received)++; > if (*total_num_events_received >= kNumEvents * kNumObservers) { > // All expected events received, exiting. > base::MessageLoop::current()->Quit(); > } > thanks, done https://codereview.chromium.org/140583003/diff/1610001/content/browser/power_... content/browser/power_profiler/power_profiler_service_unittest.cc:85: base::TimeDelta::FromMilliseconds(5)); On 2014/02/18 10:01:22, qsr wrote: > If you do not check anything related to time, make that even smaller to have the > test as quick as possible. FromMicroseconds(1) for example. thanks, done.
LGTM One additional nit, no need for another review round from me. https://codereview.chromium.org/140583003/diff/1780001/content/browser/power_... File content/browser/power_profiler/power_profiler_service.cc (right): https://codereview.chromium.org/140583003/diff/1780001/content/browser/power_... content/browser/power_profiler/power_profiler_service.cc:29: if (data_provider_.get()) { nit: if (!...get()) return ...
The CQ bit was checked by pan.deng@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pan.deng@intel.com/140583003/1880001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel
Hi @jam, This CL has been updated with the help from qsr, pfeldman and jeremy. Could you please have a look at it? thanks! Pan
since there are > 60 comments on this and I won't be able to read all of them for a while, can you summarize why this code is in content instead of chrome? are there tests that specifically want to test content based browsers, instead of just chrome? https://codereview.chromium.org/140583003/diff/1880001/content/browser/power_... File content/browser/power_profiler/power_data_provider.h (right): https://codereview.chromium.org/140583003/diff/1880001/content/browser/power_... content/browser/power_profiler/power_data_provider.h:15: struct PowerEvent { nit: put this in a separate file https://codereview.chromium.org/140583003/diff/1880001/content/browser/power_... File content/browser/power_profiler/power_data_provider_factory.h (right): https://codereview.chromium.org/140583003/diff/1880001/content/browser/power_... content/browser/power_profiler/power_data_provider_factory.h:13: class PowerDataProviderFactory { what is the point of this factory? https://codereview.chromium.org/140583003/diff/1880001/content/browser/power_... File content/browser/power_profiler/power_profiler_service_unittest.cc (right): https://codereview.chromium.org/140583003/diff/1880001/content/browser/power_... content/browser/power_profiler/power_profiler_service_unittest.cc:142: } this is a lot of test code to test a trivial piece of code which calls one method on an interface and then calls all the observers with that data sure this test verifies that the ~10 lines in PowerProfilerService work, but it seems like such basic code to test which involves adding a few interfaces for and different constructors. is it really worth it?
On 2014/02/24 21:53:44, jam wrote: > since there are > 60 comments on this and I won't be able to read all of them > for a while, can you summarize why this code is in content instead of chrome? > are there tests that specifically want to test content based browsers, instead > of just chrome? > thanks @jam, one reason is that we would like the power reporting on content shell, the 2nd one is, it also works for DevTools, power feature in DevTools is also expected in content shell. thanks Pan https://codereview.chromium.org/140583003/diff/1880001/content/browser/power_... File content/browser/power_profiler/power_data_provider.h (right): https://codereview.chromium.org/140583003/diff/1880001/content/browser/power_... content/browser/power_profiler/power_data_provider.h:15: struct PowerEvent { On 2014/02/24 21:53:46, jam wrote: > nit: put this in a separate file Done. https://codereview.chromium.org/140583003/diff/1880001/content/browser/power_... File content/browser/power_profiler/power_data_provider_factory.h (right): https://codereview.chromium.org/140583003/diff/1880001/content/browser/power_... content/browser/power_profiler/power_data_provider_factory.h:13: class PowerDataProviderFactory { On 2014/02/24 21:53:46, jam wrote: > what is the point of this factory? to create power data provider for specific platform, a comment added, thanks. https://codereview.chromium.org/140583003/diff/1880001/content/browser/power_... File content/browser/power_profiler/power_profiler_service_unittest.cc (right): https://codereview.chromium.org/140583003/diff/1880001/content/browser/power_... content/browser/power_profiler/power_profiler_service_unittest.cc:142: } On 2014/02/24 21:53:46, jam wrote: > this is a lot of test code to test a trivial piece of code which calls one > method on an interface and then calls all the observers with that data > > sure this test verifies that the ~10 lines in PowerProfilerService work, but it > seems like such basic code to test which involves adding a few interfaces for > and different constructors. is it really worth it? Not only the interface is tested, it also make sure tasks run on its right thread. I think it worth :)
https://codereview.chromium.org/140583003/diff/1880001/content/browser/power_... File content/browser/power_profiler/power_data_provider_factory.h (right): https://codereview.chromium.org/140583003/diff/1880001/content/browser/power_... content/browser/power_profiler/power_data_provider_factory.h:13: class PowerDataProviderFactory { On 2014/02/25 01:20:39, Pan wrote: > On 2014/02/24 21:53:46, jam wrote: > > what is the point of this factory? > > to create power data provider for specific platform, > a comment added, thanks. you don't need a separate class factory for that. for this platform specific implementation, a pattern we use is to have a static method, i.e. PowerDataProvder::Create(). then each different platform's implementation file (i.e. power_data_provider_win.cc) has that method. https://codereview.chromium.org/140583003/diff/1880001/content/browser/power_... File content/browser/power_profiler/power_profiler_service_unittest.cc (right): https://codereview.chromium.org/140583003/diff/1880001/content/browser/power_... content/browser/power_profiler/power_profiler_service_unittest.cc:142: } On 2014/02/25 01:20:39, Pan wrote: > On 2014/02/24 21:53:46, jam wrote: > > this is a lot of test code to test a trivial piece of code which calls one > > method on an interface and then calls all the observers with that data > > > > sure this test verifies that the ~10 lines in PowerProfilerService work, but > it > > seems like such basic code to test which involves adding a few interfaces for > > and different constructors. is it really worth it? > > Not only the interface is tested, it also make sure tasks run on its right > thread. I think it worth :) where does this test verify that a different thread is used?
https://codereview.chromium.org/140583003/diff/1880001/content/browser/power_... File content/browser/power_profiler/power_data_provider_factory.h (right): https://codereview.chromium.org/140583003/diff/1880001/content/browser/power_... content/browser/power_profiler/power_data_provider_factory.h:13: class PowerDataProviderFactory { > you don't need a separate class factory for that. for this platform specific > implementation, a pattern we use is to have a static method, i.e. > PowerDataProvder::Create(). then each different platform's implementation file > (i.e. power_data_provider_win.cc) has that method. thanks, not only OS, but also hardware platform. for example we will have: power_data_provider_ia derives from power_data_provider, and then power_data_provider_ia_win, power_data_provider_ia_mac, to impl etc. so in the static Create, there should be macros like: #if defined(ARCH_CPU_X86_FAMILY) ...new PowerDataProviderIA()... then the specific OS impls will be handled through gyp. in this way, I think a factory make things clear, make sense? https://codereview.chromium.org/140583003/diff/1880001/content/browser/power_... File content/browser/power_profiler/power_profiler_service_unittest.cc (right): https://codereview.chromium.org/140583003/diff/1880001/content/browser/power_... content/browser/power_profiler/power_profiler_service_unittest.cc:142: } Oh, sorry, I mean some DCHECKs are used in power_profiler_service, to make sure interface called on UI thread(otherwise it will cry). threading in this test cases has been simplified, everything run in a same thread.
https://codereview.chromium.org/140583003/diff/1880001/content/browser/power_... File content/browser/power_profiler/power_data_provider_factory.h (right): https://codereview.chromium.org/140583003/diff/1880001/content/browser/power_... content/browser/power_profiler/power_data_provider_factory.h:13: class PowerDataProviderFactory { On 2014/02/26 00:17:05, Pan wrote: > > you don't need a separate class factory for that. for this platform specific > > implementation, a pattern we use is to have a static method, i.e. > > PowerDataProvder::Create(). then each different platform's implementation file > > (i.e. power_data_provider_win.cc) has that method. > > thanks, not only OS, but also hardware platform. for example we will have: > power_data_provider_ia derives from power_data_provider, and then > power_data_provider_ia_win, power_data_provider_ia_mac, to impl etc. > so in the static Create, there should be macros like: > #if defined(ARCH_CPU_X86_FAMILY) > ...new PowerDataProviderIA()... > then the specific OS impls will be handled through gyp. > in this way, I think a factory make things clear, make sense? I still don't understand why this makes things clear? you already need gyp rules to pick the right file. so why not have each platform/os file have the static method in it. otherwise you have this factory header/implementation file, and it will need a set of ifdefs in the implementation of the method, and also in the include section
> I still don't understand why this makes things clear? you already need gyp rules > to pick the right file. so why not have each platform/os file have the static > method in it. otherwise you have this factory header/implementation file, and it > will need a set of ifdefs in the implementation of the method, and also in the > include section thanks, another context is that not all platform have its power data provider. The PowerDataProvider defined here is a abstract class with minimal interface, PowerDataProviderIA derives from PowerDataProvider, rather than impl PowerDataProvider. and PowerDataProviderIA has its own impl files. (class PowerDataProviderOther may be quite different class, which is also derives from PowerDataProvider though) so the factory creates a sub-class, rather than a impl. and PowerProfilerService depends on PowerDataProvider and factory, rather than a PowerDataProviderIA class. make sense? Pan
Hi @jam, I finally understand your suggestion, impl static PowerDataProvider::Create() together with sub class's impl files, and return a sub class there. Now, factory is removed, a dummy provider is introduced as we don't have any real provider in this Cl. thanks for your help! Pan
lgtm https://codereview.chromium.org/140583003/diff/2360001/content/browser/power_... File content/browser/power_profiler/power_data_provider.h (right): https://codereview.chromium.org/140583003/diff/2360001/content/browser/power_... content/browser/power_profiler/power_data_provider.h:27: DISALLOW_COPY_AND_ASSIGN(PowerDataProvider); nit: this isn't needed because an abstract class isn't copyable
https://codereview.chromium.org/140583003/diff/2360001/content/browser/power_... File content/browser/power_profiler/power_data_provider.h (right): https://codereview.chromium.org/140583003/diff/2360001/content/browser/power_... content/browser/power_profiler/power_data_provider.h:27: DISALLOW_COPY_AND_ASSIGN(PowerDataProvider); On 2014/02/27 21:55:04, jam wrote: > nit: this isn't needed because an abstract class isn't copyable thanks, done!
The CQ bit was checked by pan.deng@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pan.deng@intel.com/140583003/2380001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel
The CQ bit was checked by pan.deng@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pan.deng@intel.com/140583003/2400001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel
The CQ bit was checked by pan.deng@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pan.deng@intel.com/140583003/2420001
Message was sent while issue was closed.
Change committed as 254116
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/184583004/ by erikwright@chromium.org. The reason for reverting is: Appears to have caused a memory leak. http://build.chromium.org/p/chromium.memory/builders/Linux%20ASAN%20Tests%20%... Indirect leak of 176 byte(s) in 1 object(s) allocated from: #0 0x49c001 in operator new(unsigned long) /usr/local/google/work/chromium/src/third_party/llvm/projects/compiler-rt/lib/asan/asan_new_delete.cc:54 #1 0xf42228 in content::PowerProfilerServiceTest::ServiceStartTest() content/browser/power_profiler/power_profiler_service_unittest.cc:85 #2 0xf4212d in content::PowerProfilerServiceTest_AvailableService_Test::TestBody() content/browser/power_profiler/power_profiler_service_unittest.cc:138 #3 0x2d3587a in HandleExceptionsInMethodIfSupported\u003Ctesting::Test, void> testing/gtest/src/gtest.cc:2045 #4 0x2d3587a in testing::Test::Run() testing/gtest/src/gtest.cc:2061 #5 0x2d379ca in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2237 #6 0x2d38793 in testing::TestCase::Run() testing/gtest/src/gtest.cc:2344 #7 0x2d4984a in testing::internal::UnitTestImpl::RunAllTests() testing/gtest/src/gtest.cc:4065 #8 0x2d48e30 in HandleExceptionsInMethodIfSupported\u003Ctesting::internal::UnitTestImpl, bool> testing/gtest/src/gtest.cc:2045 #9 0x2d48e30 in testing::UnitTest::Run() testing/gtest/src/gtest.cc:3697 #10 0x2cc09cc in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2231 #11 0x2cc09cc in base::TestSuite::Run() base/test/test_suite.cc:213 #12 0x2cb4041 in Run base/callback.h:401 #13 0x2cb4041 in base::(anonymous namespace)::LaunchUnitTestsInternal(int, char**, base::Callback\u003Cint ()> const&, int) base/test/launcher/unit_test_launcher.cc:494 #14 0x195d8ce in main content/test/run_all_unittests.cc:14 #15 0x7f515353d76c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226 . |
