SystemMonitor refactoring: move power state monitor into a separate class called PowerMonitor

Split the power monitoring feature from SystemMonitor

The SystemMonitor is a mixed monitor which not only monitors
the power state changes but also the devices changes. This
patch is to separate the power monitor from SystemMonitor
as a new class PowerMonitor which is dedicated to monitor
power state.

The next step is to seek a opportunity to refactor SystemMonitor
as something like DeviceMonitor.

BUG=149059
TEST=base_unittests --gtest_filter=PowerMonitorTest.*

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192114

[Hongbo Min, 2012-09-20 16:18:31]

I try to make power state management as a separate class and move it out of
base/system_monitor. I grep all the cases using Power-related API and modify
them correspondingly. 

This is the first step to refactor SystemMonitor. Does it make sense? Please
have a review.

[Hongbo Min, 2012-09-20 23:54:03]

Any review comments?

[vandebo (ex-Chrome), 2012-09-21 20:12:18]

I suspect that the system monitor files where in a subdirectory because they are
somewhat numerous; the replacement should do the same.  Manager is an overused
term in class names and furthermore, this class doesn't manage the power state
(you can't change it through the class), you can only monitor it.  So files
should go into base/power_state_monitor/power_state_monitor*

I'm trying to get an answer an answer on the window message issue and the
singleton issue.

http://codereview.chromium.org/10959020/diff/65/ui/views/win/hwnd_message_han...
File ui/views/win/hwnd_message_handler.cc (right):

http://codereview.chromium.org/10959020/diff/65/ui/views/win/hwnd_message_han...
ui/views/win/hwnd_message_handler.cc:1854: base::PowerStateManager* manager =
base::PowerStateManager::Get();
It's unclear exactly how we want to handle this part.  BenG is out of the office
with a cold.

[Hongbo Min, 2012-09-22 01:01:29]

On 2012/09/21 20:12:18, vandebo wrote:
> I suspect that the system monitor files where in a subdirectory because they
are
> somewhat numerous; the replacement should do the same.  Manager is an overused
> term in class names and furthermore, this class doesn't manage the power state
> (you can't change it through the class), you can only monitor it.  So files
> should go into base/power_state_monitor/power_state_monitor*
> 
> I'm trying to get an answer an answer on the window message issue and the
> singleton issue.
> 
>
http://codereview.chromium.org/10959020/diff/65/ui/views/win/hwnd_message_han...
> File ui/views/win/hwnd_message_handler.cc (right):
> 
>
http://codereview.chromium.org/10959020/diff/65/ui/views/win/hwnd_message_han...
> ui/views/win/hwnd_message_handler.cc:1854: base::PowerStateManager* manager =
> base::PowerStateManager::Get();
> It's unclear exactly how we want to handle this part.  BenG is out of the
office
> with a cold.

That is fine. Any update pls let me know. Thanks.

[vandebo (ex-Chrome), 2012-10-09 22:23:35]

On 2012/09/22 01:01:29, Hongbo Min wrote:
> On 2012/09/21 20:12:18, vandebo wrote:
> > I suspect that the system monitor files where in a subdirectory because they
> are
> > somewhat numerous; the replacement should do the same.  Manager is an
overused
> > term in class names and furthermore, this class doesn't manage the power
state
> > (you can't change it through the class), you can only monitor it.  So files
> > should go into base/power_state_monitor/power_state_monitor*
> > 
> > I'm trying to get an answer an answer on the window message issue and the
> > singleton issue.

Will saw no problem with making the power monitor class a singleton, so we
should do that.

As I said on the bug, there is no resolution on the window message issue.  Until
there is, we should preserve the current mechanism as much as makes sense. 
Here, PowerStateMonitor can have HWNDMessageHandler as a friend.

If you'd like to proceed with this CL, please address my comments, otherwise let
me know and I will pick it up.  Note that since you did this CL, android as
merged in their power monitor implementation to trunk.

>
http://codereview.chromium.org/10959020/diff/65/ui/views/win/hwnd_message_han...
> > File ui/views/win/hwnd_message_handler.cc (right):
> > 
> >
>
http://codereview.chromium.org/10959020/diff/65/ui/views/win/hwnd_message_han...
> > ui/views/win/hwnd_message_handler.cc:1854: base::PowerStateManager* manager
=
> > base::PowerStateManager::Get();
> > It's unclear exactly how we want to handle this part.  BenG is out of the
> office
> > with a cold.
> 
> That is fine. Any update pls let me know. Thanks.

[Hongbo Min, 2012-10-10 11:42:02]

I will update the CL soon. Thanks for your information.

[Hongbo Min, 2012-10-10 23:17:48]

The patch is updated. Pls have a review again.

[vandebo (ex-Chrome), 2012-10-11 00:58:43]

https://codereview.chromium.org/10959020/diff/12002/base/hi_res_timer_manager...
File base/hi_res_timer_manager_unittest.cc (right):

https://codereview.chromium.org/10959020/diff/12002/base/hi_res_timer_manager...
base/hi_res_timer_manager_unittest.cc:16: scoped_ptr<base::PowerMonitor>
system_monitor(
system_monitor -> power_monitor

https://codereview.chromium.org/10959020/diff/12002/base/hi_res_timer_manager...
File base/hi_res_timer_manager_win.cc (right):

https://codereview.chromium.org/10959020/diff/12002/base/hi_res_timer_manager...
base/hi_res_timer_manager_win.cc:13:
UseHiResClock(!system_monitor->BatteryPower());
system_monitor -> power_monitor

https://codereview.chromium.org/10959020/diff/12002/base/power_monitor/power_...
File base/power_monitor/power_monitor.h (right):

https://codereview.chromium.org/10959020/diff/12002/base/power_monitor/power_...
base/power_monitor/power_monitor.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
It'd be nice to have this file tracked as a move.  You can adjust the threshold
for that by adding a -M option in depot_tools/third_party/upload.py:1330
(GenerateDiff)

https://codereview.chromium.org/10959020/diff/12002/base/power_monitor/power_...
base/power_monitor/power_monitor.h:19: #include
"base/observer_list_threadsafe.h"
Move this to line 10

https://codereview.chromium.org/10959020/diff/12002/base/power_monitor/power_...
base/power_monitor/power_monitor.h:42: // Callbacks will be called on the thread
which creates the SystemMonitor.
SystemMonitor -> PowerMonitor

https://codereview.chromium.org/10959020/diff/12002/base/power_monitor/power_...
base/power_monitor/power_monitor.h:47: class BASE_EXPORT PowerObserver {
per willchan's comment, move this to the top of the class with the other type
definitions.

https://codereview.chromium.org/10959020/diff/12002/base/power_monitor/power_...
base/power_monitor/power_monitor.h:63: void AddPowerObserver(PowerObserver*
observer);
Add a copy of the comment from system_monitor.h:151 here.

https://codereview.chromium.org/10959020/diff/12002/base/power_monitor/power_...
base/power_monitor/power_monitor.h:73: static PowerMonitor* Get();
We should use Singleton instead of the custom Get method.

https://codereview.chromium.org/10959020/diff/12002/base/power_monitor/power_...
base/power_monitor/power_monitor.h:75: #if defined(OS_MACOSX)
Move these init type methods to just below the con/destructor.

https://codereview.chromium.org/10959020/diff/12002/base/power_monitor/power_...
base/power_monitor/power_monitor.h:88: friend class HWNDMessageHandler;
Looks like the Mac implementation needs a friend function declaration as well

https://codereview.chromium.org/10959020/diff/12002/base/power_monitor/power_...
base/power_monitor/power_monitor.h:120:
scoped_refptr<ObserverListThreadSafe<PowerObserver> > power_observer_list_;
power_observer_list_ -> observers_

https://codereview.chromium.org/10959020/diff/12002/base/power_monitor/power_...
File base/power_monitor/power_monitor_unittest.cc (right):

https://codereview.chromium.org/10959020/diff/12002/base/power_monitor/power_...
base/power_monitor/power_monitor_unittest.cc:62: //  scoped_ptr<PowerMonitor>
power_monitor_;
Go back to the scoped_ptr ?

https://codereview.chromium.org/10959020/diff/12002/base/system_monitor/syste...
File base/system_monitor/system_monitor.h (left):

https://codereview.chromium.org/10959020/diff/12002/base/system_monitor/syste...
base/system_monitor/system_monitor.h:151: // Add a new observer.
Leave a copy of this comment in this file.

https://codereview.chromium.org/10959020/diff/12002/base/system_monitor/syste...
base/system_monitor/system_monitor.h:168: // Windows-specific handling of a
WM_POWERBROADCAST message.
Move this entire comment over.

https://codereview.chromium.org/10959020/diff/12002/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager_unittest.cc (left):

https://codereview.chromium.org/10959020/diff/12002/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager_unittest.cc:100: #if defined(OS_MACOSX)
If you look at the change that introduced this, I think this should should stay
and the system monitor should be changed to power monitor.  Adding that author
to the review process.
http://codereview.chromium.org/7235023/

https://codereview.chromium.org/10959020/diff/12002/ui/views/win/hwnd_message...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/10959020/diff/12002/ui/views/win/hwnd_message...
ui/views/win/hwnd_message_handler.cc:1856: base::PowerMonitor::PowerEvent
power_event;
this variable shadows the function argument.

[Hongbo Min, 2012-10-11 07:57:53]

http://codereview.chromium.org/10959020/diff/12002/base/hi_res_timer_manager_...
File base/hi_res_timer_manager_unittest.cc (right):

http://codereview.chromium.org/10959020/diff/12002/base/hi_res_timer_manager_...
base/hi_res_timer_manager_unittest.cc:16: scoped_ptr<base::PowerMonitor>
system_monitor(
On 2012/10/11 00:58:43, vandebo wrote:
> system_monitor -> power_monitor

Done.

http://codereview.chromium.org/10959020/diff/12002/base/hi_res_timer_manager_...
File base/hi_res_timer_manager_win.cc (right):

http://codereview.chromium.org/10959020/diff/12002/base/hi_res_timer_manager_...
base/hi_res_timer_manager_win.cc:13:
UseHiResClock(!system_monitor->BatteryPower());
On 2012/10/11 00:58:43, vandebo wrote:
> system_monitor -> power_monitor

Done.

http://codereview.chromium.org/10959020/diff/12002/base/power_monitor/power_m...
File base/power_monitor/power_monitor.h (right):

http://codereview.chromium.org/10959020/diff/12002/base/power_monitor/power_m...
base/power_monitor/power_monitor.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2012/10/11 00:58:43, vandebo wrote:
> It'd be nice to have this file tracked as a move.  You can adjust the
threshold
> for that by adding a -M option in depot_tools/third_party/upload.py:1330
> (GenerateDiff)

Done. git cl upload have an option "--similarity" to do the same thing. I set
the similarity to 10.

http://codereview.chromium.org/10959020/diff/12002/base/power_monitor/power_m...
base/power_monitor/power_monitor.h:19: #include
"base/observer_list_threadsafe.h"
On 2012/10/11 00:58:43, vandebo wrote:
> Move this to line 10

Done.

http://codereview.chromium.org/10959020/diff/12002/base/power_monitor/power_m...
base/power_monitor/power_monitor.h:42: // Callbacks will be called on the thread
which creates the SystemMonitor.
On 2012/10/11 00:58:43, vandebo wrote:
> SystemMonitor -> PowerMonitor

Done.

http://codereview.chromium.org/10959020/diff/12002/base/power_monitor/power_m...
base/power_monitor/power_monitor.h:47: class BASE_EXPORT PowerObserver {
On 2012/10/11 00:58:43, vandebo wrote:
> per willchan's comment, move this to the top of the class with the other type
> definitions.

Done.

http://codereview.chromium.org/10959020/diff/12002/base/power_monitor/power_m...
base/power_monitor/power_monitor.h:63: void AddPowerObserver(PowerObserver*
observer);
On 2012/10/11 00:58:43, vandebo wrote:
> Add a copy of the comment from system_monitor.h:151 here.

Done.

http://codereview.chromium.org/10959020/diff/12002/base/power_monitor/power_m...
base/power_monitor/power_monitor.h:73: static PowerMonitor* Get();
On 2012/10/11 00:58:43, vandebo wrote:
> We should use Singleton instead of the custom Get method.

Done.

http://codereview.chromium.org/10959020/diff/12002/base/power_monitor/power_m...
base/power_monitor/power_monitor.h:75: #if defined(OS_MACOSX)
On 2012/10/11 00:58:43, vandebo wrote:
> Move these init type methods to just below the con/destructor.

Done.

http://codereview.chromium.org/10959020/diff/12002/base/power_monitor/power_m...
base/power_monitor/power_monitor.h:88: friend class HWNDMessageHandler;
On 2012/10/11 00:58:43, vandebo wrote:
> Looks like the Mac implementation needs a friend function declaration as well

If making a friend function for Mac, it will introduce additional type
dependency in power_monitor.h, and lots of places also need to access
ProcessPowerMessage, so I just keep the original methods instead of renaming to
HandlePowerEvent and making it private in this CL.

http://codereview.chromium.org/10959020/diff/12002/base/power_monitor/power_m...
base/power_monitor/power_monitor.h:120:
scoped_refptr<ObserverListThreadSafe<PowerObserver> > power_observer_list_;
On 2012/10/11 00:58:43, vandebo wrote:
> power_observer_list_ -> observers_

Done.

http://codereview.chromium.org/10959020/diff/12002/base/power_monitor/power_m...
File base/power_monitor/power_monitor_unittest.cc (right):

http://codereview.chromium.org/10959020/diff/12002/base/power_monitor/power_m...
base/power_monitor/power_monitor_unittest.cc:62: //  scoped_ptr<PowerMonitor>
power_monitor_;
On 2012/10/11 00:58:43, vandebo wrote:
> Go back to the scoped_ptr ?

Instead create the PowerMonitor on the stack and make PowerMonitorTest as a
friend of PowerMonitor.

http://codereview.chromium.org/10959020/diff/12002/base/system_monitor/system...
File base/system_monitor/system_monitor.h (left):

http://codereview.chromium.org/10959020/diff/12002/base/system_monitor/system...
base/system_monitor/system_monitor.h:151: // Add a new observer.
On 2012/10/11 00:58:43, vandebo wrote:
> Leave a copy of this comment in this file.

Done.

http://codereview.chromium.org/10959020/diff/12002/base/system_monitor/system...
base/system_monitor/system_monitor.h:168: // Windows-specific handling of a
WM_POWERBROADCAST message.
On 2012/10/11 00:58:43, vandebo wrote:
> Move this entire comment over.

Done.

http://codereview.chromium.org/10959020/diff/12002/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_manager_unittest.cc (left):

http://codereview.chromium.org/10959020/diff/12002/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_manager_unittest.cc:100: #if defined(OS_MACOSX)
On 2012/10/11 00:58:43, vandebo wrote:
> If you look at the change that introduced this, I think this should should
stay
> and the system monitor should be changed to power monitor.  Adding that author
> to the review process.
> http://codereview.chromium.org/7235023/

I keep it for safety.

http://codereview.chromium.org/10959020/diff/12002/ui/views/win/hwnd_message_...
File ui/views/win/hwnd_message_handler.cc (right):

http://codereview.chromium.org/10959020/diff/12002/ui/views/win/hwnd_message_...
ui/views/win/hwnd_message_handler.cc:1856: base::PowerMonitor::PowerEvent
power_event;
On 2012/10/11 00:58:43, vandebo wrote:
> this variable shadows the function argument.

Keep the original way by using ProcessWmPowerBroadcastMessage

http://codereview.chromium.org/10959020/diff/19096/base/hi_res_timer_manager_...
File base/hi_res_timer_manager_unittest.cc (left):

http://codereview.chromium.org/10959020/diff/19096/base/hi_res_timer_manager_...
base/hi_res_timer_manager_unittest.cc:16: scoped_ptr<base::SystemMonitor>
system_monitor(new base::SystemMonitor());
No need to create PowerMonitor since the HighResolutionTimerManager ctor has
already accesses PowerMonitor via GetInstance.

[Hongbo Min, 2012-10-11 12:34:07]

Hi, vandebo

The current PowerMonitor implementation is now a singleton instance using
base/memory/singleton.h instead of the original  way which maintains a global
pointer.

My concern about the singleton is, on Mac, since the dtor of PowerMonitor will
call PlatformDestroy to cleaup IO port resources, and as suggested in
singleton.h, "Singletons get problematic when they attempt to do much in their
destructor",  I am not sure whether it would happen in our case.

What is your opinion? Thanks.

[jam, 2012-10-11 15:38:19]

Which parts did you want me to review?

Regarding this statement in the description: "Since the power related APIs are
consumed across several layers including base/, net/ and ui/, other capabilities
exposed by SystemMonitor have to be also in base/ even if they are only used in
content/ or chrome/.", can you clarify what you mean? i.e. I thought we agreed
that if something is only used in chrome, it wouldn't be in base?

https://codereview.chromium.org/10959020/diff/19096/base/power_monitor/power_...
File base/power_monitor/power_monitor.h (right):

https://codereview.chromium.org/10959020/diff/19096/base/power_monitor/power_...
base/power_monitor/power_monitor.h:36: class BASE_EXPORT PowerObserver {
nit: put this interface in its own header

[vandebo (ex-Chrome), 2012-10-11 20:49:56]

On 2012/10/11 15:38:19, John Abd-El-Malek wrote:
> Which parts did you want me to review?

Hongbo.Min: When you add multiple reviewers you should always specify which part
of the change you'd like each of the reviewer to look at.
 
> Regarding this statement in the description: "Since the power related APIs are
> consumed across several layers including base/, net/ and ui/, other
capabilities
> exposed by SystemMonitor have to be also in base/ even if they are only used
in
> content/ or chrome/.", can you clarify what you mean? i.e. I thought we agreed
> that if something is only used in chrome, it wouldn't be in base?

That statement is wrong and needs to be updated.  As evidence by the files
changed, PowerMonitor is consumed in net/ and for windows fed by ui/


I'm sending this change to the try servers so you can see what might be broken
on platforms you haven't tested.

https://codereview.chromium.org/10959020/diff/12002/base/power_monitor/power_...
File base/power_monitor/power_monitor.h (right):

https://codereview.chromium.org/10959020/diff/12002/base/power_monitor/power_...
base/power_monitor/power_monitor.h:88: friend class HWNDMessageHandler;
On 2012/10/11 07:57:53, Hongbo Min wrote:
> On 2012/10/11 00:58:43, vandebo wrote:
> > Looks like the Mac implementation needs a friend function declaration as
well
> 
> If making a friend function for Mac, it will introduce additional type
> dependency in power_monitor.h, and lots of places also need to access
> ProcessPowerMessage, so I just keep the original methods instead of renaming
to
> HandlePowerEvent and making it private in this CL.

Part of the point of refactoring is to remove/make private the ProcessFoo
methods.  I don't think a friend declaration here would introduce new type
dependencies.

https://codereview.chromium.org/10959020/diff/19096/base/power_monitor/power_...
File base/power_monitor/power_monitor.cc (left):

https://codereview.chromium.org/10959020/diff/19096/base/power_monitor/power_...
base/power_monitor/power_monitor.cc:5: #include "base/threading/worker_pool.h"
It detected this as a copy from the wrong file, you might try dialing back the
similarity factor.

https://codereview.chromium.org/10959020/diff/19096/base/power_monitor/power_...
File base/power_monitor/power_monitor.h (right):

https://codereview.chromium.org/10959020/diff/19096/base/power_monitor/power_...
base/power_monitor/power_monitor.h:64: void AddObserver(PowerObserver*
observer);
Move these two methods down to line 87.  The constructor like and init type
methods should come first.

https://codereview.chromium.org/10959020/diff/19096/base/power_monitor/power_...
base/power_monitor/power_monitor.h:89: // Windows-specific handling of a
WM_POWERBROADCAST message.
These two methods need to be private.

https://codereview.chromium.org/10959020/diff/19096/base/power_monitor/power_...
base/power_monitor/power_monitor.h:104: friend class HiResTimerManagerTest;
Do you need this friend?  It looks like the test calls the observer
implementation directly.

https://codereview.chromium.org/10959020/diff/19096/base/power_monitor/power_...
base/power_monitor/power_monitor.h:132:
scoped_refptr<ObserverListThreadSafe<PowerObserver> > observers_;
This is created in the constructor and never destroyed, just make it a member
instead of a scoped_ptr.  It was probably a scoped_ptr to avoid a static
initializer, but Singleton should take care of that now.

https://codereview.chromium.org/10959020/diff/19096/base/power_monitor/power_...
File base/power_monitor/power_monitor_win.cc (right):

https://codereview.chromium.org/10959020/diff/19096/base/power_monitor/power_...
base/power_monitor/power_monitor_win.cc:8: void
PowerMonitor::ProcessWmPowerBroadcastMessage(int event_id) {
nit: blank line after namespace.

https://codereview.chromium.org/10959020/diff/19096/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager_unittest.cc (right):

https://codereview.chromium.org/10959020/diff/19096/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager_unittest.cc:104:
system_monitor_dummy_.reset(new base::SystemMonitor);
I don't think this line is needed.  Please remove it (and its include) and see
if the test still works.

[Hongbo Min, 2012-10-11 23:53:59]

On 2012/10/11 20:49:56, vandebo wrote:
> On 2012/10/11 15:38:19, John Abd-El-Malek wrote:
> > Which parts did you want me to review?
> 
> Hongbo.Min: When you add multiple reviewers you should always specify which
part
> of the change you'd like each of the reviewer to look at.

Thanks for your suggestion. I will keep it in my mind.

> On 2012/10/11 07:57:53, Hongbo Min wrote:
> > On 2012/10/11 00:58:43, vandebo wrote:
> > > Looks like the Mac implementation needs a friend function declaration as
> well
> > 
> > If making a friend function for Mac, it will introduce additional type
> > dependency in power_monitor.h, and lots of places also need to access
> > ProcessPowerMessage, so I just keep the original methods instead of renaming
> to
> > HandlePowerEvent and making it private in this CL.
> 
> Part of the point of refactoring is to remove/make private the ProcessFoo
> methods.  I don't think a friend declaration here would introduce new type
> dependencies.
See the following function to be a friend of PowerMonitor:
void SystemPowerEventCallback(void*,
                              io_service_t service,
                              natural_t message_type,
                              void* message_argument)
I am afraid io_service_t and natural_t should be forward-declared in
power_monitor.h.

[Hongbo Min, 2012-10-12 00:01:03]

vandebo,

Does it make more sense to define PowerObserver in a separate class file? It can
reduce the additional dependency on PowerMonitor for the client code, with that,
the client code is able to only include power_observer.h in their header file.

[vandebo (ex-Chrome), 2012-10-12 00:15:51]

On 2012/10/12 00:01:03, Hongbo Min wrote:
> vandebo,
> 
> Does it make more sense to define PowerObserver in a separate class file? It
can
> reduce the additional dependency on PowerMonitor for the client code, with
that,
> the client code is able to only include power_observer.h in their header file.

That's what jam@ requested and it looks like a good plan.

http://codereview.chromium.org/10959020/diff/12002/base/power_monitor/power_m...
File base/power_monitor/power_monitor.h (right):

http://codereview.chromium.org/10959020/diff/12002/base/power_monitor/power_m...
base/power_monitor/power_monitor.h:88: friend class HWNDMessageHandler;
On 2012/10/11 20:49:57, vandebo wrote:
> On 2012/10/11 07:57:53, Hongbo Min wrote:
> > On 2012/10/11 00:58:43, vandebo wrote:
> > > Looks like the Mac implementation needs a friend function declaration as
> well
> > 
> > If making a friend function for Mac, it will introduce additional type
> > dependency in power_monitor.h, and lots of places also need to access
> > ProcessPowerMessage, so I just keep the original methods instead of renaming
> to
> > HandlePowerEvent and making it private in this CL.
> 
> Part of the point of refactoring is to remove/make private the ProcessFoo
> methods.  I don't think a friend declaration here would introduce new type
> dependencies. 

If need be, you can create another function that mirrors the ProcessFoo
arguments within the Mac file in order to avoid forward declaring other types.

[Hongbo Min, 2012-10-12 14:43:53]

The patch is updated. Thank for the trybots result, it exposes lot of underlying
issues in the code base after splitting out PowerMonitor from SystemMonitor.

You may notice some unrelated files are also changed with including the missing
header file, such as base/message_loop.h. The reason is that those files
reference MessageLoop but not include the base/message_loop.h file directly, and
depends on the header files included in system_monitor.h which indirectly
includes base/message_loop.h.

http://codereview.chromium.org/10959020/diff/19096/base/hi_res_timer_manager_...
File base/hi_res_timer_manager_unittest.cc (left):

http://codereview.chromium.org/10959020/diff/19096/base/hi_res_timer_manager_...
base/hi_res_timer_manager_unittest.cc:16: scoped_ptr<base::SystemMonitor>
system_monitor(new base::SystemMonitor());
On 2012/10/11 07:57:53, Hongbo Min wrote:
> No need to create PowerMonitor since the HighResolutionTimerManager ctor has
> already accesses PowerMonitor via GetInstance.

Done.

http://codereview.chromium.org/10959020/diff/19096/base/power_monitor/power_m...
File base/power_monitor/power_monitor.cc (left):

http://codereview.chromium.org/10959020/diff/19096/base/power_monitor/power_m...
base/power_monitor/power_monitor.cc:5: #include "base/threading/worker_pool.h"
On 2012/10/11 20:49:57, vandebo wrote:
> It detected this as a copy from the wrong file, you might try dialing back the
> similarity factor.

Done.

http://codereview.chromium.org/10959020/diff/19096/base/power_monitor/power_m...
File base/power_monitor/power_monitor.h (right):

http://codereview.chromium.org/10959020/diff/19096/base/power_monitor/power_m...
base/power_monitor/power_monitor.h:36: class BASE_EXPORT PowerObserver {
On 2012/10/11 15:38:19, John Abd-El-Malek wrote:
> nit: put this interface in its own header

Done.

http://codereview.chromium.org/10959020/diff/19096/base/power_monitor/power_m...
base/power_monitor/power_monitor.h:64: void AddObserver(PowerObserver*
observer);
On 2012/10/11 20:49:57, vandebo wrote:
> Move these two methods down to line 87.  The constructor like and init type
> methods should come first.

Done.

http://codereview.chromium.org/10959020/diff/19096/base/power_monitor/power_m...
base/power_monitor/power_monitor.h:89: // Windows-specific handling of a
WM_POWERBROADCAST message.
On 2012/10/11 20:49:57, vandebo wrote:
> These two methods need to be private.

Done.

http://codereview.chromium.org/10959020/diff/19096/base/power_monitor/power_m...
base/power_monitor/power_monitor.h:104: friend class HiResTimerManagerTest;
On 2012/10/11 20:49:57, vandebo wrote:
> Do you need this friend?  It looks like the test calls the observer
> implementation directly. 

Removed it.

Done.

http://codereview.chromium.org/10959020/diff/19096/base/power_monitor/power_m...
base/power_monitor/power_monitor.h:132:
scoped_refptr<ObserverListThreadSafe<PowerObserver> > observers_;
On 2012/10/11 20:49:57, vandebo wrote:
> This is created in the constructor and never destroyed, just make it a member
> instead of a scoped_ptr.  It was probably a scoped_ptr to avoid a static
> initializer, but Singleton should take care of that now.

It should be destroyed on process exiting, the same life time as PowerMonitor
instance.

http://codereview.chromium.org/10959020/diff/19096/base/power_monitor/power_m...
File base/power_monitor/power_monitor_win.cc (right):

http://codereview.chromium.org/10959020/diff/19096/base/power_monitor/power_m...
base/power_monitor/power_monitor_win.cc:8: void
PowerMonitor::ProcessWmPowerBroadcastMessage(int event_id) {
On 2012/10/11 20:49:57, vandebo wrote:
> nit: blank line after namespace.

Done.

http://codereview.chromium.org/10959020/diff/19096/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_manager_unittest.cc (right):

http://codereview.chromium.org/10959020/diff/19096/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_manager_unittest.cc:104:
system_monitor_dummy_.reset(new base::SystemMonitor);
On 2012/10/11 20:49:57, vandebo wrote:
> I don't think this line is needed.  Please remove it (and its include) and see
> if the test still works.

Done.

[jam, 2012-10-12 17:55:13]

content & chrome lgtm

[willchan no longer on Chromium, 2012-10-12 18:11:59]

base/ is mostly fine with me, although my one nit is the new power_monitor.cc
test is not a branch. Are you using git or svn? If svn, can you redo it with an
svn cp prior to editing, so it's clear where it came from? If you're using git,
there's nothing you can do, so it's fine.

[willchan no longer on Chromium, 2012-10-12 18:17:15]

https://codereview.chromium.org/10959020/diff/41042/base/power_monitor/power_...
File base/power_monitor/power_monitor_mac.mm (right):

https://codereview.chromium.org/10959020/diff/41042/base/power_monitor/power_...
base/power_monitor/power_monitor_mac.mm:69: return;
This makes me minorly nervous, especially now that we lazily create the
PowerMonitor singleton instance. I think before, when we explicitly created it,
anyone trying to use PowerMonitor (SystemMonitor) early would crash on NULL,
since it didn't exist. This could lead to subtle, non-obvious errors. What do
you think about making this a CHECK_NE instead?

https://codereview.chromium.org/10959020/diff/41042/net/url_request/url_reque...
File net/url_request/url_request_job.h (right):

https://codereview.chromium.org/10959020/diff/41042/net/url_request/url_reque...
net/url_request/url_request_job.h:10: #include <map>
alphabetical order please

[vandebo (ex-Chrome), 2012-10-12 22:12:30]

https://codereview.chromium.org/10959020/diff/19096/base/power_monitor/power_...
File base/power_monitor/power_monitor.h (right):

https://codereview.chromium.org/10959020/diff/19096/base/power_monitor/power_...
base/power_monitor/power_monitor.h:132:
scoped_refptr<ObserverListThreadSafe<PowerObserver> > observers_;
On 2012/10/12 14:43:53, Hongbo Min wrote:
> On 2012/10/11 20:49:57, vandebo wrote:
> > This is created in the constructor and never destroyed, just make it a
member
> > instead of a scoped_ptr.  It was probably a scoped_ptr to avoid a static
> > initializer, but Singleton should take care of that now.
> 
> It should be destroyed on process exiting, the same life time as PowerMonitor
> instance.

Yes, making it a member will make it have the same lifetime.  Having it as a
scoped_ptr has overhead - two memory allocations instead of one.

https://codereview.chromium.org/10959020/diff/41042/base/power_monitor/power_...
File base/power_monitor/power_monitor.h (right):

https://codereview.chromium.org/10959020/diff/41042/base/power_monitor/power_...
base/power_monitor/power_monitor.h:72: friend struct
DefaultSingletonTraits<PowerMonitor>;
nit: put friend declarations right after private:

https://codereview.chromium.org/10959020/diff/41042/base/power_monitor/power_...
File base/power_monitor/power_monitor_mac.mm (right):

https://codereview.chromium.org/10959020/diff/41042/base/power_monitor/power_...
base/power_monitor/power_monitor_mac.mm:69: return;
On 2012/10/12 18:17:15, willchan wrote:
> This makes me minorly nervous, especially now that we lazily create the
> PowerMonitor singleton instance. I think before, when we explicitly created
it,
> anyone trying to use PowerMonitor (SystemMonitor) early would crash on NULL,
> since it didn't exist. This could lead to subtle, non-obvious errors. What do
> you think about making this a CHECK_NE instead?

SGTM

https://codereview.chromium.org/10959020/diff/41042/chrome/browser/system_mon...
File
chrome/browser/system_monitor/removable_device_notifications_mac_unittest.mm
(right):

https://codereview.chromium.org/10959020/diff/41042/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_mac_unittest.mm:52:
base::PowerMonitor::AllocateSystemIOPorts();
Just remove this line.  This test doesn't use PowerMonitor

https://codereview.chromium.org/10959020/diff/41042/content/browser/browser_m...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/10959020/diff/41042/content/browser/browser_m...
content/browser/browser_main_loop.cc:15: #include
"base/system_monitor/system_monitor.h"
SystemMonitor isn't used in this file, why add the include?

https://codereview.chromium.org/10959020/diff/41042/content/browser/gamepad/g...
File content/browser/gamepad/gamepad_test_helpers.h (left):

https://codereview.chromium.org/10959020/diff/41042/content/browser/gamepad/g...
content/browser/gamepad/gamepad_test_helpers.h:14: 
I don't think changes in this file are required (yet).  This has nothing to do
with PowerMonitor

[Hongbo Min, 2012-10-13 00:03:11]

On 2012/10/12 18:11:59, willchan wrote:
> base/ is mostly fine with me, although my one nit is the new power_monitor.cc
> test is not a branch. Are you using git or svn? If svn, can you redo it with
an
> svn cp prior to editing, so it's clear where it came from? If you're using
git,
> there's nothing you can do, so it's fine.

I am using git, the similarity threshold I use is 30, otherwise there will be
wrong for file renaming detection.

[Hongbo Min, 2012-10-13 00:09:12]

On 2012/10/12 22:12:30, vandebo wrote:
>
https://codereview.chromium.org/10959020/diff/19096/base/power_monitor/power_...
> File base/power_monitor/power_monitor.h (right):
> 
>
https://codereview.chromium.org/10959020/diff/19096/base/power_monitor/power_...
> base/power_monitor/power_monitor.h:132:
> scoped_refptr<ObserverListThreadSafe<PowerObserver> > observers_;
> On 2012/10/12 14:43:53, Hongbo Min wrote:
> > On 2012/10/11 20:49:57, vandebo wrote:
> > > This is created in the constructor and never destroyed, just make it a
> member
> > > instead of a scoped_ptr.  It was probably a scoped_ptr to avoid a static
> > > initializer, but Singleton should take care of that now.
> > 
> > It should be destroyed on process exiting, the same life time as
PowerMonitor
> > instance.
> 
> Yes, making it a member will make it have the same lifetime.  Having it as a
> scoped_ptr has overhead - two memory allocations instead of one.

The fact is that you can not allocate ObserverListThreadSafe on the stack since
the dtor of ObserverListThreadSafe is private.

>
https://codereview.chromium.org/10959020/diff/41042/content/browser/gamepad/g...
> File content/browser/gamepad/gamepad_test_helpers.h (left):
> 
>
https://codereview.chromium.org/10959020/diff/41042/content/browser/gamepad/g...
> content/browser/gamepad/gamepad_test_helpers.h:14: 
> I don't think changes in this file are required (yet).  This has nothing to do
> with PowerMonitor

Since gamepad_test_helpers.cc uses PowerMonitor::AllocateSystemIOPorts. Need to
the author to double-check whether it is required, brettw@, is it safe to remove
the calling of SystemMonitor::AllocateSystemIOPorts?

[Hongbo Min, 2012-10-13 06:36:07]

https://codereview.chromium.org/10959020/diff/41042/base/power_monitor/power_...
File base/power_monitor/power_monitor.h (right):

https://codereview.chromium.org/10959020/diff/41042/base/power_monitor/power_...
base/power_monitor/power_monitor.h:72: friend struct
DefaultSingletonTraits<PowerMonitor>;
On 2012/10/12 22:12:30, vandebo wrote:
> nit: put friend declarations right after private:

Done.

https://codereview.chromium.org/10959020/diff/41042/base/power_monitor/power_...
File base/power_monitor/power_monitor_mac.mm (right):

https://codereview.chromium.org/10959020/diff/41042/base/power_monitor/power_...
base/power_monitor/power_monitor_mac.mm:69: return;
On 2012/10/12 22:12:30, vandebo wrote:
> On 2012/10/12 18:17:15, willchan wrote:
> > This makes me minorly nervous, especially now that we lazily create the
> > PowerMonitor singleton instance. I think before, when we explicitly created
> it,
> > anyone trying to use PowerMonitor (SystemMonitor) early would crash on NULL,
> > since it didn't exist. This could lead to subtle, non-obvious errors. What
do
> > you think about making this a CHECK_NE instead?
> 
> SGTM

Done.

https://codereview.chromium.org/10959020/diff/41042/chrome/browser/system_mon...
File
chrome/browser/system_monitor/removable_device_notifications_mac_unittest.mm
(right):

https://codereview.chromium.org/10959020/diff/41042/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_mac_unittest.mm:52:
base::PowerMonitor::AllocateSystemIOPorts();
On 2012/10/12 22:12:30, vandebo wrote:
> Just remove this line.  This test doesn't use PowerMonitor

Done.

https://codereview.chromium.org/10959020/diff/41042/content/browser/browser_m...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/10959020/diff/41042/content/browser/browser_m...
content/browser/browser_main_loop.cc:15: #include
"base/system_monitor/system_monitor.h"
On 2012/10/12 22:12:30, vandebo wrote:
> SystemMonitor isn't used in this file, why add the include?

Because BrowserMainLoop still references SystemMonitor member, it do need to
include system_monitor.h explicitly since the indirect inclusion from
hi_res_timer_manager.h was broken after including power_observer.h.

https://codereview.chromium.org/10959020/diff/41042/net/url_request/url_reque...
File net/url_request/url_request_job.h (right):

https://codereview.chromium.org/10959020/diff/41042/net/url_request/url_reque...
net/url_request/url_request_job.h:10: #include <map>
On 2012/10/12 18:17:15, willchan wrote:
> alphabetical order please

Done.

[Hongbo Min, 2012-10-13 08:22:15]

https://codereview.chromium.org/10959020/diff/41042/base/power_monitor/power_...
File base/power_monitor/power_monitor_mac.mm (right):

https://codereview.chromium.org/10959020/diff/41042/base/power_monitor/power_...
base/power_monitor/power_monitor_mac.mm:69: return;
On 2012/10/13 06:36:07, Hongbo Min wrote:
> On 2012/10/12 22:12:30, vandebo wrote:
> > On 2012/10/12 18:17:15, willchan wrote:
> > > This makes me minorly nervous, especially now that we lazily create the
> > > PowerMonitor singleton instance. I think before, when we explicitly
created
> > it,
> > > anyone trying to use PowerMonitor (SystemMonitor) early would crash on
NULL,
> > > since it didn't exist. This could lead to subtle, non-obvious errors. What
> do
> > > you think about making this a CHECK_NE instead?
> > 
> > SGTM
> 
> Done.

Willchan, I try to use CHECK_NE instead, but some tests failed on Mac (see the
build bot result of Patch Set 6). So I revert to keep it again.

I suggest to fix this issue in a new CL.

[vandebo (ex-Chrome), 2012-10-15 23:41:25]

https://codereview.chromium.org/10959020/diff/41042/base/power_monitor/power_...
File base/power_monitor/power_monitor_mac.mm (right):

https://codereview.chromium.org/10959020/diff/41042/base/power_monitor/power_...
base/power_monitor/power_monitor_mac.mm:69: return;
On 2012/10/13 08:22:15, Hongbo Min wrote:
> On 2012/10/13 06:36:07, Hongbo Min wrote:
> > On 2012/10/12 22:12:30, vandebo wrote:
> > > On 2012/10/12 18:17:15, willchan wrote:
> > > > This makes me minorly nervous, especially now that we lazily create the
> > > > PowerMonitor singleton instance. I think before, when we explicitly
> created
> > > it,
> > > > anyone trying to use PowerMonitor (SystemMonitor) early would crash on
> NULL,
> > > > since it didn't exist. This could lead to subtle, non-obvious errors.
What
> > do
> > > > you think about making this a CHECK_NE instead?
> > > 
> > > SGTM
> > 
> > Done.
> 
> Willchan, I try to use CHECK_NE instead, but some tests failed on Mac (see the
> build bot result of Patch Set 6). So I revert to keep it again.
> 
> I suggest to fix this issue in a new CL.

It's important to get this stuff right because it will cause crashes otherwise. 
It looks like you need to make SystemMonitor not a class member of
PowerMonitorTest to fix that problem.  AllocateSystemIOPorts has to be called
before PowerMonitor is constructed.

[Hongbo Min, 2012-10-16 13:43:31]

vandebo,

The updated patch is mainly for the change brought by using CHECK_NE instead of
DCHECK_NE in PowerMonitor::PlatformInit.

I hope it would not give your a surprise due to a bundle of files are required
to be changed to call AllocateSystemIOPorts before PowerMonitor construction.
The reason is mainly caused by URLRequestJob which is a PowerObserver and in its
ctor it will get the PowerMonitor instance. In the test code, any code using
URLRequestJob requires the AllocateSystemIOPorts is called prior to PowerMonitor
ctor.

Pls help review it again.

[Hongbo Min, 2012-10-16 13:53:52]

http://codereview.chromium.org/10959020/diff/68050/base/power_monitor/power_m...
File base/power_monitor/power_monitor_mac.mm (right):

http://codereview.chromium.org/10959020/diff/68050/base/power_monitor/power_m...
base/power_monitor/power_monitor_mac.mm:53: if (g_system_power_io_port != 0u)
Here I removed the DCHECK_EQ stmt because AllocateSystemIOPorts is designed to
be called once, otherwise, the g_system_power_io_port will be leaked without
IOServiceClose calling when AllocateSystemSystemIOPorts is called multiple
times.

If the AllocateSystemIOPort was ever called and so g_system_power_io_port is
retrieved correctly, we just returns the value got already immediately.

http://codereview.chromium.org/10959020/diff/68050/net/proxy/proxy_script_fet...
File net/proxy/proxy_script_fetcher_impl_unittest.cc (right):

http://codereview.chromium.org/10959020/diff/68050/net/proxy/proxy_script_fet...
net/proxy/proxy_script_fetcher_impl_unittest.cc:219:
base::PowerMonitor::AllocateSystemIOPorts();
ProxyScriptFetcherImplTest indirectly has a dependency on URLRequestJob.

http://codereview.chromium.org/10959020/diff/68050/webkit/glue/dom_operations...
File webkit/glue/dom_operations_unittest.cc (right):

http://codereview.chromium.org/10959020/diff/68050/webkit/glue/dom_operations...
webkit/glue/dom_operations_unittest.cc:44:
base::PowerMonitor::AllocateSystemIOPorts();
Because the DomOperationTests indirectly has a dependency on URLRequestJob.

[Hongbo Min, 2012-10-16 14:14:40]

http://codereview.chromium.org/10959020/diff/68051/base/power_monitor/power_m...
File base/power_monitor/power_monitor_mac.mm (right):

http://codereview.chromium.org/10959020/diff/68051/base/power_monitor/power_m...
base/power_monitor/power_monitor_mac.mm:53: if (g_system_power_io_port != 0u)
base/power_monitor/power_monitor_mac.mm:53: if (g_system_power_io_port != 0u)
Here I removed the DCHECK_EQ stmt because AllocateSystemIOPorts is designed to
be called once, otherwise, the g_system_power_io_port will be leaked without
IOServiceClose calling when AllocateSystemSystemIOPorts is called multiple
times.

If the AllocateSystemIOPort was ever called and so g_system_power_io_port is
retrieved correctly, we just returns the value got already immediately.

http://codereview.chromium.org/10959020/diff/68051/net/proxy/proxy_script_fet...
File net/proxy/proxy_script_fetcher_impl_unittest.cc (right):

http://codereview.chromium.org/10959020/diff/68051/net/proxy/proxy_script_fet...
net/proxy/proxy_script_fetcher_impl_unittest.cc:219:
base::PowerMonitor::AllocateSystemIOPorts();
ProxyScriptFetcherImplTest indirectly has a dependency on URLRequestJob.

http://codereview.chromium.org/10959020/diff/68051/webkit/glue/dom_operations...
File webkit/glue/dom_operations_unittest.cc (right):

http://codereview.chromium.org/10959020/diff/68051/webkit/glue/dom_operations...
webkit/glue/dom_operations_unittest.cc:44:
base::PowerMonitor::AllocateSystemIOPorts();
Because the DomOperationTests indirectly has a dependency on URLRequestJob.

http://codereview.chromium.org/10959020/diff/68051/webkit/tools/test_shell/te...
File webkit/tools/test_shell/test_shell.cc (right):

http://codereview.chromium.org/10959020/diff/68051/webkit/tools/test_shell/te...
webkit/tools/test_shell/test_shell.cc:154:
PowerMonitor::AllocateSystemIOPorts();
Because the TestShell indirectly has a dependency on URLRequestJob.

[vandebo (ex-Chrome), 2012-10-16 18:50:16]

There are some conflicting changes in 162124.  I thought it was committed last
week, but it just went in.  Once you rebase after that change, I think the
change will be ready.

http://codereview.chromium.org/10959020/diff/68051/base/power_monitor/power_m...
File base/power_monitor/power_monitor_mac.mm (right):

http://codereview.chromium.org/10959020/diff/68051/base/power_monitor/power_m...
base/power_monitor/power_monitor_mac.mm:53: if (g_system_power_io_port != 0u)
On 2012/10/16 14:14:40, Hongbo Min wrote:
> base/power_monitor/power_monitor_mac.mm:53: if (g_system_power_io_port != 0u)
> Here I removed the DCHECK_EQ stmt because AllocateSystemIOPorts is designed to
> be called once, otherwise, the g_system_power_io_port will be leaked without
> IOServiceClose calling when AllocateSystemSystemIOPorts is called multiple
> times.
> 
> If the AllocateSystemIOPort was ever called and so g_system_power_io_port is
> retrieved correctly, we just returns the value got already immediately.

Hmm, this is a consequence of changing it to a Singleton. I'm not happy with the
extra changes that have to be made, but it's not too bad either.  I'll defer to
Will on this one.

http://codereview.chromium.org/10959020/diff/68051/webkit/tools/test_shell/te...
File webkit/tools/test_shell/test_shell.cc (right):

http://codereview.chromium.org/10959020/diff/68051/webkit/tools/test_shell/te...
webkit/tools/test_shell/test_shell.cc:154:
PowerMonitor::AllocateSystemIOPorts();
On 2012/10/16 14:14:40, Hongbo Min wrote:
> Because the TestShell indirectly has a dependency on URLRequestJob.

Move this down to line 165, above SetUserAgent

[willchan no longer on Chromium, 2012-10-16 20:32:37]

Let's fix the layering violation. If you need a test API, please explicitly add
it.

http://codereview.chromium.org/10959020/diff/68051/base/power_monitor/power_m...
File base/power_monitor/power_monitor.h (right):

http://codereview.chromium.org/10959020/diff/68051/base/power_monitor/power_m...
base/power_monitor/power_monitor.h:30: namespace views {
This is a layering violation. Can we remove this requirement?

http://codereview.chromium.org/10959020/diff/68051/base/power_monitor/power_m...
File base/power_monitor/power_monitor_mac.mm (right):

http://codereview.chromium.org/10959020/diff/68051/base/power_monitor/power_m...
base/power_monitor/power_monitor_mac.mm:53: if (g_system_power_io_port != 0u)
On 2012/10/16 18:50:16, vandebo wrote:
> On 2012/10/16 14:14:40, Hongbo Min wrote:
> > base/power_monitor/power_monitor_mac.mm:53: if (g_system_power_io_port !=
0u)
> > Here I removed the DCHECK_EQ stmt because AllocateSystemIOPorts is designed
to
> > be called once, otherwise, the g_system_power_io_port will be leaked without
> > IOServiceClose calling when AllocateSystemSystemIOPorts is called multiple
> > times.
> > 
> > If the AllocateSystemIOPort was ever called and so g_system_power_io_port is
> > retrieved correctly, we just returns the value got already immediately.
> 
> Hmm, this is a consequence of changing it to a Singleton. I'm not happy with
the
> extra changes that have to be made, but it's not too bad either.  I'll defer
to
> Will on this one.

I'm confused. Why is this better than before? Let me restate my understanding.
Before:
* Asserts in debug mode if this is ever called more than once.
* Leaks in release mode if this is ever called more than once.

After:
* Gracefully handles calling it more than once.

I think it's a bug to call it more than once and would prefer to catch it. I do
think it's an improvement to gracefully handle it in release mode, but it'd be
great to keep the DCHECK there for debug mode to prevent people from calling
this multiple times.

http://codereview.chromium.org/10959020/diff/68051/base/power_monitor/power_m...
File base/power_monitor/power_monitor_unittest.cc (right):

http://codereview.chromium.org/10959020/diff/68051/base/power_monitor/power_m...
base/power_monitor/power_monitor_unittest.cc:62:
DISALLOW_COPY_AND_ASSIGN(PowerMonitorTest);
Don't worry about this macro for test fixtures. No one ever copies them anyways
(that'd be weird).

http://codereview.chromium.org/10959020/diff/68051/base/power_monitor/power_m...
base/power_monitor/power_monitor_unittest.cc:76:
EXPECT_EQ(test[0].power_state_changes(), 0);
Your expectations are all backwards. It should be (expected, actual) in order to
match gtest message output.

[vandebo (ex-Chrome), 2012-10-16 23:55:08]

http://codereview.chromium.org/10959020/diff/68051/base/power_monitor/power_m...
File base/power_monitor/power_monitor.h (right):

http://codereview.chromium.org/10959020/diff/68051/base/power_monitor/power_m...
base/power_monitor/power_monitor.h:30: namespace views {
On 2012/10/16 20:32:37, willchan wrote:
> This is a layering violation. Can we remove this requirement?

We could hide it by using inheritance instead of friends to call
ProcessPowerEventHelper.

You had earlier suggested using an interface-impl pattern to have per platform
implementations, but I don't know of a way to use that pattern with Singleton
where the interface and impl are in different modules.

http://codereview.chromium.org/10959020/diff/68051/base/power_monitor/power_m...
File base/power_monitor/power_monitor_mac.mm (right):

http://codereview.chromium.org/10959020/diff/68051/base/power_monitor/power_m...
base/power_monitor/power_monitor_mac.mm:53: if (g_system_power_io_port != 0u)
On 2012/10/16 20:32:37, willchan wrote:
> On 2012/10/16 18:50:16, vandebo wrote:
> > On 2012/10/16 14:14:40, Hongbo Min wrote:
> > > base/power_monitor/power_monitor_mac.mm:53: if (g_system_power_io_port !=
> 0u)
> > > Here I removed the DCHECK_EQ stmt because AllocateSystemIOPorts is
designed
> to
> > > be called once, otherwise, the g_system_power_io_port will be leaked
without
> > > IOServiceClose calling when AllocateSystemSystemIOPorts is called multiple
> > > times.
> > > 
> > > If the AllocateSystemIOPort was ever called and so g_system_power_io_port
is
> > > retrieved correctly, we just returns the value got already immediately.
> > 
> > Hmm, this is a consequence of changing it to a Singleton. I'm not happy with
> the
> > extra changes that have to be made, but it's not too bad either.  I'll defer
> to
> > Will on this one.
> 
> I'm confused. Why is this better than before? Let me restate my understanding.
> Before:
> * Asserts in debug mode if this is ever called more than once.
> * Leaks in release mode if this is ever called more than once.
> 
> After:
> * Gracefully handles calling it more than once.
> 
> I think it's a bug to call it more than once and would prefer to catch it. I
do
> think it's an improvement to gracefully handle it in release mode, but it'd be
> great to keep the DCHECK there for debug mode to prevent people from calling
> this multiple times.

Maybe this and the other issue is why it wasn't a Singleton before... As a
Singleton it's created once per test binary.  However, the test instance will
call AllocateSystemIOPorts once per test.

[Hongbo Min, 2012-10-17 00:23:21]

On 2012/10/16 23:55:08, vandebo wrote:
>
http://codereview.chromium.org/10959020/diff/68051/base/power_monitor/power_m...
> File base/power_monitor/power_monitor.h (right):
> 
>
http://codereview.chromium.org/10959020/diff/68051/base/power_monitor/power_m...
> base/power_monitor/power_monitor.h:30: namespace views {
> On 2012/10/16 20:32:37, willchan wrote:
> > This is a layering violation. Can we remove this requirement?
> 
> We could hide it by using inheritance instead of friends to call
> ProcessPowerEventHelper.
> 
> You had earlier suggested using an interface-impl pattern to have per platform
> implementations, but I don't know of a way to use that pattern with Singleton
> where the interface and impl are in different modules.
>

Do you mean we need to abstract the PowerMonitor interface and provide impls for
each platform? e.g. PowerMonitorMac, PowerMonitorWin etc

>
http://codereview.chromium.org/10959020/diff/68051/base/power_monitor/power_m...
> File base/power_monitor/power_monitor_mac.mm (right):
> 
>
http://codereview.chromium.org/10959020/diff/68051/base/power_monitor/power_m...
> base/power_monitor/power_monitor_mac.mm:53: if (g_system_power_io_port != 0u)
> On 2012/10/16 20:32:37, willchan wrote:
> > On 2012/10/16 18:50:16, vandebo wrote:
> > > On 2012/10/16 14:14:40, Hongbo Min wrote:
> > > > base/power_monitor/power_monitor_mac.mm:53: if (g_system_power_io_port
!=
> > 0u)
> > > > Here I removed the DCHECK_EQ stmt because AllocateSystemIOPorts is
> designed
> > to
> > > > be called once, otherwise, the g_system_power_io_port will be leaked
> without
> > > > IOServiceClose calling when AllocateSystemSystemIOPorts is called
multiple
> > > > times.
> > > > 
> > > > If the AllocateSystemIOPort was ever called and so
g_system_power_io_port
> is
> > > > retrieved correctly, we just returns the value got already immediately.
> > > 
> > > Hmm, this is a consequence of changing it to a Singleton. I'm not happy
with
> > the
> > > extra changes that have to be made, but it's not too bad either.  I'll
defer
> > to
> > > Will on this one.
> > 
> > I'm confused. Why is this better than before? Let me restate my
understanding.
> > Before:
> > * Asserts in debug mode if this is ever called more than once.
> > * Leaks in release mode if this is ever called more than once.
> > 
> > After:
> > * Gracefully handles calling it more than once.
> > 
> > I think it's a bug to call it more than once and would prefer to catch it. I
> do
> > think it's an improvement to gracefully handle it in release mode, but it'd
be
> > great to keep the DCHECK there for debug mode to prevent people from calling
> > this multiple times.
> 
> Maybe this and the other issue is why it wasn't a Singleton before... As a
> Singleton it's created once per test binary.  However, the test instance will
> call AllocateSystemIOPorts once per test.

Yes, Each test instance will call AllocateSystemIOPorts and so the
AllocateSystemIOPorts is called multiple times. This is another reason why I
change the AllocateSystemIOPorts impl which returns immediately if it has been
called already.

[Hongbo Min, 2012-10-17 08:11:06]

On 2012/10/16 20:32:37, willchan wrote:
> Let's fix the layering violation. If you need a test API, please explicitly
add
> it.
> 
>
http://codereview.chromium.org/10959020/diff/68051/base/power_monitor/power_m...
> File base/power_monitor/power_monitor.h (right):
> 
>
http://codereview.chromium.org/10959020/diff/68051/base/power_monitor/power_m...
> base/power_monitor/power_monitor.h:30: namespace views {
> This is a layering violation. Can we remove this requirement?

The violation arises from the refactoring requirement for making the ProcessFoo
as protected or private since it exposes the implementation details as described
in http://crbug.com/149059.

Since the ProcessPowerMessage is private right now, but the class in other layer
need to access it, e.g. HWNDMessageHandler in views.

Regarding the inheritance approach to remove the friend declaration, I think it
is actually a heavy-weight approach because it introduces more sub-classes which
seems to not be necessary.

Let's go back the refactoring requirement again, why must we make the
ProcessPowerMessage as private?
In fact, the current PowerMonitor implementation does't take over the whole
responsibility of monitoring power state change. The power state change is
actually triggered by other external component, e.g. HWNDMessageHandler on
Windows, OnBatteryingChange on android. Its main responsibility is just to
notify the observers that power state change. Therefore, how about making
ProcessPowerMessage as public to avoid the layering violation?

[willchan no longer on Chromium, 2012-10-17 16:59:46]

How about we take the easy way out and use UNIT_TEST defines to expose
a public ProcessPowerMessageForTests() that inlines to calling
ProcessPowerMessage().

On Wed, Oct 17, 2012 at 1:11 AM,  <hongbo.min@intel.com> wrote:
> On 2012/10/16 20:32:37, willchan wrote:
>>
>> Let's fix the layering violation. If you need a test API, please
>> explicitly
>
> add
>>
>> it.
>
>
>
>
http://codereview.chromium.org/10959020/diff/68051/base/power_monitor/power_m...
>>
>> File base/power_monitor/power_monitor.h (right):
>
>
>
>
http://codereview.chromium.org/10959020/diff/68051/base/power_monitor/power_m...
>>
>> base/power_monitor/power_monitor.h:30: namespace views {
>>
>> This is a layering violation. Can we remove this requirement?
>
>
> The violation arises from the refactoring requirement for making the
> ProcessFoo
> as protected or private since it exposes the implementation details as
> described
> in http://crbug.com/149059.
>
> Since the ProcessPowerMessage is private right now, but the class in other
> layer
> need to access it, e.g. HWNDMessageHandler in views.
>
> Regarding the inheritance approach to remove the friend declaration, I think
> it
> is actually a heavy-weight approach because it introduces more sub-classes
> which
> seems to not be necessary.
>
> Let's go back the refactoring requirement again, why must we make the
> ProcessPowerMessage as private?
> In fact, the current PowerMonitor implementation does't take over the whole
> responsibility of monitoring power state change. The power state change is
> actually triggered by other external component, e.g. HWNDMessageHandler on
> Windows, OnBatteryingChange on android. Its main responsibility is just to
> notify the observers that power state change. Therefore, how about making
> ProcessPowerMessage as public to avoid the layering violation?
>
>
> http://codereview.chromium.org/10959020/

[vandebo (ex-Chrome), 2012-10-17 17:31:01]

On 2012/10/17 16:59:46, willchan wrote:
> How about we take the easy way out and use UNIT_TEST defines to expose
> a public ProcessPowerMessageForTests() that inlines to calling
> ProcessPowerMessage().

It's not needed just for testing, the friend/whatever is needed for the
production code.  Let's hash this out over chat or VC when you're free.

> 
> On Wed, Oct 17, 2012 at 1:11 AM,  <mailto:hongbo.min@intel.com> wrote:
> > On 2012/10/16 20:32:37, willchan wrote:
> >>
> >> Let's fix the layering violation. If you need a test API, please
> >> explicitly
> >
> > add
> >>
> >> it.
> >
> >
> >
> >
>
http://codereview.chromium.org/10959020/diff/68051/base/power_monitor/power_m...
> >>
> >> File base/power_monitor/power_monitor.h (right):
> >
> >
> >
> >
>
http://codereview.chromium.org/10959020/diff/68051/base/power_monitor/power_m...
> >>
> >> base/power_monitor/power_monitor.h:30: namespace views {
> >>
> >> This is a layering violation. Can we remove this requirement?
> >
> >
> > The violation arises from the refactoring requirement for making the
> > ProcessFoo
> > as protected or private since it exposes the implementation details as
> > described
> > in http://crbug.com/149059.
> >
> > Since the ProcessPowerMessage is private right now, but the class in other
> > layer
> > need to access it, e.g. HWNDMessageHandler in views.
> >
> > Regarding the inheritance approach to remove the friend declaration, I think
> > it
> > is actually a heavy-weight approach because it introduces more sub-classes
> > which
> > seems to not be necessary.
> >
> > Let's go back the refactoring requirement again, why must we make the
> > ProcessPowerMessage as private?
> > In fact, the current PowerMonitor implementation does't take over the whole
> > responsibility of monitoring power state change. The power state change is
> > actually triggered by other external component, e.g. HWNDMessageHandler on
> > Windows, OnBatteryingChange on android. Its main responsibility is just to
> > notify the observers that power state change. Therefore, how about making
> > ProcessPowerMessage as public to avoid the layering violation?
> >
> >
> > http://codereview.chromium.org/10959020/

[willchan no longer on Chromium, 2012-10-17 18:59:26]

Oh fascinating! I see, thanks for pointing that out. I see...so views
*also* needs to generate power events? That's intriguing.

I think the Singleton lazy instantiation pattern is fine for
registering observers, but if we have multiple possible sources of
events, we probably need to explicitly set the source. It sounds to me
like we need to add a SetSource() member function or something on
PowerMonitor that CHECKs to make sure it's only called once. Then we
should provide a base::PowerMonitorImpl that would be a good default,
but also be composable. We'd probably want to friend a base class with
a protected member function that allowed subclasses (like
base::PowerMonitorImpl and whatever views thing) to call the
ProcessPowerEvent() thing.

That's my rough suggestion. Steve, I'm happy to chat about this.

On Wed, Oct 17, 2012 at 10:31 AM,  <vandebo@chromium.org> wrote:
> On 2012/10/17 16:59:46, willchan wrote:
>>
>> How about we take the easy way out and use UNIT_TEST defines to expose
>> a public ProcessPowerMessageForTests() that inlines to calling
>> ProcessPowerMessage().
>
>
> It's not needed just for testing, the friend/whatever is needed for the
> production code.  Let's hash this out over chat or VC when you're free.
>
>
>
>> On Wed, Oct 17, 2012 at 1:11 AM,  <mailto:hongbo.min@intel.com> wrote:
>> > On 2012/10/16 20:32:37, willchan wrote:
>> >>
>> >> Let's fix the layering violation. If you need a test API, please
>> >> explicitly
>> >
>> > add
>> >>
>> >> it.
>> >
>> >
>> >
>> >
>
>
>
http://codereview.chromium.org/10959020/diff/68051/base/power_monitor/power_m...
>>
>> >>
>> >> File base/power_monitor/power_monitor.h (right):
>> >
>> >
>> >
>> >
>
>
>
http://codereview.chromium.org/10959020/diff/68051/base/power_monitor/power_m...
>>
>> >>
>> >> base/power_monitor/power_monitor.h:30: namespace views {
>> >>
>> >> This is a layering violation. Can we remove this requirement?
>> >
>> >
>> > The violation arises from the refactoring requirement for making the
>> > ProcessFoo
>> > as protected or private since it exposes the implementation details as
>> > described
>> > in http://crbug.com/149059.
>> >
>> > Since the ProcessPowerMessage is private right now, but the class in
>> > other
>> > layer
>> > need to access it, e.g. HWNDMessageHandler in views.
>> >
>> > Regarding the inheritance approach to remove the friend declaration, I
>> > think
>> > it
>> > is actually a heavy-weight approach because it introduces more
>> > sub-classes
>> > which
>> > seems to not be necessary.
>> >
>> > Let's go back the refactoring requirement again, why must we make the
>> > ProcessPowerMessage as private?
>> > In fact, the current PowerMonitor implementation does't take over the
>> > whole
>> > responsibility of monitoring power state change. The power state change
>> > is
>> > actually triggered by other external component, e.g. HWNDMessageHandler
>> > on
>> > Windows, OnBatteryingChange on android. Its main responsibility is just
>> > to
>> > notify the observers that power state change. Therefore, how about
>> > making
>> > ProcessPowerMessage as public to avoid the layering violation?
>> >
>> >
>> > http://codereview.chromium.org/10959020/
>
>
>
>
> http://codereview.chromium.org/10959020/

[willchan no longer on Chromium, 2012-10-17 18:59:49]

Oh, and my naming sucks, so feel free to rename anything I said.

On Wed, Oct 17, 2012 at 11:59 AM, William Chan (陈智昌)
<willchan@chromium.org> wrote:
> Oh fascinating! I see, thanks for pointing that out. I see...so views
> *also* needs to generate power events? That's intriguing.
>
> I think the Singleton lazy instantiation pattern is fine for
> registering observers, but if we have multiple possible sources of
> events, we probably need to explicitly set the source. It sounds to me
> like we need to add a SetSource() member function or something on
> PowerMonitor that CHECKs to make sure it's only called once. Then we
> should provide a base::PowerMonitorImpl that would be a good default,
> but also be composable. We'd probably want to friend a base class with
> a protected member function that allowed subclasses (like
> base::PowerMonitorImpl and whatever views thing) to call the
> ProcessPowerEvent() thing.
>
> That's my rough suggestion. Steve, I'm happy to chat about this.
>
> On Wed, Oct 17, 2012 at 10:31 AM,  <vandebo@chromium.org> wrote:
>> On 2012/10/17 16:59:46, willchan wrote:
>>>
>>> How about we take the easy way out and use UNIT_TEST defines to expose
>>> a public ProcessPowerMessageForTests() that inlines to calling
>>> ProcessPowerMessage().
>>
>>
>> It's not needed just for testing, the friend/whatever is needed for the
>> production code.  Let's hash this out over chat or VC when you're free.
>>
>>
>>
>>> On Wed, Oct 17, 2012 at 1:11 AM,  <mailto:hongbo.min@intel.com> wrote:
>>> > On 2012/10/16 20:32:37, willchan wrote:
>>> >>
>>> >> Let's fix the layering violation. If you need a test API, please
>>> >> explicitly
>>> >
>>> > add
>>> >>
>>> >> it.
>>> >
>>> >
>>> >
>>> >
>>
>>
>>
http://codereview.chromium.org/10959020/diff/68051/base/power_monitor/power_m...
>>>
>>> >>
>>> >> File base/power_monitor/power_monitor.h (right):
>>> >
>>> >
>>> >
>>> >
>>
>>
>>
http://codereview.chromium.org/10959020/diff/68051/base/power_monitor/power_m...
>>>
>>> >>
>>> >> base/power_monitor/power_monitor.h:30: namespace views {
>>> >>
>>> >> This is a layering violation. Can we remove this requirement?
>>> >
>>> >
>>> > The violation arises from the refactoring requirement for making the
>>> > ProcessFoo
>>> > as protected or private since it exposes the implementation details as
>>> > described
>>> > in http://crbug.com/149059.
>>> >
>>> > Since the ProcessPowerMessage is private right now, but the class in
>>> > other
>>> > layer
>>> > need to access it, e.g. HWNDMessageHandler in views.
>>> >
>>> > Regarding the inheritance approach to remove the friend declaration, I
>>> > think
>>> > it
>>> > is actually a heavy-weight approach because it introduces more
>>> > sub-classes
>>> > which
>>> > seems to not be necessary.
>>> >
>>> > Let's go back the refactoring requirement again, why must we make the
>>> > ProcessPowerMessage as private?
>>> > In fact, the current PowerMonitor implementation does't take over the
>>> > whole
>>> > responsibility of monitoring power state change. The power state change
>>> > is
>>> > actually triggered by other external component, e.g. HWNDMessageHandler
>>> > on
>>> > Windows, OnBatteryingChange on android. Its main responsibility is just
>>> > to
>>> > notify the observers that power state change. Therefore, how about
>>> > making
>>> > ProcessPowerMessage as public to avoid the layering violation?
>>> >
>>> >
>>> > http://codereview.chromium.org/10959020/
>>
>>
>>
>>
>> http://codereview.chromium.org/10959020/

[vandebo (ex-Chrome), 2012-10-17 20:09:13]

On 2012/10/17 18:59:26, willchan wrote:
> Oh fascinating! I see, thanks for pointing that out. I see...so views
> *also* needs to generate power events? That's intriguing.

To clarify, on windows, views is the only source of power events, it's
not an additional source.

> I think the Singleton lazy instantiation pattern is fine for
> registering observers, but if we have multiple possible sources of
> events, we probably need to explicitly set the source. It sounds to me
> like we need to add a SetSource() member function or something on
> PowerMonitor that CHECKs to make sure it's only called once. Then we
> should provide a base::PowerMonitorImpl that would be a good default,
> but also be composable. We'd probably want to friend a base class with
> a protected member function that allowed subclasses (like
> base::PowerMonitorImpl and whatever views thing) to call the
> ProcessPowerEvent() thing.

To make sure I understand, I'll restate what you said, please let me know
if I get it wrong....

Add PowerMonitor::SetSource(base::PowerMonitorSource* source), which checks
that it is only called once.  It will have a protected method that exposes
the ProcessPowerEvent functionality and will be a friend of PowerMonitor
to allow the call.

SGTM

[willchan no longer on Chromium, 2012-10-17 20:16:04]

Yep.

On Wed, Oct 17, 2012 at 1:09 PM,  <vandebo@chromium.org> wrote:
> On 2012/10/17 18:59:26, willchan wrote:
>>
>> Oh fascinating! I see, thanks for pointing that out. I see...so views
>> *also* needs to generate power events? That's intriguing.
>
>
> To clarify, on windows, views is the only source of power events, it's
> not an additional source.
>
>
>> I think the Singleton lazy instantiation pattern is fine for
>> registering observers, but if we have multiple possible sources of
>> events, we probably need to explicitly set the source. It sounds to me
>> like we need to add a SetSource() member function or something on
>> PowerMonitor that CHECKs to make sure it's only called once. Then we
>> should provide a base::PowerMonitorImpl that would be a good default,
>> but also be composable. We'd probably want to friend a base class with
>> a protected member function that allowed subclasses (like
>> base::PowerMonitorImpl and whatever views thing) to call the
>> ProcessPowerEvent() thing.
>
>
> To make sure I understand, I'll restate what you said, please let me know
> if I get it wrong....
>
> Add PowerMonitor::SetSource(base::PowerMonitorSource* source), which checks
> that it is only called once.  It will have a protected method that exposes
> the ProcessPowerEvent functionality and will be a friend of PowerMonitor
> to allow the call.
>
> SGTM
>
> http://codereview.chromium.org/10959020/

[Hongbo Min, 2012-10-17 23:29:43]

On 2012/10/17 20:09:13, vandebo wrote:
> On 2012/10/17 18:59:26, willchan wrote:
> > Oh fascinating! I see, thanks for pointing that out. I see...so views
> > *also* needs to generate power events? That's intriguing.
> 
> To clarify, on windows, views is the only source of power events, it's
> not an additional source.
> 
> > I think the Singleton lazy instantiation pattern is fine for
> > registering observers, but if we have multiple possible sources of
> > events, we probably need to explicitly set the source. It sounds to me
> > like we need to add a SetSource() member function or something on
> > PowerMonitor that CHECKs to make sure it's only called once. Then we
> > should provide a base::PowerMonitorImpl that would be a good default,
> > but also be composable. We'd probably want to friend a base class with
> > a protected member function that allowed subclasses (like
> > base::PowerMonitorImpl and whatever views thing) to call the
> > ProcessPowerEvent() thing.
> 
> To make sure I understand, I'll restate what you said, please let me know
> if I get it wrong....
> 
> Add PowerMonitor::SetSource(base::PowerMonitorSource* source), which checks
> that it is only called once.  It will have a protected method that exposes
> the ProcessPowerEvent functionality and will be a friend of PowerMonitor
> to allow the call.
> 
> SGTM
 
So what methods should be exposed by base::PowerMonitorSource? Or it is just a
empty class to workaround the friend declaration?

[Hongbo Min, 2012-10-17 23:31:37]

On 2012/10/17 20:09:13, vandebo wrote:
> On 2012/10/17 18:59:26, willchan wrote:
> > Oh fascinating! I see, thanks for pointing that out. I see...so views
> > *also* needs to generate power events? That's intriguing.
> 
> To clarify, on windows, views is the only source of power events, it's
> not an additional source.
> 
> > I think the Singleton lazy instantiation pattern is fine for
> > registering observers, but if we have multiple possible sources of
> > events, we probably need to explicitly set the source. It sounds to me
> > like we need to add a SetSource() member function or something on
> > PowerMonitor that CHECKs to make sure it's only called once. Then we
> > should provide a base::PowerMonitorImpl that would be a good default,
> > but also be composable. We'd probably want to friend a base class with
> > a protected member function that allowed subclasses (like
> > base::PowerMonitorImpl and whatever views thing) to call the
> > ProcessPowerEvent() thing.
> 
> To make sure I understand, I'll restate what you said, please let me know
> if I get it wrong....
> 
> Add PowerMonitor::SetSource(base::PowerMonitorSource* source), which checks
> that it is only called once.  It will have a protected method that exposes
> the ProcessPowerEvent functionality and will be a friend of PowerMonitor
> to allow the call.
> 
> SGTM
 
So what methods should be exposed by base::PowerMonitorSource? Or it is just a
empty class to workaround the friend declaration?

And how to handle the case in the android implementation which triggers the
ProcessPowerEvent in a global function OnBatteryCharingChanged? See
power_monitor_android.cc.

[vandebo (ex-Chrome), 2012-10-18 01:18:34]

On 2012/10/17 23:31:37, Hongbo Min wrote:
> On 2012/10/17 20:09:13, vandebo wrote:
> > On 2012/10/17 18:59:26, willchan wrote:
> > > Oh fascinating! I see, thanks for pointing that out. I see...so views
> > > *also* needs to generate power events? That's intriguing.
> > 
> > To clarify, on windows, views is the only source of power events, it's
> > not an additional source.
> > 
> > > I think the Singleton lazy instantiation pattern is fine for
> > > registering observers, but if we have multiple possible sources of
> > > events, we probably need to explicitly set the source. It sounds to me
> > > like we need to add a SetSource() member function or something on
> > > PowerMonitor that CHECKs to make sure it's only called once. Then we
> > > should provide a base::PowerMonitorImpl that would be a good default,
> > > but also be composable. We'd probably want to friend a base class with
> > > a protected member function that allowed subclasses (like
> > > base::PowerMonitorImpl and whatever views thing) to call the
> > > ProcessPowerEvent() thing.
> > 
> > To make sure I understand, I'll restate what you said, please let me know
> > if I get it wrong....
> > 
> > Add PowerMonitor::SetSource(base::PowerMonitorSource* source), which checks
> > that it is only called once.  It will have a protected method that exposes
> > the ProcessPowerEvent functionality and will be a friend of PowerMonitor
> > to allow the call.
> > 
> > SGTM
>  
> So what methods should be exposed by base::PowerMonitorSource? Or it is just a
> empty class to workaround the friend declaration?

Will, any objections to the following?

Thinking about it more, the SetSource approach seems overly complicated.
Instead we can just create a PowerMonitorNotifier (is there a better name?)
class:

class PowerMonitorNotifier {
 public:
  ProcessPowerEvent(args) {
    power_monitor_->ProcessPowerEvent(args);
  }
 private:
  friend class PowerMonitor;
  PowerMonitorNotifier(PowerMonitor* power_monitor) { ... }
};

And PowerMonitor can have a GetPowerMonitorNotifier method, that will create one
and return it (but only once). 

> And how to handle the case in the android implementation which triggers the
> ProcessPowerEvent in a global function OnBatteryCharingChanged? See
> power_monitor_android.cc.

With the above approach, you can store the PowerMonitorNotifier in a file level
global.

My original thought on SetSource...
Thinking about this a bit more, we don't need to use a friend declaration, and
in fact using one would make SetSource unnecessary...

class PowerMonitorSource {
 public:
  ProcessPowerEvent(args) {
    event_callback_.Run(args);
  }

 private:
  friend class PowerMonitor;

  SetEventCallback(CallbackType callback) {
    event_callback_ = callback;
  }

  CallbackType event_callback_;

  ...
};

Then PowerMonitor::SetSource will make sure the argument is the first source and
then set the callback function on the source.

[Hongbo Min, 2012-10-18 02:05:37]

On 2012/10/18 01:18:34, vandebo wrote:
> On 2012/10/17 23:31:37, Hongbo Min wrote:
> > On 2012/10/17 20:09:13, vandebo wrote:
> > > On 2012/10/17 18:59:26, willchan wrote:
> > > > Oh fascinating! I see, thanks for pointing that out. I see...so views
> > > > *also* needs to generate power events? That's intriguing.
> > > 
> > > To clarify, on windows, views is the only source of power events, it's
> > > not an additional source.
> > > 
> > > > I think the Singleton lazy instantiation pattern is fine for
> > > > registering observers, but if we have multiple possible sources of
> > > > events, we probably need to explicitly set the source. It sounds to me
> > > > like we need to add a SetSource() member function or something on
> > > > PowerMonitor that CHECKs to make sure it's only called once. Then we
> > > > should provide a base::PowerMonitorImpl that would be a good default,
> > > > but also be composable. We'd probably want to friend a base class with
> > > > a protected member function that allowed subclasses (like
> > > > base::PowerMonitorImpl and whatever views thing) to call the
> > > > ProcessPowerEvent() thing.
> > > 
> > > To make sure I understand, I'll restate what you said, please let me know
> > > if I get it wrong....
> > > 
> > > Add PowerMonitor::SetSource(base::PowerMonitorSource* source), which
checks
> > > that it is only called once.  It will have a protected method that exposes
> > > the ProcessPowerEvent functionality and will be a friend of PowerMonitor
> > > to allow the call.
> > > 
> > > SGTM
> >  
> > So what methods should be exposed by base::PowerMonitorSource? Or it is just
a
> > empty class to workaround the friend declaration?
> 
> Will, any objections to the following?
> 
> Thinking about it more, the SetSource approach seems overly complicated.
> Instead we can just create a PowerMonitorNotifier (is there a better name?)
> class:
> 
> class PowerMonitorNotifier {
>  public:
>   ProcessPowerEvent(args) {
>     power_monitor_->ProcessPowerEvent(args);
>   }
>  private:
>   friend class PowerMonitor;
>   PowerMonitorNotifier(PowerMonitor* power_monitor) { ... }
> };
> 
> And PowerMonitor can have a GetPowerMonitorNotifier method, that will create
one
> and return it (but only once). 

Seems the PowerMonitorNotifier should be a friend class of PowerMonitor, not
vice versa.

In this case, we actually encapsulate the ProcessPowerEvent in a separate class
which can be accessed via PoweMonitor::GetNotifier method, and anyone can now
invoke ProcessPowerEvent via
PowerMonitor::GetInstance()->GetNotifier()->ProcessPowerEvent();

What could we benefit from it in comparison with the solution of declaring
ProcessPowerEvent as private? Just provide a better encapsulation?

[willchan no longer on Chromium, 2012-10-18 02:06:06]

SG. It means we have to leak it, but that's fine. Maybe sure to leak
the PowerMonitor too so we don't have any shutdown issues.

On Wed, Oct 17, 2012 at 6:18 PM,  <vandebo@chromium.org> wrote:
> On 2012/10/17 23:31:37, Hongbo Min wrote:
>>
>> On 2012/10/17 20:09:13, vandebo wrote:
>> > On 2012/10/17 18:59:26, willchan wrote:
>> > > Oh fascinating! I see, thanks for pointing that out. I see...so views
>> > > *also* needs to generate power events? That's intriguing.
>> >
>> > To clarify, on windows, views is the only source of power events, it's
>> > not an additional source.
>> >
>> > > I think the Singleton lazy instantiation pattern is fine for
>> > > registering observers, but if we have multiple possible sources of
>> > > events, we probably need to explicitly set the source. It sounds to me
>> > > like we need to add a SetSource() member function or something on
>> > > PowerMonitor that CHECKs to make sure it's only called once. Then we
>> > > should provide a base::PowerMonitorImpl that would be a good default,
>> > > but also be composable. We'd probably want to friend a base class with
>> > > a protected member function that allowed subclasses (like
>> > > base::PowerMonitorImpl and whatever views thing) to call the
>> > > ProcessPowerEvent() thing.
>> >
>> > To make sure I understand, I'll restate what you said, please let me
>> > know
>> > if I get it wrong....
>> >
>> > Add PowerMonitor::SetSource(base::PowerMonitorSource* source), which
>> > checks
>> > that it is only called once.  It will have a protected method that
>> > exposes
>> > the ProcessPowerEvent functionality and will be a friend of PowerMonitor
>> > to allow the call.
>> >
>> > SGTM
>
>
>> So what methods should be exposed by base::PowerMonitorSource? Or it is
>> just a
>> empty class to workaround the friend declaration?
>
>
> Will, any objections to the following?
>
> Thinking about it more, the SetSource approach seems overly complicated.
> Instead we can just create a PowerMonitorNotifier (is there a better name?)
> class:
>
> class PowerMonitorNotifier {
>  public:
>   ProcessPowerEvent(args) {
>     power_monitor_->ProcessPowerEvent(args);
>   }
>  private:
>   friend class PowerMonitor;
>   PowerMonitorNotifier(PowerMonitor* power_monitor) { ... }
> };
>
> And PowerMonitor can have a GetPowerMonitorNotifier method, that will create
> one
> and return it (but only once).
>
>
>> And how to handle the case in the android implementation which triggers
>> the
>> ProcessPowerEvent in a global function OnBatteryCharingChanged? See
>> power_monitor_android.cc.
>
>
> With the above approach, you can store the PowerMonitorNotifier in a file
> level
> global.
>
> My original thought on SetSource...
> Thinking about this a bit more, we don't need to use a friend declaration,
> and
> in fact using one would make SetSource unnecessary...
>
> class PowerMonitorSource {
>  public:
>   ProcessPowerEvent(args) {
>     event_callback_.Run(args);
>   }
>
>  private:
>   friend class PowerMonitor;
>
>   SetEventCallback(CallbackType callback) {
>     event_callback_ = callback;
>   }
>
>   CallbackType event_callback_;
>
>   ...
> };
>
> Then PowerMonitor::SetSource will make sure the argument is the first source
> and
> then set the callback function on the source.
>
> http://codereview.chromium.org/10959020/

[Hongbo Min, 2012-10-18 12:18:27]

> > class PowerMonitorNotifier {
> >  public:
> >   ProcessPowerEvent(args) {
> >     power_monitor_->ProcessPowerEvent(args);
> >   }
> >  private:
> >   friend class PowerMonitor;
> >   PowerMonitorNotifier(PowerMonitor* power_monitor) { ... }
> > };
> > 
> > And PowerMonitor can have a GetPowerMonitorNotifier method, that will create
> one
> > and return it (but only once). 
> 
> Seems the PowerMonitorNotifier should be a friend class of PowerMonitor, not
> vice versa.
> 
> In this case, we actually encapsulate the ProcessPowerEvent in a separate
class
> which can be accessed via PoweMonitor::GetNotifier method, and anyone can now
> invoke ProcessPowerEvent via
> PowerMonitor::GetInstance()->GetNotifier()->ProcessPowerEvent();
> 
> What could we benefit from it in comparison with the solution of declaring
> ProcessPowerEvent as private? Just provide a better encapsulation?

UPDATE: what could we benefit from it in comparison with the solution of
declaring ProcessPowerEvent as public instead? Just provide a better
encapsulation?

[willchan no longer on Chromium, 2012-10-18 17:25:09]

I don't really want anyone anywhere in the code to be able to generate
power events.

On Thu, Oct 18, 2012 at 5:18 AM,  <hongbo.min@intel.com> wrote:
>> > class PowerMonitorNotifier {
>> >  public:
>> >   ProcessPowerEvent(args) {
>> >     power_monitor_->ProcessPowerEvent(args);
>> >   }
>> >  private:
>> >   friend class PowerMonitor;
>> >   PowerMonitorNotifier(PowerMonitor* power_monitor) { ... }
>> > };
>> >
>> > And PowerMonitor can have a GetPowerMonitorNotifier method, that will
>> > create
>> one
>> > and return it (but only once).
>
>
>> Seems the PowerMonitorNotifier should be a friend class of PowerMonitor,
>> not
>> vice versa.
>
>
>> In this case, we actually encapsulate the ProcessPowerEvent in a separate
>
> class
>>
>> which can be accessed via PoweMonitor::GetNotifier method, and anyone can
>> now
>> invoke ProcessPowerEvent via
>> PowerMonitor::GetInstance()->GetNotifier()->ProcessPowerEvent();
>
>
>> What could we benefit from it in comparison with the solution of declaring
>> ProcessPowerEvent as private? Just provide a better encapsulation?
>
>
> UPDATE: what could we benefit from it in comparison with the solution of
> declaring ProcessPowerEvent as public instead? Just provide a better
> encapsulation?
>
> http://codereview.chromium.org/10959020/

[vandebo (ex-Chrome), 2012-10-18 18:55:41]

On 2012/10/18 02:05:37, Hongbo Min wrote:
> On 2012/10/18 01:18:34, vandebo wrote:
> > Will, any objections to the following?
> > 
> > Thinking about it more, the SetSource approach seems overly complicated.
> > Instead we can just create a PowerMonitorNotifier (is there a better name?)
> > class:
> > 
> > class PowerMonitorNotifier {
> >  public:
> >   ProcessPowerEvent(args) {
> >     power_monitor_->ProcessPowerEvent(args);
> >   }
> >  private:
> >   friend class PowerMonitor;
> >   PowerMonitorNotifier(PowerMonitor* power_monitor) { ... }
> > };
> > 
> > And PowerMonitor can have a GetPowerMonitorNotifier method, that will create
> one
> > and return it (but only once). 
> 
> Seems the PowerMonitorNotifier should be a friend class of PowerMonitor, not
> vice versa.

PowerMonitorNotifier has to be a friend of PowerMonitor so that it can call
ProcessPowerEvent.  PowerMonitor has to be a friend of PowerMonitorNotifier
so that it can create the PowerMonitorNotifier (private constructor).

> In this case, we actually encapsulate the ProcessPowerEvent in a separate
class
> which can be accessed via PoweMonitor::GetNotifier method, and anyone can now
> invoke ProcessPowerEvent via
> PowerMonitor::GetInstance()->GetNotifier()->ProcessPowerEvent();
> 
> What could we benefit from it in comparison with the solution of declaring
> ProcessPowerEvent as private? Just provide a better encapsulation?

GetNotifier() will only return a notifier the first time it is called, after
that
it will return NULL.  So only the first caller gets access and it has to save
the notifier to use it in the future.

As Will points out, that means that ownership and references between the
Notifier
and PowerMonitor can't easily be handled, so both should leak on shutdown, i.e.
use the leaky singleton traits instead of the Default traits.

[Hongbo Min, 2012-10-19 03:44:43]

On 2012/10/18 18:55:41, vandebo wrote:
> On 2012/10/18 02:05:37, Hongbo Min wrote:
> > On 2012/10/18 01:18:34, vandebo wrote:
> > > Will, any objections to the following?
> > > 
> > > Thinking about it more, the SetSource approach seems overly complicated.
> > > Instead we can just create a PowerMonitorNotifier (is there a better
name?)
> > > class:
> > > 
> > > class PowerMonitorNotifier {
> > >  public:
> > >   ProcessPowerEvent(args) {
> > >     power_monitor_->ProcessPowerEvent(args);
> > >   }
> > >  private:
> > >   friend class PowerMonitor;
> > >   PowerMonitorNotifier(PowerMonitor* power_monitor) { ... }
> > > };
> > > 
> > > And PowerMonitor can have a GetPowerMonitorNotifier method, that will
create
> > one
> > > and return it (but only once). 
> > 
> > Seems the PowerMonitorNotifier should be a friend class of PowerMonitor, not
> > vice versa.
> 
> PowerMonitorNotifier has to be a friend of PowerMonitor so that it can call
> ProcessPowerEvent.  PowerMonitor has to be a friend of PowerMonitorNotifier
> so that it can create the PowerMonitorNotifier (private constructor).
> 
> > In this case, we actually encapsulate the ProcessPowerEvent in a separate
> class
> > which can be accessed via PoweMonitor::GetNotifier method, and anyone can
now
> > invoke ProcessPowerEvent via
> > PowerMonitor::GetInstance()->GetNotifier()->ProcessPowerEvent();
> > 
> > What could we benefit from it in comparison with the solution of declaring
> > ProcessPowerEvent as private? Just provide a better encapsulation?
> 
> GetNotifier() will only return a notifier the first time it is called, after
> that
> it will return NULL.  So only the first caller gets access and it has to save
> the notifier to use it in the future.
> 
> As Will points out, that means that ownership and references between the
> Notifier
> and PowerMonitor can't easily be handled, so both should leak on shutdown,
i.e.
> use the leaky singleton traits instead of the Default traits.

Thanks for your explanation. Right now I think I understand your point. And I
will update the patch soon.

[Hongbo Min, 2012-10-21 09:56:03]

Add PowerMonitor::Notifier class proposed by vandebo. Pls have a review again. 

Seems sync_unit_tests fails too often due to timeout. Do you know why?

http://codereview.chromium.org/10959020/diff/95046/base/power_monitor/power_m...
File base/power_monitor/power_monitor_mac.mm (left):

http://codereview.chromium.org/10959020/diff/95046/base/power_monitor/power_m...
base/power_monitor/power_monitor_mac.mm:49: DCHECK_EQ(g_system_power_io_port,
0u);
If I keep the original DCHECK_EQ stmt, lots of test will fail. The root cause
is, AllocateSystemIOPorts is only allowed to be called once, but each test needs
to call it at the beginning of testing, as descried by vandebo.

The thing makes me confused is, DCHECK_EQ should only have an effect in debug
mode, according to the trybot result, the DCHECK also happens although building
mac_rel with Relase mode, and therefore the test fails.

[Hongbo Min, 2012-10-23 00:16:43]

On 2012/10/21 09:56:03, Hongbo Min wrote:
> Add PowerMonitor::Notifier class proposed by vandebo. Pls have a review again.

> 
> Seems sync_unit_tests fails too often due to timeout. Do you know why?
> 
>
http://codereview.chromium.org/10959020/diff/95046/base/power_monitor/power_m...
> File base/power_monitor/power_monitor_mac.mm (left):
> 
>
http://codereview.chromium.org/10959020/diff/95046/base/power_monitor/power_m...
> base/power_monitor/power_monitor_mac.mm:49: DCHECK_EQ(g_system_power_io_port,
> 0u);
> If I keep the original DCHECK_EQ stmt, lots of test will fail. The root cause
> is, AllocateSystemIOPorts is only allowed to be called once, but each test
needs
> to call it at the beginning of testing, as descried by vandebo.
> 
> The thing makes me confused is, DCHECK_EQ should only have an effect in debug
> mode, according to the trybot result, the DCHECK also happens although
building
> mac_rel with Relase mode, and therefore the test fails.

Any hint for this?

[vandebo (ex-Chrome), 2012-10-23 01:27:57]

On 2012/10/21 09:56:03, Hongbo Min wrote:
> Seems sync_unit_tests fails too often due to timeout. Do you know why?

Since it's only Mac, I suspect that you're missing a call to
AllocateSystemIOPorts some place.  Try adding some debug code to ensure that
it's called before PowerMonitor in constructed and run that on the bot.

http://codereview.chromium.org/10959020/diff/95046/base/power_monitor/power_m...
File base/power_monitor/power_monitor.h (right):

http://codereview.chromium.org/10959020/diff/95046/base/power_monitor/power_m...
base/power_monitor/power_monitor.h:49: class BASE_EXPORT Notifier {
Notifier doesn't feel like a very good name, but I can't think of anything much
better.  The best I can come up with is SourceInstance or SourceNotifier....

http://codereview.chromium.org/10959020/diff/95046/base/power_monitor/power_m...
base/power_monitor/power_monitor.h:62: Notifier(PowerMonitor* monitor) :
monitor_(monitor) {}
Add a comment about why the constructor is private (so that only PowerMonitor
can create one).

http://codereview.chromium.org/10959020/diff/95046/base/power_monitor/power_m...
base/power_monitor/power_monitor.h:65: PowerMonitor* monitor_;
Actually, this don't need to store the PowerMonitor, it can just call
GetInstance()

http://codereview.chromium.org/10959020/diff/95046/base/power_monitor/power_m...
base/power_monitor/power_monitor.h:152: PowerMonitor::Notifier* notifier_;
I think the caller that gets the notifier owns it.  I don't think we need to
save it either.  Just use a bool to note if we've handed it out.

http://codereview.chromium.org/10959020/diff/95046/base/power_monitor/power_m...
File base/power_monitor/power_monitor_mac.mm (left):

http://codereview.chromium.org/10959020/diff/95046/base/power_monitor/power_m...
base/power_monitor/power_monitor_mac.mm:49: DCHECK_EQ(g_system_power_io_port,
0u);
On 2012/10/21 09:56:03, Hongbo Min wrote:
> If I keep the original DCHECK_EQ stmt, lots of test will fail. The root cause
> is, AllocateSystemIOPorts is only allowed to be called once, but each test
needs
> to call it at the beginning of testing, as descried by vandebo.
> 
> The thing makes me confused is, DCHECK_EQ should only have an effect in debug
> mode, according to the trybot result, the DCHECK also happens although
building
> mac_rel with Relase mode, and therefore the test fails.
> 
DCHECKs are used on the bots even in release mode.  We started doing this
because debug mode would often fail on DCHECKs without people noticing.

http://codereview.chromium.org/10959020/diff/95046/base/power_monitor/power_m...
File base/power_monitor/power_monitor_mac.mm (right):

http://codereview.chromium.org/10959020/diff/95046/base/power_monitor/power_m...
base/power_monitor/power_monitor_mac.mm:27: g_power_notifier =
PowerMonitor::GetInstance()->GetNotifierOnce();
Set this in PlatformInit and assert that we get it?

http://codereview.chromium.org/10959020/diff/95046/ui/views/win/hwnd_message_...
File ui/views/win/hwnd_message_handler.cc (right):

http://codereview.chromium.org/10959020/diff/95046/ui/views/win/hwnd_message_...
ui/views/win/hwnd_message_handler.cc:44: base::PowerMonitor::Notifier*
g_power_notifier = NULL;
Set this in the constructor and assert that we get it?

[Hongbo Min, 2012-10-23 01:55:01]

On 2012/10/23 01:27:57, vandebo wrote:
> On 2012/10/21 09:56:03, Hongbo Min wrote:
> > Seems sync_unit_tests fails too often due to timeout. Do you know why?
> 
> Since it's only Mac, I suspect that you're missing a call to
> AllocateSystemIOPorts some place.  Try adding some debug code to ensure that
> it's called before PowerMonitor in constructed and run that on the bot.
> 

I ever also met the same problem on Windows bot, but after I tried again and
again, it passed.

[vandebo (ex-Chrome), 2012-10-23 02:40:43]

On 2012/10/23 01:55:01, Hongbo Min wrote:
> On 2012/10/23 01:27:57, vandebo wrote:
> > On 2012/10/21 09:56:03, Hongbo Min wrote:
> > > Seems sync_unit_tests fails too often due to timeout. Do you know why?
> > 
> > Since it's only Mac, I suspect that you're missing a call to
> > AllocateSystemIOPorts some place.  Try adding some debug code to ensure that
> > it's called before PowerMonitor in constructed and run that on the bot.
> > 
> 
> I ever also met the same problem on Windows bot, but after I tried again and
> again, it passed.

Some bots are flaky, go ahead and try mac more if you don't think it is this
change.  But ignore it at your peril.  The other things you can do is make a
change that just changes some whitespace in a file and send that to a bot to see
if the bot is behaving.

[Hongbo Min, 2012-10-23 15:11:14]

On 2012/10/23 01:27:57, vandebo wrote:
>
http://codereview.chromium.org/10959020/diff/95046/base/power_monitor/power_m...
> base/power_monitor/power_monitor.h:49: class BASE_EXPORT Notifier {
> Notifier doesn't feel like a very good name, but I can't think of anything
much
> better.  The best I can come up with is SourceInstance or SourceNotifier....
> 
I prefer EventNotifier name instead, it is a notifier of PowerMonitor which
notify the power observer that a specific power event has happened. Like
EventDispatcher of DOM element in WebKit world, the event dispatcher of a dom
element will dispatch the event to the right child element. Also,
ProcessPowerEvent is renamed to NotifyPowerEvent. Does it look good to you? 

> 
>
http://codereview.chromium.org/10959020/diff/95046/base/power_monitor/power_m...
> base/power_monitor/power_monitor.h:65: PowerMonitor* monitor_;
> Actually, this don't need to store the PowerMonitor, it can just call
> GetInstance()
> 
>
http://codereview.chromium.org/10959020/diff/95046/base/power_monitor/power_m...
> base/power_monitor/power_monitor.h:152: PowerMonitor::Notifier* notifier_;
> I think the caller that gets the notifier owns it.  I don't think we need to
> save it either.  Just use a bool to note if we've handed it out.
> 
My original thought is, the PowerMonitor owns the notifier and leaks it via
LeakySingletonTraits. If the caller owns the notifier, the leaky code would be
scattered in other place, not managed by the Singleton.

Regarding to the GetNotifierOnce method, it is actually a bit tricky, returns
the notifier first time, and NULL later. I found the code is becoming hard to
understand. Could we make it just acting as GetNotifier and always returns the
Notifier instance owned by PowerMonitor? Because the PowerMonitor singleton
ensures the notifier singleton, I am not sure if it is a good approach to limit
the sole power event source via the tricky return value of a method.

[vandebo (ex-Chrome), 2012-10-23 17:53:04]

On 2012/10/23 15:11:14, Hongbo Min wrote:
> On 2012/10/23 01:27:57, vandebo wrote:
> >
>
http://codereview.chromium.org/10959020/diff/95046/base/power_monitor/power_m...
> > base/power_monitor/power_monitor.h:49: class BASE_EXPORT Notifier {
> > Notifier doesn't feel like a very good name, but I can't think of anything
> much
> > better.  The best I can come up with is SourceInstance or SourceNotifier....
> > 
> I prefer EventNotifier name instead, it is a notifier of PowerMonitor which
> notify the power observer that a specific power event has happened. Like
> EventDispatcher of DOM element in WebKit world, the event dispatcher of a dom
> element will dispatch the event to the right child element. Also,
> ProcessPowerEvent is renamed to NotifyPowerEvent. Does it look good to you? 

It's not really a notifier because it doesn't know when anything happens, it's
just the thing enables the notification.

Thinking on it a bit more, how about Power/EventSignaler, or even just Signaler?
I still prefer ProcessPowerEven over NotifyPowerEvent a little because some
processing happens before the Observers are notified.


http://codereview.chromium.org/10959020/diff/95046/base/power_monitor/power_m...
> > base/power_monitor/power_monitor.h:65: PowerMonitor* monitor_;
> > Actually, this don't need to store the PowerMonitor, it can just call
> > GetInstance()
> > 
> >
>
http://codereview.chromium.org/10959020/diff/95046/base/power_monitor/power_m...
> > base/power_monitor/power_monitor.h:152: PowerMonitor::Notifier* notifier_;
> > I think the caller that gets the notifier owns it.  I don't think we need to
> > save it either.  Just use a bool to note if we've handed it out.
> > 
> My original thought is, the PowerMonitor owns the notifier and leaks it via
> LeakySingletonTraits. If the caller owns the notifier, the leaky code would be
> scattered in other place, not managed by the Singleton.

Hmm, ok that's fine.
 
> Regarding to the GetNotifierOnce method, it is actually a bit tricky, returns
> the notifier first time, and NULL later. I found the code is becoming hard to
> understand. Could we make it just acting as GetNotifier and always returns the
> Notifier instance owned by PowerMonitor? Because the PowerMonitor singleton
> ensures the notifier singleton, I am not sure if it is a good approach to
limit
> the sole power event source via the tricky return value of a method.

This is Will's point of contention.  He wants to make sure that we can only get
power events from one place.  I don't think GetNotifierOnce is too tricky.

[Hongbo Min, 2012-10-23 23:43:55]

On 2012/10/23 17:53:04, vandebo wrote:
> On 2012/10/23 15:11:14, Hongbo Min wrote:
> > On 2012/10/23 01:27:57, vandebo wrote:
> > >
> >
>
http://codereview.chromium.org/10959020/diff/95046/base/power_monitor/power_m...
> > > base/power_monitor/power_monitor.h:49: class BASE_EXPORT Notifier {
> > > Notifier doesn't feel like a very good name, but I can't think of anything
> > much
> > > better.  The best I can come up with is SourceInstance or
SourceNotifier....
> > > 
> > I prefer EventNotifier name instead, it is a notifier of PowerMonitor which
> > notify the power observer that a specific power event has happened. Like
> > EventDispatcher of DOM element in WebKit world, the event dispatcher of a
dom
> > element will dispatch the event to the right child element. Also,
> > ProcessPowerEvent is renamed to NotifyPowerEvent. Does it look good to you? 
> 
> It's not really a notifier because it doesn't know when anything happens, it's
> just the thing enables the notification.
> 
> Thinking on it a bit more, how about Power/EventSignaler, or even just
Signaler?
> I still prefer ProcessPowerEven over NotifyPowerEvent a little because some
> processing happens before the Observers are notified.
> 

Signaler is good to me:)

[Hongbo Min, 2012-10-24 07:49:17]

vandebo, I have figured out why the sync_unit_tests always fails. It is caused
by the Singleton pattern for PowerMonitor. 

The sync_unit_tests indirectly depends on net::URLRequestJob
(net/url_request/url_request_job.cc) which is a PowerObserver. In the ctor of
URLRequestJob, it will add itself as the observer of PowerMonitor, and may
initialize the PowerMonitor instance if it is not created yet.

However, the original code based on SystemMonitor doesn't create the
SystemMonitor instance if it is not created yet. 

In sync_unit_tests testing code, the PowerMonitor instance would be initialized
in URLRequestJob ctor. I have some experiments:

1. Remove the AddObserver/RemoveObserver code from URLRequestJob ctor/dtor. The
sync_unit_tests can PASS.
2. Call PowerMonitor::AllocateSystemIOPorts in SyncHttpBridgeTest (which
indirectly depends on URLRequestJob). The sync_unit_tests FAIL. See PatchSet 12.

I think it is a side-effect introduced by the Singleton pattern since we
actually change the logic of PowerMonitor creation for test.

[vandebo (ex-Chrome), 2012-10-24 16:22:49]

On 2012/10/24 07:49:17, Hongbo Min wrote:
> vandebo, I have figured out why the sync_unit_tests always fails. It is caused
> by the Singleton pattern for PowerMonitor. 
> 
> The sync_unit_tests indirectly depends on net::URLRequestJob
> (net/url_request/url_request_job.cc) which is a PowerObserver. In the ctor of
> URLRequestJob, it will add itself as the observer of PowerMonitor, and may
> initialize the PowerMonitor instance if it is not created yet.
> 
> However, the original code based on SystemMonitor doesn't create the
> SystemMonitor instance if it is not created yet. 
> 
> In sync_unit_tests testing code, the PowerMonitor instance would be
initialized
> in URLRequestJob ctor. I have some experiments:
> 
> 1. Remove the AddObserver/RemoveObserver code from URLRequestJob ctor/dtor.
The
> sync_unit_tests can PASS.
> 2. Call PowerMonitor::AllocateSystemIOPorts in SyncHttpBridgeTest (which
> indirectly depends on URLRequestJob). The sync_unit_tests FAIL. See PatchSet
12.
> 
> I think it is a side-effect introduced by the Singleton pattern since we
> actually change the logic of PowerMonitor creation for test.

Sounds reasonable.  If we can fix the tests to behave correctly (and it doesn't
require touching every test), then we should do that.

[Hongbo Min, 2012-10-29 09:14:10]

vandebo, I revert using singleton pattern for PowerMonitor. Pls review it again.
Thanks.

[vandebo (ex-Chrome), 2012-10-29 18:04:38]

http://codereview.chromium.org/10959020/diff/134004/base/hi_res_timer_manager...
File base/hi_res_timer_manager_unittest.cc (left):

http://codereview.chromium.org/10959020/diff/134004/base/hi_res_timer_manager...
base/hi_res_timer_manager_unittest.cc:16: scoped_ptr<base::SystemMonitor>
system_monitor(new base::SystemMonitor());
HighResolutionTimerManager::HighResolutionTimerManager uses PowerMonitor without
checking for NULL, so the test needs to create one so that Get() returns one.

http://codereview.chromium.org/10959020/diff/134004/base/power_monitor/power_...
File base/power_monitor/power_monitor.cc (right):

http://codereview.chromium.org/10959020/diff/134004/base/power_monitor/power_...
base/power_monitor/power_monitor.cc:42: delete signaler_;
Don't manage lifetimes in the destructor:  If this class owns signaler_, then it
should be in a scoped_ptr<>.  But I don't think this class can own it since the
class that gets it could outlive this class.  In that case, there's no reason to
store it, just store a bool indicating if we've given own out.

Furthermore, since signaler_ can outlive PowerMonitor, signaler_ should
internally call PowerMonitor::Get() to see if it's still alive.

http://codereview.chromium.org/10959020/diff/134004/base/power_monitor/power_...
base/power_monitor/power_monitor.cc:53: CHECK(signaler_ == NULL);
The interface says it will return NULL on the second call, this checks = crash.

http://codereview.chromium.org/10959020/diff/134004/base/power_monitor/power_...
base/power_monitor/power_monitor.cc:56: // The signaler is allowed to be leaked
since the PowerMonitor and Signaler
Comment is out of date.

http://codereview.chromium.org/10959020/diff/134004/base/power_monitor/power_...
File base/power_monitor/power_monitor.h (right):

http://codereview.chromium.org/10959020/diff/134004/base/power_monitor/power_...
base/power_monitor/power_monitor.h:55: CHECK(monitor_);
These checks don't do anything.

http://codereview.chromium.org/10959020/diff/134004/base/power_monitor/power_...
base/power_monitor/power_monitor.h:68: ~Signaler() {}
Don't inline the implementation, but it in the implementation file.

http://codereview.chromium.org/10959020/diff/134004/base/power_monitor/power_...
base/power_monitor/power_monitor.h:79: // Return the |signaler_| pointer at the
first time it gets called, otherwise
nit: The first call to this method will return an instance of Signaler which the
caller can use to signal power events.  All subsequent calls will return NULL.

http://codereview.chromium.org/10959020/diff/134004/base/power_monitor/power_...
File base/power_monitor/power_monitor_android.cc (right):

http://codereview.chromium.org/10959020/diff/134004/base/power_monitor/power_...
base/power_monitor/power_monitor_android.cc:10: static PowerMonitor::Signaler*
g_power_signaler = NULL;
This should go in an anonymous namespace.

http://codereview.chromium.org/10959020/diff/134004/base/power_monitor/power_...
File base/power_monitor/power_monitor_mac.mm (left):

http://codereview.chromium.org/10959020/diff/134004/base/power_monitor/power_...
base/power_monitor/power_monitor_mac.mm:76: if (g_system_power_io_port == 0)
Leave these lines in.

http://codereview.chromium.org/10959020/diff/134004/base/power_monitor/power_...
File base/power_monitor/power_monitor_mac.mm (right):

http://codereview.chromium.org/10959020/diff/134004/base/power_monitor/power_...
base/power_monitor/power_monitor_mac.mm:19: io_object_t g_signaler_object = 0;
no reason to change the name of this variable.

http://codereview.chromium.org/10959020/diff/134004/base/power_monitor/power_...
base/power_monitor/power_monitor_mac.mm:57: &g_signaler_object);
and here

http://codereview.chromium.org/10959020/diff/134004/base/power_monitor/power_...
base/power_monitor/power_monitor_mac.mm:67: CHECK_NE(g_system_power_io_port,
0u);
Move this just under the comment online 64

http://codereview.chromium.org/10959020/diff/134004/base/power_monitor/power_...
base/power_monitor/power_monitor_mac.mm:76: void PowerMonitor::PlatformDestroy()
{
Maybe PlatformDestroy should clean up g_power_signaler

http://codereview.chromium.org/10959020/diff/134004/base/power_monitor/power_...
base/power_monitor/power_monitor_mac.mm:77: CHECK_NE(g_system_power_io_port,
0u);
Levae this as a DCHECK

http://codereview.chromium.org/10959020/diff/134004/base/power_monitor/power_...
base/power_monitor/power_monitor_mac.mm:86:
IODeregisterForSystemPower(&g_signaler_object);
and here

http://codereview.chromium.org/10959020/diff/134004/chrome/browser/extensions...
File chrome/browser/extensions/event_router_forwarder_unittest.cc (right):

http://codereview.chromium.org/10959020/diff/134004/chrome/browser/extensions...
chrome/browser/extensions/event_router_forwarder_unittest.cc:95:
base::PowerMonitor::AllocateSystemIOPorts();
Is this needed?

http://codereview.chromium.org/10959020/diff/134004/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager_unittest.cc (right):

http://codereview.chromium.org/10959020/diff/134004/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager_unittest.cc:101:
base::PowerMonitor::AllocateSystemIOPorts();
Is this needed?

http://codereview.chromium.org/10959020/diff/134004/content/browser/gamepad/g...
File content/browser/gamepad/gamepad_test_helpers.cc (right):

http://codereview.chromium.org/10959020/diff/134004/content/browser/gamepad/g...
content/browser/gamepad/gamepad_test_helpers.cc:41:
base::PowerMonitor::AllocateSystemIOPorts();
This probably isn't needed.

http://codereview.chromium.org/10959020/diff/134004/content/browser/gamepad/g...
File content/browser/gamepad/gamepad_test_helpers.h (left):

http://codereview.chromium.org/10959020/diff/134004/content/browser/gamepad/g...
content/browser/gamepad/gamepad_test_helpers.h:63:
scoped_ptr<base::SystemMonitor> system_monitor_;
I think this is still needed because gampadProvider uses SystemMonitor

http://codereview.chromium.org/10959020/diff/134004/ui/views/win/hwnd_message...
File ui/views/win/hwnd_message_handler.cc (right):

http://codereview.chromium.org/10959020/diff/134004/ui/views/win/hwnd_message...
ui/views/win/hwnd_message_handler.cc:43: // Reference the Signaler defined in
the global PowerMonitor instance.
Remove or update this comment to say *why* this is here.

[Hongbo Min, 2012-10-30 14:33:46]

vandebo, thanks for your careful review. I have updated patch, and now they are
built by buildbots. I may not wait until they are finished. Pls help check the
buildbot result and re-try if one fails. Thanks.

http://codereview.chromium.org/10959020/diff/134004/base/hi_res_timer_manager...
File base/hi_res_timer_manager_unittest.cc (left):

http://codereview.chromium.org/10959020/diff/134004/base/hi_res_timer_manager...
base/hi_res_timer_manager_unittest.cc:16: scoped_ptr<base::SystemMonitor>
system_monitor(new base::SystemMonitor());
On 2012/10/29 18:04:38, vandebo wrote:
> HighResolutionTimerManager::HighResolutionTimerManager uses PowerMonitor
without
> checking for NULL, so the test needs to create one so that Get() returns one.

Done.

http://codereview.chromium.org/10959020/diff/134004/base/power_monitor/power_...
File base/power_monitor/power_monitor.cc (right):

http://codereview.chromium.org/10959020/diff/134004/base/power_monitor/power_...
base/power_monitor/power_monitor.cc:42: delete signaler_;
On 2012/10/29 18:04:38, vandebo wrote:
> Don't manage lifetimes in the destructor:  If this class owns signaler_, then
it
> should be in a scoped_ptr<>.  But I don't think this class can own it since
the
> class that gets it could outlive this class.  In that case, there's no reason
to
> store it, just store a bool indicating if we've given own out.
> 
> Furthermore, since signaler_ can outlive PowerMonitor, signaler_ should
> internally call PowerMonitor::Get() to see if it's still alive.

You are right.

http://codereview.chromium.org/10959020/diff/134004/base/power_monitor/power_...
base/power_monitor/power_monitor.cc:53: CHECK(signaler_ == NULL);
On 2012/10/29 18:04:38, vandebo wrote:
> The interface says it will return NULL on the second call, this checks =
crash.

Sorry for this low level mistake:)

http://codereview.chromium.org/10959020/diff/134004/base/power_monitor/power_...
base/power_monitor/power_monitor.cc:56: // The signaler is allowed to be leaked
since the PowerMonitor and Signaler
On 2012/10/29 18:04:38, vandebo wrote:
> Comment is out of date.

Done.

http://codereview.chromium.org/10959020/diff/134004/base/power_monitor/power_...
File base/power_monitor/power_monitor.h (right):

http://codereview.chromium.org/10959020/diff/134004/base/power_monitor/power_...
base/power_monitor/power_monitor.h:55: CHECK(monitor_);
On 2012/10/29 18:04:38, vandebo wrote:
> These checks don't do anything.

Use if stmt instead.

http://codereview.chromium.org/10959020/diff/134004/base/power_monitor/power_...
base/power_monitor/power_monitor.h:68: ~Signaler() {}
On 2012/10/29 18:04:38, vandebo wrote:
> Don't inline the implementation, but it in the implementation file.

Done.

http://codereview.chromium.org/10959020/diff/134004/base/power_monitor/power_...
base/power_monitor/power_monitor.h:79: // Return the |signaler_| pointer at the
first time it gets called, otherwise
On 2012/10/29 18:04:38, vandebo wrote:
> nit: The first call to this method will return an instance of Signaler which
the
> caller can use to signal power events.  All subsequent calls will return NULL.

Done.

http://codereview.chromium.org/10959020/diff/134004/base/power_monitor/power_...
File base/power_monitor/power_monitor_android.cc (right):

http://codereview.chromium.org/10959020/diff/134004/base/power_monitor/power_...
base/power_monitor/power_monitor_android.cc:10: static PowerMonitor::Signaler*
g_power_signaler = NULL;
On 2012/10/29 18:04:38, vandebo wrote:
> This should go in an anonymous namespace.

Done.

http://codereview.chromium.org/10959020/diff/134004/base/power_monitor/power_...
File base/power_monitor/power_monitor_mac.mm (left):

http://codereview.chromium.org/10959020/diff/134004/base/power_monitor/power_...
base/power_monitor/power_monitor_mac.mm:76: if (g_system_power_io_port == 0)
On 2012/10/29 18:04:38, vandebo wrote:
> Leave these lines in.

Done.

http://codereview.chromium.org/10959020/diff/134004/base/power_monitor/power_...
File base/power_monitor/power_monitor_mac.mm (right):

http://codereview.chromium.org/10959020/diff/134004/base/power_monitor/power_...
base/power_monitor/power_monitor_mac.mm:19: io_object_t g_signaler_object = 0;
On 2012/10/29 18:04:38, vandebo wrote:
> no reason to change the name of this variable.
It mainly arises from the grep and replace action in VIM. 

Fix it now.

http://codereview.chromium.org/10959020/diff/134004/base/power_monitor/power_...
base/power_monitor/power_monitor_mac.mm:57: &g_signaler_object);
On 2012/10/29 18:04:38, vandebo wrote:
> and here

Done.

http://codereview.chromium.org/10959020/diff/134004/base/power_monitor/power_...
base/power_monitor/power_monitor_mac.mm:67: CHECK_NE(g_system_power_io_port,
0u);
On 2012/10/29 18:04:38, vandebo wrote:
> Move this just under the comment online 64

Done.

http://codereview.chromium.org/10959020/diff/134004/base/power_monitor/power_...
base/power_monitor/power_monitor_mac.mm:76: void PowerMonitor::PlatformDestroy()
{
On 2012/10/29 18:04:38, vandebo wrote:
> Maybe PlatformDestroy should clean up g_power_signaler

I don't clean it in PlatformDestroy. The reason is it can't keep the same
behavior on other platform, e.g. Windows, Android.

Current approach is just to leak the Signaler.

http://codereview.chromium.org/10959020/diff/134004/base/power_monitor/power_...
base/power_monitor/power_monitor_mac.mm:77: CHECK_NE(g_system_power_io_port,
0u);
On 2012/10/29 18:04:38, vandebo wrote:
> Levae this as a DCHECK

Done.

http://codereview.chromium.org/10959020/diff/134004/base/power_monitor/power_...
base/power_monitor/power_monitor_mac.mm:86:
IODeregisterForSystemPower(&g_signaler_object);
On 2012/10/29 18:04:38, vandebo wrote:
> and here

Done.

http://codereview.chromium.org/10959020/diff/134004/chrome/browser/extensions...
File chrome/browser/extensions/event_router_forwarder_unittest.cc (right):

http://codereview.chromium.org/10959020/diff/134004/chrome/browser/extensions...
chrome/browser/extensions/event_router_forwarder_unittest.cc:95:
base::PowerMonitor::AllocateSystemIOPorts();
On 2012/10/29 18:04:38, vandebo wrote:
> Is this needed?

Remove it now.

http://codereview.chromium.org/10959020/diff/134004/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager_unittest.cc (right):

http://codereview.chromium.org/10959020/diff/134004/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager_unittest.cc:101:
base::PowerMonitor::AllocateSystemIOPorts();
On 2012/10/29 18:04:38, vandebo wrote:
> Is this needed?
Remove it now.

http://codereview.chromium.org/10959020/diff/134004/content/browser/gamepad/g...
File content/browser/gamepad/gamepad_test_helpers.cc (right):

http://codereview.chromium.org/10959020/diff/134004/content/browser/gamepad/g...
content/browser/gamepad/gamepad_test_helpers.cc:41:
base::PowerMonitor::AllocateSystemIOPorts();
On 2012/10/29 18:04:38, vandebo wrote:
> This probably isn't needed.

Remove it now.

http://codereview.chromium.org/10959020/diff/134004/content/browser/gamepad/g...
File content/browser/gamepad/gamepad_test_helpers.h (left):

http://codereview.chromium.org/10959020/diff/134004/content/browser/gamepad/g...
content/browser/gamepad/gamepad_test_helpers.h:63:
scoped_ptr<base::SystemMonitor> system_monitor_;
On 2012/10/29 18:04:38, vandebo wrote:
> I think this is still needed because gampadProvider uses SystemMonitor

I grep the code and didn't find the related code, and the testing code might not
need a SystemMonitor instance.

http://codereview.chromium.org/10959020/diff/134004/ui/views/win/hwnd_message...
File ui/views/win/hwnd_message_handler.cc (right):

http://codereview.chromium.org/10959020/diff/134004/ui/views/win/hwnd_message...
ui/views/win/hwnd_message_handler.cc:43: // Reference the Signaler defined in
the global PowerMonitor instance.
On 2012/10/29 18:04:38, vandebo wrote:
> Remove or update this comment to say *why* this is here.

Done.

[vandebo (ex-Chrome), 2012-10-30 20:41:23]

Getting there.  Mostly nits.

http://codereview.chromium.org/10959020/diff/134004/base/power_monitor/power_...
File base/power_monitor/power_monitor_mac.mm (right):

http://codereview.chromium.org/10959020/diff/134004/base/power_monitor/power_...
base/power_monitor/power_monitor_mac.mm:76: void PowerMonitor::PlatformDestroy()
{
On 2012/10/30 14:33:47, Hongbo Min wrote:
> On 2012/10/29 18:04:38, vandebo wrote:
> > Maybe PlatformDestroy should clean up g_power_signaler
> 
> I don't clean it in PlatformDestroy. The reason is it can't keep the same
> behavior on other platform, e.g. Windows, Android.
> 
> Current approach is just to leak the Signaler.

I'm ok having PlatformInit/PlatformDestroy on all platforms, even if it is empty
on Windows.  If we don't have to leak the Signaler, then we shouldn't.

http://codereview.chromium.org/10959020/diff/134012/base/power_monitor/power_...
File base/power_monitor/power_monitor.cc (right):

http://codereview.chromium.org/10959020/diff/134012/base/power_monitor/power_...
base/power_monitor/power_monitor.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
Can you set similarity so that this is detected as a copy?

http://codereview.chromium.org/10959020/diff/134012/base/power_monitor/power_...
File base/power_monitor/power_monitor.h (right):

http://codereview.chromium.org/10959020/diff/134012/base/power_monitor/power_...
base/power_monitor/power_monitor.h:43: // A helper class to wrap the process of
power event in PowerMonitor. The
nit: 

A helper class that exposes ProcessPowerEvent to the power information provider.
This approach is taken instead of using friends because some providers live
outside of base (i.e. views::HWNDMessageHandler).

http://codereview.chromium.org/10959020/diff/134012/base/power_monitor/power_...
base/power_monitor/power_monitor.h:60: // Only allows PowerMonitor to create it.
nit: Only PowerMonitor can create this.

http://codereview.chromium.org/10959020/diff/134012/base/power_monitor/power_...
base/power_monitor/power_monitor.h:62: ~Signaler();
The destructor probably should be public so that the caller can free it.

http://codereview.chromium.org/10959020/diff/134012/base/power_monitor/power_...
base/power_monitor/power_monitor.h:68: // Return the singleton instance in the
application wide.
nit: Get the application-wide PowerMonitor (if not present, returns NULL).

http://codereview.chromium.org/10959020/diff/134012/base/power_monitor/power_...
base/power_monitor/power_monitor.h:73: // return NULL.
add: The caller owns the result.

http://codereview.chromium.org/10959020/diff/134012/base/power_monitor/power_...
base/power_monitor/power_monitor.h:75: // Note that we assume there is at most
only one power event source.
Remove this line.

http://codereview.chromium.org/10959020/diff/134012/base/power_monitor/power_...
base/power_monitor/power_monitor.h:148: bool is_get_signaler_called_;
This name is a bit cumbersome. How about signaller_available_ ?

http://codereview.chromium.org/10959020/diff/134012/base/power_monitor/power_...
File base/power_monitor/power_monitor_unittest.cc (right):

http://codereview.chromium.org/10959020/diff/134012/base/power_monitor/power_...
base/power_monitor/power_monitor_unittest.cc:55: void
ProcessPowerEvent(PowerMonitor::PowerEvent event) {
I think the test can now use Signaler, and doesn't need to be a friend.

http://codereview.chromium.org/10959020/diff/134012/base/power_monitor/power_...
base/power_monitor/power_monitor_unittest.cc:104: 
Add a test that Signaler can only be retrieved once.

http://codereview.chromium.org/10959020/diff/134012/content/browser/gamepad/g...
File content/browser/gamepad/gamepad_test_helpers.cc (right):

http://codereview.chromium.org/10959020/diff/134012/content/browser/gamepad/g...
content/browser/gamepad/gamepad_test_helpers.cc:7: #include
"base/power_monitor/power_monitor.h"
not needed

http://codereview.chromium.org/10959020/diff/134012/net/url_request/url_reque...
File net/url_request/url_request_job.h (right):

http://codereview.chromium.org/10959020/diff/134012/net/url_request/url_reque...
net/url_request/url_request_job.h:8: #include <map>
extra

http://codereview.chromium.org/10959020/diff/134012/ui/views/win/hwnd_message...
File ui/views/win/hwnd_message_handler.cc (right):

http://codereview.chromium.org/10959020/diff/134012/ui/views/win/hwnd_message...
ui/views/win/hwnd_message_handler.cc:1856: CHECK(!g_power_signaler);
CHECK(g_power_signaler)

http://codereview.chromium.org/10959020/diff/134012/ui/views/win/hwnd_message...
ui/views/win/hwnd_message_handler.cc:1856: CHECK(!g_power_signaler);
Wrap both of these lines in an if (base::SystemMonitir::Get())

[Hongbo Min, 2012-10-31 12:52:50]

http://codereview.chromium.org/10959020/diff/134012/base/power_monitor/power_...
File base/power_monitor/power_monitor.cc (right):

http://codereview.chromium.org/10959020/diff/134012/base/power_monitor/power_...
base/power_monitor/power_monitor.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2012/10/30 20:41:24, vandebo wrote:
> Can you set similarity so that this is detected as a copy?

Done.

http://codereview.chromium.org/10959020/diff/134012/base/power_monitor/power_...
File base/power_monitor/power_monitor.h (right):

http://codereview.chromium.org/10959020/diff/134012/base/power_monitor/power_...
base/power_monitor/power_monitor.h:43: // A helper class to wrap the process of
power event in PowerMonitor. The
On 2012/10/30 20:41:24, vandebo wrote:
> nit: 
> 
> A helper class that exposes ProcessPowerEvent to the power information
provider.
> This approach is taken instead of using friends because some providers live
> outside of base (i.e. views::HWNDMessageHandler).

Done.

http://codereview.chromium.org/10959020/diff/134012/base/power_monitor/power_...
base/power_monitor/power_monitor.h:60: // Only allows PowerMonitor to create it.
On 2012/10/30 20:41:24, vandebo wrote:
> nit: Only PowerMonitor can create this.

Done.

http://codereview.chromium.org/10959020/diff/134012/base/power_monitor/power_...
base/power_monitor/power_monitor.h:62: ~Signaler();
On 2012/10/30 20:41:24, vandebo wrote:
> The destructor probably should be public so that the caller can free it.

Done.

http://codereview.chromium.org/10959020/diff/134012/base/power_monitor/power_...
base/power_monitor/power_monitor.h:68: // Return the singleton instance in the
application wide.
On 2012/10/30 20:41:24, vandebo wrote:
> nit: Get the application-wide PowerMonitor (if not present, returns NULL).

Done.

http://codereview.chromium.org/10959020/diff/134012/base/power_monitor/power_...
base/power_monitor/power_monitor.h:73: // return NULL.
On 2012/10/30 20:41:24, vandebo wrote:
> add: The caller owns the result.

Done.

http://codereview.chromium.org/10959020/diff/134012/base/power_monitor/power_...
base/power_monitor/power_monitor.h:75: // Note that we assume there is at most
only one power event source.
On 2012/10/30 20:41:24, vandebo wrote:
> Remove this line.

Done.

http://codereview.chromium.org/10959020/diff/134012/base/power_monitor/power_...
base/power_monitor/power_monitor.h:148: bool is_get_signaler_called_;
On 2012/10/30 20:41:24, vandebo wrote:
> This name is a bit cumbersome. How about signaller_available_ ?

Done.

http://codereview.chromium.org/10959020/diff/134012/base/power_monitor/power_...
File base/power_monitor/power_monitor_unittest.cc (right):

http://codereview.chromium.org/10959020/diff/134012/base/power_monitor/power_...
base/power_monitor/power_monitor_unittest.cc:55: void
ProcessPowerEvent(PowerMonitor::PowerEvent event) {
On 2012/10/30 20:41:24, vandebo wrote:
> I think the test can now use Signaler, and doesn't need to be a friend.

Done.

http://codereview.chromium.org/10959020/diff/134012/base/power_monitor/power_...
base/power_monitor/power_monitor_unittest.cc:104: 
On 2012/10/30 20:41:24, vandebo wrote:
> Add a test that Signaler can only be retrieved once.

Done.

http://codereview.chromium.org/10959020/diff/134012/content/browser/gamepad/g...
File content/browser/gamepad/gamepad_test_helpers.cc (right):

http://codereview.chromium.org/10959020/diff/134012/content/browser/gamepad/g...
content/browser/gamepad/gamepad_test_helpers.cc:7: #include
"base/power_monitor/power_monitor.h"
On 2012/10/30 20:41:24, vandebo wrote:
> not needed

Done.

http://codereview.chromium.org/10959020/diff/134012/content/browser/gamepad/g...
content/browser/gamepad/gamepad_test_helpers.cc:7: #include
"base/power_monitor/power_monitor.h"
On 2012/10/30 20:41:24, vandebo wrote:
> not needed

Done.

http://codereview.chromium.org/10959020/diff/134012/net/url_request/url_reque...
File net/url_request/url_request_job.h (right):

http://codereview.chromium.org/10959020/diff/134012/net/url_request/url_reque...
net/url_request/url_request_job.h:8: #include <map>
On 2012/10/30 20:41:24, vandebo wrote:
> extra

It is not extra. It is required now because we only include power_observer.h
file.

http://codereview.chromium.org/10959020/diff/134012/ui/views/win/hwnd_message...
File ui/views/win/hwnd_message_handler.cc (right):

http://codereview.chromium.org/10959020/diff/134012/ui/views/win/hwnd_message...
ui/views/win/hwnd_message_handler.cc:1856: CHECK(!g_power_signaler);
On 2012/10/30 20:41:24, vandebo wrote:
> CHECK(g_power_signaler)

Done.

http://codereview.chromium.org/10959020/diff/134012/ui/views/win/hwnd_message...
ui/views/win/hwnd_message_handler.cc:1856: CHECK(!g_power_signaler);
On 2012/10/30 20:41:24, vandebo wrote:
> Wrap both of these lines in an if (base::SystemMonitir::Get())

Done.

[Hongbo Min, 2012-10-31 13:11:38]

http://codereview.chromium.org/10959020/diff/152087/base/power_monitor/power_...
File base/power_monitor/power_monitor_mac.mm (right):

http://codereview.chromium.org/10959020/diff/152087/base/power_monitor/power_...
base/power_monitor/power_monitor_mac.mm:28: if (!g_power_signaler)
g_power_signaler gets initialized here instead of PlatformInit  because it can
facilitate the testing code for GetSignalerOnce.

If the g_power_signaler is assigned in the PlatformInit, it means that we can't
not easily test the GetSignalerOnce for the first time calling.

From the sequence of initialization on other platforms, the g_power_signaler
only gets initialized on-demand.

I don't think that we need to free g_power_signaler in PlatformDestroy because
current implementation will not crash the application even the lifetime of
Signaler is longer than that of PowerMonitor's.

[vandebo (ex-Chrome), 2012-10-31 17:30:34]

http://codereview.chromium.org/10959020/diff/134012/net/url_request/url_reque...
File net/url_request/url_request_job.h (right):

http://codereview.chromium.org/10959020/diff/134012/net/url_request/url_reque...
net/url_request/url_request_job.h:8: #include <map>
On 2012/10/31 12:52:50, Hongbo Min wrote:
> On 2012/10/30 20:41:24, vandebo wrote:
> > extra
> 
> It is not extra. It is required now because we only include power_observer.h
> file.

I don't see map being used in this file.  What needs it?

http://codereview.chromium.org/10959020/diff/134012/ui/views/win/hwnd_message...
File ui/views/win/hwnd_message_handler.cc (right):

http://codereview.chromium.org/10959020/diff/134012/ui/views/win/hwnd_message...
ui/views/win/hwnd_message_handler.cc:1856: CHECK(!g_power_signaler);
On 2012/10/30 20:41:24, vandebo wrote:
> Wrap both of these lines in an if (base::SystemMonitir::Get())

This didn't get done.

http://codereview.chromium.org/10959020/diff/152087/base/power_monitor/power_...
File base/power_monitor/power_monitor.h (right):

http://codereview.chromium.org/10959020/diff/152087/base/power_monitor/power_...
base/power_monitor/power_monitor.h:141: bool is_signaler_available_;
The style guide says that variable should be nouns and methods should be verbs,
that's why this should be signaler_available_, not is_signaler_available_
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#General_Naming...

http://codereview.chromium.org/10959020/diff/152087/base/power_monitor/power_...
File base/power_monitor/power_monitor_mac.mm (right):

http://codereview.chromium.org/10959020/diff/152087/base/power_monitor/power_...
base/power_monitor/power_monitor_mac.mm:28: if (!g_power_signaler)
On 2012/10/31 13:11:38, Hongbo Min wrote:
> g_power_signaler gets initialized here instead of PlatformInit  because it can
> facilitate the testing code for GetSignalerOnce.
>
> If the g_power_signaler is assigned in the PlatformInit, it means that we
can't
> not easily test the GetSignalerOnce for the first time calling.

Hmm, I think it's better to get it in PlatformInit and we can still make the
test work.  PowerMonitorTest can friend PowerMonitor and reset
is_signaler_available_ in its SetUp method to get its own signaler.
 
> From the sequence of initialization on other platforms, the g_power_signaler
> only gets initialized on-demand.

I think it's better to make PlatformInit valid on all platforms and get the
signaler in there if possible (doesn't work for Windows).

> I don't think that we need to free g_power_signaler in PlatformDestroy because
> current implementation will not crash the application even the lifetime of
> Signaler is longer than that of PowerMonitor's.

It won't crash, but it will leak and our memory tests will see that.  We can
clean up the memory, so we should.

You can run the linux_valgrind bot or win_drmemory to tests memory issues.

http://codereview.chromium.org/10959020/diff/152087/base/power_monitor/power_...
File base/power_monitor/power_monitor_unittest.cc (right):

http://codereview.chromium.org/10959020/diff/152087/base/power_monitor/power_...
base/power_monitor/power_monitor_unittest.cc:63:
scoped_ptr<PowerMonitor::Signaler> power_signaler;
nit: you can put this in PowerMonitorTest and init it in the constructor.

[Hongbo Min, 2012-11-01 09:26:23]

http://codereview.chromium.org/10959020/diff/152087/base/power_monitor/power_...
File base/power_monitor/power_monitor.h (right):

http://codereview.chromium.org/10959020/diff/152087/base/power_monitor/power_...
base/power_monitor/power_monitor.h:141: bool is_signaler_available_;
On 2012/10/31 17:30:34, vandebo wrote:
> The style guide says that variable should be nouns and methods should be
verbs,
> that's why this should be signaler_available_, not is_signaler_available_
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#General_Naming...

Done.

http://codereview.chromium.org/10959020/diff/152087/base/power_monitor/power_...
File base/power_monitor/power_monitor_mac.mm (right):

http://codereview.chromium.org/10959020/diff/152087/base/power_monitor/power_...
base/power_monitor/power_monitor_mac.mm:28: if (!g_power_signaler)
On 2012/10/31 17:30:34, vandebo wrote:
> On 2012/10/31 13:11:38, Hongbo Min wrote:
> > g_power_signaler gets initialized here instead of PlatformInit  because it
can
> > facilitate the testing code for GetSignalerOnce.
> >
> > If the g_power_signaler is assigned in the PlatformInit, it means that we
> can't
> > not easily test the GetSignalerOnce for the first time calling.
> 
> Hmm, I think it's better to get it in PlatformInit and we can still make the
> test work.  PowerMonitorTest can friend PowerMonitor and reset
> is_signaler_available_ in its SetUp method to get its own signaler.
>  
> > From the sequence of initialization on other platforms, the g_power_signaler
> > only gets initialized on-demand.
> 
> I think it's better to make PlatformInit valid on all platforms and get the
> signaler in there if possible (doesn't work for Windows).
> 
> > I don't think that we need to free g_power_signaler in PlatformDestroy
because
> > current implementation will not crash the application even the lifetime of
> > Signaler is longer than that of PowerMonitor's.
> 
> It won't crash, but it will leak and our memory tests will see that.  We can
> clean up the memory, so we should.
> 
> You can run the linux_valgrind bot or win_drmemory to tests memory issues.

Right now the PlatformInit/PlatformDestroy is available for all platform
although empty impl on Windows.

http://codereview.chromium.org/10959020/diff/152087/base/power_monitor/power_...
File base/power_monitor/power_monitor_unittest.cc (right):

http://codereview.chromium.org/10959020/diff/152087/base/power_monitor/power_...
base/power_monitor/power_monitor_unittest.cc:63:
scoped_ptr<PowerMonitor::Signaler> power_signaler;
On 2012/10/31 17:30:34, vandebo wrote:
> nit: you can put this in PowerMonitorTest and init it in the constructor.

Done.

[Hongbo Min, 2012-11-01 09:26:57]

http://codereview.chromium.org/10959020/diff/134012/ui/views/win/hwnd_message...
File ui/views/win/hwnd_message_handler.cc (right):

http://codereview.chromium.org/10959020/diff/134012/ui/views/win/hwnd_message...
ui/views/win/hwnd_message_handler.cc:1856: CHECK(!g_power_signaler);
On 2012/10/31 17:30:34, vandebo wrote:
> On 2012/10/30 20:41:24, vandebo wrote:
> > Wrap both of these lines in an if (base::SystemMonitir::Get())
> 
> This didn't get done.

Done.

[vandebo (ex-Chrome), 2012-11-01 16:50:26]

Will, there are a few minor issues noted below, but I'm about ready to sign off.
Can you take a look?

http://codereview.chromium.org/10959020/diff/145218/base/power_monitor/power_...
File base/power_monitor/power_monitor.cc (right):

http://codereview.chromium.org/10959020/diff/145218/base/power_monitor/power_...
base/power_monitor/power_monitor.cc:21: signaler_available_(false)  {
This is backward (consistently).  signaler_available = false should mean that
we've already handed out the signaler.

http://codereview.chromium.org/10959020/diff/145218/base/power_monitor/power_...
File base/power_monitor/power_monitor_android.cc (right):

http://codereview.chromium.org/10959020/diff/145218/base/power_monitor/power_...
base/power_monitor/power_monitor_android.cc:21: CHECK(g_power_signaler);
There doesn't seem to be anything unregistering the OS callback here, so change
this to an if and add a CHECK(g_power_signaler) on line 30.

http://codereview.chromium.org/10959020/diff/145218/base/power_monitor/power_...
File base/power_monitor/power_monitor_posix.cc (right):

http://codereview.chromium.org/10959020/diff/145218/base/power_monitor/power_...
base/power_monitor/power_monitor_posix.cc:9: #if !defined(OS_MACOSX) &&
!defined(OS_IOS)
I think OS_MACOSX is defined when OS_IOS is defined, so you want just
!defined(OS_MACOSX) here.

http://codereview.chromium.org/10959020/diff/145218/base/power_monitor/power_...
File base/power_monitor/power_monitor_unittest.cc (right):

http://codereview.chromium.org/10959020/diff/145218/base/power_monitor/power_...
base/power_monitor/power_monitor_unittest.cc:52:
power_signaler_.reset(PowerMonitor::Get()->GetSignalerOnce());
Platform init is called in the PowerMonitor constructor, so don't you need to
call MakeSignalerAvailable before getting it?  On Windows this will work as it
is, but not on Mac.

http://codereview.chromium.org/10959020/diff/145218/base/power_monitor/power_...
base/power_monitor/power_monitor_unittest.cc:58: monitor->signaler_available_ =
false;
combine lines: power_monitor->signaler_available_ = false;

http://codereview.chromium.org/10959020/diff/145218/base/power_monitor/power_...
base/power_monitor/power_monitor_unittest.cc:110: EXPECT_TRUE(NULL !=
power_monitor_->GetSignalerOnce());
This leaks the signaler. You need the scoped_ptr like you had before.

[Hongbo Min, 2012-11-02 00:29:07]

http://codereview.chromium.org/10959020/diff/145218/base/power_monitor/power_...
File base/power_monitor/power_monitor.cc (right):

http://codereview.chromium.org/10959020/diff/145218/base/power_monitor/power_...
base/power_monitor/power_monitor.cc:21: signaler_available_(false)  {
On 2012/11/01 16:50:26, vandebo wrote:
> This is backward (consistently).  signaler_available = false should mean that
> we've already handed out the signaler.

Done.

http://codereview.chromium.org/10959020/diff/145218/base/power_monitor/power_...
File base/power_monitor/power_monitor_android.cc (right):

http://codereview.chromium.org/10959020/diff/145218/base/power_monitor/power_...
base/power_monitor/power_monitor_android.cc:21: CHECK(g_power_signaler);
On 2012/11/01 16:50:26, vandebo wrote:
> There doesn't seem to be anything unregistering the OS callback here, so
change
> this to an if and add a CHECK(g_power_signaler) on line 30.

Done.

http://codereview.chromium.org/10959020/diff/145218/base/power_monitor/power_...
File base/power_monitor/power_monitor_unittest.cc (right):

http://codereview.chromium.org/10959020/diff/145218/base/power_monitor/power_...
base/power_monitor/power_monitor_unittest.cc:52:
power_signaler_.reset(PowerMonitor::Get()->GetSignalerOnce());
On 2012/11/01 16:50:26, vandebo wrote:
> Platform init is called in the PowerMonitor constructor, so don't you need to
> call MakeSignalerAvailable before getting it?  On Windows this will work as it
> is, but not on Mac.

Done.

http://codereview.chromium.org/10959020/diff/145218/base/power_monitor/power_...
base/power_monitor/power_monitor_unittest.cc:58: monitor->signaler_available_ =
false;
On 2012/11/01 16:50:26, vandebo wrote:
> combine lines: power_monitor->signaler_available_ = false;

Done.

http://codereview.chromium.org/10959020/diff/145218/base/power_monitor/power_...
base/power_monitor/power_monitor_unittest.cc:110: EXPECT_TRUE(NULL !=
power_monitor_->GetSignalerOnce());
On 2012/11/01 16:50:26, vandebo wrote:
> This leaks the signaler. You need the scoped_ptr like you had before.

Done.

[vandebo (ex-Chrome), 2012-11-02 00:52:09]

LGTM with nit.

You now need OWNERS approval

http://codereview.chromium.org/10959020/diff/153091/base/power_monitor/power_...
File base/power_monitor/power_monitor_unittest.cc (right):

http://codereview.chromium.org/10959020/diff/153091/base/power_monitor/power_...
base/power_monitor/power_monitor_unittest.cc:112: EXPECT_TRUE(NULL !=
signaler.get());
nit: we prefer signaler.get() != NULL. Similar on line 116.

[willchan no longer on Chromium, 2012-11-02 00:57:58]

Sorry, I had this sitting in my browser all day. I'll try to get to it
tomorrow morning.


On Thu, Nov 1, 2012 at 5:52 PM, <vandebo@chromium.org> wrote:

> LGTM with nit.
>
> You now need OWNERS approval
>
>
> http://codereview.chromium.**org/10959020/diff/153091/base/**
>
power_monitor/power_monitor_**unittest.cc<http://codereview.chromium.org/10959020/diff/153091/base/power_monitor/power_monitor_unittest.cc>
> File base/power_monitor/power_**monitor_unittest.cc (right):
>
> http://codereview.chromium.**org/10959020/diff/153091/base/**
>
power_monitor/power_monitor_**unittest.cc#newcode112<http://codereview.chromium.org/10959020/diff/153091/base/power_monitor/power_monitor_unittest.cc#newcode112>
> base/power_monitor/power_**monitor_unittest.cc:112: EXPECT_TRUE(NULL !=
> signaler.get());
> nit: we prefer signaler.get() != NULL. Similar on line 116.
>
>
http://codereview.chromium.**org/10959020/<http://codereview.chromium.org/109...
>

[willchan no longer on Chromium, 2012-11-02 21:35:27]

LGTM, thanks for the changes. Thanks Steve for doing the primary review here.

http://codereview.chromium.org/10959020/diff/152096/base/power_monitor/power_...
File base/power_monitor/power_monitor.cc (right):

http://codereview.chromium.org/10959020/diff/152096/base/power_monitor/power_...
base/power_monitor/power_monitor.cc:51: if (signaler_available_) {
indentation off

http://codereview.chromium.org/10959020/diff/152096/base/power_monitor/power_...
File base/power_monitor/power_monitor.h (right):

http://codereview.chromium.org/10959020/diff/152096/base/power_monitor/power_...
base/power_monitor/power_monitor.h:52: void
ProcessPowerEvent(PowerMonitor::PowerEvent event);
s/PowerMonitor::PowerEvent/PowerEvent/

http://codereview.chromium.org/10959020/diff/152096/base/power_monitor/power_...
base/power_monitor/power_monitor.h:69: PowerMonitor::Signaler*
GetSignalerOnce();
Please note that this is not threadsafe. It's up to the embedder to enforce
thread safety (since it should only be called once anyway).

[Hongbo Min, 2012-11-05 09:36:42]

http://codereview.chromium.org/10959020/diff/152096/base/power_monitor/power_...
File base/power_monitor/power_monitor.cc (right):

http://codereview.chromium.org/10959020/diff/152096/base/power_monitor/power_...
base/power_monitor/power_monitor.cc:51: if (signaler_available_) {
On 2012/11/02 21:35:27, willchan wrote:
> indentation off

Done.

http://codereview.chromium.org/10959020/diff/152096/base/power_monitor/power_...
File base/power_monitor/power_monitor.h (right):

http://codereview.chromium.org/10959020/diff/152096/base/power_monitor/power_...
base/power_monitor/power_monitor.h:52: void
ProcessPowerEvent(PowerMonitor::PowerEvent event);
On 2012/11/02 21:35:27, willchan wrote:
> s/PowerMonitor::PowerEvent/PowerEvent/

Done.

http://codereview.chromium.org/10959020/diff/152096/base/power_monitor/power_...
base/power_monitor/power_monitor.h:69: PowerMonitor::Signaler*
GetSignalerOnce();
On 2012/11/02 21:35:27, willchan wrote:
> Please note that this is not threadsafe. It's up to the embedder to enforce
> thread safety (since it should only be called once anyway).

Add a comment for the method.

[commit-bot: I haz the power, 2012-11-05 09:37:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hongbo.min@intel.com/10959020/153145

[commit-bot: I haz the power, 2012-11-05 09:38:05]

Presubmit check for 10959020-153145 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    ui/views
    webkit/blob

Presubmit checks took 4.2s to calculate.

[Hongbo Min, 2012-11-05 10:14:58]

On 2012/11/05 09:38:05, I haz the power (commit-bot) wrote:
> Presubmit check for 10959020-153145 failed and returned exit status 1.
> 
> 
> Running presubmit commit checks ...
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for files in these directories:
>     ui/views
>     webkit/blob
> 
> Presubmit checks took 4.2s to calculate.

ben@, could you pls help review ui/views change?
jianli@, could you pls help review webkit/blob change? Thanks.

[Ben Goodger (Google), 2012-11-05 16:13:01]

http://codereview.chromium.org/10959020/diff/153145/ui/views/win/hwnd_message...
File ui/views/win/hwnd_message_handler.cc (right):

http://codereview.chromium.org/10959020/diff/153145/ui/views/win/hwnd_message...
ui/views/win/hwnd_message_handler.cc:1855: LRESULT
HWNDMessageHandler::OnPowerBroadcast(DWORD power_event, DWORD data) {
I think we should be moving to get rid of this stuff from Views, not making its
footprint larger.

[jianli, 2012-11-05 18:33:12]

webkit/blob LGTM.

[vandebo (ex-Chrome), 2012-11-05 18:36:11]

http://codereview.chromium.org/10959020/diff/153145/ui/views/win/hwnd_message...
File ui/views/win/hwnd_message_handler.cc (right):

http://codereview.chromium.org/10959020/diff/153145/ui/views/win/hwnd_message...
ui/views/win/hwnd_message_handler.cc:1855: LRESULT
HWNDMessageHandler::OnPowerBroadcast(DWORD power_event, DWORD data) {
On 2012/11/05 16:13:01, Ben Goodger (Google) wrote:
> I think we should be moving to get rid of this stuff from Views, not making
its
> footprint larger.

Ben, as I noted in the bug there is a fair amount of uncertainty in how we
should "get rid of this stuff."  Doing the other parts of the SystemMonitor
refactor may provide clarity on the right solution. Is this small increase in
footprint acceptable in the short term?

[Hongbo Min, 2012-11-07 01:26:46]

On 2012/11/05 18:36:11, vandebo wrote:
>
http://codereview.chromium.org/10959020/diff/153145/ui/views/win/hwnd_message...
> File ui/views/win/hwnd_message_handler.cc (right):
> 
>
http://codereview.chromium.org/10959020/diff/153145/ui/views/win/hwnd_message...
> ui/views/win/hwnd_message_handler.cc:1855: LRESULT
> HWNDMessageHandler::OnPowerBroadcast(DWORD power_event, DWORD data) {
> On 2012/11/05 16:13:01, Ben Goodger (Google) wrote:
> > I think we should be moving to get rid of this stuff from Views, not making
> its
> > footprint larger.
> 
> Ben, as I noted in the bug there is a fair amount of uncertainty in how we
> should "get rid of this stuff."  Doing the other parts of the SystemMonitor
> refactor may provide clarity on the right solution. Is this small increase in
> footprint acceptable in the short term?

I agree vandebo's opinion. In short term, we need to firstly decouple the
functionalities exposed by SystemMonitor, and then consider how to fix the power
broadcast message handler in views.

[vandebo (ex-Chrome), 2012-11-08 03:27:32]

ping... Ben.  Is this ok for now?

[Ben Goodger (Google), 2012-11-12 16:16:36]

No, I don't want to see more stuff get added here. If this stuff is going
to be refactored, it should be removed.

-Ben


On Wed, Nov 7, 2012 at 7:27 PM, <vandebo@chromium.org> wrote:

> ping... Ben.  Is this ok for now?
>
>
http://codereview.chromium.**org/10959020/<http://codereview.chromium.org/109...
>

[Hongbo Min, 2012-11-16 05:07:13]

On 2012/11/12 16:16:36, Ben Goodger (Google) wrote:
> No, I don't want to see more stuff get added here. If this stuff is going
> to be refactored, it should be removed.
> 
> -Ben
> 
> 
> On Wed, Nov 7, 2012 at 7:27 PM, <mailto:vandebo@chromium.org> wrote:
> 
> > ping... Ben.  Is this ok for now?
> >
> >
>
http://codereview.chromium.**org/10959020/%3Chttp://codereview.chromium.org/1...>
> >

Do you have any suggestion to remove it? Thanks.

[vandebo (ex-Chrome), 2013-01-03 21:52:51]

On 2012/11/16 05:07:13, Hongbo Min wrote:
> Do you have any suggestion to remove it? Thanks.

Hongbo, I took a look, and it seems that this code could be moved into 
BrowserFrameWin::PreHandleMSG.  I would suggest making a small class
'PowerSignallerWin' that contains the code that's currently in
hwnd_message_handler.cc and then have BrowserFrameWin create one of those and
use it in PreHandleMSG.  This will minimize the change to 
BrowserFrameWin and pull the code more to the implementation layer and out of
the framework layer.

[Hongbo Min, 2013-01-04 02:16:19]

On 2013/01/03 21:52:51, vandebo wrote:
> On 2012/11/16 05:07:13, Hongbo Min wrote:
> > Do you have any suggestion to remove it? Thanks.
> 
> Hongbo, I took a look, and it seems that this code could be moved into 
> BrowserFrameWin::PreHandleMSG.  I would suggest making a small class
> 'PowerSignallerWin' that contains the code that's currently in
> hwnd_message_handler.cc and then have BrowserFrameWin create one of those and
> use it in PreHandleMSG.  This will minimize the change to 
> BrowserFrameWin and pull the code more to the implementation layer and out of
> the framework layer.

OK. I will have a try. Thanks for your hint.

[Hongbo Min, 2013-01-04 15:12:28]

vandebo, the patch is updated with the change that processing power event in
BrowserFrameWin instead of HWNDMessageHandler, as suggested by you.

One change I would like to highlight is, since BrowserFrameWin may have multiple
instances at runtime, and each one hold a scoped power event signaler, but on
the other side, we need to make sure that only one BrowserFrameWin instance is
processing power event. So I add MakeSignalerAvailable to make the
GetSignalerOnce can return a new signaler again. It is called in the signaler's
dtor. Once one BrowserFrameWin is destroyed and so its power event signaler is
also destroyed, other BrowserFrameWin instance still have an opportunity to
process power event since the GetSignalerOnce return non-NULL value now.

Please have a review. Thanks.

[vandebo (ex-Chrome), 2013-01-04 22:42:29]

Ben, is this better?

[Ben Goodger (Google), 2013-01-08 21:44:53]

https://codereview.chromium.org/10959020/diff/183012/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_frame_win.cc (right):

https://codereview.chromium.org/10959020/diff/183012/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame_win.cc:248:
power_signaler_->ProcessWmPowerBroadcastMessage(power_event);
I still don't like having this code in UI code. It's not logically UI-related.
Also, there is an implication that an embedder of content will need to provide
this in their UI layer to get power management APIs to work. This is an
unreasonable burden. In general you should seek to isolate all the code related
to a feature in the lowest layer that needs it. This is why I had advocated for
a message window, and it could be initialized from content (or lower) wherever
it is needed.

[Hongbo Min, 2013-01-09 05:00:54]

On 2013/01/08 21:44:53, Ben Goodger (Google) wrote:
>
https://codereview.chromium.org/10959020/diff/183012/chrome/browser/ui/views/...
> File chrome/browser/ui/views/frame/browser_frame_win.cc (right):
> 
>
https://codereview.chromium.org/10959020/diff/183012/chrome/browser/ui/views/...
> chrome/browser/ui/views/frame/browser_frame_win.cc:248:
> power_signaler_->ProcessWmPowerBroadcastMessage(power_event);
> I still don't like having this code in UI code. It's not logically UI-related.
> Also, there is an implication that an embedder of content will need to provide
> this in their UI layer to get power management APIs to work. This is an
> unreasonable burden. In general you should seek to isolate all the code
related
> to a feature in the lowest layer that needs it. This is why I had advocated
for
> a message window, and it could be initialized from content (or lower) wherever
> it is needed.

How about add the power message handling logic into SystemMessageWindow::WndProc
defined in content/browser/system_message_window.cc file. The
SystemMessageWindow is initialized before browser main loop start. See
https://code.google.com/searchframe#OAMlx_jo-ck/src/content/browser/system_me...

[Ben Goodger (Google), 2013-01-09 17:28:21]

On 2013/01/09 05:00:54, Hongbo Min wrote:
> How about add the power message handling logic into
SystemMessageWindow::WndProc
> defined in content/browser/system_message_window.cc file. The
> SystemMessageWindow is initialized before browser main loop start. See
>
https://code.google.com/searchframe#OAMlx_jo-ck/src/content/browser/system_me...

That does sound like an improvement.

[Hongbo Min, 2013-01-10 14:28:40]

On 2013/01/09 17:28:21, Ben Goodger (Google) wrote:
> On 2013/01/09 05:00:54, Hongbo Min wrote:
> > How about add the power message handling logic into
> SystemMessageWindow::WndProc
> > defined in content/browser/system_message_window.cc file. The
> > SystemMessageWindow is initialized before browser main loop start. See
> >
>
https://code.google.com/searchframe#OAMlx_jo-ck/src/content/browser/system_me...
> 
> That does sound like an improvement.

Ben, thanks for your suggestions. Since this CL is a little big. To sort out
this improvement quickly, I create a new CL at
https://codereview.chromium.org/11821050/.

[Hongbo Min, 2013-01-12 11:22:58]

vandebo@ and willchan@, I have a concern about the GetSignalerOnce function
which is intended to make sure the power event is only processed in one place. 

Firstly, the GetSignalerOnce still can not prevent the client code from calling
from multiple points. For example,

class ClientCode {
public:
 ClientCode() {
  signaler_ = base::PowerMonitor::GetSignalerOnce();
 }
 void ProcessPowerEvent(...) {
   if (signaler_)
     signaler_->ProcessPowerEvent();
 }
private:
 PowerEventSignaler* signaler_;
};

However, the ClientCode::ProcessPowerEvent can be called from multiple points,
which is not our intentation. Actually, we have an implication that the client
code doesn't expose the ProcessPowerEvent again.

Secondly, On Windows, the power event handling is now moved out of ui/ layer to
the SystemMessageWindow in content/ layer, see the CL
https://codereview.chromium.org/11821050/, it guarantees that the power event is
only processed from the single SystemMessageWindow instance.

My concern, is it necessary to introduce the new GetSignalerOnce function to
pose such an implication to client code? The thing we need to ensure is that
ProcessPowerEvent is called in a single instance, e.g. SystemMessageWindow on
Windows.

Your opinions? Do you agree to remove it now? Thanks.

[Hongbo Min, 2013-01-13 14:19:23]

jam@, according to your suggestion, the updated patch creates a message window
for listening and handling power event.

vandebo@ and willchan@, I have a try to remove PowerEventSignaler and make the
ProcessPowerEvent as private member of PowerMonitor, which should be our target
of SystemMonitor refactoring.

Please have a review. Thanks.

[vandebo (ex-Chrome), 2013-01-15 23:43:48]

On 2013/01/13 14:19:23, Hongbo Min wrote:
> jam@, according to your suggestion, the updated patch creates a message window
> for listening and handling power event.
> 
> vandebo@ and willchan@, I have a try to remove PowerEventSignaler and make the
> ProcessPowerEvent as private member of PowerMonitor, which should be our
target
> of SystemMonitor refactoring.
> 
> Please have a review. Thanks.

Let's see what Carlos says in the other CL.

I don't know if it's appropriate to have a message window in base.  If it is,
then jam's suggest was that it be overrideable. So that probably means taking a
class in the constructor which ends up getting some special privilege, so you'll
probably still need the signaler.

[vandebo (ex-Chrome), 2013-03-15 20:24:39]

I think we still want to do this -- pull the power monitor stuff into it's own,
properly named class, even though it will still all live in base. 
base/system_monitor should go away eventually.

[Hongbo Min, 2013-03-15 20:39:46]

On 2013/03/15 20:24:39, vandebo wrote:
> I think we still want to do this -- pull the power monitor stuff into it's
own,
> properly named class, even though it will still all live in base. 
> base/system_monitor should go away eventually.

It is also my major concern that it will still all live in base including device
monitor. My original plan is

1) Move the device monitor from base/ to content/.
2) Rename SystemMonitor to PowerMonitor and do some code cleanup and refactor.

I think it should be more easier if we do the 1) firstly. However, you know the
step 1 is blocked by the media component which also depends on device monitor.

So do you mean we just leave the device monitor in base/ and refactor it once
the media dependency is resolved?

[vandebo (ex-Chrome), 2013-03-15 20:42:20]

On 2013/03/15 20:39:46, Hongbo Min wrote:
> On 2013/03/15 20:24:39, vandebo wrote:
> > I think we still want to do this -- pull the power monitor stuff into it's
> own,
> > properly named class, even though it will still all live in base. 
> > base/system_monitor should go away eventually.
> 
> It is also my major concern that it will still all live in base including
device
> monitor. My original plan is
> 
> 1) Move the device monitor from base/ to content/.
> 2) Rename SystemMonitor to PowerMonitor and do some code cleanup and refactor.
> 
> I think it should be more easier if we do the 1) firstly. However, you know
the
> step 1 is blocked by the media component which also depends on device monitor.
> 
> So do you mean we just leave the device monitor in base/ and refactor it once
> the media dependency is resolved?

I think you can patch up this CL and move power monitor out of system monitor
and that doesn't need to be blocked on figuring out device monitor.  If it ends
up in base in the end, it can be base/device_monitor

[Hongbo Min, 2013-03-15 20:43:09]

On 2013/03/15 20:39:46, Hongbo Min wrote:
> On 2013/03/15 20:24:39, vandebo wrote:
> > I think we still want to do this -- pull the power monitor stuff into it's
> own,
> > properly named class, even though it will still all live in base. 
> > base/system_monitor should go away eventually.
> 
> It is also my major concern that it will still all live in base including
device
> monitor. My original plan is
> 
> 1) Move the device monitor from base/ to content/.
> 2) Rename SystemMonitor to PowerMonitor and do some code cleanup and refactor.
> 
> I think it should be more easier if we do the 1) firstly. However, you know
the
> step 1 is blocked by the media component which also depends on device monitor.
> 
> So do you mean we just leave the device monitor in base/ and refactor it once
> the media dependency is resolved?

Make sense.

[Hongbo Min, 2013-03-18 08:35:46]

vandeo, the patch is rebased now and merged more fixes into it.

The major changes include:
* Use PowerMonitor instead of SystemMonitor in XXXMain entry function.
  There is a not-so-good pattern in Main entry function. At the beginning of
each Main entry function, you have to put down these 2 statements:

    base::PowerMonitor power_monitor;
    HighResolutionTimerManager hi_res_timer_manager;

  However, it only takes effect on Windows since HighResolutionTimerManager
implementation acts as a PowerObserver which is attached to a 'global'
PowerMonitor instance. Therefore, the PowerMonitor must be defined prior to
HighResolutionTimerManager.

 I think we can move the PowerMonitor definition to HighResolutionTimerManager
ctor if no available 'global' PowerMonitor instance is found, and move
HighResolutionTimerManager definition into content_main_runner.cc.

* Remove GetSignalerOnce now since there is no strong reason to convince me. 

Please help review it again and include the related owner into this CL. You
absolutely know their focus:)

[Hongbo Min, 2013-03-18 09:09:12]

I try to optimize the usage of PowerMonitor and HighResolutionTimerManager in
Patch Set 27. 

Do you think it is suitable to optimize it in this CL if it is a right way to go
down?

[vandebo (ex-Chrome), 2013-03-19 00:38:27]

On 2013/03/18 09:09:12, Hongbo Min wrote:
> I try to optimize the usage of PowerMonitor and HighResolutionTimerManager in
> Patch Set 27. 
> 
> Do you think it is suitable to optimize it in this CL if it is a right way to
go
> down?

Better to do it in a different CL, this one is very big.  I'll try to make it
through the CL tomorrow.

[Hongbo Min, 2013-03-19 00:44:58]

On 2013/03/19 00:38:27, vandebo wrote:
> On 2013/03/18 09:09:12, Hongbo Min wrote:
> > I try to optimize the usage of PowerMonitor and HighResolutionTimerManager
in
> > Patch Set 27. 
> > 
> > Do you think it is suitable to optimize it in this CL if it is a right way
to
> go
> > down?
> 
> Better to do it in a different CL, this one is very big.  I'll try to make it
> through the CL tomorrow.

OK. I removed that Patch Set now.

[vandebo (ex-Chrome), 2013-03-19 23:36:04]

https://codereview.chromium.org/10959020/diff/242003/base/android/base_jni_re...
File base/android/base_jni_registrar.cc (right):

https://codereview.chromium.org/10959020/diff/242003/base/android/base_jni_re...
base/android/base_jni_registrar.cc:32: { "SystemMonitor",
base::RegisterSystemMonitor },
SysMon -> PowMon

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
File base/power_monitor/power_monitor.cc (left):

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
base/power_monitor/power_monitor.cc:11: #include "base/time.h"
Keep base/time.h

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
File base/power_monitor/power_monitor.cc (right):

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
base/power_monitor/power_monitor.cc:88: BatteryPower());
nit: move up to previous line.

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
base/power_monitor/power_monitor.cc:101: void PowerMonitor::BatteryCheck() {
Nit: definition order should match declaration order, move to line 83

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
base/power_monitor/power_monitor.cc:106: 
nit: remove blank line.

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
File base/power_monitor/power_monitor.h (right):

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
base/power_monitor/power_monitor.h:10: #include "base/memory/scoped_ptr.h"
you want base/memory/ref_counted.h for scoped_refptr

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
base/power_monitor/power_monitor.h:16: // Windows HiRes timers drain the battery
faster so we need to know the battery
Nit: add an empty line above comment.

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
File base/power_monitor/power_monitor_android.cc (right):

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
base/power_monitor/power_monitor_android.cc:6: #include
"jni/SystemMonitor_jni.h"
SysMon->PowMon

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
base/power_monitor/power_monitor_android.cc:38: // TODO(hmin): Rename it to
RegisterPowerMonitor.
Do this in this CL.

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
File base/power_monitor/power_monitor_android.h (right):

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
base/power_monitor/power_monitor_android.h:12: // Registers the JNI bindings for
SystemMonitor.
SysMon ->PowMon

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
base/power_monitor/power_monitor_android.h:13: bool
RegisterSystemMonitor(JNIEnv* env);
SysMon ->PowMon

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
File base/power_monitor/power_monitor_mac.mm (left):

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
base/power_monitor/power_monitor_mac.mm:64: if (g_system_power_io_port == 0)
Why did you move this to AllocateSystemIOPorts?  Here it verifies that
AllocateSystemIOPorts was called.

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
File base/power_monitor/power_monitor_mac.mm (right):

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
base/power_monitor/power_monitor_mac.mm:78: 
remove extra line

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
base/power_monitor/power_monitor_mac.mm:81: 
remove extra line

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
File base/power_monitor/power_monitor_win.cc (left):

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
base/power_monitor/power_monitor_win.cc:104:
reinterpret_cast<SystemMonitor::PowerMessageWindow*>(
This was the correct indenting, please undo.

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
File base/power_monitor/power_observer.h (right):

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
base/power_monitor/power_observer.h:9: #include "base/compiler_specific.h"
I don't think you need this.

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
base/power_monitor/power_observer.h:13: // Callbacks will be called on the
thread which creates the PowerMonitor.
This conflicts with the documentation in observer_list_threadsafe.h

https://codereview.chromium.org/10959020/diff/242003/base/system_monitor/syst...
File base/system_monitor/system_monitor.h (right):

https://codereview.chromium.org/10959020/diff/242003/base/system_monitor/syst...
base/system_monitor/system_monitor.h:11: #include "build/build_config.h"
Also add base/memory/ref_counted.h for the scoped_refptr

https://codereview.chromium.org/10959020/diff/242003/chrome/browser/profiles/...
File chrome/browser/profiles/profile_manager_unittest.cc (right):

https://codereview.chromium.org/10959020/diff/242003/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager_unittest.cc:12: #include
"base/power_monitor/power_monitor.h"
Does this need a dummy system monitor, or a dummy power_monitor, or neither?
Please explain.

https://codereview.chromium.org/10959020/diff/242003/net/url_request/url_requ...
File net/url_request/url_request_job.h (right):

https://codereview.chromium.org/10959020/diff/242003/net/url_request/url_requ...
net/url_request/url_request_job.h:42: public base::PowerObserver {
Please fix indent here.  : should start four spaces from the beginning of line
42.

[Hongbo Min, 2013-03-20 13:01:50]

https://codereview.chromium.org/10959020/diff/242003/base/android/base_jni_re...
File base/android/base_jni_registrar.cc (right):

https://codereview.chromium.org/10959020/diff/242003/base/android/base_jni_re...
base/android/base_jni_registrar.cc:32: { "SystemMonitor",
base::RegisterSystemMonitor },
On 2013/03/19 23:36:04, vandebo wrote:
> SysMon -> PowMon

Done.

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
File base/power_monitor/power_monitor.cc (left):

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
base/power_monitor/power_monitor.cc:11: #include "base/time.h"
On 2013/03/19 23:36:04, vandebo wrote:
> Keep base/time.h

Done.

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
File base/power_monitor/power_monitor.cc (right):

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
base/power_monitor/power_monitor.cc:88: BatteryPower());
On 2013/03/19 23:36:04, vandebo wrote:
> nit: move up to previous line.

Done.

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
base/power_monitor/power_monitor.cc:101: void PowerMonitor::BatteryCheck() {
On 2013/03/19 23:36:04, vandebo wrote:
> Nit: definition order should match declaration order, move to line 83

Done.

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
base/power_monitor/power_monitor.cc:106: 
On 2013/03/19 23:36:04, vandebo wrote:
> nit: remove blank line.

Done.

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
File base/power_monitor/power_monitor.h (right):

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
base/power_monitor/power_monitor.h:10: #include "base/memory/scoped_ptr.h"
On 2013/03/19 23:36:04, vandebo wrote:
> you want base/memory/ref_counted.h for scoped_refptr

Added. But I am still a bit confused why to use scoped_refptr for observers_, do
you agree to declare the observers_ directly without scoped_refptr? like:

  ObserverListThreadSafe<PowerObserver> observers_;

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
base/power_monitor/power_monitor.h:16: // Windows HiRes timers drain the battery
faster so we need to know the battery
On 2013/03/19 23:36:04, vandebo wrote:
> Nit: add an empty line above comment.

Done.

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
File base/power_monitor/power_monitor_android.cc (right):

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
base/power_monitor/power_monitor_android.cc:6: #include
"jni/SystemMonitor_jni.h"
On 2013/03/19 23:36:04, vandebo wrote:
> SysMon->PowMon

Done.

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
base/power_monitor/power_monitor_android.cc:38: // TODO(hmin): Rename it to
RegisterPowerMonitor.
On 2013/03/19 23:36:04, vandebo wrote:
> Do this in this CL.

Done.

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
File base/power_monitor/power_monitor_android.h (right):

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
base/power_monitor/power_monitor_android.h:12: // Registers the JNI bindings for
SystemMonitor.
On 2013/03/19 23:36:04, vandebo wrote:
> SysMon ->PowMon

Done.

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
base/power_monitor/power_monitor_android.h:13: bool
RegisterSystemMonitor(JNIEnv* env);
On 2013/03/19 23:36:04, vandebo wrote:
> SysMon ->PowMon

Done.

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
File base/power_monitor/power_monitor_mac.mm (left):

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
base/power_monitor/power_monitor_mac.mm:64: if (g_system_power_io_port == 0)
On 2013/03/19 23:36:04, vandebo wrote:
> Why did you move this to AllocateSystemIOPorts?  Here it verifies that
> AllocateSystemIOPorts was called.

Recover it now.

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
File base/power_monitor/power_monitor_mac.mm (right):

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
base/power_monitor/power_monitor_mac.mm:78: 
On 2013/03/19 23:36:04, vandebo wrote:
> remove extra line

Done.

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
base/power_monitor/power_monitor_mac.mm:81: 
On 2013/03/19 23:36:04, vandebo wrote:
> remove extra line

Done.

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
File base/power_monitor/power_observer.h (right):

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
base/power_monitor/power_observer.h:9: #include "base/compiler_specific.h"
On 2013/03/19 23:36:04, vandebo wrote:
> I don't think you need this.

It is needed since OVERRIDE directive should be used in any implementations of
PowerObserver.

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
base/power_monitor/power_observer.h:13: // Callbacks will be called on the
thread which creates the PowerMonitor.
On 2013/03/19 23:36:04, vandebo wrote:
> This conflicts with the documentation in observer_list_threadsafe.h

Removed the comments.

https://codereview.chromium.org/10959020/diff/242003/base/system_monitor/syst...
File base/system_monitor/system_monitor.h (right):

https://codereview.chromium.org/10959020/diff/242003/base/system_monitor/syst...
base/system_monitor/system_monitor.h:11: #include "build/build_config.h"
On 2013/03/19 23:36:04, vandebo wrote:
> Also add base/memory/ref_counted.h for the scoped_refptr

Added, but have the same concern about scoped_refptr usage.

https://codereview.chromium.org/10959020/diff/242003/chrome/browser/profiles/...
File chrome/browser/profiles/profile_manager_unittest.cc (right):

https://codereview.chromium.org/10959020/diff/242003/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager_unittest.cc:12: #include
"base/power_monitor/power_monitor.h"
On 2013/03/19 23:36:04, vandebo wrote:
> Does this need a dummy system monitor, or a dummy power_monitor, or neither?
> Please explain.

I found neither. I really doubt the purpose of the previous code. According to
the trybots, it won't break any tests if removing that code.

https://codereview.chromium.org/10959020/diff/242003/net/url_request/url_requ...
File net/url_request/url_request_job.h (right):

https://codereview.chromium.org/10959020/diff/242003/net/url_request/url_requ...
net/url_request/url_request_job.h:42: public base::PowerObserver {
On 2013/03/19 23:36:04, vandebo wrote:
> Please fix indent here.  : should start four spaces from the beginning of line
> 42.

Done.

[vandebo (ex-Chrome), 2013-03-20 20:36:25]

LGTM with two nits.

willchan: are you able to do a final review, or should I get a dinner base
OWNER?

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
File base/power_monitor/power_monitor.h (right):

https://codereview.chromium.org/10959020/diff/242003/base/power_monitor/power...
base/power_monitor/power_monitor.h:10: #include "base/memory/scoped_ptr.h"
On 2013/03/20 13:01:50, Hongbo Min wrote:
> On 2013/03/19 23:36:04, vandebo wrote:
> > you want base/memory/ref_counted.h for scoped_refptr
> 
> Added. But I am still a bit confused why to use scoped_refptr for observers_,
do
> you agree to declare the observers_ directly without scoped_refptr? like:
> 
>   ObserverListThreadSafe<PowerObserver> observers_;

I think it is kept in a scoped_refptr to avoid including the header for it in
the power_monitor.h header.  I think it would be ok with way, so lets just leave
it this way.

https://codereview.chromium.org/10959020/diff/242003/net/url_request/url_requ...
File net/url_request/url_request_job.h (right):

https://codereview.chromium.org/10959020/diff/242003/net/url_request/url_requ...
net/url_request/url_request_job.h:42: public base::PowerObserver {
On 2013/03/20 13:01:50, Hongbo Min wrote:
> On 2013/03/19 23:36:04, vandebo wrote:
> > Please fix indent here.  : should start four spaces from the beginning of
line
> > 42.
> 
> Done.

Should be:
class NET_EXPORT URLRequestJob 
    : public base::RefCounted<URLRequestJob>,
      public base::PowerObserver {

https://codereview.chromium.org/10959020/diff/299007/base/power_monitor/power...
File base/power_monitor/power_monitor.h (right):

https://codereview.chromium.org/10959020/diff/299007/base/power_monitor/power...
base/power_monitor/power_monitor.h:11: #include "base/memory/scoped_ptr.h"
nit: You don't need scoped_ptr.h

[vandebo (ex-Chrome), 2013-03-20 22:55:20]

Removing willchan (away) and Ben (no longer touches ui/views)

+brett.  Can you please do an OWNERS review for base/

[vandebo (ex-Chrome), 2013-03-20 22:57:22]

Removing willchan (away) and Ben (no longer touches ui/views)

+brett.  Can you please do an OWNERS review for base/ and webkit/
+nilesh. Can you please do an OWNERS review for testing/android

[nilesh, 2013-03-20 23:08:53]

On 2013/03/20 22:57:22, vandebo wrote:
> Removing willchan (away) and Ben (no longer touches ui/views)
> 
> +brett.  Can you please do an OWNERS review for base/ and webkit/
> +nilesh. Can you please do an OWNERS review for testing/android
LGTM for testing/android

[Hongbo Min, 2013-03-21 23:27:00]

ping brett

[Hongbo Min, 2013-03-26 01:12:48]

brettw@, do you have a time to review this CL? Thanks!

[brettw, 2013-04-01 22:52:06]

LGTM, thanks for putting up with the latency.

[commit-bot: I haz the power, 2013-04-02 01:33:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hongbo.min@intel.com/10959020/341001

[commit-bot: I haz the power, 2013-04-02 02:45:28]

Retried try job too often on android_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-04-02 05:21:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hongbo.min@intel.com/10959020/341001

[commit-bot: I haz the power, 2013-04-02 06:29:41]

Retried try job too often on android_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-04-02 06:54:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hongbo.min@intel.com/10959020/362001

[commit-bot: I haz the power, 2013-04-02 06:54:58]

Presubmit check for 10959020-362001 failed and returned exit status 1.

INFO:root:Found 41 file(s).

INFO:PRESUBMIT:Skipping pylint: no matching changes.
INFO:PRESUBMIT:Skipping pylint: no matching changes.
Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/base/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/build/android/PRESUBMIT.py
Running
/b/commit-queue/workdir/chromium/build/android/buildbot/tests/bb_run_bot_test.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/extensions/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/net/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/testing/PRESUBMIT.py

** Presubmit Warnings **
Following files should only contain deletions.
  build/android/findbugs_filter/findbugs_known_bugs.txt

Presubmit checks took 8.6s to calculate.

[Hongbo Min, 2013-04-02 06:59:54]

On 2013/04/02 06:54:58, I haz the power (commit-bot) wrote:
> Presubmit check for 10959020-362001 failed and returned exit status 1.
> 
> INFO:root:Found 41 file(s).
> 
> INFO:PRESUBMIT:Skipping pylint: no matching changes.
> INFO:PRESUBMIT:Skipping pylint: no matching changes.
> Running presubmit commit checks ...
> Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
> Running /b/commit-queue/workdir/chromium/base/PRESUBMIT.py
> Running /b/commit-queue/workdir/chromium/build/android/PRESUBMIT.py
> Running
>
/b/commit-queue/workdir/chromium/build/android/buildbot/tests/bb_run_bot_test.py
> Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
> Running
/b/commit-queue/workdir/chromium/chrome/browser/extensions/PRESUBMIT.py
> Running /b/commit-queue/workdir/chromium/net/PRESUBMIT.py
> Running /b/commit-queue/workdir/chromium/testing/PRESUBMIT.py
> 
> ** Presubmit Warnings **
> Following files should only contain deletions.
>   build/android/findbugs_filter/findbugs_known_bugs.txt
> 
> Presubmit checks took 8.6s to calculate.

vandebo@, any suggestion on this issue?

Since the SystemMonitor is renamed to PowerMonitor, and so the
findbugs_known_bugs.txt should be changed accordingly, otherwise android_dbg
will always fail. But the presubmit check only allows deletions in
findbugs_known_bugs.txt file.

[vandebo (ex-Chrome), 2013-04-02 18:26:52]

On 2013/04/02 06:59:54, Hongbo Min wrote:
> On 2013/04/02 06:54:58, I haz the power (commit-bot) wrote:
> > Presubmit check for 10959020-362001 failed and returned exit status 1.
> > 
> > INFO:root:Found 41 file(s).
> > 
> > INFO:PRESUBMIT:Skipping pylint: no matching changes.
> > INFO:PRESUBMIT:Skipping pylint: no matching changes.
> > Running presubmit commit checks ...
> > Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
> > Running /b/commit-queue/workdir/chromium/base/PRESUBMIT.py
> > Running /b/commit-queue/workdir/chromium/build/android/PRESUBMIT.py
> > Running
> >
>
/b/commit-queue/workdir/chromium/build/android/buildbot/tests/bb_run_bot_test.py
> > Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
> > Running
> /b/commit-queue/workdir/chromium/chrome/browser/extensions/PRESUBMIT.py
> > Running /b/commit-queue/workdir/chromium/net/PRESUBMIT.py
> > Running /b/commit-queue/workdir/chromium/testing/PRESUBMIT.py
> > 
> > ** Presubmit Warnings **
> > Following files should only contain deletions.
> >   build/android/findbugs_filter/findbugs_known_bugs.txt
> > 
> > Presubmit checks took 8.6s to calculate.
> 
> vandebo@, any suggestion on this issue?
> 
> Since the SystemMonitor is renamed to PowerMonitor, and so the
> findbugs_known_bugs.txt should be changed accordingly, otherwise android_dbg
> will always fail. But the presubmit check only allows deletions in
> findbugs_known_bugs.txt file.

+michaelbai  What's the right thing to do here, is it ok to bypass this
presubmit check in this case?

[michaelbai, 2013-04-02 18:45:50]

Yes, you could bypass this one.

[michaelbai, 2013-04-02 20:41:38]

It needs to change presubmit script to bypass, I think you'd better to fix the
findbug issue in your change, it should be easy by apply the below changes to
SystemMonitor.java


diff --git a/base/android/java/src/org/chromium/base/SystemMonitor.java b/base/
index 138ae28..e95d4b6 100644
--- a/base/android/java/src/org/chromium/base/SystemMonitor.java
+++ b/base/android/java/src/org/chromium/base/SystemMonitor.java
@@ -19,6 +19,7 @@ import android.os.Looper;
 public class SystemMonitor implements ActivityStatus.StateListener {
     private static final long SUSPEND_DELAY_MS = 1 * 60 * 1000;  // 1 minute.
     private static SystemMonitor sInstance;
+    private static Object sInstanceLock = new Object();
 
     private boolean mIsBatteryPower;
     private final Handler mHandler = new Handler(Looper.getMainLooper());
@@ -42,12 +43,14 @@ public class SystemMonitor implements ActivityStatus.StateL
     }
 
     public static void create(Context context) {
-        if (sInstance == null) {
-            sInstance = new SystemMonitor();
-            ActivityStatus.registerStateListener(sInstance);
-            IntentFilter ifilter = new IntentFilter(Intent.ACTION_BATTERY_CHAN
-            Intent batteryStatusIntent = context.registerReceiver(null, ifilte
-            onBatteryChargingChanged(batteryStatusIntent);
+        synchronized(sInstanceLock) {
+            if (sInstance == null) {
+                sInstance = new SystemMonitor();
+                ActivityStatus.registerStateListener(sInstance);
+                IntentFilter ifilter = new IntentFilter(Intent.ACTION_BATTERY_
+                Intent batteryStatusIntent = context.registerReceiver(null, if
+                onBatteryChargingChanged(batteryStatusIntent);
+            }
         }
     }

[nilesh, 2013-04-02 21:24:24]

Lazy initialization using holder class is preferred over explicit
synchronization.  For eg.
@@ -18,6 +18,9 @@ import android.os.Looper;
 @JNINamespace("base::android")
 public class SystemMonitor implements ActivityStatus.StateListener {
     private static final long SUSPEND_DELAY_MS = 1 * 60 * 1000;  // 1 minute.
+    private static class LazyHolder {
+        private static final SystemMonitor INSTANCE = new SystemMonitor();
+    }
     private static SystemMonitor sInstance;
 
     private boolean mIsBatteryPower;
@@ -38,12 +41,12 @@ public class SystemMonitor implements
ActivityStatus.StateListener {
         // Applications will create this once the JNI side has been fully wired
up both sides. For
         // tests, we just need native -> java, that is, we don't need to notify
java -> native on
         // creation.
-        sInstance = new SystemMonitor();
+        sInstance = LazyHolder.INSTANCE;
     }
 
     public static void create(Context context) {
         if (sInstance == null) {
-            sInstance = new SystemMonitor();
+            sInstance = LazyHolder.INSTANCE;;
             ActivityStatus.registerStateListener(sInstance);
             IntentFilter ifilter = new
IntentFilter(Intent.ACTION_BATTERY_CHANGED);
             Intent batteryStatusIntent = context.registerReceiver(null,
ifilter);



On 2013/04/02 20:41:38, michaelbai wrote:
> It needs to change presubmit script to bypass, I think you'd better to fix the
> findbug issue in your change, it should be easy by apply the below changes to
> SystemMonitor.java
> 
> 
> diff --git a/base/android/java/src/org/chromium/base/SystemMonitor.java
b/base/
> index 138ae28..e95d4b6 100644
> --- a/base/android/java/src/org/chromium/base/SystemMonitor.java
> +++ b/base/android/java/src/org/chromium/base/SystemMonitor.java
> @@ -19,6 +19,7 @@ import android.os.Looper;
>  public class SystemMonitor implements ActivityStatus.StateListener {
>      private static final long SUSPEND_DELAY_MS = 1 * 60 * 1000;  // 1 minute.
>      private static SystemMonitor sInstance;
> +    private static Object sInstanceLock = new Object();
>  
>      private boolean mIsBatteryPower;
>      private final Handler mHandler = new Handler(Looper.getMainLooper());
> @@ -42,12 +43,14 @@ public class SystemMonitor implements
ActivityStatus.StateL
>      }
>  
>      public static void create(Context context) {
> -        if (sInstance == null) {
> -            sInstance = new SystemMonitor();
> -            ActivityStatus.registerStateListener(sInstance);
> -            IntentFilter ifilter = new
IntentFilter(Intent.ACTION_BATTERY_CHAN
> -            Intent batteryStatusIntent = context.registerReceiver(null,
ifilte
> -            onBatteryChargingChanged(batteryStatusIntent);
> +        synchronized(sInstanceLock) {
> +            if (sInstance == null) {
> +                sInstance = new SystemMonitor();
> +                ActivityStatus.registerStateListener(sInstance);
> +                IntentFilter ifilter = new
IntentFilter(Intent.ACTION_BATTERY_
> +                Intent batteryStatusIntent = context.registerReceiver(null,
if
> +                onBatteryChargingChanged(batteryStatusIntent);
> +            }
>          }
>      }

[Hongbo Min, 2013-04-03 13:59:52]

nilesh@, pls help review the PowerMonitor.java changes with lazy holder usage.

If it looks good to you, then pls help check the commit box since I am in a
different time zone. Thanks

[nilesh, 2013-04-03 15:27:16]

On 2013/04/03 13:59:52, Hongbo Min wrote:
> nilesh@, pls help review the PowerMonitor.java changes with lazy holder usage.
> 
> If it looks good to you, then pls help check the commit box since I am in a
> different time zone. Thanks

LGTM

[commit-bot: I haz the power, 2013-04-03 15:27:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hongbo.min@intel.com/10959020/375001

[commit-bot: I haz the power, 2013-04-03 19:06:57]

Change committed as 192114