Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix for #64 #94

Closed
wants to merge 1 commit into from
Closed

fix for #64 #94

wants to merge 1 commit into from

Conversation

doomb0t
Copy link

@doomb0t doomb0t commented Jun 29, 2015

This is my attempt to implement the optimizations mentioned in the issue. I assumed it was also safe to remove the 'in' check from 'getPositiveSpacing' as well because if it is null then will not be >= 0 I would be happy to make any requested changes if this isn't quite right.
Thanks!

return node.style[key];
}

return node.style[key] || node.style[type + key + suffix];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not safe :(

var node = { style: { marginTop: 0, margin: 10 } };
// before
if ('marginTop' in node.style) {
  return node.style['marginTop'];
}
// returns 0
// after
return node.style['marginTop'] || node.style['margin'];
// returns 10

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A safe way to do it is by writing node.style['marginTop'] !== undefined. But, in order to make sure that it has performance improvement, we need to benchmark it.

Can you use the code that generates a random test to generate a big random tree and then time the layout algorithm before and after? Otherwise we can't know if this is a performance optimization or not.

@doomb0t
Copy link
Author

doomb0t commented Jun 29, 2015

Hey,
I will gladly make this safer then time the tests and give you the output. I should have thought out my changes a bit more thoroughly, sorry about that!

@doomb0t
Copy link
Author

doomb0t commented Jun 29, 2015

Also would making it
return node.style['marginTop'] || node.style['margin'] || 0;
work if I excluded the return 0 on exiting the loop? Otherwise I can add check for !== undefined.
Also for the tests do I use the RunLayoutRandomTests.html in chrome to do this?

@doomb0t
Copy link
Author

doomb0t commented Jun 30, 2015

Hey so after working on this for a while yesterday I found that there wasn't much of a performance improvement on my machine, and that the changes caused a slight increase in the number of failures on the layout tests. Thanks for taking a look at what I had. I will close the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants