[webkit-reviews] review denied: [Bug 26356] Need v8 bindings for DOM Storage : [Attachment 31214] v1

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied Jeremy Orlow
<jorlow at chromium.org>'s request for review:
Bug 26356: Need v8 bindings for DOM Storage
https://bugs.webkit.org/show_bug.cgi?id=26356

Attachment 31214: v1
https://bugs.webkit.org/attachment.cgi?id=31214&action=review

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
> +2009-06-12  jorlow  <jorlow at chromium.org>

Need to set your environment variables:
http://webkit.org/coding/contributing.html

> +	   * bindings/v8/custom/V8StorageCustom.cpp: Added.

When you add a file, 

> +	   (WebCore::V8Custom::v8StorageNamedPropertyEnumerator):
> +	   (WebCore::storageGetter):
> +	   (WebCore::INDEXED_PROPERTY_GETTER):
> +	   (WebCore::NAMED_PROPERTY_GETTER):
> +	   (WebCore::storageSetter):
> +	   (WebCore::INDEXED_PROPERTY_SETTER):
> +	   (WebCore::NAMED_PROPERTY_SETTER):
> +	   (WebCore::storageDeleter):
> +	   (WebCore::INDEXED_PROPERTY_DELETER):
> +	   (WebCore::NAMED_PROPERTY_DELETER):

... there's no need to leave all the members listed in ChangeLog.

> +#include "config.h"
> +
> +#include "v8_binding.h"
V8Binding.h

> +#include "v8_custom.h"
V8CustomBinding.h

> +#include "V8Proxy.h"
> +
> +#include "Storage.h"

headers are out of order.

> +static v8::Handle<v8::Value> storageSetter(v8::Local<v8::String> nameV8,
v8::Local<v8::Value> valueV8, const v8::AccessorInfo& info)

The convention is prefixes, rather than suffixes: v8Name, v8Value

> +
> +NAMED_PROPERTY_DELETER(Storage) {

Brace on new line.

> +    INC_STATS("DOM.Storage.NamedPropertyDeleter");
> +    return storageDeleter(name, info);
> +}