Remove unnecessary spin in ToolTipController

Remove unnecessary spin in ToolTipController

The motivation for the change is to reduce idle wakeups and therefore 
reduce idle power.

ToolTipController has a repeating 500ms timer that tries to update 
itself periodically, while most of the time there is no change at all.

This CL removes this spin but keeps the same functionality by 
1) upgrading OnMouseEvent() in TooltipController;
2) overriding OnWindowPropertyChanged() in TooltipController;
3) overriding OnCursorVisibilityChanged() in TooltipController.

After the change, tooltip is updated on demand via observer pattern,
rather than unnecessary periodic checking.

BUG=668198
Review-Url: https://codereview.chromium.org/2615993002
Cr-Commit-Position: refs/heads/master@{#443584}
Committed: https://chromium.googlesource.com/chromium/src/+/6b8312a4903fc6965a64a3835524f1a8f384f26b

[chengx, 2017-01-05 23:32:06]

Description was changed from

==========
Avoid unnecessary periodic checks of tooltip.
Merge branch 'master' of https://chromium.googlesource.com/chromium/src into
fixtooltip


Remove repete timer in tooltip controller.

BUG=668198
==========

to

==========
Remove unnecessary spin in ToolTipController

ToolTipController has a repeating 500ms timer that tries to update 
itself periodically even when it has no change at all.

Since ToolTipController has event handlers to update itself when events 
(e.g., mouse events) take place, this 500ms spin simply does nothing but
consumes more battery power.

This CL removes this spin.

BUG=668198
==========

[chengx, 2017-01-05 23:32:25]

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

[chengx, 2017-01-05 23:32:45]

Description was changed from

==========
Remove unnecessary spin in ToolTipController

ToolTipController has a repeating 500ms timer that tries to update 
itself periodically even when it has no change at all.

Since ToolTipController has event handlers to update itself when events 
(e.g., mouse events) take place, this 500ms spin simply does nothing but
consumes more battery power.

This CL removes this spin.

BUG=668198
==========

to

==========
Remove unnecessary spin in ToolTipController

ToolTipController has a repeating 500ms timer that tries to update 
itself periodically even when there is no change.

Since ToolTipController has event handlers to update itself when events 
(e.g., mouse events) take place, this 500ms spin simply does nothing but
consumes more battery power.

This CL removes this spin.

BUG=668198
==========

[chengx, 2017-01-05 23:33:07]

Patchset #1 (id:1) has been deleted

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

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-05 23:33:53]

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

[commit-bot: I haz the power, 2017-01-05 23:34: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-05 23:56:20]

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

[commit-bot: I haz the power, 2017-01-05 23:56:21]

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

[chengx, 2017-01-06 00:14:12]

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

[commit-bot: I haz the power, 2017-01-06 00:14:43]

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-06 00:55:40]

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

[commit-bot: I haz the power, 2017-01-06 00:56:19]

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-06 01:28:53]

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

[commit-bot: I haz the power, 2017-01-06 01:29:37]

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-06 01:30:20]

Patchset #3 (id:60001) has been deleted

[chengx, 2017-01-06 01:33:42]

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

[chengx, 2017-01-06 01:34:01]

Patchset #3 (id:80001) has been deleted

[commit-bot: I haz the power, 2017-01-06 01:34:48]

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-06 01:47:00]

Description was changed from

==========
Remove unnecessary spin in ToolTipController

ToolTipController has a repeating 500ms timer that tries to update 
itself periodically even when there is no change.

Since ToolTipController has event handlers to update itself when events 
(e.g., mouse events) take place, this 500ms spin simply does nothing but
consumes more battery power.

This CL removes this spin.

BUG=668198
==========

to

==========
Remove unnecessary spin in ToolTipController

ToolTipController has a repeating 500ms timer that tries to update 
itself periodically, while most of the time there is no change at all.

On the other hand, ToolTipController has event handlers to update itself 
when events (e.g., mouse events) take place. Tooltip becomes visible 
only when certain mouse events take place.

This CL removes this spin and make event handlers responsible for
triggering effective updates.

BUG=668198
==========

[chengx, 2017-01-06 01:47:29]

Description was changed from

==========
Remove unnecessary spin in ToolTipController

ToolTipController has a repeating 500ms timer that tries to update 
itself periodically, while most of the time there is no change at all.

On the other hand, ToolTipController has event handlers to update itself 
when events (e.g., mouse events) take place. Tooltip becomes visible 
only when certain mouse events take place.

This CL removes this spin and make event handlers responsible for
triggering effective updates.

