Move IsRunningOnChromeOS to base/chromeos

Move IsRunningOnChromeOS to base/chromeos so that ui/, /chromeos and other components can use it.
plus, a couple of cleanups including
 - removed unnecessary includes
 - removed unnecessary ifdef chromeos in chromeos only code.

BUG=115967
TEST=none

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

[satorux1, 2012-03-07 00:01:20]

LGTM

[oshima, 2012-03-07 04:29:20]

willchan -> base
ben -> chrome/browser/ui

[willchan no longer on Chromium, 2012-03-07 18:26:46]

Isn't this a terrible hack? :P Checking for USER==chronos? And don't we have all
this information at compile-time? Why are we determining this at run-time?

[willchan no longer on Chromium, 2012-03-07 18:43:37]

derat swung by and explained this to me. He also noted that we have used
delegates to expose this to lower layers like ui/. Is this really necessary
in base/? It seems awfully chromeos specific.

On Wed, Mar 7, 2012 at 10:26 AM, <willchan@chromium.org> wrote:

> Isn't this a terrible hack? :P Checking for USER==chronos? And don't we
> have all
> this information at compile-time? Why are we determining this at run-time?
>
>
http://codereview.chromium.**org/9546013/<http://codereview.chromium.org/9546...
>

[satorux1, 2012-03-07 18:55:19]

