Summary: | [V8] generate indexedPropertyGetter instead of using collectionStringIndexedPropertyGetter() | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | foo <kojih> | ||||||||||
Component: | New Bugs | Assignee: | foo <kojih> | ||||||||||
Status: | RESOLVED INVALID | ||||||||||||
Severity: | Normal | CC: | abarth, andersca, cmarcelo, dglazkov, esprehn+autocc, haraken, japhet, ojan.autocc, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 111810 | ||||||||||||
Attachments: |
|
Description
foo
2013-03-07 01:59:21 PST
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. |