Bug 195242

Summary: Test freshness page should improve the ability to correlating issues from same builder.
Product: WebKit Reporter: dewei_zhu
Component: New BugsAssignee: dewei_zhu
Status: NEW ---    
Severity: Normal CC: dewei_zhu, rniwa
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch rniwa: review+

Description dewei_zhu 2019-03-01 23:16:47 PST
Test freshness page should highlight status cells from same builder when mouse is hovering over a cell.
Comment 1 dewei_zhu 2019-03-01 23:50:47 PST
Created attachment 363413 [details]
Patch
Comment 2 Ryosuke Niwa 2019-03-04 16:00:35 PST
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 3 dewei_zhu 2019-03-04 16:49:20 PST
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.
Comment 4 Ryosuke Niwa 2019-03-04 17:13:28 PST
(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.
Comment 5 dewei_zhu 2019-03-06 02:35:14 PST
Created attachment 363737 [details]
Patch
Comment 6 dewei_zhu 2019-03-06 13:51:58 PST
Created attachment 363787 [details]
Patch
Comment 7 dewei_zhu 2019-03-06 20:29:37 PST
Created attachment 363835 [details]
Patch
Comment 8 Ryosuke Niwa 2019-03-11 15:36:21 PDT
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?