Closed Bug 1417519 Opened 7 years ago Closed 7 years ago

Allow APZ to use webrender hit-testing code

Categories

(Core :: Graphics: WebRender, enhancement, P1)

Product:
Core
Component:
Graphics: WebRender
Version:
Other Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

(Whiteboard: [wr-mvp] [gfx-noted])

Attachments

(4 files)

Bug 1417519 - Log layers id as a hex value.
59 bytes, text/x-review-board-request
botond
: review+
Details
Bug 1417519 - Don't allow things to get raw pointers to WebRenderAPI.
59 bytes, text/x-review-board-request
sotaro
: review+
Details
Bug 1417519 - Add a mechanism for APZ to get the WebRenderAPI instance.
59 bytes, text/x-review-board-request
botond
: review+
Details
Bug 1417519 - Hook up APZ to use the hit-testing API.
59 bytes, text/x-review-board-request
botond
: review+
mstange
: review+
Details 
Bug 1389149 allows us to send hit-testing info to webrender. This bug is to have APZ utilize webrender to do hit-testing.
Status: NEW → ASSIGNED
Priority: P3 → P1
Whiteboard: [gfx-noted][wr-mvp][triage] → [wr-mvp] [gfx-noted]
Comment on attachment 8929507 [details]
Bug 1417519 - Hook up APZ to use the hit-testing API.

https://reviewboard.mozilla.org/r/200788/#review206048
Attachment #8929507 - Flags: review?(mstange) → review+
Comment on attachment 8929504 [details]
Bug 1417519 - Log layers id as a hex value.

https://reviewboard.mozilla.org/r/200782/#review206082
Attachment #8929504 - Flags: review?(botond) → review+
Comment on attachment 8929506 [details]
Bug 1417519 - Add a mechanism for APZ to get the WebRenderAPI instance.

https://reviewboard.mozilla.org/r/200786/#review206092
Attachment #8929506 - Flags: review?(botond) → review+
Comment on attachment 8929507 [details]
Bug 1417519 - Hook up APZ to use the hit-testing API.

https://reviewboard.mozilla.org/r/200788/#review206096

::: gfx/layers/apz/src/APZCTreeManager.h:526
(Diff revision 1)
>    AsyncPanZoomController* GetTargetApzcForNode(HitTestingTreeNode* aNode);
>    AsyncPanZoomController* GetAPZCAtPoint(HitTestingTreeNode* aNode,
>                                           const ParentLayerPoint& aHitTestPoint,
>                                           HitTestResult* aOutHitResult,
>                                           HitTestingTreeNode** aOutScrollbarNode);
> +  already_AddRefed<AsyncPanZoomController> GetAPZCAtPointWR(const ScreenPoint& aPoint,

It feels weird for GetAPZCAtPoint() to take a ParentLayerPoint but GetAPZCAtPointWR() to take a ScreenPoint.

GetAPZCAtPoint() has a single call site, in GetTargetAPZC(), where the input ParentLayerPoint is obtained from a ScreenPoint via a cast with justification "ScreenIsParentLayerForRoot".

I would prefer that we change GetAPZCAtPoint() to take its input as a ScreenPoint, to match GetAPZCAtPointWR(), and perform the cast inside GetAPZCAtPoint(). (Back when GetAPZCAtPoint() was recursive, we couldn't do this without introducing a helper, but now that it uses ForEachNode, we can.)

::: gfx/layers/apz/src/APZCTreeManager.cpp:2203
(Diff revision 1)
> -  RefPtr<AsyncPanZoomController> target = GetAPZCAtPoint(mRootNode, point,
> -      &hitResult, &scrollbarNode);
> +    target = GetAPZCAtPoint(mRootNode, point, &hitResult, &scrollbarNode);
> +  }
> +
> +  if (gfxPrefs::WebRenderHitTest()) {
> +    HitTestResult wrHitResult = HitNothing;
> +    RefPtr<AsyncPanZoomController> wrTarget = GetAPZCAtPointWR(aPoint, wrHitResult);

I assume doing both hit tests is a temporary state for testing?

::: gfx/layers/apz/src/APZCTreeManager.cpp:2230
(Diff revision 1)
>    return target.forget();
>  }
>  
> +already_AddRefed<AsyncPanZoomController>
> +APZCTreeManager::GetAPZCAtPointWR(const ScreenPoint& aPoint,
> +                                  HitTestResult& aOutHitResult)

For consistency with GetAPZCAtPoint(), use a pointer for the out-param.

::: gfx/layers/apz/src/APZCTreeManager.cpp:2257
(Diff revision 1)
> +    result = FindRootApzcForLayersId(layersId);
> +    MOZ_ASSERT(result);
> +  }
> +
> +  aOutHitResult = HitLayer;
> +  if (hitInfo & gfx::CompositorHitTestInfo::eDispatchToContent) {

Not strictly related to this patch, but can the enums HitTestResult and CompositorHitTestInfo be unified?
Attachment #8929507 - Flags: review?(botond) → review+
Comment on attachment 8929505 [details]
Bug 1417519 - Don't allow things to get raw pointers to WebRenderAPI.

https://reviewboard.mozilla.org/r/200784/#review206162
Attachment #8929505 - Flags: review?(sotaro.ikeda.g) → review+
(In reply to Botond Ballo [:botond] from comment #10)
> It feels weird for GetAPZCAtPoint() to take a ParentLayerPoint but
> GetAPZCAtPointWR() to take a ScreenPoint.
> 
> GetAPZCAtPoint() has a single call site, in GetTargetAPZC(), where the input
> ParentLayerPoint is obtained from a ScreenPoint via a cast with
> justification "ScreenIsParentLayerForRoot".
> 
> I would prefer that we change GetAPZCAtPoint() to take its input as a
> ScreenPoint, to match GetAPZCAtPointWR(), and perform the cast inside
> GetAPZCAtPoint(). (Back when GetAPZCAtPoint() was recursive, we couldn't do
> this without introducing a helper, but now that it uses ForEachNode, we can.)

Good point, fixed.

> ::: gfx/layers/apz/src/APZCTreeManager.cpp:2203
> > +    RefPtr<AsyncPanZoomController> wrTarget = GetAPZCAtPointWR(aPoint, wrHitResult);
> 
> I assume doing both hit tests is a temporary state for testing?

Yup.

> ::: gfx/layers/apz/src/APZCTreeManager.cpp:2230
> > +                                  HitTestResult& aOutHitResult)
> 
> For consistency with GetAPZCAtPoint(), use a pointer for the out-param.

Done

> ::: gfx/layers/apz/src/APZCTreeManager.cpp:2257
> > +  aOutHitResult = HitLayer;
> > +  if (hitInfo & gfx::CompositorHitTestInfo::eDispatchToContent) {
> 
> Not strictly related to this patch, but can the enums HitTestResult and
> CompositorHitTestInfo be unified?

I guess they could be. I'd go in the direction of removing HitTestResult and using CompositorHitTestInfo everywhere, since I'm going to be adding more stuff to CompositorHitTestInfo for scrollbars. I can file a follow-up for that.
Filed bug 1418541 for the unification.
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2eaf8ed1338b
Log layers id as a hex value. r=botond
https://hg.mozilla.org/integration/autoland/rev/273eac46d15c
Don't allow things to get raw pointers to WebRenderAPI. r=sotaro
https://hg.mozilla.org/integration/autoland/rev/f9e128e8c8a6
Add a mechanism for APZ to get the WebRenderAPI instance. r=botond
https://hg.mozilla.org/integration/autoland/rev/ff8e0f72dd9d
Hook up APZ to use the hit-testing API. r=botond,mstange
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: