[webkit-reviews] review denied: [Bug 107207] [V8] Support selectively wrapping DOM accesses from certain V8 contexts. : [Attachment 196060] Patch

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has denied Ulfar Erlingsson
<ulfar.chromium at gmail.com>'s request for review:
Bug 107207: [V8] Support selectively wrapping DOM accesses from certain V8
contexts.
https://bugs.webkit.org/show_bug.cgi?id=107207

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

------- Additional Comments from Kentaro Hara <haraken at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=196060&action=review


r-ing because you don't yet resolve the performance regression caused by
V8PerContextData::from(v8::Context::GetCurrent())->activityLogger().

> Source/WebCore/ChangeLog:7
> +

Please describe a rationale for the change.

> Source/WebCore/ChangeLog:8
> +	   No new tests (OOPS!).

You need to add a test to run-bindings-tests. See
https://trac.webkit.org/wiki/WebKitIDL#RunBindingsTests

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:945
> +    my $accessType = shift;

Let's use "getter", "setter" and "method", instead of "get", "set" and "call".

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:958
> +	   OwnArrayPtr<v8::Handle<v8::Value> > loggerArgs =
toOwnArrayPtrArguments<v8::Handle<v8::Value> >(args, 0);

Why do you need to use OwnArrayPtr?

Given that values in 'args' don't need to outlive logger->log(), I think you
don't need to own them here. (c.f. you are not protecting the lifetime in the
"get" case below.)

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:963
> +	   v8::Handle<v8::Value> loggerArg[] = { value };

You don't own value here. I think this is OK.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1360
> +    if (HasActivityLogging($forMainWorldSuffix, $attrExt, "Getter")) {

"Getter" => "Setter"

> Source/WebCore/bindings/scripts/IDLAttributes.txt:137
>
+V8ActivityLog=Access|Setter|Getter|AccessForAllWorlds|SetterForAllWorlds|Gette
rForAllWorlds

Nit: I'd prefer
Access|Setter|Getter|AccessForIsolatedWorlds|SetterForIsolatedWorlds|GetterForI
solatedWorlds. "ForAllWorlds by default" sounds a bit better to me.

> Source/WebCore/bindings/v8/V8Binding.h:399
> +    PassOwnArrayPtr<T> toOwnArrayPtrArguments(const v8::Arguments& args, int
startIndex)

Nit: Will startIndex be helpful?