Bug 8563 - [DOMHTMLDocument doctype] always returning nil.
Summary: [DOMHTMLDocument doctype] always returning nil.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-04-24 01:47 PDT by Matt Gough
Modified: 2006-05-05 12:31 PDT (History)
2 users (show)

See Also:


Attachments
Accessing the doctype when there is one declared (733 bytes, text/html)
2006-04-24 03:02 PDT, Matt Gough
no flags Details
Accessing the doctype when there is not one declared (662 bytes, text/html)
2006-04-24 03:05 PDT, Matt Gough
no flags Details
Patch for this bug (1.81 KB, patch)
2006-04-24 07:14 PDT, Matt Gough
darin: review-
Details | Formatted Diff | Diff
Improved Patch (2.75 KB, patch)
2006-04-26 09:21 PDT, Matt Gough
darin: review-
Details | Formatted Diff | Diff
Patch (4.55 KB, patch)
2006-05-02 02:56 PDT, Matt Gough
darin: review+
Details | Formatted Diff | Diff
Layout test (1.74 KB, text/html)
2006-05-02 02:57 PDT, Matt Gough
no flags Details
Layout test expected result (189 bytes, text/plain)
2006-05-02 02:58 PDT, Matt Gough
no flags Details
Patch including test files (7.34 KB, patch)
2006-05-03 01:01 PDT, Matt Gough
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Gough 2006-04-24 01:47:14 PDT
Sometime since 418, the implementation of HTMLDocument::doctype was changed to:

DocumentType *HTMLDocument::doctype() const
{
    // According to a comment in dom_doc.cpp, doctype is null for HTML documents.
    return 0;
}


Firstly, where is dom_doc.cpp? It doesn't seem a part of WebKit/Core.

What is the comment and why is it deemed correct that doctype should be null for HTML documents?

Why do I care:

Well, I am writing some code to take a DOM (or portions thereof) and convert it back into a fairly self-contained HTML file. Without being able to reconstitute the correct <!DOCTYPE ...> I have no way of ensuring that quirks mode is set correctly when displaying the resulting html.
Comment 1 Matt Gough 2006-04-24 03:02:49 PDT
Created attachment 7938 [details]
Accessing the doctype when there is one declared

The following Mac browsers 'pass' this test -Firefox 1.5.0.1, Opera 8.54.2200 and Internet Explorer 5.2.3.
WebKit-based ones fail it.
Comment 2 Matt Gough 2006-04-24 03:05:00 PDT
Created attachment 7939 [details]
Accessing the doctype when there is not one declared

All browser passed this test, including WebKit-based ones
Comment 3 Matt Gough 2006-04-24 03:14:59 PDT
My original assertion that this bug did not affect released versions of WebKit is wrong. It fails in 417.9.2
Comment 4 Matt Gough 2006-04-24 03:21:07 PDT
Ignore the 417.9.2 version mentioned above. It should have been 418.
Comment 5 Matt Gough 2006-04-24 07:14:13 PDT
Created attachment 7940 [details]
Patch for this bug

