|
|
DescriptionRoute IME Events to Focused RenderWidgets (Android)
ImeAdapterAndroid routes its IME events to the either RenderFrame or
RenderWidget. While the IME events for RenderFrame are correctly sent to
the focused frame, the ones for RenderWidget are still being sent to the
page's RenderWidgetHostViewAndorid.
This CL will change the destination by using the focused widget, namely,
the RenderWidgetHost underneath the RenderWidgetHostViewAndroid
(including itself) which has keyboard focused. This is essential to make
IME for android work with OOPIFs.
BUG=578168
Review-Url: https://codereview.chromium.org/2653283002
Cr-Commit-Position: refs/heads/master@{#452176}
Committed: https://chromium.googlesource.com/chromium/src/+/a06dc6c5060618f557be9310cf37080000b77db3
Patch Set 1 #
Total comments: 24
Patch Set 2 : Addressing creis@'s comments #
Total comments: 1
Patch Set 3 : Fixed an error #
Total comments: 4
Patch Set 4 : Addressing creis@'s comments #
Total comments: 8
Patch Set 5 : Rebased #Patch Set 6 : Addressed creis@'s comments #Patch Set 7 : Rebased #Patch Set 8 : Added a Content Browser Test #
Total comments: 1
Patch Set 9 : Minor fixes #
Total comments: 4
Patch Set 10 : Rebased #Patch Set 11 : Addressing creis@'s comments #
Messages
Total messages: 59 (42 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
Hello Charlie, Hopefully, the last browser side CL for android IME. Please take a look. Thanks! https://codereview.chromium.org/2653283002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/ime_adapter_android.cc (right): https://codereview.chromium.org/2653283002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/ime_adapter_android.cc:327: DCHECK(rwhva_); This DCHECK here and everywhere else in this file seems unnecessary. I only added it to conform to the way code was before (specifically with GetPageRenderWidgetHostImpl). It is also inconsistently used since in SendKeyEvent we do not DCHECK. This class is created by RWHVAndroid which passes a reference to itself in the CTOR of this class. I would suggest we remove all such DCHECKs (either in this CL or in another) and if needed, add one DCHECK to the CTOR. WDYT?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hmm, I have some concerns (or at least confusion) about these methods below. Maybe you can help me understand how they work? We should also include tests if possible (particularly if the issues I mention below are real bugs). https://codereview.chromium.org/2653283002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/ime_adapter_android.cc (right): https://codereview.chromium.org/2653283002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/ime_adapter_android.cc:318: RenderWidgetHost* rwh = rwhva_->GetRenderWidgetHost(); Isn't this the RWH associated with RenderWidgetHostViewAndroid? For an OOPIF, wouldn't that be a subframe widget rather than a "page" (main frame?) widget? I'm confused about calling this "page" level. https://codereview.chromium.org/2653283002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/ime_adapter_android.cc:327: DCHECK(rwhva_); On 2017/01/25 19:27:47, EhsanK wrote: > This DCHECK here and everywhere else in this file seems unnecessary. I only > added it to conform to the way code was before (specifically with > GetPageRenderWidgetHostImpl). It is also inconsistently used since in > SendKeyEvent we do not DCHECK. > > This class is created by RWHVAndroid which passes a reference to itself in the > CTOR of this class. I would suggest we remove all such DCHECKs (either in this > CL or in another) and if needed, add one DCHECK to the CTOR. WDYT? Looks like the only previous use of it is in GetPageRenderWidgetHostImpl, and I agree that it's unnecessary there or here. Moving it to the constructor in this CL sounds good. https://codereview.chromium.org/2653283002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/ime_adapter_android.cc:328: return rwhva_->GetFocusedWidget(); I'm getting confused looking at RenderWidgetHostViewBase::GetFocusedWidget(), where this is implemented. We pass the RWHV's widget into WebContentsImpl::GetFocusedRenderWidgetHost, which returns that same widget if it's not equal to the main frame's current widget (with a comment about popup menus). Wouldn't this be broken for OOPIFs? In other words, if an OOPIF's RWHV calls GetFocusedWidget, it will always get back its own RWH. Am I reading that incorrectly? https://codereview.chromium.org/2653283002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/ime_adapter_android.cc:332: RenderWidgetHostImpl* rwh = GetPageRenderWidgetHostImpl(); What's the reason for GetFocusedFrame() use GetPageRenderWidgetHostImpl() instead of GetFocusedWidget()? Is there a case that GetFocusedWidget() would return null while there's a focused frame, or some other case that they differ? https://codereview.chromium.org/2653283002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/ime_adapter_android.cc:347: RenderWidgetHostImpl* rwh = GetPageRenderWidgetHostImpl(); Again, just wondering whether we need GetPageRenderWidgetHostImpl at all. It's ok if so. https://codereview.chromium.org/2653283002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/ime_adapter_android.h (right): https://codereview.chromium.org/2653283002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/ime_adapter_android.h:90: RenderWidgetHostImpl* GetPageRenderWidgetHostImpl(); Let's add a comment saying when you'd want to use one vs the other. (When is GetFocusedWidget the wrong choice?)
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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
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.
Thanks Charlie! PTAL. https://codereview.chromium.org/2653283002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/ime_adapter_android.cc (right): https://codereview.chromium.org/2653283002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/ime_adapter_android.cc:318: RenderWidgetHost* rwh = rwhva_->GetRenderWidgetHost(); On 2017/01/25 22:00:22, Charlie Reis wrote: > Isn't this the RWH associated with RenderWidgetHostViewAndroid? For an OOPIF, > wouldn't that be a subframe widget rather than a "page" (main frame?) widget? > I'm confused about calling this "page" level. I think |rwhva_| is always a RenderWidgetHostViewAndroid (that is the type as defined in the header); and that should always be the page's view? Also RenderWidgetHostViewAndroid is the only thing which creates the ImeAdapterAndroid (AFAIK) and passes its own reference as a RWHVA. https://codereview.chromium.org/2653283002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/ime_adapter_android.cc:326: DCHECK_CURRENTLY_ON(BrowserThread::UI); If it wasn't for this threading DCHECK, I could remove GetFocusedWidget() and use rwhva_->GetFocusedWidget() instead. https://codereview.chromium.org/2653283002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/ime_adapter_android.cc:327: DCHECK(rwhva_); On 2017/01/25 22:00:22, Charlie Reis wrote: > On 2017/01/25 19:27:47, EhsanK wrote: > > This DCHECK here and everywhere else in this file seems unnecessary. I only > > added it to conform to the way code was before (specifically with > > GetPageRenderWidgetHostImpl). It is also inconsistently used since in > > SendKeyEvent we do not DCHECK. > > > > This class is created by RWHVAndroid which passes a reference to itself in the > > CTOR of this class. I would suggest we remove all such DCHECKs (either in this > > CL or in another) and if needed, add one DCHECK to the CTOR. WDYT? > > Looks like the only previous use of it is in GetPageRenderWidgetHostImpl, and I > agree that it's unnecessary there or here. Moving it to the constructor in this > CL sounds good. Acknowledged. https://codereview.chromium.org/2653283002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/ime_adapter_android.cc:328: return rwhva_->GetFocusedWidget(); Since |rwhva_| is a RenderWidgetHostViewAndroid and is for the page, I believe what we are doing here is correct. This is supposed to return the widget inside the page (or under the tab's view) which takes keyboard focus. This is also the same thing we have been doing for other platforms. https://codereview.chromium.org/2653283002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/ime_adapter_android.cc:332: RenderWidgetHostImpl* rwh = GetPageRenderWidgetHostImpl(); > What's the reason for GetFocusedFrame() use GetPageRenderWidgetHostImpl() > instead of GetFocusedWidget()? I don't think we can get to RenderFrameHostImpl from RenderWidgetHostImpl at all (can we?). > Is there a case that GetFocusedWidget() would > return null while there's a focused frame, or some other case that they differ? Yes, I believe there is. In WebContentsImpl::GetFocusedRenderWidgetHost() we will get nullptr if the focused widget's view is nullptr. Therefore, GetFocusedFrame()->GetRenderWidgetHost() could be non-nullptr when GetFocusedWidget() returns null. I think in those cases we should not proceed with keyboard input IPCs. https://codereview.chromium.org/2653283002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/ime_adapter_android.cc:347: RenderWidgetHostImpl* rwh = GetPageRenderWidgetHostImpl(); On 2017/01/25 22:00:22, Charlie Reis wrote: > Again, just wondering whether we need GetPageRenderWidgetHostImpl at all. It's > ok if so. We actually don't since RenderWidgetHostImpl::From(rwhva_->GetRenderWidgetHost()) could replace that. I also used rwh->delegate()->GetAsWebContents() here since it will need only one cast from RenderWidgetHostDelegate to WebContents. The way it is now casts RenderWidgetHostOwnerDelegate to RenderViewHost and then RenderViewHostDelegate to WebContents. WDYT? https://codereview.chromium.org/2653283002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/ime_adapter_android.h (right): https://codereview.chromium.org/2653283002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/ime_adapter_android.h:90: RenderWidgetHostImpl* GetPageRenderWidgetHostImpl(); On 2017/01/25 22:00:22, Charlie Reis wrote: > Let's add a comment saying when you'd want to use one vs the other. (When is > GetFocusedWidget the wrong choice?) Following our other discussions in the cc file, I actually decided to remove this method since it was adding more confusion than code reuse. https://codereview.chromium.org/2653283002/diff/20001/content/browser/rendere... File content/browser/renderer_host/ime_adapter_android.cc (right): https://codereview.chromium.org/2653283002/diff/20001/content/browser/rendere... content/browser/renderer_host/ime_adapter_android.cc:331: I deliberately added space here since I though it might be better formatted to this way. WDYT?
Thanks-- looks almost ready. What did you think about my earlier question about tests? https://codereview.chromium.org/2653283002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/ime_adapter_android.cc (right): https://codereview.chromium.org/2653283002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/ime_adapter_android.cc:318: RenderWidgetHost* rwh = rwhva_->GetRenderWidgetHost(); On 2017/01/26 19:16:29, EhsanK wrote: > On 2017/01/25 22:00:22, Charlie Reis wrote: > > Isn't this the RWH associated with RenderWidgetHostViewAndroid? For an OOPIF, > > wouldn't that be a subframe widget rather than a "page" (main frame?) widget? > > I'm confused about calling this "page" level. > > I think |rwhva_| is always a RenderWidgetHostViewAndroid (that is the type as > defined in the header); and that should always be the page's view? Also > RenderWidgetHostViewAndroid is the only thing which creates the > ImeAdapterAndroid (AFAIK) and passes its own reference as a RWHVA. Ah yes, I was confused about how RWHVs work (with platform views at the top and ChildFrame views for OOPIFs). https://codereview.chromium.org/2653283002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/ime_adapter_android.cc:326: DCHECK_CURRENTLY_ON(BrowserThread::UI); On 2017/01/26 19:16:29, EhsanK wrote: > If it wasn't for this threading DCHECK, I could remove GetFocusedWidget() and > use rwhva_->GetFocusedWidget() instead. That DCHECK isn't strictly necessary, but given the amount of thought we're putting in to which method is the right one to call, it's probably worth having this helper method for it. https://codereview.chromium.org/2653283002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/ime_adapter_android.cc:328: return rwhva_->GetFocusedWidget(); On 2017/01/26 19:16:29, EhsanK wrote: > Since |rwhva_| is a RenderWidgetHostViewAndroid and is for the page, I believe > what we are doing here is correct. This is supposed to return the widget inside > the page (or under the tab's view) which takes keyboard focus. This is also the > same thing we have been doing for other platforms. Thanks-- that was the same mixup I had above. https://codereview.chromium.org/2653283002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/ime_adapter_android.cc:332: RenderWidgetHostImpl* rwh = GetPageRenderWidgetHostImpl(); On 2017/01/26 19:16:29, EhsanK wrote: > > What's the reason for GetFocusedFrame() use GetPageRenderWidgetHostImpl() > > instead of GetFocusedWidget()? > I don't think we can get to RenderFrameHostImpl from RenderWidgetHostImpl at all > (can we?). > > > Is there a case that GetFocusedWidget() would > > return null while there's a focused frame, or some other case that they > differ? > Yes, I believe there is. In WebContentsImpl::GetFocusedRenderWidgetHost() we > will get nullptr if the focused widget's view is nullptr. Therefore, > GetFocusedFrame()->GetRenderWidgetHost() could be non-nullptr when > GetFocusedWidget() returns null. I think in those cases we should not proceed > with keyboard input IPCs. Ok. https://codereview.chromium.org/2653283002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/ime_adapter_android.cc:347: RenderWidgetHostImpl* rwh = GetPageRenderWidgetHostImpl(); On 2017/01/26 19:16:29, EhsanK wrote: > On 2017/01/25 22:00:22, Charlie Reis wrote: > > Again, just wondering whether we need GetPageRenderWidgetHostImpl at all. > It's > > ok if so. > We actually don't since > RenderWidgetHostImpl::From(rwhva_->GetRenderWidgetHost()) could replace that. I > also used rwh->delegate()->GetAsWebContents() here since it will need only one > cast from RenderWidgetHostDelegate to WebContents. The way it is now casts > RenderWidgetHostOwnerDelegate to RenderViewHost and then RenderViewHostDelegate > to WebContents. WDYT? Sure. https://codereview.chromium.org/2653283002/diff/40001/content/browser/rendere... File content/browser/renderer_host/ime_adapter_android.cc (right): https://codereview.chromium.org/2653283002/diff/40001/content/browser/rendere... content/browser/renderer_host/ime_adapter_android.cc:324: RenderWidgetHostImpl::From(rwhva_->GetRenderWidgetHost()); This would effectively revert boliu's https://codereview.chromium.org/22764006. Then again, I don't understand how that had any effect on null pointer crashes in the first place. RenderWidgetHostImpl::From is just a static_cast, and that can't crash. Seems like that CL was a no-op. (Ah, yes, https://codereview.chromium.org/22882002/ was the one that actually fixed the crash.) Ok, I'm fine with this. Please include a comment about why it uses the page-level widget instead of GetFocusedWidget (like most other places). https://codereview.chromium.org/2653283002/diff/40001/content/browser/rendere... content/browser/renderer_host/ime_adapter_android.cc:343: RenderWidgetHostImpl::From(rwhva_->GetRenderWidgetHost()); As above, please include a comment.
Hello Charlie, PTAL. Sorry I forgot to answer your question on the tests. I think the test which would make sense in this context is a unit test for RenderWidgetHostViewAndroid, where we focus different widgets and verify that the IME IPC ends up at the right widget or frame, e.g. something similar to the ones in render_widget_host_view_mac/aura_unittest.cc/mm; maybe slightly more sophisticated since for Android some IPCs are sent to RenderFrame and some to RenderWidget. That being said, I do not know the scope/difficulty of creating such unittest file. Any suggestions on 1) if this seems like a doable idea, and 2) who to talk to about it? I will start with kenrb@ here. Thanks! https://codereview.chromium.org/2653283002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/ime_adapter_android.cc (right): https://codereview.chromium.org/2653283002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/ime_adapter_android.cc:318: RenderWidgetHost* rwh = rwhva_->GetRenderWidgetHost(); On 2017/01/27 01:07:22, Charlie Reis wrote: > On 2017/01/26 19:16:29, EhsanK wrote: > > On 2017/01/25 22:00:22, Charlie Reis wrote: > > > Isn't this the RWH associated with RenderWidgetHostViewAndroid? For an > OOPIF, > > > wouldn't that be a subframe widget rather than a "page" (main frame?) > widget? > > > I'm confused about calling this "page" level. > > > > I think |rwhva_| is always a RenderWidgetHostViewAndroid (that is the type as > > defined in the header); and that should always be the page's view? Also > > RenderWidgetHostViewAndroid is the only thing which creates the > > ImeAdapterAndroid (AFAIK) and passes its own reference as a RWHVA. > > Ah yes, I was confused about how RWHVs work (with platform views at the top and > ChildFrame views for OOPIFs). Acknowledged. https://codereview.chromium.org/2653283002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/ime_adapter_android.cc:326: DCHECK_CURRENTLY_ON(BrowserThread::UI); On 2017/01/27 01:07:22, Charlie Reis wrote: > On 2017/01/26 19:16:29, EhsanK wrote: > > If it wasn't for this threading DCHECK, I could remove GetFocusedWidget() and > > use rwhva_->GetFocusedWidget() instead. > > That DCHECK isn't strictly necessary, but given the amount of thought we're > putting in to which method is the right one to call, it's probably worth having > this helper method for it. Acknowledged. https://codereview.chromium.org/2653283002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/ime_adapter_android.cc:328: return rwhva_->GetFocusedWidget(); On 2017/01/27 01:07:22, Charlie Reis wrote: > On 2017/01/26 19:16:29, EhsanK wrote: > > Since |rwhva_| is a RenderWidgetHostViewAndroid and is for the page, I believe > > what we are doing here is correct. This is supposed to return the widget > inside > > the page (or under the tab's view) which takes keyboard focus. This is also > the > > same thing we have been doing for other platforms. > > Thanks-- that was the same mixup I had above. Acknowledged. https://codereview.chromium.org/2653283002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/ime_adapter_android.cc:332: RenderWidgetHostImpl* rwh = GetPageRenderWidgetHostImpl(); On 2017/01/27 01:07:21, Charlie Reis wrote: > On 2017/01/26 19:16:29, EhsanK wrote: > > > What's the reason for GetFocusedFrame() use GetPageRenderWidgetHostImpl() > > > instead of GetFocusedWidget()? > > I don't think we can get to RenderFrameHostImpl from RenderWidgetHostImpl at > all > > (can we?). > > > > > Is there a case that GetFocusedWidget() would > > > return null while there's a focused frame, or some other case that they > > differ? > > Yes, I believe there is. In WebContentsImpl::GetFocusedRenderWidgetHost() we > > will get nullptr if the focused widget's view is nullptr. Therefore, > > GetFocusedFrame()->GetRenderWidgetHost() could be non-nullptr when > > GetFocusedWidget() returns null. I think in those cases we should not proceed > > with keyboard input IPCs. > > Ok. Acknowledged. https://codereview.chromium.org/2653283002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/ime_adapter_android.cc:347: RenderWidgetHostImpl* rwh = GetPageRenderWidgetHostImpl(); On 2017/01/26 19:16:29, EhsanK wrote: > On 2017/01/25 22:00:22, Charlie Reis wrote: > > Again, just wondering whether we need GetPageRenderWidgetHostImpl at all. > It's > > ok if so. > We actually don't since > RenderWidgetHostImpl::From(rwhva_->GetRenderWidgetHost()) could replace that. I > also used rwh->delegate()->GetAsWebContents() here since it will need only one > cast from RenderWidgetHostDelegate to WebContents. The way it is now casts > RenderWidgetHostOwnerDelegate to RenderViewHost and then RenderViewHostDelegate > to WebContents. WDYT? Acknowledged. https://codereview.chromium.org/2653283002/diff/40001/content/browser/rendere... File content/browser/renderer_host/ime_adapter_android.cc (right): https://codereview.chromium.org/2653283002/diff/40001/content/browser/rendere... content/browser/renderer_host/ime_adapter_android.cc:324: RenderWidgetHostImpl::From(rwhva_->GetRenderWidgetHost()); On 2017/01/27 01:07:22, Charlie Reis wrote: > This would effectively revert boliu's https://codereview.chromium.org/22764006. > > Then again, I don't understand how that had any effect on null pointer crashes > in the first place. RenderWidgetHostImpl::From is just a static_cast, and that > can't crash. Seems like that CL was a no-op. (Ah, yes, > https://codereview.chromium.org/22882002/ was the one that actually fixed the > crash.) > > Ok, I'm fine with this. > > Please include a comment about why it uses the page-level widget instead of > GetFocusedWidget (like most other places). Acknowledged. Thanks for finding out about those CLs. I actually changed this part a bit more and reduced the code size and kind of avoided exposing FrameTreeNode here. https://codereview.chromium.org/2653283002/diff/40001/content/browser/rendere... content/browser/renderer_host/ime_adapter_android.cc:343: RenderWidgetHostImpl::From(rwhva_->GetRenderWidgetHost()); On 2017/01/27 01:07:22, Charlie Reis wrote: > As above, please include a comment. Acknowledged.
ekaramad@chromium.org changed reviewers: + kenrb@chromium.org
Adding kenrb@ regarding the question for tests. I just talked to Ken an he was suggesting we should prefer browser tests for this matter over unit tests. I will try and see if I can add a test to site_per_process_browsertest.cc for this CL (and maybe some of the earlier ones which don't have any tests).
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.
On 2017/01/27 16:21:15, EhsanK wrote: > Adding kenrb@ regarding the question for tests. > > I just talked to Ken an he was suggesting we should prefer browser tests for > this matter over unit tests. I will try and see if I can add a test to > site_per_process_browsertest.cc for this CL (and maybe some of the earlier ones > which don't have any tests). Thanks! The change itself LGTM now. If it's reasonable to add the browser test in this CL, I think that's the best plan. (Let me know if that proves to be problematic, and we can revisit in another CL.) https://codereview.chromium.org/2653283002/diff/60001/content/browser/rendere... File content/browser/renderer_host/ime_adapter_android.cc (right): https://codereview.chromium.org/2653283002/diff/60001/content/browser/rendere... content/browser/renderer_host/ime_adapter_android.cc:321: // |rwhva_->GetFocusedWidget()| does a similar thing, but, there is no direct nit: Drop "but," (since you already lead with "Although") https://codereview.chromium.org/2653283002/diff/60001/content/browser/rendere... content/browser/renderer_host/ime_adapter_android.cc:325: if (!rwh || !rwh->delegate()) I think you might need a null check on the GetAsWebContents result as well, since not all RWHs belong to a WebContents (e.g., context menus). https://codereview.chromium.org/2653283002/diff/60001/content/browser/rendere... content/browser/renderer_host/ime_adapter_android.cc:328: return rwh->delegate()->GetAsWebContents()->GetFocusedFrame(); Thanks-- much better to go through WebContents than FrameTree for 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: This issue passed the CQ dry run.
Thanks Charlie. I removed yet another method which seemed to have been unused for a while. So, PTAL. I think (but not a 100% sure) to properly test IME for android we need some java tests. I am yet to figure out the right place to add the tests (maybe close to ImeTest.java) and also figure out how to use SPP-ness and corresponding helpers with smallest amount of change possible. https://codereview.chromium.org/2653283002/diff/60001/content/browser/rendere... File content/browser/renderer_host/ime_adapter_android.cc (right): https://codereview.chromium.org/2653283002/diff/60001/content/browser/rendere... content/browser/renderer_host/ime_adapter_android.cc:321: // |rwhva_->GetFocusedWidget()| does a similar thing, but, there is no direct On 2017/01/29 18:08:14, Charlie Reis (OOO soon) wrote: > nit: Drop "but," (since you already lead with "Although") Done. Thanks! https://codereview.chromium.org/2653283002/diff/60001/content/browser/rendere... content/browser/renderer_host/ime_adapter_android.cc:325: if (!rwh || !rwh->delegate()) On 2017/01/29 18:08:14, Charlie Reis (OOO soon) wrote: > I think you might need a null check on the GetAsWebContents result as well, > since not all RWHs belong to a WebContents (e.g., context menus). Acknowledged. https://codereview.chromium.org/2653283002/diff/60001/content/browser/rendere... content/browser/renderer_host/ime_adapter_android.cc:328: return rwh->delegate()->GetAsWebContents()->GetFocusedFrame(); On 2017/01/29 18:08:14, Charlie Reis (OOO soon) wrote: > Thanks-- much better to go through WebContents than FrameTree for this. Acknowledged. https://codereview.chromium.org/2653283002/diff/60001/content/browser/rendere... content/browser/renderer_host/ime_adapter_android.cc:332: DCHECK_CURRENTLY_ON(BrowserThread::UI); This method was not actually used given that: 1- There is no refs to it in this file 2- It is private 3- There is no friend methods/classes. I removed it in this CL.
On 2017/01/31 15:53:15, EhsanK wrote: > Thanks Charlie. I removed yet another method which seemed to have been unused > for a while. So, PTAL. > > I think (but not a 100% sure) to properly test IME for android we need some java > tests. I am yet to figure out the right place to add the tests (maybe close to > ImeTest.java) and also figure out how to use SPP-ness and corresponding helpers > with smallest amount of change possible. That sounds plausible-- maybe check with some of the folks who have edited ImeTest.java (e.g., tedchoc, changwan, or yabinh?) to see if that's a good place for the test you're considering? https://codereview.chromium.org/2653283002/diff/60001/content/browser/rendere... File content/browser/renderer_host/ime_adapter_android.cc (right): https://codereview.chromium.org/2653283002/diff/60001/content/browser/rendere... content/browser/renderer_host/ime_adapter_android.cc:332: DCHECK_CURRENTLY_ON(BrowserThread::UI); On 2017/01/31 15:53:14, EhsanK wrote: > This method was not actually used given that: > > 1- There is no refs to it in this file > 2- It is private > 3- There is no friend methods/classes. > > I removed it in this CL. Great!
Description was changed from ========== Route IME Events to Focused RenderWidgets (Android) ImeAdapterAndroid routes its IME events to the either RenderFrame or RenderWidget. While the IME events for RenderFrame are correctly sent to the focused frame, the ones for RenderWidget are still being sent to the page's RenderWidgetHostViewAndorid. This CL will change the destination by using the focused widget, namely, the RenderWidgetHost underneath the RenderWidgetHostViewAndroid (including itself) which has keyboard focused. This is essential to make IME for android work with OOPIFs. BUG=578168 ========== to ========== Route IME Events to Focused RenderWidgets (Android) ImeAdapterAndroid routes its IME events to the either RenderFrame or RenderWidget. While the IME events for RenderFrame are correctly sent to the focused frame, the ones for RenderWidget are still being sent to the page's RenderWidgetHostViewAndorid. This CL will change the destination by using the focused widget, namely, the RenderWidgetHost underneath the RenderWidgetHostViewAndroid (including itself) which has keyboard focused. This is essential to make IME for android work with OOPIFs. BUG=578168 ==========
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 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...
Hello Charlie and sorry it took a while to update this. I ended up adding a C++ test rather than Java (not much luck with EmbeddedTestServer.java...) PTAL. https://codereview.chromium.org/2653283002/diff/140001/content/browser/render... File content/browser/renderer_host/ime_adapter_android.h (right): https://codereview.chromium.org/2653283002/diff/140001/content/browser/render... content/browser/renderer_host/ime_adapter_android.h:34: class CONTENT_EXPORT ImeAdapterAndroid { Link errors without this.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Thanks. LGTM, assuming the android_n5x_swarming_rel failure isn't related. https://codereview.chromium.org/2653283002/diff/160001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2653283002/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:253: ImeAdapterAndroid* get_ime_adapter_for_testing() { nit: ime_adapter_for_testing() (No "get") https://codereview.chromium.org/2653283002/diff/160001/content/browser/site_p... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2653283002/diff/160001/content/browser/site_p... content/browser/site_per_process_browsertest.cc:9445: // A valid caller is needed for ImeAdapterAndroid::GetUnerlinesFromSpans. typo: Underlines
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. The failure in the bot seems hardly relevant. I am running dry-run again anyway. I will CQ if all bots turn green. Thanks! PS:I might submit a couple of other tests related to recent changes. https://codereview.chromium.org/2653283002/diff/160001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2653283002/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:253: ImeAdapterAndroid* get_ime_adapter_for_testing() { On 2017/02/21 19:30:58, Charlie Reis wrote: > nit: ime_adapter_for_testing() > (No "get") Acknowledged. https://codereview.chromium.org/2653283002/diff/160001/content/browser/site_p... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2653283002/diff/160001/content/browser/site_p... content/browser/site_per_process_browsertest.cc:9445: // A valid caller is needed for ImeAdapterAndroid::GetUnerlinesFromSpans. On 2017/02/21 19:30:58, Charlie Reis wrote: > typo: Underlines Done.
On 2017/02/21 20:41:38, EhsanK wrote: > Thanks Charlie. The failure in the bot seems hardly relevant. I am running > dry-run again anyway. I will CQ if all bots turn green. > > Thanks! > > PS:I might submit a couple of other tests related to recent changes. > > https://codereview.chromium.org/2653283002/diff/160001/content/browser/render... > File content/browser/renderer_host/render_widget_host_view_android.h (right): > > https://codereview.chromium.org/2653283002/diff/160001/content/browser/render... > content/browser/renderer_host/render_widget_host_view_android.h:253: > ImeAdapterAndroid* get_ime_adapter_for_testing() { > On 2017/02/21 19:30:58, Charlie Reis wrote: > > nit: ime_adapter_for_testing() > > (No "get") > > Acknowledged. > > https://codereview.chromium.org/2653283002/diff/160001/content/browser/site_p... > File content/browser/site_per_process_browsertest.cc (right): > > https://codereview.chromium.org/2653283002/diff/160001/content/browser/site_p... > content/browser/site_per_process_browsertest.cc:9445: // A valid caller is > needed for ImeAdapterAndroid::GetUnerlinesFromSpans. > On 2017/02/21 19:30:58, Charlie Reis wrote: > > typo: Underlines > > Done. I meant new tests in followup CLs.
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_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
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 ekaramad@chromium.org
The CQ bit was checked by ekaramad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://codereview.chromium.org/2653283002/#ps190001 (title: "Addressing creis@'s comments")
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
Exceeded global retry quota
The CQ bit was checked by ekaramad@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 190001, "attempt_start_ts": 1487792750034880, "parent_rev": "0f78a6b4ca7b46cc6047ecea9d09987ed0a16e70", "commit_rev": "a06dc6c5060618f557be9310cf37080000b77db3"}
Message was sent while issue was closed.
Description was changed from ========== Route IME Events to Focused RenderWidgets (Android) ImeAdapterAndroid routes its IME events to the either RenderFrame or RenderWidget. While the IME events for RenderFrame are correctly sent to the focused frame, the ones for RenderWidget are still being sent to the page's RenderWidgetHostViewAndorid. This CL will change the destination by using the focused widget, namely, the RenderWidgetHost underneath the RenderWidgetHostViewAndroid (including itself) which has keyboard focused. This is essential to make IME for android work with OOPIFs. BUG=578168 ========== to ========== Route IME Events to Focused RenderWidgets (Android) ImeAdapterAndroid routes its IME events to the either RenderFrame or RenderWidget. While the IME events for RenderFrame are correctly sent to the focused frame, the ones for RenderWidget are still being sent to the page's RenderWidgetHostViewAndorid. This CL will change the destination by using the focused widget, namely, the RenderWidgetHost underneath the RenderWidgetHostViewAndroid (including itself) which has keyboard focused. This is essential to make IME for android work with OOPIFs. BUG=578168 Review-Url: https://codereview.chromium.org/2653283002 Cr-Commit-Position: refs/heads/master@{#452176} Committed: https://chromium.googlesource.com/chromium/src/+/a06dc6c5060618f557be9310cf37... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:190001) as https://chromium.googlesource.com/chromium/src/+/a06dc6c5060618f557be9310cf37... |