[Webkit-unassigned] [Bug 180126] Add a test freshness page.

[bugzilla-daemon at webkit.org]

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

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #329214|review?                     |review+
              Flags|                            |

--- 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()).

-- 
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/20171214/757d9c9c/attachment-0001.html>