WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(19.09 KB, patch)
2012-05-07 02:41 PDT
,
Jon Lee
darin
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Maciej Stachowiak
Comment 1
2010-07-14 20:59:26 PDT
<
rdar://problem/8193641
>
Jon Lee
Comment 2
2012-05-07 02:37:14 PDT
Created
attachment 140498
[details]
Patch
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
Committed
r116338
: <
http://trac.webkit.org/changeset/116338
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug