Add desktop vs. ash context to ui_controls

Add desktop vs. ash context to ui_controls

Currently, ui_controls methods are global. This patch pulls the methods into a UIControls object that is implemented and installed at startup for the target platform. This is to allow dispatch to either desktop or ash implementations on Windows.

ui_controls.cc still contains global (now deprecated) implementations of the methods that forward to the native implementation. So, everything is still wrong on desktop+ash on Windows, but it makes this patch a lot smaller and shouldn't change behavior on any platform. I'll do a follow up CL to change all the callsites to retrieve the correct UIControls object which will make desktop+ash work properly.

BUG=128578

[oshima, 2012-11-15 23:37:41]

http://codereview.chromium.org/11419013/diff/12001/ash/shell.cc
File ash/shell.cc (right):

http://codereview.chromium.org/11419013/diff/12001/ash/shell.cc#newcode213
ash/shell.cc:213: ANNOTATE_LEAKING_OBJECT_PTR(uicontrols);
Isn't it possible to delete old one in SetUIControlsInstance?

http://codereview.chromium.org/11419013/diff/12001/ash/shell.cc#newcode219
ash/shell.cc:219: ui_controls::UI_CONTROLS_TYPE_NATIVE, uicontrols);
you need {} in this case.

http://codereview.chromium.org/11419013/diff/12001/ui/aura/desktop_ui_controls.h
File ui/aura/desktop_ui_controls.h (right):

http://codereview.chromium.org/11419013/diff/12001/ui/aura/desktop_ui_control...
ui/aura/desktop_ui_controls.h:8: #include "ui/views/views_export.h"
I'm not familiar with aura desktop structure, but using views in aura looks
wrong. Is this by design?

http://codereview.chromium.org/11419013/diff/12001/ui/ui_controls/ui_controls.cc
File ui/ui_controls/ui_controls.cc (right):

http://codereview.chromium.org/11419013/diff/12001/ui/ui_controls/ui_controls...
ui/ui_controls/ui_controls.cc:13: UIControls*
g_ui_controls_[UI_CONTROLS_TYPE_LAST + 1];
 = { 0 }

(You don't really need for arrays, but just for clearness)

http://codereview.chromium.org/11419013/diff/12001/ui/ui_controls/ui_controls...
ui/ui_controls/ui_controls.cc:14: UIControlsTypeDelegate*
g_ui_controls_type_delegate_ = NULL;
optional: my understanding is that g_ is for globally accessible variable and _
is private members, which doesn't
apply here, so you should be able to just say ui_controls.
It's up to you tho.

http://codereview.chromium.org/11419013/diff/12001/ui/ui_controls/ui_controls...
ui/ui_controls/ui_controls.cc:88: const base::Closure& task) {
indent

http://codereview.chromium.org/11419013/diff/12001/ui/ui_controls/ui_controls.h
File ui/ui_controls/ui_controls.h (right):

http://codereview.chromium.org/11419013/diff/12001/ui/ui_controls/ui_controls...
ui/ui_controls/ui_controls.h:55: 
remove extra (2) new lines

http://codereview.chromium.org/11419013/diff/12001/ui/ui_controls/ui_controls...
File ui/ui_controls/ui_controls_gtk.cc (right):

http://codereview.chromium.org/11419013/diff/12001/ui/ui_controls/ui_controls...
ui/ui_controls/ui_controls_gtk.cc:253: }
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/11419013/diff/12001/ui/ui_controls/ui_controls...
File ui/ui_controls/ui_controls_mac.mm (right):

http://codereview.chromium.org/11419013/diff/12001/ui/ui_controls/ui_controls...
ui/ui_controls/ui_controls_mac.mm:363: }
ditto

http://codereview.chromium.org/11419013/diff/12001/ui/ui_controls/ui_controls...
File ui/ui_controls/ui_controls_type_delegate.h (right):

