Move compositor InputHandler from RenderViewImpl to RenderWidget.

Move compositor InputHandler from RenderViewImpl to RenderWidget.

At present the input handler for cross-process iframes is installed 
only on the RenderViewImpl, meaning that events sent to them can only
be handled on the main thread (instead of the compositor thread).

Note: The compositor layer tree on the RenderViewImpl for a cross-
process frame renderer is actually empty.

This CL attaches an input handler to each RenderWidget instead, 
allowing compositor-thread handling of input events for all
the frames. This is a precursor for handling GesturePinch events for
subframes.

TBR=alexmos@chromium.org
BUG=654917
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2479663002
Cr-Commit-Position: refs/heads/master@{#445295}
Committed: https://chromium.googlesource.com/chromium/src/+/1d97062b110831423488e9bc5bd67a1bde1fded5

[wjmaclean, 2016-11-03 20:00:50]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 20:02:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[wjmaclean, 2016-11-03 20:05:18]

Description was changed from

==========
Move compositor InputHandler from RenderViewImpl to RenderWidget.

At present the input handler for cross-process iframes is installed only
on the RenderWidgetImpl, meaning that events sent to them can only be
handled on the min thread (instead of the compositor thread).

The compositor layer tree on the RenderViewImpl for a cross-process
frame renderer is actually empty.

This CL attaches an input handler to each RenderWidget instead, allowing
compositor-thread handling of input events. This is a precursor for
handling GesturePinch events for subframes.

BUG=654917
==========

to

==========
Move compositor InputHandler from RenderViewImpl to RenderWidget.

At present the input handler for cross-process iframes is installed 
only on the RenderViewImpl, meaning that events sent to them can only
be handled on the min thread (instead of the compositor thread).

The compositor layer tree on the RenderViewImpl for a cross-process
frame renderer is actually empty.

This CL attaches an input handler to each RenderWidget instead, 
allowing compositor-thread handling of input events. This is a
precursor for handling GesturePinch events for subframes.

BUG=654917
==========

[wjmaclean, 2016-11-03 20:22:24]

Description was changed from

==========
Move compositor InputHandler from RenderViewImpl to RenderWidget.

At present the input handler for cross-process iframes is installed 
only on the RenderViewImpl, meaning that events sent to them can only
be handled on the min thread (instead of the compositor thread).

The compositor layer tree on the RenderViewImpl for a cross-process
frame renderer is actually empty.

This CL attaches an input handler to each RenderWidget instead, 
allowing compositor-thread handling of input events. This is a
precursor for handling GesturePinch events for subframes.

BUG=654917
==========

to

==========
Move compositor InputHandler from RenderViewImpl to RenderWidget.

At present the input handler for cross-process iframes is installed 
only on the RenderViewImpl, meaning that events sent to them can only
be handled on the main thread (instead of the compositor thread).

Note: The compositor layer tree on the RenderViewImpl for a cross-
process frame renderer is actually empty.

This CL attaches an input handler to each RenderWidget instead, 
allowing compositor-thread handling of input events for all
the frames. This is a precursor for handling GesturePinch events for
subframes.

BUG=654917
==========

[commit-bot: I haz the power, 2016-11-03 20:37:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-03 20:37:47]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[wjmaclean, 2016-11-03 21:54:16]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 21:54:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-03 23:39:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-03 23:39:13]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[wjmaclean, 2016-11-04 14:42:34]

wjmaclean@chromium.org changed reviewers:
+ lfg@chromium.org

[wjmaclean, 2016-11-04 14:43:42]

lfg@ - Would you like to give this a first look-over?

[wjmaclean, 2016-11-04 14:45:09]

wjmaclean@chromium.org changed reviewers:
+ dcheng@chromium.org

[wjmaclean, 2016-11-04 14:45:38]

dcheng@ - Could you please look over the blink parts?

[lfg, 2016-11-04 17:22:05]

Overall looks good.

https://codereview.chromium.org/2479663002/diff/60001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2479663002/diff/60001/content/renderer/render...
content/renderer/render_widget.cc:1145: routing_id_,
compositor()->GetInputHandler(), AsWeakPtr(), true);
We were using webkit_preferences_.enable_scroll_animator in RenderView, won't
this cause issues (like people not being able to disable smooth scrolling?)

[wjmaclean, 2016-11-04 17:27:52]

https://codereview.chromium.org/2479663002/diff/60001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2479663002/diff/60001/content/renderer/render...
content/renderer/render_widget.cc:1145: routing_id_,
compositor()->GetInputHandler(), AsWeakPtr(), true);
On 2016/11/04 17:22:05, lfg wrote:
> We were using webkit_preferences_.enable_scroll_animator in RenderView, won't
> this cause issues (like people not being able to disable smooth scrolling?)

I'll plumb that through from RenderViewImpl in the next version of the patch.

[wjmaclean, 2016-11-04 17:44:52]

On 2016/11/04 17:27:52, wjmaclean wrote:
>
https://codereview.chromium.org/2479663002/diff/60001/content/renderer/render...
> File content/renderer/render_widget.cc (right):
> 
>
https://codereview.chromium.org/2479663002/diff/60001/content/renderer/render...
> content/renderer/render_widget.cc:1145: routing_id_,
> compositor()->GetInputHandler(), AsWeakPtr(), true);
> On 2016/11/04 17:22:05, lfg wrote:
> > We were using webkit_preferences_.enable_scroll_animator in RenderView,
won't
> > this cause issues (like people not being able to disable smooth scrolling?)
> 
> I'll plumb that through from RenderViewImpl in the next version of the patch.

lfg@ - it's ok to get the RVI using the RW's routing_id_, is that right?

[wjmaclean, 2016-11-04 18:01:03]

wjmaclean@chromium.org changed reviewers:
+ avi@chromium.org

[wjmaclean, 2016-11-04 18:01:38]

avi@ - do you have time to do an OWNER's review for the content/ parts of this
CL?

[Avi (use Gerrit), 2016-11-04 19:53:33]

The trybots don't seem to like this patch. :(

[dcheng, 2016-11-04 21:00:02]

