[webkit-reviews] review denied: [Bug 21719] JS Profiler notification system needed : [Attachment 24470] Proposed patch for the JS Profiler notification server.

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at hatcher.name> has denied Kevin Lindeman
<klindeman at apple.com>'s request for review:
Bug 21719: JS Profiler notification system needed
https://bugs.webkit.org/show_bug.cgi?id=21719

Attachment 24470: Proposed patch for the JS Profiler notification server.
https://bugs.webkit.org/attachment.cgi?id=24470&action=edit

------- Additional Comments from Timothy Hatcher <timothy at hatcher.name>
+#import <Foundation/NSDistributedNotificationCenter.h>
+#import <Foundation/NSProcessInfo.h>
+#import <Foundation/NSString.h>
+#import <Foundation/NSUserDefaults.h>
+#import "JSStringRef.h"

All of these #imports should move to the .mm file, you only need to #import
NSObject.h in the JSProfileServer.h header. And you should add @class NSString
to the header.

You need a header guard around the contents of JSProfileServer.h.

#ifndef JSProfileServer_h
#define JSProfileServer_h

// ...

#endif // JSProfileServer_h

I am thinking you don't even need to put the ObjC @interface in the header,
since no one in the process needs to call it except in the .mm file. So just
put startSharedProfileServer() in the header. Then you can remove the #ifdef
__OBJC__ and #ifdef __cplusplus. The header will be C++ only, and the .mm with
be ObjC++.

Also startSharedProfileServer() should be put in the JSC C++ namespace.

+static JSProfileServer *sharedServer = nil;

That can move into the sharedProfileServer class method, since only it accesses
it. Also you can leave off the "= nil" part, since statics default to 0.

+    listenerCount++;
+    if (listenerCount > 0) {
+	 JSStringRef profileName = JSStringCreateWithUTF8CString([serverName
UTF8String]);
+	 JSStartProfiling(0, profileName);
+	 JSStringRelease(profileName);
+    }

I think that should be:

+    if (++listenerCount > 1)
+	 return;
+    JSRetainPtr<JSStringRef> profileName(Adopt,
JSStringCreateWithUTF8CString([serverName UTF8String]));
+    JSStartProfiling(0, profileName.get());

I don't think you want to start the profiler multiple times if another start is
requested (since you don't end it the same number of times in stopProfiling).
Also you can use JSRetainPtr since this is C++, and that will do the
JSStringRelease for you. Just include JSRetainPtr.h.

+    listenerCount--;
+    if (listenerCount <= 0) {

That can be an early return.

+    ASSERT(listenerCount);
+    if (--listenerCount > 0)
+	 return;
+    JSRetainPtr<JSStringRef> profileName(Adopt,
JSStringCreateWithUTF8CString([serverName UTF8String]));
+    JSEndProfiling(0, profileName.get());

Also you should assert the listenerCount is non-zero before decrementing it.
Include <wtf/Assertions.h>.