Created multi-process-friendly PowerMonitor interface.

Created multi-process-friendly PowerMonitor interface. 

PowerMonitor status is now captured in the browser process, which has the
appropriate UI thread, and then sent via IPC to other processes which are
interested in the power state.

BUG=236031
R=apatrick@chromium.org, jam@chromium.org, jar@chromium.org, jvoung@chromium.org, kbr@chromium.org, mpcomplete@chromium.org, palmer@chromium.org, piman@chromium.org, vandebo@chromium.org

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

[bajones, 2013-06-21 23:11:45]

Fair warning: This is my first CL defining a new cross-process interface, and as
such I'm entirely certain that I've botched the naming conventions, file layout,
class hierarchy, or something equally silly. Please don't hesitate to point out
anything that feels a little off.

The basic structure is:

A PowerMonitorService is created for each interested process which is registered
as a PowerObserver on the browser's PowerMonitor (which is unchanged.) The
PowerMonitorServices sends the IPC messages, which are dispatched by the
PowerMonitorMessageFilter to a per-process PowerMonitorClient, which is
functionally identical to the PowerMonitor, but functions in any process. Any
code that wants to be notified of power events registers a PowerObserver with
the PowerMonitorClient.

[scottmg, 2013-06-22 00:11:24]

i'm probably not the best reviewer, but seems reasonable to me.

https://codereview.chromium.org/17074009/diff/1/content/common/power_monitor_...
File content/common/power_monitor_client.h (right):

https://codereview.chromium.org/17074009/diff/1/content/common/power_monitor_...
content/common/power_monitor_client.h:34: // Using a lock here is not necessary
for just a bool.
tsan will not agree

https://codereview.chromium.org/17074009/diff/1/content/common/power_monitor_...
File content/common/power_monitor_service.cc (right):

https://codereview.chromium.org/17074009/diff/1/content/common/power_monitor_...
content/common/power_monitor_service.cc:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
it seems like this file should be in content/browser, and the gpu-side (or
eventually, child) parts should live in content/child. is there some reason why
that can't work? (the messages can stay in common).

[bajones, 2013-06-23 20:56:26]

On 2013/06/22 00:11:24, scottmg wrote:
> i'm probably not the best reviewer, but seems reasonable to me.

I'm not sure would be the best reviewer either, unfortunately. I added you
because you filed the original ticket. If you do know of a better reviewer I'm
happy to hear it!
 
>
https://codereview.chromium.org/17074009/diff/1/content/common/power_monitor_...
> File content/common/power_monitor_client.h (right):
> 
>
https://codereview.chromium.org/17074009/diff/1/content/common/power_monitor_...
> content/common/power_monitor_client.h:34: // Using a lock here is not
necessary
> for just a bool.
> tsan will not agree

I'm happy to drop this property. This particular code was copied directly from
the original PowerMonitor, but I don't see this property as being terribly
useful (not to mention at least on OSX the battery status doesn't seem to work
anyway.)
 
>
https://codereview.chromium.org/17074009/diff/1/content/common/power_monitor_...
> File content/common/power_monitor_service.cc (right):
> 
>
https://codereview.chromium.org/17074009/diff/1/content/common/power_monitor_...
> content/common/power_monitor_service.cc:1: // Copyright 2013 The Chromium
> Authors. All rights reserved.
> it seems like this file should be in content/browser, and the gpu-side (or
> eventually, child) parts should live in content/child. is there some reason
why
> that can't work? (the messages can stay in common).

This sounds a lot more reasonable to me, but produces some layering violations.
PowerMonitorService needs to be accessed by
content/common/gpu/client/gpu_channel_host.cc, and PowerMonitorMessageFilter
(and with it the PowerMonitorClient) are accessed from
content/common/gpu/gpu_channel.cc. Open to suggestions on how to work around
these requirements.

[wrong vandebo, 2013-06-24 16:34:49]

https://codereview.chromium.org/17074009/diff/1/content/common/power_monitor_...
File content/common/power_monitor_client.h (right):

https://codereview.chromium.org/17074009/diff/1/content/common/power_monitor_...
content/common/power_monitor_client.h:18: class CONTENT_EXPORT
PowerMonitorClient {
So this class looks basically the same as base::SysteMonitor.  Why not just
reuse/extend that? One way would be to change base::SystemMonitor to have a
constructor that takes a SystemMonitorSource thing and then
PowerMonitorMessageFilter can be an implementation of that.

[Ken Russell (switch to Gerrit), 2013-06-24 22:54:38]

A good start.

https://codereview.chromium.org/17074009/diff/1/content/common/gpu/client/gpu...
File content/common/gpu/client/gpu_channel_host.cc (right):

https://codereview.chromium.org/17074009/diff/1/content/common/gpu/client/gpu...
content/common/gpu/client/gpu_channel_host.cc:63:
power_monitor->AddObserver(power_monitor_service_.get());
Unless I'm misremembering and misreading the code, GpuChannelHosts are
instantiated both in the browser process and renderer processes. See
RenderThreadImpl::EstablishGpuChannelSync and
BrowserGpuChannelHostFactory::EstablishGpuChannelSync . Did you run into this
problem and/or expect this? I don't think the renderer process should attempt to
instantiate any power monitor-related objects. If the code deliberately runs
differently in the browser and renderer processes then big comments should be
added to the power_monitor_service_ field in the header pointing out that it's
only used in the browser, and the PowerMontiorService shouldn't even be created
if base::PowerMonitor::Get() returns NULL.

https://codereview.chromium.org/17074009/diff/1/content/common/power_monitor_...
File content/common/power_monitor_client.cc (right):

https://codereview.chromium.org/17074009/diff/1/content/common/power_monitor_...
content/common/power_monitor_client.cc:9: static PowerMonitorClient*
g_power_monitor_client = NULL;
This and PowerMonitorClient::Get() should use base::LazyInstance.

https://codereview.chromium.org/17074009/diff/1/content/common/power_monitor_...
File content/common/power_monitor_client.h (right):

https://codereview.chromium.org/17074009/diff/1/content/common/power_monitor_...
content/common/power_monitor_client.h:20: PowerMonitorClient();
Since this class is a singleton the constructor and destructor should be
private, and the sole instance only available via PowerMonitorClient::Get().

https://codereview.chromium.org/17074009/diff/1/content/common/power_monitor_...
File content/common/power_monitor_message_filter.h (right):

https://codereview.chromium.org/17074009/diff/1/content/common/power_monitor_...
content/common/power_monitor_message_filter.h:17: PowerMonitorMessageFilter();
It seems confusing to me that the PowerMonitorMessageFilter retrieves the
singleton PowerMonitorClient itself, and callers also fetch the singleton
PowerMonitorClient and add themselves as observers to it. It would be more
understandable in my opinion if the PowerMonitorClient were passed in to the
constructor by the caller.

https://codereview.chromium.org/17074009/diff/1/content/common/power_monitor_...
File content/common/power_monitor_messages.h (right):

https://codereview.chromium.org/17074009/diff/1/content/common/power_monitor_...
content/common/power_monitor_messages.h:18: bool /* on_battery_power */)
Indentation?

https://codereview.chromium.org/17074009/diff/1/content/common/power_monitor_...
File content/common/power_monitor_service.h (right):

https://codereview.chromium.org/17074009/diff/1/content/common/power_monitor_...
content/common/power_monitor_service.h:21: // the change event.
Isn't this documentation inaccurate? This class monitors power state changes and
sends messages through the given Sender about them. I think this class could be
named something more obvious like PowerMonitorMessageSender,
PowerMonitorMessageBroadcaster, etc. Unless there is precedent for this kind of
naming convention?

https://codereview.chromium.org/17074009/diff/1/content/gpu/gpu_main.cc
File content/gpu/gpu_main.cc (right):

https://codereview.chromium.org/17074009/diff/1/content/gpu/gpu_main.cc#newco...
content/gpu/gpu_main.cc:173: PowerMonitorClient power_monitor;
I don't think this function should stack-allocate a PowerMonitorClient instance.
If necessary, the PowerMonitorClient can be fetched and initialized here for use
elsewhere, but the PowerMonitorClient singleton should manage its own lifetime.
(If necessary, a Shutdown() method can be added to PowerMonitorClient to eagerly
terminate it and delete the singleton.)

[bajones, 2013-06-25 18:33:34]

I'm in the middle of refactoring this code to be more inline with what vandebo
said: I'm changing PowerMonitor to take in a PowerMonitorSource, and on the
child process side that source will be the message filter.

https://codereview.chromium.org/17074009/diff/1/content/common/power_monitor_...
File content/common/power_monitor_client.h (right):

