New issue
Advanced search Search tips

Issue 785986 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug


Sign in to add a comment

Viz needs HitTestRegion Testing Api

Project Member Reported by jonr...@chromium.org, Nov 16 2017

Issue description

There are a lot of content_browsertests which currently wait for a frame to be produced.

They use this as a signal that input can be sent to tests.

With --enable-viz the frame production is no longer visible, so current test helpers do not work.

We should create a proper api to observe that hit test regions have been received by Viz. Then port tests to that.
 
Components: Internals>Services>Viz
Owner: jonr...@chromium.org
Blocking: 785013
Cc: riajiang@chromium.org
Hey riajjiang@

I was curious about the state of the hit testing refactor. Currently there are tests which want to know that hit-testing is ready. They are keying off of frame submission, but would it be possible for them to key off of hit test data submission?

Will there be a path in both class/viz where the browser process is aware of hit testing submission?
By "hit testing submission", do you mean when Viz service have only received the hit-test data, or do you also need to wait until all the aggregation is done and that hit-test data is ready for Viz host to use for targeting?
The tests would be interested in knowing when all aggregation is done. As they wish to block until that point. Then begin sending input for their test cases
https://cs.chromium.org/chromium/src/components/viz/host/hit_test/hit_test_query.cc?type=cs&q=hit_test_query.cc&l=24 is where Viz host (browser) has been notified of which hit-test data block it should be using for targeting - maybe content_browsertests can observe that?
Cc: jonr...@chromium.org
Owner: riajiang@chromium.org
Status: Assigned (was: Untriaged)
As discussed in triage:
  We would like a testing api, which would allow tests to block until hit test regions have been submitted. As well as to block until hit test regions update.

  Currently many tests rely on compositor frame submission to signify this, but that is racy, and not accurate.
Blocking: 763452
I did some investigation into this.

Relying solely on compositor frame submission is too racy. The browser process receives the frame tokens from Viz far too early.

components/viz/host/hit_test/hit_test_query.cc receives notifications of when the aggregated region changes. This is triggered later on in the viz process.

It looks like the current WaitForChildSurfaceReady will be replaceable with a combination of:
  - RenderFrameSubmissionObserver waiting for frame submission from the root and child
  - Waiting for the next HitTestQuery::SwitchActiveAggregatedHitTestRegionList to arrive in the browser.

Once the hit testing work is on by default this can be used for classic and viz paths.

Though we will need a refactor so that browser_test_utils can observer changes in HitTestQuery
Planning to finch trial new hit testing path on April 3rd.
Targeting M67 Stable launch in May
riajiang@ has example of manipulating HitTestQuery from browser_test_utils, can use as basis for test listener.
Cc: rjkroege@chromium.org sadrul@chromium.org
Blockedon: 846113
Owner: jonr...@chromium.org
Status: Started (was: Assigned)
Blockedon: 846798

Comment 14 by sunxd@chromium.org, May 25 2018

Blockedon: -846798
Blockedon: 848021
Blockedon: 848325
Blockedon: 848348
Blockedon: 848825
Project Member

Comment 19 by bugdroid1@chromium.org, Jun 12 2018

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

commit d6b47f3af0860e976a37a06d56d8a8717cd1288b
Author: jonross <jonross@chromium.org>
Date: Tue Jun 12 19:43:39 2018

Create HitTest Region Testing API

This creates a new observer in viz, viz::HitTestRegionObserver, which can be used to listen for the arrival of hit test data within a Viz Host.
This also creates a test class, viz::HitTestRegionObserver, which can be used by tests to wait upon hit test data. Since the Viz hit test path is not yet on by default, this has fallbacks to classic helpers which wait on cc::Surface embedding.

Furthermore a series of existing callsites of WaitForChildSurfaceReady and WaitForGuestSurfaceReady have been converted to use the new api. They are being re-enable for Viz test configs.

