<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#c2">Comment # 2</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=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/ChangeLog:8
> + Set summary page to be the home page of v3 UI.</span >
I'd call it the default page since that's the terminology used in the dashboard.
<span class="quote">> Websites/perf.webkit.org/ChangeLog:9
> + Show warning icon while part of data is missing.</span >
when either baseline or current data is missing
would be clearer.
<span class="quote">> Websites/perf.webkit.org/ChangeLog:11
> + Update unit test for MeasurementSet.</span >
You should explain what kind of updates you're making.
<span class="quote">> Websites/perf.webkit.org/ChangeLog:14
> + * public/v3/components/ratio-bar-graph.js:</span >
Please add per-function change description below.
<span class="quote">> Websites/perf.webkit.org/public/v3/components/ratio-bar-graph.js:33
> + var children = [element('div', {class: 'seperator'}), element('div', {class: 'bar'}), element('div', {class: 'label'})];</span >
Why don't we declare this after adding updating classes?
<span class="quote">> Websites/perf.webkit.org/public/v3/components/ratio-bar-graph.js:37
> + else if(this._ratio != null)</span >
Nit: a space between "if" and "(".
<span class="quote">> Websites/perf.webkit.org/public/v3/components/ratio-bar-graph.js:44
> + var warningIcon = new WarningIcon;
> + children.push(warningIcon);</span >
We can do this in a single line.
<span class="quote">> Websites/perf.webkit.org/public/v3/components/ratio-bar-graph.js:49
> + if (this._ratio) {</span >
You should check that this._ratio is not NaN here.
<span class="quote">> Websites/perf.webkit.org/public/v3/components/ratio-bar-graph.js:51
> + this.content().querySelector('.bar').style.width = Math.min(percent, 50) + '%';</span >
We should just specify this as an inline style declaration as in:
var barStyle = '';
if (this._ratio) {...}
element('div', {class: 'bar', style: barStyle});
<span class="quote">> Websites/perf.webkit.org/public/v3/components/ratio-bar-graph.js:53
> + this.content().querySelector('.label').textContent = this._label;</span >
We should just put the text above where we create the element as in:
element('div', {class: 'label'}, this._label).
<span class="quote">> Websites/perf.webkit.org/public/v3/main.js:23
> + var summaryPage = manifest.summary? new SummaryPage(manifest.summary) : null;</span >
Nit: We need a space before ?.
<span class="quote">> Websites/perf.webkit.org/public/v3/main.js:50
> - if (dashboardPages)
> + if (summaryPage)
> + router.setDefaultPage(summaryPage);
> + else if (dashboardPages)</span >
Please mention in the change log that you're changing the default.
<span class="quote">> Websites/perf.webkit.org/public/v3/models/measurement-set.js:58
> fetchBetween(startTime, endTime, callback)</span >
You should mention that you're rewriting these functions in the change log,
and what the differences between the old & the new implementations are.
<span class="quote">> Websites/perf.webkit.org/public/v3/models/measurement-set.js:65
> + .then(function() {</span >
We usually put .then(function () { in the previous line.
Also note that we need a space between "function" and "()".
<span class="quote">> Websites/perf.webkit.org/public/v3/models/measurement-set.js:122
> + this._fetchedPrimary = true;</span >
I don't think this instance variable is no longer used in the promise-based implementation.
<span class="quote">> Websites/perf.webkit.org/public/v3/pages/heading.js:19
> + group = group.filter(function (page) { return page; });</span >
We should do this at the call site instead of magically filtering it in this function.
In general, we should try to keep helper functions, components, modules, etc... as simple as possible
and put as much convoluted logics at the application level (i.e. main.js, -page.js).
Otherwise, we'd end up having to second guess what each function/class *might* do.
ComponentBase.renderReplace (along with other methods on that class) is an exception
since the whole point of that function is helping each component instantiate DOM nodes easily.
(i.e. absorb the complexity in favor of simplifying call sites)
<span class="quote">> Websites/perf.webkit.org/public/v3/pages/heading.js:20
> for (var page of group)</span >
We should add "console.assert(page instanceof Page)" here... (don't need to do it in this patch).
<span class="quote">> Websites/perf.webkit.org/public/v3/pages/page-router.js:16
> + if(!page)
> + return;</span >
The same thing here. It would be weird for someone to be calling addPage and then this function silently ignored it.
We'd have to debug or read the code to figure out that page was null.
It's better if this code just threw an exception instead.
<span class="quote">> Websites/perf.webkit.org/public/v3/pages/summary-page.js:15
> + this._excludedConfigurations = summarySettings.excludedConfigurations;</span >
Please mention this feature in the change log.
<span class="quote">> Websites/perf.webkit.org/public/v3/pages/summary-page.js:81
> + headings = [element('th', {class: 'minorHeader'}, row.name)];
> + if (!rowIndex)
> + headings.unshift(element('th', {class: 'majorHeader', rowspan: rowGroup.rows.length}, rowGroup.name));</span >
Also explain what this change does.
<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 >
Now that I think about more, we don't need to instantiate the link element here at all.
We just need to store 'Open charts' to a local variable like 'anchorLabel' and then use that at the last line of this function.
<span class="quote">> Websites/perf.webkit.org/public/v3/pages/summary-page.js:187
> + this._warnings = new Map;</span >
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 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()))</span >
Just do "platform.id() in excludedConfigurations" instead.
Also, there's no need to convert metric.id() to a number.
<span class="quote">> Websites/perf.webkit.org/public/v3/pages/summary-page.js:262
> + var platformName = Platform.findById(set.platformId()).name();</span >
Please just store the platform object instead of returning its name.
<span class="quote">> Websites/perf.webkit.org/public/v3/pages/summary-page.js:264
> + var missingBaselinePlatforms = self._warnings.get('Missing baseline') || new Set;</span >
Why don't we add "Missing" when we construct the string in render instead?
<span class="quote">> Websites/perf.webkit.org/public/v3/pages/summary-page.js:266
> + self._warnings.set('Missing baseline', missingBaselinePlatforms);</span >
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 class="quote">> Websites/perf.webkit.org/public/v3/pages/summary-page.js:269
> + var missingCurrentPlatforms = self._warnings.get('Missing current') || new Set;</span >
Ditto.
<span class="quote">> Websites/perf.webkit.org/unit-tests/measurement-set-tests.js:74
> + function waitForMeasurementSet()</span >
You should mention what this function does / why these test changes are needed.</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>