Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2029423003:
OOPIF IME: Renderer Side Changes (Closed)
|
Created:
4 years, 6 months ago by EhsanK Modified:
4 years, 5 months ago CC:
dcheng, blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, dglazkov+blink, jam, kinuko+watch, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-blink_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, 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. |
DescriptionOOPIF IME: Renderer Side Changes
Currently, all IME-related IPC are handled at the RenderViewImpl first
before calling the base class
methods on RenderWidget (ImeSetComposition and
ImeConfirmComposition). Further, to query IME-related
state, e.g., GetTextInputType(), RenderViewImpl overrides the method
from RenderWidget so that it can access the
state from pepper plugin.
These overrides are needed to either:
1- Access focused PepperPluginInstacneImpl to set/get IME-related
state.
2- Use focused frame to set/get IME-related state.
This CL removes all those methods from RenderViewImpl and handles
them in RenderWidget. To this end, a reference to
the focused plugin is kept in RenderWidget and updated by its owning
RenderFrame (to get IME-related state). Also, new API is added to blink
to enable RenderWidget handle IME tasks related to focusedFrame.
In line with the separation of WebViewImpl from WebWidget,
as well as avoiding potential race conditions between
mutiple Widgets on the same process, most of the IME-
related WebWidget methods are now specifically
implemented for WebFrameWidgetImpl.
Finally, a new private API is added to WebFrameWidgetImpl to help the
widget distinguish between in process frames which do or do not belong
to widget's local root.
BUG=578168, 618774, 626746
Committed: https://crrev.com/2a46d63b3c442f1f0d26cd1484c25eb766a064bc
Cr-Commit-Position: refs/heads/master@{#406263}
Patch Set 1 #Patch Set 2 : Fixed Initialization Order #Patch Set 3 : Implemented WebViewImpl::getCompositionCharacterBounds #
Total comments: 11
Patch Set 4 : Rebase #Patch Set 5 : Implementing Methods in both WebViewImpl and WebFrameWidgetImpl #Patch Set 6 : Undid Changes to render_frame_impl.h #
Total comments: 12
Patch Set 7 : Addressing kenrb@'s comments #
Total comments: 4
Patch Set 8 : Rebase and Small Changes #Patch Set 9 : Addressing kenrb@'s comments #Patch Set 10 : Rebase #
Total comments: 19
Patch Set 11 : Addressing lfg@'s comments. #
Total comments: 10
Patch Set 12 : More verifications of the focused frame being the right one. #Patch Set 13 : Rebased #
Total comments: 2
Patch Set 14 : Changed Implementation of WebFrameWidgetImpl::setFocus to mimic that of WebViewImpl #
Total comments: 2
Patch Set 15 : Fixed an Error #
Total comments: 12
Patch Set 16 : Addressing comments #Patch Set 17 : Checking for focusedLocalFrameInWidget() in more methods #Patch Set 18 : Using focusedCoreFrame() instead of focusController().focusedFrame() in WebViewImpl #Messages
Total messages: 65 (32 generated)
Description was changed from ========== OOPIF IME: Renerer Side Changes Moved some of the IME-related methods in RenderViewImpl to RenderWidget. These are methods which RenderViewImpl overrides from RenderWidget. The reason for override is often: 1- Access to focusedFrame(). 2- Get/Set-ing IME-related state from the focused PepperPluginInstanceImpl. To address 1 in adjusting replacement range and 2 for setting IME state, the IPC calls to RenderWidget will first go through the frame using an obsrever call. The call will then bounce back to the widget is the IPC is not destined to a focused plugin. Another change is to hold a reference to focused pepper plugin in RenderWidget so that the IME-related state can be obtained and sent via the IME-related IPC back to the browser. Finally, to get the composition character bounds, the RenderWidget uses the WebFrameWidgetImpl which again needs access to the focused frame. BUG=578168 ========== to ========== OOPIF IME: Renerer Side Changes Currently, all IME-related IPC are handled at the RenderViewImpl first before calling the base class methods on RenderWidget (ImeSetComposition and ImeConfirmComposition). Further, to query IME-related state, e.g.., GetTextInputType(), the RenderViewImpl override the method from RenderWidget. These overrides are needed to either: 1- Access focused PepperPluginInstacneImpl to set/get IME-related state. 2- Use focused frame to set/get IME-related state. This CL removes all those methods from RenderViewImpl and handles them in RenderWidget. To this end, a reference to the focused plugin is kept in RenderWidget and updated by its owning RenderFrame (to get IME-related state). Also, some of the IPC handlers for IME take a detour into RenderFrame to perform the tasks which need frame-level knowledge. This CL is a prerequisite for OOPIF IME and should conclude the implementation of IME on the renderer side. BUG=578168 ==========
Description was changed from ========== OOPIF IME: Renerer Side Changes Currently, all IME-related IPC are handled at the RenderViewImpl first before calling the base class methods on RenderWidget (ImeSetComposition and ImeConfirmComposition). Further, to query IME-related state, e.g.., GetTextInputType(), the RenderViewImpl override the method from RenderWidget. These overrides are needed to either: 1- Access focused PepperPluginInstacneImpl to set/get IME-related state. 2- Use focused frame to set/get IME-related state. This CL removes all those methods from RenderViewImpl and handles them in RenderWidget. To this end, a reference to the focused plugin is kept in RenderWidget and updated by its owning RenderFrame (to get IME-related state). Also, some of the IPC handlers for IME take a detour into RenderFrame to perform the tasks which need frame-level knowledge. This CL is a prerequisite for OOPIF IME and should conclude the implementation of IME on the renderer side. BUG=578168 ========== to ========== OOPIF IME: Renerer Side Changes Currently, all IME-related IPC are handled at the RenderViewImpl first before calling the base class methods on RenderWidget (ImeSetComposition and ImeConfirmComposition). Further, to query IME-related state, e.g.., GetTextInputType(), the RenderViewImpl override the method from RenderWidget. These overrides are needed to either: 1- Access focused PepperPluginInstacneImpl to set/get IME-related state. 2- Use focused frame to set/get IME-related state. This CL removes all those methods from RenderViewImpl and handles them in RenderWidget. To this end, a reference to the focused plugin is kept in RenderWidget and updated by its owning RenderFrame (to get IME-related state). Also, some of the IPC handlers for IME take a detour into RenderFrame to perform the tasks which need frame-level knowledge. This CL is a prerequisite for OOPIF IME and should conclude the implementation of IME on the renderer side. BUG=578168 ==========
Description was changed from ========== OOPIF IME: Renerer Side Changes Currently, all IME-related IPC are handled at the RenderViewImpl first before calling the base class methods on RenderWidget (ImeSetComposition and ImeConfirmComposition). Further, to query IME-related state, e.g.., GetTextInputType(), the RenderViewImpl override the method from RenderWidget. These overrides are needed to either: 1- Access focused PepperPluginInstacneImpl to set/get IME-related state. 2- Use focused frame to set/get IME-related state. This CL removes all those methods from RenderViewImpl and handles them in RenderWidget. To this end, a reference to the focused plugin is kept in RenderWidget and updated by its owning RenderFrame (to get IME-related state). Also, some of the IPC handlers for IME take a detour into RenderFrame to perform the tasks which need frame-level knowledge. This CL is a prerequisite for OOPIF IME and should conclude the implementation of IME on the renderer side. BUG=578168 ========== to ========== OOPIF IME: Renerer Side Changes Currently, all IME-related IPC are handled at the RenderViewImpl first before calling the base class methods on RenderWidget (ImeSetComposition and ImeConfirmComposition). Further, to query IME-related state, e.g., GetTextInputType(), RenderViewImpl overrides the method from RenderWidget so that it can access the state from pepper plugin. These overrides are needed to either: 1- Access focused PepperPluginInstacneImpl to set/get IME-related state. 2- Use focused frame to set/get IME-related state. This CL removes all those methods from RenderViewImpl and handles them in RenderWidget. To this end, a reference to the focused plugin is kept in RenderWidget and updated by its owning RenderFrame (to get IME-related state). Also, some of the IPC handlers for IME take a detour into RenderFrame to perform the tasks which need frame-level knowledge. This CL is a prerequisite for OOPIF IME and should conclude the implementation of IME on the renderer side. BUG=578168 ==========
Description was changed from ========== OOPIF IME: Renerer Side Changes Currently, all IME-related IPC are handled at the RenderViewImpl first before calling the base class methods on RenderWidget (ImeSetComposition and ImeConfirmComposition). Further, to query IME-related state, e.g., GetTextInputType(), RenderViewImpl overrides the method from RenderWidget so that it can access the state from pepper plugin. These overrides are needed to either: 1- Access focused PepperPluginInstacneImpl to set/get IME-related state. 2- Use focused frame to set/get IME-related state. This CL removes all those methods from RenderViewImpl and handles them in RenderWidget. To this end, a reference to the focused plugin is kept in RenderWidget and updated by its owning RenderFrame (to get IME-related state). Also, some of the IPC handlers for IME take a detour into RenderFrame to perform the tasks which need frame-level knowledge. This CL is a prerequisite for OOPIF IME and should conclude the implementation of IME on the renderer side. BUG=578168 ========== to ========== OOPIF IME: Renerer Side Changes Currently, all IME-related IPC are handled at the RenderViewImpl first before calling the base class methods on RenderWidget (ImeSetComposition and ImeConfirmComposition). Further, to query IME-related state, e.g., GetTextInputType(), RenderViewImpl overrides the method from RenderWidget so that it can access the state from pepper plugin. These overrides are needed to either: 1- Access focused PepperPluginInstacneImpl to set/get IME-related state. 2- Use focused frame to set/get IME-related state. This CL removes all those methods from RenderViewImpl and handles them in RenderWidget. To this end, a reference to the focused plugin is kept in RenderWidget and updated by its owning RenderFrame (to get IME-related state). Also, some of the IPC handlers for IME take a detour into RenderFrame to perform the tasks which need frame-level knowledge. This CL is a prerequisite for OOPIF IME and should conclude the implementation of IME on the renderer side. BUG=578168 ==========
Description was changed from ========== OOPIF IME: Renerer Side Changes Currently, all IME-related IPC are handled at the RenderViewImpl first before calling the base class methods on RenderWidget (ImeSetComposition and ImeConfirmComposition). Further, to query IME-related state, e.g., GetTextInputType(), RenderViewImpl overrides the method from RenderWidget so that it can access the state from pepper plugin. These overrides are needed to either: 1- Access focused PepperPluginInstacneImpl to set/get IME-related state. 2- Use focused frame to set/get IME-related state. This CL removes all those methods from RenderViewImpl and handles them in RenderWidget. To this end, a reference to the focused plugin is kept in RenderWidget and updated by its owning RenderFrame (to get IME-related state). Also, some of the IPC handlers for IME take a detour into RenderFrame to perform the tasks which need frame-level knowledge. This CL is a prerequisite for OOPIF IME and should conclude the implementation of IME on the renderer side. BUG=578168 ========== to ========== OOPIF IME: Renderer Side Changes Currently, all IME-related IPC are handled at the RenderViewImpl first before calling the base class methods on RenderWidget (ImeSetComposition and ImeConfirmComposition). Further, to query IME-related state, e.g., GetTextInputType(), RenderViewImpl overrides the method from RenderWidget so that it can access the state from pepper plugin. These overrides are needed to either: 1- Access focused PepperPluginInstacneImpl to set/get IME-related state. 2- Use focused frame to set/get IME-related state. This CL removes all those methods from RenderViewImpl and handles them in RenderWidget. To this end, a reference to the focused plugin is kept in RenderWidget and updated by its owning RenderFrame (to get IME-related state). Also, some of the IPC handlers for IME take a detour into RenderFrame to perform the tasks which need frame-level knowledge. This CL is a prerequisite for OOPIF IME and should conclude the implementation of IME on the renderer side. BUG=578168 ==========
ekaramad@chromium.org changed reviewers: + dcheng@chromium.org, kenrb@chromium.org
Hello Ken, Daniel, Could you please take a look? I think it is much easier to keep IME inside RenderWidget. But I could not move the code from RenderViewImpl to RenderWidget very smoothly. What do you think? https://codereview.chromium.org/2029423003/diff/40001/content/renderer/render... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2029423003/diff/40001/content/renderer/render... content/renderer/render_widget.cc:1490: OnImeSetComposition(this, text, underlines, replacement_range, This and the one below for OnImeConfirmComposition could be removed if we send the IME IPC to the RenderFrameImpl instead (assuming that the whole change makes sense). https://codereview.chromium.org/2029423003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2029423003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebViewImpl.cpp:2859: bool WebViewImpl::getCompositionCharacterBounds(WebVector<WebRect>& bounds) This is basically RenderViewImpl::GetCompositionCharacterBounds which I moved here.
Thanks for putting this up. Sorry it took a while for me to get to it. There is one thing that needs to be resolved before getting further review. https://codereview.chromium.org/2029423003/diff/40001/content/renderer/render... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2029423003/diff/40001/content/renderer/render... content/renderer/render_widget.cc:1490: OnImeSetComposition(this, text, underlines, replacement_range, On 2016/06/03 02:38:54, ehsaaan wrote: > This and the one below for OnImeConfirmComposition could be removed if we send > the IME IPC to the RenderFrameImpl instead (assuming that the whole change makes > sense). This is a bit tricky. The code is a bit tortured as is, because you are notifying every RenderFrameImpl when you only need to let the local root have a chance to intercept the message. The trouble with moving it to RenderFrameImpl would be that it might arrive out of order with other InputMsg_ events going to the RenderWidget, potentially causing race bugs. Isn't it unnecessary now, though? You have moved a pointer to focused_pepper_plugin_ into RenderWidget, and it really looks a lot more at home here. Why not remove it from RenderFrameImpl and just have related Pepper instance tracking all in RenderWidget, along with accented character replacement? I don't see anything there that needs to live in RenderFrameImpl. https://codereview.chromium.org/2029423003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2029423003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebViewImpl.cpp:2859: bool WebViewImpl::getCompositionCharacterBounds(WebVector<WebRect>& bounds) On 2016/06/03 02:38:54, ehsaaan wrote: > This is basically RenderViewImpl::GetCompositionCharacterBounds which I moved > here. Seems fine, to me. https://codereview.chromium.org/2029423003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebViewImpl.h (right): https://codereview.chromium.org/2029423003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebViewImpl.h:161: bool getCompositionCharacterBounds(WebVector<WebRect>& bounds) override; The IME methods in WebViewImpl could possibly be moved to WebFrameWidgetImpl, but I don't have a strong opinion. Things that deal strictly with a focused frame should be fine here. Daniel might have an opinion on that one way or the other.
https://codereview.chromium.org/2029423003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebViewImpl.h (right): https://codereview.chromium.org/2029423003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebViewImpl.h:161: bool getCompositionCharacterBounds(WebVector<WebRect>& bounds) override; On 2016/06/07 at 20:18:13, kenrb wrote: > The IME methods in WebViewImpl could possibly be moved to WebFrameWidgetImpl, but I don't have a strong opinion. Things that deal strictly with a focused frame should be fine here. Daniel might have an opinion on that one way or the other. It seems inconsistent to move the code off RenderView to RenderWidget but not off WebView to WebFrameWidget. So I think we should do that in this CL as well, rather than just having WebFrameWidget delegate to WebViewImpl.
https://codereview.chromium.org/2029423003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebViewImpl.h (right): https://codereview.chromium.org/2029423003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebViewImpl.h:161: bool getCompositionCharacterBounds(WebVector<WebRect>& bounds) override; On 2016/06/08 04:39:43, dcheng wrote: > On 2016/06/07 at 20:18:13, kenrb wrote: > > The IME methods in WebViewImpl could possibly be moved to WebFrameWidgetImpl, > but I don't have a strong opinion. Things that deal strictly with a focused > frame should be fine here. Daniel might have an opinion on that one way or the > other. > > It seems inconsistent to move the code off RenderView to RenderWidget but not > off WebView to WebFrameWidget. So I think we should do that in this CL as well, > rather than just having WebFrameWidget delegate to WebViewImpl. A bit more concrete argument for this that I thought of is that if there is race contention between IME messages and focus changes, we could end up with IME messages going to a RenderWidget that does not have the currently focused frame underneath it. If focus is changing to another process then it should be fine, nothing will happen. But if it is moving to a frame another RenderWidget in the same process then that might change the behavior. It seems a lot more consistent to have this code in WebFrameWidgetImpl and verify that focused frame (if not nullptr) has the correct local root.
PTAL. https://codereview.chromium.org/2029423003/diff/40001/content/renderer/render... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2029423003/diff/40001/content/renderer/render... content/renderer/render_widget.cc:1490: OnImeSetComposition(this, text, underlines, replacement_range, On 2016/06/07 20:18:13, kenrb wrote: > On 2016/06/03 02:38:54, ehsaaan wrote: > > This and the one below for OnImeConfirmComposition could be removed if we send > > the IME IPC to the RenderFrameImpl instead (assuming that the whole change > makes > > sense). > > This is a bit tricky. The code is a bit tortured as is, because you are > notifying every RenderFrameImpl when you only need to let the local root have a > chance to intercept the message. The trouble with moving it to RenderFrameImpl > would be that it might arrive out of order with other InputMsg_ events going to > the RenderWidget, potentially causing race bugs. > > Isn't it unnecessary now, though? You have moved a pointer to > focused_pepper_plugin_ into RenderWidget, and it really looks a lot more at home > here. Why not remove it from RenderFrameImpl and just have related Pepper > instance tracking all in RenderWidget, along with accented character > replacement? I don't see anything there that needs to live in RenderFrameImpl. Done. https://codereview.chromium.org/2029423003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): https://codereview.chromium.org/2029423003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:469: return view()->setComposition(text, underlines, selectionStart, selectionEnd); Following the comments on getCompositionCharacterBounds, I believe these have to also be implemented for WebFrameWidgetImpl separately. At this point I am not sure yet how much work this needs. Does not seem like much. https://codereview.chromium.org/2029423003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebViewImpl.h (right): https://codereview.chromium.org/2029423003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebViewImpl.h:161: bool getCompositionCharacterBounds(WebVector<WebRect>& bounds) override; On 2016/06/08 15:26:04, kenrb wrote: > On 2016/06/08 04:39:43, dcheng wrote: > > On 2016/06/07 at 20:18:13, kenrb wrote: > > > The IME methods in WebViewImpl could possibly be moved to > WebFrameWidgetImpl, > > but I don't have a strong opinion. Things that deal strictly with a focused > > frame should be fine here. Daniel might have an opinion on that one way or the > > other. > > > > It seems inconsistent to move the code off RenderView to RenderWidget but not > > off WebView to WebFrameWidget. So I think we should do that in this CL as > well, > > rather than just having WebFrameWidget delegate to WebViewImpl. > > A bit more concrete argument for this that I thought of is that if there is race > contention between IME messages and focus changes, we could end up with IME > messages going to a RenderWidget that does not have the currently focused frame > underneath it. If focus is changing to another process then it should be fine, > nothing will happen. But if it is moving to a frame another RenderWidget in the > same process then that might change the behavior. It seems a lot more consistent > to have this code in WebFrameWidgetImpl and verify that focused frame (if not > nullptr) has the correct local root. Acknowledged. https://codereview.chromium.org/2029423003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebViewImpl.h:161: bool getCompositionCharacterBounds(WebVector<WebRect>& bounds) override; On 2016/06/08 04:39:43, dcheng wrote: > On 2016/06/07 at 20:18:13, kenrb wrote: > > The IME methods in WebViewImpl could possibly be moved to WebFrameWidgetImpl, > but I don't have a strong opinion. Things that deal strictly with a focused > frame should be fine here. Daniel might have an opinion on that one way or the > other. > > It seems inconsistent to move the code off RenderView to RenderWidget but not > off WebView to WebFrameWidget. So I think we should do that in this CL as well, > rather than just having WebFrameWidget delegate to WebViewImpl. Acknowledged.
ekaramad@chromium.org changed reviewers: + lfg@chromium.org
+lfg@ who is also working on the separation of widget and view.
https://codereview.chromium.org/2029423003/diff/100001/content/renderer/rende... File content/renderer/render_widget.h (right): https://codereview.chromium.org/2029423003/diff/100001/content/renderer/rende... content/renderer/render_widget.h:466: Nit: new line not needed. https://codereview.chromium.org/2029423003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): https://codereview.chromium.org/2029423003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:655: WebLocalFrame* frame = view()->focusedFrame()->toWebLocalFrame(); Should this call focusedWebLocalFrameInWidget()? https://codereview.chromium.org/2029423003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:1155: return m_localRoot->frame() == frame ? WebLocalFrameImpl::fromFrame(frame) : nullptr; This is wrong for a couple of reasons, I think you need this: LocalFrame* frame = page()->focusController().focusedFrame(); return (frame && frame->localFrameRoot() == m_localRoot->frame()) ? WebLocalFrameImpl::fromFrame(frame) : nullptr; https://codereview.chromium.org/2029423003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebFrameWidgetImpl.h (right): https://codereview.chromium.org/2029423003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebFrameWidgetImpl.h:198: WebLocalFrame* focusedWebLocalFrameInWidget() const; This method needs a comment describing what it is for. https://codereview.chromium.org/2029423003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2029423003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:2807: bool WebViewImpl::getCompositionCharacterBounds(WebVector<WebRect>& bounds) I'm a bit confused here, I thought all of this was coming out of WebViewImpl. Can we move these and all the other IME methods from WebViewImpl into WebViewFrameWidget.cpp? Conceivably we might want to duplicated it all between there and WebFrameWidgetImpl, until WebViewFrameWidget goes away and then the WebFrameWidgetImpl will be the one that handles it all.
Description was changed from ========== OOPIF IME: Renderer Side Changes Currently, all IME-related IPC are handled at the RenderViewImpl first before calling the base class methods on RenderWidget (ImeSetComposition and ImeConfirmComposition). Further, to query IME-related state, e.g., GetTextInputType(), RenderViewImpl overrides the method from RenderWidget so that it can access the state from pepper plugin. These overrides are needed to either: 1- Access focused PepperPluginInstacneImpl to set/get IME-related state. 2- Use focused frame to set/get IME-related state. This CL removes all those methods from RenderViewImpl and handles them in RenderWidget. To this end, a reference to the focused plugin is kept in RenderWidget and updated by its owning RenderFrame (to get IME-related state). Also, some of the IPC handlers for IME take a detour into RenderFrame to perform the tasks which need frame-level knowledge. This CL is a prerequisite for OOPIF IME and should conclude the implementation of IME on the renderer side. BUG=578168 ========== to ========== OOPIF IME: Renderer Side Changes Currently, all IME-related IPC are handled at the RenderViewImpl first before calling the base class methods on RenderWidget (ImeSetComposition and ImeConfirmComposition). Further, to query IME-related state, e.g., GetTextInputType(), RenderViewImpl overrides the method from RenderWidget so that it can access the state from pepper plugin. These overrides are needed to either: 1- Access focused PepperPluginInstacneImpl to set/get IME-related state. 2- Use focused frame to set/get IME-related state. This CL removes all those methods from RenderViewImpl and handles them in RenderWidget. To this end, a reference to the focused plugin is kept in RenderWidget and updated by its owning RenderFrame (to get IME-related state). Also, new API is added to blink to enable RenderWidget handle IME tasks related to focusedFrame. Finally, a new private API is added to WebFrameWidgetImpl to help the widget distinguish between in process frames which do or do not belong to widget's local root. BUG=578168,618774 ==========
Description was changed from ========== OOPIF IME: Renderer Side Changes Currently, all IME-related IPC are handled at the RenderViewImpl first before calling the base class methods on RenderWidget (ImeSetComposition and ImeConfirmComposition). Further, to query IME-related state, e.g., GetTextInputType(), RenderViewImpl overrides the method from RenderWidget so that it can access the state from pepper plugin. These overrides are needed to either: 1- Access focused PepperPluginInstacneImpl to set/get IME-related state. 2- Use focused frame to set/get IME-related state. This CL removes all those methods from RenderViewImpl and handles them in RenderWidget. To this end, a reference to the focused plugin is kept in RenderWidget and updated by its owning RenderFrame (to get IME-related state). Also, new API is added to blink to enable RenderWidget handle IME tasks related to focusedFrame. Finally, a new private API is added to WebFrameWidgetImpl to help the widget distinguish between in process frames which do or do not belong to widget's local root. BUG=578168,618774 ========== to ========== OOPIF IME: Renderer Side Changes Currently, all IME-related IPC are handled at the RenderViewImpl first before calling the base class methods on RenderWidget (ImeSetComposition and ImeConfirmComposition). Further, to query IME-related state, e.g., GetTextInputType(), RenderViewImpl overrides the method from RenderWidget so that it can access the state from pepper plugin. These overrides are needed to either: 1- Access focused PepperPluginInstacneImpl to set/get IME-related state. 2- Use focused frame to set/get IME-related state. This CL removes all those methods from RenderViewImpl and handles them in RenderWidget. To this end, a reference to the focused plugin is kept in RenderWidget and updated by its owning RenderFrame (to get IME-related state). Also, new API is added to blink to enable RenderWidget handle IME tasks related to focusedFrame. Finally, a new private API is added to WebFrameWidgetImpl to help the widget distinguish between in process frames which do or do not belong to widget's local root. BUG=578168,618774 ==========
Description was changed from ========== OOPIF IME: Renderer Side Changes Currently, all IME-related IPC are handled at the RenderViewImpl first before calling the base class methods on RenderWidget (ImeSetComposition and ImeConfirmComposition). Further, to query IME-related state, e.g., GetTextInputType(), RenderViewImpl overrides the method from RenderWidget so that it can access the state from pepper plugin. These overrides are needed to either: 1- Access focused PepperPluginInstacneImpl to set/get IME-related state. 2- Use focused frame to set/get IME-related state. This CL removes all those methods from RenderViewImpl and handles them in RenderWidget. To this end, a reference to the focused plugin is kept in RenderWidget and updated by its owning RenderFrame (to get IME-related state). Also, new API is added to blink to enable RenderWidget handle IME tasks related to focusedFrame. Finally, a new private API is added to WebFrameWidgetImpl to help the widget distinguish between in process frames which do or do not belong to widget's local root. BUG=578168,618774 ========== to ========== OOPIF IME: Renderer Side Changes Currently, all IME-related IPC are handled at the RenderViewImpl first before calling the base class methods on RenderWidget (ImeSetComposition and ImeConfirmComposition). Further, to query IME-related state, e.g., GetTextInputType(), RenderViewImpl overrides the method from RenderWidget so that it can access the state from pepper plugin. These overrides are needed to either: 1- Access focused PepperPluginInstacneImpl to set/get IME-related state. 2- Use focused frame to set/get IME-related state. This CL removes all those methods from RenderViewImpl and handles them in RenderWidget. To this end, a reference to the focused plugin is kept in RenderWidget and updated by its owning RenderFrame (to get IME-related state). Also, new API is added to blink to enable RenderWidget handle IME tasks related to focusedFrame. Finally, a new private API is added to WebFrameWidgetImpl to help the widget distinguish between in process frames which do or do not belong to widget's local root. BUG=578168,618774 ==========
PTAL. https://codereview.chromium.org/2029423003/diff/100001/content/renderer/rende... File content/renderer/render_widget.h (right): https://codereview.chromium.org/2029423003/diff/100001/content/renderer/rende... content/renderer/render_widget.h:466: On 2016/06/10 20:52:03, kenrb wrote: > Nit: new line not needed. Done. https://codereview.chromium.org/2029423003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): https://codereview.chromium.org/2029423003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:655: WebLocalFrame* frame = view()->focusedFrame()->toWebLocalFrame(); On 2016/06/10 20:52:03, kenrb wrote: > Should this call focusedWebLocalFrameInWidget()? Yes, I think it has to. Thanks for pointing this out. As I understand anything on the compositing code path must be called on one widget only. Here is the stack: WebFrameWidgetImpl::getCompositionCharacterBounds() RenderWidget::GetCompositionSelectionBounds() RenderWidget::UpdateSelectionBounds() RenderWidget::WillBeginCompositorFrame() So if we don't, we might get multiples from different widgets. Also need to address this for WebViewImpl's copy. https://codereview.chromium.org/2029423003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:1155: return m_localRoot->frame() == frame ? WebLocalFrameImpl::fromFrame(frame) : nullptr; On 2016/06/10 20:52:03, kenrb wrote: > This is wrong for a couple of reasons, I think you need this: > LocalFrame* frame = page()->focusController().focusedFrame(); > return (frame && frame->localFrameRoot() == m_localRoot->frame()) ? > WebLocalFrameImpl::fromFrame(frame) : nullptr; Yes I think I missed the frame->localFrameRoot() part and also did not think of frame being nullptr. Sorry for the error. https://codereview.chromium.org/2029423003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebFrameWidgetImpl.h (right): https://codereview.chromium.org/2029423003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebFrameWidgetImpl.h:198: WebLocalFrame* focusedWebLocalFrameInWidget() const; On 2016/06/10 20:52:03, kenrb wrote: > This method needs a comment describing what it is for. Acknowledged. https://codereview.chromium.org/2029423003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2029423003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:2807: bool WebViewImpl::getCompositionCharacterBounds(WebVector<WebRect>& bounds) On 2016/06/10 20:52:03, kenrb wrote: > I'm a bit confused here, I thought all of this was coming out of WebViewImpl. > Can we move these and all the other IME methods from WebViewImpl into > WebViewFrameWidget.cpp? Conceivably we might want to duplicated it all between > there and WebFrameWidgetImpl, until WebViewFrameWidget goes away and then the > WebFrameWidgetImpl will be the one that handles it all. What (I think) I did was to move the code out of RenderViewImpl into RenderWidget. So these methods are called on |webwidget_| rather than |frame_widget_|. I think it should stay in WebFrameWidgetImpl and WebViewImpl. I think it should be removed from here if RenderView is not a RenderWidget. https://codereview.chromium.org/2029423003/diff/100001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebWidget.h (right): https://codereview.chromium.org/2029423003/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/web/WebWidget.h:252: virtual void adjustReplacementRangeForAccentedCharacters(int start, int length) {} I renamed this to make it shorter. I think we might as well consider not adding this new method but instead add |replacement_range| (gfx::Range or something similar) as an extra argument to WebWidget::set/confirmComposition.
lgtm, although we need to figure out if you should land this before or after https://codereview.chromium.org/2012823003/. https://codereview.chromium.org/2029423003/diff/100001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebWidget.h (right): https://codereview.chromium.org/2029423003/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/web/WebWidget.h:252: virtual void adjustReplacementRangeForAccentedCharacters(int start, int length) {} On 2016/06/27 21:14:50, EhsanK wrote: > I renamed this to make it shorter. > > I think we might as well consider not adding this new method but instead > add |replacement_range| (gfx::Range or something similar) as an extra argument > to WebWidget::set/confirmComposition. That sounds fine. https://codereview.chromium.org/2029423003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebFrameWidgetImpl.h (right): https://codereview.chromium.org/2029423003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebFrameWidgetImpl.h:200: // to this widget. Also specify it returns nullptr if there is no focused LocalFrame, or the focused LocalFrame has a different local root. https://codereview.chromium.org/2029423003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2029423003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:2819: if (frame->localRoot() != mainFrameImpl()) A brief comment explaining this check would be useful.
Thanks for the reviews! PTAL. https://codereview.chromium.org/2029423003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebFrameWidgetImpl.h (right): https://codereview.chromium.org/2029423003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebFrameWidgetImpl.h:200: // to this widget. On 2016/06/28 20:55:56, kenrb wrote: > Also specify it returns nullptr if there is no focused LocalFrame, or the > focused LocalFrame has a different local root. Acknowledged. https://codereview.chromium.org/2029423003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2029423003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:2819: if (frame->localRoot() != mainFrameImpl()) On 2016/06/28 20:55:57, kenrb wrote: > A brief comment explaining this check would be useful. Acknowledged.
Sorry for the delay, added some comments. https://codereview.chromium.org/2029423003/diff/180001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2029423003/diff/180001/content/renderer/rende... content/renderer/render_view_impl.cc:3119: PepperPluginInstanceImpl* RenderViewImpl::GetFocusedPepperPlugin() { Is this still needed? https://codereview.chromium.org/2029423003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): https://codereview.chromium.org/2029423003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:474: return view()->setComposition(text, underlines, selectionStart, selectionEnd); Can you implement those functions here, instead of delegating to the WebViewImpl? https://codereview.chromium.org/2029423003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:675: if (!frame) Why |frame| can be null? Do we call this function when this widget doesn't have the focus? https://codereview.chromium.org/2029423003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2029423003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:2842: if (frame->localRoot() != mainFrameImpl()) Why is this needed? https://codereview.chromium.org/2029423003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:2845: if (!frame) This would've crashed in the if statement before. https://codereview.chromium.org/2029423003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.h (right): https://codereview.chromium.org/2029423003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.h:160: bool getCompositionCharacterBounds(WebVector<WebRect>& bounds) override; Can you move those functions to the private section, and make sure that they are called from WebFrameWidget? (for the main frame, it should go through WebViewFrameWidget).
Description was changed from ========== OOPIF IME: Renderer Side Changes Currently, all IME-related IPC are handled at the RenderViewImpl first before calling the base class methods on RenderWidget (ImeSetComposition and ImeConfirmComposition). Further, to query IME-related state, e.g., GetTextInputType(), RenderViewImpl overrides the method from RenderWidget so that it can access the state from pepper plugin. These overrides are needed to either: 1- Access focused PepperPluginInstacneImpl to set/get IME-related state. 2- Use focused frame to set/get IME-related state. This CL removes all those methods from RenderViewImpl and handles them in RenderWidget. To this end, a reference to the focused plugin is kept in RenderWidget and updated by its owning RenderFrame (to get IME-related state). Also, new API is added to blink to enable RenderWidget handle IME tasks related to focusedFrame. Finally, a new private API is added to WebFrameWidgetImpl to help the widget distinguish between in process frames which do or do not belong to widget's local root. BUG=578168,618774 ========== to ========== OOPIF IME: Renderer Side Changes Currently, all IME-related IPC are handled at the RenderViewImpl first before calling the base class methods on RenderWidget (ImeSetComposition and ImeConfirmComposition). Further, to query IME-related state, e.g., GetTextInputType(), RenderViewImpl overrides the method from RenderWidget so that it can access the state from pepper plugin. These overrides are needed to either: 1- Access focused PepperPluginInstacneImpl to set/get IME-related state. 2- Use focused frame to set/get IME-related state. This CL removes all those methods from RenderViewImpl and handles them in RenderWidget. To this end, a reference to the focused plugin is kept in RenderWidget and updated by its owning RenderFrame (to get IME-related state). Also, new API is added to blink to enable RenderWidget handle IME tasks related to focusedFrame. Towards separating WebViewImpl from WebWidget as well as avoiding potential race conditions between mutiple Widgets on the same process, most of the IME-related WebWidget methods are now specifically implemented for WebFrameWidgetImpl. Finally, a new private API is added to WebFrameWidgetImpl to help the widget distinguish between in process frames which do or do not belong to widget's local root. BUG=578168,618774 ==========
Description was changed from ========== OOPIF IME: Renderer Side Changes Currently, all IME-related IPC are handled at the RenderViewImpl first before calling the base class methods on RenderWidget (ImeSetComposition and ImeConfirmComposition). Further, to query IME-related state, e.g., GetTextInputType(), RenderViewImpl overrides the method from RenderWidget so that it can access the state from pepper plugin. These overrides are needed to either: 1- Access focused PepperPluginInstacneImpl to set/get IME-related state. 2- Use focused frame to set/get IME-related state. This CL removes all those methods from RenderViewImpl and handles them in RenderWidget. To this end, a reference to the focused plugin is kept in RenderWidget and updated by its owning RenderFrame (to get IME-related state). Also, new API is added to blink to enable RenderWidget handle IME tasks related to focusedFrame. Towards separating WebViewImpl from WebWidget as well as avoiding potential race conditions between mutiple Widgets on the same process, most of the IME-related WebWidget methods are now specifically implemented for WebFrameWidgetImpl. Finally, a new private API is added to WebFrameWidgetImpl to help the widget distinguish between in process frames which do or do not belong to widget's local root. BUG=578168,618774 ========== to ========== OOPIF IME: Renderer Side Changes Currently, all IME-related IPC are handled at the RenderViewImpl first before calling the base class methods on RenderWidget (ImeSetComposition and ImeConfirmComposition). Further, to query IME-related state, e.g., GetTextInputType(), RenderViewImpl overrides the method from RenderWidget so that it can access the state from pepper plugin. These overrides are needed to either: 1- Access focused PepperPluginInstacneImpl to set/get IME-related state. 2- Use focused frame to set/get IME-related state. This CL removes all those methods from RenderViewImpl and handles them in RenderWidget. To this end, a reference to the focused plugin is kept in RenderWidget and updated by its owning RenderFrame (to get IME-related state). Also, new API is added to blink to enable RenderWidget handle IME tasks related to focusedFrame. In line with the separation of WebViewImpl from WebWidget, as well as avoiding potential race conditions between mutiple Widgets on the same process, most of the IME- related WebWidget methods are now specifically implemented for WebFrameWidgetImpl. Finally, a new private API is added to WebFrameWidgetImpl to help the widget distinguish between in process frames which do or do not belong to widget's local root. BUG=578168,618774 ==========
Patchset #11 (id:200001) has been deleted
Patchset #11 (id:220001) has been deleted
Unfortunately I have ended up rebasing and making changes to address comments in the same patch. Apologies. Please take another look. https://codereview.chromium.org/2029423003/diff/180001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2029423003/diff/180001/content/renderer/rende... content/renderer/render_view_impl.cc:3119: PepperPluginInstanceImpl* RenderViewImpl::GetFocusedPepperPlugin() { On 2016/07/05 18:30:46, lfg wrote: > Is this still needed? I think it will be really nice to not have this. But apparently there is external dependency on this from TextInputClientObserver: https://cs.chromium.org/chromium/src/content/renderer/text_input_client_obser... The TextInputClientObserver observers the RenderView but also keeps a reference to it ONLY to get the pepper plugin. I am leaving a TODO here abd followup with that class soon (since it is IME related) to see if I need to change the signature to take RenderWidget instead/as well. If so, I will then safely comeback and remove this method. https://codereview.chromium.org/2029423003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): https://codereview.chromium.org/2029423003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:474: return view()->setComposition(text, underlines, selectionStart, selectionEnd); On 2016/07/05 18:30:46, lfg wrote: > Can you implement those functions here, instead of delegating to the > WebViewImpl? I initially wanted to keep this CL short so I only implemented those whose WebFrameWidget implementations were crucial. But I guess it might be better to have them all implemented here. It is mostly copying code with minor modifications. https://codereview.chromium.org/2029423003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:675: if (!frame) On 2016/07/05 18:30:46, lfg wrote: > Why |frame| can be null? Do we call this function when this widget doesn't have > the focus? Yes as the route of this call could be an update on the compositing path. Sample stack: WebWidget::getCompositionCharacterBounds() RenderWidget::GetCompositionCharacterBounds() RenderWidget::UpdateCompositionInfo() RenderWidget::UpdateSelectionBounds() RenderWidget::WillBeginCompositorFrame(). Also, here we look for the focused frame in widget, so there could be a focused local frame in the process, but not belonging to the local root we are interested in here. https://codereview.chromium.org/2029423003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2029423003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:2842: if (frame->localRoot() != mainFrameImpl()) On 2016/07/05 18:30:46, lfg wrote: > Why is this needed? I need to make sure that only WebFrameWidgetImpl sends out this information. If we don't check for this, the information maybe updated by both the WebViewImpl and WebFrameWidgetImpl with WebViewImpl sending the incorrect values. https://codereview.chromium.org/2029423003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:2845: if (!frame) On 2016/07/05 18:30:46, lfg wrote: > This would've crashed in the if statement before. Thanks! Acknowledged. https://codereview.chromium.org/2029423003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.h (right): https://codereview.chromium.org/2029423003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.h:160: bool getCompositionCharacterBounds(WebVector<WebRect>& bounds) override; On 2016/07/05 18:30:46, lfg wrote: > Can you move those functions to the private section, and make sure that they are > called from WebFrameWidget? (for the main frame, it should go through > WebViewFrameWidget). Given our offline discussion this needs a change here: https://cs.chromium.org/chromium/src/content/renderer/render_widget.cc?rcl=14... to create a WebViewFrameWidget instead of a WebVewImpl. So leave it for a later time I guess.
https://codereview.chromium.org/2029423003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): https://codereview.chromium.org/2029423003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:675: if (!frame) On 2016/07/07 14:40:47, EhsanK wrote: > On 2016/07/05 18:30:46, lfg wrote: > > Why |frame| can be null? Do we call this function when this widget doesn't > have > > the focus? > > Yes as the route of this call could be an update on the compositing path. Sample > stack: > WebWidget::getCompositionCharacterBounds() > RenderWidget::GetCompositionCharacterBounds() > RenderWidget::UpdateCompositionInfo() > RenderWidget::UpdateSelectionBounds() > RenderWidget::WillBeginCompositorFrame(). > > Also, here we look for the focused frame in widget, so there could be a focused > local frame in the process, but not belonging to the local root we are > interested in here. So, in this example, can't you just early return in RenderWidget::UpdateSelectionBounds if there's no focus? Why do we need to descend all the way in the stack? You would then be able to get rid of all these checks if the update is happening in the correct widget, and just assert that they are. https://codereview.chromium.org/2029423003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2029423003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:2842: if (frame->localRoot() != mainFrameImpl()) On 2016/07/07 14:40:47, EhsanK wrote: > On 2016/07/05 18:30:46, lfg wrote: > > Why is this needed? > > I need to make sure that only WebFrameWidgetImpl sends out this information. If > we don't check for this, the information maybe updated by both the WebViewImpl > and WebFrameWidgetImpl with WebViewImpl sending the incorrect values. See my comment in WebFrameWidgetImpl.cpp. https://codereview.chromium.org/2029423003/diff/240001/content/renderer/rende... File content/renderer/render_widget.h (right): https://codereview.chromium.org/2029423003/diff/240001/content/renderer/rende... content/renderer/render_widget.h:779: PepperPluginInstanceImpl* focused_pepper_plugin_; I'm not convinced this belongs in widget. This was a question that avi@ raised in https://codereview.chromium.org/1962923002/ , and we agreed that it was better to track plugins only in RenderFrame. https://codereview.chromium.org/2029423003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): https://codereview.chromium.org/2029423003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:113: , m_imeAcceptEvents(true) Should this be initialized to false? https://codereview.chromium.org/2029423003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:449: m_imeAcceptEvents = enable; Is it possible for the widget to have focus, but m_imeAcceptEvents be false?
https://codereview.chromium.org/2029423003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): https://codereview.chromium.org/2029423003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:474: return view()->setComposition(text, underlines, selectionStart, selectionEnd); On 2016/07/07 14:40:47, EhsanK wrote: > On 2016/07/05 18:30:46, lfg wrote: > > Can you implement those functions here, instead of delegating to the > > WebViewImpl? > > I initially wanted to keep this CL short so I only implemented those whose > WebFrameWidget implementations were crucial. But I guess it might be better to > have them all implemented here. It is mostly copying code with minor > modifications. Is it really better this way? One advantage of delegating to WebViewImpl is that there is only one copy of the method implementations. Copying and pasting becomes a problem if people land changes to one copy and not the other. Eventually all the code will live exclusively here, but that change isn't imminent.
PTAL. https://codereview.chromium.org/2029423003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): https://codereview.chromium.org/2029423003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:474: return view()->setComposition(text, underlines, selectionStart, selectionEnd); On 2016/07/07 15:30:38, kenrb wrote: > On 2016/07/07 14:40:47, EhsanK wrote: > > On 2016/07/05 18:30:46, lfg wrote: > > > Can you implement those functions here, instead of delegating to the > > > WebViewImpl? > > > > I initially wanted to keep this CL short so I only implemented those whose > > WebFrameWidget implementations were crucial. But I guess it might be better to > > have them all implemented here. It is mostly copying code with minor > > modifications. > > Is it really better this way? One advantage of delegating to WebViewImpl is that > there is only one copy of the method implementations. Copying and pasting > becomes a problem if people land changes to one copy and not the other. > Eventually all the code will live exclusively here, but that change isn't > imminent. Some of the changes made can be argued to be not related to this CL, in the sense that I did not need them to make IME work. Except for the verification of focused frame being part of the WebWidget, the rest of the code is identical between WebViewImpl and WebFrameWidgetImpl. It is crucial to vrify that frame is part of WebWidget for the following methods: textInputType() compositionRange() getCompositionCharacterBounds(). We do not want to update the TextInputState for one WebWidget by others given that the browser is tracking the state based on RenderWidgetHostViews. Also, for other methods such as set/confirmComposition, which are in response to IPC from browser (so we are at the correct WebWidget) you could argue that the focus might have changed by the time we receive the IPC so we might as well implement those in WebFrameWidgetImpl as well. https://codereview.chromium.org/2029423003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:675: if (!frame) On 2016/07/07 14:40:47, EhsanK wrote: > On 2016/07/05 18:30:46, lfg wrote: > > Why |frame| can be null? Do we call this function when this widget doesn't > have > > the focus? > > Yes as the route of this call could be an update on the compositing path. Sample > stack: > WebWidget::getCompositionCharacterBounds() > RenderWidget::GetCompositionCharacterBounds() > RenderWidget::UpdateCompositionInfo() > RenderWidget::UpdateSelectionBounds() > RenderWidget::WillBeginCompositorFrame(). > > Also, here we look for the focused frame in widget, so there could be a focused > local frame in the process, but not belonging to the local root we are > interested in here. We still need to send the IPC so it comes down to checking for focused frame in this method or before that in RenderWidget. As for "why we still need to send the IPC" I think one reason is Android: https://cs.chromium.org/chromium/src/content/renderer/render_widget.cc?rcl=0&... which comes from the ImeAdapterAndroid. I cannot confirm not sending the IPC in response to the request might cause issues in android. I suggest we postpone removing this code to when we work on IME for android. I will try to dig more meanwhile. https://codereview.chromium.org/2029423003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2029423003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:2842: if (frame->localRoot() != mainFrameImpl()) On 2016/07/07 15:13:31, lfg wrote: > On 2016/07/07 14:40:47, EhsanK wrote: > > On 2016/07/05 18:30:46, lfg wrote: > > > Why is this needed? > > > > I need to make sure that only WebFrameWidgetImpl sends out this information. > If > > we don't check for this, the information maybe updated by both the WebViewImpl > > and WebFrameWidgetImpl with WebViewImpl sending the incorrect values. > > See my comment in WebFrameWidgetImpl.cpp. Acknowledged. https://codereview.chromium.org/2029423003/diff/240001/content/renderer/rende... File content/renderer/render_widget.h (right): https://codereview.chromium.org/2029423003/diff/240001/content/renderer/rende... content/renderer/render_widget.h:779: PepperPluginInstanceImpl* focused_pepper_plugin_; On 2016/07/07 15:13:31, lfg wrote: > I'm not convinced this belongs in widget. This was a question that avi@ raised > in https://codereview.chromium.org/1962923002/ , and we agreed that it was > better to track plugins only in RenderFrame. Technically, RW does not own this. We need this reference to 1- Forward IME IPC calls 2- Get IME related state which is collected for every compositor frame. (1) is doable by making observer calls (though a bit ugly). For (2) we either need this, or a reference to the render frame that owns the RW. I am not quite sure if that is a better idea. https://codereview.chromium.org/2029423003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): https://codereview.chromium.org/2029423003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:113: , m_imeAcceptEvents(true) On 2016/07/07 15:13:31, lfg wrote: > Should this be initialized to false? WebViewImpl initializes this to true. But I guess more importantly, the question is could we have a focused frame in a Widget but not a page level focus (i.e., m_imeAcceptEvents == false)? https://codereview.chromium.org/2029423003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:449: m_imeAcceptEvents = enable; On 2016/07/07 15:13:31, lfg wrote: > Is it possible for the widget to have focus, but m_imeAcceptEvents be false? m_imeAcceptEvents is apparent tracking page level focus. I am not sure, but if a frame is focused on a page, then the page itself must be focused, right? If that is the case, then I think we should just remove this variable from both WebViewImpl and WebFrameWidgetImpl.
https://codereview.chromium.org/2029423003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): https://codereview.chromium.org/2029423003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:675: if (!frame) On 2016/07/07 20:25:07, EhsanK wrote: > On 2016/07/07 14:40:47, EhsanK wrote: > > On 2016/07/05 18:30:46, lfg wrote: > > > Why |frame| can be null? Do we call this function when this widget doesn't > > have > > > the focus? > > > > Yes as the route of this call could be an update on the compositing path. > Sample > > stack: > > WebWidget::getCompositionCharacterBounds() > > RenderWidget::GetCompositionCharacterBounds() > > RenderWidget::UpdateCompositionInfo() > > RenderWidget::UpdateSelectionBounds() > > RenderWidget::WillBeginCompositorFrame(). > > > > Also, here we look for the focused frame in widget, so there could be a > focused > > local frame in the process, but not belonging to the local root we are > > interested in here. > > We still need to send the IPC so it comes down to checking for focused frame in > this method or before that in RenderWidget. > > As for "why we still need to send the IPC" I think one reason is Android: > https://cs.chromium.org/chromium/src/content/renderer/render_widget.cc?rcl=0&... > which comes from the ImeAdapterAndroid. I cannot confirm not sending the IPC in > response to the request might cause issues in android. I suggest we postpone > removing this code to when we work on IME for android. I will try to dig more > meanwhile. Acknowledged. https://codereview.chromium.org/2029423003/diff/240001/content/renderer/rende... File content/renderer/render_widget.h (right): https://codereview.chromium.org/2029423003/diff/240001/content/renderer/rende... content/renderer/render_widget.h:779: PepperPluginInstanceImpl* focused_pepper_plugin_; On 2016/07/07 20:25:07, EhsanK wrote: > On 2016/07/07 15:13:31, lfg wrote: > > I'm not convinced this belongs in widget. This was a question that avi@ raised > > in https://codereview.chromium.org/1962923002/ , and we agreed that it was > > better to track plugins only in RenderFrame. > > Technically, RW does not own this. We need this reference to > 1- Forward IME IPC calls > 2- Get IME related state which is collected for every compositor frame. > > (1) is doable by making observer calls (though a bit ugly). For (2) we either > need this, or a reference to the render frame that owns the RW. I am not quite > sure if that is a better idea. We could extract the local root from the WebFrameWidget, but I'm not sure if it would be better, since all we have is a pointer to a WebWidget. I can't really think of an easy way to handle this better (one that wouldn't involve a lot of refactors), so unless someone else has a better suggestion, this is fine by me. https://codereview.chromium.org/2029423003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): https://codereview.chromium.org/2029423003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:113: , m_imeAcceptEvents(true) On 2016/07/07 20:25:08, EhsanK wrote: > On 2016/07/07 15:13:31, lfg wrote: > > Should this be initialized to false? > > WebViewImpl initializes this to true. But I guess more importantly, the question > is could we have a focused frame in a Widget but not a page level focus (i.e., > m_imeAcceptEvents == false)? Yes, indeed. https://codereview.chromium.org/2029423003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:449: m_imeAcceptEvents = enable; On 2016/07/07 20:25:08, EhsanK wrote: > On 2016/07/07 15:13:31, lfg wrote: > > Is it possible for the widget to have focus, but m_imeAcceptEvents be false? > > m_imeAcceptEvents is apparent tracking page level focus. I am not sure, but if a > frame is focused on a page, then the page itself must be focused, right? If that > is the case, then I think we should just remove this variable from both > WebViewImpl and WebFrameWidgetImpl. That's right, can you dig a bit more to see if we can get rid of this entirely? That would be the best scenario.
Description was changed from ========== OOPIF IME: Renderer Side Changes Currently, all IME-related IPC are handled at the RenderViewImpl first before calling the base class methods on RenderWidget (ImeSetComposition and ImeConfirmComposition). Further, to query IME-related state, e.g., GetTextInputType(), RenderViewImpl overrides the method from RenderWidget so that it can access the state from pepper plugin. These overrides are needed to either: 1- Access focused PepperPluginInstacneImpl to set/get IME-related state. 2- Use focused frame to set/get IME-related state. This CL removes all those methods from RenderViewImpl and handles them in RenderWidget. To this end, a reference to the focused plugin is kept in RenderWidget and updated by its owning RenderFrame (to get IME-related state). Also, new API is added to blink to enable RenderWidget handle IME tasks related to focusedFrame. In line with the separation of WebViewImpl from WebWidget, as well as avoiding potential race conditions between mutiple Widgets on the same process, most of the IME- related WebWidget methods are now specifically implemented for WebFrameWidgetImpl. Finally, a new private API is added to WebFrameWidgetImpl to help the widget distinguish between in process frames which do or do not belong to widget's local root. BUG=578168,618774 ========== to ========== OOPIF IME: Renderer Side Changes Currently, all IME-related IPC are handled at the RenderViewImpl first before calling the base class methods on RenderWidget (ImeSetComposition and ImeConfirmComposition). Further, to query IME-related state, e.g., GetTextInputType(), RenderViewImpl overrides the method from RenderWidget so that it can access the state from pepper plugin. These overrides are needed to either: 1- Access focused PepperPluginInstacneImpl to set/get IME-related state. 2- Use focused frame to set/get IME-related state. This CL removes all those methods from RenderViewImpl and handles them in RenderWidget. To this end, a reference to the focused plugin is kept in RenderWidget and updated by its owning RenderFrame (to get IME-related state). Also, new API is added to blink to enable RenderWidget handle IME tasks related to focusedFrame. In line with the separation of WebViewImpl from WebWidget, as well as avoiding potential race conditions between mutiple Widgets on the same process, most of the IME- related WebWidget methods are now specifically implemented for WebFrameWidgetImpl. Finally, a new private API is added to WebFrameWidgetImpl to help the widget distinguish between in process frames which do or do not belong to widget's local root. BUG=578168,618774,626746 ==========
Thank you Lucas for the reviews! Please take another look. https://codereview.chromium.org/2029423003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): https://codereview.chromium.org/2029423003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:449: m_imeAcceptEvents = enable; On 2016/07/07 20:58:27, lfg wrote: > On 2016/07/07 20:25:08, EhsanK wrote: > > On 2016/07/07 15:13:31, lfg wrote: > > > Is it possible for the widget to have focus, but m_imeAcceptEvents be false? > > > > m_imeAcceptEvents is apparent tracking page level focus. I am not sure, but if > a > > frame is focused on a page, then the page itself must be focused, right? If > that > > is the case, then I think we should just remove this variable from both > > WebViewImpl and WebFrameWidgetImpl. > > That's right, can you dig a bit more to see if we can get rid of this entirely? > That would be the best scenario. I have explained what might happen in WebViewImpl::setFocus. https://codereview.chromium.org/2029423003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2029423003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:2307: focusedFrame->inputMethodController().confirmComposition(); Let's say we have an ongoing composition and then suddenly change tabs. If the IPC for page focus arrives sooner we confirm the composition here. Now if the IPC from IME to confirm arrives later, and we allow it to be processed (by not checking the flag m_imeAcceptEvents), the same text is confirmed again and things will appear twice. I artificially tested this by post tasking IME methods after some delay.
Thanks for fixing the WebFrameWidgetImpl implementation of confirmComposition. https://codereview.chromium.org/2029423003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2029423003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:2307: focusedFrame->inputMethodController().confirmComposition(); On 2016/07/11 20:29:53, EhsanK wrote: > Let's say we have an ongoing composition and then suddenly change tabs. If the > IPC for page focus arrives sooner we confirm the composition here. Now if the > IPC from IME to confirm arrives later, and we allow it to be processed (by not > checking the flag m_imeAcceptEvents), the same text is confirmed again and > things will appear twice. > > I artificially tested this by post tasking IME methods after some delay. Acknowledged. https://codereview.chromium.org/2029423003/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2029423003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:2323: // When mainFrameImpl() is nullptr/remote, IME is handled at the Widget What happens when mainFrameImpl() is not remote, but the widget that should receive the composition is a WebFrameWidgetImpl? This happens in a A-embeds B-embeds A case.
Thank you Lucas for reviews and thanks for catching that error. Please take another look. https://codereview.chromium.org/2029423003/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2029423003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:2323: // When mainFrameImpl() is nullptr/remote, IME is handled at the Widget On 2016/07/11 20:55:51, lfg wrote: > What happens when mainFrameImpl() is not remote, but the widget that should > receive the composition is a WebFrameWidgetImpl? This happens in a A-embeds > B-embeds A case. As we discussed offline, I think the change in line 2334 should catch those scenarios.
ekaramad@chromium.org changed reviewers: + creis@chromium.org
Adding creis@ as the content/ owner reviewer.
Thanks. lgtm
Rubber stamp LGTM for content/, as I'm mostly deferring to Ken and Lucas. https://codereview.chromium.org/2029423003/diff/320001/content/renderer/rende... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/2029423003/diff/320001/content/renderer/rende... content/renderer/render_view_impl.h:245: // obtain the plugin. Come back to this later when implementing IME for MAC nit: s/MAC/Mac/
As a general question, can you help me understand the overall direction that we're taking this code? I looked at the Blink changes, and it seems like there's a lot of nearly identical code that needs to be duplicated between WebViewImpl and WebFrameWidgetImpl. Do you see any way to extract some of the common logic out into a helper class that could be shared? It's not necessary to do that in this CL, but we're starting to duplicate increasingly large amounts of code. The eventual Widget / View split will help a lot with this... but unfortunately, we're not there quite yet. https://codereview.chromium.org/2029423003/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): https://codereview.chromium.org/2029423003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:498: if (!m_imeAcceptEvents) I assume this is largely based on the implementation in WebViewImpl? https://codereview.chromium.org/2029423003/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2029423003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:2325: if (!mainFrameImpl()) I don't understand why we need these early returns. Is there documentation that describes the potential races with IME and how they're handled? It also seems like these checks are duplicated a lot, so we should be able to put them in a common helper somewhere. https://codereview.chromium.org/2029423003/diff/320001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebWidget.h (right): https://codereview.chromium.org/2029423003/diff/320001/third_party/WebKit/pub... third_party/WebKit/public/web/WebWidget.h:252: virtual void applyReplacementRange(int start, int length) {} Does it make sense to move these to WebFrameWidget instead? I took a look at the WebWidget implementations. It doesn't seem like anything other than frames is going to bother implementing this interface anyway. (Ideally, I would also like to hide the pre-existing methods on WebView and force callers to go through WebViewFrameWidget, but that is somewhat outside the scope of this change)
Thanks for the reviews. PTAL. Daniel: The direction I had in mind was to move everything to RenderWidget. So all the code in WebViewImpl, plus some extra methods I had to add to take care of some frame level changes, has live in both WebViewImpl and WebFrameWidgetImpl now since we delegate all those calls to RenderWidget::webwidget_. I think it is a very good idea to move the IME code into another class and have RenderWidget or WebWidget own an instance of it. This will remove roughly half of the code related to IME. I think it will also definitely make the transition easier. If this is the direction we will be going I will put some TODOs and work on the implementation after IME lands. I think it will come down to having a nice and concise API for the helper class which might need some careful thinking about the general shape of the API right now. But other than that, it sounds like a very likely and likable idea. https://codereview.chromium.org/2029423003/diff/320001/content/renderer/rende... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/2029423003/diff/320001/content/renderer/rende... content/renderer/render_view_impl.h:245: // obtain the plugin. Come back to this later when implementing IME for MAC On 2016/07/12 00:00:12, Charlie Reis wrote: > nit: s/MAC/Mac/ Done. https://codereview.chromium.org/2029423003/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): https://codereview.chromium.org/2029423003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:498: if (!m_imeAcceptEvents) On 2016/07/12 14:43:21, dcheng wrote: > I assume this is largely based on the implementation in WebViewImpl? Yes. As tested on WebViewImpl itself, we need this to make sure we do not "confirmComposition()" twice on a tab switch while doing IME. I did not test "setComposition()" but I believe there should be a similar issue. https://codereview.chromium.org/2029423003/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2029423003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:2325: if (!mainFrameImpl()) On 2016/07/12 14:43:21, dcheng wrote: > I don't understand why we need these early returns. Is there documentation that > describes the potential races with IME and how they're handled? > > It also seems like these checks are duplicated a lot, so we should be able to > put them in a common helper somewhere. This check is to make sure we only do IME inside a WebWidget. For the main frame, it happens to be the WebViewImpl which is not swapped out (or whose RenderWidget is not swapped out). In WebFrameWidgetImpl, I added the method 'focusedLocalFrameInWidget()' which guarantees we are dealing with the intended frame in the intended widget. Here I did not add the method so I check every time (line 2334). The reason that I have been doing this alot is that there is actual problems with reporting the IME state from multiple Widgets on page which makes tracking the state on the browser side impossible. I guess having a helper class would go along way. Before, inside WebFrameWidgetImpl, we were delegating all these calls to wiewImpl(). I can see how both WebViewImpl() and WebWidget() could call a third helper class for this. I can file a bug for it to follow up on if we will be OK with having the duplicated code temporarily. https://codereview.chromium.org/2029423003/diff/320001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebWidget.h (right): https://codereview.chromium.org/2029423003/diff/320001/third_party/WebKit/pub... third_party/WebKit/public/web/WebWidget.h:252: virtual void applyReplacementRange(int start, int length) {} On 2016/07/12 14:43:21, dcheng wrote: > Does it make sense to move these to WebFrameWidget instead? I took a look at the > WebWidget implementations. It doesn't seem like anything other than frames is > going to bother implementing this interface anyway. > > (Ideally, I would also like to hide the pre-existing methods on WebView and > force callers to go through WebViewFrameWidget, but that is somewhat outside the > scope of this change) Sorry if the question is simplistic: I am making these calls from inside RenderWidget which only knows a WebWidget, right? Can we know about WebFrameWidget from inside content/renderer? Also, another approach could be to remove applyReplacement() and add a range argument to both WebWidget::setComposition and WebWidget::confirmComposition. All this being said, if we go for helper class, say TextInputHelper obtained from WebWidget::getTextInputHelper() and then add all these methods to that, would it be a better solution?
On 2016/07/12 14:43:22, dcheng wrote: > As a general question, can you help me understand the overall direction that > we're taking this code? I looked at the Blink changes, and it seems like there's > a lot of nearly identical code that needs to be duplicated between WebViewImpl > and WebFrameWidgetImpl. > > Do you see any way to extract some of the common logic out into a helper class > that could be shared? It's not necessary to do that in this CL, but we're > starting to duplicate increasingly large amounts of code. The eventual Widget / > View split will help a lot with this... but unfortunately, we're not there quite > yet. > I share that concern, as I mentioned earlier. All that code duplication is being done just for the sake of the call to focusedLocalFrameInWidget(). There is a lot of potential for bugs to appear from divergence of the two near-identical implementations of those WebWidget methods, and there should be a better way to do this.
On 2016/07/12 15:14:11, kenrb wrote: > On 2016/07/12 14:43:22, dcheng wrote: > > As a general question, can you help me understand the overall direction that > > we're taking this code? I looked at the Blink changes, and it seems like > there's > > a lot of nearly identical code that needs to be duplicated between WebViewImpl > > and WebFrameWidgetImpl. > > > > Do you see any way to extract some of the common logic out into a helper class > > that could be shared? It's not necessary to do that in this CL, but we're > > starting to duplicate increasingly large amounts of code. The eventual Widget > / > > View split will help a lot with this... but unfortunately, we're not there > quite > > yet. > > > > I share that concern, as I mentioned earlier. All that code duplication is being > done just for the sake of the call to focusedLocalFrameInWidget(). There is a > lot of potential for bugs to appear from divergence of the two near-identical > implementations of those WebWidget methods, and there should be a better way to > do this. FWIW, here's what I would personally do: - Create WebFrameWidgetBase in Source/web - Give WebFrameWidgetBase a focusedFrame() method. - focusedFrame() only returns a frame if the focused frame is in the local root (I think we've had several other instances where we needed to care about things being in the same local root? IIRC, something about the compositor?) - Make WebFrameWidgetImpl and WebViewFrameWidget both inherit from WebFrameWidgetBase and override focusedFrame() do the right thing for each one (only returns a non-null pointer if the focused frame has the same local root). - Move the IME code itself into WebFrameWidgetBase (so this includes state like m_imeAcceptEvents and m_suppressNextKeypressEvent). Change //content to always to talk to IME through the frame widget. While I would prefer to see the CL landed with something like this implemented, I suspect it's probably not quite that simple, so rs lgtm with the code duplication comment in WebViewImpl.cpp (and WebFrameWidgetImpl.cpp) about the focused frame checks addressed. https://codereview.chromium.org/2029423003/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2029423003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:2325: if (!mainFrameImpl()) On 2016/07/12 15:13:24, EhsanK wrote: > On 2016/07/12 14:43:21, dcheng wrote: > > I don't understand why we need these early returns. Is there documentation > that > > describes the potential races with IME and how they're handled? > > > > It also seems like these checks are duplicated a lot, so we should be able to > > put them in a common helper somewhere. > > This check is to make sure we only do IME inside a WebWidget. For the main > frame, it happens to be the WebViewImpl which is not swapped out (or whose > RenderWidget is not swapped out). > > In WebFrameWidgetImpl, I added the method 'focusedLocalFrameInWidget()' which > guarantees we are dealing with the intended frame in the intended widget. > > Here I did not add the method so I check every time (line 2334). > > The reason that I have been doing this alot is that there is actual problems > with reporting the IME state from multiple Widgets on page which makes tracking > the state on the browser side impossible. > > I guess having a helper class would go along way. Before, inside > WebFrameWidgetImpl, we were delegating all these calls to wiewImpl(). I can see > how both WebViewImpl() and WebWidget() could call a third helper class for this. > > I can file a bug for it to follow up on if we will be OK with having the > duplicated code temporarily. As a starting point, I think we can have separate helpers for WebFrameWidgetImpl and WebViewImpl? That should still reduce a bunch of code duplication. https://codereview.chromium.org/2029423003/diff/320001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebWidget.h (right): https://codereview.chromium.org/2029423003/diff/320001/third_party/WebKit/pub... third_party/WebKit/public/web/WebWidget.h:252: virtual void applyReplacementRange(int start, int length) {} On 2016/07/12 15:13:24, EhsanK wrote: > On 2016/07/12 14:43:21, dcheng wrote: > > Does it make sense to move these to WebFrameWidget instead? I took a look at > the > > WebWidget implementations. It doesn't seem like anything other than frames is > > going to bother implementing this interface anyway. > > > > (Ideally, I would also like to hide the pre-existing methods on WebView and > > force callers to go through WebViewFrameWidget, but that is somewhat outside > the > > scope of this change) > > Sorry if the question is simplistic: I am making these calls from inside > RenderWidget which only knows a WebWidget, right? Can we know about > WebFrameWidget from inside content/renderer? WebLocalFrame has a frameWidget method (which should probably return a WebFrameWidget, not a WebWidget). Maybe RenderWidget itself should know when it has a frame widget associated with it though... either way, this is fine for a followup. > > Also, another approach could be to remove applyReplacement() and add a range > argument to both WebWidget::setComposition and WebWidget::confirmComposition. I don't really know enough about IME to say what the right design here is. I guess this is fine (since my comment applies to all the methods added here) > > All this being said, if we go for helper class, say TextInputHelper obtained > from WebWidget::getTextInputHelper() and then add all these methods to that, > would it be a better solution? I think making this WebFrameWidget specific would be better for now.
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Daniel, PTAL. Thanks! Some notes: I used the focusedLocalFrameInWidget() (now added to WebViewImpl as well) in a couple of more places for IME. At first, I was using m_page->focusController().focusedFrame() for those methods (knowingly) against the fact that in all such methods, we used to have focusedCoreFrame() instead (talking about WebViewImpl). This lead to a failed test: WebViewTest.SelectionOnReadOnlyInput. The reason was that mainFrame() == focusedCoreFrame() != m_page->focusController().focusedFrame(). So I changed focusedLocalFrameInWidget() to use focusedCoreFrame(). While this seems fine, I am now changing things slightly: in WebViewImpl::setFocus(false) and WebViewImpl::textInputType() we used to use m_page->focusController().focusedFrame(). But now we are using focusedCoreFrame(). I think this is fine. But if we are to mimic the exact usage as it is before this change, I can modify the code to use m_page->focusController().focusedFrame() for those two methods as well. WDYT? https://codereview.chromium.org/2029423003/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2029423003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:2325: if (!mainFrameImpl()) On 2016/07/18 16:38:33, dcheng wrote: > On 2016/07/12 15:13:24, EhsanK wrote: > > On 2016/07/12 14:43:21, dcheng wrote: > > > I don't understand why we need these early returns. Is there documentation > > that > > > describes the potential races with IME and how they're handled? > > > > > > It also seems like these checks are duplicated a lot, so we should be able > to > > > put them in a common helper somewhere. > > > > This check is to make sure we only do IME inside a WebWidget. For the main > > frame, it happens to be the WebViewImpl which is not swapped out (or whose > > RenderWidget is not swapped out). > > > > In WebFrameWidgetImpl, I added the method 'focusedLocalFrameInWidget()' which > > guarantees we are dealing with the intended frame in the intended widget. > > > > Here I did not add the method so I check every time (line 2334). > > > > The reason that I have been doing this alot is that there is actual problems > > with reporting the IME state from multiple Widgets on page which makes > tracking > > the state on the browser side impossible. > > > > I guess having a helper class would go along way. Before, inside > > WebFrameWidgetImpl, we were delegating all these calls to wiewImpl(). I can > see > > how both WebViewImpl() and WebWidget() could call a third helper class for > this. > > > > I can file a bug for it to follow up on if we will be OK with having the > > duplicated code temporarily. > > As a starting point, I think we can have separate helpers for WebFrameWidgetImpl > and WebViewImpl? That should still reduce a bunch of code duplication. If I have understood correctly, focusedLocalFrameInWidget() and focusedLocalFrameAvailableForIme() should do this (added for both WebFrameWidgetImpl and WebViewImpl). Although the latter method is basically the first one but returning nullptr when |m_imeAcceptEvents| is false. I could remove this one and do this instead: LocalFrame* focusedFrame = focusedLocalFrameInWidget(); if (!focusedFrame || !m_imeAcceptEvents) return false; Which right now looks like: LocalFrame* focusedFrame = focusedLocalFrameAvailableForIme(); if (!focusedFrame) return false; https://codereview.chromium.org/2029423003/diff/320001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebWidget.h (right): https://codereview.chromium.org/2029423003/diff/320001/third_party/WebKit/pub... third_party/WebKit/public/web/WebWidget.h:252: virtual void applyReplacementRange(int start, int length) {} On 2016/07/18 16:38:33, dcheng wrote: > On 2016/07/12 15:13:24, EhsanK wrote: > > On 2016/07/12 14:43:21, dcheng wrote: > > > Does it make sense to move these to WebFrameWidget instead? I took a look at > > the > > > WebWidget implementations. It doesn't seem like anything other than frames > is > > > going to bother implementing this interface anyway. > > > > > > (Ideally, I would also like to hide the pre-existing methods on WebView and > > > force callers to go through WebViewFrameWidget, but that is somewhat outside > > the > > > scope of this change) > > > > Sorry if the question is simplistic: I am making these calls from inside > > RenderWidget which only knows a WebWidget, right? Can we know about > > WebFrameWidget from inside content/renderer? > > WebLocalFrame has a frameWidget method (which should probably return a > WebFrameWidget, not a WebWidget). Maybe RenderWidget itself should know when it > has a frame widget associated with it though... either way, this is fine for a > followup. > > > > > Also, another approach could be to remove applyReplacement() and add a range > > argument to both WebWidget::setComposition and WebWidget::confirmComposition. > > I don't really know enough about IME to say what the right design here is. I > guess this is fine (since my comment applies to all the methods added here) > > > > > All this being said, if we go for helper class, say TextInputHelper obtained > > from WebWidget::getTextInputHelper() and then add all these methods to that, > > would it be a better solution? > > I think making this WebFrameWidget specific would be better for now. I think moving common logic in IME to a base class would be good. Later when the separation is complete, we can remove the base class and move methods back to Widget. I too am not certain of creating a helper class for Ime. It might be neater. The problem with RenderWidget is that, right now, it has little code (if any) using RenderFrame or WebFrame. I will ask avi@ about that again since he was working on separating RenderViewImpl and RenderWidget.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
still lgtm. let's try to dedupe this (I have a change which adds a WebFrameWidgetBase class, I'm hopeful that we can use this to start sharing logic)
On 2016/07/19 09:14:59, dcheng wrote: > still lgtm. let's try to dedupe this (I have a change which adds a > WebFrameWidgetBase class, I'm hopeful that we can use this to start sharing > logic) Thanks! I will CQ after dry-run. Will start the de-duping after this one lands.
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ekaramad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org, creis@chromium.org, lfg@chromium.org Link to the patchset: https://codereview.chromium.org/2029423003/#ps380001 (title: "Using focusedCoreFrame() instead of focusController().focusedFrame() in WebViewImpl")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== OOPIF IME: Renderer Side Changes Currently, all IME-related IPC are handled at the RenderViewImpl first before calling the base class methods on RenderWidget (ImeSetComposition and ImeConfirmComposition). Further, to query IME-related state, e.g., GetTextInputType(), RenderViewImpl overrides the method from RenderWidget so that it can access the state from pepper plugin. These overrides are needed to either: 1- Access focused PepperPluginInstacneImpl to set/get IME-related state. 2- Use focused frame to set/get IME-related state. This CL removes all those methods from RenderViewImpl and handles them in RenderWidget. To this end, a reference to the focused plugin is kept in RenderWidget and updated by its owning RenderFrame (to get IME-related state). Also, new API is added to blink to enable RenderWidget handle IME tasks related to focusedFrame. In line with the separation of WebViewImpl from WebWidget, as well as avoiding potential race conditions between mutiple Widgets on the same process, most of the IME- related WebWidget methods are now specifically implemented for WebFrameWidgetImpl. Finally, a new private API is added to WebFrameWidgetImpl to help the widget distinguish between in process frames which do or do not belong to widget's local root. BUG=578168,618774,626746 ========== to ========== OOPIF IME: Renderer Side Changes Currently, all IME-related IPC are handled at the RenderViewImpl first before calling the base class methods on RenderWidget (ImeSetComposition and ImeConfirmComposition). Further, to query IME-related state, e.g., GetTextInputType(), RenderViewImpl overrides the method from RenderWidget so that it can access the state from pepper plugin. These overrides are needed to either: 1- Access focused PepperPluginInstacneImpl to set/get IME-related state. 2- Use focused frame to set/get IME-related state. This CL removes all those methods from RenderViewImpl and handles them in RenderWidget. To this end, a reference to the focused plugin is kept in RenderWidget and updated by its owning RenderFrame (to get IME-related state). Also, new API is added to blink to enable RenderWidget handle IME tasks related to focusedFrame. In line with the separation of WebViewImpl from WebWidget, as well as avoiding potential race conditions between mutiple Widgets on the same process, most of the IME- related WebWidget methods are now specifically implemented for WebFrameWidgetImpl. Finally, a new private API is added to WebFrameWidgetImpl to help the widget distinguish between in process frames which do or do not belong to widget's local root. BUG=578168,618774,626746 ==========
Message was sent while issue was closed.
Committed patchset #18 (id:380001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== OOPIF IME: Renderer Side Changes Currently, all IME-related IPC are handled at the RenderViewImpl first before calling the base class methods on RenderWidget (ImeSetComposition and ImeConfirmComposition). Further, to query IME-related state, e.g., GetTextInputType(), RenderViewImpl overrides the method from RenderWidget so that it can access the state from pepper plugin. These overrides are needed to either: 1- Access focused PepperPluginInstacneImpl to set/get IME-related state. 2- Use focused frame to set/get IME-related state. This CL removes all those methods from RenderViewImpl and handles them in RenderWidget. To this end, a reference to the focused plugin is kept in RenderWidget and updated by its owning RenderFrame (to get IME-related state). Also, new API is added to blink to enable RenderWidget handle IME tasks related to focusedFrame. In line with the separation of WebViewImpl from WebWidget, as well as avoiding potential race conditions between mutiple Widgets on the same process, most of the IME- related WebWidget methods are now specifically implemented for WebFrameWidgetImpl. Finally, a new private API is added to WebFrameWidgetImpl to help the widget distinguish between in process frames which do or do not belong to widget's local root. BUG=578168,618774,626746 ========== to ========== OOPIF IME: Renderer Side Changes Currently, all IME-related IPC are handled at the RenderViewImpl first before calling the base class methods on RenderWidget (ImeSetComposition and ImeConfirmComposition). Further, to query IME-related state, e.g., GetTextInputType(), RenderViewImpl overrides the method from RenderWidget so that it can access the state from pepper plugin. These overrides are needed to either: 1- Access focused PepperPluginInstacneImpl to set/get IME-related state. 2- Use focused frame to set/get IME-related state. This CL removes all those methods from RenderViewImpl and handles them in RenderWidget. To this end, a reference to the focused plugin is kept in RenderWidget and updated by its owning RenderFrame (to get IME-related state). Also, new API is added to blink to enable RenderWidget handle IME tasks related to focusedFrame. In line with the separation of WebViewImpl from WebWidget, as well as avoiding potential race conditions between mutiple Widgets on the same process, most of the IME- related WebWidget methods are now specifically implemented for WebFrameWidgetImpl. Finally, a new private API is added to WebFrameWidgetImpl to help the widget distinguish between in process frames which do or do not belong to widget's local root. BUG=578168,618774,626746 Committed: https://crrev.com/2a46d63b3c442f1f0d26cd1484c25eb766a064bc Cr-Commit-Position: refs/heads/master@{#406263} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/2a46d63b3c442f1f0d26cd1484c25eb766a064bc Cr-Commit-Position: refs/heads/master@{#406263} |
