Fix typeof for native classes with shared global scope #1173

[Schmidor]

Closes #1173

[gbrail]

This fix seems right to me. Anyone else have an opinion?

[rbri]

+ 1

[p-bakker]

Does it make sense that this change is only applied to the .getBase(Scriptable, String) variant and not to the Scriptable , int/Symbol variants?

[Schmidor]

Good point, I'm updating the other variants and checking

[p-bakker]

Just curious: your PR changes what gets send into the .has(key, start) method for the start parameter.

However, quickly looking I don't see any Scriptable.has(key, start) impl. in the Rhino codebase that actually uses the start parameter.

Any thoughts why the changes in your PR actually fix anything?

[Schmidor]

For the perceived bug a change in getBase(Scriptable start, String name) would be enough, as it was in the previous commit.
There it is the ImporterTopLevel implementation of has(String name, Scriptable start) which uses the start-Parameter within getPackageProperty.

But as you mentioned the other getBase variants, it is probably better to correct them to also to use start as parameter instead of the current obj, as it is documented in the ScriptableObject.has methods. Even it is currently not used.

[p-bakker]

There it is the ImporterTopLevel implementation of has(String name, Scriptable start)

Ah, missed that one

But as you mentioned the other getBase variants, it is probably better to correct them to also to use start as parameter

Yeah, wasn't suggesting the fix was wrong in any way, was just wondering why the changes would fix anything, cause it looked to me that the start param was never used, but as you pointed out it is. Passing the start object (as you do now) instead of the current object is indeed the correct way, according to EcmaScript

So now, with the other variants adjusted as well, this PR looks good to me

[gbrail]

This looks good to me too -- just testing for a quick merge. Thanks!

[rPraml]

Looks good to me also