Adding Proj-SiteIsolation-LaunchBlocking out of caution

Friendly ping-- any updates on the progress here?  We're hoping to get this fix into M67 if possible.

I'm not sure if this is a launch blocker (per comment 4), but it does sound like it means scroll bubbling won't work in mobile device emulation mode.  If it's helpful context at all, there's another mobile emulation OOPIF bug in  issue 826651 , where it appears that OOPIFs aren't aware of being in that mode.

I took a look at https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_widget_host_view_event_handler.cc?dr=C&l=481.

The issue is that routing events happens within the functions that generate web events from ui events and dev tools injects web events and that's why the event routing gets skipped. Maybe the proper approach here is to decouple routing web events from the functions that generate them.

Note that we already route most events to the right OOPIF using RenderWidgetHostInputEventRouter, just not the emulated touch. The problem is that touch sequences require heavy handling of ACKs, and wiring that up differently is cumbersome.

I suspect that having callbacks for ACKs might help. Dave, do we have plans for switching to callbacks instead of separate ACKs?

I don't have any immediate plans this quarter. We have to remove the Chrome IPC path first; we can do that once M66 ships with the mojo input messages on for webview. Then some of this code can become cleaner.

dtapuska@/wjmaclean@/dgozman@: Is there another way we can make the touch sequences and ACKs easier for DevTools to emulate in the short term, if callbacks aren't an option?  We're aiming for a fix in M67 to avoid regressing scroll bubbling in this case.

All touch events generated by devtools are marked with fromdebugger modiifer. Does that help any?

Secondly all touch events have a unique ID on the ack so it is possible to keep track of the ones you've seen and not seen. 

I'm not entirely certain how call backs would help devtools solve this but i've proposed solutions currently used in the ack situations. 

I looked at the TouchEmulator a little more. It seems relatively complicated in that it generates Gestures itself as well. I think the ideal model is that it actually generates a touch sequence that injected back into the top of the stack and it causes gesture recognizer to handle them and then the flow would be correct. 

What was the motivation for doing it the current way as opposed to the way gpu benchmarking/chrome driver works?

+1 to re-injecting the appropriately created touch events back at the top-level RenderWidgetHostViewX (or maybe even higher?). The less we special case this pathway the better.

It's good that you ask! The motivation was:
- we convert shift+drag into pinch instead of scroll;
- at least mac platform entirely lacks gesture recognition support.

A while ago I tried to address the  issue #2  and move our pipeline higher, but my first preparation patch has never landed [1]. I'm glad that there is interest in fixing this now!

[1] https://chromium-review.googlesource.com/c/chromium/src/+/706184

Re C#14: Is there anything we can do to assist with this work? If there are things we can discuss over VC I'd be happy to arrange a time.

I was not planning to work on this in the nearest future. The feature is minor, and I want to get rid of existing implementation anyway. That said, the underlying issue with gestures on mac is not going anywhere.

Comment 16: We were treating this as a launch blocker for M67.  Are you saying it's ok for scroll bubbling to not work in DevTools mobile device emulation?  There will be a lot of OOPIFs out there starting in M67.

re #c17: oh, I misunderstood the issue. If this affects device emulation, not emulateTouchFromMouse which is used for remote debugging screencast, then we should definitely address it soon.

dgozman@: Can you follow up on comment 18?  Will you or einbinder@ be able to address this soon, possibly with a merge to M67?  In theory we could launch Site Isolation without this, but it would be unfortunate to regress scroll bubbling in mobile device emulation mode.

I don't think we have bandwidth to address this soon. I feel like some help from SiteIsolation and/or input team is required, due to issues outlined in #c0 and #c14. Someone needs to come up with a design which satisfies stakeholders, and my last quick attempt was rejected.

Ah, sorry-- I didn't realize there were still questions about the approach to take here.  dtapuska@ and wjmaclean@, can we try to come up with a design for something that can work here, ideally in the short term?

