Dom Storage (webkit side)

This is ugly....definitely needs some cleanup.

This is the webkit portion of http://codereview.chromium.org/147248

[michaeln, 2009-07-14 19:02:04]

http://codereview.chromium.org/155477/diff/1/7
File bindings/v8/V8Index.cpp (right):

http://codereview.chromium.org/155477/diff/1/7#newcode377
Line 377: #endif
ditto no changes in this file... should there be some additional includes in
here...

#if ENABLE(DOM_STORAGE)
??

http://codereview.chromium.org/155477/diff/1/5
File bindings/v8/V8Index.h (right):

http://codereview.chromium.org/155477/diff/1/5#newcode35
Line 35: #include "PlatformString.h"  // for WebCore::String
I don't see any changes in this file... does it have to be in the CL?

http://codereview.chromium.org/155477/diff/1/4
File page/DOMWindow.idl (right):

http://codereview.chromium.org/155477/diff/1/4#newcode164
Line 164: readonly attribute [V8Custom] Storage localStorage;
Unfortunate we need V8Custom here so the .idl is different for V8 vs JSC? Is
there a v8 binding techinique that would allow us to not embellish this
attributely differently?

Just checking.

http://codereview.chromium.org/155477/diff/1/2
File storage/StorageAreaImpl.cpp (right):

http://codereview.chromium.org/155477/diff/1/2#newcode235
Line 235: return;
I realize this is temporary only until you resolve how you're going to deal with
events.

If this test and early return is done at the top of the method body, the the
Vector<> ctor and dtor will not have to run (both smaller and faster code)... so
everything else being equal... would be better to put this test up front.

http://codereview.chromium.org/155477/diff/1/2#newcode239
Line 239: if (!page)
while your here... ditto for this test

http://codereview.chromium.org/155477/diff/1/3
File storage/StorageNamespaceImpl.cpp (right):

http://codereview.chromium.org/155477/diff/1/3#newcode41
Line 41: return localStorageNamespaceMap;
So... incognito vs regular local storage will be distinquished by different
'paths'... is that right?

http://codereview.chromium.org/155477/diff/1/3#newcode89
Line 89: StorageNamespaceImpl* newNamespace = new
StorageNamespaceImpl(m_storageType, m_path);
I think this method is not valid to call on 'local' storage instance. If the
clone where to be made here, the ASSERT on line 79 would fire upon destruction.

If that's correct... maybe

ASSERT(m_storageType == SessionStorage);
newNamespace = new StorageNamespaceImpl(SessionStorage, String());

[jorlow, 2009-07-14 19:12:50]

http://codereview.chromium.org/155477/diff/1/7
File bindings/v8/V8Index.cpp (right):

http://codereview.chromium.org/155477/diff/1/7#newcode377
Line 377: #endif
On 2009/07/14 19:02:05, michaeln wrote:
> ditto no changes in this file... should there be some additional includes in
> here...
> 
> #if ENABLE(DOM_STORAGE)
> ??
> 

it's in here.

another file that's now reverted

http://codereview.chromium.org/155477/diff/1/5
File bindings/v8/V8Index.h (right):

http://codereview.chromium.org/155477/diff/1/5#newcode35
Line 35: #include "PlatformString.h"  // for WebCore::String
On 2009/07/14 19:02:05, michaeln wrote:
> I don't see any changes in this file... does it have to be in the CL?

I think there were line ending issues...I'll revert.

http://codereview.chromium.org/155477/diff/1/4
File page/DOMWindow.idl (right):

http://codereview.chromium.org/155477/diff/1/4#newcode164
Line 164: readonly attribute [V8Custom] Storage localStorage;
On 2009/07/14 19:02:05, michaeln wrote:
> Unfortunate we need V8Custom here so the .idl is different for V8 vs JSC? Is
> there a v8 binding techinique that would allow us to not embellish this
> attributely differently?
> 
> Just checking.

If you look, we do this a lot.

The binding is pretty simple.  I don't think this is a big deal, but I think
it's necessary because DOMWindow::localStorage and sessionStorage can return
NULL, but it's improper for us to wrap them in a Storage object...it should
return undefined.

_maybe_ the correct behavior, instead of making this custom (at least once we
get rid of the --enable-*-storage flags) is to change the function that creates
the v8 object...I guess I could try it out and see if the world explodes.

http://codereview.chromium.org/155477/diff/1/2
File storage/StorageAreaImpl.cpp (right):

http://codereview.chromium.org/155477/diff/1/2#newcode235
Line 235: return;
On 2009/07/14 19:02:05, michaeln wrote:
> I realize this is temporary only until you resolve how you're going to deal
with
> events.
> 
> If this test and early return is done at the top of the method body, the the
> Vector<> ctor and dtor will not have to run (both smaller and faster code)...
so
> everything else being equal... would be better to put this test up front.

Sure.  Might as well.

http://codereview.chromium.org/155477/diff/1/2#newcode239
Line 239: if (!page)
On 2009/07/14 19:02:05, michaeln wrote:
> while your here... ditto for this test

k

http://codereview.chromium.org/155477/diff/1/3
File storage/StorageNamespaceImpl.cpp (right):

http://codereview.chromium.org/155477/diff/1/3#newcode41
Line 41: return localStorageNamespaceMap;
On 2009/07/14 19:02:05, michaeln wrote:
> So... incognito vs regular local storage will be distinquished by different
> 'paths'... is that right?

Yup.

http://codereview.chromium.org/155477/diff/1/3#newcode89
Line 89: StorageNamespaceImpl* newNamespace = new
StorageNamespaceImpl(m_storageType, m_path);
On 2009/07/14 19:02:05, michaeln wrote:
> I think this method is not valid to call on 'local' storage instance. If the
> clone where to be made here, the ASSERT on line 79 would fire upon
destruction.
> 
> If that's correct... maybe
> 
> ASSERT(m_storageType == SessionStorage);
> newNamespace = new StorageNamespaceImpl(SessionStorage, String());
> 

Makes sense.