[webkit-reviews] review granted: [Bug 201155] Regression: ITP does a lot more IPC that it used to since its logic was moved to the network process : [Attachment 377278] Patch

[bugzilla-daemon at webkit.org]

John Wilander <wilander at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 201155: Regression: ITP does a lot more IPC that it used to since its logic
was moved to the network process
https://bugs.webkit.org/show_bug.cgi?id=201155

Attachment 377278: Patch

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




--- Comment #3 from John Wilander <wilander at apple.com> ---
Comment on attachment 377278
  --> https://bugs.webkit.org/attachment.cgi?id=377278
Patch

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

Looks good to me. See minor comments.

> Source/WebCore/ChangeLog:3
> +	   Regression: ITP does a lot more IPC that it used to since its logic
was moved to the network process

I think "after its logic was moved" makes it more clear. Otherwise, some may
interpret that the move itself causes more IPC.

> Source/WebCore/ChangeLog:8
> +	   ITP does a lot more IPC that it used to since its logic was moved to
the network process. Web processes used to

since -> after

> Source/WebKit/ChangeLog:3
> +	   Regression: ITP does a lot more IPC that it used to since its logic
was moved to the network process

since –> after

> Source/WebCore/loader/ResourceLoadObserver.cpp:125
> +	   scheduleNotificationIfNeeded();

We used to have a local boolean that captured whether this call to the log
function had caused a statistics update or not, and only schedule a
notification if so. But the generalized check for the existence of any batched
up statistics is fine too. As long as
m_perSessionResourceStatisticsMap.isEmpty() is performant which I assume it is.

> Source/WebKit/WebProcess/WebProcess.cpp:228
> +   
ResourceLoadObserver::shared().setLogUserInteractionNotificationCallback([this]
(PAL::SessionID sessionID, const RegistrableDomain& domain) {	
ensureNetworkProcessConnection().connection().send(Messages::NetworkConnectionT
oWebProcess::LogUserInteraction(sessionID, domain), 0);

I believe we used to have a code comment explaining why this particular IPC
call was done directly so that others understand why.