Summary: | StorageNamespace::storageArea() should take in a PassRefPtr<StorageOrigin> | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jeremy Orlow <jorlow> | ||||||||
Component: | New Bugs | Assignee: | Jeremy Orlow <jorlow> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | eric, fishd | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Jeremy Orlow
2009-09-15 17:55:06 PDT
Created attachment 39626 [details]
Patch v1
Comment on attachment 39626 [details]
Patch v1
Is this the only StorageNamespace implementation which needs changing? I'm surprised by the separate headers, but I don't know much about how storage is written.
(In reply to comment #2) > (From update of attachment 39626 [details]) > Is this the only StorageNamespace implementation which needs changing? I'm > surprised by the separate headers, but I don't know much about how storage is > written. There is only one implementation in WebKit. Chromium uses the interface to proxy data from process to process. Created attachment 39672 [details]
Patch v1
Eric, should be easy for you since this is just a new rev of what I posted before. Comment on attachment 39672 [details]
Patch v1
This code has some strange memory management.
Release is not needed here:
113 m_storageAreaMap.set(origin.release(), storageArea);
PassRefPtr's automatically release.
111112 storageArea = adoptRef(new StorageAreaImpl(m_storageType, origin, m_syncManager));
Really should be using a create() function instead.
I guess r- for the extra unneeded .release(). (Unless I'm misunderstanding something?)
(In reply to comment #6) > (From update of attachment 39672 [details]) > This code has some strange memory management. > > Release is not needed here: > 113 m_storageAreaMap.set(origin.release(), storageArea); > > PassRefPtr's automatically release. > > 111112 storageArea = adoptRef(new StorageAreaImpl(m_storageType, origin, > m_syncManager)); > Really should be using a create() function instead. > > I guess r- for the extra unneeded .release(). (Unless I'm misunderstanding > something?) .release() is never needed, but it's a performance optimization that's often used when you know that the variable is never going to be needed again. I see plenty examples of it being done throughout WebCore--both in the return statement and outside of it. That said, I'm fine with removing it. I don't think it's possible it'd have a measurable affect on perf. Comment on attachment 39672 [details]
Patch v1
Sorry, I misread the patch.
113 m_storageAreaMap.set(origin.release(), storageArea);
I thought that was prpOrigin, which would not need a release since it's already a PassRefPtr.
(In reply to comment #7) > .release() is never needed, but it's a performance optimization that's often > used when you know that the variable is never going to be needed again. I see > plenty examples of it being done throughout WebCore--both in the return > statement and outside of it. Yes. I simply misread the patch. I thought the variable in question was already a PassRefPtr. Sorry Eric, but I had already started making your change to StorageAreaImpl by the time you r+'ed it. :-) Oh...ha! I made it in the "wrong" svn client. So never mind. :-) Created attachment 39881 [details]
Patch v1
Oh....actually, I'm just plain an idiot. 1) I actually did make the change in this svn client. 2) I uploaded another diff which killed an r+ it shouldn't have. So please do take one last look. :-( Comment on attachment 39881 [details]
Patch v1
LGTM.
Committed r48734: <http://trac.webkit.org/changeset/48734> |