[webkit-reviews] review denied: [Bug 132269] Document.contentType implementation : [Attachment 234634] Patch v6

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Tibor Mészáros
<tmeszaros.u-szeged at partner.samsung.com>'s request for review:
Bug 132269: Document.contentType implementation
https://bugs.webkit.org/show_bug.cgi?id=132269

Attachment 234634: Patch v6
https://bugs.webkit.org/attachment.cgi?id=234634&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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.