Bug 75557 - [chromium] Create unit tests for CCTiledLayerImpl
Summary: [chromium] Create unit tests for CCTiledLayerImpl
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: Adrienne Walker
URL:
Keywords:
Depends on: 75574
Blocks:
  Show dependency tree / graph
 
Reported: 2012-01-04 09:53 PST by Adrienne Walker
Modified: 2012-01-05 10:27 PST (History)
3 users (show)

See Also:

Attachments
Patch (11.80 KB, patch)
2012-01-04 09:55 PST, Adrienne Walker 		no flags Details | Formatted Diff | Diff
Fix win build errors (11.89 KB, patch)
2012-01-05 10:25 PST, Adrienne Walker 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Adrienne Walker 2012-01-04 09:53:31 PST 
[chromium] Create unit tests for CCTiledLayerImpl
Comment 1 Adrienne Walker 2012-01-04 09:55:26 PST 
Created attachment 121120 [details]
Patch
Comment 2 James Robinson 2012-01-04 11:03:44 PST 
Comment on attachment 121120 [details]
Patch

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

Awesome! R=me

> Source/WebKit/chromium/tests/CCTiledLayerImplTest.cpp:38
> +#define QUAD(i) "    Quad: " << i

can you move this down closer to the first use?

do we really really need this? we have a lot of quad-named things going on. what about a string constant that's <<'d in like normal?

> Source/WebKit/chromium/tests/CCTiledLayerImplTest.cpp:246
> +        EXPECT_EQ(quads[i]->material(), CCDrawQuad::TiledContent) << QUAD(i);

think this should be ASSERT_EQ() or the next few lines will do really crazy and potentially hard-to-diagnose things
Comment 3 Adrienne Walker 2012-01-04 11:07:50 PST 
Comment on attachment 121120 [details]
Patch

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

>> Source/WebKit/chromium/tests/CCTiledLayerImplTest.cpp:38
>> +#define QUAD(i) "    Quad: " << i
> 
> can you move this down closer to the first use?
> 
> do we really really need this? we have a lot of quad-named things going on. what about a string constant that's <<'d in like normal?

Sure.  I'll make it a string constant.

>> Source/WebKit/chromium/tests/CCTiledLayerImplTest.cpp:246
>> +        EXPECT_EQ(quads[i]->material(), CCDrawQuad::TiledContent) << QUAD(i);
> 
> think this should be ASSERT_EQ() or the next few lines will do really crazy and potentially hard-to-diagnose things

Good point.  Will change to an assert.
Comment 4 Adrienne Walker 2012-01-04 11:36:26 PST 
Committed r104052: <http://trac.webkit.org/changeset/104052>
Comment 5 Adrienne Walker 2012-01-04 14:25:55 PST 
Reopening, due to OwnPtr copy constructor failure.  :(

http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Win%20Builder%20%28dbg%29/builds/18038/steps/compile/logs/stdio
Comment 6 Adrienne Walker 2012-01-05 10:25:21 PST 
Created attachment 121295 [details]
Fix win build errors
Comment 7 Adrienne Walker 2012-01-05 10:25:51 PST 
(In reply to comment #6)
> Created an attachment (id=121295) [details]
> Fix win build errors

^ Just uploading this for posterity.  No review needed.
Comment 8 Adrienne Walker 2012-01-05 10:27:03 PST 
Committed r104176: <http://trac.webkit.org/changeset/104176>