Bug 29290 - StorageNamespace::storageArea() should take in a PassRefPtr<StorageOrigin>
Summary: StorageNamespace::storageArea() should take in a PassRefPtr<StorageOrigin>
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Jeremy Orlow
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-09-15 17:55 PDT by Jeremy Orlow
Modified: 2009-09-24 14:36 PDT (History)
2 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Orlow 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.
Comment 1 Jeremy Orlow 2009-09-15 17:58:52 PDT
Created attachment 39626 [details]
Patch v1
Comment 2 Eric Seidel (no email) 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.
Comment 3 Jeremy Orlow 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.
Comment 4 Jeremy Orlow 2009-09-16 17:43:05 PDT
Created attachment 39672 [details]
Patch v1
Comment 5 Jeremy Orlow 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.
Comment 6 Eric Seidel (no email) 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?)
Comment 7 Jeremy Orlow 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.
Comment 8 Eric Seidel (no email) 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.
Comment 9 Eric Seidel (no email) 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.
Comment 10 Jeremy Orlow 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.  :-)
Comment 11 Jeremy Orlow 2009-09-21 14:41:57 PDT
Oh...ha!  I made it in the "wrong" svn client.  So never mind.  :-)
Comment 12 Jeremy Orlow 2009-09-21 14:48:21 PDT
Created attachment 39881 [details]
Patch v1
Comment 13 Jeremy Orlow 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.  :-(
Comment 14 Eric Seidel (no email) 2009-09-21 15:00:09 PDT
Comment on attachment 39881 [details]
Patch v1

LGTM.
Comment 15 Jeremy Orlow 2009-09-24 14:36:57 PDT
Committed r48734: <http://trac.webkit.org/changeset/48734>