[webkit-reviews] review requested: [Bug 100851] [V8] use toV8Fast in all relevant Node getters : [Attachment 171854] Patch

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has asked  for review:
Bug 100851: [V8] use toV8Fast in all relevant Node getters
https://bugs.webkit.org/show_bug.cgi?id=100851

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

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


> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:527
> +    my $has_empty_toV8 = 0;
> +    my $has_fast_toV8 = 0;

WebKit uses camelCase for variable names.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:604
> +    die("HasFastToV8 is out of sync with respect to interface $interfaceName
[$has_fast_toV8]\n") if HasFastToV8($interfaceName) != $has_fast_toV8;
> +    die("HasEmptyToV8 is out of sync with respect to interface
$interfaceName [$has_fast_toV8]\n") if HasEmptyToV8($interfaceName) !=
$has_empty_toV8;

Basically we don't want to introduce hard-coded class lists (i.e. HasFastToV8()
and HasEmptyToV8()). See below comments.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:4060
> +sub HasEmptyToV8
> +{
> +    my $type = shift;
> +
> +    return 1 if $type eq 'Element';
> +    return 1 if $type eq 'SVGRenderingIntent';
> +    return 1 if $type eq 'SVGUnitTypes';
> +    return 1 if $type eq 'SVGZoomAndPan';
> +
> +    return 0;
> +}

What is this?

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:4071
> +    return 0 if $type eq 'Document';
> +    return 0 if $type eq 'Element';
> +    return 0 if $type eq 'HTMLDocument';
> +    return 0 if $type eq 'HTMLElement';
> +    return 0 if $type eq 'SVGDocument';
> +    return 0 if $type eq 'SVGElement';

Wouldn't it be possible to rename toV8() in custom bindings to toV8Fast() and
thus remove this hard-coded class list from CodeGeneratorV8.pm? (It would be
better if we could make the implementation of custom toV8Fast() *fast*er, but
let's do it in follow-up patches.)

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:4076
>  sub IsDOMNodeType

Can't we just replace IsDOMNodeType() with IsNodeSubType(), and thus kill
IsDOMNodeType() ?