Bug 106506 – Bindings: Remove special cases for DOMString[]

• Component: New Bugs
• Status: RESOLVED FIXED

[Joshua Bell]

Bindings: Remove special cases for DOMString[]

[Joshua Bell]

Created attachment 182017 [details]
Patch

[Joshua Bell]

I'm expecting some bot failures here, since I ran only some of the tests locally and they didn't exercise everything in Internals.

[Kentaro Hara]

Comment on attachment 182017 [details]
Patch

LGTM. Thanks for the clean up.

[Build Bot]

Comment on attachment 182017 [details]
Patch

Attachment 182017 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15771734

[WebKit Review Bot]

Comment on attachment 182017 [details]
Patch

Attachment 182017 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15760700

New failing tests:
fast/forms/state-restore-broken-state.html
fast/forms/file/selected-files-from-history-state.html
fast/forms/state-restore-skip-stateless.html

[Build Bot]

Comment on attachment 182017 [details]
Patch

Attachment 182017 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15761745

New failing tests:
fast/forms/state-restore-broken-state.html
fast/forms/file/selected-files-from-history-state.html
fast/forms/state-restore-skip-stateless.html

[Build Bot]

Comment on attachment 182017 [details]
Patch

Attachment 182017 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15771807

New failing tests:
fast/forms/state-restore-broken-state.html
fast/forms/file/selected-files-from-history-state.html
fast/forms/state-restore-skip-stateless.html

[Joshua Bell]

Created attachment 182166 [details]
Patch

[Joshua Bell]

Latest patch "should" work, but doesn't - V8Binding's toDOMStringList is failing its IsArray() test for reasons I can't figure out right now. Also, the changes to the generated code are a little greater than I would have expected. Tabling this for now.

[Joshua Bell]

Created attachment 182417 [details]
Patch

[Joshua Bell]

Did this over from scratch; turns out we had better T[] support than I thought so the correct fix was:

* removing special DOMString[] cases in code gen
* patching a few places where T[] wasn't handled quite right in code gen
* update Internals.idl/h/cpp to match the test usage: arrays of strings; so replaced DOMStringList with DOMString[] / sequence<DOMString>

Let's see what the bots have to say...

[Benjamin Poulain]

Does this have any impact on binding's performance?

[Joshua Bell]

(In reply to comment #12)
> Does this have any impact on binding's performance?

The only places using DOMString[] were Source/WebCore/bindings/scripts/test/TestObj.idl and Source/WebCore/testing/Internals.idl

Nothing else should be affected.

[Benjamin Poulain]

(In reply to comment #13)
> (In reply to comment #12)
> > Does this have any impact on binding's performance?
> 
> The only places using DOMString[] were Source/WebCore/bindings/scripts/test/TestObj.idl and Source/WebCore/testing/Internals.idl
> 
> Nothing else should be affected.

Oh ok, never mind :)

[Adam Barth]

Comment on attachment 182417 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182417&action=review

This is great!  I think we added DOMString[] before we knew what we were doing with sequence and [].

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3963
> +    # FIXME: Should this return false for Sequence and Array types?

In principle, it looks like "yes", but there looks like one caller (the first one in the caller textually) who seems to expect array types to return true here.

[WebKit Review Bot]

Comment on attachment 182417 [details]
Patch

Clearing flags on attachment: 182417

Committed r139641: <http://trac.webkit.org/changeset/139641>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.