Refactor screen/monitor so that gfx::Screen returns monitor object.

* Separated implementation class from gfx::Screen
* Moved Monitor class to gfx/.
* Converted all use of gfx::Screen to match new API

BUG=115347, 111990
TEST=none

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

[oshima, 2012-04-09 21:08:39]

David, Elliot can you review screen_mac/screen_gtk changes?

I'm changing gfx::Screen to return Monitor object and want to make sure they're
getting the correct size.

[Elliot Glaysher, 2012-04-09 21:42:23]

lgtm, i guess

I'm a bit worried about users grabbing the Monitor object and using it after
bounds have been reassigned, but this is probably not worth worrying about.

[dhollowa, 2012-04-09 23:41:36]

LGTM w/ question.

https://chromiumcodereview.appspot.com/9960042/diff/3054/ui/gfx/screen_mac.mm
File ui/gfx/screen_mac.mm (right):

https://chromiumcodereview.appspot.com/9960042/diff/3054/ui/gfx/screen_mac.mm...
ui/gfx/screen_mac.mm:45: static gfx::SimpleMonitor* monitor = new
gfx::SimpleMonitor();
This will cause SimpleMonitor destructor to fire post-exit of |main()|.  That
may be ok, as long as it is trivial and doesn't hold on to anything important. 
I believe we have shutdown paths that will never reach static destructors too. 
Would a formal singleton be better?

[oshima, 2012-04-10 00:09:07]

>I'm a bit worried about users grabbing the Monitor object and using it after
>bounds have been reassigned, but this is probably not worth worrying about.

Such usage is explicitly prohibited by the comment,
for now. Since monitor can be unplugged any time, you shouldn't access it unless
you listen to the monitor
change event, which is only available on aura right now.
I believe this is necessary on on other platforms in order
to reposition panel and notifications correctly when
monitor configuration changes.
 For example, notifications do not get repositioned when I change the monitor
configuration on Mac and Linux. I'll look into other platforms in separate CL.

https://chromiumcodereview.appspot.com/9960042/diff/3054/ui/gfx/screen_mac.mm
File ui/gfx/screen_mac.mm (right):

https://chromiumcodereview.appspot.com/9960042/diff/3054/ui/gfx/screen_mac.mm...
ui/gfx/screen_mac.mm:45: static gfx::SimpleMonitor* monitor = new
gfx::SimpleMonitor();
This isn't uncommon for near POD objects such as Font and I think Monitor class
is simple enough not to worry about shutdown.

[oshima, 2012-04-10 00:21:34]

Ben, please review the entire CL except for screen_mac.mm and screen_gtk.cc.

[Ben Goodger (Google), 2012-04-10 16:37:20]

https://chromiumcodereview.appspot.com/9960042/diff/2006/ui/gfx/monitor.h
File ui/gfx/monitor.h (right):

https://chromiumcodereview.appspot.com/9960042/diff/2006/ui/gfx/monitor.h#new...
ui/gfx/monitor.h:29: // standard monitors (which is around 100~120dpi.) For
aura, this
don't mention aura here, just note that the return value depends on platform.

https://chromiumcodereview.appspot.com/9960042/diff/2006/ui/gfx/monitor_obser...
File ui/gfx/monitor_observer.h (right):

https://chromiumcodereview.appspot.com/9960042/diff/2006/ui/gfx/monitor_obser...
ui/gfx/monitor_observer.h:27: MonitorObserver();
don't think you need this ctor

https://chromiumcodereview.appspot.com/9960042/diff/2006/ui/gfx/screen.h
File ui/gfx/screen.h (right):

https://chromiumcodereview.appspot.com/9960042/diff/2006/ui/gfx/screen.h#newc...
ui/gfx/screen.h:76: // A class that provides |gfx::Screen|'s implementation on
aura.
yes please move to a separate file. note that you can inline the dtors.

