Bug 93757 – [chromium] Clean up dependencies for Canvas2DLayerBridgeTest and GraphicsLayerChromiumTest unit tests

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

[chromium] Clean up dependencies for Canvas2DLayerBridgeTest and GraphicsLayerChromiumTest unit tests

[James Robinson]

Created attachment 157842 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adrienne Walker]

Comment on attachment 157842 [details]
Patch

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

> Source/WebKit/chromium/tests/GraphicsLayerChromiumTest.cpp:-122
> -    DebugScopedSetMainThread m_main;

Ok, but please run these unit tests in a debug build before committing this.

> Source/WebKit/chromium/tests/GraphicsLayerChromiumTest.cpp:274
> +    EXPECT_TRUE(m_platformLayer->hasActiveAnimation());

I'm not sure that these GraphicsLayerChromiumTests are really doing anything anymore? I agree with your ChangeLog entry that this used to just test conversion of WebCore types into CC types, but now all they're testing is that different types of animations get added and now exist.  Does it make sense to even have more than one of these?

[James Robinson]

Comment on attachment 157842 [details]
Patch

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

>> Source/WebKit/chromium/tests/GraphicsLayerChromiumTest.cpp:-122
>> -    DebugScopedSetMainThread m_main;
> 
> Ok, but please run these unit tests in a debug build before committing this.

Now that these tests don't use CC things they can't really get to the compositor thread (and I've verified that the dchecks are happy).

>> Source/WebKit/chromium/tests/GraphicsLayerChromiumTest.cpp:274
>> +    EXPECT_TRUE(m_platformLayer->hasActiveAnimation());
> 
> I'm not sure that these GraphicsLayerChromiumTests are really doing anything anymore? I agree with your ChangeLog entry that this used to just test conversion of WebCore types into CC types, but now all they're testing is that different types of animations get added and now exist.  Does it make sense to even have more than one of these?

That's a good point.  I think that the Preserve3DWith.. one is still useful since we're switching an animation between different WebLayers.  The rest of these mostly test that GraphicsLayerChromium doesn't bail completely on these animation types.  Maybe I should just be checking the return value of addAnimation() ?  Note that createTransform...BigRotation() is testing that GraphicsLayerChromium _does_ barf on a particular animation....but that would be better as an AnimationTranslationUtil test.

I'll take a pass at cleaning these up.

[James Robinson]

Created attachment 158105 [details]
Patch

[James Robinson]

Take two - I've moved most of the animation-related tests to AnimationTranslationUtilTest.cpp.  I also had to clean up a few unit tests that were leaking their "fake" thread identity.

[Adrienne Walker]

Comment on attachment 158105 [details]
Patch

R=me.  Thanks for moving those tests around.

[James Robinson]

Committed r125466: <http://trac.webkit.org/changeset/125466>