The terrible hack was by me. :~( IIRC, determining this by compilation time
wasn't an option as some people were running the same binary on Linux desktop
and chrome device (via sshfs). 
http://code.google.com/p/chromium-os/issues/detail?id=20924#c1 has some
background.

Re base or not, as long as it's accessible from places where needed, any place
would work. Since oshima wants to use from src/ui, 'base/chromeos' seemed to be
a convenient place. Putting this src/chromeos may also be an option, but not
sure introducing a new directory is a good idea.

[willchan no longer on Chromium, 2012-03-07 18:57:38]

On Wed, Mar 7, 2012 at 10:55 AM, <satorux@chromium.org> wrote:

> The terrible hack was by me. :~( IIRC, determining this by compilation time
> wasn't an option as some people were running the same binary on Linux
> desktop
> and chrome device (via sshfs).
>
http://code.google.com/p/**chromium-os/issues/detail?id=**20924#c1<http://cod...
some
> background.
>
> Re base or not, as long as it's accessible from places where needed, any
> place
> would work. Since oshima wants to use from src/ui, 'base/chromeos' seemed
> to be
> a convenient place. Putting this src/chromeos may also be an option, but
> not
> sure introducing a new directory is a good idea.
>

derat said we have previously passed Delegates into ui/ in order to expose
this type of functionality. Is there a reason that wouldn't work here?


>
>
http://codereview.chromium.**org/9546013/<http://codereview.chromium.org/9546...
>

[oshima, 2012-03-07 19:18:39]

On 2012/03/07 18:57:38, willchan wrote:
> On Wed, Mar 7, 2012 at 10:55 AM, <mailto:satorux@chromium.org> wrote:
> 
> > The terrible hack was by me. :~( IIRC, determining this by compilation time
> > wasn't an option as some people were running the same binary on Linux
> > desktop
> > and chrome device (via sshfs).
> >
>
http://code.google.com/p/**chromium-os/issues/detail?id=**20924#c1%3Chttp://c...
> some
> > background.
> >
> > Re base or not, as long as it's accessible from places where needed, any
> > place
> > would work. Since oshima wants to use from src/ui, 'base/chromeos' seemed
> > to be
> > a convenient place. Putting this src/chromeos may also be an option, but
> > not
> > sure introducing a new directory is a good idea.
> >
> 
> derat said we have previously passed Delegates into ui/ in order to expose
> this type of functionality. Is there a reason that wouldn't work here?

This is very fundamental function to tell if it's running on chromeos device,
which has no
other dependency, but other packages need to use. For such functionality, base
sounds the
right place to me. To be honest, this change is NOT necessary at all to fix the
issue
I'm working on (I even don't need Delegate), but I thought this is good chance
to refactor first
so that I can fix it in more straightforward way. Also, chrome/browser/chromeos
will be gone
in near future, and that's another reason why I'm trying to move this file now.

We already have base/win, base/mac and base/wayland, which have very platform
specific functions,
and I'm just following the same pattern. What is the reason why chromeos should
not have such package?

> 
> 
> >
> >
>
http://codereview.chromium.**org/9546013/%3Chttp://codereview.chromium.org/95...>
> >

[willchan no longer on Chromium, 2012-03-07 19:28:05]

On 2012/03/07 19:18:39, oshima wrote:
> On 2012/03/07 18:57:38, willchan wrote:
> > On Wed, Mar 7, 2012 at 10:55 AM, <mailto:satorux@chromium.org> wrote:
> > 
> > > The terrible hack was by me. :~( IIRC, determining this by compilation
time
> > > wasn't an option as some people were running the same binary on Linux
> > > desktop
> > > and chrome device (via sshfs).
> > >
> >
>
http://code.google.com/p/**chromium-os/issues/detail?id=**20924#c1%253Chttp:/...
> > some
> > > background.
> > >
> > > Re base or not, as long as it's accessible from places where needed, any
> > > place
> > > would work. Since oshima wants to use from src/ui, 'base/chromeos' seemed
> > > to be
> > > a convenient place. Putting this src/chromeos may also be an option, but
> > > not
> > > sure introducing a new directory is a good idea.
> > >
> > 
> > derat said we have previously passed Delegates into ui/ in order to expose
> > this type of functionality. Is there a reason that wouldn't work here?
> 
> This is very fundamental function to tell if it's running on chromeos device,
> which has no
> other dependency, but other packages need to use. For such functionality, base
> sounds the
> right place to me. To be honest, this change is NOT necessary at all to fix
the
> issue
> I'm working on (I even don't need Delegate), but I thought this is good chance
> to refactor first
> so that I can fix it in more straightforward way. Also,
chrome/browser/chromeos
> will be gone
> in near future, and that's another reason why I'm trying to move this file
now.
> 
> We already have base/win, base/mac and base/wayland, which have very platform
> specific functions,
> and I'm just following the same pattern. What is the reason why chromeos
should
> not have such package?

That's a very good argument. You've mostly convinced me.

It's not immediately obvious to me that we'd want to use a
base::chromeos::IsRunningOnChromeOS() check from within ui/, but I don't know
much about the ui package. ben@, can you comment here? If you feel like this
function would be useful from within ui/, then I would approve this for base/.

> 
> > 
> > 
> > >
> > >
> >
>
http://codereview.chromium.**org/9546013/%253Chttp://codereview.chromium.org/...>
> > >

[oshima, 2012-03-08 02:10:11]

On 2012/03/07 19:28:05, willchan wrote:
> On 2012/03/07 19:18:39, oshima wrote:
> > On 2012/03/07 18:57:38, willchan wrote:
> > > On Wed, Mar 7, 2012 at 10:55 AM, <mailto:satorux@chromium.org> wrote:
> > > 
> > > > The terrible hack was by me. :~( IIRC, determining this by compilation
> time
> > > > wasn't an option as some people were running the same binary on Linux
> > > > desktop
> > > > and chrome device (via sshfs).
> > > >
> > >
> >
>
http://code.google.com/p/**chromium-os/issues/detail?id=**20924#c1%25253Chttp...
> > > some
> > > > background.
> > > >
> > > > Re base or not, as long as it's accessible from places where needed, any
> > > > place
> > > > would work. Since oshima wants to use from src/ui, 'base/chromeos'
seemed
> > > > to be
> > > > a convenient place. Putting this src/chromeos may also be an option, but
> > > > not
> > > > sure introducing a new directory is a good idea.
> > > >
> > > 
> > > derat said we have previously passed Delegates into ui/ in order to expose
> > > this type of functionality. Is there a reason that wouldn't work here?
> > 
> > This is very fundamental function to tell if it's running on chromeos
device,
> > which has no
> > other dependency, but other packages need to use. For such functionality,
base
> > sounds the
> > right place to me. To be honest, this change is NOT necessary at all to fix
> the
> > issue
> > I'm working on (I even don't need Delegate), but I thought this is good
chance
> > to refactor first
> > so that I can fix it in more straightforward way. Also,
> chrome/browser/chromeos
> > will be gone
> > in near future, and that's another reason why I'm trying to move this file
> now.
> > 
> > We already have base/win, base/mac and base/wayland, which have very
platform
> > specific functions,
> > and I'm just following the same pattern. What is the reason why chromeos
> should
> > not have such package?
> 
> That's a very good argument. You've mostly convinced me.
> 
> It's not immediately obvious to me that we'd want to use a
> base::chromeos::IsRunningOnChromeOS() check from within ui/, but I don't know
> much about the ui package. ben@, can you comment here? If you feel like this
> function would be useful from within ui/, then I would approve this for base/.

Additional note. This will also be used from ash/ once we move stuff in
chrome/browser/chromeos 
into ash.

> 
> > 
> > > 
> > > 
> > > >
> > > >
> > >
> >
>
http://codereview.chromium.**org/9546013/%25253Chttp://codereview.chromium.or...>
> > > >

[Ben Goodger (Google), 2012-03-08 02:53:59]

Why not add this to ui/base? We have a bunch of other stuff there that isn't
directly UI related, but is required to support our apps with a UI.

[oshima, 2012-03-08 03:27:47]

On 2012/03/08 02:53:59, Ben Goodger (Google) wrote:
> Why not add this to ui/base? We have a bunch of other stuff there that isn't
> directly UI related, but is required to support our apps with a UI.

Because this functionality is not just to support UI.  For example, all files
under chromeos/dbus
have no dependency to UI, but needs to use this. (and there are a few others
such as audio/network stuff)

[oshima, 2012-03-15 22:32:59]

ping?

I was going to revisit this after m19, but we decided to move files before m19
and I need this now to move some of chromeos files into /chromeos. satorux told
me that he also need to use this in src/webkit for other issue.

On 2012/03/08 03:27:47, oshima wrote:
> On 2012/03/08 02:53:59, Ben Goodger (Google) wrote:
> > Why not add this to ui/base? We have a bunch of other stuff there that isn't
> > directly UI related, but is required to support our apps with a UI.
> 
> Because this functionality is not just to support UI.  For example, all files
> under chromeos/dbus
> have no dependency to UI, but needs to use this. (and there are a few others
> such as audio/network stuff)

[brettw, 2012-03-16 17:11:16]

I think this is OK to add in principle.

But I don't think "runtime_environment.h" makes sense. Sorry to make you change
everything. Can it be called base/chromeos/chromeos_version.h[.cc]? This matches
the windows one.

[oshima, 2012-03-16 17:20:52]

On 2012/03/16 17:11:16, brettw wrote:
> I think this is OK to add in principle.
> 
> But I don't think "runtime_environment.h" makes sense. Sorry to make you
change
> everything. Can it be called base/chromeos/chromeos_version.h[.cc]? This
matches
> the windows one.

No problem. I'll change it. Satoru, let me know if you have opinion.

[Ben Goodger (Google), 2012-03-16 17:23:08]

blanket LGTM for updating call sites to what brettw recommends.

[satorux1, 2012-03-16 17:29:00]

Oshima, the new name sounds good.

[oshima, 2012-03-16 19:21:46]

Uploaded new patch. Brett, Satoru, can you take another look?

[brettw, 2012-03-16 19:24:29]

lgtm

[satorux1, 2012-03-16 19:47:49]

LGTM

[commit-bot: I haz the power, 2012-03-16 20:03:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/9546013/15051

[commit-bot: I haz the power, 2012-03-16 20:04:15]

Presubmit check for 9546013-15051 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/chromeos/status

Presubmit checks took 3.7s to calculate.

[stevenjb, 2012-03-16 20:23:16]

lgtm

[commit-bot: I haz the power, 2012-03-16 20:25:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/9546013/15051

[commit-bot: I haz the power, 2012-03-16 23:01:07]

Change committed as 127283