TBR=fsamuel@chromium.org
TEST=viz_browser_tests, viz_content_browsertests

Bug:  785986 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: I77889df44a96d81a37d4fb26255401c433069531
Reviewed-on: https://chromium-review.googlesource.com/1071886
Commit-Queue: Jonathan Ross <jonross@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Reviewed-by: dsinclair <dsinclair@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566534}
[modify] https://crrev.com/d6b47f3af0860e976a37a06d56d8a8717cd1288b/chrome/browser/apps/guest_view/web_view_browsertest.cc
[modify] https://crrev.com/d6b47f3af0860e976a37a06d56d8a8717cd1288b/chrome/browser/devtools/devtools_sanity_browsertest.cc
[modify] https://crrev.com/d6b47f3af0860e976a37a06d56d8a8717cd1288b/chrome/browser/pdf/pdf_extension_test.cc
[modify] https://crrev.com/d6b47f3af0860e976a37a06d56d8a8717cd1288b/components/viz/host/BUILD.gn
[modify] https://crrev.com/d6b47f3af0860e976a37a06d56d8a8717cd1288b/components/viz/host/hit_test/hit_test_query.cc
[modify] https://crrev.com/d6b47f3af0860e976a37a06d56d8a8717cd1288b/components/viz/host/hit_test/hit_test_query.h
[add] https://crrev.com/d6b47f3af0860e976a37a06d56d8a8717cd1288b/components/viz/host/hit_test/hit_test_region_observer.h
[modify] https://crrev.com/d6b47f3af0860e976a37a06d56d8a8717cd1288b/components/viz/host/host_frame_sink_manager.cc
[modify] https://crrev.com/d6b47f3af0860e976a37a06d56d8a8717cd1288b/components/viz/host/host_frame_sink_manager.h
[modify] https://crrev.com/d6b47f3af0860e976a37a06d56d8a8717cd1288b/content/browser/site_per_process_hit_test_browsertest.cc
[modify] https://crrev.com/d6b47f3af0860e976a37a06d56d8a8717cd1288b/content/public/test/DEPS
[modify] https://crrev.com/d6b47f3af0860e976a37a06d56d8a8717cd1288b/content/public/test/browser_test_utils.cc
[modify] https://crrev.com/d6b47f3af0860e976a37a06d56d8a8717cd1288b/content/public/test/browser_test_utils.h
[add] https://crrev.com/d6b47f3af0860e976a37a06d56d8a8717cd1288b/content/public/test/hit_test_region_observer.cc
[add] https://crrev.com/d6b47f3af0860e976a37a06d56d8a8717cd1288b/content/public/test/hit_test_region_observer.h
[modify] https://crrev.com/d6b47f3af0860e976a37a06d56d8a8717cd1288b/content/test/BUILD.gn
[modify] https://crrev.com/d6b47f3af0860e976a37a06d56d8a8717cd1288b/extensions/browser/guest_view/web_view/web_view_apitest.cc
[modify] https://crrev.com/d6b47f3af0860e976a37a06d56d8a8717cd1288b/testing/buildbot/filters/viz.browser_tests.filter
[modify] https://crrev.com/d6b47f3af0860e976a37a06d56d8a8717cd1288b/testing/buildbot/filters/viz.content_browsertests.filter

Project Member

Comment 20 by bugdroid1@chromium.org, Jun 13 2018

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

commit 09d21dec87601ff5c94b20c445aa642554d48f4d
Author: jonross <jonross@chromium.org>
Date: Wed Jun 13 12:31:36 2018

Update More content_browsertests to use new hit test api

This updates some of SitePerProcessBrowserTests, PointerLockBrowserTests, and TouchSelectionForCrossProcessFramesTests to use the new hit testing api.

Based on: https://chromium-review.googlesource.com/c/chromium/src/+/1071886

TBR=piman@chromium.org
TEST= SitePerProcessBrowserTests,
PointerLockBrowserTests,
TouchSelectionForCrossProcessFramesTests

