[webkit-reviews] review denied: [Bug 107207] [V8] Support selectively wrapping DOM accesses from certain V8 contexts. : [Attachment 196060] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Apr 1 21:44:55 PDT 2013
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?
More information about the webkit-reviews
mailing list