Bug 68572 – Create unit tests for LayerChromium

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Shawn Singh]

This patch is ready for review, but not for commit.   I still need to comb over it once more to make sure tests actually test things properly.

4 cases are currently DISABLED.  I'm OK with removing the removeChild() tests, but I kept them there for now just since they were already implemented.

[Shawn Singh]

Created attachment 108238 [details]
Patch

[WebKit Review Bot]

Comment on attachment 108238 [details]
Patch

Attachment 108238 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9792022

[Shawn Singh]

Created attachment 108272 [details]
Patch

[Shawn Singh]

This new patch is much more correct, testing destructors properly.  I have "tested the tests" mostly, but should do one more thorough pass to really make sure.

Additionally, please search for the key word "semantics" --> 3 or 4 places in comments where I use that word are questions about whether the semantics of LayerChromium are appropriate or could be improved.

Once semantics are discussed I can enable the last "disabled" test and submit the final patch.   All other tests are enabled.

Thanks!

[James Robinson]

Comment on attachment 108272 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108272&action=review

First round of comments.  This definitely needs some work, but I'm really excited about having more coverage here.

> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:34
> +using ::testing::_;

what is this?

> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:67
> +    static int m_numInstancesDestroyed;

we tend to use an s_ prefix for statics (as opposed to an m_ prefix for member variables) so this should be s_numInstancesDestroyed

> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:74
> +    void SetUp()

this function is virtual (since testing::Test::SetUp() is declared virtual in gtest.h).  Please include the virtual keyword here

> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:83
> +        // silentDelegate is initialized to be just a stub and will
> +        // not print any warnings. It is used when we are not worried
> +        // about testing how the delegate is called.
> +
> +        EXPECT_CALL(silentDelegate, drawsContent()).Times(AnyNumber());
> +        EXPECT_CALL(silentDelegate, preserves3D()).Times(AnyNumber());
> +        EXPECT_CALL(silentDelegate, paintContents(_, _)).Times(AnyNumber());
> +        EXPECT_CALL(silentDelegate, notifySyncRequired()).Times(AnyNumber());

isn't this exactly the same as passing 0 in for the delegate?

> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:86
> +    void explicitlyDestroyLayer(PassRefPtr<LayerChromium> layer) const

This appears to be redundant with LayerChromiumWithInstrumentedDestructor.  Is there any reason to have both?

> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:156
> +    // All delegates and layers are declared here so that each test case
> +    // is more readable. The meaning behind variable names should be
> +    // clear when reading the specific test cases.
> +    MockLayerDelegate mockDelegate, silentDelegate, parentDelegate, childDelegate;
> +    RefPtr<LayerChromium> testLayer, parent, child, child1, child2, child3, child4, grandChild1, grandChild2, grandChild3;

While I appreciate the sentiment, I actually think doing this makes the tests much harder to read.  It still just just as many lines of code to instantiate each LayerChromium for a given test case (one) and you have to jump through unnatural hoops in order to drop references, etc.  I think you should instantiate the objects you need for a test within that test.

> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:160
> +// FIXME: this fixture is temporary just while developing test cases
> +class LayerChromiumTest_WORK_IN_PROGRESS : public LayerChromiumTest {

I don't understand what the point of this is - do you intend to check this in? does the _WORK_IN_PROGRESS suffix have some special meaning to the tools, or is it intended for humans?

> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:162
> +    void SetUp() { LayerChromiumTest::SetUp(); }

this line is a complete no-op.  LayerChromiumTest::SetUp() is virtual (because void SetUp() is declared virtual in testing::Test()), so if you left this out calling SetUp() on an instance of LayerChromiumTest_WORK_IN_PROGRESS would call LayerChromiumTest::SetUp()

> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:163
> +    void createSimpleTestTree() { LayerChromiumTest::createSimpleTestTree(); }

why is this here?

> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:188
> +    EXPECT_EQ(parent->children().size(), (size_t)0);

We use C++ style casts, not C style.  That means this should be static_cast<size_t>(0)

> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:193
> +    EXPECT_EQ(parent->children().size(), (size_t)1);

should be static_cast<>. there are many more instances of this in the rest of the code, so I didn't mark them all.

> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:275
> +    // insert to empty list
> +    EXPECT_CALL(parentDelegate, notifySyncRequired()).Times(1);
> +    parent->insertChild(child3, 0);
> +    Mock::VerifyAndClearExpectations(&parentDelegate);

It's not useful to include a comment that merely repeats what the next line of code does.  We can all read the code.  In a comment you should try to provide information that is not obvious from just reading the code - for instance, explain why you are doing things in a certain way, subtle interactions between systems that aren't clear from just reading the code, etc.  If there aren't any such things to note, then just don't add a comment.

> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:308
> +    // FIXME: There are awkward semantics that notifySyncRequired gets
> +    //        called for each child that gets removed from within the
> +    //        parent's destructor. are those semantics OK? It feels
> +    //        like it may come back to bite us down the road.
> +    //        for now, this will print warnings in the test but not fail.

Does it matter how many times notifySyncRequired is called?

Tests should be silent when passing.

> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:358
> +TEST_F(LayerChromiumTest_WORK_IN_PROGRESS, replaceChildWithNewChild)

these tests raise an interesting point: why is replaceChild part of our interface anyway? I can't find any callers, and it looks like it was added in the initial import of the GraphicsLayer stuff.  I'll follow up with Simon to see if we need to keep this.

> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:507
> +    // FIXME: the base class LayerChromium always returns false and doesn't use the delegate drawsContent().
> +    // this test should be disabled until we resolve whether those semantics are appropriate or not.
> +    // if it should return false without calling delegate, then we have to subclass the LayerChromium
> +    // and override that function so that we can test descendantDrawsContent().

It's not a good idea to check in intentionally disabled code.  Code that is in the repo is code that the whole project has to maintain, even if it's disabled or dead code.  If you aren't sure whether to land something then keep it in a local git repo or in a patch on another bug.

> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:584
> +    /*

why is this part commented out?

> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:658
> +    EXPECT_CALL(mockDelegate, notifySyncRequired()).Times(1);
> +    testLayer->setAnchorPoint(FloatPoint(1.23f, 4.56f));
> +    Mock::VerifyAndClearExpectations(&mockDelegate);

we repeat this stanza a lot, seems like it could use a helper function of some sort

> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:730
> +    // FIXME: uncomment this when we can reasonably stub the CCLayerTreeHost.
> +    //
> +    // EXPECT_CALL(mockDelegate, notifySyncRequired()).Times(1);
> +    // testLayer->setLayerTreeHost(dummyPointer);
> +    // Mock::VerifyAndClearExpectations(&mockDelegate);
> +    // verifyFloatRectsAlmostEqual(testLayer->dirtyRect(), FloatRect(0.0f, 0.0f, 5.0f, 10.0f));
> +    //
> +    // testLayer->resetNeedsDisplay();
> +    // EXPECT_TRUE(testLayer->dirtyRect().isEmpty());

see commments above about not checking in commented out code

[John Bates]

> > Source/WebKit/chromium/tests/LayerChromiumTest.cpp:188
> > +    EXPECT_EQ(parent->children().size(), (size_t)0);
> 
> We use C++ style casts, not C style.  That means this should be static_cast<size_t>(0)
> 
> > Source/WebKit/chromium/tests/LayerChromiumTest.cpp:193
> > +    EXPECT_EQ(parent->children().size(), (size_t)1);
> 
> should be static_cast<>. there are many more instances of this in the rest of the code, so I didn't mark them all.

Or just 0u and 1u, etc.

[Shawn Singh]

Comment on attachment 108272 [details]
Patch

Thanks for the comments.  I wasnt intending to land disabled tests or messy FIXME comments.  That was my (apparently unsuccessful) way of asking questions in-line with the code. Would it be better if I just put a bullet list of issues in a separate post on the bug?   How do you want me to communicate those points next time?

Anyway, I'll fix the style issues, and rest of my responses are inline.  Most of my responses will need one more pass of feedback from people to decide how to proceed.  Thanks!

View in context: https://bugs.webkit.org/attachment.cgi?id=108272&action=review

>> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:34
>> +using ::testing::_;
> 
> what is this?

This is gtest's symbol to represent "any parameter" in an EXPECT_CALL macro - when we expect a mocked function to be called, but we don't care what parameters are passed to it.  This is used with the silentDelegate.  If we end up removing silentDelegate, we can remove this, too.   Or if you prefer I can just have one line that says "using namespace ::testing" instead.

>> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:83
>> +        EXPECT_CALL(silentDelegate, notifySyncRequired()).Times(AnyNumber());
> 
> isn't this exactly the same as passing 0 in for the delegate?

in my opinion -  A "mock delegate" and "no delegate at all" are not the same - LayerChromium should believe it has a working delegate, but we don't care how it uses the delegate.   In our particular implementation of LayerChromium, yes they would produce the same "test results", but that's only a side effect of how we programmed the LayerChromium.  For example, if later we find its useful to add an early-exit if m_delegate is NULL...  Then we may not be testing what we think we are testing.  Using a stub seems less brittle to me.  Thoughts?

>> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:86
>> +    void explicitlyDestroyLayer(PassRefPtr<LayerChromium> layer) const
> 
> This appears to be redundant with LayerChromiumWithInstrumentedDestructor.  Is there any reason to have both?

I think its important that we directly test LayerChromium when possible.  Testing the subclass could be misleading if we introduce errors into the subclass.  However, to make the leak test, we had no choice but to passively check the destructor is called, and that should be the only test that uses this subclass.

>> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:156
>> +    RefPtr<LayerChromium> testLayer, parent, child, child1, child2, child3, child4, grandChild1, grandChild2, grandChild3;
> 
> While I appreciate the sentiment, I actually think doing this makes the tests much harder to read.  It still just just as many lines of code to instantiate each LayerChromium for a given test case (one) and you have to jump through unnatural hoops in order to drop references, etc.  I think you should instantiate the objects you need for a test within that test.

OK, will do =)

>> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:160
>> +class LayerChromiumTest_WORK_IN_PROGRESS : public LayerChromiumTest {
> 
> I don't understand what the point of this is - do you intend to check this in? does the _WORK_IN_PROGRESS suffix have some special meaning to the tools, or is it intended for humans?

No, this will not be in the final patch.  Sorry, I thought the comment was clear enough.  This was my way of keeping track of when I really considered each test to be "verified" that it test what we think its testing.  As I've been verifying tests, I've been migrating them to LayerChromiumTest, and this WORK_IN_PROGRESS class will be deleted.

>> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:308
>> +    //        for now, this will print warnings in the test but not fail.
> 
> Does it matter how many times notifySyncRequired is called?
> 
> Tests should be silent when passing.

I feel like it does indeed matter how many times notifySyncRequired is called.  If we're planning to make LayerChromium part of a "public CC API", where the user has to receive these notifySyncRequired calls, we should clearly define when/why it would be called.  To me, it makes sense if this is called exactly once per public function, or not at all, but definitely not multiple times.  The way the code is now, whenever we remove all children, or even in the destructor which feels particularly weird, notifySyncRequired can get called several times in a row.   It doesn't seem like a good idea to have that sort of behavior on the public api side of the code... thoughts?

>> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:358
>> +TEST_F(LayerChromiumTest_WORK_IN_PROGRESS, replaceChildWithNewChild)
> 
> these tests raise an interesting point: why is replaceChild part of our interface anyway? I can't find any callers, and it looks like it was added in the initial import of the GraphicsLayer stuff.  I'll follow up with Simon to see if we need to keep this.

OK, I'll adapt this code to whatever you all decide.

>> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:507
>> +    // and override that function so that we can test descendantDrawsContent().
> 
> It's not a good idea to check in intentionally disabled code.  Code that is in the repo is code that the whole project has to maintain, even if it's disabled or dead code.  If you aren't sure whether to land something then keep it in a local git repo or in a patch on another bug.

Yes, I didn't intend to land any disabled tests.  the point was for us to discuss the LayerChromium semantics.  I believe that we're discussing this right now in https://bugs.webkit.org/show_bug.cgi?id=67750 and I would prefer to settle that issue before we land this patch.  So yeah, this will either be enabled, or removed entirely, before landing.

>> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:584
>> +    /*
> 
> why is this part commented out?

my mistake, I should uncomment and fix this code.

>> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:658
>> +    Mock::VerifyAndClearExpectations(&mockDelegate);
> 
> we repeat this stanza a lot, seems like it could use a helper function of some sort

OK, sure.  But I think it would require a macro because of the arbitrary function in the middle (including arbitrary parameters, so templates don't seem practical either).

[Shawn Singh]

One more issue arose with the semantics of LayerChromium:

in setNeedsDisplay( dirtyRect ), we are calling unite(dirtyRect).  This means that an empty dirty rect can cause the united dirty rect to be larger.

Is this important for some reason?  Can we change it to uniteIfNonZero(...) instead?

Once this and all the other questions are answered, I'll go ahead and submit another patch.

Thanks!

[Shawn Singh]

Created attachment 108867 [details]
Patch

[Shawn Singh]

This patch is significantly cleaned up and fixed.  From my side, this patch is ready for commit.   Please let me know any more changes you would like to make.

All tests are enabled and pass;  There are two FIXMEs that need to be addressed by separate bugs.

[WebKit Review Bot]

Comment on attachment 108867 [details]
Patch

Attachment 108867 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9881385

[Shawn Singh]

Created attachment 108872 [details]
Patch

[James Robinson]

(In reply to comment #7)
> >> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:83
> >> +        EXPECT_CALL(silentDelegate, notifySyncRequired()).Times(AnyNumber());
> > 
> > isn't this exactly the same as passing 0 in for the delegate?
> 
> in my opinion -  A "mock delegate" and "no delegate at all" are not the same - LayerChromium should believe it has a working delegate, but we don't care how it uses the delegate.   In our particular implementation of LayerChromium, yes they would produce the same "test results", but that's only a side effect of how we programmed the LayerChromium.  For example, if later we find its useful to add an early-exit if m_delegate is NULL...  Then we may not be testing what we think we are testing.  Using a stub seems less brittle to me.  Thoughts?

I don't think that's true.  We create LayerChromiums will NULL delegates and NULL out the delegate on LayerChromiums and expect that to behave the same way as having a no-op delegate.

> 
> >> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:86
> >> +    void explicitlyDestroyLayer(PassRefPtr<LayerChromium> layer) const
> > 
> > This appears to be redundant with LayerChromiumWithInstrumentedDestructor.  Is there any reason to have both?
> 
> I think its important that we directly test LayerChromium when possible.  Testing the subclass could be misleading if we introduce errors into the subclass.  However, to make the leak test, we had no choice but to passively check the destructor is called, and that should be the only test that uses this subclass.
> 

I think having a subclass that only provides a destructor is perfectly valid to use in tests where you want the subclass to behave the same in every way except for the destruction behavior.

[James Robinson]

(In reply to comment #8)
> One more issue arose with the semantics of LayerChromium:
> 
> in setNeedsDisplay( dirtyRect ), we are calling unite(dirtyRect).  This means that an empty dirty rect can cause the united dirty rect to be larger.
> 

Do we ever actually do this?

> Is this important for some reason?  Can we change it to uniteIfNonZero(...) instead?
> 
> Once this and all the other questions are answered, I'll go ahead and submit another patch.
> 
> Thanks!

[Shawn Singh]

In response to the remaining 3 issues:

(1)  Discussed offline with jamesr, we agreed to keep silentDelegate there.

(2) removed the explicitlyDestroy stuff, and used the instrumented destructor instead, which is more correct.

(3) the issue about unite vs uniteIfNonZero is actually a non-issue.  Oddly, those two functions in FloatRect seem to have almost the exact same semantics, and that should be reported in a separate issue.


patch will be uploaded in just a moment.

[Shawn Singh]

Created attachment 108896 [details]
Patch

[James Robinson]

Comment on attachment 108896 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108896&action=review

R=me. To upload a new patch to fix the nits and request commit, but without requiring a new review, replaced 'Reviewed by NOBODY (OOPS!).' in the ChangeLog with 'Reviewed by James Robinson.' and do:

webkit-patch upload --no-review --request-commit

> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:56
> +    LayerChromiumWithInstrumentedDestructor(CCLayerDelegate* delegate)

nit: explicit keyword on 1-arg constructors, please.

c++ got the default wrong for this behavior - i think you should have to say 'implicit' to get implicit type conversion behavior. it's super annoying :(

> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:262
> +    // The child delegate notifySynRequired should not be called when inserting.

typo: notifySynRequired->notifySyncRequired

[Shawn Singh]

Created attachment 109386 [details]
Patch

[WebKit Review Bot]

Comment on attachment 109386 [details]
Patch

Clearing flags on attachment: 109386

Committed r96450: <http://trac.webkit.org/changeset/96450>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.