https://chromiumcodereview.appspot.com/9960042/diff/2006/ui/gfx/screen_aura.cc
File ui/gfx/screen_aura.cc (right):

https://chromiumcodereview.appspot.com/9960042/diff/2006/ui/gfx/screen_aura.c...
ui/gfx/screen_aura.cc:16: ScreenImpl* instance_ = NULL;
g_instance

https://chromiumcodereview.appspot.com/9960042/diff/2006/ui/gfx/screen_win.cc
File ui/gfx/screen_win.cc (right):

https://chromiumcodereview.appspot.com/9960042/diff/2006/ui/gfx/screen_win.cc...
ui/gfx/screen_win.cc:14: gfx::SimpleMonitor* monitor = NULL;
g_monitor

https://chromiumcodereview.appspot.com/9960042/diff/2006/ui/gfx/screen_win.cc...
ui/gfx/screen_win.cc:25: monitor = new gfx::SimpleMonitor();
nit: no braces

https://chromiumcodereview.appspot.com/9960042/diff/2006/ui/gfx/screen_win.cc...
ui/gfx/screen_win.cc:27: monitor->set_bounds(gfx::Rect());
since you're using a global, if anyone stores a copy of the gfx::Monitor* they
get from any of the Screen functions, and then someone else calls GetMonitor for
a different mi, the other place's monitor is going to change out from under
them. seems like you have to create a new monitor every time when you return it.

alternatively you could replace your virtual monitor interface with a details
struct like what windows uses (MONITORINFO) and just have the functions below
populate this struct on each platform.

https://chromiumcodereview.appspot.com/9960042/diff/2006/ui/gfx/screen_win.cc...
ui/gfx/screen_win.cc:34: monitor = new gfx::SimpleMonitor();
nit: no braces

[Fady Samuel, 2012-04-10 16:58:09]

A couple of drive-by general comments. They're not necessary related to this
particular change. Feel free to ignore for this cl.

http://codereview.chromium.org/9960042/diff/2006/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_aura.cc (left):

http://codereview.chromium.org/9960042/diff/2006/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_aura.cc:1256: //
TODO(derat): Don't hardcode this?
Drive-by: Could we please not hardcode this anymore so that WebKit does the
right thing on hardware without passing in command line flags?

http://codereview.chromium.org/9960042/diff/2006/ui/aura/monitor_manager.cc
File ui/aura/monitor_manager.cc (right):

