WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(10.28 KB, patch)
2015-03-25 18:38 PDT
,
Joonghun Park
no flags
Details
Formatted Diff
Diff
Patch
(10.31 KB, patch)
2015-04-13 01:39 PDT
,
Joonghun Park
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Joonghun Park
Comment 1
2015-03-25 12:36:17 PDT
Created
attachment 249423
[details]
Patch
Joonghun Park
Comment 2
2015-03-25 18:38:15 PDT
Created
attachment 249460
[details]
Patch
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
Created
attachment 250630
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug