The Resource Load Statistics stuff should skip private browsing sessions (at least for now).
<rdar://problem/24987873>
Created attachment 273063 [details] Patch
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.
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.
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.
Committed r197608: <http://trac.webkit.org/changeset/197608>