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

[bugzilla-daemon at webkit.org]

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

--- Comment #3 from dewei_zhu at apple.com ---
Comment on attachment 278069
  --> https://bugs.webkit.org/attachment.cgi?id=278069
Patch

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

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

I don't think it will satisfy the condition when this._ratio equals NaN.

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

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.

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

[1].includes("1") returns false. And metric.id() returns a string instead of a number. So I think the conversion is necessary.

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

I can see same platform occurs multiple times here. Because pair of <platform, metric> is unique.

-- 
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/04498738/attachment.html>