Bug 137776 - Allow reuse of BitmapTextureImageBuffer instances
Summary: Allow reuse of BitmapTextureImageBuffer instances
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-10-16 05:57 PDT by Adrien Destugues
Modified: 2016-09-17 07:04 PDT (History)
8 users (show)

See Also:

Attachments
Patch (2.19 KB, patch)
2014-10-16 06:02 PDT, Adrien Destugues 		no flags Details | Formatted Diff | Diff
Patch (2.17 KB, patch)
2014-10-16 13:52 PDT, Adrien Destugues 		no flags Details | Formatted Diff | Diff
Patch (2.47 KB, patch)
2014-10-16 13:58 PDT, Adrien Destugues 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Adrien Destugues 2014-10-16 05:57:17 PDT 
Allow reuse of BitmapTextureImageBuffer instances
Comment 1 Adrien Destugues 2014-10-16 06:02:01 PDT 
Created attachment 239945 [details]
Patch
Comment 2 Chris Dumez 2014-10-16 13:31:32 PDT 
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 3 Chris Dumez 2014-10-16 13:41:50 PDT 
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).
Comment 4 Adrien Destugues 2014-10-16 13:52:33 PDT 
Created attachment 239962 [details]
Patch
Comment 5 Chris Dumez 2014-10-16 13:54:08 PDT 
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?
Comment 6 Adrien Destugues 2014-10-16 13:58:57 PDT 
Created attachment 239964 [details]
Patch
Comment 7 Brent Fulgham 2014-10-31 10:56:29 PDT 
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 8 Michael Catanzaro 2016-09-17 07:04:50 PDT 
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.