[webkit-reviews] review denied: [Bug 17897] Not Rendering Images Imported from XHTML Document : [Attachment 22964] Bug fix: fetch image when inserted if necessary and add an error flag in the loader

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 17897: Not Rendering Images Imported from XHTML Document
https://bugs.webkit.org/show_bug.cgi?id=17897

Attachment 22964: Bug fix: fetch image when inserted if necessary and add an
error flag in the loader
https://bugs.webkit.org/attachment.cgi?id=22964&action=edit

------- Additional Comments from Eric Seidel <eric at webkit.org>
So are new HTMLImageLoader objects created every time a load is issued? 
Otherwise I fear your patch will break things like:

image.src = "willfail.png"
// first load returns an error, possibly from the error callback we do:
image.src = "willwork.png"

Also, I'm not a big fan of functions being declared in the middle of function
calls:

 26		    icon.addEventListener("load", function () {
 27			var console = document.getElementById('console');
 28			// If the image was loaded correctly, then height and
width are not
 29			// zero.
 30			if (icon.height && icon.width)
 31			    console.innerHTML = "PASSED";
 32			else
 33			    console.innerHTML = "FAILED";
 34 
 35			if (window.layoutTestController)
 36			    layoutTestController.notifyDone();
 37 
 38		    }, true);

I'd rather see that declared in a local variable and used instead of as part of
that function call.

Marking r- for now.  If you are convinced that the "load works after error
case" is OK (i.e. that we already have a test case which checks that and it
passes), then you can go ahead and land this.  Otherwise we'll need a test case
to prove that the "load a new url after the first one fails" case works.