http://codereview.chromium.org/11419013/diff/12001/ui/ui_controls/ui_controls...
ui/ui_controls/ui_controls_type_delegate.h:7: 
#include "ui/base/ui_export.h"

http://codereview.chromium.org/11419013/diff/12001/ui/ui_controls/ui_controls...
File ui/ui_controls/ui_controls_win.cc (right):

http://codereview.chromium.org/11419013/diff/12001/ui/ui_controls/ui_controls...
ui/ui_controls/ui_controls_win.cc:67: };
ditto

[scottmg, 2012-11-16 17:58:54]

thanks

https://codereview.chromium.org/11419013/diff/12001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/11419013/diff/12001/ash/shell.cc#newcode213
ash/shell.cc:213: ANNOTATE_LEAKING_OBJECT_PTR(uicontrols);
On 2012/11/15 23:37:41, oshima wrote:
> Isn't it possible to delete old one in SetUIControlsInstance?

Done.

https://codereview.chromium.org/11419013/diff/12001/ash/shell.cc#newcode219
ash/shell.cc:219: ui_controls::UI_CONTROLS_TYPE_NATIVE, uicontrols);
On 2012/11/15 23:37:41, oshima wrote:
> you need {} in this case.

Done.

https://codereview.chromium.org/11419013/diff/12001/ui/aura/desktop_ui_contro...
File ui/aura/desktop_ui_controls.h (right):

https://codereview.chromium.org/11419013/diff/12001/ui/aura/desktop_ui_contro...
ui/aura/desktop_ui_controls.h:8: #include "ui/views/views_export.h"
On 2012/11/15 23:37:41, oshima wrote:
> I'm not familiar with aura desktop structure, but using views in aura looks
> wrong. Is this by design?

Oops, thanks. Done.

https://codereview.chromium.org/11419013/diff/12001/ui/ui_controls/ui_control...
File ui/ui_controls/ui_controls.cc (right):

https://codereview.chromium.org/11419013/diff/12001/ui/ui_controls/ui_control...
ui/ui_controls/ui_controls.cc:13: UIControls*
g_ui_controls_[UI_CONTROLS_TYPE_LAST + 1];
On 2012/11/15 23:37:41, oshima wrote:
>  = { 0 }
> 
> (You don't really need for arrays, but just for clearness)

Done.

https://codereview.chromium.org/11419013/diff/12001/ui/ui_controls/ui_control...
ui/ui_controls/ui_controls.cc:14: UIControlsTypeDelegate*
g_ui_controls_type_delegate_ = NULL;
On 2012/11/15 23:37:41, oshima wrote:
> optional: my understanding is that g_ is for globally accessible variable and
_
> is private members, which doesn't
> apply here, so you should be able to just say ui_controls.
> It's up to you tho.

Done.

https://codereview.chromium.org/11419013/diff/12001/ui/ui_controls/ui_control...
ui/ui_controls/ui_controls.cc:88: const base::Closure& task) {
On 2012/11/15 23:37:41, oshima wrote:
> indent

Done.

https://codereview.chromium.org/11419013/diff/12001/ui/ui_controls/ui_controls.h
File ui/ui_controls/ui_controls.h (right):

https://codereview.chromium.org/11419013/diff/12001/ui/ui_controls/ui_control...
ui/ui_controls/ui_controls.h:55: 
On 2012/11/15 23:37:41, oshima wrote:
> remove extra (2) new lines

Done.

https://codereview.chromium.org/11419013/diff/12001/ui/ui_controls/ui_control...
File ui/ui_controls/ui_controls_gtk.cc (right):

https://codereview.chromium.org/11419013/diff/12001/ui/ui_controls/ui_control...
ui/ui_controls/ui_controls_gtk.cc:253: }
On 2012/11/15 23:37:41, oshima wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/11419013/diff/12001/ui/ui_controls/ui_control...
File ui/ui_controls/ui_controls_mac.mm (right):

