Bug 45479 - Implement layoutTestController.dumpResourceResponseMIMETypes in Chromium DRT
Summary: Implement layoutTestController.dumpResourceResponseMIMETypes in Chromium DRT
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other All
: P2 Normal
Assignee: Mihai Parparita
URL:
Keywords:
: 45506 (view as bug list)
Depends on: 45505
Blocks:
  Show dependency treegraph
 
Reported: 2010-09-09 11:09 PDT by Mihai Parparita
Modified: 2010-09-11 02:49 PDT (History)
3 users (show)

See Also:


Attachments
Patch (11.77 KB, patch)
2010-09-09 11:13 PDT, Mihai Parparita
no flags Details | Formatted Diff | Diff
Patch (24.38 KB, patch)
2010-09-09 12:35 PDT, Mihai Parparita
no flags Details | Formatted Diff | Diff
Patch (12.57 KB, patch)
2010-09-09 18:33 PDT, Mihai Parparita
no flags Details | Formatted Diff | Diff
Patch (12.24 KB, patch)
2010-09-10 17:33 PDT, Mihai Parparita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mihai Parparita 2010-09-09 11:09:45 PDT
Implement layoutTestController.dumpResourceResponseMIMETypes in Chromium DRT
Comment 1 Mihai Parparita 2010-09-09 11:13:12 PDT
Created attachment 67065 [details]
Patch
Comment 2 Mihai Parparita 2010-09-09 11:18:02 PDT
Tony, can you review this?

test_shell changes are in http://codereview.chromium.org/3317016. As long as those land before this, we should be OK (tests will pass unexpectedly till a WebKit roll picks this up).
Comment 3 Tony Chang 2010-09-09 11:40:30 PDT
Comment on attachment 67065 [details]
Patch

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

> WebKitTools/DumpRenderTree/chromium/LayoutTestController.h:108
> +    // This function sets a flag that tells the test_shell to dump the MIME type
> +    // for each resource that was loaded. It takes no arguments, and ignores any
> +    // that may be present.
> +    void dumpResourceResponseMIMETypes(const CppArgumentList&, CppVariant* result);
Nit: remove |result|

> WebKitTools/DumpRenderTree/chromium/WebViewHost.cpp:922
> +        fputs(" - didReceiveResponse ", stdout);
> +        printResponseDescription(response);
> +        fputs("\n", stdout);
Nit: Can we use puts instead of fputs?

> WebKitTools/DumpRenderTree/chromium/WebViewHost.cpp:930
> +            // Simulate NSURLResponse's mapping of empty/unknown MIME types to application/octet-stream
> +            mimeType.isEmpty() ? "application/octet-stream" : mimeType.utf8().data());
Same question about mimeType.utf().data(), do we need to null terminate it?
Comment 4 Mihai Parparita 2010-09-09 12:35:01 PDT
Created attachment 67081 [details]
Patch
Comment 5 Mihai Parparita 2010-09-09 12:36:16 PDT
(In reply to comment #3)
> Nit: remove |result|

Done.

> Nit: Can we use puts instead of fputs?

Sure, switched the whole file over.
 
> Same question about mimeType.utf().data(), do we need to null terminate it?

CString::init null-terminates, so it should be OK.
Comment 6 Tony Chang 2010-09-09 14:48:23 PDT
Comment on attachment 67081 [details]
Patch

Clearing flags on attachment: 67081

Committed r67119: <http://trac.webkit.org/changeset/67119>
Comment 7 Tony Chang 2010-09-09 14:48:27 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Tony Chang 2010-09-09 17:47:32 PDT
Rolled out.  I'm dumb.  puts adds a newline and fputs doesn't, so we can't just replace them.
Comment 9 Mihai Parparita 2010-09-09 18:30:42 PDT
*** Bug 45506 has been marked as a duplicate of this bug. ***
Comment 10 Mihai Parparita 2010-09-09 18:33:40 PDT
Created attachment 67132 [details]
Patch
Comment 11 Mihai Parparita 2010-09-09 18:34:50 PDT
New version of the patch leaves the fputs calls alone and adds a Linux expectation for fast/preloader/script.html, since we get a different MIME type for script files in file:/// URLs there.
Comment 12 Kent Tamura 2010-09-09 18:40:34 PDT
Comment on attachment 67132 [details]
Patch

Looks ok.
Comment 13 WebKit Commit Bot 2010-09-10 16:09:23 PDT
Comment on attachment 67132 [details]
Patch

Rejecting patch 67132 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Kent Tamura', u'--force']" exit_code: 1
Last 500 characters of output:
nk #2 succeeded at 292 (offset 6 lines).
Hunk #3 succeeded at 493 (offset 6 lines).
patching file WebKitTools/DumpRenderTree/chromium/LayoutTestController.h
Hunk #1 succeeded at 100 (offset 4 lines).
Hunk #2 succeeded at 240 (offset 4 lines).
Hunk #3 succeeded at 337 (offset 5 lines).
Hunk #4 succeeded at 458 (offset 5 lines).
patching file WebKitTools/DumpRenderTree/chromium/TestShell.h
patching file WebKitTools/DumpRenderTree/chromium/WebViewHost.cpp
Hunk #1 succeeded at 920 (offset 5 lines).

Full output: http://queues.webkit.org/results/3941446
Comment 14 Mihai Parparita 2010-09-10 17:33:30 PDT
Created attachment 67272 [details]
Patch
Comment 15 Mihai Parparita 2010-09-10 17:34:55 PDT
Hopefully the latest version of the patch will merge cleanly.
Comment 16 WebKit Commit Bot 2010-09-11 02:49:17 PDT
Comment on attachment 67272 [details]
Patch

Clearing flags on attachment: 67272

Committed r67294: <http://trac.webkit.org/changeset/67294>
Comment 17 WebKit Commit Bot 2010-09-11 02:49:23 PDT
All reviewed patches have been landed.  Closing bug.