Bug 42324 - WebKitTestRunner needs testRunner.dumpDOMAsWebArchive
Summary: WebKitTestRunner needs testRunner.dumpDOMAsWebArchive
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 115745 115795
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-14 20:50 PDT by Maciej Stachowiak
Modified: 2013-05-08 05:02 PDT (History)
4 users (show)

See Also:


Attachments
Patch (30.90 KB, patch)
2013-05-06 18:33 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (11.37 KB, patch)
2013-05-07 10:46 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (30.82 KB, patch)
2013-05-07 13:44 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (30.77 KB, patch)
2013-05-07 13:55 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 Maciej Stachowiak 2010-07-14 20:50:40 PDT
WebKitTestRunner needs layoutTestController.dumpDOMAsWebArchive
Comment 1 Maciej Stachowiak 2010-07-14 20:58:21 PDT
<rdar://problem/8193633>
Comment 2 Alex Christensen 2013-05-06 18:33:51 PDT
Created attachment 200863 [details]
Patch
Comment 3 WebKit Commit Bot 2013-05-06 18:35:00 PDT
Attachment 200863 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl', u'Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp', u'Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.h', u'Tools/WebKitTestRunner/InjectedBundle/TestRunner.h', u'Tools/WebKitTestRunner/WebKitTestRunner.xcodeproj/project.pbxproj', u'Tools/WebKitTestRunner/cf/WebArchiveDumpSupport.cpp', u'Tools/WebKitTestRunner/cf/WebArchiveDumpSupport.h', u'Tools/WebKitTestRunner/mac/WebArchiveDumpSupportMac.mm']" exit_code: 1
Tools/WebKitTestRunner/cf/WebArchiveDumpSupport.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
Tools/WebKitTestRunner/cf/WebArchiveDumpSupport.cpp:35:  The parameter name "response" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/WebKitTestRunner/cf/WebArchiveDumpSupport.cpp:36:  The parameter name "response" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/WebKitTestRunner/cf/WebArchiveDumpSupport.cpp:37:  The parameter name "response" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/WebKitTestRunner/cf/WebArchiveDumpSupport.cpp:38:  The parameter name "response" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/WebKitTestRunner/cf/WebArchiveDumpSupport.cpp:39:  The parameter name "response" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/WebKitTestRunner/cf/WebArchiveDumpSupport.cpp:152:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Tools/WebKitTestRunner/cf/WebArchiveDumpSupport.cpp:230:  Could not find a newline character at the end of the file.  [whitespace/ending_newline] [5]
Total errors found: 8 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Tim Horton 2013-05-06 18:41:03 PDT
Comment on attachment 200863 [details]
Patch

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

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:843
> +    // create web archive data from the frame

Comments should start with capital letters and end with punctuation.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:849
> +    CFDataRef cfData = CFDataCreate(0, bytes, length);

I find it vaguely surprising we don't have a better way to make a CF/NSData from a WKData, but perhaps we don't.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:850
> +    CFStringRef cfString = createXMLStringFromWebArchiveData(cfData);

No clear need for this local.

> Tools/WebKitTestRunner/cf/WebArchiveDumpSupport.cpp:1
> +/*

It would of course be nice not to copy these files, but Sam and Alex and I don't really know of a good shared place.

> Tools/WebKitTestRunner/cf/WebArchiveDumpSupport.h:38
> +CFURLResponseRef createCFURLResponseFromResponseData(CFDataRef responseData);

You mentioned that we might not need this. If you find out that you don't, can you remove it from the copied files?
Comment 5 Tim Horton 2013-05-06 18:41:52 PDT
It looks like you need to wrap some of this stuff in #if PLATFORM(MAC) or #if USE(CF) or something.
Comment 6 Tim Horton 2013-05-06 18:47:19 PDT
Comment on attachment 200863 [details]
Patch

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

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:846
> +    // create an xml string from the web archive data

Also, these are both "what" comments, not "why" comments, and they describe exactly what the code is already doing, so there's really no reason to have them at all.
Comment 7 Tim Horton 2013-05-06 19:09:36 PDT
Comment on attachment 200863 [details]
Patch

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

> Tools/ChangeLog:10
> +        added dumpDOMAsWebArchive javascript function to be called by test cases

These should be capitalized/punctuated too.

> Tools/ChangeLog:13
> +        added dumpDOMAsWebArchive c++ code that is called when dumping        

"C++ code" bit is ... unnecessary.

> Tools/ChangeLog:18
> +        added dumpDOMAsWebArchive c++ declaration

Here too.
Comment 8 Alex Christensen 2013-05-07 10:46:22 PDT
Created attachment 200927 [details]
Patch
Comment 9 Tim Horton 2013-05-07 11:25:41 PDT
Comment on attachment 200927 [details]
Patch

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

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:850
> +    const unsigned char* bytes = WKDataGetBytes(wkData);
> +    size_t length = WKDataGetSize(wkData);
> +    CFDataRef cfData = CFDataCreate(0, bytes, length);

This could be all-in-one-line too.

Also, you're leaking the CFData object.
Comment 10 Tim Horton 2013-05-07 11:28:14 PDT
Comment on attachment 200927 [details]
Patch

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

>> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:850
>> +    CFDataRef cfData = CFDataCreate(0, bytes, length);
> 
> This could be all-in-one-line too.
> 
> Also, you're leaking the CFData object.

You should use something like RetainPtr<CFDataRef> cfData = adoptCF(CFDataCreate(0, ...));
Comment 11 Alex Christensen 2013-05-07 13:44:13 PDT
Created attachment 200967 [details]
Patch
Comment 12 Tim Horton 2013-05-07 13:50:20 PDT
Comment on attachment 200967 [details]
Patch

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

> Tools/ChangeLog:10
> +        Added dumpDOMAsWebArchive javascript function to be called by test cases.

JavaScript

> Tools/ChangeLog:21
> +        Added dumpDOMAsWebArchive JS callback function that sets m_whatToDump to DOMASWebArchive.

I think it's "As" not "AS". Too many caps :)

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:53
> +#import <CoreFoundation/CoreFoundation.h>

Isn't this already included by WebArchiveDumpSupport.h?
Comment 13 Alex Christensen 2013-05-07 13:55:00 PDT
Created attachment 200970 [details]
Patch
Comment 14 WebKit Commit Bot 2013-05-07 14:26:12 PDT
Comment on attachment 200970 [details]
Patch

Clearing flags on attachment: 200970

Committed r149692: <http://trac.webkit.org/changeset/149692>
Comment 15 WebKit Commit Bot 2013-05-07 14:26:14 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Tim Horton 2013-05-07 14:30:20 PDT
Comment on attachment 200970 [details]
Patch

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

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:848
> +    WKDataRef wkData = WKBundleFrameCopyWebArchive(frame);

Anders points out that this is also an object you own, so you should WKRetainPtr it up.