mash: Migrate ShelfTooltipManager to wm common types.

mash: Migrate ShelfTooltipManager to wm common types.

ShelfLayoutManagerObserver -> WmShelfObserver
Use OnBeforeBubbleWidgetInit for display/container init.

Add WmWindow::Add[Remove]LimitedPreTargetHandler
(only supports that window itself, not embedded windows)

Make it a pre-target handler for the shelf WmWindow.
(as opposed to the root aura window for the display)

BUG=629255
TEST=Automated tests; no shelf tooltip behavior changes.
R=jamescook@chromium.org,sky@chromium.org

Committed: https://crrev.com/7d55249c4cf653a3c699764e37658b4ba1e11710
Cr-Commit-Position: refs/heads/master@{#407927}

[msw, 2016-07-22 22:41:24]

Description was changed from

==========
mash: Migrate ShelfTooltipManager to wm common types.

BUG=629255
==========

to

==========
mash: Migrate ShelfTooltipManager to wm common types.

ShelfLayoutManagerObserver -> WmShelfObserver
Use OnBeforeBubbleWidgetInit for display/container init.

Make it a pre-target handler for the shelf aura::Window.
(as opposed to the root aura window for the display)

BUG=629255
TEST=Automated tests; no shelf tooltip behavior changes.
R=jamescook@chromium.org,sky@chromium.org
==========

[msw, 2016-07-22 22:41:24]

msw@chromium.org changed reviewers:
+ jamescook@chromium.org, sky@chromium.org

[msw, 2016-07-22 22:41:39]

Hey James and Scott, please take a look; thanks!

https://codereview.chromium.org/2176813002/diff/40001/ash/shelf/shelf_tooltip...
File ash/shelf/shelf_tooltip_manager.cc (right):

https://codereview.chromium.org/2176813002/diff/40001/ash/shelf/shelf_tooltip...
ash/shelf/shelf_tooltip_manager.cc:18: #include "ui/aura/window.h"
Can/should I avoid this? Use WmWindow and add support via ui::Window?

[msw, 2016-07-22 22:41:52]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 22:42:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-22 22:45:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-22 22:45:43]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[msw, 2016-07-22 22:58:37]

Patchset #4 (id:60001) has been deleted

[msw, 2016-07-22 22:59:05]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 22:59:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-22 23:10:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-22 23:10:22]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[James Cook, 2016-07-22 23:48:21]

The WM conversion stuff looks fine, but you might want to hold off on more work
until after we talk on Monday about how to handle pointer watchers / event
handlers.

https://codereview.chromium.org/2176813002/diff/80001/ash/shelf/shelf_tooltip...
File ash/shelf/shelf_tooltip_manager.cc (left):

https://codereview.chromium.org/2176813002/diff/80001/ash/shelf/shelf_tooltip...
ash/shelf/shelf_tooltip_manager.cc:155: views::BubbleBorder::Arrow arrow =
shelf->SelectValueForShelfAlignment(
Aside, not for this CL, do you think we should port SelectValueForShelfAlignment
to WmShelf?  I don't feel strongly about it, which is why I haven't done it. :-)

https://codereview.chromium.org/2176813002/diff/80001/ash/shelf/shelf_tooltip...
ash/shelf/shelf_tooltip_manager.cc:235: 
It's so nice that you can get rid of these.

https://codereview.chromium.org/2176813002/diff/80001/ash/shelf/shelf_tooltip...
File ash/shelf/shelf_tooltip_manager.cc (right):

https://codereview.chromium.org/2176813002/diff/80001/ash/shelf/shelf_tooltip...
ash/shelf/shelf_tooltip_manager.cc:111:
shelf_view_->GetWidget()->GetNativeWindow()->PrependPreTargetHandler(this);
Remove not prepend?

https://codereview.chromium.org/2176813002/diff/80001/ash/shelf/shelf_view.h
File ash/shelf/shelf_view.h (right):

https://codereview.chromium.org/2176813002/diff/80001/ash/shelf/shelf_view.h#...
ash/shelf/shelf_view.h:78: WmShelf* wm_shelf() const { return wm_shelf_; }
optional: This is OK, but what do you think of passing WmShelf into the
ShelfTooltipManager constructor instead? I've been trying to dependency-inject
things in the shelf code because I think it makes it a little more obvious about
what needs to be constructed before what else. I don't feel strongly, though.

