<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:rniwa@webkit.org" title="Ryosuke Niwa <rniwa@webkit.org>"> <span class="fn">Ryosuke Niwa</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - Summary page enhancement."
href="https://bugs.webkit.org/show_bug.cgi?id=157339">bug 157339</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #278151 Flags</td>
<td>review?
</td>
<td>review+
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Summary page enhancement."
href="https://bugs.webkit.org/show_bug.cgi?id=157339#c6">Comment # 6</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Summary page enhancement."
href="https://bugs.webkit.org/show_bug.cgi?id=157339">bug 157339</a>
from <span class="vcard"><a class="email" href="mailto:rniwa@webkit.org" title="Ryosuke Niwa <rniwa@webkit.org>"> <span class="fn">Ryosuke Niwa</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=278151&action=diff" name="attach_278151" title="Patch">attachment 278151</a> <a href="attachment.cgi?id=278151&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=278151&action=review">https://bugs.webkit.org/attachment.cgi?id=278151&action=review</a>
<span class="quote">>> Websites/perf.webkit.org/ChangeLog:27
>> + (MeasurementSet.prototype.fetchBetween): Returns a promise. Fix the bug in previous implementation that we miss some callbacks sometimes. Basically, we will fetch primary cluster first, then secondary clusters. For each secondary cluster fetch, we will always invoke callback even when it fail.
>
> This should be "when it fails".</span >
Please break lines after 120 characters or so instead of putting the entire comment in a single line.
<span class="quote">> Websites/perf.webkit.org/ChangeLog:41
> + (SummaryPageConfigurationGroup.prototype.warnings):
> + (SummaryPageConfigurationGroup.prototype.fetchAndComputeSummary.set return):
> + (SummaryPageConfigurationGroup.return.set fetchBetween):
> + (SummaryPageConfigurationGroup.set then): Deleted.
> + (SummaryPageConfigurationGroup.set var): Deleted.</span >
Please update this list with the list of actually modified functions and describe changes to computeSummary.
<span class="quote">> Websites/perf.webkit.org/ChangeLog:42
> + * unit-tests/measurement-set-tests.js: Add a helper function to wait for fetchBetween. Update unit tests since fetchBetween returns a promise now.</span >
We should also mention that we need to wait longer for fetchBetween now due to promise chains.
<span class="quote">> Websites/perf.webkit.org/public/v3/components/ratio-bar-graph.js:31
> + var validClassNames = ['better', 'worse'];
> + for (var name of validClassNames)
> + this._ratioBarGraph.classList.remove(name);
> </span >
On my second thought, we should just put better/worse classes on div.bar & div.label and just CSS rules.
That'll simplify this code.
<span class="quote">> Websites/perf.webkit.org/public/v3/models/measurement-set.js:66
> + var promiseList = [];
> + for (var clusterEndTime of self.findClusters(startTime, endTime)) {</span >
I guess we should just use .map instead:
self.findClusters(startTime, endTime).map(function (clusterEndTime) {
...
})
<span class="quote">> Websites/perf.webkit.org/public/v3/pages/summary-page.js:97
> + var anchor = link(ratioGraph, 'Open charts', this.router().url('charts', state));</span >
I think omit 'Open charts' here:
link(ratioGraph, this.router().url('charts', state));
<span class="quote">> Websites/perf.webkit.org/public/v3/pages/summary-page.js:103
> + warningText += 'Missing ' + type + ' for following platform(s): ' + Array.from(platforms).map(function (platform) { return platform.name(); }).join(', ');</span >
Use template literal here: `Missing ${type} for following platform(s): ${platformList}` where platformList is the result of Array.from...</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>