gn: simplify dbus logic in device/battery

gn: make device/battery build on embedded linux

The logic for when to include the linux dbus implementation
falls into three cases: chromeos, non-chromeos linux with
dbus, and non-chromeos linux without dbus.

By using just the is_chromeos, is_linux and use_dbus
variables, this code will also build successfully on
embedded linux targets.

Committed: https://crrev.com/d9a25193e2021ec2cbfd18d5dd348c7380c43ace
Cr-Commit-Position: refs/heads/master@{#374654}

[Mostyn Bramley-Moore, 2016-02-08 16:44:23]

The CQ bit was checked by mostynb@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-02-08 16:44:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1674233002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1674233002/1

[commit-bot: I haz the power, 2016-02-08 16:52:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-08 16:52:39]

Dry run: Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mostyn Bramley-Moore, 2016-02-08 17:05:07]

The CQ bit was checked by mostynb@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-02-08 17:06:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1674233002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1674233002/20001

[commit-bot: I haz the power, 2016-02-08 17:46:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-08 17:46:15]

Dry run: This issue passed the CQ dry run.

[Mostyn Bramley-Moore, 2016-02-08 17:50:45]

mostynb@opera.com changed reviewers:
+ timvolodine@chromium.org

[Mostyn Bramley-Moore, 2016-02-08 17:50:45]

@timvolodine: please take a look at this GN fixup.

[timvolodine, 2016-02-08 19:02:55]

On 2016/02/08 17:50:45, Mostyn Bramley-Moore wrote:
> @timvolodine: please take a look at this GN fixup.

actually i think the desktop_linux was intentional, looking at
https://code.google.com/p/chromium/codesearch#chromium/src/build/config/BUILD...
is_linux seems to be true for desktop linux + chromeOS which is probably not
what we want as we have a separate clause for chromeOS..

[Mostyn Bramley-Moore, 2016-02-08 19:19:22]

On 2016/02/08 19:02:55, timvolodine wrote:
> actually i think the desktop_linux was intentional, looking at
>
https://code.google.com/p/chromium/codesearch#chromium/src/build/config/BUILD...
> is_linux seems to be true for desktop linux + chromeOS which is probably not
> what we want as we have a separate clause for chromeOS..

With this proposed change, I think the form is nicer:
if (is_chromeos) {
...
} else if ("linux but not chromeos") {
...
}

This is nicer because the two blocks remove a the same source from from a list,
which is an error in GN (if you're building a platform like mine which is linux
but neither chromeos nor desktop linux).  

Another option would be to refactor like this (which has less duplication):

if (is_linux) {
  if (use_dbus) {
    # remove dbus-related files (battery_status_manager_linux.cc)
    # add dbus dep + config
  }
  if (is_chromeos) {
    # remove stub (battery_status_manager_default.cc)
    # add chromeos deps
  }
}

[Mostyn Bramley-Moore, 2016-02-08 20:38:31]

Patchset 3 shows the refactored option.

[Mostyn Bramley-Moore, 2016-02-08 20:41:28]

Description was changed from

==========
gn: toggle dbus code in device/battery just like in gyp

The gyp build checks is_linux and use_dbus to decide if the dbus code
should be enabled.  The gn build checks is_desktop_linux instead of
is_linux.  Let's make the gn build consistent with the gyp build.
==========

to

==========
gn: simplify dbus logic in device/battery

The logic for when to include the linux dbus or default implementation
of this code depends on is_linux, use_dbus and is_chromeos.
Let's remove the use of is_desktop_linux, since it is not
directly related.
==========

[Mostyn Bramley-Moore, 2016-02-08 20:41:40]

Description was changed from

==========
gn: simplify dbus logic in device/battery

The logic for when to include the linux dbus or default implementation
of this code depends on is_linux, use_dbus and is_chromeos.
Let's remove the use of is_desktop_linux, since it is not
directly related.
==========

to

==========
gn: simplify dbus logic in device/battery

The logic for when to include the linux dbus or default implementation
of this code depends on is_linux, use_dbus and is_chromeos.
Let's remove the use of is_desktop_linux, since it is not directly related.
==========

[Mostyn Bramley-Moore, 2016-02-08 20:42:12]

The CQ bit was checked by mostynb@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-02-08 20:43:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1674233002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1674233002/40001

[commit-bot: I haz the power, 2016-02-08 21:41:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-08 21:41:42]

Dry run: This issue passed the CQ dry run.

