I have a CL for the PointerLockBrowserTest and TouchpadPinchBrowserTest at https://crrev.com/c/1369059

The SitePerProcessHitTestBrowserTest is a separate issue. What I've found so far is: when BGPT is turned on the RenderPass given to the browser is missing the SurfaceQuad that's used to hit test to the correct RenderWidgetHostView. Still digging into where that's going wrong. Is there a known issue with surface hit testing with BGPT?

I'm not aware of any differences in SurfaceQuads or RenderPasses, but I know very little about them. I suspect something is not right, or is different, earlier in the pipeline.

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

commit 6a4a425fdd7343dab3948c2dce293c109e0aabf8
Author: David Bokan <bokan@chromium.org>
Date: Tue Dec 11 18:00:32 2018

[BlinkGenPropertyTrees] Fix layer commit for wheel handler region

When a wheel event listener is added or removed on a page, we set each
layer as having a WheelEventHandlerRegion equal to the entire bounds of
the layer. This happens in Layer::PushPropertiesTo.

This means that when an event listener is changed, we need to cause all
layers to need a push properties and commit. Prior to this CL, this
happens in LayerTreeHost::SetEventListenerProperties by setting
SetNeedsFullTreeSync and SetSubtreePropertyChanged. However, as noted by
surrounding comments, this looks wrong; SetNeedsFullTreeSync only causes
the tree structure to be synced, properties aren't pushed.
SetSubtreePropertyChanged causes a property sync only on the root layer.

This works prior to BGPT because SetNeedsFullTreeSync causes a property
tree rebuild during which all layers are marked as needing a property
push. See PropertyTreeBuilderContext::BuildPropertyTreesInternal which
calls SetLayerPropertyTreeChangedForChild for all child layers as the
tree is built.

When BGPT is turned on, property trees are built in Blink independent of
the layer tree so this doesn't cause us to SetNeedsPushProperties. This
CL fixes SetEventListenerProperties to set the correct flags to
propagate the listener state to the active tree.

This CL fixes:

PointerLockBrowserTest.PointerLockWheelEventRouting
TouchpadPinchBrowserTest.WheelListenerAllowingPinch/0
TouchpadPinchBrowserTest.WheelListenerAllowingPinch/1

With BGPT turned on.

Bug:  912334 , 881011 
Change-Id: I4446ccfd32de12e618219e5fa15c48f5093bcc98
Reviewed-on: https://chromium-review.googlesource.com/c/1369059
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615579}
[modify] https://crrev.com/6a4a425fdd7343dab3948c2dce293c109e0aabf8/cc/trees/layer_tree_host.cc

Ok, so I've dug a bit deeper into SitePerProcessHitTestBrowserTest.RootConsumesScrollDuringOverscrollGesture, I believe this is a difference in how we handle border-radius on an out of process iframe. Removing border radius from the iframe makes the test pass.

The test sets up a simple page with a 110x110 sized, out-of-process iframe with a border-radius. The test fails an assertion that checks a hit test over the iframe returns the RenderWidgetHostView of the iframe. This assertion isn't part of the test's behavior, it's asserting the setup.

Pre-BGPT, this hit test would find the RWHV for the child iframe. When we turn on BGPT, the hit test returns the root view instead but sets should_verify_result to tell the caller the hit test must be performed in the renderer. The assertion fails because it assumes the hit test returns the correct view immediately. I've verified that sending events later in the test do note the "should_verify_result" flag and correctly target the iframe so the test appears to work and pass with the assertion removed.

The difference has to do with the render passes produced by the main frame's renderer. We produce additional RenderPasses related to the iframe when BGPT is turned on (attached). Hit testing these additional passes causes the hit test to be ambiguous so we don't return the hit surface and instead mark the hit test as needing to be done on the renderer. This happens in SurfaceHittest::GetTargetSurfaceAtPointInternal when we hit test the SURFACE_CONTENT quad and notice we also hit a non-surface earlier. !BGPT also sets this flag but because it doesn't hit a non-surface it returns the target surface in the hit test result.

