Bug 175918 - Speedometer 2.0: Update the About page
Summary: Speedometer 2.0: Update the About page
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks: 172339
  Show dependency treegraph
 
Reported: 2017-08-23 19:05 PDT by Ryosuke Niwa
Modified: 2018-01-09 20:52 PST (History)
9 users (show)

See Also:


Attachments
Updates the about page and the version number (3.96 KB, patch)
2017-08-23 19:28 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Revised (4.51 KB, patch)
2017-08-23 21:00 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Adding missing 'as' (4.51 KB, patch)
2017-08-23 21:02 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Removed elm from the list of frameworks (4.51 KB, patch)
2017-08-23 21:07 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Revised per Maciej's comments (5.12 KB, patch)
2017-08-23 21:53 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Revised (5.16 KB, patch)
2017-08-24 02:03 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Capitalized elm (5.16 KB, patch)
2017-08-29 13:06 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Updated again per Maciej's comments (5.10 KB, patch)
2017-09-05 22:23 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Reverted the style change (4.67 KB, patch)
2017-09-05 22:24 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Removed nbsps (4.62 KB, patch)
2018-01-09 19:23 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (5.33 KB, patch)
2018-01-09 20:41 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2017-08-23 19:05:52 PDT
Update the description for Speedometer 2.0.
Comment 1 Ryosuke Niwa 2017-08-23 19:28:00 PDT
Created attachment 318956 [details]
Updates the about page and the version number
Comment 2 Addy Osmani 2017-08-23 19:39:33 PDT
Comment on attachment 318956 [details]
Updates the about page and the version number

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

LGTM overall with minor suggestions

> PerformanceTests/Speedometer/index.html:72
> +            and with ten popular JavaScript frameworks: React, Ember.js, Backbone.js, AngularJS, Vue.js, jQuery, Preact, Inferno, elm, and Flight.

