[webkit-reviews] review granted: [Bug 17897] Not Rendering Images Imported from XHTML Document : [Attachment 24957] New iteration: Tackles Darin's comments and match FF & Opera

[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 24957: New iteration: Tackles Darin's comments and match FF & Opera
https://bugs.webkit.org/attachment.cgi?id=24957&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
> -	       m_imageLoader->updateFromElement();
> +	       m_imageLoader->updateFromElement(true);

The reason I don't like boolean arguments to functions like this is it's so
hard to understand them at the call site. It would be a lot nicer if that
argument had a name like TryAgain instead of just being true, or if this was a
separate function with a sensible name, even if it only ends up being a boolean
argument anyway.

> -void ImageLoader::updateFromElement()
> +void ImageLoader::updateFromElement(bool shouldIgnorePreviousLoadFailure)

You could also call this shouldTryLoadingAgain, or shouldRetryLoad. Perhaps a
bit less precise but also maybe a little easier to understand.

> +    AtomicString m_sourceURL;
>      bool m_firedLoad : 1;
>      bool m_imageComplete : 1;
>      bool m_loadManually : 1;
> +    bool m_hadError : 1;

After reading the logic carefully, I realize that you only need to store the
old URL in the case where the load failed. I think we could change this to work
that way. Then you could have m_failedLoadURL instead of m_sourceURL and not
bother with m_hadError at all. I think the logic would stay simple and clear in
that case.

r=me as-is, but we could consider making a little further improvement.