Bug 58592 – WebKit2: Need a way to keep the WebProcess alive for testing purposes

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Jessie Berlin]

<rdar://problem/9278779>

[Jessie Berlin]

Created attachment 89679 [details]
Patch

[Brian Weinstein]

Comment on attachment 89679 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=89679&action=review

I feel like everywhere this should be webProcessCanTerminate instead of webProcessCanTerminateEarly.

> Source/WebKit2/UIProcess/WebContext.cpp:267
> +    m_webProcessCanTerminateEarly = allowed;

Is an early return if allowed == m_webProcessCanTerminateEarly helpful?

> Source/WebKit2/UIProcess/WebContext.h:265
> +    bool m_webProcessCanTerminateEarly;

Blank line before this might be nice.

> Source/WebKit2/UIProcess/API/C/WKContextPrivate.h:64
> +// FIXME: This is a workaround for testing purposes only.

So is this the FIXME to remove this? I'm not sure what the FIXME represents here.

[Jessie Berlin]

(In reply to comment #2)
> (From update of attachment 89679 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89679&action=review
> 
> I feel like everywhere this should be webProcessCanTerminate instead of webProcessCanTerminateEarly.

Ok, I don't feel strongly about this. Changed to webProcessCanTerminate.

> 
> > Source/WebKit2/UIProcess/WebContext.cpp:267
> > +    m_webProcessCanTerminateEarly = allowed;
> 
> Is an early return if allowed == m_webProcessCanTerminateEarly helpful?

I don't think so, given the current implementation of shouldTerminate, but I will add it in case shouldTerminate for some reason gets more complicated.

> 
> > Source/WebKit2/UIProcess/WebContext.h:265
> > +    bool m_webProcessCanTerminateEarly;
> 
> Blank line before this might be nice.

Done.

> 
> > Source/WebKit2/UIProcess/API/C/WKContextPrivate.h:64
> > +// FIXME: This is a workaround for testing purposes only.
> 
> So is this the FIXME to remove this? I'm not sure what the FIXME represents here.

Yes, we should come up with a way to do the testing such that this is not necessary. I will revise the comment to say:

// FIXME: This is a workaround for testing purposes only and should be removed once a better solution has been found for testing.

[Anders Carlsson]

Comment on attachment 89679 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=89679&action=review

> Source/WebKit2/UIProcess/API/C/WKContextPrivate.h:66
> +WK_EXPORT void WKContextSetWebProcessCanTerminateEarly(WKContextRef context, bool canTerminate);
> +

I don't like this name. Could we instead have

WKContextDisableProcessTermination and WKContextEnableProcessTermination?

[Jessie Berlin]

(In reply to comment #4)
> (From update of attachment 89679 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89679&action=review
> 
> > Source/WebKit2/UIProcess/API/C/WKContextPrivate.h:66
> > +WK_EXPORT void WKContextSetWebProcessCanTerminateEarly(WKContextRef context, bool canTerminate);
> > +
> 
> I don't like this name. Could we instead have
> 
> WKContextDisableProcessTermination and WKContextEnableProcessTermination?

I will change it to use those names, and since that is so much of the patch I will send it out for review again.

[Jessie Berlin]

Created attachment 89798 [details]
Patch (Take 2)

[Jessie Berlin]

Comment on attachment 89798 [details]
Patch (Take 2)

Committed in http://trac.webkit.org/changeset/83988