[webkit-reviews] review denied: [Bug 197973] 'analysis-task-configurator-pane' does not update when switch from one analysis task to another : [Attachment 372586] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Zhifei Fang
<zhifei_fang at apple.com>'s request for review:
Bug 197973: 'analysis-task-configurator-pane' does not update when switch from
one analysis task to another
https://bugs.webkit.org/show_bug.cgi?id=197973

Attachment 372586: Patch

https://bugs.webkit.org/attachment.cgi?id=372586&action=review




--- Comment #14 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 372586
  --> https://bugs.webkit.org/attachment.cgi?id=372586
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=372586&action=review

r-. Let's not invent brand new mechanism to write tests just for this one off
case.
We can talk about the benefit of improving ways to mock data but we need to
tackle that in a separate patch.
In general, it would be a good practice to have a discussion in person or over
email before embarking on a big patch like that.

> Websites/perf.webkit.org/ChangeLog:6
> +	   Reviewed by NOBODY (OOPS!).

Nit: Need a blank line after this.

> Websites/perf.webkit.org/browser-tests/analysis-task-page-tests.js:35
> +	   const mockData = new ModelV3MockData(context);
> +	   await mockData.setup();
> +	   mockData.Manifest();

Code like this makes it completely opaque as to what this mock data is setting
up.
In general, we really want all the test data to be next to the tests.

> Websites/perf.webkit.org/browser-tests/analysis-task-page-tests.js:41
> +	   // Wait for the RemoteAPI triggered.
> +	   await wait(0);

This is precisely what context.symbols.MockRemoteAPI.requests is for. See
chart-revision-range-tests.js for an example.

> Websites/perf.webkit.org/browser-tests/mock-data.js:7
> +    'models/data-model.js',

Let's not have yet another list of V3 model files.

> Websites/perf.webkit.org/browser-tests/mock-data.js:60
> +class MockData {

This is too complicated of a setup for mock data.
With this much code running, we have to verify that this mock data setup is
correct.
In general, we should keep the test data and its expectation as simple as
possible.

> Websites/perf.webkit.org/browser-tests/mock-data.js:226
> +	   this._context.iframe.contentWindow.RemoteAPI = {

We don't want to be overriding all tests' RemoteAPI like this.

>
Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator
.js:165
> +	   const fileUploaderEventHandler =  () => {

Nit: Two spaces between = and ().

>
Websites/perf.webkit.org/public/v3/components/custom-configuration-test-group-f
orm.js:27
> +	   if (configurator.hasUserModified()) {
> +	       return;
> +	   }

Nit: No braces around a single statement.

>
Websites/perf.webkit.org/public/v3/components/custom-configuration-test-group-f
orm.js:36
> +    resetUserModified() {

Nit: { should appear on the next line.

>
Websites/perf.webkit.org/public/v3/components/custom-configuration-test-group-f
orm.js:38
> +	   const configurator = this.part('configurator');
> +	   configurator.resetUserModified();

This should really be one liner: this.part('configurator').resetUserModified()

> Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:208
> +	   this._currentTestGroups = null;

Why does this need to say "current"? It seems sufficient to say
this._testGroup?

> Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:221
> +	   if (!this._currentTestGroups) {

Nit: No braces around a singe line statement.

> Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:224
> +	   if (this._currentTestGroups.length !== testGroups.length) {

Nit: Braces.
There is no point in using === here. Just use ==.

> Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:225
> +	     return false;

Nit: Wrong indentation.

> Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:228
> +	       if (testGroups[i].id() !== this._currentTestGroups[i].id()) {

Again, no need to use !==. id() is always a number.

Nit: Wrong braces & indentation.

> Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:238
> +	   if (!this._compareTestGroups(testGroups)) {

Why does it matter that the list of test groups had changed?
This will only happen when the user had unhid hidden test groups or hid a test
group.
Why does that matter? Or are you concerned about the case when analysis task
had changed?
In that case, a better approach is to pass the analysis task ID as a separate
argument, or check the task ID of the test groups.

> Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:242
> +	       if (!this._currentGroup || this._currentGroup.id() !==
currentGroup.id()) {

Ditto.