Bug 93757 - [chromium] Clean up dependencies for Canvas2DLayerBridgeTest and GraphicsLayerChromiumTest unit tests
Summary: [chromium] Clean up dependencies for Canvas2DLayerBridgeTest and GraphicsLaye...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-10 17:12 PDT by James Robinson
Modified: 2012-08-13 15:47 PDT (History)
8 users (show)

See Also:


Attachments
Patch (24.44 KB, patch)
2012-08-10 17:21 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch (43.77 KB, patch)
2012-08-13 14:30 PDT, James Robinson
enne: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2012-08-10 17:12:15 PDT
[chromium] Clean up dependencies for Canvas2DLayerBridgeTest and GraphicsLayerChromiumTest unit tests
Comment 1 James Robinson 2012-08-10 17:21:24 PDT
Created attachment 157842 [details]
Patch
Comment 2 WebKit Review Bot 2012-08-10 17:24:19 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Adrienne Walker 2012-08-10 17:39:41 PDT
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?
Comment 4 James Robinson 2012-08-10 17:55:56 PDT
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.
Comment 5 James Robinson 2012-08-13 14:30:07 PDT
Created attachment 158105 [details]
Patch
Comment 6 James Robinson 2012-08-13 14:31:37 PDT
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.
Comment 7 Adrienne Walker 2012-08-13 14:38:58 PDT
Comment on attachment 158105 [details]
Patch

R=me.  Thanks for moving those tests around.
Comment 8 James Robinson 2012-08-13 15:47:02 PDT
Committed r125466: <http://trac.webkit.org/changeset/125466>