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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Apr 3 14:41:55 PDT 2017


https://bugs.webkit.org/show_bug.cgi?id=170379

--- Comment #3 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 306045
  --> https://bugs.webkit.org/attachment.cgi?id=306045
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.

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


More information about the webkit-unassigned mailing list