[webkit-reviews] review granted: [Bug 219628] Add max age for a root to be reused. : [Attachment 415611] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted dewei_zhu at apple.com's request for
review:
Bug 219628: Add max age for a root to be reused.
https://bugs.webkit.org/show_bug.cgi?id=219628

Attachment 415611: Patch

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




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

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

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

max*RootReuse*AgeInDays

> Websites/perf.webkit.org/public/v3/models/build-request.js:100
> +	   const earliestAllowRootCreationDate =
rawManifest.maxReuseRootAgeInDays ?
> +	       Date.now() - rawManifest.maxReuseRootAgeInDays * 24 * 3600 *
1000 : 0;

I'd call this earliestRootCreatingTimeForReuse

> Websites/perf.webkit.org/public/v3/models/commit-set.js:77
> -    areAllRootsAvailable()
> +    areAllRootsAvailableAndNewerThan(earliestCreationDate)

I don't think it's necessary to call it "andNewerThan".
Also, I'd call the new argument earliestCreationTime instead.

> Websites/perf.webkit.org/public/v3/models/manifest.js:25
>      static fetch()

Why don't we make this async as well?

> Websites/perf.webkit.org/public/v3/models/manifest.js:34
> +	       return await RemoteAPI.getJSON('/data/manifest.json');

Neat.

> Websites/perf.webkit.org/public/v3/models/manifest.js:35
> +	   } catch(error) {

We shouldn't swallow all errors though. Just 404.
We should probably add a test for that.

> Websites/perf.webkit.org/unit-tests/build-request-tests.js:470
> +	   it('should not reuse a root that older than "maxReuseRootAge"',
async () => {

that *is* older than.