New issue
Advanced search Search tips

Issue 602723 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 24
Cc:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 3
Type: Bug



Sign in to add a comment

Need tests for InputMethod-RenderWidgetHostView Interaction

Project Member Reported by ekaramad@chromium.org, Apr 12 2016

Issue description

The initial fix for  Issue 578168  disabled text input state updates for RenderWidgetHostViewMac and caused issues such as  Issue 601738  (IME was not working on Mac OSX). We should have tests spotting errors on this scale.
 
Summary: Need tests for InputMethod-RenderWidgetHostView Interaction (was: Need some tests for InputMethod-RenderWidgetHostViewMac interaction)
 Issue 602488  is also caused due to the fix for  Issue 578168  and lack of testing.
Adding a note to this CL
https://codereview.chromium.org/1894473002/

Can we have a test to prevent this? The issue was due to use of uninitialized bool in C++ leading to crashes in JNI conversion for Android.
Status: Assigned (was: Available)
Project Member

Comment 4 by bugdroid1@chromium.org, Jun 8 2016

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

commit add8822927e21f8f2d10e9257e7c3158fd02f223
Author: ekaramad <ekaramad@chromium.org>
Date: Wed Jun 08 15:22:56 2016

[reland] Browser Side Text Input State Tracking for OOPIF (Aura Only)

This CL relands the original issue https://codereview.chromium.org/1652483002/,
with modifications and for aura only (Linux, Windows, etc.). T

A TextInputManager is introduced which owned by the RenderWidgetHostDelegate
(mostly, WebContentsImpl). Each RWHV will observe the TextInputManager for its destruction
and lifetime while the TextInputManager observers all the RWHVs on the tab.

The role of the TextInputManager is to bookkeep and store TextInputState from all RWHV and
provide the correct TextInputState and the RWHV with active TextInputState at anytime. These
information are used by the tab-RWHV in handling IME related tasks.

This CL also adds several tests and some utility methods to help testing IME and TextInputState
for OOPIF.

