[webkit-reviews] review denied: [Bug 86191] Mac DRT should be able to load external URLs for replay performance tests : [Attachment 141497] Adds the feature

[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 141497: Adds the feature
https://bugs.webkit.org/attachment.cgi?id=141497&action=review

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


The idea is perfectly good. I think that the patch would benefit from another
iteration.

> Tools/DumpRenderTree/mac/ResourceLoadDelegate.mm:132
> +    return NSOrderedSame == [host compare:@"127.0.0.1"] || NSOrderedSame ==
[host caseInsensitiveCompare:@"localhost"];

We block IPv6 today, but it seems somewhat undesirable to rely on that here.

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

I'd say "tests", not "test", and also probably "simulate failure" or "generate
error".

> Tools/DumpRenderTree/mac/ResourceLoadDelegate.mm:164
> +	   NSURL *testUrl = done ? 0 : [[NSURL alloc] initWithString: [NSString
stringWithUTF8String: gLayoutTestController->testPathOrURL().c_str()]];
> +	   NSString *testUrlHost = testUrl ? [testUrl host] : 0;

s/Url/URL/

The function you use is named "testPathOrURL". Why is it OK to just create a
URL from its result? This looks suspicious.

> Tools/DumpRenderTree/mac/ResourceLoadDelegate.mm:166
> +	   if (!testUrlHost || (isLocalhost(testUrlHost) && !isLocalhost(host))
|| hostIsUsedBySomeTestToGetError(host)) {
> +	       printf("Blocked access to external URL %s\n", [[url
absoluteString] cStringUsingEncoding:NSUTF8StringEncoding]);

This message is untrue when testUrlHost is nil.

Also, it looks like the message will be printed if
hostIsUsedBySomeTestToGetError returns true, which is a change from original
behavior.