[webkit-reviews] review granted: [Bug 193047] User should be able to add build request manually. : [Attachment 358119] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted dewei_zhu at apple.com's request for
review:
Bug 193047: User should be able to add build request manually.
https://bugs.webkit.org/show_bug.cgi?id=193047

Attachment 358119: Patch

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




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

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

> Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:46
> +    if (array_key_exists('needsNotification', $data)) {
> +	   $db->update_row('analysis_test_groups', 'testgroup', array('id' =>
$test_group_id),

So what happens if the user who created the test group and the user who added
more build requests were different?
Our simplistic model of having a single author per test group may no longer be
valid...

> Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:403
> +		   <test-group-form
id="add-build-request-form">Add</test-group-form>

It seems a bit unintuitive that "Add" adds as many iterations as specified for
retry...

> Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:561
> +	   groupPane.listenToAction('bisectTestGroup', (testGroup, commitSets,
repetitionCount, notifyOnCompletion) => this._bisectCurrentTestGroup(testGroup,
commitSets, repetitionCount,
notifyOnCompletion));groupPane.listenToAction('retryTestGroup', (testGroup,
repetitionCount, notifyOnCompletion) => this._retryCurrentTestGroup(testGroup,
repetitionCount, notifyOnCompletion));

You have a duplicate code for retryTestGroup. Remove that.