BUG=668198
==========

to

==========
Remove unnecessary spin in ToolTipController

ToolTipController has a repeating 500ms timer that tries to update 
itself periodically, while most of the time there is no change at all.

On the other hand, ToolTipController has event handlers to update itself 
when events (e.g., mouse events) take place. Tooltip becomes visible 
only when certain mouse events take place.

This CL removes this spin and makes event handlers responsible for
triggering effective updates.

BUG=668198
==========

[chengx, 2017-01-06 01:47:47]

Description was changed from

==========
Remove unnecessary spin in ToolTipController

ToolTipController has a repeating 500ms timer that tries to update 
itself periodically, while most of the time there is no change at all.

On the other hand, ToolTipController has event handlers to update itself 
when events (e.g., mouse events) take place. Tooltip becomes visible 
only when certain mouse events take place.

This CL removes this spin and makes event handlers responsible for
triggering effective updates.

BUG=668198
==========

to

==========
Remove unnecessary spin in ToolTipController

ToolTipController has a repeating 500ms timer that tries to update 
itself periodically, while most of the time there is no change at all.

On the other hand, ToolTipController has event handlers to update itself 
when events (e.g., mouse events) take place. Tooltip becomes visible 
only when certain mouse events take place.

This CL removes this spin and makes event handlers responsible for
triggering effective tooltip updates.

BUG=668198
==========

[commit-bot: I haz the power, 2017-01-06 02:39:40]

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

[commit-bot: I haz the power, 2017-01-06 02:39:40]

Dry run: This issue passed the CQ dry run.

[chengx, 2017-01-06 02:45:31]

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

[chengx, 2017-01-06 02:45:32]

PTAL~

[brucedawson, 2017-01-06 20:37:28]

I'm not at all expert in this area, but I wonder if the repeating timer was to
update the tooltip when its contents change - i.e; perhaps when the name of a
tab changes. Have you checked to see whether that situation works before/after?

[brucedawson, 2017-01-06 20:45:23]

Maybe mention the motivation for the change which is to reduce idle wakeups and
therefore reduce idle power.

Seems reasonable as long as it handles the case where the tooltip text changes
spontaneously as well as it used to, but it will need owner review.

https://codereview.chromium.org/2615993002/diff/100001/ash/tooltips/tooltip_c...
File ash/tooltips/tooltip_controller_unittest.cc (right):

https://codereview.chromium.org/2615993002/diff/100001/ash/tooltips/tooltip_c...
ash/tooltips/tooltip_controller_unittest.cc:112: // Fire tooltip timer so
tooltip becomes visible.
Comment is no longer accurate.

https://codereview.chromium.org/2615993002/diff/100001/ui/views/corewm/toolti...
File ui/views/corewm/tooltip_controller.cc (right):

https://codereview.chromium.org/2615993002/diff/100001/ui/views/corewm/toolti...
ui/views/corewm/tooltip_controller.cc:201: tooltip_text_ !=
aura::client::GetTooltipText(tooltip_window_)))
I don't understand why this logic has changed. Can you explain?

I don't know the code well so maybe it is obvious to somebody who does know it.

[chengx, 2017-01-06 20:58:18]

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

[chengx, 2017-01-06 20:58:19]

sky@, PTAL~

For the repeating timer in the ToolTipController, I cannot think of a reason why
it is necessary. It may be necessary in some situations, just I am not aware of.


Thank you!

https://codereview.chromium.org/2615993002/diff/100001/ash/tooltips/tooltip_c...
File ash/tooltips/tooltip_controller_unittest.cc (right):

https://codereview.chromium.org/2615993002/diff/100001/ash/tooltips/tooltip_c...
ash/tooltips/tooltip_controller_unittest.cc:112: // Fire tooltip timer so
tooltip becomes visible.
On 2017/01/06 20:45:23, brucedawson wrote:
> Comment is no longer accurate.

Sure, I will update in the next patch.

https://codereview.chromium.org/2615993002/diff/100001/ui/views/corewm/toolti...
File ui/views/corewm/tooltip_controller.cc (right):

https://codereview.chromium.org/2615993002/diff/100001/ui/views/corewm/toolti...
ui/views/corewm/tooltip_controller.cc:201: tooltip_text_ !=
aura::client::GetTooltipText(tooltip_window_)))
On 2017/01/06 20:45:23, brucedawson wrote:
> I don't understand why this logic has changed. Can you explain?
> 
> I don't know the code well so maybe it is obvious to somebody who does know
it.

Previously, mouse event handlers update tooltip only when tooltip is visible.
Therefore, if we remove the repeating timer, tooltip won't be updated to visible
state no matter what. I changed the logic here so that when mouse is moved to
hover on a window where there is tooltip, the tooltip will become visible.

[chengx, 2017-01-06 21:26:21]

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

[commit-bot: I haz the power, 2017-01-06 21:26: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-06 21:28:30]

Description was changed from

==========
Remove unnecessary spin in ToolTipController

ToolTipController has a repeating 500ms timer that tries to update 
itself periodically, while most of the time there is no change at all.

On the other hand, ToolTipController has event handlers to update itself 
when events (e.g., mouse events) take place. Tooltip becomes visible 
only when certain mouse events take place.

This CL removes this spin and makes event handlers responsible for
triggering effective tooltip updates.

BUG=668198
==========

to

==========
Remove unnecessary spin in ToolTipController

The motivation for the change is to reduce idle wakeups and therefore 
reduce idle power.

ToolTipController has a repeating 500ms timer that tries to update 
itself periodically, while most of the time there is no change at all.

On the other hand, ToolTipController has event handlers to update itself 
when events (e.g., mouse events) take place. Tooltip becomes visible 
only when certain mouse events take place.

This CL removes this spin and makes event handlers responsible for
triggering effective tooltip updates.

BUG=668198
==========

[commit-bot: I haz the power, 2017-01-06 22:25:51]

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

[commit-bot: I haz the power, 2017-01-06 22:25:51]

Dry run: This issue passed the CQ dry run.

[chengx, 2017-01-06 23:08:31]

brucedawson@, comments updated.

https://codereview.chromium.org/2615993002/diff/100001/ash/tooltips/tooltip_c...
File ash/tooltips/tooltip_controller_unittest.cc (right):

https://codereview.chromium.org/2615993002/diff/100001/ash/tooltips/tooltip_c...
ash/tooltips/tooltip_controller_unittest.cc:112: // Fire tooltip timer so
tooltip becomes visible.
On 2017/01/06 20:58:19, chengx wrote:
> On 2017/01/06 20:45:23, brucedawson wrote:
> > Comment is no longer accurate.
> 
> Sure, I will update in the next patch. 

Comment updated.

[sky, 2017-01-06 23:36:29]

On 2017/01/06 20:58:19, chengx wrote:
> sky@, PTAL~
> 
> For the repeating timer in the ToolTipController, I cannot think of a reason
why
> it is necessary. It may be necessary in some situations, just I am not aware
of.

It's necessary to pick up changes to the tooltip text, and it looks like it
handles disabling tooltips during mouse lock. There may be more, but these are
the ones I noticed.

> 
> 
> Thank you!
> 
>
https://codereview.chromium.org/2615993002/diff/100001/ash/tooltips/tooltip_c...
> File ash/tooltips/tooltip_controller_unittest.cc (right):
> 
>
https://codereview.chromium.org/2615993002/diff/100001/ash/tooltips/tooltip_c...
> ash/tooltips/tooltip_controller_unittest.cc:112: // Fire tooltip timer so
> tooltip becomes visible.
> On 2017/01/06 20:45:23, brucedawson wrote:
> > Comment is no longer accurate.
> 
> Sure, I will update in the next patch.
> 
>
https://codereview.chromium.org/2615993002/diff/100001/ui/views/corewm/toolti...
> File ui/views/corewm/tooltip_controller.cc (right):
> 
>
https://codereview.chromium.org/2615993002/diff/100001/ui/views/corewm/toolti...
> ui/views/corewm/tooltip_controller.cc:201: tooltip_text_ !=
> aura::client::GetTooltipText(tooltip_window_)))
> On 2017/01/06 20:45:23, brucedawson wrote:
> > I don't understand why this logic has changed. Can you explain?
> > 
> > I don't know the code well so maybe it is obvious to somebody who does know
> it.
> 
> Previously, mouse event handlers update tooltip only when tooltip is visible.
> Therefore, if we remove the repeating timer, tooltip won't be updated to
visible
> state no matter what. I changed the logic here so that when mouse is moved to
> hover on a window where there is tooltip, the tooltip will become visible.

[sky, 2017-01-07 00:03:43]

