Summary: | [V8] Remove custom toV8() calls for TypedArray. | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Vineet Chaudhary (vineetc) <code.vineet> | ||||||||||||||||
Component: | New Bugs | Assignee: | Vineet Chaudhary (vineetc) <code.vineet> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | abarth, dglazkov, haraken, japhet, ojan, webkit.review.bot | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Attachments: |
|
Description
Vineet Chaudhary (vineetc)
2012-08-06 03:33:38 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 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? (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. Created attachment 156683 [details]
UpdatedPatch
Updated patch. This also removes the custom bindings for Int8Array.idl, Uint8Array.idl and Uint8ClampedArray.idl.
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 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 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
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 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. (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. (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! Created attachment 156889 [details]
updatedPatch
Done!
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'; Created attachment 156893 [details]
another_attempt
Done..
Comment on attachment 156893 [details]
another_attempt
Thank you very much for the patch!
(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 on attachment 156893 [details] another_attempt Clearing flags on attachment: 156893 Committed r124872: <http://trac.webkit.org/changeset/124872> All reviewed patches have been landed. Closing bug. 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.
(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:-) |