Closed Bug 1035356 Opened 6 years ago Closed 6 years ago

APZCTM::ReceiveInputEvent should do an in-place modification of the InputData

Categories

(Core :: Panning and Zooming, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(1 file, 3 obsolete files)

APZCTreeManager has two ReceiveInputEvent functions. One takes a WidgetInputEvent& and modifies it in-place. The other takes a const InputData& which is not modified. The WidgetInputEvent version is meant for use on the main thread, and the InputData version is meant for use off the main thread (but is not currently used on any platform). As part of bug 930939 and friends we want to migrate input events off the main thread in some places, so we should modify the InputData version of ReceiveInputEvent to also do the in-place modification.
Attached patch Patch v2 (obsolete) — Splinter Review
Fixed the OS X bustage that showed in the try run. Tested that locally.
Attachment #8451813 - Attachment is obsolete: true
Attachment #8451896 - Flags: review?(mstange)
Attachment #8451896 - Flags: review?(botond)
Attachment #8451896 - Flags: review?(mstange) → review+
Comment on attachment 8451896 [details] [diff] [review]
Patch v2

Review of attachment 8451896 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the comments about const_cast and copying the event addressed.

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +373,5 @@
> +  ApplyTransform(aPoint, transformToApzc * transformToGecko);
> +}
> +
> +void
> +APZCTreeManager::TransformScreenToGecko(ScreenIntPoint* aPoint, AsyncPanZoomController* aApzc)

We could avoid code duplication by making this function a template, at the expense of  needing to place the implementation in the header file.

If we made the template sufficiently general, so that it can operate on Layer[Int]Point as well as Screen[Int]Point, we could replace the calls to ApplyTransform in APZCTReeManager::ProcessEvent() and APZCTreeManager::TransformCoordinateToGecko() with calls to this function as well. (There is also a call in ProcessTouchInput() which it would be nice make consistent with the rest, but that's inside a loop so it's better to keep it the way it is so we only call GetInputTransforms and multiply the matrices once.)

Up to you whether you want to do this.

@@ +389,5 @@
>    gfx3DMatrix transformToApzc;
>    gfx3DMatrix transformToGecko;
>    switch (aEvent.mInputType) {
>      case MULTITOUCH_INPUT: {
> +      MultiTouchInput& touchInput = const_cast<MultiTouchInput&>(aEvent.AsMultiTouchInput());

Instead of these const_casts, let's add overloads of the AsXXXInput() functions which return a non-const reference when called on a non-const object.

That is, we would have two overloads:

  const XXXInput& AsXXXInput() const;  // the current one
  XXXInput& AsXXXInput();              // the new one

This can be achieved with a straightforward modification of the INPUTDATA_AS_CHILD_TYPE macro (http://dxr.mozilla.org/mozilla-central/source/widget/InputData.h#41).

@@ +404,5 @@
>            BuildOverscrollHandoffChain(apzc);
>          }
>          apzc->GetGuid(aOutTargetGuid);
>          GetInputTransforms(apzc, transformToApzc, transformToGecko);
>          PanGestureInput inputForApzc(panInput);

Now that we are modifying the original event, there should be no need to make a copy and transform it separately.

@@ +422,5 @@
>                                                              &inOverscrolledApzc);
>        if (apzc) {
>          apzc->GetGuid(aOutTargetGuid);
>          GetInputTransforms(apzc, transformToApzc, transformToGecko);
>          PinchGestureInput inputForApzc(pinchInput);

Ditto.
Attachment #8451896 - Flags: review?(botond) → review+
(In reply to Botond Ballo [:botond] from comment #3)
> If we made the template sufficiently general, so that it can operate on
> Layer[Int]Point as well as Screen[Int]Point, we could replace the calls to
> ApplyTransform in APZCTReeManager::ProcessEvent() and
> APZCTreeManager::TransformCoordinateToGecko() with calls to this function as
> well. (There is also a call in ProcessTouchInput() which it would be nice
> make consistent with the rest, but that's inside a loop so it's better to
> keep it the way it is so we only call GetInputTransforms and multiply the
> matrices once.)

I tried doing this but then I'd either have to inline the static ApplyTransform code (resulting in another copy of that stuff) or move it into the header file as well. I don't really like that any better.

> 
> Instead of these const_casts, let's add overloads of the AsXXXInput()
> functions which return a non-const reference when called on a non-const
> object.
> 
> That is, we would have two overloads:
> 
>   const XXXInput& AsXXXInput() const;  // the current one
>   XXXInput& AsXXXInput();              // the new one
> 
> This can be achieved with a straightforward modification of the
> INPUTDATA_AS_CHILD_TYPE macro
> (http://dxr.mozilla.org/mozilla-central/source/widget/InputData.h#41).

Makes sense, done.

> >          apzc->GetGuid(aOutTargetGuid);
> >          GetInputTransforms(apzc, transformToApzc, transformToGecko);
> >          PanGestureInput inputForApzc(panInput);
> 
> Now that we are modifying the original event, there should be no need to
> make a copy and transform it separately.

But it's possible that when the APZC handles the event, it changes internal state, so transformToApzc is different. In this case we want to use the latest version of transformToApzc on the original input point when converting to gecko space. In other words, if we do this:

  GetInputTransforms(apzc, transformToApzc, transformToGecko);
  ApplyTransform(&(panInput.mPanStartPoint), transformToApzc);
  apzc->ReceiveInputEvent(panInput);
  ApplyTransform(&(panInput.mPanStartPoint), transformToGecko);

then the returned mPanStartPoint is effectively transformed using a stale transformToApzc. The only other way I can see to address your comment is to unapply transformToApzc after the call apzc->ReceiveInputEvent, before calling TransformScreenToGecko. That seems worse because it will have more rounding error.
Attached patch Patch v3 (obsolete) — Splinter Review
Attachment #8451896 - Attachment is obsolete: true
Attachment #8452136 - Flags: review?(botond)
Comment on attachment 8452136 [details] [diff] [review]
Patch v3

Review of attachment 8452136 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> (In reply to Botond Ballo [:botond] from comment #3)
> > If we made the template sufficiently general, so that it can operate on
> > Layer[Int]Point as well as Screen[Int]Point, we could replace the calls to
> > ApplyTransform in APZCTReeManager::ProcessEvent() and
> > APZCTreeManager::TransformCoordinateToGecko() with calls to this function as
> > well. (There is also a call in ProcessTouchInput() which it would be nice
> > make consistent with the rest, but that's inside a loop so it's better to
> > keep it the way it is so we only call GetInputTransforms and multiply the
> > matrices once.)
> 
> I tried doing this but then I'd either have to inline the static
> ApplyTransform code (resulting in another copy of that stuff) or move it
> into the header file as well. I don't really like that any better.

I just realized you could actually make TransformScreenToGecko a static function in APZCTreeManager.cpp as well. The only member of APZCTreeManager that it accesses is GetInputTransforms(), and that's public.

I would prefer this (independently of whether we then make it a template) because it keeps things out of header files, but I'll leave it up to you as well.

> > >          apzc->GetGuid(aOutTargetGuid);
> > >          GetInputTransforms(apzc, transformToApzc, transformToGecko);
> > >          PanGestureInput inputForApzc(panInput);
> > 
> > Now that we are modifying the original event, there should be no need to
> > make a copy and transform it separately.
> 
> But it's possible that when the APZC handles the event, it changes internal
> state, so transformToApzc is different. In this case we want to use the
> latest version of transformToApzc on the original input point when
> converting to gecko space. In other words, if we do this:
> 
>   GetInputTransforms(apzc, transformToApzc, transformToGecko);
>   ApplyTransform(&(panInput.mPanStartPoint), transformToApzc);
>   apzc->ReceiveInputEvent(panInput);
>   ApplyTransform(&(panInput.mPanStartPoint), transformToGecko);
> 
> then the returned mPanStartPoint is effectively transformed using a stale
> transformToApzc. The only other way I can see to address your comment is to
> unapply transformToApzc after the call apzc->ReceiveInputEvent, before
> calling TransformScreenToGecko. That seems worse because it will have more
> rounding error.

Right. I was confused about several things when I wrote that comment, so disregard it :)

I do think that now that the function parameter is non-const, a comment explaining why we need to copy it here would be helpful. Something along the lines of:

"When passing the event to the APZC, we need to apply a different transform than the one we apply for the caller to see, so we need to make a copy of the event."
Attachment #8452136 - Flags: review?(botond) → review+
Attached patch Patch v4Splinter Review
Moved the function back into the .cpp, made it static and templated. It now also takes an APZCTreeManager* so that it has something to call GetInputTransforms on (as per IRC discussion).

I also rearranged a few lines and added spaces and comments to the handling of the various non-touch InputData types, so that hopefully things are a little cleaner.
Attachment #8452136 - Attachment is obsolete: true
Attachment #8452492 - Flags: review+
Comment on attachment 8452492 [details] [diff] [review]
Patch v4

Review of attachment 8452492 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +364,5 @@
>    aPoint->x = NS_lround(result.x);
>    aPoint->y = NS_lround(result.y);
>  }
>  
> +/*static*/ template<class T> void

Sorry to continue nitpicking, but I think it would be nicer to have BasePoint<T, Sub> rather than an unrestricted template parameter. (Or, at the very least, call the parameter 'Point'.)
argh, i didn't see the above comment and landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d3ec4c615ab4

I'll fix it the next time I touch that code.
https://hg.mozilla.org/mozilla-central/rev/d3ec4c615ab4
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.