Implement ui_controls for aura

Implement ui_controls for aura
- Move windows impl to ui_controls_internal_win.cc so that aurawin can use it.
- For windows, we can simply adjust coordinates, so ConvertPointToNativeScreen is added.
- Aurax11 uses PostNativeEvent to send event. I changed XPutBackEvent to XSendEvent as PutBackEvent puts the event at the top.
- Aurax11 uses synthesized flash event to run "run-after-events" task. 
- exclude events_x.cc from gtk build as it's not necessary.


BUG=103497, 104396
TEST=following tests now passes on aura
PrintDialogCloudTest.HandlersRegistered (browser tests)
several tests in interactive_ui_tests (MenuModelAdapterTest,MenuItemViewTestInsert00) now passes. I'll post the list in the bug.



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

[oshima, 2011-11-22 00:00:17]

sky -> everything
derat -> can you check if my use of atom and XSendEvent is correct?

[Daniel Erat, 2011-11-22 00:25:32]

http://codereview.chromium.org/8585015/diff/18021/ui/aura/desktop.h
File ui/aura/desktop.h (right):

http://codereview.chromium.org/8585015/diff/18021/ui/aura/desktop.h#newcode131
ui/aura/desktop.h:131: // Converts the |point| in the desktop's coordinate to
native screen's
nit: would probably reword as this:

  // Converts |point| from the desktop's coordinate system to
  // the native screen's.

http://codereview.chromium.org/8585015/diff/18021/ui/aura/desktop_host.h
File ui/aura/desktop_host.h (right):

http://codereview.chromium.org/8585015/diff/18021/ui/aura/desktop_host.h#newc...
ui/aura/desktop_host.h:65: // Converts the |point| in the desktop's coordinate
to native screen's
(see comment in desktop.h)

http://codereview.chromium.org/8585015/diff/18021/ui/aura/desktop_host_linux.cc
File ui/aura/desktop_host_linux.cc (right):

