WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch v1
(3.12 KB, patch)
2009-09-16 17:43 PDT
,
Jeremy Orlow
no flags
Details
Formatted Diff
Diff
Patch v1
(4.94 KB, patch)
2009-09-21 14:48 PDT
,
Jeremy Orlow
eric
: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
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
Committed
r48734
: <
http://trac.webkit.org/changeset/48734
>
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