Add context to gfx::Screen calls in support of simultaneous desktop+ash

Makes gfx::Screen an instance, rather than a collection of static methods.

This is in support of supporting separate Screen implementations on Aura for desktop and metro on Windows.

Some callsites are not yet correct, and noted with a reference to the http://crbug.com/133312. As-is those sites will behave the same as before this patch, but may not be correct once desktop/metro can run simultaneously.


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

[scottmg, 2012-10-04 00:15:37]

(not ready for full review, just looking for some feedback)

Feeling optimistic today, I thought I'd just start whacking away at the two
worlds desktop/metro and especially gfx::Screen not having any context.

Mostly, of the files add context to gfx::Screen so it knows whether coordinates
are referring to desktop windows or ash windows.

In ash/ code, it can be given an Ash root window, so that code is probably
correct. (?)

In chrome, I didn't try to address all of the callsites yet, so unless they had
an obviously correct NativeView available, I simply modified them to pass
Screen::BadTwoWorldsContext() as a placeholder that can be fixed as we go. It
seems like some of them will need quite a bit more plumbing to get a correct
NativeView. I also haven't updated all the _unittest files yet.


Before I go farther this way, does this seem like a reasonable patch? Or is
there something less invasive I could try instead?

[Ben Goodger (Google), 2012-10-04 15:38:37]

yeah i suppose this is fine. i'll defer to oshima though wrt screen stuff in
ash.

some day i hope we'll come up with a nicer way to handle all of this.

http://codereview.chromium.org/11030017/diff/8001/ash/display/display_control...
File ash/display/display_controller.cc (right):

http://codereview.chromium.org/11030017/diff/8001/ash/display/display_control...
ash/display/display_controller.cc:472: if
(gfx::Screen::GetNumDisplays(ash::Shell::GetRootNativeView()) <= 1)
is it acceptable to use the active root window in these cases? what did oshima
think of this?

[scottmg, 2012-10-04 16:36:53]

On 2012/10/04 15:38:37, Ben Goodger (Google) wrote:
> yeah i suppose this is fine. i'll defer to oshima though wrt screen stuff in
> ash.
> 
> some day i hope we'll come up with a nicer way to handle all of this.
> 
>
http://codereview.chromium.org/11030017/diff/8001/ash/display/display_control...
> File ash/display/display_controller.cc (right):
> 
>
http://codereview.chromium.org/11030017/diff/8001/ash/display/display_control...
> ash/display/display_controller.cc:472: if
> (gfx::Screen::GetNumDisplays(ash::Shell::GetRootNativeView()) <= 1)
> is it acceptable to use the active root window in these cases? what did oshima
> think of this?

It generally doesn't really matter which Root it is, it's just so that Screen
can dispatch to the Ash implementation of ScreenImpl, rather than the Desktop
ScreenImpl. (see
http://codereview.chromium.org/11030017/diff/8001/ui/gfx/screen_aura.cc
GetImplForContext).

It'd probably be clearer in the ash code to use something like SCREEN_ID_ASH
rather than passing a NativeView. In chrome and ui code though, it doesn't know
whether it should be desktop or ash, so it would have to use something like a
GetScreenIdFromNativeView helper there.

[Ben Goodger (Google), 2012-10-04 16:43:11]

ic.


On Thu, Oct 4, 2012 at 9:36 AM, <scottmg@chromium.org> wrote:

> On 2012/10/04 15:38:37, Ben Goodger (Google) wrote:
>
>> yeah i suppose this is fine. i'll defer to oshima though wrt screen stuff
>> in
>> ash.
>>
>
>  some day i hope we'll come up with a nicer way to handle all of this.
>>
>
>
> http://codereview.chromium.**org/11030017/diff/8001/ash/**
>
display/display_controller.cc<http://codereview.chromium.org/11030017/diff/8001/ash/display/display_controller.cc>
>
>> File ash/display/display_**controller.cc (right):
>>
>
>
> http://codereview.chromium.**org/11030017/diff/8001/ash/**
>
display/display_controller.cc#**newcode472<http://codereview.chromium.org/11030017/diff/8001/ash/display/display_controller.cc#newcode472>
>
>> ash/display/display_**controller.cc:472: if
>> (gfx::Screen::GetNumDisplays(**ash::Shell::GetRootNativeView(**)) <= 1)
>> is it acceptable to use the active root window in these cases? what did
>> oshima
>> think of this?
>>
>
> It generally doesn't really matter which Root it is, it's just so that
> Screen
> can dispatch to the Ash implementation of ScreenImpl, rather than the
> Desktop
> ScreenImpl. (see
> http://codereview.chromium.**org/11030017/diff/8001/ui/gfx/**
>
screen_aura.cc<http://codereview.chromium.org/11030017/diff/8001/ui/gfx/screen_aura.cc>
> GetImplForContext).
>
> It'd probably be clearer in the ash code to use something like
> SCREEN_ID_ASH
> rather than passing a NativeView. In chrome and ui code though, it doesn't
> know
> whether it should be desktop or ash, so it would have to use something
> like a
> GetScreenIdFromNativeView helper there.
>
>
>
http://codereview.chromium.**org/11030017/<http://codereview.chromium.org/110...
>

[oshima, 2012-10-04 20:18:26]

Thanks for writing this up.

After looking at this, I now feel that it may be better to make
Screen an object. Screen object should be permanent and we can
guarante that they exist while UI code is running.

Screen::GetScreenOf(aura::NativeView* view)->GetDisplayNearestPoint(point);

or you can keep Screen object around in objects.

In ash, all ash code must use Ash's screen, so we can simplify it by
providing accessor like:

ash::Shell::GetAshScreen() or ash::GetAshScreen

If there is a place that always assume native platform's
screen, then you can provide GetNativeSystemScreen() too.
(For desktop integration for example)

Let me know what you think. (or I'm happy to discuss offline)

http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/panels/p...
File chrome/browser/ui/panels/panel_mouse_watcher_timer.cc (right):

http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_mouse_watcher_timer.cc:67: gfx::Point
PanelMouseWatcherTimer::GetMousePosition() const {
I guess we'll need separate wachertimer per screen?

http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/startup/...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/startup/...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:445:
gfx::Screen::BadTwoWorldsContext()).work_area();
I guess session restore has to remember in which screen restoring window belongs
to?

http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/views/ap...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:177:
gfx::Screen::BadTwoWorldsContext(), anchor);
this can be view->GetWidget()->GetNativeVew() correct?

http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/views/ch...
File chrome/browser/ui/views/chrome_views_delegate.cc (right):

http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/views/ch...
chrome/browser/ui/views/chrome_views_delegate.cc:83:
gfx::Screen::BadTwoWorldsContext(), bounds).work_area());
shouldn't this be |window|?

http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/extension_dialog.cc (right):

http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/extension_dialog.cc:168:
gfx::Screen::BadTwoWorldsContext(), center).bounds();
base_window?

http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/views/fu...
File chrome/browser/ui/views/fullscreen_exit_bubble_views.cc (right):

http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/views/fu...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:361:
gfx::Screen::BadTwoWorldsContext());
root_view_->GetWidget()->GetNativeView()?

http://codereview.chromium.org/11030017/diff/15077/content/browser/web_conten...
File content/browser/web_contents/web_contents_view_aura.cc (right):

http://codereview.chromium.org/11030017/diff/15077/content/browser/web_conten...
content/browser/web_contents/web_contents_view_aura.cc:273: aura::Window* window
= rvh->GetView()->GetNativeView();
you can use this |window| as a reference.

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

