Fix regression: empty tooltip appears in Linux and Chrome OS

Fix regression: empty tooltip appears in Linux and Chrome OS

After landing the tooltip fix as in crrev.com/2652833002, UI regression 
was captured in Linux and Chrome OS where empty tooltip shows up. It 
seems Windows has not have this issue.

This CL fixes this regression.

BUG=685078, 685408
Review-Url: https://codereview.chromium.org/2652353003
Cr-Commit-Position: refs/heads/master@{#446486}
Committed: https://chromium.googlesource.com/chromium/src/+/6335c597c43363e29d6757d60aa34a895aa5398a

[chengx, 2017-01-25 21:54:13]

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

[commit-bot: I haz the power, 2017-01-25 21:54:49]

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

[chengx, 2017-01-25 21:59:28]

Description was changed from

==========
Fix tooltip regression by not showing blank tooltips.

BUG=685078
==========

to

==========
Fix regression: Weird tooltips are seen on hovering on any links in Ubuntu 14.04

After landing the tooltip fix crrev.com/2652833002, UI regression was
captured in Linux where blank tooltips show up (issue 685078). This 
regression seems to be Linux only as reported.

This CL fixes this regression.

BUG=685078
==========

[chengx, 2017-01-25 22:01:13]

Description was changed from

==========
Fix regression: Weird tooltips are seen on hovering on any links in Ubuntu 14.04

After landing the tooltip fix crrev.com/2652833002, UI regression was
captured in Linux where blank tooltips show up (issue 685078). This 
regression seems to be Linux only as reported.

This CL fixes this regression.

BUG=685078
==========

to

==========
Fix regression: Weird tooltips are seen on hovering on any links in 
Ubuntu 14.04

After landing the tooltip fix crrev.com/2652833002, UI regression was
captured in Linux where blank tooltips show up (issue 685078). This 
regression seems to be Linux only as reported.

This CL fixes this regression.

BUG=685078
==========

[chengx, 2017-01-25 22:01:48]

Description was changed from

==========
Fix regression: Weird tooltips are seen on hovering on any links in 
Ubuntu 14.04

After landing the tooltip fix crrev.com/2652833002, UI regression was
captured in Linux where blank tooltips show up (issue 685078). This 
regression seems to be Linux only as reported.

This CL fixes this regression.

BUG=685078
==========

to

==========
Fix regression: Weird tooltips are seen on hovering on any links in 
Ubuntu 14.04

After landing the tooltip fix as in crrev.com/2652833002, UI regression 
was captured in Linux where blank tooltips show up (issue 685078). This 
regression seems to be Linux only as reported.

This CL fixes this regression.

BUG=685078
==========

[chengx, 2017-01-25 22:31:26]

Description was changed from

==========
Fix regression: Weird tooltips are seen on hovering on any links in 
Ubuntu 14.04

After landing the tooltip fix as in crrev.com/2652833002, UI regression 
was captured in Linux where blank tooltips show up (issue 685078). This 
regression seems to be Linux only as reported.

This CL fixes this regression.

BUG=685078
==========

to

==========
Fix regression: weird tooltips are seen on hovering on any links in 
Ubuntu 14.04

After landing the tooltip fix as in crrev.com/2652833002, UI regression 
was captured in Linux where blank tooltips show up (issue 685078). This 
regression seems to be Linux only as reported.

This CL fixes this regression.

BUG=685078
==========

[commit-bot: I haz the power, 2017-01-25 23:01:41]

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

[commit-bot: I haz the power, 2017-01-25 23:01:42]

Dry run: This issue passed the CQ dry run.

[chengx, 2017-01-25 23:03:41]

Description was changed from

==========
Fix regression: weird tooltips are seen on hovering on any links in 
Ubuntu 14.04

After landing the tooltip fix as in crrev.com/2652833002, UI regression 
was captured in Linux where blank tooltips show up (issue 685078). This 
regression seems to be Linux only as reported.

This CL fixes this regression.

BUG=685078
==========

to

==========
Fix regression: weird tooltips are seen on hovering on any links in 
Ubuntu 14.04

After landing the tooltip fix as in crrev.com/2652833002, UI regression 
was captured in Linux where blank tooltips show up (issue 685078). This 
regression seems to be Linux only as reported.

This CL fixes this regression.

BUG=685078, 685408
==========

[chengx, 2017-01-25 23:10:33]

Description was changed from

==========
Fix regression: weird tooltips are seen on hovering on any links in 
Ubuntu 14.04

After landing the tooltip fix as in crrev.com/2652833002, UI regression 
was captured in Linux where blank tooltips show up (issue 685078). This 
regression seems to be Linux only as reported.

This CL fixes this regression.

BUG=685078, 685408
==========

to

==========
Fix regression: empty tooltip appears in Linux and Chrome OS

After landing the tooltip fix as in crrev.com/2652833002, UI regression 
was captured in Linux and Chrome OS where empty tooltip shows up. It 
seems Windows has not have this issue.

This CL fixes this regression.

BUG=685078, 685408
==========

[chengx, 2017-01-25 23:13:57]

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

[chengx, 2017-01-25 23:13:57]

Hi sky@, sorry to bother you again. The CL about tooltip I checked-in yesterday
caused UI regression in Linux and Chrome OS, where empty tooltip appears which
is unexpected. This doesn't take place (at least visually) in Windows, so I
failed to capture it before landing it.

This small CL fixes this regression. PTAL~ 

Thanks!

[sky, 2017-01-25 23:41:51]

Can you add test coverage of this?

[sky, 2017-01-25 23:43:21]

https://codereview.chromium.org/2652353003/diff/1/ui/views/corewm/tooltip_con...
File ui/views/corewm/tooltip_controller.cc (right):

https://codereview.chromium.org/2652353003/diff/1/ui/views/corewm/tooltip_con...
ui/views/corewm/tooltip_controller.cc:330: if (!tooltip_window_ ||
tooltip_text_whitespace_trimmed_.empty())
Also, how do we end up here with an empty tooltip?

[chengx, 2017-01-26 01:10:57]

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

[commit-bot: I haz the power, 2017-01-26 01:11:53]

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

[chengx, 2017-01-26 01:16:24]

Unit test added. PTAL~

https://codereview.chromium.org/2652353003/diff/1/ui/views/corewm/tooltip_con...
File ui/views/corewm/tooltip_controller.cc (right):

https://codereview.chromium.org/2652353003/diff/1/ui/views/corewm/tooltip_con...
ui/views/corewm/tooltip_controller.cc:330: if (!tooltip_window_ ||
tooltip_text_whitespace_trimmed_.empty())
On 2017/01/25 23:43:21, sky wrote:
> Also, how do we end up here with an empty tooltip?

Because this function is called from UpdateIfRequired() above but with a delay
of 500 ms. In UpdateIfRequired(), tooltip_text_whitespace_trimmed_ is checked
first. If the string is empty, then ShowTooltip() won't be called. However,
since the ShowTooltip() function call is delayed with 500 ms,
tooltip_text_whitespace_trimmed_ can be set to empty by other event handlers
during that period. So we need to double check if string is empty here.

[commit-bot: I haz the power, 2017-01-26 02:55:16]

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

[commit-bot: I haz the power, 2017-01-26 02:55:17]

Dry run: This issue passed the CQ dry run.

[sky, 2017-01-26 04:15:07]

Once the text is set empty shouldn't we cancel the timer?

On Wed, Jan 25, 2017 at 5:16 PM,  <chengx@chromium.org> wrote:
> Unit test added. PTAL~
>
>
>
https://codereview.chromium.org/2652353003/diff/1/ui/views/corewm/tooltip_con...
> File ui/views/corewm/tooltip_controller.cc (right):
>
>
https://codereview.chromium.org/2652353003/diff/1/ui/views/corewm/tooltip_con...
> ui/views/corewm/tooltip_controller.cc:330: if (!tooltip_window_ ||
> tooltip_text_whitespace_trimmed_.empty())
> On 2017/01/25 23:43:21, sky wrote:
>> Also, how do we end up here with an empty tooltip?
>
> Because this function is called from UpdateIfRequired() above but with a
> delay of 500 ms. In UpdateIfRequired(), tooltip_text_whitespace_trimmed_
> is checked first. If the string is empty, then ShowTooltip() won't be
> called. However, since the ShowTooltip() function call is delayed with
> 500 ms, tooltip_text_whitespace_trimmed_ can be set to empty by other
> event handlers during that period. So we need to double check if string
> is empty here.
>
> https://codereview.chromium.org/2652353003/

-- 
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.

[chengx, 2017-01-26 05:35:12]

> Once the text is set empty shouldn't we cancel the timer?

I tried to remove the empty check of tooltip_text_whitespace_trimmed_ in
ShowTooltip(), but cancel the timer (i.e., timer.Reset()) after the empty check
of tooltip_text_whitespace_trimmed_ in UpdateIfRequired() (line 309). However,
this doesn't fix the regression as I tested in Linux. Possible explanation is
that UpdateIfRequired() can return earlier than the empty check in line 309,
therefore the timer won't be cancelled and empty tooltip still shows up.

So the safest or maybe the only thing I can do is to do the empty check of
tooltip_text_whitespace_trimmed_ in ShowTooltip(), as in this CL. If I keep this
check, then cancelling the timer after the empty check in UpdateIfRequired()
won't be critical, but can still save some computational efforts. 

I have uploaded another patch where the timer the cancelled when the text is set
empty. As discussed above, I kept the empty check of
tooltip_text_whitespace_trimmed_ in ShowTooltip().

[chengx, 2017-01-26 05:35:24]

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

[commit-bot: I haz the power, 2017-01-26 05:35:50]

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, 2017-01-26 06:25:44]

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

