Bug 143056

Created attachment 249423 [details] Patch

Created attachment 249460 [details] Patch

FYI, https://bugs.webkit.org/show_bug.cgi?id=142995 for introducing adoptUniquePtr and leakUniquePtr.

Comment on attachment 249460 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249460&action=review > Source/WebCore/loader/WorkerThreadableLoader.cpp:169 > delete responseData; I wonder if we should delete responseData. Because response.copyData() releases own pointer in 163 line. Then *responseData* looks manage the memory. > Source/WebCore/platform/network/cf/ResourceResponse.h:105 > + void doPlatformAdopt(std::unique_ptr<CrossThreadResourceResponseData>) { } One question. Can't we move doPlatformCopyData() and doPlatformAdopt() declarations to ResourceResponseBase.h ?

(In reply to comment #4) > Comment on attachment 249460 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=249460&action=review > > > Source/WebCore/loader/WorkerThreadableLoader.cpp:169 > > delete responseData; > > I wonder if we should delete responseData. Because response.copyData() > releases own pointer in 163 line. Then *responseData* looks manage the > memory. > As you said, after we call response.copyData().leakPtr(), it releases the raw pointer of CrossThreadResourceResponseData. And it is passed into lambda, and in lambda body, it is used as parameter of unique_ptr ctor. As you can see in StdLibExtras.h:335, make_unique() is only a wrapper of unique_ptr ctor. In this case, we already have CrossThreadResourceResponseData*, and we want to use it. So no need to use make_unique, that is why I use std::unique_ptr<CrossThreadResourceResponseData>(responseData) in 166 line. > > Source/WebCore/platform/network/cf/ResourceResponse.h:105 > > + void doPlatformAdopt(std::unique_ptr<CrossThreadResourceResponseData>) { } > > One question. Can't we move doPlatformCopyData() and doPlatformAdopt() > declarations to ResourceResponseBase.h ? Well, I guess moving this to the ResourceResponseBase.h is possible. But it seems that the name is intended for the platform specific use(e.g.cf, curl, soup specific). So I doubt that it would be needed though the current implementation state is the same among them.

Comment on attachment 249460 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249460&action=review > Source/WebCore/loader/WorkerThreadableLoader.cpp:93 > + CrossThreadResourceRequestData* requestData = request.copyData().release(); Why do you change auto with CrossThreadResourceRequestData* ? > Source/WebCore/loader/WorkerThreadableLoader.cpp:163 > + CrossThreadResourceResponseData* responseData = response.copyData().release(); *auto* instead of CrossThreadResourceResponseData* ?

(In reply to comment #6) > Comment on attachment 249460 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=249460&action=review > > > Source/WebCore/loader/WorkerThreadableLoader.cpp:93 > > + CrossThreadResourceRequestData* requestData = request.copyData().release(); > > Why do you change auto with CrossThreadResourceRequestData* ? > I thought that it's not a type of std::unique_ptr but raw pointer, so I changed it back to raw pointer type. Do you think it would be better to use auto* here? > > Source/WebCore/loader/WorkerThreadableLoader.cpp:163 > > + CrossThreadResourceResponseData* responseData = response.copyData().release(); > > *auto* instead of CrossThreadResourceResponseData* ? Same question here as I wrote above. How do you think?

Comment on attachment 249460 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249460&action=review >>> Source/WebCore/loader/WorkerThreadableLoader.cpp:93 >>> + CrossThreadResourceRequestData* requestData = request.copyData().release(); >> >> Why do you change auto with CrossThreadResourceRequestData* ? > > I thought that it's not a type of std::unique_ptr but raw pointer, so I changed it back to raw pointer type. Do you think it would be better to use auto* here? yes.

(In reply to comment #8) > Comment on attachment 249460 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=249460&action=review > > >>> Source/WebCore/loader/WorkerThreadableLoader.cpp:93 > >>> + CrossThreadResourceRequestData* requestData = request.copyData().release(); > >> > >> Why do you change auto with CrossThreadResourceRequestData* ? > > > > I thought that it's not a type of std::unique_ptr but raw pointer, so I changed it back to raw pointer type. Do you think it would be better to use auto* here? > > yes. Ok, then I will change the type to auto* because the return type is CrossThreadResourceRequestData*. And I will use auto* instead of CrossThreadResourceResponseData* in 163 line, too.

Created attachment 250630 [details] Patch

Comment on attachment 250630 [details] Patch Clearing flags on attachment: 250630 Committed r182707: <http://trac.webkit.org/changeset/182707>

All reviewed patches have been landed. Closing bug.