[webkit-reviews] review denied: [Bug 86191] Mac DRT should be able to load external URLs for replay performance tests : [Attachment 143164] Fixed per Alexey's comment

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Ryosuke Niwa
<rniwa at webkit.org>'s request for review:
Bug 86191: Mac DRT should be able to load external URLs for replay performance
tests
https://bugs.webkit.org/show_bug.cgi?id=86191

Attachment 143164: Fixed per Alexey's comment
https://bugs.webkit.org/attachment.cgi?id=143164&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=143164&action=review


Looks better. I think that I should look at the final variant once more, so r-.


> Tools/DumpRenderTree/mac/ResourceLoadDelegate.mm:130
> +BOOL isLocalhost(const NSString *host)

There should be no "const" on NSString - this class is immutable.

> Tools/DumpRenderTree/mac/ResourceLoadDelegate.mm:136
> +BOOL hostIsUsedBySomeTestsToToGenerateError(const NSString *host)

Ditto.

ToTo?

> Tools/DumpRenderTree/mac/ResourceLoadDelegate.mm:164
> +	   NSString *testURLString = [NSString stringWithUTF8String:
gLayoutTestController->testPathOrURL().c_str()];

In WebKit code, there is usually no space after colon.

Maybe the variable should be called "testPathOrURLString" to match function
name?

> Tools/DumpRenderTree/mac/ResourceLoadDelegate.mm:165
> +	   NSString *lowercaseTestURL = [testURLString lowercaseString];

I would just combine these into a chain call: 

NSString *lowercaseTestPathOrURL = [[NSString stringWithUTF8String:
gLayoutTestController->testPathOrURL().c_str()] lowercaseString];

> Tools/DumpRenderTree/mac/ResourceLoadDelegate.mm:167
> +	   if ([lowercaseTestURL hasPrefix: [NSString stringWithCString:
"http:"]] || [lowercaseTestURL hasPrefix: [NSString stringWithCString:
"https:"]]) {

Instead of stringWithCString, please use literals: @"http:".

> Tools/DumpRenderTree/mac/ResourceLoadDelegate.mm:169
> +	       NSURL *testURL = [[NSURL alloc] initWithString: testURLString];
> +	       testHost = testURL ? [testURL host] : 0;

Alloc/init create an object that needs to be released. Also, it's usually fine
to call methods on nil objects (with rare exceptions that depend on return
type). A better way to write this is:

testHost = [[NSURL URLWithString:testURLString] host];