The Path IDL interface is currently called DOMPath and the file DOMPath.idl. We currently use the [InterfaceName] WebKit-IDL extended attribute to make sure the interface is exposed as "Path" on JavaScript side. We usually use the same name as in JavaScript for the Web IDL and the [ImplementedAs] IDL extended attribute is used to map it to another name on C++ side. This is at least the case of IDL operations and attributes. For interfaces, we currently do the opposite, which is confusing / inconsistent. We should consistently use [ImplementedAs] for interfaces for consistency with operations / attributes and deprecate [InterfaceName]. This patch will add support [ImplementedAs] extended attribute support to the JSC bindings generator and rename DOMPath to Path. This is an initial step towards removing [InterfaceName] extended attribute. Other interfaces using [InterfaceName] will be corrected in a follow up patch. Corresponding Blink patch: https://src.chromium.org/viewvc/blink?view=rev&revision=150154
Created attachment 201460 [details] Patch
Attachment 201460 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/DerivedSources.cpp', u'Source/WebCore/DerivedSources.make', u'Source/WebCore/DerivedSources.pri', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/WebCore.vcproj/WebCore.vcproj', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/bindings/scripts/CodeGeneratorJS.pm', u'Source/WebCore/bindings/scripts/test/CPP/WebDOMTestInterfaceImplementedAs.cpp', u'Source/WebCore/bindings/scripts/test/CPP/WebDOMTestInterfaceImplementedAs.h', u'Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterfaceImplementedAs.cpp', u'Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterfaceImplementedAs.h', u'Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterfaceImplementedAsPrivate.h', u'Source/WebCore/bindings/scripts/test/JS/JSTestCallback.h', u'Source/WebCore/bindings/scripts/test/JS/JSTestInterfaceImplementedAs.cpp', u'Source/WebCore/bindings/scripts/test/JS/JSTestInterfaceImplementedAs.h', u'Source/WebCore/bindings/scripts/test/ObjC/DOMTestCallback.h', u'Source/WebCore/bindings/scripts/test/ObjC/DOMTestCallback.mm', u'Source/WebCore/bindings/scripts/test/ObjC/DOMTestInterfaceImplementedAs.h', u'Source/WebCore/bindings/scripts/test/ObjC/DOMTestInterfaceImplementedAs.mm', u'Source/WebCore/bindings/scripts/test/ObjC/DOMTestInterfaceImplementedAsInternal.h', u'Source/WebCore/bindings/scripts/test/TestCallback.idl', u'Source/WebCore/bindings/scripts/test/TestInterfaceImplementedAs.idl', u'Source/WebCore/html/canvas/CanvasRenderingContext2D.idl', u'Source/WebCore/html/canvas/DOMPath.idl', u'Source/WebCore/html/canvas/Path.idl']" exit_code: 1 Source/WebCore/bindings/scripts/test/JS/JSTestInterfaceImplementedAs.cpp:26: "JSTestInterfaceImplementedAs.h" already included at Source/WebCore/bindings/scripts/test/JS/JSTestInterfaceImplementedAs.cpp:22 [build/include] [4] Source/WebCore/bindings/scripts/test/JS/JSTestInterfaceImplementedAs.cpp:277: Do not add platform specific code in WebCore outside of platform. [build/webcore_platform_layering_violation] [5] Source/WebCore/bindings/scripts/test/JS/JSTestInterfaceImplementedAs.cpp:279: Extra space before ( in function call [whitespace/parens] [4] Source/WebCore/bindings/scripts/test/JS/JSTestInterfaceImplementedAs.cpp:288: More than one command on the same line in if [whitespace/parens] [4] Source/WebCore/bindings/scripts/test/JS/JSTestInterfaceImplementedAs.cpp:292: Do not add platform specific code in WebCore outside of platform. [build/webcore_platform_layering_violation] [5] Source/WebCore/bindings/scripts/test/ObjC/DOMTestInterfaceImplementedAsInternal.h:32: Code inside a namespace should not be indented. [whitespace/indent] [4] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterfaceImplementedAs.cpp:146: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterfaceImplementedAs.cpp:148: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterfaceImplementedAs.cpp:149: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterfaceImplementedAs.cpp:150: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterfaceImplementedAs.cpp:151: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterfaceImplementedAs.cpp:153: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterfaceImplementedAs.cpp:155: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterfaceImplementedAs.cpp:156: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterfaceImplementedAs.cpp:157: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterfaceImplementedAs.cpp:158: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/bindings/scripts/test/JS/JSTestInterfaceImplementedAs.h:59: More than one command on the same line in if [whitespace/parens] [4] Source/WebCore/bindings/scripts/test/CPP/WebDOMTestInterfaceImplementedAs.cpp:26: "WebDOMTestInterfaceImplementedAs.h" already included at Source/WebCore/bindings/scripts/test/CPP/WebDOMTestInterfaceImplementedAs.cpp:22 [build/include] [4] Source/WebCore/bindings/scripts/test/CPP/WebDOMTestInterfaceImplementedAs.cpp:28: wtf includes should be <wtf/file.h> instead of "wtf/file.h". [build/include] [4] Total errors found: 19 in 26 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 201468 [details] Patch
Attachment 201468 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/DerivedSources.cpp', u'Source/WebCore/DerivedSources.make', u'Source/WebCore/DerivedSources.pri', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/WebCore.vcproj/WebCore.vcproj', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/bindings/scripts/CodeGeneratorJS.pm', u'Source/WebCore/bindings/scripts/test/CPP/WebDOMTestInterfaceImplementedAs.cpp', u'Source/WebCore/bindings/scripts/test/CPP/WebDOMTestInterfaceImplementedAs.h', u'Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterfaceImplementedAs.cpp', u'Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterfaceImplementedAs.h', u'Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterfaceImplementedAsPrivate.h', u'Source/WebCore/bindings/scripts/test/JS/JSTestCallback.h', u'Source/WebCore/bindings/scripts/test/JS/JSTestInterfaceImplementedAs.cpp', u'Source/WebCore/bindings/scripts/test/JS/JSTestInterfaceImplementedAs.h', u'Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp', u'Source/WebCore/bindings/scripts/test/ObjC/DOMTestCallback.h', u'Source/WebCore/bindings/scripts/test/ObjC/DOMTestCallback.mm', u'Source/WebCore/bindings/scripts/test/ObjC/DOMTestInterfaceImplementedAs.h', u'Source/WebCore/bindings/scripts/test/ObjC/DOMTestInterfaceImplementedAs.mm', u'Source/WebCore/bindings/scripts/test/ObjC/DOMTestInterfaceImplementedAsInternal.h', u'Source/WebCore/bindings/scripts/test/TestCallback.idl', u'Source/WebCore/bindings/scripts/test/TestInterfaceImplementedAs.idl', u'Source/WebCore/html/canvas/CanvasRenderingContext2D.idl', u'Source/WebCore/html/canvas/DOMPath.idl', u'Source/WebCore/html/canvas/Path.idl']" exit_code: 1 Source/WebCore/bindings/scripts/test/JS/JSTestInterfaceImplementedAs.cpp:276: Do not add platform specific code in WebCore outside of platform. [build/webcore_platform_layering_violation] [5] Source/WebCore/bindings/scripts/test/JS/JSTestInterfaceImplementedAs.cpp:278: Extra space before ( in function call [whitespace/parens] [4] Source/WebCore/bindings/scripts/test/JS/JSTestInterfaceImplementedAs.cpp:292: Do not add platform specific code in WebCore outside of platform. [build/webcore_platform_layering_violation] [5] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterfaceImplementedAs.cpp:146: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterfaceImplementedAs.cpp:148: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterfaceImplementedAs.cpp:149: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterfaceImplementedAs.cpp:150: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterfaceImplementedAs.cpp:151: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterfaceImplementedAs.cpp:153: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterfaceImplementedAs.cpp:155: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterfaceImplementedAs.cpp:156: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterfaceImplementedAs.cpp:157: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestInterfaceImplementedAs.cpp:158: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 13 in 26 files If any of these errors are false positives, please file a bug against check-webkit-style.
While it’s nice to have the IDL file match the specification IDL file, I am not sure that outweighs the desire to have the IDL file name match the names of the underlying .h and .cpp files, since the set of them work together to create and export a DOM class. Is this really an important desirable change? What programmers does this help?
I think we chose wrong for interfaces and should go the other direction.
(In reply to comment #6) > I think we chose wrong for interfaces and should go the other direction. So do I understand correctly that you agree with this change now? Basically, we currently have 2 WebKit-specific IDL attributes that solve the same problem (Naming limitations on C++ side): [InterfaceName], [ImplementedAs], but in opposite ways. This is a bit confusing. Also, I believe it is a good idea to limit the number of WebKit-specific IDL attributes. This makes our IDL easier to understand / learn and makes the generator a bit simpler. The reasons I would like to keep [ImplementedAs] and remove [InterfaceName] are: - ImplementedAs is currently used more than InterfaceName (for operations and attributes) - The naming restriction is on C++ side, not on IDL side. Therefore, using the right name on IDL side (and an alternative name on C++ side) makes more sense to me. Therefore, I think [ImplementedAs] solves the problem best. - This is what is being done in Blink and I'm trying to stay compatible as much as possible.
(In reply to comment #7) > (In reply to comment #6) > > I think we chose wrong for interfaces and should go the other direction. > > So do I understand correctly that you agree with this change now? No. The “we” here is the patch, which I think choses wrong. > - The naming restriction is on C++ side, not on IDL side. Therefore, using the right name on IDL side (and an alternative name on C++ side) makes more sense to me. Therefore, I think [ImplementedAs] solves the problem best. I see the IDL as a helper to export the C++ class, not as a platform independent header file that needs to match a specification. I think it is better for the source file to match the .cpp and .h source file, rather than matching the exported class name from the DOM specification. The three source files work together as a team, and should have the same names. Maybe you could ask others in the WebKit project. There’s no urgent need to rush to change this before we have consensus.
Comment on attachment 201468 [details] Patch Attachment 201468 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/491435
Comment on attachment 201468 [details] Patch Attachment 201468 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/483869