Bug 118306 – [WTR] Should dump as text when the mimetype is text/plain

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Peter Gal]

WebKitTestRunner should dump the result as text when the content has text/plain mimetype. This behaviour is also present in the DumpRenderTree.

[Peter Gal]

Created attachment 205918 [details]
patch

[Chris Dumez]

Comment on attachment 205918 [details]
patch

Why doesn't this unskip any test?

[Peter Gal]

(In reply to comment #2)
> (From update of attachment 205918 [details])
> Why doesn't this unskip any test?

That's because I wanted to do it incrementally to make sure that the bots will stay green. A follow up patch would contain the unskipping.

[Chris Dumez]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 205918 [details] [details])
> > Why doesn't this unskip any test?
> 
> That's because I wanted to do it incrementally to make sure that the bots will stay green. A follow up patch would contain the unskipping.

We usually unskip in the same patch so that we can see the impact of the change more easily.

[Chris Dumez]

Comment on attachment 205918 [details]
patch

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

Please unskip tests in the same patch.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:876
> +    WTF::String mimeType = toWTFString(adoptWK(WKBundleFrameCopyMIMETypeForResourceWithURL(frame, WKBundleFrameCopyURL(frame))));

This is leaking the return value of WKBundleFrameCopyURL().

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:877
> +    if (url.find("dumpAsText/") != WTF::notFound || mimeType == "text/plain")

Don't you mean "&& mimeType != "text/plain" ?

[Peter Gal]