+nzolghadr@ could you or wjmaclean@ come up with a plan here?

Sounds like wjmaclean@ and nzolghadr@ are working on a plan for this.  Thanks!

It sounds like it may not be viable to fix this in M67, meaning scroll bubbling would regress for cross-origin iframes in DevTools mobile device emulation mode when Site Isolation is turned on.  Maybe that's ok for one milestone, if we can get the fix into M68.  (And if the fix is possible to get into M67, so much the better.)

I'm leaving the milestone labels for now, until we have a more concrete sense of what the fix involves.  We can update as needed from there.

CL up for review at https://chromium-review.googlesource.com/c/chromium/src/+/1069661

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a1403ff3efbd8f412e2e0c9b0257da3933f8692b

commit a1403ff3efbd8f412e2e0c9b0257da3933f8692b
Author: W. James MacLean <wjmaclean@chromium.org>
Date: Mon Jun 11 19:28:08 2018

Route Emulated Touch Events for OOPIFs

This CL makes TouchEmulator work for OOPIFs by

1) Instantiating a single TouchEmulator for all views,

2) Moving ownership of the single TouchEmulator to
   RenderWidgetHostInputEventRouter (RWHIER), and

3) Routing the subsequent touch/gesture events through
   the input event router.

The last item is important so that scroll bubbling from
OOPIFs to parent views will work properly. It's important
to note that emulated touch events are *always* routed to
the same view that originally received the mouse event
that is being converted into a touch event. But GestureEvent
targetting will now follow the usual rules enforced by
RWHIER.

A core component of this CL involves having RWHIER implement
TouchEmulatorClient, so that it can mediate actions among
multiple views without the TouchEmulator needing to be aware
of their existence.

Ideally we would like the single TouchEmulator to use
the same GestureProvider as the root view, but since touch
emulation requires custom configuration of the GestureProvider,
and since (at present) GestureProvider does not allow most
configuration parameters to change after instantiation,
having a single GestureProvider will have to wait.

Bug:  824772 
Change-Id: I1c007631e08fd7f4ece8accb896cad9bc4982f97
Reviewed-on: https://chromium-review.googlesource.com/1069661
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Commit-Queue: James MacLean <wjmaclean@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566103}
[modify] https://crrev.com/a1403ff3efbd8f412e2e0c9b0257da3933f8692b/content/browser/devtools/protocol/emulation_handler.cc
[modify] https://crrev.com/a1403ff3efbd8f412e2e0c9b0257da3933f8692b/content/browser/devtools/protocol/input_handler.cc
[modify] https://crrev.com/a1403ff3efbd8f412e2e0c9b0257da3933f8692b/content/browser/renderer_host/input/main_thread_event_queue_browsertest.cc
[modify] https://crrev.com/a1403ff3efbd8f412e2e0c9b0257da3933f8692b/content/browser/renderer_host/input/touch_emulator.cc
[modify] https://crrev.com/a1403ff3efbd8f412e2e0c9b0257da3933f8692b/content/browser/renderer_host/input/touch_emulator.h
[modify] https://crrev.com/a1403ff3efbd8f412e2e0c9b0257da3933f8692b/content/browser/renderer_host/input/touch_emulator_client.h
[modify] https://crrev.com/a1403ff3efbd8f412e2e0c9b0257da3933f8692b/content/browser/renderer_host/input/touch_emulator_unittest.cc
[modify] https://crrev.com/a1403ff3efbd8f412e2e0c9b0257da3933f8692b/content/browser/renderer_host/render_widget_host_browsertest.cc
[modify] https://crrev.com/a1403ff3efbd8f412e2e0c9b0257da3933f8692b/content/browser/renderer_host/render_widget_host_impl.cc
[modify] https://crrev.com/a1403ff3efbd8f412e2e0c9b0257da3933f8692b/content/browser/renderer_host/render_widget_host_impl.h
[modify] https://crrev.com/a1403ff3efbd8f412e2e0c9b0257da3933f8692b/content/browser/renderer_host/render_widget_host_input_event_router.cc
[modify] https://crrev.com/a1403ff3efbd8f412e2e0c9b0257da3933f8692b/content/browser/renderer_host/render_widget_host_input_event_router.h
[modify] https://crrev.com/a1403ff3efbd8f412e2e0c9b0257da3933f8692b/content/browser/renderer_host/render_widget_host_unittest.cc
[modify] https://crrev.com/a1403ff3efbd8f412e2e0c9b0257da3933f8692b/content/browser/site_per_process_hit_test_browsertest.cc
[modify] https://crrev.com/a1403ff3efbd8f412e2e0c9b0257da3933f8692b/testing/buildbot/filters/viz.content_browsertests.filter