http://codereview.chromium.org/8585015/diff/18021/ui/aura/desktop_host_linux....
ui/aura/desktop_host_linux.cc:505: case ButtonRelease: {
i know that we don't do anything with them (now), but for completeness, you
should include EnterNotify and LeaveNotify (and the corresponding masks above)

http://codereview.chromium.org/8585015/diff/18021/ui/aura/desktop_host_linux....
ui/aura/desktop_host_linux.cc:506: xevent.xmotion.root =
DefaultRootWindow(xdisplay_);
maybe add a comment mentioning that you're choosing xmotion arbitrarily from the
union since the fields that you're assigning to are in the same place in all of
these structs

http://codereview.chromium.org/8585015/diff/18021/ui/aura/desktop_host_linux....
ui/aura/desktop_host_linux.cc:517: ::XSendEvent(xdisplay_, xwindow_, False,
kInputEventMask, &xevent);
nit: "::" seems unnecessary here; this function name is unlikely to conflict
with anything in chrome and we don't use it for most other Xlib function calls

http://codereview.chromium.org/8585015/diff/18021/ui/base/x/events_x.cc
File ui/base/x/events_x.cc (right):

http://codereview.chromium.org/8585015/diff/18021/ui/base/x/events_x.cc#newco...
ui/base/x/events_x.cc:366: noop->xclient.display = NULL;
just memset it to 0 first?  that way you don't need to initialize these later

[sky, 2011-11-22 00:28:33]

http://codereview.chromium.org/8585015/diff/18021/chrome/browser/automation/u...
File chrome/browser/automation/ui_controls_aurawin.cc (right):

http://codereview.chromium.org/8585015/diff/18021/chrome/browser/automation/u...
chrome/browser/automation/ui_controls_aurawin.cc:60: void
MoveMouseToCenterAndPress(views::View* view, MouseButton button,
nit: each param on its own line.

http://codereview.chromium.org/8585015/diff/18021/chrome/browser/automation/u...
File chrome/browser/automation/ui_controls_aurax11.cc (right):

http://codereview.chromium.org/8585015/diff/18021/chrome/browser/automation/u...
chrome/browser/automation/ui_controls_aurax11.cc:21: // matches condition
implemented by |matcher|.
Event waiter executes the specified closure when a matching event is found.

http://codereview.chromium.org/8585015/diff/18021/chrome/browser/automation/u...
chrome/browser/automation/ui_controls_aurax11.cc:38: virtual base::EventStatus
WillProcessEvent(const base::NativeEvent& event) {
OVERRIDE here and on DidProcessEvent.

http://codereview.chromium.org/8585015/diff/18021/chrome/browser/automation/u...
chrome/browser/automation/ui_controls_aurax11.cc:47: 
nit: remove empty line.

http://codereview.chromium.org/8585015/diff/18021/chrome/browser/automation/u...
chrome/browser/automation/ui_controls_aurax11.cc:49: private:
nit: newline between 48 and 49.

http://codereview.chromium.org/8585015/diff/18021/chrome/browser/automation/u...
File chrome/browser/automation/ui_controls_internal.h (right):

http://codereview.chromium.org/8585015/diff/18021/chrome/browser/automation/u...
chrome/browser/automation/ui_controls_internal.h:22: bool control, bool shift,
bool alt,
nit: each param on its own line here and 25.

http://codereview.chromium.org/8585015/diff/18021/ui/aura/desktop.h
File ui/aura/desktop.h (right):

http://codereview.chromium.org/8585015/diff/18021/ui/aura/desktop.h#newcode132
ui/aura/desktop.h:132: // cooridnates.
coordinates

http://codereview.chromium.org/8585015/diff/18021/ui/aura/desktop_host.h
File ui/aura/desktop_host.h (right):

http://codereview.chromium.org/8585015/diff/18021/ui/aura/desktop_host.h#newc...
ui/aura/desktop_host.h:67: virtual void ConvertPointToNativeScreen(gfx::Point*
point) const = 0;
How about GetLocationOnScreen and move it beneath SetSize.

http://codereview.chromium.org/8585015/diff/18021/ui/base/x/events_x.cc
File ui/base/x/events_x.cc (right):

http://codereview.chromium.org/8585015/diff/18021/ui/base/x/events_x.cc#newco...
ui/base/x/events_x.cc:11: #if !defined(TOOLKIT_USES_GTK)
nit: conditional includes after non-conditional includes.

[oshima, 2011-11-22 01:36:09]

uploaded new patch. PTAL.

http://codereview.chromium.org/8585015/diff/18021/chrome/browser/automation/u...
File chrome/browser/automation/ui_controls_aurawin.cc (right):

http://codereview.chromium.org/8585015/diff/18021/chrome/browser/automation/u...
chrome/browser/automation/ui_controls_aurawin.cc:60: void
MoveMouseToCenterAndPress(views::View* view, MouseButton button,
On 2011/11/22 00:28:33, sky wrote:
> nit: each param on its own line.

Done.

http://codereview.chromium.org/8585015/diff/18021/chrome/browser/automation/u...
File chrome/browser/automation/ui_controls_aurax11.cc (right):

http://codereview.chromium.org/8585015/diff/18021/chrome/browser/automation/u...
chrome/browser/automation/ui_controls_aurax11.cc:21: // matches condition
implemented by |matcher|.
On 2011/11/22 00:28:33, sky wrote:
> Event waiter executes the specified closure when a matching event is found.

Done.

http://codereview.chromium.org/8585015/diff/18021/chrome/browser/automation/u...
chrome/browser/automation/ui_controls_aurax11.cc:38: virtual base::EventStatus
WillProcessEvent(const base::NativeEvent& event) {
On 2011/11/22 00:28:33, sky wrote:
> OVERRIDE here and on DidProcessEvent.

Done.

http://codereview.chromium.org/8585015/diff/18021/chrome/browser/automation/u...
chrome/browser/automation/ui_controls_aurax11.cc:47: 
On 2011/11/22 00:28:33, sky wrote:
> nit: remove empty line.

Done.

http://codereview.chromium.org/8585015/diff/18021/chrome/browser/automation/u...
chrome/browser/automation/ui_controls_aurax11.cc:49: private:
On 2011/11/22 00:28:33, sky wrote:
> nit: newline between 48 and 49.

Done.

http://codereview.chromium.org/8585015/diff/18021/chrome/browser/automation/u...
File chrome/browser/automation/ui_controls_internal.h (right):

http://codereview.chromium.org/8585015/diff/18021/chrome/browser/automation/u...
chrome/browser/automation/ui_controls_internal.h:22: bool control, bool shift,
bool alt,
On 2011/11/22 00:28:33, sky wrote:
> nit: each param on its own line here and 25.

Done.

http://codereview.chromium.org/8585015/diff/18021/ui/aura/desktop.h
File ui/aura/desktop.h (right):

http://codereview.chromium.org/8585015/diff/18021/ui/aura/desktop.h#newcode131
ui/aura/desktop.h:131: // Converts the |point| in the desktop's coordinate to
native screen's
On 2011/11/22 00:25:32, Daniel Erat wrote:
> nit: would probably reword as this:
> 
>   // Converts |point| from the desktop's coordinate system to
>   // the native screen's.

Done.

http://codereview.chromium.org/8585015/diff/18021/ui/aura/desktop_host.h
File ui/aura/desktop_host.h (right):

http://codereview.chromium.org/8585015/diff/18021/ui/aura/desktop_host.h#newc...
ui/aura/desktop_host.h:67: virtual void ConvertPointToNativeScreen(gfx::Point*
point) const = 0;
On 2011/11/22 00:28:33, sky wrote:
> How about GetLocationOnScreen and move it beneath SetSize.

Done.

http://codereview.chromium.org/8585015/diff/18021/ui/aura/desktop_host_linux.cc
File ui/aura/desktop_host_linux.cc (right):

http://codereview.chromium.org/8585015/diff/18021/ui/aura/desktop_host_linux....
ui/aura/desktop_host_linux.cc:505: case ButtonRelease: {
On 2011/11/22 00:25:32, Daniel Erat wrote:
> i know that we don't do anything with them (now), but for completeness, you
> should include EnterNotify and LeaveNotify (and the corresponding masks above)

Because X's Enter/Leave notify doesn't trigger aura events, they do not seem
to be necessary. Can you explain?

http://codereview.chromium.org/8585015/diff/18021/ui/aura/desktop_host_linux....
ui/aura/desktop_host_linux.cc:506: xevent.xmotion.root =
DefaultRootWindow(xdisplay_);
On 2011/11/22 00:25:32, Daniel Erat wrote:
> maybe add a comment mentioning that you're choosing xmotion arbitrarily from
the
> union since the fields that you're assigning to are in the same place in all
of
> these structs

Done.

http://codereview.chromium.org/8585015/diff/18021/ui/aura/desktop_host_linux....
ui/aura/desktop_host_linux.cc:517: ::XSendEvent(xdisplay_, xwindow_, False,
kInputEventMask, &xevent);
On 2011/11/22 00:25:32, Daniel Erat wrote:
> nit: "::" seems unnecessary here; this function name is unlikely to conflict
> with anything in chrome and we don't use it for most other Xlib function calls

Done.

http://codereview.chromium.org/8585015/diff/18021/ui/base/x/events_x.cc
File ui/base/x/events_x.cc (right):

http://codereview.chromium.org/8585015/diff/18021/ui/base/x/events_x.cc#newco...
ui/base/x/events_x.cc:11: #if !defined(TOOLKIT_USES_GTK)
On 2011/11/22 00:28:33, sky wrote:
> nit: conditional includes after non-conditional includes.

Done.

http://codereview.chromium.org/8585015/diff/18021/ui/base/x/events_x.cc#newco...
ui/base/x/events_x.cc:366: noop->xclient.display = NULL;
On 2011/11/22 00:25:32, Daniel Erat wrote:
> just memset it to 0 first?  that way you don't need to initialize these later

Actually this wasn't necessary. constructor should take care of it.

[Daniel Erat, 2011-11-22 01:46:14]

http://codereview.chromium.org/8585015/diff/18021/ui/aura/desktop_host_linux.cc
File ui/aura/desktop_host_linux.cc (right):

http://codereview.chromium.org/8585015/diff/18021/ui/aura/desktop_host_linux....
ui/aura/desktop_host_linux.cc:505: case ButtonRelease: {
On 2011/11/22 01:36:10, oshima wrote:
> On 2011/11/22 00:25:32, Daniel Erat wrote:
> > i know that we don't do anything with them (now), but for completeness, you
> > should include EnterNotify and LeaveNotify (and the corresponding masks
above)
> 
> Because X's Enter/Leave notify doesn't trigger aura events, they do not seem
> to be necessary. Can you explain?

They could get added to aura later, and things would break if they weren't also
added here.  Are there any downsides to handling them here?

http://codereview.chromium.org/8585015/diff/17004/ui/base/x/events_x.cc
File ui/base/x/events_x.cc (right):

http://codereview.chromium.org/8585015/diff/17004/ui/base/x/events_x.cc#newco...
ui/base/x/events_x.cc:380: noop = new XEvent();
i don't think there's any non-default constructor here; this is a C struct with
POD members

http://codereview.chromium.org/8585015/diff/17004/ui/base/x/events_x.cc#newco...
ui/base/x/events_x.cc:382: noop->xclient.window = None;
None == 0

[oshima, 2011-11-22 03:02:09]

http://codereview.chromium.org/8585015/diff/18021/ui/aura/desktop_host_linux.cc
File ui/aura/desktop_host_linux.cc (right):

http://codereview.chromium.org/8585015/diff/18021/ui/aura/desktop_host_linux....
ui/aura/desktop_host_linux.cc:505: case ButtonRelease: {
On 2011/11/22 01:46:14, Daniel Erat wrote:
> On 2011/11/22 01:36:10, oshima wrote:
> > On 2011/11/22 00:25:32, Daniel Erat wrote:
> > > i know that we don't do anything with them (now), but for completeness,
you
> > > should include EnterNotify and LeaveNotify (and the corresponding masks
> above)
> > 
> > Because X's Enter/Leave notify doesn't trigger aura events, they do not seem
> > to be necessary. Can you explain?
> 
> They could get added to aura later, and things would break if they weren't
also
> added here.  Are there any downsides to handling them here?

I just couldn't imagine how this will be needed. Only downside I can imagine
is that it would make it looks like Notify/LeaveNotify does something, which is
not (that is, we're sending event that are not handled in Dispatch.) That's
being said, it's less likely to cause real confusion, so done.

http://codereview.chromium.org/8585015/diff/17004/ui/base/x/events_x.cc
File ui/base/x/events_x.cc (right):

http://codereview.chromium.org/8585015/diff/17004/ui/base/x/events_x.cc#newco...
ui/base/x/events_x.cc:380: noop = new XEvent();
On 2011/11/22 01:46:14, Daniel Erat wrote:
> i don't think there's any non-default constructor here; this is a C struct
with
> POD members

C++ supposed to be able to initialize POD types in struct when it's allocated
with (), BUT I just realized that XEvent is union, so this doesn't work (not
sure HOW this works on union). my bad. changed to use memset.

http://codereview.chromium.org/8585015/diff/17004/ui/base/x/events_x.cc#newco...
ui/base/x/events_x.cc:382: noop->xclient.window = None;
On 2011/11/22 01:46:14, Daniel Erat wrote:
> None == 0

Hmm, it's implementation details and I think we shouldn't rely on that.

[sky, 2011-11-22 16:01:30]

LGTM

[Daniel Erat, 2011-11-22 16:37:06]

http://codereview.chromium.org/8585015/diff/20001/ui/aura/desktop_host_linux.cc
File ui/aura/desktop_host_linux.cc (right):

http://codereview.chromium.org/8585015/diff/20001/ui/aura/desktop_host_linux....
ui/aura/desktop_host_linux.cc:489: const long kInputEventMask = KeyReleaseMask |
KeyPressMask |
This is actually a bit weird.  From the man page:

"If propagate is False, the event is sent to every client selecting on
destination any of the event types in the event_mask argument."

I think that the technically-correct thing to do would be to map the event type
to the corresponding mask and just use that; otherwise other clients could hear
about events that they don't care about.  Maybe it's not a huge deal in
practice, since we just care about Chrome getting the event.

[oshima, 2011-11-22 17:22:54]

http://codereview.chromium.org/8585015/diff/20001/ui/aura/desktop_host_linux.cc
File ui/aura/desktop_host_linux.cc (right):

http://codereview.chromium.org/8585015/diff/20001/ui/aura/desktop_host_linux....
ui/aura/desktop_host_linux.cc:489: const long kInputEventMask = KeyReleaseMask |
KeyPressMask |
On 2011/11/22 16:37:06, Daniel Erat wrote:
> This is actually a bit weird.  From the man page:
> 
> "If propagate is False, the event is sent to every client selecting on
> destination any of the event types in the event_mask argument."
> 
> I think that the technically-correct thing to do would be to map the event
type
> to the corresponding mask and just use that; otherwise other clients could
hear
> about events that they don't care about.  Maybe it's not a huge deal in
> practice, since we just care about Chrome getting the event.

Seems like I misunderstood the doc. I believe I don't need to set mask at all
because it should send to the client that xwindow_ created. What do you think?

" If event_mask is the empty set, the event is sent to the client that created
the destination window.  If that client no longer exists, no
            event is sent."

[Daniel Erat, 2011-11-22 17:25:07]

On 2011/11/22 17:22:54, oshima wrote:
>
http://codereview.chromium.org/8585015/diff/20001/ui/aura/desktop_host_linux.cc
> File ui/aura/desktop_host_linux.cc (right):
> 
>
http://codereview.chromium.org/8585015/diff/20001/ui/aura/desktop_host_linux....
> ui/aura/desktop_host_linux.cc:489: const long kInputEventMask = KeyReleaseMask
|
> KeyPressMask |
> On 2011/11/22 16:37:06, Daniel Erat wrote:
> > This is actually a bit weird.  From the man page:
> > 
> > "If propagate is False, the event is sent to every client selecting on
> > destination any of the event types in the event_mask argument."
> > 
> > I think that the technically-correct thing to do would be to map the event
> type
> > to the corresponding mask and just use that; otherwise other clients could
> hear
> > about events that they don't care about.  Maybe it's not a huge deal in
> > practice, since we just care about Chrome getting the event.
> 
> Seems like I misunderstood the doc. I believe I don't need to set mask at all
> because it should send to the client that xwindow_ created. What do you think?
> 
> " If event_mask is the empty set, the event is sent to the client that created
> the destination window.  If that client no longer exists, no
>             event is sent."

Yes, I think that should be safe to do too.  Another option, perhaps messier,
might be to pass the event to the message loop.  But if you just set the event
mask to 0, this change LGTM.

[commit-bot: I haz the power, 2011-11-22 17:34:01]

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

[commit-bot: I haz the power, 2011-11-22 17:34:34]

Presubmit check for 8585015-22002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
chrome/browser/printing/print_dialog_cloud_uitest.cc

Presubmit checks took 5.5s to calculate.

[oshima, 2011-11-22 17:37:50]

On 2011/11/22 17:25:07, Daniel Erat wrote:
> On 2011/11/22 17:22:54, oshima wrote:
> >
>
http://codereview.chromium.org/8585015/diff/20001/ui/aura/desktop_host_linux.cc
> > File ui/aura/desktop_host_linux.cc (right):
> > 
> >
>
http://codereview.chromium.org/8585015/diff/20001/ui/aura/desktop_host_linux....
> > ui/aura/desktop_host_linux.cc:489: const long kInputEventMask =
KeyReleaseMask
> |
> > KeyPressMask |
> > On 2011/11/22 16:37:06, Daniel Erat wrote:
> > > This is actually a bit weird.  From the man page:
> > > 
> > > "If propagate is False, the event is sent to every client selecting on
> > > destination any of the event types in the event_mask argument."
> > > 
> > > I think that the technically-correct thing to do would be to map the event
> > type
> > > to the corresponding mask and just use that; otherwise other clients could
> > hear
> > > about events that they don't care about.  Maybe it's not a huge deal in
> > > practice, since we just care about Chrome getting the event.
> > 
> > Seems like I misunderstood the doc. I believe I don't need to set mask at
all
> > because it should send to the client that xwindow_ created. What do you
think?
> > 
> > " If event_mask is the empty set, the event is sent to the client that
created
> > the destination window.  If that client no longer exists, no
> >             event is sent."
> 
> Yes, I think that should be safe to do too.  Another option, perhaps messier,
> might be to pass the event to the message loop.

Other options I looked into are:
1) Just pass events to dispatcher. This doesn't work as it doesn't invoke
message pump observer.
2) Add event queue to MessagePumpX and check both the queue and XPending. I
didn't do this
because the execution pass becomes different for tests and real case.
3) Use XPutBackEvent in reverse order. I didn't do this because I felt it's
hacky and may be error prone.
Let me know if you have better idea.

>  But if you just set the event
> mask to 0, this change LGTM.

[oshima, 2011-11-22 17:39:19]

Lei, can you give me OWNERS approval for chrome/browser/printing? It's just
enabling test for aura.

[Lei Zhang, 2011-11-22 18:35:23]

chrome/browser/printing LGTM

[commit-bot: I haz the power, 2011-11-22 19:26:46]

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

[commit-bot: I haz the power, 2011-11-22 21:34:19]

Change committed as 111219