[webkit-reviews] review denied: [Bug 55306] Web Inspector: add first network test, improve harness. : [Attachment 84021] Patch

[bugzilla-daemon at webkit.org]

Yury Semikhatsky <yurys at chromium.org> has denied Pavel Feldman
<pfeldman at chromium.org>'s request for review:
Bug 55306: Web Inspector: add first network test, improve harness.
https://bugs.webkit.org/show_bug.cgi?id=55306

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

------- Additional Comments from Yury Semikhatsky <yurys at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=84021&action=review

> LayoutTests/http/tests/inspector/inspector-test.js:308
>      layoutTestController.closeWebInspector();

Please delete watchdog timer here to make sure we don't close inspector twice.

> LayoutTests/http/tests/inspector/network/network-size.html:33
> +	   var resource1 = WebInspector.panels.network.resources[0];

Image order might change. Can you sort the results to avoid inconsistent
output? r- for this.

> LayoutTests/http/tests/inspector/network/network-timing.html:34
> +	   InspectorTest.addResult(resource1.url);

Results' order should not depend on network activity.

> LayoutTests/http/tests/inspector/network/network-timing.html:36
> +	   InspectorTest.assertGreaterOrEqual(300, resource1.duration * 1000,
"Duration of the first resource");

I don't like that we deliberately slow down layout tests. Can we use
mocks/something else to avoid this. Inspector tests are already notorious for
being quite slow.

> LayoutTests/http/tests/inspector/network/network-timing.html:41
> +	   InspectorTest.assertGreaterOrEqual(100, resource2.latency * 1000,
"Latency of the second resource");

I think we should print something in case of success.

> LayoutTests/http/tests/inspector/network/resources/resource.php:42
> +    var now = new Date();

Just use Date.now(). This way you won't need Number(now) cast below.

> LayoutTests/http/tests/inspector/network/resources/resource.php:55
> +	       echo("function foo() {}");

Could be rewritten as
if (!$jscontent)
    $jscontent = "function foo() {}";
and use same logic for both branches.