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

[bugzilla-daemon at webkit.org]

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


Alexey Proskuryakov <ap at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #234634|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #32 from Alexey Proskuryakov <ap at webkit.org>  2014-08-04 10:26:53 PST ---
(From update of attachment 234634)
View in context: https://bugs.webkit.org/attachment.cgi?id=234634&action=review

> LayoutTests/ChangeLog:4
> +        Document.contentType implementation
> +        https://bugs.webkit.org/show_bug.cgi?id=132269

Some code paths that are not explicitly handled, and not tested here are documents created with DOMParser or XSLT. We'll probably get an incorrect result in some cases because of fallback to suggestedMIMEType().

> LayoutTests/ChangeLog:32
> +        * http/tests/dom/resources/js-test-post-async.js: Added.
> +        * http/tests/dom/resources/js-test-post.js: Added.
> +        * http/tests/dom/resources/js-test-pre.js: Added.

There is no need to copy these files, they are available to http tests at /js-test-resources/.

> LayoutTests/fast/dom/document-contentType-createDocument.html:11
> +doc = document.implementation.createDocument(null, 'root', null);
> +shouldBe('doc.contentType', '"application/xml"');

Can you please also test some non-default type, e.g. application/xml+foobar?

> LayoutTests/fast/dom/document-contentType-data-uri.html:20
> +    if (e.data) {
> +        tests[--expectedMessagesCount]=e.data;
> +    } else
> +        testFailed("Null message payload");

Inconsistent coding style - we don't use braces in either case in WebKit.

> LayoutTests/http/tests/dom/document-contentType-meta.html:10
> +  shouldBeEqualToString('document.getElementsByTagName("META")[0].content', document.getElementsByTagName("META")[0].content);

This compares a string to itself.

What did you intend to test here? I think that the META will be ignored, because file:// documents are handled as if they had an HTTP header, which always wins.

I asked you to test this case in earlier review, however I don't know exactly how it should work.

> LayoutTests/http/tests/dom/document-contentType-xhr.html:33
> +var __testCounter = 0;

Why the double underscore?

> LayoutTests/http/tests/dom/document-contentType-xhr.html:53
> +    var scriptElement = document.createElement("script")
> +    scriptElement.src = 'resources/js-test-post-async.js'
> +    document.body.appendChild(scriptElement);

This is tricky, and should not be necessary. I think that you are working around a weird behavior that occurs when using testRunner.waitUntilDone() with resources/js-test-post-async.js.

The correct solution is to have both js-test-pre.js and js-test-post.js, set window.jsTestIsAsync, and call finishJSTest() when done. It may be that js-test.js alone works, too.

> LayoutTests/http/tests/dom/document-contentType.html:7
> +<iframe data-mt="text/css" src="resources/dummy.css"></iframe>

It would be nice to use a descriptive name in place of "mt" (something that would spell out "expected content type"). 

Besides, is it expected media type, content type, or MIME type?

> LayoutTests/http/tests/dom/document-contentType.html:34
> +    if (window.jsTestIsAsync) {

This test isn't async, why test for that? I don't think that you need anything in this block.

> Source/WebCore/dom/Document.h:1408
> +    // MIME type of the document in case it was cloned or created by XHR.
> +    String m_mimeType;

It's good to have a comment, yet it would be even better to have a descriptive name. The comment is not available where the variable is used, so the name can confuse readers.

m_overriddenMIMEType would be fine.

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