https://codereview.chromium.org/11419013/diff/12001/ui/ui_controls/ui_control...
ui/ui_controls/ui_controls_mac.mm:363: }
On 2012/11/15 23:37:41, oshima wrote:
> ditto

Done.

https://codereview.chromium.org/11419013/diff/12001/ui/ui_controls/ui_control...
File ui/ui_controls/ui_controls_type_delegate.h (right):

https://codereview.chromium.org/11419013/diff/12001/ui/ui_controls/ui_control...
ui/ui_controls/ui_controls_type_delegate.h:7: 
On 2012/11/15 23:37:41, oshima wrote:
> #include "ui/base/ui_export.h"

Done.

https://codereview.chromium.org/11419013/diff/12001/ui/ui_controls/ui_control...
File ui/ui_controls/ui_controls_win.cc (right):

https://codereview.chromium.org/11419013/diff/12001/ui/ui_controls/ui_control...
ui/ui_controls/ui_controls_win.cc:67: };
On 2012/11/15 23:37:41, oshima wrote:
> ditto

Done.

[oshima, 2012-11-16 20:41:39]

http://codereview.chromium.org/11419013/diff/5062/chrome/browser/ui/views/ash...
File chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc (right):

http://codereview.chromium.org/11419013/diff/5062/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc:50: private:
nit: new line before private:

http://codereview.chromium.org/11419013/diff/5062/ui/ui_controls/ui_controls.cc
File ui/ui_controls/ui_controls.cc (right):

http://codereview.chromium.org/11419013/diff/5062/ui/ui_controls/ui_controls....
ui/ui_controls/ui_controls.cc:38: DCHECK_LE(type, UI_CONTROLS_TYPE_LAST);
I was thinking about

delete g_ui_controls[type];

instead of using scoped ptr in Shell. I believe you can prevent a leak in the
tests this way.

http://codereview.chromium.org/11419013/diff/5062/ui/ui_controls/ui_controls_...
File ui/ui_controls/ui_controls_gtk.cc (right):

http://codereview.chromium.org/11419013/diff/5062/ui/ui_controls/ui_controls_...
ui/ui_controls/ui_controls_gtk.cc:101: UIControlsGtk() {}
virtual ~UIControlsGtk() {}

http://codereview.chromium.org/11419013/diff/5062/ui/ui_controls/ui_controls_...
File ui/ui_controls/ui_controls_mac.mm (right):

http://codereview.chromium.org/11419013/diff/5062/ui/ui_controls/ui_controls_...
ui/ui_controls/ui_controls_mac.mm:220: UIControlsMac() {}
ditto

http://codereview.chromium.org/11419013/diff/5062/ui/ui_controls/ui_controls_...
File ui/ui_controls/ui_controls_win.cc (right):

http://codereview.chromium.org/11419013/diff/5062/ui/ui_controls/ui_controls_...
ui/ui_controls/ui_controls_win.cc:19: UIControlsWin() {}
ditto

[scottmg, 2012-11-16 22:34:05]

thank you

https://codereview.chromium.org/11419013/diff/5062/chrome/browser/ui/views/as...
File chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc (right):

https://codereview.chromium.org/11419013/diff/5062/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc:50: private:
On 2012/11/16 20:41:39, oshima wrote:
> nit: new line before private:

Done.

https://codereview.chromium.org/11419013/diff/5062/ui/ui_controls/ui_controls.cc
File ui/ui_controls/ui_controls.cc (right):

https://codereview.chromium.org/11419013/diff/5062/ui/ui_controls/ui_controls...
ui/ui_controls/ui_controls.cc:38: DCHECK_LE(type, UI_CONTROLS_TYPE_LAST);
On 2012/11/16 20:41:39, oshima wrote:
> I was thinking about
> 
> delete g_ui_controls[type];
> 
> instead of using scoped ptr in Shell. I believe you can prevent a leak in the
> tests this way.

Done.

https://codereview.chromium.org/11419013/diff/5062/ui/ui_controls/ui_controls...
File ui/ui_controls/ui_controls_gtk.cc (right):

