Bug 168866 - Commit should order by 'commit_order' as secondary key.
Summary: Commit should order by 'commit_order' as secondary key.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-02-24 22:54 PST by dewei_zhu
Modified: 2017-02-25 01:00 PST (History)
3 users (show)

See Also:


Attachments
Patch (7.32 KB, patch)
2017-02-24 22:57 PST, dewei_zhu
no flags Details | Formatted Diff | Diff
Patch for landing (10.19 KB, patch)
2017-02-25 00:18 PST, dewei_zhu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description dewei_zhu 2017-02-24 22:54:53 PST
Commit should order by 'commit_order' as secondary key.
Comment 1 dewei_zhu 2017-02-24 22:57:33 PST
Created attachment 302736 [details]
Patch
Comment 2 Ryosuke Niwa 2017-02-24 23:07:15 PST
Comment on attachment 302736 [details]
Patch

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

> Websites/perf.webkit.org/public/include/commit-log-fetcher.php:87
> -        return $this->format_single_commit($this->db->select_first_row('commits', 'commit', array('repository' => $repository_id), 'time'));
> +        return $this->format_single_commit($this->db->select_first_row('commits', 'commit', array('repository' => $repository_id), ['time', 'order']));

[] syntax doesn't work on an older versions of PHP.
So please use array() instead.

> Websites/perf.webkit.org/public/include/db.php:217
> +            if (!is_array($order_by)) {
> +                $order_by = array($order_by);
> +            }

Nit: No curtly braces around a single line statement.

> Websites/perf.webkit.org/public/include/db.php:223
> +                $order_column = $this->prefixed_name($order_key, $prefix);
> +                if ($descending_order)
> +                    $order_column .= ' DESC';

Might be cleaner to always specify ASC/DES using tertiary expression as in:
$order_column = $this->prefixed_name($order_key, $prefix) . ' ' . ($descending_order ? 'DESC' : 'ASC');

> Websites/perf.webkit.org/server-tests/api-commits.js:137
> +        it("should return the list of ordered commits for a given repository", () => {

Please also add tests for latest, & oldest, and last-reported.

> Websites/perf.webkit.org/server-tests/api-commits.js:146
> +                assert.equal(commits.length, 6);

You can just check against submittedCommits.length here.

> Websites/perf.webkit.org/server-tests/api-commits.js:147
> +                var submittedCommits = systemVersionCommits['commits'];

Use const instead of var.
Comment 3 dewei_zhu 2017-02-25 00:18:23 PST
Created attachment 302748 [details]
Patch for landing
Comment 4 WebKit Commit Bot 2017-02-25 01:00:08 PST
Comment on attachment 302748 [details]
Patch for landing

Clearing flags on attachment: 302748

Committed r213003: <http://trac.webkit.org/changeset/213003>
Comment 5 WebKit Commit Bot 2017-02-25 01:00:12 PST
All reviewed patches have been landed.  Closing bug.