https://codereview.chromium.org/17074009/diff/1/content/common/power_monitor_...
content/common/power_monitor_client.h:18: class CONTENT_EXPORT
PowerMonitorClient {
On 2013/06/24 16:34:49, wrong vandebo wrote:
> So this class looks basically the same as base::SysteMonitor.  Why not just
> reuse/extend that? One way would be to change base::SystemMonitor to have a
> constructor that takes a SystemMonitorSource thing and then
> PowerMonitorMessageFilter can be an implementation of that.

I assume you mean base::PowerMonitor, not base::SystemMonitor (which doesn't
look related)? If so then yes, that sounds like a great idea!

https://codereview.chromium.org/17074009/diff/1/content/common/power_monitor_...
content/common/power_monitor_client.h:20: PowerMonitorClient();
On 2013/06/24 22:54:38, Ken Russell wrote:
> Since this class is a singleton the constructor and destructor should be
> private, and the sole instance only available via PowerMonitorClient::Get().

That would be my thinking as well, but that's not how the existing PowerMonitor
(which this is modeled after) instances work.

https://codereview.chromium.org/17074009/diff/1/content/common/power_monitor_...
File content/common/power_monitor_message_filter.h (right):

https://codereview.chromium.org/17074009/diff/1/content/common/power_monitor_...
content/common/power_monitor_message_filter.h:17: PowerMonitorMessageFilter();
On 2013/06/24 22:54:38, Ken Russell wrote:
> It seems confusing to me that the PowerMonitorMessageFilter retrieves the
> singleton PowerMonitorClient itself, and callers also fetch the singleton
> PowerMonitorClient and add themselves as observers to it. It would be more
> understandable in my opinion if the PowerMonitorClient were passed in to the
> constructor by the caller.

I agree. This entire class is being removed, however, so it's a moot point.

https://codereview.chromium.org/17074009/diff/1/content/common/power_monitor_...
File content/common/power_monitor_service.h (right):

https://codereview.chromium.org/17074009/diff/1/content/common/power_monitor_...
content/common/power_monitor_service.h:21: // the change event.
On 2013/06/24 22:54:38, Ken Russell wrote:
> Isn't this documentation inaccurate? This class monitors power state changes
and
> sends messages through the given Sender about them. I think this class could
be
> named something more obvious like PowerMonitorMessageSender,
> PowerMonitorMessageBroadcaster, etc. Unless there is precedent for this kind
of
> naming convention?

No precedent aside from my still-fuzzy understanding of how existing
IPC-spanning classes work (Like I said in the first comment, I'm sure I'm
breaking convention). I like your names better, and I think I'll go with
PowerMonitorMessageBroadcaster.

https://codereview.chromium.org/17074009/diff/1/content/gpu/gpu_main.cc
File content/gpu/gpu_main.cc (right):

https://codereview.chromium.org/17074009/diff/1/content/gpu/gpu_main.cc#newco...
content/gpu/gpu_main.cc:173: PowerMonitorClient power_monitor;
On 2013/06/24 22:54:38, Ken Russell wrote:
> I don't think this function should stack-allocate a PowerMonitorClient
instance.
> If necessary, the PowerMonitorClient can be fetched and initialized here for
use
> elsewhere, but the PowerMonitorClient singleton should manage its own
lifetime.
> (If necessary, a Shutdown() method can be added to PowerMonitorClient to
eagerly
> terminate it and delete the singleton.)

While my gut reaction is to agree with you, this is actually a well established
pattern for use of the PowerMonitor. (See
https://code.google.com/p/chromium/codesearch#search/&q=base::PowerMonitor%20...).

[vandebo (ex-Chrome), 2013-06-25 18:37:14]

https://codereview.chromium.org/17074009/diff/1/content/common/power_monitor_...
File content/common/power_monitor_client.h (right):

https://codereview.chromium.org/17074009/diff/1/content/common/power_monitor_...
content/common/power_monitor_client.h:18: class CONTENT_EXPORT
PowerMonitorClient {
On 2013/06/25 18:33:34, bajones wrote:
> On 2013/06/24 16:34:49, wrong vandebo wrote:
> > So this class looks basically the same as base::SysteMonitor.  Why not just
> > reuse/extend that? One way would be to change base::SystemMonitor to have a
> > constructor that takes a SystemMonitorSource thing and then
> > PowerMonitorMessageFilter can be an implementation of that.
> 
> I assume you mean base::PowerMonitor, not base::SystemMonitor (which doesn't
> look related)? If so then yes, that sounds like a great idea!

Oops, yes, base::PowerMonitor  (PowerMonitor was extract from SystemMonitor
fairly recently.)

[bajones, 2013-06-26 18:36:54]

On 2013/06/25 18:37:14, vandebo wrote:
>
https://codereview.chromium.org/17074009/diff/1/content/common/power_monitor_...
> File content/common/power_monitor_client.h (right):
> 
>
https://codereview.chromium.org/17074009/diff/1/content/common/power_monitor_...
> content/common/power_monitor_client.h:18: class CONTENT_EXPORT
> PowerMonitorClient {
> On 2013/06/25 18:33:34, bajones wrote:
> > On 2013/06/24 16:34:49, wrong vandebo wrote:
> > > So this class looks basically the same as base::SysteMonitor.  Why not
just
> > > reuse/extend that? One way would be to change base::SystemMonitor to have
a
> > > constructor that takes a SystemMonitorSource thing and then
> > > PowerMonitorMessageFilter can be an implementation of that.
> > 
> > I assume you mean base::PowerMonitor, not base::SystemMonitor (which doesn't
> > look related)? If so then yes, that sounds like a great idea!
> 
> Oops, yes, base::PowerMonitor  (PowerMonitor was extract from SystemMonitor
> fairly recently.)

Large refactor uploaded. This version breaks things up a bit to make
base::PowerMonitor usable from both the browser and child processes by providing
different PowerMonitorSources on construction. The default constructor creates a
PowerMonitorDeviceSource, which gives it the same behavior as before, while
child processes can create one with the PowerMonitorBroadcastSource which
listens for state changes via IPC messages). On the browser side a
PowerMonitorMessageBroadcaster is created with the desired channel and added as
an observer to the "main" PowerMonitor.

I like the usage of this version more than the previous CL revision, even though
it's complicated the internals a bit. Suggestions for clarifying or further
simplifying the usage are welcome!

[vandebo (ex-Chrome), 2013-06-26 21:40:33]

I like this direction.  It'll make doing the same thing for renderer processes
nearly trivial.

https://codereview.chromium.org/17074009/diff/17001/base/power_monitor/power_...
File base/power_monitor/power_monitor_android.cc (right):

https://codereview.chromium.org/17074009/diff/17001/base/power_monitor/power_...
base/power_monitor/power_monitor_android.cc:11: // A helper function which is a
friend of PowerMonitor.
nit: ...friend of PowerMonitorSource.

https://codereview.chromium.org/17074009/diff/17001/base/power_monitor/power_...
File base/power_monitor/power_monitor_source.h (right):

https://codereview.chromium.org/17074009/diff/17001/base/power_monitor/power_...
base/power_monitor/power_monitor_source.h:23: // Normalized list of power
events.
nit: comment indent; and below.

https://codereview.chromium.org/17074009/diff/17001/base/power_monitor/power_...
base/power_monitor/power_monitor_source.h:36: void ProcessPowerEvent(PowerEvent
event_id);
Since PowerMonitor::Source() is public, this has to be private or protected.

https://codereview.chromium.org/17074009/diff/17001/base/power_monitor/power_...
base/power_monitor/power_monitor_source.h:41: void SetMonitor(PowerMonitor*
monitor);
Do you need this method, or can it just use PowerMonitor::Get() ?

https://codereview.chromium.org/17074009/diff/17001/content/common/gpu/client...
File content/common/gpu/client/gpu_channel_host.h (right):

https://codereview.chromium.org/17074009/diff/17001/content/common/gpu/client...
content/common/gpu/client/gpu_channel_host.h:252:
scoped_ptr<PowerMonitorMessageBroadcaster> power_monitor_broadcaster_;
direct member?

https://codereview.chromium.org/17074009/diff/17001/content/common/gpu/gpu_ch...
File content/common/gpu/gpu_channel.cc (right):

https://codereview.chromium.org/17074009/diff/17001/content/common/gpu/gpu_ch...
content/common/gpu/gpu_channel.cc:493:
static_cast<PowerMonitorBroadcastSource*>(
Is it easy to get at the original source instead of having to do this cast?

https://codereview.chromium.org/17074009/diff/17001/content/common/gpu/gpu_ch...
content/common/gpu/gpu_channel.cc:495:
channel_->AddFilter(power_monitor_source_->MessageFilter());
Is there ever a RemoveFilter call?  The concern is that it may be possible for
MessageFilter() to continue receiving messages after power_monitor_source has
been cleaned up/destroyed.

https://codereview.chromium.org/17074009/diff/17001/content/common/power_moni...
File content/common/power_monitor_message_broadcaster.h (right):

https://codereview.chromium.org/17074009/diff/17001/content/common/power_moni...
content/common/power_monitor_message_broadcaster.h:20: // A class used to
monitor the power state change and notify the observers about
nit: update comment re ...notify the observers...

https://codereview.chromium.org/17074009/diff/17001/content/common/power_moni...
File content/common/power_monitor_message_filter.cc (right):

https://codereview.chromium.org/17074009/diff/17001/content/common/power_moni...
content/common/power_monitor_message_filter.cc:13: message_filter_ = new
PowerMessageFilter(this);
memory leak

https://codereview.chromium.org/17074009/diff/17001/content/common/power_moni...
File content/common/power_monitor_message_filter.h (right):

https://codereview.chromium.org/17074009/diff/17001/content/common/power_moni...
content/common/power_monitor_message_filter.h:52: PowerMessageFilter*
message_filter_;
scoped_ptr, or just direct member?

[bajones, 2013-06-26 22:37:21]

Thanks for the review! Changes incoming, but a few comments/questions in the
meantime:

https://codereview.chromium.org/17074009/diff/17001/base/power_monitor/power_...
File base/power_monitor/power_monitor_source.h (right):

https://codereview.chromium.org/17074009/diff/17001/base/power_monitor/power_...
base/power_monitor/power_monitor_source.h:36: void ProcessPowerEvent(PowerEvent
event_id);
On 2013/06/26 21:40:33, vandebo wrote:
> Since PowerMonitor::Source() is public, this has to be private or protected.

I'm happy to do this, but I'd like to understand why a bit better. This sounds
like layering policy that I'm just not familiar with yet.

https://codereview.chromium.org/17074009/diff/17001/content/common/gpu/client...
File content/common/gpu/client/gpu_channel_host.h (right):

https://codereview.chromium.org/17074009/diff/17001/content/common/gpu/client...
content/common/gpu/client/gpu_channel_host.h:252:
scoped_ptr<PowerMonitorMessageBroadcaster> power_monitor_broadcaster_;
On 2013/06/26 21:40:33, vandebo wrote:
> direct member?

This class needs to be provided with the Sender*. I could do that as a separate
call, but I felt like during the constructor was a bit more natural.

https://codereview.chromium.org/17074009/diff/17001/content/common/power_moni...
File content/common/power_monitor_message_filter.cc (right):

https://codereview.chromium.org/17074009/diff/17001/content/common/power_moni...
content/common/power_monitor_message_filter.cc:13: message_filter_ = new
PowerMessageFilter(this);
On 2013/06/26 21:40:33, vandebo wrote:
> memory leak

This is tricky. This filter will be passed to the GpuChannel's
channel_->AddFilter() method, which appears to take ownership of the filter.
Agreed that this is an awkward place to construct it, though. Probably be better
to add a MessageFilter factory method to this class, now that I think about it.

https://codereview.chromium.org/17074009/diff/17001/content/common/power_moni...
File content/common/power_monitor_message_filter.h (right):

https://codereview.chromium.org/17074009/diff/17001/content/common/power_moni...
content/common/power_monitor_message_filter.h:52: PowerMessageFilter*
message_filter_;
On 2013/06/26 21:40:33, vandebo wrote:
> scoped_ptr, or just direct member?

See comments re: AddFilter()

[vandebo (ex-Chrome), 2013-06-26 22:50:12]

https://codereview.chromium.org/17074009/diff/17001/base/power_monitor/power_...
File base/power_monitor/power_monitor_source.h (right):

https://codereview.chromium.org/17074009/diff/17001/base/power_monitor/power_...
base/power_monitor/power_monitor_source.h:36: void ProcessPowerEvent(PowerEvent
event_id);
On 2013/06/26 22:37:22, bajones wrote:
> On 2013/06/26 21:40:33, vandebo wrote:
> > Since PowerMonitor::Source() is public, this has to be private or protected.
> 
> I'm happy to do this, but I'd like to understand why a bit better. This sounds
> like layering policy that I'm just not familiar with yet.

One bug/flaw in previous versions of SystemMonitr/PowerMonitor was that anyone
could call ProcessFooEvent.  That was determined to be bad because then anyone
could jump in and trigger the Foo event.  Same thing here.  I admit the chain is
weaker because of the IPC, etc - someone could inject a message at any one of
those steps - but it should be clear that they shouldn't do that and I think it
is except for this case.

https://codereview.chromium.org/17074009/diff/17001/content/common/gpu/client...
File content/common/gpu/client/gpu_channel_host.h (right):

https://codereview.chromium.org/17074009/diff/17001/content/common/gpu/client...
content/common/gpu/client/gpu_channel_host.h:252:
scoped_ptr<PowerMonitorMessageBroadcaster> power_monitor_broadcaster_;
On 2013/06/26 22:37:22, bajones wrote:
> On 2013/06/26 21:40:33, vandebo wrote:
> > direct member?
> 
> This class needs to be provided with the Sender*. I could do that as a
separate
> call, but I felt like during the constructor was a bit more natural. 

I may be misunderstanding your reply... you can still initialize this in the
constructor (and in fact you have to), by doing:
  power_monitor_broadcaster_(this)
instead of:
  power_monitor_broadcaster_(new PowerMonitorMessageBroadcaster(this))

https://codereview.chromium.org/17074009/diff/17001/content/common/power_moni...
File content/common/power_monitor_message_filter.cc (right):

https://codereview.chromium.org/17074009/diff/17001/content/common/power_moni...
content/common/power_monitor_message_filter.cc:13: message_filter_ = new
PowerMessageFilter(this);
On 2013/06/26 22:37:22, bajones wrote:
> On 2013/06/26 21:40:33, vandebo wrote:
> > memory leak
> 
> This is tricky. This filter will be passed to the GpuChannel's
> channel_->AddFilter() method, which appears to take ownership of the filter.
> Agreed that this is an awkward place to construct it, though. Probably be
better
> to add a MessageFilter factory method to this class, now that I think about
it.

If channel_ owns it, are we sure that it won't be deleted while
PowerMonitorBroadcastSource is alive?  If so, a comment in the header above
|message_filter_| explaining why the raw pointer is safe would be helpful.

[bajones, 2013-06-26 22:57:50]

https://codereview.chromium.org/17074009/diff/17001/content/common/gpu/client...
File content/common/gpu/client/gpu_channel_host.h (right):

https://codereview.chromium.org/17074009/diff/17001/content/common/gpu/client...
content/common/gpu/client/gpu_channel_host.h:252:
scoped_ptr<PowerMonitorMessageBroadcaster> power_monitor_broadcaster_;
On 2013/06/26 22:50:12, vandebo wrote:
> On 2013/06/26 22:37:22, bajones wrote:
> > On 2013/06/26 21:40:33, vandebo wrote:
> > > direct member?
> > 
> > This class needs to be provided with the Sender*. I could do that as a
> separate
> > call, but I felt like during the constructor was a bit more natural. 
> 
> I may be misunderstanding your reply... you can still initialize this in the
> constructor (and in fact you have to), by doing:
>   power_monitor_broadcaster_(this)
> instead of:
>   power_monitor_broadcaster_(new PowerMonitorMessageBroadcaster(this))

*facepalm*

This is what too much Javascript does to your C++ skills... Thanks for reminding
me how the language works. :)

[Ken Russell (switch to Gerrit), 2013-06-27 13:08:23]

The direction of the refactoring looks good but it looks to me like more changes
are needed. Also please solicit apatrick's feedback since he is much more
familiar with how IPCs are sent between the browser and GPU processes.

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

https://codereview.chromium.org/17074009/diff/17001/base/power_monitor/power_...
base/power_monitor/power_monitor_mac.mm:34: // If this message is not handled
the system may delay sleep for 30 seconds
Nice find! How did you discover the need for this? Do you think Chrome has been
blocking machines' going to sleep?

https://codereview.chromium.org/17074009/diff/17001/content/common/gpu/client...
File content/common/gpu/client/gpu_channel_host.cc (right):

https://codereview.chromium.org/17074009/diff/17001/content/common/gpu/client...
content/common/gpu/client/gpu_channel_host.cc:63:
power_monitor->AddObserver(power_monitor_broadcaster_.get());
As I mentioned in earlier review, I think the way this currently works at all is
that GpuChannelHost is used both in the browser and renderer processes to talk
to the GPU process. You really only want to hook up the
PowerMonitorMessageBroadcaster in the browser process, over the single channel
that it uses to talk to the GPU process. I think a more appropriate place for
this setup would be after the call to GpuChannelHost::Create in
BrowserGpuChannelHostFactory::EstablishGpuChannelSync. That way the broadcaster
wouldn't have to be contained in the GpuChannelHost, either.

(Looking more deeply, I can't see where a GpuChannelHost is instantiated in the
browser process -- it looks to me like
BrowserGpuChannelHostFactory::EstablishGpuChannelSync is not called.)

https://codereview.chromium.org/17074009/diff/17001/content/common/gpu/gpu_ch...
File content/common/gpu/gpu_channel.cc (right):

https://codereview.chromium.org/17074009/diff/17001/content/common/gpu/gpu_ch...
content/common/gpu/gpu_channel.cc:495:
channel_->AddFilter(power_monitor_source_->MessageFilter());
This setup doesn't look correct to me. You want to process control messages
coming from the browser process to the GPU process. As far as I can tell, the
recipient of the GPU-related control messages coming from the browser in
gpu_process_host.cc are handled in GpuChildThread::OnControlMessageReceived and
GpuChannelManager::OnMessageReceived. In fact, I don't see where those messages
would be re-dispatched to all of the GpuChannels that have been established
between renderer processes and the GPU process, and it looks to me like this
filter is being added only to those connections. Basically, I think that the
setup for receiving the power messages should be done in gpu_main.cc, after the
initialization of the GpuChildThread, and that you should add the MessageFilter
to the GpuChildThread's channel().

https://codereview.chromium.org/17074009/diff/17001/content/common/power_moni...
File content/common/power_monitor_message_filter.cc (right):

https://codereview.chromium.org/17074009/diff/17001/content/common/power_moni...
content/common/power_monitor_message_filter.cc:13: message_filter_ = new
PowerMessageFilter(this);
On 2013/06/26 22:50:12, vandebo wrote:
> On 2013/06/26 22:37:22, bajones wrote:
> > On 2013/06/26 21:40:33, vandebo wrote:
> > > memory leak
> > 
> > This is tricky. This filter will be passed to the GpuChannel's
> > channel_->AddFilter() method, which appears to take ownership of the filter.
> > Agreed that this is an awkward place to construct it, though. Probably be
> better
> > to add a MessageFilter factory method to this class, now that I think about
> it.
> 
> If channel_ owns it, are we sure that it won't be deleted while
> PowerMonitorBroadcastSource is alive?  If so, a comment in the header above
> |message_filter_| explaining why the raw pointer is safe would be helpful.

See gpu_channel.cc for an example and ipc_channel_proxy.h for documentation;
MessageFilter is ref-counted. message_filter_ should be
scoped_refptr<PowerMessageFilter>.

https://codereview.chromium.org/17074009/diff/17001/content/common/power_moni...
File content/common/power_monitor_message_filter.h (right):

https://codereview.chromium.org/17074009/diff/17001/content/common/power_moni...
content/common/power_monitor_message_filter.h:5: #ifndef
CONTENT_COMMON_POWER_MONITOR_MESSAGE_FILTER_H_
This file and the .cc should be named power_monitor_broadcast_source.

https://codereview.chromium.org/17074009/diff/17001/content/common/power_moni...
File content/common/power_monitor_messages.h (right):

https://codereview.chromium.org/17074009/diff/17001/content/common/power_moni...
content/common/power_monitor_messages.h:18: bool /* on_battery_power */)
Looks like incorrect indentation here.

[bajones, 2013-07-01 20:21:58]

+apatrick on Ken's suggestion. If you could look this over for Browser/GPU IPC
sanity I'd appreciate it.

Updated to address feedback, thanks for guiding me through some of the tricker
parts!

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

https://codereview.chromium.org/17074009/diff/17001/base/power_monitor/power_...
base/power_monitor/power_monitor_mac.mm:36:
IOAllowPowerChange(g_system_power_io_port,
See http://developer.apple.com/library/mac/#qa/qa1340/_index.html

From the code comments: "Power Management waits up to 30 seconds for you to
either allow or deny idle sleep. If you don't acknowledge this power change by
calling either IOAllowPowerChange or IOCancelPowerChange, the system will wait
30 seconds then go to sleep."

https://codereview.chromium.org/17074009/diff/17001/content/common/gpu/client...
File content/common/gpu/client/gpu_channel_host.cc (right):

https://codereview.chromium.org/17074009/diff/17001/content/common/gpu/client...
content/common/gpu/client/gpu_channel_host.cc:63:
power_monitor->AddObserver(power_monitor_broadcaster_.get());
On 2013/06/27 13:08:23, Ken Russell wrote:
> As I mentioned in earlier review, I think the way this currently works at all
is
> that GpuChannelHost is used both in the browser and renderer processes to talk
> to the GPU process. You really only want to hook up the
> PowerMonitorMessageBroadcaster in the browser process, over the single channel
> that it uses to talk to the GPU process. I think a more appropriate place for
> this setup would be after the call to GpuChannelHost::Create in
> BrowserGpuChannelHostFactory::EstablishGpuChannelSync. That way the
broadcaster
> wouldn't have to be contained in the GpuChannelHost, either.
> 
> (Looking more deeply, I can't see where a GpuChannelHost is instantiated in
the
> browser process -- it looks to me like
> BrowserGpuChannelHostFactory::EstablishGpuChannelSync is not called.)

It actually appears that BrowserGpuChannelHostFactory is hardly used by the
browser process. Also, rather than a GpuChannelHost the browser process
primarily communicates through the GpuProcessHost, which is established with a
PostTask in BrowserMainLoop
(https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br...).

I've moved the PowerMonitorBroadcaster onto the GpuProcessHost as a result. Not
sure if there are further concerns regarding it's location.

https://codereview.chromium.org/17074009/diff/17001/content/common/gpu/gpu_ch...
File content/common/gpu/gpu_channel.cc (right):

https://codereview.chromium.org/17074009/diff/17001/content/common/gpu/gpu_ch...
content/common/gpu/gpu_channel.cc:495:
channel_->AddFilter(power_monitor_source_->MessageFilter());
On 2013/06/26 21:40:33, vandebo wrote:
> Is there ever a RemoveFilter call?  The concern is that it may be possible for
> MessageFilter() to continue receiving messages after power_monitor_source has
> been cleaned up/destroyed.

I don't see any examples of a filter being removed in the code base.

https://codereview.chromium.org/17074009/diff/17001/content/common/gpu/gpu_ch...
content/common/gpu/gpu_channel.cc:495:
channel_->AddFilter(power_monitor_source_->MessageFilter());
On 2013/06/27 13:08:23, Ken Russell wrote:
> This setup doesn't look correct to me. You want to process control messages
> coming from the browser process to the GPU process. As far as I can tell, the
> recipient of the GPU-related control messages coming from the browser in
> gpu_process_host.cc are handled in GpuChildThread::OnControlMessageReceived
and
> GpuChannelManager::OnMessageReceived. In fact, I don't see where those
messages
> would be re-dispatched to all of the GpuChannels that have been established
> between renderer processes and the GPU process, and it looks to me like this
> filter is being added only to those connections. Basically, I think that the
> setup for receiving the power messages should be done in gpu_main.cc, after
the
> initialization of the GpuChildThread, and that you should add the
MessageFilter
> to the GpuChildThread's channel().

I've moved the addition of the message filter into gpu_main.cc as suggested,
which has the happy side effect of completely avoiding the nasty cast!

[apatrick_chromium, 2013-07-01 23:59:58]

The GpuProcessHost should generally only run on the IO thread. If you want an
IPC::Sender on the UI thread, which I think is where these power management
events originate from, you can use GPUProcessHostUIShim. It will post tasks to
the GpuProcessHost on the IO thread, which will in turn send the IPC message to
the GPU process.

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

https://codereview.chromium.org/17074009/diff/32001/base/power_monitor/power_...
base/power_monitor/power_monitor.h:23: PowerMonitor(PowerMonitorSource* source);
This constructor should be explicit because it takes only one arg.

https://codereview.chromium.org/17074009/diff/32001/base/power_monitor/power_...
File base/power_monitor/power_monitor_source.cc (right):

https://codereview.chromium.org/17074009/diff/32001/base/power_monitor/power_...
base/power_monitor/power_monitor_source.cc:17: 
nit: extra newline

https://codereview.chromium.org/17074009/diff/32001/content/common/power_moni...
File content/common/power_monitor_broadcast_source.h (right):

https://codereview.chromium.org/17074009/diff/32001/content/common/power_moni...
content/common/power_monitor_broadcast_source.h:27:
PowerMessageFilter(PowerMonitorBroadcastSource* source);
This constructor should be explicit.

https://codereview.chromium.org/17074009/diff/32001/content/common/power_moni...
File content/common/power_monitor_message_broadcaster.h (right):

https://codereview.chromium.org/17074009/diff/32001/content/common/power_moni...
content/common/power_monitor_message_broadcaster.h:25:
PowerMonitorMessageBroadcaster(IPC::Sender* sender);
Constructor should be explicit

[Ken Russell (switch to Gerrit), 2013-07-02 09:05:15]

Very nice. LGTM

https://codereview.chromium.org/17074009/diff/32001/base/power_monitor/power_...
File base/power_monitor/power_monitor_ios.mm (right):

https://codereview.chromium.org/17074009/diff/32001/base/power_monitor/power_...
base/power_monitor/power_monitor_ios.mm:11: void
PowerMonitorDeviceSource::PlatformInit() {
This file should probably be renamed to power_monitor_device_source_ios.mm.

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

https://codereview.chromium.org/17074009/diff/32001/base/power_monitor/power_...
base/power_monitor/power_monitor_mac.mm:59: void
PowerMonitorDeviceSource::AllocateSystemIOPorts() {
Probably should rename to power_monitor_device_source_mac.mm.

https://codereview.chromium.org/17074009/diff/32001/base/power_monitor/power_...
File base/power_monitor/power_monitor_posix.cc (right):

https://codereview.chromium.org/17074009/diff/32001/base/power_monitor/power_...
base/power_monitor/power_monitor_posix.cc:9: bool
PowerMonitorDeviceSource::IsBatteryPower() {
power_monitor_device_source_posix.cc?

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

https://codereview.chromium.org/17074009/diff/32001/base/power_monitor/power_...
base/power_monitor/power_monitor_win.cc:19: bool
PowerMonitorDeviceSource::IsBatteryPower() {
power_monitor_device_source_win.cc?

https://codereview.chromium.org/17074009/diff/32001/content/common/power_moni...
File content/common/power_monitor_broadcast_source.h (right):

https://codereview.chromium.org/17074009/diff/32001/content/common/power_moni...
content/common/power_monitor_broadcast_source.h:57: #endif  //
CONTENT_COMMON_POWER_MONITOR_MESSAGE_FILTER_H_
Wrong name in #include guard.

https://codereview.chromium.org/17074009/diff/32001/content/common/power_moni...
File content/common/power_monitor_message_broadcaster.h (right):

https://codereview.chromium.org/17074009/diff/32001/content/common/power_moni...
content/common/power_monitor_message_broadcaster.h:41: #endif  //
CONTENT_COMMON_POWER_MONITOR_CLIENT_H_
Wrong name in #include guard.

[Ken Russell (switch to Gerrit), 2013-07-02 09:08:31]

On 2013/07/01 23:59:58, apatrick_chromium wrote:
> The GpuProcessHost should generally only run on the IO thread. If you want an
> IPC::Sender on the UI thread, which I think is where these power management
> events originate from, you can use GPUProcessHostUIShim. It will post tasks to
> the GpuProcessHost on the IO thread, which will in turn send the IPC message
to
> the GPU process.

Very good point. Please try suspending/resuming with a debug build on Mac; there
are DCHECKs in GpuProcessHost which should catch attempts to use it on the wrong
thread.

[bajones, 2013-07-02 18:12:04]

On 2013/07/02 09:08:31, Ken Russell wrote:
> On 2013/07/01 23:59:58, apatrick_chromium wrote:
> > The GpuProcessHost should generally only run on the IO thread. If you want
an
> > IPC::Sender on the UI thread, which I think is where these power management
> > events originate from, you can use GPUProcessHostUIShim. It will post tasks
to
> > the GpuProcessHost on the IO thread, which will in turn send the IPC message
> to
> > the GPU process.
> 
> Very good point. Please try suspending/resuming with a debug build on Mac;
there
> are DCHECKs in GpuProcessHost which should catch attempts to use it on the
wrong
> thread.

Moved the broadcaster to the UI Shim and am not getting any DCHECKs with my
debug build. The rest of the feedback has been addressed as well.

[apatrick_chromium, 2013-07-02 19:39:28]

https://codereview.chromium.org/17074009/diff/46002/base/power_monitor/power_...
File base/power_monitor/power_monitor.cc (right):

https://codereview.chromium.org/17074009/diff/46002/base/power_monitor/power_...
base/power_monitor/power_monitor.cc:52: void
PowerMonitor::NotifyPowerStateChange(bool battery_in_use) {
In the browser process, this is invoked on the UI thread because it is called
indirectly from a window message handler (at least on Windows). In the GPU
process, this will be invoked on the IO thread because it is called from a
message filter. As a result, the observers registered with the PowerMonitor will
also run on the IO thread. Is this intentional?

You could have the observers run on the GPU process' main thread by not using a
MessageFilter and instead handling them in GpuChannelManager::OnMessageReceived
or GpuChildThread::OnMessageReceived.

https://codereview.chromium.org/17074009/diff/46002/content/common/gpu/client...
File content/common/gpu/client/gpu_channel_host.cc (right):

https://codereview.chromium.org/17074009/diff/46002/content/common/gpu/client...
content/common/gpu/client/gpu_channel_host.cc:14: #include
"base/power_monitor/power_monitor.h"
This does not seem to be used.

[bajones, 2013-07-02 20:35:26]

https://codereview.chromium.org/17074009/diff/46002/base/power_monitor/power_...
File base/power_monitor/power_monitor.cc (right):

https://codereview.chromium.org/17074009/diff/46002/base/power_monitor/power_...
base/power_monitor/power_monitor.cc:52: void
PowerMonitor::NotifyPowerStateChange(bool battery_in_use) {
On 2013/07/02 19:39:29, apatrick_chromium wrote:
> In the browser process, this is invoked on the UI thread because it is called
> indirectly from a window message handler (at least on Windows). In the GPU
> process, this will be invoked on the IO thread because it is called from a
> message filter. As a result, the observers registered with the PowerMonitor
will
> also run on the IO thread. Is this intentional?
> 
> You could have the observers run on the GPU process' main thread by not using
a
> MessageFilter and instead handling them in
GpuChannelManager::OnMessageReceived
> or GpuChildThread::OnMessageReceived.

I considered just adding the message handlers directly, but it's very likely
that the render process (are there any others?) will want this same behavior.
There's already power monitor code in render process that isn't functioning
correctly, and these changes would help it. By encapsulating the behavior in a
message filter it's trivial to extend the power monitor into new locations.

[apatrick_chromium, 2013-07-02 20:47:09]

Then you could make the MessageFilter have a MessageLoopProxy for the thread
that the power manager observers should be notified on and post the events there
from the IO thread.

[vandebo (ex-Chrome), 2013-07-02 22:53:11]

https://codereview.chromium.org/17074009/diff/46002/base/power_monitor/power_...
File base/power_monitor/power_monitor.cc (right):

https://codereview.chromium.org/17074009/diff/46002/base/power_monitor/power_...
base/power_monitor/power_monitor.cc:13: // This constructor is provided to
preserve backwards compatibility with the
Hmm, I wonder if we should just change the browser process callsite to construct
the PowerMonitorDeviceSource()... I guess we should leave that for a future
change when all the current uses of the empty constructor are changed.

Add TODO to remove this constructor?

https://codereview.chromium.org/17074009/diff/46002/base/power_monitor/power_...
base/power_monitor/power_monitor.cc:52: void
PowerMonitor::NotifyPowerStateChange(bool battery_in_use) {
On 2013/07/02 19:39:29, apatrick_chromium wrote:
> In the browser process, this is invoked on the UI thread because it is called
> indirectly from a window message handler (at least on Windows). In the GPU
> process, this will be invoked on the IO thread because it is called from a
> message filter. As a result, the observers registered with the PowerMonitor
will
> also run on the IO thread. Is this intentional?
> 
> You could have the observers run on the GPU process' main thread by not using
a
> MessageFilter and instead handling them in
GpuChannelManager::OnMessageReceived
> or GpuChildThread::OnMessageReceived.

ObserverListThreadSafe explicitly says it's ok to notify from any thread, so
this seems fine.

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

https://codereview.chromium.org/17074009/diff/46002/base/power_monitor/power_...
base/power_monitor/power_monitor.h:22: PowerMonitor();
nit: Add comment that this constructor is only useful in the browser process and
that in any other process the other constructor needs to be used.

https://codereview.chromium.org/17074009/diff/46002/base/power_monitor/power_...
base/power_monitor/power_monitor.h:23: explicit PowerMonitor(PowerMonitorSource*
source);
nit: Add comment that PowerMonitor assumes ownership of |source|.

https://codereview.chromium.org/17074009/diff/46002/base/power_monitor/power_...
File base/power_monitor/power_monitor_device_source_android.cc (right):

https://codereview.chromium.org/17074009/diff/46002/base/power_monitor/power_...
base/power_monitor/power_monitor_device_source_android.cc:11: // A helper
function which is a friend of PowerMonitorSource.
Shouldn't it be PowerMonitorDeviceSource?

https://codereview.chromium.org/17074009/diff/46002/base/power_monitor/power_...
File base/power_monitor/power_monitor_source.h (right):

https://codereview.chromium.org/17074009/diff/46002/base/power_monitor/power_...
base/power_monitor/power_monitor_source.h:37: friend class PowerMonitor;
What is this friend statement needed for?

https://codereview.chromium.org/17074009/diff/46002/content/common/power_moni...
File content/common/power_monitor_broadcast_source.h (right):

https://codereview.chromium.org/17074009/diff/46002/content/common/power_moni...
content/common/power_monitor_broadcast_source.h:40: PowerMonitorBroadcastSource*
source_;
How are we sure that this pointer is still valid?  At the least it needs a
comment.  Couldn't you just use PowerMonitor::Source() though?

https://codereview.chromium.org/17074009/diff/46002/content/common/power_moni...
File content/common/power_monitor_message_broadcaster.cc (right):

https://codereview.chromium.org/17074009/diff/46002/content/common/power_moni...
content/common/power_monitor_message_broadcaster.cc:16: if (power_monitor)
Is there a specific case where this is not true?  Start up should be
deterministic/easy to validate.  Probably just want to DCHECK(power_monitor) ?

https://codereview.chromium.org/17074009/diff/46002/content/common/power_moni...
File content/common/power_monitor_messages.h (right):

https://codereview.chromium.org/17074009/diff/46002/content/common/power_moni...
content/common/power_monitor_messages.h:8: #include "ipc/ipc_param_traits.h"
Are ipc_param_traits.h and ipc_platform_file.h needed?

https://codereview.chromium.org/17074009/diff/46002/content/gpu/gpu_main.cc
File content/gpu/gpu_main.cc (right):

https://codereview.chromium.org/17074009/diff/46002/content/gpu/gpu_main.cc#n...
content/gpu/gpu_main.cc:176: base::PowerMonitor
power_monitor(power_monitor_source);
nit: base::PowerMonitor power_monitor(new PowerMonitorBroadcastSource()); ?

[Ken Russell (switch to Gerrit), 2013-07-03 09:49:42]

https://codereview.chromium.org/17074009/diff/68001/content/common/power_moni...
File content/common/power_monitor_broadcast_source.cc (right):

https://codereview.chromium.org/17074009/diff/68001/content/common/power_moni...
content/common/power_monitor_broadcast_source.cc:36: on_battery_power_ =
on_battery_power;
Is synchronization needed around this variable? It's modified on the IO thread
and queried on the UI thread.

https://codereview.chromium.org/17074009/diff/68001/content/common/power_moni...
content/common/power_monitor_broadcast_source.cc:40:
//ProcessPowerEvent(PowerMonitorSource::POWER_STATE_EVENT);
Here and below: remove commented out code.

[bajones, 2013-07-03 20:32:26]

Reverted the MessageLoopProxy use based on vandebo's comments. All other
feedback has been addressed.

https://codereview.chromium.org/17074009/diff/46002/base/power_monitor/power_...
File base/power_monitor/power_monitor_device_source_android.cc (right):

https://codereview.chromium.org/17074009/diff/46002/base/power_monitor/power_...
base/power_monitor/power_monitor_device_source_android.cc:11: // A helper
function which is a friend of PowerMonitorSource.
On 2013/07/02 22:53:12, vandebo wrote:
> Shouldn't it be PowerMonitorDeviceSource? 

No, it's PowerMonitorSource. See power_monitor_source.h line 40

https://codereview.chromium.org/17074009/diff/46002/base/power_monitor/power_...
File base/power_monitor/power_monitor_source.h (right):

https://codereview.chromium.org/17074009/diff/46002/base/power_monitor/power_...
base/power_monitor/power_monitor_source.h:37: friend class PowerMonitor;
On 2013/07/02 22:53:12, vandebo wrote:
> What is this friend statement needed for?

Old code. Deleted.

https://codereview.chromium.org/17074009/diff/46002/content/common/power_moni...
File content/common/power_monitor_messages.h (right):

https://codereview.chromium.org/17074009/diff/46002/content/common/power_moni...
content/common/power_monitor_messages.h:8: #include "ipc/ipc_param_traits.h"
On 2013/07/02 22:53:12, vandebo wrote:
> Are ipc_param_traits.h and ipc_platform_file.h needed?

It appears not! Deleted.

https://codereview.chromium.org/17074009/diff/46002/content/gpu/gpu_main.cc
File content/gpu/gpu_main.cc (right):

https://codereview.chromium.org/17074009/diff/46002/content/gpu/gpu_main.cc#n...
content/gpu/gpu_main.cc:176: base::PowerMonitor
power_monitor(power_monitor_source);
On 2013/07/02 22:53:12, vandebo wrote:
> nit: base::PowerMonitor power_monitor(new PowerMonitorBroadcastSource()); ?

See line 292. The power_monitor_source is referenced later in this file.

[bajones, 2013-07-08 22:07:59]

Need LG on /base, /chrome/browser/extensions, /content, and /ipc. Darin, you
cover all of that. Any chance I could get either a review or suggestions for
more appropriate reviewers?

[apatrick_chromium, 2013-07-08 22:09:05]

LGTM for my part with resolution of threading issues.

[bajones, 2013-07-08 22:15:23]

On 2013/07/08 22:09:05, apatrick_chromium wrote:
> LGTM for my part with resolution of threading issues.

I assume you are referring to the notifications being invoked on the IO thread?
@vandebo mentioned that the ObserverListThreadSafe should handle this. Do you
still feel it needs to be explicitly addressed?

[apatrick_chromium, 2013-07-08 22:41:26]

On 2013/07/08 22:15:23, bajones wrote:
> On 2013/07/08 22:09:05, apatrick_chromium wrote:
> > LGTM for my part with resolution of threading issues.
> 
> I assume you are referring to the notifications being invoked on the IO
thread?
> @vandebo mentioned that the ObserverListThreadSafe should handle this. Do you
> still feel it needs to be explicitly addressed?

Yeah I'm cool with the notifications happening on the IO thread if it is
intentional. Sorry my reply wasn't well worded.

[bajones, 2013-07-09 17:39:09]

vandebo@: Do you have any further outstanding concerns?

All: I need security reviews on ipc_message_start.h and
power_monitor_messages.h. Beyond that I need LG on:

base/
chrome/browser/extensions/
content/app
content/browser
content/common

Thank you!

[Matt Perry, 2013-07-09 19:41:37]

LGTM extensions

[palmer, 2013-07-09 21:16:09]

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

https://codereview.chromium.org/17074009/diff/106001/base/power_monitor/power...
base/power_monitor/power_monitor.cc:18: // new PowerMonitorDeviceSource()
Maybe this is the real fix, rather than DCHECKing on process type. (Then you
could just get rid of the 0-arg constructor.) How bad is it if anyone uses this
version of the constructor in a non-browser process?

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

https://codereview.chromium.org/17074009/diff/106001/base/power_monitor/power...
base/power_monitor/power_monitor.h:23: // other processes should explicitly pass
an appropriate PowerMonitorSource
Is there any way to enforce that, with e.g. a DCHECK on process type? (Maybe you
do, I haven't checked the implementation yet.)

https://codereview.chromium.org/17074009/diff/106001/base/power_monitor/power...
base/power_monitor/power_monitor.h:25: // PowerMonitor assumes ownership of
source
Nit: I'd rewrite this comment as:

    Takes ownership of |source|.

https://codereview.chromium.org/17074009/diff/106001/base/power_monitor/power...
base/power_monitor/power_monitor.h:40: // Is the computer currently on battery
power.
While you're here, give this a more boolean-sounding name, e.g.
|IsOnBatteryPower|.

https://codereview.chromium.org/17074009/diff/106001/base/power_monitor/power...
File base/power_monitor/power_monitor_device_source.h (right):

https://codereview.chromium.org/17074009/diff/106001/base/power_monitor/power...
base/power_monitor/power_monitor_device_source.h:86: virtual bool
IsBatteryPower() OVERRIDE;
Harmonize this name with whatever you choose for the |BatteryPower| method in
the other file. I like |IsOnBatteryPower| (or maybe |IsUsingBatteryPower|) best,
but |IsBatteryPower| is fine, too.

https://codereview.chromium.org/17074009/diff/106001/base/power_monitor/power...
File base/power_monitor/power_monitor_device_source_mac.mm (right):

https://codereview.chromium.org/17074009/diff/106001/base/power_monitor/power...
base/power_monitor/power_monitor_device_source_mac.mm:34: // If this message is
not handled the system may delay sleep for 30 seconds
Nit: Punctuate the sentence.

https://codereview.chromium.org/17074009/diff/106001/base/power_monitor/power...
File base/power_monitor/power_monitor_source.cc (right):

https://codereview.chromium.org/17074009/diff/106001/base/power_monitor/power...
base/power_monitor/power_monitor_source.cc:24: bool
PowerMonitorSource::IsBatteryPower() {
This is a very strange wart.

https://codereview.chromium.org/17074009/diff/106001/base/power_monitor/power...
base/power_monitor/power_monitor_source.cc:37: bool on_battery =
IsBatteryPower();
This will trigger NOTIMPLEMENTED.

https://codereview.chromium.org/17074009/diff/106001/base/power_monitor/power...
File base/power_monitor/power_monitor_source.h (right):

https://codereview.chromium.org/17074009/diff/106001/base/power_monitor/power...
base/power_monitor/power_monitor_source.h:32: bool BatteryPower();
Same here: harmonize this name. Why do you have this public and then also the
protected |IsBatteryPower|?

https://codereview.chromium.org/17074009/diff/106001/content/common/power_mon...
File content/common/power_monitor_broadcast_source.cc (right):

https://codereview.chromium.org/17074009/diff/106001/content/common/power_mon...
content/common/power_monitor_broadcast_source.cc:53: bool msg_is_ok = true;
So we don't intend to handle any kind of message serialization errors or other
problems? Shouldn't we?

[vandebo (ex-Chrome), 2013-07-09 22:22:08]

It's looking good.  The several different levels of "OnBattery" and how that all
works is very confusing as Palmer noticed.  Some better names probably will
help.

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

https://codereview.chromium.org/17074009/diff/106001/base/android/base_jni_re...
base/android/base_jni_registrar.cc:45: { "PowerMonitor",
base::RegisterPowerMonitor },
Does this need to be updated?

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

https://codereview.chromium.org/17074009/diff/106001/base/power_monitor/power...
base/power_monitor/power_monitor.cc:18: // new PowerMonitorDeviceSource()
On 2013/07/09 21:16:10, Chromium Palmer wrote:
> Maybe this is the real fix, rather than DCHECKing on process type. (Then you
> could just get rid of the 0-arg constructor.) How bad is it if anyone uses
this
> version of the constructor in a non-browser process?

All that will happen if the 0-arg constructor is used in a non-browser process
is that it won't work.  Renderer processes already have this problem and (since
it was found and worked around) it hasn't caused any noticeable negative
affects.

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

https://codereview.chromium.org/17074009/diff/106001/base/power_monitor/power...
base/power_monitor/power_monitor.h:23: // other processes should explicitly pass
an appropriate PowerMonitorSource
On 2013/07/09 21:16:10, Chromium Palmer wrote:
> Is there any way to enforce that, with e.g. a DCHECK on process type? (Maybe
you
> do, I haven't checked the implementation yet.)

This should be a short lived comment.  A follow up CL should update other uses
of PowerMonitor and remove this constructor.

https://codereview.chromium.org/17074009/diff/106001/base/power_monitor/power...
base/power_monitor/power_monitor.h:38: PowerMonitorSource* Source();
Is Source() just needed for the device source implementations?  It's ugly to
expose it.  This might be uglier, but we could hide it by having protected:
static PowerMonitorSource::Source() and private: PowerMonitor:Source().  The
existing friend declarations should then let ProcessPowerEventHelper() get
Source().

Actually even easier than that would be to make ProcessPowerEvent() be private
static.  Internally it can do the PowerMonitor::Get()

https://codereview.chromium.org/17074009/diff/106001/base/power_monitor/power...
File base/power_monitor/power_monitor_device_source_android.cc (right):

https://codereview.chromium.org/17074009/diff/106001/base/power_monitor/power...
base/power_monitor/power_monitor_device_source_android.cc:4: 
include power_monitor_device_source_android.h

https://codereview.chromium.org/17074009/diff/106001/base/power_monitor/power...
File base/power_monitor/power_monitor_device_source_android.h (right):

https://codereview.chromium.org/17074009/diff/106001/base/power_monitor/power...
base/power_monitor/power_monitor_device_source_android.h:5: #ifndef
BASE_POWER_MONITOR_POWER_MONITOR_ANDROID_H_
POWER_MONITOR_DEVICE_SOURCE_ANDROID_H_

https://codereview.chromium.org/17074009/diff/106001/base/power_monitor/power...
base/power_monitor/power_monitor_device_source_android.h:12: // Registers the
JNI bindings for PowerMonitor.
PowerMonitorDeviceSource

https://codereview.chromium.org/17074009/diff/106001/base/power_monitor/power...
File base/power_monitor/power_monitor_device_source_mac.mm (right):

https://codereview.chromium.org/17074009/diff/106001/base/power_monitor/power...
base/power_monitor/power_monitor_device_source_mac.mm:17: void
ProcessPowerEventHelper(PowerMonitorSource::PowerEvent event) {
IWYU - include power_monitor_source.h

https://codereview.chromium.org/17074009/diff/106001/base/power_monitor/power...
File base/power_monitor/power_monitor_device_source_win.cc (right):

https://codereview.chromium.org/17074009/diff/106001/base/power_monitor/power...
base/power_monitor/power_monitor_device_source_win.cc:70:
PowerMonitorSource::PowerEvent power_event;
IWYU: power_monitor_source.h

https://codereview.chromium.org/17074009/diff/106001/base/power_monitor/power...
File base/power_monitor/power_monitor_source.h (right):

https://codereview.chromium.org/17074009/diff/106001/base/power_monitor/power...
base/power_monitor/power_monitor_source.h:44: virtual bool IsBatteryPower();
Make this pure virtual ?

https://codereview.chromium.org/17074009/diff/106001/base/power_monitor/power...
base/power_monitor/power_monitor_source.h:44: virtual bool IsBatteryPower();
Maybe IsBatteryPowerImpl() ?

https://codereview.chromium.org/17074009/diff/106001/base/power_monitor/power...
base/power_monitor/power_monitor_source.h:46: bool battery_in_use_;
Shouldn't these all be private?

https://codereview.chromium.org/17074009/diff/106001/base/power_monitor/power...
File base/power_monitor/power_monitor_unittest.cc (right):

https://codereview.chromium.org/17074009/diff/106001/base/power_monitor/power...
base/power_monitor/power_monitor_unittest.cc:55: void
ProcessPowerEvent(PowerMonitorSource::PowerEvent event_id) {
IWYU

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

https://codereview.chromium.org/17074009/diff/106001/base/power_monitor/power...
base/power_monitor/power_monitor_win.cc:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
Strange - the CL says M, not D for this file, is it deleted in your client?

https://codereview.chromium.org/17074009/diff/106001/content/common/power_mon...
File content/common/power_monitor_broadcast_source.cc (right):

https://codereview.chromium.org/17074009/diff/106001/content/common/power_mon...
content/common/power_monitor_broadcast_source.cc:31: void
PowerMonitorBroadcastSource::OnPowerStateChange(bool on_battery_power) {
A comment on this method might help...

https://codereview.chromium.org/17074009/diff/106001/content/common/power_mon...
File content/common/power_monitor_broadcast_source.h (right):

https://codereview.chromium.org/17074009/diff/106001/content/common/power_mon...
content/common/power_monitor_broadcast_source.h:15: class
PowerMonitorBroadcastSource : public base::PowerMonitorSource {
Definitely need a comment on this class.  It's a PowerMonitorSource, but it's
the destination of the IPCs...

https://codereview.chromium.org/17074009/diff/106001/content/common/power_mon...
content/common/power_monitor_broadcast_source.h:52: bool on_battery_power_;
Oh man, this is confusing...  Is there a name that might help explain what is
going on with this variable?

https://codereview.chromium.org/17074009/diff/106001/content/common/power_mon...
File content/common/power_monitor_message_broadcaster.cc (right):

https://codereview.chromium.org/17074009/diff/106001/content/common/power_mon...
content/common/power_monitor_message_broadcaster.cc:16: if (power_monitor)
Is this ever false?

[bajones, 2013-07-09 22:27:59]

Thanks palmer@. Working on addressing your feedback now, with a few comments
below.

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

https://codereview.chromium.org/17074009/diff/106001/base/power_monitor/power...
base/power_monitor/power_monitor.cc:18: // new PowerMonitorDeviceSource()
On 2013/07/09 21:16:10, Chromium Palmer wrote:
> Maybe this is the real fix, rather than DCHECKing on process type. (Then you
> could just get rid of the 0-arg constructor.) How bad is it if anyone uses
this
> version of the constructor in a non-browser process?

If used on a non-browser process then they most likely won't get power events at
all.

I left this in place to avoid touching even more files in this already large CL,
but it's not unreasonable to update everything. The constructor would need to be
updated in:

src/base/power_monitor/power_monitor_unittest.cc
src/chrome/browser/extensions/event_router_forwarder_unittest.cc
src/chrome/nacl/nacl_main.cc
src/content/browser/browser_main_loop.cc
src/content/plugin/plugin_main.cc
src/content/renderer/renderer_main.cc // Shouldn't be here in the first place,
actually...
src/content/utility/utility_main.cc
src/content/worker/worker_main.cc

https://codereview.chromium.org/17074009/diff/106001/base/power_monitor/power...
File base/power_monitor/power_monitor_source.h (right):

https://codereview.chromium.org/17074009/diff/106001/base/power_monitor/power...
base/power_monitor/power_monitor_source.h:32: bool BatteryPower();
On 2013/07/09 21:16:10, Chromium Palmer wrote:
> Same here: harmonize this name. Why do you have this public and then also the
> protected |IsBatteryPower|?

Better naming could clear this up. (This was in place before this CL, simply in
a different file)

the public |BatteryPower| is (now) threadsafe and returns the cached battery
state. |IsBatteryPowered| actually queries the hardware's current battery
status, which is probably assumed to be a slow operation and thus is only done
when necessary.

How about |IsOnBatteryPower| for the public call and |QueryBatteryPowerState|
for the protected?

https://codereview.chromium.org/17074009/diff/106001/content/common/power_mon...
File content/common/power_monitor_broadcast_source.cc (right):

https://codereview.chromium.org/17074009/diff/106001/content/common/power_mon...
content/common/power_monitor_broadcast_source.cc:53: bool msg_is_ok = true;
On 2013/07/09 21:16:10, Chromium Palmer wrote:
> So we don't intend to handle any kind of message serialization errors or other
> problems? Shouldn't we?

Forgive my lack of familiarity with these things, but I'm not sure what sort of
situations would need to be handled here. (Aside from unhandled messaged, which
do get flagged appropriately.) Do you know of any good message handlers to
reference for error handling?

[palmer, 2013-07-10 21:21:56]

> If used on a non-browser process then they most likely won't get power events
at
> all.
> 
> I left this in place to avoid touching even more files in this already large
CL,
> but it's not unreasonable to update everything. The constructor would need to
be
> updated in:

Understandable. I'm not super exercised about it right now, so I'd be fine with
just a TODO(bajones) on it.

> the public |BatteryPower| is (now) threadsafe and returns the cached battery
> state. |IsBatteryPowered| actually queries the hardware's current battery
> status, which is probably assumed to be a slow operation and thus is only done
> when necessary.

Makes sense.

> How about |IsOnBatteryPower| for the public call and |QueryBatteryPowerState|
> for the protected?

SGTM!

> > So we don't intend to handle any kind of message serialization errors or
other
> > problems? Shouldn't we?
> 
> Forgive my lack of familiarity with these things, but I'm not sure what sort
of
> situations would need to be handled here. (Aside from unhandled messaged,
which
> do get flagged appropriately.) Do you know of any good message handlers to
> reference for error handling?

At a minimum, a maniac or an accident could always cause the code to receive a
malformed or mangled IPC message. In general, the best thing to do is just drop
it and clean up any pending protocol state; in your case, there is no additional
cleanup you'd need to do. The canonical example from ipc/ipc_message_macros.h:

 918 //   bool MyClass::OnMessageReceived(const IPC::Message& msg) {
 919 //     bool handled = true;
 920 //     bool msg_is_good = false;
 921 //     IPC_BEGIN_MESSAGE_MAP_EX(MyClass, msg, msg_is_good)
 922 //       IPC_MESSAGE_HANDLER(MsgClassOne, OnMsgClassOne)
 923 //       ...more handlers here ...
 924 //       IPC_MESSAGE_HANDLER(MsgClassTen, OnMsgClassTen)
 925 //       IPC_MESSAGE_UNHANDLED(handled = false)
 926 //     IPC_END_MESSAGE_MAP_EX()
 927 //     if (!msg_is_good) {
 928 //       // Signal error here or terminate offending process.
 929 //     }
 930 //     return handled;
 931 //   }

would work for you, here, too. Where the example has "// Signal error here or
terminate offending process.", you can just return false.

It just "feels" better to me to make sure we check all our instruments before
returning true for take-off.

[bajones, 2013-07-10 22:08:34]

Feedback addressed, please take another look.

FYI: I opted to go with vandebo's suggestion of |IsOnBatteryPower| and
|IsOnBatteryPowerImpl| to differentiate the public/private functions.

[vandebo (ex-Chrome), 2013-07-10 22:37:33]

https://codereview.chromium.org/17074009/diff/146001/base/power_monitor/power...
File base/power_monitor/power_monitor_source.cc (right):

https://codereview.chromium.org/17074009/diff/146001/base/power_monitor/power...
base/power_monitor/power_monitor_source.cc:36: PowerMonitorSource* source =
monitor->source_.get();
I think the preference is to have a private accessor, even for friends.

https://codereview.chromium.org/17074009/diff/146001/base/power_monitor/power...
File base/power_monitor/power_monitor_source.h (right):

https://codereview.chromium.org/17074009/diff/146001/base/power_monitor/power...
base/power_monitor/power_monitor_source.h:53: bool on_battery_power_;
Please make these private

[palmer, 2013-07-10 22:40:38]

IPC security LGTM

[bajones, 2013-07-10 22:44:38]

https://codereview.chromium.org/17074009/diff/146001/base/power_monitor/power...
File base/power_monitor/power_monitor_source.cc (right):

https://codereview.chromium.org/17074009/diff/146001/base/power_monitor/power...
base/power_monitor/power_monitor_source.cc:36: PowerMonitorSource* source =
monitor->source_.get();
On 2013/07/10 22:37:34, vandebo wrote:
> I think the preference is to have a private accessor, even for friends.

Done.

https://codereview.chromium.org/17074009/diff/146001/base/power_monitor/power...
File base/power_monitor/power_monitor_source.h (right):

https://codereview.chromium.org/17074009/diff/146001/base/power_monitor/power...
base/power_monitor/power_monitor_source.h:53: bool on_battery_power_;
On 2013/07/10 22:37:34, vandebo wrote:
> Please make these private

Done.

[vandebo (ex-Chrome), 2013-07-10 22:49:37]

LGTM

[bajones, 2013-07-10 23:05:41]

On 2013/07/10 22:49:37, vandebo wrote:
> LGTM

Thanks for slogging through this with me. :)

Still need an owners LG on /base and /content/*

[bajones, 2013-07-12 18:55:07]

Expanded the unit tests to cover the components in content and improve coverage
of PowerMonitor and PowerMonitorSource

https://codereview.chromium.org/17074009/diff/167001/content/common/power_mon...
File content/common/power_monitor_broadcast_source_unittest.cc (right):

https://codereview.chromium.org/17074009/diff/167001/content/common/power_mon...
content/common/power_monitor_broadcast_source_unittest.cc:67:
/*PowerMonitorMsg_PowerStateChange on_battery_msg(true);
Testing these messages currently has me stumped. Parameterless messages (like
suspend and resume) work fine, but when I attempt to construct the
PowerStateChange messages, which takes a boolean, I get linker errors. Typically
these types of errors would be resolved by adding MODULENAME_EXPORT to a class,
but since the messages are defined by macros that's not an option.

I don't intend to commit this test until I can cover these cases, but I'm not
sure where to look to figure out the linker errors. Any suggestions for
applicable contacts or resources?

[bajones, 2013-07-12 20:23:59]

Unit test linker issues resolved with some help from @kbr.

Still need an owners LG on:
/base
/content
/content/app
/content/browser
/content/common

[piman, 2013-07-12 21:09:57]

Some threading (potential or real) issues, and some nits

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

https://codereview.chromium.org/17074009/diff/172001/base/power_monitor/power...
base/power_monitor/power_monitor.h:26: explicit PowerMonitor(PowerMonitorSource*
source);
nit: pass a scoped_ptr instead to make it obvious (on the caller site) that it
takes ownership.

https://codereview.chromium.org/17074009/diff/172001/base/power_monitor/power...
File base/power_monitor/power_monitor_source.cc (right):

https://codereview.chromium.org/17074009/diff/172001/base/power_monitor/power...
base/power_monitor/power_monitor_source.cc:43:
monitor->NotifyPowerStateChange(source->IsOnBatteryPower());
note: maybe it's benign, but since you release the lock in UpdateOnBatteryPower
and grab it again in IsOnBatteryPower you could have a race where
on_battery_power_ changes (on another thread) in the middle. Does it matter?

https://codereview.chromium.org/17074009/diff/172001/base/power_monitor/power...
File base/power_monitor/power_monitor_source.h (right):

https://codereview.chromium.org/17074009/diff/172001/base/power_monitor/power...
base/power_monitor/power_monitor_source.h:56: Lock lock_;
What does the lock protect? Can you document that?
ISTM it protects on_battery_power_ but not suspended_, meaning suspended_ should
only be accessed from one thread - which one? Can you document that?
It also looks like IsOnBatteryPowerImpl is called with the lock held, meaning
the implementation should not call other methods that will take the lock again,
such as IsOnBatteryPower, UpdateOnBatteryPower or ProcessPowerEvent. Can you
document that?

https://codereview.chromium.org/17074009/diff/172001/content/common/power_mon...
File content/common/power_monitor_broadcast_source.cc (right):

https://codereview.chromium.org/17074009/diff/172001/content/common/power_mon...
content/common/power_monitor_broadcast_source.cc:66: if (source_.get())
The weak reference gets destructed on the main thread, whereas this runs on the
IO thread, making it unsafe.

More generally, what is the expected threading behavior of the various classes?
- can you document that? Are PowerMonitor and PowerMonitorSource meant to be
thread-safe? What makes it so? Can you document that?

https://codereview.chromium.org/17074009/diff/172001/content/common/power_mon...
File content/common/power_monitor_broadcast_source.h (right):

https://codereview.chromium.org/17074009/diff/172001/content/common/power_mon...
content/common/power_monitor_broadcast_source.h:59: };
nit: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/17074009/diff/172001/content/common/power_mon...
File content/common/power_monitor_message_broadcaster_unittest.cc (right):

https://codereview.chromium.org/17074009/diff/172001/content/common/power_mon...
content/common/power_monitor_message_broadcaster_unittest.cc:37: }
nit: delete msg

https://codereview.chromium.org/17074009/diff/172001/content/common/power_mon...
content/common/power_monitor_message_broadcaster_unittest.cc:38: return false;
nit: true?

https://codereview.chromium.org/17074009/diff/172001/content/gpu/gpu_main.cc
File content/gpu/gpu_main.cc (right):

https://codereview.chromium.org/17074009/diff/172001/content/gpu/gpu_main.cc#...
content/gpu/gpu_main.cc:292:
child_thread->channel()->AddFilter(power_monitor_source->MessageFilter());
So, what is meant to happen on Android, where the GpuChildThread is in-process,
created in GpuProcessHost?
Should we avoid creating the PowerMonitorMessageBroadcaster then, assuming the
GPU thread would get notification from the process-wide PowerMonitor directly,
so that we avoid sending messages that have no receiver?

[bajones, 2013-07-13 00:33:19]

Still have a threading issue and some documentation to address. Pushed WIP so I
don't lose track of anything over the weekend.

https://codereview.chromium.org/17074009/diff/172001/base/power_monitor/power...
File base/power_monitor/power_monitor_source.cc (right):

https://codereview.chromium.org/17074009/diff/172001/base/power_monitor/power...
base/power_monitor/power_monitor_source.cc:43:
monitor->NotifyPowerStateChange(source->IsOnBatteryPower());
On 2013/07/12 21:09:58, piman wrote:
> note: maybe it's benign, but since you release the lock in
UpdateOnBatteryPower
> and grab it again in IsOnBatteryPower you could have a race where
> on_battery_power_ changes (on another thread) in the middle. Does it matter?

I doubt it does, but for the sake of avoiding any future questions like this
I've refactored it to handle the locking better (though the code is a bit uglier
for it.)

https://codereview.chromium.org/17074009/diff/172001/base/power_monitor/power...
File base/power_monitor/power_monitor_source.h (right):

https://codereview.chromium.org/17074009/diff/172001/base/power_monitor/power...
base/power_monitor/power_monitor_source.h:56: Lock lock_;
On 2013/07/12 21:09:58, piman wrote:
> What does the lock protect? Can you document that?
> ISTM it protects on_battery_power_ but not suspended_, meaning suspended_
should
> only be accessed from one thread - which one? Can you document that?
> It also looks like IsOnBatteryPowerImpl is called with the lock held, meaning
> the implementation should not call other methods that will take the lock
again,
> such as IsOnBatteryPower, UpdateOnBatteryPower or ProcessPowerEvent. Can you
> document that?

IsOnBatteryPowerImpl is now called outside of the lock.

https://codereview.chromium.org/17074009/diff/172001/content/gpu/gpu_main.cc
File content/gpu/gpu_main.cc (right):

https://codereview.chromium.org/17074009/diff/172001/content/gpu/gpu_main.cc#...
content/gpu/gpu_main.cc:292:
child_thread->channel()->AddFilter(power_monitor_source->MessageFilter());
On 2013/07/12 21:09:58, piman wrote:
> So, what is meant to happen on Android, where the GpuChildThread is
in-process,
> created in GpuProcessHost?
> Should we avoid creating the PowerMonitorMessageBroadcaster then, assuming the
> GPU thread would get notification from the process-wide PowerMonitor directly,
> so that we avoid sending messages that have no receiver?

That's a good point. I've added a guard on in_browser_process around the
PowerMonitor creation in this file.

[piman, 2013-07-13 00:44:40]

https://codereview.chromium.org/17074009/diff/172001/content/gpu/gpu_main.cc
File content/gpu/gpu_main.cc (right):

https://codereview.chromium.org/17074009/diff/172001/content/gpu/gpu_main.cc#...
content/gpu/gpu_main.cc:292:
child_thread->channel()->AddFilter(power_monitor_source->MessageFilter());
On 2013/07/13 00:33:20, bajones wrote:
> On 2013/07/12 21:09:58, piman wrote:
> > So, what is meant to happen on Android, where the GpuChildThread is
> in-process,
> > created in GpuProcessHost?
> > Should we avoid creating the PowerMonitorMessageBroadcaster then, assuming
the
> > GPU thread would get notification from the process-wide PowerMonitor
directly,
> > so that we avoid sending messages that have no receiver?
> 
> That's a good point. I've added a guard on in_browser_process around the
> PowerMonitor creation in this file.

That's not exactly what I had in mind... On Android (or with --single-process)
GpuMain isn't called at all. Instead a GpuChildThread is created from
GpuMainThread in gpu_process_host.cc (from the GpuProcessHost creation). That
one won't have a filter, which is probably ok, since you don't want to forward
the state from the existing PowerMonitor to itself.
However, the GpuProcessHostUIShim will still be created, and with it the
PowerMonitorMessageBroadcaster, which will send the messages when there's no
receiver.

[bajones, 2013-07-15 20:31:10]

On 2013/07/13 00:44:40, piman wrote:
> https://codereview.chromium.org/17074009/diff/172001/content/gpu/gpu_main.cc
> File content/gpu/gpu_main.cc (right):
> 
>
https://codereview.chromium.org/17074009/diff/172001/content/gpu/gpu_main.cc#...
> content/gpu/gpu_main.cc:292:
> child_thread->channel()->AddFilter(power_monitor_source->MessageFilter());
> On 2013/07/13 00:33:20, bajones wrote:
> > On 2013/07/12 21:09:58, piman wrote:
> > > So, what is meant to happen on Android, where the GpuChildThread is
> > in-process,
> > > created in GpuProcessHost?
> > > Should we avoid creating the PowerMonitorMessageBroadcaster then, assuming
> the
> > > GPU thread would get notification from the process-wide PowerMonitor
> directly,
> > > so that we avoid sending messages that have no receiver?
> > 
> > That's a good point. I've added a guard on in_browser_process around the
> > PowerMonitor creation in this file.
> 
> That's not exactly what I had in mind... On Android (or with --single-process)
> GpuMain isn't called at all. Instead a GpuChildThread is created from
> GpuMainThread in gpu_process_host.cc (from the GpuProcessHost creation). That
> one won't have a filter, which is probably ok, since you don't want to forward
> the state from the existing PowerMonitor to itself.
> However, the GpuProcessHostUIShim will still be created, and with it the
> PowerMonitorMessageBroadcaster, which will send the messages when there's no
> receiver.

Thanks for the clarification. I've moved the check into the GpuProcessHostUiShim
instead. The rest of your feedback should now be addressed. Any further
comments?

As a reminder, still need an owners LG on:
/base
/content
/content/app
/content/browser
/content/common

[piman, 2013-07-15 20:51:29]

Mostly nits at this point

https://codereview.chromium.org/17074009/diff/172001/base/power_monitor/power...
File base/power_monitor/power_monitor_source.h (right):

https://codereview.chromium.org/17074009/diff/172001/base/power_monitor/power...
base/power_monitor/power_monitor_source.h:56: Lock lock_;
On 2013/07/13 00:33:20, bajones wrote:
> On 2013/07/12 21:09:58, piman wrote:
> > What does the lock protect? Can you document that?

ping.

https://codereview.chromium.org/17074009/diff/192001/base/power_monitor/power...
File base/power_monitor/power_monitor_source.cc (right):

https://codereview.chromium.org/17074009/diff/192001/base/power_monitor/power...
base/power_monitor/power_monitor_source.cc:39: source->lock_.Acquire();
nit: if you want, it's a common pattern to create a scope and use an autolock:

{
  AutoLock lock(source->lock_);
  if (source->on_battery_power_ != new_on_battery_power) {
    changed = true;
    source->on_battery_power_ = new_on_battery_power;
  }
}

https://codereview.chromium.org/17074009/diff/192001/base/power_monitor/power...
File base/power_monitor/power_monitor_source.h (right):

https://codereview.chromium.org/17074009/diff/192001/base/power_monitor/power...
base/power_monitor/power_monitor_source.h:44: // the UI thread or, in child
processes, the IO thread.
With your changes, it is now called on the main thread, which is also the thread
it gets created on. Do you still need the lock?

https://codereview.chromium.org/17074009/diff/192001/content/browser/gpu/gpu_...
File content/browser/gpu/gpu_process_host_ui_shim.h (right):

https://codereview.chromium.org/17074009/diff/192001/content/browser/gpu/gpu_...
content/browser/gpu/gpu_process_host_ui_shim.h:84: explicit
GpuProcessHostUIShim(int host_id, bool single_process);
nit: no need for explicit any more.

[bajones, 2013-07-15 22:27:17]

Nits addressed

https://codereview.chromium.org/17074009/diff/192001/base/power_monitor/power...
File base/power_monitor/power_monitor_source.h (right):

https://codereview.chromium.org/17074009/diff/192001/base/power_monitor/power...
base/power_monitor/power_monitor_source.h:44: // the UI thread or, in child
processes, the IO thread.
On 2013/07/15 20:51:30, piman wrote:
> With your changes, it is now called on the main thread, which is also the
thread
> it gets created on. Do you still need the lock?

The lock is for IsOnBatteryPower(), which (as the comment says) can be called on
any thread. This was behavior that was documented via comment (but not enforced
by the code!) prior to this patch and thus I'm trying to preserve it.
Documentation has been added and the name changed for clarity.

[piman, 2013-07-15 22:43:36]

LGTM for content/, thanks.

[bajones, 2013-07-15 23:42:06]

On 2013/07/15 22:43:36, piman wrote:
> LGTM for content/, thanks.

Thank you!

@jar, Additional unit tests have been added as per your request and the code's
previous "owner" (vandebo) has given his LGTM. Any chance you can rubber stamp
this for base/?

[jar (doing other things), 2013-07-16 17:15:32]

The refactor plus changes made the review more difficult... so I'm mostly
depending on Vandebo's more detailed review (and my lame attempt to scan/read)
to rubber stamp: base LGTM

Please fix the nits before landing.

Thanks.

https://codereview.chromium.org/17074009/diff/161001/base/power_monitor/power...
File base/power_monitor/power_monitor_ios.mm (left):

https://codereview.chromium.org/17074009/diff/161001/base/power_monitor/power...
base/power_monitor/power_monitor_ios.mm:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
Please be sure that after you land... these files vanish. (I'm not sure how the
CL review tool is displaying this).

https://codereview.chromium.org/17074009/diff/208002/base/power_monitor/power...
File base/power_monitor/power_monitor_device_source_win.cc (right):

https://codereview.chromium.org/17074009/diff/208002/base/power_monitor/power...
base/power_monitor/power_monitor_device_source_win.cc:100: HWND hwnd, UINT
message, WPARAM wparam, LPARAM lparam) {
nit: when wrapping lines for declarations or definitions, put one arg on each
line.  Same on line 115

https://codereview.chromium.org/17074009/diff/208002/base/power_monitor/power...
File base/power_monitor/power_monitor_test_base.cc (right):

https://codereview.chromium.org/17074009/diff/208002/base/power_monitor/power...
base/power_monitor/power_monitor_test_base.cc:36: bool
PowerMonitorTestSource::IsOnBatteryPowerImpl() {
nit: undent

https://codereview.chromium.org/17074009/diff/208002/content/browser/gpu/gpu_...
File content/browser/gpu/gpu_process_host_ui_shim.cc (right):

https://codereview.chromium.org/17074009/diff/208002/content/browser/gpu/gpu_...
content/browser/gpu/gpu_process_host_ui_shim.cc:115: bool single_process) {
nit: align argument indentation

https://codereview.chromium.org/17074009/diff/208002/content/common/power_mon...
File content/common/power_monitor_broadcast_source.cc (right):

https://codereview.chromium.org/17074009/diff/208002/content/common/power_mon...
content/common/power_monitor_broadcast_source.cc:15: message_filter_ = new
PowerMessageFilter(weak_factory_.GetWeakPtr(),
nit: undent 2

https://codereview.chromium.org/17074009/diff/208002/content/common/power_mon...
File content/common/power_monitor_broadcast_source.h (right):

https://codereview.chromium.org/17074009/diff/208002/content/common/power_mon...
content/common/power_monitor_broadcast_source.h:22: PowerMonitorBroadcastSource(
nit: explicit

https://codereview.chromium.org/17074009/diff/208002/content/common/power_mon...
content/common/power_monitor_broadcast_source.h:33: explicit PowerMessageFilter(
nit: no need for explicit

https://codereview.chromium.org/17074009/diff/208002/content/common/power_mon...
File content/common/power_monitor_message_broadcaster.cc (right):

https://codereview.chromium.org/17074009/diff/208002/content/common/power_mon...
content/common/power_monitor_message_broadcaster.cc:14: IPC::Sender* sender) :
sender_(sender) {
nit: When wrapping constructor definition (for any reason), each member
initializer goes on a separate line

https://codereview.chromium.org/17074009/diff/208002/content/gpu/gpu_main.cc
File content/gpu/gpu_main.cc (right):

https://codereview.chromium.org/17074009/diff/208002/content/gpu/gpu_main.cc#...
content/gpu/gpu_main.cc:179:
power_monitor_source.PassAs<base::PowerMonitorSource>()));
nit: undent 2

[commit-bot: I haz the power, 2013-07-16 18:03:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/17074009/195003

[commit-bot: I haz the power, 2013-07-16 20:53:22]

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

[jam, 2013-07-17 00:29:41]

drive by. main points are that it seems the broadcaster is in the wrong
directory, and it's the same amount of effort to hook up all child processes

https://codereview.chromium.org/17074009/diff/195003/content/browser/gpu/gpu_...
File content/browser/gpu/gpu_process_host_ui_shim.cc (right):

https://codereview.chromium.org/17074009/diff/195003/content/browser/gpu/gpu_...
content/browser/gpu/gpu_process_host_ui_shim.cc:110:
power_monitor_broadcaster_.reset(new PowerMonitorMessageBroadcaster(this));
since this is a generic class, it should be attached to
BrowserChildProcessHostImpl and to RenderProcessHostImpl. that way it'll work
for all child processes.

see TraceMessageFilter, ProfilerMessageFilter, HistogramMessageFilter etc as
examples

https://codereview.chromium.org/17074009/diff/195003/content/common/power_mon...
File content/common/power_monitor_message_broadcaster.cc (right):

https://codereview.chromium.org/17074009/diff/195003/content/common/power_mon...
content/common/power_monitor_message_broadcaster.cc:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
why is this file in content/common instead of content/browser?

https://codereview.chromium.org/17074009/diff/195003/content/gpu/gpu_main.cc
File content/gpu/gpu_main.cc (right):

https://codereview.chromium.org/17074009/diff/195003/content/gpu/gpu_main.cc#...
content/gpu/gpu_main.cc:178: scoped_ptr<PowerMonitorBroadcastSource>
power_monitor_source(
this should be connected in all processes (i.e. in ChildProcess or ChildThread)
since you put this class in content/common

[bajones, 2013-07-19 22:44:55]

https://codereview.chromium.org/17074009/diff/195003/content/browser/gpu/gpu_...
File content/browser/gpu/gpu_process_host_ui_shim.cc (right):

https://codereview.chromium.org/17074009/diff/195003/content/browser/gpu/gpu_...
content/browser/gpu/gpu_process_host_ui_shim.cc:110:
power_monitor_broadcaster_.reset(new PowerMonitorMessageBroadcaster(this));
On 2013/07/17 00:29:42, jam wrote:
> since this is a generic class, it should be attached to
> BrowserChildProcessHostImpl and to RenderProcessHostImpl. that way it'll work
> for all child processes.
> 
> see TraceMessageFilter, ProfilerMessageFilter, HistogramMessageFilter etc as
> examples

I generally agree with this sentiment, but as apatrick pointed out earlier on
(comment #15) the power monitor notifications are created on the UI thread and
thus should be handled there, but the BrowserChildProcessHostImpl is created on
the IO thread. As such it seems to me that the current location in the
GpuProcessHostUIShim is more appropriate (RenderProcessHostImpl is UI thread, so
that should be okay.) Thoughts?

https://codereview.chromium.org/17074009/diff/195003/content/common/power_mon...
File content/common/power_monitor_message_broadcaster.cc (right):

https://codereview.chromium.org/17074009/diff/195003/content/common/power_mon...
content/common/power_monitor_message_broadcaster.cc:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
On 2013/07/17 00:29:42, jam wrote:
> why is this file in content/common instead of content/browser?

Good catch, thanks. A much earlier version of the code required this to be in
common (see comment 3), but that's no longer the case. Moved.

[jam, 2013-07-20 00:28:54]

https://codereview.chromium.org/17074009/diff/195003/content/browser/gpu/gpu_...
File content/browser/gpu/gpu_process_host_ui_shim.cc (right):

https://codereview.chromium.org/17074009/diff/195003/content/browser/gpu/gpu_...
content/browser/gpu/gpu_process_host_ui_shim.cc:110:
power_monitor_broadcaster_.reset(new PowerMonitorMessageBroadcaster(this));
On 2013/07/19 22:44:56, bajones wrote:
> On 2013/07/17 00:29:42, jam wrote:
> > since this is a generic class, it should be attached to
> > BrowserChildProcessHostImpl and to RenderProcessHostImpl. that way it'll
work
> > for all child processes.
> > 
> > see TraceMessageFilter, ProfilerMessageFilter, HistogramMessageFilter etc as
> > examples
> 
> I generally agree with this sentiment, but as apatrick pointed out earlier on
> (comment #15) the power monitor notifications are created on the UI thread and
> thus should be handled there, but the BrowserChildProcessHostImpl is created
on
> the IO thread. As such it seems to me that the current location in the
> GpuProcessHostUIShim is more appropriate (RenderProcessHostImpl is UI thread,
so
> that should be okay.) Thoughts?

There are two kinds of child process hosts: the renderer one lives on the UI
thread, and all the other process live on the IO thread. If you just do RPHImpl
and GPUProcessHostUIShim, then these power messages won't be responded to for
all non-renderer and non-gpu child processes. It would be confusing to have such
an API which doesn't work in other processes.

If you create a message filter (like the above examples), you can make it
internally register ot the power notifications on the UI thread. one way to do
this would be to have a refcounted inner class which is owned by the filter. at
construction of the filter, it creates the inner class. in the inner class's
constructor, it posts a task to the UI thread to do the necessary registration.
It can use the DeleteOnUIThread trait to ensure it's destructed on the UI
thread. when the filter goes away, its release of the inner class will make it
destroy on the UI thread, and it can unregister there. the inner class can hold
on to a naked pointer to the filter, which is only accessed on the IO thread, to
send IPCs. the filter can null it out on the IO thread when it gets
OnChannelClosing()

[bajones, 2013-07-29 20:54:34]

All the child processes are wired up now. @jam: I'd appreciate it if you could
take a look and make sure this is what you had in mind.

Additionally it's worth noting that the default constructor for PowerMonitor has
been removed, since in order to make the latest changes I was required to touch
every file that the default constructor was used by anyway.

[piman, 2013-07-29 21:18:17]

https://codereview.chromium.org/17074009/diff/254001/content/browser/power_mo...
File content/browser/power_monitor_message_broadcaster.cc (right):

https://codereview.chromium.org/17074009/diff/254001/content/browser/power_mo...
content/browser/power_monitor_message_broadcaster.cc:19:
base::Unretained(this)));
What prevents the filter from being destroyed before this executes on the UI
thread?

https://codereview.chromium.org/17074009/diff/254001/content/browser/power_mo...
content/browser/power_monitor_message_broadcaster.cc:33:
base::Unretained(this)));
What prevents the filter from being destroyed before this executes on the UI
thread?

https://codereview.chromium.org/17074009/diff/254001/content/browser/power_mo...
content/browser/power_monitor_message_broadcaster.cc:61:
power_monitor->RemoveObserver(this);
Because this class is refcounted, it may be destroyed on a different thread
(e.g. IO thread since the PowerMonitorMessageFilter probably has the last
reference) than the one that created it (the UI thread).
Calling RemoveObserver from a different thread than the one that called
AddObserver isn't supported (it would be racy anyway).

https://codereview.chromium.org/17074009/diff/254001/content/browser/power_mo...
content/browser/power_monitor_message_broadcaster.cc:72: sender_->Send(new
PowerMonitorMsg_PowerStateChange(on_battery_power));
This is unsafe: sender_ is the message filter, which can only send from the IO
thread.

https://codereview.chromium.org/17074009/diff/254001/content/browser/power_mo...
File content/browser/power_monitor_message_broadcaster.h (right):

https://codereview.chromium.org/17074009/diff/254001/content/browser/power_mo...
content/browser/power_monitor_message_broadcaster.h:50: class
PowerMonitorMessageFilter : public BrowserMessageFilter {
At a high level, I'm not sure why you create a filter to own the broadcaster.
Why not directly create the broadcaster in the process hosts (which already know
how to send IPCs)?

https://codereview.chromium.org/17074009/diff/254001/content/child/power_moni...
File content/child/power_monitor_broadcast_source.cc (right):

https://codereview.chromium.org/17074009/diff/254001/content/child/power_moni...
content/child/power_monitor_broadcast_source.cc:23: // not need to retain a
reference.
nit: ... but it does anyway, making this comment misleading more than anything
else. Just remove?

[bajones, 2013-07-29 21:33:35]

https://codereview.chromium.org/17074009/diff/254001/content/browser/power_mo...
File content/browser/power_monitor_message_broadcaster.cc (right):

https://codereview.chromium.org/17074009/diff/254001/content/browser/power_mo...
content/browser/power_monitor_message_broadcaster.cc:33:
base::Unretained(this)));
On 2013/07/29 21:18:18, piman wrote:
> What prevents the filter from being destroyed before this executes on the UI
> thread?

I'll switch this and the prior post task over to pass a weak reference.

https://codereview.chromium.org/17074009/diff/254001/content/browser/power_mo...
content/browser/power_monitor_message_broadcaster.cc:61:
power_monitor->RemoveObserver(this);
On 2013/07/29 21:18:18, piman wrote:
> Because this class is refcounted, it may be destroyed on a different thread
> (e.g. IO thread since the PowerMonitorMessageFilter probably has the last
> reference) than the one that created it (the UI thread).
> Calling RemoveObserver from a different thread than the one that called
> AddObserver isn't supported (it would be racy anyway).

Please see the class declaration. The RefCountedThreadSafe superclass is
declared with a BrowserThread::DeleteOnUIThread deleter.

https://codereview.chromium.org/17074009/diff/254001/content/browser/power_mo...
content/browser/power_monitor_message_broadcaster.cc:72: sender_->Send(new
PowerMonitorMsg_PowerStateChange(on_battery_power));
On 2013/07/29 21:18:18, piman wrote:
> This is unsafe: sender_ is the message filter, which can only send from the IO
> thread.

I just realized that by allowing the constructor to take any IPC::Sender I can't
enforce this in the current code, but the current uses of this should be safe.
There are only two senders that are provided to this class currently: The
RenderProcessHostImpl, which is explicitly lives on the UI thread, and the
PowerMonitorMessageFilter, which derives from a BrowserMessageFilter which
explicitly states that it's Send can be called from any thread.

I should change the constructor to make this more explicit, though. It may be
sufficient to create to constructors that take in the aforementioned classes
explicitly.

https://codereview.chromium.org/17074009/diff/254001/content/browser/power_mo...
File content/browser/power_monitor_message_broadcaster.h (right):

https://codereview.chromium.org/17074009/diff/254001/content/browser/power_mo...
content/browser/power_monitor_message_broadcaster.h:50: class
PowerMonitorMessageFilter : public BrowserMessageFilter {
On 2013/07/29 21:18:18, piman wrote:
> At a high level, I'm not sure why you create a filter to own the broadcaster.
> Why not directly create the broadcaster in the process hosts (which already
know
> how to send IPCs)?

Please see @jam's two previous comments.

[piman, 2013-07-29 21:40:49]

https://codereview.chromium.org/17074009/diff/254001/content/browser/power_mo...
File content/browser/power_monitor_message_broadcaster.cc (right):

https://codereview.chromium.org/17074009/diff/254001/content/browser/power_mo...
content/browser/power_monitor_message_broadcaster.cc:33:
base::Unretained(this)));
On 2013/07/29 21:33:36, bajones wrote:
> On 2013/07/29 21:18:18, piman wrote:
> > What prevents the filter from being destroyed before this executes on the UI
> > thread?
> 
> I'll switch this and the prior post task over to pass a weak reference.

That won't work.
WeakPtrs are only safe if the instance is deleted on the same thread as where
it's dereferenced.
In this case PowerMonitorMessageFilter will (likely) be deleted on the IO
thread, and you'll need to dereference the weakptr on the UI thread.

Also, you still have the issue that PowerMonitorMessageBroadcaster may try to
access sender_ from the UI thread, and you have no way to ensure it's not
deleted.

[bajones, 2013-07-29 23:31:38]

On 2013/07/29 21:40:49, piman wrote:
>
https://codereview.chromium.org/17074009/diff/254001/content/browser/power_mo...
> File content/browser/power_monitor_message_broadcaster.cc (right):
> 
>
https://codereview.chromium.org/17074009/diff/254001/content/browser/power_mo...
> content/browser/power_monitor_message_broadcaster.cc:33:
> base::Unretained(this)));
> On 2013/07/29 21:33:36, bajones wrote:
> > On 2013/07/29 21:18:18, piman wrote:
> > > What prevents the filter from being destroyed before this executes on the
UI
> > > thread?
> > 
> > I'll switch this and the prior post task over to pass a weak reference.
> 
> That won't work.
> WeakPtrs are only safe if the instance is deleted on the same thread as where
> it's dereferenced.
> In this case PowerMonitorMessageFilter will (likely) be deleted on the IO
> thread, and you'll need to dereference the weakptr on the UI thread.
> 
> Also, you still have the issue that PowerMonitorMessageBroadcaster may try to
> access sender_ from the UI thread, and you have no way to ensure it's not
> deleted.

I've refactored this now so that sender_ (now filter_) is only accessed from the
IO thread, but I'm at a bit of a loss for how to safely create the Broadcaster
on the UI thread and still associate it with the IO-thread-resident Filter.
What's the recommended pattern here?

[bajones, 2013-07-30 17:27:27]

Talked with @kbr about it this morning and I think the threading concerns should
now be addressed. @piman, could you take another look? Also, I would still
appreciate a quick review from @jam to ensure that your concerns are addressed
as well.

https://codereview.chromium.org/17074009/diff/263001/content/browser/power_mo...
File content/browser/power_monitor_message_filter.cc (right):

https://codereview.chromium.org/17074009/diff/263001/content/browser/power_mo...
content/browser/power_monitor_message_filter.cc:18: this));
PowerMonitorMessageFilter is a RefCountedThreadSafe class, as such my
understanding is that this should now be safe. (Removed base::Unretained, which
was a copy/paste bug) Is this Correct?

https://codereview.chromium.org/17074009/diff/263001/content/browser/power_mo...
content/browser/power_monitor_message_filter.cc:87: if (filter_)
With the exception of setting filter_ in the constructor all access to filter_
now happens on the IO thread.

[piman, 2013-07-30 19:38:48]

https://codereview.chromium.org/17074009/diff/263001/content/browser/power_mo...
File content/browser/power_monitor_message_filter.cc (right):

https://codereview.chromium.org/17074009/diff/263001/content/browser/power_mo...
content/browser/power_monitor_message_filter.cc:26: if (broadcaster_)
This is on the IO thread, it races with CreateBroadcaster, does it not?

Taking a step back, do you really need a filter and a broadcaster? Can't you
have a single class that lives on the IO thread and registers for power
notifications there (since it's an ObserverListThreadSafe) - assuming the
PowerMonitor outlives the IO thread?
How different would that be from PowerMonitorMessageBroadcaster?

[bajones, 2013-07-30 21:45:08]

Talked with @piman and @jam offline to clarify a couple of points. New code is
much simpler and thread safe, since everything in the broadcaster is piped
through the ObserverListThreadSafe.

[piman, 2013-07-30 21:55:51]

lgtm

[commit-bot: I haz the power, 2013-07-30 22:01:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/17074009/280001

[bajones, 2013-07-30 23:41:08]

So it turns out that after the most recent set of changes I need one more LGTM
because I moved the power monitor/high res timer monitor out of nacl_main and
into child_thread.

@bradchen, @bradnelson, @jvoung?

[jam, 2013-07-30 23:51:59]

https://codereview.chromium.org/17074009/diff/280001/content/browser/power_mo...
File content/browser/power_monitor_message_broadcaster.h (right):

https://codereview.chromium.org/17074009/diff/280001/content/browser/power_mo...
content/browser/power_monitor_message_broadcaster.h:17: class Sender;
nit: no need, since you're including browser_message_filter which by definition
derives from this. also extra blank line above

https://codereview.chromium.org/17074009/diff/280001/content/browser/power_mo...
content/browser/power_monitor_message_broadcaster.h:24: // thread.
when you say all interactions, do you mean also construction? because
BrowserChildProcessHostImpl is creating this on the IO thread

https://codereview.chromium.org/17074009/diff/280001/content/browser/power_mo...
content/browser/power_monitor_message_broadcaster.h:27: public:
nit: here and below, space before public

https://codereview.chromium.org/17074009/diff/280001/content/child/power_moni...
File content/child/power_monitor_broadcast_source.cc (right):

https://codereview.chromium.org/17074009/diff/280001/content/child/power_moni...
content/child/power_monitor_broadcast_source.cc:64: return handled && msg_is_ok;
nit: take out the msg_is_ok stuff. this is only needed in the browser, and in
which case we kill the child process. it's not need here since the browser is
trusted. also, OnMessageHandled should return true if it takes the IPC,
irrespective of whether there was a problem parsing it or not

https://codereview.chromium.org/17074009/diff/280001/content/child/power_moni...
File content/child/power_monitor_broadcast_source.h (right):

https://codereview.chromium.org/17074009/diff/280001/content/child/power_moni...
content/child/power_monitor_broadcast_source.h:26:
IPC::ChannelProxy::MessageFilter* MessageFilter();
nit: GetMessageFilter

https://codereview.chromium.org/17074009/diff/280001/content/child/power_moni...
content/child/power_monitor_broadcast_source.h:31: class PowerMessageFilter :
public IPC::ChannelProxy::MessageFilter {
nit: since this is a private class, just move the whole declaration/definition
to the cc file and you can make this header faster to read

[scottmg, 2013-07-30 23:52:53]

https://codereview.chromium.org/17074009/diff/280001/content/child/power_moni...
File content/child/power_monitor_broadcast_source.h (right):

https://codereview.chromium.org/17074009/diff/280001/content/child/power_moni...
content/child/power_monitor_broadcast_source.h:5: #ifndef
CONTENT_COMMON_POWER_MONITOR_BROADCAST_SOURCE_H_
nit; header guard CONTENT_CHILD_...

[bajones, 2013-07-31 00:19:21]

Nits addressed

[jam, 2013-07-31 17:27:16]

https://codereview.chromium.org/17074009/diff/295001/content/child/child_thre...
File content/child/child_thread.cc (right):

https://codereview.chromium.org/17074009/diff/295001/content/child/child_thre...
content/child/child_thread.cc:160: new
PowerMonitorBroadcastSource(message_loop_->message_loop_proxy()));
nit: no need to pass message_loop_ here, PowerMonitorBroadcastSource should just
do MessageLoopProxy::current()

https://codereview.chromium.org/17074009/diff/295001/content/child/power_moni...
File content/child/power_monitor_broadcast_source.cc (right):

https://codereview.chromium.org/17074009/diff/295001/content/child/power_moni...
content/child/power_monitor_broadcast_source.cc:39: on_battery_power));
I didn't know this works?

http://www.chromium.org/developers/design-documents/threading#base_WeakPtr_an...
says 
" NOTE:  This only works when the task is posted to the same thread. Currently
there is not a general solution that works for tasks posted to other threads.
See the next section about CancelableTaskTracker for an alternative solution."

am i misreading?

[bajones, 2013-07-31 19:17:06]

On 2013/07/31 17:27:16, jam wrote:
>
https://codereview.chromium.org/17074009/diff/295001/content/child/child_thre...
> File content/child/child_thread.cc (right):
> 
>
https://codereview.chromium.org/17074009/diff/295001/content/child/child_thre...
> content/child/child_thread.cc:160: new
> PowerMonitorBroadcastSource(message_loop_->message_loop_proxy()));
> nit: no need to pass message_loop_ here, PowerMonitorBroadcastSource should
just
> do MessageLoopProxy::current()
> 
>
https://codereview.chromium.org/17074009/diff/295001/content/child/power_moni...
> File content/child/power_monitor_broadcast_source.cc (right):
> 
>
https://codereview.chromium.org/17074009/diff/295001/content/child/power_moni...
> content/child/power_monitor_broadcast_source.cc:39: on_battery_power));
> I didn't know this works?
> 
>
http://www.chromium.org/developers/design-documents/threading#base_WeakPtr_an...
> says 
> " NOTE:  This only works when the task is posted to the same thread. Currently
> there is not a general solution that works for tasks posted to other threads.
> See the next section about CancelableTaskTracker for an alternative solution."
> 
> am i misreading?

Nope, your reading is correct. Thanks for pointing this out.

The code in question has been refactored in a similar fashion to previous
suggestions for the PowerMonitorMessageBroadcaster. All access to the source_
member (previously a weak pointer, now a raw pointer) is restricted to a single
thread.

Also, review ping from NaCl owners. I need an LG for the minor changes to
nacl_main.cc

[jam, 2013-07-31 21:01:36]

lgtm

[commit-bot: I haz the power, 2013-07-31 21:22:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/17074009/311001

[commit-bot: I haz the power, 2013-07-31 21:23:18]

Failed to apply patch for chrome/nacl/nacl_main.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: chrome/nacl/nacl_main.cc
  |diff --git a/chrome/nacl/nacl_main.cc b/chrome/nacl/nacl_main.cc
  |index
24f96f2c9fea4688bad5e790368cc676f9e1767d..57d8b90b5963775a1998a9b471b6d85a095bb741
100644
  |--- a/chrome/nacl/nacl_main.cc
  |+++ b/chrome/nacl/nacl_main.cc
  --------------------------
  No file to patch.  Skipping patch.
  2 out of 2 hunks ignored

Patch:       chrome/nacl/nacl_main.cc
Index: chrome/nacl/nacl_main.cc
diff --git a/chrome/nacl/nacl_main.cc b/chrome/nacl/nacl_main.cc
index
24f96f2c9fea4688bad5e790368cc676f9e1767d..57d8b90b5963775a1998a9b471b6d85a095bb741
100644
--- a/chrome/nacl/nacl_main.cc
+++ b/chrome/nacl/nacl_main.cc
@@ -6,8 +6,6 @@
 
 #include "base/command_line.h"
 #include "base/message_loop/message_loop.h"
-#include "base/power_monitor/power_monitor.h"
-#include "base/timer/hi_res_timer_manager.h"
 #include "chrome/common/chrome_result_codes.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/logging_chrome.h"
@@ -24,9 +22,6 @@ int NaClMain(const content::MainFunctionParams& parameters) {
   base::MessageLoopForIO main_message_loop;
   base::PlatformThread::SetName("CrNaClMain");
 
-  base::PowerMonitor power_monitor;
-  base::HighResolutionTimerManager hi_res_timer_manager;
-
 #if defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_LINUX)
   NaClMainPlatformDelegate platform(parameters);

[jvoung (off chromium), 2013-07-31 21:24:38]

https://codereview.chromium.org/17074009/diff/311001/chrome/nacl/nacl_main.cc
File chrome/nacl/nacl_main.cc (left):

https://codereview.chromium.org/17074009/diff/311001/chrome/nacl/nacl_main.cc...
chrome/nacl/nacl_main.cc:29: 
Note that nacl_main got moved to components, I think.

I'm not familiar with this code, but bbudge did mention that at some point the
NaCl + Pepper timers on Windows would not give > 16ms timer resolution.  Would
that regress w/ this?

[bajones, 2013-07-31 21:26:33]

On 2013/07/31 21:24:38, jvoung (cr) wrote:
> https://codereview.chromium.org/17074009/diff/311001/chrome/nacl/nacl_main.cc
> File chrome/nacl/nacl_main.cc (left):
> 
>
https://codereview.chromium.org/17074009/diff/311001/chrome/nacl/nacl_main.cc...
> chrome/nacl/nacl_main.cc:29: 
> Note that nacl_main got moved to components, I think.
> 
> I'm not familiar with this code, but bbudge did mention that at some point the
> NaCl + Pepper timers on Windows would not give > 16ms timer resolution.  Would
> that regress w/ this?

This will not change the behavior of timers at all.

[jvoung (off chromium), 2013-07-31 21:31:33]

On 2013/07/31 21:26:33, bajones wrote:
> On 2013/07/31 21:24:38, jvoung (cr) wrote:
> >
https://codereview.chromium.org/17074009/diff/311001/chrome/nacl/nacl_main.cc
> > File chrome/nacl/nacl_main.cc (left):
> > 
> >
>
https://codereview.chromium.org/17074009/diff/311001/chrome/nacl/nacl_main.cc...
> > chrome/nacl/nacl_main.cc:29: 
> > Note that nacl_main got moved to components, I think.
> > 
> > I'm not familiar with this code, but bbudge did mention that at some point
the
> > NaCl + Pepper timers on Windows would not give > 16ms timer resolution. 
Would
> > that regress w/ this?
> 
> This will not change the behavior of timers at all.

Ok, in that case LGTM

[vandebo (ex-Chrome), 2013-07-31 21:44:38]

On 2013/07/31 21:26:33, bajones wrote:
> On 2013/07/31 21:24:38, jvoung (cr) wrote:
> >
https://codereview.chromium.org/17074009/diff/311001/chrome/nacl/nacl_main.cc
> > File chrome/nacl/nacl_main.cc (left):
> > 
> >
>
https://codereview.chromium.org/17074009/diff/311001/chrome/nacl/nacl_main.cc...
> > chrome/nacl/nacl_main.cc:29: 
> > Note that nacl_main got moved to components, I think.
> > 
> > I'm not familiar with this code, but bbudge did mention that at some point
the
> > NaCl + Pepper timers on Windows would not give > 16ms timer resolution. 
Would
> > that regress w/ this?
> 
> This will not change the behavior of timers at all.

Are you sure about that?  HighResoluationTimeManager listens to PowerManager.  I
didn't trace through exactly what effect it has, but I know they are related.

[bajones, 2013-07-31 21:55:07]

On 2013/07/31 21:44:38, vandebo wrote:
> On 2013/07/31 21:26:33, bajones wrote:
> > On 2013/07/31 21:24:38, jvoung (cr) wrote:
> > >
> https://codereview.chromium.org/17074009/diff/311001/chrome/nacl/nacl_main.cc
> > > File chrome/nacl/nacl_main.cc (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/17074009/diff/311001/chrome/nacl/nacl_main.cc...
> > > chrome/nacl/nacl_main.cc:29: 
> > > Note that nacl_main got moved to components, I think.
> > > 
> > > I'm not familiar with this code, but bbudge did mention that at some point
> the
> > > NaCl + Pepper timers on Windows would not give > 16ms timer resolution. 
> Would
> > > that regress w/ this?
> > 
> > This will not change the behavior of timers at all.
> 
> Are you sure about that?  HighResoluationTimeManager listens to PowerManager. 
I
> didn't trace through exactly what effect it has, but I know they are related.

The HighResolutionTimeManager receives battery status messages from the
PowerManager (Windows only, I think) to indicate that the timer should use a
less battery intensive method. This change relocates the
HighResolutionTimeManager to ensure that it can still register as a
PowerObserver, but otherwise does not change it's functionality. Due to the fact
that PowerMonitor messages will now be received via IPC the high res timer may
clock down slightly later than previously.

[commit-bot: I haz the power, 2013-07-31 21:59:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/17074009/321001

[commit-bot: I haz the power, 2013-07-31 22:37:08]

Failed to trigger a try job on win_x64_rel
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-08-01 00:00:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/17074009/339002

[commit-bot: I haz the power, 2013-08-01 02:35:59]

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

[piman, 2013-08-01 18:40:54]

https://codereview.chromium.org/17074009/diff/295001/content/child/power_moni...
File content/child/power_monitor_broadcast_source.cc (right):

https://codereview.chromium.org/17074009/diff/295001/content/child/power_moni...
content/child/power_monitor_broadcast_source.cc:39: on_battery_power));
On 2013/07/31 17:27:17, jam wrote:
> I didn't know this works?
> 
>
http://www.chromium.org/developers/design-documents/threading#base_WeakPtr_an...
> says 
> " NOTE:  This only works when the task is posted to the same thread. Currently
> there is not a general solution that works for tasks posted to other threads.
> See the next section about CancelableTaskTracker for an alternative solution."
> 
> am i misreading?

As long as you only dereference the weakptr on the same thread as the one that
cancels it, you're fine. You can copy the weakptr around.

In this case, the weakptr is cancelled on the thread owning the
PowerMonitorBroadcastSource, which is the same thread that executes the posted
task, so you're fine.

[commit-bot: I haz the power, 2013-08-02 20:14:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/17074009/363001

[commit-bot: I haz the power, 2013-08-02 21:18:44]

Retried try job too often on mac_rel for step(s) remoting_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[bajones, 2013-08-02 22:09:32]

Committed patchset #27 manually as r215381 (presubmit successful).