[timvolodine, 2016-02-09 13:58:02]

On 2016/02/08 19:19:22, Mostyn Bramley-Moore wrote:
> On 2016/02/08 19:02:55, timvolodine wrote:
> > actually i think the desktop_linux was intentional, looking at
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/build/config/BUILD...
> > is_linux seems to be true for desktop linux + chromeOS which is probably not
> > what we want as we have a separate clause for chromeOS..
> 
> With this proposed change, I think the form is nicer:
> if (is_chromeos) {
> ...
> } else if ("linux but not chromeos") {
> ...
> }
> 
> This is nicer because the two blocks remove a the same source from from a
list,
> which is an error in GN (if you're building a platform like mine which is
linux
> but neither chromeos nor desktop linux).  
> 
> Another option would be to refactor like this (which has less duplication):
> 
> if (is_linux) {
>   if (use_dbus) {
>     # remove dbus-related files (battery_status_manager_linux.cc)
>     # add dbus dep + config
>   }
>   if (is_chromeos) {
>     # remove stub (battery_status_manager_default.cc)
>     # add chromeos deps
>   }
> }

right, I didn't realize the current gn doesn't cover some linux systems. if not
a secret, what system are you running that is not chrome_os and not
desktop_linux?

while your refactoring looks clever, it feels less readable to me, can we maybe
add explicitly (as you mentioned in the 'first form'):
if (is_linux && !is_chromeos) { .. }
to clearly capture the configs by system..

[timvolodine, 2016-02-09 13:59:05]

https://codereview.chromium.org/1674233002/diff/40001/device/battery/BUILD.gn
File device/battery/BUILD.gn (left):

