Bug 162060

Summary: Refactor CachedResourceClient::notifyFinished
Product: WebKit Reporter: youenn fablet <youennf>
Component: WebCore Misc.Assignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch for landing none

Description youenn fablet 2016-09-16 07:10:56 PDT
CachedResourceClient::notifyFinished takes a CachedResource*.
It should at least take a CachedResource& and maybe no CachedResource at all.
Comment 1 youenn fablet 2016-09-16 07:15:44 PDT
Created attachment 289060 [details]
Patch
Comment 2 youenn fablet 2016-09-16 07:29:09 PDT
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.
Comment 3 youenn fablet 2016-09-27 07:00:34 PDT
Created attachment 289940 [details]
Patch
Comment 4 youenn fablet 2016-09-27 08:16:04 PDT
Created attachment 289943 [details]
Patch
Comment 5 Alex Christensen 2016-09-28 08:43:43 PDT
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->
Comment 6 youenn fablet 2016-09-28 09:05:40 PDT
> 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.
Comment 7 youenn fablet 2016-10-06 09:04:50 PDT
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.
Comment 8 Darin Adler 2016-10-06 20:13:35 PDT
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?
Comment 9 youenn fablet 2016-10-06 23:30:31 PDT
Created attachment 290901 [details]
Patch for landing
Comment 10 youenn fablet 2016-10-06 23:31:54 PDT
(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.
Comment 11 WebKit Commit Bot 2016-10-07 00:04:34 PDT
Comment on attachment 290901 [details]
Patch for landing

Clearing flags on attachment: 290901

Committed r206903: <http://trac.webkit.org/changeset/206903>
Comment 12 WebKit Commit Bot 2016-10-07 00:04:38 PDT
All reviewed patches have been landed.  Closing bug.