On 2017/01/06 23:36:29, sky wrote:
> On 2017/01/06 20:58:19, chengx wrote:
> > sky@, PTAL~
> > 
> > For the repeating timer in the ToolTipController, I cannot think of a reason
> why
> > it is necessary. It may be necessary in some situations, just I am not aware
> of.
> 
> It's necessary to pick up changes to the tooltip text, and it looks like it
> handles disabling tooltips during mouse lock. There may be more, but these are
> the ones I noticed.
> 
> > 
> > 
> > Thank you!
> > 
> >
>
https://codereview.chromium.org/2615993002/diff/100001/ash/tooltips/tooltip_c...
> > File ash/tooltips/tooltip_controller_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/2615993002/diff/100001/ash/tooltips/tooltip_c...
> > ash/tooltips/tooltip_controller_unittest.cc:112: // Fire tooltip timer so
> > tooltip becomes visible.
> > On 2017/01/06 20:45:23, brucedawson wrote:
> > > Comment is no longer accurate.
> > 
> > Sure, I will update in the next patch.
> > 
> >
>
https://codereview.chromium.org/2615993002/diff/100001/ui/views/corewm/toolti...
> > File ui/views/corewm/tooltip_controller.cc (right):
> > 
> >
>
https://codereview.chromium.org/2615993002/diff/100001/ui/views/corewm/toolti...
> > ui/views/corewm/tooltip_controller.cc:201: tooltip_text_ !=
> > aura::client::GetTooltipText(tooltip_window_)))
> > On 2017/01/06 20:45:23, brucedawson wrote:
> > > I don't understand why this logic has changed. Can you explain?
> > > 
> > > I don't know the code well so maybe it is obvious to somebody who does
know
> > it.
> > 
> > Previously, mouse event handlers update tooltip only when tooltip is
visible.
> > Therefore, if we remove the repeating timer, tooltip won't be updated to
> visible
> > state no matter what. I changed the logic here so that when mouse is moved
to
> > hover on a window where there is tooltip, the tooltip will become visible.

I think you could handle the tooltip text/id changing by overriding
OnWindowPropertyChanged(). The mouse lock one could likely be handled directly
in SetTooltipsEnabled (assuming it isn't already).

[chengx, 2017-01-07 01:41:54]

> > It's necessary to pick up changes to the tooltip text, and it looks like it
> > handles disabling tooltips during mouse lock. There may be more, but these
are
> > the ones I noticed.
> > 
> I think you could handle the tooltip text/id changing by overriding
> OnWindowPropertyChanged(). The mouse lock one could likely be handled directly
> in SetTooltipsEnabled (assuming it isn't already).

Thanks sky@ for the feedback!

For "disable tooltips during mouse lock", I think SetTooltipsEnabled() already
handles
this. It calls UpdateTooltip(), which calls UpdateIfRequired() to hide the
tooltip.
No mouse event is needed for this.

For "pick up changes to the tooltip text", I did the following experiment. I
wrote a 
webpage where a tooltip will show up when the mouse hovers on some text. The
tooltip 
content is the current time, constantly updated via Javascript below. 

<script>
var myVar = setInterval(myTimer,1000);
function myTimer() {
	var d = new Date();
	document.getElementById("myspan").innerHTML = d.toLocaleTimeString();;
}
</script>

I loaded this webpage in the built chrome with the CL, hover the mouse on the
text, 
leave the mouse untouched. The tooltip is visible and updating itself. So I
think the
changes to the tooltip text is being picked, even without adding WindowProperty
observer. 

How do you think please?

[sky, 2017-01-09 16:14:56]

I think you're asking me to debug how it works. You'll have to
investigate that. Not all code paths necessarily go through the
renderer, and it's entirely possible the renderer is triggering
updating the tooltip in other ways.

  -Scott

On Fri, Jan 6, 2017 at 5:41 PM,  <chengx@chromium.org> wrote:
>> > It's necessary to pick up changes to the tooltip text, and it looks like
>> > it
>> > handles disabling tooltips during mouse lock. There may be more, but
>> > these
> are
>> > the ones I noticed.
>> >
>> I think you could handle the tooltip text/id changing by overriding
>> OnWindowPropertyChanged(). The mouse lock one could likely be handled
>> directly
>> in SetTooltipsEnabled (assuming it isn't already).
>
> Thanks sky@ for the feedback!
>
> For "disable tooltips during mouse lock", I think SetTooltipsEnabled()
> already
> handles
> this. It calls UpdateTooltip(), which calls UpdateIfRequired() to hide the
> tooltip.
> No mouse event is needed for this.
>
> For "pick up changes to the tooltip text", I did the following experiment. I
> wrote a
> webpage where a tooltip will show up when the mouse hovers on some text. The
> tooltip
> content is the current time, constantly updated via Javascript below.
>
> <script>
> var myVar = setInterval(myTimer,1000);
> function myTimer() {
> var d = new Date();
> document.getElementById("myspan").innerHTML = d.toLocaleTimeString();;
> }
> </script>
>
> I loaded this webpage in the built chrome with the CL, hover the mouse on
> the
> text,
> leave the mouse untouched. The tooltip is visible and updating itself. So I
> think the
> changes to the tooltip text is being picked, even without adding
> WindowProperty
> observer.
>
> How do you think please?
>
>
>
>
> https://codereview.chromium.org/2615993002/

-- 
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-09 19:30:38]

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

[commit-bot: I haz the power, 2017-01-09 19:32:41]

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-09 20:29:10]

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

[commit-bot: I haz the power, 2017-01-09 20:29:11]

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

[chengx, 2017-01-09 21:25:08]

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

[commit-bot: I haz the power, 2017-01-09 21:25:58]

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-09 22:31:39]

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

