Bug 82425 - [chromium] Split off tiled layer constructs for unit tests into a common header
Summary: [chromium] Split off tiled layer constructs for unit tests into a common header
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: Dana Jansens
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-27 22:02 PDT by Dana Jansens
Modified: 2012-03-29 11:19 PDT (History)
6 users (show)

See Also:


Attachments
Patch (23.13 KB, patch)
2012-03-27 22:03 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (24.92 KB, patch)
2012-03-29 09:57 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dana Jansens 2012-03-27 22:02:32 PDT
[chromium] Split off tiled layer constructs for unit tests into a common header
Comment 1 Dana Jansens 2012-03-27 22:03:12 PDT
Created attachment 134218 [details]
Patch

Since I had this done already for some other failed changes, here's splitting the tiled layer test stuff off into a common.h so other tests can make use of it.
Comment 2 Adrienne Walker 2012-03-28 12:46:53 PDT
Comment on attachment 134218 [details]
Patch

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

I'll admit that I'm not totally sold on this change since there really aren't other clients of these classes yet.

If you're going to move these things to the header so that they can be reused, I think you should push a lot more definitions into the cpp file to make compilation/linking faster and for style reasons.  In particular, all the constructors and destructors here should probably not be inlined.

> Source/WebKit/chromium/tests/CCTiledLayerTestCommon.h:45
> +class FakeTextureAllocator : public WebCore::TextureAllocator {
> +public:
> +    virtual unsigned createTexture(const WebCore::IntSize&, GC3Denum) { return 1; }
> +    virtual void deleteTexture(unsigned, const WebCore::IntSize&, GC3Denum) { }
> +};

Looks like TextureManagerTest also does this?
Comment 3 Dana Jansens 2012-03-29 09:56:59 PDT
(In reply to comment #2)
> (From update of attachment 134218 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134218&action=review
> 
> I'll admit that I'm not totally sold on this change since there really aren't other clients of these classes yet.

Yeh, though since it's done already I feel like splitting things out like this will make the bar lower for good unit tests to be written in the future, especially for newer people.

> If you're going to move these things to the header so that they can be reused, I think you should push a lot more definitions into the cpp file to make compilation/linking faster and for style reasons.  In particular, all the constructors and destructors here should probably not be inlined.

Yes! done. I just left simple getter/setters in the .h file.

> > Source/WebKit/chromium/tests/CCTiledLayerTestCommon.h:45
> > +class FakeTextureAllocator : public WebCore::TextureAllocator {
> > +public:
> > +    virtual unsigned createTexture(const WebCore::IntSize&, GC3Denum) { return 1; }
> > +    virtual void deleteTexture(unsigned, const WebCore::IntSize&, GC3Denum) { }
> > +};
> 
> Looks like TextureManagerTest also does this?

Thanks :) I grepped for Tile things but not Texture things.
Comment 4 Dana Jansens 2012-03-29 09:57:16 PDT
Created attachment 134605 [details]
Patch
Comment 5 Adrienne Walker 2012-03-29 10:22:35 PDT
Comment on attachment 134605 [details]
Patch

Thanks for the changes.  R=me.
Comment 6 WebKit Review Bot 2012-03-29 11:19:07 PDT
Comment on attachment 134605 [details]
Patch

Clearing flags on attachment: 134605

Committed r112550: <http://trac.webkit.org/changeset/112550>
Comment 7 WebKit Review Bot 2012-03-29 11:19:12 PDT
All reviewed patches have been landed.  Closing bug.