We name our interface DOMWindow when it should be called Window: assertTrue(window.Window !== undefined)
That assert was not so useful. Here are better test assertTrue(String(Window) === 'function Window() { [native code] }') // V8 assertTrue(String(Window) === '[object WindowConstructor]') // JSC
Haraken, do you have any suggestions how to do this? Right now the interface name must match the file name as far as I can tell.
Created attachment 128083 [details] WIP
Comment on attachment 128083 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=128083&action=review The approach looks good to me. > Source/WebCore/ChangeLog:8 > + No new tests. (OOPS!) Please add run-bindings-tests. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:-3782 > -sub GetVisibleInterfaceName Maybe you can remove GetVisibleInterfaceName() from CodeGeneratorJS.pm too? > Source/WebCore/bindings/scripts/IDLAttributes.txt:55 > +InterfaceName=* Thanks for using IDLAttributes.txt, which was introduced today:-)
This adds a new extended attribute to the interface section called InterfaceName interface [ InterfaceName=Window ] DOMWindow { } This is kind of backwards. A better outcome would be to use ImplementedAs on the interface and update all the IDL files to use the real interface name as spec'ed. interface [ ImplementedAs=DOMWindow ] Window { } However, making this change would require a lot more work. I'm tempted to say that we should do that since forcing us to use non standard interface names in our IDL files suck. WDYT?
Comment on attachment 128083 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=128083&action=review > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1326 > - my $visibleClassName = GetVisibleClassName($interfaceName); > + my $visibleClassName = $codeGenerator->GetVisibleInterfaceName($dataNode); Should we rename $visibleClassName to $visibleInterfaceName ? Can we remove GetVisibleClassName from CodeGeneratorJS.pm too? > Source/WebCore/bindings/scripts/IDLAttributes.txt:55 > +InterfaceName=* Please add a test to Source/WebCore/bindings/scripts/tests for this IDL attribute. > LayoutTests/fast/dom/Window/atob-btoa-expected.txt:18 > -PASS window.btoa(window) is "W29iamVjdCBET01XaW5kb3dd" > +FAIL window.btoa(window) should be W29iamVjdCBET01XaW5kb3dd. Was W29iamVjdCBXaW5kb3dd. Please update the test so that this says "PASS"
> However, making this change would require a lot more work. I'm tempted to say that we should do that since forcing us to use non standard interface names in our IDL files suck. I would add a FIXME near the GetVisibleInterfaceName function that says something to that effect.
Created attachment 128256 [details] Patch
Comment on attachment 128256 [details] Patch Attachment 128256 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11560582 New failing tests: inspector/profiler/heap-snapshot.html
Created attachment 128276 [details] Patch
Please consider landing the patch in stages. For example, you could land the code generator infrastructure/tests first with attributes for the current whitelisted interfaces and then change DOMWindow in a separate patch. That will make our lives easier if we have to roll back the DOMWindow change do to compat issues.
Thanks Adam. That makes sense. I will split it into 2 patches.
Created attachment 128565 [details] Patch for landing
Comment on attachment 128565 [details] Patch for landing Missing ChangeLogs
Created attachment 128566 [details] Patch for landing
Comment on attachment 128566 [details] Patch for landing Clearing flags on attachment: 128566 Committed r108729: <http://trac.webkit.org/changeset/108729>
All reviewed patches have been landed. Closing bug.