[webkit-reviews] review granted: [Bug 7296] JavaScript error not thrown when trying to set a CSS property to an invalid value : [Attachment 11888] proposed fix

[bugzilla-request-daemon at macosforge.org]

Darin Adler <darin at apple.com> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 7296: JavaScript error not thrown when trying to set a CSS property to an
invalid value
http://bugs.webkit.org/show_bug.cgi?id=7296

Attachment 11888: proposed fix
http://bugs.webkit.org/attachment.cgi?id=11888&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
The mechanics of the change look good. But the bug report has conflicting
information about Firefox's behavior.

Generally, fixes that cause us to raise more exceptions have been problematic
in the past -- it's one of the easiest ways to stop sites from working.

+      if (Document* doc =
static_cast<CSSStyleSheet*>(styleDecl.stylesheet())->doc())

No need to nil-check the document. A document can have a nil frame, but a
stylesheet can't have a nil document.

+	     styleDecl.removeProperty(prop, exception);
+	     if (!exception) {
+	       ExceptionCode ec = 0;
+	       styleDecl.setProperty(prop, propValue, ec);
+	       return;
+	     }

I think it's confusing here to have one variable named exception and another
named ec. The difference is that the second one is ignored, and so perhaps its
name should reflect that.

+    // When replacing an existing property value, this moves the property to
the end of the list.
+    // Firefox preserves the position, and MSIE moves the property to the
beginning.

Should we have a test case or bug report for this?

I'm going to say review+ even though I think this is risky for compatibility.