Bug 33252 - Allow custom memory allocation control for IconDatabaseClient class
Summary: Allow custom memory allocation control for IconDatabaseClient class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Zoltan Horvath
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-06 04:32 PST by Zoltan Horvath
Modified: 2010-02-01 23:56 PST (History)
3 users (show)

See Also:


Attachments
proposed patch (1.46 KB, patch)
2010-01-06 04:34 PST, Zoltan Horvath
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zoltan Horvath 2010-01-06 04:32:59 PST
Inherits the following struct from Noncopyable because it is instantiated by 'new' and no need to be copyable:

class name - instantiated at: WebCore/'location'
class IconDatabaseClient - loader/icon/IconDatabase.cpp:89
Comment 1 Zoltan Horvath 2010-01-06 04:34:23 PST
Created attachment 45960 [details]
proposed patch
Comment 2 WebKit Review Bot 2010-01-06 04:36:29 PST
style-queue ran check-webkit-style on attachment 45960 [details] without any errors.
Comment 3 Eric Seidel (no email) 2010-01-06 09:00:02 PST
Comment on attachment 45960 [details]
proposed patch

This is an Abstract Base Class.  I don't think that this interface should require non-copyable.  The implementors of this interface should inherit from NonCopyable or FastAllocBase instead, no?
Comment 4 Darin Adler 2010-01-06 09:38:22 PST
(In reply to comment #3)
> This is an Abstract Base Class.

It's not. It has no pure virtual functions and is instantiated, as the patch says, on line 89 of IconDatabase.cpp.

However, I think it would make more sense if it was an abstract base class. The version that does nothing could be derived from it.

I think there's no harm in making the change from this patch, but it's not great to have this client different from other clients.
Comment 5 Zoltan Horvath 2010-01-18 00:02:40 PST
So, what should be the best solution? Using fastNew in this case wouldn't be nice. Add an extra class? Or Inherit this client?
Comment 6 Darin Adler 2010-01-18 10:52:32 PST
(In reply to comment #5)
> So, what should be the best solution? Using fastNew in this case wouldn't be
> nice. Add an extra class? Or Inherit this client?

I know you’re hoping I’ll decide, but both options seem OK to me.
Comment 7 Zoltan Horvath 2010-01-28 05:59:04 PST
Comment on attachment 45960 [details]
proposed patch

Okay, this change modifies fewest. I marked r? the patch again.
Comment 8 Eric Seidel (no email) 2010-02-01 16:11:39 PST
Attachment 45960 [details] was posted by a committer and has review+, assigning to Zoltan Horvath for commit.
Comment 9 Zoltan Horvath 2010-02-01 23:56:33 PST
Comment on attachment 45960 [details]
proposed patch

Clearing flags on attachment: 45960

Committed r54210: <http://trac.webkit.org/changeset/54210>
Comment 10 Zoltan Horvath 2010-02-01 23:56:44 PST
All reviewed patches have been landed.  Closing bug.