Skip to content

Conversation

@NickGerleman
Copy link
Contributor

@NickGerleman NickGerleman commented Apr 15, 2020

Relates to #4363

We needed to remove some JS stubbing during the 0.62 upgrade. This exposed a require-time dependence on some form of StatusBarManager module existing, as we require both Android and iOS JS which will get both. We already have a UWP implementation but don't have one for Win32. Add a new module to share between platforms.

Some design points:

  1. The methods implemented are the intersection of Android and iOS methods, which have different specs. I have an item to try to share more code upstream.
  2. We do this here instead of Devmain since there shouldn't be any Office dependencies
  3. Our story around implementing internal shared native modules between platforms is still bad. We need to hook into OInstance and use legacy modules. This should hopefully get better with the Core DLL refactor?
  4. A real implementation would likely be different between Win32 and UWP, but for now both are a stub so we stick this in ReactWindowsCore.

While I was doing this I discovered some other issues, like a shared stub version of AppThemeModule which we don't need or want, and never expose. This is removed, and cleaned up. There was also a bit of cleanup for ReactWindowsCore filters, missing rvalue tokens.

Tested we can boot RNTester for UWP, and that the AppTheme RNTester page shows correct results.

Microsoft Reviewers: Open in CodeFlow

@NickGerleman NickGerleman requested a review from a team as a code owner April 15, 2020 12:21
// Licensed under the MIT License.

#include "StatusBarManagerModule.h"
#include <VersionHelpers.h>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nit: I can remove this header.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, please remove: it seems out of place.


In reply to: 408908658 [](ancestors = 408908658)

modules.push_back(std::make_unique<CxxNativeModule>(
m_innerInstance,
StatusBarManagerModule::Name,
[]() { return std::make_unique<StatusBarManagerModule>(); },
Copy link
Contributor Author

@NickGerleman NickGerleman Apr 15, 2020

Choose a reason for hiding this comment

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

Intentionally not noexcept. I know this has come up before.

When we call a throwing function (e.g. make_unique) in something noexcept, we need to generate the implicit catch and terminate. This paradoxically means declaring this as noexcept actually increases binary overhead when the caller isn't noexcept either.

I.e. sandwiching noexcept in thin layers between throwing call graphs doesn't help anything.

@vmoroz
Copy link
Member

vmoroz commented Apr 15, 2020

  DeviceInfoModule::name, [deviceInfo]() { return std::make_unique<DeviceInfoModule>(deviceInfo); }, messageQueue);

std::move?


Refers to: vnext/ReactUWP/Base/CoreNativeModules.cpp:81 in 88558b3. [](commit_id = 88558b3, deletion_comment = False)

});
}

AppTheme::~AppTheme() = default;
Copy link
Member

Choose a reason for hiding this comment

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

Why is it removed? There are some advantages keeping default destructors in .cpp file. Especially if the default constructor has to clean up a lot of variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you elaborate a bit?

Copy link
Member

@vmoroz vmoroz left a comment

Choose a reason for hiding this comment

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

:shipit:

@asklar asklar changed the title Implelent Shared StatusBarManagerModule and Do Module Cleanup Implement Shared StatusBarManagerModule and Do Module Cleanup Apr 16, 2020
@NickGerleman NickGerleman added the AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity) label Apr 17, 2020
@ghost
Copy link

ghost commented Apr 17, 2020

Hello @NickGerleman!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 609c18c into microsoft:master Apr 17, 2020
asklar pushed a commit to asklar/react-native-windows that referenced this pull request Apr 19, 2020
…oft#4609)

* Change files

* Fix build for ReactUwp/Win32 Playground

* Yarn format

* Remove unneeded module
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants