Remove PassRefPtr from ResourceRequest and FormData
Created attachment 269264 [details] Patch
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.
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.
Created attachment 269446 [details] Removing clearHTTPBody
Created attachment 269452 [details] Updated according review
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?
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.
(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.
Created attachment 269558 [details] Patch for landing
Created attachment 269560 [details] Patch for landing
> 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 ++/--.
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
Created attachment 269571 [details] Patch for landing with mac build fix
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>
All reviewed patches have been landed. Closing bug.