Bindings: Remove special cases for DOMString[]
Created attachment 182017 [details] Patch
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 on attachment 182017 [details] Patch LGTM. Thanks for the clean up.
Comment on attachment 182017 [details] Patch Attachment 182017 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15771734
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 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 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
Created attachment 182166 [details] Patch
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.
Created attachment 182417 [details] Patch
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...
Does this have any impact on binding's performance?
(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.
(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 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 on attachment 182417 [details] Patch Clearing flags on attachment: 182417 Committed r139641: <http://trac.webkit.org/changeset/139641>
All reviewed patches have been landed. Closing bug.