Skip to content

Commit

Permalink
[Fabric] Let surface presenter manage start of surface
Browse files Browse the repository at this point in the history
  • Loading branch information
zhongwuzw committed Mar 29, 2019
1 parent 6345fcf commit e7cda3b
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 5 deletions.
6 changes: 5 additions & 1 deletion React/Fabric/RCTSurfacePresenter.mm
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ @implementation RCTSurfacePresenter {
std::shared_ptr<const ReactNativeConfig> _reactNativeConfig;
better::shared_mutex _observerListMutex;
NSMutableArray<id<RCTSurfacePresenterObserver>> *_observers;
BOOL _isFirstJavaScriptLoaded;
}

- (instancetype)initWithBridge:(RCTBridge *)bridge config:(std::shared_ptr<const ReactNativeConfig>)config
Expand All @@ -76,6 +77,8 @@ - (instancetype)initWithBridge:(RCTBridge *)bridge config:(std::shared_ptr<const
}

_observers = [NSMutableArray array];

_isFirstJavaScriptLoaded = YES;

[[NSNotificationCenter defaultCenter] addObserver:self
selector:@selector(handleBridgeWillReloadNotification:)
Expand Down Expand Up @@ -394,8 +397,9 @@ - (void)handleBridgeWillReloadNotification:(NSNotification *)notification
- (void)handleJavaScriptDidLoadNotification:(NSNotification *)notification
{
RCTBridge *bridge = notification.userInfo[@"bridge"];
if (bridge != _batchedBridge) {
if (bridge != _batchedBridge || _isFirstJavaScriptLoaded) {

This comment has been minimized.

Copy link
@shergin

shergin Mar 29, 2019

What's the logic behind this? What we are trying to fix and how?

This comment has been minimized.

Copy link
@shergin

This comment has been minimized.

Copy link
@zhongwuzw

zhongwuzw Mar 30, 2019

Author Owner

@shergin I removed startSurface in RCTFabricSurface e7cda3b#diff-7628f7b0ec2c3808e67e5e4a6b3a9bd6L71. To only let presenter to start surface. The reason I added _isFirstJavaScriptLoaded is the first run,bridge is equal to _batchedBridge, but we also need to trigger start surface operations.

_batchedBridge = bridge;
_isFirstJavaScriptLoaded = NO;

[self _startAllSurfaces];
}
Expand Down
5 changes: 1 addition & 4 deletions React/Fabric/Surface/RCTFabricSurface.mm
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,6 @@ - (instancetype)initWithSurfacePresenter:(RCTSurfacePresenter *)surfacePresenter
_touchHandler = [RCTSurfaceTouchHandler new];

_stage = RCTSurfaceStageSurfaceDidInitialize;

[_surfacePresenter registerSurface:self];
}

return self;
Expand All @@ -67,8 +65,7 @@ - (BOOL)start
if (![self _setStage:RCTSurfaceStageStarted]) {
return NO;
}

[_surfacePresenter startSurface:self];
[_surfacePresenter registerSurface:self];

This comment has been minimized.

Copy link
@shergin

shergin Mar 29, 2019

That might break some internal stuff, but I like this changes in general!

This comment has been minimized.

Copy link
@zhongwuzw

zhongwuzw Mar 30, 2019

Author Owner

Yeah, my opinion is register the surface only when it started.


return YES;
}
Expand Down

0 comments on commit e7cda3b

Please sign in to comment.