Add the WebKit API for DOM Storage. This is used to re-enter WebKit. A late...

Add the WebKit API for DOM Storage.  This is used to re-enter WebKit.  A later patch will implement the WebKitClient version of it.

BUG=4360
TEST=none



Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=20918

[jorlow, 2009-07-15 23:11:05]

Ready for review.

Lots of patches stacked behind this and there's not much here, so please try to
take a look soon?  :-)

[darin (slow to review), 2009-07-15 23:24:05]

http://codereview.chromium.org/149716/diff/1/7
File webkit/api/public/WebStorageArea.h (right):

http://codereview.chromium.org/149716/diff/1/7#newcode34
Line 34: #if ENABLE(DOM_STORAGE)
this #ifdef is not visible outside of webcore, so we can't use it here.  i
wouldn't bother making this interface conditional.  just make the implementation
conditional.

http://codereview.chromium.org/149716/diff/1/7#newcode43
Line 43: virtual ~WebStorageArea() { };
nit: kill trailing ";"

http://codereview.chromium.org/149716/diff/1/7#newcode44
Line 44: virtual void lock(bool& invalidateCache, size_t& bytesLeftInQuota) = 0;
nit: it would be good to document these interfaces since they are part of the
public API.  what do these out params mean, etc.?

http://codereview.chromium.org/149716/diff/1/8
File webkit/api/public/WebStorageNamespace.h (right):

http://codereview.chromium.org/149716/diff/1/8#newcode34
Line 34: #if ENABLE(DOM_STORAGE)
ditto

http://codereview.chromium.org/149716/diff/1/8#newcode44
Line 44: WEBKIT_API static WebStorageNamespace* localStorageNamespace(const
WebString& path);
nit: path -> filePath

(a bit more clear what kind of path this is)

http://codereview.chromium.org/149716/diff/1/8#newcode44
Line 44: WEBKIT_API static WebStorageNamespace* localStorageNamespace(const
WebString& path);
i think it would be helpful to document the ownership semantics here.  it seems
like i can or perhaps should delete the returned object when i am done with it,
but is that what you really intended?  when should i call close versus delete?

http://codereview.chromium.org/149716/diff/1/8#newcode47
Line 47: virtual ~WebStorageNamespace() { };
nit: kill trailing ";"

http://codereview.chromium.org/149716/diff/1/4
File webkit/api/src/WebStorageAreaImpl.cpp (right):

http://codereview.chromium.org/149716/diff/1/4#newcode41
Line 41: WebStorageAreaImpl::WebStorageAreaImpl(PassRefPtr<WebCore::StorageArea>
storageArea)
it's up to you, but i've been preferring to just stick a using namespace WebCore
at the top of .cpp files in api/src.  over time this file might accumulate more
WebCore usage, and so it'll probably be nice to have the using directive.

http://codereview.chromium.org/149716/diff/1/4#newcode74
Line 74: keyException = true;
nit: 4 space indent

http://codereview.chromium.org/149716/diff/1/4#newcode92
Line 92: ASSERT(exceptionCode == WebCore::QUOTA_EXCEEDED_ERR);
nit: 4 space indent

http://codereview.chromium.org/149716/diff/1/6
File webkit/api/src/WebStorageAreaImpl.h (right):

