John An (Gerrit)

unread,

May 11, 2026, 7:41:15 PMMay 11

to Olga Gerchikov, Gaston Rodriguez, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, James Su, alexmo...@chromium.org, creis...@chromium.org, dtapuska+ch...@chromium.org, jbauma...@chromium.org, keithle...@chromium.org, navigation...@chromium.org, nona+...@chromium.org, shuche...@chromium.org, tranbaod...@chromium.org, yhanad...@chromium.org

Attention needed from Gaston Rodriguez and Olga Gerchikov

John An added 1 comment

Patchset-level comments

File-level comment, Patchset 1 (Latest):

John An . resolved

Hi all, this is a small change around calling FocusFailed in the case of RWHV destruction. Would appreciate a look when you get the chance.

Open in Gerrit

Related details

Attention is currently required from:

Gaston Rodriguez
Olga Gerchikov

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Gaston Rodriguez (Gerrit)

unread,

May 12, 2026, 11:49:03 AMMay 12

to John An, Olga Gerchikov, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, James Su, alexmo...@chromium.org, creis...@chromium.org, dtapuska+ch...@chromium.org, jbauma...@chromium.org, keithle...@chromium.org, navigation...@chromium.org, nona+...@chromium.org, shuche...@chromium.org, tranbaod...@chromium.org, yhanad...@chromium.org

Attention needed from John An and Olga Gerchikov

Gaston Rodriguez added 2 comments

Patchset-level comments

File-level comment, Patchset 2 (Latest):

Gaston Rodriguez . resolved

LGTM. Is this testable? Seems like it would be hard to test, but would be nice to have something.

File content/browser/renderer_host/render_widget_host_view_child_frame.cc

