Bug 126330 – Refactor NSActivity handling code from ChildProcess to UserActivity

• Component: WebKit2
• Status: RESOLVED FIXED

[Gavin Barraclough]

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.

[Gavin Barraclough]

Created attachment 220175 [details]
Fix

[EFL EWS Bot]

Comment on attachment 220175 [details]
Fix

Attachment 220175 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/5535485384458240

[Sam Weinig]

Comment on attachment 220175 [details]
Fix

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)?

[Gavin Barraclough]

Created attachment 220179 [details]
GTK fix

[Gavin Barraclough]

> > 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?

Ooops, debug code I was using to test, meant to delete. :-|

Will clean up & fix a couple of the other review comments, then put up a new patch. Clearing r? for now.

[Gavin Barraclough]

Created attachment 220282 [details]
Fix

[EFL EWS Bot]

Comment on attachment 220282 [details]
Fix

Attachment 220282 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/6405113719554048

[Gavin Barraclough]

Created attachment 220285 [details]
gtk fix

[EFL EWS Bot]

Comment on attachment 220285 [details]
gtk fix

Attachment 220285 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/5789202524405760

[Gavin Barraclough]

Committed revision 161271.