Bug 93248 – [V8] Remove custom toV8() calls for TypedArray.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Vineet Chaudhary (vineetc)]

With the support of [TypedArray] we can remove the custom calls toV8().
TypedArray spec : http://www.khronos.org/registry/typedarray/specs/latest/#7

[Vineet Chaudhary (vineetc)]

Created attachment 156663 [details]
wipPatch

With current patch I had to skip below idls with custom bindings because of 
naming conventions at v8 bindings side.
ExternalArrayType enum values are declared in v8.h and used in v8/heap and v8/d8 code.

IDL File              could be          Actual  
Int8Array.idl         signed char   ==> Byte
Uint8Array.idl        unsigned char ==> UnsignedByte
Uint8ClampedArray.idl unsigned char ==> Pixel

[Vineet Chaudhary (vineetc)]

Comment on attachment 156663 [details]
wipPatch

View in context: https://bugs.webkit.org/attachment.cgi?id=156663&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2600
> +        $viewType =~ s/\s//g;;

Sorry I couldn't combine these in one do we have better solution?

[Kentaro Hara]

(In reply to comment #1)
> ExternalArrayType enum values are declared in v8.h and used in v8/heap and v8/d8 code.
> 
> IDL File              could be          Actual  
> Int8Array.idl         signed char   ==> Byte
> Uint8Array.idl        unsigned char ==> UnsignedByte
> Uint8ClampedArray.idl unsigned char ==> Pixel

We cannot change the variable names for V8 compatibility. How about hard-coding name mapping in CodeGeneratorV8.pm? I agree that it's dirty but it would be better than having custom binding code.

[Vineet Chaudhary (vineetc)]

Created attachment 156683 [details]
UpdatedPatch

Updated patch. This also removes the custom bindings for Int8Array.idl, Uint8Array.idl and Uint8ClampedArray.idl.

[Kentaro Hara]

Comment on attachment 156683 [details]
UpdatedPatch

View in context: https://bugs.webkit.org/attachment.cgi?id=156683&action=review

> Source/WebCore/ChangeLog:11
> +        Tests: TestTypedArray.idl

Nit: Please just confirm that WebGL tests pass.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3925
> +sub GetTypedArrayView

"GetTypeNameOfExternalTypedArray" might be a better name.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3936
> +    $viewType = "byte" if $viewType eq "signed char";
> +    $viewType = "pixel" if $viewType eq "unsigned char" and $interfaceName eq "Uint8ClampedArray";
> +    $viewType = "unsigned byte" if $viewType eq "unsigned char";
> +    $viewType =~ s/\b(\w)/\u$1/g;
> +    $viewType =~ s/\s//g;
> +
> +    return $viewType = "v8::kExternal${viewType}Array";

For readability and maintainability, I would prefer the following code:

  return "v8::kExternalByteArray" if ...;
  return "v8::kExternalPixelArray" if ...;
  return "v8::kExternalUnsignedByteArray" if ...;
  return "v8::..." if ...;
  return "v8::..." if ...;
  return "v8::..." if ...;

[WebKit Review Bot]

Comment on attachment 156683 [details]
UpdatedPatch

Attachment 156683 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13445307

New failing tests:
http/tests/cache/post-with-cached-subresources.php

[WebKit Review Bot]

Created attachment 156692 [details]
Archive of layout-test-results from gce-cr-linux-08

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-08  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Vineet Chaudhary (vineetc)]

Created attachment 156884 [details]
updatedPatch

> Nit: Please just confirm that WebGL tests pass.

I just checked webgl tests running and got few IMAGE Mismatch failures but both were same
with and without this patch. IMO This should not cause any regression as there are no
behavioural changes.

> New failing tests:
> http/tests/cache/post-with-cached-subresources.php

This shouldn't be because of these changes.

> +sub GetTypedArrayView
>"GetTypeNameOfExternalTypedArray" might be a better name.
Done.

> For readability and maintainability, I would prefer the following code:
>  return "v8::kExternalByteArray" if ...;
>  return "v8::kExternalPixelArray" if ...; .. 
Done.

[Kentaro Hara]

Comment on attachment 156884 [details]
updatedPatch

View in context: https://bugs.webkit.org/attachment.cgi?id=156884&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3934
> +    $viewType =~ s/\b(\w)/\u$1/g;
> +    $viewType =~ s/\s//g;

This looks like an error-prone magic. (We might want to avoid Perl magic for readability.) Shall we hard-code everything? There are only six variables.

[Vineet Chaudhary (vineetc)]

(In reply to comment #9)
> (From update of attachment 156884 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156884&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3934
> > +    $viewType =~ s/\b(\w)/\u$1/g;
> > +    $viewType =~ s/\s//g;
> 
> This looks like an error-prone magic. (We might want to avoid Perl magic for readability.) Shall we hard-code everything? There are only six variables.

> > +    $viewType =~ s/\b(\w)/\u$1/g;
$viewType =~ s/\b(\w)/\ucfirst$1/g; //will also do for readability
only reason to use regExp was minimal hard-coding.

How about
return "v8::kExternalByteArray" if $viewType eq "signed char" and $interfaceName eq "Int8Array";
return "v8::kExternalFloatArray" if $viewType eq "float" and $interfaceName eq "Float32Array";
....
This will make sure correct interface name with external view type.

[Kentaro Hara]

(In reply to comment #10)
> How about
> return "v8::kExternalByteArray" if $viewType eq "signed char" and $interfaceName eq "Int8Array";
> return "v8::kExternalFloatArray" if $viewType eq "float" and $interfaceName eq "Float32Array";
> ....
> This will make sure correct interface name with external view type.

Sounds best!

[Vineet Chaudhary (vineetc)]

Created attachment 156889 [details]
updatedPatch

Done!

[Kentaro Hara]

Comment on attachment 156889 [details]
updatedPatch

View in context: https://bugs.webkit.org/attachment.cgi?id=156889&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3939
> +}

Let's add:

    die 'TypedArray of unknown type is found';

[Vineet Chaudhary (vineetc)]

Created attachment 156893 [details]
another_attempt

Done..

[Kentaro Hara]

Comment on attachment 156893 [details]
another_attempt

Thank you very much for the patch!

[Vineet Chaudhary (vineetc)]

(In reply to comment #15)
> (From update of attachment 156893 [details])
> Thank you very much for the patch!

Thanks I will add description of [TypedArray] to WebIDL now.

[WebKit Review Bot]

Comment on attachment 156893 [details]
another_attempt

Clearing flags on attachment: 156893

Committed r124872: <http://trac.webkit.org/changeset/124872>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Vineet Chaudhary (vineetc)]

Created attachment 156898 [details]
WebIDL_Wiki page

haraken: I have just prepared draft for WebIDL wiki page for [TypedArray] Can you please have look?
Please let me know you want me to add more.

[Kentaro Hara]

(In reply to comment #19)
> haraken: I have just prepared draft for WebIDL wiki page for [TypedArray] Can you please have look?
> Please let me know you want me to add more.

Is TypedArray=* supported? Either way please feel free to edit the wiki page. (I will add something if I want more:-)