Line 86, Patchset 2 (Latest):

  if (StylusHandwritingControllerWin::IsHandwritingAPIAvailable() &&
      StylusHandwritingControllerWin::GetInstance()
          ->IsWaitingForFocusResult()) {

Gaston Rodriguez . unresolved

Do you have to null-check this controller or is it a certainty that `GetInstance` will return an object?

It also seems like `GetInstance` is being called regardless of false return in `IsHandwritingAPIAvailable`, which is making the tests hit a CHECK.


```suggestion
  if (StylusHandwritingControllerWin::IsHandwritingAPIAvailable()) {
    if (StylusHandwritingControllerWin::GetInstance() && // if you need to null check
      StylusHandwritingControllerWin::GetInstance()
          ->IsWaitingForFocusResult()) {
```

Open in Gerrit

Related details

Attention is currently required from:

John An
Olga Gerchikov

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

No-Unresolved-Comments

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Olga Gerchikov (Gerrit)

unread,

May 12, 2026, 5:26:32 PMMay 12

to John An, Gaston Rodriguez, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, James Su, alexmo...@chromium.org, creis...@chromium.org, dtapuska+ch...@chromium.org, jbauma...@chromium.org, keithle...@chromium.org, navigation...@chromium.org, nona+...@chromium.org, shuche...@chromium.org, tranbaod...@chromium.org, yhanad...@chromium.org

Attention needed from John An

Olga Gerchikov added 1 comment

Commit Message

Line 10, Patchset 2 (Latest):API ie. TSF that focus has failed if RenderWidgetHostView is destroyed

Olga Gerchikov . unresolved

Can we have tests for this scenario?

Open in Gerrit

Related details

Attention is currently required from:

John An

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

John An (Gerrit)

unread,

May 13, 2026, 8:06:50 PMMay 13

to Olga Gerchikov, Gaston Rodriguez, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, Zhe Su, alexmo...@chromium.org, creis...@chromium.org, dtapuska+ch...@chromium.org, jbauma...@chromium.org, keithle...@chromium.org, navigation...@chromium.org, nona+...@chromium.org, shuche...@chromium.org, tranbaod...@chromium.org, yhanad...@chromium.org

Attention needed from Gaston Rodriguez and Olga Gerchikov

John An added 2 comments

Commit Message

Line 10, Patchset 2:API ie. TSF that focus has failed if RenderWidgetHostView is destroyed

Olga Gerchikov . unresolved

Can we have tests for this scenario?

John An

Sure I'll write some tests.

File content/browser/renderer_host/render_widget_host_view_child_frame.cc

Line 86, Patchset 2:

  if (StylusHandwritingControllerWin::IsHandwritingAPIAvailable() &&
      StylusHandwritingControllerWin::GetInstance()
          ->IsWaitingForFocusResult()) {

Gaston Rodriguez . resolved

Do you have to null-check this controller or is it a certainty that `GetInstance` will return an object?
It also seems like `GetInstance` is being called regardless of false return in `IsHandwritingAPIAvailable`, which is making the tests hit a CHECK.
```suggestion
  if (StylusHandwritingControllerWin::IsHandwritingAPIAvailable()) {
    if (StylusHandwritingControllerWin::GetInstance() && // if you need to null check
      StylusHandwritingControllerWin::GetInstance()
          ->IsWaitingForFocusResult()) {
```

John An

We have to null check the controller is initialized on hover with stylus. If that never happens, then GetInstance can be nullptr.

I believe the issue in CQ is just because I didn't check IsHandwritingAPIAvailable in RWHVA. Fixing that.

Open in Gerrit

Related details

Attention is currently required from:

Gaston Rodriguez
Olga Gerchikov

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

John An (Gerrit)

unread,

May 15, 2026, 11:24:11 PMMay 15

to Olga Gerchikov, Gaston Rodriguez, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, Zhe Su, alexmo...@chromium.org, creis...@chromium.org, dtapuska+ch...@chromium.org, jbauma...@chromium.org, keithle...@chromium.org, navigation...@chromium.org, nona+...@chromium.org, shuche...@chromium.org, tranbaod...@chromium.org, yhanad...@chromium.org

Attention needed from Gaston Rodriguez and Olga Gerchikov

John An added 1 comment

Commit Message

Line 10, Patchset 2:API ie. TSF that focus has failed if RenderWidgetHostView is destroyed

Olga Gerchikov . resolved

Can we have tests for this scenario?

John An

Sure I'll write some tests.

John An

Done

Open in Gerrit

Related details

Attention is currently required from:

Gaston Rodriguez
Olga Gerchikov

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Olga Gerchikov (Gerrit)

unread,

May 19, 2026, 12:33:21 AMMay 19

to John An, Gaston Rodriguez, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, Zhe Su, alexmo...@chromium.org, creis...@chromium.org, dtapuska+ch...@chromium.org, jbauma...@chromium.org, keithle...@chromium.org, navigation...@chromium.org, nona+...@chromium.org, shuche...@chromium.org, tranbaod...@chromium.org, yhanad...@chromium.org

Attention needed from Gaston Rodriguez and John An

Olga Gerchikov voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Gaston Rodriguez
John An

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

John An (Gerrit)

unread,

May 19, 2026, 1:43:04 PMMay 19

to Jonathan Ross, Olga Gerchikov, Gaston Rodriguez, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, Zhe Su, alexmo...@chromium.org, creis...@chromium.org, dtapuska+ch...@chromium.org, jbauma...@chromium.org, keithle...@chromium.org, navigation...@chromium.org, nona+...@chromium.org, shuche...@chromium.org, tranbaod...@chromium.org, yhanad...@chromium.org

Attention needed from Gaston Rodriguez and Jonathan Ross

John An added 1 comment

Patchset-level comments

File-level comment, Patchset 5 (Latest):

John An . resolved

Hi Jon, here's the follow up CL for OnFocusFailed fixups.

Open in Gerrit

Related details

Attention is currently required from:

Gaston Rodriguez
Jonathan Ross

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Jonathan Ross (Gerrit)

unread,

May 20, 2026, 10:32:12 AMMay 20

to John An, Olga Gerchikov, Gaston Rodriguez, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, Zhe Su, alexmo...@chromium.org, creis...@chromium.org, dtapuska+ch...@chromium.org, jbauma...@chromium.org, keithle...@chromium.org, navigation...@chromium.org, nona+...@chromium.org, shuche...@chromium.org, tranbaod...@chromium.org, yhanad...@chromium.org

Attention needed from Gaston Rodriguez and John An

Jonathan Ross added 1 comment

File content/browser/renderer_host/input/stylus_handwriting_controller_win.cc

Line 184, Patchset 5 (Latest):

bool StylusHandwritingControllerWin::IsWaitingForFocusResult() const {
  return handwriting_callback_sink_ &&
         handwriting_callback_sink_->IsFocusHandwritingTargetPending();

Jonathan Ross . unresolved

Do we have a way to know what RWHV was the focus or potential candidate for handwriting recognition?

Currently the teardown of any RWHV would cancel the handwriting, even if it wasn't the target.

Consider two open windows. User starts handwriting in one, and taps to close the other

Open in Gerrit

Related details

Attention is currently required from:

Gaston Rodriguez
John An

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

No-Unresolved-Comments

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

John An (Gerrit)

unread,

Jun 24, 2026, 9:38:57 PM (5 days ago) Jun 24

to Jonathan Ross, Olga Gerchikov, Gaston Rodriguez, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, Zhe Su, alexmo...@chromium.org, creis...@chromium.org, dtapuska+ch...@chromium.org, jbauma...@chromium.org, keithle...@chromium.org, navigation...@chromium.org, nona+...@chromium.org, shuche...@chromium.org, tranbaod...@chromium.org, yhanad...@chromium.org

Attention needed from Gaston Rodriguez and Olga Gerchikov

John An added 1 comment

File content/browser/renderer_host/input/stylus_handwriting_controller_win.cc

Line 184, Patchset 5:bool StylusHandwritingControllerWin::IsWaitingForFocusResult() const {


  return handwriting_callback_sink_ &&
         handwriting_callback_sink_->IsFocusHandwritingTargetPending();

Jonathan Ross . resolved

Do we have a way to know what RWHV was the focus or potential candidate for handwriting recognition?
Currently the teardown of any RWHV would cancel the handwriting, even if it wasn't the target.
Consider two open windows. User starts handwriting in one, and taps to close the other

John An

This is legit. It's not possible to touch close during ongoing pen interaction on windows, but this is technically possible with window close on timeout. Performed refactor that allows for this and added tests.

Open in Gerrit

Related details

Attention is currently required from:

Gaston Rodriguez
Olga Gerchikov

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

John An (Gerrit)

unread,

Jun 24, 2026, 9:40:30 PM (5 days ago) Jun 24

to Dave Tapuska, Jonathan Ross, Olga Gerchikov, Gaston Rodriguez, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, Zhe Su, alexmo...@chromium.org, creis...@chromium.org, dtapuska+ch...@chromium.org, jbauma...@chromium.org, keithle...@chromium.org, navigation...@chromium.org, nona+...@chromium.org, shuche...@chromium.org, tranbaod...@chromium.org, yhanad...@chromium.org

Attention needed from Dave Tapuska, Gaston Rodriguez, Nasko Oskov and Olga Gerchikov

John An added 1 comment

Patchset-level comments

File-level comment, Patchset 6 (Latest):

John An . resolved

Hi folks, Jon Ross was reviewing this until he went OOO. Would someone mind taking a look or adding an owner?

Open in Gerrit

Related details

Attention is currently required from:

Dave Tapuska
Gaston Rodriguez
Nasko Oskov
Olga Gerchikov

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Dave Tapuska (Gerrit)

unread,

Jun 25, 2026, 12:25:47 PM (4 days ago) Jun 25

to John An, Jonathan Ross, Olga Gerchikov, Gaston Rodriguez, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, Zhe Su, alexmo...@chromium.org, creis...@chromium.org, dtapuska+ch...@chromium.org, jbauma...@chromium.org, keithle...@chromium.org, navigation...@chromium.org, nona+...@chromium.org, shuche...@chromium.org, tranbaod...@chromium.org, yhanad...@chromium.org

Attention needed from Gaston Rodriguez, John An, Nasko Oskov and Olga Gerchikov

Dave Tapuska added 2 comments

File content/browser/renderer_host/render_widget_host_view_aura.cc

Line 2852, Patchset 7 (Latest):#if BUILDFLAG(IS_WIN)

Dave Tapuska . unresolved

Why is this replicated both in child frame and aura, wouldn't this just go in base?

File content/browser/renderer_host/render_widget_host_view_base.h

Line 606, Patchset 7 (Latest): base::WeakPtr<RenderWidgetHostViewBase> view,

Dave Tapuska . unresolved

I'd prefer if was passed as a RenderWidgetHostViewBase* and then the WeakPtr was taken inside where the storage happened. (GetWeakPtr() is available as a public on RenderWidgetHostViewBase from my understanding).

Can we name this StartStylusWritingFromChildHostView(RenderWidgetHostViewBase* child_view...);

Open in Gerrit

Related details

Attention is currently required from:

Gaston Rodriguez
John An
Nasko Oskov
Olga Gerchikov

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

No-Unresolved-Comments

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Dave Tapuska (Gerrit)

unread,

Jun 25, 2026, 12:39:43 PM (4 days ago) Jun 25

to John An, Jonathan Ross, Olga Gerchikov, Gaston Rodriguez, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, Zhe Su, alexmo...@chromium.org, creis...@chromium.org, dtapuska+ch...@chromium.org, jbauma...@chromium.org, keithle...@chromium.org, navigation...@chromium.org, nona+...@chromium.org, shuche...@chromium.org, tranbaod...@chromium.org, yhanad...@chromium.org

Attention needed from Gaston Rodriguez, John An, Nasko Oskov and Olga Gerchikov

Dave Tapuska added 1 comment

File content/browser/renderer_host/render_widget_host_view_child_frame.cc

Line 508, Patchset 7 (Latest): StylusHandwritingControllerWin::GetInstance()->OnFocusFailed();

Dave Tapuska . unresolved

Would it be useful to pass back the RenderWidgetHostViewBase on the FocusFailed call?

satisfied_requirement

unsatisfied_requirement

open

diffy

Nasko Oskov (Gerrit)

unread,

Jun 25, 2026, 7:26:13 PM (4 days ago) Jun 25

to John An, Jonathan Ross, Dave Tapuska, Olga Gerchikov, Gaston Rodriguez, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, Zhe Su, alexmo...@chromium.org, creis...@chromium.org, dtapuska+ch...@chromium.org, jbauma...@chromium.org, keithle...@chromium.org, navigation...@chromium.org, nona+...@chromium.org, shuche...@chromium.org, tranbaod...@chromium.org, yhanad...@chromium.org

Attention needed from Gaston Rodriguez, John An and Olga Gerchikov

Nasko Oskov added 1 comment

Patchset-level comments

File-level comment, Patchset 7 (Latest):

Nasko Oskov . resolved

dtapuska@ is the right owner for this. You don't need me, especially for input related code.

Open in Gerrit

Related details

Attention is currently required from:

Gaston Rodriguez
John An
Olga Gerchikov

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

John An (Gerrit)

unread,

Jun 25, 2026, 8:38:07 PM (4 days ago) Jun 25

to Jonathan Ross, Dave Tapuska, Olga Gerchikov, Gaston Rodriguez, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, Zhe Su, alexmo...@chromium.org, creis...@chromium.org, dtapuska+ch...@chromium.org, jbauma...@chromium.org, keithle...@chromium.org, navigation...@chromium.org, nona+...@chromium.org, shuche...@chromium.org, tranbaod...@chromium.org, yhanad...@chromium.org

Attention needed from Dave Tapuska, Gaston Rodriguez and Olga Gerchikov

John An added 3 comments

File content/browser/renderer_host/render_widget_host_view_aura.cc

Line 2852, Patchset 7:#if BUILDFLAG(IS_WIN)

Dave Tapuska . resolved

Why is this replicated both in child frame and aura, wouldn't this just go in base?

John An

Had it this way because controller mgmt logic is located in those files. It's not a big deal to me to put it in the base so I can do that as well.

File content/browser/renderer_host/render_widget_host_view_base.h

Line 606, Patchset 7: base::WeakPtr<RenderWidgetHostViewBase> view,

Dave Tapuska . resolved

I'd prefer if was passed as a RenderWidgetHostViewBase* and then the WeakPtr was taken inside where the storage happened. (GetWeakPtr() is available as a public on RenderWidgetHostViewBase from my understanding).
Can we name this StartStylusWritingFromChildHostView(RenderWidgetHostViewBase* child_view...);

John An

Sure that sounds good

File content/browser/renderer_host/render_widget_host_view_child_frame.cc

Line 508, Patchset 7: StylusHandwritingControllerWin::GetInstance()->OnFocusFailed();

Dave Tapuska . resolved

Would it be useful to pass back the RenderWidgetHostViewBase on the FocusFailed call?

John An

There's no need to validate the view that is invoking OnFocusFailed/Handled. If pending_target_args_ is set, then the check in OnHandwritingViewDestroyed will prevent an unrelated view from causing the session to fail. If it is not set, the methods will no-op.

Open in Gerrit

Related details

Attention is currently required from:

Dave Tapuska
Gaston Rodriguez
Olga Gerchikov

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Dave Tapuska (Gerrit)

unread,

Jun 26, 2026, 11:29:10 AM (3 days ago) Jun 26

to John An, Jonathan Ross, Olga Gerchikov, Gaston Rodriguez, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, Zhe Su, alexmo...@chromium.org, creis...@chromium.org, dtapuska+ch...@chromium.org, jbauma...@chromium.org, keithle...@chromium.org, navigation...@chromium.org, nona+...@chromium.org, shuche...@chromium.org, tranbaod...@chromium.org, yhanad...@chromium.org

Attention needed from Gaston Rodriguez, John An and Olga Gerchikov

Dave Tapuska voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Gaston Rodriguez
John An
Olga Gerchikov

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Reply all

Reply to author

Forward

Improving Stylus OnFocusFailed coverage [chromium/src : main]

John An (Gerrit)

John An added 1 comment

Related details

Gaston Rodriguez (Gerrit)

Gaston Rodriguez added 2 comments

Related details

Olga Gerchikov (Gerrit)

Olga Gerchikov added 1 comment

Related details

John An (Gerrit)

John An added 2 comments

Related details

John An (Gerrit)

John An added 1 comment

Related details

Olga Gerchikov (Gerrit)

Olga Gerchikov voted Code-Review+1

Related details

John An (Gerrit)

John An added 1 comment

Related details

Jonathan Ross (Gerrit)

Jonathan Ross added 1 comment

Related details

John An (Gerrit)

John An added 1 comment

Related details

John An (Gerrit)

John An added 1 comment

Related details

Dave Tapuska (Gerrit)

Dave Tapuska added 2 comments

Related details

Dave Tapuska (Gerrit)

Dave Tapuska added 1 comment

Nasko Oskov (Gerrit)

Nasko Oskov added 1 comment

Related details

John An (Gerrit)

John An added 3 comments

Related details

Dave Tapuska (Gerrit)

Dave Tapuska voted Code-Review+1

Related details