[webkit-reviews] review granted: [Bug 182664] Resource Load Statistics: Classify resources as prevalent based on redirects to other prevalent resources : [Attachment 333539] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has granted John Wilander
<wilander at apple.com>'s request for review:
Bug 182664: Resource Load Statistics: Classify resources as prevalent based on
redirects to other prevalent resources
https://bugs.webkit.org/show_bug.cgi?id=182664

Attachment 333539: Patch

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




--- Comment #4 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 333539
  --> https://bugs.webkit.org/attachment.cgi?id=333539
Patch

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

Very nice! I asked you to make a few adjustments to calm my fears, but
otherwise this looks good.

> Source/WebCore/loader/ResourceLoadObserver.cpp:167
> +    auto& targetStatistics =
ensureResourceStatisticsForPrimaryDomain(targetPrimaryDomain);

I don't see why this line had to be moved. It makes me nervous, because we have
had many threading/memory corruption bugs caused by invalidated iterators, and
I'd prefer to limit the scope of these references to the smaller area possible.
It doesn't seem like this is referenced again in your new code below, so I
don't think you should make this change.

Or did you do this to make sure there is an existing entry for the top frame in
the redirect code below? If so, I propose you do so at the point you need it,
and add the same comment you do elsewhere ("For consistency, make sure we also
have a statistics entry for the top frame domain."). I would also give it the
name "topFrameStatistics" in the redirect sections below.

> Source/WebCore/loader/ResourceLoadObserver.cpp:234
> +    auto& targetStatistics =
ensureResourceStatisticsForPrimaryDomain(targetPrimaryDomain);

Ditto my comments in the "logFrameNavigation" code.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:488
> +{

Please add "ASSERT(!RunLoop::isMain());" here.