Modulo any follow-on refactors, which should have little-to-no behavioural effects, this is fixed.

Thanks James!  I can verify the fix in Windows Canary 69.0.3460.0, using the steps below (with the scroll bubbling URL from  issue 852932 ):

1) Start Chrome with Site Isolation.
2) Visit https://thewirecutter.com/reviews/best-patio-furniture/
3) Open DevTools and enable Mobile device emulation mode (2 box icon in upper left).
4) Emulate touch scrolls down to the comment section.
5) Try to scroll in the comment section.

In 67.0.3396.87 (Stable) and 69.0.3452.0 (Dev), the scrolling is very janky and doesn't really make progress.  Works as expected in 69.0.3460.0.  (The r566103 fix landed in 69.0.3456.0.)

James, is this mergeable to M68?  Would be good for scrolling in this mode to be functional with Site Isolation enabled, depending on how risky you think the change is.  (Maybe try it out in a local checkout of M68?)

Yes, I think it's potentially mergeable (the CL lived throughout it's development cycle without any merge conflicts I think).

I'll try merging it to a local M68 checkout as you suggest and give it a spin.

Thanks!  If it works locally, please request merge to M68 on Monday in time for the next beta cut.

For reference, I was able to merge this cleanly to my local M68 branch and verify that it fixed the bug and passed the newly added content_browsertests.  If you think it's unlikely to cause regressions, let's request merge on Monday.  Thanks!

The NextAction date has arrived: 2018-06-18

Thanks for doing the testing Charlie! Yes, I think it's unlikely to cause problems, and the net win is big. It's certainly worth putting it in the beta and giving it some bake time.

This bug requires manual review: M68 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Approving merge to M68. Branch:3440

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/40eecf0fe920314d1ffda724b9225ae6f977b7b5

commit 40eecf0fe920314d1ffda724b9225ae6f977b7b5
Author: W. James MacLean <wjmaclean@chromium.org>
Date: Mon Jun 18 20:18:24 2018

Route Emulated Touch Events for OOPIFs

This CL makes TouchEmulator work for OOPIFs by

1) Instantiating a single TouchEmulator for all views,

2) Moving ownership of the single TouchEmulator to
   RenderWidgetHostInputEventRouter (RWHIER), and

3) Routing the subsequent touch/gesture events through
   the input event router.

The last item is important so that scroll bubbling from
OOPIFs to parent views will work properly. It's important
to note that emulated touch events are *always* routed to
the same view that originally received the mouse event
that is being converted into a touch event. But GestureEvent
targetting will now follow the usual rules enforced by
RWHIER.

A core component of this CL involves having RWHIER implement
TouchEmulatorClient, so that it can mediate actions among
multiple views without the TouchEmulator needing to be aware
of their existence.

Ideally we would like the single TouchEmulator to use
the same GestureProvider as the root view, but since touch
emulation requires custom configuration of the GestureProvider,
and since (at present) GestureProvider does not allow most
configuration parameters to change after instantiation,
having a single GestureProvider will have to wait.

TBR=wjmaclean@chromium.org

