Bug 106506 - Bindings: Remove special cases for DOMString[]
Summary: Bindings: Remove special cases for DOMString[]
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joshua Bell
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-09 16:59 PST by Joshua Bell
Modified: 2013-01-14 12:26 PST (History)
7 users (show)

See Also:


Attachments
Patch (16.20 KB, patch)
2013-01-09 17:05 PST, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (18.81 KB, patch)
2013-01-10 10:33 PST, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (28.16 KB, patch)
2013-01-11 14:27 PST, Joshua Bell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joshua Bell 2013-01-09 16:59:43 PST
Bindings: Remove special cases for DOMString[]
Comment 1 Joshua Bell 2013-01-09 17:05:00 PST
Created attachment 182017 [details]
Patch
Comment 2 Joshua Bell 2013-01-09 17:05:50 PST
I'm expecting some bot failures here, since I ran only some of the tests locally and they didn't exercise everything in Internals.
Comment 3 Kentaro Hara 2013-01-09 17:22:02 PST
Comment on attachment 182017 [details]
Patch

LGTM. Thanks for the clean up.
Comment 4 Build Bot 2013-01-09 17:35:33 PST
Comment on attachment 182017 [details]
Patch

Attachment 182017 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15771734
Comment 5 WebKit Review Bot 2013-01-09 19:00:33 PST
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
Comment 6 Build Bot 2013-01-09 21:36:28 PST
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
Comment 7 Build Bot 2013-01-09 22:33:50 PST
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
Comment 8 Joshua Bell 2013-01-10 10:33:49 PST
Created attachment 182166 [details]
Patch
Comment 9 Joshua Bell 2013-01-10 10:36:25 PST
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.
Comment 10 Joshua Bell 2013-01-11 14:27:27 PST
Created attachment 182417 [details]
Patch
Comment 11 Joshua Bell 2013-01-11 14:30:14 PST
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...
Comment 12 Benjamin Poulain 2013-01-11 15:18:07 PST
Does this have any impact on binding's performance?
Comment 13 Joshua Bell 2013-01-11 15:20:59 PST
(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.
Comment 14 Benjamin Poulain 2013-01-11 15:21:31 PST
(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 :)
Comment 15 Adam Barth 2013-01-11 16:56:52 PST
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.
Comment 16 WebKit Review Bot 2013-01-14 12:26:53 PST
Comment on attachment 182417 [details]
Patch

Clearing flags on attachment: 182417

Committed r139641: <http://trac.webkit.org/changeset/139641>
Comment 17 WebKit Review Bot 2013-01-14 12:26:57 PST
All reviewed patches have been landed.  Closing bug.