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.

Sign up for GitHub

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

Jump to bottom

chore: DCHECK that events are only emitted on the UI thread #15873

Merged
merged 2 commits into from
Nov 30, 2018
Merged

chore: DCHECK that events are only emitted on the UI thread #15873

merged 2 commits into from
Nov 30, 2018

Conversation

nornagon
Copy link
Member

Description of Change

Emitting events on non-UI threads is a Very Bad Idea

Checklist

  • PR description included and stakeholders cc'd
  • PR title follows semantic commit guidelines

Release Notes

Notes: no-notes

@nornagon nornagon requested review from MarshallOfSound and a team November 29, 2018 00:54
@deepak1556
Copy link
Member

Just to get a bit more context on what triggered this, was there an improper usage of this api ?

@nornagon
Copy link
Member Author

@MarshallOfSound mentioned that there have been improper usages in the past, and I was worried that Emit might have been getting called from an IPC thread or IO thread in some cases and that might have been a cause of the test flakiness fixed by #15871. It wasn't, but if this was here it would have been easy to be sure about it :)

deepak1556
deepak1556 approved these changes Nov 29, 2018
View reviewed changes
Copy link
Member

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

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

Thanks for the clarification! In that case can you also add the DCHECK to EventEmitter::EmitWithEvent.

@zcbenz zcbenz merged commit 83d951d into master Nov 30, 2018
@release-clerk
Copy link

release-clerk bot commented Nov 30, 2018

No Release Notes

@zcbenz zcbenz deleted the dcheck-emit-on-ui branch November 30, 2018 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zcbenz zcbenz zcbenz approved these changes

@deepak1556 deepak1556 deepak1556 approved these changes

@MarshallOfSound MarshallOfSound Awaiting requested review from MarshallOfSound

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@nornagon @deepak1556 @zcbenz