Gen index getter by unified way instead of using several helper function in some special cases.

Gen index getter by unified way instead of using several helper function in some special cases.

Currently there's 3 kind of helper below:
setCollectionStringOrUndefinedIndexedGetter
setCollectionStringIndexedGetter
setCollectionIndexedGetter
Those register 3 fixed indexed property getter(template function).
In this situation, it is difficult to customize or debug some specified IDL.
With this update, those are all auto-generated by CodeGeneratorV8 so flexibility is improved.

R=haraken@chromium.org
BUG=229740
TEST=V8TestEventTarget.cpp (generated by run-bindings-tests)

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=148617

[kojih, 2013-04-17 03:50:54]

haraken:
This is just for discsussion for now.

This patch replace setCollectionXXX to auto-generate code for index getter. with
index check before call item().
but some problem remained:
performance regression on GridSort:
HTMLCollection, length() is slow


https://code.google.com/p/chromium/issues/detail?id=230684&q=kojih&colspec=ID...

[haraken, 2013-04-17 04:05:18]

Although I don't yet understand what your patch is doing, I have two
suggestions:

- The previous patch looks perfect except that it caused performance regression.
Would it be hard to simply replace 'if (index >= collections->length())' with
'if (collections->isValidIndex(index))' ?

- Would it be hard to implement collections->isValidIndex() by using existing
HTMLCollection's methods as much as possible? It looks like your patch
duplicates a lot of code.

[kojih, 2013-04-17 06:09:00]

As discussed offline, I'd like to implement isValidIndex() but I don't know how
to do that.

I have 2 parallel work related to this.

(1) [Change behavior better for spec] Replacing null->undefined with
length<index -> undefined
  This is blocked by difficulty of making isValidIndex()


(2) [Refactoring] Replacing setCollectionXXX... to auto-generated index getter

My suggestion is landing (2) before (1).

[kojih, 2013-04-17 06:10:25]

My previous patch is for (1), which causes performance regression in case of
HTMLCollection, HTMLAllCollection, HTMLFormControlsCollection and
HTMLOptionsCollection.

[kojih, 2013-04-17 08:11:09]

r?

[kojih, 2013-04-17 08:20:32]

added comments.

https://codereview.chromium.org/13933014/diff/6001/LayoutTests/fast/dom/CSSSt...
File
LayoutTests/fast/dom/CSSStyleDeclaration/css-computed-style-item-expected.txt
(right):

https://codereview.chromium.org/13933014/diff/6001/LayoutTests/fast/dom/CSSSt...
LayoutTests/fast/dom/CSSStyleDeclaration/css-computed-style-item-expected.txt:7:
PASS computedStyle[computedStyle.length] is ""
Contains revert of 148030 (Included in this patch because I heard real revert
causes lots of timeout tests)

https://codereview.chromium.org/13933014/diff/6001/LayoutTests/fast/dom/CSSSt...
File LayoutTests/fast/dom/CSSStyleDeclaration/css-style-item-expected.txt
(right):

https://codereview.chromium.org/13933014/diff/6001/LayoutTests/fast/dom/CSSSt...
LayoutTests/fast/dom/CSSStyleDeclaration/css-style-item-expected.txt:8: PASS
style[1] is ""
Contains revert of 148030 (Included in this patch because I heard real revert
causes lots of timeout tests)

https://codereview.chromium.org/13933014/diff/6001/LayoutTests/fast/dom/CSSSt...
File
LayoutTests/fast/dom/CSSStyleDeclaration/script-tests/css-computed-style-item.js
(right):

https://codereview.chromium.org/13933014/diff/6001/LayoutTests/fast/dom/CSSSt...
LayoutTests/fast/dom/CSSStyleDeclaration/script-tests/css-computed-style-item.js:9:
shouldBeEqualToString('computedStyle[computedStyle.length]', '');
Contains revert of 148030 (Included in this patch because I heard real revert
causes lots of timeout tests)

https://codereview.chromium.org/13933014/diff/6001/Source/WebCore/dom/NamedNo...
File Source/WebCore/dom/NamedNodeMap.idl (left):

https://codereview.chromium.org/13933014/diff/6001/Source/WebCore/dom/NamedNo...
Source/WebCore/dom/NamedNodeMap.idl:25: CustomIndexedGetter,
Actually NamedNodeMap doesn't have custom implementation.

https://codereview.chromium.org/13933014/diff/6001/Source/WebCore/html/HTMLFo...
File Source/WebCore/html/HTMLFormElement.idl (left):

https://codereview.chromium.org/13933014/diff/6001/Source/WebCore/html/HTMLFo...
Source/WebCore/html/HTMLFormElement.idl:22: IndexedGetter,
Actually HTMLFormElement has custom implementation of indexed property getter.

https://codereview.chromium.org/13933014/diff/6001/Source/WebCore/html/HTMLSe...
File Source/WebCore/html/HTMLSelectElement.idl (left):

