Disables showing tooltips while a system menu is showing on windows

Disables showing tooltips while a system menu is showing on windows

When a system menu is showing we continue to get and receive mouse
events while the mouse is over the menu. The tooltip code thinks the
mouse is over the aura::Window and will then show the tooltip. The fix
is to disable tooltips while system menus are showing.

I'm also making the only way to enable/disable tooltips be by way of a
scoper. This should hopefully be less error prone.

BUG=327970
TEST=none
R=varunjain@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243350

[varunjain, 2014-01-07 05:07:05]

LGTM

https://codereview.chromium.org/109403008/diff/50001/ui/aura/client/tooltip_c...
File ui/aura/client/tooltip_client.h (right):

https://codereview.chromium.org/109403008/diff/50001/ui/aura/client/tooltip_c...
ui/aura/client/tooltip_client.h:27: // Enables/Disables tooltips. This is
treated as a reference count. Consumers
I am not sure how much the ScopedDisabler buys. I am not opposed to to it, but I
would have just created a static void SetTooltipsEnabled(Window* window, bool
enable), same as Set/GetTooltipText() below. That way, you do not need to add an
observer.
I think I am not sure what sort of errors are you trying to protect against. Are
you worried that the root window might change between a disable and enable? If
so, first, I do not see what situation that could happen in and second, even if
it happens, ScopedDisabler in its current form is not set up to re-enable the
tooltip of the old root window and disable for the new root.

[sky, 2014-01-07 16:52:02]

https://codereview.chromium.org/109403008/diff/50001/ui/aura/client/tooltip_c...
File ui/aura/client/tooltip_client.h (right):

https://codereview.chromium.org/109403008/diff/50001/ui/aura/client/tooltip_c...
ui/aura/client/tooltip_client.h:27: // Enables/Disables tooltips. This is
treated as a reference count. Consumers
On 2014/01/07 05:07:06, varunjain wrote:
> I am not sure how much the ScopedDisabler buys. I am not opposed to to it, but
I
> would have just created a static void SetTooltipsEnabled(Window* window, bool
> enable), same as Set/GetTooltipText() below. That way, you do not need to add
an
> observer.
> I think I am not sure what sort of errors are you trying to protect against.
Are
> you worried that the root window might change between a disable and enable? If
> so, first, I do not see what situation that could happen in and second, even
if
> it happens, ScopedDisabler in its current form is not set up to re-enable the
> tooltip of the old root window and disable for the new root.

RWHVA may move between root windows. You are correct that ScopedDisabler doesn't
entirely deal with this situation, but at least it does not have the possibility
of disabling on one and then reenabling on the wrong root.

My assumption here is that if you've moving the window to a different root
you'll recreate the scoper. But worse case you don't, the code won't cause any
problems.

[commit-bot: I haz the power, 2014-01-07 16:53:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/109403008/50001

[commit-bot: I haz the power, 2014-01-07 19:24:10]

Change committed as 243350

[varunjain, 2014-01-08 15:43:08]

https://codereview.chromium.org/109403008/diff/50001/ui/aura/client/tooltip_c...
File ui/aura/client/tooltip_client.h (right):

https://codereview.chromium.org/109403008/diff/50001/ui/aura/client/tooltip_c...
ui/aura/client/tooltip_client.h:27: // Enables/Disables tooltips. This is
treated as a reference count. Consumers
On 2014/01/07 16:52:03, sky wrote:
> On 2014/01/07 05:07:06, varunjain wrote:
> > I am not sure how much the ScopedDisabler buys. I am not opposed to to it,
but
> I
> > would have just created a static void SetTooltipsEnabled(Window* window,
bool
> > enable), same as Set/GetTooltipText() below. That way, you do not need to
add
> an
> > observer.
> > I think I am not sure what sort of errors are you trying to protect against.
> Are
> > you worried that the root window might change between a disable and enable?
If
> > so, first, I do not see what situation that could happen in and second, even
> if
> > it happens, ScopedDisabler in its current form is not set up to re-enable
the
> > tooltip of the old root window and disable for the new root.
> 
> RWHVA may move between root windows. You are correct that ScopedDisabler
doesn't
> entirely deal with this situation, but at least it does not have the
possibility
> of disabling on one and then reenabling on the wrong root.

But in that case, the ScopedDisabler is even more incorrect. Consider for
example, two roots R1 and R2 with RWHV1 and RWHV2 respectively. Both use scoped
disabler to disable tooltips. Now both switch root windows so RWHV1 is in R2 and
RWHV2 is in R1. Now if say RWHV1 tries to enable tooltips by resetting scoped
disabler, it will enable R1's tooltips.

> 
> My assumption here is that if you've moving the window to a different root
> you'll recreate the scoper. But worse case you don't, the code won't cause any
> problems.

The code will cause the problem I described above (although arguably its a
degenerate case). Previous code would have just enabled the tooltip in the right
root window.

[sky, 2014-01-08 15:53:35]

On Wed, Jan 8, 2014 at 7:43 AM,  <varunjain@chromium.org> wrote:
>
>
https://codereview.chromium.org/109403008/diff/50001/ui/aura/client/tooltip_c...
> File ui/aura/client/tooltip_client.h (right):
>
>
https://codereview.chromium.org/109403008/diff/50001/ui/aura/client/tooltip_c...
> ui/aura/client/tooltip_client.h:27: // Enables/Disables tooltips. This
> is treated as a reference count. Consumers
> On 2014/01/07 16:52:03, sky wrote:
>>
>> On 2014/01/07 05:07:06, varunjain wrote:
>> > I am not sure how much the ScopedDisabler buys. I am not opposed to
>
> to it, but
>>
>> I
>> > would have just created a static void SetTooltipsEnabled(Window*
>
> window, bool
>>
>> > enable), same as Set/GetTooltipText() below. That way, you do not
>
> need to add
>>
>> an
>> > observer.
>> > I think I am not sure what sort of errors are you trying to protect
>
> against.
>>
>> Are
>> > you worried that the root window might change between a disable and
>
> enable? If
>>
>> > so, first, I do not see what situation that could happen in and
>
> second, even
>>
>> if
>> > it happens, ScopedDisabler in its current form is not set up to
>
> re-enable the
>>
>> > tooltip of the old root window and disable for the new root.
>
>
>> RWHVA may move between root windows. You are correct that
>
> ScopedDisabler doesn't
>>
>> entirely deal with this situation, but at least it does not have the
>
> possibility
>>
>> of disabling on one and then reenabling on the wrong root.
>
>
> But in that case, the ScopedDisabler is even more incorrect. Consider
> for example, two roots R1 and R2 with RWHV1 and RWHV2 respectively. Both
> use scoped disabler to disable tooltips. Now both switch root windows so
> RWHV1 is in R2 and RWHV2 is in R1. Now if say RWHV1 tries to enable
> tooltips by resetting scoped disabler, it will enable R1's tooltips.

That's the desired outcome though, right? Otherwise we've left
tooltips disabled on R1 and they'll never be reenabled.

  -Scott

>
>
>
>> My assumption here is that if you've moving the window to a different
>
> root
>>
>> you'll recreate the scoper. But worse case you don't, the code won't
>
> cause any
>>
>> problems.
>
>
> The code will cause the problem I described above (although arguably its
> a degenerate case). Previous code would have just enabled the tooltip in
> the right root window.
>
> https://codereview.chromium.org/109403008/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.