[webkit-reviews] review granted: [Bug 180126] Add a test freshness page. : [Attachment 329214] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted dewei_zhu at apple.com's request for
review:
Bug 180126: Add a test freshness page.
https://bugs.webkit.org/show_bug.cgi?id=180126

Attachment 329214: Patch

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




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

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

> Websites/perf.webkit.org/ChangeLog:10
> +	   Using an S-shape logistic function to evaluate the freshness of the
data points.

Nit: S-shaped but I don't think we need to say that since logistic function is
by definition S-shaped.

> Websites/perf.webkit.org/public/include/manifest-generator.php:47
> +	       'acceptableLastDataPointDurationInHour' =>
config('acceptableLastDataPointDurationInHour'),

This is a very long name. How about something like testAgeToleranceInHours or
testFreshnessToleranceInHours,
or even expectedTestFreshness.

> Websites/perf.webkit.org/public/v3/components/freshness-indicator.js:25
> +	   this._lastDataPointDuration = lastDataPointDuration;
> +	   this._summary = summary;
> +	   this._acceptableLastDataPointDurationInHour =
acceptableLastDataPointDurationInHour;
> +	   this._url = url;
> +	   this.enqueueToRender();

You should do this work in update, and then render() should lazily update the
DOM.

> Websites/perf.webkit.org/public/v3/components/freshness-indicator.js:32
> +	   this.renderReplace(this.content('container'), icon);

You should do this renderReplace in _createIcon. Should be renamed to
_renderIndicator or something.

> Websites/perf.webkit.org/public/v3/components/freshness-indicator.js:41
> +	   const hoursSinceLastDataPoint = this._lastDataPointDuration / 3600 ;

Nit: Trailing space before ;.

> Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:7
> +	   this._acceptableLastDataPointDurationInHour =
acceptableLastDataPointDurationInHour || 24;
> +	   this._timeDurationInMillionSeconds =
this._acceptableLastDataPointDurationInHour * 2 * 3600 * 1000;

We should store all these values in milliseconds always.

> Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:34
> +		       const platform = Platform.findById(platformId);
> +		       if (!platform)
> +			   continue;
> +		       this._platforms.push(platform);

Why don't we just add them all to the set and sort them later?
In fact, Set preserves the order in JS.

> Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:84
> +		   measurementSet.fetchBetween(startTime,
this._measurementSetFetchTime).then(() => {

It's a bit odd that you're keep running this code without setting fetchBetwen's
noCache option to false.

> Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:102
> +	   this.renderReplace(this.content('test-health'),
this._constructTableLazily.evaluate(this._platforms, this._metrics));

You wanna do the replace call in _constructTable because otherwise, it would
run appendChild each time.

> Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:115
> +		   indicator.update(timeDurationInMillionSeconds / 1000,
this._acceptableLastDataPointDurationInHour, summary, url);

You should just make update take milliseconds instead since that's the
canonical time unit in JS.

> Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:127
> +	       tableHeadElements.push(element('th', {class: 'diagonal-header'},
element('div', metric.test().path()[0].label())));

You need to call metric.test().fullName() as discussed in person.
Otherwise, you won't be able to distinguish subtests under a single top-level
test like Dromaeo.

> Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:143
> +	       &&
this._excludedConfigurations[platform.id()].includes(+metric.id()))

Instead of converting metric.id() to a number, do: some((x) => x ==
metric.id()).