(cherry picked from commit a1403ff3efbd8f412e2e0c9b0257da3933f8692b)

Bug:  824772 
Change-Id: I1c007631e08fd7f4ece8accb896cad9bc4982f97
Reviewed-on: https://chromium-review.googlesource.com/1069661
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Commit-Queue: James MacLean <wjmaclean@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#566103}
Reviewed-on: https://chromium-review.googlesource.com/1105030
Reviewed-by: James MacLean <wjmaclean@chromium.org>
Cr-Commit-Position: refs/branch-heads/3440@{#411}
Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733}
[modify] https://crrev.com/40eecf0fe920314d1ffda724b9225ae6f977b7b5/content/browser/devtools/protocol/emulation_handler.cc
[modify] https://crrev.com/40eecf0fe920314d1ffda724b9225ae6f977b7b5/content/browser/devtools/protocol/input_handler.cc
[modify] https://crrev.com/40eecf0fe920314d1ffda724b9225ae6f977b7b5/content/browser/renderer_host/input/main_thread_event_queue_browsertest.cc
[modify] https://crrev.com/40eecf0fe920314d1ffda724b9225ae6f977b7b5/content/browser/renderer_host/input/touch_emulator.cc
[modify] https://crrev.com/40eecf0fe920314d1ffda724b9225ae6f977b7b5/content/browser/renderer_host/input/touch_emulator.h
[modify] https://crrev.com/40eecf0fe920314d1ffda724b9225ae6f977b7b5/content/browser/renderer_host/input/touch_emulator_client.h
[modify] https://crrev.com/40eecf0fe920314d1ffda724b9225ae6f977b7b5/content/browser/renderer_host/input/touch_emulator_unittest.cc
[modify] https://crrev.com/40eecf0fe920314d1ffda724b9225ae6f977b7b5/content/browser/renderer_host/render_widget_host_browsertest.cc
[modify] https://crrev.com/40eecf0fe920314d1ffda724b9225ae6f977b7b5/content/browser/renderer_host/render_widget_host_impl.cc
[modify] https://crrev.com/40eecf0fe920314d1ffda724b9225ae6f977b7b5/content/browser/renderer_host/render_widget_host_impl.h
[modify] https://crrev.com/40eecf0fe920314d1ffda724b9225ae6f977b7b5/content/browser/renderer_host/render_widget_host_input_event_router.cc
[modify] https://crrev.com/40eecf0fe920314d1ffda724b9225ae6f977b7b5/content/browser/renderer_host/render_widget_host_input_event_router.h
[modify] https://crrev.com/40eecf0fe920314d1ffda724b9225ae6f977b7b5/content/browser/renderer_host/render_widget_host_unittest.cc
[modify] https://crrev.com/40eecf0fe920314d1ffda724b9225ae6f977b7b5/content/browser/site_per_process_hit_test_browsertest.cc
[modify] https://crrev.com/40eecf0fe920314d1ffda724b9225ae6f977b7b5/testing/buildbot/filters/viz.content_browsertests.filter

Unable to reproduce the issue on Win-10 using chrome reported version #67.0.3396.87 as per comment #30. Observed that the scrolling is very smooth. Hence, unable to proceed with verifying the issue on chrome version #68.0.3440.33.

Attached a screen cast for reference on chrome version #68.0.3440.33.

wjmaclean@ - Could you please check the attached screen cast and please help us in verifying the fix.

Thanks...!!

Deleted: 824772.mp4 13.5 MB

	824772.mp4 13.5 MB View Download

Comment 39: Are you scrolling by clicking and dragging the page, to simulate a touch scroll on a mobile device?  It looks like you might be scrolling using the mousewheel or trackpad instead, which doesn't repro the bug.

Sorry for leaving that detail out of comment 30!

creis@, thank you for the clarification. I'm able to reproduce this issue and verified the above fix on Win, Mac & Linux platforms for today's M68 Beta RC# 68.0.3440.33 - Working as intended.