Bug 93248 - [V8] Remove custom toV8() calls for TypedArray.
Summary: [V8] Remove custom toV8() calls for TypedArray.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Vineet Chaudhary (vineetc)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-06 03:33 PDT by Vineet Chaudhary (vineetc)
Modified: 2012-08-07 02:36 PDT (History)
6 users (show)

See Also:


Attachments
wipPatch (8.20 KB, patch)
2012-08-06 04:53 PDT, Vineet Chaudhary (vineetc)
no flags Details | Formatted Diff | Diff
UpdatedPatch (16.18 KB, patch)
2012-08-06 06:42 PDT, Vineet Chaudhary (vineetc)
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-08 (625.74 KB, application/zip)
2012-08-06 07:46 PDT, WebKit Review Bot
no flags Details
updatedPatch (16.38 KB, patch)
2012-08-07 00:15 PDT, Vineet Chaudhary (vineetc)
no flags Details | Formatted Diff | Diff
updatedPatch (16.95 KB, patch)
2012-08-07 00:59 PDT, Vineet Chaudhary (vineetc)
haraken: review+
Details | Formatted Diff | Diff
another_attempt (17.00 KB, patch)
2012-08-07 01:25 PDT, Vineet Chaudhary (vineetc)
no flags Details | Formatted Diff | Diff
WebIDL_Wiki page (661 bytes, text/plain)
2012-08-07 02:34 PDT, Vineet Chaudhary (vineetc)
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Vineet Chaudhary (vineetc) 2012-08-06 03:33:38 PDT
With the support of [TypedArray] we can remove the custom calls toV8().
TypedArray spec : http://www.khronos.org/registry/typedarray/specs/latest/#7
Comment 1 Vineet Chaudhary (vineetc) 2012-08-06 04:53:17 PDT
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
Comment 2 Vineet Chaudhary (vineetc) 2012-08-06 04:55:06 PDT
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?
Comment 3 Kentaro Hara 2012-08-06 04:56:28 PDT
(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.
Comment 4 Vineet Chaudhary (vineetc) 2012-08-06 06:42:07 PDT
Created attachment 156683 [details]
UpdatedPatch

Updated patch. This also removes the custom bindings for Int8Array.idl, Uint8Array.idl and Uint8ClampedArray.idl.
Comment 5 Kentaro Hara 2012-08-06 06:56:54 PDT
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 ...;
Comment 6 WebKit Review Bot 2012-08-06 07:46:39 PDT
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
Comment 7 WebKit Review Bot 2012-08-06 07:46:42 PDT
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
Comment 8 Vineet Chaudhary (vineetc) 2012-08-07 00:15:40 PDT
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.
Comment 9 Kentaro Hara 2012-08-07 00:25:53 PDT
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.
Comment 10 Vineet Chaudhary (vineetc) 2012-08-07 00:39:18 PDT
(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.
Comment 11 Kentaro Hara 2012-08-07 00:40:14 PDT
(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!
Comment 12 Vineet Chaudhary (vineetc) 2012-08-07 00:59:42 PDT
Created attachment 156889 [details]
updatedPatch

Done!
Comment 13 Kentaro Hara 2012-08-07 01:07:14 PDT
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';
Comment 14 Vineet Chaudhary (vineetc) 2012-08-07 01:25:58 PDT
Created attachment 156893 [details]
another_attempt

Done..
Comment 15 Kentaro Hara 2012-08-07 01:27:28 PDT
Comment on attachment 156893 [details]
another_attempt

Thank you very much for the patch!
Comment 16 Vineet Chaudhary (vineetc) 2012-08-07 01:29:50 PDT
(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.
Comment 17 WebKit Review Bot 2012-08-07 02:27:54 PDT
Comment on attachment 156893 [details]
another_attempt

Clearing flags on attachment: 156893

Committed r124872: <http://trac.webkit.org/changeset/124872>
Comment 18 WebKit Review Bot 2012-08-07 02:28:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Vineet Chaudhary (vineetc) 2012-08-07 02:34:02 PDT
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.
Comment 20 Kentaro Hara 2012-08-07 02:36:22 PDT
(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:-)