[webkit-reviews] review denied: [Bug 180126] Add a test health page. : [Attachment 327826] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied  review:
Bug 180126: Add a test health page.
https://bugs.webkit.org/show_bug.cgi?id=180126

Attachment 327826: Patch

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




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

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

> Websites/perf.webkit.org/public/v3/components/test-health-cell.js:19
> +	       const now = new Date();

Use Date.now() instead.

>> Websites/perf.webkit.org/public/v3/components/test-health-cell.js:42
>> +	    return `/v3/#/charts?since=${Date.now() - 7 * 24 * 3600 *
1000}&paneList=((${this._platform.id()}-${this._metric.id()}))`;
> 
> Does performance dashboard code have a centralized place where URLs are
built? Doing it in every view controller seems less than ideal.

See PageRouter and how it's used. We should never build a URL string like this.
r-.

> Websites/perf.webkit.org/public/v3/components/test-health-cell.js:52
> +	   return (new Date() - this._timeForLastDataPoint) / 1000 / 3600;

Ditto. Also, it's not okay to keep calling Date.now() in different places.
This page can be open for multiple hours or even days.
The value of new Date/Date.now() would change over time.'
r- for that reason as well.