http://codereview.chromium.org/149716/diff/1/6#newcode39
Line 39: namespace WebKit {
nit: insert new line after namespace line

http://codereview.chromium.org/149716/diff/1/5
File webkit/api/src/WebStorageNamespaceImpl.cpp (right):

http://codereview.chromium.org/149716/diff/1/5#newcode45
Line 45: return new
WebStorageNamespaceImpl(WebCore::StorageNamespaceImpl::localStorageNamespace(path));
ah, so you do expect the receiver to delete these.  then i think these methods
should be named "create*StorageNamespace".  the "create" prefix is the clue to
the caller that they are receiving ownership.

http://codereview.chromium.org/149716/diff/1/5#newcode65
Line 65: return new
WebStorageAreaImpl(m_storageNamespace->storageArea(origin.get()));
ditto... if the caller receives ownership, then name the method "createFoo"

http://codereview.chromium.org/149716/diff/1/3
File webkit/api/src/WebStorageNamespaceImpl.h (right):

http://codereview.chromium.org/149716/diff/1/3#newcode39
Line 39: namespace WebKit {
nit: add extra new line below this line

http://codereview.chromium.org/149716/diff/1/3#newcode42
Line 42: WebStorageNamespaceImpl(PassRefPtr<WebCore::StorageNamespace>
sessionStorageNamespace);
nit: did you really mean to name this parameter "sessionStorageNamespace" ?

[jorlow, 2009-07-16 00:01:45]

Done.  new patch uploaded.

http://codereview.chromium.org/149716/diff/1/7
File webkit/api/public/WebStorageArea.h (right):

http://codereview.chromium.org/149716/diff/1/7#newcode34
Line 34: #if ENABLE(DOM_STORAGE)
On 2009/07/15 23:24:05, darin wrote:
> this #ifdef is not visible outside of webcore, so we can't use it here.  i
> wouldn't bother making this interface conditional.  just make the
implementation
> conditional.

Done.

http://codereview.chromium.org/149716/diff/1/7#newcode43
Line 43: virtual ~WebStorageArea() { };
On 2009/07/15 23:24:05, darin wrote:
> nit: kill trailing ";"

Done.

http://codereview.chromium.org/149716/diff/1/7#newcode44
Line 44: virtual void lock(bool& invalidateCache, size_t& bytesLeftInQuota) = 0;
On 2009/07/15 23:24:05, darin wrote:
> nit: it would be good to document these interfaces since they are part of the
> public API.  what do these out params mean, etc.?

Done.

http://codereview.chromium.org/149716/diff/1/8
File webkit/api/public/WebStorageNamespace.h (right):

http://codereview.chromium.org/149716/diff/1/8#newcode34
Line 34: #if ENABLE(DOM_STORAGE)
On 2009/07/15 23:24:05, darin wrote:
> ditto

Done.

http://codereview.chromium.org/149716/diff/1/8#newcode44
Line 44: WEBKIT_API static WebStorageNamespace* localStorageNamespace(const
WebString& path);
On 2009/07/15 23:24:05, darin wrote:
> i think it would be helpful to document the ownership semantics here.  it
seems
> like i can or perhaps should delete the returned object when i am done with
it,
> but is that what you really intended?  when should i call close versus delete?

Done.

http://codereview.chromium.org/149716/diff/1/8#newcode47
Line 47: virtual ~WebStorageNamespace() { };
On 2009/07/15 23:24:05, darin wrote:
> nit: kill trailing ";"

Done.

http://codereview.chromium.org/149716/diff/1/4
File webkit/api/src/WebStorageAreaImpl.cpp (right):

http://codereview.chromium.org/149716/diff/1/4#newcode41
Line 41: WebStorageAreaImpl::WebStorageAreaImpl(PassRefPtr<WebCore::StorageArea>
storageArea)
On 2009/07/15 23:24:05, darin wrote:
> it's up to you, but i've been preferring to just stick a using namespace
WebCore
> at the top of .cpp files in api/src.  over time this file might accumulate
more
> WebCore usage, and so it'll probably be nice to have the using directive.

In this case, I think it's more clear to leave the WebCore::'s.  There aren't
too many.

http://codereview.chromium.org/149716/diff/1/4#newcode74
Line 74: keyException = true;
On 2009/07/15 23:24:05, darin wrote:
> nit: 4 space indent

Done.

http://codereview.chromium.org/149716/diff/1/4#newcode92
Line 92: ASSERT(exceptionCode == WebCore::QUOTA_EXCEEDED_ERR);
On 2009/07/15 23:24:05, darin wrote:
> nit: 4 space indent

Done.

http://codereview.chromium.org/149716/diff/1/6
File webkit/api/src/WebStorageAreaImpl.h (right):

http://codereview.chromium.org/149716/diff/1/6#newcode39
Line 39: namespace WebKit {
On 2009/07/15 23:24:05, darin wrote:
> nit: insert new line after namespace line

Done.

http://codereview.chromium.org/149716/diff/1/5
File webkit/api/src/WebStorageNamespaceImpl.cpp (right):

http://codereview.chromium.org/149716/diff/1/5#newcode45
Line 45: return new
WebStorageNamespaceImpl(WebCore::StorageNamespaceImpl::localStorageNamespace(path));
On 2009/07/15 23:24:05, darin wrote:
> ah, so you do expect the receiver to delete these.  then i think these methods
> should be named "create*StorageNamespace".  the "create" prefix is the clue to
> the caller that they are receiving ownership.

Done.

http://codereview.chromium.org/149716/diff/1/5#newcode65
Line 65: return new
WebStorageAreaImpl(m_storageNamespace->storageArea(origin.get()));
On 2009/07/15 23:24:05, darin wrote:
> ditto... if the caller receives ownership, then name the method "createFoo"

Done.

http://codereview.chromium.org/149716/diff/1/3
File webkit/api/src/WebStorageNamespaceImpl.h (right):

http://codereview.chromium.org/149716/diff/1/3#newcode39
Line 39: namespace WebKit {
On 2009/07/15 23:24:05, darin wrote:
> nit: add extra new line below this line

Done.

http://codereview.chromium.org/149716/diff/1/3#newcode42
Line 42: WebStorageNamespaceImpl(PassRefPtr<WebCore::StorageNamespace>
sessionStorageNamespace);
On 2009/07/15 23:24:05, darin wrote:
> nit: did you really mean to name this parameter "sessionStorageNamespace" ?

no

[jam, 2009-07-16 00:50:18]

Darin is more qualified than me to review the WebKit API stuff, so I defer to
him.  Nothing wrong jumped at me.

[darin (slow to review), 2009-07-16 04:55:59]

LGTM