[Webkit-unassigned] [Bug 170379] Add sub-commit UI in commit log viewer.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Apr 14 01:19:02 PDT 2017


Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
 Attachment #307094|review?                     |review+
              Flags|                            |

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

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);


> 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);


> 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?

You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170414/f2fcb931/attachment-0001.html>

More information about the webkit-unassigned mailing list