[webkit-reviews] review denied: [Bug 13919] Autogenerate the JS bindings for the CSSRule and its subclasses : [Attachment 14778] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Sam Weinig <sam at webkit.org>'s request
for review:
Bug 13919: Autogenerate the JS bindings for the CSSRule and its subclasses
http://bugs.webkit.org/show_bug.cgi?id=13919

Attachment 14778: patch
http://bugs.webkit.org/attachment.cgi?id=14778&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
	 // FIXME: the DOM spec says that this attribute
	 // should not be readonly.  The correct declaration
	 // is commented out below.

This comment is no longer accurate, and so it should be removed.

+		  attribute [ConvertNullStringTo=Null, ConvertNullToNullString]
DOMString encoding
+		      /*setter raises(DOMException);*/

You can avoid commenting out the DOMException part by adding an ExceptionCode&
parameter to setEncoding.

Index: WebCore/css/CSSPageRule.h
===================================================================
--- WebCore/css/CSSPageRule.h	(revision 21870)
+++ WebCore/css/CSSPageRule.h	(working copy)
@@ -26,6 +26,7 @@
 
 #include "CSSRule.h"
 #include <wtf/RefPtr.h>
+#include "PlatformString.h"

It's not correct to include PlatformString.h in this header. You need
PlatformString.h to call selectorText(), setSelectorText(), or cssText() but
not to process the header. If there's a problem in the auto-generated code that
should be resolved in the auto-generation, not here.

		  // FIXME: DOM spec says that this can raise an
		  // exception on setting.
-		  attribute [ConvertNullToNullString] DOMString       
selectorText
+		  attribute [ConvertNullStringTo=Null, ConvertNullToNullString]
DOMString selectorText
		      /*setter raises(DOMException)*/;

		  // FIXME: DOM spec says that this can raise an
		  // exception on setting.
-		  attribute [ConvertNullToNullString] DOMString 	  
selectorText
+		  attribute [ConvertNullStringTo=Null, ConvertNullToNullString]
DOMString	     selectorText
		      /*setter raises(DOMException)*/;

You can resolve these FIXMEs by adding an ExceptionCode& parameter to
setSelectorText.

-		  // FIXME: DOM spec says that this can raise an
+		  // FIXME: DOM spec says that this can be set and can raise an

		  // exception on setting.
+#if defined(LANGUAGE_JAVASCRIPT)
+	 readonly attribute [ConvertNullToNullString] DOMString        cssText;

+#else
		  attribute [ConvertNullToNullString] DOMString        cssText
		      /*setter raises (DOMException)*/;
+#endif

Should add a setCSSText function with an ExceptionCode& parameter instead of
having an ifdef in the IDL.

Otherwise looks great.

I'm going to say review- because I want you to address at least some of these.