[msw, 2016-07-25 18:42:15]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-25 18:42:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msw, 2016-07-25 19:14:35]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-25 19:15:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-25 20:55:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-25 20:55:59]

Dry run: This issue passed the CQ dry run.

[msw, 2016-07-25 22:52:56]

Patchset #7 (id:140001) has been deleted

[msw, 2016-07-25 22:54:30]

Description was changed from

==========
mash: Migrate ShelfTooltipManager to wm common types.

ShelfLayoutManagerObserver -> WmShelfObserver
Use OnBeforeBubbleWidgetInit for display/container init.

Make it a pre-target handler for the shelf aura::Window.
(as opposed to the root aura window for the display)

BUG=629255
TEST=Automated tests; no shelf tooltip behavior changes.
R=jamescook@chromium.org,sky@chromium.org
==========

to

==========
mash: Migrate ShelfTooltipManager to wm common types.

ShelfLayoutManagerObserver -> WmShelfObserver
Use OnBeforeBubbleWidgetInit for display/container init.

Add WmWindow::Add[Remove]LimitedPreTargetHandler
(only supports that window itself, not embedded windows)

Make it a pre-target handler for the shelf WmWindow.
(as opposed to the root aura window for the display)

BUG=629255
TEST=Automated tests; no shelf tooltip behavior changes.
R=jamescook@chromium.org,sky@chromium.org
==========

[msw, 2016-07-25 22:56:44]

Hey James and Scott, please take a look; thanks!
Scott: wm_window; James: everything.

https://codereview.chromium.org/2176813002/diff/80001/ash/shelf/shelf_tooltip...
File ash/shelf/shelf_tooltip_manager.cc (left):

https://codereview.chromium.org/2176813002/diff/80001/ash/shelf/shelf_tooltip...
ash/shelf/shelf_tooltip_manager.cc:155: views::BubbleBorder::Arrow arrow =
shelf->SelectValueForShelfAlignment(
On 2016/07/22 23:48:21, James Cook wrote:
> Aside, not for this CL, do you think we should port
SelectValueForShelfAlignment
> to WmShelf?  I don't feel strongly about it, which is why I haven't done it.
:-)

Maybe to ash/common/shelf/wm_shelf_util.h? It's shorter than a switch, but less
self-evident which values are used for each alignment; I don't feel strongly.

https://codereview.chromium.org/2176813002/diff/80001/ash/shelf/shelf_tooltip...
ash/shelf/shelf_tooltip_manager.cc:235: 
On 2016/07/22 23:48:21, James Cook wrote:
> It's so nice that you can get rid of these.

Acknowledged.

https://codereview.chromium.org/2176813002/diff/80001/ash/shelf/shelf_tooltip...
File ash/shelf/shelf_tooltip_manager.cc (right):

https://codereview.chromium.org/2176813002/diff/80001/ash/shelf/shelf_tooltip...
ash/shelf/shelf_tooltip_manager.cc:111:
shelf_view_->GetWidget()->GetNativeWindow()->PrependPreTargetHandler(this);
On 2016/07/22 23:48:21, James Cook wrote:
> Remove not prepend?

Done.

https://codereview.chromium.org/2176813002/diff/80001/ash/shelf/shelf_view.h
File ash/shelf/shelf_view.h (right):

https://codereview.chromium.org/2176813002/diff/80001/ash/shelf/shelf_view.h#...
ash/shelf/shelf_view.h:78: WmShelf* wm_shelf() const { return wm_shelf_; }
On 2016/07/22 23:48:21, James Cook wrote:
> optional: This is OK, but what do you think of passing WmShelf into the
> ShelfTooltipManager constructor instead? I've been trying to dependency-inject
> things in the shelf code because I think it makes it a little more obvious
about
> what needs to be constructed before what else. I don't feel strongly, though.

The WmShelf is injected into ShelfView via ctor, seems okay to use that value
from an 'internal' object owned by ShelfView (like tooltip manager), but I don't
feel too strongly, and would gladly change it.

https://codereview.chromium.org/2176813002/diff/160001/ash/mus/bridge/wm_wind...
File ash/mus/bridge/wm_window_mus.cc (right):

https://codereview.chromium.org/2176813002/diff/160001/ash/mus/bridge/wm_wind...
ash/mus/bridge/wm_window_mus.cc:744:
widget_->GetNativeWindow()->AddPreTargetHandler(handler);
q: Should this use GetInternalWidget (and bail if null)? Something else?

