Bug 28094 - Cleanup DOM Storage namespace shutdown code + usage of security origin.
Summary: Cleanup DOM Storage namespace shutdown code + usage of security origin.
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-08-07 23:11 PDT by Jeremy Orlow
Modified: 2009-08-10 22:28 PDT (History)
2 users (show)

See Also:


Attachments
Patch v1 (6.45 KB, patch)
2009-08-07 23:13 PDT, Jeremy Orlow
fishd: 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-08-07 23:11:59 PDT
If a storage namespace is not explicitly closed, be sure to do so on 
destruction of the object.  In addition, the close call should wait
on the background thread finishing its syncing.  (Not doing so is actually
a regression from the original LocalStorage code.)

There's no point to passing in the SecurityOrigin when copying a storage
area since what was passed in is exactly what is stored within each
storage area.  In addition, the non-copy constructor should take in a
PassRefPtr rather than a pointer since that pointer was only passed into
the constuctor for RefPtr's anyway.
Comment 1 Jeremy Orlow 2009-08-07 23:13:57 PDT
Created attachment 34362 [details]
Patch v1
Comment 2 Michael Nordman 2009-08-09 21:21:10 PDT
A small drive by comment...

97     for (StorageAreaMap::iterator i = m_storageAreaMap.begin(); i != end; ++i) {
98         RefPtr<StorageAreaImpl> areaCopy = i->second->copy();
99         newNamespace->m_storageAreaMap.set(i->first, areaCopy.release());
100    }

I think you could remove a couple of lines with...

for (StorageAreaMap::iterator i = m_storageAreaMap.begin(); i != end; ++i)
    newNamespace->m_storageAreaMap.set(i->first, i->second->copy());
Comment 3 Jeremy Orlow 2009-08-09 22:25:49 PDT
(In reply to comment #2)
> A small drive by comment...
> 
> 97     for (StorageAreaMap::iterator i = m_storageAreaMap.begin(); i != end;
> ++i) {
> 98         RefPtr<StorageAreaImpl> areaCopy = i->second->copy();
> 99         newNamespace->m_storageAreaMap.set(i->first, areaCopy.release());
> 100    }
> 
> I think you could remove a couple of lines with...
> 
> for (StorageAreaMap::iterator i = m_storageAreaMap.begin(); i != end; ++i)
>     newNamespace->m_storageAreaMap.set(i->first, i->second->copy());

Good point.  I'll change that.
Comment 4 Darin Fisher (:fishd, Google) 2009-08-10 00:19:12 PDT
Comment on attachment 34362 [details]
Patch v1

r=me w/ michael's suggested cleanup
Comment 5 Jeremy Orlow 2009-08-10 22:28:35 PDT
Was landed