I've been coordinating with rniwa@ (and other vendors) as I've been working on updating Speedometer to use the latest version of all frameworks in the benchmark while expanding the workloads it tests. This issue is to track upstreaming this work into Webkit. At present, this work is hosted in a private GitHub repo but we would love to land in trunk so everyone is working off the same canonical version. Given the number of changes involved, rniwa@ suggested trying to land each framework implementation piecemeal. We're happy to do this. I'm also more than happy to welcome help from anyone interested in helping upstream this work as I'm still new to the Webkit contribution process :) More information about the updates to the benchmark can be found below: Speedometer 2.0 is an incremental rewrite of the 1.0 benchmark to both update and expand it’s workloads to better reflect modern patterns, libraries & bundling used in the JS ecosystem. What’s new? --------------------- All Speedometer 1.0 framework apps rewritten for their latest versions. S1 was released in 2014 and included six popular JS frameworks & libraries at the time: Ember, Angular, React, Backbone, jQuery and Flight. Although there have been some large shifts in these libraries over the last two years the benchmark versions have remained stale until now. S2 includes a full rewrite of these apps to use the latest patterns and dependencies for these frameworks. For example, the Ember app was updated using Ember CLI, the now the prescribed way to create new Ember apps in that community. The React app was rewritten with newer best practices and also no longer uses a runtime based JSX transformer (now deprecated). Instead just uses Babel + a build-time Babel-plugin for accomplishing the same. New frameworks and libraries added, including work reflecting modern bundler output The popularity of React has spawned a whole suite of libraries that iterated on it, focusing on different characteristics of performance and lower parse/eval costs and overall developer experience. Two such frameworks are Preact and VueJS (used by and supported by Alibaba). The S2 versions of these apps use Webpack + RollUp (as prescribed by their respective tooling). Vue is predicted to gain significant traction in 2017/18 and is worth tracking. S2 also now tracks state management patterns, via a React + Redux implementation. TypeScript now supported --------------------------------- S2 includes an Angular.js 2.0 implementation transpiled from TypeScript to JS. We believe this to be a common workflow Angular developers will use for new projects in 2017 and transpiled TS code to become more widely used as framework authors warm to it. TypeScript (TS) is a typed superset of JS from Microsoft that required transpilation back to JS in order to be executed. It has been gaining traction in the ecosystem due to considering types as a first-class syntax, generally fast compilation and rich tooling for type-aware autocompletion and error highlighting during iteration. Framework rewrites, such as Angular 2.0 have gone all-in on TS as the default language for their developers and there may be value in engines tracking transpiled TS code. Better reflective of ES2015 code and workflows ---------------------------------------------------- S1 included a vanilla JS implementation. S2 builds on this, introducing an ES2015 implementation taking advantage of features like Classes, const/let, arrow functions and Template strings. Although tracking vanilla ES2015 support has value, the reality is transpilers like Babel have made it trivial to transpile all of your sources back to ES5 for maximum cross-browser compatibility. To reflect this increasingly common workload, S2 includes an ES2015 implementation that uses both ES Modules and has ES5 output generated by Babel. As V8 gains full support for module semantics, we can also include a non-Babelified version of this workload. Future facing ----------------------- We identified functional programming patterns as a set of concepts that aren’t well tracked or optimized for today. Consulting the FP JS community, two primary suggestions that arose were supporting Elm and PureScript, both of which transpile down to JS. S2 includes implementations for both of these. Other alternatives like Om and ClojureScript were considered, but have patterns sufficiently close to the other two that we should be covered.
Created attachment 307290 [details] Patch
Getting to grips with the contribution process as this is my first Webkit patch - some additional styling issues & a bug in the new Ember implementation I'll be addressing shortly.
I think the patch here is too big for me to review. As in, I can't even open it. Please split into chunks of 1MB patches.
Please describe what changes you're making under "reviewed by NOBODY (OOPS!)" followed by a blank line. You don't need per-function/CSS rule comment starting with (~): for each file other than modifications to the existing files. It's sufficient have a list of times for updating frameworks. I think it's best to split the patch that modifies existing files from patches that add new files / updates frameworks.
That's reasonable. I'll follow up with patches that match those suggestions.
Created attachment 307989 [details] Patch
Resubmitted with just an update to Vanilla as a start as I had a question: We've switched from Bower -> npm for package management. Is the way this is being done in the latest patch okay (checked in node_modules)? If so, I'll follow through with the other apps similarly.
(In reply to Addy Osmani from comment #7) > Resubmitted with just an update to Vanilla as a start as I had a question: > > We've switched from Bower -> npm for package management. Is the way this is > being done in the latest patch okay (checked in node_modules)? If so, I'll > follow through with the other apps similarly. What's the benefit of switching to npm here? What does this accomplish for the benchmark?
The benefit of switching to npm (with a package.json for each implem) is that it enables us to more easily update app dependencies over time. With most of the front-end ecosystem switching from Bower to npm for this purpose, this change allows us pull deps from their canonical locations on npm vs. trying to make Bower continue to work for us. To give a more concrete example, many of the frameworks we want to benchmark now recommend a CLI (written in Node, using npm) for creating apps with them. A lot of the app rewrites for S2 use such as CLI and focus on node_modules as their source of truth for library code.
(In reply to Addy Osmani from comment #9) > The benefit of switching to npm (with a package.json for each implem) is > that it enables us to more easily update app dependencies over time. With > most of the front-end ecosystem switching from Bower to npm for this > purpose, this change allows us pull deps from their canonical locations on > npm vs. trying to make Bower continue to work for us. Sounds reasonable to me. > To give a more concrete example, many of the frameworks we want to benchmark > now recommend a CLI (written in Node, using npm) for creating apps with > them. A lot of the app rewrites for S2 use such as CLI and focus on > node_modules as their source of truth for library code. If such apps rely on a CLI, how do they ship to the browser? Do they do something a-la browserify to bundle all the dependencies? If so, do we plan to do the same?
Good question :) The output of these tools is generally a `dist` directory which in many cases uses Webpack (or another module bundler) to turn everything into a browser-friendly version of the app. The benchmark runs against the `dist` version (which I'm also planning on checking in for those apps) so we don't need to run anything extra on our side.
Comment on attachment 307989 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=307989&action=review > PerformanceTests/ChangeLog:7 > +Update the Speedometer > Vanilla application implementation to latest TodoMVC. This change also switches to npm for packaging. This line should be indented like all other lines. We should have a separate bug for each one of these updates, and we should make them block this umbrella bug.
Created attachment 308035 [details] Patch
> We should have a separate bug for each one of these updates, > and we should make them block this umbrella bug. That sounds completely reasonable. I've started the ball rolling on individual bugs for each of these updates.
Fyi, work on each patch here is going to be split into two parts: 1. Updating/adding the newer implementation 2. Hooking up the implementation to the benchmark runner so both Full.html and Interactive can be run against them individually.
Created attachment 308636 [details] Patch
Quick update: - This patch (170513) now includes the updated benchmark runner - Patches updating ReactJS, AngularJS, Backbone, jQuery, EmberJS, VanillaJS, FlightJS are up - Patches adding Angular 2, React + Redux, Preact, VueJS, Inferno, ES2015, ES2015 (transpiled) with Babel + Webpack and Elm are all up.
Comment on attachment 308636 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=308636&action=review > PerformanceTests/Speedometer/InteractiveRunner.html:78 > +// Parse query string parameters in case a specific 'suite' is specified > +// Part of enabling InteractiveRunner.html?suite=React-TodoMVC support > +// so that an individual test can be automatically run via a URL In WebKit, we avoid adding comments these "what" comments. > PerformanceTests/Speedometer/InteractiveRunner.html:79 > +var queryString = (function(a) { Instad, we use a descriptive function name like "function parseQueryString()". Also, please don't use a single letter variable like "a": https://webkit.org/code-style-guidelines/#names-full-words How about pairList? Also, for any anonymous function, there should be a space between function and (). See https://trac.webkit.org/wiki/WebInspectorCodingStyleGuide > PerformanceTests/Speedometer/InteractiveRunner.html:80 > + if (a == '') return {}; How can this ever be true? I think we can remove this code. If we need to keep this code, return should be on a separate line with an indention as in: if (~) return {}; https://webkit.org/code-style-guidelines/#linebreaking-multiple-statements > PerformanceTests/Speedometer/InteractiveRunner.html:83 > + for (var i = 0; i < a.length; ++i) > + { Please place { after for followed by exactly one space as in: for (~) {. > PerformanceTests/Speedometer/InteractiveRunner.html:84 > + var p=a[i].split('=', 2); There should be a space around each assignment and comparison operator such as =: https://webkit.org/code-style-guidelines/#spacing-binary-ternary-op So pairList[i].split('=', 2); I'd call this variable pair or keyValue. > PerformanceTests/Speedometer/InteractiveRunner.html:95 > + if (suiteName !== undefined && suiteName !== '') { We prefer early exits over wrapping in if so: if (~) return; Suites.forEach... > PerformanceTests/Speedometer/InteractiveRunner.html:99 > + if (element.name !== suiteName) { > + element.disabled = true; > + } No curly braces around a single statement: https://webkit.org/code-style-guidelines/#braces-one-line > PerformanceTests/Speedometer/InteractiveRunner.html:105 > + var queryParam = queryString['suite']; Why don't we call parseQueryString here and store the parsed query string in a local variable instead of a global variable? > PerformanceTests/Speedometer/InteractiveRunner.html:107 > + // Reset all selected options except selection This comment repeats the code below. Please remove, and remove the curly braces since it would be a single line statement. > PerformanceTests/Speedometer/InteractiveRunner.html:154 > + if (queryParam !== undefined) { > + // Individually run the specified test automatically > + document.getElementById('runSuites').click(); > + } I think this should be a separate query string option like autoStart, startOnLoad, startAutomatically. Again, the comment repeats what the code below says. Please remove along with the curly braces. > PerformanceTests/Speedometer/resources/tests.js:4 > +function triggerKeyboardEvent(el, keyCode) { Please don't use abbreviations such as el: https://webkit.org/code-style-guidelines/#names-full-words > PerformanceTests/Speedometer/resources/tests.js:6 > + var eventObj = document.createEventObject ? > + document.createEventObject() : document.createEvent("Events"); Do we really need a separate branch for IE? I tried to stay way from having IE-specific code for the sake of running the same code on all browsers. > PerformanceTests/Speedometer/resources/tests.js:11 > + eventObj.initEvent("keydown", true, true); Nit: indentation here is three spaces. Also, no braces around a single line statement. > PerformanceTests/Speedometer/resources/tests.js:17 > + el.dispatchEvent ? el.dispatchEvent(eventObj) : el.fireEvent("onkeydown", eventObj); Do we really need to have a branch for fireEvent? > PerformanceTests/Speedometer/resources/tests.js:33 > + triggerKeyboardEvent(newTodo, 13); Instead of repeating the magic key code of 13 everywhere, can we define a variable with a descriptive name somewhere? e.g. var ENTER_KEY_CODE = 13 in the global code. > PerformanceTests/Speedometer/resources/tests.js:67 > + for (var i = 0; i < checkboxes.length; i++) Given this is a ES2015 test, we might as well as use for (const box of checkboxes) here. > PerformanceTests/Speedometer/resources/tests.js:72 > + for (var i = 0; i < deleteButtons.length; i++) Ditto. > PerformanceTests/Speedometer/resources/tests.js:126 > + var keydownEvent = document.createEvent('Event'); > + keydownEvent.initEvent('keydown', true, true); > + keydownEvent.which = 13; // VK_ENTER > + newTodo.dispatchEvent(keydownEvent); We can't use triggerKeyboardEvent here? > PerformanceTests/Speedometer/resources/tests.js:132 > + for (var i = 0; i < checkboxes.length; i++) > + checkboxes[i].click(); Does this click result in React doing some work? If it's just dirtying React tests, we might be just testing how fast click executes, which isn't really all that interesting. This is why my Ember test case had manually called emberRun to force flushing of the DOM state updates since what we're really interested in measuring how long framework code takes to run after user interaction not so much how many user interactions it can handle per their event loop (since a normal user would never click 25 elements in a single task). > PerformanceTests/Speedometer/resources/tests.js:318 > + var keydownEvent = document.createEvent('Event'); > + keydownEvent.initEvent('keydown', true, true); > + keydownEvent.which = 13; > + newTodo.dispatchEvent(keydownEvent); Can't use triggerKeyboardEvent here either? > PerformanceTests/Speedometer/resources/tests.js:359 > - for (var i = 0; i < checkboxes.length; i++) > + for (var i = 0; i < checkboxes.length; i++) { > itemIndexToId[i] = $(checkboxes[i]).closest('li').data('id'); > + } Nit: This code change is unnecessary, and in fact makes the code violate WebKit's code style guideline: https://webkit.org/code-style-guidelines/#braces-one-line > PerformanceTests/Speedometer/resources/tests.js:381 > + var clear = contentDocument.querySelector('#clear-completed'); > + if (clear !== null) { > + clear.click(); > + } Huh, it's kind of interesting that we only clear items in the jQuery test case. It's probably an oversight when I first wrote it. But why do we need to add this if now? It's bad for benchmarks to sometimes call clear. Either we should get rid this call to clear() altogether or make the test case deterministic. r- because we can't have this non-determinism / ambiguity. > PerformanceTests/Speedometer/resources/tests.js:395 > - name: 'AngularJS-TodoMVC', > - url: 'todomvc/architecture-examples/angularjs/index.html', > + name: 'Preact-TodoMVC', > + url: 'todomvc/architecture-examples/preact/build/index.html', I would have preferred if you didn't change the ordering so that the diff doesn't look so crazy but perhaps you're ordering them lexicologically? If so, we could just do that after all suites are added. > PerformanceTests/Speedometer/resources/tests.js:424 > + url: 'todomvc/vanilla-examples/es2015-babel-webpack/dist/index.html', I think you meant to refer to todomvc/architecture-examples/inferno/index.html ?
Created attachment 309999 [details] Patch
This is a revision to the original CL with the following: - Attempted to address feedback, in particular around style - Fewer considerations for older browsers (IE), simplifying event code - Added 'startAutomatically' support. Works with the full suite or individual suites - Discovered some apps we committed required some very minor fixes to work with the updated runner - Tested both Full.html and InteractiveRunner.html in multiple browsers. Appears to now be working end to end
Comment on attachment 309999 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=309999&action=review > PerformanceTests/Speedometer/InteractiveRunner.html:79 > + for (var i = 0; i < pairList.length; ++i) > + { Nit: { should be on the same line as for with a space as in: for (~) { > PerformanceTests/Speedometer/InteractiveRunner.html:89 > +// Disable all suites other than a suite specified with a query string This comment repeats what the code below says. Please remove it. > PerformanceTests/Speedometer/resources/tests.js:13 > +var eventCache = {}; > +['keyup', 'keydown', 'keypress'].forEach(function (type) { > + var event = document.createEvent('Events'); > + event.initEvent(type, true, true); > + Object.defineProperty(event, 'keyCode', {value: ENTER_KEY_CODE}); > + Object.defineProperty(event, 'which', {value: ENTER_KEY_CODE}); > + event.key = 'ENTER'; > + eventCache[type] = event; > +}) I think it's better to create new events each time. Re-using events like this is not a common pattern. > PerformanceTests/Speedometer/resources/tests.js:215 > + var appView = contentWindow.appView; > + var fakeEvent = {which: 13}; Do we need appView/fakeEvent here? > PerformanceTests/Speedometer/resources/tests.js:348 > - var todomvc = contentWindow.todomvc; > + var app = contentWindow.app; Looks like this variable is never used? > PerformanceTests/Speedometer/resources/tests.js:355 > + var app = contentWindow.app; Ditto. > PerformanceTests/Speedometer/resources/tests.js:364 > + if (clear !== null) { > + clear.click(); > + } Nit: No curly braces around a single statement. > PerformanceTests/Speedometer/resources/tests.js:371 > + for (var i = 0; i < deleteButtons.length; i++) { > + app.currentItemIndex = i; > + app.destroy({ target: deleteButtons[i] }); > + } Really!? We can't invoke click() on the buttons? > PerformanceTests/Speedometer/resources/todomvc/architecture-examples/emberjs/index.html:9 > - <base href="/PerformanceTests/Speedometer/resources/todomvc/architecture-examples/emberjs" /> > + <base href="/Speedometer/resources/todomvc/architecture-examples/emberjs/" /> Please get rid of this base altogether. Speedometer is not always hosted at the top-level directory.
Created attachment 310006 [details] Patch
Created attachment 310007 [details] Patch
Created attachment 310008 [details] Patch
Thanks for the feedback. PTAL. > Really!? We can't invoke click() on the buttons? I thought this was necessary due to an Edge bug, but it appears to work just fine with the pattern used in the other apps. Switched to this in the latest changes. > I think it's better to create new events each time. Re-using events like this is not a common pattern. Switched to a pattern that doesn't use caching. > Emberjs > Please get rid of this base altogether. Speedometer is not always hosted at the top-level directory. The version of ember-cli used for this application prescribes the use of base to specify the root (yes, it's a bizarre pattern). Without this in place, the rest of their routing and configuration fails to correctly load. I've read that there are some hacks possible to work around this limitation, but figured getting this working initially would allow us to chase that up in a follow up CL. I'm happy to work on that.
Comment on attachment 310007 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310007&action=review > PerformanceTests/Speedometer/InteractiveRunner.html:142 > + if (parseQueryString['startAutomatically'] !== undefined) I guess we could also do 'startAutomatically' in parseQueryString. > PerformanceTests/Speedometer/resources/tests.js:9 > + Object.defineProperty(event, 'keyCode', {value: ENTER_KEY_CODE}); > + Object.defineProperty(event, 'which', {value: ENTER_KEY_CODE}); Why do we need to use defineProperty? Probably deserves a comment here or a comment in the change log in the form of Speedometer/resources/tests.js (triggerEnter): ~ > PerformanceTests/Speedometer/resources/tests.js:178 > + var appView = contentWindow.App; It looks like this variable is not used. > PerformanceTests/Speedometer/resources/tests.js:210 > + var appView = contentWindow.appView; > + var fakeEvent = {which: 13}; It looks like these variables aren't used. > PerformanceTests/Speedometer/resources/tests.js:274 > + var appView = contentWindow.App; Ditto. > PerformanceTests/Speedometer/resources/tests.js:308 > var todomvc = contentWindow.todomvc; It looks like this variable is never used either. We might as well as delete it while we're at. > PerformanceTests/Speedometer/resources/tests.js:356 > + var clear = contentDocument.querySelector('#clear-completed'); > + if (clear !== null) > + clear.click(); Not saying that we should in this patch but we should consider either deleting this code or doing it in all other test cases. It's odd to do this only in jQuery test case. > PerformanceTests/Speedometer/resources/tests.js:358 > + Maybe delete this blank line? We don't have it elsewhere. > PerformanceTests/Speedometer/resources/todomvc/architecture-examples/angular/dist/index.html:12 > -<link href="styles.d41d8cd98f00b204e980.bundle.css" rel="stylesheet"/></head> > +</head> For these changes in the benchmark contents, you should explain why you're making changes. > PerformanceTests/Speedometer/resources/todomvc/architecture-examples/emberjs/index.html:9 > - <base href="/PerformanceTests/Speedometer/resources/todomvc/architecture-examples/emberjs" /> > + <base href="/Speedometer/resources/todomvc/architecture-examples/emberjs/" /> Again, please get rid of this base element. We can't require the benchmark be always hosted on a server or placed at /. > PerformanceTests/Speedometer/resources/todomvc/architecture-examples/emberjs/source/app/services/repo.js:2 > -import * as localStorageMemory from './memory'; > +import localStorageMemory from './memory'; Why is this change needed at all even it's not be used in the benchmark? You should at least explain the rationale in the change log after <filename>:. > PerformanceTests/Speedometer/resources/todomvc/architecture-examples/jquery/node_modules/director/LICENSE:1 > +Copyright (c) 2011 Charlie Robbins, Paolo Fragomeni, & the Contributors. You should explain in the change log why you're adding these files.
(In reply to Addy Osmani from comment #25) > > The version of ember-cli used for this application prescribes the use of > base to specify the root (yes, it's a bizarre pattern). Without this in > place, the rest of their routing and configuration fails to correctly load. > I've read that there are some hacks possible to work around this limitation, > but figured getting this working initially would allow us to chase that up > in a follow up CL. I'm happy to work on that. Okay, that certainly needs to be fixed. We shouldn't be requiring the benchmark to be always hosted on a server or be placed at /.
As you suggested we can fix it in a separate patch but we can't call it done until that's fixed.
Created attachment 310011 [details] Patch
> Ember fixes I completely agree. I'll file a separate bug and self-assign to address the path issue. > Maybe delete this blank line? We don't have it elsewhere. Deleted > Not saying that we should in this patch but we should consider either deleting this code or doing it in all other test cases. It's odd to do this only in jQuery test case. Removed this from the test. > It looks like this variable is never used either. We might as well as delete it while we're at. Removed. > It looks like this variable is not used. Removed x2 > Why do we need to use defineProperty? Dropped in favor of event.x assignment.
Created attachment 310057 [details] Patch
In addition to the other requested edits addressed, I have also fixed the issue of the Ember implementation needing a base tag to function correctly in https://bugs.webkit.org/attachment.cgi?id=310057&action=diff#a/PerformanceTests/Speedometer/resources/todomvc/architecture-examples/emberjs/index.html_sec1. The workaround was fixing Ember's embedded environment configuration to set locationType to 'none'. The decoded version of that string is accordingly: ``` <meta name="todomvc/config/environment" content="{"modulePrefix":"todomvc","environment":"development","locationType":"none","EmberENV":{"FEATURES":{}},"APP":{"name":"todomvc","version":"0.0.0 2c3f8158"},"exportApplicationGlobal":true}" /> ``` which has the (encoded/final built) form of: ``` <meta name="todomvc/config/environment" content="%7B%22modulePrefix%22%3A%22todomvc%22%2C%22environment%22%3A%22development%22%2C%22locationType%22%3A%22none%22%2C%22EmberENV%22%3A%7B%22FEATURES%22%3A%7B%7D%7D%2C%22APP%22%3A%7B%22name%22%3A%22todomvc%22%2C%22version%22%3A%220.0.0%202c3f8158%22%7D%2C%22exportApplicationGlobal%22%3Atrue%7D" /> ```
Comment on attachment 310057 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310057&action=review > PerformanceTests/ChangeLog:8 > + Speedometer: refresh test runner and fix apps to work with it I don't think "Speedometer: " prefix is needed. Also please end the sentence with a period. > PerformanceTests/ChangeLog:11 > + (parseQueryString): Adds support for startAutomatically param Nit: query parameter* and missing a period. > PerformanceTests/ChangeLog:18 > + (base): enable Ember TodoMVC to be run from any directory/level Nit: Capitalize "enable" and add a period as well. > PerformanceTests/ChangeLog:31 > + (director): Add missing director.js dependency for jQuery TodoMVC implementation (~) is used for a function. Here, you're talking about adding a new directory so your comment should appear next to the first file being added. Since it's a long time, you can wrap the line after the first sentence and align your second line of comment to "* " at the beginning of each file as in: * Speedometer/~/: This is some comment. More comment here. > PerformanceTests/ChangeLog:33 > + (path): Fix path to built Elm TodoMVC scripts (build -> dist) You're not fixing a function named path so this should appear after : in the previous line. You can wrap the line at a reasonable length, of course. > PerformanceTests/Speedometer/InteractiveRunner.html:90 > + if (suiteName === undefined || suiteName === '') > + return; Why are we checking this twice in its call site and here? > PerformanceTests/Speedometer/resources/tests.js:56 > + newTodo.value = 'Something to do' + i; I think we should have a space after "do" otherwise, the text would look like "Something to do5". > PerformanceTests/Speedometer/resources/tests.js:86 > + newTodo.value = 'Something to do' + i; Ditto about having a space after do. > PerformanceTests/Speedometer/resources/tests.js:150 > + newTodo.value = 'Something to do' + i; Ditto about having a space after do. > PerformanceTests/Speedometer/resources/tests.js:179 > + newTodo.value = 'Something to do' + i; Ditto about having a space after do. > PerformanceTests/Speedometer/resources/tests.js:272 > + newTodo.value = 'Something to do' + i; Ditto about having a space after do. > PerformanceTests/Speedometer/resources/tests.js:306 > + newTodo.value = 'Something to do ' + i; > + newTodo.value = 'Something to do' + i; Duplicated code. Also the first statement contains one extra space. > PerformanceTests/Speedometer/resources/tests.js:368 > + newTodo.value = 'Something to do' + i; Ditto about having a space after do. > PerformanceTests/Speedometer/resources/tests.js:397 > + newTodo.value = 'Something to do' + i; Ditto about having a space after do. > PerformanceTests/Speedometer/resources/tests.js:430 > + newTodo.value = 'Something to do' + i; Ditto about having a space after do. > PerformanceTests/Speedometer/resources/tests.js:463 > + newTodo.value = 'Something to do' + i; Ditto about having a space after do. > PerformanceTests/Speedometer/resources/todomvc/architecture-examples/emberjs/index.html:20 > - <script src="/PerformanceTests/Speedometer/resources/todomvc/architecture-examples/emberjs/assets/vendor.js"></script> > - <script src="/PerformanceTests/Speedometer/resources/todomvc/architecture-examples/emberjs/assets/todomvc.js"></script> > + <script src="assets/vendor.js"></script> > + <script src="assets/todomvc.js"></script> Nice fix!
Created attachment 310234 [details] Patch
Comment on attachment 310234 [details] Patch PTAL if you get a chance. Sorry for last round of nits!
Thanks for updating the frameworks!
Comment on attachment 310234 [details] Patch Clearing flags on attachment: 310234 Committed r216946: <http://trac.webkit.org/changeset/216946>
All reviewed patches have been landed. Closing bug.
*** Bug 147768 has been marked as a duplicate of this bug. ***