Bug 116125 - pathSuitableForTestResult returns duplicate path
Summary: pathSuitableForTestResult returns duplicate path
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Minor
Assignee: Nobody
URL:
Keywords:
Depends on: 116572
Blocks:
  Show dependency treegraph
 
Reported: 2013-05-14 15:10 PDT by Alex Christensen
Modified: 2013-08-08 10:41 PDT (History)
4 users (show)

See Also:


Attachments
Patch (4.84 KB, patch)
2013-05-14 15:15 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-cq-03 for mac-mountainlion (818.04 KB, application/zip)
2013-05-14 18:40 PDT, WebKit Commit Bot
no flags Details
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (722.42 KB, application/zip)
2013-05-15 04:46 PDT, Build Bot
no flags Details
Patch (11.35 KB, patch)
2013-05-20 12:03 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (12.92 KB, patch)
2013-05-20 12:10 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (12.68 KB, patch)
2013-05-20 12:55 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (12.82 KB, patch)
2013-05-20 13:41 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2013-05-14 15:10:48 PDT
When running LayoutTests/webarchive/loading/test-loading-archive-subresource-null-mimetype.html, WTR::pathSuitableForTestResult is given the string "file:///test.png" and returns "test.png/test.png" because WKURLCopyPath returns "/test.png" which sets indexBaseName to 0, which is not handled correctly.  This patch fixes this minor bug which is only encountered in this test case, which should be unskipped.
Comment 1 Alex Christensen 2013-05-14 15:15:57 PDT
Created attachment 201763 [details]
Patch
Comment 2 WebKit Commit Bot 2013-05-14 18:39:59 PDT
Comment on attachment 201763 [details]
Patch

Rejecting attachment 201763 [details] from commit-queue.

New failing tests:
webarchive/loading/test-loading-archive-subresource-null-mimetype.html
Full output: http://webkit-queues.appspot.com/results/468672
Comment 3 WebKit Commit Bot 2013-05-14 18:40:00 PDT
Created attachment 201777 [details]
Archive of layout-test-results from webkit-cq-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: webkit-cq-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.3
Comment 4 Build Bot 2013-05-15 04:46:22 PDT
Comment on attachment 201763 [details]
Patch

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

New failing tests:
webarchive/loading/test-loading-archive-subresource-null-mimetype.html
Comment 5 Build Bot 2013-05-15 04:46:23 PDT
Created attachment 201818 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.2
Comment 6 Alex Christensen 2013-05-15 11:42:23 PDT
One of these failures is my fault.  I should have made pathSuitableForTestResult behave the same as DumpRenderTree's _drt_descriptionSuitableForTestResult
Comment 7 Alex Christensen 2013-05-20 12:03:33 PDT
Created attachment 202300 [details]
Patch
Comment 8 Alex Christensen 2013-05-20 12:10:25 PDT
Created attachment 202301 [details]
Patch
Comment 9 Tim Horton 2013-05-20 12:20:34 PDT
Comment on attachment 202301 [details]
Patch

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

> Tools/ChangeLog:22
> +        Pass the main frame's URL to pathSuitableForTestResult.
> +        (WTR::dumpResponseDescriptionSuitableForTestResult):
> +        Pass the main frame's URL to pathSuitableForTestResult.
> +        (WTR::InjectedBundlePage::willPerformClientRedirectForFrame):
> +        Pass the main frame's URL to pathSuitableForTestResult.
> +        (WTR::InjectedBundlePage::didInitiateLoadForResource):
> +        Pass the main frame's URL to pathSuitableForTestResult.
> +        (WTR::InjectedBundlePage::willSendRequestForFrame):
> +        Pass the main frame's URL to pathSuitableForTestResult.
> +        (WTR::InjectedBundlePage::didReceiveResponseForResource):
> +        Pass the main frame's URL to pathSuitableForTestResult.

You could have this once at the bottom of the set of things-that-have-the-same-comment.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:605
> +    static_cast<InjectedBundlePage*>(const_cast<void*>(clientInfo))->willPerformClientRedirectForFrame(frame, url, delay, date, WKBundleFrameCopyURL(WKBundlePageGetMainFrame(page)));

Who owns the string copy you're making here? Who will free it?
Comment 10 Alex Christensen 2013-05-20 12:55:09 PDT
Created attachment 202309 [details]
Patch
Comment 11 Alex Christensen 2013-05-20 13:41:16 PDT
Created attachment 202315 [details]
Patch
Comment 12 Tim Horton 2013-05-20 13:55:13 PDT
Comment on attachment 202315 [details]
Patch

Looks reasonable to me, assuming the tests pass.
Comment 13 WebKit Commit Bot 2013-05-20 16:27:43 PDT
Comment on attachment 202315 [details]
Patch

Clearing flags on attachment: 202315

Committed r150386: <http://trac.webkit.org/changeset/150386>
Comment 14 WebKit Commit Bot 2013-05-20 16:27:46 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 WebKit Commit Bot 2013-05-21 13:18:38 PDT
Re-opened since this is blocked by bug 116572
Comment 16 Alexey Proskuryakov 2013-08-08 10:41:02 PDT
Committed with minor tweaks in <http://trac.webkit.org/changeset/153830>.