[commit-bot: I haz the power, 2017-01-26 06:25:45]

Dry run: This issue passed the CQ dry run.

[sky, 2017-01-26 18:25:35]

If you can reproduce the regression can't you determine why
ShowTooltip() is being called when we don't expect it? It seems like
the empty check there shouldn't be necessary, so I would like to
understand how we end up in ShowTooltip with an empty tooltip.

  -Scott

On Wed, Jan 25, 2017 at 9:35 PM,  <chengx@chromium.org> wrote:
>> Once the text is set empty shouldn't we cancel the timer?
>
> I tried to remove the empty check of tooltip_text_whitespace_trimmed_ in
> ShowTooltip(), but cancel the timer (i.e., timer.Reset()) after the empty
> check
> of tooltip_text_whitespace_trimmed_ in UpdateIfRequired() (line 309).
> However,
> this doesn't fix the regression as I tested in Linux. Possible explanation
> is
> that UpdateIfRequired() can return earlier than the empty check in line 309,
> therefore the timer won't be cancelled and empty tooltip still shows up.
>
> So the safest or maybe the only thing I can do is to do the empty check of
> tooltip_text_whitespace_trimmed_ in ShowTooltip(), as in this CL. If I keep
> this
> check, then cancelling the timer after the empty check in UpdateIfRequired()
> won't be critical, but can still save some computational efforts.
>
> I have uploaded another patch where the timer the cancelled when the text is
> set
> empty. As discussed above, I kept the empty check of
> tooltip_text_whitespace_trimmed_ in ShowTooltip().
>
>
> https://codereview.chromium.org/2652353003/

