WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(24.92 KB, patch)
2012-03-29 09:57 PDT
,
Dana Jansens
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
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
Created
attachment 134605
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug