[webkit-reviews] review denied: [Bug 111699] [V8] generate indexedPropertyGetter instead of using collectionStringIndexedPropertyGetter() : [Attachment 192126] Patch

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has denied Koji Hara <kojih at chromium.org>'s
request for review:
Bug 111699: [V8] generate indexedPropertyGetter instead of using
collectionStringIndexedPropertyGetter()
https://bugs.webkit.org/show_bug.cgi?id=111699

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

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


> Source/WebCore/ChangeLog:14
> +	   No new tests: code refactoring.

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

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2500
> +    # FIXME: Clarify condition when enumerator should be enabled.
> +    # Basically an interface that has indexedPropertyGetter must have
indexedPropertyEnumerator but currently there are some special cases.

A bit better:

  FIXME: Remove the special cases. Interfaces that have indexedPropertyGetter
should have indexedPropertyEnumerator.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2526
> +    # FIXME: Remove this block to use generated indexed getter instead of
halfway ones such as V8Collection::collectionXXXIndexedPropertyGetter().

FIXME: This block is full of hacks to incrementally replace custom indexed
getters with auto-generated ones. Remove this block and V8Collection.h.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2566
> +    # FIXME: Clarify condition for indexedPropertyEnumerator to be available
then implement it.

I think you can remove the FIXME. You already wrote the same thing above.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3291
> +	   if (!$indexer) {
> +	       $indexer = $codeGenerator->FindSuperMethod($interface, "item");
> +	   }

Remove this. This is already done above.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3294
> +	   # FIXME: Figure out what NumericIndexedGetter is really supposed to
do. Right now, it's only set on WebGL-related files.

Remove the FIXME. This is already written above. (Let's kill
NumericIndexedGetter in the near future.)

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3297
> +	       if ($indexerType eq "DOMString") {

I think you can remove the branch by using NativeToJSValue().

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3300
> +		   my $conversion =
$indexer->extendedAttributes->{"TreatReturnedNullStringAs"};
> +		   if ($conversion && $conversion eq "Null") {
> +		       # FIXME: generate indexedPropertyGetter instead of
generating setCollectionStringOrUndefinedIndexedGetter() call in
GenerateImplementationIndexer.

NativeToJSValue() will care about this.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3309
> +    return v8String(result, info.GetIsolate());

This part should be generated by NativeToJSValue().

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3315
> +	       } else {
> +		   # FIXME: generate indexedPropertyGetter instead of
generating setCollectionIndexedGetter() call in GenerateImplementationIndexer.
> +	       }

NativeToJSValue() will care about this.