Bug 175818 – Speedometer: Make React/Redux item order consistent

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Mathias Bynens]

Speedometer: Make React/Redux item order consistent

[Mathias Bynens]

Created attachment 318749 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 318749 [details]
Patch

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

Don't you need to rebuild? We're loading bundle files in the actual test?

> PerformanceTests/Speedometer/resources/todomvc/architecture-examples/react-redux/src/reducers/todos.js:14
> +        ...state,
>          {
>            id: state.reduce((maxId, todo) => Math.max(todo.id, maxId), -1) + 1,
>            completed: false,
>            text: action.text
> -        },
> -        ...state
> +        }

Hm... I swear I tried this and didn't work :(

[Ryosuke Niwa]

Comment on attachment 318749 [details]
Patch

As I suspected, the problem is not fixed if I just apply the patch.

[Ryosuke Niwa]

Created attachment 318854 [details]
Fixes the bug

[Mathias Bynens]

Comment on attachment 318854 [details]
Fixes the bug

LGTM.

[Mathias Bynens]

(In reply to Ryosuke Niwa from comment #2)
> Comment on attachment 318749 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318749&action=review
> 
> Don't you need to rebuild? We're loading bundle files in the actual test?
> 
> > PerformanceTests/Speedometer/resources/todomvc/architecture-examples/react-redux/src/reducers/todos.js:14
> > +        ...state,
> >          {
> >            id: state.reduce((maxId, todo) => Math.max(todo.id, maxId), -1) + 1,
> >            completed: false,
> >            text: action.text
> > -        },
> > -        ...state
> > +        }
> 
> Hm... I swear I tried this and didn't work :(

Yeah, I forgot to include the re-built files — sorry about that.

The code change is correct though. I’ve upstreamed it here along with tests: https://github.com/reactjs/redux/pull/2575

[Joseph Pecoraro]

Comment on attachment 318854 [details]
Fixes the bug

rs=me

[Ryosuke Niwa]

There doesn't appears to be a measurable difference in score for this subtest (each number is the average of 10 iterations).

== Before ==
Safari
74.35
75.24
74.47
74.80

Chrome
88.20
88.38
88.29
89.68

Firefox
44.24
43.50
44.74
44.12

== After ==
Safari
75.19
74.88
74.22
74.57

Chrome
88.74
89.00
89.47
89.60

Firefox
41.47
44.09
43.42
44.12

[Ryosuke Niwa]

Comment on attachment 318854 [details]
Fixes the bug

Clearing flags on attachment: 318854

Committed r221105: <http://trac.webkit.org/changeset/221105>

[Ryosuke Niwa]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34044454>