https://codereview.chromium.org/11419013/diff/5062/ui/ui_controls/ui_controls...
ui/ui_controls/ui_controls_gtk.cc:101: UIControlsGtk() {}
On 2012/11/16 20:41:39, oshima wrote:
> virtual ~UIControlsGtk() {}

Done.

https://codereview.chromium.org/11419013/diff/5062/ui/ui_controls/ui_controls...
File ui/ui_controls/ui_controls_mac.mm (right):

https://codereview.chromium.org/11419013/diff/5062/ui/ui_controls/ui_controls...
ui/ui_controls/ui_controls_mac.mm:220: UIControlsMac() {}
On 2012/11/16 20:41:39, oshima wrote:
> ditto

Done.

https://codereview.chromium.org/11419013/diff/5062/ui/ui_controls/ui_controls...
File ui/ui_controls/ui_controls_win.cc (right):

https://codereview.chromium.org/11419013/diff/5062/ui/ui_controls/ui_controls...
ui/ui_controls/ui_controls_win.cc:19: UIControlsWin() {}
On 2012/11/16 20:41:39, oshima wrote:
> ditto

Done.

[oshima, 2012-11-16 23:04:07]

lgtm with nits

https://codereview.chromium.org/11419013/diff/4034/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/11419013/diff/4034/ash/shell.cc#newcode212
ash/shell.cc:212: ui_controls::UIControls* uicontrols =
internal::CreateUIControls();
nit: can this be in in argument list? If it
desn't fit 80 chars, this is fine.

https://codereview.chromium.org/11419013/diff/4034/chrome/browser/ui/views/as...
File chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc (right):

https://codereview.chromium.org/11419013/diff/4034/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc:43:
UIControlsTypeDelegateWin() {}
virtual dtor

https://codereview.chromium.org/11419013/diff/4034/ui/aura/ui_controls_x11.cc
File ui/aura/ui_controls_x11.cc (right):

https://codereview.chromium.org/11419013/diff/4034/ui/aura/ui_controls_x11.cc...
ui/aura/ui_controls_x11.cc:82: UIControlsX11(RootWindow* root_window) :
root_window_(root_window) {
it's probaly my fault, but can you add explicit, and virtual dtor?

[scottmg, 2012-11-16 23:10:28]

thanks.

ben (for owners)?

https://codereview.chromium.org/11419013/diff/4034/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/11419013/diff/4034/ash/shell.cc#newcode212
ash/shell.cc:212: ui_controls::UIControls* uicontrols =
internal::CreateUIControls();
On 2012/11/16 23:04:07, oshima wrote:
> nit: can this be in in argument list? If it
> desn't fit 80 chars, this is fine.

Done.

https://codereview.chromium.org/11419013/diff/4034/chrome/browser/ui/views/as...
File chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc (right):

https://codereview.chromium.org/11419013/diff/4034/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc:43:
UIControlsTypeDelegateWin() {}
On 2012/11/16 23:04:07, oshima wrote:
> virtual dtor

Done.

https://codereview.chromium.org/11419013/diff/4034/ui/aura/ui_controls_x11.cc
File ui/aura/ui_controls_x11.cc (right):

https://codereview.chromium.org/11419013/diff/4034/ui/aura/ui_controls_x11.cc...
ui/aura/ui_controls_x11.cc:82: UIControlsX11(RootWindow* root_window) :
root_window_(root_window) {
On 2012/11/16 23:04:07, oshima wrote:
> it's probaly my fault, but can you add explicit, and virtual dtor?

Done.

[Ben Goodger (Google), 2012-11-19 20:21:13]

There are too many layers here and my head is exploding.

Can you just fix ui_controls? Sorry to do this to you, but sometimes it needs to
be done. I don't want to make a category 1 storm a category 5 is all.

https://codereview.chromium.org/11419013/diff/5066/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/11419013/diff/5066/ash/shell.cc#newcode213
ash/shell.cc:213: ui_controls::UI_CONTROLS_TYPE_ALTERNATE,
internal::CreateUIControls());
4-space indent

[scottmg, 2012-11-19 20:51:29]

On 2012/11/19 20:21:13, Ben Goodger (Google) wrote:
> There are too many layers here and my head is exploding.

Welcome to the Chrome project! When did you start?

> 
> Can you just fix ui_controls?

Do you mean do-something-completely-different? Or do you mean getting rid of all
the global functions in ui_controls.h in this CL?

> Sorry to do this to you, but sometimes it needs to
> be done. I don't want to make a category 1 storm a category 5 is all.
> 
> https://codereview.chromium.org/11419013/diff/5066/ash/shell.cc
> File ash/shell.cc (right):
> 
> https://codereview.chromium.org/11419013/diff/5066/ash/shell.cc#newcode213
> ash/shell.cc:213: ui_controls::UI_CONTROLS_TYPE_ALTERNATE,
> internal::CreateUIControls());
> 4-space indent

