Bug 16693 - Acid3 expects WebKit to raise exceptions for invalid qualified names (affects Acid3 test 25)
Summary: Acid3 expects WebKit to raise exceptions for invalid qualified names (affects...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Eric Seidel (no email)
URL: http://www.w3.org/TR/REC-xml-names/#n...
Keywords:
: 17074 (view as bug list)
Depends on: 16833
Blocks: Acid3
  Show dependency treegraph
 
Reported: 2008-01-01 01:32 PST by Eric Seidel (no email)
Modified: 2019-02-06 09:04 PST (History)
2 users (show)

See Also:


Attachments
Test case for createDocumentType (2.45 KB, patch)
2008-01-01 18:07 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
first attempt (9.59 KB, patch)
2008-02-09 12:01 PST, Rob Buis
eric: review-
Details | Formatted Diff | Diff
Unify handling of NAMESPACE_ERR and fix Acid3 test 25 (16.51 KB, patch)
2008-03-22 02:20 PDT, Eric Seidel (no email)
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2008-01-01 01:32:34 PST
Acid3 expects WebKit to raise exceptions for invalid qualified names

At least qualified names passed to:
document.implementation.createDocumentType

The bug is in DOMImplementation.cpp:

// FIXME: An implementation of this is still waiting for me to understand the distinction between
// a "malformed" qualified name and one with bad characters in it. For example, is a second colon
// an illegal character or a malformed qualified name? This will determine both what parameters
// this function needs to take and exactly what it will do. Should also be exported so that
// Element can use it too.
static bool qualifiedNameIsMalformed(const String&)
{
    return false;
}

The qualified name parsing rules are actually pretty easy:

http://www.w3.org/TR/REC-xml-names/#ns-qualnames

I think we can fix this to make it spec-compliant w/o raising any compatibility concerns.
Comment 1 Eric Seidel (no email) 2008-01-01 18:07:54 PST
Created attachment 18230 [details]
Test case for createDocumentType

 .../dom/DOMImplementation/createDocumentType.html  |   13 +++++++++++++
 .../dom/DOMImplementation/resources/TEMPLATE.html  |   13 +++++++++++++
 .../resources/createDocumentType.js                |   14 ++++++++++++++
 3 files changed, 40 insertions(+), 0 deletions(-)
Comment 2 Eric Seidel (no email) 2008-01-01 18:09:03 PST
A more complicated test case could be created. But the one I attached at least shows the bug.  We should probably actually check the type of exception thrown, but I didn't see a good should* function to check that.

Firefox passes all of the subtests in this test case.
Comment 3 Eric Seidel (no email) 2008-01-29 14:56:58 PST
*** Bug 17074 has been marked as a duplicate of this bug. ***
Comment 4 Rob Buis 2008-02-09 12:01:00 PST
Created attachment 19020 [details]
first attempt

This patch incorporates and passes Erics testcase as well as the acid3 test.
Note that in most cases the check could be done inside parseQualifiedName too, though createAttributeNS is a bit different here. To pass the tests this suffices though, and there are no regressions in the current layout tests.
Cheers,

Rob.
Comment 5 Eric Seidel (no email) 2008-02-09 13:26:49 PST
Comment on attachment 19020 [details]
first attempt

Looks like you have tabs in Document.cpp

Document:: functions don't need to use Document:: to reference static methods in the same class.

Since you're changing method behavior in Element, you will need to add tests for the changed behavior.  (Similar to what was added by bug 16833.)  One probably which will probably stop this bug as well is that createElement calls createElementNS (caused the regression that caused bug 16833 to get rolled out).  Likewise setAttribute might be calling setAtributeNS.  That means that this patch would not only need to test createElementNS and setAttributeNS, but also createElement and setAttribute (it's the lack of testing of those two which is currently blocking bug 16833 from completion).

r- w/o more test cases to prove this is correct.
Comment 6 Eric Seidel (no email) 2008-02-09 13:27:18 PST
Comment on attachment 19020 [details]
first attempt

(Thank you for the patch btw.)  It's always great to see you pop out of the woodwork and throw up a patch here and there. :)
Comment 7 Eric Seidel (no email) 2008-03-22 02:20:11 PDT
Created attachment 19964 [details]
Unify handling of NAMESPACE_ERR and fix Acid3 test 25

 .../createAttributeNS-namespace-err-expected.txt   |    8 +-
 .../createElementNS-namespace-err-expected.txt     |    8 +-
 WebCore/dom/DOMImplementation.cpp                  |   77 ++++----------------
 WebCore/dom/DOMImplementation.idl                  |    6 +-
 WebCore/dom/Document.cpp                           |   51 ++++++++------
 WebCore/dom/Document.h                             |    9 ++-
 WebCore/dom/Element.cpp                            |    8 +-
 WebCore/editing/FormatBlockCommand.cpp             |    9 ++-
 8 files changed, 72 insertions(+), 104 deletions(-)
Comment 8 Maciej Stachowiak 2008-03-22 02:21:48 PDT
Comment on attachment 19964 [details]
Unify handling of NAMESPACE_ERR and fix Acid3 test 25

r=me
Comment 9 Eric Seidel (no email) 2008-03-22 02:52:11 PDT
r31231
Comment 10 Eric Seidel (no email) 2008-03-22 02:52:33 PDT
r31231
Comment 11 Lucas Forschler 2019-02-06 09:04:04 PST
Mass moving XML DOM bugs to the "DOM" Component.