[webkit-reviews] review denied: [Bug 175978] Performance Dashboard backend should support A/B testing for owned components. : [Attachment 320633] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied dewei_zhu at apple.com's request for
review:
Bug 175978: Performance Dashboard backend should support A/B testing for owned
components.
https://bugs.webkit.org/show_bug.cgi?id=175978

Attachment 320633: Patch

https://bugs.webkit.org/attachment.cgi?id=320633&action=review




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

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

>> Websites/perf.webkit.org/ChangeLog:22
>> +			    ADD CONSTRAINT
commitset_item_with_owned_commit_must_requires_build CHECK
(commitset_commit_owner IS NULL OR commitset_requires_build = TRUE);
> 
> I may need more comprehensive sql update query which also updates the
'requires_build' field of commit set item, which is from same repository, in
the same test group but does not have patch specified, to be true.

As I previously mentioned, please move this to the long description section
above this. You shouldn't have something like this as an inline comment.

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:98
> +	       assert($requires_build !== 't' && $requires_build !== 'f');

I don't think we need to assert this if we're asserting is_bool.

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:175
> +	   $existing_owner_revisions = array();

I think this should be renamed to something like $owner_revisions_in_set so
that it's clear it's existing in the set, not somewhere in the database.

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:216
> +		   $repository = $db->select_first_row('repositories',
'repository', array('id' => intval($repository_id)));
> +		   if (!$repository)
> +		       exit_with_error('RepositoryNotFound', array('repository'
=> $repository_id));
> +		   $owner_commit = $db->select_first_row('commits', 'commit',
array('repository' => intval($repository['repository_owner']), 'revision' =>
$owner_revision));

There's no need to call intval on these. We've already checked that
$repository_id is numeric and repository_owner is a value we're getting out of
the database.

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:218
> +		       exit_with_error('InvalidOwnerRevision',
array('ownerRevision' => $owner_revision));

You should also report the repository. You can just use 'revision',
'repository' as keys since the error name already mentions that they're about
an owner revision.

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:224
> +	       } else {
> +		   $existing_owner_revisions[$revision] = TRUE;
> +	       }

Nit: a single line statement shouldn't have curly brackets around it:
https://webkit.org/code-style-guidelines/#braces-one-line

This is wrong because multiple repositories can have an identical revision. r-
because of this.
I think we should use commit IDs instead.

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:227
> +	       array_push($commit_set, array('commit' => $commit_id,
'patch_file' => $patch_file_id, 'requires_build' => !!$owner_revision,
'commit_owner' => $owner_commit_id));

We should initialize requires_build to be always FALSE here.
Instead, we should add a code to set repositories_require_build to true when
the owner revision is set.

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:230
> +	       $repository_commit_set_items_map[$repository_id][] =
&$commit_set[count($commit_set) - 1];

This should really be named $repository_to_commit_set_items_map or
$commit_set_items_by_repository

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:235
> +	       if ($repository && $repository['repository_owner']) {
> +		   $repository_owner_list[$repository['repository_owner']] =
TRUE;
> +		   continue;
> +	       }

I don't think we need this code if we checked the commit ID instead.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:178
> +	   this._revisionListByOwnedRepository = new Map;

Why don't we just call this _repositoryToOwnerRevisionMap or something similar?

> Websites/perf.webkit.org/public/v3/models/commit-set.js:186
> +	   this._revisionListByRepository.set(repository, {revision, patch,
ownerRevision});

Here, you have to set _revisionListByOwnedRepository. r- because of this.
Please write tests for this.