Bug 175918

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

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

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

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.

Created attachment 318963 [details] Revised

Created attachment 318964 [details] Adding missing 'as'

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

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?

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

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

Comment on attachment 318965 [details] Removed elm from the list of frameworks LGTM (with or without a change for phrasing of numbers)

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

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/

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

Created attachment 318977 [details] Revised

Comment on attachment 318977 [details] Revised As we capitalize the names of other implementations, should we also do this with Elm?

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.

Created attachment 319275 [details] Capitalized elm

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

Created attachment 319984 [details] Reverted the style change

Created attachment 330871 [details] Removed nbsps

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?

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

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

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.

Created attachment 330878 [details] Patch for landing

Comment on attachment 330878 [details] Patch for landing LGTM. r=me.

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

<rdar://problem/36397481>