[webkit-reviews] review denied: [Bug 68234] Web Inspector: UI performance tests are required. : [Attachment 107624] [patch] initial version

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Ilya Tikhonovsky
<loislo at chromium.org>'s request for review:
Bug 68234: Web Inspector: UI performance tests are required.
https://bugs.webkit.org/show_bug.cgi?id=68234

Attachment 107624: [patch] initial version
https://bugs.webkit.org/attachment.cgi?id=107624&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=107624&action=review


> LayoutTests/inspector/performance/show-panel-network.html:19
> +    RuntimeAgent.evaluate("makeXHRRequests(30)");

You should do InspectorTest.evaluateInPage("...", callback)

> LayoutTests/inspector/performance/show-panel-network.html:21
> +    var Test = function() {

I don't think declarative notation will be flexible enough for this kind of
tests. I'd suggest to stick with imperative form:

InspectorTest.runPerformanceTest({
    setUp: function(done)
    {
	WebInspector.showPanel("audits");
	done();
    },

    run: function(done)
    {
	WebInspector.showPanel("network");
	WebInspector.panels.network.refresh();
	done();
    }
}, "show-network-panel", 1000);

Or

InspectorTest.runPerformanceTest({
    run: function(timer, done)
    {
	WebInspector.showPanel("audits");

	var timer = startTimer("show-network-panel");
	WebInspector.showPanel("network");
	WebInspector.panels.network.refresh();
	timer.endTimer();

	done();
    }
}, 1000);

> LayoutTests/inspector/performance/show-panel-network.html:23
> +	   this.object = WebInspector;

This is too generic.

> LayoutTests/inspector/performance/show-panel-network.html:27
> +	       WebInspector.showPanel("console");

console is animating, choose audits over it.

> LayoutTests/platform/chromium/test_expectations.txt:582
> +WONTFIX SKIP : inspector/performance = TEXT TIMEOUT

If you put the test to under inspector/performance/resources, they won't run
automatically.

> Source/WebCore/inspector/front-end/ElementsPanel.js:300
> +	   WebInspector.setCurrentPanel(this);

I'd suggest that you land this change separately + convert currentPanel getter
to a function (currentPanel()).