Bug 28094

Summary: Cleanup DOM Storage namespace shutdown code + usage of security origin.
Product: WebKit Reporter: Jeremy Orlow <jorlow>
Component: New BugsAssignee: Jeremy Orlow <jorlow>
Status: RESOLVED FIXED    
Severity: Normal CC: beidson, michaeln
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch v1 fishd: review+

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