RESOLVED FIXED 82425
[chromium] Split off tiled layer constructs for unit tests into a common header
https://bugs.webkit.org/show_bug.cgi?id=82425
Summary [chromium] Split off tiled layer constructs for unit tests into a common header
Dana Jansens
Reported 2012-03-27 22:02:32 PDT
[chromium] Split off tiled layer constructs for unit tests into a common header
Attachments
Patch (23.13 KB, patch)
2012-03-27 22:03 PDT, Dana Jansens
no flags
Patch (24.92 KB, patch)
2012-03-29 09:57 PDT, Dana Jansens
no flags
Dana Jansens
Comment 1 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.
Adrienne Walker
Comment 2 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?
Dana Jansens
Comment 3 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.
Dana Jansens
Comment 4 2012-03-29 09:57:16 PDT
Adrienne Walker
Comment 5 2012-03-29 10:22:35 PDT
Comment on attachment 134605 [details] Patch Thanks for the changes. R=me.
WebKit Review Bot
Comment 6 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>
WebKit Review Bot
Comment 7 2012-03-29 11:19:12 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.