Bug 11876 – CSSMediaRule functions insertRule and deleteRule don't raise exceptions

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Sam Weinig]

According to the spec (http://www.w3.org/TR/2000/REC-DOM-Level-2-Style-20001113/css.html#CSS-CSSMediaRule)

- the insertRule(rule, index) method can raise the following exceptions:
HIERARCHY_REQUEST_ERR: Raised if the rule cannot be inserted at the specified index, e.g., if an @import rule is inserted after a standard rule set or other at-rule.
INDEX_SIZE_ERR: Raised if the specified index is not a valid insertion point.
NO_MODIFICATION_ALLOWED_ERR: Raised if this media rule is readonly.
SYNTAX_ERR: Raised if the specified rule has a syntax error and is unparsable.

- the deleteRule(index) method can raise the following exceptions:
INDEX_SIZE_ERR: Raised if the specified index does not correspond to a rule in the media rule list.
NO_MODIFICATION_ALLOWED_ERR: Raised if this media rule is readonly.

We never raise exceptions for either method.

[Sam Weinig]

Upon further testing, it looks as if the two methods are not even being called.  I'll do further testing to make sure.

[Sam Weinig]

Created attachment 11973 [details]
patch

This patch adds support for all the specified exceptions but the HIERARCHY_REQUEST_ERR for insertRule (I added a FIXME for that case).

[Geoffrey Garen]

Comment on attachment 11973 [details]
patch

r=me

I'm still a little unclear on what a "valid insertion point" is.

[David Kilzer (:ddkilzer)]

Comment on attachment 11973 [details]
patch

>Index: WebCore/ChangeLog
>===================================================================
>--- WebCore/ChangeLog	(revision 18395)
>+++ WebCore/ChangeLog	(working copy)
>@@ -1,3 +1,21 @@
>[phone]  Sam Weinig  <[e-mail]>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Patch for http://bugs.webkit.org/show_bug.cgi?id=11876
>+        CSSMediaRule functions insertRule and deleteRule don't raise exceptions
>+
>+        Test added:
>+        * fast/dom/css-mediarule-functions.html
>+
>+        * bindings/js/kjs_css.cpp:
>+        (KJS::DOMCSSRuleFunc::callAsFunction):
>+        * css/CSSMediaRule.cpp: add exception handling
>+        (WebCore::CSSMediaRule::insertRule):
>+        (WebCore::CSSMediaRule::deleteRule):
>+        * css/CSSMediaRule.h: add ExceptionCode arguments
>+        * css/CSSMediaRule.idl: un-comment exections

Typo: "exections" should be "exceptions"

[Sam Weinig]

Created attachment 11988 [details]
newer testcase

I've updated the testcase to test more cases and test for HIERARCHY_REQUEST_ERR's for insertRule.

[Sam Weinig]

Created attachment 11989 [details]
updated patch

Updating patch to include handeling for HIERARCHY_REQUEST_ERR, updated test and typo fixes.  Currently, the test includes 2 FAILS that should according to the spec, should be PASSes.  I'm not sure whether I should leave them in or remove them.

Comment on attachment 11989 [details]
updated patch

I find expected results that say "FAIL" quite confusing, since they always leave room for doubt (e.g. maybe they were checked in as part of some big change that affected many tests). It's good to have the current behavior documented, but I argue that the test should be written to expect the current behavior (and say PASS) with a comment that it does not follow the spec. Then if WebKit's behavior changes, it is easy to determine that the new failure is actually a progression and fix the test accordingly.

[Sam Weinig]

Created attachment 11990 [details]
more updated patch

Fixed.

[Geoffrey Garen]

Comment on attachment 11990 [details]
more updated patch

Seems to address Mitz's comment.

[Sam Weinig]

Landed in r18404.