[webkit-reviews] review denied: [Bug 114529] HeapTimer lifetime should be less complicated : [Attachment 197880] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Mark Hahnenberg
<mhahnenberg at apple.com>'s request for review:
Bug 114529: HeapTimer lifetime should be less complicated
https://bugs.webkit.org/show_bug.cgi?id=114529

Attachment 197880: Patch
https://bugs.webkit.org/attachment.cgi?id=197880&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=197880&action=review


review- because I think you’ll want to make at least one of the changes I
suggest

> Source/JavaScriptCore/jsc.cpp:765
> -    RefPtr<JSGlobalData> globalData = JSGlobalData::create(LargeHeap);
> -    JSLockHolder lock(globalData.get());
> +    JSGlobalData* globalData = JSGlobalData::create(LargeHeap).leakRef();
> +    APIEntryShim shim(globalData);

I don’t understand this change. Change log does not make it clear. Per-function
comments can sometimes help with this.

> Source/JavaScriptCore/testRegExp.cpp:504
> -    RefPtr<JSGlobalData> globalData = JSGlobalData::create(LargeHeap);
> -    JSLockHolder lock(globalData.get());
> +    JSGlobalData* globalData = JSGlobalData::create(LargeHeap).leakRef();
> +    APIEntryShim shim(globalData);

I don’t understand this change.

> Source/JavaScriptCore/heap/Heap.cpp:266
> +    , m_activityCallback(adoptPtr(DefaultGCActivityCallback::create(this)))
> +    , m_sweeper(adoptPtr(IncrementalSweeper::create(this)))

Instead of changing these call sites, the return types of the create functions
should be changed to PassOwnPtr. We want the calls to adoptPtr to be right
where the call to new is, not somewhere else.

> Source/JavaScriptCore/heap/Heap.cpp:816
>  void Heap::setActivityCallback(GCActivityCallback* activityCallback)
>  {
> -    m_activityCallback = activityCallback;
> +    m_activityCallback = adoptPtr(activityCallback);
>  }

The argument type of setActivityCallback should be changed to PassOwnPtr. We
want the calls to adoptPtr to be right where the call to new is, not somewhere
else.

> Source/JavaScriptCore/heap/HeapTimer.cpp:56
> +    m_context.retain = &retainAPILock;
> +    m_context.release = &releaseAPILock;

I don’t think these & operators are needed, and I normally prefer to omit them.


> Source/JavaScriptCore/heap/HeapTimer.h:29
> +#include "JSLock.h"

Why is this include needed? I don’t see any changes to the header below that
seem to require it.

> Source/JavaScriptCore/heap/HeapTimer.h:73
> +    static const void* retainAPILock(const void*);
> +    static void releaseAPILock(const void*);

Why do these need to be member functions? Can’t they just be free functions,
private to the cpp file?