[webkit-reviews] review granted: [Bug 191557] Extend commits table to contain testability warning information. : [Attachment 357803] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted dewei_zhu at apple.com's request for
review:
Bug 191557: Extend commits table to contain testability warning information.
https://bugs.webkit.org/show_bug.cgi?id=191557

Attachment 357803: Patch

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




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

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

> Websites/perf.webkit.org/public/include/commit-updater.php:74
> +    private function &construct_update_data_list(&$commit_info_list,
$should_insert)

We can probably just call this construct_update_list.

> Websites/perf.webkit.org/public/include/commit-updater.php:83
> +	       $update_data =  array('commit' => $commit_data, 'repository' =>
$commit_info['repository']);

Nit: Two spaces after =.
I don't think we really need to call this update_data. Just call it update.
Also, use references?

> Websites/perf.webkit.org/public/include/commit-updater.php:95
> +		   $this->exit_with_error('OwnedCommitsUpdateNotSupportedYet',
array('commit' => $commit_info));

I think a better / less verbose error would be AttemptToUpdateOwnedCommits

> Websites/perf.webkit.org/public/include/commit-updater.php:101
> +		   $update_data_list[] = $update_data;

I think we can just use array_push.

> Websites/perf.webkit.org/public/include/commit-updater.php:106
> +	       foreach($commit_info['ownedCommits'] as $owned_repository_name
=> $owned_commit_info) {

Use references?

> Websites/perf.webkit.org/public/include/commit-updater.php:108
> +		   $owned_commit_update_data = array('commit' => $owned_commit,
'repository' => $owned_repository_name);

Ditto.

> Websites/perf.webkit.org/public/include/commit-updater.php:111
> +		       $owned_commit_update_data['previous_commit'] =
$owned_commit_info['previousCommit'];

Ditto.

> Websites/perf.webkit.org/public/include/commit-updater.php:114
> +		       $owned_commit_update_data['author'] =
$owned_commit_info['author'];

Ditto.

> Websites/perf.webkit.org/public/include/commit-updater.php:115
> +		   $owned_commit_update_data_list[] =
$owned_commit_update_data;

Just use array_push

> Websites/perf.webkit.org/public/include/commit-updater.php:118
> +		   $update_data['owned_commits'] =
$owned_commit_update_data_list;

Use a reference.

> Websites/perf.webkit.org/public/include/commit-updater.php:120
> +	       $update_data_list[] = $update_data;

Use array_push.

> Websites/perf.webkit.org/public/include/commit-updater.php:142
> +		   exit_with_error('OwnedCommitShouldNotContainTimestamp',
array('commit' => $commit_info));

I think it should be OwnedCommitShouldNot*Have*Timestamp.

> Websites/perf.webkit.org/public/include/commit-updater.php:147
> +	   if ($should_insert)
> +	       $commit_data['reported'] = true;

We should have a test to check that an API call with insert=false wouldn't set
reported to true if there isn't one already.