WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
NEW
195242
Test freshness page should improve the ability to correlating issues from same builder.
https://bugs.webkit.org/show_bug.cgi?id=195242
Summary
Test freshness page should improve the ability to correlating issues from sam...
dewei_zhu
Reported
2019-03-01 23:16:47 PST
Test freshness page should highlight status cells from same builder when mouse is hovering over a cell.
Attachments
Patch
(9.03 KB, patch)
2019-03-01 23:50 PST
,
dewei_zhu
no flags
Details
Formatted Diff
Diff
Patch
(16.68 KB, patch)
2019-03-06 02:35 PST
,
dewei_zhu
no flags
Details
Formatted Diff
Diff
Patch
(15.76 KB, patch)
2019-03-06 13:51 PST
,
dewei_zhu
no flags
Details
Formatted Diff
Diff
Patch
(15.74 KB, patch)
2019-03-06 20:29 PST
,
dewei_zhu
rniwa
: review+
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
dewei_zhu
Comment 1
2019-03-01 23:50:47 PST
Created
attachment 363413
[details]
Patch
Ryosuke Niwa
Comment 2
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.
dewei_zhu
Comment 3
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.
Ryosuke Niwa
Comment 4
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.
dewei_zhu
Comment 5
2019-03-06 02:35:14 PST
Created
attachment 363737
[details]
Patch
dewei_zhu
Comment 6
2019-03-06 13:51:58 PST
Created
attachment 363787
[details]
Patch
dewei_zhu
Comment 7
2019-03-06 20:29:37 PST
Created
attachment 363835
[details]
Patch
Ryosuke Niwa
Comment 8
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?
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug