| Summary: | Allow reuse of BitmapTextureImageBuffer instances | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Adrien Destugues <pulkomandy> | ||||||||
| Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||
| Status: | NEW --- | ||||||||||
| Severity: | Normal | CC: | cdumez, cmarcelo, commit-queue, kondapallykalyan, luiz, noam, rniwa, simon.fraser | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Adrien Destugues
2014-10-16 05:57:17 PDT
Created attachment 239945 [details]
Patch
Comment on attachment 239945 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239945&action=review > Source/WebCore/platform/graphics/texmap/TextureMapperImageBuffer.h:36 > + bool canReuseWith(const IntSize& /* contentsSize */, Flags = 0) { return true; } Isn't this function virtual? if so, please mark it explicitly as virtual. You will also want to use override (and likely final). Comment on attachment 239945 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239945&action=review Someone else should comment on the general approach. >> Source/WebCore/platform/graphics/texmap/TextureMapperImageBuffer.h:36 >> + bool canReuseWith(const IntSize& /* contentsSize */, Flags = 0) { return true; } > > Isn't this function virtual? if so, please mark it explicitly as virtual. You will also want to use override (and likely final). Yes, it is virtual, please use: virtual bool canReuseWith(const IntSize& /* contentsSize */, Flags = 0) override { return true; } Also, please mark the class as final (I don't think anyone subclasses it). Created attachment 239962 [details]
Patch
Comment on attachment 239962 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239962&action=review > Source/WebCore/platform/graphics/texmap/TextureMapperImageBuffer.h:36 > + virtual bool canReuseWith(const IntSize& /* contentsSize */, Flags = 0) override final { return true; } Cannot the class be final instead? Created attachment 239964 [details]
Patch
Comment on attachment 239964 [details]
Patch
This seems fine, but the gtk bot seems to fail with this change, and that's one of the builds that would make use of this change.
Comment on attachment 239964 [details]
Patch
Hi,
Apologies that your patch was not reviewed in a timely manner. Since it's now quite old, I am removing it from the review request queue. Please consider rebasing it on trunk and resubmitting.
To increase the chances of getting a review, consider using 'Tools/Scripts/webkit-patch upload --suggest-reviewers' to CC reviewers who might be interested in this bug.
|