Skip to content

Commit

Permalink
Fabric: Making EventEmitter::setEnabled additive
Browse files Browse the repository at this point in the history
Summary: In the previous approach, when event emitter got disabled for split second, we could lose the EventTarget because JS GC can collect it before we re-enable this. Now we "over-enable" this first, and "under-disable" later.

Reviewed By: mdvacca

Differential Revision: D12990112

fbshipit-source-id: 4e3c0c0e05f03509ec72ca570f59ce16597353f0
  • Loading branch information
shergin authored and facebook-github-bot committed Nov 9, 2018
1 parent 803e993 commit d2408dd
Show file tree
Hide file tree
Showing 3 changed files with 30 additions and 18 deletions.
23 changes: 15 additions & 8 deletions ReactCommon/fabric/events/EventEmitter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -63,13 +63,24 @@ void EventEmitter::dispatchEvent(
priority);
}

void EventEmitter::setEnabled(bool enabled) const {
bool alreadyEnabled = eventTarget_ != nullptr;
if (enabled == alreadyEnabled) {
void EventEmitter::enable() const {
enableCounter_++;
toggleEventTargetOwnership_();
}

void EventEmitter::disable() const {
enableCounter_--;
toggleEventTargetOwnership_();
}

void EventEmitter::toggleEventTargetOwnership_() const {
bool shouldBeRetained = enableCounter_ > 0;
bool alreadyBeRetained = eventTarget_ != nullptr;
if (shouldBeRetained == alreadyBeRetained) {
return;
}

if (enabled) {
if (shouldBeRetained) {
eventTarget_ = weakEventTarget_.lock();
weakEventTarget_.reset();
} else {
Expand All @@ -78,9 +89,5 @@ void EventEmitter::setEnabled(bool enabled) const {
}
}

bool EventEmitter::getEnabled() const {
return eventTarget_ != nullptr;
}

} // namespace react
} // namespace facebook
17 changes: 11 additions & 6 deletions ReactCommon/fabric/events/EventEmitter.h
Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,15 @@ class EventEmitter {
virtual ~EventEmitter() = default;

/*
* Indicates that an event can be delivered to `eventTarget`.
* Callsite must acquire `DispatchMutex` to access those methods.
* The `setEnabled` operation is not guaranteed: sometimes `EventEmitter`
* can be re-enabled after disabling, sometimes not.
* `DispatchMutex` must be acquired before calling.
* Enables/disables event emitter.
* Enabled event emitter retains a pointer to `eventTarget` strongly (as
* `std::shared_ptr`) whereas disabled one weakly (as `std::weak_ptr`).
* The enable state is additive; a number of `enable` calls should be equal to
* a number of `disable` calls to release the event target.
*/
void setEnabled(bool enabled) const;
bool getEnabled() const;
void enable() const;
void disable() const;

protected:
#ifdef ANDROID
Expand All @@ -78,10 +80,13 @@ class EventEmitter {
const EventPriority &priority = EventPriority::AsynchronousBatched) const;

private:
void toggleEventTargetOwnership_() const;

mutable SharedEventTarget eventTarget_;
mutable WeakEventTarget weakEventTarget_;
Tag tag_;
WeakEventDispatcher eventDispatcher_;
mutable int enableCounter_{0};
};

} // namespace react
Expand Down
8 changes: 4 additions & 4 deletions ReactCommon/fabric/uimanager/ShadowTree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -181,14 +181,14 @@ void ShadowTree::toggleEventEmitters(
std::lock_guard<std::recursive_mutex> lock(EventEmitter::DispatchMutex());

for (const auto &mutation : mutations) {
if (mutation.type == ShadowViewMutation::Delete) {
mutation.oldChildShadowView.eventEmitter->setEnabled(false);
if (mutation.type == ShadowViewMutation::Create) {
mutation.newChildShadowView.eventEmitter->enable();
}
}

for (const auto &mutation : mutations) {
if (mutation.type == ShadowViewMutation::Create) {
mutation.newChildShadowView.eventEmitter->setEnabled(true);
if (mutation.type == ShadowViewMutation::Delete) {
mutation.oldChildShadowView.eventEmitter->disable();
}
}
}
Expand Down

0 comments on commit d2408dd

Please sign in to comment.