http://codereview.chromium.org/11030017/diff/15077/ui/gfx/screen_aura.cc#newc...
ui/gfx/screen_aura.cc:15: #if defined(USE_AURA)
this is aura only file.

http://codereview.chromium.org/11030017/diff/8003/ui/gfx/screen_type_delegate.h
File ui/gfx/screen_type_delegate.h (right):

http://codereview.chromium.org/11030017/diff/8003/ui/gfx/screen_type_delegate...
ui/gfx/screen_type_delegate.h:15: SCREEN_TYPE_COUNT,
_COUNT is a bit confusing to me. How about _LAST _END?

[Ben Goodger (Google), 2012-10-04 20:28:50]

Yeah perhaps this is inevitable.

-Ben


On Thu, Oct 4, 2012 at 1:18 PM, <oshima@chromium.org> wrote:

> Thanks for writing this up.
>
> After looking at this, I now feel that it may be better to make
> Screen an object. Screen object should be permanent and we can
> guarante that they exist while UI code is running.
>
> Screen::GetScreenOf(aura::**NativeView* view)->GetDisplayNearestPoint(**
> point);
>
> or you can keep Screen object around in objects.
>
> In ash, all ash code must use Ash's screen, so we can simplify it by
> providing accessor like:
>
> ash::Shell::GetAshScreen() or ash::GetAshScreen
>
> If there is a place that always assume native platform's
> screen, then you can provide GetNativeSystemScreen() too.
> (For desktop integration for example)
>
> Let me know what you think. (or I'm happy to discuss offline)
>
>
>
>
> http://codereview.chromium.**org/11030017/diff/15077/**
>
chrome/browser/ui/panels/**panel_mouse_watcher_timer.cc<http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/panels/panel_mouse_watcher_timer.cc>
> File chrome/browser/ui/panels/**panel_mouse_watcher_timer.cc (right):
>
> http://codereview.chromium.**org/11030017/diff/15077/**
>
chrome/browser/ui/panels/**panel_mouse_watcher_timer.cc#**newcode67<http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/panels/panel_mouse_watcher_timer.cc#newcode67>
> chrome/browser/ui/panels/**panel_mouse_watcher_timer.cc:**67: gfx::Point
> PanelMouseWatcherTimer::**GetMousePosition() const {
> I guess we'll need separate wachertimer per screen?
>
> http://codereview.chromium.**org/11030017/diff/15077/**
>
chrome/browser/ui/startup/**startup_browser_creator_impl.**cc<http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/startup/startup_browser_creator_impl.cc>
> File chrome/browser/ui/startup/**startup_browser_creator_impl.**cc
> (right):
>
> http://codereview.chromium.**org/11030017/diff/15077/**
>
chrome/browser/ui/startup/**startup_browser_creator_impl.**cc#newcode445<http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/startup/startup_browser_creator_impl.cc#newcode445>
> chrome/browser/ui/startup/**startup_browser_creator_impl.**cc:445:
> gfx::Screen::**BadTwoWorldsContext()).work_**area();
> I guess session restore has to remember in which screen restoring window
> belongs to?
>
> http://codereview.chromium.**org/11030017/diff/15077/**
>
chrome/browser/ui/views/app_**list/app_list_controller_win.**cc<http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/views/app_list/app_list_controller_win.cc>
> File chrome/browser/ui/views/app_**list/app_list_controller_win.**cc
> (right):
>
> http://codereview.chromium.**org/11030017/diff/15077/**
>
chrome/browser/ui/views/app_**list/app_list_controller_win.**cc#newcode177<http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/views/app_list/app_list_controller_win.cc#newcode177>
> chrome/browser/ui/views/app_**list/app_list_controller_win.**cc:177:
> gfx::Screen::**BadTwoWorldsContext(), anchor);
> this can be view->GetWidget()->**GetNativeVew() correct?
>
> http://codereview.chromium.**org/11030017/diff/15077/**
>
chrome/browser/ui/views/**chrome_views_delegate.cc<http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/views/chrome_views_delegate.cc>
> File chrome/browser/ui/views/**chrome_views_delegate.cc (right):
>
> http://codereview.chromium.**org/11030017/diff/15077/**
>
chrome/browser/ui/views/**chrome_views_delegate.cc#**newcode83<http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/views/chrome_views_delegate.cc#newcode83>
> chrome/browser/ui/views/**chrome_views_delegate.cc:83:
> gfx::Screen::**BadTwoWorldsContext(), bounds).work_area());
> shouldn't this be |window|?
>
> http://codereview.chromium.**org/11030017/diff/15077/**
>
chrome/browser/ui/views/**extensions/extension_dialog.cc<http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/views/extensions/extension_dialog.cc>
> File chrome/browser/ui/views/**extensions/extension_dialog.cc (right):
>
> http://codereview.chromium.**org/11030017/diff/15077/**
>
chrome/browser/ui/views/**extensions/extension_dialog.**cc#newcode168<http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/views/extensions/extension_dialog.cc#newcode168>
> chrome/browser/ui/views/**extensions/extension_dialog.**cc:168:
> gfx::Screen::**BadTwoWorldsContext(), center).bounds();
> base_window?
>
> http://codereview.chromium.**org/11030017/diff/15077/**
>
chrome/browser/ui/views/**fullscreen_exit_bubble_views.**cc<http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/views/fullscreen_exit_bubble_views.cc>
> File chrome/browser/ui/views/**fullscreen_exit_bubble_views.**cc (right):
>
> http://codereview.chromium.**org/11030017/diff/15077/**
>
chrome/browser/ui/views/**fullscreen_exit_bubble_views.**cc#newcode361<http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/views/fullscreen_exit_bubble_views.cc#newcode361>
> chrome/browser/ui/views/**fullscreen_exit_bubble_views.**cc:361:
> gfx::Screen::**BadTwoWorldsContext());
> root_view_->GetWidget()->**GetNativeView()?
>
> http://codereview.chromium.**org/11030017/diff/15077/**
>
content/browser/web_contents/**web_contents_view_aura.cc<http://codereview.chromium.org/11030017/diff/15077/content/browser/web_contents/web_contents_view_aura.cc>
> File content/browser/web_contents/**web_contents_view_aura.cc (right):
>
> http://codereview.chromium.**org/11030017/diff/15077/**
>
content/browser/web_contents/**web_contents_view_aura.cc#**newcode273<http://codereview.chromium.org/11030017/diff/15077/content/browser/web_contents/web_contents_view_aura.cc#newcode273>
> content/browser/web_contents/**web_contents_view_aura.cc:273:
> aura::Window* window = rvh->GetView()->GetNativeView(**);
> you can use this |window| as a reference.
>
> http://codereview.chromium.**org/11030017/diff/15077/ui/**
>
gfx/screen_aura.cc<http://codereview.chromium.org/11030017/diff/15077/ui/gfx/screen_aura.cc>
> File ui/gfx/screen_aura.cc (right):
>
> http://codereview.chromium.**org/11030017/diff/15077/ui/**
>
gfx/screen_aura.cc#newcode15<http://codereview.chromium.org/11030017/diff/15077/ui/gfx/screen_aura.cc#newcode15>
> ui/gfx/screen_aura.cc:15: #if defined(USE_AURA)
> this is aura only file.
>
> http://codereview.chromium.**org/11030017/diff/8003/ui/gfx/**
>
screen_type_delegate.h<http://codereview.chromium.org/11030017/diff/8003/ui/gfx/screen_type_delegate.h>
> File ui/gfx/screen_type_delegate.h (right):
>
> http://codereview.chromium.**org/11030017/diff/8003/ui/gfx/**
>
screen_type_delegate.h#**newcode15<http://codereview.chromium.org/11030017/diff/8003/ui/gfx/screen_type_delegate.h#newcode15>
> ui/gfx/screen_type_delegate.h:**15: SCREEN_TYPE_COUNT,
> _COUNT is a bit confusing to me. How about _LAST _END?
>
>
http://codereview.chromium.**org/11030017/<http://codereview.chromium.org/110...
>

[scottmg, 2012-10-04 21:07:29]

On 2012/10/04 20:18:26, oshima wrote:
> Thanks for writing this up.
> 
> After looking at this, I now feel that it may be better to make
> Screen an object. Screen object should be permanent and we can
> guarante that they exist while UI code is running.
> 
> Screen::GetScreenOf(aura::NativeView* view)->GetDisplayNearestPoint(point);
> 
> or you can keep Screen object around in objects.
> 
> In ash, all ash code must use Ash's screen, so we can simplify it by
> providing accessor like:
> 
> ash::Shell::GetAshScreen() or ash::GetAshScreen
> 
> If there is a place that always assume native platform's
> screen, then you can provide GetNativeSystemScreen() too.
> (For desktop integration for example)
> 
> Let me know what you think. (or I'm happy to discuss offline)

OK, thank you, that sounds fine to me. I'll rework this patch in that style.

"GetScreenOf" sounds bit awkward... maybe just "GetScreen"? or "GetScreenFor"?


> 
>
http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/panels/p...
> File chrome/browser/ui/panels/panel_mouse_watcher_timer.cc (right):
> 
>
http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/panels/p...
> chrome/browser/ui/panels/panel_mouse_watcher_timer.cc:67: gfx::Point
> PanelMouseWatcherTimer::GetMousePosition() const {
> I guess we'll need separate wachertimer per screen?
> 
>
http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/startup/...
> File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):
> 
>
http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/startup/...
> chrome/browser/ui/startup/startup_browser_creator_impl.cc:445:
> gfx::Screen::BadTwoWorldsContext()).work_area();
> I guess session restore has to remember in which screen restoring window
belongs
> to?
> 
>
http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/views/ap...
> File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):
> 
>
http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/views/ap...
> chrome/browser/ui/views/app_list/app_list_controller_win.cc:177:
> gfx::Screen::BadTwoWorldsContext(), anchor);
> this can be view->GetWidget()->GetNativeVew() correct?
> 
>
http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/views/ch...
> File chrome/browser/ui/views/chrome_views_delegate.cc (right):
> 
>
http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/views/ch...
> chrome/browser/ui/views/chrome_views_delegate.cc:83:
> gfx::Screen::BadTwoWorldsContext(), bounds).work_area());
> shouldn't this be |window|?
> 
>
http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/views/ex...
> File chrome/browser/ui/views/extensions/extension_dialog.cc (right):
> 
>
http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/views/ex...
> chrome/browser/ui/views/extensions/extension_dialog.cc:168:
> gfx::Screen::BadTwoWorldsContext(), center).bounds();
> base_window?
> 
>
http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/views/fu...
> File chrome/browser/ui/views/fullscreen_exit_bubble_views.cc (right):
> 
>
http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/views/fu...
> chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:361:
> gfx::Screen::BadTwoWorldsContext());
> root_view_->GetWidget()->GetNativeView()?
> 
>
http://codereview.chromium.org/11030017/diff/15077/content/browser/web_conten...
> File content/browser/web_contents/web_contents_view_aura.cc (right):
> 
>
http://codereview.chromium.org/11030017/diff/15077/content/browser/web_conten...
> content/browser/web_contents/web_contents_view_aura.cc:273: aura::Window*
window
> = rvh->GetView()->GetNativeView();
> you can use this |window| as a reference.
> 
> http://codereview.chromium.org/11030017/diff/15077/ui/gfx/screen_aura.cc
> File ui/gfx/screen_aura.cc (right):
> 
>
http://codereview.chromium.org/11030017/diff/15077/ui/gfx/screen_aura.cc#newc...
> ui/gfx/screen_aura.cc:15: #if defined(USE_AURA)
> this is aura only file.
> 
>
http://codereview.chromium.org/11030017/diff/8003/ui/gfx/screen_type_delegate.h
> File ui/gfx/screen_type_delegate.h (right):
> 
>
http://codereview.chromium.org/11030017/diff/8003/ui/gfx/screen_type_delegate...
> ui/gfx/screen_type_delegate.h:15: SCREEN_TYPE_COUNT,
> _COUNT is a bit confusing to me. How about _LAST _END?

[oshima, 2012-10-04 22:09:40]

On Thu, Oct 4, 2012 at 2:07 PM, <scottmg@chromium.org> wrote:

> On 2012/10/04 20:18:26, oshima wrote:
>
>> Thanks for writing this up.
>>
>
>  After looking at this, I now feel that it may be better to make
>> Screen an object. Screen object should be permanent and we can
>> guarante that they exist while UI code is running.
>>
>
>  Screen::GetScreenOf(aura::**NativeView* view)->GetDisplayNearestPoint(**
>> point);
>>
>
>  or you can keep Screen object around in objects.
>>
>
>  In ash, all ash code must use Ash's screen, so we can simplify it by
>> providing accessor like:
>>
>
>  ash::Shell::GetAshScreen() or ash::GetAshScreen
>>
>
>  If there is a place that always assume native platform's
>> screen, then you can provide GetNativeSystemScreen() too.
>> (For desktop integration for example)
>>
>
>  Let me know what you think. (or I'm happy to discuss offline)
>>
>
> OK, thank you, that sounds fine to me. I'll rework this patch in that
> style.
>
> "GetScreenOf" sounds bit awkward... maybe just "GetScreen"? or
> "GetScreenFor"?
>
> I prefer "GetScreenFor"

- oshima


>
>
>
> http://codereview.chromium.**org/11030017/diff/15077/**
>
chrome/browser/ui/panels/**panel_mouse_watcher_timer.cc<http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/panels/panel_mouse_watcher_timer.cc>
>
>> File chrome/browser/ui/panels/**panel_mouse_watcher_timer.cc (right):
>>
>
>
> http://codereview.chromium.**org/11030017/diff/15077/**
>
chrome/browser/ui/panels/**panel_mouse_watcher_timer.cc#**newcode67<http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/panels/panel_mouse_watcher_timer.cc#newcode67>
>
>> chrome/browser/ui/panels/**panel_mouse_watcher_timer.cc:**67: gfx::Point
>> PanelMouseWatcherTimer::**GetMousePosition() const {
>> I guess we'll need separate wachertimer per screen?
>>
>
>
> http://codereview.chromium.**org/11030017/diff/15077/**
>
chrome/browser/ui/startup/**startup_browser_creator_impl.**cc<http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/startup/startup_browser_creator_impl.cc>
>
>> File chrome/browser/ui/startup/**startup_browser_creator_impl.**cc
>> (right):
>>
>
>
> http://codereview.chromium.**org/11030017/diff/15077/**
>
chrome/browser/ui/startup/**startup_browser_creator_impl.**cc#newcode445<http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/startup/startup_browser_creator_impl.cc#newcode445>
>
>> chrome/browser/ui/startup/**startup_browser_creator_impl.**cc:445:
>> gfx::Screen::**BadTwoWorldsContext()).work_**area();
>> I guess session restore has to remember in which screen restoring window
>>
> belongs
>
>> to?
>>
>
>
> http://codereview.chromium.**org/11030017/diff/15077/**
>
chrome/browser/ui/views/app_**list/app_list_controller_win.**cc<http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/views/app_list/app_list_controller_win.cc>
>
>> File chrome/browser/ui/views/app_**list/app_list_controller_win.**cc
>> (right):
>>
>
>
> http://codereview.chromium.**org/11030017/diff/15077/**
>
chrome/browser/ui/views/app_**list/app_list_controller_win.**cc#newcode177<http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/views/app_list/app_list_controller_win.cc#newcode177>
>
>> chrome/browser/ui/views/app_**list/app_list_controller_win.**cc:177:
>> gfx::Screen::**BadTwoWorldsContext(), anchor);
>> this can be view->GetWidget()->**GetNativeVew() correct?
>>
>
>
> http://codereview.chromium.**org/11030017/diff/15077/**
>
chrome/browser/ui/views/**chrome_views_delegate.cc<http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/views/chrome_views_delegate.cc>
>
>> File chrome/browser/ui/views/**chrome_views_delegate.cc (right):
>>
>
>
> http://codereview.chromium.**org/11030017/diff/15077/**
>
chrome/browser/ui/views/**chrome_views_delegate.cc#**newcode83<http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/views/chrome_views_delegate.cc#newcode83>
>
>> chrome/browser/ui/views/**chrome_views_delegate.cc:83:
>> gfx::Screen::**BadTwoWorldsContext(), bounds).work_area());
>> shouldn't this be |window|?
>>
>
>
> http://codereview.chromium.**org/11030017/diff/15077/**
>
chrome/browser/ui/views/**extensions/extension_dialog.cc<http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/views/extensions/extension_dialog.cc>
>
>> File chrome/browser/ui/views/**extensions/extension_dialog.cc (right):
>>
>
>
> http://codereview.chromium.**org/11030017/diff/15077/**
>
chrome/browser/ui/views/**extensions/extension_dialog.**cc#newcode168<http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/views/extensions/extension_dialog.cc#newcode168>
>
>> chrome/browser/ui/views/**extensions/extension_dialog.**cc:168:
>> gfx::Screen::**BadTwoWorldsContext(), center).bounds();
>> base_window?
>>
>
>
> http://codereview.chromium.**org/11030017/diff/15077/**
>
chrome/browser/ui/views/**fullscreen_exit_bubble_views.**cc<http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/views/fullscreen_exit_bubble_views.cc>
>
>> File chrome/browser/ui/views/**fullscreen_exit_bubble_views.**cc (right):
>>
>
>
> http://codereview.chromium.**org/11030017/diff/15077/**
>
chrome/browser/ui/views/**fullscreen_exit_bubble_views.**cc#newcode361<http://codereview.chromium.org/11030017/diff/15077/chrome/browser/ui/views/fullscreen_exit_bubble_views.cc#newcode361>
>
>> chrome/browser/ui/views/**fullscreen_exit_bubble_views.**cc:361:
>> gfx::Screen::**BadTwoWorldsContext());
>> root_view_->GetWidget()->**GetNativeView()?
>>
>
>
> http://codereview.chromium.**org/11030017/diff/15077/**
>
content/browser/web_contents/**web_contents_view_aura.cc<http://codereview.chromium.org/11030017/diff/15077/content/browser/web_contents/web_contents_view_aura.cc>
>
>> File content/browser/web_contents/**web_contents_view_aura.cc (right):
>>
>
>
> http://codereview.chromium.**org/11030017/diff/15077/**
>
content/browser/web_contents/**web_contents_view_aura.cc#**newcode273<http://codereview.chromium.org/11030017/diff/15077/content/browser/web_contents/web_contents_view_aura.cc#newcode273>
>
>> content/browser/web_contents/**web_contents_view_aura.cc:273:
>> aura::Window*
>>
> window
>
>> = rvh->GetView()->GetNativeView(**);
>> you can use this |window| as a reference.
>>
>
>  http://codereview.chromium.**org/11030017/diff/15077/ui/**
>>
gfx/screen_aura.cc<http://codereview.chromium.org/11030017/diff/15077/ui/gfx/screen_aura.cc>
>> File ui/gfx/screen_aura.cc (right):
>>
>
>
> http://codereview.chromium.**org/11030017/diff/15077/ui/**
>
gfx/screen_aura.cc#newcode15<http://codereview.chromium.org/11030017/diff/15077/ui/gfx/screen_aura.cc#newcode15>
>
>> ui/gfx/screen_aura.cc:15: #if defined(USE_AURA)
>> this is aura only file.
>>
>
>
> http://codereview.chromium.**org/11030017/diff/8003/ui/gfx/**
>
screen_type_delegate.h<http://codereview.chromium.org/11030017/diff/8003/ui/gfx/screen_type_delegate.h>
>
>> File ui/gfx/screen_type_delegate.h (right):
>>
>
>
> http://codereview.chromium.**org/11030017/diff/8003/ui/gfx/**
>
screen_type_delegate.h#**newcode15<http://codereview.chromium.org/11030017/diff/8003/ui/gfx/screen_type_delegate.h#newcode15>
>
>> ui/gfx/screen_type_delegate.h:**15: SCREEN_TYPE_COUNT,
>> _COUNT is a bit confusing to me. How about _LAST _END?
>>
>
>
>
>
http://codereview.chromium.**org/11030017/<http://codereview.chromium.org/110...
>

[scottmg, 2012-10-08 21:20:45]

Hi oshima, could you please take another look now?

I've reworked this change to have a screen object created and located
appropriately.

I'm sorry this change got so large to review. The majority of the files are very
simple, just changing gfx::Screen:: to ash::Shell::GetAshScreen()-> or
Screen::GetScreenFor() in non-Ash. There's also some plumbing to make a
NativeView available in some places.

[oshima, 2012-10-09 00:26:14]

I'm still halfway through and looked at just under ash/.

Can you remove ash:: namespace from ash/ ?

http://codereview.chromium.org/11030017/diff/40174/ash/shell.cc
File ash/shell.cc (right):

http://codereview.chromium.org/11030017/diff/40174/ash/shell.cc#newcode341
ash/shell.cc:341: return gfx::Screen::GetScreenFor(GetActiveRootWindow());
GetPrimaryRootWindow() is better. (it will never change)

http://codereview.chromium.org/11030017/diff/40174/ash/system/tray/tray_backg...
File ash/system/tray/tray_background_view.h (right):

http://codereview.chromium.org/11030017/diff/40174/ash/system/tray/tray_backg...
ash/system/tray/tray_background_view.h:102: StatusAreaWidget*
status_area_widget() const {
const StatusAreaWidget*

[scottmg, 2012-10-09 04:30:47]

thanks

> Can you remove ash:: namespace from ash/ ?

Done. (and same for some in chrome/browser/ui/ash/)

http://codereview.chromium.org/11030017/diff/40174/ash/shell.cc
File ash/shell.cc (right):

http://codereview.chromium.org/11030017/diff/40174/ash/shell.cc#newcode341
ash/shell.cc:341: return gfx::Screen::GetScreenFor(GetActiveRootWindow());
On 2012/10/09 00:26:14, oshima wrote:
> GetPrimaryRootWindow() is better. (it will never change)

Done.

http://codereview.chromium.org/11030017/diff/40174/ash/system/tray/tray_backg...
File ash/system/tray/tray_background_view.h (right):

http://codereview.chromium.org/11030017/diff/40174/ash/system/tray/tray_backg...
ash/system/tray/tray_background_view.h:102: StatusAreaWidget*
status_area_widget() const {
On 2012/10/09 00:26:14, oshima wrote:
> const StatusAreaWidget*

Done.

[oshima, 2012-10-10 17:58:22]

Mostly good.

http://codereview.chromium.org/11030017/diff/51338/ash/keyboard_overlay/keybo...
File ash/keyboard_overlay/keyboard_overlay_delegate.cc (right):

http://codereview.chromium.org/11030017/diff/51338/ash/keyboard_overlay/keybo...
ash/keyboard_overlay/keyboard_overlay_delegate.cc:84: const gfx::Rect& rect =
ash::Shell::GetAshScreen()->GetDisplayNearestWindow(
this class should be in ash namespace. (confirmed with mazda)
Can you change the namespace instead?

http://codereview.chromium.org/11030017/diff/51338/ash/system/tray/system_tra...
File ash/system/tray/system_tray.cc (right):

http://codereview.chromium.org/11030017/diff/51338/ash/system/tray/system_tra...
ash/system/tray/system_tray.cc:252:
status_area_widget()->GetNativeView())->GetCursorScreenPoint());
Any reason this isn't Shell::GetAshScreen() ?

http://codereview.chromium.org/11030017/diff/51338/ash/wm/shelf_layout_manage...
File ash/wm/shelf_layout_manager.cc (right):

http://codereview.chromium.org/11030017/diff/51338/ash/wm/shelf_layout_manage...
ash/wm/shelf_layout_manager.cc:884: AUTO_HIDE_HIDDEN;
I slightly prefer the original format, but I'll leave it to OWNERS

http://codereview.chromium.org/11030017/diff/51338/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/wallpaper_manager_util.cc (right):

http://codereview.chromium.org/11030017/diff/51338/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/wallpaper_manager_util.cc:65: int height =
extension->launch_height();
Hmm, this looks not exactly correct. Could you please add TODO like this ?
// TODO(oshima|bshe): Open WallpaperManager in the dispay
// it is requested.

http://codereview.chromium.org/11030017/diff/51338/chrome/browser/notificatio...
File chrome/browser/notifications/balloon_collection_impl.cc (right):

http://codereview.chromium.org/11030017/diff/51338/chrome/browser/notificatio...
chrome/browser/notifications/balloon_collection_impl.cc:467: // TODO(scottmg):
NativeScreen might be wrong. http://crbug.com/133312
Yep, this should be a screen on which notifications belong to.

http://codereview.chromium.org/11030017/diff/51338/chrome/browser/ui/ash/ash_...
File chrome/browser/ui/ash/ash_util.cc (right):

http://codereview.chromium.org/11030017/diff/51338/chrome/browser/ui/ash/ash_...
chrome/browser/ui/ash/ash_util.cc:10: #include "ui/gfx/screen.h"
do you need this?

http://codereview.chromium.org/11030017/diff/51338/chrome/browser/ui/panels/d...
File chrome/browser/ui/panels/display_settings_provider.cc (right):

http://codereview.chromium.org/11030017/diff/51338/chrome/browser/ui/panels/d...
chrome/browser/ui/panels/display_settings_provider.cc:71: gfx::Rect
DisplaySettingsProvider::GetPrimaryScreenArea() const {
just FYI: I think this should be moved under ui at some point.

http://codereview.chromium.org/11030017/diff/51338/chrome/browser/ui/views/as...
File chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc (right):

http://codereview.chromium.org/11030017/diff/51338/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc:31: private:
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/11030017/diff/51338/chrome/browser/ui/views/bo...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.h (right):

http://codereview.chromium.org/11030017/diff/51338/chrome/browser/ui/views/bo...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.h:148: // the locale so that
pure LTR strings are displayed properly in RTL locales.
update the comment

http://codereview.chromium.org/11030017/diff/51338/chrome/browser/ui/views/bo...
File chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.h (right):

http://codereview.chromium.org/11030017/diff/51338/chrome/browser/ui/views/bo...
chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.h:87: views::Widget*
parent() const { return parent_; }
const method should return const reference. If you need to get parent from const
object, please add const method instead.

 I know there are a few methods that doesn't follow this pattern and
they needs to be fixed.

http://codereview.chromium.org/11030017/diff/51338/chrome/browser/ui/views/ch...
File chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc (right):

http://codereview.chromium.org/11030017/diff/51338/chrome/browser/ui/views/ch...
chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc:10: #include
"ui/gfx/screen.h"
do you need this?

http://codereview.chromium.org/11030017/diff/51338/chrome/browser/ui/views/st...
File chrome/browser/ui/views/status_bubble_views.cc (right):

http://codereview.chromium.org/11030017/diff/51338/chrome/browser/ui/views/st...
chrome/browser/ui/views/status_bubble_views.cc:769: gfx::NativeView widget =
base_view_->GetWidget()->GetNativeView();
s/widget/window/

http://codereview.chromium.org/11030017/diff/51338/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right):

http://codereview.chromium.org/11030017/diff/51338/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:676:
GetDisplayNearestPoint(origin).work_area();
Any reason this and next change are using different widgets?

http://codereview.chromium.org/11030017/diff/51338/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc
(right):

http://codereview.chromium.org/11030017/diff/51338/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1006:
gfx::Rect work_area = gfx::Screen::GetNativeScreen()->GetDisplayNearestWindow(
Testing ash+native environment is going to be challenging. Do you have any plan?
Maybe we should have ash only win bot?

http://codereview.chromium.org/11030017/diff/51338/chrome/browser/ui/window_s...
File chrome/browser/ui/window_sizer/window_sizer.cc (right):

http://codereview.chromium.org/11030017/diff/51338/chrome/browser/ui/window_s...
chrome/browser/ui/window_sizer/window_sizer.cc:42: }
Looks like passing native window (or screen) from browser isn't hard. You can do
that now if you want.

http://codereview.chromium.org/11030017/diff/51338/ui/gfx/screen.cc
File ui/gfx/screen.cc (right):

http://codereview.chromium.org/11030017/diff/51338/ui/gfx/screen.cc#newcode11
ui/gfx/screen.cc:11: Screen* g_screen_[SCREEN_TYPE_LAST + 1];
put these global into anonymous namespace.

http://codereview.chromium.org/11030017/diff/51338/ui/gfx/screen.cc#newcode12
ui/gfx/screen.cc:12: ScreenTypeDelegate* g_screen_type_delegate_;
= NULL;

Strictly speaking, this isn't necessary, but mostly for consistency with the
rest of chrome.

http://codereview.chromium.org/11030017/diff/51338/ui/gfx/screen.cc#newcode32
ui/gfx/screen.cc:32: void Screen::SetScreenInstance(ScreenType type, Screen*
instance) {
DCHECK_LE(type, SCREEN_TYPE_LAST);

http://codereview.chromium.org/11030017/diff/51338/ui/gfx/screen_type_delegate.h
File ui/gfx/screen_type_delegate.h (right):

http://codereview.chromium.org/11030017/diff/51338/ui/gfx/screen_type_delegat...
ui/gfx/screen_type_delegate.h:13: SCREEN_TYPE_NATIVE,
SCREEN_TYPE_NATIVE = 0,  (assuming that's what you expect)

http://codereview.chromium.org/11030017/diff/51338/ui/gfx/screen_type_delegat...
ui/gfx/screen_type_delegate.h:16: SCREEN_TYPE_INVALID,
Do you need this _INVALID? (looks like this isn't used anywhere?)

http://codereview.chromium.org/11030017/diff/51338/ui/views/controls/menu/men...
File ui/views/controls/menu/menu_controller.h (right):

http://codereview.chromium.org/11030017/diff/51338/ui/views/controls/menu/men...
ui/views/controls/menu/menu_controller.h:468: gfx::Screen* GetScreen() const;
const gfx::Screen

http://codereview.chromium.org/11030017/diff/51338/ui/views/widget/native_wid...
File ui/views/widget/native_widget_aura.h (right):

http://codereview.chromium.org/11030017/diff/51338/ui/views/widget/native_wid...
ui/views/widget/native_widget_aura.h:39: gfx::NativeView parent = NULL);
default argument is now allowed.
remove explicit (assuming you'll keep 2nd argument)

http://codereview.chromium.org/11030017/diff/51338/ui/views/widget/tooltip_ma...
File ui/views/widget/tooltip_manager.h (right):

http://codereview.chromium.org/11030017/diff/51338/ui/views/widget/tooltip_ma...
ui/views/widget/tooltip_manager.h:36: // the tooltip is to be displayed on in
screen coordinates.
update the comment

http://codereview.chromium.org/11030017/diff/51338/ui/views/widget/tooltip_ma...
ui/views/widget/tooltip_manager.h:58: // location of the tooltip in screen
coordinates.
ditto

[scottmg, 2012-10-10 19:04:46]

Thank you for your thorough review. I didn't rebase yet, so the diff from
patchset 24->25 should be relatively easy to read.

https://codereview.chromium.org/11030017/diff/51338/ash/keyboard_overlay/keyb...
File ash/keyboard_overlay/keyboard_overlay_delegate.cc (right):

https://codereview.chromium.org/11030017/diff/51338/ash/keyboard_overlay/keyb...
ash/keyboard_overlay/keyboard_overlay_delegate.cc:84: const gfx::Rect& rect =
ash::Shell::GetAshScreen()->GetDisplayNearestWindow(
On 2012/10/10 17:58:23, oshima wrote:
> this class should be in ash namespace. (confirmed with mazda)
> Can you change the namespace instead?

Done.

https://codereview.chromium.org/11030017/diff/51338/ash/system/tray/system_tr...
File ash/system/tray/system_tray.cc (right):

https://codereview.chromium.org/11030017/diff/51338/ash/system/tray/system_tr...
ash/system/tray/system_tray.cc:252:
status_area_widget()->GetNativeView())->GetCursorScreenPoint());
On 2012/10/10 17:58:23, oshima wrote:
> Any reason this isn't Shell::GetAshScreen() ?

Done.

https://codereview.chromium.org/11030017/diff/51338/ash/wm/shelf_layout_manag...
File ash/wm/shelf_layout_manager.cc (right):

https://codereview.chromium.org/11030017/diff/51338/ash/wm/shelf_layout_manag...
ash/wm/shelf_layout_manager.cc:884: AUTO_HIDE_HIDDEN;
On 2012/10/10 17:58:23, oshima wrote:
> I slightly prefer the original format, but I'll leave it to OWNERS

Done.

https://codereview.chromium.org/11030017/diff/51338/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/wallpaper_manager_util.cc (right):

https://codereview.chromium.org/11030017/diff/51338/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/wallpaper_manager_util.cc:65: int height =
extension->launch_height();
On 2012/10/10 17:58:23, oshima wrote:
> Hmm, this looks not exactly correct. Could you please add TODO like this ?
> // TODO(oshima|bshe): Open WallpaperManager in the dispay
> // it is requested.

Done.

https://codereview.chromium.org/11030017/diff/51338/chrome/browser/notificati...
File chrome/browser/notifications/balloon_collection_impl.cc (right):

https://codereview.chromium.org/11030017/diff/51338/chrome/browser/notificati...
chrome/browser/notifications/balloon_collection_impl.cc:467: // TODO(scottmg):
NativeScreen might be wrong. http://crbug.com/133312
On 2012/10/10 17:58:23, oshima wrote:
> Yep, this should be a screen on which notifications belong to.

Done. (more emphatic :)

https://codereview.chromium.org/11030017/diff/51338/chrome/browser/ui/ash/ash...
File chrome/browser/ui/ash/ash_util.cc (right):

https://codereview.chromium.org/11030017/diff/51338/chrome/browser/ui/ash/ash...
chrome/browser/ui/ash/ash_util.cc:10: #include "ui/gfx/screen.h"
On 2012/10/10 17:58:23, oshima wrote:
> do you need this?

Done.

https://codereview.chromium.org/11030017/diff/51338/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/display_settings_provider.cc (right):

https://codereview.chromium.org/11030017/diff/51338/chrome/browser/ui/panels/...
chrome/browser/ui/panels/display_settings_provider.cc:71: gfx::Rect
DisplaySettingsProvider::GetPrimaryScreenArea() const {
On 2012/10/10 17:58:23, oshima wrote:
> just FYI: I think this should be moved under ui at some point.

Added TODO.

https://codereview.chromium.org/11030017/diff/51338/chrome/browser/ui/views/a...
File chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc (right):

https://codereview.chromium.org/11030017/diff/51338/chrome/browser/ui/views/a...
chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc:31: private:
On 2012/10/10 17:58:23, oshima wrote:
> DISALLOW_COPY_AND_ASSIGN 

Done.

https://codereview.chromium.org/11030017/diff/51338/chrome/browser/ui/views/b...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.h (right):

https://codereview.chromium.org/11030017/diff/51338/chrome/browser/ui/views/b...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.h:148: // the locale so that
pure LTR strings are displayed properly in RTL locales.
On 2012/10/10 17:58:23, oshima wrote:
> update the comment

Done.

https://codereview.chromium.org/11030017/diff/51338/chrome/browser/ui/views/b...
File chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.h (right):

https://codereview.chromium.org/11030017/diff/51338/chrome/browser/ui/views/b...
chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.h:87: views::Widget*
parent() const { return parent_; }
On 2012/10/10 17:58:23, oshima wrote:
> const method should return const reference. If you need to get parent from
const
> object, please add const method instead.
> 
>  I know there are a few methods that doesn't follow this pattern and
> they needs to be fixed.

Done.

https://codereview.chromium.org/11030017/diff/51338/chrome/browser/ui/views/c...
File chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc (right):

https://codereview.chromium.org/11030017/diff/51338/chrome/browser/ui/views/c...
chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc:10: #include
"ui/gfx/screen.h"
On 2012/10/10 17:58:23, oshima wrote:
> do you need this?

Done.

https://codereview.chromium.org/11030017/diff/51338/chrome/browser/ui/views/s...
File chrome/browser/ui/views/status_bubble_views.cc (right):

https://codereview.chromium.org/11030017/diff/51338/chrome/browser/ui/views/s...
chrome/browser/ui/views/status_bubble_views.cc:769: gfx::NativeView widget =
base_view_->GetWidget()->GetNativeView();
On 2012/10/10 17:58:23, oshima wrote:
> s/widget/window/

Done.

https://codereview.chromium.org/11030017/diff/51338/chrome/browser/ui/views/t...
File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right):

https://codereview.chromium.org/11030017/diff/51338/chrome/browser/ui/views/t...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:676:
GetDisplayNearestPoint(origin).work_area();
On 2012/10/10 17:58:23, oshima wrote:
> Any reason this and next change are using different widgets?

No, I'm just not familiar with code, so I wasn't clear on when particular
members are valid.

Looking at where GetWindowCreatePoint is called from pointed to source_tabstrip_
being valid at this point, whereas below,
GetAttachedBrowserWidget()/attached_tabstrip_ look to be valid. So, using a
different one in both places seemed the safest change.

https://codereview.chromium.org/11030017/diff/51338/chrome/browser/ui/views/t...
File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc
(right):

https://codereview.chromium.org/11030017/diff/51338/chrome/browser/ui/views/t...
chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1006:
gfx::Rect work_area = gfx::Screen::GetNativeScreen()->GetDisplayNearestWindow(
On 2012/10/10 17:58:23, oshima wrote:
> Testing ash+native environment is going to be challenging. Do you have any
plan?
> Maybe we should have ash only win bot?

Agreed. we'll definitely have to set up more bots to handle both paths once more
of the code is working.

https://codereview.chromium.org/11030017/diff/51338/chrome/browser/ui/window_...
File chrome/browser/ui/window_sizer/window_sizer.cc (right):

https://codereview.chromium.org/11030017/diff/51338/chrome/browser/ui/window_...
chrome/browser/ui/window_sizer/window_sizer.cc:42: }
On 2012/10/10 17:58:23, oshima wrote:
> Looks like passing native window (or screen) from browser isn't hard. You can
do
> that now if you want.

Done.

https://codereview.chromium.org/11030017/diff/51338/ui/gfx/screen.cc
File ui/gfx/screen.cc (right):

https://codereview.chromium.org/11030017/diff/51338/ui/gfx/screen.cc#newcode11
ui/gfx/screen.cc:11: Screen* g_screen_[SCREEN_TYPE_LAST + 1];
On 2012/10/10 17:58:23, oshima wrote:
> put these global into anonymous namespace.

Done.

https://codereview.chromium.org/11030017/diff/51338/ui/gfx/screen.cc#newcode12
ui/gfx/screen.cc:12: ScreenTypeDelegate* g_screen_type_delegate_;
On 2012/10/10 17:58:23, oshima wrote:
> = NULL;
> 
> Strictly speaking, this isn't necessary, but mostly for consistency with the
> rest of chrome.

Done.

https://codereview.chromium.org/11030017/diff/51338/ui/gfx/screen.cc#newcode32
ui/gfx/screen.cc:32: void Screen::SetScreenInstance(ScreenType type, Screen*
instance) {
On 2012/10/10 17:58:23, oshima wrote:
> DCHECK_LE(type, SCREEN_TYPE_LAST);

Done.

https://codereview.chromium.org/11030017/diff/51338/ui/gfx/screen_type_delega...
File ui/gfx/screen_type_delegate.h (right):

https://codereview.chromium.org/11030017/diff/51338/ui/gfx/screen_type_delega...
ui/gfx/screen_type_delegate.h:13: SCREEN_TYPE_NATIVE,
On 2012/10/10 17:58:23, oshima wrote:
> SCREEN_TYPE_NATIVE = 0,  (assuming that's what you expect)

Done.

https://codereview.chromium.org/11030017/diff/51338/ui/gfx/screen_type_delega...
ui/gfx/screen_type_delegate.h:16: SCREEN_TYPE_INVALID,
On 2012/10/10 17:58:23, oshima wrote:
> Do you need this _INVALID? (looks like this isn't used anywhere?)

Done.

https://codereview.chromium.org/11030017/diff/51338/ui/views/controls/menu/me...
File ui/views/controls/menu/menu_controller.h (right):

https://codereview.chromium.org/11030017/diff/51338/ui/views/controls/menu/me...
ui/views/controls/menu/menu_controller.h:468: gfx::Screen* GetScreen() const;
On 2012/10/10 17:58:23, oshima wrote:
> const gfx::Screen

Done.

https://codereview.chromium.org/11030017/diff/51338/ui/views/widget/native_wi...
File ui/views/widget/native_widget_aura.h (right):

https://codereview.chromium.org/11030017/diff/51338/ui/views/widget/native_wi...
ui/views/widget/native_widget_aura.h:39: gfx::NativeView parent = NULL);
On 2012/10/10 17:58:23, oshima wrote:
> default argument is now allowed.
> remove explicit (assuming you'll keep 2nd argument)

Done.

https://codereview.chromium.org/11030017/diff/51338/ui/views/widget/tooltip_m...
File ui/views/widget/tooltip_manager.h (right):

https://codereview.chromium.org/11030017/diff/51338/ui/views/widget/tooltip_m...
ui/views/widget/tooltip_manager.h:36: // the tooltip is to be displayed on in
screen coordinates.
On 2012/10/10 17:58:23, oshima wrote:
> update the comment

Done.

https://codereview.chromium.org/11030017/diff/51338/ui/views/widget/tooltip_m...
ui/views/widget/tooltip_manager.h:58: // location of the tooltip in screen
coordinates.
On 2012/10/10 17:58:23, oshima wrote:
> ditto

Done.

[oshima, 2012-10-10 20:08:38]

lgtm with nits

http://codereview.chromium.org/11030017/diff/48322/ash/keyboard_overlay/keybo...
File ash/keyboard_overlay/keyboard_overlay_view.cc (right):

http://codereview.chromium.org/11030017/diff/48322/ash/keyboard_overlay/keybo...
ash/keyboard_overlay/keyboard_overlay_view.cc:72: ash::KeyboardOverlayDelegate*
delegate = new ash::KeyboardOverlayDelegate(
Move this class to ash namespace as well.

http://codereview.chromium.org/11030017/diff/48322/chrome/browser/ui/window_s...
File chrome/browser/ui/window_sizer/window_sizer.cc (right):

http://codereview.chromium.org/11030017/diff/48322/chrome/browser/ui/window_s...
chrome/browser/ui/window_sizer/window_sizer.cc:28:
DefaultMonitorInfoProvider(gfx::Screen* screen) : screen_(screen) {}
const gfx::Screen*

http://codereview.chromium.org/11030017/diff/48322/chrome/browser/ui/window_s...
chrome/browser/ui/window_sizer/window_sizer.cc:41: gfx::Screen* screen_;
const

DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/11030017/diff/48322/ui/gfx/screen.cc
File ui/gfx/screen.cc (right):

http://codereview.chromium.org/11030017/diff/48322/ui/gfx/screen.cc#newcode14
ui/gfx/screen.cc:14: }  // namespace
We usually put anonymous namespace nested. That way, you don't need to specify
gfx::

http://codereview.chromium.org/11030017/diff/48322/ui/gfx/screen.cc#newcode53
ui/gfx/screen.cc:53: }
  // namespace gfx

[scottmg, 2012-10-10 20:52:00]

Thanks!

https://codereview.chromium.org/11030017/diff/48322/ash/keyboard_overlay/keyb...
File ash/keyboard_overlay/keyboard_overlay_view.cc (right):

https://codereview.chromium.org/11030017/diff/48322/ash/keyboard_overlay/keyb...
ash/keyboard_overlay/keyboard_overlay_view.cc:72: ash::KeyboardOverlayDelegate*
delegate = new ash::KeyboardOverlayDelegate(
On 2012/10/10 20:08:39, oshima wrote:
> Move this class to ash namespace as well. 

Done.

https://codereview.chromium.org/11030017/diff/48322/chrome/browser/ui/window_...
File chrome/browser/ui/window_sizer/window_sizer.cc (right):

https://codereview.chromium.org/11030017/diff/48322/chrome/browser/ui/window_...
chrome/browser/ui/window_sizer/window_sizer.cc:28:
DefaultMonitorInfoProvider(gfx::Screen* screen) : screen_(screen) {}
On 2012/10/10 20:08:39, oshima wrote:
> const gfx::Screen*

Done.

https://codereview.chromium.org/11030017/diff/48322/chrome/browser/ui/window_...
chrome/browser/ui/window_sizer/window_sizer.cc:41: gfx::Screen* screen_;
On 2012/10/10 20:08:39, oshima wrote:
> const
> 
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/11030017/diff/48322/ui/gfx/screen.cc
File ui/gfx/screen.cc (right):

https://codereview.chromium.org/11030017/diff/48322/ui/gfx/screen.cc#newcode14
ui/gfx/screen.cc:14: }  // namespace
On 2012/10/10 20:08:39, oshima wrote:
> We usually put anonymous namespace nested. That way, you don't need to specify
> gfx::

Done.

https://codereview.chromium.org/11030017/diff/48322/ui/gfx/screen.cc#newcode53
ui/gfx/screen.cc:53: }
On 2012/10/10 20:08:39, oshima wrote:
>   // namespace gfx

Done.

[scottmg, 2012-10-10 20:53:41]

ben, for OWNERS
+jam, just for content/shell/shell_aura.cc OWNERS stamp.

[jam, 2012-10-10 21:46:48]

lgtm

[Ben Goodger (Google), 2012-10-10 22:52:09]

https://codereview.chromium.org/11030017/diff/15732/chrome/browser/ui/views/a...
File chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc (right):

https://codereview.chromium.org/11030017/diff/15732/chrome/browser/ui/views/a...
chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc:23: #if
defined(OS_WIN)
if !defined(OS_CHROMEOS) ?

https://codereview.chromium.org/11030017/diff/15732/chrome/browser/ui/views/a...
chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc:27:
gfx::ScreenType GetScreenTypeForNativeView(gfx::NativeView view) {
virtual blablah OVERRIDE {

https://codereview.chromium.org/11030017/diff/15732/chrome/browser/ui/views/a...
chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc:51: #if
defined(OS_WIN)
again if !defined(OS_CHROMEOS)?

https://codereview.chromium.org/11030017/diff/49560/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/11030017/diff/49560/ash/shell.h#newcode176
ash/shell.h:176: static gfx::Screen* GetAshScreen();
imma be that guy that's gonna ask you to call this GetScreen(), ash being
implied since you are on the ash::Shell object.

https://codereview.chromium.org/11030017/diff/49560/chrome/browser/ui/views/c...
File chrome/browser/ui/views/chrome_views_delegate.cc (right):

https://codereview.chromium.org/11030017/diff/49560/chrome/browser/ui/views/c...
chrome/browser/ui/views/chrome_views_delegate.cc:171: views::NativeWidgetAura*
native_widget, gfx::NativeView parent) {
given that you don't use this value here, I don't think you need to add this
param to this method or to NWA's ctor.

[scottmg, 2012-10-11 00:22:28]

https://codereview.chromium.org/11030017/diff/15732/chrome/browser/ui/views/a...
File chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc (right):

https://codereview.chromium.org/11030017/diff/15732/chrome/browser/ui/views/a...
chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc:23: #if
defined(OS_WIN)
On 2012/10/10 22:52:09, Ben Goodger (Google) wrote:
> if !defined(OS_CHROMEOS) ?

Done.

https://codereview.chromium.org/11030017/diff/15732/chrome/browser/ui/views/a...
chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc:27:
gfx::ScreenType GetScreenTypeForNativeView(gfx::NativeView view) {
On 2012/10/10 22:52:09, Ben Goodger (Google) wrote:
> virtual blablah OVERRIDE {

Done.

https://codereview.chromium.org/11030017/diff/15732/chrome/browser/ui/views/a...
chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc:51: #if
defined(OS_WIN)
On 2012/10/10 22:52:09, Ben Goodger (Google) wrote:
> again if !defined(OS_CHROMEOS)?

Done.

https://codereview.chromium.org/11030017/diff/49560/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/11030017/diff/49560/ash/shell.h#newcode176
ash/shell.h:176: static gfx::Screen* GetAshScreen();
On 2012/10/10 22:52:10, Ben Goodger (Google) wrote:
> imma be that guy that's gonna ask you to call this GetScreen(), ash being
> implied since you are on the ash::Shell object.

imma be the guy crying in the refactoring corner. But you're right.

Done.

https://codereview.chromium.org/11030017/diff/49560/chrome/browser/ui/views/c...
File chrome/browser/ui/views/chrome_views_delegate.cc (right):

https://codereview.chromium.org/11030017/diff/49560/chrome/browser/ui/views/c...
chrome/browser/ui/views/chrome_views_delegate.cc:171: views::NativeWidgetAura*
native_widget, gfx::NativeView parent) {
On 2012/10/10 22:52:10, Ben Goodger (Google) wrote:
> given that you don't use this value here, I don't think you need to add this
> param to this method or to NWA's ctor.

Oops, thanks. I think that was from when this method was more complicated. Done.

[Ben Goodger (Google), 2012-10-12 16:25:33]

lgtm

[commit-bot: I haz the power, 2012-10-12 16:30:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottmg@chromium.org/11030017/40544

[commit-bot: I haz the power, 2012-10-12 16:31:15]

Failed to apply patch for
chrome/browser/chromeos/display/primary_display_switch_observer.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
chrome/browser/chromeos/display/primary_display_switch_observer.cc
  Hunk #1 FAILED at 16.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/chromeos/display/primary_display_switch_observer.cc.rej

Patch:       chrome/browser/chromeos/display/primary_display_switch_observer.cc
Index: chrome/browser/chromeos/display/primary_display_switch_observer.cc
diff --git a/chrome/browser/chromeos/display/primary_display_switch_observer.cc
b/chrome/browser/chromeos/display/primary_display_switch_observer.cc
index
352edb389ac8babe74bf0b5b541af596cd5a1959..5469567c2f2af0251a4be5057fb9885ea74b8932
100644
--- a/chrome/browser/chromeos/display/primary_display_switch_observer.cc
+++ b/chrome/browser/chromeos/display/primary_display_switch_observer.cc
@@ -16,8 +16,9 @@ namespace chromeos {
 
 void PrimaryDisplaySwitchObserver::OnRootWindowMoved(
     const aura::RootWindow* root_window, const gfx::Point& new_origin) {
-  StorePrimaryDisplayIDPref(ProfileManager::GetDefaultProfile()->GetPrefs(),
-                            gfx::Screen::GetPrimaryDisplay().id());
+  StorePrimaryDisplayIDPref(
+      ProfileManager::GetDefaultProfile()->GetPrefs(),
+      ash::Shell::GetScreen()->GetPrimaryDisplay().id());
 }
 
 PrimaryDisplaySwitchObserver::PrimaryDisplaySwitchObserver()

[commit-bot: I haz the power, 2012-10-12 16:44:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottmg@chromium.org/11030017/57001

[commit-bot: I haz the power, 2012-10-12 17:14:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottmg@chromium.org/11030017/58153

[commit-bot: I haz the power, 2012-10-12 19:51:24]

Change committed as 161644