[webkit-reviews] review denied: [Bug 30457] Allow image requests started from unload handlers to outlive the page : [Attachment 64760] ResourceHandleClient-level solution + rewritten layout test

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> 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 64760: ResourceHandleClient-level solution + rewritten layout test
https://bugs.webkit.org/attachment.cgi?id=64760&action=review

------- Additional Comments from David Levin <levin at chromium.org>
I like the approach as it is very simple. My biggest concern is if the
ResourceHandle may have an unfortunate amount of knowledge about the frame (see
below).


WebCore/loader/DocLoader.cpp:126
 +	    if (f->loader()->pageDismissalEventBeingDispatched()) {
Shouldn't this be after the allowImages check just below this?

It seems odd that the user would disable this and then we have a loophole here.


WebCore/loader/PingLoader.cpp:41
 +  void PingLoader::loadOutlivingImage(Frame* frame, const String& url)
How about just naming this loadImage?
(I can't parse loadOutlivingImage without already knowing what it does
already.)


WebCore/loader/PingLoader.cpp:42
 +  {
Shouldn't we check to see if the url is valid? (after completing it).


WebCore/loader/PingLoader.cpp:43
 +	if (SecurityOrigin::restrictAccessToLocal() &&
!SecurityOrigin::canLoad(KURL(ParsedURLString, url), String(),
frame->document())) {
Shouldn't the canLoad call be done using the "completeURL"?


WebCore/loader/PingLoader.cpp:48
 +	ResourceRequest r(frame->document()->completeURL(url));
Please avoid using abbreviates for variables "r".

WebCore/loader/PingLoader.cpp:54
 +	new PingLoader(frame, r);
This needs a PassOwnPtr and leakPtr around it.


WebCore/loader/PingLoader.cpp:59
 +	m_handle = ResourceHandle::create(request, this, frame, false, false);
How does this live past the lifetime of the frame? Does any platform store
information from the frame that will live on past this request? And if so, how
does that work?

For example WebCore/platform/network/qt/ResourceHandleQt.cpp, does this
    getInternal()->m_frame =
static_cast<FrameLoaderClientQt*>(frame->loader()->client())->webFrame();

WebCore/loader/DocLoader.cpp:127
 +		PingLoader::loadOutlivingImage(f, url);
It looks like this also skips checks that would normally be done for image
loading in DocLoader::canRequest.

WebCore/loader/PingLoader.h:47
 +  // and don't depend on their Frame staying alive (i.e., auditing
pingbacks).
Consider:
  
Triggers asynchronous loads independent of Frame staying alive (i.e., auditing
pingbacks).


WebCore/loader/PingLoader.h:58
 +	~PingLoader() { m_handle->cancel(); }
Please move the destructor to the cpp file, and then get rid of the "#include
"ResourceHandle.h"" from this header file.


LayoutTests/http/tests/navigation/resources/check-ping.php:7
 +	// This refresh header is teh sadness, but if the file doesn't
typo:teh Also, I understand what you are saying but it seems like a
colloquialism and I'm not sure how well it will work for non-native speakers.

Perhaps, you might want to consider something more standard like
"Unfortunately, we have to do a refresh because..."


LayoutTests/http/tests/navigation/resources/check-ping.php:19
 +  $result = ($expectedPingValue == $actualPingValue) ? "PASS" : "FAIL";
Should this be part of the "refresh" sequence? Just because the file exists
does that mean that its contents have been flushed to disk? (and the unlink
done after this step).