Bug 13919 – Autogenerate the JS bindings for the CSSRule and its subclasses

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Sam Weinig]

Patch forthcoming.

[Sam Weinig]

Created attachment 14778 [details]
patch

The only part of this patch that I am not sure of is in CSSRule.idl where I special cased JS for the cssText attribute.  The hand rolled bindings had the attribute as readonly, but the spec and ObjC bindings don't have this restriction.  On the otherhand, we don't really support it in the implementation so the issue is rather insignificant.  A similar issue (but in the other direction) is CSSCharsetRule.idl.

[Sam Weinig]

One note, this patch fixes a previously failing case in fast/dom/wrapper-classes.html

[David Kilzer (:ddkilzer)]

*** Bug 13918 has been marked as a duplicate of this bug. ***

[Darin Adler]

Comment on attachment 14778 [details]
patch

         // 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.

[Sam Weinig]

Created attachment 14785 [details]
Updated Patch

Patch addresses Darin's concerns and adds new files to WebCore.pro so as not to break the Qt and Gdk builds again.

[Sam Weinig]

Urk, +void CSSPageRule::setSelectorText(const String& /*selectorText*/, ExceptionCode& /*ex*/)
the /*ex*/ should be a /*ec*/ 

[Darin Adler]

Comment on attachment 14785 [details]
Updated Patch

     // Not part of the CSSOM

What does that comment mean?

r=me

[Sam Weinig]

Landed in r21896.