Bug 11876 - CSSMediaRule functions insertRule and deleteRule don't raise exceptions
Summary: CSSMediaRule functions insertRule and deleteRule don't raise exceptions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-12-19 10:39 PST by Sam Weinig
Modified: 2006-12-23 17:39 PST (History)
2 users (show)

See Also:


Attachments
patch (12.38 KB, patch)
2006-12-22 14:25 PST, Sam Weinig
ggaren: review+
Details | Formatted Diff | Diff
newer testcase (6.79 KB, text/html)
2006-12-23 09:12 PST, Sam Weinig
no flags Details
updated patch (16.57 KB, patch)
2006-12-23 09:23 PST, Sam Weinig
no flags Details | Formatted Diff | Diff
more updated patch (16.65 KB, patch)
2006-12-23 12:15 PST, Sam Weinig
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2006-12-19 10:39:56 PST
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.
Comment 1 Sam Weinig 2006-12-19 13:42:58 PST
Upon further testing, it looks as if the two methods are not even being called.  I'll do further testing to make sure.
Comment 2 Sam Weinig 2006-12-22 14:25:42 PST
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).
Comment 3 Geoffrey Garen 2006-12-22 15:10:41 PST
Comment on attachment 11973 [details]
patch

r=me

I'm still a little unclear on what a "valid insertion point" is.
Comment 4 David Kilzer (:ddkilzer) 2006-12-22 16:07:57 PST
Comment on attachment 11973 [details]
patch

>Index: WebCore/ChangeLog
>===================================================================
>--- WebCore/ChangeLog	(revision 18395)
>+++ WebCore/ChangeLog	(working copy)
>@@ -1,3 +1,21 @@
>+2006-12-22  Sam Weinig  <sam@webkit.org>
>+
>+        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"
Comment 5 Sam Weinig 2006-12-23 09:12:28 PST
Created attachment 11988 [details]
newer testcase

I've updated the testcase to test more cases and test for HIERARCHY_REQUEST_ERR's for insertRule.
Comment 6 Sam Weinig 2006-12-23 09:23:28 PST
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 7 mitz 2006-12-23 09:37:07 PST
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.
Comment 8 Sam Weinig 2006-12-23 12:15:42 PST
Created attachment 11990 [details]
more updated patch

Fixed.
Comment 9 Geoffrey Garen 2006-12-23 16:29:52 PST
Comment on attachment 11990 [details]
more updated patch

Seems to address Mitz's comment.
Comment 10 Sam Weinig 2006-12-23 17:39:19 PST
Landed in r18404.