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

[bugzilla-daemon at webkit.org]

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





--- Comment #13 from Darin Adler <darin at apple.com>  2014-04-28 10:13:02 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 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.

I think the intention is that in the future for almost all tests we can use that simpler version instead of pre/post. I like that idea, but maybe it’s buggy. Definitely don’t want TEST COMPLETE before the test results!

>> 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).

So it could have a different name. Like overrideMIMEType maybe?

> Source/WebCore/dom/Document.cpp:1412
> +        return String(mimeType);

No need for the String() here.

>> Source/WebCore/dom/Document.cpp:1414
>> +    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)?

If this really is correct behavior, then it should be done:

    return ASCIILiteral("application/xml");

No reason to call String constructor explicitly, and using ASCIILiteral will improve speed.

> Source/WebCore/dom/Document.cpp:3179
> +    setMimeType(other.contentType());

Is this really right? Seems like it will override the MIME type in the new document in a way that is not desirable. Do we have test cases that cover this?

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