Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2792063003:
Factor out RenderWidgetHostConnector (Closed)
|
Created:
3 years, 8 months ago by Jinsuk Kim Modified:
3 years, 8 months ago Reviewers:
boliu CC:
chromium-reviews, yusukes+watch_chromium.org, shuchen+watch_chromium.org, jam, nona+watch_chromium.org, darin-cc_chromium.org, agrieve+watch_chromium.org, James Su Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFactor out RenderWidgetHostConnector
Factored out RenderWidgetHostConnector from ImeAdapter with an intent
to share the functionally with other classes. Upcoming SelectionPopup-
Controller will also inherit it to manage its connection to Render-
WidgetHostView.
BUG=662908
Review-Url: https://codereview.chromium.org/2792063003
Cr-Commit-Position: refs/heads/master@{#465180}
Committed: https://chromium.googlesource.com/chromium/src/+/7e377438d1c86ec06009e10d24a88ea84342e594
Patch Set 1 #Patch Set 2 : fix tests #Patch Set 3 : format #
Total comments: 16
Patch Set 4 : RenderWidgetHostConnnector #
Total comments: 7
Patch Set 5 : rebase #Patch Set 6 : no rwhva() #Patch Set 7 : no weakptr #Patch Set 8 : test #
Total comments: 1
Patch Set 9 : rebase #Patch Set 10 : composed connector #
Total comments: 1
Patch Set 11 : destruction observer #
Total comments: 10
Patch Set 12 : class #
Total comments: 6
Patch Set 13 : observer in ::Destroy #
Total comments: 10
Patch Set 14 : dcheck swap #
Total comments: 4
Patch Set 15 : RenderWidgetHostObserver #Patch Set 16 : private observer #
Total comments: 8
Patch Set 17 : add tests back #Patch Set 18 : fix tests #Patch Set 19 : fix more tests #Patch Set 20 : fix remaining tests #
Total comments: 4
Patch Set 21 : add more test #Patch Set 22 : no interstitial flag/var #
Total comments: 5
Patch Set 23 : base::ObserverList #Messages
Total messages: 106 (49 generated)
Description was changed from ========== Pull SelectionPopupController out ov ContentViewCore BUG= ========== to ========== Bypass ContentViewCore for SelectionPopupController update Defined the native part of SelectionPopupController to handle selection events without going through ContentViewCore. As the basic skeleton used to connect to RenderWidgetHostViewAndroid is similar to that used for ImeAdapterAndroid, factored the common part out to RenderWidgetHostViewConnector. The change is illustrated here https://goo.gl/dIygxh Renamed |selection_controller_| to |touch_selection_controller_| to reduce the possible confusion against |selection_popup_controller_|. ==========
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 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...
Patchset #5 (id:80001) has been deleted
Description was changed from ========== Bypass ContentViewCore for SelectionPopupController update Defined the native part of SelectionPopupController to handle selection events without going through ContentViewCore. As the basic skeleton used to connect to RenderWidgetHostViewAndroid is similar to that used for ImeAdapterAndroid, factored the common part out to RenderWidgetHostViewConnector. The change is illustrated here https://goo.gl/dIygxh Renamed |selection_controller_| to |touch_selection_controller_| to reduce the possible confusion against |selection_popup_controller_|. ========== to ========== Bypass ContentViewCore for SelectionPopupController update Defined the native part of SelectionPopupController to handle selection events without going through ContentViewCore. As the basic skeleton used to connect to RenderWidgetHostViewAndroid is similar to that used for ImeAdapterAndroid, factored the common part out to RenderProcessConnector. The change is illustrated here https://goo.gl/dIygxh Renamed |selection_controller_| to |touch_selection_controller_| to reduce the possible confusion against |selection_popup_controller_|. ==========
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
jinsukkim@chromium.org changed reviewers: + boliu@chromium.org, tedchoc@chromium.org
boliu@chromium.org: Please review changes in content/ tedchoc@chromium.org: Please review changes in chrome/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
skimmed this I think this can be broken up into smaller pieces: factor out the connector class, with tests rename re-route calls for SelectionPopupController https://codereview.chromium.org/2792063003/diff/120001/content/browser/androi... File content/browser/android/ime_adapter_android.cc (right): https://codereview.chromium.org/2792063003/diff/120001/content/browser/androi... content/browser/android/ime_adapter_android.cc:125: UpdateRenderProcessConnection(rwhva(), nullptr); I think RenderProcessConnector can do this part as well. Connector already knows all changes. Yes its destructor should not call virtual methods, but WebContentsDestroyed can https://codereview.chromium.org/2792063003/diff/120001/content/browser/androi... content/browser/android/ime_adapter_android.cc:129: RenderProcessConnector::RenderViewReady(); shouldn't this just be the first call of UpdateRenderProcessConnection? https://codereview.chromium.org/2792063003/diff/120001/content/browser/androi... File content/browser/android/ime_adapter_android.h (right): https://codereview.chromium.org/2792063003/diff/120001/content/browser/androi... content/browser/android/ime_adapter_android.h:88: // RendetWidgetHostViewBinder implementation. wrong name? https://codereview.chromium.org/2792063003/diff/120001/content/browser/androi... File content/browser/android/render_process_connector.h (right): https://codereview.chromium.org/2792063003/diff/120001/content/browser/androi... content/browser/android/render_process_connector.h:24: class RenderProcessConnector : public WebContentsObserver { this has nothing to do with RenderProcess though, since RWHVA lifetime is totally different from the actual renderer process RenderWidgetHostConnector? Also I think composition over inheritance applies here for WebContentsObserver, don't want to make all methods of Observer available to subclasses https://codereview.chromium.org/2792063003/diff/120001/content/browser/androi... content/browser/android/render_process_connector.h:26: RenderProcessConnector(WebContents* web_contents); explicit https://codereview.chromium.org/2792063003/diff/120001/content/browser/androi... content/browser/android/render_process_connector.h:48: RenderWidgetHostViewAndroid* rwhva() const { return rwhva_.get(); } Hmm, people will forget the null check in the future. How about separate HasFooView and GetFooView, where Get DCHECKs it's not null? Or just GetFooViewMaybeNull? https://codereview.chromium.org/2792063003/diff/120001/content/browser/androi... content/browser/android/render_process_connector.h:54: base::WeakPtr<RenderWidgetHostViewAndroid> rwhva_; DISALLOW_
https://codereview.chromium.org/2792063003/diff/120001/content/browser/androi... File content/browser/android/render_process_connector.h (right): https://codereview.chromium.org/2792063003/diff/120001/content/browser/androi... content/browser/android/render_process_connector.h:48: RenderWidgetHostViewAndroid* rwhva() const { return rwhva_.get(); } On 2017/04/07 18:47:10, boliu wrote: > Hmm, people will forget the null check in the future. > > How about separate HasFooView and GetFooView, where Get DCHECKs it's not null? > Or just GetFooViewMaybeNull? Actually, even better idea, this should not exist at all. It should be redundant given UpdateRenderProcessConnection
Description was changed from ========== Bypass ContentViewCore for SelectionPopupController update Defined the native part of SelectionPopupController to handle selection events without going through ContentViewCore. As the basic skeleton used to connect to RenderWidgetHostViewAndroid is similar to that used for ImeAdapterAndroid, factored the common part out to RenderProcessConnector. The change is illustrated here https://goo.gl/dIygxh Renamed |selection_controller_| to |touch_selection_controller_| to reduce the possible confusion against |selection_popup_controller_|. ========== to ========== Factor out RenderWidgetHostConnector Factored out RenderWidgetHostConnector from ImeAdapter with an intent to share the functionally with other classes. Upcoming SelectionPopup- Controller will also inherit it to manage its connection to Render- WidgetHostView. BUG=662908 ==========
jinsukkim@chromium.org changed reviewers: - tedchoc@chromium.org
Left only the RenderWidgetHostConnector. Haven't finished writing the test. Will add one soon. The tests in mind are: - RWHVA switching in TestRenderWidgetHostConnector - Ensure RwHVA Cleanup at WebContentsDestroyed https://codereview.chromium.org/2792063003/diff/120001/content/browser/androi... File content/browser/android/ime_adapter_android.cc (right): https://codereview.chromium.org/2792063003/diff/120001/content/browser/androi... content/browser/android/ime_adapter_android.cc:125: UpdateRenderProcessConnection(rwhva(), nullptr); On 2017/04/07 18:47:10, boliu wrote: > I think RenderProcessConnector can do this part as well. Connector already knows > all changes. Yes its destructor should not call virtual methods, but > WebContentsDestroyed can Good catch.. done. https://codereview.chromium.org/2792063003/diff/120001/content/browser/androi... content/browser/android/ime_adapter_android.cc:129: RenderProcessConnector::RenderViewReady(); On 2017/04/07 18:47:10, boliu wrote: > shouldn't this just be the first call of UpdateRenderProcessConnection? In the new patch it is not necessary to override |RenderViewReady|. Removed the method. Now Java layer is called on the first invocation of UpdateRenderProcessConnection with a valid RWHVA. https://codereview.chromium.org/2792063003/diff/120001/content/browser/androi... File content/browser/android/ime_adapter_android.h (right): https://codereview.chromium.org/2792063003/diff/120001/content/browser/androi... content/browser/android/ime_adapter_android.h:88: // RendetWidgetHostViewBinder implementation. On 2017/04/07 18:47:10, boliu wrote: > wrong name? Fixed. You can see the trace of me testing a few different names... https://codereview.chromium.org/2792063003/diff/120001/content/browser/androi... File content/browser/android/render_process_connector.h (right): https://codereview.chromium.org/2792063003/diff/120001/content/browser/androi... content/browser/android/render_process_connector.h:24: class RenderProcessConnector : public WebContentsObserver { On 2017/04/07 18:47:10, boliu wrote: > this has nothing to do with RenderProcess though, since RWHVA lifetime is > totally different from the actual renderer process > > RenderWidgetHostConnector? > > Also I think composition over inheritance applies here for WebContentsObserver, > don't want to make all methods of Observer available to subclasses - Renamed as suggested. - Defined an inner class (Observer) to compose (as opposed to) inherit WebContentsObserver. https://codereview.chromium.org/2792063003/diff/120001/content/browser/androi... content/browser/android/render_process_connector.h:26: RenderProcessConnector(WebContents* web_contents); On 2017/04/07 18:47:10, boliu wrote: > explicit Done. https://codereview.chromium.org/2792063003/diff/120001/content/browser/androi... content/browser/android/render_process_connector.h:48: RenderWidgetHostViewAndroid* rwhva() const { return rwhva_.get(); } On 2017/04/07 18:47:10, boliu wrote: > Hmm, people will forget the null check in the future. > > How about separate HasFooView and GetFooView, where Get DCHECKs it's not null? > Or just GetFooViewMaybeNull? Acknowledged. https://codereview.chromium.org/2792063003/diff/120001/content/browser/androi... content/browser/android/render_process_connector.h:48: RenderWidgetHostViewAndroid* rwhva() const { return rwhva_.get(); } On 2017/04/07 18:48:40, boliu wrote: > On 2017/04/07 18:47:10, boliu wrote: > > Hmm, people will forget the null check in the future. > > > > How about separate HasFooView and GetFooView, where Get DCHECKs it's not null? > > Or just GetFooViewMaybeNull? > > Actually, even better idea, this should not exist at all. It should be redundant > given UpdateRenderProcessConnection Sorry but what should not exist? null checking? If so, I think null checking is still necessary for the calls coming from ThreadedInputConnection while rwhva_ is set to null. https://codereview.chromium.org/2792063003/diff/120001/content/browser/androi... content/browser/android/render_process_connector.h:54: base::WeakPtr<RenderWidgetHostViewAndroid> rwhva_; On 2017/04/07 18:47:10, boliu wrote: > DISALLOW_ Done.
I think biggest thing remaining is we don't want to to track rwhva lifetime explicitly, and not use weakptr anymore https://codereview.chromium.org/2792063003/diff/140001/content/browser/androi... File content/browser/android/ime_adapter_android.h (right): https://codereview.chromium.org/2792063003/diff/140001/content/browser/androi... content/browser/android/ime_adapter_android.h:116: bool connected_; java side already has this state, no need to repeat it here https://codereview.chromium.org/2792063003/diff/140001/content/browser/androi... File content/browser/android/render_widget_host_connector.h (right): https://codereview.chromium.org/2792063003/diff/140001/content/browser/androi... content/browser/android/render_widget_host_connector.h:36: RenderWidgetHostViewAndroid* rwhva() const { I mean this getter is not really needed explicitly, or if it is here, it should match the last "new_rwhva" in UpdateRenderProcessConnection . The UpdateRenderProcessConnection API already allows observing rwhva destruction as well, so we should do that. Probably means RWHVA should explicitly tell this object it's being destroyed though I guess, instead of implicitly through weak pointer.
https://codereview.chromium.org/2792063003/diff/140001/content/browser/androi... File content/browser/android/ime_adapter_android.h (right): https://codereview.chromium.org/2792063003/diff/140001/content/browser/androi... content/browser/android/ime_adapter_android.h:116: bool connected_; On 2017/04/10 18:08:43, boliu wrote: > java side already has this state, no need to repeat it here Done. Replacing it with (!old_rwhva && new_rwhva) to call Java layer will work too. https://codereview.chromium.org/2792063003/diff/140001/content/browser/androi... File content/browser/android/render_widget_host_connector.h (right): https://codereview.chromium.org/2792063003/diff/140001/content/browser/androi... content/browser/android/render_widget_host_connector.h:36: RenderWidgetHostViewAndroid* rwhva() const { On 2017/04/10 18:08:43, boliu wrote: > I mean this getter is not really needed explicitly, or if it is here, it should > match the last "new_rwhva" in UpdateRenderProcessConnection . The > UpdateRenderProcessConnection API already allows observing rwhva destruction as Removed rwhva() but let ImeAdapterAndroid keep the reference to RWHVA. > well, so we should do that. Probably means RWHVA should explicitly tell this > object it's being destroyed though I guess, instead of implicitly through weak > pointer. I think we already tried the idea but decided to turn to weak ptr as discussed here https://codereview.chromium.org/2752113005/diff2/380001:400001/content/browse... are you okay with going back or mean something different?
https://codereview.chromium.org/2792063003/diff/140001/content/browser/androi... File content/browser/android/render_widget_host_connector.h (right): https://codereview.chromium.org/2792063003/diff/140001/content/browser/androi... content/browser/android/render_widget_host_connector.h:36: RenderWidgetHostViewAndroid* rwhva() const { On 2017/04/11 01:41:58, Jinsuk Kim wrote: > On 2017/04/10 18:08:43, boliu wrote: > > I mean this getter is not really needed explicitly, or if it is here, it > should > > match the last "new_rwhva" in UpdateRenderProcessConnection . The > > UpdateRenderProcessConnection API already allows observing rwhva destruction > as > > Removed rwhva() but let ImeAdapterAndroid keep the reference to RWHVA. > > > well, so we should do that. Probably means RWHVA should explicitly tell this > > object it's being destroyed though I guess, instead of implicitly through weak > > pointer. > > > I think we already tried the idea but decided to turn to weak ptr as discussed > here > https://codereview.chromium.org/2752113005/diff2/380001:400001/content/browse... > are you okay with going back or mean something different? Yeah I prefer explicit destroy now. That was a one off where adding a few checks is ok. This is factoring a class, so should keep the interface simple and safe if possible
I'll add a test soon. Not sure how to set up render widget host/view in a unittest... I guess it should be a browser test? https://codereview.chromium.org/2792063003/diff/140001/content/browser/androi... File content/browser/android/render_widget_host_connector.h (right): https://codereview.chromium.org/2792063003/diff/140001/content/browser/androi... content/browser/android/render_widget_host_connector.h:36: RenderWidgetHostViewAndroid* rwhva() const { On 2017/04/11 01:47:04, boliu wrote: > On 2017/04/11 01:41:58, Jinsuk Kim wrote: > > On 2017/04/10 18:08:43, boliu wrote: > > > I mean this getter is not really needed explicitly, or if it is here, it > > should > > > match the last "new_rwhva" in UpdateRenderProcessConnection . The > > > UpdateRenderProcessConnection API already allows observing rwhva destruction > > as > > > > Removed rwhva() but let ImeAdapterAndroid keep the reference to RWHVA. > > > > > well, so we should do that. Probably means RWHVA should explicitly tell this > > > object it's being destroyed though I guess, instead of implicitly through > weak > > > pointer. > > > > > > I think we already tried the idea but decided to turn to weak ptr as discussed > > here > > > https://codereview.chromium.org/2752113005/diff2/380001:400001/content/browse... > > are you okay with going back or mean something different? > > Yeah I prefer explicit destroy now. > > That was a one off where adding a few checks is ok. This is factoring a class, > so should keep the interface simple and safe if possible Done. Could you let me know if there's a reason weak ptr is not preferred? Was it introduced to a certain limited use only?
https://codereview.chromium.org/2792063003/diff/140001/content/browser/androi... File content/browser/android/render_widget_host_connector.h (right): https://codereview.chromium.org/2792063003/diff/140001/content/browser/androi... content/browser/android/render_widget_host_connector.h:36: RenderWidgetHostViewAndroid* rwhva() const { On 2017/04/11 02:28:09, Jinsuk Kim wrote: > On 2017/04/11 01:47:04, boliu wrote: > > On 2017/04/11 01:41:58, Jinsuk Kim wrote: > > > On 2017/04/10 18:08:43, boliu wrote: > > > > I mean this getter is not really needed explicitly, or if it is here, it > > > should > > > > match the last "new_rwhva" in UpdateRenderProcessConnection . The > > > > UpdateRenderProcessConnection API already allows observing rwhva > destruction > > > as > > > > > > Removed rwhva() but let ImeAdapterAndroid keep the reference to RWHVA. > > > > > > > well, so we should do that. Probably means RWHVA should explicitly tell > this > > > > object it's being destroyed though I guess, instead of implicitly through > > weak > > > > pointer. > > > > > > > > > I think we already tried the idea but decided to turn to weak ptr as > discussed > > > here > > > > > > https://codereview.chromium.org/2752113005/diff2/380001:400001/content/browse... > > > are you okay with going back or mean something different? > > > > Yeah I prefer explicit destroy now. > > > > That was a one off where adding a few checks is ok. This is factoring a class, > > so should keep the interface simple and safe if possible > > Done. Could you let me know if there's a reason weak ptr is not preferred? Was > it introduced to a certain limited use only? It's purely to make this interface simpler. If you use weakptrs, then subclass has to null check rwhva every single time. If you use explicit destroy, then could just UpdateRenderProcessConnection to null and subclass would just know.
Add tests. PTAL
https://codereview.chromium.org/2792063003/diff/220001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2792063003/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:500: ime_adapter_android_->ResetRenderWidgetHostView(this); no I mean this should be render_widget_host_connector
Patchset #9 (id:240001) has been deleted
Patchset #9 (id:260001) has been deleted
Made several changes to the previous patch about following. Please see if they make sense or agrees with what you know: - Nulling out ime_adapter_ pointer in RWVHA being swapped out: This can be done just like how content_view_core_ pointer in RWHVA is handled. |CVCImpl::RenderViewHostChanged| calls RWVHA::SetContentViewCore(NULL) only if it can get RWHVA from |old_host|. I think this works because, if it can't get RWHVA from the old_host it means the RWHVA already got deleted - doesn't have to care about nulling out the pointer anyway. - Nulling out rwhva pointer in ImeAdapterAndroid at RWHVA destruction: Tested the order of operations - ImeAdapterAndroid (i.e. its associated RenderWidgetHostConnector)'s |RenderViewHostChanged| always gets called and updated with the new RWHVA before the old RWHVA gets destroyed. So the RWHVA, upon destruction, doesn't have to null out its reference in the connector, because the connector will already be connected to a new RWHVA. - Renamed |UpdateRenderProcessConnection| to |UpdateRenderWidgetHostView| - Removed impl of WebContentsObserver::RenderViewReady: This was only to call Java ImeAdapter to notify it of the RWHVA connection. But it can also be done on the first |UpdateRenderWidgetHostView|| that sets non-null RWHVA. - Removed impl of WebContentsObserver:: Attach/DetachInterstitial: I remember changwan@ mentioned he hadn't have to care about it. I think it worked because interstitials don't need IME (or popups for SelectionPopupController to come), and get destroyed immediately after use. The ImeAdapter can keep the reference to RWHVA to be restored while interstitial is showing. - Applied composition over inheritance for ImeAdapterAndroid & RenderWidgetHostConnector. - Removed the tests for now, until the refactoring gets more concrete.
On 2017/04/12 09:43:15, Jinsuk Kim wrote: > - Nulling out rwhva pointer in ImeAdapterAndroid at RWHVA destruction: Tested > the order of operations - ImeAdapterAndroid (i.e. its associated > RenderWidgetHostConnector)'s |RenderViewHostChanged| always gets called and > updated with the new RWHVA before the old RWHVA gets destroyed. So the RWHVA, > upon destruction, doesn't have to null out its reference in the connector, > because the connector will already be connected to a new RWHVA. > Let me add that this was purely observational. I'll look into code to verify if this is always true.
On 2017/04/12 12:26:10, Jinsuk Kim wrote: > On 2017/04/12 09:43:15, Jinsuk Kim wrote: > > - Nulling out rwhva pointer in ImeAdapterAndroid at RWHVA destruction: Tested > > the order of operations - ImeAdapterAndroid (i.e. its associated > > RenderWidgetHostConnector)'s |RenderViewHostChanged| always gets called and > > updated with the new RWHVA before the old RWHVA gets destroyed. So the RWHVA, > > upon destruction, doesn't have to null out its reference in the connector, > > because the connector will already be connected to a new RWHVA. > > > Let me add that this was purely observational. I'll look into code to > verify if this is always true. What if you crash the active renderer with chrome://crash. That looks like view gets destroyed first before host. And no swapping happens in this case afaik
On 2017/04/12 09:43:15, Jinsuk Kim wrote: > Made several changes to the previous patch about following. Please see if they > make sense or agrees with what you know: > > - Nulling out ime_adapter_ pointer in RWVHA being swapped out: This can be done > just like how content_view_core_ pointer in RWHVA is handled. > |CVCImpl::RenderViewHostChanged| calls RWVHA::SetContentViewCore(NULL) only if > it can get RWHVA from |old_host|. I think this works because, if it can't get > RWHVA from the old_host it means the RWHVA already got deleted - doesn't have > to care about nulling out the pointer anyway. > > - Nulling out rwhva pointer in ImeAdapterAndroid at RWHVA destruction: Tested > the order of operations - ImeAdapterAndroid (i.e. its associated > RenderWidgetHostConnector)'s |RenderViewHostChanged| always gets called and > updated with the new RWHVA before the old RWHVA gets destroyed. So the RWHVA, > upon destruction, doesn't have to null out its reference in the connector, > because the connector will already be connected to a new RWHVA. > > - Renamed |UpdateRenderProcessConnection| to |UpdateRenderWidgetHostView| > > - Removed impl of WebContentsObserver::RenderViewReady: This was only to call > Java ImeAdapter to notify it of the RWHVA connection. But it can also be done on > the first |UpdateRenderWidgetHostView|| that sets non-null RWHVA. > > - Removed impl of WebContentsObserver:: Attach/DetachInterstitial: I remember > changwan@ mentioned he hadn't have to care about it. I think it worked because > interstitials don't need IME (or popups for SelectionPopupController to come), > and get destroyed immediately after use. The ImeAdapter can keep the reference > to RWHVA to be restored while interstitial is showing. Does that also apply to selection controller? Regardless, that just seems to be broken though. We *should* care about interstitials. > > - Applied composition over inheritance for ImeAdapterAndroid & > RenderWidgetHostConnector. > > - Removed the tests for now, until the refactoring gets more concrete.
https://codereview.chromium.org/2792063003/diff/300001/content/browser/androi... File content/browser/android/render_widget_host_connector.h (right): https://codereview.chromium.org/2792063003/diff/300001/content/browser/androi... content/browser/android/render_widget_host_connector.h:35: RenderWidgetHostViewAndroid* new_rwhva) = 0; as discussed in chat, add a client interface so that Connector is composed in ImeAdapter?
On 2017/04/12 16:07:24, boliu wrote: > On 2017/04/12 12:26:10, Jinsuk Kim wrote: > > On 2017/04/12 09:43:15, Jinsuk Kim wrote: > > > - Nulling out rwhva pointer in ImeAdapterAndroid at RWHVA destruction: > Tested > > > the order of operations - ImeAdapterAndroid (i.e. its associated > > > RenderWidgetHostConnector)'s |RenderViewHostChanged| always gets called and > > > updated with the new RWHVA before the old RWHVA gets destroyed. So the > RWHVA, > > > upon destruction, doesn't have to null out its reference in the connector, > > > because the connector will already be connected to a new RWHVA. > > > > > Let me add that this was purely observational. I'll look into code to > > verify if this is always true. > > What if you crash the active renderer with chrome://crash. That looks like view > gets destroyed first before host. And no swapping happens in this case afaik At this point, could you reconsider using WeakPtr instead of introducing a new explicit destruction observer? Adding a new interface seems no simpler than utilizing WeakPtr which is already present and nicely addresses it. And even though explicitly setting rwhva is straightforward, shouldn't it also require null checking? rwhva can still be set to null by the observer, and ime adapter needs to deal with it. > Regardless, that just seems to be broken though. We *should* care about > interstitials. No problem. Will put them back. > as discussed in chat, add a client interface so that Connector is composed in > ImeAdapter? Sorry but would you elaborate? Now the connector (with its pure method implemented) is composed in ImeAdapter. How differently would you like to see it to work?
I'm thinking of public interface like this:
class ActiveRenderWidgetHostViewObserver {
public:
void RWHVAChanged(RWHVA* old, RWHVA* new) = 0;
void Destroy() = 0; // maybe?
}
class Connector : WebContentsObserver {
public:
Connector(WebContents* wc, ActiveRenderWidgetHostViewObserver* observer);
}
That's the simplest interface I can think of. WeakPtr semantics doesn't really
work with that interface.
Looking at that vs composing WebContentsObserver, I guess neither one is
actually better, so if you want to go back to composing WebContentsObserver,
that's fine too.
On 2017/04/12 23:17:46, boliu wrote:
> I'm thinking of public interface like this:
>
> class ActiveRenderWidgetHostViewObserver {
> public:
> void RWHVAChanged(RWHVA* old, RWHVA* new) = 0;
> void Destroy() = 0; // maybe?
> }
>
> class Connector : WebContentsObserver {
> public:
> Connector(WebContents* wc, ActiveRenderWidgetHostViewObserver* observer);
> }
>
>
> That's the simplest interface I can think of. WeakPtr semantics doesn't really
> work with that interface.
>
> Looking at that vs composing WebContentsObserver, I guess neither one is
> actually better, so if you want to go back to composing WebContentsObserver,
> that's fine too.
The former approach (patch #8) has an advantage that ImeAdapter needs less
things to do (just overriding UpdateRenderWidgetHostView vs. adding a new class
Connector inheriting RenderWidgetHostConnector and overriding the method). Went
back to that.
From the way I see it, the new destruction observer and WebContentsObserver look
better when treated in parallel as base classes for |Observer| in
RenderWidgetHostConnector. In that way the destruction observer interface can be
simpler, by letting |RWHVAChanged| be handled via WebContentsObserver which
already provides with one. PTAL.
https://codereview.chromium.org/2792063003/diff/320001/content/browser/androi... File content/browser/android/render_widget_host_connector.cc (right): https://codereview.chromium.org/2792063003/diff/320001/content/browser/androi... content/browser/android/render_widget_host_connector.cc:31: UpdateRenderWidgetHostView(old_view, new_view); hmm, I'm not sure how interstitials are implemented, but can the background page navigate while interstitial is showing? I think the correct state machine is current_non_interstitial_rwhva* interstitial_rwhva* https://codereview.chromium.org/2792063003/diff/320001/content/browser/androi... content/browser/android/render_widget_host_connector.cc:52: connector_->UpdateRenderProcessConnection(nullptr, nullptr); why is old null? https://codereview.chromium.org/2792063003/diff/320001/content/browser/androi... content/browser/android/render_widget_host_connector.cc:63: new_rwhva->add_destruction_observer(this); you need to remove this from the old_rwhva https://codereview.chromium.org/2792063003/diff/320001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2792063003/diff/320001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:82: struct DestructionObserver { write a proper class if you need polymorphism, no random structs or you can juse use a base::Closure
https://codereview.chromium.org/2792063003/diff/320001/content/browser/androi... File content/browser/android/render_widget_host_connector.cc (right): https://codereview.chromium.org/2792063003/diff/320001/content/browser/androi... content/browser/android/render_widget_host_connector.cc:31: UpdateRenderWidgetHostView(old_view, new_view); On 2017/04/13 03:44:13, boliu wrote: > hmm, I'm not sure how interstitials are implemented, but can the background page > navigate while interstitial is showing? > > I think the correct state machine is > current_non_interstitial_rwhva* > interstitial_rwhva* I don't know right now, but that doesn't sound right. Anyway the one in the foreground needs to get the connection. https://codereview.chromium.org/2792063003/diff/320001/content/browser/androi... content/browser/android/render_widget_host_connector.cc:52: connector_->UpdateRenderProcessConnection(nullptr, nullptr); On 2017/04/13 03:44:13, boliu wrote: > why is old null? This is to set the current rwhva cached in the connection impl to null. It doesn't have to do anything for old/new rwhva (in this case it would have called old/new rwhva->set_ime_adapter()). Setting both to null prevents that. https://codereview.chromium.org/2792063003/diff/320001/content/browser/androi... content/browser/android/render_widget_host_connector.cc:63: new_rwhva->add_destruction_observer(this); On 2017/04/13 03:44:13, boliu wrote: > you need to remove this from the old_rwhva Done. https://codereview.chromium.org/2792063003/diff/320001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2792063003/diff/320001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:82: struct DestructionObserver { On 2017/04/13 03:44:13, boliu wrote: > write a proper class if you need polymorphism, no random structs > > or you can juse use a base::Closure Done. Turned to class.
https://codereview.chromium.org/2792063003/diff/320001/content/browser/androi... File content/browser/android/render_widget_host_connector.cc (right): https://codereview.chromium.org/2792063003/diff/320001/content/browser/androi... content/browser/android/render_widget_host_connector.cc:52: connector_->UpdateRenderProcessConnection(nullptr, nullptr); On 2017/04/13 04:20:20, Jinsuk Kim wrote: > On 2017/04/13 03:44:13, boliu wrote: > > why is old null? > > This is to set the current rwhva cached in the connection impl to null. It > doesn't have to do anything for old/new rwhva (in this case it would have called > old/new rwhva->set_ime_adapter()). Setting both to null prevents that. That's not part of the interface. The interface is transition from old to new, so old should always match the previous new. https://codereview.chromium.org/2792063003/diff/340001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2792063003/diff/340001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:500: ime_adapter_android_ = nullptr; this should be redundant, this should be an DCHECK null https://codereview.chromium.org/2792063003/diff/340001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:501: for (auto* observer : destruction_observers_) this should be called from Destroy, not here, otherwise could lead to be calling virtual methods from destructor https://codereview.chromium.org/2792063003/diff/340001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2792063003/diff/340001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:83: public: protected virtual destructor
https://codereview.chromium.org/2792063003/diff/320001/content/browser/androi... File content/browser/android/render_widget_host_connector.cc (right): https://codereview.chromium.org/2792063003/diff/320001/content/browser/androi... content/browser/android/render_widget_host_connector.cc:52: connector_->UpdateRenderProcessConnection(nullptr, nullptr); On 2017/04/13 04:20:20, Jinsuk Kim wrote: > On 2017/04/13 03:44:13, boliu wrote: > > why is old null? > > This is to set the current rwhva cached in the connection impl to null. It > doesn't have to do anything for old/new rwhva (in this case it would have called > old/new rwhva->set_ime_adapter()). Setting both to null prevents that. That's not part of the interface. The interface is transition from old to new, so old should always match the previous new. https://codereview.chromium.org/2792063003/diff/340001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2792063003/diff/340001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:500: ime_adapter_android_ = nullptr; this should be redundant, this should be an DCHECK null https://codereview.chromium.org/2792063003/diff/340001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:501: for (auto* observer : destruction_observers_) this should be called from Destroy, not here, otherwise could lead to be calling virtual methods from destructor https://codereview.chromium.org/2792063003/diff/340001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2792063003/diff/340001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:83: public: protected virtual destructor
https://codereview.chromium.org/2792063003/diff/320001/content/browser/androi... File content/browser/android/render_widget_host_connector.cc (right): https://codereview.chromium.org/2792063003/diff/320001/content/browser/androi... content/browser/android/render_widget_host_connector.cc:52: connector_->UpdateRenderProcessConnection(nullptr, nullptr); On 2017/04/13 04:29:41, boliu wrote: > On 2017/04/13 04:20:20, Jinsuk Kim wrote: > > On 2017/04/13 03:44:13, boliu wrote: > > > why is old null? > > > > This is to set the current rwhva cached in the connection impl to null. It > > doesn't have to do anything for old/new rwhva (in this case it would have > called > > old/new rwhva->set_ime_adapter()). Setting both to null prevents that. > > That's not part of the interface. The interface is transition from old to new, > so old should always match the previous new. Done. This will help ensure ime_adapter_ in rwhva is null at its destructor. Used rwhva_ in the comparison to make the meaning clearer. https://codereview.chromium.org/2792063003/diff/340001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2792063003/diff/340001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:500: ime_adapter_android_ = nullptr; On 2017/04/13 04:29:41, boliu wrote: > this should be redundant, this should be an DCHECK null Done. https://codereview.chromium.org/2792063003/diff/340001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:501: for (auto* observer : destruction_observers_) On 2017/04/13 04:29:41, boliu wrote: > this should be called from Destroy, not here, otherwise could lead to be calling > virtual methods from destructor Done. https://codereview.chromium.org/2792063003/diff/340001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2792063003/diff/340001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:83: public: On 2017/04/13 04:29:41, boliu wrote: > protected virtual destructor Done.
https://codereview.chromium.org/2792063003/diff/360001/content/browser/androi... File content/browser/android/render_widget_host_connector.cc (right): https://codereview.chromium.org/2792063003/diff/360001/content/browser/androi... content/browser/android/render_widget_host_connector.cc:27: : nullptr; Like I said earlier, UpdateRenderWidgetHostView is always a transition, where old_rwhva is always equal to the *previous* new_rwhva. I'm not sure that condition holds here with interstitials, ie if you can DCHECK old_view and rwhva_ always matches, or if this needs to be dealt with explicitly https://codereview.chromium.org/2792063003/diff/360001/content/browser/androi... File content/browser/android/render_widget_host_connector.h (right): https://codereview.chromium.org/2792063003/diff/360001/content/browser/androi... content/browser/android/render_widget_host_connector.h:18: // away so it won't access the object any more. definitely should mention this object "owns itself", and is deleted when WebContents is deleted https://codereview.chromium.org/2792063003/diff/360001/content/browser/androi... content/browser/android/render_widget_host_connector.h:71: std::unique_ptr<Observer> render_widget_observer_; oh this is unique_ptr now, then you can move the entire Observer declaration into .cc file, and just leave a forward declaration here
https://codereview.chromium.org/2792063003/diff/360001/content/browser/androi... File content/browser/android/render_widget_host_connector.cc (right): https://codereview.chromium.org/2792063003/diff/360001/content/browser/androi... content/browser/android/render_widget_host_connector.cc:27: : nullptr; On 2017/04/13 21:26:59, boliu wrote: > Like I said earlier, UpdateRenderWidgetHostView is always a transition, where > old_rwhva is always equal to the *previous* new_rwhva. I'm not sure that > condition holds here with interstitials, ie if you can DCHECK old_view and > rwhva_ always matches, or if this needs to be dealt with explicitly It holds true for interstitials too. Put DCHECK. https://codereview.chromium.org/2792063003/diff/360001/content/browser/androi... File content/browser/android/render_widget_host_connector.h (right): https://codereview.chromium.org/2792063003/diff/360001/content/browser/androi... content/browser/android/render_widget_host_connector.h:18: // away so it won't access the object any more. On 2017/04/13 21:26:59, boliu wrote: > definitely should mention this object "owns itself", and is deleted when > WebContents is deleted Done. https://codereview.chromium.org/2792063003/diff/360001/content/browser/androi... content/browser/android/render_widget_host_connector.h:71: std::unique_ptr<Observer> render_widget_observer_; On 2017/04/13 21:26:59, boliu wrote: > oh this is unique_ptr now, then you can move the entire Observer declaration > into .cc file, and just leave a forward declaration here Connector::rwhva_for_testing() uses Observer::rwhva() at line 35 for which the header declaration is necessary here. It's not used now but will be when I put back the tests. Or do you want the method to be defined in .cc too?
https://codereview.chromium.org/2792063003/diff/360001/content/browser/androi... File content/browser/android/render_widget_host_connector.h (right): https://codereview.chromium.org/2792063003/diff/360001/content/browser/androi... content/browser/android/render_widget_host_connector.h:71: std::unique_ptr<Observer> render_widget_observer_; On 2017/04/13 23:00:43, Jinsuk Kim wrote: > On 2017/04/13 21:26:59, boliu wrote: > > oh this is unique_ptr now, then you can move the entire Observer declaration > > into .cc file, and just leave a forward declaration here > > Connector::rwhva_for_testing() uses Observer::rwhva() at line 35 for which the > header declaration is necessary here. It's not used now but will be when I put > back the tests. Or do you want the method to be defined in .cc too? Yes. rwhva_for_testing shouldn't be inlined anyway, so should be renamed GetRenderWidgetHostViewAndroidForTesting
https://codereview.chromium.org/2792063003/diff/380001/content/browser/androi... File content/browser/android/render_widget_host_connector.cc (right): https://codereview.chromium.org/2792063003/diff/380001/content/browser/androi... content/browser/android/render_widget_host_connector.cc:31: DCHECK(old_view == rwhva_); nit: DCHECK_EQ https://codereview.chromium.org/2792063003/diff/380001/content/browser/androi... content/browser/android/render_widget_host_connector.cc:44: UpdateRenderWidgetHostView(GetRenderWidgetHostViewAndroid(), nullptr); DCHECK this matches rwhva_. Then I think you can safely remove old_rwhva from Observer::UpdateRenderWidgetHostView, and always rwhva_ as old
https://codereview.chromium.org/2792063003/diff/360001/content/browser/androi... File content/browser/android/render_widget_host_connector.h (right): https://codereview.chromium.org/2792063003/diff/360001/content/browser/androi... content/browser/android/render_widget_host_connector.h:71: std::unique_ptr<Observer> render_widget_observer_; On 2017/04/13 23:03:32, boliu wrote: > On 2017/04/13 23:00:43, Jinsuk Kim wrote: > > On 2017/04/13 21:26:59, boliu wrote: > > > oh this is unique_ptr now, then you can move the entire Observer declaration > > > into .cc file, and just leave a forward declaration here > > > > Connector::rwhva_for_testing() uses Observer::rwhva() at line 35 for which the > > header declaration is necessary here. It's not used now but will be when I put > > back the tests. Or do you want the method to be defined in .cc too? > > Yes. rwhva_for_testing shouldn't be inlined anyway, so should be renamed > GetRenderWidgetHostViewAndroidForTesting Done. Renamed the class to be more descriptive as it's not an inner class any more. https://codereview.chromium.org/2792063003/diff/380001/content/browser/androi... File content/browser/android/render_widget_host_connector.cc (right): https://codereview.chromium.org/2792063003/diff/380001/content/browser/androi... content/browser/android/render_widget_host_connector.cc:31: DCHECK(old_view == rwhva_); On 2017/04/13 23:06:50, boliu wrote: > nit: DCHECK_EQ Done. https://codereview.chromium.org/2792063003/diff/380001/content/browser/androi... content/browser/android/render_widget_host_connector.cc:44: UpdateRenderWidgetHostView(GetRenderWidgetHostViewAndroid(), nullptr); On 2017/04/13 23:06:50, boliu wrote: > DCHECK this matches rwhva_. Then I think you can safely remove old_rwhva from > Observer::UpdateRenderWidgetHostView, and always rwhva_ as old Done.
https://codereview.chromium.org/2792063003/diff/360001/content/browser/androi... File content/browser/android/render_widget_host_connector.h (right): https://codereview.chromium.org/2792063003/diff/360001/content/browser/androi... content/browser/android/render_widget_host_connector.h:71: std::unique_ptr<Observer> render_widget_observer_; On 2017/04/13 23:31:10, Jinsuk Kim wrote: > On 2017/04/13 23:03:32, boliu wrote: > > On 2017/04/13 23:00:43, Jinsuk Kim wrote: > > > On 2017/04/13 21:26:59, boliu wrote: > > > > oh this is unique_ptr now, then you can move the entire Observer > declaration > > > > into .cc file, and just leave a forward declaration here > > > > > > Connector::rwhva_for_testing() uses Observer::rwhva() at line 35 for which > the > > > header declaration is necessary here. It's not used now but will be when I > put > > > back the tests. Or do you want the method to be defined in .cc too? > > > > Yes. rwhva_for_testing shouldn't be inlined anyway, so should be renamed > > GetRenderWidgetHostViewAndroidForTesting > > Done. Renamed the class to be more descriptive as it's not an inner class any > more. Hmm? It can stay an private inner class. You can forward declare inner classes too private: class Observer; unique_ptr<Observer> observer_;
https://codereview.chromium.org/2792063003/diff/360001/content/browser/androi... File content/browser/android/render_widget_host_connector.h (right): https://codereview.chromium.org/2792063003/diff/360001/content/browser/androi... content/browser/android/render_widget_host_connector.h:71: std::unique_ptr<Observer> render_widget_observer_; On 2017/04/13 23:33:38, boliu wrote: > On 2017/04/13 23:31:10, Jinsuk Kim wrote: > > On 2017/04/13 23:03:32, boliu wrote: > > > On 2017/04/13 23:00:43, Jinsuk Kim wrote: > > > > On 2017/04/13 21:26:59, boliu wrote: > > > > > oh this is unique_ptr now, then you can move the entire Observer > > declaration > > > > > into .cc file, and just leave a forward declaration here > > > > > > > > Connector::rwhva_for_testing() uses Observer::rwhva() at line 35 for which > > the > > > > header declaration is necessary here. It's not used now but will be when I > > put > > > > back the tests. Or do you want the method to be defined in .cc too? > > > > > > Yes. rwhva_for_testing shouldn't be inlined anyway, so should be renamed > > > GetRenderWidgetHostViewAndroidForTesting > > > > Done. Renamed the class to be more descriptive as it's not an inner class any > > more. > > Hmm? It can stay an private inner class. You can forward declare inner classes > too > private: > class Observer; > unique_ptr<Observer> observer_; Haven't done that before... Thanks. Done.
lgtm /w comments https://codereview.chromium.org/2792063003/diff/420001/content/browser/androi... File content/browser/android/render_widget_host_connector.cc (right): https://codereview.chromium.org/2792063003/diff/420001/content/browser/androi... content/browser/android/render_widget_host_connector.cc:51: RenderWidgetHostConnector::Observer::~Observer() {} DCHECK(rwhva_) is null here https://codereview.chromium.org/2792063003/diff/420001/content/browser/androi... content/browser/android/render_widget_host_connector.cc:120: new RenderWidgetHostConnector::Observer(web_contents, this)) {} is namespace here required? https://codereview.chromium.org/2792063003/diff/420001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2792063003/diff/420001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1036: maybe clear the list to be safe https://codereview.chromium.org/2792063003/diff/420001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2792063003/diff/420001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:420: std::list<DestructionObserver*> destruction_observers_; nit: vector is generally more memory efficient and probably also faster than linked list
Patchset #17 (id:440001) has been deleted
Also put back the tests. https://codereview.chromium.org/2792063003/diff/420001/content/browser/androi... File content/browser/android/render_widget_host_connector.cc (right): https://codereview.chromium.org/2792063003/diff/420001/content/browser/androi... content/browser/android/render_widget_host_connector.cc:51: RenderWidgetHostConnector::Observer::~Observer() {} On 2017/04/14 01:10:07, boliu wrote: > DCHECK(rwhva_) is null here Done. https://codereview.chromium.org/2792063003/diff/420001/content/browser/androi... content/browser/android/render_widget_host_connector.cc:120: new RenderWidgetHostConnector::Observer(web_contents, this)) {} On 2017/04/14 01:10:07, boliu wrote: > is namespace here required? No. removed. https://codereview.chromium.org/2792063003/diff/420001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2792063003/diff/420001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1036: On 2017/04/14 01:10:07, boliu wrote: > maybe clear the list to be safe Done. https://codereview.chromium.org/2792063003/diff/420001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2792063003/diff/420001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:420: std::list<DestructionObserver*> destruction_observers_; On 2017/04/14 01:10:07, boliu wrote: > nit: vector is generally more memory efficient and probably also faster than > linked list Done.
Patchset #17 (id:460001) has been deleted
Patchset #18 (id:500001) 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...
Patchset #18 (id:520001) has been deleted
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 #19 (id:560001) has been deleted
Modified further to fix failing tests. PTAL. - Your were right about the possiblity of main page navigating while an interstitial is active - it can. Added another pointer |interstitial_rwhva| (and renamed |rwhva_| to |main_rwhva_|) and used both to keep track of the active one. |interstitial_rwhva| always points to an active one (and stays nuled out if interstitial is not showing), while |main_rwhva_| can also point to an inactive one if an interstitial is active. - For popups whose RWHVA gets created before WebContents, |main_rwhva_| in RenderWidgetHostConnector may not be correct in its initial state (nullptr). Reinstating the override method WebContentsObserver::RenderViewReady sets it right. - NULL can be passed to |old_view| when the connector still points to an old rwhva. Removed DCHECK in |RenderViewHostChanged| because of that. I thought the rwhva got already deleted in such case, but not sure about it now - I still had to clean up the pointer to ime_adapter in the |main_rwhva_| to pass DCHECK in its ::Destroy() method...
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...)
https://codereview.chromium.org/2792063003/diff/600001/content/browser/androi... File content/browser/android/render_widget_host_connector.cc (right): https://codereview.chromium.org/2792063003/diff/600001/content/browser/androi... content/browser/android/render_widget_host_connector.cc:128: old_rwhva->RemoveDestructionObserver(this); this doesn't work anymore. I think you need to observe destructor on both interstitial and regular rwhva?
https://codereview.chromium.org/2792063003/diff/600001/content/browser/androi... File content/browser/android/render_widget_host_connector.cc (right): https://codereview.chromium.org/2792063003/diff/600001/content/browser/androi... content/browser/android/render_widget_host_connector.cc:128: old_rwhva->RemoveDestructionObserver(this); On 2017/04/15 00:52:54, boliu wrote: > this doesn't work anymore. I think you need to observe destructor on both > interstitial and regular rwhva? At certain time there is (and should be) only one rwhva (active) that keeps the observer in its list. I made sure the |old_rwhva| is the one that was keeping this observer. So I think this will still work. Do you see a bug?
https://codereview.chromium.org/2792063003/diff/600001/content/browser/androi... File content/browser/android/render_widget_host_connector.cc (right): https://codereview.chromium.org/2792063003/diff/600001/content/browser/androi... content/browser/android/render_widget_host_connector.cc:128: old_rwhva->RemoveDestructionObserver(this); On 2017/04/15 01:19:07, Jinsuk Kim wrote: > On 2017/04/15 00:52:54, boliu wrote: > > this doesn't work anymore. I think you need to observe destructor on both > > interstitial and regular rwhva? > > At certain time there is (and should be) only one rwhva (active) that keeps the > observer in its list. I made sure the |old_rwhva| is the one that was keeping > this observer. So I think this will still work. Do you see a bug? what if the non-interstitial rwhva is destroyed while interstitial is showing?
https://codereview.chromium.org/2792063003/diff/600001/content/browser/androi... File content/browser/android/render_widget_host_connector.cc (right): https://codereview.chromium.org/2792063003/diff/600001/content/browser/androi... content/browser/android/render_widget_host_connector.cc:128: old_rwhva->RemoveDestructionObserver(this); On 2017/04/15 02:06:21, boliu wrote: > On 2017/04/15 01:19:07, Jinsuk Kim wrote: > > On 2017/04/15 00:52:54, boliu wrote: > > > this doesn't work anymore. I think you need to observe destructor on both > > > interstitial and regular rwhva? > > > > At certain time there is (and should be) only one rwhva (active) that keeps > the > > observer in its list. I made sure the |old_rwhva| is the one that was keeping > > this observer. So I think this will still work. Do you see a bug? > > what if the non-interstitial rwhva is destroyed while interstitial is showing? Main rwhva is not connected to ime, so its destruction doesn't affect much. It is handled in ::RenderWidgetHostViewDestroyed where |main_rwhva_| will be set to null, and interstitial keeps its connection to the ime. So I think it will still work?
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.
Patchset #21 (id:620001) has been deleted
> Main rwhva is not connected to ime, so its destruction doesn't affect much. It > is handled in ::RenderWidgetHostViewDestroyed where |main_rwhva_| will be set to > null, and interstitial keeps its connection to the ime. So I think it will still > work? Added another test to ensure main/interstitial rwhva is updated as expected. My logic above is wrong - it is at |DidDetachInterstitialPage| that |main_rwhva_| is reset. |GetRenderWidgetHostViewAndroid()| returns null by this time (since it was already destroyed), so |main_rwhva_| is set to null here.
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.
On 2017/04/16 23:16:43, Jinsuk Kim wrote: > https://codereview.chromium.org/2792063003/diff/600001/content/browser/androi... > File content/browser/android/render_widget_host_connector.cc (right): > > https://codereview.chromium.org/2792063003/diff/600001/content/browser/androi... > content/browser/android/render_widget_host_connector.cc:128: > old_rwhva->RemoveDestructionObserver(this); > On 2017/04/15 02:06:21, boliu wrote: > > On 2017/04/15 01:19:07, Jinsuk Kim wrote: > > > On 2017/04/15 00:52:54, boliu wrote: > > > > this doesn't work anymore. I think you need to observe destructor on both > > > > interstitial and regular rwhva? > > > > > > At certain time there is (and should be) only one rwhva (active) that keeps > > the > > > observer in its list. I made sure the |old_rwhva| is the one that was > keeping > > > this observer. So I think this will still work. Do you see a bug? > > > > what if the non-interstitial rwhva is destroyed while interstitial is showing? > > Main rwhva is not connected to ime, so its destruction doesn't affect much. It > is handled in ::RenderWidgetHostViewDestroyed where |main_rwhva_| will be set to > null, and interstitial keeps its connection to the ime. So I think it will still > work? Well, this has the same problem that you have to observe RWHVA destruction rather than relying solely on RenderWidgetHostViewDestroyed? * Interstitial is created, and ime moves to it * the renderer backing main rwhva crashes so rwhva is deleted, but rwhv is not * interstitial is dismissed, and now you are switching to a dangling pointer?
On 2017/04/17 16:24:15, boliu wrote: > On 2017/04/16 23:16:43, Jinsuk Kim wrote: > > > https://codereview.chromium.org/2792063003/diff/600001/content/browser/androi... > > File content/browser/android/render_widget_host_connector.cc (right): > > > > > https://codereview.chromium.org/2792063003/diff/600001/content/browser/androi... > > content/browser/android/render_widget_host_connector.cc:128: > > old_rwhva->RemoveDestructionObserver(this); > > On 2017/04/15 02:06:21, boliu wrote: > > > On 2017/04/15 01:19:07, Jinsuk Kim wrote: > > > > On 2017/04/15 00:52:54, boliu wrote: > > > > > this doesn't work anymore. I think you need to observe destructor on > both > > > > > interstitial and regular rwhva? > > > > > > > > At certain time there is (and should be) only one rwhva (active) that > keeps > > > the > > > > observer in its list. I made sure the |old_rwhva| is the one that was > > keeping > > > > this observer. So I think this will still work. Do you see a bug? > > > > > > what if the non-interstitial rwhva is destroyed while interstitial is > showing? > > > > Main rwhva is not connected to ime, so its destruction doesn't affect much. It > > is handled in ::RenderWidgetHostViewDestroyed where |main_rwhva_| will be set > to > > null, and interstitial keeps its connection to the ime. So I think it will > still > > work? > > Well, this has the same problem that you have to observe RWHVA destruction > rather than relying solely on RenderWidgetHostViewDestroyed? > * Interstitial is created, and ime moves to it > * the renderer backing main rwhva crashes so rwhva is deleted, but rwhv is not > * interstitial is dismissed, and now you are switching to a dangling pointer? I think the new test I added shows how this case is covered. In step 4 (interstitial is dismissed) DidDetachInterstitialPage() is called. It gets the main rwhva through GetRenderWidgetHostViewAndroid(). What's returned is null since rwhva was already destroyed - hence main rwhva is set to null.
why do you need to separate main vs interstitial then? Seems like you don't care about keeping main at all when interstitial is showing? it seems like if DidDetachInterstitialPage, the active page is no longer the interstitial, then we should just ignore that call?
On 2017/04/17 22:44:40, boliu wrote: > why do you need to separate main vs interstitial then? Seems like you don't care > about keeping main at all when interstitial is showing? > > it seems like if DidDetachInterstitialPage, the active page is no longer the > interstitial, then we should just ignore that call? Right we may not need two. Another way to track the states correctly would be having rwhva_ and is_interstitial_showing_. Still we need two variables... I can try switching to the latter if you want.
On 2017/04/17 23:00:08, Jinsuk Kim wrote: > On 2017/04/17 22:44:40, boliu wrote: > > why do you need to separate main vs interstitial then? Seems like you don't > care > > about keeping main at all when interstitial is showing? > > > > it seems like if DidDetachInterstitialPage, the active page is no longer the > > interstitial, then we should just ignore that call? > > Right we may not need two. Another way to track the states correctly would be > having rwhva_ and is_interstitial_showing_. Still we need two variables... I can > try switching to the latter if you want. In theory you don't even need the bool. But I think keeping that bool for DCHECK purposes is a good idea.
On 2017/04/17 23:02:55, boliu wrote: > On 2017/04/17 23:00:08, Jinsuk Kim wrote: > > On 2017/04/17 22:44:40, boliu wrote: > > > why do you need to separate main vs interstitial then? Seems like you don't > > care > > > about keeping main at all when interstitial is showing? > > > > > > it seems like if DidDetachInterstitialPage, the active page is no longer the > > > interstitial, then we should just ignore that call? > > > > Right we may not need two. Another way to track the states correctly would be > > having rwhva_ and is_interstitial_showing_. Still we need two variables... I > can > > try switching to the latter if you want. > > In theory you don't even need the bool. But I think keeping that bool for DCHECK > purposes is a good idea. Why is it not necessary? It is possible that render view host can switch while interstitial is showing. The flag tells us whether we should move connection to the new rwvha or not.
On 2017/04/17 23:11:53, Jinsuk Kim wrote: > On 2017/04/17 23:02:55, boliu wrote: > > On 2017/04/17 23:00:08, Jinsuk Kim wrote: > > > On 2017/04/17 22:44:40, boliu wrote: > > > > why do you need to separate main vs interstitial then? Seems like you > don't > > > care > > > > about keeping main at all when interstitial is showing? > > > > > > > > it seems like if DidDetachInterstitialPage, the active page is no longer > the > > > > interstitial, then we should just ignore that call? > > > > > > Right we may not need two. Another way to track the states correctly would > be > > > having rwhva_ and is_interstitial_showing_. Still we need two variables... I > > can > > > try switching to the latter if you want. > > > > In theory you don't even need the bool. But I think keeping that bool for > DCHECK > > purposes is a good idea. > > Why is it not necessary? It is possible that render view host can switch while > interstitial is showing. The flag tells us whether we should move connection to > the new rwvha or not. Isn't it redundant with web_contents_impl->ShowingInterstitialPage()?
On 2017/04/17 23:16:50, boliu wrote: > On 2017/04/17 23:11:53, Jinsuk Kim wrote: > > On 2017/04/17 23:02:55, boliu wrote: > > > On 2017/04/17 23:00:08, Jinsuk Kim wrote: > > > > On 2017/04/17 22:44:40, boliu wrote: > > > > > why do you need to separate main vs interstitial then? Seems like you > > don't > > > > care > > > > > about keeping main at all when interstitial is showing? > > > > > > > > > > it seems like if DidDetachInterstitialPage, the active page is no longer > > the > > > > > interstitial, then we should just ignore that call? > > > > > > > > Right we may not need two. Another way to track the states correctly would > > be > > > > having rwhva_ and is_interstitial_showing_. Still we need two variables... > I > > > can > > > > try switching to the latter if you want. > > > > > > In theory you don't even need the bool. But I think keeping that bool for > > DCHECK > > > purposes is a good idea. > > > > Why is it not necessary? It is possible that render view host can switch while > > interstitial is showing. The flag tells us whether we should move connection > to > > the new rwvha or not. > > Isn't it redundant with web_contents_impl->ShowingInterstitialPage()? Right. I'll try to get rid of the unnecessary var/flag. Thanks!
PTAL
lgtm % comment https://codereview.chromium.org/2792063003/diff/660001/content/browser/androi... File content/browser/android/render_widget_host_connector.cc (right): https://codereview.chromium.org/2792063003/diff/660001/content/browser/androi... content/browser/android/render_widget_host_connector.cc:96: connector_->UpdateRenderProcessConnection(active_rwhva_, nullptr); should this just be UpdateRenderWidgetHostView(nullptr);?
https://codereview.chromium.org/2792063003/diff/660001/content/browser/androi... File content/browser/android/render_widget_host_connector.cc (right): https://codereview.chromium.org/2792063003/diff/660001/content/browser/androi... content/browser/android/render_widget_host_connector.cc:96: connector_->UpdateRenderProcessConnection(active_rwhva_, nullptr); On 2017/04/18 03:53:58, boliu wrote: > should this just be UpdateRenderWidgetHostView(nullptr);? This came from RWHVA destruction observer. So no need to (should not) do something again to add observer to that RWHVA again.
https://codereview.chromium.org/2792063003/diff/660001/content/browser/androi... File content/browser/android/render_widget_host_connector.cc (right): https://codereview.chromium.org/2792063003/diff/660001/content/browser/androi... content/browser/android/render_widget_host_connector.cc:96: connector_->UpdateRenderProcessConnection(active_rwhva_, nullptr); On 2017/04/18 04:25:34, Jinsuk Kim wrote: > On 2017/04/18 03:53:58, boliu wrote: > > should this just be UpdateRenderWidgetHostView(nullptr);? > > This came from RWHVA destruction observer. So no need to (should not) do > something again to add observer to that RWHVA again. Well, won't be adding observer because it's calling with null. And remove should still be safe because we went over this, this is called in RWHVA::Destroy, not destructor. If it's destructor, then it should not pass up the pointer at all because it's not safe at that point.
https://codereview.chromium.org/2792063003/diff/660001/content/browser/androi... File content/browser/android/render_widget_host_connector.cc (right): https://codereview.chromium.org/2792063003/diff/660001/content/browser/androi... content/browser/android/render_widget_host_connector.cc:96: connector_->UpdateRenderProcessConnection(active_rwhva_, nullptr); On 2017/04/18 04:45:32, boliu wrote: > On 2017/04/18 04:25:34, Jinsuk Kim wrote: > > On 2017/04/18 03:53:58, boliu wrote: > > > should this just be UpdateRenderWidgetHostView(nullptr);? > > > > This came from RWHVA destruction observer. So no need to (should not) do > > something again to add observer to that RWHVA again. > > Well, won't be adding observer because it's calling with null. And remove should > still be safe because we went over this, this is called in RWHVA::Destroy, not > destructor. If it's destructor, then it should not pass up the pointer at all > because it's not safe at that point. Oh you mean it's not safe modifying the vector and iterating it at the same time? Then maybe it should be a base::ObserverList
On 2017/04/18 04:47:09, boliu wrote: > https://codereview.chromium.org/2792063003/diff/660001/content/browser/androi... > File content/browser/android/render_widget_host_connector.cc (right): > > https://codereview.chromium.org/2792063003/diff/660001/content/browser/androi... > content/browser/android/render_widget_host_connector.cc:96: > connector_->UpdateRenderProcessConnection(active_rwhva_, nullptr); > On 2017/04/18 04:45:32, boliu wrote: > > On 2017/04/18 04:25:34, Jinsuk Kim wrote: > > > On 2017/04/18 03:53:58, boliu wrote: > > > > should this just be UpdateRenderWidgetHostView(nullptr);? > > > > > > This came from RWHVA destruction observer. So no need to (should not) do > > > something again to add observer to that RWHVA again. > > > > Well, won't be adding observer because it's calling with null. And remove > should > > still be safe because we went over this, this is called in RWHVA::Destroy, not > > destructor. If it's destructor, then it should not pass up the pointer at all > > because it's not safe at that point. > > Oh you mean it's not safe modifying the vector and iterating it at the same > time? Then maybe it should be a base::ObserverList I should have said 'to _remove_ observer to that RWHV again'. How about adding a comment there to clarify why calling |UpdateRenderProcessConnection| not |UpdateRenderWidgetHostView|? The observers are cleared after looping through anyway. Let me know if you still want to switch to iterate-safe list.
On 2017/04/18 05:00:16, Jinsuk Kim wrote: > On 2017/04/18 04:47:09, boliu wrote: > > > https://codereview.chromium.org/2792063003/diff/660001/content/browser/androi... > > File content/browser/android/render_widget_host_connector.cc (right): > > > > > https://codereview.chromium.org/2792063003/diff/660001/content/browser/androi... > > content/browser/android/render_widget_host_connector.cc:96: > > connector_->UpdateRenderProcessConnection(active_rwhva_, nullptr); > > On 2017/04/18 04:45:32, boliu wrote: > > > On 2017/04/18 04:25:34, Jinsuk Kim wrote: > > > > On 2017/04/18 03:53:58, boliu wrote: > > > > > should this just be UpdateRenderWidgetHostView(nullptr);? > > > > > > > > This came from RWHVA destruction observer. So no need to (should not) do > > > > something again to add observer to that RWHVA again. > > > > > > Well, won't be adding observer because it's calling with null. And remove > > should > > > still be safe because we went over this, this is called in RWHVA::Destroy, > not > > > destructor. If it's destructor, then it should not pass up the pointer at > all > > > because it's not safe at that point. > > > > Oh you mean it's not safe modifying the vector and iterating it at the same > > time? Then maybe it should be a base::ObserverList > > I should have said 'to _remove_ observer to that RWHV again'. > How about adding a comment there to clarify why calling > |UpdateRenderProcessConnection| not |UpdateRenderWidgetHostView|? I still think it should call UpdateRenderWidgetHostView(nullptr). Having a single place where active_rwhva_ is modified has benefits. > The observers > are cleared after looping through anyway. Let me know if you still want to > switch to iterate-safe list. Yeah, switch to ObserverList. lgtm after
Thanks for the review. https://codereview.chromium.org/2792063003/diff/660001/content/browser/androi... File content/browser/android/render_widget_host_connector.cc (right): https://codereview.chromium.org/2792063003/diff/660001/content/browser/androi... content/browser/android/render_widget_host_connector.cc:96: connector_->UpdateRenderProcessConnection(active_rwhva_, nullptr); On 2017/04/18 04:47:09, boliu wrote: > On 2017/04/18 04:45:32, boliu wrote: > > On 2017/04/18 04:25:34, Jinsuk Kim wrote: > > > On 2017/04/18 03:53:58, boliu wrote: > > > > should this just be UpdateRenderWidgetHostView(nullptr);? > > > > > > This came from RWHVA destruction observer. So no need to (should not) do > > > something again to add observer to that RWHVA again. > > > > Well, won't be adding observer because it's calling with null. And remove > should > > still be safe because we went over this, this is called in RWHVA::Destroy, not > > destructor. If it's destructor, then it should not pass up the pointer at all > > because it's not safe at that point. > > Oh you mean it's not safe modifying the vector and iterating it at the same > time? Then maybe it should be a base::ObserverList 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 boliu@chromium.org Link to the patchset: https://codereview.chromium.org/2792063003/#ps680001 (title: "base::ObserverList")
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": 680001, "attempt_start_ts": 1492505286855200,
"parent_rev": "479c5684bd7b22ab6305c20ae7266506f39149f5", "commit_rev":
"7e377438d1c86ec06009e10d24a88ea84342e594"}
Message was sent while issue was closed.
Description was changed from ========== Factor out RenderWidgetHostConnector Factored out RenderWidgetHostConnector from ImeAdapter with an intent to share the functionally with other classes. Upcoming SelectionPopup- Controller will also inherit it to manage its connection to Render- WidgetHostView. BUG=662908 ========== to ========== Factor out RenderWidgetHostConnector Factored out RenderWidgetHostConnector from ImeAdapter with an intent to share the functionally with other classes. Upcoming SelectionPopup- Controller will also inherit it to manage its connection to Render- WidgetHostView. BUG=662908 Review-Url: https://codereview.chromium.org/2792063003 Cr-Commit-Position: refs/heads/master@{#465180} Committed: https://chromium.googlesource.com/chromium/src/+/7e377438d1c86ec06009e10d24a8... ==========
Message was sent while issue was closed.
Committed patchset #23 (id:680001) as https://chromium.googlesource.com/chromium/src/+/7e377438d1c86ec06009e10d24a8... |