This looks good. Perhaps the one suggestion here is including Elm in the earlier list with ES5 and ES2015 as it is a compile-to-JS language. We could also include "ES2015 (transpiled with Babel and Webpack)" in that line if you wanted (as it is another workload we include now).
Comment 3 Ryosuke Niwa 2017-08-23 19:42:26 PDT
(In reply to Addy Osmani from comment #2)
> Comment on attachment 318956 [details]
> Updates the about page and the version number
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318956&action=review
> 
> LGTM overall with minor suggestions
> 
> > PerformanceTests/Speedometer/index.html:72
> > +            and with ten popular JavaScript frameworks: React, Ember.js, Backbone.js, AngularJS, Vue.js, jQuery, Preact, Inferno, elm, and Flight.
> 
> This looks good. Perhaps the one suggestion here is including Elm in the
> earlier list with ES5 and ES2015 as it is a compile-to-JS language. We could
> also include "ES2015 (transpiled with Babel and Webpack)" in that line if
> you wanted (as it is another workload we include now).

Should we mention Redux there as well? I couldn't come up with a concise & natural sentence to include all that. Any ideas on how to phrase it?
Comment 4 Addy Osmani 2017-08-23 20:28:46 PDT
Oh. Good catch. I tend to consider Redux best coupled with React when listing them. Perhaps "React, React + Redux, Ember.js, Backbone.js etc"? Alternatively, could be included at the end of the list. I'd bucket it with the frameworks list here either way.
Comment 5 Ryosuke Niwa 2017-08-23 21:00:46 PDT
Created attachment 318963 [details]
Revised
Comment 6 Ryosuke Niwa 2017-08-23 21:02:52 PDT
Created attachment 318964 [details]
Adding missing 'as'
Comment 7 Ryosuke Niwa 2017-08-23 21:03:55 PDT
It's kind of weird to say "ten frameworks" and then list twelve things though...
Comment 8 Addy Osmani 2017-08-23 21:04:30 PDT
Comment on attachment 318964 [details]
Adding missing 'as'

We currently still include elm in the frameworks list "Inferno, elm, and Flight. ". Perhaps drop now that it is mentioned earlier?
Comment 9 Ryosuke Niwa 2017-08-23 21:06:30 PDT
(In reply to Addy Osmani from comment #8)
> Comment on attachment 318964 [details]
> Adding missing 'as'
> 
> We currently still include elm in the frameworks list "Inferno, elm, and
> Flight. ". Perhaps drop now that it is mentioned earlier?

Good catch.
Comment 10 Ryosuke Niwa 2017-08-23 21:07:26 PDT
Created attachment 318965 [details]
Removed elm from the list of frameworks
Comment 11 Addy Osmani 2017-08-23 21:10:11 PDT
Comment on attachment 318965 [details]
Removed elm from the list of frameworks

LGTM (with or without a change for phrasing of numbers)
Comment 12 Ryosuke Niwa 2017-08-23 21:53:57 PDT
Created attachment 318968 [details]
Revised per Maciej's comments
Comment 13 Mathias Bynens 2017-08-24 00:51:20 PDT
Comment on attachment 318968 [details]
Revised per Maciej's comments

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

Left some comments with minor nits, but LGTM either way!

> PerformanceTests/Speedometer/index.html:73
> +            It calls DOM APIs directly in ECMAScript 5, ECMAScript 2015 (also known as ES6), ECMAScript 2015 and elm transpiled into ECMAScript 5.

This seems to mention ECMAScript 2015 twice in a row. Maybe rephrase to something like this?

    It calls DOM APIs directly in ECMAScript 5 and ECMAScript 2015 (also known as ES6). Additionally, it tests transpiled versions of ECMAScript 2015 and elm code.

> PerformanceTests/Speedometer/index.html:75
> +            React, React with Redux, Ember.js, Backbone.js, two versions of AngularJS, Vue.js, jQuery, Preact, Inferno, and Flight. 

Minor nit: “AngularJS” refers to v1, so we’re not using “two versions of AngularJS”. The other version is called “Angular”.  Maybe just say “Angular 1[.6.5] and Angular 4”?

> PerformanceTests/Speedometer/index.html:94
> +            Speedometer approximates the runtime of these asynchronous work with a zero-second timer

s/these/this/
Comment 14 Ryosuke Niwa 2017-08-24 02:03:46 PDT
(In reply to Mathias Bynens from comment #13)
> Comment on attachment 318968 [details]
> Revised per Maciej's comments
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318968&action=review
> 
> Left some comments with minor nits, but LGTM either way!
> 
> > PerformanceTests/Speedometer/index.html:73
> > +            It calls DOM APIs directly in ECMAScript 5, ECMAScript 2015 (also known as ES6), ECMAScript 2015 and elm transpiled into ECMAScript 5.
> 
> This seems to mention ECMAScript 2015 twice in a row. Maybe rephrase to
> something like this?
> 
>     It calls DOM APIs directly in ECMAScript 5 and ECMAScript 2015 (also
> known as ES6). Additionally, it tests transpiled versions of ECMAScript 2015
> and elm code.

Sure. Let's connect ES6/elm part with frameworks.

> > PerformanceTests/Speedometer/index.html:75
> > +            React, React with Redux, Ember.js, Backbone.js, two versions of AngularJS, Vue.js, jQuery, Preact, Inferno, and Flight. 
> 
> Minor nit: “AngularJS” refers to v1, so we’re not using “two versions of
> AngularJS”. The other version is called “Angular”.  Maybe just say “Angular
> 1[.6.5] and Angular 4”?

That's a good point. Replaced it with "AngularJS, (new) Angular" since we don't mention the versions of frameworks elsewhere.

> 
> > PerformanceTests/Speedometer/index.html:94
> > +            Speedometer approximates the runtime of these asynchronous work with a zero-second timer
> 
> s/these/this/

Fixed.
Comment 15 Ryosuke Niwa 2017-08-24 02:03:53 PDT
Created attachment 318977 [details]
Revised
Comment 16 Addy Osmani 2017-08-25 18:21:16 PDT
Comment on attachment 318977 [details]
Revised

As we capitalize the names of other implementations, should we also do this with Elm?
Comment 17 Ryosuke Niwa 2017-08-25 20:02:54 PDT
Oh, I was under the impression that elm is always written in lowercase but it doesn't seem to be the case after all. Will fix that.
Comment 18 Ryosuke Niwa 2017-08-29 13:06:46 PDT
Created attachment 319275 [details]
Capitalized elm
Comment 19 Ryosuke Niwa 2017-09-05 22:23:09 PDT
Created attachment 319983 [details]
Updated again per Maciej's comments
Comment 20 Ryosuke Niwa 2017-09-05 22:24:35 PDT
Created attachment 319984 [details]
Reverted the style change
Comment 21 Ryosuke Niwa 2018-01-09 19:23:19 PST
Created attachment 330871 [details]
Removed nbsps
Comment 22 Keith Miller 2018-01-09 20:03:46 PST
Comment on attachment 330871 [details]
Removed nbsps

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

r=me with a comment.

> PerformanceTests/Speedometer/index.html:92
> +            Speedometer approximates the run time of this asynchronous work with a zero-second timer
> +            that is scheduled immediately after each execution of synchronous operations.</p>

Perhaps it's worth describing some Pros and Cons to this approach?
Comment 23 Ryosuke Niwa 2018-01-09 20:28:00 PST
(In reply to Keith Miller from comment #22)
> Comment on attachment 330871 [details]
> Removed nbsps
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=330871&action=review
> 
> r=me with a comment.
> 
> > PerformanceTests/Speedometer/index.html:92
> > +            Speedometer approximates the run time of this asynchronous work with a zero-second timer
> > +            that is scheduled immediately after each execution of synchronous operations.</p>
> 
> Perhaps it's worth describing some Pros and Cons to this approach?

After some IRC conversions, what we're missing is clarifying that "asynchronous work" here refers to work in the main thread. So I'm rephrasing it as "Speedometer approximates the run time of this asynchronous work in the main thread with a zero-second timer"
Comment 24 Ryosuke Niwa 2018-01-09 20:31:54 PST
We'd go with "the UI thread" since "the main thread" isn't really a thing on the Web.
Comment 25 Ryosuke Niwa 2018-01-09 20:41:30 PST
We're adding one more clause at the very end:

Speedometer does not attempt to measure concurrent asynchronous work that does not directly impact the UI thread, which tends not to affect app responsiveness.
Comment 26 Ryosuke Niwa 2018-01-09 20:41:58 PST
Created attachment 330878 [details]
Patch for landing
Comment 27 Keith Miller 2018-01-09 20:44:54 PST
Comment on attachment 330878 [details]
Patch for landing

LGTM. r=me.
Comment 28 Ryosuke Niwa 2018-01-09 20:51:11 PST
Committed r226694: <https://trac.webkit.org/changeset/226694>
Comment 29 Radar WebKit Bug Importer 2018-01-09 20:52:18 PST
<rdar://problem/36397481>