Bug 201105 - [results.webkit.org Timline] Add symbols to the timeline dot
Summary: [results.webkit.org Timline] Add symbols to the timeline dot
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zhifei Fang
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-08-23 16:47 PDT by Zhifei Fang
Modified: 2019-08-26 16:37 PDT (History)
6 users (show)

See Also:


Attachments
Patch (10.07 KB, patch)
2019-08-23 16:50 PDT, Zhifei Fang
no flags Details | Formatted Diff | Diff
Symbols (deleted)
2019-08-23 16:58 PDT, Zhifei Fang
no flags Details
Symbols (deleted)
2019-08-23 17:01 PDT, Zhifei Fang
no flags Details
Archive of layout-test-results from ews215 for win-future (13.81 MB, application/zip)
2019-08-23 19:05 PDT, EWS Watchlist
no flags Details
Patch (19.65 KB, patch)
2019-08-26 14:46 PDT, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch (19.76 KB, patch)
2019-08-26 15:30 PDT, Zhifei Fang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zhifei Fang 2019-08-23 16:47:41 PDT
Add symbols to the timeline dot, make them more meaningful
Comment 1 Zhifei Fang 2019-08-23 16:50:32 PDT
Created attachment 377180 [details]
Patch
Comment 2 Zhifei Fang 2019-08-23 16:58:32 PDT
Created attachment 377183 [details]
Symbols
Comment 3 Zhifei Fang 2019-08-23 17:01:20 PDT
Created attachment 377184 [details]
Symbols
Comment 4 Ryosuke Niwa 2019-08-23 17:13:02 PDT
Comment on attachment 377180 [details]
Patch

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

> Tools/resultsdbpy/resultsdbpy/view/static/js/timeline.js:531
> +            failed: 'X',

Can we use ✘ or ❌ instead?

> Tools/resultsdbpy/resultsdbpy/view/static/js/timeline.js:563
> -                        if (data.stats[`tests${willFilterExpected ? '_unexpected_' : '_'}${type}`] > 0)
> +                        if (data.stats[`tests${willFilterExpected ? '_unexpected_' : '_'}${type}`] > 0) {

Can data.stats be negative? Otherwise we should be omitting > 0 per:
https://webkit.org/code-style-guidelines/#zero-null
Comment 5 Ryosuke Niwa 2019-08-23 17:13:09 PDT
Thanks for this patch!
Comment 6 Ryosuke Niwa 2019-08-23 17:13:45 PDT
I'd refrain from r+ since I don't know this codebase yet.
Comment 7 Ling Ho 2019-08-23 17:27:58 PDT
The content of attachment 377183 [details] has been deleted
Comment 8 Ling Ho 2019-08-23 17:28:14 PDT
The content of attachment 377184 [details] has been deleted
Comment 9 Jonathan Bedard 2019-08-23 17:32:02 PDT
(In reply to Ryosuke Niwa from comment #4)
> Comment on attachment 377180 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377180&action=review
> 
> > Tools/resultsdbpy/resultsdbpy/view/static/js/timeline.js:531
> > +            failed: 'X',
> 
> Can we use ✘ or ❌ instead?

I would prefer ✘ so we don't have and colored unicode elements.

> 
> > Tools/resultsdbpy/resultsdbpy/view/static/js/timeline.js:563
> > -                        if (data.stats[`tests${willFilterExpected ? '_unexpected_' : '_'}${type}`] > 0)
> > +                        if (data.stats[`tests${willFilterExpected ? '_unexpected_' : '_'}${type}`] > 0) {
> 
> Can data.stats be negative? Otherwise we should be omitting > 0 per:
> https://webkit.org/code-style-guidelines/#zero-null
Comment 10 Jonathan Bedard 2019-08-23 17:38:17 PDT
Comment on attachment 377180 [details]
Patch

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

> Tools/resultsdbpy/resultsdbpy/view/static/js/timeline.js:529
> +        const symbolMap = {

We actually generate the legend in this file ( Legend(...) ) Can we define these globally and fix the scale in this patch too?
Comment 11 Zhifei Fang 2019-08-23 17:45:49 PDT
(In reply to Jonathan Bedard from comment #10)
> Comment on attachment 377180 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377180&action=review
> 
> > Tools/resultsdbpy/resultsdbpy/view/static/js/timeline.js:529
> > +        const symbolMap = {
> 
> We actually generate the legend in this file ( Legend(...) ) Can we define
> these globally and fix the scale in this patch too?

Will do
Comment 12 EWS Watchlist 2019-08-23 19:05:01 PDT
Comment on attachment 377180 [details]
Patch

Attachment 377180 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12963467

New failing tests:
js/error-should-not-strong-reference-global-object.html
Comment 13 EWS Watchlist 2019-08-23 19:05:03 PDT
Created attachment 377194 [details]
Archive of layout-test-results from ews215 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews215  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 14 Zhifei Fang 2019-08-26 14:46:08 PDT
Created attachment 377272 [details]
Patch
Comment 15 Jonathan Bedard 2019-08-26 15:09:21 PDT
Comment on attachment 377272 [details]
Patch

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

Looks really good! The only changes remaining are changing the x style (Ryosuke asked for that, I think the unicode x looks better) and making dots un-selectable.

Great work!

> Tools/resultsdbpy/resultsdbpy/view/static/library/css/webkit.css:2156
> +  left: calc(0px - var(--tinySize));

I don't think that we want dots to be selectable.
Comment 16 Zhifei Fang 2019-08-26 15:30:36 PDT
Created attachment 377277 [details]
Patch
Comment 17 WebKit Commit Bot 2019-08-26 16:36:57 PDT
Comment on attachment 377277 [details]
Patch

Clearing flags on attachment: 377277

Committed r249124: <https://trac.webkit.org/changeset/249124>
Comment 18 WebKit Commit Bot 2019-08-26 16:36:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Radar WebKit Bug Importer 2019-08-26 16:37:26 PDT
<rdar://problem/54727835>