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

make sure, equivalentValues(Object) is used if available #1721

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rbri
Copy link
Collaborator

@rbri rbri commented Nov 7, 2024

The HTMLAllCollection implements some strange behavior - https://developer.mozilla.org/en-US/docs/Web/API/HTMLAllCollection#special_type_conversion_behavior. Of course we have to support this in HtmlUnit - see HtmlUnit/htmlunit#896

Luckily Rhino supports this kind of customized equals behavior by overriding the ScriptableObject#equivalentValues() method.

But there are two bugs

  • the equivalentValues() method is called if the lhs is null or undefined but NOT if the rhs is null or undefined (interpreter).
  • the optimized version works correct if the lhs is undefined or rhs is undefined but does not call the equivalentValues() method in case lhs/rhs is null

This PR fixes both cases and also adds test cases.

@rbri
Copy link
Collaborator Author

rbri commented Nov 11, 2024

any feedback for this? I need it for HtmlUnit and i like to know if there is a chance to have it in rhino or if i have to put it in my fork.

I did already some test and i did not see any negative performance impact.

@gbrail
Copy link
Collaborator

gbrail commented Nov 13, 2024

This makes sense to me -- I've been travelling a bit and haven't had a chance to test things myself yet. I'm also optimistic that a few of the optimizations like the ones in the bytecode here that you had to undo might be less necessary as we can keep working on dynamic linking -- I'll have some PRs for that coming up in the future. For now I'll check this out in detail later in the week.

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.

2 participants