[webkit-reviews] review denied: [Bug 106513] Groundwork to enable process suppression by default on Mac : [Attachment 182682] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Kiran Muppala
<cmuppala at apple.com>'s request for review:
Bug 106513: Groundwork to enable process suppression by default on Mac
https://bugs.webkit.org/show_bug.cgi?id=106513

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

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=182682&action=review


This looks good to me. Since you are not a committer, and need to update the
patch anyway to fix the build, I'll say r-, but there is nothing really else
blocking this from being landed.

Comments below are optional, although I think that you'll want to address some
of them.

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:65
> +static bool s_applicationIsOccluded = false;

I'm not sure if this variable's value is accurate. Do we get an occlusion
notification if the application launches in background in an invisible space
(e.g. as part of relaunching applications after a restart)? Even if we get one,
that might happen before we installed a handler.

This is more like a stored value of latest occlusion handler invocation than
accurate information about occlusion state, unless I'm missing something. This
variable name is too tempting, and someone could use it in an incorrect way
later.

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:67
> +static bool s_processSuppressionEnabledForAllContexts = true; //
Initializing to true simplifies WebContext::platformInitialize.

I don't think that this comment is helpful. The value is logically true when
there are no contexts (because it's a result of statement applied to an empty
set).

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:85
> +static void setApplicationIsOccludedForChildProcessesInContext(bool
isOccluded, WebContext* context)

Despite what I said about moving functions to .cpp files, this might be going
further than we usually go, an is thus surprising.

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:109
> +static void setApplicationIsOccluded(bool isOccluded)

I think that behavior of this function is just fine, but the name makes it
confusingly look like a simple setter (same applies to specialized versions
above). Perhaps "notifyProcessesIfOcclusionStateChanged()" would be correct and
more helpful?

Alternatively, one can think about this as common code for
applicationBecameVisible and applicationBecameOccluded functions, and name this
"applicationBecameOccluded()".

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:114
> +    if (s_applicationIsOccluded == isOccluded)
> +	   return;
> +
> +    s_applicationIsOccluded = isOccluded;

In fact, I'd consider moving this logic to callers, even though this means a
few lines of duplicated code. That way, this function would only do one thing,
not two.

Also, this would make it clearer that we don't rely on these notifications to
not come twice, and perhaps trigger an interesting investigation of why we
can't rely on them.

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:161
> +static void setProcessSuppressionEnabledForAnyContext(bool enabled)

Here as well, the name is more appropriate for a plain setter, not for a
function whose primary goal is to register or unregister handlers. It's an
implementation detail that a static variable tracks having the handlers
currently installed.

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:193
> +static bool isProcessSuppressionEnabledForAnyContext()

We normally try to make function names are grammatically correct as feasible.
Here, processSuppressionIsEnabledForAnyContext() would be more in accordance
with WebKit style.

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:226
> +    if (m_processSuppressionEnabled)
> +	   setProcessSuppressionEnabledForAnyContext(true);
> +    else
> +	   setProcessSuppressionEnabledForAllContexts(false);

I don't get this. Isn't this condition a constant? I think that
platformInitialize() is called before a client could have a chance to change
the value of m_processSuppressionEnabled.

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:436
> +    return s_applicationIsOccluded;

This function is dead code AFAICT, and should be removed.