[webkit-reviews] review granted: [Bug 195242] Test freshness page should improve the ability to correlating issues from same builder. : [Attachment 363835] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted dewei_zhu at apple.com's request for
review:
Bug 195242: Test freshness page should improve the ability to correlating
issues from same builder.
https://bugs.webkit.org/show_bug.cgi?id=195242

Attachment 363835: Patch

https://bugs.webkit.org/attachment.cgi?id=363835&action=review




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

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

> Websites/perf.webkit.org/public/v3/components/freshness-indicator.js:25
> +	   container.addEventListener('mouseenter', () =>
this.dispatchAction('mouseEnterIndicator', this));

How about "highlight" and "unhighlight" or "select" and "unselect".
By the way, it's not great that this UI is only triggering from mouse from
accessibility point of view.
Maybe also make it possible to focus these indicators and trigger these when
focus / blur?
We can do that in a separate patch though.

> Websites/perf.webkit.org/public/v3/components/freshness-indicator.js:78
> +	       a:hover,

We probably shouldn't automatically highlight during :hover if highlighting is
an explicit operation now.

> Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:12
> +	   this._hoveringIndicator = null;

I think more canonical way to name these things in perf dashboard codebase is
like _currentlyHighlightedIndicator.

> Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:151
> +	   const containerPadding = 0.3;
> +	   const containerMargin = 0.3;

Just get this out of the computed style:
getComputedStyle(indicator.element()).paddingLeft. You'd get it in px even!

> Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:153
> +	   const containerHeight = 2;
> +	   const containerWidth = 19;

Ditto.

> Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:158
> +	   tooltipContainer.style.top = rect.top - (containerHeight +
containerPadding * 2 + containerMargin * 2 - cellMargin) * pixelsPerREM  +
'px';

This is kind of silly. Why don't we simply set style.bottom instead?

> Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:159
> +	   tooltipContainer.style.left = rect.left + rect.width / 2 -
containerWidth / 2 * pixelsPerREM + containerPadding +	containerMargin + 'px';

Nit: two spaces between + and containerMargin

> Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:305
> +		   width: 19rem;

19 seems like an oddly specific number. How about 20rem?