Bug 118306 - [WTR] Should dump as text when the mimetype is text/plain
Summary: [WTR] Should dump as text when the mimetype is text/plain
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-07-02 08:24 PDT by Peter Gal
Modified: 2013-08-23 04:18 PDT (History)
7 users (show)

See Also:


Attachments
patch (1.55 KB, patch)
2013-07-02 08:34 PDT, Peter Gal
cdumez: review-
cdumez: commit-queue-
Details | Formatted Diff | Diff
patch v2 (3.55 KB, patch)
2013-07-08 08:24 PDT, Peter Gal
cdumez: review-
cdumez: commit-queue-
Details | Formatted Diff | Diff
patch v3 (3.55 KB, patch)
2013-07-08 10:04 PDT, Peter Gal
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (550.29 KB, application/zip)
2013-07-08 11:34 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (530.88 KB, application/zip)
2013-07-08 14:45 PDT, Build Bot
no flags Details
patch v4 (3.03 KB, patch)
2013-07-23 05:31 PDT, Peter Gal
no flags Details | Formatted Diff | Diff
patch v4.1 (2.99 KB, patch)
2013-07-23 05:36 PDT, Peter Gal
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from APPLE-EWS-5 for win-future (327.41 KB, application/zip)
2013-07-28 01:04 PDT, Build Bot
no flags Details
patch v5 (3.03 KB, patch)
2013-08-14 02:26 PDT, Peter Gal
cdumez: review+
cdumez: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Gal 2013-07-02 08:24:54 PDT
WebKitTestRunner should dump the result as text when the content has text/plain mimetype. This behaviour is also present in the DumpRenderTree.
Comment 1 Peter Gal 2013-07-02 08:34:39 PDT
Created attachment 205918 [details]
patch
Comment 2 Chris Dumez 2013-07-03 01:19:29 PDT
Comment on attachment 205918 [details]
patch

Why doesn't this unskip any test?
Comment 3 Peter Gal 2013-07-03 04:55:05 PDT
(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.
Comment 4 Chris Dumez 2013-07-03 04:56:26 PDT
(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.
Comment 5 Chris Dumez 2013-07-03 05:03:04 PDT
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" ?
Comment 6 Peter Gal 2013-07-03 05:13:23 PDT
(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.
Comment 7 Chris Dumez 2013-07-03 05:35:17 PDT
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.
Comment 8 Peter Gal 2013-07-08 08:24:35 PDT
Created attachment 206247 [details]
patch v2

Now with test unskipping (also tested on mac)
Comment 9 Chris Dumez 2013-07-08 08:26:30 PDT
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.
Comment 10 Peter Gal 2013-07-08 08:29:46 PDT
(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.
Comment 11 Mikhail Pozdnyakov 2013-07-08 08:39:17 PDT
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?
Comment 12 Chris Dumez 2013-07-08 08:42:16 PDT
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?
Comment 13 Mikhail Pozdnyakov 2013-07-08 08:54:29 PDT
(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.
Comment 14 Peter Gal 2013-07-08 09:04:21 PDT
(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?
Comment 15 Chris Dumez 2013-07-08 09:23:32 PDT
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.
Comment 16 Peter Gal 2013-07-08 10:04:36 PDT
Created attachment 206255 [details]
patch v3
Comment 17 Build Bot 2013-07-08 11:34:29 PDT
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
Comment 18 Build Bot 2013-07-08 11:34:31 PDT
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
Comment 19 Build Bot 2013-07-08 14:45:46 PDT
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
Comment 20 Build Bot 2013-07-08 14:45:49 PDT
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
Comment 21 Chris Dumez 2013-07-09 00:00:09 PDT
(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.
Comment 22 Peter Gal 2013-07-09 01:11:37 PDT
(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.
Comment 23 Chris Dumez 2013-07-09 01:13:51 PDT
(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.
Comment 24 Peter Gal 2013-07-09 01:18:00 PDT
(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.
Comment 25 Chris Dumez 2013-07-09 01:19:33 PDT
(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).
Comment 26 Peter Gal 2013-07-09 01:22:06 PDT
(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 :)
Comment 27 Peter Gal 2013-07-23 05:31:29 PDT
Created attachment 207322 [details]
patch v4

Sorry for the big delay.
Comment 28 Peter Gal 2013-07-23 05:32:40 PDT
Comment on attachment 207322 [details]
patch v4

ahh sorry didn't updated the changelog.
Comment 29 Peter Gal 2013-07-23 05:36:00 PDT
Created attachment 207323 [details]
patch v4.1
Comment 30 Chris Dumez 2013-07-23 05:54:43 PDT
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.
Comment 31 Peter Gal 2013-07-23 06:39:02 PDT
(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.
Comment 32 Build Bot 2013-07-28 01:04:01 PDT
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
Comment 33 Build Bot 2013-07-28 01:04:06 PDT
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
Comment 34 Peter Gal 2013-08-14 02:22:51 PDT
(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.
Comment 35 Peter Gal 2013-08-14 02:26:53 PDT
Created attachment 208709 [details]
patch v5

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

as per reviewer request.
Comment 36 Chris Dumez 2013-08-14 03:38:43 PDT
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
Comment 37 Csaba Osztrogonác 2013-08-23 04:18:04 PDT
Committed r154478: <http://trac.webkit.org/changeset/154478>