New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 881545 link

Starred by 2 users
Status: Fixed
Owner:
sdy@chromium.org
Closed: Nov 16
Cc:
█ spqc...@chromium.org
ellyjo...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 2
Type: Bug



Sign in to add a comment

Touch Bar Text Suggestions recreates bar on every keystroke

Project Member Reported by sdy@chromium.org, Sep 6 
Chrome Version: 71.0.3544.2
OS: macOS

What steps will reproduce the problem?
(1) Enable chrome://flags/#enable-text-suggestions-touch-bar
(2) Type furiously into a text field on the web while watching Activity Monitor.

What is the expected result?
Middling CPU usage.

What happens instead?
Fairly high CPU usage. From looking at the code, the bar is invalidated on every keystroke.

Comment 1 by spqc...@chromium.org, Sep 6 

How bad is it? Do you think it'll be bad enough to block a release for M70?

Comment 2 by sdy@chromium.org, Sep 6 

Here's a super preliminary CL from earlier. I haven't had a chance to look into how bad it is, unfortunately.

https://chromium-review.googlesource.com/c/chromium/src/+/1212004

Comment 3 by sdy@chromium.org, Sep 7 

spqchan@: Can we run an experiment for it, if we’re not already, and measure the impact?

Comment 4 by spqc...@chromium.org, Sep 7 

Sure, we can do that. 50/50 sounds good? I can set up the Finch for that

Comment 5 by sdy@chromium.org, Sep 7

Cc: ellyjo...@chromium.org
Components: UI>Browser>Touchbar UI>Input>Text
SGTM. +ellyjones@.

Comment 6 by sdy@chromium.org, Sep 11 

After some local testing and a chat w/ ellyjones@, I'm going to work on this next week.

Comment 7 by sdy@chromium.org, Sep 11 

(And we should not release it in M70.)

Comment 8 by robliao@chromium.org, Sep 20

Labels: Proj-DesktopUI

Comment 9 by robliao@chromium.org, Sep 26

Labels: Hotlist-DesktopUITriaged
Project Member

Comment 10 by bugdroid1@chromium.org, Oct 5 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ad7c191591846d2138feeaa27d29db322a4cd0fe

commit ad7c191591846d2138feeaa27d29db322a4cd0fe
Author: Sidney San Martín <sdy@chromium.org>
Date: Fri Oct 05 23:44:11 2018

Fix bugs and improve perf in Touch Bar text suggestions.

- The old implementation recreated the Touch Bar on each keystroke (including
  when hidden, I believe), which burns a significant amount of power while
  typing. This new implementation updates the existing candidate list, if it
  exists and isn't collapsed.

- Removes a bunch of support code for a behavior that was disabled in r590669.
  It moves the remaining code into RWHV itself instead of a dedicated
  controller class, which I have mixed feelings about, but ultimately picked
  because after the support code was removed, most of what was left was
  plumbing between RWHV and TextSuggestionsTouchBarController.

- [Bigish change] Moves text suggestions out of the window and into the RWHV,
  so that hiding and showing as focus and web contents change is left to AppKit
  and the responder chain.

- Fixes small lifecyle-ish bugs around when use this kind of Touch Bar — now
  it's tied to the RWHV's input type instead of using a special
  WebContentsTextObserver.

