mus: Use TooltipManagerAura in NativeWidgetMus.

mus: Use TooltipManagerAura in NativeWidgetMus.

mash now shows tooltips on hover over items.

BUG=599558
Committed: https://crrev.com/1b3d5f1878974c14c21a29a13a93ef630c74d1f6
Cr-Commit-Position: refs/heads/master@{#422562}

[Elliot Glaysher, 2016-09-29 20:19:36]

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

[commit-bot: I haz the power, 2016-09-29 20:19:58]

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

[Elliot Glaysher, 2016-09-29 21:10:36]

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

[commit-bot: I haz the power, 2016-09-29 21:10:55]

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

[Elliot Glaysher, 2016-09-29 21:53:41]

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

[commit-bot: I haz the power, 2016-09-29 21:54:00]

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

[Elliot Glaysher, 2016-09-29 22:34:05]

erg@chromium.org changed reviewers:
+ jonross@chromium.org, sky@chromium.org

[Elliot Glaysher, 2016-09-29 22:34:06]

jonross: So I took a stab at this bug. The linked bug says that this will
require mus integration, but direct integration of the aura code in the client
appears to work just fine in mash. Is there some case here that I'm not thinking
of that would require a dedicated TooltipControllerMus with mus server
integration?

sky: Review/owners

[commit-bot: I haz the power, 2016-09-29 22:44:20]

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

[commit-bot: I haz the power, 2016-09-29 22:44:21]

Dry run: This issue passed the CQ dry run.

[sky, 2016-09-30 02:29:39]

On 2016/09/29 22:34:06, Elliot Glaysher wrote:
> jonross: So I took a stab at this bug. The linked bug says that this will
> require mus integration, but direct integration of the aura code in the client
> appears to work just fine in mash. Is there some case here that I'm not
thinking
> of that would require a dedicated TooltipControllerMus with mus server
> integration?
> 
> sky: Review/owners

Having NativeWidgetMus install a TooltipController means each client is going to
attempt to display tooltips. As each client doesn't necessarily see other client
windows this could result in multiple clients attempting to show a tooltip at
the same time for different windows because they think their window is visible
under the cursor.

I think ash needs to be responsible for handling showing of tooltips.

[sky, 2016-09-30 15:07:12]

I think I'm wrong. I was worried tooltips would get confused by any observed
pointerevents, but those are routed differently. So, I think this should just
work. The only concern I have is does it work for renderwidgethostview? My
understanding is each renderwidgethostviewmus has a ui::Window, but isn't
contained in a widget.

[sky, 2016-09-30 15:15:31]

https://codereview.chromium.org/2379073003/diff/40001/ui/views/BUILD.gn
File ui/views/BUILD.gn (right):

https://codereview.chromium.org/2379073003/diff/40001/ui/views/BUILD.gn#newco...
ui/views/BUILD.gn:928: "corewm/tooltip_controller_unittest.cc",
Can you make sure this test actually ends up creating a NativeWidgetMus? On a
quick look I suspect it doesn't, but I could be wrong.

https://codereview.chromium.org/2379073003/diff/40001/ui/views/mus/native_wid...
File ui/views/mus/native_widget_mus.cc (right):

https://codereview.chromium.org/2379073003/diff/40001/ui/views/mus/native_wid...
ui/views/mus/native_widget_mus.cc:600:
aura::client::SetTooltipClient(window_tree_host_->window(), NULL);
nullptr

[jonross, 2016-09-30 17:10:31]

On 2016/09/30 15:15:31, sky wrote:
> https://codereview.chromium.org/2379073003/diff/40001/ui/views/BUILD.gn
> File ui/views/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2379073003/diff/40001/ui/views/BUILD.gn#newco...
> ui/views/BUILD.gn:928: "corewm/tooltip_controller_unittest.cc",
> Can you make sure this test actually ends up creating a NativeWidgetMus? On a
> quick look I suspect it doesn't, but I could be wrong.
> 
>
https://codereview.chromium.org/2379073003/diff/40001/ui/views/mus/native_wid...
> File ui/views/mus/native_widget_mus.cc (right):
> 
>
https://codereview.chromium.org/2379073003/diff/40001/ui/views/mus/native_wid...
> ui/views/mus/native_widget_mus.cc:600:
> aura::client::SetTooltipClient(window_tree_host_->window(), NULL);
> nullptr

Ths bug referred to mus integration, as at the time there had been talk about
having certain features are mus services, including tooltips.

However if the current structure works I am in support of a simpler solution.

[Elliot Glaysher, 2016-09-30 21:58:13]

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

[commit-bot: I haz the power, 2016-09-30 21:58:52]

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-09-30 22:24:57]

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

