Bug 162060 – Refactor CachedResourceClient::notifyFinished

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

CachedResourceClient::notifyFinished takes a CachedResource*.
It should at least take a CachedResource& and maybe no CachedResource at all.

[youenn fablet]

Created attachment 289060 [details]
Patch

[youenn fablet]

Comment on attachment 289060 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        On notifyFinished call, a RenderImage would only call contentChanged if the passed resource is the same as expected.

This change may actually affect some tests, such as css3/shapes/shape-outside/shape-image/shape-image-000.html.
This needs further investigation.

[youenn fablet]

Created attachment 289940 [details]
Patch

[youenn fablet]

Created attachment 289943 [details]
Patch

[Alex Christensen]

Comment on attachment 289943 [details]
Patch

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

Some of these could probably even be const CachedResource&

> Source/WebCore/loader/ContentFilter.cpp:173
> -    if (resource->errorOccurred())
> +    if (m_mainResource->errorOccurred())

I think a better change would be to use resource. instead of m_mainResource-> in this function.

> Source/WebCore/loader/DocumentLoader.cpp:-941
> -    ASSERT_UNUSED(resource, resource == m_mainResource);

I think having these assertions is worth passing an unused reference around.

> Source/WebCore/loader/ImageLoader.cpp:285
> -    if (resource->resourceError().isAccessControl()) {
> +    if (m_image->resourceError().isAccessControl()) {

resource. instead of m_image->

[youenn fablet]

> I think having these assertions is worth passing an unused reference around.

I was contemplating removing the parameters for some if not all these functions actually. notifyFinished is the only one that is using the resource parameter.
And the use is related to optimization AIUI.

I am not sure how much we have caught/may catch errors using those asserts.
If this is important maybe we should have CachedResource make a security-check call before calling these functions.
Something like ASSERT(m_client->isClientOf(*this)); in CachedResource methods.

[youenn fablet]

Ping review?

(In reply to comment #6)
> > I think having these assertions is worth passing an unused reference around.
> 
> I am not sure how much we have caught/may catch errors using those asserts.
> If this is important maybe we should have CachedResource make a
> security-check call before calling these functions.
> Something like ASSERT(m_client->isClientOf(*this)); in CachedResource
> methods.

I can work on that approach as a follow-up patch.

[Darin Adler]

Comment on attachment 289943 [details]
Patch

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

>> Source/WebCore/loader/ContentFilter.cpp:173
>> +    if (m_mainResource->errorOccurred())
> 
> I think a better change would be to use resource. instead of m_mainResource-> in this function.

Given that the comment on this patch says that might want to refactor to not pass resource, I think this makes sense and is consistent with that plan.

>> Source/WebCore/loader/DocumentLoader.cpp:-941
>> -    ASSERT_UNUSED(resource, resource == m_mainResource);
> 
> I think having these assertions is worth passing an unused reference around.

I am not sure I agree. Has this assertion ever helped us?

>> Source/WebCore/loader/ImageLoader.cpp:285
>> +    if (m_image->resourceError().isAccessControl()) {
> 
> resource. instead of m_image->

Same basic issue again. Did the two of you ever talk this through?

[youenn fablet]

Created attachment 290901 [details]
Patch for landing

[youenn fablet]

(In reply to comment #9)
> Created attachment 290901 [details]
> Patch for landing

I'll land the patch as is.
Let's further discuss the next steps on this area.

[WebKit Commit Bot]

Comment on attachment 290901 [details]
Patch for landing

Clearing flags on attachment: 290901

Committed r206903: <http://trac.webkit.org/changeset/206903>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.