[webkit-reviews] review granted: [Bug 170379] Add sub-commit UI in commit log viewer. : [Attachment 307094] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted dewei_zhu at apple.com's request for
review:
Bug 170379: Add sub-commit UI in commit log viewer.
https://bugs.webkit.org/show_bug.cgi?id=170379

Attachment 307094: Patch

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




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

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

> Websites/perf.webkit.org/public/v3/components/expand-collapse-button.js:6
> +	   this.expanded = false;

This instance variable should be named _expanded instead.

> Websites/perf.webkit.org/public/v3/components/sub-commit-viewer.js:33
> +	   const showSpinner = (this._previousSubCommits &&
this._currentSubCommits) || !this._showingSubCommits;

It appears to me, this boolean is really hideSpinner. When this condition is
true, we're hiding the spinner.
Please either negate the condition or rename the variable.

> Websites/perf.webkit.org/public/v3/components/sub-commit-viewer.js:36
> +	   this.content('spinner-container').style.display = showSpinner ?
'none' : null;

If you negated the condition, don't forget to update this line.

> Websites/perf.webkit.org/public/v3/components/sub-commit-viewer.js:45
> +	   const element = ComponentBase.createElement;

Please add a blank line before this.
Also, I'd prefer declaring element immediately before tableEntries is created.
In general, declare each variable right before it's used to reduce the scope in
which it's usable/visible.
It reduces the number of variables that the reader of the code needs to be
aware of.

> Websites/perf.webkit.org/public/v3/components/sub-commit-viewer.js:53
> +		   element('td', revisions[0]? revisions[0].revision() : ''),
> +		   element('td', revisions[1]? revisions[1].revision() : '')]);

Nit: Spaces are needed before ?.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:39
> +    ownsSubCommits() { return this._rawData['ownsSubCommits']; }
> +    subCommits() { return this._subCommits; }

Why do we have two methods named ownsSubCommits() and subCommits()?
I think the intent here is that you'd check ownsSubCommits(),
and it does, we call fetchSubCommits() to fetch the sub commits.
Once we've done that, we can call subCommits() to retrieve them.

The name subCommits() doesn't communicate this semantics.
I think we should call this method fetchedSubCommits() instead
to make this need of calling fetchSubCommits first clear.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:127
> +	   subCommitRepositories.forEach((subCommitRepository) => {

I'd call this variable just "repository" since this function is diff'ing sub
commits of two commits,
and there's no ambiguity as to which repositories we're talking about here
especially since the thing we're calling on forEach is called
subCommitRepositories.
By the way, I think it's cleaner to use for (const repository of
subCommitRepositories) instead although it's fine as is.

> Websites/perf.webkit.org/server-tests/api-commits-tests.js:397
> +	   it("should handle commit revision with space", () => {

Did you fix this in this patch? Or was this fixed in some other change?

> Websites/perf.webkit.org/server-tests/api-commits-tests.js:449
> +		   assert.equal(results.commits.length, 0);

Why not assert.deepEqual(results.commits, [])?
This way, when commits start containing results, we'd see it in the error
message (makes debugging easier).

> Websites/perf.webkit.org/server-tests/api-commits-tests.js:464
> +		   assert.equal(results.commits.length, 0);

Ditto.

> Websites/perf.webkit.org/server-tests/api-manifest-tests.js:103
> +	   let db = TestServer.database();

Use const instead of let since we're not modifying it.
Ditto for the rest of the test.

> Websites/perf.webkit.org/unit-tests/commit-log-tests.js:172
> +	       MockRemoteAPI.reset('http://build.webkit.org');

Why are we doing this!? What's the point of pointing MockRemoteAPI to
build.webkit.org?
This should be only needed in the tests for buildbot syncing scripts.

> Websites/perf.webkit.org/unit-tests/commit-log-tests.js:181
> +		   assert.equal(error, undefined);

I don't think we really need to assert the error is undefined.

> Websites/perf.webkit.org/unit-tests/commit-log-tests.js:190
> +		   assert.equal(error, undefined);

Ditto.

> Websites/perf.webkit.org/unit-tests/commit-log-tests.js:196
> +	       assert.equal(MockRemoteAPI.requests.length, 1);

Instead of keep referring to MockRemoteAPI.requests,
why don't we just store const requests = MockRemoteAPI.requests after
beforeEach?
MockRemoteAPI is designed so that MockRemoteAPI.requests never changes from
tests to tests.

> Websites/perf.webkit.org/unit-tests/commit-log-tests.js:228
> +	       MockRemoteAPI.reset();

Why do we need to call this?

> Websites/perf.webkit.org/unit-tests/commit-log-tests.js:238
> +		   assert.equal(MockRemoteAPI.requests.length, 0);

We can just assert that the request.length is still 1 instead.

> Websites/perf.webkit.org/unit-tests/commit-log-tests.js:269
> +	       MockRemoteAPI.reset();

Why do we need to call this?