Bug 175918 – Speedometer 2.0: Update the About page

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Ryosuke Niwa]

Update the description for Speedometer 2.0.

[Ryosuke Niwa]

Created attachment 318956 [details]
Updates the about page and the version number

[Addy Osmani]

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).

[Ryosuke Niwa]

(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?

[Addy Osmani]

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.

[Ryosuke Niwa]

Created attachment 318963 [details]
Revised

[Ryosuke Niwa]

Created attachment 318964 [details]
Adding missing 'as'

[Ryosuke Niwa]

It's kind of weird to say "ten frameworks" and then list twelve things though...

[Addy Osmani]

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?

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Created attachment 318965 [details]
Removed elm from the list of frameworks

[Addy Osmani]

Comment on attachment 318965 [details]
Removed elm from the list of frameworks

LGTM (with or without a change for phrasing of numbers)

[Ryosuke Niwa]

Created attachment 318968 [details]
Revised per Maciej's comments

[Mathias Bynens]

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/

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Created attachment 318977 [details]
Revised

[Addy Osmani]

Comment on attachment 318977 [details]
Revised

As we capitalize the names of other implementations, should we also do this with Elm?

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 319275 [details]
Capitalized elm

[Ryosuke Niwa]

Created attachment 319983 [details]
Updated again per Maciej's comments

[Ryosuke Niwa]

Created attachment 319984 [details]
Reverted the style change

[Ryosuke Niwa]

Created attachment 330871 [details]
Removed nbsps

[Keith Miller]

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?

[Ryosuke Niwa]

(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"

[Ryosuke Niwa]

We'd go with "the UI thread" since "the main thread" isn't really a thing on the Web.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 330878 [details]
Patch for landing

[Keith Miller]

Comment on attachment 330878 [details]
Patch for landing

LGTM. r=me.

[Ryosuke Niwa]

Committed r226694: <https://trac.webkit.org/changeset/226694>

[Radar WebKit Bug Importer]

<rdar://problem/36397481>