[Webkit-unassigned] [Bug 157339] Summary page enhancement.
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed May 4 02:16:31 PDT 2016
https://bugs.webkit.org/show_bug.cgi?id=157339
--- Comment #2 from Ryosuke Niwa <rniwa at webkit.org> ---
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/ChangeLog:8
> + Set summary page to be the home page of v3 UI.
I'd call it the default page since that's the terminology used in the dashboard.
> Websites/perf.webkit.org/ChangeLog:9
> + Show warning icon while part of data is missing.
when either baseline or current data is missing
would be clearer.
> Websites/perf.webkit.org/ChangeLog:11
> + Update unit test for MeasurementSet.
You should explain what kind of updates you're making.
> Websites/perf.webkit.org/ChangeLog:14
> + * public/v3/components/ratio-bar-graph.js:
Please add per-function change description below.
> 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'})];
Why don't we declare this after adding updating classes?
> Websites/perf.webkit.org/public/v3/components/ratio-bar-graph.js:37
> + else if(this._ratio != null)
Nit: a space between "if" and "(".
> Websites/perf.webkit.org/public/v3/components/ratio-bar-graph.js:44
> + var warningIcon = new WarningIcon;
> + children.push(warningIcon);
We can do this in a single line.
> 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.
> Websites/perf.webkit.org/public/v3/components/ratio-bar-graph.js:51
> + this.content().querySelector('.bar').style.width = Math.min(percent, 50) + '%';
We should just specify this as an inline style declaration as in:
var barStyle = '';
if (this._ratio) {...}
element('div', {class: 'bar', style: barStyle});
> Websites/perf.webkit.org/public/v3/components/ratio-bar-graph.js:53
> + this.content().querySelector('.label').textContent = this._label;
We should just put the text above where we create the element as in:
element('div', {class: 'label'}, this._label).
> Websites/perf.webkit.org/public/v3/main.js:23
> + var summaryPage = manifest.summary? new SummaryPage(manifest.summary) : null;
Nit: We need a space before ?.
> Websites/perf.webkit.org/public/v3/main.js:50
> - if (dashboardPages)
> + if (summaryPage)
> + router.setDefaultPage(summaryPage);
> + else if (dashboardPages)
Please mention in the change log that you're changing the default.
> Websites/perf.webkit.org/public/v3/models/measurement-set.js:58
> fetchBetween(startTime, endTime, callback)
You should mention that you're rewriting these functions in the change log,
and what the differences between the old & the new implementations are.
> Websites/perf.webkit.org/public/v3/models/measurement-set.js:65
> + .then(function() {
We usually put .then(function () { in the previous line.
Also note that we need a space between "function" and "()".
> Websites/perf.webkit.org/public/v3/models/measurement-set.js:122
> + this._fetchedPrimary = true;
I don't think this instance variable is no longer used in the promise-based implementation.
> Websites/perf.webkit.org/public/v3/pages/heading.js:19
> + group = group.filter(function (page) { return page; });
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)
> Websites/perf.webkit.org/public/v3/pages/heading.js:20
> for (var page of group)
We should add "console.assert(page instanceof Page)" here... (don't need to do it in this patch).
> Websites/perf.webkit.org/public/v3/pages/page-router.js:16
> + if(!page)
> + return;
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.
> Websites/perf.webkit.org/public/v3/pages/summary-page.js:15
> + this._excludedConfigurations = summarySettings.excludedConfigurations;
Please mention this feature in the change log.
> 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));
Also explain what this change does.
> Websites/perf.webkit.org/public/v3/pages/summary-page.js:97
> + var anchor = link(ratioGraph, 'Open charts', this.router().url('charts', state));
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.
> 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.
> 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.
> Websites/perf.webkit.org/public/v3/pages/summary-page.js:262
> + var platformName = Platform.findById(set.platformId()).name();
Please just store the platform object instead of returning its name.
> Websites/perf.webkit.org/public/v3/pages/summary-page.js:264
> + var missingBaselinePlatforms = self._warnings.get('Missing baseline') || new Set;
Why don't we add "Missing" when we construct the string in render instead?
> 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.
> Websites/perf.webkit.org/public/v3/pages/summary-page.js:269
> + var missingCurrentPlatforms = self._warnings.get('Missing current') || new Set;
Ditto.
> Websites/perf.webkit.org/unit-tests/measurement-set-tests.js:74
> + function waitForMeasurementSet()
You should mention what this function does / why these test changes are needed.
--
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/20160504/2cac27bd/attachment-0001.html>
More information about the webkit-unassigned
mailing list