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

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied 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 455124: Patch

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




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

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

> 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:2117
> +    RELEASE_LOG(ProcessSuspension, "%p - NetworkProcess::prepareToSuspend(),
isSuspensionImminent=%d Process is %s in background", this,
isSuspensionImminent, m_enterBackgroundTimestamp ? "" : "not");

There is going to be a extra space between "is" and "in": "Process is  in
background"

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2151
> +    m_enterBackgroundTimestamp = WallTime::now();

We want to be using MonotonicTime, not WallTime. If the timezone changes (or
daylight savings) while suspended for e.g., you'd get unexpected results.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2168
> +	   auto interval = WallTime::now() -
m_enterBackgroundTimestamp.value();

MonotonicTime

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2169
> +	   RELEASE_LOG_ERROR(ProcessSuspension, "%p -
NetworkProcess::processDidResume() Process is in background for %f ms", this,
interval.value());

How is that an error?

Also, I have a feeling interval.value() is seconds and the logging claims it is
milliseconds?

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewTesting.mm:216
> +	   _page->process().sendProcessDidResume(true);

This is the reason we use enum classes for parameters and not booleans, the
reader has no idea what true means here.

> Source/WebKit/UIProcess/ios/ProcessAssertionIOS.mm:97
> +	   WebKit::WebsiteDataStore::forEachWebsiteDataStore([]
(WebKit::WebsiteDataStore& dataStore) {

Seems like a layer violation to be using WebsiteDataStore inside
ProcessAssertionIOS.mm (as a ProcessAssertion could theoretically be taken in
any process).
Odds are we want to set a new listener inside NetworkProcessCocoa.mm instead.

> Source/WebKit/UIProcess/ios/ProcessAssertionIOS.mm:99
> +		   networkProcess->applicationWillEnterForeground();

Looks like this will send duplicate IPCs if several data stores share the same
network process (which is always the case on Cocoa)_

> Source/WebKit/UIProcess/ios/ProcessAssertionIOS.mm:109
> +		   networkProcess->applicationDidEnterBackground();

ditto.