[Webkit-unassigned] [Bug 140722] Simplify CachedImage clients to avoid overriding both or either only imageChanged or notifyFinished

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=140722

--- Comment #9 from Tim Horton <thorton at apple.com> ---
Comment on attachment 245530
  --> https://bugs.webkit.org/attachment.cgi?id=245530
CachedImage: ensure clients overrides imageChanged instead of notifyFinished

View in context: https://bugs.webkit.org/attachment.cgi?id=245530&action=review

> Source/WebCore/ChangeLog:9
> +        notifyFinished was called when the image was enterely loaded.

sp.: "entirely"

> Source/WebCore/ChangeLog:13
> +        some both (ex: RenderImage).

comma at the end of this line and into the 'which'

> Source/WebCore/ChangeLog:16
> +        For example when the image finished to load, both imageChanged

"finished to load" should probably read "finished loading"

> Source/WebCore/ChangeLog:18
> +        first one isLoaded() return false.

s/return/returned/

> Source/WebCore/ChangeLog:19
> +        It could result in functions called twice in a row,

"being called"

> Source/WebCore/ChangeLog:23
> +        CachedImageClient::notifyFinished final. In order to

In order to what?

> Source/WebCore/ChangeLog:25
> +        Clients can now differenciates intermediates and end

sp.: differentiate
and intermediate should not be plural

> Source/WebCore/ChangeLog:28
> +        Because I moved setLoading(false) from CachedResource::finishLoading
> +        call to an explicit call in CachedImage::finishLoading.

Not sure what this means or if it needs to be here.

> Source/WebCore/ChangeLog:32
> +        * html/HTMLImageLoader.cpp:

It's possible you should split your changelog up down here and put relevant bits next to the names of the functions that those changes affected.

> Source/WebCore/html/HTMLImageLoader.h:38
> +    virtual void imageChanged(CachedImage*, const IntRect* = 0) override;

0 should be nullptr here.

> Source/WebCore/loader/ImageLoader.h:76
> +    virtual void imageChanged(CachedImage*, const IntRect* = 0) override;

0 should be nullptr here.

> Source/WebCore/rendering/RenderImage.cpp:-368
> -void RenderImage::notifyFinished(CachedResource* newImage)

Because of laziness, I'm not completely convinced that RenderImage::imageChanged's call to RenderImage::imageDimensionsChanged is going to always end up doing the invalidateBackgroundObscurationStatus and contentChanged that were here in all the same cases as it previously would have. Can you please confirm that it will? Most concerningly, here we unconditionally call contentChanged, but in imageDimensionsChanged we only call it conditionally (with a whole bunch of conditions).

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150128/45d94b15/attachment-0002.html>