http://codereview.chromium.org/9960042/diff/2006/ui/aura/monitor_manager.cc#n...
ui/aura/monitor_manager.cc:35: if (sscanf(spec.c_str(), "%dx%d*%f", &width,
&height, &scale) == 3) {
Where is this spec coming from? Are you reading the value from the
--default-device-scale-factor flag? We would like to be able to force a
"high-DPI" mode on development workstations.

[oshima, 2012-04-10 21:31:46]

Uploaded new patch. PTAL.

http://codereview.chromium.org/9960042/diff/2006/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_aura.cc (left):

http://codereview.chromium.org/9960042/diff/2006/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_aura.cc:1256: //
TODO(derat): Don't hardcode this?
On 2012/04/10 16:58:09, Fady Samuel wrote:
> Drive-by: Could we please not hardcode this anymore so that WebKit does the
> right thing on hardware without passing in command line flags?

Since it's unrelated to this CL, I'd prefer it to be in separate CL.

http://codereview.chromium.org/9960042/diff/2006/ui/aura/monitor_manager.cc
File ui/aura/monitor_manager.cc (right):

http://codereview.chromium.org/9960042/diff/2006/ui/aura/monitor_manager.cc#n...
ui/aura/monitor_manager.cc:35: if (sscanf(spec.c_str(), "%dx%d*%f", &width,
&height, &scale) == 3) {
On 2012/04/10 16:58:09, Fady Samuel wrote:
> Where is this spec coming from? Are you reading the value from the
> --default-device-scale-factor flag? We would like to be able to force a
> "high-DPI" mode on development workstations.

The value comes from --aura-host-window-size.
I want this to be per monitor because I need to support multi-density
environment.
It needs to be renamed to something more relevant though.

http://codereview.chromium.org/9960042/diff/2006/ui/gfx/monitor.h
File ui/gfx/monitor.h (right):

http://codereview.chromium.org/9960042/diff/2006/ui/gfx/monitor.h#newcode29
ui/gfx/monitor.h:29: // standard monitors (which is around 100~120dpi.) For
aura, this
On 2012/04/10 16:37:20, Ben Goodger (Google) wrote:
> don't mention aura here, just note that the return value depends on platform.

Done.

http://codereview.chromium.org/9960042/diff/2006/ui/gfx/monitor_observer.h
File ui/gfx/monitor_observer.h (right):

http://codereview.chromium.org/9960042/diff/2006/ui/gfx/monitor_observer.h#ne...
ui/gfx/monitor_observer.h:27: MonitorObserver();
On 2012/04/10 16:37:20, Ben Goodger (Google) wrote:
> don't think you need this ctor

Done.

http://codereview.chromium.org/9960042/diff/2006/ui/gfx/screen.h
File ui/gfx/screen.h (right):

http://codereview.chromium.org/9960042/diff/2006/ui/gfx/screen.h#newcode76
ui/gfx/screen.h:76: // A class that provides |gfx::Screen|'s implementation on
aura.
On 2012/04/10 16:37:20, Ben Goodger (Google) wrote:
> yes please move to a separate file. note that you can inline the dtors.

Done.

http://codereview.chromium.org/9960042/diff/2006/ui/gfx/screen_aura.cc
File ui/gfx/screen_aura.cc (right):

http://codereview.chromium.org/9960042/diff/2006/ui/gfx/screen_aura.cc#newcode16
ui/gfx/screen_aura.cc:16: ScreenImpl* instance_ = NULL;
On 2012/04/10 16:37:20, Ben Goodger (Google) wrote:
> g_instance

Done.

http://codereview.chromium.org/9960042/diff/2006/ui/gfx/screen_win.cc
File ui/gfx/screen_win.cc (right):

http://codereview.chromium.org/9960042/diff/2006/ui/gfx/screen_win.cc#newcode14
ui/gfx/screen_win.cc:14: gfx::SimpleMonitor* monitor = NULL;
On 2012/04/10 16:37:20, Ben Goodger (Google) wrote:
> g_monitor

Done.

http://codereview.chromium.org/9960042/diff/2006/ui/gfx/screen_win.cc#newcode25
ui/gfx/screen_win.cc:25: monitor = new gfx::SimpleMonitor();
On 2012/04/10 16:37:20, Ben Goodger (Google) wrote:
> nit: no braces

Done.

http://codereview.chromium.org/9960042/diff/2006/ui/gfx/screen_win.cc#newcode27
ui/gfx/screen_win.cc:27: monitor->set_bounds(gfx::Rect());
On 2012/04/10 16:37:20, Ben Goodger (Google) wrote:
> since you're using a global, if anyone stores a copy of the gfx::Monitor* they
> get from any of the Screen functions, and then someone else calls GetMonitor
for
> a different mi, the other place's monitor is going to change out from under
> them. seems like you have to create a new monitor every time when you return
it.
> 
> alternatively you could replace your virtual monitor interface with a details
> struct like what windows uses (MONITORINFO) and just have the functions below
> populate this struct on each platform.

 This is temporary until we have MonitorObserver implementation on other
platorms
(see my comment in screen.h)
 I'm working with Panels team to fix this now, as they need this feature too
to reposition panels when monitor changes. Notification is also somewhat broken
when monitor changes due to the lack of this feature on other platforms.
I filed a bug (crbug.com/122863) and put it to the comment in screen.h

http://codereview.chromium.org/9960042/diff/2006/ui/gfx/screen_win.cc#newcode34
ui/gfx/screen_win.cc:34: monitor = new gfx::SimpleMonitor();
On 2012/04/10 16:37:20, Ben Goodger (Google) wrote:
> nit: no braces

Done.

[Ben Goodger (Google), 2012-04-11 21:05:58]

http://codereview.chromium.org/9960042/diff/14001/ui/gfx/screen_win.cc
File ui/gfx/screen_win.cc (right):

http://codereview.chromium.org/9960042/diff/14001/ui/gfx/screen_win.cc#newcode23
ui/gfx/screen_win.cc:23: gfx::Monitor* GetEmptyMonitor() {
So even though you say it's temporary, it really troubles me that we'd add code
that has this bug. It means that code even within the same call stack could have
unpredictable behavior if any of these functions are called with different
monitors. I can't guarantee that doesn't happen in Chrome today, so as a result
I'm not really comfortable with this going in even temporarily.

Here's my question.

Monitor is actually pretty simple. It's bounds, work area dimensions, scale
factor etc. i.e. it's just data. It should just be a struct, like MONITORINFO,
but cross platform. The gfx::Screen methods that currently return a
gfx::Monitor* could take a gfx::Monitor* out param and prefill the struct the
caller holds on the stack.

If you need this struct to carry a handle to the native monitor you can invent a
typedef for that too.

[oshima, 2012-04-11 22:05:06]

http://codereview.chromium.org/9960042/diff/14001/ui/gfx/screen_win.cc
File ui/gfx/screen_win.cc (right):

http://codereview.chromium.org/9960042/diff/14001/ui/gfx/screen_win.cc#newcode23
ui/gfx/screen_win.cc:23: gfx::Monitor* GetEmptyMonitor() {
On 2012/04/11 21:05:58, Ben Goodger (Google) wrote:
> So even though you say it's temporary, it really troubles me that we'd add
code
> that has this bug. It means that code even within the same call stack could
have
> unpredictable behavior if any of these functions are called with different
> monitors. I can't guarantee that doesn't happen in Chrome today, so as a
result
> I'm not really comfortable with this going in even temporarily.
> 
> Here's my question.
> 
> Monitor is actually pretty simple. It's bounds, work area dimensions, scale
> factor etc. i.e. it's just data. It should just be a struct, like MONITORINFO,
> but cross platform. The gfx::Screen methods that currently return a
> gfx::Monitor* could take a gfx::Monitor* out param and prefill the struct the
> caller holds on the stack.
> 
> If you need this struct to carry a handle to the native monitor you can invent
a
> typedef for that too.

Ok, that's good suggestion. I'll make that change.

[oshima, 2012-04-13 19:43:01]

Uploaded new patch. PTAL.

* Made gfx::Monitor POD like object.
* Moved MonitorObserver back to ui/aura because. It's not clear how this will
work on other platforms and probably better to keep it i ui/aura until we have
implementation on other platforms.

[Ben Goodger (Google), 2012-04-13 19:57:19]

http://codereview.chromium.org/9960042/diff/32002/ui/aura/monitor_aura.h
File ui/aura/monitor_aura.h (right):

http://codereview.chromium.org/9960042/diff/32002/ui/aura/monitor_aura.h#newc...
ui/aura/monitor_aura.h:20: class AURA_EXPORT MonitorAura {
What's the purpose of this class when you have gfx::Monitor? It looks pretty
much identical.

http://codereview.chromium.org/9960042/diff/32002/ui/gfx/monitor.h
File ui/gfx/monitor.h (right):

http://codereview.chromium.org/9960042/diff/32002/ui/gfx/monitor.h#newcode23
ui/gfx/monitor.h:23: class UI_EXPORT Monitor {
this could just be a struct. you can skip the setters.

e.g. 

struct UI_EXPORT Monitor {
  gfx::Rect bounds;
  gfx::Rect work_area;
  float device_scale_factor;
}

http://codereview.chromium.org/9960042/diff/32002/ui/gfx/screen.h
File ui/gfx/screen.h (right):

http://codereview.chromium.org/9960042/diff/32002/ui/gfx/screen.h#newcode50
ui/gfx/screen.h:50: gfx::Monitor* monitor_out);
Now that I've seen all the call sites, I think this would work nicer as:

static gfx::Monitor GetMonitorNearestPoint(const gfx::Point& point);

gfx::Monitor is tiny, so returning a copy of it isn't the end of the world. This
would change the call sites from:

gfx::Monitor monitor;
gfx::Screen::MonitorNearestPoint(point, &monitor);
return monitor.work_area();

to just:

return gfx::Screen::GetMonitorNearestPoint(point).work_area;

You can apply this to the other methods here too that take a Monitor out param.

http://codereview.chromium.org/9960042/diff/32002/ui/gfx/screen_aura.cc
File ui/gfx/screen_aura.cc (right):

http://codereview.chromium.org/9960042/diff/32002/ui/gfx/screen_aura.cc#newco...
ui/gfx/screen_aura.cc:38: if (!g_instance_)
Is this for gfx tests? Can you just provide a stub impl nearby for this purpose?

[oshima, 2012-04-13 21:18:40]

http://codereview.chromium.org/9960042/diff/32002/ui/aura/monitor_aura.h
File ui/aura/monitor_aura.h (right):

http://codereview.chromium.org/9960042/diff/32002/ui/aura/monitor_aura.h#newc...
ui/aura/monitor_aura.h:20: class AURA_EXPORT MonitorAura {
On 2012/04/13 19:57:19, Ben Goodger (Google) wrote:
> What's the purpose of this class when you have gfx::Monitor? It looks pretty
> much identical.

WorkArea is computed from bounds and insets on Aura, which is set by WM.
Alternatively, I can compute the insets on the fly when bounds changes.
I'm fine with either way, so let me know which way you prefer.

http://codereview.chromium.org/9960042/diff/32002/ui/gfx/monitor.h
File ui/gfx/monitor.h (right):

http://codereview.chromium.org/9960042/diff/32002/ui/gfx/monitor.h#newcode23
ui/gfx/monitor.h:23: class UI_EXPORT Monitor {
On 2012/04/13 19:57:19, Ben Goodger (Google) wrote:
> this could just be a struct. you can skip the setters.
> 
> e.g. 
> 
> struct UI_EXPORT Monitor {
>   gfx::Rect bounds;
>   gfx::Rect work_area;
>   float device_scale_factor;
> }

I was going to add a few methods, such as method that returns DIP
in addition to Pixels, a method that converts Pixels to DIP.
If it's ok to make it struct with these methods, I can.

If not, other options I can think of are

1) Add fields for DIP.
2) Let caller handle DIP.
3) Add GetMonitorXXInPixels() to gfx::Screen that returns Monitor with pixels
coordinate system.

These doesn't seem to look nice though. Let me know what you think.

http://codereview.chromium.org/9960042/diff/32002/ui/gfx/screen.h
File ui/gfx/screen.h (right):

http://codereview.chromium.org/9960042/diff/32002/ui/gfx/screen.h#newcode50
ui/gfx/screen.h:50: gfx::Monitor* monitor_out);
On 2012/04/13 19:57:19, Ben Goodger (Google) wrote:
> Now that I've seen all the call sites, I think this would work nicer as:
> 
> static gfx::Monitor GetMonitorNearestPoint(const gfx::Point& point);
> 
> gfx::Monitor is tiny, so returning a copy of it isn't the end of the world.
This
> would change the call sites from:
> 
> gfx::Monitor monitor;
> gfx::Screen::MonitorNearestPoint(point, &monitor);
> return monitor.work_area();
> 
> to just:
> 
> return gfx::Screen::GetMonitorNearestPoint(point).work_area;
> 
> You can apply this to the other methods here too that take a Monitor out
param.

Ok, will do that.

http://codereview.chromium.org/9960042/diff/32002/ui/gfx/screen_aura.cc
File ui/gfx/screen_aura.cc (right):

http://codereview.chromium.org/9960042/diff/32002/ui/gfx/screen_aura.cc#newco...
ui/gfx/screen_aura.cc:38: if (!g_instance_)
On 2012/04/13 19:57:19, Ben Goodger (Google) wrote:
> Is this for gfx tests? Can you just provide a stub impl nearby for this
purpose?

I think erg added this. Elliot, do we still need this?

[oshima, 2012-04-14 15:23:36]

Uploaded new patch. PTAL.

- I kept Monitor class as discussed.
- aura::MonitorAura is merged into gfx::Monitor
- gfx::Screen now returns gfx::Monitor instance instead of the pointer.
- Removed utility functions and updated all use of gfx::Screen::GetMonitorXXX to
new API.

http://codereview.chromium.org/9960042/diff/32002/ui/gfx/screen_aura.cc
File ui/gfx/screen_aura.cc (right):

http://codereview.chromium.org/9960042/diff/32002/ui/gfx/screen_aura.cc#newco...
ui/gfx/screen_aura.cc:38: if (!g_instance_)
talked to erg and this is no longer necessary. Removed.

On 2012/04/13 21:18:41, oshima wrote:
> On 2012/04/13 19:57:19, Ben Goodger (Google) wrote:
> > Is this for gfx tests? Can you just provide a stub impl nearby for this
> purpose?
> 
> I think erg added this. Elliot, do we still need this?

[oshima, 2012-04-24 16:28:17]

Ping? I updated screen_aurax11 to match new API. This needs to be converted to a
subclass of ScreenImpl with multi monitor support,
and I'll do that in follow up CL.

On 2012/04/14 15:23:36, oshima wrote:
> Uploaded new patch. PTAL.
> 
> - I kept Monitor class as discussed.
> - aura::MonitorAura is merged into gfx::Monitor
> - gfx::Screen now returns gfx::Monitor instance instead of the pointer.
> - Removed utility functions and updated all use of gfx::Screen::GetMonitorXXX
to
> new API.
> 
> http://codereview.chromium.org/9960042/diff/32002/ui/gfx/screen_aura.cc
> File ui/gfx/screen_aura.cc (right):
> 
>
http://codereview.chromium.org/9960042/diff/32002/ui/gfx/screen_aura.cc#newco...
> ui/gfx/screen_aura.cc:38: if (!g_instance_)
> talked to erg and this is no longer necessary. Removed.
> 
> On 2012/04/13 21:18:41, oshima wrote:
> > On 2012/04/13 19:57:19, Ben Goodger (Google) wrote:
> > > Is this for gfx tests? Can you just provide a stub impl nearby for this
> > purpose?
> > 
> > I think erg added this. Elliot, do we still need this?

[Elliot Glaysher, 2012-04-24 16:48:24]

On 2012/04/24 16:28:17, oshima wrote:
> Ping? I updated screen_aurax11 to match new API. This needs to be converted to
a
> subclass of ScreenImpl with multi monitor support,
> and I'll do that in follow up CL.
> 
> On 2012/04/14 15:23:36, oshima wrote:
> > Uploaded new patch. PTAL.
> > 
> > - I kept Monitor class as discussed.
> > - aura::MonitorAura is merged into gfx::Monitor
> > - gfx::Screen now returns gfx::Monitor instance instead of the pointer.
> > - Removed utility functions and updated all use of
gfx::Screen::GetMonitorXXX
> to
> > new API.
> > 
> > http://codereview.chromium.org/9960042/diff/32002/ui/gfx/screen_aura.cc
> > File ui/gfx/screen_aura.cc (right):
> > 
> >
>
http://codereview.chromium.org/9960042/diff/32002/ui/gfx/screen_aura.cc#newco...
> > ui/gfx/screen_aura.cc:38: if (!g_instance_)
> > talked to erg and this is no longer necessary. Removed.
> > 
> > On 2012/04/13 21:18:41, oshima wrote:
> > > On 2012/04/13 19:57:19, Ben Goodger (Google) wrote:
> > > > Is this for gfx tests? Can you just provide a stub impl nearby for this
> > > purpose?
> > > 
> > > I think erg added this. Elliot, do we still need this?

Temporary aurax11 implementation lgtm.

[Ben Goodger (Google), 2012-04-24 21:17:07]

My general comment given the simplicity of Monitor is to make it a type that is
copied/passed by reference like Rect, which seems an analog. I have reflected
this in comments here which are not exhaustive for this CL. You might need to
add an id field to monitor to disambiguate monitors.

http://codereview.chromium.org/9960042/diff/57001/ui/aura/monitor_manager.cc
File ui/aura/monitor_manager.cc (right):

http://codereview.chromium.org/9960042/diff/57001/ui/aura/monitor_manager.cc#...
ui/aura/monitor_manager.cc:73: void MonitorManager::NotifyBoundsChanged(const
gfx::Monitor* monitor) {
const gfx::Monitor& monitor

http://codereview.chromium.org/9960042/diff/57001/ui/aura/monitor_manager.cc#...
ui/aura/monitor_manager.cc:78: void
MonitorManager::NotifyMonitorAdded(gfx::Monitor* monitor) {
etc

http://codereview.chromium.org/9960042/diff/57001/ui/aura/monitor_manager.cc#...
ui/aura/monitor_manager.cc:83: void MonitorManager::NotifyMonitorRemoved(const
gfx::Monitor* monitor) {
etc

http://codereview.chromium.org/9960042/diff/57001/ui/aura/monitor_manager.h
File ui/aura/monitor_manager.h (right):

http://codereview.chromium.org/9960042/diff/57001/ui/aura/monitor_manager.h#n...
ui/aura/monitor_manager.h:61: const std::vector<const gfx::Monitor*>& monitors)
= 0;
const std::vector<gfx::Monitor>& monitors

http://codereview.chromium.org/9960042/diff/57001/ui/aura/monitor_manager.h#n...
ui/aura/monitor_manager.h:64: virtual RootWindow*
CreateRootWindowForMonitor(gfx::Monitor* monitor) = 0;
const gfx::Monitor& monitor

http://codereview.chromium.org/9960042/diff/57001/ui/aura/monitor_observer.h
File ui/aura/monitor_observer.h (right):

http://codereview.chromium.org/9960042/diff/57001/ui/aura/monitor_observer.h#...
ui/aura/monitor_observer.h:21: virtual void OnMonitorBoundsChanged(const
gfx::Monitor* monitor) = 0;
const gfx::Monitor& monitor

for all methods in this class.

http://codereview.chromium.org/9960042/diff/57001/ui/gfx/monitor.h
File ui/gfx/monitor.h (right):

http://codereview.chromium.org/9960042/diff/57001/ui/gfx/monitor.h#newcode16
ui/gfx/monitor.h:16: // Note: The screen and monitor currently uses pixels
coordinate
pixel

http://codereview.chromium.org/9960042/diff/57001/ui/gfx/monitor.h#newcode67
ui/gfx/monitor.h:67: };
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/9960042/diff/57001/ui/gfx/screen_aura.cc
File ui/gfx/screen_aura.cc (right):

http://codereview.chromium.org/9960042/diff/57001/ui/gfx/screen_aura.cc#newco...
ui/gfx/screen_aura.cc:26: #if defined(USE_ASH)
ui/gfx shouldn't have any knowledge of ash.

[oshima, 2012-04-25 02:38:01]

http://codereview.chromium.org/9960042/diff/57001/ui/aura/monitor_manager.cc
File ui/aura/monitor_manager.cc (right):

http://codereview.chromium.org/9960042/diff/57001/ui/aura/monitor_manager.cc#...
ui/aura/monitor_manager.cc:73: void MonitorManager::NotifyBoundsChanged(const
gfx::Monitor* monitor) {
On 2012/04/24 21:17:08, Ben Goodger (Google) wrote:
> const gfx::Monitor& monitor

Done.

http://codereview.chromium.org/9960042/diff/57001/ui/aura/monitor_manager.cc#...
ui/aura/monitor_manager.cc:78: void
MonitorManager::NotifyMonitorAdded(gfx::Monitor* monitor) {
On 2012/04/24 21:17:08, Ben Goodger (Google) wrote:
> etc

Done.

http://codereview.chromium.org/9960042/diff/57001/ui/aura/monitor_manager.cc#...
ui/aura/monitor_manager.cc:83: void MonitorManager::NotifyMonitorRemoved(const
gfx::Monitor* monitor) {
On 2012/04/24 21:17:08, Ben Goodger (Google) wrote:
> etc

Done.

http://codereview.chromium.org/9960042/diff/57001/ui/aura/monitor_manager.h
File ui/aura/monitor_manager.h (right):

http://codereview.chromium.org/9960042/diff/57001/ui/aura/monitor_manager.h#n...
ui/aura/monitor_manager.h:61: const std::vector<const gfx::Monitor*>& monitors)
= 0;
On 2012/04/24 21:17:08, Ben Goodger (Google) wrote:
> const std::vector<gfx::Monitor>& monitors

Done.

http://codereview.chromium.org/9960042/diff/57001/ui/aura/monitor_manager.h#n...
ui/aura/monitor_manager.h:64: virtual RootWindow*
CreateRootWindowForMonitor(gfx::Monitor* monitor) = 0;
On 2012/04/24 21:17:08, Ben Goodger (Google) wrote:
> const gfx::Monitor& monitor

Done.

http://codereview.chromium.org/9960042/diff/57001/ui/aura/monitor_observer.h
File ui/aura/monitor_observer.h (right):

http://codereview.chromium.org/9960042/diff/57001/ui/aura/monitor_observer.h#...
ui/aura/monitor_observer.h:21: virtual void OnMonitorBoundsChanged(const
gfx::Monitor* monitor) = 0;
On 2012/04/24 21:17:08, Ben Goodger (Google) wrote:
> const gfx::Monitor& monitor
> 
> for all methods in this class.

Done.

http://codereview.chromium.org/9960042/diff/57001/ui/gfx/monitor.h
File ui/gfx/monitor.h (right):

http://codereview.chromium.org/9960042/diff/57001/ui/gfx/monitor.h#newcode16
ui/gfx/monitor.h:16: // Note: The screen and monitor currently uses pixels
coordinate
On 2012/04/24 21:17:08, Ben Goodger (Google) wrote:
> pixel

Done.

http://codereview.chromium.org/9960042/diff/57001/ui/gfx/monitor.h#newcode67
ui/gfx/monitor.h:67: };
On 2012/04/24 21:17:08, Ben Goodger (Google) wrote:
> DISALLOW_COPY_AND_ASSIGN

We do allow copy and assign for gfx::Rect/gfx::Size etc and I need them
in order to use it in stl classes.

http://codereview.chromium.org/9960042/diff/57001/ui/gfx/screen_aura.cc
File ui/gfx/screen_aura.cc (right):

http://codereview.chromium.org/9960042/diff/57001/ui/gfx/screen_aura.cc#newco...
ui/gfx/screen_aura.cc:26: #if defined(USE_ASH)
On 2012/04/24 21:17:08, Ben Goodger (Google) wrote:
> ui/gfx shouldn't have any knowledge of ash.

This is unfortunate side effect of USE_AURA in screen_aurax11.cc. I will get rid
of this in follow CL that replaces screen_aurax11 with proper implementation
that supports multiple monitor for linux/aura.

[Ben Goodger (Google), 2012-04-25 19:38:03]

LGTM