I've tracked these new render passes back to differences in the effect tree (attached). At least one effect node is coming from PropertyTreeManager::SynthesizeCcEffectsForClipsIfNeeded. The comments there mention needing an additional render surface so this appears to be intentional. It makes sense given that I don't think we can hit-test border-radius in the browser so perhaps all there is to do is remove the assert? Trying this out on a live page, it looks like border-radius on an OOPIFs appears _more_ correct when we turn on BGPT and hit testing works as expected.

+wangxianzhu@, I see you added the synthetic effect code. Could you verify that what's happening here is expected? I don't understand effects and clipping so I'm unsure if this is expected.

(One additional difference I noticed is that the effect nodes with BGPT have double_sided=false whereas this used to be true in !BGPT. Not sure if that's related or known)

Deleted: effect_tree.bgpt 2.9 KB

effect_tree.bgpt 2.9 KB Download

Deleted: effect_tree.stable 3.6 KB

effect_tree.stable 3.6 KB Download

Deleted: render_passes.bgpt 16.1 KB

render_passes.bgpt 16.1 KB Download

Deleted: render_passes.stable 11.0 KB

render_passes.stable 11.0 KB Download

I think the BGPT effect tree is valid, and we just need to remove the assert. The difference about double_sided seems irrelevant because it's not in a 3d context.

Actually trchen@ (who has left) wrote the synthetic effect code. It's supposed to produce the same effect tree as pre-BGPT, or a different but rendering-equivalent effect tree.

In BGPT, a non-trivial clip (i.e. a clip with rounded corners or clip-path) is implemented with a mask layer which is blended onto the clipped contents in kDstIn mode, thus we need a render surface for the clipped contents to allow the blending.

In Pre-BGPT we also do such mask and blending thing, perhaps with some different conditions for optimization. I'm not sure why BGPT and non-BGPT create different effect trees for the test cases without looking into them, but the BGPT effect tree looks valid to me.

Thank you, I'll remove the assert.

From what I can tell, pre-BGPT we don't actually clip the OOPIF correctly in the border-radius case. There's a rounded border, drawn in the parent, but the OOPIF contents appear square overlayed on top of the border. I've attached a BGPT vs !BGPT screenshot.

Deleted: non-bgpt.png 158 KB

	non-bgpt.png 158 KB View Download

Deleted: bgpt.png 167 KB

	bgpt.png 167 KB View Download

Excellent analysis David!

I actually don't see the difference in your screenshots. But, I think it could be due to the change to unify how clipping is done in layout_replaced. Trchen landed several patches related to this recently: https://chromium.googlesource.com/chromium/src/+log/master?author=trchen

You have to open the images so they're at full size. Note the corners of the white area (the iframe content) are rounded (i.e. clipped by the border) in the BGPT version but they're not rounded in non-BGPT.

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

commit 2401ba3b8dfcfc2d940b0f84b97f625e377470a7
Author: David Bokan <bokan@chromium.org>
Date: Fri Dec 14 18:12:10 2018

[BlinkGenPropertyTrees] Fix SitePerProcessHitTestBrowserTest

This CL fixes the test:
SitePerProcessHitTestBrowserTest.RootConsumesScrollDuringOverscrollGesture

Turning on BlinkGenPropertyTrees changes how OOPIFs are clipped in non
trivial clipping scenarios, such as border-radius. In this test, the
OOPIF has a border-radius. Prior to BGPT, the OOPIF would not be masked
correctly.

The fix here is to simply remove the failing ASSERT. It's simply
checking that events sent to the given point get routed to the iframe's
RenderWidgetHostView. With BGPT, we correctly perform masking so the hit
test realizes the result will be ambiguous and requires a hit test in
the renderer. Additional details are in the bug.

The ASSERT is unneeded as the InputEventAckWaiter will wait on the ACK
in the child RWHV so if it gets mis-routed the waiter will never awaken.

Bug:  912334 
Change-Id: Iaf76181ed7e89971e445076905057ebde30abffb
Reviewed-on: https://chromium-review.googlesource.com/c/1374467
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616744}
[modify] https://crrev.com/2401ba3b8dfcfc2d940b0f84b97f625e377470a7/content/browser/site_per_process_hit_test_browsertest.cc