Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2208093004:
Use focused RenderWidgetHostImpl instead of TextInputManager::GetActiveWidget() to obtain TextSelec… (Closed)
|
Created:
4 years, 4 months ago by EhsanK Modified:
4 years, 4 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, shuchen+watch_chromium.org, jam, sievers+watch_chromium.org, jbauman+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, kalyank, danakj+watch_chromium.org, James Su, site-isolation-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse focused RenderWidgetHostImpl instead of TextInputManager::GetActiveWidget() to obtain TextSelection information.
TextInputManager::GetActiveWidget() should be exclusively used in
scenarios where <input> is focused as it returns the
RenderWidgetHostImpl corresponding to a RenderWidget which has a focused
<input>.
BUG=634148
Committed: https://crrev.com/0b1041f656728be7aba9923c9ac21fd4448b0c15
Cr-Commit-Position: refs/heads/master@{#410665}
Patch Set 1 #Patch Set 2 : Fixed some crashes #
Total comments: 12
Patch Set 3 : Get TextSelection from focused RWH only in X11 code #Patch Set 4 : Added a comment #Patch Set 5 : Added a test #
Total comments: 4
Patch Set 6 : Addressing kenrb@'s comments #
Messages
Total messages: 37 (26 generated)
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ekaramad@chromium.org changed reviewers: + creis@chromium.org
PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
creis@chromium.org changed reviewers: + kenrb@chromium.org
Adding Ken as well. I'm not sure I understand the bug enough to know how this fixes it, and I'm concerned about some of the patterns. Comments below. https://codereview.chromium.org/2208093004/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2208093004/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:1538: RenderWidgetHostViewBase* view = nullptr; nit: Wrong indent. Also, this block needs a comment explaining it. https://codereview.chromium.org/2208093004/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:1539: if (is_guest_view_hack_) Wow. I can't wait till we switch from BrowserPlugin to OOPIFs and remove this member. :) https://codereview.chromium.org/2208093004/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:1540: view = const_cast<RenderWidgetHostViewAura*>(this); Using const_cast here doesn't seem ok. https://codereview.chromium.org/2208093004/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:1541: else { Style nit: If the else branch needs braces, so does the other branch. https://codereview.chromium.org/2208093004/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:1603: RenderWidgetHostViewBase* view = nullptr; We shouldn't be copy/pasting this pattern. https://codereview.chromium.org/2208093004/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/2208093004/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_base.h:421: // This method returns the active widget form TextInputManager. If there are s/form/from/ Also, do you mean "If there are no active widgets" for the focused case? Can you add something to the comment about why focused makes sense as a fallback when there is no active one? I'm not sure I understand.
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ekaramad@chromium.org changed reviewers: + nasko@chromium.org
Sorry for missing kenrb@ I though I had added him in the original email. Also adding nasko@ since this is an site isolation IME CL. PTAL. https://codereview.chromium.org/2208093004/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2208093004/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:1538: RenderWidgetHostViewBase* view = nullptr; On 2016/08/04 19:56:50, Charlie Reis (OOO til 8-8) wrote: > nit: Wrong indent. > > Also, this block needs a comment explaining it. Acknowledged. https://codereview.chromium.org/2208093004/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:1539: if (is_guest_view_hack_) On 2016/08/04 19:56:50, Charlie Reis (OOO til 8-8) wrote: > Wow. I can't wait till we switch from BrowserPlugin to OOPIFs and remove this > member. :) Me too. Although this could take a while since MimeHandlerViewGuest is expected to keep using BrowserPlugin for a while :(. https://codereview.chromium.org/2208093004/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:1540: view = const_cast<RenderWidgetHostViewAura*>(this); On 2016/08/04 19:56:50, Charlie Reis (OOO til 8-8) wrote: > Using const_cast here doesn't seem ok. This is similar to the issue we had with RWHV::GetSelectedText(). One way out of this is to make TextInputManager work with const RWHV* rather than RWHV*. https://codereview.chromium.org/2208093004/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:1541: else { On 2016/08/04 19:56:50, Charlie Reis (OOO til 8-8) wrote: > Style nit: If the else branch needs braces, so does the other branch. Acknowledged. https://codereview.chromium.org/2208093004/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:1603: RenderWidgetHostViewBase* view = nullptr; On 2016/08/04 19:56:50, Charlie Reis (OOO til 8-8) wrote: > We shouldn't be copy/pasting this pattern. Now I am using (something similar) this pattern only in one spot. https://codereview.chromium.org/2208093004/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/2208093004/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_base.h:421: // This method returns the active widget form TextInputManager. If there are On 2016/08/04 19:56:50, Charlie Reis (OOO til 8-8) wrote: > s/form/from/ > > Also, do you mean "If there are no active widgets" for the focused case? > > Can you add something to the comment about why focused makes sense as a fallback > when there is no active one? I'm not sure I understand. I removed this from patch. But why I had it: GetActiveWidget() should either be nullptr or equal to the focused RWH. This is not true though when we have race due to slow renderers but that is a problem for another day I guess. When we implemented TextSelection tracking, the understanding I had was that all text selection code on the browser side through SelectionChanged() runs only for RWH which have <input>; which was incorrect in the case of X11 clipboard code. After I saw this bug, my thoughts immediately went to using focused RWHV all over the code. That adds complications as it does not interact well with <webview>-browser plugin and also the const-ness issue.
This change seems ok to me from an owner's perspective, but I'll need Ken to review. Is there a way to add a test for it? Sounds like it's fixing an important regression. @kenrb: Can you review?
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Charlie for the reviews! I added a unit test to aura for X11 minus CHROMEOS. PTAL. https://codereview.chromium.org/2208093004/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/2208093004/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:4563: TEST_F(InputMethodStateAuraTest, SelectedTextCopiedToClipboard) { This is technically not an IME test but I am reusing this class for the |views_| and since I am querying a value I used the StateAuraTest one.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with one comment. https://codereview.chromium.org/2208093004/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2208093004/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:3039: host_->delegate()->GetFocusedRenderWidgetHost(host_)->GetView(); It looks like GetFocusedRenderWidgetHost can return nullptr, which means that has to be checked.
LGTM! https://codereview.chromium.org/2208093004/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/2208093004/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:4563: TEST_F(InputMethodStateAuraTest, SelectedTextCopiedToClipboard) { On 2016/08/08 19:00:05, EhsanK wrote: > This is technically not an IME test but I am reusing this class for the |views_| > and since I am querying a value I used the StateAuraTest one. Acknowledged.
Thanks for the reviews. I will CQ after dry-run. https://codereview.chromium.org/2208093004/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2208093004/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:3039: host_->delegate()->GetFocusedRenderWidgetHost(host_)->GetView(); On 2016/08/08 20:45:03, kenrb wrote: > It looks like GetFocusedRenderWidgetHost can return nullptr, which means that > has to be checked. Acknowledged. And Done. Thanks for spotting this.
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by ekaramad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/2208093004/#ps100001 (title: "Addressing kenrb@'s comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Use focused RenderWidgetHostImpl instead of TextInputManager::GetActiveWidget() to obtain TextSelection information. TextInputManager::GetActiveWidget() should be exclusively used in scenarios where <input> is focused as it returns the RenderWidgetHostImpl corresponding to a RenderWidget which has a focused <input>. BUG=634148 ========== to ========== Use focused RenderWidgetHostImpl instead of TextInputManager::GetActiveWidget() to obtain TextSelection information. TextInputManager::GetActiveWidget() should be exclusively used in scenarios where <input> is focused as it returns the RenderWidgetHostImpl corresponding to a RenderWidget which has a focused <input>. BUG=634148 Committed: https://crrev.com/0b1041f656728be7aba9923c9ac21fd4448b0c15 Cr-Commit-Position: refs/heads/master@{#410665} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/0b1041f656728be7aba9923c9ac21fd4448b0c15 Cr-Commit-Position: refs/heads/master@{#410665} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
