Extracting placeholder information from Webkit to Blimp

content/browser/renderer_host/render_widget_host_view_base.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/renderer_host/render_widget_host_view_base.h"
 
 #include "base/logging.h"
 #include "build/build_config.h"
 #include "content/browser/accessibility/browser_accessibility_manager.h"
 #include "content/browser/gpu/gpu_data_manager_impl.h"
 #include "content/browser/renderer_host/input/synthetic_gesture_target_base.h"
 #include "content/browser/renderer_host/render_process_host_impl.h"
 #include "content/browser/renderer_host/render_widget_host_delegate.h"
 #include "content/browser/renderer_host/render_widget_host_impl.h"
 #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"
 #include "ui/gfx/geometry/size_conversions.h"
 #include "ui/gfx/geometry/size_f.h"
 
 namespace content {
 
 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()
     : is_fullscreen_(false),
       popup_type_(blink::WebPopupTypeNone),
       background_color_(SK_ColorWHITE),
       mouse_locked_(false),
       showing_context_menu_(false),
 #if !defined(USE_AURA)
       selection_text_offset_(0),
       selection_range_(gfx::Range::InvalidRange()),
 #endif
       current_device_scale_factor_(0),
       current_display_rotation_(display::Display::ROTATE_0),
       text_input_manager_(nullptr),
       renderer_frame_number_(0),
       weak_factory_(this) {
 }
 
 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
   // RenderWidgetHostInputEventRouter's surface map before relinquishing a
   // host pointer, as in RenderWidgetHostViewGuest. There is no harm in calling
   // NotifyObserversAboutShutdown() twice, as the observers are required to
   // de-register on the first call, and so the second call does nothing.
   NotifyObserversAboutShutdown();
   // If we have a live reference to |text_input_manager_|, we should unregister
   // so that the |text_input_manager_| will free its state.
@@ skipping 87 matching lines @@
 
 gfx::Size RenderWidgetHostViewBase::GetRequestedRendererSize() const {
   return GetViewBounds().size();
 }
 
 ui::TextInputClient* RenderWidgetHostViewBase::GetTextInputClient() {
   NOTREACHED();
   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_;
 }
 
 void RenderWidgetHostViewBase::SetShowingContextMenu(bool showing) {
   DCHECK_NE(showing_context_menu_, showing);
   showing_context_menu_ = showing;
 }
 
 base::string16 RenderWidgetHostViewBase::GetSelectedText() {
@@ skipping 352 matching lines @@
 
 bool RenderWidgetHostViewBase::IsChildFrameForTesting() const {
   return false;
 }
 
 cc::SurfaceId RenderWidgetHostViewBase::SurfaceIdForTesting() const {
   return cc::SurfaceId();
 }
 
 }  // namespace content