[webkit-reviews] review granted: [Bug 209928] [iOS] AuxiliaryProcessProxy::sendWithAsyncReply() should prevent auxiliary process suspension while processing the IPC : [Attachment 398676] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 209928: [iOS] AuxiliaryProcessProxy::sendWithAsyncReply() should prevent
auxiliary process suspension while processing the IPC
https://bugs.webkit.org/show_bug.cgi?id=209928

Attachment 398676: Patch

https://bugs.webkit.org/attachment.cgi?id=398676&action=review




--- Comment #14 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 398676
  --> https://bugs.webkit.org/attachment.cgi?id=398676
Patch

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

> Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:155
> +    if (asyncReplyInfo && canSendMessage() &&
shouldStartProcessThrottlerActivity ==
ShouldStartProcessThrottlerActivity::Yes) {

This is fine like this, but it seems that, in theory we should only do that
when sending the messages, so for State::Running.
We would then need to take a similar assertion in
AuxiliaryProcessProxy::didFinishLaunching.
With a helper routine, we would share the code between both.

> Source/WebKit/UIProcess/AuxiliaryProcessProxy.h:53
> +    virtual ProcessThrottler& throttler() = 0;

Do we need to have it public?

> Source/WebKit/UIProcess/GPU/GPUProcessProxy.h:60
> +    ProcessThrottler& throttler() final { return m_throttler; }

Do we need to have it public?

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.h:196
> +    ProcessThrottler& throttler() final { return m_throttler; }

Do we need to have it public?

> Source/WebKit/UIProcess/Plugins/PluginProcessProxy.h:92
> +    ProcessThrottler& throttler() final { return m_throttler; }

Do we need to have it public?

> Source/WebKit/UIProcess/ProcessThrottler.h:68
> +	       if (!isQuietActivity()) {

We are losing some logging, this might be fine but just wondering whether we
should add some logging at some of call sites to replace this logging.