[webkit-reviews] review denied: [Bug 126330] Refactor NSActivity handling code from ChildProcess to UserActivity : [Attachment 220175] Fix

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied Gavin Barraclough
<barraclough at apple.com>'s request for review:
Bug 126330: Refactor NSActivity handling code from ChildProcess to UserActivity
https://bugs.webkit.org/show_bug.cgi?id=126330

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

------- Additional Comments from Sam Weinig <sam at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=220175&action=review


> Source/WTF/ChangeLog:16
> +	   UserActivity is a mechanism to express to the operating system
(where appropriate)
> +	   that a user initiated activity is taking place, and as such that
resources should be
> +	   made available to the process accordingly.
> +
> +	   Currently we hold a single NSActivity, at the WebKit layer. This
refactoring allows us
> +	   to hold different activity tokens for different user actions (which
simplifies the
> +	   handling, and aides debugging since the token can more accurately
express the activity
> +	   taking place), and also will allow us to avoid the layering
difficulty of calling back
> +	   up the stack to WebKit to register that an activity is taking place.


Why is WTF the right layer for this?  Will JSC want to use this? If not, it
might make more sense to put this in WebCore/platform.

> Source/WTF/wtf/UserActivity.h:34
> +class UserActivity {

I think this is an abstract enough concept that this could use a comment
explaining what this about.

> Source/WTF/wtf/UserActivity.h:46
> +#if PLATFORM(MAC) && !PLATFORM(IOS) && __MAC_OS_X_VERSION_MIN_REQUIRED >=
1090

Instead of this complicated #ifdef here, I would rather there be a single
HAVE(NS_ACTIVITY) or something like that.

> Source/WTF/wtf/UserActivity.h:50
> +    RetainPtr<id> m_description;

Can this be a WTF::String or at least a RetainPtr<NSString>.

> Source/WTF/wtf/mac/UserActivityMac.mm:66
> +    syslog(LOG_ERR, "beginActivity(%s) -> %d", [m_description.get()
UTF8String], (int)m_count);

Why syslog and not one of the normal logging means?

> Source/WTF/wtf/mac/UserActivityMac.mm:93
> +//	 ASSERT(isValid());

This looks bogus.

> Source/WebKit2/Shared/ChildProcess.h:125
> +    UserActivity m_processSuppressionDisabled;
> +    UserActivity m_activeTasks;

Since we have cross platform type now, can we move these out of PLATFORM(MAC)?