Bug:  785986 
Change-Id: Ic3d7aca3f214d0ca0c1952e1f29b84a5ac31e13c
Reviewed-on: https://chromium-review.googlesource.com/1082573
Commit-Queue: Jonathan Ross <jonross@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566806}
[modify] https://crrev.com/09d21dec87601ff5c94b20c445aa642554d48f4d/content/browser/pointer_lock_browsertest.cc
[modify] https://crrev.com/09d21dec87601ff5c94b20c445aa642554d48f4d/content/browser/renderer_host/input/touch_selection_controller_client_aura_browsertest.cc
[modify] https://crrev.com/09d21dec87601ff5c94b20c445aa642554d48f4d/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/09d21dec87601ff5c94b20c445aa642554d48f4d/testing/buildbot/filters/viz.content_browsertests.filter

Project Member

Comment 21 by bugdroid1@chromium.org, Jun 13 2018

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

commit 9585e016ecd3f5fb8f822d45f091ba3ec17c0a0d
Author: jonross <jonross@chromium.org>
Date: Wed Jun 13 16:52:26 2018

Update Interactive UI Tests Hit Test Waiting

A few sites in interactive_ui_tests were using WaitForChildSurfaceReady to wait
for hit test data. This change updates them to use the new hit test data api.

TEST=CrossSiteSubframe/DragAndDropBrowserTest.DragStartInFrame/0,
CrossSiteSubframe/DragAndDropBrowserTest.DropTextFromOutside/0,
SitePerProcessInteractiveBrowserTest.TabAndMouseFocusNavigation

TBR=kenrb@chromium.org
TBR=paulmeyer@chromium.org

Bug:  785986 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel
Change-Id: Ic4e3dd3979af4e422e882801b2630f1639144d92
Reviewed-on: https://chromium-review.googlesource.com/1096914
Commit-Queue: Jonathan Ross <jonross@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566874}
[modify] https://crrev.com/9585e016ecd3f5fb8f822d45f091ba3ec17c0a0d/chrome/browser/site_per_process_interactive_browsertest.cc
[modify] https://crrev.com/9585e016ecd3f5fb8f822d45f091ba3ec17c0a0d/chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc

Project Member

Comment 22 by bugdroid1@chromium.org, Jun 13 2018

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

commit d4e1c1cc922e3b89db23ca9ae814f809fa03abe7
Author: jonross <jonross@chromium.org>
Date: Wed Jun 13 18:11:01 2018

Update SitePerProcessHitTest to use the new hit testing api

Based on: https://chromium-review.googlesource.com/c/chromium/src/+/1071886

This change updates all callsites in site_per_process_hit_test_browsertest.cc to use the new api.

TBR=kenrb@chromium.org
TEST=SitePerProcessGestureHitTestBrowserTest,
SitePerProcessHighDPIHitTestBrowserTest,
SitePerProcessInternalsHitTestBrowserTest,
SitePerProcessMouseWheelHitTestBrowserTest,
SitePerProcessHitTestBrowserTest.
SitePerProcessNonIntegerScaleFactorHitTestBrowserTest,
SitePerProcessMouseWheelHitTestBrowserTestWheelScrollLatchingDisabled

Bug:  785986 
Change-Id: I042d7fdf214b8045a9d5265c0fd7af3d547f0882
Reviewed-on: https://chromium-review.googlesource.com/1081998
Commit-Queue: Jonathan Ross <jonross@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566914}
[modify] https://crrev.com/d4e1c1cc922e3b89db23ca9ae814f809fa03abe7/content/browser/site_per_process_hit_test_browsertest.cc
[modify] https://crrev.com/d4e1c1cc922e3b89db23ca9ae814f809fa03abe7/testing/buildbot/filters/viz.content_browsertests.filter

Status: Fixed (was: Started)
HitTestRegionObserver is up and running, all test sites have been converted.

Sign in to add a comment