[webkit-reviews] review granted: [Bug 48002] network-simulator.php makes for very slow layout tests : [Attachment 72250] faster network simulator

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Michael Nordman
<michaeln at google.com>'s request for review:
Bug 48002: network-simulator.php makes for very slow layout tests
https://bugs.webkit.org/show_bug.cgi?id=48002

Attachment 72250: faster network simulator
https://bugs.webkit.org/attachment.cgi?id=72250&action=review

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

OK. Please address the comments, and verify how much of a performance win the
test change is.

> LayoutTests/ChangeLog:9
> +	   multiple content-length headers. Chrome's network stacks consider
that an

Typo: considers.

> LayoutTests/ChangeLog:11
> +	   This avoids time outs due to using PHP via slow CGI on windows.

Typo: timeouts.

> LayoutTests/ChangeLog:21
> +	   * http/tests/resources/offline-access.html:

This change is in http/tests/appcache/resources/offline-access-frame.html, not
offline-access.html.

> LayoutTests/http/tests/appcache/fallback.html:57
> +	   log("FAIL:Cannot load a URL from fallback namespace when network is
enabled, ex = " + ex);

Missing space.

> LayoutTests/http/tests/appcache/fallback.html:59
>	   hadError = true;
>      }

I remembered what the benefit from canLoad() was - it helped write more concise
and readable code. It's much harder to read the new version of the test.

If you get measurable speedup from these changes, let's land them - but if it
can't be measured, let's keep the test as is.

> LayoutTests/http/tests/appcache/fallback.html:149
> -    if (!canLoad(testURL)) {
> -	   log("FAIL: Cannot load an URL from fallback namespace when network
is disabled");
> -	   hadError = true;
> -    } else if (load(testURL) != load("resources/simple.txt")) {
> -	   log("FAIL: Loading an URL from fallback namespace when network is
disabled returned unexpected response");
> +    try {
> +	 if (load(testURL) != load("resources/simple.txt")) {

There is a slight difference here - an exception from loading
resources/simple.txt would be attributed to loading testURL. Not a big deal.