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

avm1: Remove incorrect properties from display object prototypes (fix #768) #6921

Merged
merged 6 commits into from
May 10, 2022

Conversation

Herschel
Copy link
Member

@Herschel Herschel commented May 8, 2022

  • Remove _root, _global, and _parent from MovieClip.prototype and others.
    • Instead, these are magic properties now handled in StageObject::resolve_path_property.
    • Clean up StageObject::get_local_stored a bit.
  • Remove toString from MovieClip.prototype and others. StageObjects instead naturally use their path when coerced to string.
  • At this point, avm1::globals::display_object is mostly empty, so remove it. Move the items up one level into globals.
  • Remove incorrect Video.getDepth declaration.

Fixes #768.

@Herschel Herschel requested a review from Toad06 May 8, 2022 19:53
Herschel added 5 commits May 8, 2022 13:33
…rs#768)

Remove `_root`, `_parent` and `_global` from `MovieClip.prototype`.
Instead, these are "magic" properties similar to `_x` and `_y`.
Add `StageObject::resolve_path_property` to handle these, alongside
the `_levelN` property.

Fixes ruffle-rs#768.
 * Avoid an extraneous check to `ScriptObject::has_own_property`.
 * Avoid magic property branches if property name does not start
   with `_`.
`MovieClip` and others do not have a `toString`; instead, stage
objects are special cased to return their path when coerced to
string.
This module is now mostly empty, so move the items up to `globals`.
`getDepth` was the only shared method, so declare this property
inline in each display object type. `Video` was also incorrectly
declaring `getDepth`.
}

// 2) Path properties such as `_root`, `_parent`, `_levelN` (obeys case sensitivity)
let magic_property = name.starts_with(b'_');
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this check if for performance only, since it doesn't affect the lookup logic at all.

Copy link
Member Author

@Herschel Herschel May 9, 2022

Choose a reason for hiding this comment

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

Yes, seemed a little wasteful to do extra hashes and string compares if we can tell quickly that it isn't a _ property. Haven't measured if it makes a noticeable difference though.

"[type Function]".into()
} else {
"[type Object]".into()
if let Some(object) = object.as_display_object() {
Copy link
Contributor

@relrelb relrelb May 9, 2022

Choose a reason for hiding this comment

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

That's another argument for #6548 (comment).

@Toad06
Copy link
Member

Toad06 commented May 9, 2022

LGTM, I haven't found any regression in my testings. 👍

Checking the other related issues:

@Herschel Herschel merged commit 6547fcd into ruffle-rs:master May 10, 2022
@Herschel Herschel deleted the issue-768 branch May 10, 2022 00:39
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.

_root, _parent, _global are not members of MovieClip.prototype
3 participants