Bug: 717553,  881545 
Change-Id: I8e1eff6da7918f92cfdb47465196d5841a88a7e4
Reviewed-on: https://chromium-review.googlesource.com/c/1212004
Commit-Queue: Sidney San Martín <sdy@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597371}
[modify] https://crrev.com/ad7c191591846d2138feeaa27d29db322a4cd0fe/base/mac/sdk_forward_declarations.h
[modify] https://crrev.com/ad7c191591846d2138feeaa27d29db322a4cd0fe/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/ad7c191591846d2138feeaa27d29db322a4cd0fe/chrome/browser/ui/cocoa/touchbar/browser_window_touch_bar_controller.mm
[delete] https://crrev.com/e149b499781e45dad92c3a21818d6520185e52dc/chrome/browser/ui/cocoa/touchbar/text_suggestions_touch_bar_controller.h
[delete] https://crrev.com/e149b499781e45dad92c3a21818d6520185e52dc/chrome/browser/ui/cocoa/touchbar/text_suggestions_touch_bar_controller.mm
[delete] https://crrev.com/e149b499781e45dad92c3a21818d6520185e52dc/chrome/browser/ui/cocoa/touchbar/text_suggestions_touch_bar_controller_browsertest.mm
[delete] https://crrev.com/e149b499781e45dad92c3a21818d6520185e52dc/chrome/browser/ui/cocoa/touchbar/text_suggestions_touch_bar_controller_unittest.mm
[modify] https://crrev.com/ad7c191591846d2138feeaa27d29db322a4cd0fe/chrome/browser/ui/cocoa/touchbar/web_textfield_touch_bar_controller.h
[modify] https://crrev.com/ad7c191591846d2138feeaa27d29db322a4cd0fe/chrome/browser/ui/cocoa/touchbar/web_textfield_touch_bar_controller.mm
[modify] https://crrev.com/ad7c191591846d2138feeaa27d29db322a4cd0fe/chrome/browser/ui/passwords/password_generation_popup_controller_impl.cc
[modify] https://crrev.com/ad7c191591846d2138feeaa27d29db322a4cd0fe/chrome/common/chrome_features.cc
[modify] https://crrev.com/ad7c191591846d2138feeaa27d29db322a4cd0fe/chrome/common/chrome_features.h
[modify] https://crrev.com/ad7c191591846d2138feeaa27d29db322a4cd0fe/chrome/test/BUILD.gn
[modify] https://crrev.com/ad7c191591846d2138feeaa27d29db322a4cd0fe/content/browser/frame_host/render_widget_host_view_guest.cc
[modify] https://crrev.com/ad7c191591846d2138feeaa27d29db322a4cd0fe/content/browser/frame_host/render_widget_host_view_guest.h
[modify] https://crrev.com/ad7c191591846d2138feeaa27d29db322a4cd0fe/content/browser/renderer_host/render_widget_host_delegate.h
[modify] https://crrev.com/ad7c191591846d2138feeaa27d29db322a4cd0fe/content/browser/renderer_host/render_widget_host_view_base.cc
[modify] https://crrev.com/ad7c191591846d2138feeaa27d29db322a4cd0fe/content/browser/renderer_host/render_widget_host_view_base.h
[modify] https://crrev.com/ad7c191591846d2138feeaa27d29db322a4cd0fe/content/browser/renderer_host/render_widget_host_view_cocoa.h
[modify] https://crrev.com/ad7c191591846d2138feeaa27d29db322a4cd0fe/content/browser/renderer_host/render_widget_host_view_cocoa.mm
[modify] https://crrev.com/ad7c191591846d2138feeaa27d29db322a4cd0fe/content/browser/renderer_host/render_widget_host_view_mac.mm
[modify] https://crrev.com/ad7c191591846d2138feeaa27d29db322a4cd0fe/content/browser/renderer_host/render_widget_host_view_mac_unittest.mm
[modify] https://crrev.com/ad7c191591846d2138feeaa27d29db322a4cd0fe/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/ad7c191591846d2138feeaa27d29db322a4cd0fe/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/ad7c191591846d2138feeaa27d29db322a4cd0fe/content/public/browser/render_widget_host_view.h
[modify] https://crrev.com/ad7c191591846d2138feeaa27d29db322a4cd0fe/content/public/browser/web_contents_observer.h
[modify] https://crrev.com/ad7c191591846d2138feeaa27d29db322a4cd0fe/content/public/common/content_features.cc
[modify] https://crrev.com/ad7c191591846d2138feeaa27d29db322a4cd0fe/content/public/common/content_features.h
Project Member

Comment 11 by bugdroid1@chromium.org, Oct 8 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6068cf6b0b7348f09085a3256588f831653b4a52

commit 6068cf6b0b7348f09085a3256588f831653b4a52
Author: Avi Drissman <avi@chromium.org>
Date: Mon Oct 08 19:52:36 2018

Revert "Fix bugs and improve perf in Touch Bar text suggestions."

This reverts commit ad7c191591846d2138feeaa27d29db322a4cd0fe.

Reason for revert: Causes crashes (893038, 893222).

