Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2370393002:
Extracting placeholder information from Webkit to Blimp (Closed)
|
Created:
4 years, 2 months ago by shaktisahu Modified:
4 years, 1 month ago CC:
site-isolation-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionExtracting placeholder information from Webkit to Blimp
For text input, the new Blimp IME requires few additional information
associated with the text field such as label and placeholder attributes.
For a mock UI, visit go/blimp-type-3
In this CL, the Blimp tab makes a call to RenderFrameHost to access
the information about the currently focused text input field and
supplies a callback to act upon obtaining the information. The
RenderFrame gets the information, populates it into a FormFieldData
struct and passes it to the browser which then invokes the supplied
callback by the embedder.
BUG=651902
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/aacf8d1d11beaff516bd79f88b8571eae0f5d031
Cr-Commit-Position: refs/heads/master@{#433364}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Piping through WebContentsDelegate #Patch Set 3 : Observing InputMethod #
Total comments: 10
Patch Set 4 : Added the method to TextInputClient #Patch Set 5 : Extracting info through RHVW #
Total comments: 18
Patch Set 6 : dtrainor comments #
Total comments: 3
Patch Set 7 : Bypassing the typical text entry path #
Total comments: 11
Patch Set 8 : Addressed some comments #
Total comments: 15
Patch Set 9 : Switched from ViewMsg_* to FrameMsg_* #
Total comments: 2
Patch Set 10 : Added test #
Total comments: 26
Patch Set 11 : Sending empty IPC on error #Patch Set 12 : comments from @creis #
Total comments: 2
Patch Set 13 : Removed callback from RenderFrameHost destructor #
Total comments: 6
Patch Set 14 : nits #Patch Set 15 : merge origin/master #Messages
Total messages: 64 (22 generated)
Description was changed from ========== Extracting placeholder information from Webkit to Blimp For text input, the new Blimp IME requires few additional information associated with the text field such as label and placeholder attributes. For a mock UI, visit go/blimp-type-3 Blink can easily add this information to the WebTextInputInfo struct which it passes to the renderer. From the RenderWidget, henceforth we can add this info to content::TextInputState struct and send it to the browser. Blimp engine can be added as an observer to the content::TextInputManager from which it extracts the TextInputState. With this CL, I propose to add TextInputManager and TextInputState to the content/public. As of today, blimp engine is added as an InputMethodObserver which passes a ui::TextInputClient (implemented by RenderWidgetHostViewAura). TextInputClient however doesn't provide all the correct methods to obtain the information about text field. e.g. it doesn't even have a straight forward method to get the current text. For this usage, content::TextInputState seems more appropriate and hence the changes I propose. BUG= ========== to ========== Extracting placeholder information from Webkit to Blimp For text input, the new Blimp IME requires few additional information associated with the text field such as label and placeholder attributes. For a mock UI, visit go/blimp-type-3 Blink can easily add this information to the WebTextInputInfo struct which it passes to the renderer. From the RenderWidget, henceforth we can add this info to content::TextInputState struct and send it to the browser. Blimp engine can be added as an observer to the content::TextInputManager from which it extracts the TextInputState. With this CL, I propose to add TextInputManager and TextInputState to the content/public. As of today, blimp engine is added as an InputMethodObserver which passes a ui::TextInputClient (implemented by RenderWidgetHostViewAura). TextInputClient however doesn't provide all the correct methods to obtain the information about text field. e.g. it doesn't even have a straight forward method to get the current text. For this usage, content::TextInputState seems more appropriate and hence the changes I propose. BUG= ==========
shaktisahu@chromium.org changed reviewers: + creis@chromium.org, ekaramad@chromium.org
creis@, ekaramad@ - PTAL. Please suggest if making content::TextInputManager/TextInputState public is reasonable, since for Blimp's usage it seems like the right way to go.
I left some comments and in short, I don't think you should directly observe TextInputManager. There might be lifetime issues in using RWHV and TextInputManager (I could not find errors by looking at the code since you are removing the observer at a seemingly right time). I think it might be safer to have a a method somewhere else to notify you safely from outside content. Maybe WebContentsDelegate. But all that is something a contents/ reviewer should comment on. I'd rather wait for creis@ comments and if necessary I will do another more detailed pass. https://codereview.chromium.org/2370393002/diff/1/blimp/engine/session/blimp_... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/2370393002/diff/1/blimp/engine/session/blimp_... blimp/engine/session/blimp_engine_session.cc:381: void BlimpEngineSession::OnUpdateTextInputStateCalled( I guess I see why you had to observe TextInputManager. Basically, you need to know the RWH which changed its TextInputState. This is a totally different structure then the rest of InputMethod and TextInputClient that I know of. Passing some info about RWH to InputMethod is also not a good idea since AFAIK InputMethod does not really care which RWH is focused or not; as long as there is a client which consumes the results methods. But could there be something added to WebContentsDelegate about the change instead of exposing all this? We are exposing TextInputManager, RenderWidgetHostViewAura and WebContentsImpl which are all content/ internal. https://codereview.chromium.org/2370393002/diff/1/blimp/engine/session/blimp_... blimp/engine/session/blimp_engine_session.cc:393: tab_->tab_id(), updated_view->GetRenderWidgetHost(), state); RWHV::GetRenderWidgetHost() may return nullptr. I could not tell if this would cause issues down the stack. As I see it it will be used for a map to retrieve a RWHId. I assume the map will not find any id for a nullptr entry? https://codereview.chromium.org/2370393002/diff/1/blimp/engine/session/blimp_... File blimp/engine/session/blimp_engine_session.h (right): https://codereview.chromium.org/2370393002/diff/1/blimp/engine/session/blimp_... blimp/engine/session/blimp_engine_session.h:22: #include "content/browser/renderer_host/text_input_manager.h" Whether or not exposing RWHVBase and TextInputManager is OK is something creis@ (as the content/ owner) should comment on. My question is, do we really need to observer TextInputManager? If yes, then maybe we should have a separate content/public class which observers TextInputManager. There are other APIs in TextInputManager which should absolutely not called from any class other than RWHVBase (such as UpdateTextInputState()). I also left some comments in the cc file. https://codereview.chromium.org/2370393002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/2370393002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_base.h:412: TextInputManager* GetTextInputManager(); Why is this public now? I couldn't find the usage.
ekaramad@ - Thanks a lot for the feedback!
I think it makes more sense now to pipe this call through WebContentsDelegate.
This will avoid exposing any of the internal stuff. I will upload a patch later
today with the proposed changes. The interface should look like
void WebContentsDelegate::OnUpdateTextInputState(
content::RenderWidgetHost * rwh,
std::string& existing_text,
std::string& placeholder
);
BlimpEngineSession already implements this interface.
https://codereview.chromium.org/2370393002/diff/1/blimp/engine/session/blimp_...
File blimp/engine/session/blimp_engine_session.cc (right):
https://codereview.chromium.org/2370393002/diff/1/blimp/engine/session/blimp_...
blimp/engine/session/blimp_engine_session.cc:381: void
BlimpEngineSession::OnUpdateTextInputStateCalled(
On 2016/09/28 16:14:11, EhsanK wrote:
> I guess I see why you had to observe TextInputManager. Basically, you need to
> know the RWH which changed its TextInputState. This is a totally different
> structure then the rest of InputMethod and TextInputClient that I know of.
>
> Passing some info about RWH to InputMethod is also not a good idea since AFAIK
> InputMethod does not really care which RWH is focused or not; as long as there
> is a client which consumes the results methods.
>
> But could there be something added to WebContentsDelegate about the change
> instead of exposing all this? We are exposing TextInputManager,
> RenderWidgetHostViewAura and WebContentsImpl which are all content/ internal.
I think I pipe this call to RenderWidgetHostDelegate -> WebContentsDelegate ->
BlimpEngineSession. That makes more sense and in that way we don't have to
expose any of the content internal stuff.
https://codereview.chromium.org/2370393002/diff/1/blimp/engine/session/blimp_...
blimp/engine/session/blimp_engine_session.cc:393: tab_->tab_id(),
updated_view->GetRenderWidgetHost(), state);
On 2016/09/28 16:14:11, EhsanK wrote:
> RWHV::GetRenderWidgetHost() may return nullptr. I could not tell if this would
> cause issues down the stack. As I see it it will be used for a map to retrieve
a
> RWHId. I assume the map will not find any id for a nullptr entry?
Ya, this is checked. The function will return 0 for nullptr entry and we are
DCHECKing the id for non-zero after that. So indirectly the DCHECK checks
thatthe GetRenderWidgetHost() should return non-null.
https://codereview.chromium.org/2370393002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_base.h (right):
https://codereview.chromium.org/2370393002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_base.h:412:
TextInputManager* GetTextInputManager();
On 2016/09/28 16:14:11, EhsanK wrote:
> Why is this public now? I couldn't find the usage.
Oh. I will change it back to protected. I don't need it any more.
ekaramad@ - PTAL. I just updated the patch to route the calls through RWVHAura->RenderWidgetHostDelegate(WebContentsImpl)->WebContentsDelegate(BlimpEngineSession) and it looks better. I also considered using RWVHAura->InputMethod->InputMethodObserver(BlimpEngineSession) route. For now, this one seems better, but much would depend on how the UX for blimp user input evolves.
Description was changed from ========== Extracting placeholder information from Webkit to Blimp For text input, the new Blimp IME requires few additional information associated with the text field such as label and placeholder attributes. For a mock UI, visit go/blimp-type-3 Blink can easily add this information to the WebTextInputInfo struct which it passes to the renderer. From the RenderWidget, henceforth we can add this info to content::TextInputState struct and send it to the browser. Blimp engine can be added as an observer to the content::TextInputManager from which it extracts the TextInputState. With this CL, I propose to add TextInputManager and TextInputState to the content/public. As of today, blimp engine is added as an InputMethodObserver which passes a ui::TextInputClient (implemented by RenderWidgetHostViewAura). TextInputClient however doesn't provide all the correct methods to obtain the information about text field. e.g. it doesn't even have a straight forward method to get the current text. For this usage, content::TextInputState seems more appropriate and hence the changes I propose. BUG= ========== to ========== Extracting placeholder information from Webkit to Blimp For text input, the new Blimp IME requires few additional information associated with the text field such as label and placeholder attributes. For a mock UI, visit go/blimp-type-3 Blink can easily add this information to the WebTextInputInfo struct which it passes to the renderer. From the RenderWidget, henceforth we can add this info to content::TextInputState struct and extract it at the browser and then pass it along to BlimpEngineSession through WebContentsDelegate. BUG=651902 ==========
shaktisahu@chromium.org changed reviewers: + dtrainor@chromium.org
dtrainor@ - PTAL
Thanks! LGTM. https://codereview.chromium.org/2370393002/diff/1/blimp/engine/session/blimp_... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/2370393002/diff/1/blimp/engine/session/blimp_... blimp/engine/session/blimp_engine_session.cc:381: void BlimpEngineSession::OnUpdateTextInputStateCalled( On 2016/09/28 20:17:23, shaktisahu wrote: > On 2016/09/28 16:14:11, EhsanK wrote: > > I guess I see why you had to observe TextInputManager. Basically, you need to > > know the RWH which changed its TextInputState. This is a totally different > > structure then the rest of InputMethod and TextInputClient that I know of. > > > > Passing some info about RWH to InputMethod is also not a good idea since AFAIK > > InputMethod does not really care which RWH is focused or not; as long as there > > is a client which consumes the results methods. > > > > But could there be something added to WebContentsDelegate about the change > > instead of exposing all this? We are exposing TextInputManager, > > RenderWidgetHostViewAura and WebContentsImpl which are all content/ internal. > > I think I pipe this call to RenderWidgetHostDelegate -> WebContentsDelegate -> > BlimpEngineSession. That makes more sense and in that way we don't have to > expose any of the content internal stuff. I too think going through the delegates is better. https://codereview.chromium.org/2370393002/diff/1/blimp/engine/session/blimp_... blimp/engine/session/blimp_engine_session.cc:393: tab_->tab_id(), updated_view->GetRenderWidgetHost(), state); On 2016/09/28 20:17:23, shaktisahu wrote: > On 2016/09/28 16:14:11, EhsanK wrote: > > RWHV::GetRenderWidgetHost() may return nullptr. I could not tell if this would > > cause issues down the stack. As I see it it will be used for a map to retrieve > a > > RWHId. I assume the map will not find any id for a nullptr entry? > > Ya, this is checked. The function will return 0 for nullptr entry and we are > DCHECKing the id for non-zero after that. So indirectly the DCHECK checks > thatthe GetRenderWidgetHost() should return non-null. Acknowledged.
Description was changed from ========== Extracting placeholder information from Webkit to Blimp For text input, the new Blimp IME requires few additional information associated with the text field such as label and placeholder attributes. For a mock UI, visit go/blimp-type-3 Blink can easily add this information to the WebTextInputInfo struct which it passes to the renderer. From the RenderWidget, henceforth we can add this info to content::TextInputState struct and extract it at the browser and then pass it along to BlimpEngineSession through WebContentsDelegate. BUG=651902 ========== to ========== Extracting placeholder information from Webkit to Blimp For text input, the new Blimp IME requires few additional information associated with the text field such as label and placeholder attributes. For a mock UI, visit go/blimp-type-3 Blink can easily add this information to the WebTextInputInfo struct which it passes to the renderer. From the RenderWidget, henceforth we can add this info to content::TextInputState struct and extract it at the browser and then pass it along to BlimpEngineSession through WebContentsDelegate. BUG=651902 ==========
shaktisahu@chromium.org changed reviewers: + nyquist@chromium.org
https://codereview.chromium.org/2370393002/diff/40001/blimp/engine/feature/en... File blimp/engine/feature/engine_render_widget_feature_unittest.cc (right): https://codereview.chromium.org/2370393002/diff/40001/blimp/engine/feature/en... blimp/engine/feature/engine_render_widget_feature_unittest.cc:268: std::string text = "green apple"; Should these be declared as anonymous constants? you could possibly use it in the mock? https://codereview.chromium.org/2370393002/diff/40001/blimp/engine/feature/en... blimp/engine/feature/engine_render_widget_feature_unittest.cc:269: std::string placeholder = "fruit name"; Will we add support for passing along the placeholder later? https://codereview.chromium.org/2370393002/diff/40001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.h (right): https://codereview.chromium.org/2370393002/diff/40001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.h:145: Nit: Remove empty line, since this is also part of the WebContentsDelegate implementation. https://codereview.chromium.org/2370393002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2370393002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:2996: if (state && host_->delegate()) Nit: I think this needs curlies. It's multi-line. https://codereview.chromium.org/2370393002/diff/40001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2370393002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:2485: if (delegate_) Nit: if (!delegate_) return; [empty line] delegate_->...
Thanks for working through this, and sorry for the late review. I'm a bit uncomfortable having these delegate methods only get called on one platform. Maybe we can more clearly document or ifdef them, so that people don't try to use them on other platforms? https://codereview.chromium.org/2370393002/diff/40001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/2370393002/diff/40001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:431: tab_->web_contents()->GetRenderWidgetHostView()->GetRenderWidgetHost(), Sanity check: I'm not sure where this is running, but this pattern looks broken for out-of-process iframes (and possibly GuestViews). In those cases, the WebContents can have more than one RenderWidgetHost. Is that a problem here? https://codereview.chromium.org/2370393002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_delegate.h (right): https://codereview.chromium.org/2370393002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_delegate.h:220: // Notifies a change in the text input state. nit: Notifies about a https://codereview.chromium.org/2370393002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2370393002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:2996: if (state && host_->delegate()) On 2016/10/05 04:20:56, nyquist (OOO after 10-6) wrote: > Nit: I think this needs curlies. It's multi-line. Yes, that's right. https://codereview.chromium.org/2370393002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:2999: state->show_ime_if_needed); Why is this only done in Aura? If it's not needed on other platforms, we should more clearly document that in the delegate's declaration, so that other people don't implement it and assume it works everywhere. https://codereview.chromium.org/2370393002/diff/40001/content/public/browser/... File content/public/browser/web_contents_delegate.h (right): https://codereview.chromium.org/2370393002/diff/40001/content/public/browser/... content/public/browser/web_contents_delegate.h:533: virtual void OnUpdateTextInputState( All public methods here should have comments. Also, it's a bigger problem here if this only works on Aura. WebContentsDelegate methods are generally expected to be platform agnostic, unless it's clear otherwise (e.g., ifdef or clear explanation). I'm not sure it makes sense to only have this for one platform.
On 2016/10/05 21:19:34, Charlie Reis wrote: > Thanks for working through this, and sorry for the late review. > > I'm a bit uncomfortable having these delegate methods only get called on one > platform. Maybe we can more clearly document or ifdef them, so that people > don't try to use them on other platforms? What about if we just rely on InputMethodObserver which is a part of UI? We could add a method to ui::TextInputClient called GetPlaceholder() that returns the placeholder text. The ui::InputMethodObserver::On*Changed() methods all pass the ui::TextInputClient along so we can query it as necessary. And RenderWidgetHostViewAura implements the ui::TextInputClient method and seems to already do a lot of piping of content::TextInputState -> ui::TextInputClient for all of the other values. wdyt? > > https://codereview.chromium.org/2370393002/diff/40001/blimp/engine/session/bl... > File blimp/engine/session/blimp_engine_session.cc (right): > > https://codereview.chromium.org/2370393002/diff/40001/blimp/engine/session/bl... > blimp/engine/session/blimp_engine_session.cc:431: > tab_->web_contents()->GetRenderWidgetHostView()->GetRenderWidgetHost(), > Sanity check: I'm not sure where this is running, but this pattern looks broken > for out-of-process iframes (and possibly GuestViews). In those cases, the > WebContents can have more than one RenderWidgetHost. > > Is that a problem here? > > https://codereview.chromium.org/2370393002/diff/40001/content/browser/rendere... > File content/browser/renderer_host/render_widget_host_delegate.h (right): > > https://codereview.chromium.org/2370393002/diff/40001/content/browser/rendere... > content/browser/renderer_host/render_widget_host_delegate.h:220: // Notifies a > change in the text input state. > nit: Notifies about a > > https://codereview.chromium.org/2370393002/diff/40001/content/browser/rendere... > File content/browser/renderer_host/render_widget_host_view_aura.cc (right): > > https://codereview.chromium.org/2370393002/diff/40001/content/browser/rendere... > content/browser/renderer_host/render_widget_host_view_aura.cc:2996: if (state && > host_->delegate()) > On 2016/10/05 04:20:56, nyquist (OOO after 10-6) wrote: > > Nit: I think this needs curlies. It's multi-line. > > Yes, that's right. > > https://codereview.chromium.org/2370393002/diff/40001/content/browser/rendere... > content/browser/renderer_host/render_widget_host_view_aura.cc:2999: > state->show_ime_if_needed); > Why is this only done in Aura? > > If it's not needed on other platforms, we should more clearly document that in > the delegate's declaration, so that other people don't implement it and assume > it works everywhere. > > https://codereview.chromium.org/2370393002/diff/40001/content/public/browser/... > File content/public/browser/web_contents_delegate.h (right): > > https://codereview.chromium.org/2370393002/diff/40001/content/public/browser/... > content/public/browser/web_contents_delegate.h:533: virtual void > OnUpdateTextInputState( > All public methods here should have comments. > > Also, it's a bigger problem here if this only works on Aura. > WebContentsDelegate methods are generally expected to be platform agnostic, > unless it's clear otherwise (e.g., ifdef or clear explanation). I'm not sure it > makes sense to only have this for one platform.
On 2016/10/06 16:44:40, David Trainor wrote: > On 2016/10/05 21:19:34, Charlie Reis wrote: > > Thanks for working through this, and sorry for the late review. > > > > I'm a bit uncomfortable having these delegate methods only get called on one > > platform. Maybe we can more clearly document or ifdef them, so that people > > don't try to use them on other platforms? > > What about if we just rely on InputMethodObserver which is a part of UI? > > We could add a method to ui::TextInputClient called GetPlaceholder() that > returns the placeholder text. The ui::InputMethodObserver::On*Changed() methods > all pass the ui::TextInputClient along so we can query it as necessary. And > RenderWidgetHostViewAura implements the ui::TextInputClient method and seems to > already do a lot of piping of content::TextInputState -> ui::TextInputClient for > all of the other values. > > wdyt? > Sounds reasonable from my perspective. Caveat: I don't know the ui::TextInputClient code at all, and I'm just reacting to the general RWH vs platform-specific RWHVA aspect. You'll probably want a more knowledgeable reviewer to confirm, but it sounds like a good idea to me.
shaktisahu@chromium.org changed reviewers: + shuchen@chromium.org, tkent@chromium.org
https://codereview.chromium.org/2370393002/diff/80001/blimp/engine/feature/en... File blimp/engine/feature/engine_render_widget_feature.cc (right): https://codereview.chromium.org/2370393002/diff/80001/blimp/engine/feature/en... blimp/engine/feature/engine_render_widget_feature.cc:140: const std::string& placeholder) { Do we want to add this to the proto as well? https://codereview.chromium.org/2370393002/diff/80001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/2370393002/diff/80001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:425: base::Callback<void(const std::string&, const std::string&)> reply = This should probably live inside Tab so we can do a bunch of extra tracking. 1. It'll be specific to the tab that called it (if tabs change you won't get a show IME request for the wrong tab). 2. If we ever get another show IME request we should wait for the callback to complete, drop the results, and call the callback again (IMO). https://codereview.chromium.org/2370393002/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_base.cc (right): https://codereview.chromium.org/2370393002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_base.cc:173: if (!GetRenderWidgetHost()) I would trigger the callback with an empty struct or something. We should honor the API. Either that or return false to indicate this won't run? https://codereview.chromium.org/2370393002/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/2370393002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_base.h:506: typedef std::map<int, using? https://codereview.chromium.org/2370393002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_base.h:507: base::Callback<void(const std::string&, const std::string&)>> Pull out a callback with using? https://codereview.chromium.org/2370393002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_base.h:511: int next_request_id_; Should this just be in an anonymous namespace in the CC file? https://codereview.chromium.org/2370393002/diff/80001/content/public/browser/... File content/public/browser/render_widget_host_view.h (right): https://codereview.chromium.org/2370393002/diff/80001/content/public/browser/... content/public/browser/render_widget_host_view.h:96: virtual void FetchTextInputInfo( It might make sense to pull a struct out for the arguments if we're going to be adding a few more in the near future. https://codereview.chromium.org/2370393002/diff/80001/content/renderer/render... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2370393002/diff/80001/content/renderer/render... content/renderer/render_widget.cc:1538: blink::WebTextInputInfo info = GetWebWidget()->textInputInfo(); See note below. I think we want to avoid using textInputInfo. Is there a way to pull the currently focused element and do some processing on it? If not, maybe we can expose something? https://codereview.chromium.org/2370393002/diff/80001/content/renderer/render... File content/renderer/render_widget.h (right): https://codereview.chromium.org/2370393002/diff/80001/content/renderer/render... content/renderer/render_widget.h:511: void OnFormTextInputInfoRequested(int request_id); Can we call this (and all associated methods/messages) onFocusedFormFieldDataRequested(int request_id)? wdyt? https://codereview.chromium.org/2370393002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2370393002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:970: element = frame().document()->focusedElement(); This still has the problem of being in the common path for typical text entry right? I think once we add label we don't want to add any extra work here. Is there a way to pull out these attributes that doesn't go through this code path? Also will this work for non-text input fields (for label in the future)?
dtrainor@ - PTAL https://codereview.chromium.org/2370393002/diff/80001/blimp/engine/feature/en... File blimp/engine/feature/engine_render_widget_feature.cc (right): https://codereview.chromium.org/2370393002/diff/80001/blimp/engine/feature/en... blimp/engine/feature/engine_render_widget_feature.cc:140: const std::string& placeholder) { On 2016/10/26 01:36:42, David Trainor (OOO to oct 31) wrote: > Do we want to add this to the proto as well? Yes. I will add it in a subsequent CL for blimp changes. https://codereview.chromium.org/2370393002/diff/80001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/2370393002/diff/80001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:425: base::Callback<void(const std::string&, const std::string&)> reply = On 2016/10/26 01:36:42, David Trainor (OOO to oct 31) wrote: > This should probably live inside Tab so we can do a bunch of extra tracking. > > 1. It'll be specific to the tab that called it (if tabs change you won't get a > show IME request for the wrong tab). > 2. If we ever get another show IME request we should wait for the callback to > complete, drop the results, and call the callback again (IMO). Done. https://codereview.chromium.org/2370393002/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_base.cc (right): https://codereview.chromium.org/2370393002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_base.cc:173: if (!GetRenderWidgetHost()) On 2016/10/26 01:36:42, David Trainor (OOO to oct 31) wrote: > I would trigger the callback with an empty struct or something. We should honor > the API. Either that or return false to indicate this won't run? Done. https://codereview.chromium.org/2370393002/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/2370393002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_base.h:506: typedef std::map<int, On 2016/10/26 01:36:42, David Trainor (OOO to oct 31) wrote: > using? Done. https://codereview.chromium.org/2370393002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_base.h:507: base::Callback<void(const std::string&, const std::string&)>> On 2016/10/26 01:36:42, David Trainor (OOO to oct 31) wrote: > Pull out a callback with using? Done. https://codereview.chromium.org/2370393002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_base.h:511: int next_request_id_; On 2016/10/26 01:36:42, David Trainor (OOO to oct 31) wrote: > Should this just be in an anonymous namespace in the CC file? Done. https://codereview.chromium.org/2370393002/diff/80001/content/public/browser/... File content/public/browser/render_widget_host_view.h (right): https://codereview.chromium.org/2370393002/diff/80001/content/public/browser/... content/public/browser/render_widget_host_view.h:96: virtual void FetchTextInputInfo( On 2016/10/26 01:36:42, David Trainor (OOO to oct 31) wrote: > It might make sense to pull a struct out for the arguments if we're going to be > adding a few more in the near future. Done. Added a struct in content/public/common/form_field_data.h https://codereview.chromium.org/2370393002/diff/80001/content/renderer/render... File content/renderer/render_widget.h (right): https://codereview.chromium.org/2370393002/diff/80001/content/renderer/render... content/renderer/render_widget.h:511: void OnFormTextInputInfoRequested(int request_id); On 2016/10/26 01:36:42, David Trainor (OOO to oct 31) wrote: > Can we call this (and all associated methods/messages) > onFocusedFormFieldDataRequested(int request_id)? > > wdyt? Sounds good. I am adding a struct FormFieldData in content/public/common, naming the callback as ExtractFormFieldDataCallback, get/set method as GetFocusedFormFieldCallBack/Set...
https://codereview.chromium.org/2370393002/diff/100001/blimp/engine/session/t... File blimp/engine/session/tab.cc (right): https://codereview.chromium.org/2370393002/diff/100001/blimp/engine/session/t... blimp/engine/session/tab.cc:177: web_contents()->GetRenderWidgetHostView()->GetFocusedFormFieldData(reply); What if I get a ShowIme() request while I'm waiting for the callback? I mentioned a way around it in the previous comment. We should do something to handle it properly. https://codereview.chromium.org/2370393002/diff/100001/blimp/engine/session/t... File blimp/engine/session/tab.h (right): https://codereview.chromium.org/2370393002/diff/100001/blimp/engine/session/t... blimp/engine/session/tab.h:74: void ShowIme(); ShowFormInputUI/HideFormInputUI? https://codereview.chromium.org/2370393002/diff/100001/content/public/common/... File content/public/common/form_field_data.h (right): https://codereview.chromium.org/2370393002/diff/100001/content/public/common/... content/public/common/form_field_data.h:18: ui::TextInputType text_input_type; Do we need to set a default value? https://codereview.chromium.org/2370393002/diff/120001/blimp/engine/session/t... File blimp/engine/session/tab.cc (right): https://codereview.chromium.org/2370393002/diff/120001/blimp/engine/session/t... blimp/engine/session/tab.cc:187: VLOG(0) << "field : " << field.text << " " << field.placeholder; Remove? https://codereview.chromium.org/2370393002/diff/120001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2370393002/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.cc:501: OnFocusedFormFieldDataReply) Make this method name match the message name? https://codereview.chromium.org/2370393002/diff/120001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.cc (right): https://codereview.chromium.org/2370393002/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.cc:169: extract_form_field_data_callbacks_[request_id].Run(field_data); What if we have callbacks when the RWHV dies? Should we just trigger all callbacks with an invalid result set? https://codereview.chromium.org/2370393002/diff/120001/content/common/view_me... File content/common/view_messages.h (right): https://codereview.chromium.org/2370393002/diff/120001/content/common/view_me... content/common/view_messages.h:811: IPC_MESSAGE_ROUTED2(ViewHostMsg_SetFocusedFormFieldData, Should this have the word "Reply" in the message? It's not really setting the data if there was no get call. https://codereview.chromium.org/2370393002/diff/120001/content/public/common/... File content/public/common/form_field_data.h (right): https://codereview.chromium.org/2370393002/diff/120001/content/public/common/... content/public/common/form_field_data.h:18: ui::TextInputType text_input_type; Add a constructor and set a default for this IMO https://codereview.chromium.org/2370393002/diff/120001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2370393002/diff/120001/content/renderer/rende... content/renderer/render_view_impl.cc:1396: blink::WebFrame* web_frame = webview()->focusedFrame(); Should we check if webview() is null or webview()->focusedFrame() is null (some methods do check these)? https://codereview.chromium.org/2370393002/diff/120001/content/renderer/rende... content/renderer/render_view_impl.cc:1397: WebElement element = web_frame->document().focusedElement(); Check if document().isNull()? Check if focusedElement.isNull()? https://codereview.chromium.org/2370393002/diff/120001/content/renderer/rende... content/renderer/render_view_impl.cc:1399: blink::WebTextInputInfo info = GetWebWidget()->textInputInfo(); Check if GetWebWidget() is null? https://codereview.chromium.org/2370393002/diff/120001/content/renderer/rende... content/renderer/render_view_impl.cc:1401: field.text = info.value.utf8(); Are all of these guaranteed to be valid? info.value, element, element.getAttribute()? Just make sure we're checking the right thing. Dig into this a bit, a lot of these objects have isNull().
creis@chromium.org changed reviewers: + kenrb@chromium.org
[+kenrb, site-isolation-reviews] I think there's an out-of-process iframe (OOPIF) issue here by relying on RenderView. Also, Ken, can you take a look at the RWHV destructor question? https://codereview.chromium.org/2370393002/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.cc (right): https://codereview.chromium.org/2370393002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.cc:62: callback.Run(FormFieldData()); This is a bit surprising to me-- it seems like we shouldn't be doing a lot of extra work in this destructor. Is this safe? https://codereview.chromium.org/2370393002/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/2370393002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:504: using FormFieldDataReplyMap = std::map<int, ExtractFormFieldDataCallback>; Style nit: I think this belongs at the top of the private section, around line 498. https://codereview.chromium.org/2370393002/diff/140001/content/public/browser... File content/public/browser/render_widget_host_view.h (right): https://codereview.chromium.org/2370393002/diff/140001/content/public/browser... content/public/browser/render_widget_host_view.h:37: using ExtractFormFieldDataCallback = base::Callback<void(const FormFieldData&)>; nit: Drop "Extract" from the name? Sounds kind of like the callback is expected to do the extraction, which isn't right. https://codereview.chromium.org/2370393002/diff/140001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2370393002/diff/140001/content/renderer/rende... content/renderer/render_view_impl.cc:1396: blink::WebFrame* web_frame = webview()->focusedFrame(); This looks wrong for OOPIFs. We should be sending a FrameHostMsg IPC to the focused RenderFrame, which might not be in the same process as the main frame. (In general, RenderViewImpl is deprecated for this reason, as noted in the .h file.) Can you include a test for the OOPIF case? ekaramad@ has lots of examples from his IME work.
https://codereview.chromium.org/2370393002/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.cc (right): https://codereview.chromium.org/2370393002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.cc:62: callback.Run(FormFieldData()); On 2016/11/04 20:59:46, Charlie Reis wrote: > This is a bit surprising to me-- it seems like we shouldn't be doing a lot of > extra work in this destructor. Is this safe? Yeah I had suggested this to make sure we properly cleaned up all outstanding callbacks but that's a great point... Would posting the callback be safer?
On 2016/11/04 20:59:46, Charlie Reis (OOO til 11-11) wrote: > [+kenrb, site-isolation-reviews] > > I think there's an out-of-process iframe (OOPIF) issue here by relying on > RenderView. > > Also, Ken, can you take a look at the RWHV destructor question? > > https://codereview.chromium.org/2370393002/diff/140001/content/browser/render... > File content/browser/renderer_host/render_widget_host_view_base.cc (right): > > https://codereview.chromium.org/2370393002/diff/140001/content/browser/render... > content/browser/renderer_host/render_widget_host_view_base.cc:62: > callback.Run(FormFieldData()); > This is a bit surprising to me-- it seems like we shouldn't be doing a lot of > extra work in this destructor. Is this safe? > > https://codereview.chromium.org/2370393002/diff/140001/content/browser/render... > File content/browser/renderer_host/render_widget_host_view_base.h (right): > > https://codereview.chromium.org/2370393002/diff/140001/content/browser/render... > content/browser/renderer_host/render_widget_host_view_base.h:504: using > FormFieldDataReplyMap = std::map<int, ExtractFormFieldDataCallback>; > Style nit: I think this belongs at the top of the private section, around line > 498. > > https://codereview.chromium.org/2370393002/diff/140001/content/public/browser... > File content/public/browser/render_widget_host_view.h (right): > > https://codereview.chromium.org/2370393002/diff/140001/content/public/browser... > content/public/browser/render_widget_host_view.h:37: using > ExtractFormFieldDataCallback = base::Callback<void(const FormFieldData&)>; > nit: Drop "Extract" from the name? Sounds kind of like the callback is expected > to do the extraction, which isn't right. > > https://codereview.chromium.org/2370393002/diff/140001/content/renderer/rende... > File content/renderer/render_view_impl.cc (right): > > https://codereview.chromium.org/2370393002/diff/140001/content/renderer/rende... > content/renderer/render_view_impl.cc:1396: blink::WebFrame* web_frame = > webview()->focusedFrame(); > This looks wrong for OOPIFs. We should be sending a FrameHostMsg IPC to the > focused RenderFrame, which might not be in the same process as the main frame. > > (In general, RenderViewImpl is deprecated for this reason, as noted in the .h > file.) > > Can you include a test for the OOPIF case? ekaramad@ has lots of examples from > his IME work. Thanks. I will move these methods from RenderView to RenderFrame and corresponding messages.
Description was changed from ========== Extracting placeholder information from Webkit to Blimp For text input, the new Blimp IME requires few additional information associated with the text field such as label and placeholder attributes. For a mock UI, visit go/blimp-type-3 Blink can easily add this information to the WebTextInputInfo struct which it passes to the renderer. From the RenderWidget, henceforth we can add this info to content::TextInputState struct and extract it at the browser and then pass it along to BlimpEngineSession through WebContentsDelegate. BUG=651902 ========== to ========== Extracting placeholder information from Webkit to Blimp For text input, the new Blimp IME requires few additional information associated with the text field such as label and placeholder attributes. For a mock UI, visit go/blimp-type-3 Blink can easily add this information to the WebTextInputInfo struct which it passes to the renderer. From the RenderWidget, henceforth we can add this info to content::TextInputState struct and extract it at the browser and then pass it along to BlimpEngineSession through WebContentsDelegate. BUG=651902 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
dtrainor@, creis@ - PTAL https://codereview.chromium.org/2370393002/diff/120001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2370393002/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.cc:501: OnFocusedFormFieldDataReply) On 2016/11/03 04:25:49, David Trainor wrote: > Make this method name match the message name? Done. https://codereview.chromium.org/2370393002/diff/120001/content/public/common/... File content/public/common/form_field_data.h (right): https://codereview.chromium.org/2370393002/diff/120001/content/public/common/... content/public/common/form_field_data.h:18: ui::TextInputType text_input_type; On 2016/11/03 04:25:49, David Trainor wrote: > Add a constructor and set a default for this IMO Done.
https://codereview.chromium.org/2370393002/diff/140001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2370393002/diff/140001/content/renderer/rende... content/renderer/render_view_impl.cc:1396: blink::WebFrame* web_frame = webview()->focusedFrame(); On 2016/11/04 20:59:46, Charlie Reis (OOO til 11-11) wrote: > This looks wrong for OOPIFs. We should be sending a FrameHostMsg IPC to the > focused RenderFrame, which might not be in the same process as the main frame. > > (In general, RenderViewImpl is deprecated for this reason, as noted in the .h > file.) > > Can you include a test for the OOPIF case? ekaramad@ has lots of examples from > his IME work. I think, if needed, this can be tested for OOPIF in a straightforward. way. There seems to be an IPC coming back in response to another one sent to the renderer. Maybe running a test with multiple <iframe>'s and sending and receiving those IPCs? https://codereview.chromium.org/2370393002/diff/160001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2370393002/diff/160001/content/renderer/rende... content/renderer/render_frame_impl.cc:2307: WebElement element = frame_->document().focusedElement(); I noticed that the assumption here is |frame_| is already focused (given that the IPC is sent to focused frame). However, it might not be the case since by the time the IPC arrives, some other WebLocalFrame (even with a different local root) might be focused (within the same process). In other words, you might end up requesting the information from one frame and then receiving it from another. When receiving the response you should make sure it is from the intended frame. Or alternatively, do not handle the IPC if the frame is no longer foucsed? WDYT?
creis@ - PTAL. https://codereview.chromium.org/2370393002/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.cc (right): https://codereview.chromium.org/2370393002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.cc:62: callback.Run(FormFieldData()); On 2016/11/04 21:14:07, David Trainor wrote: > On 2016/11/04 20:59:46, Charlie Reis wrote: > > This is a bit surprising to me-- it seems like we shouldn't be doing a lot of > > extra work in this destructor. Is this safe? > > Yeah I had suggested this to make sure we properly cleaned up all outstanding > callbacks but that's a great point... Would posting the callback be safer? I see some more examples of callbacks getting called in RenderFrameHostImpl destructor eg. visual_state_callbacks. (Look at the RFHI in new patch). For most cases, this map will be empty at this point. Only in cases where we are unable to extract the focused frame data from RenderFrame, this might happen. https://codereview.chromium.org/2370393002/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/2370393002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:504: using FormFieldDataReplyMap = std::map<int, ExtractFormFieldDataCallback>; On 2016/11/04 20:59:46, Charlie Reis (OOO til 11-11) wrote: > Style nit: I think this belongs at the top of the private section, around line > 498. Done. https://codereview.chromium.org/2370393002/diff/140001/content/public/browser... File content/public/browser/render_widget_host_view.h (right): https://codereview.chromium.org/2370393002/diff/140001/content/public/browser... content/public/browser/render_widget_host_view.h:37: using ExtractFormFieldDataCallback = base::Callback<void(const FormFieldData&)>; On 2016/11/04 20:59:46, Charlie Reis (OOO til 11-11) wrote: > nit: Drop "Extract" from the name? Sounds kind of like the callback is expected > to do the extraction, which isn't right. Done. Now it's called FormFieldDataCallback. https://codereview.chromium.org/2370393002/diff/140001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2370393002/diff/140001/content/renderer/rende... content/renderer/render_view_impl.cc:1396: blink::WebFrame* web_frame = webview()->focusedFrame(); On 2016/11/08 15:52:45, EhsanK wrote: > On 2016/11/04 20:59:46, Charlie Reis (OOO til 11-11) wrote: > > This looks wrong for OOPIFs. We should be sending a FrameHostMsg IPC to the > > focused RenderFrame, which might not be in the same process as the main frame. > > > > (In general, RenderViewImpl is deprecated for this reason, as noted in the .h > > file.) > > > > Can you include a test for the OOPIF case? ekaramad@ has lots of examples > from > > his IME work. > > I think, if needed, this can be tested for OOPIF in a straightforward. way. > There seems to be an IPC coming back in response to another one sent to the > renderer. Maybe running a test with multiple <iframe>'s and sending and > receiving those IPCs? Done. https://codereview.chromium.org/2370393002/diff/160001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2370393002/diff/160001/content/renderer/rende... content/renderer/render_frame_impl.cc:2307: WebElement element = frame_->document().focusedElement(); On 2016/11/08 15:52:45, EhsanK wrote: > I noticed that the assumption here is |frame_| is already focused (given that > the IPC is sent to focused frame). However, it might not be the case since by > the time the IPC arrives, some other WebLocalFrame (even with a different local > root) might be focused (within the same process). In other words, you might end > up requesting the information from one frame and then receiving it from another. > > When receiving the response you should make sure it is from the intended frame. > Or alternatively, do not handle the IPC if the frame is no longer foucsed? WDYT? Ack! Added check for if |frame_| is still focused.
LGTM with nits and some questions about focus. Thanks! https://codereview.chromium.org/2370393002/diff/180001/chrome/browser/rendere... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2370393002/diff/180001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:307: std::string text_; nit: you can make these two const. https://codereview.chromium.org/2370393002/diff/180001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:417: EXPECT_TRUE(ExecuteScript(rfh, script)); I believe focusing the <input> this way will not lead to an immediate update of TextInputState since it is not a user gesture: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Eleme... The next update of TextInputState, both for RenderWidget and the corresponding being sent to the browser happens on the next compositor frame: https://cs.chromium.org/chromium/src/content/renderer/render_widget.cc?rcl=0&... You send the IPC later after focus() is called. I am not sure if it is at all possible to get to the RenderFrameImpl code before TextInputState is updated (which might fail the test). It might be nice to verify that. https://codereview.chromium.org/2370393002/diff/180001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:775: nit: No need for space. https://codereview.chromium.org/2370393002/diff/180001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:776: IN_PROC_BROWSER_TEST_F(SitePerProcessTextInputManagerTest, Does this test need to run on Mac as well? https://codereview.chromium.org/2370393002/diff/180001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:796: FocusFormField(frame, values[i]); Would this also focus the right frame? If not maybe you should add "window.focus()" as well.
https://codereview.chromium.org/2370393002/diff/180001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2370393002/diff/180001/content/renderer/rende... content/renderer/render_frame_impl.cc:2317: return; We still have to respond to the request right? https://codereview.chromium.org/2370393002/diff/180001/content/renderer/rende... content/renderer/render_frame_impl.cc:2321: return; Same here.
Looking much better, and thanks for the test! A few extra questions and concerns below, but I think we're getting close. https://codereview.chromium.org/2370393002/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.cc (right): https://codereview.chromium.org/2370393002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.cc:62: callback.Run(FormFieldData()); On 2016/11/11 01:15:03, shaktisahu wrote: > On 2016/11/04 21:14:07, David Trainor wrote: > > On 2016/11/04 20:59:46, Charlie Reis wrote: > > > This is a bit surprising to me-- it seems like we shouldn't be doing a lot > of > > > extra work in this destructor. Is this safe? > > > > Yeah I had suggested this to make sure we properly cleaned up all outstanding > > callbacks but that's a great point... Would posting the callback be safer? > > I see some more examples of callbacks getting called in RenderFrameHostImpl > destructor eg. visual_state_callbacks. (Look at the RFHI in new patch). For > most cases, this map will be empty at this point. Only in cases where we are > unable to extract the focused frame data from RenderFrame, this might happen. Hmm, those aren't reasons it's safe. :) It depends on whether the callback might access some deleted state. In the case of visual_state_callbacks_, the callback explicitly takes a boolean parameter saying whether the frame has been destroyed, detached, etc to deal with this: // ... The call completes asynchronously by running // the supplied |callback| with a value of true upon successful completion and // false otherwise (when the frame is destroyed, detached, etc..). typedef base::Callback<void(bool)> VisualStateCallback; https://cs.chromium.org/chromium/src/content/public/browser/render_frame_host... Posting it as a task wouldn't be much safer in that case. The question comes down to what you need to do in the callback, which I haven't investigated. If the RenderFrameHost is partly destroyed, will that affect its execution? What if the WebContents is partly destroyed? This is what we need to be careful about. https://codereview.chromium.org/2370393002/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2370393002/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1727: form_field_data_callbacks_.end()); This should not be a DCHECK if it's in response to an IPC from the renderer, which isn't trusted. Would it be safe to return early instead if request_id isn't found? https://codereview.chromium.org/2370393002/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2370393002/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:510: void OnFocusedFormFieldDataResponse(int request_id, nit: This should be declared with the other IPC handlers. https://codereview.chromium.org/2370393002/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:884: std::map<int, FormFieldDataCallback> form_field_data_callbacks_; As dtrainor@ noted, there's a risk that these will leak if we don't get a response. We should clear them in RFHI::OnRenderProcessGone, similar to ax_tree_snapshot_callbacks_. (I'm surprised we don't do that for the two callback maps above-- feel free to add that as well, or I can do it in a followup.) https://codereview.chromium.org/2370393002/diff/180001/content/common/frame_m... File content/common/frame_messages.h (right): https://codereview.chromium.org/2370393002/diff/180001/content/common/frame_m... content/common/frame_messages.h:728: // Requests the renderer for information about currently focused text input nit: Requests information about ... from the renderer. https://codereview.chromium.org/2370393002/diff/180001/content/common/frame_m... content/common/frame_messages.h:730: IPC_MESSAGE_ROUTED1(FrameMsg_FocusedFormFieldDataRequest, int) Use a /* comment */ to indicate what the int is. https://codereview.chromium.org/2370393002/diff/180001/content/common/frame_m... content/common/frame_messages.h:1375: int, Add comments with parameter names, as above and below. https://codereview.chromium.org/2370393002/diff/180001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2370393002/diff/180001/content/renderer/rende... content/renderer/render_frame_impl.cc:2317: return; On 2016/11/11 18:44:38, David Trainor wrote: > We still have to respond to the request right? Yes, otherwise we'll have a leak in the browser for unacknowledged requests. I think it's worth putting a comment at the top of this method that a response is always needed and there should be no early returns (to avoid adding this bug in the future).
https://codereview.chromium.org/2370393002/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.cc (right): https://codereview.chromium.org/2370393002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.cc:62: callback.Run(FormFieldData()); On 2016/11/11 22:20:46, Charlie Reis (OOO til 11-11) wrote: > On 2016/11/11 01:15:03, shaktisahu wrote: > > On 2016/11/04 21:14:07, David Trainor wrote: > > > On 2016/11/04 20:59:46, Charlie Reis wrote: > > > > This is a bit surprising to me-- it seems like we shouldn't be doing a lot > > of > > > > extra work in this destructor. Is this safe? > > > > > > Yeah I had suggested this to make sure we properly cleaned up all > outstanding > > > callbacks but that's a great point... Would posting the callback be safer? > > > > I see some more examples of callbacks getting called in RenderFrameHostImpl > > destructor eg. visual_state_callbacks. (Look at the RFHI in new patch). For > > most cases, this map will be empty at this point. Only in cases where we are > > unable to extract the focused frame data from RenderFrame, this might happen. > > Hmm, those aren't reasons it's safe. :) It depends on whether the callback > might access some deleted state. In the case of visual_state_callbacks_, the > callback explicitly takes a boolean parameter saying whether the frame has been > destroyed, detached, etc to deal with this: > > // ... The call completes asynchronously by running > // the supplied |callback| with a value of true upon successful completion and > // false otherwise (when the frame is destroyed, detached, etc..). > typedef base::Callback<void(bool)> VisualStateCallback; > > https://cs.chromium.org/chromium/src/content/public/browser/render_frame_host... > > Posting it as a task wouldn't be much safer in that case. > > The question comes down to what you need to do in the callback, which I haven't > investigated. If the RenderFrameHost is partly destroyed, will that affect its > execution? What if the WebContents is partly destroyed? This is what we need > to be careful about. Posting the task guarantees you run after the class cleanup finishes, but I guess you need to make sure the callback does the right thing like you said. Either way I think the boolean would be a good idea. The request can fail for other reasons as well.
creis@ - PTAL https://codereview.chromium.org/2370393002/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.cc (right): https://codereview.chromium.org/2370393002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.cc:62: callback.Run(FormFieldData()); On 2016/11/11 22:20:46, Charlie Reis (slow) wrote: > On 2016/11/11 01:15:03, shaktisahu wrote: > > On 2016/11/04 21:14:07, David Trainor wrote: > > > On 2016/11/04 20:59:46, Charlie Reis wrote: > > > > This is a bit surprising to me-- it seems like we shouldn't be doing a lot > > of > > > > extra work in this destructor. Is this safe? > > > > > > Yeah I had suggested this to make sure we properly cleaned up all > outstanding > > > callbacks but that's a great point... Would posting the callback be safer? > > > > I see some more examples of callbacks getting called in RenderFrameHostImpl > > destructor eg. visual_state_callbacks. (Look at the RFHI in new patch). For > > most cases, this map will be empty at this point. Only in cases where we are > > unable to extract the focused frame data from RenderFrame, this might happen. > > Hmm, those aren't reasons it's safe. :) It depends on whether the callback > might access some deleted state. In the case of visual_state_callbacks_, the > callback explicitly takes a boolean parameter saying whether the frame has been > destroyed, detached, etc to deal with this: > > // ... The call completes asynchronously by running > // the supplied |callback| with a value of true upon successful completion and > // false otherwise (when the frame is destroyed, detached, etc..). > typedef base::Callback<void(bool)> VisualStateCallback; > > https://cs.chromium.org/chromium/src/content/public/browser/render_frame_host... > > Posting it as a task wouldn't be much safer in that case. > > The question comes down to what you need to do in the callback, which I haven't > investigated. If the RenderFrameHost is partly destroyed, will that affect its > execution? What if the WebContents is partly destroyed? This is what we need > to be careful about. Very good point. I thought a bit about how to make this safe by whether adding a boolean or some other means. In blimp (tab.cc), inside the callback, the first check I do is the ui::TextInputType field of the FormFieldData struct. If it has default value (ui::TEXT_INPUT_TYPE_NONE), we do an early return from the callback. That guarantees it to be safe. Let me know what you think. I was thinking of adding an extra "boolean success" as the first argument of the callback, but now I think the default TextInputType exactly does the same. https://codereview.chromium.org/2370393002/diff/180001/chrome/browser/rendere... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2370393002/diff/180001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:417: EXPECT_TRUE(ExecuteScript(rfh, script)); On 2016/11/11 01:44:17, EhsanK wrote: > I believe focusing the <input> this way will not lead to an immediate update of > TextInputState since it is not a user gesture: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Eleme... > > The next update of TextInputState, both for RenderWidget and the corresponding > being sent to the browser happens on the next compositor frame: > https://cs.chromium.org/chromium/src/content/renderer/render_widget.cc?rcl=0&... > > You send the IPC later after focus() is called. I am not sure if it is at all > possible to get to the RenderFrameImpl code before TextInputState is updated > (which might fail the test). It might be nice to verify that. Apparently, as I see from logs, it is considered as a user gesture. So the subsequent code runs fine. https://codereview.chromium.org/2370393002/diff/180001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:775: On 2016/11/11 01:44:18, EhsanK wrote: > nit: No need for space. Done. https://codereview.chromium.org/2370393002/diff/180001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:776: IN_PROC_BROWSER_TEST_F(SitePerProcessTextInputManagerTest, On 2016/11/11 01:44:17, EhsanK wrote: > Does this test need to run on Mac as well? No. Although ideally you should be able to make this call from RenderFrameHost from any platform. https://codereview.chromium.org/2370393002/diff/180001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:796: FocusFormField(frame, values[i]); On 2016/11/11 01:44:17, EhsanK wrote: > Would this also focus the right frame? If not maybe you should add > "window.focus()" as well. I think it does focus the right field and frame. Otherwise the test should fail as in RenderFrame we are checking if the frame is focused or not. https://codereview.chromium.org/2370393002/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2370393002/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1727: form_field_data_callbacks_.end()); On 2016/11/11 22:20:46, Charlie Reis (OOO til 11-11) wrote: > This should not be a DCHECK if it's in response to an IPC from the renderer, > which isn't trusted. Would it be safe to return early instead if request_id > isn't found? Done. Sure, this should be an early return with NOTREACHED log. https://codereview.chromium.org/2370393002/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2370393002/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:510: void OnFocusedFormFieldDataResponse(int request_id, On 2016/11/11 22:20:46, Charlie Reis (OOO til 11-11) wrote: > nit: This should be declared with the other IPC handlers. Done. https://codereview.chromium.org/2370393002/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:884: std::map<int, FormFieldDataCallback> form_field_data_callbacks_; On 2016/11/11 22:20:46, Charlie Reis (OOO til 11-11) wrote: > As dtrainor@ noted, there's a risk that these will leak if we don't get a > response. We should clear them in RFHI::OnRenderProcessGone, similar to > ax_tree_snapshot_callbacks_. (I'm surprised we don't do that for the two > callback maps above-- feel free to add that as well, or I can do it in a > followup.) Done. https://codereview.chromium.org/2370393002/diff/180001/content/common/frame_m... File content/common/frame_messages.h (right): https://codereview.chromium.org/2370393002/diff/180001/content/common/frame_m... content/common/frame_messages.h:728: // Requests the renderer for information about currently focused text input On 2016/11/11 22:20:46, Charlie Reis (OOO til 11-11) wrote: > nit: Requests information about ... from the renderer. Done. https://codereview.chromium.org/2370393002/diff/180001/content/common/frame_m... content/common/frame_messages.h:730: IPC_MESSAGE_ROUTED1(FrameMsg_FocusedFormFieldDataRequest, int) On 2016/11/11 22:20:46, Charlie Reis (OOO til 11-11) wrote: > Use a /* comment */ to indicate what the int is. Done. https://codereview.chromium.org/2370393002/diff/180001/content/common/frame_m... content/common/frame_messages.h:1375: int, On 2016/11/11 22:20:46, Charlie Reis (OOO til 11-11) wrote: > Add comments with parameter names, as above and below. Done. https://codereview.chromium.org/2370393002/diff/180001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2370393002/diff/180001/content/renderer/rende... content/renderer/render_frame_impl.cc:2317: return; On 2016/11/11 18:44:38, David Trainor wrote: > We still have to respond to the request right? Done. Sending an empty message. The blimp engine will check the message's text_input_type field and figure out if the message succeeded. https://codereview.chromium.org/2370393002/diff/180001/content/renderer/rende... content/renderer/render_frame_impl.cc:2321: return; On 2016/11/11 18:44:38, David Trainor wrote: > Same here. Done.
Thanks! This is ready from my perspective, apart from one request about the destructor callbacks. https://codereview.chromium.org/2370393002/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.cc (right): https://codereview.chromium.org/2370393002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.cc:62: callback.Run(FormFieldData()); On 2016/11/15 05:44:54, shaktisahu wrote: > On 2016/11/11 22:20:46, Charlie Reis (slow) wrote: > > On 2016/11/11 01:15:03, shaktisahu wrote: > > > On 2016/11/04 21:14:07, David Trainor wrote: > > > > On 2016/11/04 20:59:46, Charlie Reis wrote: > > > > > This is a bit surprising to me-- it seems like we shouldn't be doing a > lot > > > of > > > > > extra work in this destructor. Is this safe? > > > > > > > > Yeah I had suggested this to make sure we properly cleaned up all > > outstanding > > > > callbacks but that's a great point... Would posting the callback be safer? > > > > > > I see some more examples of callbacks getting called in RenderFrameHostImpl > > > destructor eg. visual_state_callbacks. (Look at the RFHI in new patch). > For > > > most cases, this map will be empty at this point. Only in cases where we are > > > unable to extract the focused frame data from RenderFrame, this might > happen. > > > > Hmm, those aren't reasons it's safe. :) It depends on whether the callback > > might access some deleted state. In the case of visual_state_callbacks_, the > > callback explicitly takes a boolean parameter saying whether the frame has > been > > destroyed, detached, etc to deal with this: > > > > // ... The call completes asynchronously by running > > // the supplied |callback| with a value of true upon successful completion > and > > // false otherwise (when the frame is destroyed, detached, etc..). > > typedef base::Callback<void(bool)> VisualStateCallback; > > > > > https://cs.chromium.org/chromium/src/content/public/browser/render_frame_host... > > > > Posting it as a task wouldn't be much safer in that case. > > > > The question comes down to what you need to do in the callback, which I > haven't > > investigated. If the RenderFrameHost is partly destroyed, will that affect > its > > execution? What if the WebContents is partly destroyed? This is what we need > > to be careful about. > > Very good point. I thought a bit about how to make this safe by whether adding a > boolean or some other means. In blimp (tab.cc), inside the callback, the first > check I do is the ui::TextInputType field of the FormFieldData struct. If it has > default value (ui::TEXT_INPUT_TYPE_NONE), we do an early return from the > callback. That guarantees it to be safe. Let me know what you think. I was > thinking of adding an extra "boolean success" as the first argument of the > callback, but now I think the default TextInputType exactly does the same. Hmm. If the callback is just going to return early whenever we run it from here, can we skip calling it from this destructor entirely and just erase the remaining callbacks? That seems much safer to me, since otherwise there's a risk of future code adding a callback that interacts with the partially destructed object. https://codereview.chromium.org/2370393002/diff/220001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2370393002/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1693: NOTREACHED() << "Received form field data response for unknown request"; Normally we omit the NOTREACHED as well-- it's known that renderers may be compromised, and the browser should expect to handle that case, not crash on it. (The typical practice is to add a ReceivedBadMessage call to kill the renderer if we think it has security implications.) In this case, I don't feel strongly. If you want to keep this for a while to try to spot bugs, I'm ok with it.
creis@, dtrainor@, kenrb@ - PTAL https://codereview.chromium.org/2370393002/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.cc (right): https://codereview.chromium.org/2370393002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.cc:62: callback.Run(FormFieldData()); On 2016/11/16 00:18:00, Charlie Reis (slow) wrote: > On 2016/11/15 05:44:54, shaktisahu wrote: > > On 2016/11/11 22:20:46, Charlie Reis (slow) wrote: > > > On 2016/11/11 01:15:03, shaktisahu wrote: > > > > On 2016/11/04 21:14:07, David Trainor wrote: > > > > > On 2016/11/04 20:59:46, Charlie Reis wrote: > > > > > > This is a bit surprising to me-- it seems like we shouldn't be doing a > > lot > > > > of > > > > > > extra work in this destructor. Is this safe? > > > > > > > > > > Yeah I had suggested this to make sure we properly cleaned up all > > > outstanding > > > > > callbacks but that's a great point... Would posting the callback be > safer? > > > > > > > > I see some more examples of callbacks getting called in > RenderFrameHostImpl > > > > destructor eg. visual_state_callbacks. (Look at the RFHI in new patch). > > For > > > > most cases, this map will be empty at this point. Only in cases where we > are > > > > unable to extract the focused frame data from RenderFrame, this might > > happen. > > > > > > Hmm, those aren't reasons it's safe. :) It depends on whether the callback > > > might access some deleted state. In the case of visual_state_callbacks_, > the > > > callback explicitly takes a boolean parameter saying whether the frame has > > been > > > destroyed, detached, etc to deal with this: > > > > > > // ... The call completes asynchronously by running > > > // the supplied |callback| with a value of true upon successful completion > > and > > > // false otherwise (when the frame is destroyed, detached, etc..). > > > typedef base::Callback<void(bool)> VisualStateCallback; > > > > > > > > > https://cs.chromium.org/chromium/src/content/public/browser/render_frame_host... > > > > > > Posting it as a task wouldn't be much safer in that case. > > > > > > The question comes down to what you need to do in the callback, which I > > haven't > > > investigated. If the RenderFrameHost is partly destroyed, will that affect > > its > > > execution? What if the WebContents is partly destroyed? This is what we > need > > > to be careful about. > > > > Very good point. I thought a bit about how to make this safe by whether adding > a > > boolean or some other means. In blimp (tab.cc), inside the callback, the first > > check I do is the ui::TextInputType field of the FormFieldData struct. If it > has > > default value (ui::TEXT_INPUT_TYPE_NONE), we do an early return from the > > callback. That guarantees it to be safe. Let me know what you think. I was > > thinking of adding an extra "boolean success" as the first argument of the > > callback, but now I think the default TextInputType exactly does the same. > > Hmm. If the callback is just going to return early whenever we run it from > here, can we skip calling it from this destructor entirely and just erase the > remaining callbacks? > > That seems much safer to me, since otherwise there's a risk of future code > adding a callback that interacts with the partially destructed object. Ok. I am removing this from the destructor and clearing the map instead. I am adding the callback tracking to blimp code instead. https://codereview.chromium.org/2370393002/diff/220001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2370393002/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1693: NOTREACHED() << "Received form field data response for unknown request"; On 2016/11/16 00:18:00, Charlie Reis (slow) wrote: > Normally we omit the NOTREACHED as well-- it's known that renderers may be > compromised, and the browser should expect to handle that case, not crash on it. > (The typical practice is to add a ReceivedBadMessage call to kill the renderer > if we think it has security implications.) > > In this case, I don't feel strongly. If you want to keep this for a while to > try to spot bugs, I'm ok with it. Ok, I will remove the else block.
Thanks. content/ LGTM. I'll defer to dtrainor on the tab.cc changes. https://codereview.chromium.org/2370393002/diff/240001/blimp/engine/session/t... File blimp/engine/session/tab.cc (right): https://codereview.chromium.org/2370393002/diff/240001/blimp/engine/session/t... blimp/engine/session/tab.cc:201: if (field.text_input_type == ui::TEXT_INPUT_TYPE_NONE) Do you still need this? Probably better to remove unless there's a case for it. https://codereview.chromium.org/2370393002/diff/240001/blimp/engine/session/t... blimp/engine/session/tab.cc:205: if (request_id < g_text_input_request_id) { Does this need to be global? Seems like a member would make more sense.
ipc lgtm
Description was changed from ========== Extracting placeholder information from Webkit to Blimp For text input, the new Blimp IME requires few additional information associated with the text field such as label and placeholder attributes. For a mock UI, visit go/blimp-type-3 Blink can easily add this information to the WebTextInputInfo struct which it passes to the renderer. From the RenderWidget, henceforth we can add this info to content::TextInputState struct and extract it at the browser and then pass it along to BlimpEngineSession through WebContentsDelegate. BUG=651902 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Extracting placeholder information from Webkit to Blimp For text input, the new Blimp IME requires few additional information associated with the text field such as label and placeholder attributes. For a mock UI, visit go/blimp-type-3 In this CL, the Blimp tab makes a call to RenderFrameHost to access the information about the currently focused text input field and supplies a callback to act upon obtaining the information. The RenderFrame gets the information, populates it into a FormFieldData struct and passes it to the browser which then invokes the supplied callback by the embedder. BUG=651902 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
dtrainor@ - PTAL https://codereview.chromium.org/2370393002/diff/240001/blimp/engine/session/t... File blimp/engine/session/tab.cc (right): https://codereview.chromium.org/2370393002/diff/240001/blimp/engine/session/t... blimp/engine/session/tab.cc:201: if (field.text_input_type == ui::TEXT_INPUT_TYPE_NONE) On 2016/11/16 20:32:49, Charlie Reis (slow) wrote: > Do you still need this? Probably better to remove unless there's a case for it. Yes, we still return TEXT_INPUT_TYPE_NONE when the renderer cannot obtain the information about the focused text field correctly. https://codereview.chromium.org/2370393002/diff/240001/blimp/engine/session/t... blimp/engine/session/tab.cc:205: if (request_id < g_text_input_request_id) { On 2016/11/16 20:32:49, Charlie Reis (slow) wrote: > Does this need to be global? Seems like a member would make more sense. I am not sure. I felt like this is cleaner. Making it a class member will properly reset it though. dtrainor@ - What do you think?
lgtm % nits thanks! https://codereview.chromium.org/2370393002/diff/240001/blimp/engine/feature/e... File blimp/engine/feature/engine_render_widget_feature.cc (right): https://codereview.chromium.org/2370393002/diff/240001/blimp/engine/feature/e... blimp/engine/feature/engine_render_widget_feature.cc:139: const content::FormFieldData& field) { This will probably encapsulate popups eventually too right? We'll want the label. https://codereview.chromium.org/2370393002/diff/240001/blimp/engine/session/t... File blimp/engine/session/tab.cc (right): https://codereview.chromium.org/2370393002/diff/240001/blimp/engine/session/t... blimp/engine/session/tab.cc:205: if (request_id < g_text_input_request_id) { On 2016/11/17 01:16:15, shaktisahu wrote: > On 2016/11/16 20:32:49, Charlie Reis (slow) wrote: > > Does this need to be global? Seems like a member would make more sense. > > I am not sure. I felt like this is cleaner. Making it a class member will > properly reset it though. dtrainor@ - What do you think? A member sounds good IMO. current_form_request_id_?
The CQ bit was checked by shaktisahu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ekaramad@chromium.org, dtrainor@chromium.org, kenrb@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/2370393002/#ps260001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_site_isolation on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by shaktisahu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for blimp/engine/feature/engine_render_widget_feature.cc:
While running git apply --index -p1;
error: patch failed: blimp/engine/feature/engine_render_widget_feature.cc:14
error: blimp/engine/feature/engine_render_widget_feature.cc: patch does not
apply
Patch: blimp/engine/feature/engine_render_widget_feature.cc
Index: blimp/engine/feature/engine_render_widget_feature.cc
diff --git a/blimp/engine/feature/engine_render_widget_feature.cc
b/blimp/engine/feature/engine_render_widget_feature.cc
index
7de5e8b39c254e8afce7e2232790e6e07d8469e4..c53b1db60b2a4fbcc39c7088c0ce49e10af8519e
100644
--- a/blimp/engine/feature/engine_render_widget_feature.cc
+++ b/blimp/engine/feature/engine_render_widget_feature.cc
@@ -14,6 +14,7 @@
#include "blimp/net/input_message_converter.h"
#include "content/public/browser/render_widget_host.h"
#include "content/public/browser/render_widget_host_view.h"
+#include "content/public/common/form_field_data.h"
#include "net/base/net_errors.h"
#include "third_party/WebKit/public/web/WebInputEvent.h"
#include "ui/events/event.h"
@@ -135,9 +136,7 @@ void EngineRenderWidgetFeature::SendCompositorMessage(
void EngineRenderWidgetFeature::SendShowImeRequest(
const int tab_id,
content::RenderWidgetHost* render_widget_host,
- const ui::TextInputClient* client) {
- DCHECK(client);
-
+ const content::FormFieldData& field) {
ImeMessage* ime_message;
std::unique_ptr<BlimpMessage> blimp_message =
CreateBlimpMessage(&ime_message, tab_id);
@@ -147,13 +146,9 @@ void EngineRenderWidgetFeature::SendShowImeRequest(
ime_message->set_render_widget_id(render_widget_id);
ime_message->set_type(ImeMessage::SHOW_IME);
ime_message->set_text_input_type(
- InputMessageConverter::TextInputTypeToProto(client->GetTextInputType()));
-
- gfx::Range text_range;
- base::string16 existing_text;
- client->GetTextRange(&text_range);
- client->GetTextFromRange(text_range, &existing_text);
- ime_message->set_ime_text(base::UTF16ToUTF8(existing_text));
+ InputMessageConverter::TextInputTypeToProto(field.text_input_type));
+ ime_message->set_ime_text(field.text);
+ // TODO(shaktisahu): Add remaining fields to proto.
ime_message_sender_->ProcessMessage(std::move(blimp_message),
net::CompletionCallback());
The CQ bit was checked by shaktisahu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for blimp/engine/feature/engine_render_widget_feature.cc:
While running git apply --index -p1;
error: patch failed: blimp/engine/feature/engine_render_widget_feature.cc:14
error: blimp/engine/feature/engine_render_widget_feature.cc: patch does not
apply
Patch: blimp/engine/feature/engine_render_widget_feature.cc
Index: blimp/engine/feature/engine_render_widget_feature.cc
diff --git a/blimp/engine/feature/engine_render_widget_feature.cc
b/blimp/engine/feature/engine_render_widget_feature.cc
index
7de5e8b39c254e8afce7e2232790e6e07d8469e4..c53b1db60b2a4fbcc39c7088c0ce49e10af8519e
100644
--- a/blimp/engine/feature/engine_render_widget_feature.cc
+++ b/blimp/engine/feature/engine_render_widget_feature.cc
@@ -14,6 +14,7 @@
#include "blimp/net/input_message_converter.h"
#include "content/public/browser/render_widget_host.h"
#include "content/public/browser/render_widget_host_view.h"
+#include "content/public/common/form_field_data.h"
#include "net/base/net_errors.h"
#include "third_party/WebKit/public/web/WebInputEvent.h"
#include "ui/events/event.h"
@@ -135,9 +136,7 @@ void EngineRenderWidgetFeature::SendCompositorMessage(
void EngineRenderWidgetFeature::SendShowImeRequest(
const int tab_id,
content::RenderWidgetHost* render_widget_host,
- const ui::TextInputClient* client) {
- DCHECK(client);
-
+ const content::FormFieldData& field) {
ImeMessage* ime_message;
std::unique_ptr<BlimpMessage> blimp_message =
CreateBlimpMessage(&ime_message, tab_id);
@@ -147,13 +146,9 @@ void EngineRenderWidgetFeature::SendShowImeRequest(
ime_message->set_render_widget_id(render_widget_id);
ime_message->set_type(ImeMessage::SHOW_IME);
ime_message->set_text_input_type(
- InputMessageConverter::TextInputTypeToProto(client->GetTextInputType()));
-
- gfx::Range text_range;
- base::string16 existing_text;
- client->GetTextRange(&text_range);
- client->GetTextFromRange(text_range, &existing_text);
- ime_message->set_ime_text(base::UTF16ToUTF8(existing_text));
+ InputMessageConverter::TextInputTypeToProto(field.text_input_type));
+ ime_message->set_ime_text(field.text);
+ // TODO(shaktisahu): Add remaining fields to proto.
ime_message_sender_->ProcessMessage(std::move(blimp_message),
net::CompletionCallback());
The CQ bit was checked by shaktisahu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ekaramad@chromium.org, dtrainor@chromium.org, kenrb@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/2370393002/#ps280001 (title: "merge origin/master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by shaktisahu@chromium.org
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.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Extracting placeholder information from Webkit to Blimp For text input, the new Blimp IME requires few additional information associated with the text field such as label and placeholder attributes. For a mock UI, visit go/blimp-type-3 In this CL, the Blimp tab makes a call to RenderFrameHost to access the information about the currently focused text input field and supplies a callback to act upon obtaining the information. The RenderFrame gets the information, populates it into a FormFieldData struct and passes it to the browser which then invokes the supplied callback by the embedder. BUG=651902 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Extracting placeholder information from Webkit to Blimp For text input, the new Blimp IME requires few additional information associated with the text field such as label and placeholder attributes. For a mock UI, visit go/blimp-type-3 In this CL, the Blimp tab makes a call to RenderFrameHost to access the information about the currently focused text input field and supplies a callback to act upon obtaining the information. The RenderFrame gets the information, populates it into a FormFieldData struct and passes it to the browser which then invokes the supplied callback by the embedder. BUG=651902 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/aacf8d1d11beaff516bd79f88b8571eae0f5d031 Cr-Commit-Position: refs/heads/master@{#433364} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/aacf8d1d11beaff516bd79f88b8571eae0f5d031 Cr-Commit-Position: refs/heads/master@{#433364}
Message was sent while issue was closed.
A revert of this CL (patchset #15 id:280001) has been created in https://codereview.chromium.org/2513333002/ by dpranke@chromium.org. The reason for reverting is: I'm speculatively reverting this to see if it causes the failure in blink_platform_unittests noted in https://crbug.com/667094 and as seen in https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Trusty... . It's not at all obvious to my why this would cause that failure, but it's not obvious to me what other changes would've, either, unless maybe it was pdr's change (which I will try if this doesn't fix it).. |
