SitePerProcessBrowserTouchActionTest::WaitForTouchActionUpdated Doesn't Actually Wait For Frames |
||
Issue descriptionOS: Linux Test Suite: content_browsertests Tests: SitePerProcessBrowserTouchActionTest.EffectiveTouchActionPropagatesAcrossFrames SitePerProcessBrowserTouchActionTest.EffectiveTouchActionPropagatesAcrossNestedFrames SitePerProcessBrowserTouchActionTest.EffectiveTouchActionPropagatesWhenChildFrameNavigates Background: WaitForChildFrameSurfaceReady, doesn't work in Viz as the surface embedding information is no longer delivered to the browser process. I'm working on replacing it with two separate APIs. One can listen to frame submissions (RenderFrameSubmissionObserver) the other is in development, and listens to hit test data arriving in the browser process. While looking at the tests in question I have ran into issues with SitePerProcessBrowserTouchActionTest::WaitForTouchActionUpdated. Currently this method does: MainThreadFrameObserver::Wait WaitForChildFrameSurfaceReady WaitForChildFrameSurfaceReady MainThreadFrameObserver::Wait However there are some subtleties with these methods: - MainThreadFrameObserver::Wait synchronizes with the renderer main thread via an IPC. With the response delivered event if no frame is submitted. (QueueMessageSwapPromise fires the message on DidNotSwap) - WaitForChildFrameSurfaceReady only ever waits the first time. Once a frame with the child surface embedded is submitted, this method just returns right away. Without ever waiting for a frame. The end result is that WaitForTouchActionUpdated can complete with two DidNotSwap messages, and no frames having been submitted. I ran into this while trying to swap in RenderFrameSubmissionObserver. I'm not sure the correct solution for what this method wants to wait for. Or if the tests should actually be generating frames, and there's a bug here. sunxd@ any thoughts?
,
May 31 2018
I spoke with flackr@ offline this morning.
One concern is why there are no frames being submitted. The tests should be having a visual change, so we'd expect frames to be produced. Could you investigate that?
Regarding the actual waiting required for the test:
- The Renderer main thread processes all DOM updates.
- 1st pass will update the touch action in the parent, which enqueues changes on the child
- 2nd pass will allow the child to update its touch action.
- MainThreadFrameObserver enqueues and event at the end of the Renderer main thread. Which allows for synchronizing after each of those DOM updates.
So it should be sufficient to simply wait on the two MainThreadFrameObserver::Wait calls without the WaitForChildFrameSurfaceReady, and without any RenderFrameSubmissionObserver usage.
,
Jun 15 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e9c5a518d8df19ed94a40f095909de227c57efbd commit e9c5a518d8df19ed94a40f095909de227c57efbd Author: jonross <jonross@chromium.org> Date: Fri Jun 15 16:13:22 2018 Update how SitePerProcessBrowserTouchActionTest Waits for Hit Testing Data This change updates how each of the SitePerProcessBrowserTouchActionTest waits for hit testing data, using the new HitTestRegionObserver apis which also work with Viz. This also updates the helper method WaitForTouchActionUpdated to no longer wait for child surfaces, as these calls were not actually doing anything. And they also don't work in Viz. These tests currently aren't actually generating frames, and syncing the renderer main thread should be sufficient. TEST=SitePerProcessBrowserTouchActionTest* Bug: 848021 Change-Id: Ibda33721785cda1202be0ede09bc393edf1b341a Reviewed-on: https://chromium-review.googlesource.com/1099036 Reviewed-by: Ken Buchanan <kenrb@chromium.org> Reviewed-by: Robert Flack <flackr@chromium.org> Commit-Queue: Jonathan Ross <jonross@chromium.org> Cr-Commit-Position: refs/heads/master@{#567678} [modify] https://crrev.com/e9c5a518d8df19ed94a40f095909de227c57efbd/content/browser/site_per_process_browsertest.cc [modify] https://crrev.com/e9c5a518d8df19ed94a40f095909de227c57efbd/testing/buildbot/filters/viz.content_browsertests.filter
,
Jun 15 2018
So the test has now been updated to wait on both the root and the child's renderer main threads. This gets us the synchronization desired for the touch action updates. I'm marking this as fixed. However feel free to open if there is a desire for either frame to be producing compositor frames. As right now they are not being marked as damaged. |
||
►
Sign in to add a comment |
||
Comment 1 by sunxd@chromium.org
, May 30 2018