[Webkit-unassigned] [Bug 157339] Summary page enhancement.

[bugzilla-daemon at webkit.org]

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

Ryosuke Niwa <rniwa at webkit.org> changed:

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

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

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

>> 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".

Please break lines after 120 characters or so instead of putting the entire comment in a single line.

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

Please update this list with the list of actually modified functions and describe changes to computeSummary.

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

We should also mention that we need to wait longer for fetchBetween now due to promise chains.

> 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);
>  

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.

> Websites/perf.webkit.org/public/v3/models/measurement-set.js:66
> +            var promiseList = [];
> +            for (var clusterEndTime of self.findClusters(startTime, endTime)) {

I guess we should just use .map instead:
self.findClusters(startTime, endTime).map(function (clusterEndTime) {
...
})

> Websites/perf.webkit.org/public/v3/pages/summary-page.js:97
> +        var anchor = link(ratioGraph, 'Open charts', this.router().url('charts', state));

I think omit 'Open charts' here:
link(ratioGraph, this.router().url('charts', state));

> 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(', ');

Use template literal here: `Missing ${type} for following platform(s): ${platformList}` where platformList is the result of Array.from...

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160505/644cabbe/attachment-0001.html>