Bug 10685 – ObjC DOM should have no unnamed parameters

• Component: WebKit API
• Status: RESOLVED FIXED

[Timothy Hatcher]

This would improve the look of the methods with more than one parameter. Now that we are auto-generating the DOM bindings this is easy to add. We will want to generate the old style for methods that already exist in our API ad generate the new version.

[Timothy Hatcher]

Created attachment 10358 [details]
Implements this and other small tweaks

- The ObjC code generation script now outputs parameter prefixes
  for methods that have more than 1 parameter. This prefix is simply
  the parameter name. Some parameter names have been changed in the IDL
  files to produce good prefixes. Please don't change existing
  parameter names, or the ObjC API will change.

- A backwards compatible version of the method is placed in a
  category with a deprecation marco for 10.5 and later. This step only
  happens if the IDL extended attribute "OldStyleObjC" is defined.
  All new functions in IDL should not get this attribute.

- Moved RemoveExcludedAttributesAndFunctions to CodeGenerator.pm
  so it can be shared between the two generators.
  
- The ObjC generate will now die before starting if the platform is
  not Mac OS. This is determined by the "OS" env variable Xcode sets.
  
- Many cleanup tweaks in CodeGeneratorObjC.pm.

- Removed IDL and CSS files from the project's resource copy phase,
  these do no need to be in WebCore's resources. 

* WebCore.xcodeproj/project.pbxproj
* bindings/scripts/CodeGenerator.pm
* bindings/scripts/CodeGeneratorJS.pm
* bindings/scripts/CodeGeneratorObjC.pm
* css/CSSPrimitiveValue.idl
* dom/CharacterData.idl
* dom/DOMImplementation.idl
* dom/Document.idl
* dom/Element.idl
* dom/KeyboardEvent.idl
* dom/MouseEvent.idl
* dom/MutationEvent.idl
* dom/NamedNodeMap.idl
* dom/UIEvent.idl
* html/HTMLCanvasElement.idl
* html/HTMLSelectElement.idl
* ksvg2/bindings/idl/svg/SVGLengthList.idl
* ksvg2/bindings/idl/svg/SVGNumberList.idl
* ksvg2/bindings/idl/svg/SVGPointList.idl
* ksvg2/bindings/idl/svg/SVGStringList.idl
* ksvg2/bindings/idl/svg/SVGTextContentElement.idl
* ksvg2/bindings/idl/svg/SVGTransformList.idl
* ksvg2/svg/SVGSVGElement.idl
* page/DOMWindow.idl

[Timothy Hatcher]

That patch is missing one part, the deprecation macro.

Here it is:

$deprecatedFunctionSig =~ s/;\n$/ DEPRECATED_IN_MAC_OS_X_VERSION_10_5_AND_LATER;\n/;

It tacks it on the end in the header. I will land the patch plus that line.

Here is an example:

@interface DOMElement : DOMNode
- (NSString *)tagName;
- (DOMCSSStyleDeclaration *)style;
- (int)offsetLeft;
- (int)offsetTop;
- (int)offsetWidth;
- (int)offsetHeight;
- (DOMElement *)offsetParent;
- (int)clientWidth;
- (int)clientHeight;
- (int)scrollLeft;
- (void)setScrollLeft:(int)newScrollLeft;
- (int)scrollTop;
- (void)setScrollTop:(int)newScrollTop;
- (int)scrollWidth;
- (int)scrollHeight;