https://codereview.chromium.org/2479663002/diff/60001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2479663002/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:5250:
std::unique_ptr<MainThreadFrameObserver> observer(
Nit: auto observer = base::MakeUnique<MTFO>(...);

https://codereview.chromium.org/2479663002/diff/60001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2479663002/diff/60001/content/renderer/render...
content/renderer/render_widget.cc:206: const size_t
kContentIntentDelayMilliseconds = 700;
constexpr base::TimeDelta kContentIntentDelay =
base::TimeDelta::FromMilliseconds(700);

https://codereview.chromium.org/2479663002/diff/60001/content/renderer/render...
content/renderer/render_widget.cc:1647:
static_cast<RenderWidget*>(this)->AsWeakPtr(),
Why do we need to do this cast?

https://codereview.chromium.org/2479663002/diff/60001/content/renderer/render...
File content/renderer/render_widget.h (right):

https://codereview.chromium.org/2479663002/diff/60001/content/renderer/render...
content/renderer/render_widget.h:124: public
base::SupportsWeakPtr<RenderWidget>,
Let's embed a WeakPtrFactory instead.

https://codereview.chromium.org/2479663002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp (right):

https://codereview.chromium.org/2479663002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp:125: frame =
node->document().frame();
Do we not care if the EventTarget is a non-node type?

https://codereview.chromium.org/2479663002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2479663002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1329: // be available.
Can we switch the order of initialization in CreateMainFrame so this case can't
happen?

https://codereview.chromium.org/2479663002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebPagePopupImpl.cpp (right):

https://codereview.chromium.org/2479663002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebPagePopupImpl.cpp:169:
eventListenerProperties(nullptr,
Why nullptr instead of passing through the LocalFrame argument?

[wjmaclean, 2016-11-07 15:55:54]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-07 15:56:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[wjmaclean, 2016-11-07 15:56:48]

On 2016/11/04 19:53:33, Avi wrote:
> The trybots don't seem to like this patch. :(

Sorry, that patch contained an experiment that, as it turns out, failed. I have
corrected it.

[wjmaclean, 2016-11-07 15:57:27]

dcheng@ - I've done my best to address you questions, but have a couple of me
own ... please see below.

https://codereview.chromium.org/2479663002/diff/60001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2479663002/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:5250:
std::unique_ptr<MainThreadFrameObserver> observer(
On 2016/11/04 21:00:02, dcheng wrote:
> Nit: auto observer = base::MakeUnique<MTFO>(...);

Done.

https://codereview.chromium.org/2479663002/diff/60001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2479663002/diff/60001/content/renderer/render...
content/renderer/render_widget.cc:206: const size_t
kContentIntentDelayMilliseconds = 700;
On 2016/11/04 21:00:02, dcheng wrote:
> constexpr base::TimeDelta kContentIntentDelay =
> base::TimeDelta::FromMilliseconds(700);

Done.

https://codereview.chromium.org/2479663002/diff/60001/content/renderer/render...
content/renderer/render_widget.cc:1647:
static_cast<RenderWidget*>(this)->AsWeakPtr(),
On 2016/11/04 21:00:02, dcheng wrote:
> Why do we need to do this cast?

We don't ... stale cruft. Fixed.

https://codereview.chromium.org/2479663002/diff/60001/content/renderer/render...
File content/renderer/render_widget.h (right):

https://codereview.chromium.org/2479663002/diff/60001/content/renderer/render...
content/renderer/render_widget.h:124: public
base::SupportsWeakPtr<RenderWidget>,
On 2016/11/04 21:00:02, dcheng wrote:
> Let's embed a WeakPtrFactory instead.

Done.

https://codereview.chromium.org/2479663002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp (right):

https://codereview.chromium.org/2479663002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp:125: frame =
node->document().frame();
On 2016/11/04 21:00:02, dcheng wrote:
> Do we not care if the EventTarget is a non-node type?

I don't think so. In the past this has always been applied, by ChromeClientImpl,
to the mainFrame, which (I'm assuming) will have a Node representation. I'm not
sure what this would mean if EventTarget wasn't a Node ... is there a case for
that?

https://codereview.chromium.org/2479663002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2479663002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1329: // be available.
On 2016/11/04 21:00:02, dcheng wrote:
> Can we switch the order of initialization in CreateMainFrame so this case
can't
> happen?

It's possible we can switch the order, though I don't know for sure yet. If we
*do* switch the order, might it be safe to try that in a follow-on CL, one that
is a bit smaller and easier to reason about?

https://codereview.chromium.org/2479663002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebPagePopupImpl.cpp (right):

https://codereview.chromium.org/2479663002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebPagePopupImpl.cpp:169:
eventListenerProperties(nullptr,
On 2016/11/04 21:00:02, dcheng wrote:
> Why nullptr instead of passing through the LocalFrame argument?

It forces setting the properties on the RenderView's LayerTree, which I suspect
best matches legacy behaviour here.

[Avi (use Gerrit), 2016-11-07 16:15:55]

I'm not up to speed here, but this looks as reasonable as I can get my head
around. LGTM stamp for when dcheng and lfg are happy.

https://codereview.chromium.org/2479663002/diff/100001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2479663002/diff/100001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:5312: // Verify touch handler in
subframe was invoked
nit: period at end of sentence

https://codereview.chromium.org/2479663002/diff/100001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/2479663002/diff/100001/content/renderer/rende...
content/renderer/render_view_impl.cc:1985: // expect the RenderWidget version of
the function to be called instead.
Soon. Soon. 🤞

[commit-bot: I haz the power, 2016-11-07 17:14:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-07 17:14:24]

Dry run: This issue passed the CQ dry run.

[wjmaclean, 2016-11-07 17:43:17]

https://codereview.chromium.org/2479663002/diff/100001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2479663002/diff/100001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:5312: // Verify touch handler in
subframe was invoked
On 2016/11/07 16:15:55, Avi wrote:
> nit: period at end of sentence

Done.

https://codereview.chromium.org/2479663002/diff/100001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/2479663002/diff/100001/content/renderer/rende...
content/renderer/render_view_impl.cc:1985: // expect the RenderWidget version of
the function to be called instead.
On 2016/11/07 16:15:55, Avi wrote:
> Soon. Soon. 🤞

Acknowledged.

[dcheng, 2016-11-08 18:11:01]

https://codereview.chromium.org/2479663002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp (right):

https://codereview.chromium.org/2479663002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp:125: frame =
node->document().frame();
On 2016/11/07 15:57:27, wjmaclean wrote:
> On 2016/11/04 21:00:02, dcheng wrote:
> > Do we not care if the EventTarget is a non-node type?
> 
> I don't think so. In the past this has always been applied, by
ChromeClientImpl,
> to the mainFrame, which (I'm assuming) will have a Node representation. I'm
not
> sure what this would mean if EventTarget wasn't a Node ... is there a case for
> that?

I did some research, and this is used for LocalDOMWindow as well. Do we need to
handle that case?

https://codereview.chromium.org/2479663002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2479663002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1329: // be available.
On 2016/11/07 15:57:27, wjmaclean wrote:
> On 2016/11/04 21:00:02, dcheng wrote:
> > Can we switch the order of initialization in CreateMainFrame so this case
> can't
> > happen?
> 
> It's possible we can switch the order, though I don't know for sure yet. If we
> *do* switch the order, might it be safe to try that in a follow-on CL, one
that
> is a bit smaller and easier to reason about?

Can we investigate doing this cleanup preemptively? We've been trying to
rationalize when things can be null in Blink: as you've experienced, it's far
too arbitrary at the moment. =)

https://codereview.chromium.org/2479663002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebPagePopupImpl.cpp (right):

https://codereview.chromium.org/2479663002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebPagePopupImpl.cpp:169:
eventListenerProperties(nullptr,
On 2016/11/07 15:57:27, wjmaclean wrote:
> On 2016/11/04 21:00:02, dcheng wrote:
> > Why nullptr instead of passing through the LocalFrame argument?
> 
> It forces setting the properties on the RenderView's LayerTree, which I
suspect
> best matches legacy behaviour here.

Right, but shouldn't the LocalFrame argument here always be the main frame
anyway?

[wjmaclean, 2016-11-09 18:01:48]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-09 18:02:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[wjmaclean, 2016-11-09 18:09:38]

dcheng@ - PTAL?

https://codereview.chromium.org/2479663002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp (right):

https://codereview.chromium.org/2479663002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp:125: frame =
node->document().frame();
On 2016/11/08 18:11:00, dcheng wrote:
> On 2016/11/07 15:57:27, wjmaclean wrote:
> > On 2016/11/04 21:00:02, dcheng wrote:
> > > Do we not care if the EventTarget is a non-node type?
> > 
> > I don't think so. In the past this has always been applied, by
> ChromeClientImpl,
> > to the mainFrame, which (I'm assuming) will have a Node representation. I'm
> not
> > sure what this would mean if EventTarget wasn't a Node ... is there a case
for
> > that?
> 
> I did some research, and this is used for LocalDOMWindow as well. Do we need
to
> handle that case?

I suppose we could always fall back to the main frame in that case. The only
question is, if the main frame isn't local, then the old codepath falls back to
the render view for the remote main frame ... I assume we would want to do the
same in this case? It might be worth experimenting with *not* doing that though
...

https://codereview.chromium.org/2479663002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2479663002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1329: // be available.
On 2016/11/08 18:11:00, dcheng wrote:
> On 2016/11/07 15:57:27, wjmaclean wrote:
> > On 2016/11/04 21:00:02, dcheng wrote:
> > > Can we switch the order of initialization in CreateMainFrame so this case
> > can't
> > > happen?
> > 
> > It's possible we can switch the order, though I don't know for sure yet. If
we
> > *do* switch the order, might it be safe to try that in a follow-on CL, one
> that
> > is a bit smaller and easier to reason about?
> 
> Can we investigate doing this cleanup preemptively? We've been trying to
> rationalize when things can be null in Blink: as you've experienced, it's far
> too arbitrary at the moment. =)

Actually, as I recall, this only seems to have been an issue in unit tests. I
checked this out further, and in those tests, the frameWidget is *never* created
it seems, so it's not something we need to change (other than handling the null
pointer case). I added a check in ChromeClientImpl that includes a LOG message
(I'm open to more useful wording for that message if you have any suggestions).

https://codereview.chromium.org/2479663002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebPagePopupImpl.cpp (right):

https://codereview.chromium.org/2479663002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebPagePopupImpl.cpp:169:
eventListenerProperties(nullptr,
On 2016/11/08 18:11:00, dcheng wrote:
> On 2016/11/07 15:57:27, wjmaclean wrote:
> > On 2016/11/04 21:00:02, dcheng wrote:
> > > Why nullptr instead of passing through the LocalFrame argument?
> > 
> > It forces setting the properties on the RenderView's LayerTree, which I
> suspect
> > best matches legacy behaviour here.
> 
> Right, but shouldn't the LocalFrame argument here always be the main frame
> anyway?

Ok. I've added a DCHECK to alert us if it *isn't* the main frame.

[commit-bot: I haz the power, 2016-11-09 19:49:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-09 19:49:33]

Dry run: This issue passed the CQ dry run.

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

https://codereview.chromium.org/2479663002/diff/160001/content/renderer/rende...
File content/renderer/render_widget.h (right):

https://codereview.chromium.org/2479663002/diff/160001/content/renderer/rende...
content/renderer/render_widget.h:534: void
scheduleContentIntentForRenderView(const blink::WebURL& intent,
Can you help me understand why this is needed? Since RenderViewImpl can call
this, can't it just call LaunchAndroidContentIntent directly?

I guess the problem is it needs a WeakPtr, but we can just a WeakPtrFactory to
RenderViewImpl, right?

https://codereview.chromium.org/2479663002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp (right):

https://codereview.chromium.org/2479663002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp:127: frame =
node->document().frame();
My reading of this is we'll need to handle the LocalDOMWindow case a well: we
can just get the frame from the LocalDOMWindow and plumb that through, right?

(We should probably also assert that those are the only EventTarget types we're
handling)

https://codereview.chromium.org/2479663002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/2479663002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:931: // The widget may be
nullptr in some unit tests.
How many tests does this break? If possible, we should fix the unit tests (a
followup is OK for this)

https://codereview.chromium.org/2479663002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:933: LOG(INFO) << "Null
frameWidget.";
Nit: remove this LOG

[wjmaclean, 2016-11-10 15:56:50]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-10 15:57:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

dcheng@ - PTAL?

https://codereview.chromium.org/2479663002/diff/160001/content/renderer/rende...
File content/renderer/render_widget.h (right):

https://codereview.chromium.org/2479663002/diff/160001/content/renderer/rende...
content/renderer/render_widget.h:534: void
scheduleContentIntentForRenderView(const blink::WebURL& intent,
On 2016/11/10 07:46:11, dcheng wrote:
> Can you help me understand why this is needed? Since RenderViewImpl can call
> this, can't it just call LaunchAndroidContentIntent directly?
> 
> I guess the problem is it needs a WeakPtr, but we can just a WeakPtrFactory to
> RenderViewImpl, right?

Yes, the weak pointer type was the issue. This is from an earlier version of the
CL where RVI was deriving from SupportsWeakPtr, which was incompatible with RW
also doing that. As long as RVI and RW can *both* have a WeakPtr factory, then I
think we can remove this change.

https://codereview.chromium.org/2479663002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp (right):

https://codereview.chromium.org/2479663002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp:127: frame =
node->document().frame();
On 2016/11/10 07:46:11, dcheng wrote:
> My reading of this is we'll need to handle the LocalDOMWindow case a well: we
> can just get the frame from the LocalDOMWindow and plumb that through, right?
> 
> (We should probably also assert that those are the only EventTarget types
we're
> handling)

Done. (DCHECK'd as ASSERT is deprecated.)

https://codereview.chromium.org/2479663002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/2479663002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:931: // The widget may be
nullptr in some unit tests.
On 2016/11/10 07:46:11, dcheng wrote:
> How many tests does this break? If possible, we should fix the unit tests (a
> followup is OK for this)

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.

https://codereview.chromium.org/2479663002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:933: LOG(INFO) << "Null
frameWidget.";
On 2016/11/10 07:46:11, dcheng wrote:
> Nit: remove this LOG

Done.

[commit-bot: I haz the power, 2016-11-10 16:00:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-10 16:00:07]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[wjmaclean, 2016-11-10 16:18:05]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-10 16:18:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[wjmaclean, 2016-11-10 16:35:15]

Patchset #11 (id:200001) has been deleted

[commit-bot: I haz the power, 2016-11-10 16:36:28]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[wjmaclean, 2016-11-10 16:56:22]

Patchset #11 (id:220001) has been deleted

[wjmaclean, 2016-11-10 17:29:12]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-10 17:29:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-10 19:08:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-10 19:08:16]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

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

https://codereview.chromium.org/2479663002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/2479663002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:931: // The widget may be
nullptr in some unit tests.
On 2016/11/10 15:57:15, wjmaclean wrote:
> On 2016/11/10 07:46:11, dcheng wrote:
> > How many tests does this break? If possible, we should fix the unit tests (a
> > followup is OK for this)
> 
> 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

Can you clarify what you mean?

> 2) Don't allow WebLocalFrameImpl::createProvisional() to call
> FrameLoader::init() without first creating a valid widget.

We can't do that =(

> 
> 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.

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 14:39:41]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 14:39:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

https://codereview.chromium.org/2479663002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp (right):

https://codereview.chromium.org/2479663002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp:127: frame =
node->document().frame();
On 2016/11/10 15:57:15, wjmaclean wrote:
> On 2016/11/10 07:46:11, dcheng wrote:
> > My reading of this is we'll need to handle the LocalDOMWindow case a well:
we
> > can just get the frame from the LocalDOMWindow and plumb that through,
right?
> > 
> > (We should probably also assert that those are the only EventTarget types
> we're
> > handling)
> 
> Done. (DCHECK'd as ASSERT is deprecated.)

Hmmm, that didn't work. It seems we must have more than just Node and
LocalDOMWindow coming through here.

https://codereview.chromium.org/2479663002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/2479663002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:931: // The widget may be
nullptr in some unit tests.
On 2016/11/11 08:51:52, dcheng wrote:
> On 2016/11/10 15:57:15, wjmaclean wrote:
> > On 2016/11/10 07:46:11, dcheng wrote:
> > > How many tests does this break? If possible, we should fix the unit tests
(a
> > > followup is OK for this)
> > 
> > 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
> 
> Can you clarify what you mean?

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.

> > 2) Don't allow WebLocalFrameImpl::createProvisional() to call
> > FrameLoader::init() without first creating a valid widget.
> 
> We can't do that =(

Sure, I'm not surprised.

> > 
> > 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.
> 
> I think I'd be OK with just special-casing this to skip this block for
> provisional frames.

Ok. But do you mean just delete this block entirely, or make it conditional on
something (and if so, what?).

> Also, now that we're tracking this on a per-local root basis, doesn't the
> FrameLoader logic need to be updated?

I don't understand? We're just concerned about setting the event listener
properties, which is what we are attempting to update here.

> 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?

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.

[commit-bot: I haz the power, 2016-11-11 17:34:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-11 17:34:43]

Dry run: This issue passed the CQ dry run.

[wjmaclean, 2016-11-11 18:39:54]

> On 2016/11/10 15:57:15, wjmaclean wrote:
> > On 2016/11/10 07:46:11, dcheng wrote:
> > 
> > I think I'd be OK with just special-casing this to skip this block for
> > provisional frames.
> 
> Ok. But do you mean just delete this block entirely, or make it conditional on
> something (and if so, what?).

Just to be clear; the entire function this appears in is doing a provisional
load, so if we're just skipping it for provisional frames, does that mean take
it out entirely? Or is there another level of check required?

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

On 2016/11/11 18:39:54, wjmaclean wrote:
> > On 2016/11/10 15:57:15, wjmaclean wrote:
> > > On 2016/11/10 07:46:11, dcheng wrote:
> > > 
> > > I think I'd be OK with just special-casing this to skip this block for
> > > provisional frames.
> > 
> > Ok. But do you mean just delete this block entirely, or make it conditional
on
> > something (and if so, what?).
> 
> Just to be clear; the entire function this appears in is doing a provisional
> load, so if we're just skipping it for provisional frames, does that mean take
> it out entirely? Or is there another level of check required?

I think we would just skip that specific block (to reset event listener state)
if it's a provisional frame (we still have to init()).

https://codereview.chromium.org/2479663002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp (right):

https://codereview.chromium.org/2479663002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp:127: frame =
node->document().frame();
On 2016/11/11 15:40:51, wjmaclean wrote:
> On 2016/11/10 15:57:15, wjmaclean wrote:
> > On 2016/11/10 07:46:11, dcheng wrote:
> > > My reading of this is we'll need to handle the LocalDOMWindow case a well:
> we
> > > can just get the frame from the LocalDOMWindow and plumb that through,
> right?
> > > 
> > > (We should probably also assert that those are the only EventTarget types
> > we're
> > > handling)
> > 
> > Done. (DCHECK'd as ASSERT is deprecated.)
> 
> Hmmm, that didn't work. It seems we must have more than just Node and
> LocalDOMWindow coming through here.

Does the debugger know what type of EventTarget is getting passed in here? It'd
be good to see what else we're not covering.

https://codereview.chromium.org/2479663002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/2479663002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:931: // The widget may be
nullptr in some unit tests.
On 2016/11/11 15:40:51, wjmaclean wrote:
> On 2016/11/11 08:51:52, dcheng wrote:
> > On 2016/11/10 15:57:15, wjmaclean wrote:
> > > On 2016/11/10 07:46:11, dcheng wrote:
> > > > How many tests does this break? If possible, we should fix the unit
tests
> (a
> > > > followup is OK for this)
> > > 
> > > 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
> > 
> > Can you clarify what you mean?
> 
> 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 don't think we can expose that to the layer tree, unfortunately.

> 
> 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.
> 
> > > 2) Don't allow WebLocalFrameImpl::createProvisional() to call
> > > FrameLoader::init() without first creating a valid widget.
> > 
> > We can't do that =(
> 
> Sure, I'm not surprised.
> 
> > > 
> > > 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.
> > 
> > I think I'd be OK with just special-casing this to skip this block for
> > provisional frames.
> 
> Ok. But do you mean just delete this block entirely, or make it conditional on
> something (and if so, what?).
> 
> > Also, now that we're tracking this on a per-local root basis, doesn't the
> > FrameLoader logic need to be updated?
> 
> I don't understand? We're just concerned about setting the event listener
> properties, which is what we are attempting to update here.

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.

> 
> > 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?
> 
> 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.

Is it possible to verify if that already happens? Or is it possible to attach an
event listener somehow?

[wjmaclean, 2016-11-22 17:03:20]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-22 17:03:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-22 17:54:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-22 17:54:32]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[wjmaclean, 2016-12-02 02:43:07]

Description was changed from

==========
Move compositor InputHandler from RenderViewImpl to RenderWidget.

At present the input handler for cross-process iframes is installed 
only on the RenderViewImpl, meaning that events sent to them can only
be handled on the main thread (instead of the compositor thread).

Note: The compositor layer tree on the RenderViewImpl for a cross-
process frame renderer is actually empty.

This CL attaches an input handler to each RenderWidget instead, 
allowing compositor-thread handling of input events for all
the frames. This is a precursor for handling GesturePinch events for
subframes.

BUG=654917
==========

to

==========
Move compositor InputHandler from RenderViewImpl to RenderWidget.

At present the input handler for cross-process iframes is installed 
only on the RenderViewImpl, meaning that events sent to them can only
be handled on the main thread (instead of the compositor thread).

Note: The compositor layer tree on the RenderViewImpl for a cross-
process frame renderer is actually empty.

This CL attaches an input handler to each RenderWidget instead, 
allowing compositor-thread handling of input events for all
the frames. This is a precursor for handling GesturePinch events for
subframes.

BUG=654917
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[wjmaclean, 2016-12-02 18:43:29]

Patchset #13 (id:280001) has been deleted

[wjmaclean, 2017-01-06 21:06:46]

Patchset #19 (id:420001) has been deleted

[wjmaclean, 2017-01-09 17:47:43]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-09 17:47:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-09 19:19:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-09 19:19:53]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[wjmaclean, 2017-01-11 18:09:37]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 18:10:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 18:13:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-11 18:13:50]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[wjmaclean, 2017-01-11 22:00:20]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 22:02:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 22:59:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-11 22:59:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[wjmaclean, 2017-01-12 14:28:36]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 14:28:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[wjmaclean, 2017-01-12 14:29:29]

Patchset #13 (id:300001) has been deleted

[wjmaclean, 2017-01-12 14:30:02]

Patchset #13 (id:320001) has been deleted

[wjmaclean, 2017-01-12 14:30:24]

Patchset #14 (id:360001) has been deleted

[commit-bot: I haz the power, 2017-01-12 15:58:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-12 15:58:28]

Dry run: This issue passed the CQ dry run.

[wjmaclean, 2017-01-12 16:38:09]

dcheng@ - Ok, I think I've resolved the issues we talked about ... could you
please give this a look over?

Specific issues:

- I can confirm that Node and LocalDOMWindow are the only targets we're seeing

- We need to keep the calls to setEventListenerProperties() in
FrameLoader::commitProvisionalLoad() since, even though they're setting the
property to 'None', they trigger the sending of ViewHostMsg_HasTouchHandlers to
the browser, which is important as it informs InputRouterImpl *not* to assume a
default of 'true' for the presence of handlers.

- I've modified the condition on making these calls to include OOPIF local-roots
as well, to match the behaviour of local mainframes

[dcheng, 2017-01-16 11:05:41]

https://codereview.chromium.org/2479663002/diff/580001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2479663002/diff/580001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:5448:
base::MakeUnique<MainThreadFrameObserver>(render_widget_host);
Nit: this can just be directly on the stack:

MainThreadFrameObserver observer(render_widget_host);
observer.Wait();

(Similar comment for other similar constructs)

https://codereview.chromium.org/2479663002/diff/580001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp (right):

https://codereview.chromium.org/2479663002/diff/580001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp:129: DCHECK(false)
<< "Unexpected target type for event handler.";
Nit: NOTREACHED()

https://codereview.chromium.org/2479663002/diff/580001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2479663002/diff/580001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1349: if
((isLoadingMainFrame() && m_frame->page()->mainFrame()->isLocalFrame()) ||
This condition can be simplified into m_frame->isLocalRoot(), as being a main
frame implies being a local root.

https://codereview.chromium.org/2479663002/diff/580001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/2479663002/diff/580001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:961: DCHECK(widget);
In Blink, prefer to pass by mutable reference to indicate this, rather than
DCHECKing.

https://codereview.chromium.org/2479663002/diff/580001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:999: if (!widget)
I guess this can also be called on provisional frames / when a frame is
detaching?

https://codereview.chromium.org/2479663002/diff/580001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:1004:
WebEventListenerProperties ChromeClientImpl::eventListenerProperties(
Would it make sense to fold this into eventListenerProperties(LocalFrame*?)

https://codereview.chromium.org/2479663002/diff/580001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/FrameTestHelpers.cpp (right):

https://codereview.chromium.org/2479663002/diff/580001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/FrameTestHelpers.cpp:334: m_layerTreeView =
WTF::wrapUnique(new WebLayerTreeViewImplForTesting);
Prefer WTF::makeUnique now that we have that.

https://codereview.chromium.org/2479663002/diff/580001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebViewTest.cpp (right):

https://codereview.chromium.org/2479663002/diff/580001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebViewTest.cpp:3391: DCHECK(layerTreeView);
Nit: Prefer ASSERT in tests

[wjmaclean, 2017-01-17 18:23:28]

dcheng@ - ptal?

https://codereview.chromium.org/2479663002/diff/580001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2479663002/diff/580001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:5448:
base::MakeUnique<MainThreadFrameObserver>(render_widget_host);
On 2017/01/16 11:05:40, dcheng wrote:
> Nit: this can just be directly on the stack:
> 
> MainThreadFrameObserver observer(render_widget_host);
> observer.Wait();
> 
> (Similar comment for other similar constructs)

Done.

https://codereview.chromium.org/2479663002/diff/580001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp (right):

https://codereview.chromium.org/2479663002/diff/580001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp:129: DCHECK(false)
<< "Unexpected target type for event handler.";
On 2017/01/16 11:05:40, dcheng wrote:
> Nit: NOTREACHED()

Done.

https://codereview.chromium.org/2479663002/diff/580001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2479663002/diff/580001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1349: if
((isLoadingMainFrame() && m_frame->page()->mainFrame()->isLocalFrame()) ||
On 2017/01/16 11:05:40, dcheng wrote:
> This condition can be simplified into m_frame->isLocalRoot(), as being a main
> frame implies being a local root.

Done.

https://codereview.chromium.org/2479663002/diff/580001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/2479663002/diff/580001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:961: DCHECK(widget);
On 2017/01/16 11:05:41, dcheng wrote:
> In Blink, prefer to pass by mutable reference to indicate this, rather than
> DCHECKing.

Done.

https://codereview.chromium.org/2479663002/diff/580001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:999: if (!widget)
On 2017/01/16 11:05:41, dcheng wrote:
> I guess this can also be called on provisional frames / when a frame is
> detaching?

I assume so ... we do see null widgets getting in here.

https://codereview.chromium.org/2479663002/diff/580001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:1004:
WebEventListenerProperties ChromeClientImpl::eventListenerProperties(
On 2017/01/16 11:05:40, dcheng wrote:
> Would it make sense to fold this into eventListenerProperties(LocalFrame*?)

Done.

https://codereview.chromium.org/2479663002/diff/580001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/FrameTestHelpers.cpp (right):

https://codereview.chromium.org/2479663002/diff/580001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/FrameTestHelpers.cpp:334: m_layerTreeView =
WTF::wrapUnique(new WebLayerTreeViewImplForTesting);
On 2017/01/16 11:05:41, dcheng wrote:
> Prefer WTF::makeUnique now that we have that.

Done.

https://codereview.chromium.org/2479663002/diff/580001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebViewTest.cpp (right):

https://codereview.chromium.org/2479663002/diff/580001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebViewTest.cpp:3391: DCHECK(layerTreeView);
On 2017/01/16 11:05:41, dcheng wrote:
> Nit: Prefer ASSERT in tests

Presubmit complains ASSERT is deprecated.

[wjmaclean, 2017-01-17 18:23:38]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-17 18:23:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-17 18:39:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-17 18:39:11]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[wjmaclean, 2017-01-17 18:40:36]

Description was changed from

==========
Move compositor InputHandler from RenderViewImpl to RenderWidget.

At present the input handler for cross-process iframes is installed 
only on the RenderViewImpl, meaning that events sent to them can only
be handled on the main thread (instead of the compositor thread).

Note: The compositor layer tree on the RenderViewImpl for a cross-
process frame renderer is actually empty.

This CL attaches an input handler to each RenderWidget instead, 
allowing compositor-thread handling of input events for all
the frames. This is a precursor for handling GesturePinch events for
subframes.

BUG=654917
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Move compositor InputHandler from RenderViewImpl to RenderWidget.

At present the input handler for cross-process iframes is installed 
only on the RenderViewImpl, meaning that events sent to them can only
be handled on the main thread (instead of the compositor thread).

Note: The compositor layer tree on the RenderViewImpl for a cross-
process frame renderer is actually empty.

This CL attaches an input handler to each RenderWidget instead, 
allowing compositor-thread handling of input events for all
the frames. This is a precursor for handling GesturePinch events for
subframes.

BUG=654917
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[wjmaclean, 2017-01-17 18:40:36]

wjmaclean@chromium.org changed reviewers:
+ enne@chromium.org

[wjmaclean, 2017-01-17 18:41:11]

enne@ - Could you please review cc/ changes?

[wjmaclean, 2017-01-17 19:15:06]

On 2017/01/17 18:41:11, wjmaclean wrote:
> enne@ - Could you please review cc/ changes?

Sorry, that last patch needs a rebase ... will upload in a minute ...

[wjmaclean, 2017-01-17 19:21:14]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-17 19:21:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-17 20:51:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-17 20:51:35]

Dry run: This issue passed the CQ dry run.

[wjmaclean, 2017-01-17 22:03:50]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-17 22:04:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-17 23:25:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-17 23:25:25]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-01-18 00:13:03]

https://codereview.chromium.org/2479663002/diff/580001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebViewTest.cpp (right):

https://codereview.chromium.org/2479663002/diff/580001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebViewTest.cpp:3391: DCHECK(layerTreeView);
On 2017/01/17 18:23:28, wjmaclean wrote:
> On 2017/01/16 11:05:41, dcheng wrote:
> > Nit: Prefer ASSERT in tests
> 
> Presubmit complains ASSERT is deprecated.

To be precise, ASSERT_TRUE. DCHECK() will get skipped in a release build without
dchecks, while ASSERT_TRUE will still be checked (and handled gracefully by
gtest)

https://codereview.chromium.org/2479663002/diff/630001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2479663002/diff/630001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1347: // this triggers
notificatins to the client.
Nit: elaborate on what sort of notifications to the client, and why they're
important to send.

https://codereview.chromium.org/2479663002/diff/630001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/ChromeClient.h (right):

https://codereview.chromium.org/2479663002/diff/630001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/ChromeClient.h:267: virtual void
setHasScrollEventHandlers(LocalFrame*, bool) = 0;
Nit: it's a bit unrelated, but it'd be nice to add some comments to these
methods describing their purpose (in a followup is OK)

https://codereview.chromium.org/2479663002/diff/630001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/2479663002/diff/630001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:944: // The widget may be
nullptr if the frame is provisional.
Please add a TODO(dcheng) referencing https://crbug.com/578349 to clean this up
eventually =)

https://codereview.chromium.org/2479663002/diff/630001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:958: void
ChromeClientImpl::setEventListenerProperties(
Nit: either merge this into the previous function, or give it a more distinctive
name. I'd probably just merge, as it doesn't appear anyone else calls this
overload other than ChromeClientImpl?

[enne (OOO), 2017-01-18 01:41:53]

https://codereview.chromium.org/2479663002/diff/630001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2479663002/diff/630001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:619: if (layer_impl == NULL) {
I worry a bit about the side effects of always considering not finding a layer
to be considered finding something.

How is there no layer tree in this case?

[wjmaclean, 2017-01-18 02:14:10]

https://codereview.chromium.org/2479663002/diff/630001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2479663002/diff/630001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:619: if (layer_impl == NULL) {
On 2017/01/18 01:41:53, enne wrote:
> I worry a bit about the side effects of always considering not finding a layer
> to be considered finding something.

That's not quite what this CL is doing. The call to eventListenerProperties is
what tells us there is (or isn't) something, but due to the lack of
touch-handler rects from ScrollingCoordinator, which doesn't know how to deal
with OOPIFs, this approach just loses the fine-grained targeting. It seems safer
to indicate there's a handler (since there is) and force the event handling onto
the main thread, than drop an event that truly has a handler.

> How is there no layer tree in this case?

OOPIFs have a weird compositor setup. Prior to this CL, each local-root subframe
in an OOPIF renderer gets it's own layer tree to draw it. The RenderView also
gets a LayerTree, but it never draws anything since the main frame is remote to
the OOPIF renderer. But up until now, all the event listener properties have
been shoved onto the RenderView's LayerTree (meaning we can't tell which
particular subframe has a handler if only one of them does).

Up until this CL, *all* events sent to an OOPIF subframe have therefore been
forced onto the main thread, and thus OOPIFs have no ability to do things like
composited scrolling. Actually, to be more accurate, until this CL non of the
subframe LayerTrees have had InputHandlers, thus their events defaulted to the
main thread.

[wjmaclean, 2017-01-18 13:54:28]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-18 13:54:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[wjmaclean, 2017-01-18 14:01:24]

Updated to address suggestions, ptal?

enne@ - I linked a bug for ScrollingCoordinator to the comment in LTHI ... is
that helpful/appropriate?

https://codereview.chromium.org/2479663002/diff/580001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebViewTest.cpp (right):

https://codereview.chromium.org/2479663002/diff/580001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebViewTest.cpp:3391: DCHECK(layerTreeView);
On 2017/01/18 00:13:03, dcheng wrote:
> On 2017/01/17 18:23:28, wjmaclean wrote:
> > On 2017/01/16 11:05:41, dcheng wrote:
> > > Nit: Prefer ASSERT in tests
> > 
> > Presubmit complains ASSERT is deprecated.
> 
> To be precise, ASSERT_TRUE. DCHECK() will get skipped in a release build
without
> dchecks, while ASSERT_TRUE will still be checked (and handled gracefully by
> gtest)

Ooops, sorry. I read your comment too literally for some reason. Fixed.

https://codereview.chromium.org/2479663002/diff/630001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2479663002/diff/630001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1347: // this triggers
notificatins to the client.
On 2017/01/18 00:13:03, dcheng wrote:
> Nit: elaborate on what sort of notifications to the client, and why they're
> important to send.

Done.

https://codereview.chromium.org/2479663002/diff/630001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/ChromeClient.h (right):

https://codereview.chromium.org/2479663002/diff/630001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/ChromeClient.h:267: virtual void
setHasScrollEventHandlers(LocalFrame*, bool) = 0;
On 2017/01/18 00:13:03, dcheng wrote:
> Nit: it's a bit unrelated, but it'd be nice to add some comments to these
> methods describing their purpose (in a followup is OK)

Acknowledged.

I tried to add something for the two setters, let me know if it seems helpful.

https://codereview.chromium.org/2479663002/diff/630001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/2479663002/diff/630001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:944: // The widget may be
nullptr if the frame is provisional.
On 2017/01/18 00:13:03, dcheng wrote:
> Please add a TODO(dcheng) referencing https://crbug.com/578349 to clean this
up
> eventually =)

Done.

https://codereview.chromium.org/2479663002/diff/630001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:958: void
ChromeClientImpl::setEventListenerProperties(
On 2017/01/18 00:13:03, dcheng wrote:
> Nit: either merge this into the previous function, or give it a more
distinctive
> name. I'd probably just merge, as it doesn't appear anyone else calls this
> overload other than ChromeClientImpl?

Done.

[commit-bot: I haz the power, 2017-01-18 15:23:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-18 15:23:49]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-01-18 18:30:11]

Blink LGTM, thanks!

https://codereview.chromium.org/2479663002/diff/650001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ChromeClientImpl.h (right):

https://codereview.chromium.org/2479663002/diff/650001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ChromeClientImpl.h:140: // and if they do whether
the handlers are passive and/or blocking. This
Nit: comma after do

[wjmaclean, 2017-01-18 18:37:11]

On 2017/01/18 18:30:11, dcheng wrote:
> Blink LGTM, thanks!
> 
>
https://codereview.chromium.org/2479663002/diff/650001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/web/ChromeClientImpl.h (right):
> 
>
https://codereview.chromium.org/2479663002/diff/650001/third_party/WebKit/Sou...
> third_party/WebKit/Source/web/ChromeClientImpl.h:140: // and if they do
whether
> the handlers are passive and/or blocking. This
> Nit: comma after do

Thanks!

[wjmaclean, 2017-01-18 18:46:34]

On 2017/01/18 01:41:53, enne wrote:
> 
>
https://codereview.chromium.org/2479663002/diff/630001/cc/trees/layer_tree_ho...
> cc/trees/layer_tree_host_impl.cc:619: if (layer_impl == NULL) {
> I worry a bit about the side effects of always considering not finding a layer
> to be considered finding something.

With regards to the concerns over side effects, we're lucky in that this is
basically turning on compositor-thread events for OOPIFs which are behind a flag
(--site-per-process in general, --isolate-extensions also). The changes ought to
have no effect on MainFrames (they'll all have viewport scroll layers, right?),
so the effects will be confined to an experimental feature. I don't imagine
we're expecting many, if any, extensions to have touch handlers.

[enne (OOO), 2017-01-18 19:11:44]

https://codereview.chromium.org/2479663002/diff/630001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2479663002/diff/630001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:619: if (layer_impl == NULL) {
On 2017/01/18 at 02:14:10, wjmaclean wrote:
> On 2017/01/18 01:41:53, enne wrote:
> > I worry a bit about the side effects of always considering not finding a
layer
> > to be considered finding something.
> 
> That's not quite what this CL is doing. The call to eventListenerProperties is
what tells us there is (or isn't) something, but due to the lack of
touch-handler rects from ScrollingCoordinator, which doesn't know how to deal
with OOPIFs, this approach just loses the fine-grained targeting. It seems safer
to indicate there's a handler (since there is) and force the event handling onto
the main thread, than drop an event that truly has a handler.

You say that the side effects aren't a worry because this is behind a flag, but
this code looks to me like it says that for all touch handling queries, if it
doesn't hit a layer then pretend like it does.  There's nothing oopif specific
here, unless I'm really misreading this.

I apologize for all the confusion, but I still don't quite fully understand what
you're trying to do.  Is there not a way to specify a touch region that covers
everything so that the cc input handling code doesn't have to special case this?

> > How is there no layer tree in this case?
> 
> OOPIFs have a weird compositor setup. Prior to this CL, each local-root
subframe in an OOPIF renderer gets it's own layer tree to draw it. The
RenderView also gets a LayerTree, but it never draws anything since the main
frame is remote to the OOPIF renderer. But up until now, all the event listener
properties have been shoved onto the RenderView's LayerTree (meaning we can't
tell which particular subframe has a handler if only one of them does).
> 
> Up until this CL, *all* events sent to an OOPIF subframe have therefore been
forced onto the main thread, and thus OOPIFs have no ability to do things like
composited scrolling. Actually, to be more accurate, until this CL non of the
subframe LayerTrees have had InputHandlers, thus their events defaulted to the
main thread.

Are events being forced on the main thread or are events being dropped? You say
both, so I'm a little confused what the previous behavior is.

Is there a design doc or something else to read for more context here?

[wjmaclean, 2017-01-18 19:42:34]

On 2017/01/18 19:11:44, enne wrote:
> 
> You say that the side effects aren't a worry because this is behind a flag,
but
> this code looks to me like it says that for all touch handling queries, if it
> doesn't hit a layer then pretend like it does.  There's nothing oopif specific
> here, unless I'm really misreading this.

I'm assuming that any non-OOPIF will have viewport scroll layers, so
!active_tree_->OuterViewportScrollLayer() will only be true for OOPIFs (let me
know if that's a bad assumption). So, since we also know that OOPIFs never get
touch handler rects sent to their layer trees, we would always expect the wrong
answer if a touch handler is installed.
 
> I apologize for all the confusion, but I still don't quite fully understand
what
> you're trying to do.  Is there not a way to specify a touch region that covers
> everything so that the cc input handling code doesn't have to special case
this?

Don't apologize :-) I could certainly have done a better job explaining it.

I've been thinking about this since yesterday, and I supposed we could have
RenderWidgetCompositor send a blanket touch-rect to the root layer for OOPIFs,
and that would avoid this. I'll put that together this afternoon and upload it.

> 
> Are events being forced on the main thread or are events being dropped? You
say
> both, so I'm a little confused what the previous behavior is.

At *present* (i.e. without this CL) all OOPIF events are routed to the main
thread. This means no composited scrolling for OOPIFs, for example.

With giving the individual subframes an InputHandler for the LayerTrees that
actually draw them, which is the purpose of this CL, then the events that
*should* go to the mainframe will get dropped if there's no handler reported for
them (see comments below). That's what the change to LTHI is about, namely
making sure we don't say there's no handler when there really is. The reason
LTHI can't find a layer is that ScrollingCoordinator only reports these for the
main frame, and not OOPIF subframes.

I had chosen this approach since
LayerTreeHostImpl::EventListenerTypeForTouchStartAt doesn't actually report
*which* layer is hit, just whether there's a handler or not. It's only caller,
InputProxy::HandleTouchStart(),

https://cs.chromium.org/chromium/src/ui/events/blink/input_handler_proxy.cc?r...

will incorrectly say to drop the event if it thinks there are no handlers.
 
> Is there a design doc or something else to read for more context here?

Not at present, sorry.

I'll go ahead and delete this in favour of having RWC send a 'blanket' touch
rect ... upon reflection that does seem better.

We will still need the change in LTHI::ApplyScroll() though, but I don't think
that's controversial.

[enne (OOO), 2017-01-18 19:46:40]

> I'll go ahead and delete this in favour of having RWC send a 'blanket' touch
rect ... upon reflection that does seem better.

Sounds good!

> We will still need the change in LTHI::ApplyScroll() though, but I don't think
that's controversial.

Yeah.  The ApplyScroll change looks fine to me.  I was mostly just concerned
about the hit testing change.  :)

[wjmaclean, 2017-01-19 18:55:47]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 18:56:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[wjmaclean, 2017-01-19 19:24:48]

enne@ - I removed the code you were concerned about, ptal?

dcheng@ - The code I removed from LTHI has been replaced by plumbing a pathway
from FrameView to RenderWidgetCompositor to have the latter set a pseudo
touch-event-rect on the LayerTree's root layer, so that the compositor can
associate the touch handler with a layer. Could you please look at the delta
with the previous patchset and let me know if you're OK with the way I plumbed
it?

https://codereview.chromium.org/2479663002/diff/650001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ChromeClientImpl.h (right):

https://codereview.chromium.org/2479663002/diff/650001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ChromeClientImpl.h:140: // and if they do whether
the handlers are passive and/or blocking. This
On 2017/01/18 18:30:11, dcheng wrote:
> Nit: comma after do

Done.

[enne (OOO), 2017-01-19 19:29:58]

Thanks.  cc lgtm

[commit-bot: I haz the power, 2017-01-19 20:53:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-19 20:53:42]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-01-20 19:27:20]

OK, so the new Blink public API is temporary? Or will we be using it once we
update the scrolling stuff as well?

https://codereview.chromium.org/2479663002/diff/670001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/2479663002/diff/670001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:982: if (WebLayerTreeView*
treeView = widget->getLayerTreeView())
When will a local frame root widget not have a layer tree view?

(Sorry if this is a dumb question)

https://codereview.chromium.org/2479663002/diff/670001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebLayerTreeView.h (right):

https://codereview.chromium.org/2479663002/diff/670001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebLayerTreeView.h:163: virtual void
updateTouchRectsForSubframeIfNecessary(){};
Nit: no ; here.

[wjmaclean, 2017-01-20 19:34:19]

> OK, so the new Blink public API is temporary? Or will we be using it once we
> update the scrolling stuff as well?

I would assume it's temporary, though the ScrollingCoordinator work might
actually make use of it when it's done (we need to try and split
ScrollingCoordinator apart somewhat I think, as it's become a bit of a catch-bin
for things).

https://codereview.chromium.org/2479663002/diff/670001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/2479663002/diff/670001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:982: if (WebLayerTreeView*
treeView = widget->getLayerTreeView())
On 2017/01/20 19:27:20, dcheng wrote:
> When will a local frame root widget not have a layer tree view?
> 
> (Sorry if this is a dumb question)

I'm being consistent with what we've done elsewhere. We can try a DCHECK, but I
suspect if we're testing for this elsewhere, we've seen it. I'd be willing to
run tests with a DCHECK after the CL lands to see if we hit it in tests, or
general browsing, if you like.

https://codereview.chromium.org/2479663002/diff/670001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebLayerTreeView.h (right):

https://codereview.chromium.org/2479663002/diff/670001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebLayerTreeView.h:163: virtual void
updateTouchRectsForSubframeIfNecessary(){};
On 2017/01/20 19:27:20, dcheng wrote:
> Nit: no ; here.

Ooops! Done.

[dcheng, 2017-01-20 19:35:47]

lgtm

https://codereview.chromium.org/2479663002/diff/670001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/2479663002/diff/670001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:982: if (WebLayerTreeView*
treeView = widget->getLayerTreeView())
On 2017/01/20 19:34:19, wjmaclean wrote:
> On 2017/01/20 19:27:20, dcheng wrote:
> > When will a local frame root widget not have a layer tree view?
> > 
> > (Sorry if this is a dumb question)
> 
> I'm being consistent with what we've done elsewhere. We can try a DCHECK, but
I
> suspect if we're testing for this elsewhere, we've seen it. I'd be willing to
> run tests with a DCHECK after the CL lands to see if we hit it in tests, or
> general browsing, if you like.

