RESOLVED FIXED 42331
WebKitTestRunner needs layoutTestController.dumpFrameLoadCallbacks
https://bugs.webkit.org/show_bug.cgi?id=42331
Summary WebKitTestRunner needs layoutTestController.dumpFrameLoadCallbacks
Maciej Stachowiak
Reported 2010-07-14 20:55:26 PDT
WebKitTestRunner needs layoutTestController.dumpFrameLoadCallbacks
Attachments
Patch (19.09 KB, patch)
2012-05-07 02:37 PDT, Jon Lee
no flags
Patch (19.09 KB, patch)
2012-05-07 02:41 PDT, Jon Lee
darin: review+
Maciej Stachowiak
Comment 1 2010-07-14 20:59:26 PDT
Jon Lee
Comment 2 2012-05-07 02:37:14 PDT
Jon Lee
Comment 3 2012-05-07 02:41:43 PDT
Created attachment 140499 [details] Patch Missing trailing space for willPerformClientRedirectToURL
Darin Adler
Comment 4 2012-05-07 10:02:59 PDT
Comment on attachment 140499 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140499&action=review r=me if you fix the storage leaks > Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:197 > +bool booleanForKey(WKDictionaryRef dictionary, const char* key) > +{ > + WKRetainPtr<WKStringRef> wkKey(AdoptWK, WKStringCreateWithUTF8CString(key)); > + return WKBooleanGetValue(static_cast<WKBooleanRef>(WKDictionaryGetItemForKey(dictionary, wkKey.get()))); > +} This function does a bad cast if the value is not a boolean. And it calls WKBooleanGetValue on a null pointer of there is no item in the dictionary with that key. Given that this is part of a testing framework, I was thinking it might be even better to check the type and do something different in those cases, like report an error somehow. > Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.h:109 > + void beginTesting(WKDictionaryRef); This argument needs a name. It’s not obvious why beginTesting would take a dictionary argument. > Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:350 > +// Similar to -[WebFrame _drt_descriptionSuitableForTestResult] Not sure that “similar” is a helpful comment. Maybe there’s something stronger you want to say here? > Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:780 > + InjectedBundle::shared().stringBuilder()->append(toWTFString(WKURLCopyString(url))); There’s a storage leak here. You can fix it by writing adoptWK(WKURLCopyString(url)).get() instead of WKURLCopyString(url). > Tools/WebKitTestRunner/InjectedBundle/LayoutTestController.h:75 > + void dumpFrameLoadCallbacks(bool value = true) { m_dumpFrameLoadCallbacks = value; } These functions have really bad names, and it’s even worse for a version that takes a boolean. setShouldDumpFrameLoadCallbacks should be a far better name. > Tools/WebKitTestRunner/TestInvocation.cpp:132 > + return strstr(pathOrURL, "loading/"); Is this really the right rule? Who came up with this? We are seriously hardcoding this into all our test runner tools?
Darin Adler
Comment 5 2012-05-07 10:03:35 PDT
(In reply to comment #4) > r=me if you fix the storage leaks storage leak singular I mean
Jon Lee
Comment 6 2012-05-07 11:24:18 PDT
Comment on attachment 140499 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140499&action=review >> Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:197 >> +} > > This function does a bad cast if the value is not a boolean. And it calls WKBooleanGetValue on a null pointer of there is no item in the dictionary with that key. Given that this is part of a testing framework, I was thinking it might be even better to check the type and do something different in those cases, like report an error somehow. Sure, I can make this function behave a lot better. I will have it output an error. >> Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.h:109 >> + void beginTesting(WKDictionaryRef); > > This argument needs a name. It’s not obvious why beginTesting would take a dictionary argument. Done. >> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:780 >> + InjectedBundle::shared().stringBuilder()->append(toWTFString(WKURLCopyString(url))); > > There’s a storage leak here. You can fix it by writing adoptWK(WKURLCopyString(url)).get() instead of WKURLCopyString(url). Done. >> Tools/WebKitTestRunner/InjectedBundle/LayoutTestController.h:75 >> + void dumpFrameLoadCallbacks(bool value = true) { m_dumpFrameLoadCallbacks = value; } > > These functions have really bad names, and it’s even worse for a version that takes a boolean. setShouldDumpFrameLoadCallbacks should be a far better name. I wanted to avoid having two of these, since dumpFrameLoadCallbacks is part of the layoutTestController idl, but it's easy to have the setter function for WTR usage, and leave this function in for JS. >> Tools/WebKitTestRunner/TestInvocation.cpp:132 >> + return strstr(pathOrURL, "loading/"); > > Is this really the right rule? Who came up with this? We are seriously hardcoding this into all our test runner tools? There's a whole series of these in DRT.
Jon Lee
Comment 7 2012-05-07 12:21:13 PDT
Note You need to log in before you can comment on or make changes to this bug.