[webkit-reviews] review denied: [Bug 68572] Create unit tests for LayerChromium : [Attachment 108272] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Shawn Singh
<shawnsingh at chromium.org>'s request for review:
Bug 68572: Create unit tests for LayerChromium
https://bugs.webkit.org/show_bug.cgi?id=68572

Attachment 108272: Patch
https://bugs.webkit.org/attachment.cgi?id=108272&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
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