Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2752113005:
Let ImeAdapterAndroid have the same lifecycle as its Java peer (Closed)
|
Created:
3 years, 9 months ago by Jinsuk Kim Modified:
3 years, 8 months ago CC:
agrieve+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, jam, nona+watch_chromium.org, shuchen+watch_chromium.org, James Su, yusukes+watch_chromium.org, mdjones Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionLet ImeAdapterAndroid have the same lifecycle as its Java peer
The native ImeAdapterAndroid(IAA) instance is owned by
RenderWidgetHostViewAndroid(RWHVA) while its Java peer
(ImeAdapter) is owned by CVC. This causes their life
cycles to be different. Attach/detach API are used to get them
linked before talking to each other.
This CL makes this mechanism simpler by having Java ImeAdapter
class create the native together, hence gets their lifetime synced.
The separate attach/detach mechanism is not necessary since
the native is an WebContentObserver, and connected to (or
disconnected from) RWHVA accordingly when notified of RenderView-
Host update callback. Connecting to interstitial page is also
taken care of by observing the appropriate callbacks.
BUG=662908
Review-Url: https://codereview.chromium.org/2752113005
Cr-Original-Original-Commit-Position: refs/heads/master@{#460028}
Committed: https://chromium.googlesource.com/chromium/src/+/153d2811b3cbcf69cda7ab4817cdc35e7c680dee
Review-Url: https://codereview.chromium.org/2752113005
Cr-Original-Commit-Position: refs/heads/master@{#461024}
Committed: https://chromium.googlesource.com/chromium/src/+/d01dcc6a7955b96aff2083461319208afc2bc9ca
Review-Url: https://codereview.chromium.org/2752113005
Cr-Commit-Position: refs/heads/master@{#461975}
Committed: https://chromium.googlesource.com/chromium/src/+/48f82db77c454a66084b056b4b67d103a681a5e8
Patch Set 1 : fix tests #
Total comments: 6
Patch Set 2 : rebased #Patch Set 3 : comments #Patch Set 4 : s/isValid/mIsConnected/ #
Total comments: 8
Patch Set 5 : comments #
Total comments: 19
Patch Set 6 : rebase #Patch Set 7 : background rwhva #
Total comments: 16
Patch Set 8 : WebContentsObserver #
Total comments: 8
Patch Set 9 : rebase #Patch Set 10 : comments #
Total comments: 2
Patch Set 11 : bug #Patch Set 12 : fix tests #Patch Set 13 : rebase #Patch Set 14 : debuggin on the bot #Patch Set 15 : avoid accessing deleted rwhva #
Total comments: 5
Patch Set 16 : weak ptr #
Total comments: 2
Patch Set 17 : rename #Patch Set 18 : fix tests #Messages
Total messages: 111 (55 generated)
The CQ bit was checked by jinsukkim@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Patchset #2 (id:20001) has been deleted
Description was changed from ========== Let ImeAdapterAndroid have the same lifecycle as Java instance Previously the native ImeAdapterAndroid instance was owned by RWHVA, and the life cycle was different from its Java counterpart. Their communication had to be preceded by attach/detach mechanism. This CL makes the native instance live with the Java instance in a same life cycle to make the management easier. BUG=662908 ========== to ========== Let ImeAdapterAndroid have the same lifecycle as its Java peer The native ImeAdapterAndroid(IAA) instance is owned by RenderWidgetHostViewAndroid(RWHVA) while its Java peer (ImeAdapter) which is owned by CVC. This causes their life cycles to be different. Attach/detach API are used to get them linked before talking to each other. This CL makes this mechanism simpler by having Java ImeAdapter class create the native together, hence gets their lifetime synced. The separate attach/detach mechanism is not necessary because the linking is done as a part of WCVA - RWHVA association job. i.e. IAA is linked to the current RWHVA when the corresponding CVCImpl is set to RWHVA (In the future this will be done separately as RWHVA will be rid of all the references to CVC). The only situation where the linking should be done "manually" is when IAA gets (temporarily) linked to RWHVA for an interstitial page. The link between IAA and the main RWHVA gets restored once the interstitial page is detached. BUG=662908 ==========
Description was changed from ========== Let ImeAdapterAndroid have the same lifecycle as its Java peer The native ImeAdapterAndroid(IAA) instance is owned by RenderWidgetHostViewAndroid(RWHVA) while its Java peer (ImeAdapter) which is owned by CVC. This causes their life cycles to be different. Attach/detach API are used to get them linked before talking to each other. This CL makes this mechanism simpler by having Java ImeAdapter class create the native together, hence gets their lifetime synced. The separate attach/detach mechanism is not necessary because the linking is done as a part of WCVA - RWHVA association job. i.e. IAA is linked to the current RWHVA when the corresponding CVCImpl is set to RWHVA (In the future this will be done separately as RWHVA will be rid of all the references to CVC). The only situation where the linking should be done "manually" is when IAA gets (temporarily) linked to RWHVA for an interstitial page. The link between IAA and the main RWHVA gets restored once the interstitial page is detached. BUG=662908 ========== to ========== Let ImeAdapterAndroid have the same lifecycle as its Java peer The native ImeAdapterAndroid(IAA) instance is owned by RenderWidgetHostViewAndroid(RWHVA) while its Java peer (ImeAdapter) is owned by CVC. This causes their life cycles to be different. Attach/detach API are used to get them linked before talking to each other. This CL makes this mechanism simpler by having Java ImeAdapter class create the native together, hence gets their lifetime synced. The separate attach/detach mechanism is not necessary since the linking is done as a part of WCVA - RWHVA association job. i.e. IAA is linked to the current RWHVA when the corresponding CVCImpl is set to RWHVA (in the future this will be done separately as RWHVA will be rid of all the references to CVC). The only situation where the linking should be done "manually" is when IAA gets (temporarily) linked to RWHVA for an interstitial page. The link between IAA and the main RWHVA gets restored once the interstitial page is detached. BUG=662908 ==========
Patchset #4 (id:80001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:1) has been deleted
The CQ bit was checked by jinsukkim@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...
jinsukkim@chromium.org changed reviewers: + aelias@chromium.org, changwan@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
Could you explain how this helps move ImeAdapter out of ContentViewCore? Or is it orthogonal to the other effort? https://codereview.chromium.org/2752113005/diff/100001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (left): https://codereview.chromium.org/2752113005/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:520: if (mNativeImeAdapterAndroid == 0) return false; I prefer to keep all these native pointer checks for the same reason mentioned above. https://codereview.chromium.org/2752113005/diff/100001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/2752113005/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:475: nativeDestroy(mNativeImeAdapterAndroid); Could you also set mNativeImeAdapterAndroid to 0 here? Calling ImeAdapter#destroy() and then calling some other methods may lead to a crash that is difficult to root-cause. I prefer not to leave such possibility.
> Could you explain how this helps move ImeAdapter out of ContentViewCore? Or is > it orthogonal to the other effort? It also helps move it out of CVC in the sense that attaching ImeAdpater is not done through CVC any more. The next CL will remove the other dependencies(such as updating ime state) too. https://codereview.chromium.org/2752113005/diff/100001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (left): https://codereview.chromium.org/2752113005/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:520: if (mNativeImeAdapterAndroid == 0) return false; On 2017/03/17 23:47:19, Changwan Ryu wrote: > I prefer to keep all these native pointer checks for the same reason mentioned > above. mIsConnected is equivalent to the native pointer in terms of representing the attach/detach state. https://codereview.chromium.org/2752113005/diff/100001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/2752113005/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:475: nativeDestroy(mNativeImeAdapterAndroid); On 2017/03/17 23:47:19, Changwan Ryu wrote: > Could you also set mNativeImeAdapterAndroid to 0 here? > > Calling ImeAdapter#destroy() and then calling some other methods may lead to a > crash that is difficult to root-cause. > > I prefer not to leave such possibility. I believe the intention of |mNativeImeAdapterAndroid == 0| was to check if the adapter is attached or not. After the CL, it cannot be used for that purpose - it is always non-zero since the native object is instantiated together with this Java class and has the same lifetime. Attached/Detached state is reported through |onConnectedRenderProcess| now, and checking |mIsConnected| can do exactly what |mNativeImeAdapterAndroid == 0| did. Is there any tests in place that will safe-guard or steps to create situations you're concerned of? Will test that.
Thanks for the answer. Please see my comment inline. https://codereview.chromium.org/2752113005/diff/100001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/2752113005/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:475: nativeDestroy(mNativeImeAdapterAndroid); On 2017/03/19 23:44:20, Jinsuk Kim wrote: > On 2017/03/17 23:47:19, Changwan Ryu wrote: > > Could you also set mNativeImeAdapterAndroid to 0 here? > > > > Calling ImeAdapter#destroy() and then calling some other methods may lead to a > > crash that is difficult to root-cause. > > > > I prefer not to leave such possibility. > > I believe the intention of |mNativeImeAdapterAndroid == 0| was to check if > the adapter is attached or not. After the CL, it cannot be used for that > purpose - it is always non-zero since the native object is instantiated together > with this Java class and has the same lifetime. Attached/Detached state is > reported through |onConnectedRenderProcess| now, and checking |mIsConnected| > can do exactly what |mNativeImeAdapterAndroid == 0| did. > > Is there any tests in place that will safe-guard or steps to create situations > you're concerned of? Will test that. ImeAdapter#destroy() -> ImeAdapter#finishComposingText() will crash in a way that is very difficult to understand. (You can try...) This may happen if ImeAdapter gets destroyed and then IME thread calls IME operations on the next UI message loop. Also I think this concern is still valid even if this is not a possible path for now because such safety logic should be self contained regardless of potential caller path changes.
PTAL. Also updated ImeAdapter.onConnectedToRenderProcess to be called when it is 'connected' only. Ar this point I don't see the case where 'disconnect' is supposed to get notified in this way, since WebContents gets destroyed before RWHVA does. https://codereview.chromium.org/2752113005/diff/100001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/2752113005/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:475: nativeDestroy(mNativeImeAdapterAndroid); On 2017/03/20 18:23:00, Changwan Ryu wrote: > On 2017/03/19 23:44:20, Jinsuk Kim wrote: > > On 2017/03/17 23:47:19, Changwan Ryu wrote: > > > Could you also set mNativeImeAdapterAndroid to 0 here? > > > > > > Calling ImeAdapter#destroy() and then calling some other methods may lead to > a > > > crash that is difficult to root-cause. > > > > > > I prefer not to leave such possibility. > > > > I believe the intention of |mNativeImeAdapterAndroid == 0| was to check if > > the adapter is attached or not. After the CL, it cannot be used for that > > purpose - it is always non-zero since the native object is instantiated > together > > with this Java class and has the same lifetime. Attached/Detached state is > > reported through |onConnectedRenderProcess| now, and checking |mIsConnected| > > can do exactly what |mNativeImeAdapterAndroid == 0| did. > > > > Is there any tests in place that will safe-guard or steps to create situations > > you're concerned of? Will test that. > > ImeAdapter#destroy() -> ImeAdapter#finishComposingText() will crash in a way > that is very difficult to understand. (You can try...) > > This may happen if ImeAdapter gets destroyed and then IME thread calls IME > operations on the next UI message loop. > > Also I think this concern is still valid even if this is not a possible path for > now because such safety logic should be self contained regardless of potential > caller path changes. Added back the checking against mNativeImeAdapterAndroid, as it was clarified that ThreadedInputConnection still can call on ImeAdapter after destroy() is called.
jinsukkim@chromium.org changed reviewers: + boliu@chromium.org
changwan@ I found your concern around crash due to accessing ImeAdapter via ThreadedInputConnection can be addressed in simpler way by setting |mIsConnected| to false at |destroy()|. Please take another look. boliu@chromium.org: Please review changes in content/
lgtm w/ comment. Thanks for fixing this! https://codereview.chromium.org/2752113005/diff/160001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/2752113005/diff/160001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:477: nativeDestroy(mNativeImeAdapterAndroid); Ok, mIsConnected = false seems fine. But could you at least set mNativeImeAdapterAndroid = 0 here? I just want to make sure not to encounter any nasty c++ pointer issue, no matter how the code changes. (For instance, even when destroy() -> onRenderProcessConnected() is called.)
so all RWHVA always points to the same IAA, but IAA points to only the "active" RWHVA? then what's stopping a non-active RWHVA calling into IAA? what's unsetting the IAA pointer in non-active RHWVA when IAA is destroyed? https://codereview.chromium.org/2752113005/diff/160001/content/browser/render... File content/browser/renderer_host/ime_adapter_android.cc (right): https://codereview.chromium.org/2752113005/diff/160001/content/browser/render... content/browser/renderer_host/ime_adapter_android.cc:153: rwhva_->SendKeyEvent(event); this is a weakptr now, all these accesses need null checks otoh, weakptr seem like the wrong pattern here. you really do need to listen to when RWHVA goes away and react appropriately https://codereview.chromium.org/2752113005/diff/160001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (left): https://codereview.chromium.org/2752113005/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:452: ime_adapter_android_(this), you still need to initialize this https://codereview.chromium.org/2752113005/diff/160001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2752113005/diff/160001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:773: * Warning: destroy() is not guranteed to be called in Android WebView. this will leak on webview
Patchset #5 (id:180001) has been deleted
> then what's stopping a non-active RWHVA calling into IAA? what's unsetting the > IAA pointer in non-active RHWVA when IAA is destroyed? Addressed it in line 1872 of https://codereview.chromium.org/2752113005/diff2/160001:200001/content/browse... https://codereview.chromium.org/2752113005/diff/160001/content/browser/render... File content/browser/renderer_host/ime_adapter_android.cc (right): https://codereview.chromium.org/2752113005/diff/160001/content/browser/render... content/browser/renderer_host/ime_adapter_android.cc:153: rwhva_->SendKeyEvent(event); On 2017/03/22 16:55:02, boliu wrote: > this is a weakptr now, all these accesses need null checks > > otoh, weakptr seem like the wrong pattern here. you really do need to listen to > when RWHVA goes away and react appropriately Add null checking to all. Chatted online to keep the weak ptr as is. https://codereview.chromium.org/2752113005/diff/160001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (left): https://codereview.chromium.org/2752113005/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:452: ime_adapter_android_(this), On 2017/03/22 16:55:02, boliu wrote: > you still need to initialize this Done. https://codereview.chromium.org/2752113005/diff/160001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2752113005/diff/160001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:773: * Warning: destroy() is not guranteed to be called in Android WebView. On 2017/03/22 16:55:02, boliu wrote: > this will leak on webview Moved it to |onNativeContentViewCoreDestroyed()| https://codereview.chromium.org/2752113005/diff/160001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/2752113005/diff/160001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:477: nativeDestroy(mNativeImeAdapterAndroid); On 2017/03/22 01:32:57, Changwan Ryu wrote: > Ok, mIsConnected = false seems fine. > But could you at least set mNativeImeAdapterAndroid = 0 here? > I just want to make sure not to encounter any nasty c++ pointer issue, no matter > how the code changes. (For instance, even when destroy() -> > onRenderProcessConnected() is called.) Reverted to checking both to make it defensive as you prefer.
https://codereview.chromium.org/2752113005/diff/200001/content/browser/render... File content/browser/renderer_host/ime_adapter_android.cc (right): https://codereview.chromium.org/2752113005/diff/200001/content/browser/render... content/browser/renderer_host/ime_adapter_android.cc:335: return rwhva_ ? rwhva_->GetFocusedWidget() : nullptr; changwan: does everything here run on the UI thread? weakptrs are not thread safe https://codereview.chromium.org/2752113005/diff/200001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2752113005/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1872: ConnectImeAdapter(content_view_core ? content_view_core->ime_adapter() I think non-active RWHVA can still have non null IAA? this is only cleared on in RWHVA destructor afaict? https://codereview.chromium.org/2752113005/diff/200001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2752113005/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:691: mImeAdapter.destroy(); that still doesn't work for webview with gc. by the time native CVC is destroyed, java CVC is has long been garbage collected I think you want native CVC owning native IAA instead
Let me update the patch once I get changwan@'s reply. https://codereview.chromium.org/2752113005/diff/200001/content/browser/render... File content/browser/renderer_host/ime_adapter_android.cc (right): https://codereview.chromium.org/2752113005/diff/200001/content/browser/render... content/browser/renderer_host/ime_adapter_android.cc:335: return rwhva_ ? rwhva_->GetFocusedWidget() : nullptr; On 2017/03/23 04:17:01, boliu wrote: > changwan: does everything here run on the UI thread? weakptrs are not thread > safe I read that everything runs on UI thread. ThreadedInputConnection which runs on IME thread posts every request onto UI thread before reaching here(I guess the missing DCHECK for UI thread in |::SendKeyEvent| is just a mistake). But I'll wait for changwan@ to confirm. https://codereview.chromium.org/2752113005/diff/200001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2752113005/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1872: ConnectImeAdapter(content_view_core ? content_view_core->ime_adapter() On 2017/03/23 04:17:01, boliu wrote: > I think non-active RWHVA can still have non null IAA? this is only cleared on in > RWHVA destructor afaict? When RenderViewHost is swapped out, SetContentViewCore(nullptr) is called on the old one that becomes non-active. Doesn't that takes care of it?
https://codereview.chromium.org/2752113005/diff/200001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2752113005/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1872: ConnectImeAdapter(content_view_core ? content_view_core->ime_adapter() On 2017/03/23 07:25:10, Jinsuk Kim wrote: > On 2017/03/23 04:17:01, boliu wrote: > > I think non-active RWHVA can still have non null IAA? this is only cleared on > in > > RWHVA destructor afaict? > > When RenderViewHost is swapped out, SetContentViewCore(nullptr) is called on the > old one that becomes non-active. Doesn't that takes care of it? Oh I missed ContentViewCoreImpl::RenderViewHostChanged. However that doesn't cover interstitials. Actually, I'm not sure the previous code worked well with interstitials either..
https://codereview.chromium.org/2752113005/diff/200001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2752113005/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:691: mImeAdapter.destroy(); On 2017/03/23 04:17:01, boliu wrote: > that still doesn't work for webview with gc. by the time native CVC is > destroyed, java CVC is has long been garbage collected > > I think you want native CVC owning native IAA instead The other option is to have ime adapter lifetime be tied to onAttachedTo/DetachedFromWindow. Not sure if that works though
https://codereview.chromium.org/2752113005/diff/200001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2752113005/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1872: ConnectImeAdapter(content_view_core ? content_view_core->ime_adapter() On 2017/03/23 14:18:50, boliu wrote: > On 2017/03/23 07:25:10, Jinsuk Kim wrote: > > On 2017/03/23 04:17:01, boliu wrote: > > > I think non-active RWHVA can still have non null IAA? this is only cleared > on > > in > > > RWHVA destructor afaict? > > > > When RenderViewHost is swapped out, SetContentViewCore(nullptr) is called on > the > > old one that becomes non-active. Doesn't that takes care of it? > > Oh I missed ContentViewCoreImpl::RenderViewHostChanged. However that doesn't > cover interstitials. Actually, I'm not sure the previous code worked well with > interstitials either.. Right I didn't handle non-active ones (by nulling out its IAA). From my testing the interstitials get destroyed immediately after being detached. Which makes me think it won't be of concern in practice. The CL works for interstitials (see CVCImpl::DidDetachInterstitialPage) besides that. If that still doesn't look enough for you, what I can do is cache the interstitial rwhva in CVCImpl, and null out its IAA when it gets detached. This may need more testing. https://codereview.chromium.org/2752113005/diff/200001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2752113005/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:691: mImeAdapter.destroy(); On 2017/03/23 19:02:02, boliu wrote: > On 2017/03/23 04:17:01, boliu wrote: > > that still doesn't work for webview with gc. by the time native CVC is > > destroyed, java CVC is has long been garbage collected > > > > I think you want native CVC owning native IAA instead > > The other option is to have ime adapter lifetime be tied to > onAttachedTo/DetachedFromWindow. Not sure if that works though I can get the native CVC to own IAA and call Java IA destroy from native as you suggested above. Will try this if that doesn't turn out well.
https://codereview.chromium.org/2752113005/diff/200001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2752113005/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1872: ConnectImeAdapter(content_view_core ? content_view_core->ime_adapter() On 2017/03/23 14:18:50, boliu wrote: > On 2017/03/23 07:25:10, Jinsuk Kim wrote: > > On 2017/03/23 04:17:01, boliu wrote: > > > I think non-active RWHVA can still have non null IAA? this is only cleared > on > > in > > > RWHVA destructor afaict? > > > > When RenderViewHost is swapped out, SetContentViewCore(nullptr) is called on > the > > old one that becomes non-active. Doesn't that takes care of it? > > Oh I missed ContentViewCoreImpl::RenderViewHostChanged. However that doesn't > cover interstitials. Actually, I'm not sure the previous code worked well with > interstitials either.. Oh by 'the previous code' you mean the code before my CL? I think it worked because it is RWHVA that actively attaches its IAA to Java IA. After the change it is the other way around (web contents connecting its IAA to active RWHVA) - this happens when it calls RWHVA::SetContentViewCore (except going back from interstitial to main RWHVA).
https://codereview.chromium.org/2752113005/diff/200001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2752113005/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1872: ConnectImeAdapter(content_view_core ? content_view_core->ime_adapter() On 2017/03/23 23:10:24, Jinsuk Kim wrote: > On 2017/03/23 14:18:50, boliu wrote: > > On 2017/03/23 07:25:10, Jinsuk Kim wrote: > > > On 2017/03/23 04:17:01, boliu wrote: > > > > I think non-active RWHVA can still have non null IAA? this is only cleared > > on > > > in > > > > RWHVA destructor afaict? > > > > > > When RenderViewHost is swapped out, SetContentViewCore(nullptr) is called on > > the > > > old one that becomes non-active. Doesn't that takes care of it? > > > > Oh I missed ContentViewCoreImpl::RenderViewHostChanged. However that doesn't > > cover interstitials. Actually, I'm not sure the previous code worked well with > > interstitials either.. > > Right I didn't handle non-active ones (by nulling out its IAA). From my testing > the interstitials get destroyed immediately after being detached. Which makes me > think it won't be of concern in practice. The CL works for interstitials (see > CVCImpl::DidDetachInterstitialPage) besides that. Well, what about the page in the back while the interstitial is showing? > > If that still doesn't look enough for you, what I can do is cache the > interstitial rwhva in CVCImpl, and null out its IAA when it gets detached. This > may need more testing.
https://codereview.chromium.org/2752113005/diff/200001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2752113005/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1872: ConnectImeAdapter(content_view_core ? content_view_core->ime_adapter() On 2017/03/23 23:16:22, Jinsuk Kim wrote: > On 2017/03/23 14:18:50, boliu wrote: > > On 2017/03/23 07:25:10, Jinsuk Kim wrote: > > > On 2017/03/23 04:17:01, boliu wrote: > > > > I think non-active RWHVA can still have non null IAA? this is only cleared > > on > > > in > > > > RWHVA destructor afaict? > > > > > > When RenderViewHost is swapped out, SetContentViewCore(nullptr) is called on > > the > > > old one that becomes non-active. Doesn't that takes care of it? > > > > Oh I missed ContentViewCoreImpl::RenderViewHostChanged. However that doesn't > > cover interstitials. Actually, I'm not sure the previous code worked well with > > interstitials either.. > > Oh by 'the previous code' you mean the code before my CL? Yes > I think it worked because it is RWHVA that actively attaches its IAA to Java IA. > After the change it is the other way around (web contents connecting its IAA to > active RWHVA) - this happens when it calls RWHVA::SetContentViewCore (except > going back from interstitial to main RWHVA). How previous code work in the case of interstitial detach?
To boliu@, jinsukkim@: I'm not sure about interstitial case. WebContentsViewAndroid::Focus() seems to reattach the native ImeAdapterAndroid to the Java counterpart, but honestly I never had a chance to test around interstitial. There was no bug reported around that. Even though there is such a bug, probably no interstitial we have simply don't require user input. jinsukkim@, what is an example interstitial case did you test? Does dangerous URL warning takes you to an interstitial? https://codereview.chromium.org/2752113005/diff/200001/content/browser/render... File content/browser/renderer_host/ime_adapter_android.cc (right): https://codereview.chromium.org/2752113005/diff/200001/content/browser/render... content/browser/renderer_host/ime_adapter_android.cc:335: return rwhva_ ? rwhva_->GetFocusedWidget() : nullptr; On 2017/03/23 07:25:10, Jinsuk Kim wrote: > On 2017/03/23 04:17:01, boliu wrote: > > changwan: does everything here run on the UI thread? weakptrs are not thread > > safe > > I read that everything runs on UI thread. ThreadedInputConnection which runs on > IME thread posts every request onto UI thread before reaching here(I guess the > missing DCHECK for UI thread in |::SendKeyEvent| is just a mistake). But I'll > wait for changwan@ to confirm. Correct. Everything from ThreadedInput* to ImeAdapter should be on UI thread. One exception is updateSelection() which goes directly to Android framework.
https://codereview.chromium.org/2752113005/diff/200001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2752113005/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1872: ConnectImeAdapter(content_view_core ? content_view_core->ime_adapter() On 2017/03/23 23:22:56, boliu wrote: > On 2017/03/23 23:16:22, Jinsuk Kim wrote: > > On 2017/03/23 14:18:50, boliu wrote: > > > On 2017/03/23 07:25:10, Jinsuk Kim wrote: > > > > On 2017/03/23 04:17:01, boliu wrote: > > > > > I think non-active RWHVA can still have non null IAA? this is only > cleared > > > on > > > > in > > > > > RWHVA destructor afaict? > > > > > > > > When RenderViewHost is swapped out, SetContentViewCore(nullptr) is called > on > > > the > > > > old one that becomes non-active. Doesn't that takes care of it? > > > > > > Oh I missed ContentViewCoreImpl::RenderViewHostChanged. However that doesn't > > > cover interstitials. Actually, I'm not sure the previous code worked well > with > > > interstitials either.. > > > > Oh by 'the previous code' you mean the code before my CL? > > Yes > > > I think it worked because it is RWHVA that actively attaches its IAA to Java > IA. > > After the change it is the other way around (web contents connecting its IAA > to > > active RWHVA) - this happens when it calls RWHVA::SetContentViewCore (except > > going back from interstitial to main RWHVA). > > How previous code work in the case of interstitial detach? Previously the attaching/detaching piggypacked on RWHVA -> CVC FrameUpdateInfo to make sure the active RWHVA/IAA always gets attached, and the one going background loses java obj (and generated too many no-op attaching/detaching attempts as well). https://codereview.chromium.org/2752113005/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1872: ConnectImeAdapter(content_view_core ? content_view_core->ime_adapter() On 2017/03/23 23:17:40, boliu wrote: > > Well, what about the page in the back while the interstitial is showing? > This may not be always true but interstitial page starts with its own content/RWHVA, and then later yields to a new, main RWVHA. So I cared about the case of interstitial page being detached. Is it possible that on Android the flow begins with main RWHVA -> interstitial -> main RHWVA for one content? Couldn't test such flow since it didn't happen in testing yet.
https://codereview.chromium.org/2752113005/diff/200001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2752113005/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1872: ConnectImeAdapter(content_view_core ? content_view_core->ime_adapter() On 2017/03/24 00:01:45, Jinsuk Kim wrote: > On 2017/03/23 23:22:56, boliu wrote: > > On 2017/03/23 23:16:22, Jinsuk Kim wrote: > > > On 2017/03/23 14:18:50, boliu wrote: > > > > On 2017/03/23 07:25:10, Jinsuk Kim wrote: > > > > > On 2017/03/23 04:17:01, boliu wrote: > > > > > > I think non-active RWHVA can still have non null IAA? this is only > > cleared > > > > on > > > > > in > > > > > > RWHVA destructor afaict? > > > > > > > > > > When RenderViewHost is swapped out, SetContentViewCore(nullptr) is > called > > on > > > > the > > > > > old one that becomes non-active. Doesn't that takes care of it? > > > > > > > > Oh I missed ContentViewCoreImpl::RenderViewHostChanged. However that > doesn't > > > > cover interstitials. Actually, I'm not sure the previous code worked well > > with > > > > interstitials either.. > > > > > > Oh by 'the previous code' you mean the code before my CL? > > > > Yes > > > > > I think it worked because it is RWHVA that actively attaches its IAA to Java > > IA. > > > After the change it is the other way around (web contents connecting its IAA > > to > > > active RWHVA) - this happens when it calls RWHVA::SetContentViewCore (except > > > going back from interstitial to main RWHVA). > > > > How previous code work in the case of interstitial detach? > > Previously the attaching/detaching piggypacked on RWHVA -> CVC FrameUpdateInfo > to make sure the active RWHVA/IAA always gets attached, and the one going > background loses java obj (and generated too many no-op attaching/detaching > attempts as well). Sorry I was wrong (completely mistaken). This is not true.
On 2017/03/23 23:45:20, Changwan Ryu wrote: > To boliu@, jinsukkim@: I'm not sure about interstitial case. > WebContentsViewAndroid::Focus() seems to reattach the native ImeAdapterAndroid > to the Java counterpart, but honestly I never had a chance to test around > interstitial. There was no bug reported around that. Even though there is such a > bug, probably no interstitial we have simply don't require user input. So to clarify, two cases about interstitials (both already mentioned) that I'm not yet convinced is correct: * WebContentsViewAndroid::Focus only moves ImeAdapter *to* interstitial. What's moving ImeAdapter back when the interstitial is detached/dismissed? This seems like a problem before this CL * While interstitial is showing, what's preventing the page in the background from calling into ImeAdapter. This is a new issue in this CL. On 2017/03/24 00:01:45, Jinsuk Kim wrote: > This may not be always true but interstitial page starts with its own > content/RWHVA, and then later yields to a new, main RWVHA. It's not safe to assume there's always a new RWHVA after interstitial is dismissed. Navigation doesn't always result in a in RWHVA. fwiw, it's possible to observer the "active" RWHVA changing, through WebContentsObserver callbacks. native AwContents does this to keep track of the "active" compositor. You can look into that
On 2017/03/24 00:45:43, boliu wrote: > On 2017/03/23 23:45:20, Changwan Ryu wrote: > > To boliu@, jinsukkim@: I'm not sure about interstitial case. > > WebContentsViewAndroid::Focus() seems to reattach the native ImeAdapterAndroid > > to the Java counterpart, but honestly I never had a chance to test around > > interstitial. There was no bug reported around that. Even though there is such > a > > bug, probably no interstitial we have simply don't require user input. > > So to clarify, two cases about interstitials (both already mentioned) that I'm > not yet convinced is correct: > > * WebContentsViewAndroid::Focus only moves ImeAdapter *to* interstitial. What's > moving ImeAdapter back when the interstitial is detached/dismissed? This seems > like a problem before this CL > * While interstitial is showing, what's preventing the page in the background > from calling into ImeAdapter. This is a new issue in this CL. > > On 2017/03/24 00:01:45, Jinsuk Kim wrote: > > This may not be always true but interstitial page starts with its own > > content/RWHVA, and then later yields to a new, main RWVHA. > > It's not safe to assume there's always a new RWHVA after interstitial is > dismissed. Navigation doesn't always result in a in RWHVA. Err.. Navigation doesn't always result in a new RWHVA. ie it is possible to navigate and stay in the same RWHVA. > > > fwiw, it's possible to observer the "active" RWHVA changing, through > WebContentsObserver callbacks. native AwContents does this to keep track of the > "active" compositor. You can look into that
> jinsukkim@, what is an example interstitial case did you test? Does dangerous > URL warning takes you to an interstitial? I test interstitials with secure web sites with expired certificate. https://codereview.chromium.org/2752113005/diff/200001/content/browser/render... File content/browser/renderer_host/ime_adapter_android.cc (right): https://codereview.chromium.org/2752113005/diff/200001/content/browser/render... content/browser/renderer_host/ime_adapter_android.cc:335: return rwhva_ ? rwhva_->GetFocusedWidget() : nullptr; On 2017/03/23 23:45:19, Changwan Ryu wrote: > On 2017/03/23 07:25:10, Jinsuk Kim wrote: > > On 2017/03/23 04:17:01, boliu wrote: > > > changwan: does everything here run on the UI thread? weakptrs are not thread > > > safe > > > > I read that everything runs on UI thread. ThreadedInputConnection which runs > on > > IME thread posts every request onto UI thread before reaching here(I guess the > > missing DCHECK for UI thread in |::SendKeyEvent| is just a mistake). But I'll > > wait for changwan@ to confirm. > > Correct. Everything from ThreadedInput* to ImeAdapter should be on UI thread. > One exception is updateSelection() which goes directly to Android framework. Thanks! https://codereview.chromium.org/2752113005/diff/200001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2752113005/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1872: ConnectImeAdapter(content_view_core ? content_view_core->ime_adapter() > > > How previous code work in the case of interstitial detach? > > > > Previously the attaching/detaching piggypacked on RWHVA -> CVC FrameUpdateInfo > > to make sure the active RWHVA/IAA always gets attached, and the one going > > background loses java obj (and generated too many no-op attaching/detaching > > attempts as well). > > Sorry I was wrong (completely mistaken). This is not true. I should have said "it piggybacked on RWHVA::OnUpdateTextInputStateCalled" instead. It's still valid to say that attach/detach happens more than necessary. I'm splitting it from the state update in this CL. https://codereview.chromium.org/2752113005/diff/200001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2752113005/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:691: mImeAdapter.destroy(); On 2017/03/23 04:17:01, boliu wrote: > that still doesn't work for webview with gc. by the time native CVC is > destroyed, java CVC is has long been garbage collected > > I think you want native CVC owning native IAA instead Done.
On 2017/03/24 00:45:43, boliu wrote: > On 2017/03/23 23:45:20, Changwan Ryu wrote: > > To boliu@, jinsukkim@: I'm not sure about interstitial case. > > WebContentsViewAndroid::Focus() seems to reattach the native ImeAdapterAndroid > > to the Java counterpart, but honestly I never had a chance to test around > > interstitial. There was no bug reported around that. Even though there is such > a > > bug, probably no interstitial we have simply don't require user input. > > So to clarify, two cases about interstitials (both already mentioned) that I'm > not yet convinced is correct: > > * WebContentsViewAndroid::Focus only moves ImeAdapter *to* interstitial. What's > moving ImeAdapter back when the interstitial is detached/dismissed? This seems > like a problem before this CL I verified from the code that this was handled when attaching the native IAA to Java IA. The Java object was keeping the old (soon-to-be-replaced) native pointer, so use it to reset the reference to Java obj in the native IAA, hence effectively detach it when it attaches a new, active IAA. This works not just for interstitial but for RWHVA update in general. > * While interstitial is showing, what's preventing the page in the background > from calling into ImeAdapter. This is a new issue in this CL. > Valid concern since it was not handled. Addressed it in a similar way, by keeping a pointer to active RWHVA in CVCImpl, and use it to invoke RWHVA::ConnectImeAdapter(nullptr) upon attaching interstitial. Other RWHVA update is handled in RenerViewHostChanged, as described before. > On 2017/03/24 00:01:45, Jinsuk Kim wrote: > > This may not be always true but interstitial page starts with its own > > content/RWHVA, and then later yields to a new, main RWVHA. > > It's not safe to assume there's always a new RWHVA after interstitial is > dismissed. Navigation doesn't always result in a in RWHVA. > > > fwiw, it's possible to observer the "active" RWHVA changing, through > WebContentsObserver callbacks. native AwContents does this to keep track of the > "active" compositor. You can look into that
https://codereview.chromium.org/2752113005/diff/240001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2752113005/diff/240001/content/browser/androi... content/browser/android/content_view_core_impl.cc:219: ime_adapter_(ime_adapter), this is backwards If CVC owns ime adapter, then CVC, then CVC should be the thing that creates it and hands out pointers to it Actually, maybe should seriously think about pulling ime out of CVC entirely. Native adapter can be a WebContentsUserData, and native can "own" java. https://codereview.chromium.org/2752113005/diff/240001/content/browser/androi... content/browser/android/content_view_core_impl.cc:334: active_rwhva_ = GetRenderWidgetHostViewAndroid(); need to update this in RenderViewHostChanged as well https://codereview.chromium.org/2752113005/diff/240001/content/browser/androi... content/browser/android/content_view_core_impl.cc:376: void ContentViewCoreImpl::DidAttachInterstitialPage() { should this stuff live in the ImeAdapter instead? I thought we want to remove stuff from CVC.. https://codereview.chromium.org/2752113005/diff/240001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2752113005/diff/240001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:751: DCHECK(ime_adapter_android_); what makes these DCHECKs hold? https://codereview.chromium.org/2752113005/diff/240001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1882: ConnectImeAdapter(content_view_core ? content_view_core->ime_adapter() is this call still needed? if so, can we move this somewhere to where the rest of ConnectImeAdapter calls are? https://codereview.chromium.org/2752113005/diff/240001/content/browser/web_co... File content/browser/web_contents/web_contents_view_android.cc (right): https://codereview.chromium.org/2752113005/diff/240001/content/browser/web_co... content/browser/web_contents/web_contents_view_android.cc:189: rwhv->ConnectImeAdapter(content_view_core_->ime_adapter()); ditto, is this still needed? https://codereview.chromium.org/2752113005/diff/240001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2752113005/diff/240001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:786: empty line change..
Moved ime_adapter_android.{cc,h} from content/browser/renderer_host to
content/browser/android
https://codereview.chromium.org/2752113005/diff/240001/content/browser/androi...
File content/browser/android/content_view_core_impl.cc (right):
https://codereview.chromium.org/2752113005/diff/240001/content/browser/androi...
content/browser/android/content_view_core_impl.cc:219:
ime_adapter_(ime_adapter),
On 2017/03/24 17:13:16, boliu wrote:
> this is backwards
>
> If CVC owns ime adapter, then CVC, then CVC should be the thing that creates
it
> and hands out pointers to it
>
> Actually, maybe should seriously think about pulling ime out of CVC entirely.
> Native adapter can be a WebContentsUserData, and native can "own" java.
Assuming that 'creator should be owner' should hold true, it is a bit tricky for
the native to create own Java. Java IA needs to be created in java CVC as it is
tightly linked to it (dependent on many CVC internal states).
In the new patch I'm destroying the IAA in
WebContentsObserver::WebContentsDestroyed. See if that looks better.
https://codereview.chromium.org/2752113005/diff/240001/content/browser/androi...
content/browser/android/content_view_core_impl.cc:334: active_rwhva_ =
GetRenderWidgetHostViewAndroid();
On 2017/03/24 17:13:16, boliu wrote:
> need to update this in RenderViewHostChanged as well
Done.
https://codereview.chromium.org/2752113005/diff/240001/content/browser/androi...
content/browser/android/content_view_core_impl.cc:376: void
ContentViewCoreImpl::DidAttachInterstitialPage() {
On 2017/03/24 17:13:16, boliu wrote:
> should this stuff live in the ImeAdapter instead? I thought we want to remove
> stuff from CVC..
Nice. IAA being WebContentsObserver helps decouple itself from CVCImpl. Done.
https://codereview.chromium.org/2752113005/diff/240001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):
https://codereview.chromium.org/2752113005/diff/240001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.cc:751:
DCHECK(ime_adapter_android_);
On 2017/03/24 17:13:16, boliu wrote:
> what makes these DCHECKs hold?
It should always be set by the time any Ime calls come in here. In fact I don't
think I need DCHECK at all. Removed.
https://codereview.chromium.org/2752113005/diff/240001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.cc:1882:
ConnectImeAdapter(content_view_core ? content_view_core->ime_adapter()
On 2017/03/24 17:13:16, boliu wrote:
> is this call still needed? if so, can we move this somewhere to where the rest
> of ConnectImeAdapter calls are?
Not necessary any more as IAA is now observing web contents directly. Removed.
https://codereview.chromium.org/2752113005/diff/240001/content/browser/web_co...
File content/browser/web_contents/web_contents_view_android.cc (right):
https://codereview.chromium.org/2752113005/diff/240001/content/browser/web_co...
content/browser/web_contents/web_contents_view_android.cc:189:
rwhv->ConnectImeAdapter(content_view_core_->ime_adapter());
On 2017/03/24 17:13:16, boliu wrote:
> ditto, is this still needed?
No. removed.
https://codereview.chromium.org/2752113005/diff/240001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):
https://codereview.chromium.org/2752113005/diff/240001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:786:
On 2017/03/24 17:13:16, boliu wrote:
> empty line change..
Done.
cool, lifetime much saner now https://codereview.chromium.org/2752113005/diff/240001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2752113005/diff/240001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:751: DCHECK(ime_adapter_android_); On 2017/03/27 03:19:33, Jinsuk Kim wrote: > On 2017/03/24 17:13:16, boliu wrote: > > what makes these DCHECKs hold? > > It should always be set by the time any Ime calls come in here. In fact I don't > think I need DCHECK at all. Removed. The is not answering my question. In the pop up flow where RWHVA is created before CVC/WC, so it's not clear to me this will hold in that case. https://codereview.chromium.org/2752113005/diff/260001/content/browser/androi... File content/browser/android/ime_adapter_android.cc (right): https://codereview.chromium.org/2752113005/diff/260001/content/browser/androi... content/browser/android/ime_adapter_android.cc:7: #include <android/input.h> that's not alphabetical order? https://codereview.chromium.org/2752113005/diff/260001/content/browser/androi... content/browser/android/ime_adapter_android.cc:156: if (new_host) { should set to null in the else case? https://codereview.chromium.org/2752113005/diff/260001/content/browser/androi... File content/browser/android/ime_adapter_android.h (right): https://codereview.chromium.org/2752113005/diff/260001/content/browser/androi... content/browser/android/ime_adapter_android.h:121: WebContentsImpl* web_contents_; you don't need this. WebContentsObserver has a web_contents method, and you can cast it to impl if needed
https://codereview.chromium.org/2752113005/diff/240001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2752113005/diff/240001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:751: DCHECK(ime_adapter_android_); On 2017/03/27 17:18:12, boliu wrote: > On 2017/03/27 03:19:33, Jinsuk Kim wrote: > > On 2017/03/24 17:13:16, boliu wrote: > > > what makes these DCHECKs hold? > > > > It should always be set by the time any Ime calls come in here. In fact I > don't > > think I need DCHECK at all. Removed. > > The is not answering my question. > > In the pop up flow where RWHVA is created before CVC/WC, so it's not clear to me > this will hold in that case. Hm, haven't checked such case. It would have been checked at java-native boundary before this CL. Then I think it is safe to add the null checking here to the same effect. https://codereview.chromium.org/2752113005/diff/260001/content/browser/androi... File content/browser/android/ime_adapter_android.cc (right): https://codereview.chromium.org/2752113005/diff/260001/content/browser/androi... content/browser/android/ime_adapter_android.cc:7: #include <android/input.h> On 2017/03/27 17:18:12, boliu wrote: > that's not alphabetical order? Thought so too but |git cl format| insists this is right... tired of reverting it. Reverted once again. https://codereview.chromium.org/2752113005/diff/260001/content/browser/androi... content/browser/android/ime_adapter_android.cc:156: if (new_host) { On 2017/03/27 17:18:12, boliu wrote: > should set to null in the else case? Not checking new_host does the trick. Done. https://codereview.chromium.org/2752113005/diff/260001/content/browser/androi... File content/browser/android/ime_adapter_android.h (right): https://codereview.chromium.org/2752113005/diff/260001/content/browser/androi... content/browser/android/ime_adapter_android.h:121: WebContentsImpl* web_contents_; On 2017/03/27 17:18:12, boliu wrote: > you don't need this. WebContentsObserver has a web_contents method, and you can > cast it to impl if needed Removed.
https://codereview.chromium.org/2752113005/diff/260001/content/browser/androi... File content/browser/android/ime_adapter_android.cc (right): https://codereview.chromium.org/2752113005/diff/260001/content/browser/androi... content/browser/android/ime_adapter_android.cc:7: #include <android/input.h> On 2017/03/27 22:38:19, Jinsuk Kim wrote: > On 2017/03/27 17:18:12, boliu wrote: > > that's not alphabetical order? > > Thought so too but |git cl format| insists this is right... tired of reverting > it. Reverted once again. Oh, keep it if clang-format did it.. https://codereview.chromium.org/2752113005/diff/300001/content/browser/androi... File content/browser/android/ime_adapter_android.cc (right): https://codereview.chromium.org/2752113005/diff/300001/content/browser/androi... content/browser/android/ime_adapter_android.cc:157: new_host->GetWidget()->GetView())); that doesn't work, this will crash if new_host is null
PTAL https://codereview.chromium.org/2752113005/diff/260001/content/browser/androi... File content/browser/android/ime_adapter_android.cc (right): https://codereview.chromium.org/2752113005/diff/260001/content/browser/androi... content/browser/android/ime_adapter_android.cc:7: #include <android/input.h> On 2017/03/27 22:49:29, boliu wrote: > On 2017/03/27 22:38:19, Jinsuk Kim wrote: > > On 2017/03/27 17:18:12, boliu wrote: > > > that's not alphabetical order? > > > > Thought so too but |git cl format| insists this is right... tired of reverting > > it. Reverted once again. > > Oh, keep it if clang-format did it.. Done. I was kind of curious what you would said :) https://codereview.chromium.org/2752113005/diff/300001/content/browser/androi... File content/browser/android/ime_adapter_android.cc (right): https://codereview.chromium.org/2752113005/diff/300001/content/browser/androi... content/browser/android/ime_adapter_android.cc:157: new_host->GetWidget()->GetView())); On 2017/03/27 22:49:29, boliu wrote: > that doesn't work, this will crash if new_host is null Oops. Fixed.
lgtm
jinsukkim@chromium.org changed reviewers: + tedchoc@chromium.org
tedchoc@ would you give owner's seal for chrome/? It's not necessary for Tab to attach ime adapter explicitly any more since it is now taken care of by ImeAdpater itself.
On 2017/03/27 23:07:19, Jinsuk Kim wrote: > tedchoc@ would you give owner's seal for chrome/? It's not necessary for Tab to > attach ime adapter explicitly any more since it is now taken care of by > ImeAdpater itself. chrome/ - lgtm
The CQ bit was checked by jinsukkim@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by jinsukkim@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.
The CQ bit was checked by jinsukkim@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org, changwan@chromium.org, tedchoc@chromium.org, boliu@chromium.org Link to the patchset: https://codereview.chromium.org/2752113005/#ps340001 (title: "fix tests")
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": 340001, "attempt_start_ts": 1490681888502750,
"parent_rev": "9c91326aed6eb90617cebd1180d26cd4c6a41d00", "commit_rev":
"153d2811b3cbcf69cda7ab4817cdc35e7c680dee"}
Message was sent while issue was closed.
Description was changed from ========== Let ImeAdapterAndroid have the same lifecycle as its Java peer The native ImeAdapterAndroid(IAA) instance is owned by RenderWidgetHostViewAndroid(RWHVA) while its Java peer (ImeAdapter) is owned by CVC. This causes their life cycles to be different. Attach/detach API are used to get them linked before talking to each other. This CL makes this mechanism simpler by having Java ImeAdapter class create the native together, hence gets their lifetime synced. The separate attach/detach mechanism is not necessary since the linking is done as a part of WCVA - RWHVA association job. i.e. IAA is linked to the current RWHVA when the corresponding CVCImpl is set to RWHVA (in the future this will be done separately as RWHVA will be rid of all the references to CVC). The only situation where the linking should be done "manually" is when IAA gets (temporarily) linked to RWHVA for an interstitial page. The link between IAA and the main RWHVA gets restored once the interstitial page is detached. BUG=662908 ========== to ========== Let ImeAdapterAndroid have the same lifecycle as its Java peer The native ImeAdapterAndroid(IAA) instance is owned by RenderWidgetHostViewAndroid(RWHVA) while its Java peer (ImeAdapter) is owned by CVC. This causes their life cycles to be different. Attach/detach API are used to get them linked before talking to each other. This CL makes this mechanism simpler by having Java ImeAdapter class create the native together, hence gets their lifetime synced. The separate attach/detach mechanism is not necessary since the linking is done as a part of WCVA - RWHVA association job. i.e. IAA is linked to the current RWHVA when the corresponding CVCImpl is set to RWHVA (in the future this will be done separately as RWHVA will be rid of all the references to CVC). The only situation where the linking should be done "manually" is when IAA gets (temporarily) linked to RWHVA for an interstitial page. The link between IAA and the main RWHVA gets restored once the interstitial page is detached. BUG=662908 Review-Url: https://codereview.chromium.org/2752113005 Cr-Commit-Position: refs/heads/master@{#460028} Committed: https://chromium.googlesource.com/chromium/src/+/153d2811b3cbcf69cda7ab4817cd... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:340001) as https://chromium.googlesource.com/chromium/src/+/153d2811b3cbcf69cda7ab4817cd...
Message was sent while issue was closed.
A revert of this CL (patchset #12 id:340001) has been created in https://codereview.chromium.org/2785543003/ by mdjones@chromium.org. The reason for reverting is: Suspected breaking Marshmallow 64 bit tester: https://uberchromegw.corp.google.com/i/chromium.android/builders/Marshmallow%....
Message was sent while issue was closed.
On 2017/03/29 00:16:58, mdjones wrote: > A revert of this CL (patchset #12 id:340001) has been created in > https://codereview.chromium.org/2785543003/ by mailto:mdjones@chromium.org. > > The reason for reverting is: Suspected breaking Marshmallow 64 bit tester: > https://uberchromegw.corp.google.com/i/chromium.android/builders/Marshmallow%.... Tested locally with the same config as the bot 'Marshmallow 64 bit Tester' (MMB29Q on bullhead - also tested on shamu) but can't reproduce the 3 failing tests - they all pass. Is there anything else I can do before just trying landing again and hope for the best? I looked through the documents about running buildbot locally, and tried a few things but I'm not sure if that's what I need...
I suspect that was a random flake as there's no obvious connection between the failures and this patch, so let's try to just reland.
The CQ bit was checked by aelias@chromium.org
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
Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jinsukkim@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: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Let ImeAdapterAndroid have the same lifecycle as its Java peer The native ImeAdapterAndroid(IAA) instance is owned by RenderWidgetHostViewAndroid(RWHVA) while its Java peer (ImeAdapter) is owned by CVC. This causes their life cycles to be different. Attach/detach API are used to get them linked before talking to each other. This CL makes this mechanism simpler by having Java ImeAdapter class create the native together, hence gets their lifetime synced. The separate attach/detach mechanism is not necessary since the linking is done as a part of WCVA - RWHVA association job. i.e. IAA is linked to the current RWHVA when the corresponding CVCImpl is set to RWHVA (in the future this will be done separately as RWHVA will be rid of all the references to CVC). The only situation where the linking should be done "manually" is when IAA gets (temporarily) linked to RWHVA for an interstitial page. The link between IAA and the main RWHVA gets restored once the interstitial page is detached. BUG=662908 Review-Url: https://codereview.chromium.org/2752113005 Cr-Commit-Position: refs/heads/master@{#460028} Committed: https://chromium.googlesource.com/chromium/src/+/153d2811b3cbcf69cda7ab4817cd... ========== to ========== Let ImeAdapterAndroid have the same lifecycle as its Java peer The native ImeAdapterAndroid(IAA) instance is owned by RenderWidgetHostViewAndroid(RWHVA) while its Java peer (ImeAdapter) is owned by CVC. This causes their life cycles to be different. Attach/detach API are used to get them linked before talking to each other. This CL makes this mechanism simpler by having Java ImeAdapter class create the native together, hence gets their lifetime synced. The separate attach/detach mechanism is not necessary since the native is an WebContentObserver, and connected to (or disconnected from) RWHVA accordingly when notified of RenderView- Host update callback. Connecting to interstitial page is also taken care of by observing the appropriate callbacks. BUG=662908 Review-Url: https://codereview.chromium.org/2752113005 Cr-Commit-Position: refs/heads/master@{#460028} Committed: https://chromium.googlesource.com/chromium/src/+/153d2811b3cbcf69cda7ab4817cd... ==========
The CQ bit was checked by jinsukkim@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, aelias@chromium.org, changwan@chromium.org, boliu@chromium.org Link to the patchset: https://codereview.chromium.org/2752113005/#ps360001 (title: "rebase")
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": 360001, "attempt_start_ts": 1490930635030330,
"parent_rev": "fcec89d6f03db12e55e42db9d5bfc9a0d7edbc99", "commit_rev":
"d01dcc6a7955b96aff2083461319208afc2bc9ca"}
Message was sent while issue was closed.
Description was changed from ========== Let ImeAdapterAndroid have the same lifecycle as its Java peer The native ImeAdapterAndroid(IAA) instance is owned by RenderWidgetHostViewAndroid(RWHVA) while its Java peer (ImeAdapter) is owned by CVC. This causes their life cycles to be different. Attach/detach API are used to get them linked before talking to each other. This CL makes this mechanism simpler by having Java ImeAdapter class create the native together, hence gets their lifetime synced. The separate attach/detach mechanism is not necessary since the native is an WebContentObserver, and connected to (or disconnected from) RWHVA accordingly when notified of RenderView- Host update callback. Connecting to interstitial page is also taken care of by observing the appropriate callbacks. BUG=662908 Review-Url: https://codereview.chromium.org/2752113005 Cr-Commit-Position: refs/heads/master@{#460028} Committed: https://chromium.googlesource.com/chromium/src/+/153d2811b3cbcf69cda7ab4817cd... ========== to ========== Let ImeAdapterAndroid have the same lifecycle as its Java peer The native ImeAdapterAndroid(IAA) instance is owned by RenderWidgetHostViewAndroid(RWHVA) while its Java peer (ImeAdapter) is owned by CVC. This causes their life cycles to be different. Attach/detach API are used to get them linked before talking to each other. This CL makes this mechanism simpler by having Java ImeAdapter class create the native together, hence gets their lifetime synced. The separate attach/detach mechanism is not necessary since the native is an WebContentObserver, and connected to (or disconnected from) RWHVA accordingly when notified of RenderView- Host update callback. Connecting to interstitial page is also taken care of by observing the appropriate callbacks. BUG=662908 Review-Url: https://codereview.chromium.org/2752113005 Cr-Original-Commit-Position: refs/heads/master@{#460028} Committed: https://chromium.googlesource.com/chromium/src/+/153d2811b3cbcf69cda7ab4817cd... Review-Url: https://codereview.chromium.org/2752113005 Cr-Commit-Position: refs/heads/master@{#461024} Committed: https://chromium.googlesource.com/chromium/src/+/d01dcc6a7955b96aff2083461319... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:360001) as https://chromium.googlesource.com/chromium/src/+/d01dcc6a7955b96aff2083461319...
Message was sent while issue was closed.
On 2017/03/31 04:19:53, commit-bot: I haz the power wrote: > Committed patchset #13 (id:360001) as > https://chromium.googlesource.com/chromium/src/+/d01dcc6a7955b96aff2083461319... Landed again. Will keep an eye on the buildbot for sometime.
Message was sent while issue was closed.
On 2017/03/31 04:24:29, Jinsuk Kim wrote: > On 2017/03/31 04:19:53, commit-bot: I haz the power wrote: > > Committed patchset #13 (id:360001) as > > > https://chromium.googlesource.com/chromium/src/+/d01dcc6a7955b96aff2083461319... > > Landed again. Will keep an eye on the buildbot for sometime. Unfortunately the 3 tests started failing again with my CL: https://build.chromium.org/p/chromium.android/builders/Marshmallow%2064%20bit... I'll wait a few more build results and then revert it if the failure persists. Haven't tried this, but setting up a buildbot locally and adding marshmallow 64bit tester config in tryserver may help debugging.
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:360001) has been created in https://codereview.chromium.org/2791603002/ by jinsukkim@chromium.org. The reason for reverting is: Relanded but the test failure persists: https://build.chromium.org/p/chromium.android/builders/Marshmallow%2064%20bit....
Message was sent while issue was closed.
Description was changed from ========== Let ImeAdapterAndroid have the same lifecycle as its Java peer The native ImeAdapterAndroid(IAA) instance is owned by RenderWidgetHostViewAndroid(RWHVA) while its Java peer (ImeAdapter) is owned by CVC. This causes their life cycles to be different. Attach/detach API are used to get them linked before talking to each other. This CL makes this mechanism simpler by having Java ImeAdapter class create the native together, hence gets their lifetime synced. The separate attach/detach mechanism is not necessary since the native is an WebContentObserver, and connected to (or disconnected from) RWHVA accordingly when notified of RenderView- Host update callback. Connecting to interstitial page is also taken care of by observing the appropriate callbacks. BUG=662908 Review-Url: https://codereview.chromium.org/2752113005 Cr-Original-Commit-Position: refs/heads/master@{#460028} Committed: https://chromium.googlesource.com/chromium/src/+/153d2811b3cbcf69cda7ab4817cd... Review-Url: https://codereview.chromium.org/2752113005 Cr-Commit-Position: refs/heads/master@{#461024} Committed: https://chromium.googlesource.com/chromium/src/+/d01dcc6a7955b96aff2083461319... ========== to ========== Let ImeAdapterAndroid have the same lifecycle as its Java peer The native ImeAdapterAndroid(IAA) instance is owned by RenderWidgetHostViewAndroid(RWHVA) while its Java peer (ImeAdapter) is owned by CVC. This causes their life cycles to be different. Attach/detach API are used to get them linked before talking to each other. This CL makes this mechanism simpler by having Java ImeAdapter class create the native together, hence gets their lifetime synced. The separate attach/detach mechanism is not necessary since the native is an WebContentObserver, and connected to (or disconnected from) RWHVA accordingly when notified of RenderView- Host update callback. Connecting to interstitial page is also taken care of by observing the appropriate callbacks. BUG=662908 Review-Url: https://codereview.chromium.org/2752113005 Cr-Original-Commit-Position: refs/heads/master@{#460028} Committed: https://chromium.googlesource.com/chromium/src/+/153d2811b3cbcf69cda7ab4817cd... Review-Url: https://codereview.chromium.org/2752113005 Cr-Commit-Position: refs/heads/master@{#461024} Committed: https://chromium.googlesource.com/chromium/src/+/d01dcc6a7955b96aff2083461319... ==========
The CQ bit was checked by jinsukkim@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.
RWHVA was observed being destroyed before ImeAdapter accesses the old RWHVA it was referencing, therefore can cause memory corruption. It is more apparent in 64 bit tester. Added a method that resets ImeAdapter's |rwhva_| once the instance gets destroyed. PTAL
https://codereview.chromium.org/2752113005/diff/400001/content/browser/androi... File content/browser/android/ime_adapter_android.cc (right): https://codereview.chromium.org/2752113005/diff/400001/content/browser/androi... content/browser/android/ime_adapter_android.cc:176: void ImeAdapterAndroid::ResetRenderProcessConnection( does RenderViewDeleted work?
https://codereview.chromium.org/2752113005/diff/400001/content/browser/androi... File content/browser/android/ime_adapter_android.cc (right): https://codereview.chromium.org/2752113005/diff/400001/content/browser/androi... content/browser/android/ime_adapter_android.cc:176: void ImeAdapterAndroid::ResetRenderProcessConnection( On 2017/04/04 15:34:22, boliu wrote: > does RenderViewDeleted work? Err, I mean WebContentsObserver::RenderViewDeleted
https://codereview.chromium.org/2752113005/diff/400001/content/browser/androi... File content/browser/android/ime_adapter_android.cc (right): https://codereview.chromium.org/2752113005/diff/400001/content/browser/androi... content/browser/android/ime_adapter_android.cc:176: void ImeAdapterAndroid::ResetRenderProcessConnection( On 2017/04/04 15:34:53, boliu wrote: > On 2017/04/04 15:34:22, boliu wrote: > > does RenderViewDeleted work? > > Err, I mean WebContentsObserver::RenderViewDeleted I also checked it out, but RWHVA from the passed render_view_host (render_view_host->GetWidget()->GetView()) was null. So couldn't use it.
https://codereview.chromium.org/2752113005/diff/400001/content/browser/androi... File content/browser/android/ime_adapter_android.cc (right): https://codereview.chromium.org/2752113005/diff/400001/content/browser/androi... content/browser/android/ime_adapter_android.cc:176: void ImeAdapterAndroid::ResetRenderProcessConnection( On 2017/04/04 22:12:09, Jinsuk Kim wrote: > On 2017/04/04 15:34:53, boliu wrote: > > On 2017/04/04 15:34:22, boliu wrote: > > > does RenderViewDeleted work? > > > > Err, I mean WebContentsObserver::RenderViewDeleted > > I also checked it out, but RWHVA from the passed render_view_host > (render_view_host->GetWidget()->GetView()) was null. So couldn't use it. hmm, RWHVA lifetime is a bit complicated, but since it supports weakptr, and there's null checks here already, seems like the simpler solution is to hold a weakptr here instead
https://codereview.chromium.org/2752113005/diff/400001/content/browser/androi... File content/browser/android/ime_adapter_android.cc (right): https://codereview.chromium.org/2752113005/diff/400001/content/browser/androi... content/browser/android/ime_adapter_android.cc:176: void ImeAdapterAndroid::ResetRenderProcessConnection( On 2017/04/04 22:22:06, boliu wrote: > On 2017/04/04 22:12:09, Jinsuk Kim wrote: > > On 2017/04/04 15:34:53, boliu wrote: > > > On 2017/04/04 15:34:22, boliu wrote: > > > > does RenderViewDeleted work? > > > > > > Err, I mean WebContentsObserver::RenderViewDeleted > > > > I also checked it out, but RWHVA from the passed render_view_host > > (render_view_host->GetWidget()->GetView()) was null. So couldn't use it. > > hmm, RWHVA lifetime is a bit complicated, but since it supports weakptr, and > there's null checks here already, seems like the simpler solution is to hold a > weakptr here instead Done.
lgtm % rename https://codereview.chromium.org/2752113005/diff/420001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2752113005/diff/420001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:234: base::WeakPtr<RenderWidgetHostViewAndroid> GetWeakPointer(); base already has GetWeakPtr. rename this to GetWeakPointerAndroid or something like that? to avoid overloading
https://codereview.chromium.org/2752113005/diff/420001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2752113005/diff/420001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:234: base::WeakPtr<RenderWidgetHostViewAndroid> GetWeakPointer(); On 2017/04/04 23:19:32, boliu wrote: > base already has GetWeakPtr. rename this to GetWeakPointerAndroid or something > like that? to avoid overloading Done.
The CQ bit was checked by jinsukkim@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.
The CQ bit was checked by jinsukkim@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, aelias@chromium.org, changwan@chromium.org, boliu@chromium.org Link to the patchset: https://codereview.chromium.org/2752113005/#ps460001 (title: "fix tests")
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": 460001, "attempt_start_ts": 1491365136198780,
"parent_rev": "a1052a48bc85cc5d68c0f7dc0782b577e92cc9c7", "commit_rev":
"48f82db77c454a66084b056b4b67d103a681a5e8"}
Message was sent while issue was closed.
Description was changed from ========== Let ImeAdapterAndroid have the same lifecycle as its Java peer The native ImeAdapterAndroid(IAA) instance is owned by RenderWidgetHostViewAndroid(RWHVA) while its Java peer (ImeAdapter) is owned by CVC. This causes their life cycles to be different. Attach/detach API are used to get them linked before talking to each other. This CL makes this mechanism simpler by having Java ImeAdapter class create the native together, hence gets their lifetime synced. The separate attach/detach mechanism is not necessary since the native is an WebContentObserver, and connected to (or disconnected from) RWHVA accordingly when notified of RenderView- Host update callback. Connecting to interstitial page is also taken care of by observing the appropriate callbacks. BUG=662908 Review-Url: https://codereview.chromium.org/2752113005 Cr-Original-Commit-Position: refs/heads/master@{#460028} Committed: https://chromium.googlesource.com/chromium/src/+/153d2811b3cbcf69cda7ab4817cd... Review-Url: https://codereview.chromium.org/2752113005 Cr-Commit-Position: refs/heads/master@{#461024} Committed: https://chromium.googlesource.com/chromium/src/+/d01dcc6a7955b96aff2083461319... ========== to ========== Let ImeAdapterAndroid have the same lifecycle as its Java peer The native ImeAdapterAndroid(IAA) instance is owned by RenderWidgetHostViewAndroid(RWHVA) while its Java peer (ImeAdapter) is owned by CVC. This causes their life cycles to be different. Attach/detach API are used to get them linked before talking to each other. This CL makes this mechanism simpler by having Java ImeAdapter class create the native together, hence gets their lifetime synced. The separate attach/detach mechanism is not necessary since the native is an WebContentObserver, and connected to (or disconnected from) RWHVA accordingly when notified of RenderView- Host update callback. Connecting to interstitial page is also taken care of by observing the appropriate callbacks. BUG=662908 Review-Url: https://codereview.chromium.org/2752113005 Cr-Original-Original-Commit-Position: refs/heads/master@{#460028} Committed: https://chromium.googlesource.com/chromium/src/+/153d2811b3cbcf69cda7ab4817cd... Review-Url: https://codereview.chromium.org/2752113005 Cr-Original-Commit-Position: refs/heads/master@{#461024} Committed: https://chromium.googlesource.com/chromium/src/+/d01dcc6a7955b96aff2083461319... Review-Url: https://codereview.chromium.org/2752113005 Cr-Commit-Position: refs/heads/master@{#461975} Committed: https://chromium.googlesource.com/chromium/src/+/48f82db77c454a66084b056b4b67... ==========
Message was sent while issue was closed.
Committed patchset #18 (id:460001) as https://chromium.googlesource.com/chromium/src/+/48f82db77c454a66084b056b4b67... |
