[webkit-reviews] review denied: [Bug 71661] Image onload callback fires when image loading disabled. : [Attachment 116779] Patch.

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Ben Murdoch <benm at google.com>'s
request for review:
Bug 71661: Image onload callback fires when image loading disabled.
https://bugs.webkit.org/show_bug.cgi?id=71661

Attachment 116779: Patch.
https://bugs.webkit.org/attachment.cgi?id=116779&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=116779&action=review


Thank you very much, the patch now appears clear enough for me to be able to
review.

I have a number of comments that I expect would make you change the patch, so
marking r-. Please feel free to mark for review again if you disagree with
suggestions and implications from the questions.

> Source/WebCore/loader/cache/CachedImage.cpp:61
> +    , m_autoLoadPreventedBySettings(false)

I still think that "was" should be somewhere there for proper English grammar.
"If auto load _was_ prevented by settings".

> Source/WebCore/loader/cache/CachedImage.cpp:121
> +    // If this image load is blocked, pretend we are loading now
> +    // so that we don't fire an onload event from
CachedResource::didAddClient.

This is extremely subtle. Could CachedResource::didAddClient() call back into
CachedImage instead, asking if it's appropriate to fire the event?
Specifically, can it simply check if there is image data available, and skip
firing the event if not?

That way, you would not need m_autoLoadPreventedBySettings at all.

> Source/WebCore/loader/cache/CachedImage.h:97
> +    void setAutoLoadPreventedBySettings(bool b) {
m_autoLoadPreventedBySettings = b; }

Please use a real variable name instead of "b". Something like "prevented"
would work.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:486
> +    bool shouldNotLoadNow = type == CachedResource::ImageResource &&
!autoLoadImages() && !inCache;

Safari preference is "Display images when the page opens".  Why does it matter
whether the resource is in cache or not?

It's a pre-existing mismatch between wording in the UI and implementation, so
you shouldn't be concerned about it. I'm just making this comment for
posterity.

However, I'm surprised that you had to add the !inCache check. Why wasn't is
needed before?