This patch:
1. Removes DocumentType *HTMLDocument::doctype() const from HTMLDocument.h and cpp.
2. Changes the comment in Document.h to say that docType() may return 0.
3. Changes LayoutTests/dom/html/level1/core/hc_documentgetdoctype.js to do a case insensitive compare of docTypeName against 'html' (Since HTML is the name specified in HTMLDocument::determineParseMode when creating the docType for HTML.) Not sure whether this is the correct approach, or to just make HTMLDocument::determineParseMode use 'html'
Comment 6 Alexey Proskuryakov 2006-04-24 10:50:08 PDT
WinIE 6 fails the test :-(

"FAILED - doctype is null even though there is a <!DOCTYPE ...> declaration 
Comment 7 Darin Adler 2006-04-24 20:39:20 PDT
Comment on attachment 7940 [details]
Patch for this bug

We normally do not make changes to tests from the DOM working group without making comments describing what the change is and why we made it, and also reporting the bug in the W3C Bugzilla. So I'm a bit uncomfortable with the change in hc_documentgetdoctype.js.
Comment 8 Matt Gough 2006-04-25 01:36:56 PDT
(In reply to comment #7)
> So I'm a bit uncomfortable with the change in hc_documentgetdoctype.js.
> 

As it stood before my patch that code (in the test) would never have been called anyway (in WebKit)

The problem is caused by the test html file using 'html' but WebKit hard-coding a 'HTML' for the docType. Conceptually, the test is consistent with itself and so should probably not have been changed.

Do you have any objections to passing in a lowercase 'html' to new DocumentType in HTMLDocument::determineParseMode instead:
     setDocType(new DocumentType(this, "html", publicID, systemID));

An even better fix, but one which is far more trouble than it is worth, would be to pass the same version of the name as read in from the <!Doctype ...>, but WebKit has already lost that information by the time the setDocType call is made.

Since html is inherently case-insensitive anyway I see no issue with using 'html' instead and not changing the test.
Comment 9 Darin Adler 2006-04-25 08:03:14 PDT
Comment on attachment 7940 [details]
Patch for this bug

(In reply to comment #8)
> Do you have any objections to passing in a lowercase 'html' to new DocumentType
> in HTMLDocument::determineParseMode instead:
>      setDocType(new DocumentType(this, "html", publicID, systemID));

Sounds fine. We should do this.

> An even better fix, but one which is far more trouble than it is worth, would
> be to pass the same version of the name as read in from the <!Doctype ...>, but
> WebKit has already lost that information by the time the setDocType call is
> made.

I'm not sure why you say this is more trouble than it's worth, though.

We could easily add a fourth return value to the function "parseDocTypeDeclaration" in HTMLDocument.cpp to complement the resultFlags, publicID, and systemID return values.

I think the real issue is not the difficulty of implementation, but the behavior of other browsers. Do those browsers always return the "html" in the same case it was in the <!doctype> or do they always return lowercase "html"? We should match them. If they always return lowercase "html", then this is not a better fix after all.
Comment 10 Matt Gough 2006-04-26 09:21:40 PDT
Created attachment 7981 [details]
Improved Patch

FireFox and Opera both return the docType.name it as it is in the source (i,e HTML, html, Html, hTMl etc). As such this new patch incorporates Darin's suggestion of returning the name out of parseDocTypeDeclaration. This name is then passed in when creating the DocumentType in determineParseMode.
Comment 11 Darin Adler 2006-04-26 15:49:23 PDT
Comment on attachment 7981 [details]
Improved Patch

Looks great! Just needs a change log and a test case, and then it's ready to land.

Marking review- because of the lack of change log and test case, otherwise would be review+.
Comment 12 Matt Gough 2006-04-27 02:24:58 PDT
> Marking review- because of the lack of change log and test case, otherwise
> would be review+.

I couldn't decide if the existing doctype test (hc_documentgetdoctype.html) was enough or not. Should I do one along the lines of the first attachment above?

Dumb as it sounds, but how do I provide a change log? Am I supposed to actually edit the Changelog file and create a patch from that, or just add a comment here? Unfortunately http://webkit.opendarwin.org/coding/contributing.html is a little ambiguous: 'Make sure that your patch includes a ChangeLog entry'
Comment 13 Alexey Proskuryakov 2006-04-27 03:26:44 PDT
When the behavior changes, some test should change from failure to success - if I understood correctly, hc_documentgetdoctype.html doesn't catch this change? I think it would also be useful to test for different capitalizations that you have mentioned (hTMl etc).

Yes, ideally, a patch to ChangeLog should be included in the patch itself. There's a prepare-Changelog script that creates a template (automatically mentioning all modified files and functions!). I run it once in each modified top-level subdirectory, that is, WebCore, LayoutTests etc.
Comment 14 Matt Gough 2006-05-02 02:56:29 PDT
Created attachment 8063 [details]
Patch

Now with ChangeLog entries.

Test files to appear shorty
(Note - I would have though that svn-create-patch would also add my tests to the patch but it didn't. Is it supposed to?)
Comment 15 Matt Gough 2006-05-02 02:57:55 PDT
Created attachment 8064 [details]
Layout test

into fast/doctypes/
Comment 16 Matt Gough 2006-05-02 02:58:29 PDT
Created attachment 8065 [details]
Layout test expected result

into fast/doctypes
Comment 17 mitz 2006-05-02 02:59:35 PDT
(In reply to comment #14)
> (Note - I would have though that svn-create-patch would also add my tests to
> the patch but it didn't. Is it supposed to?)

Yes, but you have to "svn add" them.
Comment 18 Matt Gough 2006-05-02 03:08:00 PDT
> > (Note - I would have though that svn-create-patch would also add my tests to
> > the patch but it didn't. Is it supposed to?)
> 
> Yes, but you have to "svn add" them.

I was assuming that since I am not a committer that I would not be allowed to touch svn.

http://webkit.opendarwin.org/coding/contributing.html doesn't mention what to do with new files.
Comment 19 Darin Adler 2006-05-02 08:44:35 PDT
Comment on attachment 8063 [details]
Patch

r=me.

Note that the new test case is not in the patch and must be "svn add"-ed separately.
Comment 20 Darin Adler 2006-05-02 11:05:39 PDT
(In reply to comment #18)
> http://webkit.opendarwin.org/coding/contributing.html doesn't mention what to
> do with new files.

A patch for the website to correct this would be great!
Comment 21 Matt Gough 2006-05-03 01:01:59 PDT
Created attachment 8090 [details]
Patch including test files
Comment 22 Eric Seidel (no email) 2006-05-03 01:05:56 PDT
Comment on attachment 8090 [details]
Patch including test files

This looks great.  I don't know why the old code worked that way.  Assuming you tested in FireFox and IE to make sure that our behavior agrees with theirs, r=me.

Also, we would need to clean up the changelog conflict markers before landing.
Comment 23 Matt Gough 2006-05-03 01:42:33 PDT
> Assuming you
> tested in FireFox and IE to make sure that our behavior agrees with theirs,
> r=me.

I tested on Opera and Firefox on Mac. No PC so can't check IE

> Also, we would need to clean up the changelog conflict markers before landing.

Does 'we' mean someone else, or do I have to go through the whole patch making rigmarole again :(

Comment 24 Alexey Proskuryakov 2006-05-03 03:42:04 PDT
(In reply to comment #23)
> > Assuming you
> > tested in FireFox and IE to make sure that our behavior agrees with theirs,
> > r=me.

  To reiterate comment 6, WinIE behavior is different. Not sure if this can be considered already OK'd by Darin.

> > Also, we would need to clean up the changelog conflict markers before landing.
> 
> Does 'we' mean someone else, or do I have to go through the whole patch making
> rigmarole again :(

  I'm pretty sure "we" means "the committer" :)
Comment 25 Eric Seidel (no email) 2006-05-03 10:56:46 PDT
Comment on attachment 8090 [details]
Patch including test files

I didn't read the whole history, sorry.  I'm going to leave this one to darin, since he's been working with you on this one.
Comment 26 Darin Adler 2006-05-03 11:17:35 PDT
Comment on attachment 8090 [details]
Patch including test files

Patch looks fine. Yes the committer will have to remove the conflict marker from LayoutTests/ChangeLog.

r=me
Comment 27 Matt Gough 2006-05-05 08:24:03 PDT
(In reply to comment #17)
> (In reply to comment #14)
> > (Note - I would have though that svn-create-patch would also add my tests to
> > the patch but it didn't. Is it supposed to?)
> 
> Yes, but you have to "svn add" them.
> 
OK, so now that my patch has landed update-webkit failed with:
svn: Failed to add file 'LayoutTests/fast/doctypes/005-case-preserving.html': object of the same name already exists

so I removed it and I get:

svn: Failed to add file 'LayoutTests/fast/doctypes/005-case-preserving.html': object of the same name is already scheduled for addition

Once I know what to do I'll be able to update the 'Contributing Code' part of the Website with all the info that I wished I'd known at the start.
Comment 28 Alexey Proskuryakov 2006-05-05 12:31:46 PDT
(In reply to comment #27)

You can either use the svn-unapply script to rollback your patch locally before updating, or just revert to the base revision (if you have no other local modifications):

svn revert LayoutTests -R