[commit-bot: I haz the power, 2017-01-09 22:31:40]

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

[chengx, 2017-01-09 23:15:52]

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

[commit-bot: I haz the power, 2017-01-09 23:16:36]

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

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

[commit-bot: I haz the power, 2017-01-10 00:38:01]

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-10 01:10:40]

Patchset #6 (id:160001) has been deleted

[commit-bot: I haz the power, 2017-01-10 04:33:40]

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

[commit-bot: I haz the power, 2017-01-10 04:33:41]

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

[chengx, 2017-01-11 01:04:50]

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

[commit-bot: I haz the power, 2017-01-11 01:05:17]

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-11 01:50:32]

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

[commit-bot: I haz the power, 2017-01-11 01:50:33]

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

[chengx, 2017-01-11 02:46:07]

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

[commit-bot: I haz the power, 2017-01-11 02:46:51]

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-11 03:31:35]

Patchset #7 (id:200001) has been deleted

[chengx, 2017-01-11 03:31:51]

Patchset #6 (id:180001) has been deleted

[commit-bot: I haz the power, 2017-01-11 04:48:38]

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

[commit-bot: I haz the power, 2017-01-11 04:48:39]

Dry run: This issue passed the CQ dry run.

[chengx, 2017-01-11 18:33:19]

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

[commit-bot: I haz the power, 2017-01-11 18:33:51]

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-11 19:05:46]

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

[commit-bot: I haz the power, 2017-01-11 19:06:22]

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-11 20:09:30]

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

[commit-bot: I haz the power, 2017-01-11 20:09:31]

Dry run: This issue passed the CQ dry run.

[chengx, 2017-01-11 20:29:45]

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

[commit-bot: I haz the power, 2017-01-11 20:30:11]

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-11 20:34:12]

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

[commit-bot: I haz the power, 2017-01-11 20:34:38]

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-11 21:10:38]

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

[commit-bot: I haz the power, 2017-01-11 21:11:24]

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-11 21:12:36]

Patchset #11 (id:320001) has been deleted

[commit-bot: I haz the power, 2017-01-11 21:14:19]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[chengx, 2017-01-11 21:19:51]

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

[commit-bot: I haz the power, 2017-01-11 21:20:08]

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-11 21:23:52]

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

[commit-bot: I haz the power, 2017-01-11 21:24: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-11 21:45:16]

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

[commit-bot: I haz the power, 2017-01-11 21:45:57]

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-11 22:06:59]

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

[commit-bot: I haz the power, 2017-01-11 22:08:52]

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-11 22:30:10]

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

[commit-bot: I haz the power, 2017-01-11 22:31:01]

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-11 22:59:09]

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

[commit-bot: I haz the power, 2017-01-11 23:00:12]

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

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

[commit-bot: I haz the power, 2017-01-11 23:04:32]

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-11 23:05:07]

Patchset #15 (id:420001) has been deleted

[chengx, 2017-01-11 23:05:37]

Patchset #15 (id:440001) has been deleted

[chengx, 2017-01-11 23:05:44]

Patchset #14 (id:400001) has been deleted

[chengx, 2017-01-11 23:06:40]

Patchset #13 (id:380001) has been deleted

[chengx, 2017-01-11 23:07:10]

Patchset #12 (id:360001) has been deleted

[chengx, 2017-01-11 23:07:17]

Patchset #11 (id:340001) has been deleted

[chengx, 2017-01-11 23:07:40]

Patchset #10 (id:300001) has been deleted

[chengx, 2017-01-11 23:07:47]

Patchset #9 (id:280001) has been deleted

