Move ash/system/network to ash/system/chromeos

Move Ash system network trays to ash/system/chromeos
Also adds DEPS to keep chromeos dependencies out of ash/system

This is part of an effort to move chromeos network management code from src/chrome/ to src/chromeos.

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

[stevenjb, 2012-10-04 20:44:15]

This is ready for review.

[sadrul, 2012-10-04 20:49:18]

Quick question: Pretty much all of system tray (e.g. network, bluetooth,
display, user etc.) are dependent on chromeos implementation. Is the plan that
all of this will move into system/chromeos/?

[stevenjb, 2012-10-04 21:29:34]

Hopefully at least bluetooth and power will be re-factored so that Chrome no
longer needs to be the middle man. Those are both very chromeos specific and
should be moved into system/chromeos (probably when we do the refactoring).

Probably display also, although there we might want to separate out chromeos
specific behavior into a child class, I wasn't certain. Same with user.

Some of the others like accessibility don't actually have any chromeos
dependencies and should be fine where they are.


On 2012/10/04 20:49:18, sadrul wrote:
> Quick question: Pretty much all of system tray (e.g. network, bluetooth,
> display, user etc.) are dependent on chromeos implementation. Is the plan that
> all of this will move into system/chromeos/?

[stevenjb, 2012-10-08 22:38:14]

ping

[jennyz, 2012-10-08 23:20:00]

lgtm

http://codereview.chromium.org/11039034/diff/3001/ash/system/DEPS
File ash/system/DEPS (right):

http://codereview.chromium.org/11039034/diff/3001/ash/system/DEPS#newcode5
ash/system/DEPS:5: "+chromeos/display/output_configurator.h",
Can we create a crbug to track this work item?

http://codereview.chromium.org/11039034/diff/3001/ash/system/power/DEPS
File ash/system/power/DEPS (right):

http://codereview.chromium.org/11039034/diff/3001/ash/system/power/DEPS#newcode2
ash/system/power/DEPS:2: "+chromeos/dbus/power_supply_status.h",
Should we create a crbug for moving the ash/system/power to
ash/system/chromeos/power?

[tfarina, 2012-10-09 01:53:22]

http://codereview.chromium.org/11039034/diff/3001/ash/system/chromeos/network...
File ash/system/chromeos/network/network_observer.h (right):

http://codereview.chromium.org/11039034/diff/3001/ash/system/chromeos/network...
ash/system/chromeos/network/network_observer.h:43: MessageType message_type,
nit: wrong indentation here. one space off.

[tfarina, 2012-10-09 01:53:59]

http://codereview.chromium.org/11039034/diff/3001/ash/system/chromeos/network...
File ash/system/chromeos/network/sms_observer.h (right):

http://codereview.chromium.org/11039034/diff/3001/ash/system/chromeos/network...
ash/system/chromeos/network/sms_observer.h:8: #include "base/string16.h"
you can remove this include.

[tfarina, 2012-10-09 02:35:59]

http://codereview.chromium.org/11039034/diff/3001/ash/system/chromeos/network...
File ash/system/chromeos/network/tray_network.h (right):

http://codereview.chromium.org/11039034/diff/3001/ash/system/chromeos/network...
ash/system/chromeos/network/tray_network.h:5: #ifndef
ASH_SYSTEM_NETWORK_TRAY_NETWORK_H
wrong header guard.

http://codereview.chromium.org/11039034/diff/3001/ash/system/chromeos/network...
File ash/system/chromeos/network/tray_sms.h (right):

http://codereview.chromium.org/11039034/diff/3001/ash/system/chromeos/network...
ash/system/chromeos/network/tray_sms.h:5: #ifndef ASH_SYSTEM_NETWORK_TRAY_SMS_H
wrong header guard.

[sadrul, 2012-10-09 14:40:45]

On 2012/10/04 21:29:34, stevenjb (chromium) wrote:
> Hopefully at least bluetooth and power will be re-factored so that Chrome no
> longer needs to be the middle man. Those are both very chromeos specific and
> should be moved into system/chromeos (probably when we do the refactoring).
> 
> Probably display also, although there we might want to separate out chromeos
> specific behavior into a child class, I wasn't certain. Same with user.
> 
> Some of the others like accessibility don't actually have any chromeos
> dependencies and should be fine where they are.

There was probably discussion about this move somewhere? Is there a link? Moving
just network/ into ash/system/chromeos/ without commitment to move all the
relevant bits too (i.e. having some code in ash/system/ and some in
ash/system/chromeos) would be confusing.

FWIW, I feel like we should have some common interface in src/system/ (network,
bluetooth etc), with implementations in src/chromeos/. But I probably don't have
the complete picture. I will defer to Ben for the l gtm.

[sadrul, 2012-10-09 14:40:58]

http://codereview.chromium.org/11039034/diff/3001/ash/system/status_area_widg...
File ash/system/status_area_widget.cc (left):

http://codereview.chromium.org/11039034/diff/3001/ash/system/status_area_widg...
ash/system/status_area_widget.cc:291: bool bluetooth_enabled_;
Why are these removed?

[stevenjb, 2012-10-09 19:03:46]

I went ahead and moved tray_display also since it was pretty straightforward;
the only delegate call it has is ShowDisplaySettings() which should remain in
the delegate.

I created an issue for moving tray_power: crbug.com/154842.

I also created a tracking issue for the migration for further discussion:
crbug.com/154856


