WebKitTestRunner needs layoutTestController.dumpDOMAsWebArchive
<rdar://problem/8193633>
Created attachment 200863 [details] Patch
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 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?
It looks like you need to wrap some of this stuff in #if PLATFORM(MAC) or #if USE(CF) or something.
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 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.
Created attachment 200927 [details] Patch
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 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, ...));
Created attachment 200967 [details] Patch
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?
Created attachment 200970 [details] Patch
Comment on attachment 200970 [details] Patch Clearing flags on attachment: 200970 Committed r149692: <http://trac.webkit.org/changeset/149692>
All reviewed patches have been landed. Closing bug.
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.