Move compositor InputHandler from RenderViewImpl to RenderWidget.

third_party/WebKit/Source/web/ChromeClientImpl.cpp

Index: third_party/WebKit/Source/web/ChromeClientImpl.cpp
diff --git a/third_party/WebKit/Source/web/ChromeClientImpl.cpp b/third_party/WebKit/Source/web/ChromeClientImpl.cpp
index 767849621044c8f3201fafa471cfd6b5eb58698d..e89395988146f5f1934958ee43674c91e694c686 100644
--- a/third_party/WebKit/Source/web/ChromeClientImpl.cpp
+++ b/third_party/WebKit/Source/web/ChromeClientImpl.cpp
@@ -919,19 +919,44 @@ bool ChromeClientImpl::shouldOpenModalDialogDuringPageDismissal(
 }
 
 void ChromeClientImpl::setEventListenerProperties(
+    LocalFrame* frame,
     WebEventListenerClass eventClass,
     WebEventListenerProperties properties) {
-  if (WebLayerTreeView* treeView = m_webView->layerTreeView()) {
+  WebLocalFrameImpl* webFrame = WebLocalFrameImpl::fromFrame(frame);
+  if (!webFrame || webFrame->localRoot() == m_webView->mainFrame()) {
+    setEventListenerProperties(m_webView->layerTreeView(), eventClass,
+                               properties);
+  } else {
+    WebFrameWidgetBase* widget = webFrame->localRoot()->frameWidget();
+    // The widget may be nullptr in some unit tests.

[dcheng, 2016/11/10 07:46:11]

How many tests does this break? If possible, we should fix the unit tests (a
followup is OK for this)

[wjmaclean, 2016/11/10 15:57:15]

I realize I missed a case, this isn't just broken in tests. The problem also
occurs when swapping out the main frame. We get the following call stack:

#1 0x7f860487d3f2 blink::ChromeClientImpl::setEventListenerProperties()
#2 0x7f8602266b5f blink::FrameLoader::commitProvisionalLoad()
#3 0x7f860224cd1d blink::DocumentLoader::finishedLoading()
#4 0x7f860224ecf0 blink::DocumentLoader::maybeLoadEmpty()
#5 0x7f860224ede6 blink::DocumentLoader::startLoadingMainResource()
#6 0x7f8602261c0d blink::FrameLoader::init()
#7 0x7f860490d27a blink::WebLocalFrameImpl::createProvisional()
#8 0x7f860490cf86 blink::WebLocalFrame::createProvisional()

Frame #1 is where we encounter the null widget. Frame #2 only calls
setEvenListenerProperties() in order to initialize the value for all three
handler classes to WebEventListenerProperties::Nothing, which in fact is the
*implicit* initialization when the LayerTree for the as-yet-to-be-created
widget, when it is finally created.

It would seem our safe options are:
1) Make the initialization *explicit* so we can rely on it, or
2) Don't allow WebLocalFrameImpl::createProvisional() to call
FrameLoader::init() without first creating a valid widget.

The first option seems easier, though perhaps is better done as a follow on.
This whole issue stems from the fact that event properties seem to have always
been set on the WebViewImpl's LayerTreeView, regardless of which RenderWidget's
LayerTreeView is really being used to draw the page content.

[dcheng, 2016/11/11 08:51:52]

Can you clarify what you mean?
We can't do that =(
I think I'd be OK with just special-casing this to skip this block for
provisional frames.
Also, now that we're tracking this on a per-local root basis, doesn't the
FrameLoader logic need to be updated?

Finally, it's actually not clear to me what the purpose of the call in
FrameLoader is: I would kind of expect the EventHandlerRegistry to end up in the
Nothing state just by virtue of detaching the previously attached documents.
Does that not happen?

[wjmaclean, 2016/11/11 15:40:51]

The event listener properties are stored as an array of EventListenerProperties,
which is itself an enum with a first value (0) of 'None'

https://cs.chromium.org/chromium/src/cc/trees/layer_tree.h?rcl=1478852525&l=275

This is initialized to zero, which means 'None' in terms of the enum. See
https://cs.chromium.org/chromium/src/cc/trees/layer_tree.cc?rcl=1478852525&l=58

But it would be nice if the initialization was a little clearer. Or maybe it
doesn't matter?

I guess in the provisional case, if we assume we are always going to create a
widget, and thus initialize a new set of event listener properties, then we can
just skip the block you mention below.
Sure, I'm not surprised.
Ok. But do you mean just delete this block entirely, or make it conditional on
something (and if so, what?).
I don't understand? We're just concerned about setting the event listener
properties, which is what we are attempting to update here.
I don't know anything about 'detaching' at this point in time. But yes, that
seems like it would be a reasonable thing to do on detach.

[dcheng, 2016/11/11 22:32:11]

I don't think we can expose that to the layer tree, unfortunately.
Right, but don't we need to track this on a per-local root basis now? The
current block is conditional on if it's a main frame.
Is it possible to verify if that already happens? Or is it possible to attach an
event listener somehow?

+    if (!widget) {
+      LOG(INFO) << "Null frameWidget.";

[dcheng, 2016/11/10 07:46:11]

Nit: remove this LOG

[wjmaclean, 2016/11/10 15:57:15]

Done.

+      return;
+    }
+
+    WebWidgetClient* client = widget->client();
+    // TODO(wjmaclean): Do we need to inform the WebView about these also?
+    setEventListenerProperties(client->layerTreeView(), eventClass, properties);
+  }
+}
+
+void ChromeClientImpl::setEventListenerProperties(
+    WebLayerTreeView* treeView,
+    WebEventListenerClass eventClass,
+    WebEventListenerProperties properties) {
+  if (treeView) {
     treeView->setEventListenerProperties(eventClass, properties);
     if (eventClass == WebEventListenerClass::TouchStartOrMove) {
       m_webView->hasTouchEventHandlers(
           properties != WebEventListenerProperties::Nothing ||
-          eventListenerProperties(WebEventListenerClass::TouchEndOrCancel) !=
+          eventListenerProperties(treeView,
+                                  WebEventListenerClass::TouchEndOrCancel) !=
               WebEventListenerProperties::Nothing);
     } else if (eventClass == WebEventListenerClass::TouchEndOrCancel) {
       m_webView->hasTouchEventHandlers(
           properties != WebEventListenerProperties::Nothing ||
-          eventListenerProperties(WebEventListenerClass::TouchStartOrMove) !=
+          eventListenerProperties(treeView,
+                                  WebEventListenerClass::TouchStartOrMove) !=
               WebEventListenerProperties::Nothing);
     }
   } else {
@@ -947,8 +972,21 @@ void ChromeClientImpl::beginLifecycleUpdates() {
 }
 
 WebEventListenerProperties ChromeClientImpl::eventListenerProperties(
+    LocalFrame* frame,
     WebEventListenerClass eventClass) const {
-  if (WebLayerTreeView* treeView = m_webView->layerTreeView())
+  WebLocalFrameImpl* webFrame = WebLocalFrameImpl::fromFrame(frame);
+  if (!webFrame || webFrame->localRoot() == m_webView->mainFrame())
+    return eventListenerProperties(m_webView->layerTreeView(), eventClass);
+
+  WebWidgetClient* client =
+      WebLocalFrameImpl::fromFrame(frame)->localRoot()->frameWidget()->client();
+  return eventListenerProperties(client->layerTreeView(), eventClass);
+}
+
+WebEventListenerProperties ChromeClientImpl::eventListenerProperties(
+    WebLayerTreeView* treeView,
+    WebEventListenerClass eventClass) const {
+  if (treeView)
     return treeView->eventListenerProperties(eventClass);
   return WebEventListenerProperties::Nothing;
 }