RESOLVED FIXED 143056
Use std::unique_ptr instead of PassOwnPtr|OwnPtr for ResourceResponse
https://bugs.webkit.org/show_bug.cgi?id=143056
Summary Use std::unique_ptr instead of PassOwnPtr|OwnPtr for ResourceResponse
Joonghun Park
Reported 2015-03-25 12:33:37 PDT
Change from PassOwnPtr|OwnPtr to std::unique_ptr for ResourceResponse in All ports.
Attachments
Patch (10.30 KB, patch)
2015-03-25 12:36 PDT, Joonghun Park
no flags
Patch (10.28 KB, patch)
2015-03-25 18:38 PDT, Joonghun Park
no flags
Patch (10.31 KB, patch)
2015-04-13 01:39 PDT, Joonghun Park
no flags
Joonghun Park
Comment 1 2015-03-25 12:36:17 PDT
Joonghun Park
Comment 2 2015-03-25 18:38:15 PDT
Joonghun Park
Comment 3 2015-03-25 18:42:24 PDT
FYI, https://bugs.webkit.org/show_bug.cgi?id=142995 for introducing adoptUniquePtr and leakUniquePtr.
Gyuyoung Kim
Comment 4 2015-03-29 19:39:38 PDT
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 ?
Joonghun Park
Comment 5 2015-03-31 05:38:55 PDT
(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.
Gyuyoung Kim
Comment 6 2015-04-11 07:49:16 PDT
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* ?
Joonghun Park
Comment 7 2015-04-12 10:35:16 PDT
(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?
Gyuyoung Kim
Comment 8 2015-04-13 00:56:03 PDT
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.
Joonghun Park
Comment 9 2015-04-13 01:04:40 PDT
(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.
Joonghun Park
Comment 10 2015-04-13 01:39:52 PDT
WebKit Commit Bot
Comment 11 2015-04-13 04:19:04 PDT
Comment on attachment 250630 [details] Patch Clearing flags on attachment: 250630 Committed r182707: <http://trac.webkit.org/changeset/182707>
WebKit Commit Bot
Comment 12 2015-04-13 04:19:11 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.