https://codereview.chromium.org/1674233002/diff/40001/device/battery/BUILD.gn...
device/battery/BUILD.gn:51: if (is_desktop_linux) {
maybe just use "if (is_linux && !is_chromeos) { .. }" instead for readability?

[Mostyn Bramley-Moore, 2016-02-09 21:38:51]

> right, I didn't realize the current gn doesn't cover some linux systems. if
not
> a secret, what system are you running that is not chrome_os and not
> desktop_linux?

I work on an embedded linux system, which is mostly used for internet video
streaming applications (youtube, bbc iplayer etc).

https://codereview.chromium.org/1674233002/diff/40001/device/battery/BUILD.gn
File device/battery/BUILD.gn (left):

https://codereview.chromium.org/1674233002/diff/40001/device/battery/BUILD.gn...
device/battery/BUILD.gn:51: if (is_desktop_linux) {
On 2016/02/09 13:59:04, timvolodine wrote:
> maybe just use "if (is_linux && !is_chromeos) { .. }" instead for readability?

How does patchset 5 look to you?

[timvolodine, 2016-02-10 12:40:40]

https://codereview.chromium.org/1674233002/diff/80001/device/battery/BUILD.gn
File device/battery/BUILD.gn (right):

https://codereview.chromium.org/1674233002/diff/80001/device/battery/BUILD.gn...
device/battery/BUILD.gn:49: } else if (is_linux && !is_chromeos) {
can we just have a separate "if (is_linux &&!is_chrome_os)" without the "else"
because the "!is_chromeos" is kind of redundant with the "else if". For
readability I think it's good to keep the system sections as separate "ifs" as
we have for all other mac and win below.

[Mostyn Bramley-Moore, 2016-02-10 13:01:51]

https://codereview.chromium.org/1674233002/diff/80001/device/battery/BUILD.gn
File device/battery/BUILD.gn (right):

https://codereview.chromium.org/1674233002/diff/80001/device/battery/BUILD.gn...
device/battery/BUILD.gn:49: } else if (is_linux && !is_chromeos) {
On 2016/02/10 12:40:40, timvolodine wrote:
> can we just have a separate "if (is_linux &&!is_chrome_os)" without the "else"
> because the "!is_chromeos" is kind of redundant with the "else if". For
> readability I think it's good to keep the system sections as separate "ifs" as
> we have for all other mac and win below.

Done.

[timvolodine, 2016-02-10 13:05:36]

On 2016/02/10 13:01:51, Mostyn Bramley-Moore wrote:
> https://codereview.chromium.org/1674233002/diff/80001/device/battery/BUILD.gn
> File device/battery/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1674233002/diff/80001/device/battery/BUILD.gn...
> device/battery/BUILD.gn:49: } else if (is_linux && !is_chromeos) {
> On 2016/02/10 12:40:40, timvolodine wrote:
> > can we just have a separate "if (is_linux &&!is_chrome_os)" without the
"else"
> > because the "!is_chromeos" is kind of redundant with the "else if". For
> > readability I think it's good to keep the system sections as separate "ifs"
as
> > we have for all other mac and win below.
> 
> Done.

great, thanks! lgtm.

could you please update the description to explain the replacement of
desktop_linux to cover all linux configs?

[Mostyn Bramley-Moore, 2016-02-10 13:05:50]

The CQ bit was checked by mostynb@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-02-10 13:05:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1674233002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1674233002/100001

[Mostyn Bramley-Moore, 2016-02-10 13:18:44]

Description was changed from

==========
gn: simplify dbus logic in device/battery

The logic for when to include the linux dbus or default implementation
of this code depends on is_linux, use_dbus and is_chromeos.
Let's remove the use of is_desktop_linux, since it is not directly related.
==========

to

==========
gn: make device/battery build on embedded linux

The logic for when to include the linux dbus implementation
falls into three cases: chromeos, non-chromeos linux with
dbus, and non-chromeos linux without dbus.

By using the is_chromeos, is_linux and use_dbus variables,
this code will also build successfully on embedded linux
targets.
==========

[Mostyn Bramley-Moore, 2016-02-10 13:19:31]

Description was changed from

==========
gn: make device/battery build on embedded linux

The logic for when to include the linux dbus implementation
falls into three cases: chromeos, non-chromeos linux with
dbus, and non-chromeos linux without dbus.

By using the is_chromeos, is_linux and use_dbus variables,
this code will also build successfully on embedded linux
targets.
==========

to

==========
gn: make device/battery build on embedded linux

The logic for when to include the linux dbus implementation
falls into three cases: chromeos, non-chromeos linux with
dbus, and non-chromeos linux without dbus.

By using just the is_chromeos, is_linux and use_dbus
variables, this code will also build successfully on
embedded linux targets.
==========

[Mostyn Bramley-Moore, 2016-02-10 13:20:06]

Thanks, description updated.

[Mostyn Bramley-Moore, 2016-02-10 13:20:07]

Thanks, description updated.

[commit-bot: I haz the power, 2016-02-10 14:16:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-10 14:16:15]

Dry run: This issue passed the CQ dry run.

[Mostyn Bramley-Moore, 2016-02-10 14:17:04]

The CQ bit was checked by mostynb@opera.com

[commit-bot: I haz the power, 2016-02-10 14:17:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1674233002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1674233002/100001

[commit-bot: I haz the power, 2016-02-10 14:24:07]

Description was changed from

==========
gn: make device/battery build on embedded linux

The logic for when to include the linux dbus implementation
falls into three cases: chromeos, non-chromeos linux with
dbus, and non-chromeos linux without dbus.

By using just the is_chromeos, is_linux and use_dbus
variables, this code will also build successfully on
embedded linux targets.
==========

to

==========
gn: make device/battery build on embedded linux

The logic for when to include the linux dbus implementation
falls into three cases: chromeos, non-chromeos linux with
dbus, and non-chromeos linux without dbus.

By using just the is_chromeos, is_linux and use_dbus
variables, this code will also build successfully on
embedded linux targets.
==========

[commit-bot: I haz the power, 2016-02-10 14:24:08]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-02-10 14:25:19]

Description was changed from

==========
gn: make device/battery build on embedded linux

The logic for when to include the linux dbus implementation
falls into three cases: chromeos, non-chromeos linux with
dbus, and non-chromeos linux without dbus.

By using just the is_chromeos, is_linux and use_dbus
variables, this code will also build successfully on
embedded linux targets.
==========

to

==========
gn: make device/battery build on embedded linux

The logic for when to include the linux dbus implementation
falls into three cases: chromeos, non-chromeos linux with
dbus, and non-chromeos linux without dbus.

By using just the is_chromeos, is_linux and use_dbus
variables, this code will also build successfully on
embedded linux targets.

Committed: https://crrev.com/d9a25193e2021ec2cbfd18d5dd348c7380c43ace
Cr-Commit-Position: refs/heads/master@{#374654}
==========

[commit-bot: I haz the power, 2016-02-10 14:25:20]

Patchset 6 (id:??) landed as
https://crrev.com/d9a25193e2021ec2cbfd18d5dd348c7380c43ace
Cr-Commit-Position: refs/heads/master@{#374654}