<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<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#c3">Comment # 3</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:dewei_zhu@apple.com" title="dewei_zhu@apple.com">dewei_zhu@apple.com</a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=278069&action=diff" name="attach_278069" title="Patch">attachment 278069</a> <a href="attachment.cgi?id=278069&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=278069&action=review">https://bugs.webkit.org/attachment.cgi?id=278069&action=review</a>
<span class="quote">>> Websites/perf.webkit.org/public/v3/components/ratio-bar-graph.js:49
>> + if (this._ratio) {
>
> You should check that this._ratio is not NaN here.</span >
I don't think it will satisfy the condition when this._ratio equals NaN.
<span class="quote">>> Websites/perf.webkit.org/public/v3/pages/summary-page.js:187
>> + this._warnings = new Map;
>
> We don't really need to use a Map here since we're only ever going to use string keys.
> Also, I'd prefer initializing this to `null` in the case there are no warnings.</span >
I agree that using Map is unnecessary. But if we initialize warnings to `null` we need to do null-check every time we use it. I think using {} will simplify the code.
<span class="quote">>> Websites/perf.webkit.org/public/v3/pages/summary-page.js:197
>> + if (excludedConfigurations && excludedConfigurations.hasOwnProperty(platform.id()) && excludedConfigurations[platform.id()].includes(+metric.id()))
>
> Just do "platform.id() in excludedConfigurations" instead.
> Also, there's no need to convert metric.id() to a number.</span >
[1].includes("1") returns false. And metric.id() returns a string instead of a number. So I think the conversion is necessary.
<span class="quote">>> Websites/perf.webkit.org/public/v3/pages/summary-page.js:266
>> + self._warnings.set('Missing baseline', missingBaselinePlatforms);
>
> I'd prefer to have a pattern like:
> if (!(y in x))
> x[y] = ~;
> x[y].add(~).
> since that's what we do elsewhere in the perf dashboard.
>
> Now, there isn't really much reason to be using a Set here
> because we're not gonna have same platforms appearing multiple times
> (we should probably verify that somewhere)
> so I'd suggest using an array instead to make the code to construct the string simpler.
>
> Note that Set and Map are orders of magnitudes (2x to 200x) slower on almost all browsers.</span >
I can see same platform occurs multiple times here. Because pair of <platform, metric> is unique.</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>