Separate RenderViewHost from RenderWidgetHost, part 5: move calls to the RenderWidgetHostDelegate.

content/browser/renderer_host/render_widget_host_impl.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_impl.h"
 
 #include <math.h>
 #include <set>
 #include <utility>
 
@@ skipping 547 matching lines @@
   // deal as RenderWidget::WasShown delays updating, so that the resize from
   // WasResized is usually processed before the renderer is painted.
   WasResized();
 }
 
 bool RenderWidgetHostImpl::GetResizeParams(
     ViewMsg_Resize_Params* resize_params) {
   *resize_params = ViewMsg_Resize_Params();
 
   GetWebScreenInfo(&resize_params->screen_info);
-  resize_params->resizer_rect = GetRootWindowResizerRect();
+  if (delegate_)
+    resize_params->resizer_rect = delegate_->GetRootWindowResizerRect();
 
   if (view_) {
     resize_params->new_size = view_->GetRequestedRendererSize();
     resize_params->physical_backing_size = view_->GetPhysicalBackingSize();
     resize_params->top_controls_height = view_->GetTopControlsHeight();
     resize_params->top_controls_shrink_blink_size =
         view_->DoTopControlsShrinkBlinkSize();
     resize_params->visible_viewport_size = view_->GetVisibleViewportSize();
     resize_params->is_fullscreen_granted = IsFullscreenGranted();
     resize_params->display_mode = GetDisplayMode();
@@ skipping 510 matching lines @@
     // Some keypresses that are accepted by the listener might have follow up
     // char events, which should be ignored.
     if (key_event.type == WebKeyboardEvent::RawKeyDown)
       suppress_next_char_events_ = true;
     return;
   }
 
   if (key_event.type == WebKeyboardEvent::Char &&
       (key_event.windowsKeyCode == ui::VKEY_RETURN ||
        key_event.windowsKeyCode == ui::VKEY_SPACE)) {
-    OnUserGesture();
+    if (delegate_)
+      delegate_->OnUserGesture();
   }
 
   // Double check the type to make sure caller hasn't sent us nonsense that
   // will mess up our key queue.
   if (!WebInputEvent::isKeyboardEventType(key_event.type))
     return;
 
   if (suppress_next_char_events_) {
     // If preceding RawKeyDown event was handled by the browser, then we need
     // suppress all Char events generated by it. Please note that, one
@@ skipping 250 matching lines @@
     bool keep_selection) {
   Send(new InputMsg_ImeConfirmComposition(
         GetRoutingID(), text, replacement_range, keep_selection));
 }
 
 void RenderWidgetHostImpl::ImeCancelComposition() {
   Send(new InputMsg_ImeSetComposition(GetRoutingID(), base::string16(),
             std::vector<blink::WebCompositionUnderline>(), 0, 0));
 }
 
-gfx::Rect RenderWidgetHostImpl::GetRootWindowResizerRect() const {
-  return gfx::Rect();
-}
-
 void RenderWidgetHostImpl::RequestToLockMouse(bool user_gesture,

[ncarter (slow), 2015/10/30 04:08:12]

Maybe inline this function? It's only got one caller; seems it only existed for
the sake of inheritance.

[Avi (use Gerrit), 2015/10/30 20:06:29]

Done.

                                               bool last_unlocked_by_target) {
-  // Directly reject to lock the mouse. Subclass can override this method to
-  // decide whether to allow mouse lock or not.
+  if (delegate_) {
+    delegate_->RequestToLockMouse(user_gesture, last_unlocked_by_target);
+    return;
+  }
+  // If there's no delegate, just reject it.
   GotResponseToLockMouseRequest(false);
 }
 
 void RenderWidgetHostImpl::RejectMouseLockOrUnlockIfNecessary() {
   DCHECK(!pending_mouse_lock_request_ || !IsMouseLocked());
   if (pending_mouse_lock_request_) {
     pending_mouse_lock_request_ = false;
     Send(new ViewMsg_LockMouse_ACK(routing_id_, false));
   } else if (IsMouseLocked()) {
     view_->UnlockMouse();
@@ skipping 37 matching lines @@
 
   delete this;
 }
 
 void RenderWidgetHostImpl::RendererIsUnresponsive() {
   NotificationService::current()->Notify(
       NOTIFICATION_RENDER_WIDGET_HOST_HANG,
       Source<RenderWidgetHost>(this),
       NotificationService::NoDetails());
   is_unresponsive_ = true;
-  NotifyRendererUnresponsive();
+  if (delegate_)
+    delegate_->RendererUnresponsive(this);
 }
 
 void RenderWidgetHostImpl::RendererIsResponsive() {
   if (is_unresponsive_) {
     is_unresponsive_ = false;
-    NotifyRendererResponsive();
+    if (delegate_)
+      delegate_->RendererResponsive(this);
   }
 }
 
 void RenderWidgetHostImpl::ClearDisplayedGraphics() {
   NotifyNewContentRenderingTimeoutForTesting();
   if (view_)
     view_->ClearCompositorFrame();
 }
 
 void RenderWidgetHostImpl::OnRenderProcessGone(int status, int exit_code) {
@@ skipping 393 matching lines @@
   // events are being ignored in order to keep the renderer from getting
   // confused about how many touches are active.
   if (IgnoreInputEvents() && event.type != WebInputEvent::TouchCancel)
     return INPUT_EVENT_ACK_STATE_NO_CONSUMER_EXISTS;
 
   if (!process_->HasConnection())
     return INPUT_EVENT_ACK_STATE_UNKNOWN;
 
   if (event.type == WebInputEvent::MouseDown ||
       event.type == WebInputEvent::GestureTapDown) {
-    OnUserGesture();
+    if (delegate_)
+      delegate_->OnUserGesture();

[ncarter (slow), 2015/10/30 04:08:12]

This is a behavior change for non-view widgets. What are the implications?

For subframe widgets this looks like a bug fix as far as I can tell.

However, for popup widgets (or the fullscreen widget?), are there any
consequences? If there was an original intent to prevent this from happening for
for non-view widgets, do you know why?

   }
 
   if (delegate_) {
     if (event.type == WebInputEvent::MouseDown ||
         event.type == WebInputEvent::GestureTapDown ||
         event.type == WebInputEvent::RawKeyDown) {
       delegate_->OnUserInteraction(event.type);

[ncarter (slow), 2015/10/30 04:08:12]

With the behavior change above, it seems safe to move the work done by
WebContentsImpl::OnUserGesture into WebContentsImpl::OnUserInteraction, and
eliminate the OnUserGesture delegate method altogether.

It looks like dominickn has been looking at this.

[dominickn, 2015/10/30 04:38:41]

I just landed the OnUserInteraction hook earlier this week to propagate all
keypress, click, and gesture taps to WebContentsObservers. We're using this for
the site engagement service to detect user engagement with a site.

I have an in-progress change (not submitted yet) to eliminate *UserGesture in
favour of *UserInteraction, which is more general. The current issue is that the
primary (and original) consumer of *UserGesture  is the download rate limiter,
which explicitly wants the combination of Enter/Space keys, clicks, or taps to
allow an unprompted download. OnUserInteraction deliberately doesn't propagate
the key information so it can be more generalised, while *UserGesture only fires
on that combination of inputs. sky feels quite strongly that "action keys" alone
should be used to allow downloads, rather than arbitrary keys.

One way to ensure the download limiter still responds to only enter and space
may be to have another callback which just propagates keypresses and the
keypress codes. Then the limiter can override that method and OnUserInteraction,
taking enter/space from the former and clicks/taps from the latter. It's uglier
having two methods, but propagating input to the WebContents layer isn't very
clean all round... thoughts welcome on this.

If you're okay with leaving the UserGesture() callbacks in here (or moving them
to WidgetHost), I'm happy to follow up removing them entirely for the
UserInteraction hook.

[Avi (use Gerrit), 2015/10/30 20:06:29]

I'm going to leave them as functionally unchanged as possible, and let you
handle the rest.

     }
   }
 
   return view_ ? view_->FilterInputEvent(event)
                : INPUT_EVENT_ACK_STATE_NOT_CONSUMED;
 }
 
 void RenderWidgetHostImpl::IncrementInFlightEventCount() {
   increment_in_flight_event_count();
   if (!is_hidden_)
@@ skipping 106 matching lines @@
 
 void RenderWidgetHostImpl::OnSyntheticGestureCompleted(
     SyntheticGesture::Result result) {
   Send(new InputMsg_SyntheticGestureCompleted(GetRoutingID()));
 }
 
 bool RenderWidgetHostImpl::IgnoreInputEvents() const {
   return ignore_input_events_ || process_->IgnoreInputEvents();
 }
 
-void RenderWidgetHostImpl::StartUserGesture() {

[ncarter (slow), 2015/10/30 04:08:12]

Nice find.

-  OnUserGesture();
-}
-
 void RenderWidgetHostImpl::SetBackgroundOpaque(bool opaque) {
   Send(new ViewMsg_SetBackgroundOpaque(GetRoutingID(), opaque));
 }
 
 void RenderWidgetHostImpl::SetEditCommandsForNextKeyEvent(
     const std::vector<EditCommand>& commands) {
   Send(new InputMsg_SetEditCommandsForNextKeyEvent(GetRoutingID(), commands));
 }
 
 void RenderWidgetHostImpl::ExecuteEditCommand(const std::string& command,
@@ skipping 60 matching lines @@
 
 void RenderWidgetHostImpl::DelayedAutoResized() {
   gfx::Size new_size = new_auto_size_;
   // Clear the new_auto_size_ since the empty value is used as a flag to
   // indicate that no callback is in progress (i.e. without this line
   // DelayedAutoResized will not get called again).
   new_auto_size_.SetSize(0, 0);
   if (!auto_resize_enabled_)
     return;
 
-  OnRenderAutoResized(new_size);
+  if (delegate_)
+    delegate_->ResizeDueToAutoResize(new_size);

[ncarter (slow), 2015/10/30 04:08:12]

So in theory, right, this would allow non-view widgets to notify the WebContents
of auto-size events. Which would be bad, except that auto resize happens only to
ever be enabled for view widgets. And that's why this is a safe transformation
to make now.

Do I have that right?

 }
 
 void RenderWidgetHostImpl::DetachDelegate() {
   delegate_ = NULL;
 }
 
 void RenderWidgetHostImpl::FrameSwapped(const ui::LatencyInfo& latency_info) {
   ui::LatencyInfo::LatencyComponent window_snapshot_component;
   if (latency_info.FindLatency(ui::WINDOW_SNAPSHOT_FRAME_NUMBER_COMPONENT,
                                GetLatencyComponentId(),
@@ skipping 107 matching lines @@
 }
 
 #if defined(OS_WIN)
 gfx::NativeViewAccessible
     RenderWidgetHostImpl::GetParentNativeViewAccessible() {
   return delegate_ ? delegate_->GetParentNativeViewAccessible() : NULL;
 }
 #endif
 
 }  // namespace content