Summary: | Use std::unique_ptr instead of PassOwnPtr|OwnPtr for ResourceResponse | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joonghun Park <jh718.park> | ||||||||
Component: | WebCore Misc. | Assignee: | Joonghun Park <jh718.park> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, darin, gyuyoung.kim, rniwa | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 128007 | ||||||||||
Attachments: |
|
Description
Joonghun Park
2015-03-25 12:33:37 PDT
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. |