- (NSString *)getAttribute:(NSString *)name;
- (void)setAttribute:(NSString *)name value:(NSString *)value;
- (void)removeAttribute:(NSString *)name;
- (DOMAttr *)getAttributeNode:(NSString *)name;
- (DOMAttr *)setAttributeNode:(DOMAttr *)newAttr;
- (DOMAttr *)removeAttributeNode:(DOMAttr *)oldAttr;
- (DOMNodeList *)getElementsByTagName:(NSString *)name;
- (NSString *)getAttributeNS:(NSString *)namespaceURI localName:(NSString *)localName;
- (void)setAttributeNS:(NSString *)namespaceURI qualifiedName:(NSString *)qualifiedName value:(NSString *)value;
- (void)removeAttributeNS:(NSString *)namespaceURI localName:(NSString *)localName;
- (DOMNodeList *)getElementsByTagNameNS:(NSString *)namespaceURI localName:(NSString *)localName;
- (DOMAttr *)getAttributeNodeNS:(NSString *)namespaceURI localName:(NSString *)localName;
- (DOMAttr *)setAttributeNodeNS:(DOMAttr *)newAttr;
- (BOOL)hasAttribute:(NSString *)name;
- (BOOL)hasAttributeNS:(NSString *)namespaceURI localName:(NSString *)localName;
- (void)focus;
- (void)blur;
- (void)scrollIntoView:(BOOL)alignWithTop;
- (void)scrollIntoViewIfNeeded:(BOOL)centerIfNeeded;
@end

@interface DOMElement (DOMElementDeprecated)
- (void)setAttribute:(NSString *)name :(NSString *)value DEPRECATED_IN_MAC_OS_X_VERSION_10_5_AND_LATER;
- (NSString *)getAttributeNS:(NSString *)namespaceURI :(NSString *)localName DEPRECATED_IN_MAC_OS_X_VERSION_10_5_AND_LATER;
- (void)setAttributeNS:(NSString *)namespaceURI :(NSString *)qualifiedName :(NSString *)value DEPRECATED_IN_MAC_OS_X_VERSION_10_5_AND_LATER;
- (void)removeAttributeNS:(NSString *)namespaceURI :(NSString *)localName DEPRECATED_IN_MAC_OS_X_VERSION_10_5_AND_LATER;
- (DOMNodeList *)getElementsByTagNameNS:(NSString *)namespaceURI :(NSString *)localName DEPRECATED_IN_MAC_OS_X_VERSION_10_5_AND_LATER;
- (DOMAttr *)getAttributeNodeNS:(NSString *)namespaceURI :(NSString *)localName DEPRECATED_IN_MAC_OS_X_VERSION_10_5_AND_LATER;
- (BOOL)hasAttributeNS:(NSString *)namespaceURI :(NSString *)localName DEPRECATED_IN_MAC_OS_X_VERSION_10_5_AND_LATER;
@end

[Eric Seidel (no email)]

Comment on attachment 10358 [details]
Implements this and other small tweaks

I mostly just have little nit comments:

I think this restriction could be eased using a ObjC specific keyword, if necessary:
"Please don't change existing parameter names, or the ObjC API will change."  Not something we need now, but could keep in mind for the future if another language needs specific parameter names.


This seems a little confusing:
+    die "Skipping ObjC code generation, not needed on platforms other than Mac OS." unless $ENV{"OS"} and $ENV{"OS"} eq "MACOS";

you're not actually skipping, you're dieing.  Maybe it should say.  "ObjC code generation is not supported on your platform." or similar.

Personally I found the old style more readable:

-    if ($type eq "Counter"
-            or $type eq "MediaList"
-            or $type eq "CSSStyleSheet") {
+    if ($type eq "Counter" or $type eq "MediaList" or $type eq "CSSStyleSheet") {
(actually in a couple places).

Another style comment:
+    $implIncludes{"DOMHTMLInternal.h"} = 1 if $type =~ /^HTML/;
One of the things we have started to do in our Ruby code (which also uses one-line ifs all over) it to place an extra space between the code line and the if, I find this really helps readability.  Perhaps WebKit would like to consider doing the same.

Eventually this will not be true:
+    die "A class can't have more than one parent." if @{$dataNode->parents} > 1;
The others will just be categories.  WildFox just fixed this for JS.  You might also consider saying "... more than one parent for ObjC."

I'm surprised the double-check is necessary here:
+            if ($ENV{"MACOSX_DEPLOYMENT_TARGET"} and $ENV{"MACOSX_DEPLOYMENT_TARGET"} >= 10.5) {

Why remove this?
-            if ($hasGetterException) {
-                die "We should not have any getter exceptions yet!";
-            }
-            
I don't see it used anywhere, but maybe it was already being used in code before.

All in all, this looks fine.  I think at least darin should look over the API changes.  I'm not sure simple argument names make the most sense for Obj-C argument labels.  AppKit often uses prepositions with argument names, like "to" or "with", etc.

I'm going to leave this ? for now.

[Timothy Hatcher]

> I think this restriction could be eased using a ObjC specific keyword, if
> necessary:
> "Please don't change existing parameter names, or the ObjC API will change." 
> Not something we need now, but could keep in mind for the future if another
> language needs specific parameter names.

Yeah, I should add support for that now.

> This seems a little confusing:
> +    die "Skipping ObjC code generation, not needed on platforms other than Mac
> OS." unless $ENV{"OS"} and $ENV{"OS"} eq "MACOS";
> 
> you're not actually skipping, you're dieing.  Maybe it should say.  "ObjC code
> generation is not supported on your platform." or similar.

You are right, I will fix this.

> Personally I found the old style more readable:
> 
> -    if ($type eq "Counter"
> -            or $type eq "MediaList"
> -            or $type eq "CSSStyleSheet") {
> +    if ($type eq "Counter" or $type eq "MediaList" or $type eq
> "CSSStyleSheet") {
> (actually in a couple places).

I only un-wraped ifs that could fit on one line easily, arguably easier to read.

> Eventually this will not be true:
> +    die "A class can't have more than one parent." if @{$dataNode->parents} >
> 1;
> The others will just be categories.  WildFox just fixed this for JS.  You might
> also consider saying "... more than one parent for ObjC."

We don't support SVG bindings yet.

> I'm surprised the double-check is necessary here:
> +            if ($ENV{"MACOSX_DEPLOYMENT_TARGET"} and
> $ENV{"MACOSX_DEPLOYMENT_TARGET"} >= 10.5) {

The double check is needed to prevent a warning about comparing an undefined value.

> Why remove this?
> -            if ($hasGetterException) {
> -                die "We should not have any getter exceptions yet!";
> -            }
> -            
> I don't see it used anywhere, but maybe it was already being used in code
> before.

Sam added support for $hasGetterException a while ago but this die was never removed.

> All in all, this looks fine.  I think at least darin should look over the API
> changes.  I'm not sure simple argument names make the most sense for Obj-C
> argument labels.  AppKit often uses prepositions with argument names, like "to"
> or "with", etc.

This is something Darin and I talked about offline before I worked on the patch. This approch makes the ObjC binding easy to maintain and consistent. We could add IDL attributes fro each prefix, but I don't see any good way to generate these prefixes reliably with prepositions sprinkled in without IDL attributes.

[Timothy Hatcher]

Created attachment 10365 [details]
Revised per Eric's comments

- The ObjC code generation script now outputs parameter prefixes
  for methods that have more than 1 parameter. This prefix is simply
  the parameter name. Some parameter names have been changed in the IDL
  files to produce better prefixes. If an extended attribute of "ObjCPrefix"
  exists on a parameter we will use that for the prefix.

- A backwards compatible version of the method is placed in a
  category with a deprecation marco for 10.5 and later. This step only
  happens if the IDL extended attribute "OldStyleObjC" is defined.
  All new functions in IDL should not get this attribute.

- Made the $interfaceMethodSelector regex in IDLStructure.pm allow "=" so 
  parameter extended attributes can have values.

- Moved RemoveExcludedAttributesAndFunctions to CodeGenerator.pm
  so it can be shared between the two generators.
  
- Removed the die when we encounter a getter that uses exceptions.
  Sam Weinig added support for this in an earlier change.

- Check if $ENV{"MACOSX_DEPLOYMENT_TARGET"} is defined before we compare.
  This caused a perl warning when generating on other platforms.

- The ObjC generation will not happen on platforms other than Mac OS.
  This is determined by the "OS" env variable Xcode sets. This check
  is in the DerivedSources.make.

- Added a way to skip generation if the constructor of the specific
  generator returns undef. Not used yet.

- Many cleanup tweaks in CodeGeneratorObjC.pm.

- Removed IDL and CSS files from the project's resource copy phase,
  these do no need to be in WebCore's resources.

[Darin Adler]

Comment on attachment 10365 [details]
Revised per Eric's comments

r=me

[Timothy Hatcher]

Landed in r16201.