[chengx, 2017-01-11 23:07:54]

Patchset #8 (id:260001) has been deleted

[chengx, 2017-01-11 23:08:06]

Patchset #7 (id:240001) has been deleted

[chengx, 2017-01-11 23:15:50]

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

[commit-bot: I haz the power, 2017-01-11 23:16:13]

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-11 23:39:51]

Patchset #5 (id:140001) has been deleted

[chengx, 2017-01-11 23:49:07]

Patchset #7 (id:480001) has been deleted

[chengx, 2017-01-12 00:02:06]

sky@, PTAL~

I have investigated all the related code and updated this CL. 

I overrode OnCursorVisibilityChanged() and OnWindowPropertyChanged() in
TooltipController.cc, which triggers tooltip update automatically when there is
cursor visibility change, mouse enable/disable change (which also changes the
cursor visibility), tooltip id change or tooltip content change. I believe these
changes cover what the spin is regularly checking. 

Unit tests are also updated to reflect the effectiveness of the two observer
functions.

Thanks!

[commit-bot: I haz the power, 2017-01-12 00:11:41]

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

[commit-bot: I haz the power, 2017-01-12 00:11:43]

Dry run: This issue passed the CQ dry run.

[chengx, 2017-01-12 04:29:40]

Description was changed from

==========
Remove unnecessary spin in ToolTipController

The motivation for the change is to reduce idle wakeups and therefore 
reduce idle power.

ToolTipController has a repeating 500ms timer that tries to update 
itself periodically, while most of the time there is no change at all.

On the other hand, ToolTipController has event handlers to update itself 
when events (e.g., mouse events) take place. Tooltip becomes visible 
only when certain mouse events take place.

This CL removes this spin and makes event handlers responsible for
triggering effective tooltip updates.

BUG=668198
==========

to

==========
Remove unnecessary spin in ToolTipController

The motivation for the change is to reduce idle wakeups and therefore 
reduce idle power.

ToolTipController has a repeating 500ms timer that tries to update 
itself periodically, while most of the time there is no change at all.

This CL removes this spin but keeps the same functionality by 
1) upgrading mouse event handler in TooltipController;
2) overriding OnWindowPropertyChanged() in TooltipController;
3) overriding OnCursorVisibilityChanged() in TooltipController.

After the change, tooltip is updated on demand via observer pattern,
rather than unnecessary periodic checking.

BUG=668198
==========

[chengx, 2017-01-12 04:30:58]

Description was changed from

==========
Remove unnecessary spin in ToolTipController

The motivation for the change is to reduce idle wakeups and therefore 
reduce idle power.

ToolTipController has a repeating 500ms timer that tries to update 
itself periodically, while most of the time there is no change at all.

This CL removes this spin but keeps the same functionality by 
1) upgrading mouse event handler in TooltipController;
2) overriding OnWindowPropertyChanged() in TooltipController;
3) overriding OnCursorVisibilityChanged() in TooltipController.

After the change, tooltip is updated on demand via observer pattern,
rather than unnecessary periodic checking.

BUG=668198
==========

to

==========
Remove unnecessary spin in ToolTipController

The motivation for the change is to reduce idle wakeups and therefore 
reduce idle power.

ToolTipController has a repeating 500ms timer that tries to update 
itself periodically, while most of the time there is no change at all.

This CL removes this spin but keeps the same functionality by 
1) upgrading OnMouseEvent() in TooltipController;
2) overriding OnWindowPropertyChanged() in TooltipController;
3) overriding OnCursorVisibilityChanged() in TooltipController.

After the change, tooltip is updated on demand via observer pattern,
rather than unnecessary periodic checking.

BUG=668198
==========

[sky, 2017-01-12 16:18:29]

As you asked, can you outline how the renderer was working in the
past? What was the code path that lead to updating the tooltip?

  -Scott

On Wed, Jan 11, 2017 at 4:02 PM,  <chengx@chromium.org> wrote:
> sky@, PTAL~
>
> I have investigated all the related code and updated this CL.
>
> I overrode OnCursorVisibilityChanged() and OnWindowPropertyChanged() in
> TooltipController.cc, which triggers tooltip update automatically when there
> is
> cursor visibility change, mouse enable/disable change (which also changes
> the
> cursor visibility), tooltip id change or tooltip content change. I believe
> these
> changes cover what the spin is regularly checking.
>
> Unit tests are also updated to reflect the effectiveness of the two observer
> functions.
>
> Thanks!
>
> https://codereview.chromium.org/2615993002/

