Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2045363002:
Routing IME Result Calls to the Correct RenderWidgetHost (Aura Only) (Closed)
|
Created:
4 years, 6 months ago by EhsanK Modified:
4 years, 6 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, shuchen+watch_chromium.org, jam, sievers+watch_chromium.org, jbauman+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, kalyank, piman+watch_chromium.org, danakj+watch_chromium.org, James Su, site-isolation-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRouting IME Result Calls to the Correct RenderWidgetHost (Aura Only)
Currently, the IME result calls on ui::TextInputClient (RenderWidgetHostViewAura) are only
routed to the RenderWidgetHostImpl corresponding to the tab's RenderWidgetHostView. Therefore,
the IME related IPC arrive at the main frame's RenderWidget only. In the OOPIF structure,
the IME result might be intended to a child frame's RenderWidget.
This patch will route all such IME calls to the RenderWidgetHost which currently has the
active text input state.
The patch also adds four unit tests to RenderWidgetHostViewAuraTests to verify that the
routing is done properly.
BUG=578168
Committed: https://crrev.com/d33fd7859cac797067a2b8a8e55f6dbac59bee19
Cr-Commit-Position: refs/heads/master@{#401465}
Patch Set 1 #
Total comments: 17
Patch Set 2 : Addressing kenrb@ comments #Patch Set 3 : Fixing a comment #
Total comments: 23
Patch Set 4 : Addressing creis@ and kenrb@ comments #Patch Set 5 : Fixing a compile error #Patch Set 6 : Rebase #
Total comments: 8
Patch Set 7 : Addressing creis@'s comments #Patch Set 8 : Formatting #
Messages
Total messages: 24 (8 generated)
ekaramad@chromium.org changed reviewers: + creis@chromium.org, kenrb@chromium.org
Ken, Charlie, This is the first followup patch to the TextInputState tracking which deals with IME. Can you please take a look. Thanks! https://codereview.chromium.org/2045363002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/2045363002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:1096: SetTextInputTypeForView(view_, ui::TEXT_INPUT_TYPE_TEXT); Changes such as this are needed in the new architecture because all methods are routed to the active widget. Before this was not the case as RWHVAura was calling |host_| regardless of whether or not it has a |TextInputState.type| of ui::TEXT_INPUT_TYPE_TEXT (it was only a unit test so it makes sense). Now I have to manually set this to make sure the right RWHV is set as |active_view_| in TextInputManager.
This looks good, just a few comments. https://codereview.chromium.org/2045363002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2045363002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_aura.cc:1441: // TODO(wjmaclean): can host_ ever be null? Nit: That comment can be removed. https://codereview.chromium.org/2045363002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_aura.cc:1462: text_input_manager_->GetActiveWidget()->ImeSetComposition( I have a slight concern about nullptr derefs here. RenderWidgetHostViewChildFrame has its view_ pointer cleared when Destroy() is called, but it doesn't clear itself from TextInputManager's list until it is actually deleted, and there is a delay between those things because RWHVCF::Destroy() calls DeleteSoon(). It's a bit of a pain to manage that, I'd like to investigate removing that deferred deletion when I have time. https://codereview.chromium.org/2045363002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/2045363002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:4510: class TextInputManagerTestBase : public RenderWidgetHostViewAuraTest { Is this needed, or is it feasible to roll this into RenderWidgetHostViewAuraTest? https://codereview.chromium.org/2045363002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:4578: // the call leads to sending an IPC to the corresponding renderer process. A couple of typos in this paragraph: 'veirfy', and the second sentence is a bit mangled, after the opening parenthesis. https://codereview.chromium.org/2045363002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/text_input_manager.h (right): https://codereview.chromium.org/2045363002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/text_input_manager.h:45: // The following methods give are used to obtain information about IME-related Nit: 'methods give are used' https://codereview.chromium.org/2045363002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/text_input_manager.h:51: RenderWidgetHostViewBase* GetActiveView() const; Is GetActiveView() still necessary? Looking at this now, since TextInputClient messages go to the top RWHV and then are routed straight to the active RenderWidgetHostImpl, I'm not clear on how GetActiveView() is intended to be used (or, if only for unusual side cases, it the caller could call GetActiveWidget()->GetView()).
PTAL. https://codereview.chromium.org/2045363002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2045363002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_aura.cc:1441: // TODO(wjmaclean): can host_ ever be null? On 2016/06/13 19:37:47, kenrb wrote: > Nit: That comment can be removed. Acknowledged. https://codereview.chromium.org/2045363002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_aura.cc:1462: text_input_manager_->GetActiveWidget()->ImeSetComposition( On 2016/06/13 19:37:47, kenrb wrote: > I have a slight concern about nullptr derefs here. > RenderWidgetHostViewChildFrame has its view_ pointer cleared when Destroy() is > called, but it doesn't clear itself from TextInputManager's list until it is > actually deleted, and there is a delay between those things because > RWHVCF::Destroy() calls DeleteSoon(). It's a bit of a pain to manage that, I'd > like to investigate removing that deferred deletion when I have time. Agreed. I think it is safe to unregister here rather than the dtor of the base class. https://codereview.chromium.org/2045363002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/2045363002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:4510: class TextInputManagerTestBase : public RenderWidgetHostViewAuraTest { On 2016/06/13 19:37:47, kenrb wrote: > Is this needed, or is it feasible to roll this into > RenderWidgetHostViewAuraTest? Moved into the actual test class which was subclassing this (InputMethodResultsAuraTest). Also, this is better be a separate class as I am instantiating several mock views and processes and need to clean them up later in TearDown(). https://codereview.chromium.org/2045363002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:4578: // the call leads to sending an IPC to the corresponding renderer process. On 2016/06/13 19:37:47, kenrb wrote: > A couple of typos in this paragraph: 'veirfy', and the second sentence is a bit > mangled, after the opening parenthesis. Acknowledged. https://codereview.chromium.org/2045363002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/text_input_manager.h (right): https://codereview.chromium.org/2045363002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/text_input_manager.h:45: // The following methods give are used to obtain information about IME-related On 2016/06/13 19:37:47, kenrb wrote: > Nit: 'methods give are used' Acknowledged. https://codereview.chromium.org/2045363002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/text_input_manager.h:51: RenderWidgetHostViewBase* GetActiveView() const; On 2016/06/13 19:37:47, kenrb wrote: > Is GetActiveView() still necessary? Looking at this now, since TextInputClient > messages go to the top RWHV and then are routed straight to the active > RenderWidgetHostImpl, I'm not clear on how GetActiveView() is intended to be > used (or, if only for unusual side cases, it the caller could call > GetActiveWidget()->GetView()). It is not. I removed it. We store state for each view and hence I was returning the active view here. But we do not actually need the view as all the IPC go through the widget. The way it is now, we store state for each view but we forward to widget.
https://codereview.chromium.org/2045363002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2045363002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_aura.cc:1462: text_input_manager_->GetActiveWidget()->ImeSetComposition( On 2016/06/17 22:36:19, ehsaaan (OOO till June 22) wrote: > On 2016/06/13 19:37:47, kenrb wrote: > > I have a slight concern about nullptr derefs here. > > RenderWidgetHostViewChildFrame has its view_ pointer cleared when Destroy() is > > called, but it doesn't clear itself from TextInputManager's list until it is > > actually deleted, and there is a delay between those things because > > RWHVCF::Destroy() calls DeleteSoon(). It's a bit of a pain to manage that, I'd > > like to investigate removing that deferred deletion when I have time. > > Agreed. I think it is safe to unregister here rather than the dtor of the base > class. You need to either address that problem before this patch lands, or check for null return on GetActiveWidget(). https://codereview.chromium.org/2045363002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/2045363002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:4514: // A group of tests which verify thatthe IME method results are routed to the nit: "thatthe" https://codereview.chromium.org/2045363002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:4518: // each tests is about one of the IME result methods. The method is called on I am not clear what this sentence is trying to say exactly: "Then, each tests is about one of the IME result methods."
Description was changed from ========== Routing IME Result Calls to the Correct RenderWidgetHost (Aura Only) Currently, the IME result calls on ui::TextInputClient (RenderWidgetHostViewAura) are only routed to the RenderWidgetHostImpl corresponding to the tab's RenderWidgetHostView. Therefore, the IME related IPC arrive at the main frame's RenderWidget only. In the OOPIF structure, the IME result might be intended to a child frame's RenderWidget. This patch will route all such IME calls to the RenderWidgetHost which currently has the active text input state. The patch also adds four unit tests to RenderWidgetHostViewAuraTests to verify that the routing is done properly. BUG=578168 ========== to ========== Routing IME Result Calls to the Correct RenderWidgetHost (Aura Only) Currently, the IME result calls on ui::TextInputClient (RenderWidgetHostViewAura) are only routed to the RenderWidgetHostImpl corresponding to the tab's RenderWidgetHostView. Therefore, the IME related IPC arrive at the main frame's RenderWidget only. In the OOPIF structure, the IME result might be intended to a child frame's RenderWidget. This patch will route all such IME calls to the RenderWidgetHost which currently has the active text input state. The patch also adds four unit tests to RenderWidgetHostViewAuraTests to verify that the routing is done properly. BUG=578168 ==========
Sending a few style comments while I have a chance, to get them out of the way. https://codereview.chromium.org/2045363002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2045363002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:1487: if (text_input_manager_ && text_input_manager_->GetActiveWidget()) nit: Needs braces, since body doesn't fit on one line. https://codereview.chromium.org/2045363002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.h (right): https://codereview.chromium.org/2045363002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.h:356: friend class InputMethodResultAuraTest; nit: Alphabetize. https://codereview.chromium.org/2045363002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/2045363002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:4611: RenderWidgetHostImpl* render_widget_host = Might be easier to read if you avoid declaring it as a local and just return it. https://codereview.chromium.org/2045363002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:4654: // IPC message InputMsg_ImeSetComposition sent to the right RendererProcess. nit: being sent nit: renderer process (since RendererProcess is not a class name) (same in the tests below) https://codereview.chromium.org/2045363002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:4656: std::vector<size_t> activation_sequence{0, 1, 2, 1, 1, 3, 0, 3, 1}; nit: This needs a comment and/or a clearer name. Same below. Also, all the tests seem to use the same sequence, so maybe it should be abstracted out into the test class? https://codereview.chromium.org/2045363002/diff/40001/content/browser/rendere... File content/browser/renderer_host/text_input_manager.cc (right): https://codereview.chromium.org/2045363002/diff/40001/content/browser/rendere... content/browser/renderer_host/text_input_manager.cc:51: RenderWidgetHostImpl* TextInputManager::GetActiveWidget() const { There's a bunch of places that just call GetActiveWidget()->GetView(), with additional null checks. Maybe we should have both GetActiveWidget() and GetActiveView(), to avoid the "there and back again" under the hood? https://codereview.chromium.org/2045363002/diff/40001/content/browser/rendere... File content/browser/renderer_host/text_input_manager.h (left): https://codereview.chromium.org/2045363002/diff/40001/content/browser/rendere... content/browser/renderer_host/text_input_manager.h:45: // |TextInputState.type| of ui::TEXT_INPUT_TYPE_NONE. We've lost the last sentence of the comment. Should that go on |active_view_|'s declaration?
Thanks for the reviews. PTAL. https://codereview.chromium.org/2045363002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2045363002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_aura.cc:1462: text_input_manager_->GetActiveWidget()->ImeSetComposition( On 2016/06/20 16:04:06, kenrb wrote: > On 2016/06/17 22:36:19, ehsaaan (OOO till June 22) wrote: > > On 2016/06/13 19:37:47, kenrb wrote: > > > I have a slight concern about nullptr derefs here. > > > RenderWidgetHostViewChildFrame has its view_ pointer cleared when Destroy() > is > > > called, but it doesn't clear itself from TextInputManager's list until it is > > > actually deleted, and there is a delay between those things because > > > RWHVCF::Destroy() calls DeleteSoon(). It's a bit of a pain to manage that, > I'd > > > like to investigate removing that deferred deletion when I have time. > > > > Agreed. I think it is safe to unregister here rather than the dtor of the base > > class. > > You need to either address that problem before this patch lands, or check for > null return on GetActiveWidget(). We already early return on top when GetActiveWidget() is nullptr (line 1442). As far as the functionality of TextInputManager goes, unregistering at base dtor is fine. But there will be a period of time between here and then that the TextInputManager will return the child frame's widget as active and that seems wrong. So unregistering here should be safer. https://codereview.chromium.org/2045363002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2045363002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:1487: if (text_input_manager_ && text_input_manager_->GetActiveWidget()) On 2016/06/21 21:13:56, Charlie Reis wrote: > nit: Needs braces, since body doesn't fit on one line. Done. https://codereview.chromium.org/2045363002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.h (right): https://codereview.chromium.org/2045363002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.h:356: friend class InputMethodResultAuraTest; On 2016/06/21 21:13:56, Charlie Reis wrote: > nit: Alphabetize. Done. https://codereview.chromium.org/2045363002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/2045363002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:4514: // A group of tests which verify thatthe IME method results are routed to the On 2016/06/20 16:04:06, kenrb wrote: > nit: "thatthe" Done. https://codereview.chromium.org/2045363002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:4518: // each tests is about one of the IME result methods. The method is called on On 2016/06/20 16:04:06, kenrb wrote: > I am not clear what this sentence is trying to say exactly: "Then, each tests is > about one of the IME result methods." Acknowledged. https://codereview.chromium.org/2045363002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:4611: RenderWidgetHostImpl* render_widget_host = On 2016/06/21 21:13:56, Charlie Reis wrote: > Might be easier to read if you avoid declaring it as a local and just return it. Acknowledged. https://codereview.chromium.org/2045363002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:4654: // IPC message InputMsg_ImeSetComposition sent to the right RendererProcess. On 2016/06/21 21:13:56, Charlie Reis wrote: > nit: being sent > nit: renderer process (since RendererProcess is not a class name) > > (same in the tests below) Done. https://codereview.chromium.org/2045363002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:4656: std::vector<size_t> activation_sequence{0, 1, 2, 1, 1, 3, 0, 3, 1}; On 2016/06/21 21:13:56, Charlie Reis wrote: > nit: This needs a comment and/or a clearer name. Same below. Also, all the > tests seem to use the same sequence, so maybe it should be abstracted out into > the test class? Acknowledged. https://codereview.chromium.org/2045363002/diff/40001/content/browser/rendere... File content/browser/renderer_host/text_input_manager.cc (right): https://codereview.chromium.org/2045363002/diff/40001/content/browser/rendere... content/browser/renderer_host/text_input_manager.cc:51: RenderWidgetHostImpl* TextInputManager::GetActiveWidget() const { On 2016/06/21 21:13:56, Charlie Reis wrote: > There's a bunch of places that just call GetActiveWidget()->GetView(), with > additional null checks. Maybe we should have both GetActiveWidget() and > GetActiveView(), to avoid the "there and back again" under the hood? I found three instances: in 'text_input_test_utils.cc': GetActiveViewFromWebContents() TextInputManagerTester::GetActiveView(). and in 'render_widget_host_view_aura_unittest.cc': InputMethodAuraTest::ActivateView() (which is now refactored into ActivateViewForTextInputManager and moved to RenderWidgetHostViewAuraTest). Since all the instances are for testing only, should we still have the dedicated method for view? https://codereview.chromium.org/2045363002/diff/40001/content/browser/rendere... File content/browser/renderer_host/text_input_manager.h (left): https://codereview.chromium.org/2045363002/diff/40001/content/browser/rendere... content/browser/renderer_host/text_input_manager.h:45: // |TextInputState.type| of ui::TEXT_INPUT_TYPE_NONE. On 2016/06/21 21:13:56, Charlie Reis wrote: > We've lost the last sentence of the comment. Should that go on |active_view_|'s > declaration? Yes and thanks for noticing this.
https://codereview.chromium.org/2045363002/diff/40001/content/browser/rendere... File content/browser/renderer_host/text_input_manager.cc (right): https://codereview.chromium.org/2045363002/diff/40001/content/browser/rendere... content/browser/renderer_host/text_input_manager.cc:51: RenderWidgetHostImpl* TextInputManager::GetActiveWidget() const { On 2016/06/22 18:26:33, ehsaaan (OOO till June 22) wrote: > On 2016/06/21 21:13:56, Charlie Reis wrote: > > There's a bunch of places that just call GetActiveWidget()->GetView(), with > > additional null checks. Maybe we should have both GetActiveWidget() and > > GetActiveView(), to avoid the "there and back again" under the hood? > > I found three instances: > in 'text_input_test_utils.cc': > GetActiveViewFromWebContents() > TextInputManagerTester::GetActiveView(). > and in 'render_widget_host_view_aura_unittest.cc': > InputMethodAuraTest::ActivateView() (which is now refactored into > ActivateViewForTextInputManager and moved to RenderWidgetHostViewAuraTest). > > Since all the instances are for testing only, should we still have the dedicated > method for view? Yes. Also, I missed this in the previous review, but since the implementation is trivial, it should be inlined into the .h file as active_view(). That said, if you want to steer people towards using GetActiveWidget() in practice, I'm fine with calling it active_view_for_testing(). https://codereview.chromium.org/2045363002/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/2045363002/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:4135: std::vector<RenderWidgetHostViewBase*> views_; These don't need to be public, do they? Not sure if private works, but they should be at least protected. https://codereview.chromium.org/2045363002/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:4137: // A seqnece of indices in [0, 3] which determines the index of a RWHV in nit: sequence https://codereview.chromium.org/2045363002/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:4138: // |views_|. This value is used in the tests to sequentially make a RWHV nit: s/value/sequence/ nit: make a series of RWHVs active to test the subsequent IME actions. https://codereview.chromium.org/2045363002/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:4140: std::vector<size_t> active_view_sequence; style nit: End in underscore.
Aside from creis' comments still outstanding, this looks good. lgtm https://codereview.chromium.org/2045363002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2045363002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_aura.cc:1462: text_input_manager_->GetActiveWidget()->ImeSetComposition( On 2016/06/22 18:26:32, ehsaaan (OOO till June 22) wrote: > On 2016/06/20 16:04:06, kenrb wrote: > > On 2016/06/17 22:36:19, ehsaaan (OOO till June 22) wrote: > > > On 2016/06/13 19:37:47, kenrb wrote: > > > > I have a slight concern about nullptr derefs here. > > > > RenderWidgetHostViewChildFrame has its view_ pointer cleared when > Destroy() > > is > > > > called, but it doesn't clear itself from TextInputManager's list until it > is > > > > actually deleted, and there is a delay between those things because > > > > RWHVCF::Destroy() calls DeleteSoon(). It's a bit of a pain to manage that, > > I'd > > > > like to investigate removing that deferred deletion when I have time. > > > > > > Agreed. I think it is safe to unregister here rather than the dtor of the > base > > > class. > > > > You need to either address that problem before this patch lands, or check for > > null return on GetActiveWidget(). > > We already early return on top when GetActiveWidget() is nullptr (line 1442). > > As far as the functionality of TextInputManager goes, unregistering at base dtor > is fine. But there will be a period of time between here and then that the > TextInputManager will return the child frame's widget as active and that seems > wrong. So unregistering here should be safer. Okay, I had missed the line above. This seems fine. I don't quite understand that last comment, though. What do you mean by unregistering 'here'? I don't think RenderWidgetHostViewAura should be unregistering other RenderWidgetHostViews from the TextInputManager view list. https://codereview.chromium.org/2045363002/diff/40001/content/browser/rendere... File content/browser/renderer_host/text_input_manager.cc (right): https://codereview.chromium.org/2045363002/diff/40001/content/browser/rendere... content/browser/renderer_host/text_input_manager.cc:51: RenderWidgetHostImpl* TextInputManager::GetActiveWidget() const { On 2016/06/22 19:19:15, Charlie Reis wrote: > On 2016/06/22 18:26:33, ehsaaan (OOO till June 22) wrote: > > On 2016/06/21 21:13:56, Charlie Reis wrote: > > > There's a bunch of places that just call GetActiveWidget()->GetView(), with > > > additional null checks. Maybe we should have both GetActiveWidget() and > > > GetActiveView(), to avoid the "there and back again" under the hood? > > > > I found three instances: > > in 'text_input_test_utils.cc': > > GetActiveViewFromWebContents() > > TextInputManagerTester::GetActiveView(). > > and in 'render_widget_host_view_aura_unittest.cc': > > InputMethodAuraTest::ActivateView() (which is now refactored into > > ActivateViewForTextInputManager and moved to RenderWidgetHostViewAuraTest). > > > > Since all the instances are for testing only, should we still have the > dedicated > > method for view? > > Yes. Also, I missed this in the previous review, but since the implementation > is trivial, it should be inlined into the .h file as active_view(). > > That said, if you want to steer people towards using GetActiveWidget() in > practice, I'm fine with calling it active_view_for_testing(). Charlie: I had asked for removal of GetActiveView() in an earlier round of review on this CL, since it was only used in tests. Generally, IME code in the views is on the control flow path that comes up from the renderer and doesn't come through TextInputManager. 'Downward' IME flows like SetCompositionFlows that need this class go straight to the RenderWidgetHostImpl, so GetActiveWidget() makes sense here while GetActiveView() doesn't, really.
https://codereview.chromium.org/2045363002/diff/40001/content/browser/rendere... File content/browser/renderer_host/text_input_manager.cc (right): https://codereview.chromium.org/2045363002/diff/40001/content/browser/rendere... content/browser/renderer_host/text_input_manager.cc:51: RenderWidgetHostImpl* TextInputManager::GetActiveWidget() const { On 2016/06/22 19:39:50, kenrb wrote: > On 2016/06/22 19:19:15, Charlie Reis wrote: > > On 2016/06/22 18:26:33, ehsaaan (OOO till June 22) wrote: > > > On 2016/06/21 21:13:56, Charlie Reis wrote: > > > > There's a bunch of places that just call GetActiveWidget()->GetView(), > with > > > > additional null checks. Maybe we should have both GetActiveWidget() and > > > > GetActiveView(), to avoid the "there and back again" under the hood? > > > > > > I found three instances: > > > in 'text_input_test_utils.cc': > > > GetActiveViewFromWebContents() > > > TextInputManagerTester::GetActiveView(). > > > and in 'render_widget_host_view_aura_unittest.cc': > > > InputMethodAuraTest::ActivateView() (which is now refactored into > > > ActivateViewForTextInputManager and moved to RenderWidgetHostViewAuraTest). > > > > > > Since all the instances are for testing only, should we still have the > > dedicated > > > method for view? > > > > Yes. Also, I missed this in the previous review, but since the implementation > > is trivial, it should be inlined into the .h file as active_view(). > > > > That said, if you want to steer people towards using GetActiveWidget() in > > practice, I'm fine with calling it active_view_for_testing(). > > Charlie: I had asked for removal of GetActiveView() in an earlier round of > review on this CL, since it was only used in tests. Generally, IME code in the > views is on the control flow path that comes up from the renderer and doesn't > come through TextInputManager. 'Downward' IME flows like SetCompositionFlows > that need this class go straight to the RenderWidgetHostImpl, so > GetActiveWidget() makes sense here while GetActiveView() doesn't, really. Ok, I'm fine with steering people towards GetActiveWidget(). It still seems weird to store the active_view_ internally and have to do a bunch of work to get back to it in several places, so having a active_view_for_testing() seems useful to me. (I suppose we could store active_widget_ instead of active_view_ instead? That's probably disruptive, though.)
Thanks for the reviews! PTAL. https://codereview.chromium.org/2045363002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2045363002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_aura.cc:1462: text_input_manager_->GetActiveWidget()->ImeSetComposition( On 2016/06/22 19:39:50, kenrb wrote: > On 2016/06/22 18:26:32, ehsaaan (OOO till June 22) wrote: > > On 2016/06/20 16:04:06, kenrb wrote: > > > On 2016/06/17 22:36:19, ehsaaan (OOO till June 22) wrote: > > > > On 2016/06/13 19:37:47, kenrb wrote: > > > > > I have a slight concern about nullptr derefs here. > > > > > RenderWidgetHostViewChildFrame has its view_ pointer cleared when > > Destroy() > > > is > > > > > called, but it doesn't clear itself from TextInputManager's list until > it > > is > > > > > actually deleted, and there is a delay between those things because > > > > > RWHVCF::Destroy() calls DeleteSoon(). It's a bit of a pain to manage > that, > > > I'd > > > > > like to investigate removing that deferred deletion when I have time. > > > > > > > > Agreed. I think it is safe to unregister here rather than the dtor of the > > base > > > > class. > > > > > > You need to either address that problem before this patch lands, or check > for > > > null return on GetActiveWidget(). > > > > We already early return on top when GetActiveWidget() is nullptr (line 1442). > > > > As far as the functionality of TextInputManager goes, unregistering at base > dtor > > is fine. But there will be a period of time between here and then that the > > TextInputManager will return the child frame's widget as active and that seems > > wrong. So unregistering here should be safer. > > Okay, I had missed the line above. This seems fine. > > I don't quite understand that last comment, though. What do you mean by > unregistering 'here'? I don't think RenderWidgetHostViewAura should be > unregistering other RenderWidgetHostViews from the TextInputManager view list. Sorry for that confusing line. I was thinking of another cl here:https://codereview.chromium.org/2054163003/ where I decided to unregister a RWHVCF in its Destroy function. https://codereview.chromium.org/2045363002/diff/40001/content/browser/rendere... File content/browser/renderer_host/text_input_manager.cc (right): https://codereview.chromium.org/2045363002/diff/40001/content/browser/rendere... content/browser/renderer_host/text_input_manager.cc:51: RenderWidgetHostImpl* TextInputManager::GetActiveWidget() const { On 2016/06/22 19:39:50, kenrb wrote: > On 2016/06/22 19:19:15, Charlie Reis wrote: > > On 2016/06/22 18:26:33, ehsaaan (OOO till June 22) wrote: > > > On 2016/06/21 21:13:56, Charlie Reis wrote: > > > > There's a bunch of places that just call GetActiveWidget()->GetView(), > with > > > > additional null checks. Maybe we should have both GetActiveWidget() and > > > > GetActiveView(), to avoid the "there and back again" under the hood? > > > > > > I found three instances: > > > in 'text_input_test_utils.cc': > > > GetActiveViewFromWebContents() > > > TextInputManagerTester::GetActiveView(). > > > and in 'render_widget_host_view_aura_unittest.cc': > > > InputMethodAuraTest::ActivateView() (which is now refactored into > > > ActivateViewForTextInputManager and moved to RenderWidgetHostViewAuraTest). > > > > > > Since all the instances are for testing only, should we still have the > > dedicated > > > method for view? > > > > Yes. Also, I missed this in the previous review, but since the implementation > > is trivial, it should be inlined into the .h file as active_view(). > > > > That said, if you want to steer people towards using GetActiveWidget() in > > practice, I'm fine with calling it active_view_for_testing(). > > Charlie: I had asked for removal of GetActiveView() in an earlier round of > review on this CL, since it was only used in tests. Generally, IME code in the > views is on the control flow path that comes up from the renderer and doesn't > come through TextInputManager. 'Downward' IME flows like SetCompositionFlows > that need this class go straight to the RenderWidgetHostImpl, so > GetActiveWidget() makes sense here while GetActiveView() doesn't, really. I also agree with Ken on this. Views are used as key in maps since the stats are reported by the view. But the only use of knowing the active view/widget as of now is routing IME methods. I have added |active_view_for_testing()| following Charlie's second suggestion as it seems like the active view is only used for tests. https://codereview.chromium.org/2045363002/diff/40001/content/browser/rendere... content/browser/renderer_host/text_input_manager.cc:51: RenderWidgetHostImpl* TextInputManager::GetActiveWidget() const { On 2016/06/22 20:23:08, Charlie Reis wrote: > On 2016/06/22 19:39:50, kenrb wrote: > > On 2016/06/22 19:19:15, Charlie Reis wrote: > > > On 2016/06/22 18:26:33, ehsaaan (OOO till June 22) wrote: > > > > On 2016/06/21 21:13:56, Charlie Reis wrote: > > > > > There's a bunch of places that just call GetActiveWidget()->GetView(), > > with > > > > > additional null checks. Maybe we should have both GetActiveWidget() and > > > > > GetActiveView(), to avoid the "there and back again" under the hood? > > > > > > > > I found three instances: > > > > in 'text_input_test_utils.cc': > > > > GetActiveViewFromWebContents() > > > > TextInputManagerTester::GetActiveView(). > > > > and in 'render_widget_host_view_aura_unittest.cc': > > > > InputMethodAuraTest::ActivateView() (which is now refactored into > > > > ActivateViewForTextInputManager and moved to > RenderWidgetHostViewAuraTest). > > > > > > > > Since all the instances are for testing only, should we still have the > > > dedicated > > > > method for view? > > > > > > Yes. Also, I missed this in the previous review, but since the > implementation > > > is trivial, it should be inlined into the .h file as active_view(). > > > > > > That said, if you want to steer people towards using GetActiveWidget() in > > > practice, I'm fine with calling it active_view_for_testing(). > > > > Charlie: I had asked for removal of GetActiveView() in an earlier round of > > review on this CL, since it was only used in tests. Generally, IME code in the > > views is on the control flow path that comes up from the renderer and doesn't > > come through TextInputManager. 'Downward' IME flows like SetCompositionFlows > > that need this class go straight to the RenderWidgetHostImpl, so > > GetActiveWidget() makes sense here while GetActiveView() doesn't, really. > > Ok, I'm fine with steering people towards GetActiveWidget(). > > It still seems weird to store the active_view_ internally and have to do a bunch > of work to get back to it in several places, so having a > active_view_for_testing() seems useful to me. (I suppose we could store > active_widget_ instead of active_view_ instead? That's probably disruptive, > though.) Charlie: I actually think |active_widget_| rather than |active_view_| might work just fine now that I have a better understanding of the whole IME in content/. The only reason I used |active_view_| was RenderWidgetHostViewBase::TextInputStateChanged. We might be able to remove it and report it directly from RenderWidgetHost. But it will be a bigger change than this CL is about. I have added a |active_view_for_testing()|. https://codereview.chromium.org/2045363002/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/2045363002/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:4135: std::vector<RenderWidgetHostViewBase*> views_; On 2016/06/22 19:19:15, Charlie Reis wrote: > These don't need to be public, do they? Not sure if private works, but they > should be at least protected. Done. They need to be at least protected since they get referenced in tests. https://codereview.chromium.org/2045363002/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:4137: // A seqnece of indices in [0, 3] which determines the index of a RWHV in On 2016/06/22 19:19:15, Charlie Reis wrote: > nit: sequence Done. https://codereview.chromium.org/2045363002/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:4138: // |views_|. This value is used in the tests to sequentially make a RWHV On 2016/06/22 19:19:15, Charlie Reis wrote: > nit: s/value/sequence/ > nit: make a series of RWHVs active to test the subsequent IME actions. Done. https://codereview.chromium.org/2045363002/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:4140: std::vector<size_t> active_view_sequence; On 2016/06/22 19:19:15, Charlie Reis wrote: > style nit: End in underscore. Done.
Thanks, LGTM.
The CQ bit was checked by creis@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2045363002/140001
The CQ bit was unchecked by ekaramad@chromium.org
The CQ bit was checked by ekaramad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org Link to the patchset: https://codereview.chromium.org/2045363002/#ps140001 (title: "Formatting")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2045363002/140001
Message was sent while issue was closed.
Description was changed from ========== Routing IME Result Calls to the Correct RenderWidgetHost (Aura Only) Currently, the IME result calls on ui::TextInputClient (RenderWidgetHostViewAura) are only routed to the RenderWidgetHostImpl corresponding to the tab's RenderWidgetHostView. Therefore, the IME related IPC arrive at the main frame's RenderWidget only. In the OOPIF structure, the IME result might be intended to a child frame's RenderWidget. This patch will route all such IME calls to the RenderWidgetHost which currently has the active text input state. The patch also adds four unit tests to RenderWidgetHostViewAuraTests to verify that the routing is done properly. BUG=578168 ========== to ========== Routing IME Result Calls to the Correct RenderWidgetHost (Aura Only) Currently, the IME result calls on ui::TextInputClient (RenderWidgetHostViewAura) are only routed to the RenderWidgetHostImpl corresponding to the tab's RenderWidgetHostView. Therefore, the IME related IPC arrive at the main frame's RenderWidget only. In the OOPIF structure, the IME result might be intended to a child frame's RenderWidget. This patch will route all such IME calls to the RenderWidgetHost which currently has the active text input state. The patch also adds four unit tests to RenderWidgetHostViewAuraTests to verify that the routing is done properly. BUG=578168 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Routing IME Result Calls to the Correct RenderWidgetHost (Aura Only) Currently, the IME result calls on ui::TextInputClient (RenderWidgetHostViewAura) are only routed to the RenderWidgetHostImpl corresponding to the tab's RenderWidgetHostView. Therefore, the IME related IPC arrive at the main frame's RenderWidget only. In the OOPIF structure, the IME result might be intended to a child frame's RenderWidget. This patch will route all such IME calls to the RenderWidgetHost which currently has the active text input state. The patch also adds four unit tests to RenderWidgetHostViewAuraTests to verify that the routing is done properly. BUG=578168 ========== to ========== Routing IME Result Calls to the Correct RenderWidgetHost (Aura Only) Currently, the IME result calls on ui::TextInputClient (RenderWidgetHostViewAura) are only routed to the RenderWidgetHostImpl corresponding to the tab's RenderWidgetHostView. Therefore, the IME related IPC arrive at the main frame's RenderWidget only. In the OOPIF structure, the IME result might be intended to a child frame's RenderWidget. This patch will route all such IME calls to the RenderWidgetHost which currently has the active text input state. The patch also adds four unit tests to RenderWidgetHostViewAuraTests to verify that the routing is done properly. BUG=578168 Committed: https://crrev.com/d33fd7859cac797067a2b8a8e55f6dbac59bee19 Cr-Commit-Position: refs/heads/master@{#401465} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/d33fd7859cac797067a2b8a8e55f6dbac59bee19 Cr-Commit-Position: refs/heads/master@{#401465}
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/2095813002/ by reillyg@chromium.org. The reason for reverting is: This patch added unit tests that leave behind state that will break other tests if content_unittests is run with --single-process-tests (as it is on the Dr. Memory bots).. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
