[Cleanup] Screen cleanup

Screen cleanup:
* Split ScreenAsh to ScreenUtil (set of utility functions) and the screen implementation.
* moved screen_ash from Shell to DisplayManager. Ash specific imp is now hidden in DisplayManager.
* Shell now creates a separate screen object which simply keeps the list of displays for shutdown. This allow all screen operation valid during shutdown.

BUG=None
TBR=hongbo.min@intel.com,sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245562

[oshima, 2014-01-15 03:00:32]

It looks big, but most of changes are simple renaming.

[Jun Mukai, 2014-01-15 19:20:14]

mostly nitpicks

https://codereview.chromium.org/138003007/diff/130001/ash/display/display_man...
File ash/display/display_manager.h (right):

https://codereview.chromium.org/138003007/diff/130001/ash/display/display_man...
ash/display/display_manager.h:327: gfx::Screen* screen_;
Can we just put #include "ash/display/screen_ash.h" instead of forward
declarations so that it knows ScreenAsh is a subclass of gfx::Screen?  Then you
don't have to do such trick.

https://codereview.chromium.org/138003007/diff/130001/ash/display/screen_ash.h
File ash/display/screen_ash.h (right):

https://codereview.chromium.org/138003007/diff/130001/ash/display/screen_ash....
ash/display/screen_ash.h:81: friend class Shell;
I don't think Shell is necessarily a friend class.

https://codereview.chromium.org/138003007/diff/130001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/138003007/diff/130001/ash/shell.cc#newcode572
ash/shell.cc:572: }
If the screen object is owned by DisplayManager, I prefer to put the
initialization / shutdown code to ctor/dtor of DisplayManager.

Did you try that?  At least the initialization code can be in DisplayManager I
guess, but the shutdown code seems a bit tricky?

https://codereview.chromium.org/138003007/diff/130001/chrome/browser/extensio...
File chrome/browser/extensions/api/system_display/system_display_apitest.cc
(right):

https://codereview.chromium.org/138003007/diff/130001/chrome/browser/extensio...
chrome/browser/extensions/api/system_display/system_display_apitest.cc:188:
ash::Shell::GetScreen());
Doesn't it fit into the previous line?

https://codereview.chromium.org/138003007/diff/130001/chrome/browser/extensio...
File chrome/browser/extensions/api/system_info/system_info_api.cc (right):

https://codereview.chromium.org/138003007/diff/130001/chrome/browser/extensio...
chrome/browser/extensions/api/system_info/system_info_api.cc:27: #include
"ui/gfx/screen.h"
You can put ui/gfx/screen.h outside of USE_ASH right?

[oshima, 2014-01-15 19:42:51]

https://codereview.chromium.org/138003007/diff/130001/ash/display/display_man...
File ash/display/display_manager.h (right):

https://codereview.chromium.org/138003007/diff/130001/ash/display/display_man...
ash/display/display_manager.h:327: gfx::Screen* screen_;
On 2014/01/15 19:20:15, Jun Mukai wrote:
> Can we just put #include "ash/display/screen_ash.h" instead of forward
> declarations so that it knows ScreenAsh is a subclass of gfx::Screen?  Then
you
> don't have to do such trick.

I don't like exposing unnecessary implementation details in header. We do
something similar to focus/acivation controller. If you really don't like this,
I'll add internal accessor in .cc. Please let me know.

https://codereview.chromium.org/138003007/diff/130001/ash/display/screen_ash.h
File ash/display/screen_ash.h (right):

https://codereview.chromium.org/138003007/diff/130001/ash/display/screen_ash....
ash/display/screen_ash.h:81: friend class Shell;
On 2014/01/15 19:20:15, Jun Mukai wrote:
> I don't think Shell is necessarily a friend class.

Thank you for the catch. removed.

https://codereview.chromium.org/138003007/diff/130001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/138003007/diff/130001/ash/shell.cc#newcode572
ash/shell.cc:572: }
On 2014/01/15 19:20:15, Jun Mukai wrote:
> If the screen object is owned by DisplayManager, I prefer to put the
> initialization / shutdown code to ctor/dtor of DisplayManager.
> 
> Did you try that?  At least the initialization code can be in DisplayManager I
> guess, but the shutdown code seems a bit tricky?

That's good point, although I can't move the shutdown code to dtor due to
dependency. I moved initialization to ctor, and moved impl to
CreateScreenForShutdown .

https://codereview.chromium.org/138003007/diff/130001/chrome/browser/extensio...
File chrome/browser/extensions/api/system_display/system_display_apitest.cc
(right):

https://codereview.chromium.org/138003007/diff/130001/chrome/browser/extensio...
chrome/browser/extensions/api/system_display/system_display_apitest.cc:188:
ash::Shell::GetScreen());
On 2014/01/15 19:20:15, Jun Mukai wrote:
> Doesn't it fit into the previous line?

nope (85 chars)

https://codereview.chromium.org/138003007/diff/130001/chrome/browser/extensio...
File chrome/browser/extensions/api/system_info/system_info_api.cc (right):

https://codereview.chromium.org/138003007/diff/130001/chrome/browser/extensio...
chrome/browser/extensions/api/system_info/system_info_api.cc:27: #include
"ui/gfx/screen.h"
On 2014/01/15 19:20:15, Jun Mukai wrote:
> You can put ui/gfx/screen.h outside of USE_ASH right?

I can, but shouldn't because it is used only in USE_ASH.

[Jun Mukai, 2014-01-15 19:46:00]

lgtm

https://codereview.chromium.org/138003007/diff/130001/ash/display/display_man...
File ash/display/display_manager.h (right):

https://codereview.chromium.org/138003007/diff/130001/ash/display/display_man...
ash/display/display_manager.h:327: gfx::Screen* screen_;
On 2014/01/15 19:42:51, oshima wrote:
> On 2014/01/15 19:20:15, Jun Mukai wrote:
> > Can we just put #include "ash/display/screen_ash.h" instead of forward
> > declarations so that it knows ScreenAsh is a subclass of gfx::Screen?  Then
> you
> > don't have to do such trick.
> 
> I don't like exposing unnecessary implementation details in header. We do
> something similar to focus/acivation controller. If you really don't like
this,
> I'll add internal accessor in .cc. Please let me know.

okay, it's up to you.

[commit-bot: I haz the power, 2014-01-16 21:10:57]

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

[commit-bot: I haz the power, 2014-01-16 21:41:46]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[oshima, 2014-01-16 22:05:43]

tbr'ing trivial rename change

hongbo.min -> c/b/extensions/api/system/
sky -> c/b/ui/

[commit-bot: I haz the power, 2014-01-16 22:06:29]

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

[commit-bot: I haz the power, 2014-01-16 22:25:28]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[oshima, 2014-01-16 22:37:58]

(really, this time) tbr'ing trivial rename change

hongbo.min -> c/b/extensions/api/system/
sky -> c/b/ui/

[commit-bot: I haz the power, 2014-01-16 22:38:39]

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

[commit-bot: I haz the power, 2014-01-17 00:06:38]

Retried try job too often on win_rel for step(s) ash_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-17 00:57:29]

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

[commit-bot: I haz the power, 2014-01-17 06:05:54]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Hongbo Min, 2014-01-17 06:18:44]

lgtm for chrome/browser/extension/system part

[commit-bot: I haz the power, 2014-01-17 17:21:03]

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

[commit-bot: I haz the power, 2014-01-17 18:24:41]

Change committed as 245562