Original change's description:
> Fix bugs and improve perf in Touch Bar text suggestions.
> 
> - The old implementation recreated the Touch Bar on each keystroke (including
>   when hidden, I believe), which burns a significant amount of power while
>   typing. This new implementation updates the existing candidate list, if it
>   exists and isn't collapsed.
> 
> - Removes a bunch of support code for a behavior that was disabled in r590669.
>   It moves the remaining code into RWHV itself instead of a dedicated
>   controller class, which I have mixed feelings about, but ultimately picked
>   because after the support code was removed, most of what was left was
>   plumbing between RWHV and TextSuggestionsTouchBarController.
> 
> - [Bigish change] Moves text suggestions out of the window and into the RWHV,
>   so that hiding and showing as focus and web contents change is left to AppKit
>   and the responder chain.
> 
> - Fixes small lifecyle-ish bugs around when use this kind of Touch Bar — now
>   it's tied to the RWHV's input type instead of using a special
>   WebContentsTextObserver.
> 
> Bug: 717553,  881545 
> Change-Id: I8e1eff6da7918f92cfdb47465196d5841a88a7e4
> Reviewed-on: https://chromium-review.googlesource.com/c/1212004
> Commit-Queue: Sidney San Martín <sdy@chromium.org>
> Reviewed-by: Avi Drissman <avi@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#597371}

TBR=avi@chromium.org,sdy@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 717553,  881545 , 893222, 893038
Change-Id: Ia35f365b4cd46b98a7ffe0181fbf656fc4f9e672
Reviewed-on: https://chromium-review.googlesource.com/c/1269077
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597652}
[modify] https://crrev.com/6068cf6b0b7348f09085a3256588f831653b4a52/base/mac/sdk_forward_declarations.h
[modify] https://crrev.com/6068cf6b0b7348f09085a3256588f831653b4a52/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/6068cf6b0b7348f09085a3256588f831653b4a52/chrome/browser/ui/cocoa/touchbar/browser_window_touch_bar_controller.mm
[add] https://crrev.com/6068cf6b0b7348f09085a3256588f831653b4a52/chrome/browser/ui/cocoa/touchbar/text_suggestions_touch_bar_controller.h
[add] https://crrev.com/6068cf6b0b7348f09085a3256588f831653b4a52/chrome/browser/ui/cocoa/touchbar/text_suggestions_touch_bar_controller.mm
[add] https://crrev.com/6068cf6b0b7348f09085a3256588f831653b4a52/chrome/browser/ui/cocoa/touchbar/text_suggestions_touch_bar_controller_browsertest.mm
[add] https://crrev.com/6068cf6b0b7348f09085a3256588f831653b4a52/chrome/browser/ui/cocoa/touchbar/text_suggestions_touch_bar_controller_unittest.mm
[modify] https://crrev.com/6068cf6b0b7348f09085a3256588f831653b4a52/chrome/browser/ui/cocoa/touchbar/web_textfield_touch_bar_controller.h
[modify] https://crrev.com/6068cf6b0b7348f09085a3256588f831653b4a52/chrome/browser/ui/cocoa/touchbar/web_textfield_touch_bar_controller.mm
[modify] https://crrev.com/6068cf6b0b7348f09085a3256588f831653b4a52/chrome/browser/ui/passwords/password_generation_popup_controller_impl.cc
[modify] https://crrev.com/6068cf6b0b7348f09085a3256588f831653b4a52/chrome/common/chrome_features.cc
[modify] https://crrev.com/6068cf6b0b7348f09085a3256588f831653b4a52/chrome/common/chrome_features.h
[modify] https://crrev.com/6068cf6b0b7348f09085a3256588f831653b4a52/chrome/test/BUILD.gn
[modify] https://crrev.com/6068cf6b0b7348f09085a3256588f831653b4a52/content/browser/frame_host/render_widget_host_view_guest.cc
[modify] https://crrev.com/6068cf6b0b7348f09085a3256588f831653b4a52/content/browser/frame_host/render_widget_host_view_guest.h
[modify] https://crrev.com/6068cf6b0b7348f09085a3256588f831653b4a52/content/browser/renderer_host/render_widget_host_delegate.h
[modify] https://crrev.com/6068cf6b0b7348f09085a3256588f831653b4a52/content/browser/renderer_host/render_widget_host_view_base.cc
[modify] https://crrev.com/6068cf6b0b7348f09085a3256588f831653b4a52/content/browser/renderer_host/render_widget_host_view_base.h
[modify] https://crrev.com/6068cf6b0b7348f09085a3256588f831653b4a52/content/browser/renderer_host/render_widget_host_view_cocoa.h
[modify] https://crrev.com/6068cf6b0b7348f09085a3256588f831653b4a52/content/browser/renderer_host/render_widget_host_view_cocoa.mm
[modify] https://crrev.com/6068cf6b0b7348f09085a3256588f831653b4a52/content/browser/renderer_host/render_widget_host_view_mac.mm
[modify] https://crrev.com/6068cf6b0b7348f09085a3256588f831653b4a52/content/browser/renderer_host/render_widget_host_view_mac_unittest.mm
[modify] https://crrev.com/6068cf6b0b7348f09085a3256588f831653b4a52/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/6068cf6b0b7348f09085a3256588f831653b4a52/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/6068cf6b0b7348f09085a3256588f831653b4a52/content/public/browser/render_widget_host_view.h
[modify] https://crrev.com/6068cf6b0b7348f09085a3256588f831653b4a52/content/public/browser/web_contents_observer.h
[modify] https://crrev.com/6068cf6b0b7348f09085a3256588f831653b4a52/content/public/common/content_features.cc
[modify] https://crrev.com/6068cf6b0b7348f09085a3256588f831653b4a52/content/public/common/content_features.h