(In reply to comment #5)
> (From update of attachment 205918 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205918&action=review
> 
> Please unskip tests in the same patch.
> 
> > Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:876
> > +    WTF::String mimeType = toWTFString(adoptWK(WKBundleFrameCopyMIMETypeForResourceWithURL(frame, WKBundleFrameCopyURL(frame))));
> 
> This is leaking the return value of WKBundleFrameCopyURL().
> 

Ahh okay.

> > Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:877
> > +    if (url.find("dumpAsText/") != WTF::notFound || mimeType == "text/plain")
> 
> Don't you mean "&& mimeType != "text/plain" ?

No, because we should dumpAsText if the "dumpAsText/" string is found (hence the first part) or if the mimeType is "text/plain"

In the dumpAsText(false) call the 'false' means do not dump pixels.

[Chris Dumez]

Comment on attachment 205918 [details]
patch

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

>>> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:877
>>> +    if (url.find("dumpAsText/") != WTF::notFound || mimeType == "text/plain")
>> 
>> Don't you mean "&& mimeType != "text/plain" ?
> 
> No, because we should dumpAsText if the "dumpAsText/" string is found (hence the first part) or if the mimeType is "text/plain"
> 
> In the dumpAsText(false) call the 'false' means do not dump pixels.

Ok. "dumpAsText(false)" confused me indeed.

[Peter Gal]

Created attachment 206247 [details]
patch v2

Now with test unskipping (also tested on mac)

[Chris Dumez]

Comment on attachment 206247 [details]
patch v2

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

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:875
> +    WKURLRef urlRef = WKBundleFrameCopyURL(frame);

urlRef is leaked, please adopt the returned value.

[Peter Gal]

(In reply to comment #9)
> (From update of attachment 206247 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206247&action=review
> 
> > Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:875
> > +    WKURLRef urlRef = WKBundleFrameCopyURL(frame);
> 
> urlRef is leaked, please adopt the returned value.

arrrgh... I know I've missed something, sorry.

[Mikhail Pozdnyakov]

Comment on attachment 206247 [details]
patch v2

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

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:876
> +    WTF::String url = toWTFString(adoptWK(WKURLCopyString(urlRef)));

I remember overloaded toWTFString accepting WKURLRef, can we use it?

[Chris Dumez]

Comment on attachment 206247 [details]
patch v2

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

>> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:876
>> +    WTF::String url = toWTFString(adoptWK(WKURLCopyString(urlRef)));
> 
> I remember overloaded toWTFString accepting WKURLRef, can we use it?

I don't see it in Tools/WebKitTestRunner/StringFunctions.h. Are you sure this landed Mikhail?

[Mikhail Pozdnyakov]

(In reply to comment #12)
> (From update of attachment 206247 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206247&action=review
> 
> >> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:876
> >> +    WTF::String url = toWTFString(adoptWK(WKURLCopyString(urlRef)));
> > 
> > I remember overloaded toWTFString accepting WKURLRef, can we use it?
> 
> I don't see it in Tools/WebKitTestRunner/StringFunctions.h. Are you sure this landed Mikhail?

yeah.. but it's inside Source/WebKit2/Shared/API/c/WKSharedAPICast.h.

[Peter Gal]

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 206247 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=206247&action=review
> > 
> > >> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:876
> > >> +    WTF::String url = toWTFString(adoptWK(WKURLCopyString(urlRef)));
> > > 
> > > I remember overloaded toWTFString accepting WKURLRef, can we use it?
> > 
> > I don't see it in Tools/WebKitTestRunner/StringFunctions.h. Are you sure this landed Mikhail?
> 
> yeah.. but it's inside Source/WebKit2/Shared/API/c/WKSharedAPICast.h.

I've tried it an after including the header the
WebKit::toWTFString(urlRef.get()) call seems to be working. So should I use this?

[Chris Dumez]

Comment on attachment 206247 [details]
patch v2

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

>>>>> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:876
>>>>> +    WTF::String url = toWTFString(adoptWK(WKURLCopyString(urlRef)));
>>>> 
>>>> I remember overloaded toWTFString accepting WKURLRef, can we use it?
>>> 
>>> I don't see it in Tools/WebKitTestRunner/StringFunctions.h. Are you sure this landed Mikhail?
>> 
>> yeah.. but it's inside Source/WebKit2/Shared/API/c/WKSharedAPICast.h.
> 
> I've tried it an after including the header the
> WebKit::toWTFString(urlRef.get()) call seems to be working. So should I use this?

No, I would not include WKSharedAPICast.h in WKTR. I would leave the patch as it is but fix the leak.

[Peter Gal]

Created attachment 206255 [details]
patch v3

[Build Bot]

Comment on attachment 206255 [details]
patch v3

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

New failing tests:
http/tests/multipart/load-last-non-html-frame.php

[Build Bot]

Created attachment 206261 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[Build Bot]

Comment on attachment 206255 [details]
patch v3

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

New failing tests:
http/tests/multipart/load-last-non-html-frame.php

[Build Bot]

Created attachment 206270 [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.3

[Chris Dumez]

(In reply to comment #19)
> (From update of attachment 206255 [details])
> Attachment 206255 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.appspot.com/results/1043157
> 
> New failing tests:
> http/tests/multipart/load-last-non-html-frame.php

Apparently does not work on mac wk1 or wk2.

[Peter Gal]

(In reply to comment #21)
> (In reply to comment #19)
> > (From update of attachment 206255 [details] [details])
> > Attachment 206255 [details] [details] did not pass mac-ews (mac):
> > Output: http://webkit-queues.appspot.com/results/1043157
> > 
> > New failing tests:
> > http/tests/multipart/load-last-non-html-frame.php
> 
> Apparently does not work on mac wk1 or wk2.

Yeah.. but I don't know why I tested on a mac and there it was okay. Also note that it fails due to the fact that there is no correct multipart handling. Previously it also failed because it was dumping render tree instead of simple text.

[Chris Dumez]

(In reply to comment #22)
> (In reply to comment #21)
> > (In reply to comment #19)
> > > (From update of attachment 206255 [details] [details] [details])
> > > Attachment 206255 [details] [details] [details] did not pass mac-ews (mac):
> > > Output: http://webkit-queues.appspot.com/results/1043157
> > > 
> > > New failing tests:
> > > http/tests/multipart/load-last-non-html-frame.php
> > 
> > Apparently does not work on mac wk1 or wk2.
> 
> Yeah.. but I don't know why I tested on a mac and there it was okay. Also note that it fails due to the fact that there is no correct multipart handling. Previously it also failed because it was dumping render tree instead of simple text.

So maybe remove it from the wk2 generic skip list and keep it skipped for mac. It was already skipped for mac wk1 so it is likely something else is missing in the mac port to unskip this test.

[Peter Gal]

(In reply to comment #23)
> So maybe remove it from the wk2 generic skip list and keep it skipped for mac. It was already skipped for mac wk1 so it is likely something else is missing in the mac port to unskip this test.

Okay I'll move the skip to the mac-wk2 list.

[Chris Dumez]

(In reply to comment #24)
> (In reply to comment #23)
> > So maybe remove it from the wk2 generic skip list and keep it skipped for mac. It was already skipped for mac wk1 so it is likely something else is missing in the mac port to unskip this test.
> 
> Okay I'll move the skip to the mac-wk2 list.

?

It fails for both mac wk1 and wk2 so I would leave the test where it is (in the generic mac skip list).

[Peter Gal]

(In reply to comment #25)
> (In reply to comment #24)
> > (In reply to comment #23)
> > > So maybe remove it from the wk2 generic skip list and keep it skipped for mac. It was already skipped for mac wk1 so it is likely something else is missing in the mac port to unskip this test.
> > 
> > Okay I'll move the skip to the mac-wk2 list.
> 
> ?
> 
> It fails for both mac wk1 and wk2 so I would leave the test where it is (in the generic mac skip list).

Uhh.. sorry, yeah that was I wanted to say, but got distracted :)

[Peter Gal]

Created attachment 207322 [details]
patch v4

Sorry for the big delay.

[Peter Gal]

Comment on attachment 207322 [details]
patch v4

ahh sorry didn't updated the changelog.

[Peter Gal]

Created attachment 207323 [details]
patch v4.1

[Chris Dumez]

Comment on attachment 207323 [details]
patch v4.1

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

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:876
> +    WTF::String url = toWTFString(adoptWK(WKURLCopyString(urlRef.get())));

I don't believe WTF:: is needed.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:877
> +    WTF::String mimeType = toWTFString(adoptWK(WKBundleFrameCopyMIMETypeForResourceWithURL(frame, urlRef.get())));

We don't really need a WTF String here. We can keep it as a WKRetainPtr<WKStringRef> and use WKStringIsEqualToUTF8CString(mimeType.get(), "text/plain") in the if condition.

[Peter Gal]

(In reply to comment #30)
> (From update of attachment 207323 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=207323&action=review
> 
> > Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:876
> > +    WTF::String url = toWTFString(adoptWK(WKURLCopyString(urlRef.get())));
> 
> I don't believe WTF:: is needed.
> 

Hmm.. it seems to be true. I just automatically assumed we need that, it's used that way in this file everywhere.

> > Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:877
> > +    WTF::String mimeType = toWTFString(adoptWK(WKBundleFrameCopyMIMETypeForResourceWithURL(frame, urlRef.get())));
> 
> We don't really need a WTF String here. We can keep it as a WKRetainPtr<WKStringRef> and use WKStringIsEqualToUTF8CString(mimeType.get(), "text/plain") in the if condition.

Didn't know there is function for this. I'll update the patch accordingly.

[Build Bot]

Comment on attachment 207323 [details]
patch v4.1

Attachment 207323 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1249652

New failing tests:
dom/xhtml/level1/core/documentinvalidcharacterexceptioncreateentref1.xhtml
dom/svg/level3/xpath/XPathEvaluator_evaluate_INVALID_EXPRESSION_ERR.svg
dom/html/level2/events/dispatchEvent04.html
dom/html/level2/html/HTMLSelectElement20.html
dom/svg/level3/xpath/XPathEvaluator_createExpression_INVALID_EXPRESSION_ERR.svg
dom/svg/level3/xpath/XPathEvaluator_createExpression_NAMESPACE_ERR_02.svg
dom/html/level1/core/documentinvalidcharacterexceptioncreateentref.html
dom/html/level2/core/hc_namednodemapinvalidtype1.html
dom/svg/level3/xpath/XPathEvaluator_evaluate_NAMESPACE_ERR.svg
dom/html/level2/events/dispatchEvent01.html
dom/xhtml/level1/core/hc_attrappendchild4.xhtml
dom/html/level2/events/dispatchEvent03.html
dom/html/level2/events/dispatchEvent02.html
dom/html/level2/core/createDocumentType04.html
dom/html/level1/core/documentinvalidcharacterexceptioncreateentref1.html
dom/html/level1/core/documentinvalidcharacterexceptioncreatepi1.html
dom/html/level2/events/dispatchEvent06.html
css1/basic/comments.html
dom/xhtml/level1/core/documentinvalidcharacterexceptioncreatepi1.xhtml
dom/html/level2/events/dispatchEvent07.html
dom/html/level2/core/setAttributeNS10.html
dom/html/level2/events/dispatchEvent05.html
dom/html/level1/core/hc_attrappendchild2.html
dom/html/level2/core/createAttributeNS06.html
dom/html/level1/core/hc_attrappendchild4.html
dom/xhtml/level1/core/documentinvalidcharacterexceptioncreatepi.xhtml
dom/svg/level3/xpath/XPathEvaluator_createExpression_NAMESPACE_ERR_01.svg
dom/xhtml/level1/core/documentinvalidcharacterexceptioncreateentref.xhtml
dom/xhtml/level1/core/hc_attrappendchild2.xhtml
dom/html/level1/core/documentinvalidcharacterexceptioncreatepi.html

[Build Bot]

Created attachment 207607 [details]
Archive of layout-test-results from APPLE-EWS-5 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: APPLE-EWS-5  Port: win-future  Platform: CYGWIN_NT-6.1-WOW64-1.7.20-0.266-5-3-i686-32bit

[Peter Gal]

(In reply to comment #32)
> (From update of attachment 207323 [details])
> Attachment 207323 [details] did not pass win-ews (win):
> Output: http://webkit-queues.appspot.com/results/1249652
> 
> New failing tests:
> dom/xhtml/level1/core/documentinvalidcharacterexceptioncreateentref1.xhtml
> dom/svg/level3/xpath/XPathEvaluator_evaluate_INVALID_EXPRESSION_ERR.svg
> dom/html/level2/events/dispatchEvent04.html
> dom/html/level2/html/HTMLSelectElement20.html
> dom/svg/level3/xpath/XPathEvaluator_createExpression_INVALID_EXPRESSION_ERR.svg
> dom/svg/level3/xpath/XPathEvaluator_createExpression_NAMESPACE_ERR_02.svg
> dom/html/level1/core/documentinvalidcharacterexceptioncreateentref.html
> dom/html/level2/core/hc_namednodemapinvalidtype1.html
> dom/svg/level3/xpath/XPathEvaluator_evaluate_NAMESPACE_ERR.svg
> dom/html/level2/events/dispatchEvent01.html
> dom/xhtml/level1/core/hc_attrappendchild4.xhtml
> dom/html/level2/events/dispatchEvent03.html
> dom/html/level2/events/dispatchEvent02.html
> dom/html/level2/core/createDocumentType04.html
> dom/html/level1/core/documentinvalidcharacterexceptioncreateentref1.html
> dom/html/level1/core/documentinvalidcharacterexceptioncreatepi1.html
> dom/html/level2/events/dispatchEvent06.html
> css1/basic/comments.html
> dom/xhtml/level1/core/documentinvalidcharacterexceptioncreatepi1.xhtml
> dom/html/level2/events/dispatchEvent07.html
> dom/html/level2/core/setAttributeNS10.html
> dom/html/level2/events/dispatchEvent05.html
> dom/html/level1/core/hc_attrappendchild2.html
> dom/html/level2/core/createAttributeNS06.html
> dom/html/level1/core/hc_attrappendchild4.html
> dom/xhtml/level1/core/documentinvalidcharacterexceptioncreatepi.xhtml
> dom/svg/level3/xpath/XPathEvaluator_createExpression_NAMESPACE_ERR_01.svg
> dom/xhtml/level1/core/documentinvalidcharacterexceptioncreateentref.xhtml
> dom/xhtml/level1/core/hc_attrappendchild2.xhtml
> dom/html/level1/core/documentinvalidcharacterexceptioncreatepi.html

These are DRT related crashes. This patch couldn't have caused that.

[Peter Gal]

Created attachment 208709 [details]
patch v5

Updated patch:
 - removed WTF::
 - using WKStringIsEqualToUTF8CString method

as per reviewer request.

[Chris Dumez]

Comment on attachment 208709 [details]
patch v5

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

Looks fine. r=me with nits.

> LayoutTests/ChangeLog:9
> +        Now the result should match the expected.

expected -> "expectation"

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:853
> +    WKRetainPtr<WKStringRef> mimeType= adoptWK(WKBundleFrameCopyMIMETypeForResourceWithURL(frame, urlRef.get()));

nit: missing space before equal

[Csaba Osztrogonác]

Committed r154478: <http://trac.webkit.org/changeset/154478>