Closed Bug 1170062 Opened 5 years ago Closed 4 years ago

Create a common base class for APZCBasicTester and APZCTreeManagerTester

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: kats, Assigned: ronoueb, Mentored)

Details

(Whiteboard: [gfx-noted] [lang=c++])

Attachments

(1 file, 2 obsolete files)

From what Botond said in bug 1169695 comment 9:

At some point, it might make sense to give APZCBasicTester and APZCTreeManagerTester a common base class, and make some of the static functions in this file, like Tap() and Pan(), methods of it. (The mcc would then be a member of this base class, and thus wouldn't have to be passed around.)
Whiteboard: [gfx-noted]
Mentor: botond
Whiteboard: [gfx-noted] → [gfx-noted] [lang=c++]
Do you think this is still relevant after bug 1240244?
Flags: needinfo?(botond)
I think so - not having to pass around the MockContentController would still be an improvement.
Flags: needinfo?(botond)
Just wanted to hear your thoughts before I move on to the rest of "InputUtils.h".
Attachment #8748865 - Flags: feedback?(botond)
Comment on attachment 8748865 [details] [diff] [review]
use new base class to simplify Tap

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

Yep, this is what I had in mind - thanks!

::: gfx/layers/apz/test/gtest/APZTestCommon.h
@@ +274,5 @@
>    bool mWaitForMainThread;
>    MockContentControllerDelayed* mcc;
>  };
>  
>  class APZCBaseTester : public ::testing::Test {

I'd call this APZCTesterBase, to avoid confusion with APZCBasicTester.
Attachment #8748865 - Flags: feedback?(botond) → feedback+
Assignee: nobody → bd339
Attachment #8748865 - Attachment is obsolete: true
Attachment #8749108 - Flags: feedback?(botond)
Comment on attachment 8749108 [details] [diff] [review]
use common base class to simplify APZC tests, 1st revision

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

Thanks!

Do you have a local build that you can use to verify that these tests still pass? You can run them with:

  ./mach gtest 'A*'

(This runs all gtests whose name starts with 'A', which includes all the APZ gtests.)

::: gfx/layers/apz/test/gtest/APZTestCommon.h
@@ +445,5 @@
> +  // flings, are affected by elapsed time, and we want to be able to sample
> +  // them immediately after they start, without time having elapsed.
> +}
> +
> +// A version of Pan() that only takes y coordinates rather than (x, y) points

It's better to put this comment at the declaration.

@@ +461,5 @@
> +      aKeepFingerDown, aAllowedTouchBehaviors, aOutEventStatuses, aOutInputBlockId);
> +}
> +
> +/*
> + * Dispatches mock touch events to the apzc and checks whether apzc properly

Same here.
Attachment #8749108 - Flags: feedback?(botond) → feedback+
(In reply to Botond Ballo [:botond] from comment #6)
> Comment on attachment 8749108 [details] [diff] [review]
> use common base class to simplify APZC tests, 1st revision
> 
> Review of attachment 8749108 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks!
> 
> Do you have a local build that you can use to verify that these tests still
> pass? You can run them with:
> 
>   ./mach gtest 'A*'
> 
> (This runs all gtests whose name starts with 'A', which includes all the APZ
> gtests.)


It seems to be getting stuck on APZCBasicTester.OverScroll_Bug1152051a.
[ RUN      ] APZCBasicTester.OverScroll_Bug1152051a
[8465] WARNING: Found a non-root APZ with no handoff parent: file /gfx/layers/apz/src/APZCTreeManager.cpp, line 1637

Other tests (one such example is APZCBasicTester.OverScrollPanning) issue the same warning, but the test completes with an OK status.
(In reply to bd339 from comment #7)
> It seems to be getting stuck on APZCBasicTester.OverScroll_Bug1152051a.
> [ RUN      ] APZCBasicTester.OverScroll_Bug1152051a
> [8465] WARNING: Found a non-root APZ with no handoff parent: file
> /gfx/layers/apz/src/APZCTreeManager.cpp, line 1637
> 
> Other tests (one such example is APZCBasicTester.OverScrollPanning) issue
> the same warning, but the test completes with an OK status.

The warning is a pre-existing issue, unrelated to your changes; I filed bug 1270902 for it.

Getting stuck when running the test is strange; it doesn't happen for me (I applied your patch locally). I wonder if there's a race condition or other such nondeterministic behaviour in the test?
Sometimes for me crashes in the test manifest as it getting "stuck". Try running
  ./mach gtest --debug 'A*
and then once in gdb use the "run" command to run the tests. If it's crashing the debugger should catch it and give you more useful information.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #9)
> Sometimes for me crashes in the test manifest as it getting "stuck". Try
> running
>   ./mach gtest --debug 'A*
> and then once in gdb use the "run" command to run the tests. If it's
> crashing the debugger should catch it and give you more useful information.

It doesn't seem to be crashing, i.e. it is still "stuck" when run in gdb.
As the hang is unlikely to be related to the changes being made in this bug, I split it out into bug 1270938.

The patch in this bug should be good to land (after addressing comment 6).
Fix comments.
Attachment #8749108 - Attachment is obsolete: true
Attachment #8749783 - Flags: review?(botond)
Attachment #8749783 - Flags: review?(botond) → review+
https://hg.mozilla.org/mozilla-central/rev/a72d27fbbdad
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Thanks for your work here, bd339!

If you're interested in a slightly more challenging project, you could give bug 1056381 a try.
You need to log in before you can comment on or make changes to this bug.