Bug 201821 – Simplify WebResourceLoadObserver now that we have one WebProcess per session

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

Simplify WebResourceLoadObserver now that we have one WebProcess per session.

[Chris Dumez]

Created attachment 378865 [details]
Patch

[Brent Fulgham]

WinCairo failure appears to be a bot problem, unrelated to this patch.

[Brent Fulgham]

Comment on attachment 378865 [details]
Patch

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

> Source/WebKit/ChangeLog:9
> +        No need to pass sessionIDs around or stores statistics per sessionID.

"or store"

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:-214
> -        return;

Do we really not need this anymore? Even if we don't need to pass sessionID to the rest of the ITP system, surely preventing passing of observations when we should don't log for the current network session seems like a good idea?

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:-238
> -        return;

Ditto.

> Source/WebKit/WebProcess/WebProcess.cpp:456
> +    if (parameters.resourceLoadStatisticsEnabled && !parameters.sessionID.isEphemeral())

While we don't log ITP data for ephemeral systems right now, we do want to apply these protections in the future. But I guess this is fine for now.

[Chris Dumez]

Comment on attachment 378865 [details]
Patch

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

>> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:-214
>> -        return;
> 
> Do we really not need this anymore? Even if we don't need to pass sessionID to the rest of the ITP system, surely preventing passing of observations when we should don't log for the current network session seems like a good idea?

If you look at WebProcess.cpp, you'll see that I do not even initialize the WebResourceLoadObserver if ITP is disabled or the sessionID is ephemeral.

Actually, I should probably drop the shouldLogResourceLoadStatistics() method entirely, looks like I forgot.

[Chris Dumez]

(In reply to Chris Dumez from comment #4)
> Comment on attachment 378865 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=378865&action=review
> 
> >> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:-214
> >> -        return;
> > 
> > Do we really not need this anymore? Even if we don't need to pass sessionID to the rest of the ITP system, surely preventing passing of observations when we should don't log for the current network session seems like a good idea?
> 
> If you look at WebProcess.cpp, you'll see that I do not even initialize the
> WebResourceLoadObserver if ITP is disabled or the sessionID is ephemeral.
> 
> Actually, I should probably drop the shouldLogResourceLoadStatistics()
> method entirely, looks like I forgot.

The ITP system already has the sessionID without the WebContent process sending it to it since the NetworkConnectionToWebProcess knows the sessionID.

[Chris Dumez]

Comment on attachment 378865 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:696
> +    if (auto* networkSession = this->networkSession()) {

See here, it is getting the networkSession for the sessionID of the WebProcess (See implementation of networkSession()).

[Brent Fulgham]

Comment on attachment 378865 [details]
Patch

Looks good. r=me.

[Chris Dumez]

Created attachment 378873 [details]
Patch

[Chris Dumez]

Created attachment 378874 [details]
Patch

[Chris Dumez]

Created attachment 378881 [details]
Patch

[Chris Dumez]

Created attachment 378886 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 378886 [details]
Patch

Clearing flags on attachment: 378886

Committed r249920: <https://trac.webkit.org/changeset/249920>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/55414296>