chromeos: Add support for the battery status API on chromeos.

chromeos: Add support for the battery status API on chromeos.

BUG=122593
TEST=none

[sadrul, 2012-04-09 01:41:06]

ben@: Mostly for content/browser/battery_status/DEPS. The chromeos dependency is
added with the tightest scope, only for battery_status/, and is necessary for
PowerManagerClient::Observer. I am hoping that's acceptable?

darin@: Please review all.

Webkit side of the patch is in the commit-queue: http://wkb.ug/83284

[Ben Goodger (Google), 2012-04-09 19:26:22]

+jam, who might have more time to review this week.

[jam, 2012-04-10 16:09:22]

(just saw this). i stopped after seeing the chromeos include

http://codereview.chromium.org/10024013/diff/1002/content/browser/battery_sta...
File content/browser/battery_status/DEPS (right):

http://codereview.chromium.org/10024013/diff/1002/content/browser/battery_sta...
content/browser/battery_status/DEPS:2: "+chromeos",  # For battery status.
content doesn't depend on chromeos by design, so you'll have to find another way
of having chromeos provide an implementation of a provider of this data. there
are many examples in the content api of interfaces that an embedder implements

[sadrul, 2012-04-10 16:13:04]

On 2012/04/10 16:09:22, John Abd-El-Malek wrote:
> (just saw this). i stopped after seeing the chromeos include
> 
>
http://codereview.chromium.org/10024013/diff/1002/content/browser/battery_sta...
> File content/browser/battery_status/DEPS (right):
> 
>
http://codereview.chromium.org/10024013/diff/1002/content/browser/battery_sta...
> content/browser/battery_status/DEPS:2: "+chromeos",  # For battery status.
> content doesn't depend on chromeos by design, so you'll have to find another
way
> of having chromeos provide an implementation of a provider of this data. there
> are many examples in the content api of interfaces that an embedder implements

I will find another way (was hoping a chromeos-only feature would be OK to
include chromeos dependency).

This 'other way' will affect only the files inside browser/bettery_status/, the
rest of the code should remain very similar. Can you please review the rest and
let me know if you have suggestions about them? Thanks!

[sadrul, 2012-04-10 20:00:58]

Updated patch to remove chromeos dependency. PTAL.

[jam, 2012-04-10 22:58:37]

having these chromeos ifdefs in content is best avoided. chromeos should build
on content, and content shouldn't really know what chromeos needs.

are you certain this will only be used by chromeos, and not chrome for android
for example?

if you're sure, then this can be implemented completely in chrome without
touching content. see how chrome implements autofill/spellcheck/permission
Client interfaces in
http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/...

instead of having webkit ask its embedder for an interface, the embedder can set
it. that way chrome, on render view initialization, can set its dispatcher
class.

[sadrul, 2012-04-10 23:08:08]

On 2012/04/10 22:58:37, John Abd-El-Malek wrote:
> having these chromeos ifdefs in content is best avoided. chromeos should build
> on content, and content shouldn't really know what chromeos needs.
> 
> are you certain this will only be used by chromeos, and not chrome for android
> for example?

This can indeed be used by android. Once the necessary plumbing to get this info
from the system is done on android, presumably the ifdef check can simply switch
from OS_CHROMEOS to OS_CHROMEOS || OS_ANDROID? Does that sound reasonable, or
would you rather we start off with chromeos and android? Or maybe even a new
flag HAVE_BATTERY_STATUS check for this (although that's probably overkill?)

> if you're sure, then this can be implemented completely in chrome without
> touching content. see how chrome implements autofill/spellcheck/permission
> Client interfaces in
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/...
> 
> instead of having webkit ask its embedder for an interface, the embedder can
set
> it. that way chrome, on render view initialization, can set its dispatcher
> class.

[jam, 2012-04-11 00:50:49]

ok, since it's not just chromeos specific (even if we only have cros impl for
now), and this is part of the web platform, it shouldn't be behind chromeos
ifdefs in content now (we try to avoid these as much as possible). since the
code size is minimal, is there a harm of always adding it? or do you want to
return null in javascript if there's no implementation? (if so, then a
battery_status ifdef sounds better to me)

also, please run checkdeps (which the trybots do). you'll see errors about
includes. chrome can only include the public API of content. see
http://www.chromium.org/developers/content-module/content-api for more
information.

[sadrul, 2012-04-11 07:06:50]

On 2012/04/11 00:50:49, John Abd-El-Malek wrote:
> ok, since it's not just chromeos specific (even if we only have cros impl for
> now), and this is part of the web platform, it shouldn't be behind chromeos
> ifdefs in content now (we try to avoid these as much as possible). since the
> code size is minimal, is there a harm of always adding it? or do you want to
> return null in javascript if there's no implementation? (if so, then a
> battery_status ifdef sounds better to me)

I removed the ifdefs, so this is now included in all platforms (returning NULL
is not necessary for platforms without an implementation; the corresponding
implementation in webkit returns spec-appropriate values to JS until an
implementation from the browser sends an actual update).

> also, please run checkdeps (which the trybots do). you'll see errors about
> includes. chrome can only include the public API of content. see
> http://www.chromium.org/developers/content-module/content-api for more
> information.

Ah, thanks. I have moved the provider into public API.

I am still unsure if that takes care of checkdeps, because I have been waiting
for the webkit side of the patch to land and be gardened, and running checkdeps
locally shows a whole bunch of other errors in a whole bunch of other places.
Once the webkit patch is gardened in, I will run it through the trybots.

[sadrul, 2012-04-11 20:45:47]

On 2012/04/11 07:06:50, sadrul wrote:
> On 2012/04/11 00:50:49, John Abd-El-Malek wrote:
> > ok, since it's not just chromeos specific (even if we only have cros impl
for
> > now), and this is part of the web platform, it shouldn't be behind chromeos
> > ifdefs in content now (we try to avoid these as much as possible). since the
> > code size is minimal, is there a harm of always adding it? or do you want to
> > return null in javascript if there's no implementation? (if so, then a
> > battery_status ifdef sounds better to me)
> 
> I removed the ifdefs, so this is now included in all platforms (returning NULL
> is not necessary for platforms without an implementation; the corresponding
> implementation in webkit returns spec-appropriate values to JS until an
> implementation from the browser sends an actual update).
> 
> > also, please run checkdeps (which the trybots do). you'll see errors about
> > includes. chrome can only include the public API of content. see
> > http://www.chromium.org/developers/content-module/content-api for more
> > information.
> 
> Ah, thanks. I have moved the provider into public API.
> 
> I am still unsure if that takes care of checkdeps, because I have been waiting
> for the webkit side of the patch to land and be gardened, and running
checkdeps
> locally shows a whole bunch of other errors in a whole bunch of other places.
> Once the webkit patch is gardened in, I will run it through the trybots.

check_deps is uncomfortably green! PTAL.

[Ben Goodger (Google), 2012-04-11 21:09:11]

http://codereview.chromium.org/10024013/diff/12001/chrome/browser/battery_sta...
File chrome/browser/battery_status_provider_chromeos.h (right):

http://codereview.chromium.org/10024013/diff/12001/chrome/browser/battery_sta...
chrome/browser/battery_status_provider_chromeos.h:5: #ifndef
CHROME_BROWSER_BATTERY_STATUS_PROVIDER_CHROMEOS_H_
-> chrome/browser/chromeos at least.

[sadrul, 2012-04-11 21:50:54]

http://codereview.chromium.org/10024013/diff/12001/chrome/browser/battery_sta...
File chrome/browser/battery_status_provider_chromeos.h (right):

http://codereview.chromium.org/10024013/diff/12001/chrome/browser/battery_sta...
chrome/browser/battery_status_provider_chromeos.h:5: #ifndef
CHROME_BROWSER_BATTERY_STATUS_PROVIDER_CHROMEOS_H_
On 2012/04/11 21:09:11, Ben Goodger (Google) wrote:
> -> chrome/browser/chromeos at least.

Done.

[jam, 2012-04-12 01:55:09]

(looked through most of this, but getting off shuttle so I figured I'd send
these ones now)

http://codereview.chromium.org/10024013/diff/15002/content/common/battery_sta...
File content/common/battery_status_messages.h (right):

