Make chromeos build use shared object to load battery...

Make chromeos build use shared object to load battery
functionality. Call into it and show new battery images


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=26898

[brettw, 2009-09-20 18:07:12]

http://codereview.chromium.org/215025/diff/8001/8003
File chrome/browser/chromeos/status_area_view.cc (right):

http://codereview.chromium.org/215025/diff/8001/8003#newcode352
Line 352: void* StatusAreaView::power_library_ = NULL;
We usually put these at the top right (above the constructor for the
corresponding class).

http://codereview.chromium.org/215025/diff/8001/8003#newcode356
Line 356: void StatusAreaView::LoadPowerLibrary() {
Maybe we should histogram this (doesn't have to be UMA) so we can tell if this
is a perf problem.

http://codereview.chromium.org/215025/diff/8001/8003#newcode359
Line 359: if (PathService::Get(chrome::FILE_CHROMEOS_API, &path)) {
I'd like to see these first two conditions be inverted and converted to early
returns. The bottom of this function is too deeply nested to be easily
understandable.

http://codereview.chromium.org/215025/diff/8001/8003#newcode361
Line 361: dlopen(path.value().c_str(), RTLD_NOW);
This should be indented 2 more spaces.

http://codereview.chromium.org/215025/diff/8001/8003#newcode366
Line 366: char *error = dlerror();
Put the * on the type

http://codereview.chromium.org/215025/diff/8001/8003#newcode387
Line 387: status.line_power_on) {
Intent this inside the (, two fewer spaces.

http://codereview.chromium.org/215025/diff/8001/8004
File chrome/browser/chromeos/status_area_view.h (right):

http://codereview.chromium.org/215025/diff/8001/8004#newcode62
Line 62: static void PowerStatusChangedHandler(
Can you document this?

http://codereview.chromium.org/215025/diff/8001/8004#newcode77
Line 77: static void* power_library_;
Can you document these (esp since void* doesn't give much info).

http://codereview.chromium.org/215025/diff/8001/8005
File chrome/common/chrome_paths.h (right):

http://codereview.chromium.org/215025/diff/8001/8005#newcode36
Line 36: #if defined(OS_CHROMEOS)
This file should include build/build_config.h

[DaveMoore, 2009-09-21 00:40:44]

http://codereview.chromium.org/215025/diff/8001/8003
File chrome/browser/chromeos/status_area_view.cc (right):

http://codereview.chromium.org/215025/diff/8001/8003#newcode359
Line 359: if (PathService::Get(chrome::FILE_CHROMEOS_API, &path)) {
On 2009/09/20 18:07:12, brettw wrote:
> I'd like to see these first two conditions be inverted and converted to early
> returns. The bottom of this function is too deeply nested to be easily
> understandable.

Done.

http://codereview.chromium.org/215025/diff/8001/8003#newcode361
Line 361: dlopen(path.value().c_str(), RTLD_NOW);
On 2009/09/20 18:07:12, brettw wrote:
> This should be indented 2 more spaces.

Done.

http://codereview.chromium.org/215025/diff/8001/8003#newcode366
Line 366: char *error = dlerror();
On 2009/09/20 18:07:12, brettw wrote:
> Put the * on the type

Done.

http://codereview.chromium.org/215025/diff/8001/8003#newcode387
Line 387: status.line_power_on) {
On 2009/09/20 18:07:12, brettw wrote:
> Intent this inside the (, two fewer spaces.

Done.

http://codereview.chromium.org/215025/diff/8001/8005
File chrome/common/chrome_paths.h (right):

http://codereview.chromium.org/215025/diff/8001/8005#newcode36
Line 36: #if defined(OS_CHROMEOS)
On 2009/09/20 18:07:12, brettw wrote:
> This file should include build/build_config.h

Done.

[brettw, 2009-09-21 04:20:49]

LGTM with style fix

http://codereview.chromium.org/215025/diff/55/57
File chrome/browser/chromeos/status_area_view.cc (right):

http://codereview.chromium.org/215025/diff/55/57#newcode362
Line 362: if (power_library_)
When we have multiple lines in a block, we alawya use {} (no matter how you got
multiple lines, though WebKit would do it your way). I'd say put the comments
for these two blocks as end-of-line comments after the return. Otherwise add {}