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 leaks after calling wasm drop #7038

Merged
merged 19 commits into from
Jun 20, 2023

Conversation

farmaazon
Copy link
Contributor

@farmaazon farmaazon commented Jun 15, 2023

Pull Request Description

  1. Fixed leaks of Scene. We were making several cycles of Rc references.
  2. Fix removing event listeners: the remove listener option should take same options as those passed in addEventListener.

This should finally fix #6505

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • [ ] Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@farmaazon farmaazon added the CI: No changelog needed Do not require a changelog entry for this PR. label Jun 15, 2023
@farmaazon farmaazon self-assigned this Jun 15, 2023
@farmaazon farmaazon marked this pull request as ready for review June 16, 2023 11:10
options
//
// 2. We want to prevent default action on wheel events, thus listener cannot be passive.
web::EventListenerHandleOptions::new().passive()
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be not_passive()?

// We want to prevent default action on wheel events, thus listener cannot be passive.
options.passive(false);
options
enso_web::EventListenerHandleOptions::new().passive()
Copy link
Contributor

Choose a reason for hiding this comment

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

not_passive?

Comment on lines 757 to 766
pub fn passive(mut self) -> Self {
self.passive = Some(true);
self
}

pub fn not_passive(mut self) -> Self {
self.passive = Some(false);
self
}

Copy link
Member

Choose a reason for hiding this comment

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

Please add here docs. It's not obvious what these do after reading the name if someone (like me) do not remember what the JS side does.

Comment on lines 855 to 856


Copy link
Member

Choose a reason for hiding this comment

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

too big spacing

@kazcw
Copy link
Contributor

kazcw commented Jun 20, 2023

QA: 🟢

@kazcw kazcw added the CI: Ready to merge This PR is eligible for automatic merge label Jun 20, 2023
@mergify mergify bot merged commit 31956aa into develop Jun 20, 2023
@mergify mergify bot deleted the wip/procrat/no-rendering-after-drop-6505 branch June 20, 2023 16:55
@kazcw kazcw mentioned this pull request Jun 20, 2023
5 tasks
mergify bot pushed a commit that referenced this pull request Jun 22, 2023
Remove a leak detector added in #7038, that has a large (~10s) impact on startup performance. I missed the performance impact during QA, but noticed it after merge.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using wasm drop method results in warnings flood
4 participants