Bug 54257 - Make DefaultGCActivityCallback for PLATFORM(CF) Easier to Subclass
Summary: Make DefaultGCActivityCallback for PLATFORM(CF) Easier to Subclass
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-10 16:18 PST by Joseph Pecoraro
Modified: 2011-02-10 17:18 PST (History)
2 users (show)

See Also:


Attachments
[PATCH] Proposed Solution (4.23 KB, patch)
2011-02-10 16:32 PST, Joseph Pecoraro
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2011-02-10 16:18:48 PST
Allow the CFRunLoop to be specified in a constructor.
Comment 1 Joseph Pecoraro 2011-02-10 16:32:48 PST
Created attachment 82071 [details]
[PATCH] Proposed Solution

I'm open to suggestions for a better name for "commonConstructor".
Also I'm fine with making that private.
Comment 2 Geoffrey Garen 2011-02-10 16:46:18 PST
Comment on attachment 82071 [details]
[PATCH] Proposed Solution

View in context: https://bugs.webkit.org/attachment.cgi?id=82071&action=review

r=me

> Source/JavaScriptCore/ChangeLog:8
> +        A subclass may want to specify the CFRunLoop the Garbage Collection. It

Typo, I think.

> Source/JavaScriptCore/runtime/GCActivityCallbackCF.cpp:86
> +    memset(&d->context, '\0', sizeof(CFRunLoopTimerContext));

'\0' is a strange way to say '0'. I know you didn't write this code initially, but you might as well change it to '0'.
Comment 3 Joseph Pecoraro 2011-02-10 17:10:12 PST
Landed in r78291:
http://trac.webkit.org/changeset/78291
Comment 4 Joseph Pecoraro 2011-02-10 17:10:44 PST
> > Source/JavaScriptCore/ChangeLog:8
> > +        A subclass may want to specify the CFRunLoop the Garbage Collection. It
> 
> Typo, I think.

Yep, I cleaned up my ChangeLog mistakes.

> > Source/JavaScriptCore/runtime/GCActivityCallbackCF.cpp:86
> > +    memset(&d->context, '\0', sizeof(CFRunLoopTimerContext));
> 
> '\0' is a strange way to say '0'. I know you didn't write this code initially, but you might as well change it to '0'.

Done. Thanks!
Comment 5 Pratik Solanki 2011-02-10 17:14:17 PST
(In reply to comment #4)
> > '\0' is a strange way to say '0'. I know you didn't write this code initially, but you might as well change it to '0'.
> 
> Done. Thanks!

I think Geoff meant to say pass 0, not '0'. Passing '0' to memset will initialize the memory to ASCII character 0.
Comment 6 Joseph Pecoraro 2011-02-10 17:18:32 PST
(In reply to comment #5)
> (In reply to comment #4)
> > > '\0' is a strange way to say '0'. I know you didn't write this code initially, but you might as well change it to '0'.
> > 
> > Done. Thanks!
> 
> I think Geoff meant to say pass 0, not '0'. Passing '0' to memset will initialize the memory to ASCII character 0.

Yes, entirely my fault. I took that literally. I landed a follow-up fix.
Unreviewed since initially this was a review comment.
http://trac.webkit.org/changeset/78292

Thanks!