[webkit-reviews] review granted: [Bug 86382] Enh: Add the Ability to Disable / Enable JavaScript GC Timer : [Attachment 141757] Patch with ChangeLog

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Michael Saboff <msaboff at apple.com>'s
request for review:
Bug 86382: Enh: Add the Ability to Disable / Enable JavaScript GC Timer
https://bugs.webkit.org/show_bug.cgi?id=86382

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

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


Looks like the Windows build is broken, need this in some def file perhaps:

setGCTimerEnabled at Heap@JSC@@QAEX_N at Z

I’m going to say r=me but I think you should spend a little more time on the
names of these new functions and methods.

> Source/JavaScriptCore/ChangeLog:13
> +	   (JSC):

Please don’t leave lines like this in the change log. Better, consider writing
per-file and per-function comments. Even better, consider fixing this problem
in prepare-ChangeLog some day.

> Source/JavaScriptCore/ChangeLog:15
> +	   (Heap):

Ditto.

> Source/JavaScriptCore/runtime/GCActivityCallback.h:50
> +    bool isEnabled() { return m_enabled; }

Should be const.

> Source/JavaScriptCore/runtime/GCActivityCallbackCF.cpp:67
> +    if (!heap->activityCallback()->isEnabled())
> +	   return;

This seems OK for a debug-only feature. If this was a real end-user feature,
then I’d suggest not even scheduling the timer when it’s disabled.

> Source/WebCore/bindings/js/GCController.h:47
> +	   void setJavaScriptGCTimer(bool);

This seems like a strange function name. Given the name I’d expect to pass this
a timer.

> Source/WebKit2/ChangeLog:15
> +	   (WebKit):

More strange change log lines.

> Source/WebKit2/ChangeLog:17
> +	   (WebContext):

More.

> Source/WebKit2/ChangeLog:20
> +	   (WebKit):

More.

> Source/WebKit2/UIProcess/API/C/WKContext.h:159
> +WK_EXPORT void WKContextSetJavaScriptGCTimer(WKContextRef context, bool
enable);

This name doesn’t seem great. A setter with this name should take a timer, not
a boolean. Note also that the previous function spells out GC. So I’d suggest
the name WKContextSetJavaScriptGarbageCollectionTimerEnabled.

> Source/WebKit/mac/Misc/WebCoreStatistics.h:50
> ++ (void)setJavaScriptGCTimer:(BOOL)enabled;

Same issue with naming here.