Restrict MainThreadFrameObserver to Javascript Syncing |
||
Issue description
content::browser_test_utils::MainThreadFrameObserver is a bit of a misnomer.
It sounds like it waits for a compositor frame to be generated, but this is not always true.
What it actually does:
- sends signal to the renderer's main thread.
- enqueues a response at the end of the renderer's main thread
- waits for SwapPromise to complete on either
- swap complete: before compositor frame submitted to Viz
- did not swap: no frame produced
This is great when a test wants to run some javascript on the main thread and await its completion.
This misleads when wanting to synchronize on actual frame production.
Since RenderFrameSubmissionObserver now synchronizes on frame submission between the Renderer Browser and Viz processes, it should become the default way to wait for visuals.
MainThreadFrameObserver should be restricted to only Javascript cases, and also renamed for clairty.
,
Jul 11
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5bc3c11ce575df33970f5b0f89160f38878ae9ce commit 5bc3c11ce575df33970f5b0f89160f38878ae9ce Author: Dirk Pranke <dpranke@chromium.org> Date: Wed Jul 11 23:53:03 2018 Revert "Update Chrome callsites that use MainThreadFrameObserver" This reverts commit cb82e2e48313e755713f169fa151199fec459a02. Reason for revert: Test is failing on multiple builders, e.g.: https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20ASan%20Tests%20%28sandboxed%29/48812 Original change's description: > Update Chrome callsites that use MainThreadFrameObserver > > MainThreadFrameObserver does not guarantee that a compositor frame was actually > submitted. It however does synchronize the Renderer's main thread, which allows > for syncing Javascript execution in tests. > > This updates two tests in chrome/browser to use RenderFrameSubmissionObserver > for synchronizing on compositor frame submission. As well as InputEventAckWaiter > for synchronizing on input event processing. > > TEST=WebViewTest.InterstitialPageFocusedWidget, > WebViewBrowserPluginSpecificInteractiveTest.EnsureFocusSynced > > Bug: 862683 > Change-Id: Iae40b92dedf656898b9e77c6529ef118d89e0a6e > Reviewed-on: https://chromium-review.googlesource.com/1133960 > Reviewed-by: Fady Samuel <fsamuel@chromium.org> > Commit-Queue: Jonathan Ross <jonross@chromium.org> > Cr-Commit-Position: refs/heads/master@{#574335} TBR=jonross@chromium.org,fsamuel@chromium.org Change-Id: Ica0e795ebad4dd81248a8e89b6f02c53e972a203 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 862683 Reviewed-on: https://chromium-review.googlesource.com/1133779 Reviewed-by: Dirk Pranke <dpranke@chromium.org> Commit-Queue: Dirk Pranke <dpranke@chromium.org> Cr-Commit-Position: refs/heads/master@{#574414} [modify] https://crrev.com/5bc3c11ce575df33970f5b0f89160f38878ae9ce/chrome/browser/apps/guest_view/web_view_browsertest.cc [modify] https://crrev.com/5bc3c11ce575df33970f5b0f89160f38878ae9ce/chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc
,
Jul 12
Some tests aren't working with child/guest frames, and actually want to block on hit testing data. Once viz hit testing is on by default they can become HitTestRegionObserver: BrowserSideFlingBrowserTest CompositedScrollingBrowserTest CompositorEventAckBrowserTest MainThreadEventQueueBrowserTest ScrollLatencyBrowserTest TouchpadPinchBrowserTest Some tests aren't actually generating frames, as there is no damage, so they are relying on WillNotSwap RenderWidgetHostViewBrowserTestBase.CompositorWorksWhenReusingRenderer sometimes needs WillNotSwap WebViewTest.InterstitialPageFocusedWidget SitePerProcessBrowserTests is a mix of frame waiting and hit testing SitePerProcessHitBrowserTests is a mix of frame waiting, hit testing and javascript syncing
,
Jan 14
,
Jan 14
Some context to get started.
The legacy test api is
MainThreadFrameObserver:
https://cs.chromium.org/chromium/src/content/public/test/browser_test_utils.h?rcl=5da1d21f332c756024e5cc65f2da9ae8a4ee529f&l=1175
The new way to wait for generic frame submission is
RenderFrameSubmissionObserver:
https://cs.chromium.org/chromium/src/content/public/test/browser_test_utils.h?rcl=5da1d21f332c756024e5cc65f2da9ae8a4ee529f&l=1096
The new way to wait for hit test data is
HitTestFegionObserver:
https://cs.chromium.org/chromium/src/content/public/test/hit_test_region_observer.h
,
Jan 16
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/762fb87264adbc2b298997a9114446b801122eea commit 762fb87264adbc2b298997a9114446b801122eea Author: Edwin Joe <ejoe@google.com> Date: Wed Jan 16 00:43:40 2019 Changed how CompositedScrollingBrowserTest waits for the URL to finish loading. Replaced MainThreadFrameObserver with HitTestRegionObserver on LoadURL method. The test used MainThreadFrameObserver class to wait for the test page to load before interacting with it. A more appropriate class to use is the new HitTestRegionObserver to directly wait for the hit test data. Test=CompositedScrollingBrowserTest* Bug: 862683 Change-Id: Iab243a5c27c32d8daed9b922f61ed61498be0ce2 Reviewed-on: https://chromium-review.googlesource.com/c/1412373 Reviewed-by: Jonathan Ross <jonross@chromium.org> Reviewed-by: Dave Tapuska <dtapuska@chromium.org> Commit-Queue: Edwin Joe <ejoe@google.com> Cr-Commit-Position: refs/heads/master@{#622942} [modify] https://crrev.com/762fb87264adbc2b298997a9114446b801122eea/content/browser/renderer_host/input/composited_scrolling_browsertest.cc
,
Jan 16
(6 days ago)
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3ffd6f6a6061fd407ea2ceacb36424407cb83045 commit 3ffd6f6a6061fd407ea2ceacb36424407cb83045 Author: Edwin Joe <ejoe@google.com> Date: Wed Jan 16 15:37:49 2019 Changed how ScrollLatencyBrowserTest waits for URL to load - Changed the LoadURL method to wait for the URL hit test data using HitTestFrameObserver instead of MainThreadFrameObserver. - Removed the WaitAFrame() method as it is not used anywhere else except in LoadURL. - Removed MainThreadFrameObserver unique_ptr from the private field since it is not used anywhere else except in LoadURL. Test: ScrollLatencyBrowserTest* Bug: 862683 Change-Id: I837fb52611a9d7f520780d004e230db6a3bdbb18 Reviewed-on: https://chromium-review.googlesource.com/c/1412803 Reviewed-by: Jonathan Ross <jonross@chromium.org> Reviewed-by: Dave Tapuska <dtapuska@chromium.org> Commit-Queue: Edwin Joe <ejoe@google.com> Cr-Commit-Position: refs/heads/master@{#623245} [modify] https://crrev.com/3ffd6f6a6061fd407ea2ceacb36424407cb83045/content/browser/renderer_host/input/scroll_latency_browsertest.cc
,
Jan 16
(6 days ago)
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/59c4d4d293aef3ff66a5ab900b6613704a0082be commit 59c4d4d293aef3ff66a5ab900b6613704a0082be Author: Edwin Joe <ejoe@google.com> Date: Wed Jan 16 16:41:33 2019 Changed how CompositorEventAckBrowserTest, MainThreadEventQueueBrowserTest, and TouchpadPinchBrowserTest waits for URL to load. Replaced MainThreadFrameObserver with HitTestRegionObserver to wait for hit test data to be ready. Test: CompositorEventBrowserTest*,MainThreadEventQueueBrowserTest*,TouchpadPinchBrowserTest* Bug: 862683 Change-Id: Ic4c76d0ec2acbfdc30959c476b80efa664198dc0 Reviewed-on: https://chromium-review.googlesource.com/c/1412626 Reviewed-by: Jonathan Ross <jonross@chromium.org> Reviewed-by: Navid Zolghadr <nzolghadr@chromium.org> Commit-Queue: Edwin Joe <ejoe@google.com> Cr-Commit-Position: refs/heads/master@{#623257} [modify] https://crrev.com/59c4d4d293aef3ff66a5ab900b6613704a0082be/content/browser/renderer_host/input/compositor_event_ack_browsertest.cc [modify] https://crrev.com/59c4d4d293aef3ff66a5ab900b6613704a0082be/content/browser/renderer_host/input/main_thread_event_queue_browsertest.cc [modify] https://crrev.com/59c4d4d293aef3ff66a5ab900b6613704a0082be/content/browser/renderer_host/input/touchpad_pinch_browsertest.cc |
||
►
Sign in to add a comment |
||
Comment 1 by bugdroid1@chromium.org
, Jul 11