Bug 132269 - Document.contentType implementation
Summary: Document.contentType implementation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-04-28 06:37 PDT by Tibor Mészáros
Modified: 2014-12-16 10:42 PST (History)
14 users (show)

See Also:


Attachments
Patch (24.58 KB, patch)
2014-04-28 06:37 PDT, Tibor Mészáros
no flags Details | Formatted Diff | Diff
Patch v2 (24.62 KB, patch)
2014-04-28 06:51 PDT, Tibor Mészáros
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (518.39 KB, application/zip)
2014-04-28 08:02 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (576.94 KB, application/zip)
2014-04-28 08:36 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (520.53 KB, application/zip)
2014-04-28 09:07 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (549.54 KB, application/zip)
2014-04-28 09:41 PDT, Build Bot
no flags Details
Patch v3 (52.22 KB, patch)
2014-05-29 05:47 PDT, Tibor Mészáros
ossy: review-
ossy: commit-queue-
Details | Formatted Diff | Diff
Patch (52.31 KB, patch)
2014-06-17 10:20 PDT, Tibor Mészáros
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion (531.78 KB, application/zip)
2014-06-17 12:21 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (539.90 KB, application/zip)
2014-06-17 22:03 PDT, Build Bot
no flags Details
Patch v5 (52.21 KB, patch)
2014-07-01 10:04 PDT, Tibor Mészáros
ossy: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (500.49 KB, application/zip)
2014-07-01 11:55 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion (558.18 KB, application/zip)
2014-07-01 12:43 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (482.23 KB, application/zip)
2014-07-01 14:19 PDT, Build Bot
no flags Details
Patch v6 (52.55 KB, patch)
2014-07-09 07:02 PDT, Tibor Mészáros
ap: review-
ap: commit-queue-
Details | Formatted Diff | Diff
Patch v7 (38.06 KB, patch)
2014-12-08 04:08 PST, Tibor Mészáros
no flags Details | Formatted Diff | Diff
Patch v8 (37.37 KB, patch)
2014-12-11 05:28 PST, Tibor Mészáros
darin: review+
darin: commit-queue-
Details | Formatted Diff | Diff
Patch v9 (37.54 KB, patch)
2014-12-16 09:59 PST, Tibor Mészáros
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tibor Mészáros 2014-04-28 06:37:09 PDT
The Document.contentType is not implemented in Webkit.

Chromium code: https://codereview.chromium.org/151653004
Comment 1 Tibor Mészáros 2014-04-28 06:37:58 PDT
Created attachment 230295 [details]
Patch

Feature implementation
Comment 2 WebKit Commit Bot 2014-04-28 06:39:12 PDT
Attachment 230295 [details] did not pass style-queue:


ERROR: LayoutTests/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 2 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Tibor Mészáros 2014-04-28 06:51:25 PDT
Created attachment 230296 [details]
Patch v2

Link has been added.
Comment 4 Build Bot 2014-04-28 08:02:23 PDT
Comment on attachment 230296 [details]
Patch v2

Attachment 230296 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6156206020231168

New failing tests:
http/tests/dom/document-contentType-xhr.html
fast/dom/document-contentType-createDocument.html
fast/dom/document-contentType-data-uri.html
http/tests/dom/document-contentType.html
Comment 5 Build Bot 2014-04-28 08:02:29 PDT
Created attachment 230298 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 6 Build Bot 2014-04-28 08:36:00 PDT
Comment on attachment 230296 [details]
Patch v2

Attachment 230296 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5797879046209536

New failing tests:
http/tests/dom/document-contentType-xhr.html
fast/dom/document-contentType-createDocument.html
fast/dom/document-contentType-data-uri.html
http/tests/dom/document-contentType.html
Comment 7 Build Bot 2014-04-28 08:36:07 PDT
Created attachment 230300 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 8 Build Bot 2014-04-28 09:07:15 PDT
Comment on attachment 230296 [details]
Patch v2

Attachment 230296 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6472285011574784

New failing tests:
http/tests/dom/document-contentType-xhr.html
fast/dom/document-contentType-createDocument.html
fast/dom/document-contentType-data-uri.html
http/tests/dom/document-contentType.html
Comment 9 Build Bot 2014-04-28 09:07:33 PDT
Created attachment 230301 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 10 Build Bot 2014-04-28 09:40:53 PDT
Comment on attachment 230296 [details]
Patch v2

Attachment 230296 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6688775522484224

New failing tests:
http/tests/dom/document-contentType-xhr.html
fast/dom/document-contentType-createDocument.html
fast/dom/document-contentType-data-uri.html
http/tests/dom/document-contentType.html
Comment 11 Build Bot 2014-04-28 09:41:00 PDT
Created attachment 230302 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 12 Alexey Proskuryakov 2014-04-28 10:07:36 PDT
Comment on attachment 230296 [details]
Patch v2

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.
Comment 13 Darin Adler 2014-04-28 10:13:02 PDT
Comment on attachment 230296 [details]
Patch v2

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?
Comment 14 Tibor Mészáros 2014-05-29 05:47:22 PDT
Created attachment 232245 [details]
Patch v3

The tests has been fixed.
Comment 15 Csaba Osztrogonác 2014-05-29 06:38:20 PDT
Comment on attachment 232245 [details]
Patch v3

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

> LayoutTests/fast/dom/document-contentType-data-uri.html:10
> +<iframe data-mt="text/xml"></iframe>
> +<iframe data-mt="application/xml"></iframe>
> +<iframe data-mt="image/svg+xml"></iframe>
> +<iframe data-mt="text/html"></iframe>

Changing the order of iframes isn't the proper fix here.

If text/html is the first iframe, its event still fires
at the end. It seems it isn't guaranteed that the events
fire in order. I don't know if it is normal or a bug.

Assuming that it isn't a bug, we should order the PASS messages once 
we caught all of the events in order to have deterministic test.

> LayoutTests/fast/dom/document-contentType-data-uri.html:27
> +var eventMethod = window.addEventListener ? "addEventListener" : "attachEvent";
> +var eventer = window[eventMethod];
> +var messageEvent = eventMethod == "attachEvent" ? "onmessage" : "message";
> +
> +eventer(messageEvent,function(e) {
> +    if (e.data) {
> +        shouldBe('"' + e.data.obtained + '"', '"' + e.data.expected + '"');
> +    } else
> +        testFailed("Null message payload");
> +
> +    if (--expectedMessagesCount == 0)
> +        finishJSTest();
> +},false);

What was the reason to refactor this part of the original patch?
Comment 16 Tibor Mészáros 2014-06-17 10:20:36 PDT
Created attachment 233238 [details]
Patch

The test has been modified as Csaba wished, and the source too, as Alexey and Darin wished.
Comment 17 Build Bot 2014-06-17 12:20:58 PDT
Comment on attachment 233238 [details]
Patch

Attachment 233238 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4561695351504896

New failing tests:
http/tests/dom/document-contentType-xhr.html
Comment 18 Build Bot 2014-06-17 12:21:05 PDT
Created attachment 233247 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 19 Build Bot 2014-06-17 22:03:46 PDT
Comment on attachment 233238 [details]
Patch

Attachment 233238 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4717285910511616

New failing tests:
media/W3C/video/networkState/networkState_during_loadstart.html
http/tests/dom/document-contentType-xhr.html
Comment 20 Build Bot 2014-06-17 22:03:53 PDT
Created attachment 233285 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 21 Tibor Mészáros 2014-07-01 10:04:46 PDT
Created attachment 234169 [details]
Patch v5

xhr test has been fixed
Comment 22 Build Bot 2014-07-01 11:55:40 PDT
Comment on attachment 234169 [details]
Patch v5

Attachment 234169 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4692801535082496

New failing tests:
http/tests/dom/document-contentType-xhr.html
Comment 23 Build Bot 2014-07-01 11:55:46 PDT
Created attachment 234182 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 24 Build Bot 2014-07-01 12:43:32 PDT
Comment on attachment 234169 [details]
Patch v5

Attachment 234169 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5186876294037504

