Bug 29290 – StorageNamespace::storageArea() should take in a PassRefPtr<StorageOrigin>

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jeremy Orlow]

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.

[Jeremy Orlow]

Created attachment 39626 [details]
Patch v1

[Eric Seidel (no email)]

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]

(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]

Created attachment 39672 [details]
Patch v1

[Jeremy Orlow]

Eric, should be easy for you since this is just a new rev of what I posted before.

[Eric Seidel (no email)]

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]

(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 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)]

(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]

Sorry Eric, but I had already started making your change to StorageAreaImpl by the time you r+'ed it.  :-)

[Jeremy Orlow]

Oh...ha!  I made it in the "wrong" svn client.  So never mind.  :-)

[Jeremy Orlow]

Created attachment 39881 [details]
Patch v1

[Jeremy Orlow]

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 on attachment 39881 [details]
Patch v1

LGTM.

[Jeremy Orlow]

Committed r48734: <http://trac.webkit.org/changeset/48734>