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

Remove useless message handlers #17335

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

Conversation

calixteman
Copy link
Contributor

These handlers are used to initiate the communication between the content thread and the worker and they aren't used once the document is loaded, hence we can remove them.

These handlers are used to initiate the communication between
the content thread and the worker and they aren't used once the
document is loaded, hence we can remove them.
Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

What do we actually save by doing this, since it adds a bunch of "complexity" to already somewhat complicated code?
Please note that reviewing this will require looking at a lot of code, and thinking about a bunch of edge-cases, to ensure that this is safe!

For one thing, if the default viewer is used with a workerPort I'm not convinced that this works.
Furthermore, the way that the new option is being set/used also means that it's not well covered by existing tests.

@calixteman
Copy link
Contributor Author

When trying to log what happened in the message handlers I saw some duplication which was due to these two handlers. In considering that in the Firefox pdf viewer case, they're useless (are they ?), here's an attempt to destroy them just because we don't need to consume any resources for them. I guess that in term of memory or perf they're likely insignificant, but I don't see any value in keeping them.
Maybe we could have only one handler for each side instead having two, which should be an overall simplification.
That said, we should really try to simplify things in order to be able to touch them without taking that much risks: I'm more scared by having some parts of the code we shouldn't touch compared to touch whatever file.
If you think it's too risky, don't worry to tell it and I'll close this PR.

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

Successfully merging this pull request may close these issues.

2 participants