Bug 195242 – Test freshness page should improve the ability to correlating issues from same builder.

• Component: Tools / Tests
• Status: NEW

Test freshness page should highlight status cells from same builder when mouse is hovering over a cell.

Created attachment 363413 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 363413 [details]
Patch

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

> Websites/perf.webkit.org/public/v3/components/freshness-indicator.js:9
> +        this._builder = builder;

This variable is only used by TestFreshnessPage. That's not a good practice.

> Websites/perf.webkit.org/public/v3/components/freshness-indicator.js:41
> -    _renderIndicator(lastDataPointDuration, testAgeTolerance, summary, url)
> +    _renderIndicator(lastDataPointDuration, testAgeTolerance, summary, url, builder, hightlighted)

There is no point for this function be taking builder since it doesn't use it at all.

> Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:104
> +                const shouldHighlight = this._hoveringIndicator && this._hoveringIndicator.builder() === lastDataPoint.builder
> +                    && indicator !== this._hoveringIndicator;

We should be simply storing the builder name in _hoveringIndicator and then comparing this builder against it.

Comment on attachment 363413 [details]
Patch

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

>> Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:104
>> +                    && indicator !== this._hoveringIndicator;
> 
> We should be simply storing the builder name in _hoveringIndicator and then comparing this builder against it.

The builder for an indicator may not be set during right after indicator initialization. It's set after we fetch the measurement set.
I'll push a patch with a tooltip in the indicator which will show a bigger scope of this feature.

[Ryosuke Niwa]

(In reply to dewei_zhu from comment #3)
> Comment on attachment 363413 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=363413&action=review
> 
> >> Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:104
> >> +                    && indicator !== this._hoveringIndicator;
> > 
> > We should be simply storing the builder name in _hoveringIndicator and then comparing this builder against it.
> 
> The builder for an indicator may not be set during right after indicator
> initialization. It's set after we fetch the measurement set.
> I'll push a patch with a tooltip in the indicator which will show a bigger
> scope of this feature.

Then we should just have a map between each indicator & builder in TestFreshnessPage. We shouldn't be adding an instance variable to FreshnessIndicator just because it's a convenient place to store things.

Created attachment 363737 [details]
Patch

Created attachment 363787 [details]
Patch

Created attachment 363835 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 363835 [details]
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?