Fix touch selection for Athena home card

Fix touch selection for Athena home card

This needs:
 - Changing AthenaWindowTreeClient to return parent of a transient
   parent as its transient child's default parent;
 - Relaxing the assumption that home card container can only contain the
   home card. It can also contain touch selection handles and quick
   menu;
 - Moving touch selection handles when a textfield view is moved around.
   Currently, handles are moved only when the window is moved.

BUG=401109
Committed: https://crrev.com/de4a75910d32558f43bc0b0304e58a45d3eea43a
Cr-Commit-Position: refs/heads/master@{#293424}

[mohsen, 2014-09-03 21:57:19]

mohsen@chromium.org changed reviewers:
+ oshima@chromium.org

[mohsen, 2014-09-03 21:57:19]

oshima@: Can you please take a look at Athena changes...
(mukai@ cc'd)

[oshima, 2014-09-04 00:23:35]

https://codereview.chromium.org/533403002/diff/1/athena/home/home_card_impl.cc
File athena/home/home_card_impl.cc (right):

https://codereview.chromium.org/533403002/diff/1/athena/home/home_card_impl.c...
athena/home/home_card_impl.cc:123: if (!home_card_ || home_card_ == child)
why !home_card_ ? Layout is no-op when it's NULL.

https://codereview.chromium.org/533403002/diff/1/athena/screen/screen_manager...
File athena/screen/screen_manager_impl.cc (right):

https://codereview.chromium.org/533403002/diff/1/athena/screen/screen_manager...
athena/screen/screen_manager_impl.cc:104: return container;
can you check if the window has ContainerParams instead?

https://codereview.chromium.org/533403002/diff/1/ui/views/touchui/touch_selec...
File ui/views/touchui/touch_selection_controller_impl.cc (left):

https://codereview.chromium.org/533403002/diff/1/ui/views/touchui/touch_selec...
ui/views/touchui/touch_selection_controller_impl.cc:441: HideContextMenu();
are these change intentional?

[Jun Mukai, 2014-09-04 00:31:58]

mukai@chromium.org changed reviewers:
+ mukai@chromium.org

[Jun Mukai, 2014-09-04 00:31:59]

https://codereview.chromium.org/533403002/diff/1/athena/home/home_card_impl.cc
File athena/home/home_card_impl.cc (right):

https://codereview.chromium.org/533403002/diff/1/athena/home/home_card_impl.c...
athena/home/home_card_impl.cc:123: if (!home_card_ || home_card_ == child)
On 2014/09/04 00:23:35, oshima wrote:
> why !home_card_ ? Layout is no-op when it's NULL.

This method can simply be empty instead. Anyways, home_card_ must be NULL in
this case because it's reset above OnWillRemoveWindowFromLayout().

[mohsen, 2014-09-04 14:57:23]

https://codereview.chromium.org/533403002/diff/1/athena/home/home_card_impl.cc
File athena/home/home_card_impl.cc (right):

https://codereview.chromium.org/533403002/diff/1/athena/home/home_card_impl.c...
athena/home/home_card_impl.cc:123: if (!home_card_ || home_card_ == child)
On 2014/09/04 00:31:58, Jun Mukai wrote:
> On 2014/09/04 00:23:35, oshima wrote:
> > why !home_card_ ? Layout is no-op when it's NULL.
> 
> This method can simply be empty instead. Anyways, home_card_ must be NULL in
> this case because it's reset above OnWillRemoveWindowFromLayout().

Yep, this function is no-op in any case. Removed contents.

https://codereview.chromium.org/533403002/diff/1/athena/screen/screen_manager...
File athena/screen/screen_manager_impl.cc (right):

https://codereview.chromium.org/533403002/diff/1/athena/screen/screen_manager...
athena/screen/screen_manager_impl.cc:104: return container;
On 2014/09/04 00:23:35, oshima wrote:
> can you check if the window has ContainerParams instead?

I just saw that there is a GetContainer() function defined above that does
exactly what we want here. There, it checks if parent of a window is root window
to decide if the window is a container or not. I updated the code here to use it
and also updated that function to check ContainerParams.

https://codereview.chromium.org/533403002/diff/1/ui/views/touchui/touch_selec...
File ui/views/touchui/touch_selection_controller_impl.cc (left):

https://codereview.chromium.org/533403002/diff/1/ui/views/touchui/touch_selec...
ui/views/touchui/touch_selection_controller_impl.cc:441: HideContextMenu();
On 2014/09/04 00:23:35, oshima wrote:
> are these change intentional?

Yes. These are not needed. For this case, context menu is already hidden in
SetDraggingHandle() which is called before this one. For the one in
OnWidgetBoundsChanged(), HideContextMenu() is called in next line's call to
SelectionChanged() if needed.

[oshima, 2014-09-04 15:04:53]

lg, but just one more request.

https://codereview.chromium.org/533403002/diff/20001/athena/screen/screen_man...
File athena/screen/screen_manager_impl.cc (right):

https://codereview.chromium.org/533403002/diff/20001/athena/screen/screen_man...
athena/screen/screen_manager_impl.cc:51: return GetContainer(window->parent());
this is not your fault, but you can change to use loop instead of recursion?

[mohsen, 2014-09-04 15:39:54]

https://codereview.chromium.org/533403002/diff/20001/athena/screen/screen_man...
File athena/screen/screen_manager_impl.cc (right):

https://codereview.chromium.org/533403002/diff/20001/athena/screen/screen_man...
athena/screen/screen_manager_impl.cc:51: return GetContainer(window->parent());
On 2014/09/04 15:04:53, oshima wrote:
> this is not your fault, but you can change to use loop instead of recursion?

Sure; Done.

[oshima, 2014-09-04 18:10:28]

lgtm

thanks!

[mohsen, 2014-09-04 20:44:58]

mohsen@chromium.org changed reviewers:
+ sky@chromium.org

[mohsen, 2014-09-04 20:44:59]

sky@: Can you take a look at views changes?

[sky, 2014-09-04 21:53:41]

LGTM with the following change

https://codereview.chromium.org/533403002/diff/40001/ui/views/controls/textfi...
File ui/views/controls/textfield/textfield.h (right):

https://codereview.chromium.org/533403002/diff/40001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield.h:228: virtual bool
NeedsNotificationWhenVisibleBoundsChange() const OVERRIDE;
Can we have a Get here, eg GetNeedsNo...

[mohsen, 2014-09-04 22:31:52]

https://codereview.chromium.org/533403002/diff/40001/ui/views/controls/textfi...
File ui/views/controls/textfield/textfield.h (right):

https://codereview.chromium.org/533403002/diff/40001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield.h:228: virtual bool
NeedsNotificationWhenVisibleBoundsChange() const OVERRIDE;
On 2014/09/04 21:53:41, sky wrote:
> Can we have a Get here, eg GetNeedsNo...

Yep, Like patch set 4?

[sky, 2014-09-04 22:39:34]

LGTM - thanks

[mohsen, 2014-09-05 00:13:14]

The CQ bit was checked by mohsen@chromium.org

[commit-bot: I haz the power, 2014-09-05 00:14:19]

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

[commit-bot: I haz the power, 2014-09-05 03:45:06]

Committed patchset #4 (id:60001) as 840ae7338058fc6cc42f4045d3ee15dd0c824576

[commit-bot: I haz the power, 2014-09-10 03:36:21]

Patchset 4 (id:??) landed as
https://crrev.com/de4a75910d32558f43bc0b0304e58a45d3eea43a
Cr-Commit-Position: refs/heads/master@{#293424}