On 2012/10/09 14:40:45, sadrul wrote:
> On 2012/10/04 21:29:34, stevenjb (chromium) wrote:
> > Hopefully at least bluetooth and power will be re-factored so that Chrome no
> > longer needs to be the middle man. Those are both very chromeos specific and
> > should be moved into system/chromeos (probably when we do the refactoring).
> > 
> > Probably display also, although there we might want to separate out chromeos
> > specific behavior into a child class, I wasn't certain. Same with user.
> > 
> > Some of the others like accessibility don't actually have any chromeos
> > dependencies and should be fine where they are.
> 
> There was probably discussion about this move somewhere? Is there a link?
Moving
> just network/ into ash/system/chromeos/ without commitment to move all the
> relevant bits too (i.e. having some code in ash/system/ and some in
> ash/system/chromeos) would be confusing.
> 
> FWIW, I feel like we should have some common interface in src/system/
(network,
> bluetooth etc), with implementations in src/chromeos/. But I probably don't
have
> the complete picture. I will defer to Ben for the l gtm.

[stevenjb, 2012-10-09 19:04:13]

http://codereview.chromium.org/11039034/diff/3001/ash/system/DEPS
File ash/system/DEPS (right):

http://codereview.chromium.org/11039034/diff/3001/ash/system/DEPS#newcode5
ash/system/DEPS:5: "+chromeos/display/output_configurator.h",
On 2012/10/08 23:20:01, jennyz wrote:
> Can we create a crbug to track this work item?
I went ahead and moved tray_display so this can be removed.

http://codereview.chromium.org/11039034/diff/3001/ash/system/chromeos/network...
File ash/system/chromeos/network/network_observer.h (right):

http://codereview.chromium.org/11039034/diff/3001/ash/system/chromeos/network...
ash/system/chromeos/network/network_observer.h:43: MessageType message_type,
On 2012/10/09 01:53:22, tfarina wrote:
> nit: wrong indentation here. one space off.

Done.

http://codereview.chromium.org/11039034/diff/3001/ash/system/chromeos/network...
File ash/system/chromeos/network/sms_observer.h (right):

http://codereview.chromium.org/11039034/diff/3001/ash/system/chromeos/network...
ash/system/chromeos/network/sms_observer.h:8: #include "base/string16.h"
On 2012/10/09 01:53:59, tfarina wrote:
> you can remove this include.

Done.

http://codereview.chromium.org/11039034/diff/3001/ash/system/chromeos/network...
File ash/system/chromeos/network/tray_network.h (right):

http://codereview.chromium.org/11039034/diff/3001/ash/system/chromeos/network...
ash/system/chromeos/network/tray_network.h:5: #ifndef
ASH_SYSTEM_NETWORK_TRAY_NETWORK_H
On 2012/10/09 02:36:00, tfarina wrote:
> wrong header guard.
Oops, thanks, fixed here and elsewhere.

http://codereview.chromium.org/11039034/diff/3001/ash/system/chromeos/network...
File ash/system/chromeos/network/tray_sms.h (right):

http://codereview.chromium.org/11039034/diff/3001/ash/system/chromeos/network...
ash/system/chromeos/network/tray_sms.h:5: #ifndef ASH_SYSTEM_NETWORK_TRAY_SMS_H
On 2012/10/09 02:36:00, tfarina wrote:
> wrong header guard.

Done.

http://codereview.chromium.org/11039034/diff/3001/ash/system/power/DEPS
File ash/system/power/DEPS (right):

http://codereview.chromium.org/11039034/diff/3001/ash/system/power/DEPS#newcode2
ash/system/power/DEPS:2: "+chromeos/dbus/power_supply_status.h",
On 2012/10/08 23:20:01, jennyz wrote:
> Should we create a crbug for moving the ash/system/power to
> ash/system/chromeos/power?

Done.

http://codereview.chromium.org/11039034/diff/3001/ash/system/status_area_widg...
File ash/system/status_area_widget.cc (left):

http://codereview.chromium.org/11039034/diff/3001/ash/system/status_area_widg...
ash/system/status_area_widget.cc:291: bool bluetooth_enabled_;
On 2012/10/09 14:40:58, sadrul wrote:
> Why are these removed?
Restored.

[Ben Goodger (Google), 2012-10-16 17:36:05]

So, you should think of src/chromeos as a very low level thing. Sort of the
equivalent of the "platform api" that stuff like ash etc is built on. The
ChromeOS equivalent in some ways of windows.h.

So in that sense I don't think it makes sense to have code in src/chromeos
depend on or implement interfaces from any code in src/ash.

If there are ChromeOS specific things in src/ash then it's fine to separate
those out within ash as Steven has done. Down the road we may want to use some
of this stuff on Windows too but since that stuff hasn't crystallized yet I am
fine with what Steven is doing here. LGTM for this cl.

[commit-bot: I haz the power, 2012-10-16 18:35:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/11039034/20001

[commit-bot: I haz the power, 2012-10-16 22:01:04]

Retried try job too often for step(s) browser_tests

[commit-bot: I haz the power, 2012-10-17 01:00:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/11039034/22008

[commit-bot: I haz the power, 2012-10-17 11:13:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/11039034/22008

[commit-bot: I haz the power, 2012-10-17 13:14:08]

Change committed as 162377

[tommi (sloooow) - chröme, 2012-10-17 14:39:21]

On 2012/10/17 13:14:08, I haz the power (commit-bot) wrote:
> Change committed as 162377

Had to revert this since it broke the tree.
http://src.chromium.org/viewvc/chrome?view=rev&revision=162406