Extracting placeholder information from Webkit to Blimp

content/browser/renderer_host/render_widget_host_view_base.cc

Index: content/browser/renderer_host/render_widget_host_view_base.cc
diff --git a/content/browser/renderer_host/render_widget_host_view_base.cc b/content/browser/renderer_host/render_widget_host_view_base.cc
index 24129edc19f8293342b6b580c309a230d6b7d9ee..15f1437bed681d27ff93c9024b7ed7ef7bd523a5 100644
--- a/content/browser/renderer_host/render_widget_host_view_base.cc
+++ b/content/browser/renderer_host/render_widget_host_view_base.cc
@@ -15,7 +15,9 @@
 #include "content/browser/renderer_host/render_widget_host_view_base_observer.h"
 #include "content/browser/renderer_host/text_input_manager.h"
 #include "content/common/content_switches_internal.h"
+#include "content/common/view_messages.h"
 #include "content/public/browser/render_widget_host_view_frame_subscriber.h"
+#include "content/public/common/form_field_data.h"
 #include "ui/display/display.h"
 #include "ui/display/screen.h"
 #include "ui/gfx/geometry/point_conversions.h"
@@ -29,6 +31,8 @@ namespace {
 // How many microseconds apart input events should be flushed.
 const int kFlushInputRateInUs = 16666;
 
+// For tracking requests for form field data.
+int next_request_id_ = 0;
 }
 
 RenderWidgetHostViewBase::RenderWidgetHostViewBase()
@@ -50,6 +54,14 @@ RenderWidgetHostViewBase::RenderWidgetHostViewBase()
 
 RenderWidgetHostViewBase::~RenderWidgetHostViewBase() {
   DCHECK(!mouse_locked_);
+
+  // Run callbacks for the outstanding text input info requests, if any.
+  for (auto iter = extract_form_field_data_callbacks_.begin();
+       iter != extract_form_field_data_callbacks_.end(); ++iter) {
+    ExtractFormFieldDataCallback callback = iter->second;
+    callback.Run(FormFieldData());

[Charlie Reis, 2016/11/04 20:59:46]

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?

[David Trainor- moved to gerrit, 2016/11/04 21:14:07]

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?

[shaktisahu, 2016/11/11 01:15:03]

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.

[Charlie Reis, 2016/11/11 22:20:46]

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.

[David Trainor- moved to gerrit, 2016/11/12 00:17:35]

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.

[shaktisahu, 2016/11/15 05:44:54]

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.

[Charlie Reis, 2016/11/16 00:18:00]

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.

[shaktisahu, 2016/11/16 20:26:58]

Ok. I am removing this from the destructor and clearing the map instead. I am
adding the callback tracking to blimp code instead.

+  }
+
   // We call this here to guarantee that observers are notified before we go
   // away. However, some subclasses may wish to call this earlier in their
   // shutdown process, e.g. to force removal from
@@ -157,6 +169,28 @@ ui::TextInputClient* RenderWidgetHostViewBase::GetTextInputClient() {
   return NULL;
 }
 
+void RenderWidgetHostViewBase::OnFocusedFormFieldDataReply(
+    int request_id,
+    const FormFieldData& field_data) {
+  DCHECK(extract_form_field_data_callbacks_.find(request_id) !=
+         extract_form_field_data_callbacks_.end());
+  extract_form_field_data_callbacks_[request_id].Run(field_data);
+  extract_form_field_data_callbacks_.erase(request_id);
+}
+
+void RenderWidgetHostViewBase::GetFocusedFormFieldData(
+    ExtractFormFieldDataCallback& reply) {
+  if (!GetRenderWidgetHost()) {
+    reply.Run(FormFieldData());
+    return;
+  }
+
+  int request_id = ++next_request_id_;
+  extract_form_field_data_callbacks_[request_id] = reply;
+  GetRenderWidgetHost()->Send(new ViewMsg_GetFocusedFormFieldData(
+      GetRenderWidgetHost()->GetRoutingID(), request_id));
+}
+
 bool RenderWidgetHostViewBase::IsShowingContextMenu() const {
   return showing_context_menu_;
 }