[webkit-reviews] review denied: [Bug 44678] [v8] More correct and faster error handling when converting v8 objects to various WebCore strings : [Attachment 65706] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied anton muhin <antonm at chromium.org>'s
request for review:
Bug 44678: [v8] More correct and faster error handling when converting v8
objects to various WebCore strings
https://bugs.webkit.org/show_bug.cgi?id=44678

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
> WebCore/bindings/v8/V8Binding.cpp:377
> +    // Most numbers used are <= 100. Even if they aren't used there's very
little in using the space.
very little cost?  Seems we're missing a word here.

> WebCore/bindings/v8/V8Binding.cpp:379
> +    static AtomicString lowNumbers[kLowNumbers + 1];
Shouldn't we use DECLARE_STATIC_LOCAL here?

> WebCore/bindings/v8/V8Binding.cpp:384
> +	       AtomicString valueString = AtomicString(String::number(value));
Is there an advantage to using atomic strings here?  It seems like you're
already doing the atomization.

> WebCore/bindings/v8/V8Binding.cpp:397
> +	   int32ToWebCoreString(object->Int32Value());
Why don't we use the return value here?  Is this code tested???

> WebCore/bindings/v8/V8Binding.h:218
> +	   operator AtomicString() { return toString<AtomicString>(); }
I think we'd be better off if we had explicit conversion operators.  In
general, WebKit frown on these implicit operators.

> WebCore/bindings/v8/custom/V8DeviceMotionEventCustom.cpp:114
> +    STRING_TO_V8PARAMETER_EXCEPTION_BLOCK(V8Parameter<>, type, args[0]);
Why this particular callsite?  Is this the only custom one?