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

[bugzilla-daemon at webkit.org]

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

--- Comment #34 from Tibor Mészáros <tmeszaros.u-szeged at partner.samsung.com> ---
Comment on attachment 234634
  --> https://bugs.webkit.org/attachment.cgi?id=234634
Patch v6

View in context: https://bugs.webkit.org/attachment.cgi?id=234634&action=review

>> LayoutTests/fast/dom/document-contentType-data-uri.html:20
>> +        testFailed("Null message payload");
> 
> Inconsistent coding style - we don't use braces in either case in WebKit.

These braces removed.

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

The test has been fixed, it will not compare the expected result with itself.

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

The underscore removed. It was just a naming style, what I use sometimes.

>> LayoutTests/http/tests/dom/document-contentType-xhr.html:53
>> +    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.

The js-test-pre.js changed to js-test.js, and yes, I had tried to create a workaround for a behavior that occurs when using testRunner.waitUntilDone() with resources/js-test-post-async.js. As you suggested, I modified the test, so I removed the the testComplete method, and added the finishJSTest() call.

>> 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?

mt means mimetype, so I renamed it.

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

This block has been removed, and simply called the finishJSTest() after the for cycle.

>> Source/WebCore/dom/Document.h:1408
>> +    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.

renamed

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20141208/19d652c1/attachment-0002.html>