Bug 153229 – Remove PassRefPtr from ResourceRequest and FormData

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

Remove PassRefPtr from ResourceRequest and FormData

[youenn fablet]

Created attachment 269264 [details]
Patch

[Alex Christensen]

Comment on attachment 269264 [details]
Patch

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

> Source/WebCore/platform/network/ResourceRequestBase.h:126
> +        void clearHTTPBody() { setHTTPBody(nullptr); }

This seems unnecessary.

[Darin Adler]

Comment on attachment 269264 [details]
Patch

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

> Source/WebCore/loader/FormSubmission.cpp:261
> +        frameRequest.resourceRequest().setHTTPBody(m_formData.copyRef());

Seems a shame we can’t move here. Worth investigating if we can; do we ever have to use m_formData again?

> Source/WebCore/platform/network/FormData.cpp:73
> +RefPtr<FormData> FormData::create()

Should return Ref, not RefPtr.

> Source/WebCore/platform/network/FormData.cpp:78
> +RefPtr<FormData> FormData::create(const void* data, size_t size)

Should return Ref, not RefPtr.

> Source/WebCore/platform/network/FormData.cpp:92
> +RefPtr<FormData> FormData::create(const Vector<char>& vector)

Should return Ref, not RefPtr.

> Source/WebCore/platform/network/FormData.cpp:99
> +RefPtr<FormData> FormData::create(const FormDataList& list, const TextEncoding& encoding, EncodingType encodingType)

Should return Ref, not RefPtr.

> Source/WebCore/platform/network/FormData.cpp:106
> +RefPtr<FormData> FormData::createMultiPart(const FormDataList& list, const TextEncoding& encoding, Document* document)

Should return Ref, not RefPtr.

> Source/WebCore/platform/network/FormData.cpp:113
> +RefPtr<FormData> FormData::copy() const

Should return Ref, not RefPtr.

> Source/WebCore/platform/network/FormData.cpp:118
> +RefPtr<FormData> FormData::deepCopy() const

Should return Ref, not RefPtr.

> Source/WebCore/platform/network/FormData.cpp:305
> +RefPtr<FormData> FormData::resolveBlobReferences()

Should return Ref, not RefPtr.

> Source/WebCore/platform/network/FormData.h:294
>      RefPtr<FormData> data = FormData::create();

This function uses release() below, and should not.

>> Source/WebCore/platform/network/ResourceRequestBase.h:126
>> +        WEBCORE_EXPORT void setHTTPBody(RefPtr<FormData>&&);
>> +        WEBCORE_EXPORT void setHTTPBody(FormData* httpBody) { setHTTPBody(RefPtr<FormData>(httpBody)); }
>> +        void clearHTTPBody() { setHTTPBody(nullptr); }
> 
> This seems unnecessary.

The overloading of setHTTPBody seems pretty messy. Can we avoid it?

And, as Alex says, I think setHTTPBody(nullptr) is clear enough by itself; don’t think we need to add a named clearHTTPBody.

> Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:353
>      RefPtr<FormData> formData = prpFormData;

This seems peculiar. The "prp" here stands for PassRefPtr, and so it should not have that name any more. I don’t think we need a RefPtr local variable at all now that this is not using PassRefPtr.

> Source/WebCore/platform/network/mac/FormDataStreamMac.mm:43
> +void setHTTPBody(NSMutableURLRequest *request, FormData* prpFormData)

Again, in "prpFormData" the "prp" is PassRefPtr and so we should get rid of it.

[youenn fablet]

Created attachment 269446 [details]
Removing clearHTTPBody

[youenn fablet]

Created attachment 269452 [details]
Updated according review

[youenn fablet]

Thanks for the reviews.

(In reply to comment #2)
> Comment on attachment 269264 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269264&action=review
> 
> > Source/WebCore/platform/network/ResourceRequestBase.h:126
> > +        void clearHTTPBody() { setHTTPBody(nullptr); }
> 
> This seems unnecessary.

I removed it.

> > Source/WebCore/loader/FormSubmission.cpp:261
> > +        frameRequest.resourceRequest().setHTTPBody(m_formData.copyRef());
> 
> Seems a shame we can’t move here. Worth investigating if we can; do we ever
> have to use m_formData again?

I think we can remove the copyRef, as the code is now.
But this could be broken very easily, so I kept it.
Some refactoring would probably help to remove the copyRef.
Maybe a FIXME in the code would be good to keep track of this?

[Chris Dumez]

Comment on attachment 269452 [details]
Updated according review

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

r=me

> Source/WebCore/platform/network/FormData.h:308
> +    return WTFMove(data);

I don't think we need the WTFMove() here.

> Source/WebCore/platform/network/ResourceRequestBase.cpp:79
> +    request->setHTTPBody(WTFMove(data->httpBody));

It seems risky to alter data before passing it to doPlatformAdopt(). Personally, I would copyRef() here as it seems more robust.

> Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:-394
> -    FormCreationContext formContext = { formData.release(), length };

Can't we keep this on one line to avoid calling the default constructor unnecessarily? There has to be a way.

[Michael Catanzaro]

(In reply to comment #7)
> > Source/WebCore/platform/network/FormData.h:308
> > +    return WTFMove(data);
> 
> I don't think we need the WTFMove() here.

Calling WTFMove with a return value is going to trigger clang's -Wpessimizing-move warning, so please remove this.

[youenn fablet]

Created attachment 269558 [details]
Patch for landing

[youenn fablet]

Created attachment 269560 [details]
Patch for landing

[youenn fablet]

> r=me

Thanks

> > Source/WebCore/platform/network/FormData.h:308
> > +    return WTFMove(data);
> 
> I don't think we need the WTFMove() here.

I removed it.
Is there a good way to return a RefPtr<> from a Ref<>?

> > Source/WebCore/platform/network/ResourceRequestBase.cpp:79
> > +    request->setHTTPBody(WTFMove(data->httpBody));
> 
> It seems risky to alter data before passing it to doPlatformAdopt().
> Personally, I would copyRef() here as it seems more robust.

OK

> > Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:-394
> > -    FormCreationContext formContext = { formData.release(), length };
> 
> Can't we keep this on one line to avoid calling the default constructor
> unnecessarily? There has to be a way.

I changed it back, code is better this way.
I guess formData will get its counter ++/--.

[WebKit Commit Bot]

Comment on attachment 269560 [details]
Patch for landing

Rejecting attachment 269560 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
/x86_64/FormState.dia -c /Volumes/Data/EWS/WebKit/Source/WebCore/loader/FormState.cpp -o /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/FormState.o

** BUILD FAILED **


The following build commands failed:
	CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/FormDataStreamCFNet.o platform/network/cf/FormDataStreamCFNet.cpp normal x86_64 c++ com.apple.compilers.llvm.clang.1_0.compiler
(1 failure)

Full output: http://webkit-queues.webkit.org/results/724895

[youenn fablet]

Created attachment 269571 [details]
Patch for landing with mac build fix

[WebKit Commit Bot]

Comment on attachment 269571 [details]
Patch for landing with mac build fix

Clearing flags on attachment: 269571

Committed r195450: <http://trac.webkit.org/changeset/195450>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.