Bug 170379 – Add sub-commit UI in commit log viewer.

• Component: New Bugs
• Status: RESOLVED FIXED

Add sub-commit UI in commit log viewer.

Created attachment 306044 [details]
Patch

Created attachment 306045 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 306045 [details]
Patch

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

> Websites/perf.webkit.org/ChangeLog:17
> +        * public/include/manifest-generator.php:

Please add function headers with comments.
e.g.
(ManifestGenerator::generate):
(ManifestGenerator::repositories):

> Websites/perf.webkit.org/ChangeLog:40
> +        (then):

You should get rid of these (then) comments.

> Websites/perf.webkit.org/ChangeLog:47
> +        (beforeEach):
> +        (return.commit.fetchSubCommits.catch):
> +        (return.fetchingPromise.then):
> +        (then):

As well as these.

> Websites/perf.webkit.org/public/api/commits.php:35
> +        $commit_revision = array_get($_GET, 'commit-revision');

I think should be either just 'revision' or 'owner-revision'.

> Websites/perf.webkit.org/public/include/commit-log-fetcher.php:103
> +            FROM commits owned INNER JOIN commit_ownerships ON owned.commit_id = commit_owned

Please use the standard "AS" like commits AS owned.
Also, there's no need to specify INNER. Just say JOIN.
I think it's actually more readable to say:
FROM commits AS owned, commits AS owner, commit_ownerships
WHERE owned.commit_id = commit_owned AND commit_owner = owner.commit_id
    AND owner.commit_revision = $1 AND owner.commit_repository = $2

> Websites/perf.webkit.org/public/include/commit-log-fetcher.php:159
> +        $owns_sub_commits = boolval($this->db->select_first_row('commit_ownerships', 'commit', array('owner' => $commit_row['commit_id'])));

Instead of boolval, do !!.

