| Summary: | Document.contentType implementation | ||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Tibor Mészáros <mtiborinf> | ||||||||||||||||||||||||||||||||||||||
| Component: | WebCore JavaScript | Assignee: | 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
Tibor Mészáros
2014-04-28 06:37:09 PDT
Created attachment 230295 [details]
Patch
Feature implementation
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.
Created attachment 230296 [details]
Patch v2
Link has been added.
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 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 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 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 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 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 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 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 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 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? Created attachment 232245 [details]
Patch v3
The tests has been fixed.
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? Created attachment 233238 [details]
Patch
The test has been modified as Csaba wished, and the source too, as Alexey and Darin wished.
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 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 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 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
Created attachment 234169 [details]
Patch v5
xhr test has been fixed
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 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 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 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 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 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 on attachment 234169 [details]
Patch v5
r- now due to test failure.
Created attachment 234634 [details]
Patch v6
There were some problem with the async test handling. I hope this version will work well.
Dear Reviewers, please take a look on this patch, and tell me your opinion about it. Dear Reviewers, I'm still waiting for review. 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. Created attachment 242800 [details]
Patch v7
All has been fixed, that Alexey Proskuryakov suggested.
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 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? Created attachment 243120 [details]
Patch v8
Updated patch
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 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 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. Created attachment 243364 [details]
Patch v9
Patch for landing.
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 on attachment 243364 [details] Patch v9 Clearing flags on attachment: 243364 Committed r177366: <http://trac.webkit.org/changeset/177366> All reviewed patches have been landed. Closing bug. |