Clean up Unnecessary Text Input State Tracking in Content |
|||||||
Issue descriptionFollowing the fix for Issue 578168 , the text input state tracking is done by WebContentsImpl and there is no need for extra member variables in other classes. We should do a cleanup of the code and remove the unnecessary variables. The found cases are in: RenderWidgetHostViewMac: |text_input_type_| and |can_compose_inline_| are no longer required. BrowserPluginGuest: We don't need |last_text_input_state_|. More of such variables if exist should be added to this bug.
,
Apr 27 2016
,
Jul 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c3ef49d3c300e954f631acfe601b6276a3b5b89f commit c3ef49d3c300e954f631acfe601b6276a3b5b89f Author: ekaramad <ekaramad@chromium.org> Date: Wed Jul 27 18:38:25 2016 Track TextInputState from multiple RenderWidgets in TextInputManager (Mac) This CL implements the logic to track TextInputState in TextInputManager for OOPIF structure on Mac. A similar implementation was landed on Aura here: https://codereview.chromium.org/1948343002. The CL also activates the corresponding TextInputManager tests on Mac. BUG= 578168 , 602723 , 602427 Review-Url: https://codereview.chromium.org/2166573003 Cr-Commit-Position: refs/heads/master@{#408199} [modify] https://crrev.com/c3ef49d3c300e954f631acfe601b6276a3b5b89f/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc [modify] https://crrev.com/c3ef49d3c300e954f631acfe601b6276a3b5b89f/chrome/test/BUILD.gn [modify] https://crrev.com/c3ef49d3c300e954f631acfe601b6276a3b5b89f/content/browser/renderer_host/render_widget_host_view_base.cc [modify] https://crrev.com/c3ef49d3c300e954f631acfe601b6276a3b5b89f/content/browser/renderer_host/render_widget_host_view_mac.h [modify] https://crrev.com/c3ef49d3c300e954f631acfe601b6276a3b5b89f/content/browser/renderer_host/render_widget_host_view_mac.mm [modify] https://crrev.com/c3ef49d3c300e954f631acfe601b6276a3b5b89f/content/browser/renderer_host/text_input_manager.cc
,
Jul 31 2016
What about selection offset? I can't figure out how anyone uses it -- other than for computing substrings. I can't seem to find the code that ever looks at the content, surrounding selection. Do you know, by chance?
,
Aug 1 2016
Hopefully very soon. As part of implementing IME for OOPIF, we are migrating IME related variables to content::TextInputManager one by one. This has happened for aura and Mac is on-route. As far as I can tell, the application of it in RenderWidgetHostViewBase::GetSelectedText() is not very useful as the only application of the method is in a test (we will eventually remove this method perhaps). But the offset is determined in dealing with the selected text in Mac and Aura. For instance, attributedSubstringForProposedRange() or in aura, GetTextRange(). As to how this is used on Mac I am not sure yet. But it will be removed from RWHVMac/Cocoa as soon as TextSelection tracking on browser side is moved to TextInputManager.
,
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
,
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
,
Mar 2 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/374b2669b0af7cd88882cff28555f5aaa04c52d7 commit 374b2669b0af7cd88882cff28555f5aaa04c52d7 Author: ekaramad <ekaramad@chromium.org> Date: Thu Mar 02 20:44:52 2017 [refactor] Cleanup IME State in RenderWidgetHostViewMac which is already tracked by TextInputManager This CL will cleanup some text selection and composition information related variables inside RenderWidgetHostViewMac. These values are already tracked by TextInputManager so keeping them inside RenderWidgetHostViewMac is costly. In line with this, some changes to TextInputManager::TextSelection led to small changes to both RenderWidgetHostViewAura and RenderWidgetHostViewAndroid. BUG= 602427 Review-Url: https://codereview.chromium.org/2694543002 Cr-Commit-Position: refs/heads/master@{#454367} [modify] https://crrev.com/374b2669b0af7cd88882cff28555f5aaa04c52d7/content/browser/renderer_host/render_widget_host_view_android.cc [modify] https://crrev.com/374b2669b0af7cd88882cff28555f5aaa04c52d7/content/browser/renderer_host/render_widget_host_view_aura.cc [modify] https://crrev.com/374b2669b0af7cd88882cff28555f5aaa04c52d7/content/browser/renderer_host/render_widget_host_view_base.cc [modify] https://crrev.com/374b2669b0af7cd88882cff28555f5aaa04c52d7/content/browser/renderer_host/render_widget_host_view_mac.h [modify] https://crrev.com/374b2669b0af7cd88882cff28555f5aaa04c52d7/content/browser/renderer_host/render_widget_host_view_mac.mm [modify] https://crrev.com/374b2669b0af7cd88882cff28555f5aaa04c52d7/content/browser/renderer_host/render_widget_host_view_mac_unittest.mm [modify] https://crrev.com/374b2669b0af7cd88882cff28555f5aaa04c52d7/content/browser/renderer_host/text_input_manager.cc [modify] https://crrev.com/374b2669b0af7cd88882cff28555f5aaa04c52d7/content/browser/renderer_host/text_input_manager.h [modify] https://crrev.com/374b2669b0af7cd88882cff28555f5aaa04c52d7/content/browser/site_per_process_browsertest.cc [modify] https://crrev.com/374b2669b0af7cd88882cff28555f5aaa04c52d7/content/public/test/text_input_test_utils.cc
,
Mar 3 2017
I believe with this last CL all the state tracking for IME is moved from RWHV* to TextInputManager. Closing this bug.
,
Mar 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6e3e9e553fee7cd6246ac4bd604303e5cdb3694f commit 6e3e9e553fee7cd6246ac4bd604303e5cdb3694f Author: ekaramad <ekaramad@chromium.org> Date: Mon Mar 06 18:45:10 2017 Hotfix: hasText value is incorrectly set Following the refactoring in CL: 2694543002 the value of hasText is incorrectly set. To be precise, hasText is always set to the negate of its correct value. BUG= 602427 Review-Url: https://codereview.chromium.org/2735633005 Cr-Commit-Position: refs/heads/master@{#454901} [modify] https://crrev.com/6e3e9e553fee7cd6246ac4bd604303e5cdb3694f/content/browser/renderer_host/render_widget_host_view_mac.mm
,
Mar 8 2017
We need to merge the short fix in comment 10 to 58.
,
Mar 8 2017
Your change meets the bar and is auto-approved for M58. Please go ahead and merge the CL to branch 3029 manually. Please contact milestone owner if you have questions. Owners: amineer@(clank), cmasso@(bling), bhthompson@(cros), govind@(desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Mar 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/aab5ba09d5d43ac6bf8875cec9ee89d3c291ac32 commit aab5ba09d5d43ac6bf8875cec9ee89d3c291ac32 Author: EhsanK <ekaramad@chromium.org> Date: Wed Mar 08 20:25:20 2017 Hotfix: hasText value is incorrectly set Following the refactoring in CL: 2694543002 the value of hasText is incorrectly set. To be precise, hasText is always set to the negate of its correct value. BUG= 602427 Review-Url: https://codereview.chromium.org/2735633005 Cr-Commit-Position: refs/heads/master@{#454901} (cherry picked from commit 6e3e9e553fee7cd6246ac4bd604303e5cdb3694f) Review-Url: https://codereview.chromium.org/2741503002 . Cr-Commit-Position: refs/branch-heads/3029@{#66} Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471} [modify] https://crrev.com/aab5ba09d5d43ac6bf8875cec9ee89d3c291ac32/content/browser/renderer_host/render_widget_host_view_mac.mm |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by ekaramad@chromium.org
, Apr 12 2016