Skip to content

Use remoteId as const value#1435

Merged
sharath2727 merged 1 commit intoWNP_LRPfrom
user/vemanchal/remoteIdhandling
Sep 20, 2021
Merged

Use remoteId as const value#1435
sharath2727 merged 1 commit intoWNP_LRPfrom
user/vemanchal/remoteIdhandling

Conversation

@sharath2727
Copy link
Copy Markdown
Contributor

Pass remoteId as const value to CreateChannelAsync. As the value is const we don't need to pass by reference.

@ghost ghost added the needs-triage label Sep 17, 2021
@sharath2727 sharath2727 added the area-Notifications Toast notification, badges, Live Tiles, push notifications label Sep 17, 2021
@DrusTheAxe
Copy link
Copy Markdown
Member

Works either way. Efficiency is quibbling -- GUID=16 bytes so pass-by-reference is a pointer plus dereference, pass-by-value may be the same or more efficient with registers or whatnot (depends how the compiler handles larger structures, calling convention, etc)

@jonwis is there general guidance these days on parameters as const GUID vs const GUID& ?

@loneursid
Copy link
Copy Markdown
Contributor

Works either way. Efficiency is quibbling -- GUID=16 bytes so pass-by-reference is a pointer plus dereference, pass-by-value may be the same or more efficient with registers or whatnot (depends how the compiler handles larger structures, calling convention, etc)

@jonwis is there general guidance these days on parameters as const GUID vs const GUID& ?

The reason for this change isn't about performance though, it's about fixing an async big. The function is async and makes use of the GUID parameter. If the GUID is passed by reference, then we need to take a copy inside of the function to ensure the object lifetime matches the function's. Here letting the compiler make copy the GUID seems the right thing to do (less code).

static void UnregisterAllActivators();

static winrt::Windows::Foundation::IAsyncOperationWithProgress<winrt::Microsoft::Windows::PushNotifications::PushNotificationCreateChannelResult, winrt::Microsoft::Windows::PushNotifications::PushNotificationCreateChannelStatus> CreateChannelAsync(winrt::guid const& remoteId);
static winrt::Windows::Foundation::IAsyncOperationWithProgress<winrt::Microsoft::Windows::PushNotifications::PushNotificationCreateChannelResult, winrt::Microsoft::Windows::PushNotifications::PushNotificationCreateChannelStatus> CreateChannelAsync(winrt::guid const remoteId);
Copy link
Copy Markdown
Member

@hulumane hulumane Sep 17, 2021

Choose a reason for hiding this comment

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

I'm curious why we auto generated guid& instead of guid in the header file from the idl. Is that true and does this impact the projection in any negative way?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

@kennykerr kennykerr Sep 20, 2021

Choose a reason for hiding this comment

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

C++/WinRT passes all structs by reference as this is more efficient in general and allows the optimizer to figure out the best way to minimize the code gen. This does not affect the ABI. The only time you would need to pass these by value is when implementing a coroutine.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah kenny. Thanks for your response. The above API is a coroutine and the value of remoteId is not held on the stack if the API is suspended. So that is the reason I had to convert this to const value.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@kennykerr Should we have c++/winrt generate the pass by value argument automatically for co-routines rather than pass by ref knowing some of the limitations here with argument lifetime?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We could, although its probably more reliable to build with code analysis enabled and then you end up with a compiler warning/error:

winrt::Windows::Foundation::IAsyncOperation<winrt::guid> Async(winrt::guid const& value)
{
    co_await winrt::resume_background();
    co_return value;
}
main.cpp(5): warning C26811: Lifetime of the memory referenced by parameter ''value'' might end by the time the coroutine is resumed (lifetime.1).

image

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is good to learn. However, I am wondering which is the right approach here? Enabling code analysis or make the C++/WinRT generate differently for coroutines?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

C++/WinRT has no idea how you are going to implement the method - you may not use coroutines at all - and once the code is generated all control is lost. However, the C++ compiler can always validate that the code is safe regardless of how the code evolves over time. Therefore, having the compiler check the code's correctness seems far more useful.

