[webkit-reviews] review granted: [Bug 17897] Not Rendering Images Imported from XHTML Document : [Attachment 23215] Updated patch: Store source url and check it when determining if the loader is in error

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Julien Chaffraix
<jchaffraix at pleyo.com>'s request for review:
Bug 17897: Not Rendering Images Imported from XHTML Document
https://bugs.webkit.org/show_bug.cgi?id=17897

Attachment 23215: Updated patch: Store source url and check it when determining
if the loader is in error
https://bugs.webkit.org/attachment.cgi?id=23215&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
> // our loader may have not fetched the image so do it now.

Should have a comma before "so do it now".

> // If we do not have an image here, it means that an error
> // occurred (cross-site violation...).

I don't understand the meaning of "..." in this comment.

  class HTMLImageLoader : public CachedResourceClient {
+
  public:

Why are you adding a blank line here? We normally don't do that. The other
blank line you added seems fine.

I'm not sure the behavior here is right. If changing the URL triggers a new
load, it's possible that re-setting the URL to the same value also should
trigger a new load. We should test this in other browsers to be sure we match
their behavior.

I'm going to say r=me because I think it's OK to land this patch as-is. But I'd
really like to see us test that "does changing the URL to the same value
trigger a load" question, because I think it may be needed to match the
behavior of other web browsers.