[webkit-reviews] review granted: [Bug 131954] [iOS WK2] Add test URL to crash reports for the UI process, clean up project : [Attachment 229836] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 131954: [iOS WK2] Add test URL to crash reports for the UI process, clean
up project
https://bugs.webkit.org/show_bug.cgi?id=131954

Attachment 229836: Patch
https://bugs.webkit.org/attachment.cgi?id=229836&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=229836&action=review


> Tools/WebKitTestRunner/cocoa/CrashReporterInfo.mm:26
> +#import "config.h"

Where’s the include of CrashReporterInfo.h?

> Tools/WebKitTestRunner/cocoa/CrashReporterInfo.mm:33
> +#import <wtf/text/WTFString.h>

I don’t think you need this include if you’re including StringBuilder.h.

> Tools/WebKitTestRunner/cocoa/CrashReporterInfo.mm:37
> +using namespace WTF;

Should not need this.

> Tools/WebKitTestRunner/cocoa/CrashReporterInfo.mm:48
> +    String schemeString(schemeCFString.get());

Seems a shame to convert this CFStringRef to a WTF::String just to call
equalIgnoringCase. Doesn’t CF have functions that can do that?

> Tools/WebKitTestRunner/cocoa/CrashReporterInfo.mm:52
> +	   String layoutTests("/LayoutTests/");

Doesn’t seem like this should be a WTF::String. Should use a const char[]
instead.

> Tools/WebKitTestRunner/cocoa/CrashReporterInfo.mm:64
> +    String hostString(hostCFString.get());

Doesn’t seem necessary to convert this to a WTF::String just to compare it with
"127.0.0.1".

> Tools/WebKitTestRunner/cocoa/CrashReporterInfo.mm:74
> +    if (!testPath.isNull()) {

I think early return is better.

> Tools/WebKitTestRunner/cocoa/CrashReporterInfo.mm:78
> +	   StringBuilder builder;
> +	   builder.appendLiteral("CRASHING TEST: ");
> +	   builder.append(testPath);
> +	  
WKSetCrashReportApplicationSpecificInformation(builder.toString().createCFStrin
g().get());

No need for StringBuilder just to concatenate. Can write:

    WKSetCrashReportApplicationSpecificInformation(("CRASHING TEST: " +
testPath).createCFString().get());