Bug 132269

Summary: Document.contentType implementation
Product: WebKit Reporter: Tibor Mészáros <mtiborinf>
Component: WebCore JavaScriptAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, buildbot, bunhere, cdumez, commit-queue, darin, esprehn+autocc, gyuyoung.kim, gyuyoung.kim, kangil.han, kondapallykalyan, ossy, rniwa, sergio
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch v2
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion
none
Patch v3
ossy: review-, ossy: commit-queue-
Patch
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2
none
Patch v5
ossy: review-, buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
none
Patch v6
ap: review-, ap: commit-queue-
Patch v7
none
Patch v8
darin: review+, darin: commit-queue-
Patch v9 none

Tibor Mészáros
Reported 2014-04-28 06:37:09 PDT
The Document.contentType is not implemented in Webkit. Chromium code: https://codereview.chromium.org/151653004
Attachments
Patch (24.58 KB, patch)
2014-04-28 06:37 PDT, Tibor Mészáros
no flags
Patch v2 (24.62 KB, patch)
2014-04-28 06:51 PDT, Tibor Mészáros
buildbot: commit-queue-
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
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
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
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
Patch v3 (52.22 KB, patch)
2014-05-29 05:47 PDT, Tibor Mészáros
ossy: review-
ossy: commit-queue-
Patch (52.31 KB, patch)
2014-06-17 10:20 PDT, Tibor Mészáros
buildbot: commit-queue-
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
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
Patch v5 (52.21 KB, patch)
2014-07-01 10:04 PDT, Tibor Mészáros
ossy: review-
buildbot: commit-queue-
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
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
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
Patch v6 (52.55 KB, patch)
2014-07-09 07:02 PDT, Tibor Mészáros
ap: review-
ap: commit-queue-
Patch v7 (38.06 KB, patch)
2014-12-08 04:08 PST, Tibor Mészáros
no flags
Patch v8 (37.37 KB, patch)
2014-12-11 05:28 PST, Tibor Mészáros
darin: review+
darin: commit-queue-
Patch v9 (37.54 KB, patch)
2014-12-16 09:59 PST, Tibor Mészáros
no flags
Tibor Mészáros
Comment 1 2014-04-28 06:37:58 PDT
Created attachment 230295 [details] Patch Feature implementation
WebKit Commit Bot
Comment 2 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.
Tibor Mészáros
Comment 3 2014-04-28 06:51:25 PDT
Created attachment 230296 [details] Patch v2 Link has been added.
Build Bot
Comment 4 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
Build Bot
Comment 5 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
Build Bot
Comment 6 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
Build Bot
Comment 7 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
Build Bot
Comment 8 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
Build Bot
Comment 9 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
Build Bot
Comment 10 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
Build Bot
Comment 11 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
Alexey Proskuryakov
Comment 12 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.
Darin Adler
Comment 13 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?
Tibor Mészáros
Comment 14 2014-05-29 05:47:22 PDT
Created attachment 232245 [details] Patch v3 The tests has been fixed.
Csaba Osztrogonác
Comment 15 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?
Tibor Mészáros
Comment 16 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.
Build Bot
Comment 17 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
Build Bot
Comment 18 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
Build Bot
Comment 19 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
Build Bot
Comment 20 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
Tibor Mészáros
Comment 21 2014-07-01 10:04:46 PDT
Created attachment 234169 [details] Patch v5 xhr test has been fixed
Build Bot
Comment 22 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
Build Bot
Comment 23 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
Build Bot
Comment 24 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
Build Bot
Comment 25 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
Build Bot
Comment 26 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
Build Bot
Comment 27 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
Csaba Osztrogonác
Comment 28 2014-07-02 16:39:59 PDT
Comment on attachment 234169 [details] Patch v5 r- now due to test failure.
Tibor Mészáros
Comment 29 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.
Tibor Mészáros
Comment 30 2014-07-17 02:22:02 PDT
Dear Reviewers, please take a look on this patch, and tell me your opinion about it.
Tibor Mészáros
Comment 31 2014-08-04 05:14:38 PDT
Dear Reviewers, I'm still waiting for review.
Alexey Proskuryakov
Comment 32 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.
Tibor Mészáros
Comment 33 2014-12-08 04:08:38 PST
Created attachment 242800 [details] Patch v7 All has been fixed, that Alexey Proskuryakov suggested.
Tibor Mészáros
Comment 34 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
Darin Adler
Comment 35 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?
Tibor Mészáros
Comment 36 2014-12-11 05:28:27 PST
Created attachment 243120 [details] Patch v8 Updated patch
Tibor Mészáros
Comment 37 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.
Darin Adler
Comment 38 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.
Darin Adler
Comment 39 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.
Tibor Mészáros
Comment 40 2014-12-16 09:59:15 PST
Created attachment 243364 [details] Patch v9 Patch for landing.
Tibor Mészáros
Comment 41 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.
WebKit Commit Bot
Comment 42 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>
WebKit Commit Bot
Comment 43 2014-12-16 10:42:11 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.