-- 
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-12 19:07:07]

On 2017/01/12 16:18:29, sky wrote:
> As you asked, can you outline how the renderer was working in the
> past? What was the code path that lead to updating the tooltip?
> 
>   -Scott


Sure. Here is the code path that lead to updating the tooltip. It is pretty
straightforward. For your question "outline how the renderer was working in the
past", I am sorry I fail to understand it.  

When chrome is launched, a TooltipController object is constructed
(tooltip_controller.cc, line 127). The constructor starts a RepeatingTimer,
which calls TooltipController::TooltipTimerFired every 500ms.
TooltipTimerFired() basically calls UpdateIfRequired() to update the tooltip if
it is needed. 

In UpdateIfRequired(), there are a few conditions being checked, including 
(1) TooltipController object's own flag: tooltips_enabled_;
(2) Mouse events;
(3) Cursor visibility state;
(4) Tooltip id's change;
(5) Tooltip text's change. 

Note that for (1), it is handled by TooltipController::SetTooltipsEnabled
directly, which calls UpdateTooltip(). So we don't need to worry about that. 

The periodic UpdateIfRequired() check simply tries to hide the tooltip (no
matter what its current state is) and exits. This is unnecessary.

After my change, the periodic check is gone. Other conditions are checked using
observer pattern as below:
For (2), I have updated TooltipController::OnMouseEvent to trigger
UpdateIfRequired() for certain mouse events.
For (3), I made TooltipController inherited CursorClientObserver and overrode
:OnCursorVisibilityChanged() to handle this.
For (4), I overrode OnWindowPropertyChanged() to trigger the update.
kTooltipIdKey and kTooltipTextKey are used to ignore other window property
changes.

[sky, 2017-01-12 20:15:18]

https://codereview.chromium.org/2615993002/diff/460001/ui/views/corewm/toolti...
File ui/views/corewm/tooltip_controller.cc (right):

https://codereview.chromium.org/2615993002/diff/460001/ui/views/corewm/toolti...
ui/views/corewm/tooltip_controller.cc:199: (tooltip_window_ &&
How come you are adding this condition?

https://codereview.chromium.org/2615993002/diff/460001/ui/views/corewm/toolti...
ui/views/corewm/tooltip_controller.cc:254: window && tooltip_window_ == window
&&
Why do you need to check window and tooltip_window_ == window here? The observer
is only added for tooltip_window_. I could see a DCHECK if you want.

https://codereview.chromium.org/2615993002/diff/460001/ui/wm/public/tooltip_c...
File ui/wm/public/tooltip_client.cc (right):

https://codereview.chromium.org/2615993002/diff/460001/ui/wm/public/tooltip_c...
ui/wm/public/tooltip_client.cc:9:
DECLARE_WINDOW_PROPERTY_TYPE(aura::client::TooltipClient*)
I believe you'll want DECLARE_EXPORTED_WINDOW_PROPERTY_TYPE.

https://codereview.chromium.org/2615993002/diff/460001/ui/wm/public/tooltip_c...
File ui/wm/public/tooltip_client.h (right):

https://codereview.chromium.org/2615993002/diff/460001/ui/wm/public/tooltip_c...
ui/wm/public/tooltip_client.h:9: #include "ui/aura/window.h"
I believe you only need include window_property.h.

[chengx, 2017-01-12 23:52:32]

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

[commit-bot: I haz the power, 2017-01-12 23:53:58]

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-13 00:00:31]

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

[commit-bot: I haz the power, 2017-01-13 00:03:10]

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-13 00:58:04]

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

[chengx, 2017-01-13 00:58:30]

Patchset #7 (id:500001) has been deleted

[commit-bot: I haz the power, 2017-01-13 00:58:42]

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-13 01:17:51]

Hi sky@, I have addressed your comments in the new patch. PTAL~

Thanks!

https://codereview.chromium.org/2615993002/diff/460001/ui/views/corewm/toolti...
File ui/views/corewm/tooltip_controller.cc (right):

