Change type of touch selection handles to POPUP

Change type of touch selection handles to POPUP

Previously, touch selection handles were of type TOOLTIP which in Chrome
OS would put them above almost everything else (e.g. menus and shelf;
see associated bugs). With this patch they are changed to type POPUP and
made transient children of their top-level parent, so they are
positioned correctly above their parent window, but below menus and
shelf. Also, they are not clipped to their parent window in Windows Aura
anymore, as now they have their own HWND.

BUG=303870, 316286
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=263539

[mohsen, 2014-02-27 23:03:29]

Please take a look...

[sadrul, 2014-02-28 16:43:50]

This looks good. But can you think of a test for this?

[mohsen, 2014-03-14 00:16:45]

Test added. Can you take another look, please...

[sadrul, 2014-03-14 18:30:06]

https://codereview.chromium.org/177173007/diff/40001/ui/views/touchui/touch_e...
File ui/views/touchui/touch_editing_menu.cc (right):

https://codereview.chromium.org/177173007/diff/40001/ui/views/touchui/touch_e...
ui/views/touchui/touch_editing_menu.cc:63: GetWidget()->GetNativeView(),
context);
What does the fix for this look like? Can we land this fix first?

https://codereview.chromium.org/177173007/diff/40001/ui/views/touchui/touch_s...
File ui/views/touchui/touch_selection_controller_impl.h (right):

https://codereview.chromium.org/177173007/diff/40001/ui/views/touchui/touch_s...
ui/views/touchui/touch_selection_controller_impl.h:80: gfx::NativeView
GetCursorHandleNativeView();
Hm. These are used in TouchSelectionControllerImplTest, which is already a
friend of this class. Can we remove these test methods from here, and move their
implementations in the corresponding methods of TSCImplTest?

https://codereview.chromium.org/177173007/diff/40001/ui/views/touchui/touch_s...
File ui/views/touchui/touch_selection_controller_impl_unittest.cc (right):

https://codereview.chromium.org/177173007/diff/40001/ui/views/touchui/touch_s...
ui/views/touchui/touch_selection_controller_impl_unittest.cc:530:
GetCursorHandleNativeView());
The alignment here is confusing. It should be aligned with |targeter instead.

The ordering in EXPECT_EQ is <expected>, <test>. I think in this case, the
hit-test should return the handle's native-view. So GetCursorHandleNativeView()
should b the first param.

[mohsen, 2014-03-14 19:15:25]

https://codereview.chromium.org/177173007/diff/40001/ui/views/touchui/touch_e...
File ui/views/touchui/touch_editing_menu.cc (right):

https://codereview.chromium.org/177173007/diff/40001/ui/views/touchui/touch_e...
ui/views/touchui/touch_editing_menu.cc:63: GetWidget()->GetNativeView(),
context);
 2014/03/14 18:30:07, sadrul wrote:
> What does the fix for this look like? Can we land this fix first?

There is a TransientWindowManager::OnChildStackingChanged(child) method that
restacks transient children of child properly. The simplest fix would be to use
this method after a window has become transient child of another one
(TransientWindowManager::AddTransientChild()) or when a window that is the
transient child of another one is added to the windows hierarchy (when a
transient child is created, this one needs OnChildStackingChanged() to be public
to be accessible from Window::AddChild() or a similar method). This seems a bit
ugly and also inefficient and maybe a better solution is needed.

https://codereview.chromium.org/177173007/diff/40001/ui/views/touchui/touch_s...
File ui/views/touchui/touch_selection_controller_impl.h (right):

https://codereview.chromium.org/177173007/diff/40001/ui/views/touchui/touch_s...
ui/views/touchui/touch_selection_controller_impl.h:80: gfx::NativeView
GetCursorHandleNativeView();
On 2014/03/14 18:30:07, sadrul wrote:
> Hm. These are used in TouchSelectionControllerImplTest, which is already a
> friend of this class. Can we remove these test methods from here, and move
their
> implementations in the corresponding methods of TSCImplTest?

Since EditingHandleView class is forward-declared in this header file and
declared in the .cc file, its declaration is not available in TSCImplTest and we
can't access its members there (or anywhere else other than TSCI.cc). So, these
methods are not actually 'convenience' methods!

https://codereview.chromium.org/177173007/diff/40001/ui/views/touchui/touch_s...
File ui/views/touchui/touch_selection_controller_impl_unittest.cc (right):

https://codereview.chromium.org/177173007/diff/40001/ui/views/touchui/touch_s...
ui/views/touchui/touch_selection_controller_impl_unittest.cc:530:
GetCursorHandleNativeView());
On 2014/03/14 18:30:07, sadrul wrote:
> The alignment here is confusing. It should be aligned with |targeter instead.
> 
> The ordering in EXPECT_EQ is <expected>, <test>. I think in this case, the
> hit-test should return the handle's native-view. So
GetCursorHandleNativeView()
> should b the first param.

Yep, my mistake in both cases. Fixed.

[sadrul, 2014-03-18 18:57:19]

I think it'd be good to fix the transient-window issue first, and then land this
CL without the TODOs. The rest of the code in this CL looks good.

[mohsen, 2014-04-11 15:20:16]

Now that the issue with transient windows is fixed (r263188), I have removed the
workaround in this CL. Please take a look...

[sadrul, 2014-04-11 16:51:08]

LGTM. Thanks!

[mohsen, 2014-04-12 13:33:43]

The CQ bit was checked by mohsen@chromium.org

[commit-bot: I haz the power, 2014-04-12 13:33:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mohsen@chromium.org/177173007/210001

[commit-bot: I haz the power, 2014-04-12 17:36:58]

Change committed as 263539