OK, that's fine, but let's investigate this in a followup. It doesn't really
make sense why an attached local frame root wouldn't have a layer tree view
outside of some weird edge cases in init/detach.

[wjmaclean, 2017-01-20 19:44:08]

The CQ bit was checked by wjmaclean@chromium.org

[wjmaclean, 2017-01-20 19:44:09]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
enne@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2479663002/#ps690001
(title: "Remove stray semi-colon.")

[commit-bot: I haz the power, 2017-01-20 19:44:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[wjmaclean, 2017-01-20 19:48:47]

On 2017/01/20 19:35:47, dcheng wrote:
> lgtm
> 
>
https://codereview.chromium.org/2479663002/diff/670001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):
> 
>
https://codereview.chromium.org/2479663002/diff/670001/third_party/WebKit/Sou...
> third_party/WebKit/Source/web/ChromeClientImpl.cpp:982: if (WebLayerTreeView*
> treeView = widget->getLayerTreeView())
> On 2017/01/20 19:34:19, wjmaclean wrote:
> > On 2017/01/20 19:27:20, dcheng wrote:
> > > When will a local frame root widget not have a layer tree view?
> > > 
> > > (Sorry if this is a dumb question)
> > 
> > I'm being consistent with what we've done elsewhere. We can try a DCHECK,
but
> I
> > suspect if we're testing for this elsewhere, we've seen it. I'd be willing
to
> > run tests with a DCHECK after the CL lands to see if we hit it in tests, or
> > general browsing, if you like.
> 
> OK, that's fine, but let's investigate this in a followup. It doesn't really
> make sense why an attached local frame root wouldn't have a layer tree view
> outside of some weird edge cases in init/detach.

Will do. As soon as this lands, I'll run some tests on the TryBots to see what
happens.

[commit-bot: I haz the power, 2017-01-20 21:39:54]

CQ is committing da patch.

Bot data: {"patchset_id": 690001, "attempt_start_ts": 1484941448719630,
"parent_rev": "f59dc823911d1e7fca195b638638e8458c631491", "commit_rev":
"efd05aec93fad28adb8bef9674530e7bbe64766f"}

[commit-bot: I haz the power, 2017-01-20 21:40:25]

Description was changed from

==========
Move compositor InputHandler from RenderViewImpl to RenderWidget.

At present the input handler for cross-process iframes is installed 
only on the RenderViewImpl, meaning that events sent to them can only
be handled on the main thread (instead of the compositor thread).

Note: The compositor layer tree on the RenderViewImpl for a cross-
process frame renderer is actually empty.

This CL attaches an input handler to each RenderWidget instead, 
allowing compositor-thread handling of input events for all
the frames. This is a precursor for handling GesturePinch events for
subframes.

BUG=654917
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Move compositor InputHandler from RenderViewImpl to RenderWidget.

At present the input handler for cross-process iframes is installed 
only on the RenderViewImpl, meaning that events sent to them can only
be handled on the main thread (instead of the compositor thread).

Note: The compositor layer tree on the RenderViewImpl for a cross-
process frame renderer is actually empty.

This CL attaches an input handler to each RenderWidget instead, 
allowing compositor-thread handling of input events for all
the frames. This is a precursor for handling GesturePinch events for
subframes.

BUG=654917
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2479663002
Cr-Commit-Position: refs/heads/master@{#445175}
Committed:
https://chromium.googlesource.com/chromium/src/+/efd05aec93fad28adb8bef967453...
==========

[commit-bot: I haz the power, 2017-01-20 21:40:27]

Committed patchset #29 (id:690001) as
https://chromium.googlesource.com/chromium/src/+/efd05aec93fad28adb8bef967453...

[Charlie Reis, 2017-01-20 21:52:48]

Description was changed from

==========
Move compositor InputHandler from RenderViewImpl to RenderWidget.

At present the input handler for cross-process iframes is installed 
only on the RenderViewImpl, meaning that events sent to them can only
be handled on the main thread (instead of the compositor thread).

Note: The compositor layer tree on the RenderViewImpl for a cross-
process frame renderer is actually empty.

This CL attaches an input handler to each RenderWidget instead, 
allowing compositor-thread handling of input events for all
the frames. This is a precursor for handling GesturePinch events for
subframes.

BUG=654917
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2479663002
Cr-Commit-Position: refs/heads/master@{#445175}
Committed:
https://chromium.googlesource.com/chromium/src/+/efd05aec93fad28adb8bef967453...
==========

to

==========
Move compositor InputHandler from RenderViewImpl to RenderWidget.

At present the input handler for cross-process iframes is installed 
only on the RenderViewImpl, meaning that events sent to them can only
be handled on the main thread (instead of the compositor thread).

Note: The compositor layer tree on the RenderViewImpl for a cross-
process frame renderer is actually empty.

This CL attaches an input handler to each RenderWidget instead, 
allowing compositor-thread handling of input events for all
the frames. This is a precursor for handling GesturePinch events for
subframes.

BUG=654917
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2479663002
Cr-Commit-Position: refs/heads/master@{#445175}
Committed:
https://chromium.googlesource.com/chromium/src/+/efd05aec93fad28adb8bef967453...
==========

[waffles, 2017-01-21 00:49:04]

A revert of this CL (patchset #29 id:690001) has been created in
https://codereview.chromium.org/2649573003/ by waffles@chromium.org.

The reason for reverting is: Causes flaky failures of
SitePerProcessInteractiveBrowserTest.SubframeKeyboardEventRouting..

[wjmaclean, 2017-01-21 19:49:14]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-21 19:49:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-21 19:51:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-21 19:51:20]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[wjmaclean, 2017-01-21 20:15:26]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-21 20:15:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[wjmaclean, 2017-01-21 21:08:27]

Description was changed from

==========
Move compositor InputHandler from RenderViewImpl to RenderWidget.

At present the input handler for cross-process iframes is installed 
only on the RenderViewImpl, meaning that events sent to them can only
be handled on the main thread (instead of the compositor thread).

Note: The compositor layer tree on the RenderViewImpl for a cross-
process frame renderer is actually empty.

This CL attaches an input handler to each RenderWidget instead, 
allowing compositor-thread handling of input events for all
the frames. This is a precursor for handling GesturePinch events for
subframes.

BUG=654917
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2479663002
Cr-Commit-Position: refs/heads/master@{#445175}
Committed:
https://chromium.googlesource.com/chromium/src/+/efd05aec93fad28adb8bef967453...
==========

to

==========
Move compositor InputHandler from RenderViewImpl to RenderWidget.

At present the input handler for cross-process iframes is installed 
only on the RenderViewImpl, meaning that events sent to them can only
be handled on the main thread (instead of the compositor thread).

Note: The compositor layer tree on the RenderViewImpl for a cross-
process frame renderer is actually empty.

This CL attaches an input handler to each RenderWidget instead, 
allowing compositor-thread handling of input events for all
the frames. This is a precursor for handling GesturePinch events for
subframes.

TBR=alexmos@chromium.org
BUG=654917
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[wjmaclean, 2017-01-21 21:08:27]

wjmaclean@chromium.org changed reviewers:
+ alexmos@chromium.org

[commit-bot: I haz the power, 2017-01-21 22:20:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-21 22:20:47]

Dry run: This issue passed the CQ dry run.

[wjmaclean, 2017-01-21 22:23:34]

I'm going to attempt to reland this, given the difficulties with the test seem
to have been just a synchronization issue.

[wjmaclean, 2017-01-21 22:23:48]

The CQ bit was checked by wjmaclean@chromium.org

[wjmaclean, 2017-01-21 22:23:48]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
enne@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2479663002/#ps730001
(title: "Rebase to master@{#445291}.")

[commit-bot: I haz the power, 2017-01-21 22:23:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-21 22:28:28]

CQ is committing da patch.

Bot data: {"patchset_id": 730001, "attempt_start_ts": 1485037428060120,
"parent_rev": "f771194423dfad6736b846e2bf3417b2e021a798", "commit_rev":
"1d97062b110831423488e9bc5bd67a1bde1fded5"}

[commit-bot: I haz the power, 2017-01-21 22:29:01]

Description was changed from

==========
Move compositor InputHandler from RenderViewImpl to RenderWidget.

At present the input handler for cross-process iframes is installed 
only on the RenderViewImpl, meaning that events sent to them can only
be handled on the main thread (instead of the compositor thread).

Note: The compositor layer tree on the RenderViewImpl for a cross-
process frame renderer is actually empty.

This CL attaches an input handler to each RenderWidget instead, 
allowing compositor-thread handling of input events for all
the frames. This is a precursor for handling GesturePinch events for
subframes.

TBR=alexmos@chromium.org
BUG=654917
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Move compositor InputHandler from RenderViewImpl to RenderWidget.

At present the input handler for cross-process iframes is installed 
only on the RenderViewImpl, meaning that events sent to them can only
be handled on the main thread (instead of the compositor thread).

Note: The compositor layer tree on the RenderViewImpl for a cross-
process frame renderer is actually empty.

This CL attaches an input handler to each RenderWidget instead, 
allowing compositor-thread handling of input events for all
the frames. This is a precursor for handling GesturePinch events for
subframes.

TBR=alexmos@chromium.org
BUG=654917
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2479663002
Cr-Commit-Position: refs/heads/master@{#445295}
Committed:
https://chromium.googlesource.com/chromium/src/+/1d97062b110831423488e9bc5bd6...
==========

[commit-bot: I haz the power, 2017-01-21 22:29:04]

Committed patchset #31 (id:730001) as
https://chromium.googlesource.com/chromium/src/+/1d97062b110831423488e9bc5bd6...