[Webkit-unassigned] [Bug 132269] Document.contentType implementation

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=132269





--- Comment #12 from Alexey Proskuryakov <ap at webkit.org>  2014-04-28 10:07:36 PST ---
(From update of attachment 230296)
View in context: https://bugs.webkit.org/attachment.cgi?id=230296&action=review

> LayoutTests/http/tests/dom/document-contentType-expected.txt:5
> +PASS successfullyParsed is true
> +
> +TEST COMPLETE
> +PASS iframes[0].contentDocument.contentType is "text/css"
> +PASS iframes[0].contentDocument.cloneNode(false).contentType is "text/css"

This test needs to be fixed, so that it doesn't say "TEST COMPLETE" before actual test output.

It uses an unusual version of test rig, js-test.js instead of js-test-pre.js plus js-test-post.js. I'm not sure why we have that in the tree.

> LayoutTests/resources/js-test.js:427
> +function shouldBeEqualToNumber(a, b)

Seems count-productive to add helpers to js-test.js, but not to js-test-pre.js.

If js-test.js is our new best friend, someone should send an e-mail to webkit-dev announcing that.

> Source/WebCore/ChangeLog:13
> +        Tests: fast/dom/document-contentType-createDocument.html
> +               fast/dom/document-contentType-data-uri.html
> +               http/tests/dom/document-contentType-xhr.html
> +               http/tests/dom/document-contentType.html

This is good test coverage.

One important case that's missing is how <meta http-equiv> is handled. Please add a test for that (Mozilla documentation says how it should work).

> Source/WebCore/dom/Document.cpp:1397
> +void Document::setMimeType(const String& mimeType)

Style mistake: should be setMIMEType().

It's unfortunate that we have such a generic sounding function that is only expected to be used in unusual circumstances (i.e. when the document has no loader).

> Source/WebCore/dom/Document.cpp:1402
> +String Document::contentType() const

I know that this is how the API is named, but this is not content type, this is MIME type. These are different, e.g. "text/html; charset=UTF-8" is a valid value for Content-Type HTTP header field, while the MIME type is just "text/html".

You can have a different name in implementaion with ImplementedAs IDL attribute.

> Source/WebCore/dom/Document.cpp:1414
> +    String mimeType = suggestedMIMEType();
> +    if (!mimeType.isEmpty())
> +        return String(mimeType);
> +
> +    return String("application/xml");

This looks almost like random behavior. What are the cases where we have neither m_mimeType nor a documentLoader? Can they be havndled more systemically (perhaps by setting m_mimeType)?

> Source/WebCore/dom/Document.h:1410
> +    // Mime-type of the document in case it was cloned or created by XHR.

Style: MIME type, not Mime-type.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.