[msw, 2016-07-25 22:56:51]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-25 22:57:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-25 23:37:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-25 23:37:36]

Dry run: This issue passed the CQ dry run.

[sky, 2016-07-26 13:25:20]

https://codereview.chromium.org/2176813002/diff/160001/ash/aura/wm_window_aur...
File ash/aura/wm_window_aura.cc (right):

https://codereview.chromium.org/2176813002/diff/160001/ash/aura/wm_window_aur...
ash/aura/wm_window_aura.cc:666: window_->AddPreTargetHandler(handler);
This will give different semantics than the mus side, but as long there are no
child windows it shouldn't matter. Maybe a comment here?

https://codereview.chromium.org/2176813002/diff/160001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_tooltip_manager.cc (left):

https://codereview.chromium.org/2176813002/diff/160001/ash/common/shelf/shelf...
ash/common/shelf/shelf_tooltip_manager.cc:221: void
ShelfTooltipManager::OnWindowDestroying(aura::Window* window) {
Is this and the next no loner necessary because of destruction order?

https://codereview.chromium.org/2176813002/diff/160001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_tooltip_manager.cc (right):

https://codereview.chromium.org/2176813002/diff/160001/ash/common/shelf/shelf...
ash/common/shelf/shelf_tooltip_manager.cc:111: window =
WmLookup::Get()->GetWindowForWidget(shelf_view_->GetWidget());
You have this in a number of places, is it worth a GetShelfWidgetWindow?

https://codereview.chromium.org/2176813002/diff/160001/ash/common/wm_window.h
File ash/common/wm_window.h (right):

https://codereview.chromium.org/2176813002/diff/160001/ash/common/wm_window.h...
ash/common/wm_window.h:288: // does not recieve events from embedded windows.
Does not transfer ownership.
Also document this only receives events targetted at just this window.

https://codereview.chromium.org/2176813002/diff/160001/ash/mus/bridge/wm_wind...
File ash/mus/bridge/wm_window_mus.cc (right):

https://codereview.chromium.org/2176813002/diff/160001/ash/mus/bridge/wm_wind...
ash/mus/bridge/wm_window_mus.cc:744:
widget_->GetNativeWindow()->AddPreTargetHandler(handler);
On 2016/07/25 22:56:44, msw wrote:
> q: Should this use GetInternalWidget (and bail if null)? Something else?

It's possible to make the non-internal widget case work, but we can figure that
out later if necessary. How about extending the comment in
AddLimitedPreTargetHandler to indicate only for windows associated with a widget
and a DCHECK(GetInternalWidget()) ?

[James Cook, 2016-07-26 16:31:14]

https://codereview.chromium.org/2176813002/diff/160001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_tooltip_manager.cc (left):

https://codereview.chromium.org/2176813002/diff/160001/ash/common/shelf/shelf...
ash/common/shelf/shelf_tooltip_manager.cc:221: void
ShelfTooltipManager::OnWindowDestroying(aura::Window* window) {
On 2016/07/26 13:25:20, sky wrote:
> Is this and the next no loner necessary because of destruction order?

Same question. I suspect it's OK since you're no longer directly observing root
window and WmShelf is long lived, but it might be worth manually trying out the
external monitor connect/disconnect case. (You're probably familiar with this,
but if not, run chrome with --ash-debug-shortcuts and hit Ctrl-Shift-D to
add/remove.)

https://codereview.chromium.org/2176813002/diff/160001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_tooltip_manager.cc (right):

https://codereview.chromium.org/2176813002/diff/160001/ash/common/shelf/shelf...
ash/common/shelf/shelf_tooltip_manager.cc:55: gfx::Insets
insets(kArrowOffsetTopBottom, kArrowOffsetLeftRight);
Just FYI, vakha has a lgtm'd patch that touches this line (and others in this
file), so you've got a rebase ahead of you.
https://codereview.chromium.org/2174643002/

https://codereview.chromium.org/2176813002/diff/160001/ash/common/shelf/shelf...
ash/common/shelf/shelf_tooltip_manager.cc:205:
views::View::ConvertPointFromWidget(shelf_view_, &point);
Just to double-check: Is this still needed? I'm not sure what coordinate system
is used for events matched by a pre-target handler at the window level.

https://codereview.chromium.org/2176813002/diff/160001/ash/shelf/shelf_toolti...
File ash/shelf/shelf_tooltip_manager_unittest.cc (left):

https://codereview.chromium.org/2176813002/diff/160001/ash/shelf/shelf_toolti...
ash/shelf/shelf_tooltip_manager_unittest.cc:9: #include
"ash/shelf/shelf_layout_manager.h"
Hooray for eliminating this include!

[msw, 2016-07-26 17:43:25]

Please take another look; thanks!

https://codereview.chromium.org/2176813002/diff/160001/ash/aura/wm_window_aur...
File ash/aura/wm_window_aura.cc (right):

https://codereview.chromium.org/2176813002/diff/160001/ash/aura/wm_window_aur...
ash/aura/wm_window_aura.cc:666: window_->AddPreTargetHandler(handler);
On 2016/07/26 13:25:20, sky wrote:
> This will give different semantics than the mus side, but as long there are no
> child windows it shouldn't matter. Maybe a comment here?

Added a vague comment, let me know if you have something better in mind.

https://codereview.chromium.org/2176813002/diff/160001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_tooltip_manager.cc (left):

https://codereview.chromium.org/2176813002/diff/160001/ash/common/shelf/shelf...
ash/common/shelf/shelf_tooltip_manager.cc:221: void
ShelfTooltipManager::OnWindowDestroying(aura::Window* window) {
On 2016/07/26 16:31:14, James Cook wrote:
> On 2016/07/26 13:25:20, sky wrote:
> > Is this and the next no loner necessary because of destruction order?
> 
> Same question. I suspect it's OK since you're no longer directly observing
root
> window and WmShelf is long lived, but it might be worth manually trying out
the
> external monitor connect/disconnect case. (You're probably familiar with this,
> but if not, run chrome with --ash-debug-shortcuts and hit Ctrl-Shift-D to
> add/remove.)

Yeah, this is no longer an observer of the aura root window, nor an observer of
the shelf layout manager. As the instance is owned by ShelfView, teardown in the
destructor seems sufficient. The dtor accesses shelf_view_->wm_shelf() and
shelf_view_->GetWidget (which needed a null check for unit tests), but otherwise
this works in automated and manual testing, including external monitor
connect/disconnect, and overflow views. Let me know if you're concerned, I can
split Shutdown out the of the dtor and try to call it earlier (perhaps in
ShelfView::ViewHierarchyChanged or make this a WidgetRemovalsObserver and
teardown in OnWillRemoveView, not sure if these would be better/safer).

https://codereview.chromium.org/2176813002/diff/160001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_tooltip_manager.cc (right):

https://codereview.chromium.org/2176813002/diff/160001/ash/common/shelf/shelf...
ash/common/shelf/shelf_tooltip_manager.cc:55: gfx::Insets
insets(kArrowOffsetTopBottom, kArrowOffsetLeftRight);
On 2016/07/26 16:31:14, James Cook wrote:
> Just FYI, vakha has a lgtm'd patch that touches this line (and others in this
> file), so you've got a rebase ahead of you.
> https://codereview.chromium.org/2174643002/

Acknowledged.

https://codereview.chromium.org/2176813002/diff/160001/ash/common/shelf/shelf...
ash/common/shelf/shelf_tooltip_manager.cc:111: window =
WmLookup::Get()->GetWindowForWidget(shelf_view_->GetWidget());
On 2016/07/26 13:25:20, sky wrote:
> You have this in a number of places, is it worth a GetShelfWidgetWindow?

Three is borderline, and only this needs to check the widget; left as-is.

https://codereview.chromium.org/2176813002/diff/160001/ash/common/shelf/shelf...
ash/common/shelf/shelf_tooltip_manager.cc:205:
views::View::ConvertPointFromWidget(shelf_view_, &point);
On 2016/07/26 16:31:14, James Cook wrote:
> Just to double-check: Is this still needed? I'm not sure what coordinate
system
> is used for events matched by a pre-target handler at the window level.

I tested removing this, and it breaks (reverses) the location in RTL UI, so I
left it here. I think the widget bounds and window bounds are equivalent in this
case, so given the event's location in window bounds, we can convert directly
from widget bounds to view bounds.

https://codereview.chromium.org/2176813002/diff/160001/ash/common/wm_window.h
File ash/common/wm_window.h (right):

https://codereview.chromium.org/2176813002/diff/160001/ash/common/wm_window.h...
ash/common/wm_window.h:288: // does not recieve events from embedded windows.
Does not transfer ownership.
On 2016/07/26 13:25:20, sky wrote:
> Also document this only receives events targetted at just this window.

Updated w/"targeted at this window", lmk if you have something better in mind.

https://codereview.chromium.org/2176813002/diff/160001/ash/mus/bridge/wm_wind...
File ash/mus/bridge/wm_window_mus.cc (right):

https://codereview.chromium.org/2176813002/diff/160001/ash/mus/bridge/wm_wind...
ash/mus/bridge/wm_window_mus.cc:744:
widget_->GetNativeWindow()->AddPreTargetHandler(handler);
On 2016/07/26 13:25:20, sky wrote:
> On 2016/07/25 22:56:44, msw wrote:
> > q: Should this use GetInternalWidget (and bail if null)? Something else?
> 
> It's possible to make the non-internal widget case work, but we can figure
that
> out later if necessary. How about extending the comment in
> AddLimitedPreTargetHandler to indicate only for windows associated with a
widget
> and a DCHECK(GetInternalWidget()) ?

Done.

https://codereview.chromium.org/2176813002/diff/160001/ash/shelf/shelf_toolti...
File ash/shelf/shelf_tooltip_manager_unittest.cc (left):

https://codereview.chromium.org/2176813002/diff/160001/ash/shelf/shelf_toolti...
ash/shelf/shelf_tooltip_manager_unittest.cc:9: #include
"ash/shelf/shelf_layout_manager.h"
On 2016/07/26 16:31:14, James Cook wrote:
> Hooray for eliminating this include!

Acknowledged.

[msw, 2016-07-26 17:44:28]

Please take another look; thanks!

[msw, 2016-07-26 17:44:58]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-26 17:45:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[James Cook, 2016-07-26 17:55:38]

LGTM

https://codereview.chromium.org/2176813002/diff/160001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_tooltip_manager.cc (left):

https://codereview.chromium.org/2176813002/diff/160001/ash/common/shelf/shelf...
ash/common/shelf/shelf_tooltip_manager.cc:221: void
ShelfTooltipManager::OnWindowDestroying(aura::Window* window) {
On 2016/07/26 17:43:25, msw wrote:
> On 2016/07/26 16:31:14, James Cook wrote:
> > On 2016/07/26 13:25:20, sky wrote:
> > > Is this and the next no loner necessary because of destruction order?
> > 
> > Same question. I suspect it's OK since you're no longer directly observing
> root
> > window and WmShelf is long lived, but it might be worth manually trying out
> the
> > external monitor connect/disconnect case. (You're probably familiar with
this,
> > but if not, run chrome with --ash-debug-shortcuts and hit Ctrl-Shift-D to
> > add/remove.)
> 
> Yeah, this is no longer an observer of the aura root window, nor an observer
of
> the shelf layout manager. As the instance is owned by ShelfView, teardown in
the
> destructor seems sufficient. The dtor accesses shelf_view_->wm_shelf() and
> shelf_view_->GetWidget (which needed a null check for unit tests), but
otherwise
> this works in automated and manual testing, including external monitor
> connect/disconnect, and overflow views. Let me know if you're concerned, I can
> split Shutdown out the of the dtor and try to call it earlier (perhaps in
> ShelfView::ViewHierarchyChanged or make this a WidgetRemovalsObserver and
> teardown in OnWillRemoveView, not sure if these would be better/safer).

I think it's fine as you have it.

https://codereview.chromium.org/2176813002/diff/160001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_tooltip_manager.cc (right):

https://codereview.chromium.org/2176813002/diff/160001/ash/common/shelf/shelf...
ash/common/shelf/shelf_tooltip_manager.cc:205:
views::View::ConvertPointFromWidget(shelf_view_, &point);
On 2016/07/26 17:43:25, msw wrote:
> On 2016/07/26 16:31:14, James Cook wrote:
> > Just to double-check: Is this still needed? I'm not sure what coordinate
> system
> > is used for events matched by a pre-target handler at the window level.
> 
> I tested removing this, and it breaks (reverses) the location in RTL UI, so I
> left it here. I think the widget bounds and window bounds are equivalent in
this
> case, so given the event's location in window bounds, we can convert directly
> from widget bounds to view bounds.

Ah, RTL. Good to know.

[commit-bot: I haz the power, 2016-07-26 18:38:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-26 18:38:29]

Dry run: This issue passed the CQ dry run.

[msw, 2016-07-26 20:41:46]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-26 20:42:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sky, 2016-07-26 21:07:20]

LGTM

https://codereview.chromium.org/2176813002/diff/200001/ash/common/wm_window.h
File ash/common/wm_window.h (right):

https://codereview.chromium.org/2176813002/diff/200001/ash/common/wm_window.h...
ash/common/wm_window.h:289: // from embedded windows. This only supports windows
with associated widgets.
associated -> internal
And a comment to see GetInternalWidget().

[msw, 2016-07-26 21:17:21]

https://codereview.chromium.org/2176813002/diff/200001/ash/common/wm_window.h
File ash/common/wm_window.h (right):

https://codereview.chromium.org/2176813002/diff/200001/ash/common/wm_window.h...
ash/common/wm_window.h:289: // from embedded windows. This only supports windows
with associated widgets.
On 2016/07/26 21:07:20, sky wrote:
> associated -> internal
> And a comment to see GetInternalWidget().

Done.

[msw, 2016-07-26 21:17:32]

The CQ bit was checked by msw@chromium.org

[msw, 2016-07-26 21:17:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2176813002/#ps220001
(title: "Update comment.")

[commit-bot: I haz the power, 2016-07-26 21:18:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-26 22:06:10]

Description was changed from

==========
mash: Migrate ShelfTooltipManager to wm common types.

ShelfLayoutManagerObserver -> WmShelfObserver
Use OnBeforeBubbleWidgetInit for display/container init.

Add WmWindow::Add[Remove]LimitedPreTargetHandler
(only supports that window itself, not embedded windows)

Make it a pre-target handler for the shelf WmWindow.
(as opposed to the root aura window for the display)

BUG=629255
TEST=Automated tests; no shelf tooltip behavior changes.
R=jamescook@chromium.org,sky@chromium.org
==========

to

==========
mash: Migrate ShelfTooltipManager to wm common types.

ShelfLayoutManagerObserver -> WmShelfObserver
Use OnBeforeBubbleWidgetInit for display/container init.

Add WmWindow::Add[Remove]LimitedPreTargetHandler
(only supports that window itself, not embedded windows)

Make it a pre-target handler for the shelf WmWindow.
(as opposed to the root aura window for the display)

BUG=629255
TEST=Automated tests; no shelf tooltip behavior changes.
R=jamescook@chromium.org,sky@chromium.org
==========

[commit-bot: I haz the power, 2016-07-26 22:06:11]

Committed patchset #10 (id:220001)

[commit-bot: I haz the power, 2016-07-26 22:08:57]

Description was changed from

==========
mash: Migrate ShelfTooltipManager to wm common types.

ShelfLayoutManagerObserver -> WmShelfObserver
Use OnBeforeBubbleWidgetInit for display/container init.

Add WmWindow::Add[Remove]LimitedPreTargetHandler
(only supports that window itself, not embedded windows)

Make it a pre-target handler for the shelf WmWindow.
(as opposed to the root aura window for the display)

BUG=629255
TEST=Automated tests; no shelf tooltip behavior changes.
R=jamescook@chromium.org,sky@chromium.org
==========

to

==========
mash: Migrate ShelfTooltipManager to wm common types.

ShelfLayoutManagerObserver -> WmShelfObserver
Use OnBeforeBubbleWidgetInit for display/container init.

Add WmWindow::Add[Remove]LimitedPreTargetHandler
(only supports that window itself, not embedded windows)

Make it a pre-target handler for the shelf WmWindow.
(as opposed to the root aura window for the display)

BUG=629255
TEST=Automated tests; no shelf tooltip behavior changes.
R=jamescook@chromium.org,sky@chromium.org

Committed: https://crrev.com/7d55249c4cf653a3c699764e37658b4ba1e11710
Cr-Commit-Position: refs/heads/master@{#407927}
==========

[commit-bot: I haz the power, 2016-07-26 22:08:58]

Patchset 10 (id:??) landed as
https://crrev.com/7d55249c4cf653a3c699764e37658b4ba1e11710
Cr-Commit-Position: refs/heads/master@{#407927}