[commit-bot: I haz the power, 2016-09-30 22:24:58]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Elliot Glaysher, 2016-09-30 22:29:19]

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

[commit-bot: I haz the power, 2016-09-30 22:29:42]

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-10-01 00:33:35]

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

[commit-bot: I haz the power, 2016-10-01 00:33:36]

Dry run: This issue passed the CQ dry run.

[Elliot Glaysher, 2016-10-03 17:49:03]

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

[commit-bot: I haz the power, 2016-10-03 17:49:29]

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

[Elliot Glaysher, 2016-10-03 18:04:54]

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

[commit-bot: I haz the power, 2016-10-03 18:05:25]

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

[Elliot Glaysher, 2016-10-03 19:16:14]

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

[commit-bot: I haz the power, 2016-10-03 19:16:52]

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

[Elliot Glaysher, 2016-10-03 20:38:41]

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

[commit-bot: I haz the power, 2016-10-03 20:39:38]

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-10-03 21:46:19]

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

[commit-bot: I haz the power, 2016-10-03 21:46:20]

Dry run: This issue passed the CQ dry run.

[Elliot Glaysher, 2016-10-03 21:52:26]

ptal

https://codereview.chromium.org/2379073003/diff/40001/ui/views/BUILD.gn
File ui/views/BUILD.gn (right):

https://codereview.chromium.org/2379073003/diff/40001/ui/views/BUILD.gn#newco...
ui/views/BUILD.gn:928: "corewm/tooltip_controller_unittest.cc",
On 2016/09/30 15:15:30, sky wrote:
> Can you make sure this test actually ends up creating a NativeWidgetMus? On a
> quick look I suspect it doesn't, but I could be wrong.

You're correct that this was wrong.

I've moved two of the three test suites in this file from AuraTestBase to
ViewsTestBase, and ported the native widget creation code to
CreatePlatformDesktopNativeWidgetImpl(), which has different implementation in
the ui/views/mus vs the base views code.

[sky, 2016-10-03 22:13:50]

LGTM - did you make sure tooltips set on renderers work as well?

[Elliot Glaysher, 2016-10-03 22:15:13]

(Yes. Tested with the thumbnails on the NTP.)

[Elliot Glaysher, 2016-10-03 22:15:19]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2016-10-03 22:16:12]

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-10-03 22:23:31]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-10-03 22:25:31]

Description was changed from

==========
mus: Use TooltipManagerAura in NativeWidgetMus.

mash now shows tooltips on hover over items.

BUG=599558
==========

to

==========
mus: Use TooltipManagerAura in NativeWidgetMus.

mash now shows tooltips on hover over items.

BUG=599558

Committed: https://crrev.com/1b3d5f1878974c14c21a29a13a93ef630c74d1f6
Cr-Commit-Position: refs/heads/master@{#422562}
==========

[commit-bot: I haz the power, 2016-10-03 22:25:32]

Patchset 9 (id:??) landed as
https://crrev.com/1b3d5f1878974c14c21a29a13a93ef630c74d1f6
Cr-Commit-Position: refs/heads/master@{#422562}

[sky, 2016-10-03 22:33:12]

Excellent!

On Mon, Oct 3, 2016 at 3:15 PM,  <erg@chromium.org> wrote:
> (Yes. Tested with the thumbnails on the NTP.)
>
> https://codereview.chromium.org/2379073003/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Elliot Glaysher, 2016-10-03 23:35:17]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/2384083005/ by erg@chromium.org.

The reason for reverting is: sky@ reports that
TooltipControllerCaptureTest.Capture is failing on local chromeos builds..