New failing tests:
http/tests/dom/document-contentType-xhr.html
Comment 25 Build Bot 2014-07-01 12:43:43 PDT
Created attachment 234190 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 26 Build Bot 2014-07-01 14:19:16 PDT
Comment on attachment 234169 [details]
Patch v5

Attachment 234169 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4634210396536832

New failing tests:
http/tests/dom/document-contentType-xhr.html
Comment 27 Build Bot 2014-07-01 14:19:27 PDT
Created attachment 234196 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 28 Csaba Osztrogonác 2014-07-02 16:39:59 PDT
Comment on attachment 234169 [details]
Patch v5

r- now due to test failure.
Comment 29 Tibor Mészáros 2014-07-09 07:02:30 PDT
Created attachment 234634 [details]
Patch v6

There were some problem with the async test handling. I hope this version will work well.
Comment 30 Tibor Mészáros 2014-07-17 02:22:02 PDT
Dear Reviewers, please take a look on this patch, and tell me your opinion about it.
Comment 31 Tibor Mészáros 2014-08-04 05:14:38 PDT
Dear Reviewers, I'm still waiting for review.
Comment 32 Alexey Proskuryakov 2014-08-04 10:26:53 PDT
Comment on attachment 234634 [details]
Patch v6

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.
Comment 33 Tibor Mészáros 2014-12-08 04:08:38 PST
Created attachment 242800 [details]
Patch v7

All has been fixed, that Alexey Proskuryakov suggested.
Comment 34 Tibor Mészáros 2014-12-08 04:23:40 PST
Comment on attachment 234634 [details]
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
Comment 35 Darin Adler 2014-12-09 07:39:15 PST
Comment on attachment 242800 [details]
Patch v7

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

> Source/WebCore/dom/Document.cpp:1388
> +    if (!m_overriddenMIMEType.isEmpty())

I think maybe we want isNull here instead of isEmpty.

> Source/WebCore/dom/Document.cpp:1391
> +    if (DocumentLoader* documentLoader = this->loader())

No need for "this->" here.

> Source/WebCore/dom/Document.cpp:1395
> +    if (!mimeType.isEmpty())

I think maybe we want isNull here instead of isEmpty.

> Source/WebCore/loader/DocumentLoader.h:121
>          WEBCORE_EXPORT const String& responseMIMEType() const;
> +
> +        const String& mimeType() const;
>  #if PLATFORM(IOS)
>          // FIXME: This method seems to violate the encapsulation of this class.
>          WEBCORE_EXPORT void setResponseMIMEType(const String&);

Seems strange to put the new "mimeType" function between responseMIMEType and setResponseMIMEType, which were previously right next to each other. Can we find a better place for this?

Also, it’s confusing that we have a function named responseMIMEType and a second function named mimeType. Maybe we need a longer name for the new function.

> Source/WebCore/xml/DOMParser.cpp:42
> +    if (doc->contentType() != contentType)
> +        doc->overrideMIMEType(contentType);

Why is this conditional? What’s the downside to always calling overrideMIMEType unconditionally?
Comment 36 Tibor Mészáros 2014-12-11 05:28:27 PST
Created attachment 243120 [details]
Patch v8

Updated patch
Comment 37 Tibor Mészáros 2014-12-11 05:49:51 PST
Comment on attachment 242800 [details]
Patch v7

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

>> Source/WebCore/dom/Document.cpp:1388
>> +    if (!m_overriddenMIMEType.isEmpty())
> 
> I think maybe we want isNull here instead of isEmpty.

I changed it to isNull.

>> Source/WebCore/dom/Document.cpp:1391
>> +    if (DocumentLoader* documentLoader = this->loader())
> 
> No need for "this->" here.

removed.

>> Source/WebCore/dom/Document.cpp:1395
>> +    if (!mimeType.isEmpty())
> 
> I think maybe we want isNull here instead of isEmpty.

Changed it too.

>> Source/WebCore/loader/DocumentLoader.h:121
>>          WEBCORE_EXPORT void setResponseMIMEType(const String&);
> 
> Seems strange to put the new "mimeType" function between responseMIMEType and setResponseMIMEType, which were previously right next to each other. Can we find a better place for this?
> 
> Also, it’s confusing that we have a function named responseMIMEType and a second function named mimeType. Maybe we need a longer name for the new function.

