<html>
<head>
<base href="https://bugs.webkit.org/">
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Add sub-commit UI in commit log viewer."
href="https://bugs.webkit.org/show_bug.cgi?id=170379#c3">Comment # 3</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Add sub-commit UI in commit log viewer."
href="https://bugs.webkit.org/show_bug.cgi?id=170379">bug 170379</a>
from <span class="vcard"><a class="email" href="mailto:rniwa@webkit.org" title="Ryosuke Niwa <rniwa@webkit.org>"> <span class="fn">Ryosuke Niwa</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=306045&action=diff" name="attach_306045" title="Patch">attachment 306045</a> <a href="attachment.cgi?id=306045&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=306045&action=review">https://bugs.webkit.org/attachment.cgi?id=306045&action=review</a>
<span class="quote">> Websites/perf.webkit.org/ChangeLog:17
> + * public/include/manifest-generator.php:</span >
Please add function headers with comments.
e.g.
(ManifestGenerator::generate):
(ManifestGenerator::repositories):
<span class="quote">> Websites/perf.webkit.org/ChangeLog:40
> + (then):</span >
You should get rid of these (then) comments.
<span class="quote">> Websites/perf.webkit.org/ChangeLog:47
> + (beforeEach):
> + (return.commit.fetchSubCommits.catch):
> + (return.fetchingPromise.then):
> + (then):</span >
As well as these.
<span class="quote">> Websites/perf.webkit.org/public/api/commits.php:35
> + $commit_revision = array_get($_GET, 'commit-revision');</span >
I think should be either just 'revision' or 'owner-revision'.
<span class="quote">> Websites/perf.webkit.org/public/include/commit-log-fetcher.php:103
> + FROM commits owned INNER JOIN commit_ownerships ON owned.commit_id = commit_owned</span >
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
<span class="quote">> 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'])));</span >
Instead of boolval, do !!.
<span class="quote">> Websites/perf.webkit.org/public/include/commit-log-fetcher.php:163
> + private function format_commit($commit_row, $committer_row, $owns_sub_commits) {</span >
Please fix fetch_for_tasks to call this with the right arguments.
<span class="quote">> 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');</span >
I think it's better to do this in PHP.
<span class="quote">> Websites/perf.webkit.org/public/include/manifest-generator.php:147
> </span >
We can just add a second foreach which goes over each repository,
and set ownsRepositories for each owner of the repository if any.
<span class="quote">> Websites/perf.webkit.org/public/v3/components/commit-log-viewer.js:73
> + const subCommitDifferenceExists = this._repository.ownsRepositories() && previousCommit && previousCommit.ownsSubCommits() && commit.ownsSubCommits();</span >
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.
<span class="quote">> Websites/perf.webkit.org/public/v3/components/sub-commit-difference-viewer.js:1
> +class SubCommitDifferenceViewer extends ComponentBase {</span >
I think we can just call this SubCommitViewer.
<span class="quote">> Websites/perf.webkit.org/public/v3/components/sub-commit-difference-viewer.js:8
> + this._builtDifferenceTable = false;</span >
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.
<span class="quote">> Websites/perf.webkit.org/public/v3/components/sub-commit-difference-viewer.js:9
> + this._expand = false;</span >
This should be either isExpanded, or better yet, showingSubcommits.
<span class="quote">> Websites/perf.webkit.org/public/v3/components/sub-commit-difference-viewer.js:18
> + subCommitInfo.style.display = 'none';</span >
You should have this state as an instance variable.
e.g. subCommitInfo.style.display = this._showingSubcommits ? null : 'none';
<span class="quote">> Websites/perf.webkit.org/public/v3/components/sub-commit-difference-viewer.js:22
> + caption.onclick = () => {</span >
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().
<span class="quote">> 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;</span >
This should be one inside render() function.
<span class="quote">> 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;</span >
This should also be done in the render function.
<span class="quote">> Websites/perf.webkit.org/public/v3/components/sub-commit-difference-viewer.js:43
> + static _formatSubCommitDifferenceTable(previous, current) {</span >
This whole logic should go into commit-log class so that it's unit-testable.
<span class="quote">> Websites/perf.webkit.org/public/v3/components/sub-commit-difference-viewer.js:48
> + return sortedKeys.map((sub_revision) => {</span >
sub_revision should be subRevision.
<span class="quote">> 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;</span >
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);
<span class="quote">> Websites/perf.webkit.org/public/v3/components/sub-commit-difference-viewer.js:67
> + let subCommitNames = new Set(Object.keys(currentSubCommitMap).concat(Object.keys(previousSubCommitMap)));</span >
Then you can just do new Set([...mapA.keys(), ...mapB.keys()]);
<span class="quote">> Websites/perf.webkit.org/public/v3/components/sub-commit-difference-viewer.js:95
> + content: "\u25bc";</span >
Use a small triangle \25BE.
<span class="quote">> Websites/perf.webkit.org/public/v3/components/sub-commit-difference-viewer.js:99
> + content: "\u25ba";</span >
And \25B8.
Also add a margin of 0.2rem to the right so that there's some space between the triangle and the text.
<span class="quote">> Websites/perf.webkit.org/public/v3/models/commit-log.js:38
> + ownsSubCommits() { return this._rawData['ownsSubCommits']; }</span >
Nit: you have two spaces after return.
<span class="quote">> Websites/perf.webkit.org/unit-tests/commit-log-tests.js:197
> + assert.deepEqual(subCommits[0].repository(), MockModels.webkit);</span >
Use assert.equal since they're the same object.
<span class="quote">> Websites/perf.webkit.org/unit-tests/commit-log-tests.js:203
> +
> +</span >
Two blank lines here.
<span class="quote">> Websites/perf.webkit.org/unit-tests/commit-log-tests.js:222
> + assert.deepEqual(subCommits[0].repository(), MockModels.webkit);</span >
You should use equal here since they're the same object.
<span class="quote">> Websites/perf.webkit.org/unit-tests/commit-log-tests.js:228
> + assert.deepEqual(existingSubCommits, subCommits);</span >
In this case, you can just use equal since these two array must be the same object.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>