[Webkit-unassigned] [Bug 177993] Add UI for A/B testing on owned commits.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=177993

--- Comment #13 from dewei_zhu at apple.com ---
Comment on attachment 329750
  --> https://bugs.webkit.org/attachment.cgi?id=329750
Patch

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

>> Websites/perf.webkit.org/public/v3/components/combo-box.js:35
>> +            const item = link(candidate, () => null);
> 
> What's up with the empty arrow function?

createLink assumes at least 2 arguments are supplied when called.
Otherwise, onclick event will be registered undefined and an error will be raised at 'callback.call(this, event);' of ComponentBase.createEventHandler.
Another alternative is change ComponentBase.createEventHandler to not call callback when it is undefined.

>> Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:262
>> +            }
> 
> Use "#custom-table tr:last-child td.owner-repository-label" instead

This may not work for some cases.
This class should really be renamed to 'last-row-for-owned-repository'. It is not granted the last row of owned repository is the last row of the table.

>> Websites/perf.webkit.org/public/v3/models/commit-log.js:69
>> +        if (parseInt(from) == from)// e.g. r12345.
> 
> Why are you removing the logic to add +1? This should clearly needs to be explained in the change log.
> Also, presumably you're only doing this for owned commits so we should be able to check the repository type,
> and not add "r" or add 1 just for owned commits only for owned commits.
> 
> In the long term, we should store a new field to the repository table in the database indicating
> the repository type such as SVN, Git, etc...

Actually, 'fromRevisionForURL' is an unused variable. Do we actually want 
"label = `r${fromRevisionForURL}-r${this.revision()}`;"?

-- 
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/20171221/e3d60c88/attachment.html>