https://codereview.chromium.org/2615993002/diff/460001/ui/views/corewm/toolti...
ui/views/corewm/tooltip_controller.cc:199: (tooltip_window_ &&
On 2017/01/12 20:15:18, sky wrote:
> How come you are adding this condition? 

Because before this change, OnMouseEvent() only triggers UpdateIfRequired() when
tooltip_->IsVisible() is true. It was okay because there was a repeating check.
However, after removing the repeating check, we need to rely on OnMouseEvent()
further when tooltip is invisible. TooltipControllerTest2.* in
ui/views/corewm/tooltip_controller_unittest.cc reflect this logic.

https://codereview.chromium.org/2615993002/diff/460001/ui/views/corewm/toolti...
ui/views/corewm/tooltip_controller.cc:254: window && tooltip_window_ == window
&&
On 2017/01/12 20:15:18, sky wrote:
> Why do you need to check window and tooltip_window_ == window here? The
observer
> is only added for tooltip_window_. I could see a DCHECK if you want.

You are right. I have removed those two checks.

https://codereview.chromium.org/2615993002/diff/460001/ui/wm/public/tooltip_c...
File ui/wm/public/tooltip_client.cc (right):

https://codereview.chromium.org/2615993002/diff/460001/ui/wm/public/tooltip_c...
ui/wm/public/tooltip_client.cc:9:
DECLARE_WINDOW_PROPERTY_TYPE(aura::client::TooltipClient*)
On 2017/01/12 20:15:18, sky wrote:
> I believe you'll want DECLARE_EXPORTED_WINDOW_PROPERTY_TYPE.

Fixed.

https://codereview.chromium.org/2615993002/diff/460001/ui/wm/public/tooltip_c...
File ui/wm/public/tooltip_client.h (right):

https://codereview.chromium.org/2615993002/diff/460001/ui/wm/public/tooltip_c...
ui/wm/public/tooltip_client.h:9: #include "ui/aura/window.h"
On 2017/01/12 20:15:18, sky wrote:
> I believe you only need include window_property.h.

Yes, you are right. However, presubmit upload checks (when doing git cl upload)
gave me error saying "Header files should not include
ui/aura/window_property.h". I did a search in the chrome code base, it seems
that this header file is only included in .cc files.

That is why I had to keep this header file in tooltip_client.cc file.

[commit-bot: I haz the power, 2017-01-13 01:18:59]

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

[commit-bot: I haz the power, 2017-01-13 01:19:00]

Dry run: This issue passed the CQ dry run.

[sky, 2017-01-13 15:20:48]

LGTM - nice fix!

[chengx, 2017-01-13 16:50:17]

The CQ bit was checked by chengx@chromium.org

[commit-bot: I haz the power, 2017-01-13 16:51:01]

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-13 16:56:34]

CQ is committing da patch.

Bot data: {"patchset_id": 520001, "attempt_start_ts": 1484326217505280,
"parent_rev": "1e4883b93bc28febedbf8f3927c88daebf0acbc6", "commit_rev":
"6b8312a4903fc6965a64a3835524f1a8f384f26b"}

[commit-bot: I haz the power, 2017-01-13 16:57:21]

Description was changed from

==========
Remove unnecessary spin in ToolTipController

The motivation for the change is to reduce idle wakeups and therefore 
reduce idle power.

ToolTipController has a repeating 500ms timer that tries to update 
itself periodically, while most of the time there is no change at all.

This CL removes this spin but keeps the same functionality by 
1) upgrading OnMouseEvent() in TooltipController;
2) overriding OnWindowPropertyChanged() in TooltipController;
3) overriding OnCursorVisibilityChanged() in TooltipController.

After the change, tooltip is updated on demand via observer pattern,
rather than unnecessary periodic checking.

BUG=668198
==========

to

==========
Remove unnecessary spin in ToolTipController

The motivation for the change is to reduce idle wakeups and therefore 
reduce idle power.

ToolTipController has a repeating 500ms timer that tries to update 
itself periodically, while most of the time there is no change at all.

This CL removes this spin but keeps the same functionality by 
1) upgrading OnMouseEvent() in TooltipController;
2) overriding OnWindowPropertyChanged() in TooltipController;
3) overriding OnCursorVisibilityChanged() in TooltipController.

After the change, tooltip is updated on demand via observer pattern,
rather than unnecessary periodic checking.

BUG=668198

Review-Url: https://codereview.chromium.org/2615993002
Cr-Commit-Position: refs/heads/master@{#443584}
Committed:
https://chromium.googlesource.com/chromium/src/+/6b8312a4903fc6965a64a3835524...
==========

[commit-bot: I haz the power, 2017-01-13 16:57:23]

Committed patchset #7 (id:520001) as
https://chromium.googlesource.com/chromium/src/+/6b8312a4903fc6965a64a3835524...

[chengx, 2017-01-18 22:50:40]

A revert of this CL (patchset #7 id:520001) has been created in
https://codereview.chromium.org/2643973002/ by chengx@chromium.org.

The reason for reverting is: Caused the regression 682141..