Bug 42324 – WebKitTestRunner needs testRunner.dumpDOMAsWebArchive

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Maciej Stachowiak]

WebKitTestRunner needs layoutTestController.dumpDOMAsWebArchive

[Maciej Stachowiak]

<rdar://problem/8193633>

[Alex Christensen]

Created attachment 200863 [details]
Patch

[WebKit Commit Bot]

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.

[Tim Horton]

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?

[Tim Horton]

It looks like you need to wrap some of this stuff in #if PLATFORM(MAC) or #if USE(CF) or something.

[Tim Horton]

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.

[Tim Horton]

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.

[Alex Christensen]

Created attachment 200927 [details]
Patch

[Tim Horton]

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.

[Tim Horton]

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, ...));

[Alex Christensen]

Created attachment 200967 [details]
Patch

[Tim Horton]

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?

[Alex Christensen]

Created attachment 200970 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 200970 [details]
Patch

Clearing flags on attachment: 200970

Committed r149692: <http://trac.webkit.org/changeset/149692>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Tim Horton]

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.