https://codereview.chromium.org/13933014/diff/6001/Source/WebCore/html/HTMLSe...
Source/WebCore/html/HTMLSelectElement.idl:22: IndexedGetter,
Actually HTMLFormElement has custom implementation of indexed property getter.

https://codereview.chromium.org/13933014/diff/6001/Source/bindings/scripts/Co...
File Source/bindings/scripts/CodeGeneratorV8.pm (right):

https://codereview.chromium.org/13933014/diff/6001/Source/bindings/scripts/Co...
Source/bindings/scripts/CodeGeneratorV8.pm:701: my $hasCustomIndexedGetter =
$interface->extendedAttributes->{"IndexedGetter"} ||
$interface->extendedAttributes->{"CustomIndexedGetter"} ||
$interface->extendedAttributes->{"CustomGetOwnPropertySlot"};
CustomGetOwnPropertySlot can be removed. 
remove in next patch.

https://codereview.chromium.org/13933014/diff/6001/Source/bindings/scripts/Co...
Source/bindings/scripts/CodeGeneratorV8.pm:3347: push(@implContent,
$indexPropertyGetterCode);
This will more clean by 
https://codereview.chromium.org/14261002/
that is waiting for CQ to open.

https://codereview.chromium.org/13933014/diff/6001/Source/bindings/tests/resu...
File Source/bindings/tests/results/V8TestCallback.cpp (right):

