[chromium] Split off tiled layer constructs for unit tests into a common header

• Component: New Bugs
• Status: RESOLVED FIXED

[Dana Jansens]

[chromium] Split off tiled layer constructs for unit tests into a common header

[Dana Jansens]

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.

[Adrienne Walker]

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?

[Dana Jansens]

(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.

[Dana Jansens]

Created attachment 134605 [details]
Patch

[Adrienne Walker]

Comment on attachment 134605 [details]
Patch

Thanks for the changes.  R=me.

[WebKit Review Bot]

Comment on attachment 134605 [details]
Patch

Clearing flags on attachment: 134605

Committed r112550: <http://trac.webkit.org/changeset/112550>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.