Comment 12 by robliao@chromium.org, Oct 8

Labels: Group-Touch_Bar
Project Member

Comment 13 by bugdroid1@chromium.org, Oct 30 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/28b39c180ef84b7d7763eabae317e35555dfd1b8

commit 28b39c180ef84b7d7763eabae317e35555dfd1b8
Author: Sidney San Martín <sdy@chromium.org>
Date: Tue Oct 30 02:17:39 2018

Reland set of Touch Bar text suggestions perf improvements and bug fixes.

This change is similar to the original (below) with the following additions:

- Skip requesting suggestions (and clear the candidate list) if the selection
  range is invalid. This caused crashes in the field: https://crbug.com/893038.
- Skip requesting suggestions if the candidate list is collapsed.
- Request suggestions immediately when the candidate list is shown.

This relands commit d7c191591846d2138feeaa27d29db322a4cd0fe.

Original change's description:
> Fix bugs and improve perf in Touch Bar text suggestions.
>
> - The old implementation recreated the Touch Bar on each keystroke (including
>   when hidden, I believe), which burns a significant amount of power while
>   typing. This new implementation updates the existing candidate list, if it
>   exists and isn't collapsed.
>
> - Removes a bunch of support code for a behavior that was disabled in r590669.
>   It moves the remaining code into RWHV itself instead of a dedicated
>   controller class, which I have mixed feelings about, but ultimately picked
>   because after the support code was removed, most of what was left was
>   plumbing between RWHV and TextSuggestionsTouchBarController.
>
> - [Bigish change] Moves text suggestions out of the window and into the RWHV,
>   so that hiding and showing as focus and web contents change is left to AppKit
>   and the responder chain.
>
> - Fixes small lifecyle-ish bugs around when use this kind of Touch Bar — now
>   it's tied to the RWHV's input type instead of using a special
>   WebContentsTextObserver.
>
> Bug: 717553,  881545 
> Change-Id: I8e1eff6da7918f92cfdb47465196d5841a88a7e4
> Reviewed-on: https://chromium-review.googlesource.com/c/1212004
> Commit-Queue: Sidney San Martín <sdy@chromium.org>
> Reviewed-by: Avi Drissman <avi@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#597371}