-- 
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.

[sky, 2017-01-26 18:25:37]

https://codereview.chromium.org/2652353003/diff/40001/ui/views/corewm/tooltip...
File ui/views/corewm/tooltip_controller.cc (right):

https://codereview.chromium.org/2652353003/diff/40001/ui/views/corewm/tooltip...
ui/views/corewm/tooltip_controller.cc:311: if (tooltip_defer_timer_.IsRunning())
{
no {}

[chengx, 2017-01-26 20:02:25]

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

[commit-bot: I haz the power, 2017-01-26 20:03:28]

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, 2017-01-26 20:51:21]

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

[commit-bot: I haz the power, 2017-01-26 20:51:22]

Dry run: This issue passed the CQ dry run.

[chengx, 2017-01-26 21:04:03]

On 2017/01/26 18:25:35, sky wrote:
> If you can reproduce the regression can't you determine why
> ShowTooltip() is being called when we don't expect it? It seems like
> the empty check there shouldn't be necessary, so I would like to
> understand how we end up in ShowTooltip with an empty tooltip.
> 
>   -Scott

Hi Scott, you are right. The empty check is unnecessary. It turns out that I
used the wrong Timer API. Timer.Reset(), which I used, simply resets the delay
time and then calls ShowTooltip(). That is why it didn't work. Timer.Stop()
should be used here. Also, I don't think I need to check Timer.IsRunning()
before stopping it according to the function implementation. In short, a single
call of Timer.Stop() should be enough here.

I have uploaded another patch where the accurate Timer API is used and the empty
check is removed. PTAL~

Thanks!

https://codereview.chromium.org/2652353003/diff/40001/ui/views/corewm/tooltip...
File ui/views/corewm/tooltip_controller.cc (right):

https://codereview.chromium.org/2652353003/diff/40001/ui/views/corewm/tooltip...
ui/views/corewm/tooltip_controller.cc:311: if (tooltip_defer_timer_.IsRunning())
{
On 2017/01/26 18:25:37, sky wrote:
> no {}

Done.

[chengx, 2017-01-26 21:07:13]

Verified in Linux and it works (the regression is gone).

[sky, 2017-01-26 23:01:46]

LGTM - sorry for not realizing this earlier.

[chengx, 2017-01-26 23:03:27]

The CQ bit was checked by chengx@chromium.org

[commit-bot: I haz the power, 2017-01-26 23:04:19]

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, 2017-01-26 23:09:58]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485471807984640,
"parent_rev": "9bcdced388956bfc8c137cfd20c830d30748946d", "commit_rev":
"6335c597c43363e29d6757d60aa34a895aa5398a"}

[commit-bot: I haz the power, 2017-01-26 23:10:41]

Description was changed from

==========
Fix regression: empty tooltip appears in Linux and Chrome OS

After landing the tooltip fix as in crrev.com/2652833002, UI regression 
was captured in Linux and Chrome OS where empty tooltip shows up. It 
seems Windows has not have this issue.

This CL fixes this regression.

BUG=685078, 685408
==========

to

==========
Fix regression: empty tooltip appears in Linux and Chrome OS

After landing the tooltip fix as in crrev.com/2652833002, UI regression 
was captured in Linux and Chrome OS where empty tooltip shows up. It 
seems Windows has not have this issue.

This CL fixes this regression.

BUG=685078, 685408

Review-Url: https://codereview.chromium.org/2652353003
Cr-Commit-Position: refs/heads/master@{#446486}
Committed:
https://chromium.googlesource.com/chromium/src/+/6335c597c43363e29d6757d60aa3...
==========

[commit-bot: I haz the power, 2017-01-26 23:10:42]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/6335c597c43363e29d6757d60aa3...