[Ben Goodger (Google), 2012-11-19 21:19:16]

Something that doesn't have a bunch of different objects duct-taped
together/replacing each other in various places.

By way of a start - can you characterize what these functions are supposed
to do based on what inputs?


On Mon, Nov 19, 2012 at 12:51 PM, <scottmg@chromium.org> wrote:

> On 2012/11/19 20:21:13, Ben Goodger (Google) wrote:
>
>> There are too many layers here and my head is exploding.
>>
>
> Welcome to the Chrome project! When did you start?
>
>
>
>  Can you just fix ui_controls?
>>
>
> Do you mean do-something-completely-**different? Or do you mean getting
> rid of all
> the global functions in ui_controls.h in this CL?
>
>
>  Sorry to do this to you, but sometimes it needs to
>> be done. I don't want to make a category 1 storm a category 5 is all.
>>
>
> 
https://codereview.chromium.**org/11419013/diff/5066/ash/**shell.cc<https://c...
>> File ash/shell.cc (right):
>>
>
>  https://codereview.chromium.**org/11419013/diff/5066/ash/**
>>
shell.cc#newcode213<https://codereview.chromium.org/11419013/diff/5066/ash/shell.cc#newcode213>
>> ash/shell.cc:213: ui_controls::UI_CONTROLS_TYPE_**ALTERNATE,
>> internal::CreateUIControls());
>> 4-space indent
>>
>
>
>
>
https://codereview.chromium.**org/11419013/<https://codereview.chromium.org/1...
>

[scottmg, 2012-11-19 23:09:43]

On 2012/11/19 21:19:16, Ben Goodger (Google) wrote:
> Something that doesn't have a bunch of different objects duct-taped
> together/replacing each other in various places.
> 
> By way of a start - can you characterize what these functions are supposed
> to do based on what inputs?

AFAICT, they're for simulating key and mouse events, largely for testing, but
also used by some automation, and at least one 'real' place in
BrowserView::Cut/Copy/Paste.

Currently:
- They're global. This didn't work for cros+ash
https://chromiumcodereview.appspot.com/10545058 so there's a level of dispatch
there to have an implementation-per-rootwindow based having a property on the
RootWindow (this is to do some adjustment for different screen locations for the
roots)
- desktop aura doesn't call ui_controls_aura.cc:InstallUIControlsAura at all, so
it crashes when these function are used.

It didn't make sense to add an implementation for desktop aura just to make
win_aura work, because it then wouldn't have a way of dispatching to the right
implementation when both desktop and ash are active.

So, I was intending that there'd be an implementation installed for cros,
native-win, mac, x11 (as the "Native" one). And on aura windows, there'd be one
of two implementations: one for desktop and one for ash, and have it selected
based on a NativeView for context. (ash still has the extra dispatch to modify
inputs based on RootWindow).

