Closed Bug 1468804 Opened 6 years ago Closed 6 years ago

Remove APZCTesterBase::APZCPanNoFling

Categories

(Core :: Panning and Zooming, enhancement, P3)

Product:
Core
Component:
Panning and Zooming
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: botond, Assigned: o0ignition0o, Mentored)

Details

(Keywords: good-first-bug, Whiteboard: [gfx-noted] [lang=c++])

Attachments

(1 file)

Bug 1468804 - Remove APZCTesterBase::APZCPanNoFling.
59 bytes, text/x-review-board-request
botond
: review+
Details 
APZCTesterBase::APZCPanNoFling() is a very light wrapper around APZCTesterBase::Pan() that just performs one extra operation (calls CancelAnimation() after [1]).

Now that Pan() supports a PanOptions bit-flags parameter, let's just add a new option for whether to fling or not, and remove APZCPanNoFling().

[1] https://searchfox.org/mozilla-central/rev/40577076a6e7a14d143725d0288353c250ea2b16/gfx/layers/apz/test/gtest/APZTestCommon.h#639
Hi, I've been trying to move the flag around, but I'm running into an issue : 

I can't find a Gtest actually using it (I tried commenting the cancelAnimation() call, and tests still seem to run fine.)

Could you please provide me a pointer to how to run a test that would fail if the call to cancelAnimation is commented out ? 

If there is none, could you please point me to a way I could write one ? 

Thanks a lot,
Jeremy
When I try commenting out the CancelAnimation() call, I get one failing test: APZHitTestingTester.TestRepaintFlushOnNewInputBlock. Run with:

  ./mach gtest APZHitTestingTester.TestRepaintFlushOnNewInputBlock

