[Webkit-unassigned] [Bug 107207] [V8] Support selectively wrapping DOM accesses from certain V8 contexts.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=107207


Kentaro Hara <haraken at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #196060|review?                     |review-
               Flag|                            |




--- Comment #33 from Kentaro Hara <haraken at chromium.org>  2013-04-01 21:43:07 PST ---
(From update of attachment 196060)
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|GetterForAllWorlds

Nit: I'd prefer Access|Setter|Getter|AccessForIsolatedWorlds|SetterForIsolatedWorlds|GetterForIsolatedWorlds. "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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.