[webkit-reviews] review denied: [Bug 106513] Enable process suppression by default on Mac : [Attachment 182198] 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: Enable process suppression by default on Mac
https://bugs.webkit.org/show_bug.cgi?id=106513

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

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


I'm not sure if it's beneficial for NetworkProcess to ever be suppressed.

It might be helpful to explain the initial state in
ChildProcess::platformInitialize(). Perhaps something like below, or maybe more
in-depth:

    // Starting with suppression disabled. UI process will set an actual value
from didFinishLaunching().

r- for readability refactoring, as discussed in person.

> Source/WebKit2/UIProcess/API/C/mac/WKContextPrivateMac.h:27
> +#ifndef WKContextPrivateMac_h
> +#define WKContextPrivateMac_h

I'm a little uneasy about adding an API header with "Mac" in it, but given the
precedent of WKPagePrivateMac.h, it's probably OK.

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:368
> +void WebContext::processSuppressionSupportChanged()

I have a very difficult time understanding what this function does, and when it
should be called. Kiran and myself spent some time in person discussing this,
and it seems like this would really benefit from a re-design. There are
multiple things done here:

1. Iterate over contexts, check if any or all of them disable the feature

2. Notify global processes (plugin, shared worker)

3. Register or unregister for notifications

Can you split this into functions that do each of these things, and have
descriptive names?

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:374
> +    bool newOcclusionNotificationsEnabled = false;

This should be called either newProcessSuppressionSupportEnabledForAnyContext,
or shouldEnableOccusionNotifications.

Perhaps you can split the iteration into separate functions. This is not hot
code, so it's OK to iterate contexts twice, or even more.

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:395
> +	   }
> +	   else {

Style nit: there should be no newline here.