BUG= 578168 ,  602723 
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/1948343002
Cr-Commit-Position: refs/heads/master@{#398559}

[add] https://crrev.com/add8822927e21f8f2d10e9257e7c3158fd02f223/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
[modify] https://crrev.com/add8822927e21f8f2d10e9257e7c3158fd02f223/chrome/test/BUILD.gn
[modify] https://crrev.com/add8822927e21f8f2d10e9257e7c3158fd02f223/content/browser/frame_host/interstitial_page_impl.cc
[modify] https://crrev.com/add8822927e21f8f2d10e9257e7c3158fd02f223/content/browser/frame_host/interstitial_page_impl.h
[modify] https://crrev.com/add8822927e21f8f2d10e9257e7c3158fd02f223/content/browser/frame_host/render_widget_host_view_child_frame.cc
[modify] https://crrev.com/add8822927e21f8f2d10e9257e7c3158fd02f223/content/browser/frame_host/render_widget_host_view_child_frame.h
[modify] https://crrev.com/add8822927e21f8f2d10e9257e7c3158fd02f223/content/browser/renderer_host/render_widget_host_delegate.cc
[modify] https://crrev.com/add8822927e21f8f2d10e9257e7c3158fd02f223/content/browser/renderer_host/render_widget_host_delegate.h
[modify] https://crrev.com/add8822927e21f8f2d10e9257e7c3158fd02f223/content/browser/renderer_host/render_widget_host_view_aura.cc
[modify] https://crrev.com/add8822927e21f8f2d10e9257e7c3158fd02f223/content/browser/renderer_host/render_widget_host_view_aura.h
[modify] https://crrev.com/add8822927e21f8f2d10e9257e7c3158fd02f223/content/browser/renderer_host/render_widget_host_view_base.cc
[modify] https://crrev.com/add8822927e21f8f2d10e9257e7c3158fd02f223/content/browser/renderer_host/render_widget_host_view_base.h
[add] https://crrev.com/add8822927e21f8f2d10e9257e7c3158fd02f223/content/browser/renderer_host/text_input_manager.cc
[add] https://crrev.com/add8822927e21f8f2d10e9257e7c3158fd02f223/content/browser/renderer_host/text_input_manager.h
[modify] https://crrev.com/add8822927e21f8f2d10e9257e7c3158fd02f223/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/add8822927e21f8f2d10e9257e7c3158fd02f223/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/add8822927e21f8f2d10e9257e7c3158fd02f223/content/content_browser.gypi
[modify] https://crrev.com/add8822927e21f8f2d10e9257e7c3158fd02f223/content/content_tests.gypi
[add] https://crrev.com/add8822927e21f8f2d10e9257e7c3158fd02f223/content/public/test/text_input_test_utils.cc
[add] https://crrev.com/add8822927e21f8f2d10e9257e7c3158fd02f223/content/public/test/text_input_test_utils.h

Manually adding a CL related to this bug which missed to include the bug number when they were landed:

Confirm ongoing IME composition at the right render widget in response to mouse click/tap.

After CL: https://codereview.chromium.org/2092103002/ all IME result methods are
being routed to the right RenderWidget. Similarly, when an IME session is closed
on the browser side due to a click or tap, the right widget should be notifed
by receiving an IME confirm composition with empty text. Currently, this IPC is
sent to the tab's RenderWidget regardless of what the active widget is.

This CL will route the IPC to the right widget and adds a unit test for this purpose.

BUG= 578168 

Review-Url: https://codereview.chromium.org/2116593002
Cr-Commit-Position: refs/heads/master@{#403273}
Manually adding a CL related to this bug which missed to include the bug number when they were landed:

[reland] Routing IME Result Calls to the Correct RenderWidgetHost (Aura Only)

This is a reland of the original CL: https://codereview.chromium.org/2045363002/ which was
reverted by this CL https://codereview.chromium.org/2095813002. The reason for the revert
was the regression of several SiteInstanceTests on Dr Memory bots.

The cause of the issue was that the newly introduced content unit tests in the original CL
were not deleting a RenderWidgetHostImpl which was keeping a RenderProcessHost alive. This
was interfering with the SiteInstanceTests which have explicit asserts on the number of
RenderProceessHosts alive.

BUG= 578168 ,  622793 ,  602723 
Committed: https://crrev.com/8cba7886c3e170ca2dc324dbbd31b28dae623887
Cr-Commit-Position: refs/heads/master@{#401939}
Project Member

Comment 7 by bugdroid1@chromium.org, Jul 7 2016

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

commit fcce0888975a8ad65fbeeeec78e52f2246b86020
Author: ekaramad <ekaramad@chromium.org>
Date: Thu Jul 07 02:44:51 2016

Tracking SelectionBounds for all RenderWidgets on the browser side.

This patch implements the RenderWidgetHostViewBase::SelectionBoundsChanged as well as
RenderWidgetHostViewChildFrame::SelectionBoundsChanged and moves the logic inside the
RenderWidgetHostViewAura::SelectionBoundsChanged to the TextInputManager owned by the
WebContentsImpl. This patch also adds a test to verify that TextInputManager tracks
all the RWHVs selection bounds.

The same implementation will be required for Mac; but perhaps it requires slight changes
on the logic to store the state from ViewHostMsg_SelectionBounds_Params.

BUG= 578168 ,  602723 
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2057803002
Cr-Commit-Position: refs/heads/master@{#404043}

[modify] https://crrev.com/fcce0888975a8ad65fbeeeec78e52f2246b86020/chrome/browser/renderer_host/OWNERS
[modify] https://crrev.com/fcce0888975a8ad65fbeeeec78e52f2246b86020/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
[modify] https://crrev.com/fcce0888975a8ad65fbeeeec78e52f2246b86020/content/browser/frame_host/render_widget_host_view_child_frame.cc
[modify] https://crrev.com/fcce0888975a8ad65fbeeeec78e52f2246b86020/content/browser/frame_host/render_widget_host_view_child_frame.h
[modify] https://crrev.com/fcce0888975a8ad65fbeeeec78e52f2246b86020/content/browser/renderer_host/render_widget_host_view_aura.cc
[modify] https://crrev.com/fcce0888975a8ad65fbeeeec78e52f2246b86020/content/browser/renderer_host/render_widget_host_view_aura.h
[modify] https://crrev.com/fcce0888975a8ad65fbeeeec78e52f2246b86020/content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
[modify] https://crrev.com/fcce0888975a8ad65fbeeeec78e52f2246b86020/content/browser/renderer_host/render_widget_host_view_base.cc
[modify] https://crrev.com/fcce0888975a8ad65fbeeeec78e52f2246b86020/content/browser/renderer_host/render_widget_host_view_base.h
[modify] https://crrev.com/fcce0888975a8ad65fbeeeec78e52f2246b86020/content/browser/renderer_host/text_input_manager.cc
[modify] https://crrev.com/fcce0888975a8ad65fbeeeec78e52f2246b86020/content/browser/renderer_host/text_input_manager.h
[modify] https://crrev.com/fcce0888975a8ad65fbeeeec78e52f2246b86020/content/public/test/text_input_test_utils.cc
[modify] https://crrev.com/fcce0888975a8ad65fbeeeec78e52f2246b86020/content/public/test/text_input_test_utils.h
[modify] https://crrev.com/fcce0888975a8ad65fbeeeec78e52f2246b86020/content/test/test_render_view_host.h

Project Member

Comment 8 by bugdroid1@chromium.org, Jul 13 2016

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

commit b8e23a96c9c1d2933c57ba99aeac3199845fba99
Author: ekaramad <ekaramad@chromium.org>
Date: Wed Jul 13 01:21:15 2016

Tracking composition range on the browser side (Aura)

Currently, the composition range information which is sent by RenderWidget in the IPC
InputHostMsg_ImeCompositionRangeChanged is not being used when the source is a child frame.
This CL modifies the RenderWidgetHostViewBase class to report the composition range to the
TextInputManager which aggregates all such information from sub frames.

The CL adds a unit test to verify that the InputMethod uses the correct composition range
values for the active widget. A TODO is also left in site_per_process_text_input_browsertest.cc
to come back and write an interactive test for tracking composition range for sub frames
after the renderer side of IME is fixed.

BUG= 578168 ,  602723 
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2132633002
Cr-Commit-Position: refs/heads/master@{#404873}

[modify] https://crrev.com/b8e23a96c9c1d2933c57ba99aeac3199845fba99/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
[modify] https://crrev.com/b8e23a96c9c1d2933c57ba99aeac3199845fba99/content/browser/frame_host/render_widget_host_view_child_frame.cc
[modify] https://crrev.com/b8e23a96c9c1d2933c57ba99aeac3199845fba99/content/browser/frame_host/render_widget_host_view_child_frame.h
[modify] https://crrev.com/b8e23a96c9c1d2933c57ba99aeac3199845fba99/content/browser/renderer_host/render_widget_host_view_aura.cc
[modify] https://crrev.com/b8e23a96c9c1d2933c57ba99aeac3199845fba99/content/browser/renderer_host/render_widget_host_view_aura.h
[modify] https://crrev.com/b8e23a96c9c1d2933c57ba99aeac3199845fba99/content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
[modify] https://crrev.com/b8e23a96c9c1d2933c57ba99aeac3199845fba99/content/browser/renderer_host/render_widget_host_view_base.cc
[modify] https://crrev.com/b8e23a96c9c1d2933c57ba99aeac3199845fba99/content/browser/renderer_host/render_widget_host_view_base.h
[modify] https://crrev.com/b8e23a96c9c1d2933c57ba99aeac3199845fba99/content/browser/renderer_host/text_input_manager.cc
[modify] https://crrev.com/b8e23a96c9c1d2933c57ba99aeac3199845fba99/content/browser/renderer_host/text_input_manager.h
[modify] https://crrev.com/b8e23a96c9c1d2933c57ba99aeac3199845fba99/content/public/test/text_input_test_utils.cc
[modify] https://crrev.com/b8e23a96c9c1d2933c57ba99aeac3199845fba99/content/public/test/text_input_test_utils.h
[modify] https://crrev.com/b8e23a96c9c1d2933c57ba99aeac3199845fba99/content/test/test_render_view_host.h

Project Member

Comment 9 by bugdroid1@chromium.org, Jul 21 2016

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

commit 6f1786b887a51f3fcd3cdeacfd9b455934c570ca
Author: ekaramad <ekaramad@chromium.org>
Date: Thu Jul 21 21:10:58 2016

Tracking text selection on the browser side in OOPIF.

This CL adds the required logic to track the text selection information
at the browser side. The text selection information is now stored in the
WebContent's TextInputManager (outermost WebContents). All
RenderWidgetHostViews as well as RenderWidgetHostViewAura
(TextInputClient) will retrieve the information for a specific view or
the active view from TextInputManager.

BUG= 578168 ,  602723 
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2130133004
Cr-Commit-Position: refs/heads/master@{#406937}

[modify] https://crrev.com/6f1786b887a51f3fcd3cdeacfd9b455934c570ca/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
[modify] https://crrev.com/6f1786b887a51f3fcd3cdeacfd9b455934c570ca/content/browser/frame_host/render_widget_host_view_child_frame.cc
[modify] https://crrev.com/6f1786b887a51f3fcd3cdeacfd9b455934c570ca/content/browser/frame_host/render_widget_host_view_child_frame.h
[modify] https://crrev.com/6f1786b887a51f3fcd3cdeacfd9b455934c570ca/content/browser/renderer_host/render_widget_host_view_aura.cc
[modify] https://crrev.com/6f1786b887a51f3fcd3cdeacfd9b455934c570ca/content/browser/renderer_host/render_widget_host_view_aura.h
[modify] https://crrev.com/6f1786b887a51f3fcd3cdeacfd9b455934c570ca/content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
[modify] https://crrev.com/6f1786b887a51f3fcd3cdeacfd9b455934c570ca/content/browser/renderer_host/render_widget_host_view_base.cc
[modify] https://crrev.com/6f1786b887a51f3fcd3cdeacfd9b455934c570ca/content/browser/renderer_host/render_widget_host_view_base.h
[modify] https://crrev.com/6f1786b887a51f3fcd3cdeacfd9b455934c570ca/content/browser/renderer_host/text_input_manager.cc
[modify] https://crrev.com/6f1786b887a51f3fcd3cdeacfd9b455934c570ca/content/browser/renderer_host/text_input_manager.h
[modify] https://crrev.com/6f1786b887a51f3fcd3cdeacfd9b455934c570ca/content/public/test/text_input_test_utils.cc
[modify] https://crrev.com/6f1786b887a51f3fcd3cdeacfd9b455934c570ca/content/public/test/text_input_test_utils.h

Project Member

Comment 10 by bugdroid1@chromium.org, Jul 27 2016

Project Member

Comment 11 by bugdroid1@chromium.org, Jul 27 2016

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

commit 44c242985f1c9f81dc56e7d5122776250b1f1a66
Author: ekaramad <ekaramad@chromium.org>
Date: Wed Jul 27 21:17:29 2016

In TextInputManager, reset input type to none before switching active widgets.

When there are no OOPIFs in a page, if we switch focus from one <input>
into another (i.e., through a mouse click), the TextInputState will be
first set to NONE and then back to TEXT.

This however does not always happen when there are OOPIFs since the two IPCs might
arrive from different RenderWidgets and the order of arrival is not
deterministic (e.g., the IPC to set to TEXT could arrive sooner).

Although TextInputManager tracks the state and active widget correctly
(regardless of what order the IPCs arrive from the RenderWidgets), it
still does not mimic the exact same behavior as in the single
RenderWidget case. In Aura platforms, without setting the
TextInputState to none before changing the active widget, the IME
behavior might go wrong and InputMethod behave out of sync with the
actual TextInputState. The reason is that the InputMethod would believe
that the TextInputClient is the same but the IME session has ended.

This CL will try to simulate the same behavior for all scenarios by
injecting a TextInputState of none in between an active widget change.

BUG= 626171 , 578168 , 602723 

Review-Url: https://codereview.chromium.org/2171443003
Cr-Commit-Position: refs/heads/master@{#408239}

[modify] https://crrev.com/44c242985f1c9f81dc56e7d5122776250b1f1a66/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
[modify] https://crrev.com/44c242985f1c9f81dc56e7d5122776250b1f1a66/content/browser/renderer_host/text_input_manager.cc

Project Member

Comment 12 by bugdroid1@chromium.org, Aug 9 2016

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

commit af4343904a4f27311c19dcb28249937cc3c10d17
Author: ekaramad <ekaramad@chromium.org>
Date: Tue Aug 09 20:46:11 2016

Change the test "TrackTextSelectionForAllFrames" to actually run on an OOPIF page.

This CL changes the test a bit so that it will run on a test page with
several <iframe>s ([out-of|in]-process).

BUG= 578168 , 602723 

using select() to simulate the chagne

Review-Url: https://codereview.chromium.org/2230543002
Cr-Commit-Position: refs/heads/master@{#410808}

[modify] https://crrev.com/af4343904a4f27311c19dcb28249937cc3c10d17/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc

Status: Started (was: Assigned)
Project Member

Comment 15 by bugdroid1@chromium.org, Aug 11 2016

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

commit 56a714e089de70102b0308f82bfe9b181b891022
Author: ekaramad <ekaramad@chromium.org>
Date: Thu Aug 11 06:47:10 2016

Request to start/stop calculating composition info from RenderWidget when it is active/inactive.

This CL sends the request to start/stop updating composition info to the
widget which became active/inactive, rather than sending it only to the
top level's render widget.

This CL also revises the test TrackCompositionRangeForAllFrames so that
it addresses OOPIFs.

BUG= 578168 , 602723 , 624714 

Review-Url: https://codereview.chromium.org/2208583005
Cr-Commit-Position: refs/heads/master@{#411278}

[modify] https://crrev.com/56a714e089de70102b0308f82bfe9b181b891022/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
[modify] https://crrev.com/56a714e089de70102b0308f82bfe9b181b891022/content/browser/renderer_host/render_widget_host_view_aura.cc
[modify] https://crrev.com/56a714e089de70102b0308f82bfe9b181b891022/content/browser/renderer_host/render_widget_host_view_aura.h
[modify] https://crrev.com/56a714e089de70102b0308f82bfe9b181b891022/content/public/test/text_input_test_utils.cc
[modify] https://crrev.com/56a714e089de70102b0308f82bfe9b181b891022/content/public/test/text_input_test_utils.h

Project Member

Comment 16 by bugdroid1@chromium.org, Aug 12 2016

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

commit fd5f5a89a6a3cc76fade71f9c32e4be1801d95ea
Author: ekaramad <ekaramad@chromium.org>
Date: Fri Aug 12 04:33:10 2016

Route IME-related IPCs to the active/focused RenderWidget (Mac)

This CL will route IPCs resulted from IME-related methods in RenderWidgetHostCocoaView to the correct (focused) RenderWidget. The CL also adds unit tests for each method modified.

Modified methods:
*  confirmComposition (InputMsg_ImeConfirmComposition)
*  insertText (InputMsg_ImeConfirmComposition)
*  setMarkedText (InputMsg_ImeSetComposition)
*  unmarkText (InputMsg_ImeConfirmComposition)

BUG= 578168 ,  602723 

Review-Url: https://codereview.chromium.org/2229223002
Cr-Commit-Position: refs/heads/master@{#411552}

[modify] https://crrev.com/fd5f5a89a6a3cc76fade71f9c32e4be1801d95ea/content/browser/renderer_host/render_widget_host_view_base.h
[modify] https://crrev.com/fd5f5a89a6a3cc76fade71f9c32e4be1801d95ea/content/browser/renderer_host/render_widget_host_view_mac.h
[modify] https://crrev.com/fd5f5a89a6a3cc76fade71f9c32e4be1801d95ea/content/browser/renderer_host/render_widget_host_view_mac.mm
[modify] https://crrev.com/fd5f5a89a6a3cc76fade71f9c32e4be1801d95ea/content/browser/renderer_host/render_widget_host_view_mac_unittest.mm
[modify] https://crrev.com/fd5f5a89a6a3cc76fade71f9c32e4be1801d95ea/content/browser/renderer_host/text_input_manager.cc
[modify] https://crrev.com/fd5f5a89a6a3cc76fade71f9c32e4be1801d95ea/content/browser/renderer_host/text_input_manager.h

Project Member

Comment 17 by bugdroid1@chromium.org, Aug 12 2016

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

commit d773ff47652910acdd309eca657aec6133d513b1
Author: ekaramad <ekaramad@chromium.org>
Date: Fri Aug 12 19:30:40 2016

Track composition range and character bounds on the browser side (Mac)

This CL will route updates in composition range through TextInputManager on Mac.
This is similar to how the tracking works in Aura. The corresponding test,
'TrackCompositionRangeForAllFrames' is also activated on Mac.

BUG= 578168 ,  602723 

Review-Url: https://codereview.chromium.org/2235283003
Cr-Commit-Position: refs/heads/master@{#411732}

[modify] https://crrev.com/d773ff47652910acdd309eca657aec6133d513b1/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
[modify] https://crrev.com/d773ff47652910acdd309eca657aec6133d513b1/content/browser/renderer_host/render_widget_host_view_aura.cc
[modify] https://crrev.com/d773ff47652910acdd309eca657aec6133d513b1/content/browser/renderer_host/render_widget_host_view_base.cc
[modify] https://crrev.com/d773ff47652910acdd309eca657aec6133d513b1/content/browser/renderer_host/render_widget_host_view_mac.h
[modify] https://crrev.com/d773ff47652910acdd309eca657aec6133d513b1/content/browser/renderer_host/render_widget_host_view_mac.mm
[modify] https://crrev.com/d773ff47652910acdd309eca657aec6133d513b1/content/browser/renderer_host/render_widget_host_view_mac_unittest.mm
[modify] https://crrev.com/d773ff47652910acdd309eca657aec6133d513b1/content/browser/renderer_host/text_input_manager.cc
[modify] https://crrev.com/d773ff47652910acdd309eca657aec6133d513b1/content/browser/renderer_host/text_input_manager.h
[modify] https://crrev.com/d773ff47652910acdd309eca657aec6133d513b1/content/public/test/text_input_test_utils.cc

Project Member

Comment 18 by bugdroid1@chromium.org, Aug 18 2016

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

commit 65cf5593f03c43ccaa8f97b023909040934d9ff5
Author: ekaramad <ekaramad@chromium.org>
Date: Thu Aug 18 21:44:53 2016

Track text selection on the browser side (Mac)

This CL will mimic the code for aura in using TextInputManager for tracking the
text selection information on the browser side. The CL also enables the corresponding
ui test in Mac.

BUG= 578168 , 602723 

Review-Url: https://codereview.chromium.org/2240553003
Cr-Commit-Position: refs/heads/master@{#412938}

[modify] https://crrev.com/65cf5593f03c43ccaa8f97b023909040934d9ff5/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
[modify] https://crrev.com/65cf5593f03c43ccaa8f97b023909040934d9ff5/content/browser/renderer_host/render_widget_host_view_aura.cc
[modify] https://crrev.com/65cf5593f03c43ccaa8f97b023909040934d9ff5/content/browser/renderer_host/render_widget_host_view_base.cc
[modify] https://crrev.com/65cf5593f03c43ccaa8f97b023909040934d9ff5/content/browser/renderer_host/render_widget_host_view_base.h
[modify] https://crrev.com/65cf5593f03c43ccaa8f97b023909040934d9ff5/content/browser/renderer_host/render_widget_host_view_mac.h
[modify] https://crrev.com/65cf5593f03c43ccaa8f97b023909040934d9ff5/content/browser/renderer_host/render_widget_host_view_mac.mm
[modify] https://crrev.com/65cf5593f03c43ccaa8f97b023909040934d9ff5/content/browser/renderer_host/render_widget_host_view_mac_unittest.mm
[modify] https://crrev.com/65cf5593f03c43ccaa8f97b023909040934d9ff5/content/browser/renderer_host/text_input_manager.cc
[modify] https://crrev.com/65cf5593f03c43ccaa8f97b023909040934d9ff5/content/browser/renderer_host/text_input_manager.h

Project Member

Comment 19 by bugdroid1@chromium.org, Aug 22 2016

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

commit 2f52009f42519eac3e40ff20718e31365c976773
Author: ekaramad <ekaramad@chromium.org>
Date: Mon Aug 22 23:10:24 2016

Tracking SelectionBounds for all RenderWidgets on the Browser Side (Mac)

This CL uses the TextInputManager code path for tracking selection bounds on Mac OSX (a follow up to https://codereview.chromium.org/2057803002 for aura).
Other important changes include:
  * Activating the test SitePerProcessTextInputManagerTest.TrackSelectionBoundsForAllFrames on Mac OSX.
  * Removing all the overrides of SelectionBoundsChanged form Mac and Android.

BUG= 578168 , 602723 , 602427 

Review-Url: https://codereview.chromium.org/2213503002
Cr-Commit-Position: refs/heads/master@{#413568}

[modify] https://crrev.com/2f52009f42519eac3e40ff20718e31365c976773/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
[modify] https://crrev.com/2f52009f42519eac3e40ff20718e31365c976773/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/2f52009f42519eac3e40ff20718e31365c976773/content/browser/renderer_host/render_widget_host_view_android.h
[modify] https://crrev.com/2f52009f42519eac3e40ff20718e31365c976773/content/browser/renderer_host/render_widget_host_view_aura.cc
[modify] https://crrev.com/2f52009f42519eac3e40ff20718e31365c976773/content/browser/renderer_host/render_widget_host_view_base.cc
[modify] https://crrev.com/2f52009f42519eac3e40ff20718e31365c976773/content/browser/renderer_host/render_widget_host_view_mac.h
[modify] https://crrev.com/2f52009f42519eac3e40ff20718e31365c976773/content/browser/renderer_host/render_widget_host_view_mac.mm
[modify] https://crrev.com/2f52009f42519eac3e40ff20718e31365c976773/content/browser/renderer_host/render_widget_host_view_mac_unittest.mm
[modify] https://crrev.com/2f52009f42519eac3e40ff20718e31365c976773/content/browser/renderer_host/text_input_manager.cc
[modify] https://crrev.com/2f52009f42519eac3e40ff20718e31365c976773/content/browser/renderer_host/text_input_manager.h

Project Member

Comment 20 by bugdroid1@chromium.org, Aug 23 2016

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

commit 9135773062a5f13e51348454c682d6661447c887
Author: ekaramad <ekaramad@chromium.org>
Date: Tue Aug 23 18:45:29 2016

TextInputManager::GetTextSelection() should be called on focused widget.

Currently, in the following methods in RenderWidgetHostViewAura, we use
the active widget, i.e., the widget with focused text field to get text
selection information:
GetTextRange()
GetSelectionRange()
GetTextFromRange()

Using the active widget as opposed to the focused widget regressed the
long press touch quick menu on Aura. This is due to the call to
GetSelectionRange() which returns empty when the selection is on a
non-editable text. Using the focused widget is correct and fixes the
issue.

The corresponding unit tests are also modified so that we now test the
selection on "focused widget" as opposed to "active widget".

BUG= 638898 ,  602723 ,  578168 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2255363002
Cr-Commit-Position: refs/heads/master@{#413795}

[modify] https://crrev.com/9135773062a5f13e51348454c682d6661447c887/content/browser/frame_host/render_widget_host_view_child_frame.cc
[modify] https://crrev.com/9135773062a5f13e51348454c682d6661447c887/content/browser/renderer_host/render_widget_host_view_aura.cc
[modify] https://crrev.com/9135773062a5f13e51348454c682d6661447c887/content/browser/renderer_host/render_widget_host_view_aura_unittest.cc

Project Member

Comment 21 by bugdroid1@chromium.org, Aug 24 2016

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

commit 82b2adfb3119d1bad8322ce0ff0d81d62ea6ca5b
Author: ekaramad <ekaramad@chromium.org>
Date: Wed Aug 24 01:08:01 2016

Activate a TextInputManager test on Mac OSX.

TextInputManager now collects IME and TextInputState for OOPIF on Mac OSX.
The enabled test now passes on Mac.

BUG= 578168 , 602723 

Review-Url: https://codereview.chromium.org/2269783004
Cr-Commit-Position: refs/heads/master@{#413920}

[modify] https://crrev.com/82b2adfb3119d1bad8322ce0ff0d81d62ea6ca5b/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc

Project Member

Comment 22 by bugdroid1@chromium.org, Nov 28 2016

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

commit 06b277144e9e48f754e82ec5224076eeeac347b5
Author: ekaramad <ekaramad@chromium.org>
Date: Mon Nov 28 17:55:10 2016

Browser Side TextInputState Tracking for Android

To enable OOPIF and Top Document Isolation for Android, we need to track
the text input state from multiple RenderWidgetHosts. To this end, the
TextInputStateChanged updates should be routed through TextInputManager
which keeps a map of all RenderWidgetHost(View) and their corresponding
TextInputState.

This CL will also enable some site-per-process tests on android.

BUG= 578168 , 602723 

Review-Url: https://codereview.chromium.org/2354793003
Cr-Commit-Position: refs/heads/master@{#434683}

[modify] https://crrev.com/06b277144e9e48f754e82ec5224076eeeac347b5/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
[modify] https://crrev.com/06b277144e9e48f754e82ec5224076eeeac347b5/chrome/test/BUILD.gn
[modify] https://crrev.com/06b277144e9e48f754e82ec5224076eeeac347b5/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/06b277144e9e48f754e82ec5224076eeeac347b5/content/browser/renderer_host/render_widget_host_view_android.h
[modify] https://crrev.com/06b277144e9e48f754e82ec5224076eeeac347b5/content/browser/renderer_host/render_widget_host_view_base.cc
[modify] https://crrev.com/06b277144e9e48f754e82ec5224076eeeac347b5/content/browser/renderer_host/text_input_manager.cc
[modify] https://crrev.com/06b277144e9e48f754e82ec5224076eeeac347b5/content/browser/renderer_host/text_input_manager.h

Project Member

Comment 23 by bugdroid1@chromium.org, Nov 30 2016

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

commit db49e17a8e2c5874585e5a499171163ce70104ec
Author: ekaramad <ekaramad@chromium.org>
Date: Wed Nov 30 08:44:40 2016

Fix a crash occuring during the destruction of TextInputManager on Android.

On Android, in the observer call
RenderWidgetHostViewAndroid::OnUpdateTextInputState we were using
|updated_view| passed by TextInputManager to query TextInputState. This
is correct as long as the view is not unregistered already, which
happens when
  1- If the |active_view_| is destroyed.
  2- If TextInputManager is destoryed while there is |active_view_|
  which will lead to unregistering the view.

This patch will change the way we obtain TextInputState in
RenderWidgetHostViewAndroid. We now first verify if there is an active
view/widget and then get the state. Otherwise, the default state (of
NONE) is reported.

This patch also adds a test to catch the regression.

BUG=669375,  602723 ,  578168 

Review-Url: https://codereview.chromium.org/2536943004
Cr-Commit-Position: refs/heads/master@{#435178}

[modify] https://crrev.com/db49e17a8e2c5874585e5a499171163ce70104ec/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
[modify] https://crrev.com/db49e17a8e2c5874585e5a499171163ce70104ec/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/db49e17a8e2c5874585e5a499171163ce70104ec/content/browser/renderer_host/text_input_manager.cc
[modify] https://crrev.com/db49e17a8e2c5874585e5a499171163ce70104ec/content/browser/renderer_host/text_input_manager.h

Project Member

Comment 24 by bugdroid1@chromium.org, Jan 13 2017

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

commit 5f6b70eb67a2f1633ca5fcaa04374f7abd8c000c
Author: ekaramad <ekaramad@chromium.org>
Date: Fri Jan 13 16:43:15 2017

Track composition range information on the browser side (Android)

This CL implements the logic to track composition range information
from several RenderWidgets on the same page (i.e., pages with OOPIFs) for Android platforms.

The tracking logic is now shared between RenderWidgetHostViewBase and TextInputManager,
while RenderWidgetHostViewAndroid observes TextInputManager for any
changes to the range info in one of the RenderWidgets in the page.
This has already been implemented for Mac
(https://codereview.chromium.org/2235283003/) and Aura
(https://codereview.chromium.org/2132633002/) platforms.

An interactive test is now activated on all platform which will be
handy if/when interactive browser tests run on Android.

BUG= 578168 ,  602723 

Review-Url: https://codereview.chromium.org/2612423002
Cr-Commit-Position: refs/heads/master@{#443577}

[modify] https://crrev.com/5f6b70eb67a2f1633ca5fcaa04374f7abd8c000c/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
[modify] https://crrev.com/5f6b70eb67a2f1633ca5fcaa04374f7abd8c000c/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/5f6b70eb67a2f1633ca5fcaa04374f7abd8c000c/content/browser/renderer_host/render_widget_host_view_android.h
[modify] https://crrev.com/5f6b70eb67a2f1633ca5fcaa04374f7abd8c000c/content/browser/renderer_host/render_widget_host_view_base.cc

Project Member

Comment 25 by bugdroid1@chromium.org, Jan 17 2017

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

commit df66adcb39e831a54a35a795baf7267c9a3925f5
Author: ekaramad <ekaramad@chromium.org>
Date: Tue Jan 17 23:07:38 2017

Adding a unit test to RenderWidgetHostViewAura

This CL adds a test similar to
InputMethodMacTest.ImeCancelCompositionForAllViews for Aura.

BUG= 602723 

Review-Url: https://codereview.chromium.org/2643503002
Cr-Commit-Position: refs/heads/master@{#444176}

[modify] https://crrev.com/df66adcb39e831a54a35a795baf7267c9a3925f5/content/browser/renderer_host/render_widget_host_view_aura_unittest.cc

Project Member

Comment 26 by bugdroid1@chromium.org, Jan 26 2017

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

commit 564566c072aa4cfd4f6e787a3c2d24179e4c2ce1
Author: ekaramad <ekaramad@chromium.org>
Date: Thu Jan 26 14:14:01 2017

Track Text Selection information in TextInputManager (OOPIF for Android)

Currently, TextSelection information is directly tracked by the
platform's RenderWidgetHostViewAndroid. This does not work with OOPIFs
which have their own RWHVCFs.

This CL will route the SelectionChanged updates through the
TextInputManager and then notifies the platform's view about any change
on the page. This has already been happening in other platforms:
  * Aura: https://codereview.chromium.org/2130133004
  * Mac: https://codereview.chromium.org/2240553003

An interactive test related to text selection tracking for OOPIFs is
is also enabled on all platformed now.

BUG= 578168 ,  602427 ,  602723 

Review-Url: https://codereview.chromium.org/2659433002
Cr-Commit-Position: refs/heads/master@{#446317}

[modify] https://crrev.com/564566c072aa4cfd4f6e787a3c2d24179e4c2ce1/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
[modify] https://crrev.com/564566c072aa4cfd4f6e787a3c2d24179e4c2ce1/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/564566c072aa4cfd4f6e787a3c2d24179e4c2ce1/content/browser/renderer_host/render_widget_host_view_android.h
[modify] https://crrev.com/564566c072aa4cfd4f6e787a3c2d24179e4c2ce1/content/browser/renderer_host/render_widget_host_view_base.cc
[modify] https://crrev.com/564566c072aa4cfd4f6e787a3c2d24179e4c2ce1/content/browser/renderer_host/render_widget_host_view_base.h
[modify] https://crrev.com/564566c072aa4cfd4f6e787a3c2d24179e4c2ce1/content/browser/renderer_host/render_widget_host_view_mac.h
[modify] https://crrev.com/564566c072aa4cfd4f6e787a3c2d24179e4c2ce1/content/browser/renderer_host/render_widget_host_view_mac.mm

Project Member

Comment 27 by bugdroid1@chromium.org, Feb 10 2017

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

commit b6483a05119a0ebb57d3968783371c119a83d49c
Author: ekaramad <ekaramad@chromium.org>
Date: Fri Feb 10 02:23:26 2017

Fix a recent regression in IME inside OOPIFs

IME events are only processed at the renderer if there is widget/page
focus. This used to be done by checking the variabel m_imeAcceptEvents
inside WebFrameWidgetImpl which technically tracks the page focus.

After a recent refactoring, the logic for verifying page focus was moved
to RenderWidget which saves the unnecessary call to
WebInputMethodController IME methods when page is not focused. But
unfortunately this broke IME because page focus
(RenderWidget::has_focus_) does not get updated for RenderWidgets
corresponding to OOPIFs. The only reason it was working before was that
m_imeAcceptEvents is initialized to |true| and stay because we never
update page focus for WebFrameWidgetImpl.

This CL will temporarily allow handling all IME events by checking if
the RenderWidget is for an OOPIF. This will revert the behvaior to that
of prior to the refactoring.

This CL also adds an interactive browser test which catches regressions
of this sort on Mac and Aura platforms.

BUG= 688842 ,  602723 

Review-Url: https://codereview.chromium.org/2681473002
Cr-Commit-Position: refs/heads/master@{#449521}

[modify] https://crrev.com/b6483a05119a0ebb57d3968783371c119a83d49c/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
[modify] https://crrev.com/b6483a05119a0ebb57d3968783371c119a83d49c/content/public/test/text_input_test_utils.cc
[modify] https://crrev.com/b6483a05119a0ebb57d3968783371c119a83d49c/content/public/test/text_input_test_utils.h
[modify] https://crrev.com/b6483a05119a0ebb57d3968783371c119a83d49c/content/renderer/render_widget.cc

Project Member

Comment 28 by bugdroid1@chromium.org, Feb 14 2017

Labels: merge-merged-2987
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ba75a5aca3afb730adf8699e9f6951031291d58a

commit ba75a5aca3afb730adf8699e9f6951031291d58a
Author: ekaramad <ekaramad@chromium.org>
Date: Tue Feb 14 21:21:41 2017

Fix a recent regression in IME inside OOPIFs (Merge to M-57)

IME events are only processed at the renderer if there is widget/page
focus. This used to be done by checking the variabel m_imeAcceptEvents
inside WebFrameWidgetImpl which technically tracks the page focus.

After a recent refactoring, the logic for verifying page focus was moved
to RenderWidget which saves the unnecessary call to
WebInputMethodController IME methods when page is not focused. But
unfortunately this broke IME because page focus
(RenderWidget::has_focus_) does not get updated for RenderWidgets
corresponding to OOPIFs. The only reason it was working before was that
m_imeAcceptEvents is initialized to |true| and stay because we never
update page focus for WebFrameWidgetImpl.

This CL will temporarily allow handling all IME events by checking if
the RenderWidget is for an OOPIF. This will revert the behvaior to that
of prior to the refactoring.

This CL also adds an interactive browser test which catches regressions
of this sort on Mac and Aura platforms.

BUG= 688842 ,  602723 
NOTRY=TRUE
NOPRESUBMIT=TRUE
Review-Url: https://codereview.chromium.org/2681473002
Cr-Commit-Position: refs/heads/master@{#449521}
(cherry picked from commit b6483a05119a0ebb57d3968783371c119a83d49c)

Review-Url: https://codereview.chromium.org/2696883002
Cr-Commit-Position: refs/branch-heads/2987@{#509}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

[modify] https://crrev.com/ba75a5aca3afb730adf8699e9f6951031291d58a/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
[modify] https://crrev.com/ba75a5aca3afb730adf8699e9f6951031291d58a/content/public/test/text_input_test_utils.cc
[modify] https://crrev.com/ba75a5aca3afb730adf8699e9f6951031291d58a/content/public/test/text_input_test_utils.h
[modify] https://crrev.com/ba75a5aca3afb730adf8699e9f6951031291d58a/content/renderer/render_widget.cc

Project Member

Comment 29 by bugdroid1@chromium.org, Jul 13 2017

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

commit b933c4942b1f9f74cef4d563c49fb82b3c4795da
Author: Ehsan Karamad <ekaramad@chromium.org>
Date: Thu Jul 13 17:23:52 2017

Fix an issue in WebViewImeInteractiveTest.CompositionRangeUpdates.

We need to properly clear CompositionRangeUpdateObserver used in the test body
and do not rely on tear down process. There seems to be a bug in the internals
of TextInputManagerTester where the internal observer is not properly removed
from TextInputManager's observer list.

Synthetic local testing shows that if we repeatedly pump
RenderWidget::UpdateCompositionInfo(true), the test crashes in non-OOPIF mode. The
reasons seems to be that TextInputManagerTester::observer_ is not removed from
TextInputManager's observer list and a late arriving IPC for IME composition range
during shutdown causes the test process to crash (UaF).

BUG=736752,  602723 

Change-Id: Iebf1f20dccddae63ad587ec6f03bf6951b89c5ee
Reviewed-on: https://chromium-review.googlesource.com/545223
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Commit-Queue: Ehsan Karamad <ekaramad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486425}
[modify] https://crrev.com/b933c4942b1f9f74cef4d563c49fb82b3c4795da/chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc
[modify] https://crrev.com/b933c4942b1f9f74cef4d563c49fb82b3c4795da/content/public/test/text_input_test_utils.cc

The bug number in comment #29 is incorrect. The correct bug number is 736759.
Status: Fixed (was: Started)
Closing this bug given that all IME related CLs for OOPIF have landed a long time ago.

Sign in to add a comment