Having said all that, this CL is all very-rote-moving-junk-around, so I'd be
happy to do this in some way that doesn't have so much wrapping and forwarding.

[Ben Goodger (Google), 2012-11-19 23:13:18]

but do the call sites all have a NativeView/Window that corresponds to some
window within ash that they wish to send key events to?


On Mon, Nov 19, 2012 at 3:09 PM, <scottmg@chromium.org> wrote:

> On 2012/11/19 21:19:16, Ben Goodger (Google) wrote:
>
>> Something that doesn't have a bunch of different objects duct-taped
>> together/replacing each other in various places.
>>
>
>  By way of a start - can you characterize what these functions are supposed
>> to do based on what inputs?
>>
>
> AFAICT, they're for simulating key and mouse events, largely for testing,
> but
> also used by some automation, and at least one 'real' place in
> BrowserView::Cut/Copy/Paste.
>
> Currently:
> - They're global. This didn't work for cros+ash
>
https://chromiumcodereview.**appspot.com/10545058<https://chromiumcodereview....
there's a level of dispatch
> there to have an implementation-per-rootwindow based having a property on
> the
> RootWindow (this is to do some adjustment for different screen locations
> for the
> roots)
> - desktop aura doesn't call ui_controls_aura.cc:**InstallUIControlsAura
> at all, so
> it crashes when these function are used.
>
> It didn't make sense to add an implementation for desktop aura just to make
> win_aura work, because it then wouldn't have a way of dispatching to the
> right
> implementation when both desktop and ash are active.
>
> So, I was intending that there'd be an implementation installed for cros,
> native-win, mac, x11 (as the "Native" one). And on aura windows, there'd
> be one
> of two implementations: one for desktop and one for ash, and have it
> selected
> based on a NativeView for context. (ash still has the extra dispatch to
> modify
> inputs based on RootWindow).
>
> Having said all that, this CL is all very-rote-moving-junk-around, so I'd
> be
> happy to do this in some way that doesn't have so much wrapping and
> forwarding.
>
>
>
https://codereview.chromium.**org/11419013/<https://codereview.chromium.org/1...
>

[scottmg, 2012-11-20 17:44:06]

On 2012/11/19 23:13:18, Ben Goodger (Google) wrote:
> but do the call sites all have a NativeView/Window that corresponds to some
> window within ash that they wish to send key events to?

- The keyboard ones trivially do (they already require a NativeWindow arg)
- SendMouseMove/SendMouseEvents/SendMouseClick: I haven't done the work of
trying to convert all of them yet, but by inspection it looks like the majority
do have context available. As most usage is in UI tests it seems reasonable that
they'd have to have a window/browser/something handy.

The keyboard methods suggest another approach: we could pass a NativeView/Window
to the mouse ones and do the dispatch inside a static _aurawin implementation if
that seems less Category 5. Or do you have another more direct approach in mind?

[scottmg, 2012-11-20 22:18:11]

Per conversation:

As the majority of uses are in test code, it'd be nice to move ui_controls to
either ui/base/test or chrome/test/base (and maybe merge with ui_test_utils).

I went through usages of ui_controls. The problematic areas I see are:
1. BrowserView::Cut/Copy/Paste
2. AutomationProvider and implementations of it, enable_automation in gyp, and
#if defined(ENABLE_AUTOMATION)
http://code.google.com/searchframe#search/&exact_package=chromium&q=defined+e...

The rest are in test code, or are the implementations of ui_controls in ui,
ui/aura, etc.

For #1, there must be some better way, maybe sending the accelerator command or
equivalent? I'm not exactly sure, but seems solvable.

For #2, it appears that all builds except ios and android always have automation
on. So this one might be a bit harder. It looks like this automation interface
is used for QA (pyauto tests?) and more recently for gpu performance/correctness
tests. I'm not sure if it's reasonable to have a modified build where we include
additional testing code in that case?

Anyone (esp. oshima) know of anything else that might be a problem?