[V8] generate indexedPropertyGetter instead of using collectionStringIndexedPropertyGetter()
Created attachment 192126 [details] Patch
Comment on attachment 192126 [details] Patch 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.
Please block bug 111810 when you work on indexed properties and named properties so that people can track your work easily.
Created attachment 196280 [details] Patch
Comment on attachment 196280 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=196280&action=review Basically the approach looks good. Thanks for the patch. What real IDL files will be affected by this change? > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3423 > + my $indexerType = $indexer ? $indexer->type : 0; > + Remove this. You already did it above. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3435 > + my $nullCheck; You can remove $nullCheck. See the comment below. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3437 > + if ($indexerType eq "DOMString") { You don't need to treat DOMString specially. You can get $elemType by using GetNativeTypeForCallbacks(). > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3445 > + if ($interfaceName eq "WebKitCSSKeyframesRule") { > + $jsValue = "toV8(element.release(), info.Holder(), info.GetIsolate())"; Let's remove this hack in a follow-up patch. It's OK for now. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3454 > + // FIXME: assert that object must be a collection type Why do you think we need the check here? In other DOM getters/setters/methods, we retrieve C++ pointers with toNative(info.Holder()) without checking its type. This is OK because the type check is already done when V8 looks up a prototype chain. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3462 > + // According to spec, JS must return undefined in case of index out of range. > + // see http://www.w3.org/TR/WebIDL/#idl-indexed-properties > + if ($nullCheck) > + return v8Undefined(); You should check whether a given index is out of range or not by comparing the index with collection->length(). You shouldn't rely on whether a returned value is null or not. (The return value can be null for a valid index.)
Created attachment 196289 [details] Patch
Added index bounds check instead of null check. Checking how many tests will fail...
Extra explanation: Current code is checking index out of range by collection->item(index) is NULL or not. This means if collection->item(index) returns NULL for valid index, index getter returns undefined. So current code is not good because index getter should return same value as item() method for valid index. After I added index bound check, 3 tests newly failed for run_webkit_tests.sh.
Comment on attachment 196289 [details] Patch Attachment 196289 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17224720 New failing tests: fast/dom/CSSStyleDeclaration/css-computed-style-item.html fast/dom/CSSStyleDeclaration/css-style-item.html gamepad/gamepad-polling-access.html
Created attachment 196298 [details] Archive of layout-test-results from gce-cr-linux-01 for chromium-linux-x86_64 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-01 Port: chromium-linux-x86_64 Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
V8 is gone.