I replaced it, now it's after the setResponseMIMEType function. Could you possibly suggest a better name for this simple getter?

>> Source/WebCore/xml/DOMParser.cpp:42
>> +        doc->overrideMIMEType(contentType);
> 
> Why is this conditional? What’s the downside to always calling overrideMIMEType unconditionally?

It was a previous fix, not necessary any more.
Comment 38 Darin Adler 2014-12-11 09:32:29 PST
Comment on attachment 242800 [details]
Patch v7

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

>>> Source/WebCore/loader/DocumentLoader.h:121
>>>          WEBCORE_EXPORT void setResponseMIMEType(const String&);
>> 
>> Seems strange to put the new "mimeType" function between responseMIMEType and setResponseMIMEType, which were previously right next to each other. Can we find a better place for this?
>> 
>> Also, it’s confusing that we have a function named responseMIMEType and a second function named mimeType. Maybe we need a longer name for the new function.
> 
> I replaced it, now it's after the setResponseMIMEType function. Could you possibly suggest a better name for this simple getter?

I have three thoughts about the name:

1) In the context of this class, the key issue is that there are multiple of these types. There’s the type from the response and the type that the loader has determined we should use for the document. Since both exist, neither function should be named just “type”, “content type” or “MIME type” and lack an additional clarifying modifier. So maybe one is the responseMIMEType and the other is the computedContentType or the currentContentType. Think of words that would distinguish the two. One property of the latter type is that it changes as we parse more of the document, perhaps? I don’t really know and it’s worth a little research.

2) Another way to resolve the ambiguity from (1) above would be to remove some of the DocumentLoader functions like requestURL and responseURL and responseMIMEType and instead have callers get the request and response directly and get the information from there. If that was true, then I’d be comfortable naming the function contentType since there would not be a responseMIMEType function in the same class. This would also probably improve the interface to the document loader class by cutting down the number of separate functions.

3) I personally can’t stand using the acronym MIME so often. I’d prefer to call these things content type rather than MIME type. But that’s not specific to this function. And perhaps there are other “types of types” that this would make confusing.
Comment 39 Darin Adler 2014-12-15 08:58:32 PST
Comment on attachment 243120 [details]
Patch v8

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

> Source/WebCore/dom/Document.h:469
> +    String contentType() const; // DOM 4 document.contentType

I don’t think this comment is helpful. We don’t want to comment every function with the standard it comes from.

> Source/WebCore/dom/Document.idl:108
> +    // DOM 4

I’ve heard there is no standard any more named “DOM 4”, so we might just want to leave this comment out.

> Source/WebCore/loader/DocumentLoader.h:121
> +        const String& mimeType() const;

Please consider changing the name as I requested. I gave my rationale in a comment in the bug.
Comment 40 Tibor Mészáros 2014-12-16 09:59:15 PST
Created attachment 243364 [details]
Patch v9

Patch for landing.
Comment 41 Tibor Mészáros 2014-12-16 10:01:04 PST
Comment on attachment 243120 [details]
Patch v8

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

>> Source/WebCore/dom/Document.h:469
>> +    String contentType() const; // DOM 4 document.contentType
> 
> I don’t think this comment is helpful. We don’t want to comment every function with the standard it comes from.

This was removed.

>> Source/WebCore/dom/Document.idl:108
>> +    // DOM 4
> 
> I’ve heard there is no standard any more named “DOM 4”, so we might just want to leave this comment out.

This was removed.

>> Source/WebCore/loader/DocumentLoader.h:121
>> +        const String& mimeType() const;
> 
> Please consider changing the name as I requested. I gave my rationale in a comment in the bug.

This was changed to currentContentType.
Comment 42 WebKit Commit Bot 2014-12-16 10:41:59 PST
Comment on attachment 243364 [details]
Patch v9

Clearing flags on attachment: 243364

Committed r177366: <http://trac.webkit.org/changeset/177366>
Comment 43 WebKit Commit Bot 2014-12-16 10:42:11 PST
All reviewed patches have been landed.  Closing bug.