Bug 155054 – Resource load statistics are not honoring private browsing

• Component: WebKit2
• Status: RESOLVED FIXED

[Brent Fulgham]

The Resource Load Statistics stuff should skip private browsing sessions (at least for now).

[Radar WebKit Bug Importer]

<rdar://problem/24987873>

[Brent Fulgham]

Created attachment 273063 [details]
Patch

[Andy Estes]

Comment on attachment 273063 [details]
Patch

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

r=me, but I'd like to see the policy decisions of whether or not to log be made inside ResourceLoadObserver, not by the callers.

> Source/WebCore/dom/Document.cpp:6406
> -    if (!m_lastHandledUserGestureTimestamp)
> +    bool needPrivacy = page() ? page()->usesEphemeralSession() : false;
> +    if (!m_lastHandledUserGestureTimestamp && !needPrivacy)

You're already passing a Document reference to ResourceLoadObserver. Can't the observer figure out if the Document is part of an ephemeral session and not log?

> Source/WebCore/loader/DocumentLoader.cpp:537
> -    if (Settings::resourceLoadStatisticsEnabled())
> +    bool needPrivacy = topFrame.page() ? topFrame.page()->usesEphemeralSession() : false;
> +    if (Settings::resourceLoadStatisticsEnabled() && !needPrivacy)

I'm surprised you check Settings::resourceLoadStatisticsEnabled() here, since you also check it inside logFrameNavigation().

I'd prefer to have these checks be inside ResourceLoadObserver. If you changed logFrameNavigation() to take a ResourceRequest, ResourceResponse, and Frame then it could determine everything it needs from those three things.

> Source/WebCore/loader/SubresourceLoader.cpp:195
> -    if (Settings::resourceLoadStatisticsEnabled())
> +    bool needPrivacy = (m_frame && m_frame->page()) ? m_frame->page()->usesEphemeralSession() : false;
> +    if (Settings::resourceLoadStatisticsEnabled() && !needPrivacy)

Same comment as above.

[Brent Fulgham]

Comment on attachment 273063 [details]
Patch

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

>> Source/WebCore/dom/Document.cpp:6406
>> +    if (!m_lastHandledUserGestureTimestamp && !needPrivacy)
> 
> You're already passing a Document reference to ResourceLoadObserver. Can't the observer figure out if the Document is part of an ephemeral session and not log?

Yep. I'll do that.

>> Source/WebCore/loader/DocumentLoader.cpp:537
>> +    if (Settings::resourceLoadStatisticsEnabled() && !needPrivacy)
> 
> I'm surprised you check Settings::resourceLoadStatisticsEnabled() here, since you also check it inside logFrameNavigation().
> 
> I'd prefer to have these checks be inside ResourceLoadObserver. If you changed logFrameNavigation() to take a ResourceRequest, ResourceResponse, and Frame then it could determine everything it needs from those three things.

Brady suggested this, because we want to avoid computing multiple URLs if we have this diagnostic turned off. By checking this boolean first, we only make a very simple operation before doing any meaningful work.

However, I'm open to revisiting this. For example, maybe we pass these various values into the method and calculate URL's, etc., inside the method.

[Andy Estes]

Comment on attachment 273063 [details]
Patch

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

>>> Source/WebCore/loader/DocumentLoader.cpp:537
>>> +    if (Settings::resourceLoadStatisticsEnabled() && !needPrivacy)
>> 
>> I'm surprised you check Settings::resourceLoadStatisticsEnabled() here, since you also check it inside logFrameNavigation().
>> 
>> I'd prefer to have these checks be inside ResourceLoadObserver. If you changed logFrameNavigation() to take a ResourceRequest, ResourceResponse, and Frame then it could determine everything it needs from those three things.
> 
> Brady suggested this, because we want to avoid computing multiple URLs if we have this diagnostic turned off. By checking this boolean first, we only make a very simple operation before doing any meaningful work.
> 
> However, I'm open to revisiting this. For example, maybe we pass these various values into the method and calculate URL's, etc., inside the method.

Right, I was thinking you'd just pass in redirectResponse, newRequest, and m_frame. You'd only compute the URLs if the logging is enabled.

[Brent Fulgham]

Committed r197608: <http://trac.webkit.org/changeset/197608>