@sharath2727 sharath2727 merged commit d170ce3 into WNP_LRP Sep 20, 2021
@sharath2727 sharath2727 deleted the user/vemanchal/remoteIdhandling branch September 20, 2021 18:02
loneursid added a commit that referenced this pull request Oct 13, 2021
* Original skeleton

* Corrected mismatches against PowerNotifications change

* Correcting few nits

* Added constants usage, std::call_once and improved abstraction

* Added StartupTask exe

* Fixed some bugs

* Mostly renaming and cleaning

* Removed timer externals and let winmain to own Platform lifetime

* Correction -> factory should own the platform

* Addressed lifetime crashes and other issues

* Improved timer/event management

* Addressed feedback

* Fixed compilation issues in other flavor+archs and added minor comments

* Added a unit test (more coming!)

* Test enhancements

* Addressed comments

* Moved timer/event logic to a platform component

* Install Frameworkudk for LRP (#1271)

* Introducing unpackaged foreground activation for PushNotifications (#1336)

* Cleaned original code

* Added IWpnNotificationSink COM interface to LRP MSIX

* Just stuff for testing. Needs to be undone

* Still working on it

* Com proxy objects not being stored correctly

* Receiving Data from LRP and changed sink container

* Foreground activation working

* Fix first round of nits

* Still working on adding nits

* Fix following FI of main

* Addressing nits

* Use com_array instead of byte*

* Update dev/PushNotifications/PushNotificationDummyDeferral.h

Co-authored-by: Daniel Ayala <14967941+danielayala94@users.noreply.github.com>

Co-authored-by: Daniel Ayala <joayalag@microsoft.com>
Co-authored-by: Eric Langlois <erlangl@microsoft.com>
Co-authored-by: Daniel Ayala <14967941+danielayala94@users.noreply.github.com>

* Not everything should be renamed Runtime

* Ensuring the LRP msix gets built after the framework (#1356)

Co-authored-by: Eric Langlois <erlangl@microsoft.com>

* Bringing build management settings for release/arm64 in line with debug/arm64

* Fixing arm64 build

* Addressed comments

* Using winrt constructs on NotificationsLongRunningPlatformImpl :)

* Channel Request for unpackaged applications (#1290)

* Channel requests for Packaged and Unpackaged applications

* Fix comments

* Update windowsappsdk_dll to locate PushNotificationsLongRunningTask.ProxyStub headers

* Address minor comments

* Fix channel.Close API to handle channels when requested through frameworkudk

* Address comments and introduce GetAppIdentifier in LRP

* Addressing nits

* Fixing a few more nits

* Update externs.h

* Store <processname,appidentifier> pair in windows storage localsettings

* Fix minor nits

* Remove helpers.h references from PushNotificationsDemoApp

* Remove unused APIs

* Using WIL type for CoInitializeEx instead of working too hard

Co-authored-by: Eric Langlois <erlangl@microsoft.com>
Co-authored-by: Paul Purifoy <purifoypaul@microsoft.com>
Co-authored-by: Daniel Ayala <14967941+danielayala94@users.noreply.github.com>

* Notification delivery + Protocol Activation (#1358)

* Protocol Activation skeleton

* Further impl, mostly on the LRP

* Integration with Foreground logic and some optimizations

* Added calls to the LRP from SDK, and LRP improvements

* Fixed some issues

* Improved payload deserializing

* Fixed merging goofs

* Addressed feedback + few optimizations

* Further improvements and added/removed comments

* Addressed build errors after merging

* Nit: Removed comment

Co-authored-by: Daniel Ayala
Co-authored-by: Eric Langlois

* Unpackaged notification unit tests (#1340)

* Adding initial unpackaged test series

* This function now fails silently

* This is not expected to work for unpackaged apps

Co-authored-by: Eric Langlois <erlangl@microsoft.com>

* Ensure errors get logged with telemetry

* Workaround UDK include issue (#1364)

* Workaround UDK include issue

* just duplicating files seem a better approach

* Restoring vcproje files

* PushNotificationsRT.h was interfering with package creation

Co-authored-by: Eric Langlois <erlangl@microsoft.com>

* Some nits

* Addressed comments

* Fixed mistake - Removed comments

* Add LRP binaries to CopyFilestoStagingDir

* Addressed comments

* Merge main to LRP

* Telemetry Update for Unpackaged Apps (#1385)

* Adding support for unpackaged apps

* Using inline statics to cache data

* Code cleanup

* Code cleanup

* Simplifying rethrow code

* Addressing feedback

* Distinguishing between legacy and modern impl

* caching legacy status to it is also available when the call fails

* Adding const to better express the intent

* Code cleanup

* Code cleanup

* PR feedback

Co-authored-by: Eric Langlois <erlangl@microsoft.com>

* Update to latest UDK and undo UDK hack (#1420)

Co-authored-by: Eric Langlois <erlangl@microsoft.com>

* Fixing WNP_LRP tests  (#1401)

* Working on tests

* Call ReigsterFullTrust in RegisterLongRunningActivator

* Removed g_isPackaged with IsActivatorSupported

* Addressing nits

* Protocol Activation bug fixes (#1406)

This PR fixes a couple of issues:

- Truncated payloads delivered to the app: the Push payload is now processed correctly to comply with AppLifecycle command line expectations (i.e. truncation happened at the first space character - other activation types are already escaping such characters, now we do it too. Then, the payload is unescaped at GetRawNotificationEventArgs.h).
- Payloads with special characters crashed the app: Special characters weren't playing okay with wstring->string conversion at PushNotificationReceivedEventArgs, because WideCharToMultiByte was erroring out with ERROR_INSUFFICIENT_BUFFER.

* Use remoteId as const value (#1435)

* Upgrade LRP to latest UDK (#1459)

* Upgrade LRP to latest UDK

* Missed a few checks in the vcproj

Co-authored-by: Eric Langlois <erlangl@microsoft.com>

* fix bad merge

* Update CopyFilesToStagingDir.ps1

* Remove PushNotifications from outer layer of JSON

* Adding source link to projects not in main yet (#1515)

Co-authored-by: Eric Langlois <erlangl@microsoft.com>

* Support foregroundtask registration for packaged applications (#1521)

* Support foregroundtask registration for packaged applications

* Address comments

* Address comments2

* Introduce PushNotificationsUtility.h/cpp

* Address comments

* Mark PushNotificationUtility.h file to be shared across the PushNotifications directory

* Add try/catch around onRawNotificationRecveived

* Set foregroundhandled to true by default

* Address comments for getappusermodelId API

* Address nits

* Adress one more nit

Co-authored-by: Venkata Sharath Chandra Manchala <vemancha@microsoft.com>

* Fix for regression in protocol activation in AppLifecycle (#1558)

* Hack to address AppLifecycle regression introduced in main

* Code cleanup

* Code cleanup

* Addressing PR feedback

* Adding bare bone unit test to avoid future regression

Co-authored-by: Eric Langlois <erlangl@microsoft.com>

* LRP E2E Scenario fixes (#1543)

* LRP fixes

* In progress

* Revert "In progress"

This reverts commit d5d021a.

* Add fixes to E2E testing

* Address nits

* Added nits from other PR and fixed PushNotificationUtiilty

* Use string converter with payload length

* Update APITests.cpp

* Include AppLifecycle fix for PushArgs

* Revert "Include AppLifecycle fix for PushArgs"

This reverts commit 33994da.

* Addressing nits

* Add Verify.h to demo app for helper function

* Added TAEF dependencies to Demo/TestApp

Co-authored-by: Daniel Ayala <14967941+danielayala94@users.noreply.github.com>

* Updating LRP to use latest FrameworkUDK (#1587)

Co-authored-by: Eric Langlois <erlangl@microsoft.com>

Co-authored-by: Daniel Ayala <joayalag@microsoft.com>
Co-authored-by: Sharath Manchala <10109130+sharath2727@users.noreply.github.com>
Co-authored-by: Eric Langlois <erlangl@microsoft.com>
Co-authored-by: Paul Purifoy <33183370+pmpurifoy@users.noreply.github.com>
Co-authored-by: eric langlois <email@ericlanglois.com>
Co-authored-by: Paul Purifoy <purifoypaul@microsoft.com>
Co-authored-by: Venkata Sharath Chandra Manchala <vemancha@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-Notifications Toast notification, badges, Live Tiles, push notifications needs-triage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants