[webkit-reviews] review granted: [Bug 30457] Allow image requests started from unload handlers to outlive the page : [Attachment 44996] Attempt #2

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted 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 44996: Attempt #2
https://bugs.webkit.org/attachment.cgi?id=44996&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> -		   m_unloadEventBeingDispatched = true;
> +		   m_isDispatchingUnloadEvent = true;

Maybe we should assert !m_isDispatchingUnloadEvent here?/

> -		   m_unloadEventBeingDispatched = false;
> +		   m_isDispatchingUnloadEvent = false;

Maybe we should assert !m_isDispatchingUnloadEvent here.

> +    m_isDispatchingBeforeUnloadEvent = false;

Maybe we should assert ! m_isDispatchingBeforeUnloadEvent here.

> -    m_unloadEventBeingDispatched = true;
> +    m_isDispatchingUnloadEvent = true;

Likewise.

> -    m_unloadEventBeingDispatched = false;
> +    m_isDispatchingUnloadEvent = false;

And again.

> -void DocLoader::incrementRequestCount()
> +void DocLoader::addRequest(Request* request)
>  {
>      ++m_requestCount;
> +    if (request->shouldOutlivePage()) {
> +	   if (!m_requestsThatOutlivePageCount++)
> +	       m_frameForRequestsThatOutlivePage = m_doc->frame();
> +    }
> +	   
>  }

Extra blank line there.

It seems kind of strange to choose the Frame based on the first request that
should outlive the page. Is it really right for all those later requests for
the same resource to also share that one frame? I think the code at least needs
a comment to make it clear what the policy is and why that's OK.

> +    int requestsThatOutlivePageCount() { return
m_requestsThatOutlivePageCount; }

It's a little strange to expose this just so we can use it in an assertion.
Maybe just expose the boolean condition instead? Or not have the assertion?

> +    OutlivePagePolicy shouldOutlivePage = resource->isImage() && docLoader
&& docLoader->frame() 
> +	   && docLoader->frame()->loader()->isDispatchingUnloadEvent()
> +	   ? OutlivePage : DoNotOutlivePage;

I think this should go in a paragraph of its own and have its own comment. The
comment should explain why images have a different policy than other loads.

> +    // If we're doing a load that can outlive the current page, we don't
want this request to block navigating
> +    // away from the current document.
> +    if (request->shouldOutlivePage() == DoNotOutlivePage && (priority > Low
|| !url.protocolInHTTPFamily() || !hadRequests)) {

The comment is a good idea, but its worded backward from the code, and so a bit
hard to read. If you really wanted code that matched that comment, then you'd
probably say:

    if (request->shouldOutlivePage() == DoNotOutlivePage)
	priority = Low;

> +    bool isDispatchingUnloadEvent() const { return
m_isDispatchingBeforeUnloadEvent || m_isDispatchingUnloadEvent; }

I think it's confusing to have a function member and a data member with the
same name, but different meanings. I think the function needs to be renamed.

I'm going to say review+ but I think you might decide to do a new patch based
on at least one of my comments above