[webkit-reviews] review granted: [Bug 238082] Add logging about process entering background to NetworkProcess::processDidResume : [Attachment 455323] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted Sihui Liu <sihui_liu at apple.com>'s
request for review:
Bug 238082: Add logging about process entering background to
NetworkProcess::processDidResume
https://bugs.webkit.org/show_bug.cgi?id=238082

Attachment 455323: Patch

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




--- Comment #15 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 455323
  --> https://bugs.webkit.org/attachment.cgi?id=455323
Patch

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

r=me with changes.

> Source/WebKit/ChangeLog:8
> +	   In rdar://82412733, we found network process can be suspended right
after it receives processDidResume message. 

Sure, RunningBoard can suspend us at any point once we're no longer visible. If
something takes a process assertion (causing the processDidResume IPC to get
sent) and then the 30 second timeout in the background hits shortly after. We
may or may not get notified by Runningboard that our assertion has expired
before forced suspension and if we do get notified, we may or may not have time
to send the ProcessDidSuspend IPC (or the child process may or may not have
time to process it). This is a know issue.

This is why we should do our best not to wake up new processes while
backgrounded (technically while the RunningBoard timer has started, which is
slightly different).

> Source/WebKit/ChangeLog:9
> +	   For network porcess, processDidResume means process is not suspended
and it is safe to perform database 

typo: porcess.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2132
> +    RELEASE_LOG(ProcessSuspension, "%p - NetworkProcess::prepareToSuspend(),
isSuspensionImminent=%d Process is %sin background", this,
isSuspensionImminent, m_enterBackgroundTimestamp ? "" : "not ");

%{public}s

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2179
> +    String backgroundIntervalString = "";

emptyString()

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2181
> +	   backgroundIntervalString = makeString(" Process is in background for
", (MonotonicTime::now() - m_enterBackgroundTimestamp.value()).value(), "
seconds");

is -> has been

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2182
> +    RELEASE_LOG(ProcessSuspension, "%p - NetworkProcess::processDidResume()
forForegroundActivity=%d%s", this, forForegroundActivity,
backgroundIntervalString.utf8().data());

This %s won't print on public builds. We could use %{public}s but I would just
suggest using branching:
if (m_enterBackgroundTimestamp)
    RELEASE_LOG(...)
else
    RELEASE_LOG(...)

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.h:239
> +    void sendProcessDidResume(ResumeReason =
ResumeReason::ForegroundActivity) final;

Can we avoid having a default parameter on a virtual function?
Seems also error prone to not force the caller to specify it.

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.h:305
> +    void applicationDidEnterBackground();

Can these be private?

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.h:308
> +    void addBackgroundStateObservers();

Can these be private?

> Source/WebKit/UIProcess/Network/NetworkProcessProxyCocoa.mm:101
> +	       return;

nit: Early returns for one liners seem overly verbose.

> Source/WebKit/UIProcess/Network/NetworkProcessProxyCocoa.mm:106
> +	       return;

ditto.

> Source/WebKit/UIProcess/ProcessThrottlerClient.h:41
> +    virtual void sendProcessDidResume(ResumeReason = ForegroundActivity) =
0;

Ideally we wouldn't have a default parameter value.