RESOLVED FIXED 29290
StorageNamespace::storageArea() should take in a PassRefPtr<StorageOrigin>
https://bugs.webkit.org/show_bug.cgi?id=29290
Summary StorageNamespace::storageArea() should take in a PassRefPtr<StorageOrigin>
Jeremy Orlow
Reported 2009-09-15 17:55:06 PDT
StorageAreaImpl takes in a PassRefPtr<SecurityOrigin> so we might as well do it one level higher in the call chain. I REALLY wanted to make securityOrigin() return a PassRefPtr<> but it required pretty much changing the world. (Soooo much assumed it was a pointer.) Enough so that I actually suspect it'd have noticeable perf consequences.
Attachments
Patch v1 (3.27 KB, patch)
2009-09-15 17:58 PDT, Jeremy Orlow
no flags
Patch v1 (3.12 KB, patch)
2009-09-16 17:43 PDT, Jeremy Orlow
no flags
Patch v1 (4.94 KB, patch)
2009-09-21 14:48 PDT, Jeremy Orlow
eric: review+
Jeremy Orlow
Comment 1 2009-09-15 17:58:52 PDT
Created attachment 39626 [details] Patch v1
Eric Seidel (no email)
Comment 2 2009-09-15 18:01:45 PDT
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.
Jeremy Orlow
Comment 3 2009-09-15 18:03:20 PDT
(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.
Jeremy Orlow
Comment 4 2009-09-16 17:43:05 PDT
Created attachment 39672 [details] Patch v1
Jeremy Orlow
Comment 5 2009-09-21 13:09:27 PDT
Eric, should be easy for you since this is just a new rev of what I posted before.
Eric Seidel (no email)
Comment 6 2009-09-21 13:24:45 PDT
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?)
Jeremy Orlow
Comment 7 2009-09-21 13:38:38 PDT
(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.
Eric Seidel (no email)
Comment 8 2009-09-21 13:44:43 PDT
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.
Eric Seidel (no email)
Comment 9 2009-09-21 13:45:41 PDT
(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.
Jeremy Orlow
Comment 10 2009-09-21 14:41:02 PDT
Sorry Eric, but I had already started making your change to StorageAreaImpl by the time you r+'ed it. :-)
Jeremy Orlow
Comment 11 2009-09-21 14:41:57 PDT
Oh...ha! I made it in the "wrong" svn client. So never mind. :-)
Jeremy Orlow
Comment 12 2009-09-21 14:48:21 PDT
Created attachment 39881 [details] Patch v1
Jeremy Orlow
Comment 13 2009-09-21 14:49:48 PDT
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. :-(
Eric Seidel (no email)
Comment 14 2009-09-21 15:00:09 PDT
Comment on attachment 39881 [details] Patch v1 LGTM.
Jeremy Orlow
Comment 15 2009-09-24 14:36:57 PDT
Note You need to log in before you can comment on or make changes to this bug.