I realize that it's used by more tests than this one; likely the others don't strictly need it because to actually get scroll position changes from the fling animation, you need to sample it by calling AdvanceBy() or something similar.
Oh I tried to use APCZ* as a filter, thanks for the pointer, I'll have a look at it tonight :)
Ok so after a bit of digging, it appears that the Pan function is templated against InputReceiver as shown here (https://searchfox.org/mozilla-central/rev/40577076a6e7a14d143725d0288353c250ea2b16/gfx/layers/apz/test/gtest/APZTestCommon.h#385)

In order to add a PanOptions::NoFling mask and call CancelAnimation(), I think I should :
Define a void CancelAnimation(); method in InputUtils.h (https://searchfox.org/mozilla-central/source/gfx/layers/apz/test/gtest/InputUtils.h)

which would call   
void CancelAnimation(const ScrollableLayerGuid &aGuid); for APCZTreeManager (https://searchfox.org/mozilla-central/source/gfx/layers/apz/src/APZCTreeManager.h#335)
So I would have to dig and figure out how to get this LayerGuid reference.

and void CancelAnimation(CancelAnimationFlags aFlags = Default); for AsyncPanZoomController (https://searchfox.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.h#404)
This part would be easier since there's a default flag.


I then would be able to call CancelAnimation for any InputReceiver, and add this call to the Pan method.

It sounds a bit overkill to me, am I being mistaken somewhere ? 

Thanks for your time
Flags: needinfo?(botond)
Good point about Pan() being templated, I hadn't considered that.

There's no need to add a helper function in InputUtils.h. The GTest code uses derived classes of both AsyncPanZoomController [1] and APZCTreeManager [2] to which we can add methods at will to arrive at a common interface.

In this case, we can add a zero-argument CancelAnimation() method to TestAPZCTreeManager, which will then be compatible with the existing CancelAnimation() method in AsyncPanZoomController for the zero-argument call that we want to make.

(We don't actually need to implement TestAPZCTreeManager::CancelAnimation() right now, as there are no existing call sites that would call Pan() with the NoFling option. We can just make its implementation be:

  EXPECT_TRUE(false, "This is not currently implemented.");

for now.)

[1] https://searchfox.org/mozilla-central/rev/93d2b9860b3d341258c7c5dcd4e278dea544432b/gfx/layers/apz/test/gtest/APZTestCommon.h#226
[2] https://searchfox.org/mozilla-central/rev/93d2b9860b3d341258c7c5dcd4e278dea544432b/gfx/layers/apz/test/gtest/APZTestCommon.h#199
Flags: needinfo?(botond)
(Also, I'll take the fact that you're asking these questions as an indication that you're working on this, and assign the bug to you. Hope that's cool :)).
Assignee: nobody → jeremy.lempereur
Sure, no problem I'm glad I can help and improve my c++ / moz codebase knowledge :) 

Here's a first shot, let me know if I can improve anything :)
Comment on attachment 8987369 [details]
Bug 1468804 - Remove APZCTesterBase::APZCPanNoFling.

https://reviewboard.mozilla.org/r/252602/#review259032

::: gfx/layers/apz/test/gtest/APZTestCommon.h:425
(Diff revision 1)
>                           int aTouchEndY,
>                           bool aExpectConsumed,
>                           nsTArray<uint32_t>* aAllowedTouchBehaviors,
>                           uint64_t* aOutInputBlockId = nullptr);
>  
> -  void ApzcPanNoFling(const RefPtr<TestAsyncPanZoomController>& aApzc,
> +  void Pan(const RefPtr<TestAsyncPanZoomController>& aApzc,

Is there a reason we need this function rather than just using the templated Pan() function?

::: gfx/layers/apz/test/gtest/APZTestCommon.h:602
(Diff revision 1)
>    }
>    if (aOutEventStatuses) {
>      (*aOutEventStatuses)[3] = status;
>    }
>  
> +  if((aOptions & PanOptions::NoFling)) {

Please add a space after 'if'
Attachment #8987369 - Flags: review?(botond)
(In reply to Botond Ballo [:botond] from comment #9)
> ::: gfx/layers/apz/test/gtest/APZTestCommon.h:425
> (Diff revision 1)
> >                           int aTouchEndY,
> >                           bool aExpectConsumed,
> >                           nsTArray<uint32_t>* aAllowedTouchBehaviors,
> >                           uint64_t* aOutInputBlockId = nullptr);
> >  
> > -  void ApzcPanNoFling(const RefPtr<TestAsyncPanZoomController>& aApzc,
> > +  void Pan(const RefPtr<TestAsyncPanZoomController>& aApzc,
> 
> Is there a reason we need this function rather than just using the templated
> Pan() function?

Oh, I guess some call sites are passing in a plain pointer rather than a RefPtr, and with a non-template function that converts fine, but with the template function you can a deduction error.

I'd still prefer getting rid of this function, and making any necessary adjustments to the call sites to they can successfully call the templated function. In most cases, that will just involve changing the type of the local variable being passed in as the first parameter to be a RefPtr rather than a plain pointer. There are also a couple of call sites that pass an argument for the |aOutInputBlockId| parameter, which will need to be adjusted since the templated function has some extra parameters before that.
Hey, thanks for your review.
I was about to say that there was a weird error,and I lacked c++ knowledge to address this in a clean way. 

> could not deduce template argument for 'const RefPtr<T> &' from 'TestAsyncPanZoomController *'

I'll try to adjust to your last comment, which seems to tell me how I'm supposed to proceed to get rid of this definition :)
(In reply to Jeremy Lempereur [:o0ignition0o] from comment #11)
> Hey, thanks for your review.
> I was about to say that there was a weird error,and I lacked c++ knowledge
> to address this in a clean way. 
> 
> > could not deduce template argument for 'const RefPtr<T> &' from 'TestAsyncPanZoomController *'

Let me explain this error a bit. I'll simplify the signatures to only include the first parameter, since that's the only relevant one.

RefPtr<T> can be constructed from a T*, so if you have a non-template function like this:

void Pan(RefPtr<TestAsyncPanZoomController>);

and you call it with an argument of type TestAsyncPanZoomController*, that will work.

With a function template, however, the compiler must first deduce the template arguments before considering things like conversions. Normally, deduction works like this: you have a function template:

template <typename InputReceiver>
void Pan(RefPtr<InputReceiver>);

and you call it with an argument of type RefPtr<TestAsyncPanZoomController>. The compiler compares the parameter type, RefPtr<InputReceiver>, to the argument type, RefPtr<TestAsyncPanZoomController>, and deduces that InputReceiver=TestAsyncPanZoomController.

However, if the parameter type is RefPtr<InputReceiver> and the argument type is TestAsyncPanZoomController*, the compiler doesn't know how to compare those, and it can't deduce the type of InputReceiver.

Does that clarify the error a bit?
Comment on attachment 8987369 [details]
Bug 1468804 - Remove APZCTesterBase::APZCPanNoFling.

https://reviewboard.mozilla.org/r/252602/#review259034

Looks good, thanks!
Attachment #8987369 - Flags: review?(botond) → review+
I pushed the patch to our Try server to make sure it's building on all platforms and passing tests:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bfc72bbdcb81333b85c3988d610fa12ded5a7520&selectedJob=184616247
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/14b72cf25160
Remove APZCTesterBase::APZCPanNoFling. r=botond
The Try push is looking good, so I went ahead and pushed the patch to our integration branch. It should merge into mozilla-central within a day or so.
https://hg.mozilla.org/mozilla-central/rev/14b72cf25160
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
It makes more sense now thanks to your explanation :)

One last question if I may : 

I see lots of variables have a prefix, ie : an "options" variable becomes aOptions in the code, is there a list of prefixes somewhere so I can figure out what kind of additional info they provide ? 

For example m_foo means foo is a private class member I guess ?
(In reply to Jeremy Lempereur [:o0ignition0o] from comment #19)
> One last question if I may : 
> 
> I see lots of variables have a prefix, ie : an "options" variable becomes
> aOptions in the code, is there a list of prefixes somewhere so I can figure
> out what kind of additional info they provide ?

Yep, see the "Prefixes" section of our style guide [1].

As you can see, the patch has now merged to mozilla-central. Thanks for your work here! If you're interested if working on some other bugs and would like some suggestions, let me know.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style
I do ! \o/
I had a look through my list of mentored bugs, and all the suitable ones are taken at the moment. However, there are plenty of bugs that other people are mentoring. This site allows you to browse them by language and area of the code:

https://www.joshmatthews.net/bugsahoy/
(In reply to Botond Ballo [:botond] from comment #22)
> I had a look through my list of mentored bugs, and all the suitable ones are
> taken at the moment.

Actually, I just filed bug 1471708 which might be a good one. Let me know if you're interested in working on it!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: