[webkit-reviews] review denied: [Bug 30457] Allow image requests started from unload handlers to outlive the page : [Attachment 45176] Attempt #3

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Nate Chapin <japhet at chromium.org>'s
request for review:
Bug 30457: Allow image requests started from unload handlers to outlive the
page
https://bugs.webkit.org/show_bug.cgi?id=30457

Attachment 45176: Attempt #3
https://bugs.webkit.org/attachment.cgi?id=45176&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> -PassRefPtr<SubresourceLoader> SubresourceLoader::create(Frame* frame,
SubresourceLoaderClient* client, const ResourceRequest& request,
SecurityCheckPolicy securityCheck, bool sendResourceLoadCallbacks, bool
shouldContentSniff)
> +PassRefPtr<SubresourceLoader> SubresourceLoader::create(Frame* frame,
SubresourceLoaderClient* client, const ResourceRequest& request,
SecurityCheckPolicy securityCheck, OutlivePagePolicy outlivePage, bool
sendResourceLoadCallbacks, bool shouldContentSniff)
>  {
>      if (!frame)
>	   return 0;
>  
>      FrameLoader* fl = frame->loader();
> -    if (securityCheck == DoSecurityCheck && (fl->state() ==
FrameStateProvisional || fl->activeDocumentLoader()->isStopping()))
> +    if (outlivePage == DoNotOutlivePage && (fl->state() ==
FrameStateProvisional || fl->activeDocumentLoader()->isStopping()))

Argument name here is not great. It's not an outlive, nor a page. I think
outlivePagePolicy or shouldOutlivePage would be better, even though they are
long. Same for securityCheck.

> +    // If we are loading an image during an unlaod event, we want to allow
the request to outlive the page

Typo here: "unlaod".

> +    // that we are leaving.	Some sites (most commonly ad networks) rely on
image requests in beforeunload
> +    // or unload event handlers to track time spent on the page.  This will
allow them to do the tracking

We use one space after a period, not two.

> +    , m_shouldOutlivePage(shouldOutlivePage)

I noticed that m_frameForRequestThatOutlivesPage and m_shouldOutlivePage always
match.

Because of that, you could eliminate m_shouldOutlivePage, and instead have the
shouldOutlivePage() function just check if m_frameForRequestThatOutlivesPage is
0 or not. I think it would be nice to save that data member and probably
wouldn't hurt the clarity of the code.

m_frameForRequestThatOutlivesPage is not quite right, because it's used even
when the request does not outlive the page. Maybe
m_frameForRequestThatCanOutlivePage or m_frameForRequestThatShouldOutlivePage?

Maybe all the things named shouldOutlivePage should be renamed canOutlivePage.

review- just because of the header thing -- the code itself seems fine to me,
but that's too blatant a (minor) style violation