http://codereview.chromium.org/10024013/diff/15002/content/common/battery_sta...
content/common/battery_status_messages.h:25: // a blot that should be cast to
WebBatteryStatus.
we generally don't send structs this way over ipc. it's pretty easy to use
IPC_STRUCT_TRAITS_BEGIN/IPC_STRUCT_TRAITS_MEMBER to create the param traits for
this struct. see view_messages.h for a lot of examples

http://codereview.chromium.org/10024013/diff/15002/content/public/browser/bat...
File content/public/browser/battery_status_provider.h (right):

http://codereview.chromium.org/10024013/diff/15002/content/public/browser/bat...
content/public/browser/battery_status_provider.h:16: struct CONTENT_EXPORT
PowerSupplyStatus {
the content api, like the webkit api, puts each struct/enum/class in a separate
file. so this would be in power_supply_status.h

http://codereview.chromium.org/10024013/diff/15002/content/public/browser/bat...
content/public/browser/battery_status_provider.h:33: class CONTENT_EXPORT
Observer {
this class is only used by content, so it doesn't belong on the public api

http://codereview.chromium.org/10024013/diff/15002/content/public/browser/bat...
content/public/browser/battery_status_provider.h:40: void AddObserver(Observer*
observer);
for the content API we follow webkit API and only put interfaces in the public
directory. in scontent/browser you would have BatteryStatusProviderImpl that
provides an implementation of this interface

[sadrul, 2012-04-12 05:20:09]

Thanks for the excellent advice. I have made the suggested changes. PTAL.

http://codereview.chromium.org/10024013/diff/15002/content/common/battery_sta...
File content/common/battery_status_messages.h (right):

http://codereview.chromium.org/10024013/diff/15002/content/common/battery_sta...
content/common/battery_status_messages.h:25: // a blot that should be cast to
WebBatteryStatus.
On 2012/04/12 01:55:10, John Abd-El-Malek wrote:
> we generally don't send structs this way over ipc. it's pretty easy to use
> IPC_STRUCT_TRAITS_BEGIN/IPC_STRUCT_TRAITS_MEMBER to create the param traits
for
> this struct. see view_messages.h for a lot of examples

Done.

http://codereview.chromium.org/10024013/diff/15002/content/public/browser/bat...
File content/public/browser/battery_status_provider.h (right):

http://codereview.chromium.org/10024013/diff/15002/content/public/browser/bat...
content/public/browser/battery_status_provider.h:16: struct CONTENT_EXPORT
PowerSupplyStatus {
On 2012/04/12 01:55:10, John Abd-El-Malek wrote:
> the content api, like the webkit api, puts each struct/enum/class in a
separate
> file. so this would be in power_supply_status.h

Done.

http://codereview.chromium.org/10024013/diff/15002/content/public/browser/bat...
content/public/browser/battery_status_provider.h:33: class CONTENT_EXPORT
Observer {
On 2012/04/12 01:55:10, John Abd-El-Malek wrote:
> this class is only used by content, so it doesn't belong on the public api

Done.

http://codereview.chromium.org/10024013/diff/15002/content/public/browser/bat...
content/public/browser/battery_status_provider.h:40: void AddObserver(Observer*
observer);
On 2012/04/12 01:55:10, John Abd-El-Malek wrote:
> for the content API we follow webkit API and only put interfaces in the public
> directory. in scontent/browser you would have BatteryStatusProviderImpl that
> provides an implementation of this interface

Done.

[jam, 2012-04-12 19:34:55]

http://codereview.chromium.org/10024013/diff/24007/content/browser/battery_st...
File content/browser/battery_status/battery_status_message_filter.h (right):

http://codereview.chromium.org/10024013/diff/24007/content/browser/battery_st...
content/browser/battery_status/battery_status_message_filter.h:13: class
BatteryStatusMessageFilter : public content::BrowserMessageFilter {
nit: for this class and the other new class, please put it in the content
namespace

http://codereview.chromium.org/10024013/diff/24007/content/browser/battery_st...
content/browser/battery_status/battery_status_message_filter.h:29: // The list
of renderers that want battery status updates.
nit: renderers-> RenderViews

http://codereview.chromium.org/10024013/diff/24007/content/browser/battery_st...
File content/browser/battery_status/battery_status_provider_impl.cc (right):

http://codereview.chromium.org/10024013/diff/24007/content/browser/battery_st...
content/browser/battery_status/battery_status_provider_impl.cc:19:
observers_.AddObserver(obs);
nit: here, and in RemoveObserver and SendUpdate, please add
DCHECK(BrowserThread::CurrentlyOn...) to ensure that this is happening on the
thread that you expect

http://codereview.chromium.org/10024013/diff/24007/content/browser/battery_st...
File content/browser/battery_status/battery_status_provider_impl.h (right):

http://codereview.chromium.org/10024013/diff/24007/content/browser/battery_st...
content/browser/battery_status/battery_status_provider_impl.h:8: #include <list>
nit: not used

http://codereview.chromium.org/10024013/diff/24007/content/browser/battery_st...
content/browser/battery_status/battery_status_provider_impl.h:31: virtual void
SendUpdate(
nit: why make this private? i'd put it in public, since it's odd that someone
has to cast this to the base interface to call this function (say if android's
impl is in content)

http://codereview.chromium.org/10024013/diff/24007/content/public/browser/bat...
File content/public/browser/battery_status_provider.h (right):

http://codereview.chromium.org/10024013/diff/24007/content/public/browser/bat...
content/public/browser/battery_status_provider.h:11: #include
"base/observer_list.h"
nit: get rid of the three includes above this

[sadrul, 2012-04-12 21:53:24]

http://codereview.chromium.org/10024013/diff/24007/content/browser/battery_st...
File content/browser/battery_status/battery_status_message_filter.h (right):

http://codereview.chromium.org/10024013/diff/24007/content/browser/battery_st...
content/browser/battery_status/battery_status_message_filter.h:13: class
BatteryStatusMessageFilter : public content::BrowserMessageFilter {
On 2012/04/12 19:34:55, John Abd-El-Malek wrote:
> nit: for this class and the other new class, please put it in the content
> namespace

Done.

http://codereview.chromium.org/10024013/diff/24007/content/browser/battery_st...
content/browser/battery_status/battery_status_message_filter.h:29: // The list
of renderers that want battery status updates.
On 2012/04/12 19:34:55, John Abd-El-Malek wrote:
> nit: renderers-> RenderViews

Done.

http://codereview.chromium.org/10024013/diff/24007/content/browser/battery_st...
File content/browser/battery_status/battery_status_provider_impl.cc (right):

http://codereview.chromium.org/10024013/diff/24007/content/browser/battery_st...
content/browser/battery_status/battery_status_provider_impl.cc:19:
observers_.AddObserver(obs);
On 2012/04/12 19:34:55, John Abd-El-Malek wrote:
> nit: here, and in RemoveObserver and SendUpdate, please add
> DCHECK(BrowserThread::CurrentlyOn...) to ensure that this is happening on the
> thread that you expect

Done.

http://codereview.chromium.org/10024013/diff/24007/content/browser/battery_st...
File content/browser/battery_status/battery_status_provider_impl.h (right):

http://codereview.chromium.org/10024013/diff/24007/content/browser/battery_st...
content/browser/battery_status/battery_status_provider_impl.h:8: #include <list>
On 2012/04/12 19:34:55, John Abd-El-Malek wrote:
> nit: not used

Done.

http://codereview.chromium.org/10024013/diff/24007/content/browser/battery_st...
content/browser/battery_status/battery_status_provider_impl.h:31: virtual void
SendUpdate(
On 2012/04/12 19:34:55, John Abd-El-Malek wrote:
> nit: why make this private? i'd put it in public, since it's odd that someone
> has to cast this to the base interface to call this function (say if android's
> impl is in content)

Done.

http://codereview.chromium.org/10024013/diff/24007/content/public/browser/bat...
File content/public/browser/battery_status_provider.h (right):

http://codereview.chromium.org/10024013/diff/24007/content/public/browser/bat...
content/public/browser/battery_status_provider.h:11: #include
"base/observer_list.h"
On 2012/04/12 19:34:55, John Abd-El-Malek wrote:
> nit: get rid of the three includes above this

Done.

[jam, 2012-04-13 00:46:42]

http://codereview.chromium.org/10024013/diff/26004/content/browser/battery_st...
File content/browser/battery_status/battery_status_provider_impl.cc (right):

http://codereview.chromium.org/10024013/diff/26004/content/browser/battery_st...
content/browser/battery_status/battery_status_provider_impl.cc:35:
FOR_EACH_OBSERVER(Observer, observers_, PowerChanged(status));
so observers_ is written on IO thread and read on UI? that'll lead to crashes...

[sadrul, 2012-04-13 11:28:44]

http://codereview.chromium.org/10024013/diff/26004/content/browser/battery_st...
File content/browser/battery_status/battery_status_provider_impl.cc (right):

http://codereview.chromium.org/10024013/diff/26004/content/browser/battery_st...
content/browser/battery_status/battery_status_provider_impl.cc:35:
FOR_EACH_OBSERVER(Observer, observers_, PowerChanged(status));
On 2012/04/13 00:46:42, John Abd-El-Malek wrote:
> so observers_ is written on IO thread and read on UI? that'll lead to
crashes...

Ouch! Changed to use ObserverListThreadSafe instead. Should be safe?

[jam, 2012-04-13 18:30:07]

On 2012/04/13 11:28:44, sadrul wrote:
>
http://codereview.chromium.org/10024013/diff/26004/content/browser/battery_st...
> File content/browser/battery_status/battery_status_provider_impl.cc (right):
> 
>
http://codereview.chromium.org/10024013/diff/26004/content/browser/battery_st...
> content/browser/battery_status/battery_status_provider_impl.cc:35:
> FOR_EACH_OBSERVER(Observer, observers_, PowerChanged(status));
> On 2012/04/13 00:46:42, John Abd-El-Malek wrote:
> > so observers_ is written on IO thread and read on UI? that'll lead to
> crashes...
> 
> Ouch! Changed to use ObserverListThreadSafe instead. Should be safe?

in general it's better to avoid locks and thread-safe possible if you can (see
http://dev.chromium.org/developers/design-documents/threading)

why not just dispatch the IPCs from the renderer on the UI thread? then you can
do everything on one thread. in your BrowserMessageFilter, you can implement
OverrideThreadForMessage and then do

if (IPC_MESSAGE_CLASS(message) == BatteryStatusMsgStart)
    *thread = BrowserThread::UI;

[sadrul, 2012-04-13 20:26:24]

[snip]
> > content/browser/battery_status/battery_status_provider_impl.cc:35:
> > FOR_EACH_OBSERVER(Observer, observers_, PowerChanged(status));
> > On 2012/04/13 00:46:42, John Abd-El-Malek wrote:
> > > so observers_ is written on IO thread and read on UI? that'll lead to
> > crashes...
> > 
> > Ouch! Changed to use ObserverListThreadSafe instead. Should be safe?
> 
> in general it's better to avoid locks and thread-safe possible if you can (see
> http://dev.chromium.org/developers/design-documents/threading)
> 
> why not just dispatch the IPCs from the renderer on the UI thread? then you
can
> do everything on one thread. in your BrowserMessageFilter, you can implement
> OverrideThreadForMessage and then do
> 
> if (IPC_MESSAGE_CLASS(message) == BatteryStatusMsgStart)
>     *thread = BrowserThread::UI;

Wha! That's really nice! Thanks for the suggestion. I have made this change.

I also had to change how the BatteryStatusMessageFilter::PowerManagerObserver
gets destroyed; the message-filters get destroyed on the IO thread. But it is
necessary to destroy the observer in the UI thread (since it needs to be removed
from the observer-list). So I introduced DestroyOnUI, which uses
DeleteOnUIThread::Destruct. This is OK?

[sadrul, 2012-04-17 16:32:03]

ping!

[jam, 2012-04-17 21:54:46]

lgtm

[sadrul, 2012-04-17 22:09:39]

On 2012/04/17 21:54:46, John Abd-El-Malek wrote:
> lgtm

Thanks!

Ben, Darin: Did you guys want to take a look too? Or should I land?

[Ben Goodger (Google), 2012-04-17 22:20:31]

LGTM other than nit:

http://codereview.chromium.org/10024013/diff/39002/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/10024013/diff/39002/chrome/chrome_browser.gypi...
chrome/chrome_browser.gypi:283:
'browser/chromeos/battery_status_provider_chromeos.cc',
wrong place in gyp file