https://codereview.chromium.org/13933014/diff/6001/Source/bindings/tests/resu...
Source/bindings/tests/results/V8TestCallback.cpp:42: {
I don't expected this.
I will investigate.

[kojih, 2013-04-17 08:35:27]

https://codereview.chromium.org/13933014/diff/6001/Source/bindings/tests/resu...
File Source/bindings/tests/results/V8TestCallback.cpp (right):

https://codereview.chromium.org/13933014/diff/6001/Source/bindings/tests/resu...
Source/bindings/tests/results/V8TestCallback.cpp:42: {
It was due to the change below.
(may be forgot to --reset-results)

commit 47bf870dc30d79ab48ce72078ca83bf2f624640c
Author: adamk@chromium.org
<adamk@chromium.org@bbb929c8-8fbe-4397-9dbb-9b2b20218538>
Date:   Tue Apr 16 22:07:23 2013 +0000

    Make DOMWrapperWorlds easier to use
    
    This patch adds a DOMWrapperWorld::current() static method that
    returns either the DOMWrapperWorld for the current V8 context or
    null if in a Worker.
    
    Recently-transitioned callsites that previously used WorkerContextHandle
    have been updated to call DOMWrapperWorld::current() instead, and it
    will be used in upcoming WorldContextHandle transitions.
    
    This lets us remove a FIXME in toV8Context() since all non-Worker
    callers now pass in a non-null DOMWrapperWorld (rather than the
    old behavior of passing null as a sentinel for "main world").
    
    R=abarth
    BUG=230460
    
    Review URL: https://codereview.chromium.org/14297007

[kojih, 2013-04-17 08:41:19]

https://codereview.chromium.org/13933014/diff/6001/Source/bindings/tests/resu...
File Source/bindings/tests/results/V8TestObj.cpp (right):

https://codereview.chromium.org/13933014/diff/6001/Source/bindings/tests/resu...
Source/bindings/tests/results/V8TestObj.cpp:37: #include "UseCounter.h"
(may be forgot to --reset-results)

[haraken, 2013-04-17 08:41:56]

https://codereview.chromium.org/13933014/diff/6001/Source/bindings/scripts/Co...
File Source/bindings/scripts/CodeGeneratorV8.pm (right):

https://codereview.chromium.org/13933014/diff/6001/Source/bindings/scripts/Co...
Source/bindings/scripts/CodeGeneratorV8.pm:2613: sub
GenerateImplementationIndexer
GenerateImplementationIndexer => GenerateImplementationIndexedProperty ?

https://codereview.chromium.org/13933014/diff/6001/Source/bindings/scripts/Co...
Source/bindings/scripts/CodeGeneratorV8.pm:2621: my $hasCustomSetter =
$interface->extendedAttributes->{"CustomIndexedSetter"} &&
!$interface->extendedAttributes->{"NumericIndexedGetter"};
$hasCustomSetter => $hasCustomIndexedSetter?

Let's remove [NumericIndexedGetter] in a follow-up patch, as commented in the
FIXME above.

https://codereview.chromium.org/13933014/diff/6001/Source/bindings/scripts/Co...
Source/bindings/scripts/CodeGeneratorV8.pm:2622: my $hasGetter =
$interface->extendedAttributes->{"IndexedGetter"} ||
$interface->extendedAttributes->{"CustomIndexedGetter"};
$hasSetter => $hasIndexedSetter?

https://codereview.chromium.org/13933014/diff/6001/Source/bindings/scripts/Co...
Source/bindings/scripts/CodeGeneratorV8.pm:2684: my $indexerType = $indexer ?
$indexer->type : 0;
You already did this at line 2654.

https://codereview.chromium.org/13933014/diff/6001/Source/bindings/scripts/Co...
Source/bindings/scripts/CodeGeneratorV8.pm:2692: my $elemType;
$elemType => $elementType. Blink adopts full-word naming.

https://codereview.chromium.org/13933014/diff/6001/Source/bindings/scripts/Co...
Source/bindings/scripts/CodeGeneratorV8.pm:2694: my $stmt;
This is not used.

https://codereview.chromium.org/13933014/diff/6001/Source/bindings/scripts/Co...
Source/bindings/scripts/CodeGeneratorV8.pm:2695: my $elemTypeNew =
GetNativeType($indexerType, 0);
This is not used.

https://codereview.chromium.org/13933014/diff/6001/Source/bindings/scripts/Co...
Source/bindings/scripts/CodeGeneratorV8.pm:2696: my $varName = "element";
This is not used.

https://codereview.chromium.org/13933014/diff/6001/Source/bindings/scripts/Co...
Source/bindings/scripts/CodeGeneratorV8.pm:2697: $elemTypeNew = "const
$elemTypeNew&" unless ($codeGenerator->IsRefPtrType($indexerType));
This is not used.

https://codereview.chromium.org/13933014/diff/6001/Source/bindings/scripts/Co...
Source/bindings/scripts/CodeGeneratorV8.pm:2698: $varName .= ".release()" if
($codeGenerator->IsRefPtrType($indexerType));
This is not used.

https://codereview.chromium.org/13933014/diff/6001/Source/bindings/scripts/Co...
Source/bindings/scripts/CodeGeneratorV8.pm:2701: $elemType = "const String&";
You should get $elemType by using GetNaiveTypeForXXX() (and should write it
outside of the if clause.)

https://codereview.chromium.org/13933014/diff/6001/Source/bindings/scripts/Co...
Source/bindings/scripts/CodeGeneratorV8.pm:2712: }
In the end, we want to remove all if clauses above. We can do it by removing
special cases for WebKitCSSKeyframesRule and removing the null check. You can do
it in a follow-up patch.

https://codereview.chromium.org/13933014/diff/6001/Source/bindings/scripts/Co...
Source/bindings/scripts/CodeGeneratorV8.pm:2719: 
Nit: Remove a redundant empty line.

https://codereview.chromium.org/13933014/diff/6001/Source/bindings/scripts/Co...
Source/bindings/scripts/CodeGeneratorV8.pm:2721: 
Ditto.

https://codereview.chromium.org/13933014/diff/6001/Source/bindings/scripts/Co...
Source/bindings/scripts/CodeGeneratorV8.pm:2722: if ( $isNull )
Nit: if ($isNull)

https://codereview.chromium.org/13933014/diff/6001/Source/bindings/scripts/Co...
Source/bindings/scripts/CodeGeneratorV8.pm:2724: 
Nit: Remove the line.

https://codereview.chromium.org/13933014/diff/6001/Source/bindings/scripts/Co...
Source/bindings/scripts/CodeGeneratorV8.pm:2802: 
Nit: Remove this.

https://codereview.chromium.org/13933014/diff/6001/Source/bindings/scripts/Co...
Source/bindings/scripts/CodeGeneratorV8.pm:3347: push(@implContent,
$indexPropertyGetterCode);
OK.

https://codereview.chromium.org/13933014/diff/6001/Source/bindings/tests/resu...
File Source/bindings/tests/results/V8TestCallback.cpp (right):

https://codereview.chromium.org/13933014/diff/6001/Source/bindings/tests/resu...
Source/bindings/tests/results/V8TestCallback.cpp:42: {
Please just keep the current code as is.

[kojih, 2013-04-17 09:13:38]

fixed. r?

[kojih, 2013-04-17 09:15:15]

Patch set 7 is latest. r?

[haraken, 2013-04-17 10:29:52]

https://codereview.chromium.org/13933014/diff/11003/Source/bindings/scripts/C...
File Source/bindings/scripts/CodeGeneratorV8.pm (right):

https://codereview.chromium.org/13933014/diff/11003/Source/bindings/scripts/C...
Source/bindings/scripts/CodeGeneratorV8.pm:2690: my $elementType =
GetNativeType($indexerType);
$elementType => $nativeType

https://codereview.chromium.org/13933014/diff/11003/Source/bindings/scripts/C...
Source/bindings/scripts/CodeGeneratorV8.pm:2693: if ($indexerType eq
"DOMString") {
Shall we use $codeGenerator->IsRefPtrType($indexerType) instead of hard-coding
"DOMString" ?

[kojih, 2013-04-17 11:07:38]

r?

anyway, I'll upload this after
https://codereview.chromium.org/14261002/
is landed.

[kojih, 2013-04-18 04:27:51]

rebased to latest and ready.
r?

[haraken, 2013-04-18 04:33:34]

LGTM

[commit-bot: I haz the power, 2013-04-18 04:36:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kojih@chromium.org/13933014/23001

[commit-bot: I haz the power, 2013-04-18 05:18:47]

Change committed as 148617