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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Apr 13 00:06:45 PDT 2017


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

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

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.
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
> +`;


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

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/20170413/ca15cdd3/attachment-0001.html>

More information about the webkit-unassigned mailing list