> Websites/perf.webkit.org/public/include/commit-log-fetcher.php:163
> +    private function format_commit($commit_row, $committer_row, $owns_sub_commits) {

Please fix fetch_for_tasks to call this with the right arguments.

> Websites/perf.webkit.org/public/include/manifest-generator.php:19
> -        $repositories_table = $this->db->fetch_table('repositories');
> +        $repositories_table = $this->db->query_and_fetch_all('SELECT repository_id, repository_name, repository_url, repository_owner, repository_blame_url,
> +          EXISTS (SELECT * FROM repositories owned WHERE owned.repository_owner = repositories.repository_id) as "own_repositories" FROM repositories');

I think it's better to do this in PHP.

> Websites/perf.webkit.org/public/include/manifest-generator.php:147
>  

We can just add a second foreach which goes over each repository,
and set ownsRepositories for each owner of the repository if any.

> Websites/perf.webkit.org/public/v3/components/commit-log-viewer.js:73
> +            const subCommitDifferenceExists = this._repository.ownsRepositories() && previousCommit && previousCommit.ownsSubCommits() && commit.ownsSubCommits();

I don't think we need to check ownsRepositories() since no commit can own another commit if ownsRepositories() was false.
Or maybe that should be just asserted rather.

> Websites/perf.webkit.org/public/v3/components/sub-commit-difference-viewer.js:1
> +class SubCommitDifferenceViewer extends ComponentBase {

I think we can just call this SubCommitViewer.

> Websites/perf.webkit.org/public/v3/components/sub-commit-difference-viewer.js:8
> +        this._builtDifferenceTable = false;

Instead of having this boolean, create a new LazilyEvaluatedFunction with a member function which rebuilds the table.
The function should probably take the list of commits.

> Websites/perf.webkit.org/public/v3/components/sub-commit-difference-viewer.js:9
> +        this._expand = false;

This should be either isExpanded, or better yet, showingSubcommits.

> Websites/perf.webkit.org/public/v3/components/sub-commit-difference-viewer.js:18
> +        subCommitInfo.style.display = 'none';

You should have this state as an instance variable.
e.g. subCommitInfo.style.display = this._showingSubcommits ? null : 'none';

> Websites/perf.webkit.org/public/v3/components/sub-commit-difference-viewer.js:22
> +        caption.onclick = () => {

This should be done in didConstructShadowTree.
The only thing this function should do, is to fetch subcommits, and flip this._showingSubcommits, then this.enqueueToRender().
Also, attaching click handler on caption won't make it tab-accessible.
Use create a hyperlink (anchor element) to do this instead.
So an alternative is to use createLink in render().
In that case, you probably want to create anther LazilyEvaluatedFunction that takes the title so that it won't keep updating it each call to render().

> Websites/perf.webkit.org/public/v3/components/sub-commit-difference-viewer.js:29
> +            caption.className = this._expand? 'expand' : 'collapse';
> +            subCommitInfo.style.display = this._expand? null : 'none';
> +            spinner.style.display = this._expand && !this._builtDifferenceTable? null : 'none';
> +
> +            if (!this._expand || this._builtDifferenceTable)
> +                return;

This should be one inside render() function.

> Websites/perf.webkit.org/public/v3/components/sub-commit-difference-viewer.js:36
> +                const previousSubCommit = subCommits[0];
> +                const currentSubCommit = subCommits[1];
> +                spinner.style.display = 'none';
> +                this.renderReplace(subCommitInfo, SubCommitDifferenceViewer._formatSubCommitDifferenceTable(previousSubCommit, currentSubCommit));
> +                this._builtDifferenceTable = true;

This should also be done in the render function.

> Websites/perf.webkit.org/public/v3/components/sub-commit-difference-viewer.js:43
> +    static _formatSubCommitDifferenceTable(previous, current) {

This whole logic should go into commit-log class so that it's unit-testable.

> Websites/perf.webkit.org/public/v3/components/sub-commit-difference-viewer.js:48
> +        return sortedKeys.map((sub_revision) => {

sub_revision should be subRevision.

> Websites/perf.webkit.org/public/v3/components/sub-commit-difference-viewer.js:60
> +        let subCommitMap = {};
> +        subCommits.forEach((subCommit) => {
> +            subCommitMap[subCommit.repository().name()] = subCommit.revision();
> +        });
> +        return subCommitMap;

Use Map, use the repository object itself as a key, and store the entire commit object instead of just its revision.
const subCommitMap = new Map;
for (const commit of subCommits)
    subCommitMap.set(commit.repository(), commit);

> Websites/perf.webkit.org/public/v3/components/sub-commit-difference-viewer.js:67
> +        let subCommitNames = new Set(Object.keys(currentSubCommitMap).concat(Object.keys(previousSubCommitMap)));

Then you can just do new Set([...mapA.keys(), ...mapB.keys()]);

> Websites/perf.webkit.org/public/v3/components/sub-commit-difference-viewer.js:95
> +                content: "\u25bc";

Use a small triangle \25BE.

> Websites/perf.webkit.org/public/v3/components/sub-commit-difference-viewer.js:99
> +                content: "\u25ba";

And \25B8.
Also add a margin of 0.2rem to the right so that there's some space between the triangle and the text.

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

Nit: you have two spaces after return.

> Websites/perf.webkit.org/unit-tests/commit-log-tests.js:197
> +                assert.deepEqual(subCommits[0].repository(), MockModels.webkit);

Use assert.equal since they're the same object.

> Websites/perf.webkit.org/unit-tests/commit-log-tests.js:203
> +
> +

Two blank lines here.

> Websites/perf.webkit.org/unit-tests/commit-log-tests.js:222
> +                assert.deepEqual(subCommits[0].repository(), MockModels.webkit);

You should use equal here since they're the same object.

> Websites/perf.webkit.org/unit-tests/commit-log-tests.js:228
> +                assert.deepEqual(existingSubCommits, subCommits);

In this case, you can just use equal since these two array must be the same object.

[Ryosuke Niwa]

Comment on attachment 306045 [details]
Patch

r- due to the aforementioned issues.

Created attachment 306973 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 306973 [details]
Patch

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

> Websites/perf.webkit.org/ChangeLog:15
> +        * browser-tests/commit-log-viewer-tests.js:

You should describe what kind of changes you made to tests.

> Websites/perf.webkit.org/ChangeLog:16
> +        * public/api/commits.php:

You should describe what change you're making here instead of doing so in the top-level comment.
The same comment applies to the rest of files being modified here.

> Websites/perf.webkit.org/public/include/commit-log-fetcher.php:20
> +            # FIXME: the last parameter should be determined based on commit row.

Nit: Capitalize "The last", and "based on commit_ownerships".

> Websites/perf.webkit.org/public/include/commit-log-fetcher.php:21
> +            $commit = $this->format_commit($commit_row, $commit_row, false);

Nit: false should be in caps: FALSE.
Also, please add an inline comment indicating which argument is false.
e.g. /* owns_sub_commits */ FALSE

> Websites/perf.webkit.org/public/include/commit-log-fetcher.php:100
> +        $statements = 'SELECT owned.commit_repository as "repository",

Since this string isn't dynamically built, just have this inside the call to query_and_fetch_all as in:
return $this->query_and_fetch_all('SELECT owned.comment_repository...

> Websites/perf.webkit.org/public/include/manifest-generator.php:143
> +                'ownsRepositories' => false,

Instead of saying this repository owns some other repository, simply store repository_owner.
That way, we can do this mapping on the client side without any ambiguity as to which repository owns what.

> Websites/perf.webkit.org/public/include/manifest-generator.php:147
> +        foreach($repositories_table as $row)

Nit: space between foreach and (.

> Websites/perf.webkit.org/public/v3/components/commit-log-viewer.js:73
> +            const subCommitDifferenceExists = previousCommit && previousCommit.ownsSubCommits() && commit.ownsSubCommits();

This variable name isn't descriptive. The only thing we checked is that both the current and the previous commits own sub commits.
We haven't checked if they differ.
I think a better variable name would be: ownSubCommits.

> Websites/perf.webkit.org/public/v3/components/expand-collapse-icon.js:2
> +class ExpandCollapseIcon extends ButtonBase {

This should really be called ExpandCollapseButton.

> Websites/perf.webkit.org/public/v3/components/sub-commit-viewer.js:30
> +        }).catch((error) => {
> +            console.error(error);
> +        });

This isn't great because it'd just swallow the exception. Also, we don't do this elsewhere where network can fail.
Remove the catch so that we'll see the full stack trace in the browser if this ever blows up.

> Websites/perf.webkit.org/public/v3/components/sub-commit-viewer.js:36
> +        this.content('expand-collapse').className = this._showingSubCommits ? 'expand' : '';

Nit: This class name should be "expanded".
Given the class is called expand-collapse-icon, it would be ideal if the button class automatically had done this.
e.g. You can do this by overriding didConstructShadowTree in ExpandCollapseIcon,
and then do something like:

this.listenToAction('activate', () => {
    this._expanded = !this._expanded;
    this.content('button').className = this._expanded ? 'expanded' : 'collapsed';
    this._enqueueRender();
})

and then adding some CSS rules via overriding cssTemplate() and concatenating more strings.
See AnalysisCategoryToolbar for that example.

> Websites/perf.webkit.org/public/v3/components/sub-commit-viewer.js:37
> +        this.content('spinner-container').style.display =  (this._previousSubCommits && this._currentSubCommits) || !this._showingSubCommits ? 'none' : null;

Nit: Two spaces before (.
When conditions are complicated like this, it's helpful to have a local variable with a descriptive name.
e.g.
const showSpinner = (this._previousSubCommits && this._currentSubCommits) || !this._showingSubCommits;

> Websites/perf.webkit.org/public/v3/components/sub-commit-viewer.js:46
> +        this.renderReplace(this.content('sub-commit-info'), SubCommitViewer._formatSubCommitDifferenceTable(CommitLog.subCommitDifference(this._previousCommit, this._currentCommit)));

'sub-commit-info' is a really vague name for tbody.
Give it a better name, or it's fine to dynamically construct tbody instead.

> Websites/perf.webkit.org/public/v3/components/sub-commit-viewer.js:50
> +    static _formatSubCommitDifferenceTable(difference)
> +    {

I don't think this needs to be a separate function, let alone a static function.
It makes the code harder to follow. Just merge it into _renderSubcommitTable.

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

Nit: Please put each call to element('td', ~) on a separate line.
Nit: You need a space between ?.
Revisions is an array of commit logs, so it's better to refer to as commits.
Also, why can't we just commits.map((commit) => element('td', commit ? commit.label() : null))
It's actually important to call label() since that makes the revision more human-readable for SVN/Git.

> Websites/perf.webkit.org/public/v3/components/sub-commit-viewer.js:68
> +`;

The preferred style is to put this at the end of the previous line now.

> Websites/perf.webkit.org/public/v3/components/sub-commit-viewer.js:81
> +            expand-collapse-icon {
> +                display: block;
> +            }
> +            expand-collapse-icon.expand {
> +                transform: rotate(180deg);
> +            }

Once you've made the change I suggested above, these could go into expand-collapse-icon.

> Websites/perf.webkit.org/public/v3/components/sub-commit-viewer.js:85
> +            expand-collapse-icon {
> +                margin-left: 45%;
> +            }

A better way to do that will be making expand-collapse-icon display: inline and use text-align: center.
Alternatively, you can do: margin-left: calc(50% - 0.8rem);
Otherwise, the expand-collapse-button will never be quite centered.

> Websites/perf.webkit.org/public/v3/components/sub-commit-viewer.js:88
> +                word-break: break-word;

Why do we want to use this style? It seems like we shouldn't be wrapping text
in the middle of a system component name or in-between revision numbers.

> Websites/perf.webkit.org/public/v3/components/sub-commit-viewer.js:92
> +                font-size: 0.8rem;

You should just set this on :host.

> Websites/perf.webkit.org/public/v3/components/sub-commit-viewer.js:103
> +`;

Ditto.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:96
> +            return Promise.reject(`Repository: "${this.repository().name()}"(id: ${this.repository().id()}) does not own other repositories.`);
> +
> +        if (!this.ownsSubCommits())
> +            return Promise.reject(`Commit: "${this.revision()}" does not own other commits`);

Please don't reject promises with human readable text like these in model classes.
Model classes shouldn't ever be concerned with how their output is read by humans.
Instead of just reject with null or undefined: Promise.reject().

> Websites/perf.webkit.org/public/v3/models/commit-log.js:101
> +        return CommitLog.cachedFetch(`../api/commits/${this.repository().id()}/sub-commits?owner-revision=${escape(this.revision())}`).then((data) => {

Now that I'm looking at this code, why are we even passing in the revision instead of the ID?
We should just do `owner-revision=${this.id()}` instead.
That'd make the query on the backend faster & easier too.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:115
> +    static subCommitDifference(previousCommit, currentCommit)

A function should be a verb. e.g. diffSubCommits is a good name.

> Websites/perf.webkit.org/unit-tests/commit-log-tests.js:63
> +function commitWithoutSubCommits() {

Nit: { on the next line.

[Build Bot]

Comment on attachment 306973 [details]
Patch

Attachment 306973 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3527795

New failing tests:
webrtc/no-port-zero-in-upd-candidates.html

[Build Bot]

Created attachment 306979 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

Created attachment 307094 [details]
Patch

[Build Bot]

Comment on attachment 307094 [details]
Patch

Attachment 307094 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3532744

New failing tests:
webrtc/multi-video.html

[Build Bot]

Created attachment 307099 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Ryosuke Niwa]

Comment on attachment 307094 [details]
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?

Created attachment 307152 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 307152 [details]
Patch for landing

Clearing flags on attachment: 307152

Committed r215378: <http://trac.webkit.org/changeset/215378>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.