Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2652833002:
Remove infinite spin, make tooltip delay consistent (Closed)
DescriptionRemove infinite spin, make tooltip delay consistent
The main motivation for the change is to reduce idle wakeups and
therefore reduce idle power. Another motivation is to make the
UI better, as currently tooltip shows up with random delay time.
ToolTipController has a repeating timer firing 4 times per second
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.
On UI side, current production version hides tooltip immediately,
but shows it up with a delay of a random few hundreds of ms. This
is because of the RepeatingTimer implementation. One one hand, the
delayed appearance of tooltip is preferred. On the other hand, the
random delay time from instant to a few hundreds of ms doesn't look
good. We improve this by introducing a timer to delay the tooltip's
appearance by 500 ms. This makes the intended delay consistent for
all tooltips.
BUG=668198, 684877
Review-Url: https://codereview.chromium.org/2615993002
Cr-Commit-Position: refs/heads/master@{#443584}
Committed: https://chromium.googlesource.com/chromium/src/+/6b8312a4903fc6965a64a3835524f1a8f384f26b
patch from issue 2615993002 at patchset 520001 (http://crrev.com/2615993002#ps520001)
Review-Url: https://codereview.chromium.org/2652833002
Cr-Commit-Position: refs/heads/master@{#445902}
Committed: https://chromium.googlesource.com/chromium/src/+/122326d5ae2407e11a8b7353fd60dcffba5066fa
Patch Set 1 #Patch Set 2 : Delay tooltip show up by 500ms to make it consistent with production version. #
Total comments: 6
Patch Set 3 : Fix nits. #
Messages
Total messages: 55 (49 generated)
Description was changed from ========== Patch 2615993002 - Remove unnecessary spin in ToolTipController Merge branch 'master' of https://chromium.googlesource.com/chromium/src into fixtooltip2 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... patch from issue 2615993002 at patchset 520001 (http://crrev.com/2615993002#ps520001) ========== 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... patch from issue 2615993002 at patchset 520001 (http://crrev.com/2615993002#ps520001) ==========
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 Review-Url: https://codereview.chromium.org/2615993002 Cr-Commit-Position: refs/heads/master@{#443584} Committed: https://chromium.googlesource.com/chromium/src/+/6b8312a4903fc6965a64a3835524... patch from issue 2615993002 at patchset 520001 (http://crrev.com/2615993002#ps520001) ========== 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. On UI side, current production version hides tooltip immediately, but shows it up with a delay of a few hundreds of ms. This is due to the RepeatingTimer implementation. We make this UI design consistent by introducing a OneShotTime to delay the tooltip show up by 500 ms. BUG=668198 Review-Url: https://codereview.chromium.org/2615993002 Cr-Commit-Position: refs/heads/master@{#443584} Committed: https://chromium.googlesource.com/chromium/src/+/6b8312a4903fc6965a64a3835524... patch from issue 2615993002 at patchset 520001 (http://crrev.com/2615993002#ps520001) ==========
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. On UI side, current production version hides tooltip immediately, but shows it up with a delay of a few hundreds of ms. This is due to the RepeatingTimer implementation. We make this UI design consistent by introducing a OneShotTime to delay the tooltip show up by 500 ms. BUG=668198 Review-Url: https://codereview.chromium.org/2615993002 Cr-Commit-Position: refs/heads/master@{#443584} Committed: https://chromium.googlesource.com/chromium/src/+/6b8312a4903fc6965a64a3835524... patch from issue 2615993002 at patchset 520001 (http://crrev.com/2615993002#ps520001) ========== 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. On UI side, current production version hides tooltip immediately, but shows it up with a delay of a few hundreds of ms. This is due to the RepeatingTimer implementation. We make this UI design consistent by introducing a timer to delay the tooltip's appearance by 500 ms. BUG=668198 Review-Url: https://codereview.chromium.org/2615993002 Cr-Commit-Position: refs/heads/master@{#443584} Committed: https://chromium.googlesource.com/chromium/src/+/6b8312a4903fc6965a64a3835524... patch from issue 2615993002 at patchset 520001 (http://crrev.com/2615993002#ps520001) ==========
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #5 (id:80001) has been deleted
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:60001) has been deleted
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. On UI side, current production version hides tooltip immediately, but shows it up with a delay of a few hundreds of ms. This is due to the RepeatingTimer implementation. We make this UI design consistent by introducing a timer to delay the tooltip's appearance by 500 ms. BUG=668198 Review-Url: https://codereview.chromium.org/2615993002 Cr-Commit-Position: refs/heads/master@{#443584} Committed: https://chromium.googlesource.com/chromium/src/+/6b8312a4903fc6965a64a3835524... patch from issue 2615993002 at patchset 520001 (http://crrev.com/2615993002#ps520001) ========== 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. On UI side, current production version hides tooltip immediately, but shows it up with a delay of a few hundreds of ms. This is because of the RepeatingTimer implementation. The delayed appearance of tooltip seems to be by design. We make this UI design consistent by introducing a timer to delay the tooltip's appearance by 500 ms. BUG=668198 Review-Url: https://codereview.chromium.org/2615993002 Cr-Commit-Position: refs/heads/master@{#443584} Committed: https://chromium.googlesource.com/chromium/src/+/6b8312a4903fc6965a64a3835524... patch from issue 2615993002 at patchset 520001 (http://crrev.com/2615993002#ps520001) ==========
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. On UI side, current production version hides tooltip immediately, but shows it up with a delay of a few hundreds of ms. This is because of the RepeatingTimer implementation. The delayed appearance of tooltip seems to be by design. We make this UI design consistent by introducing a timer to delay the tooltip's appearance by 500 ms. BUG=668198 Review-Url: https://codereview.chromium.org/2615993002 Cr-Commit-Position: refs/heads/master@{#443584} Committed: https://chromium.googlesource.com/chromium/src/+/6b8312a4903fc6965a64a3835524... patch from issue 2615993002 at patchset 520001 (http://crrev.com/2615993002#ps520001) ========== 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. On UI side, current production version hides tooltip immediately, but shows it up with a delay of a few hundreds of ms. This is because of the RepeatingTimer implementation. The delayed appearance of tooltip seems to be by design. We make this UI design consistent by introducing a timer to delay the tooltip's appearance by 500 ms. BUG=668198 Review-Url: https://codereview.chromium.org/2615993002 Cr-Commit-Position: refs/heads/master@{#443584} Committed: https://chromium.googlesource.com/chromium/src/+/6b8312a4903fc6965a64a3835524... patch from issue 2615993002 at patchset 520001 (http://crrev.com/2615993002#ps520001) ==========
Patchset #3 (id:100001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #2 (id:20001) has been deleted
chengx@chromium.org changed reviewers: + sky@chromium.org
Hi sky@, PTAL~ The 1st patch in this CL is the same one I checked in but reverted due to UI regression. The 2nd patch fixes this UI regression by showing the tooltip with a delay of 500 ms. Thanks!
LGTM https://codereview.chromium.org/2652833002/diff/120001/ui/views/corewm/toolti... File ui/views/corewm/tooltip_controller.cc (right): https://codereview.chromium.org/2652833002/diff/120001/ui/views/corewm/toolti... ui/views/corewm/tooltip_controller.cc:323: } else Add {} https://codereview.chromium.org/2652833002/diff/120001/ui/views/corewm/toolti... ui/views/corewm/tooltip_controller.cc:329: if (!tooltip_window_) return; Generally chrome code avoids single line if statements. https://codereview.chromium.org/2652833002/diff/120001/ui/views/corewm/toolti... File ui/views/corewm/tooltip_controller.h (right): https://codereview.chromium.org/2652833002/diff/120001/ui/views/corewm/toolti... ui/views/corewm/tooltip_controller.h:119: bool tooltip_show_delayed_; Document what this means.
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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. On UI side, current production version hides tooltip immediately, but shows it up with a delay of a few hundreds of ms. This is because of the RepeatingTimer implementation. The delayed appearance of tooltip seems to be by design. We make this UI design consistent by introducing a timer to delay the tooltip's appearance by 500 ms. BUG=668198 Review-Url: https://codereview.chromium.org/2615993002 Cr-Commit-Position: refs/heads/master@{#443584} Committed: https://chromium.googlesource.com/chromium/src/+/6b8312a4903fc6965a64a3835524... patch from issue 2615993002 at patchset 520001 (http://crrev.com/2615993002#ps520001) ========== 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. On UI side, current production version hides tooltip immediately, but shows it up with a delay of a random few hundreds of ms. This is because of the RepeatingTimer implementation. One one hand, the delayed appearance of tooltip is preferred. On the other hand, the random delay time from instant to a few hundreds of ms doesn't look good. We improve this by introducing a timer to delay the tooltip's appearance by 500 ms. This makes the intended delay consistent for all tooltips. BUG=668198 Review-Url: https://codereview.chromium.org/2615993002 Cr-Commit-Position: refs/heads/master@{#443584} Committed: https://chromium.googlesource.com/chromium/src/+/6b8312a4903fc6965a64a3835524... patch from issue 2615993002 at patchset 520001 (http://crrev.com/2615993002#ps520001) ==========
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. On UI side, current production version hides tooltip immediately, but shows it up with a delay of a random few hundreds of ms. This is because of the RepeatingTimer implementation. One one hand, the delayed appearance of tooltip is preferred. On the other hand, the random delay time from instant to a few hundreds of ms doesn't look good. We improve this by introducing a timer to delay the tooltip's appearance by 500 ms. This makes the intended delay consistent for all tooltips. BUG=668198 Review-Url: https://codereview.chromium.org/2615993002 Cr-Commit-Position: refs/heads/master@{#443584} Committed: https://chromium.googlesource.com/chromium/src/+/6b8312a4903fc6965a64a3835524... patch from issue 2615993002 at patchset 520001 (http://crrev.com/2615993002#ps520001) ========== to ========== Remove unnecessary spin in ToolTipController The motivation for the change is to reduce idle wakeups and therefore reduce idle power. Another benefit of this CL is to make the UI better, as discussed in the UI paragraph below. 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. On UI side, current production version hides tooltip immediately, but shows it up with a delay of a random few hundreds of ms. This is because of the RepeatingTimer implementation. One one hand, the delayed appearance of tooltip is preferred. On the other hand, the random delay time from instant to a few hundreds of ms doesn't look good. We improve this by introducing a timer to delay the tooltip's appearance by 500 ms. This makes the intended delay consistent for all tooltips. BUG=668198 Review-Url: https://codereview.chromium.org/2615993002 Cr-Commit-Position: refs/heads/master@{#443584} Committed: https://chromium.googlesource.com/chromium/src/+/6b8312a4903fc6965a64a3835524... patch from issue 2615993002 at patchset 520001 (http://crrev.com/2615993002#ps520001) ==========
Description was changed from ========== Remove unnecessary spin in ToolTipController The motivation for the change is to reduce idle wakeups and therefore reduce idle power. Another benefit of this CL is to make the UI better, as discussed in the UI paragraph below. 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. On UI side, current production version hides tooltip immediately, but shows it up with a delay of a random few hundreds of ms. This is because of the RepeatingTimer implementation. One one hand, the delayed appearance of tooltip is preferred. On the other hand, the random delay time from instant to a few hundreds of ms doesn't look good. We improve this by introducing a timer to delay the tooltip's appearance by 500 ms. This makes the intended delay consistent for all tooltips. BUG=668198 Review-Url: https://codereview.chromium.org/2615993002 Cr-Commit-Position: refs/heads/master@{#443584} Committed: https://chromium.googlesource.com/chromium/src/+/6b8312a4903fc6965a64a3835524... patch from issue 2615993002 at patchset 520001 (http://crrev.com/2615993002#ps520001) ========== to ========== Remove unnecessary spin in ToolTipController The main motivation for the change is to reduce idle wakeups and therefore reduce idle power. Another benefit of this CL is to make the UI better, as discussed in the UI paragraph below. 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. On UI side, current production version hides tooltip immediately, but shows it up with a delay of a random few hundreds of ms. This is because of the RepeatingTimer implementation. One one hand, the delayed appearance of tooltip is preferred. On the other hand, the random delay time from instant to a few hundreds of ms doesn't look good. We improve this by introducing a timer to delay the tooltip's appearance by 500 ms. This makes the intended delay consistent for all tooltips. BUG=668198 Review-Url: https://codereview.chromium.org/2615993002 Cr-Commit-Position: refs/heads/master@{#443584} Committed: https://chromium.googlesource.com/chromium/src/+/6b8312a4903fc6965a64a3835524... patch from issue 2615993002 at patchset 520001 (http://crrev.com/2615993002#ps520001) ==========
Description was changed from ========== Remove unnecessary spin in ToolTipController The main motivation for the change is to reduce idle wakeups and therefore reduce idle power. Another benefit of this CL is to make the UI better, as discussed in the UI paragraph below. 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. On UI side, current production version hides tooltip immediately, but shows it up with a delay of a random few hundreds of ms. This is because of the RepeatingTimer implementation. One one hand, the delayed appearance of tooltip is preferred. On the other hand, the random delay time from instant to a few hundreds of ms doesn't look good. We improve this by introducing a timer to delay the tooltip's appearance by 500 ms. This makes the intended delay consistent for all tooltips. BUG=668198 Review-Url: https://codereview.chromium.org/2615993002 Cr-Commit-Position: refs/heads/master@{#443584} Committed: https://chromium.googlesource.com/chromium/src/+/6b8312a4903fc6965a64a3835524... patch from issue 2615993002 at patchset 520001 (http://crrev.com/2615993002#ps520001) ========== to ========== Remove unnecessary spin in ToolTipController The main motivation for the change is to reduce idle wakeups and therefore reduce idle power. Another benefit of this CL is to make the UI better, as discussed in the UI paragraph below. 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. On UI side, current production version hides tooltip immediately, but shows it up with a delay of a random few hundreds of ms. This is because of the RepeatingTimer implementation. One one hand, the delayed appearance of tooltip is preferred. On the other hand, the random delay time from instant to a few hundreds of ms doesn't look good. We improve this by introducing a timer to delay the tooltip's appearance by 500 ms. This makes the intended delay consistent for all tooltips. BUG=668198, 684877 Review-Url: https://codereview.chromium.org/2615993002 Cr-Commit-Position: refs/heads/master@{#443584} Committed: https://chromium.googlesource.com/chromium/src/+/6b8312a4903fc6965a64a3835524... patch from issue 2615993002 at patchset 520001 (http://crrev.com/2615993002#ps520001) ==========
Description was changed from ========== Remove unnecessary spin in ToolTipController The main motivation for the change is to reduce idle wakeups and therefore reduce idle power. Another benefit of this CL is to make the UI better, as discussed in the UI paragraph below. 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. On UI side, current production version hides tooltip immediately, but shows it up with a delay of a random few hundreds of ms. This is because of the RepeatingTimer implementation. One one hand, the delayed appearance of tooltip is preferred. On the other hand, the random delay time from instant to a few hundreds of ms doesn't look good. We improve this by introducing a timer to delay the tooltip's appearance by 500 ms. This makes the intended delay consistent for all tooltips. BUG=668198, 684877 Review-Url: https://codereview.chromium.org/2615993002 Cr-Commit-Position: refs/heads/master@{#443584} Committed: https://chromium.googlesource.com/chromium/src/+/6b8312a4903fc6965a64a3835524... patch from issue 2615993002 at patchset 520001 (http://crrev.com/2615993002#ps520001) ========== to ========== Remove unnecessary spin in ToolTipController and make tooltip appear with consistent delay The main motivation for the change is to reduce idle wakeups and therefore reduce idle power. Another motivation is to make the UI better, as currently tooltip shows up with random delay time. 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. On UI side, current production version hides tooltip immediately, but shows it up with a delay of a random few hundreds of ms. This is because of the RepeatingTimer implementation. One one hand, the delayed appearance of tooltip is preferred. On the other hand, the random delay time from instant to a few hundreds of ms doesn't look good. We improve this by introducing a timer to delay the tooltip's appearance by 500 ms. This makes the intended delay consistent for all tooltips. BUG=668198, 684877 Review-Url: https://codereview.chromium.org/2615993002 Cr-Commit-Position: refs/heads/master@{#443584} Committed: https://chromium.googlesource.com/chromium/src/+/6b8312a4903fc6965a64a3835524... patch from issue 2615993002 at patchset 520001 (http://crrev.com/2615993002#ps520001) ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Remove unnecessary spin in ToolTipController and make tooltip appear with consistent delay The main motivation for the change is to reduce idle wakeups and therefore reduce idle power. Another motivation is to make the UI better, as currently tooltip shows up with random delay time. 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. On UI side, current production version hides tooltip immediately, but shows it up with a delay of a random few hundreds of ms. This is because of the RepeatingTimer implementation. One one hand, the delayed appearance of tooltip is preferred. On the other hand, the random delay time from instant to a few hundreds of ms doesn't look good. We improve this by introducing a timer to delay the tooltip's appearance by 500 ms. This makes the intended delay consistent for all tooltips. BUG=668198, 684877 Review-Url: https://codereview.chromium.org/2615993002 Cr-Commit-Position: refs/heads/master@{#443584} Committed: https://chromium.googlesource.com/chromium/src/+/6b8312a4903fc6965a64a3835524... patch from issue 2615993002 at patchset 520001 (http://crrev.com/2615993002#ps520001) ========== to ========== Remove infinite spin, make tooltip delay consistent The main motivation for the change is to reduce idle wakeups and therefore reduce idle power. Another motivation is to make the UI better, as currently tooltip shows up with random delay time. ToolTipController has a repeating timer firing 4 times per second 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. On UI side, current production version hides tooltip immediately, but shows it up with a delay of a random few hundreds of ms. This is because of the RepeatingTimer implementation. One one hand, the delayed appearance of tooltip is preferred. On the other hand, the random delay time from instant to a few hundreds of ms doesn't look good. We improve this by introducing a timer to delay the tooltip's appearance by 500 ms. This makes the intended delay consistent for all tooltips. BUG=668198, 684877 Review-Url: https://codereview.chromium.org/2615993002 Cr-Commit-Position: refs/heads/master@{#443584} Committed: https://chromium.googlesource.com/chromium/src/+/6b8312a4903fc6965a64a3835524... patch from issue 2615993002 at patchset 520001 (http://crrev.com/2615993002#ps520001) ==========
The CQ bit was checked by chengx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/2652833002/#ps140001 (title: "Fix nits.")
Nits addressed. Thanks, sky@ https://codereview.chromium.org/2652833002/diff/120001/ui/views/corewm/toolti... File ui/views/corewm/tooltip_controller.cc (right): https://codereview.chromium.org/2652833002/diff/120001/ui/views/corewm/toolti... ui/views/corewm/tooltip_controller.cc:323: } else On 2017/01/24 22:13:39, sky wrote: > Add {} Done. https://codereview.chromium.org/2652833002/diff/120001/ui/views/corewm/toolti... ui/views/corewm/tooltip_controller.cc:329: if (!tooltip_window_) return; On 2017/01/24 22:13:39, sky wrote: > Generally chrome code avoids single line if statements. Fixed. https://codereview.chromium.org/2652833002/diff/120001/ui/views/corewm/toolti... File ui/views/corewm/tooltip_controller.h (right): https://codereview.chromium.org/2652833002/diff/120001/ui/views/corewm/toolti... ui/views/corewm/tooltip_controller.h:119: bool tooltip_show_delayed_; On 2017/01/24 22:13:39, sky wrote: > Document what this means. Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1485307525982550,
"parent_rev": "401c5932118fb24071348b6a6ee5ce1638a7d982", "commit_rev":
"122326d5ae2407e11a8b7353fd60dcffba5066fa"}
Message was sent while issue was closed.
Description was changed from ========== Remove infinite spin, make tooltip delay consistent The main motivation for the change is to reduce idle wakeups and therefore reduce idle power. Another motivation is to make the UI better, as currently tooltip shows up with random delay time. ToolTipController has a repeating timer firing 4 times per second 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. On UI side, current production version hides tooltip immediately, but shows it up with a delay of a random few hundreds of ms. This is because of the RepeatingTimer implementation. One one hand, the delayed appearance of tooltip is preferred. On the other hand, the random delay time from instant to a few hundreds of ms doesn't look good. We improve this by introducing a timer to delay the tooltip's appearance by 500 ms. This makes the intended delay consistent for all tooltips. BUG=668198, 684877 Review-Url: https://codereview.chromium.org/2615993002 Cr-Commit-Position: refs/heads/master@{#443584} Committed: https://chromium.googlesource.com/chromium/src/+/6b8312a4903fc6965a64a3835524... patch from issue 2615993002 at patchset 520001 (http://crrev.com/2615993002#ps520001) ========== to ========== Remove infinite spin, make tooltip delay consistent The main motivation for the change is to reduce idle wakeups and therefore reduce idle power. Another motivation is to make the UI better, as currently tooltip shows up with random delay time. ToolTipController has a repeating timer firing 4 times per second 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. On UI side, current production version hides tooltip immediately, but shows it up with a delay of a random few hundreds of ms. This is because of the RepeatingTimer implementation. One one hand, the delayed appearance of tooltip is preferred. On the other hand, the random delay time from instant to a few hundreds of ms doesn't look good. We improve this by introducing a timer to delay the tooltip's appearance by 500 ms. This makes the intended delay consistent for all tooltips. BUG=668198, 684877 Review-Url: https://codereview.chromium.org/2615993002 Cr-Commit-Position: refs/heads/master@{#443584} Committed: https://chromium.googlesource.com/chromium/src/+/6b8312a4903fc6965a64a3835524... patch from issue 2615993002 at patchset 520001 (http://crrev.com/2615993002#ps520001) Review-Url: https://codereview.chromium.org/2652833002 Cr-Commit-Position: refs/heads/master@{#445902} Committed: https://chromium.googlesource.com/chromium/src/+/122326d5ae2407e11a8b7353fd60... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:140001) as https://chromium.googlesource.com/chromium/src/+/122326d5ae2407e11a8b7353fd60...
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:140001) has been created in https://codereview.chromium.org/2654343003/ by chengx@chromium.org. The reason for reverting is: Caused the UI regression 685078 and 685408 (duplicate). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