Bug: 717553,  881545 , 893222, 893038
Change-Id: I2f8405f9784514462c2bfe8a754d3ca174f47d27
Reviewed-on: https://chromium-review.googlesource.com/c/1299664
Commit-Queue: Sidney San Martín <sdy@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603741}
[modify] https://crrev.com/28b39c180ef84b7d7763eabae317e35555dfd1b8/base/mac/sdk_forward_declarations.h
[modify] https://crrev.com/28b39c180ef84b7d7763eabae317e35555dfd1b8/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/28b39c180ef84b7d7763eabae317e35555dfd1b8/chrome/browser/ui/cocoa/touchbar/browser_window_touch_bar_controller.mm
[delete] https://crrev.com/d0570c8fff4e53006b424afa7def1befdb0a55fe/chrome/browser/ui/cocoa/touchbar/text_suggestions_touch_bar_controller.h
[delete] https://crrev.com/d0570c8fff4e53006b424afa7def1befdb0a55fe/chrome/browser/ui/cocoa/touchbar/text_suggestions_touch_bar_controller.mm
[delete] https://crrev.com/d0570c8fff4e53006b424afa7def1befdb0a55fe/chrome/browser/ui/cocoa/touchbar/text_suggestions_touch_bar_controller_browsertest.mm
[delete] https://crrev.com/d0570c8fff4e53006b424afa7def1befdb0a55fe/chrome/browser/ui/cocoa/touchbar/text_suggestions_touch_bar_controller_unittest.mm
[modify] https://crrev.com/28b39c180ef84b7d7763eabae317e35555dfd1b8/chrome/browser/ui/cocoa/touchbar/web_textfield_touch_bar_controller.h
[modify] https://crrev.com/28b39c180ef84b7d7763eabae317e35555dfd1b8/chrome/browser/ui/cocoa/touchbar/web_textfield_touch_bar_controller.mm
[modify] https://crrev.com/28b39c180ef84b7d7763eabae317e35555dfd1b8/chrome/browser/ui/passwords/password_generation_popup_controller_impl.cc
[modify] https://crrev.com/28b39c180ef84b7d7763eabae317e35555dfd1b8/chrome/common/chrome_features.cc
[modify] https://crrev.com/28b39c180ef84b7d7763eabae317e35555dfd1b8/chrome/common/chrome_features.h
[modify] https://crrev.com/28b39c180ef84b7d7763eabae317e35555dfd1b8/chrome/test/BUILD.gn
[modify] https://crrev.com/28b39c180ef84b7d7763eabae317e35555dfd1b8/content/browser/frame_host/render_widget_host_view_guest.cc
[modify] https://crrev.com/28b39c180ef84b7d7763eabae317e35555dfd1b8/content/browser/frame_host/render_widget_host_view_guest.h
[modify] https://crrev.com/28b39c180ef84b7d7763eabae317e35555dfd1b8/content/browser/renderer_host/render_widget_host_delegate.h
[modify] https://crrev.com/28b39c180ef84b7d7763eabae317e35555dfd1b8/content/browser/renderer_host/render_widget_host_view_base.cc
[modify] https://crrev.com/28b39c180ef84b7d7763eabae317e35555dfd1b8/content/browser/renderer_host/render_widget_host_view_base.h
[modify] https://crrev.com/28b39c180ef84b7d7763eabae317e35555dfd1b8/content/browser/renderer_host/render_widget_host_view_cocoa.h
[modify] https://crrev.com/28b39c180ef84b7d7763eabae317e35555dfd1b8/content/browser/renderer_host/render_widget_host_view_cocoa.mm
[modify] https://crrev.com/28b39c180ef84b7d7763eabae317e35555dfd1b8/content/browser/renderer_host/render_widget_host_view_mac.mm
[modify] https://crrev.com/28b39c180ef84b7d7763eabae317e35555dfd1b8/content/browser/renderer_host/render_widget_host_view_mac_unittest.mm
[modify] https://crrev.com/28b39c180ef84b7d7763eabae317e35555dfd1b8/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/28b39c180ef84b7d7763eabae317e35555dfd1b8/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/28b39c180ef84b7d7763eabae317e35555dfd1b8/content/public/browser/render_widget_host_view.h
[modify] https://crrev.com/28b39c180ef84b7d7763eabae317e35555dfd1b8/content/public/browser/web_contents_observer.h
[modify] https://crrev.com/28b39c180ef84b7d7763eabae317e35555dfd1b8/content/public/common/content_features.cc
[modify] https://crrev.com/28b39c180ef84b7d7763eabae317e35555dfd1b8/content/public/common/content_features.h

Comment 14 by ligimole@google.com, Nov 14

Labels: Hotlist-DesktopUIChecked Hotlist-DesktopUIValid
***Mass UI Triage***

sdy@ could you please help in verifying the issue?

Comment 15 by ellyjo...@chromium.org, Nov 15 

This is Fixed now, right? sdy@

Comment 16 by sdy@chromium.org, Nov 16

Status: Fixed (was: Assigned)
👍
►

Sign in to add a comment