Modify Analytics on desktop to use a verified DLL on Windows (although it is currently still a stub). #1731
+1,342
−220
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
refactor: Remove stale comments and TODOs from Windows Analytics Cleans up src/analytics_desktop.cc by: - Removing outdated TODO comments related to header verification and type definitions (Parameter/Item) that have since been resolved. - Deleting informational comments that were no longer accurate after refactoring (e.g., comments about placeholder types, forward declarations for removed types). - Removing an empty anonymous namespace that previously held an internal type alias. This commit improves the readability and maintainability of the Windows Analytics implementation by removing distracting or misleading commentary. The TODO regarding the precise Future creation mechanism in stubbed functions remains, as it requires further investigation of Firebase internal APIs.
Replaces placeholder comments with actual calls to LogError() and LogWarning() throughout the Windows Analytics implementation in src/analytics_desktop.cc. Key changes: - Added #include "firebase/log.h". - Updated error handling in LogEvent, ConvertParametersToGAParams, and SetUserProperty to use LogError() for invalid arguments or internal failures. - Updated warnings for unexpected data types or structures in event parameters to use LogWarning(). - Added LogWarning() calls to all stubbed functions (e.g., SetConsent, InitiateOnDeviceConversionMeasurement*, SetSessionTimeoutDuration) to inform developers that these operations are not supported and have no effect on the Windows platform. This change enhances the robustness and diagnosability of the Windows Analytics module by providing clear feedback through the Firebase logging system.
Based on explicit feedback, this commit fundamentally changes how
Parameter values of type Map and Vector are processed in the Windows
Analytics C++ SDK (in src/analytics_desktop.cc):
1. **Parameter values of type Vector (`param.value.is_vector()`):**
- These are now treated as UNSUPPORTED for top-level event parameters
on Desktop.
- An error is logged, and the parameter is skipped.
- The previous logic that interpreted a vector of maps as an item array
has been removed from ConvertParametersToGAParams.
2. **Parameter values of type Map (`param.value.is_map()`):**
- This case is now explicitly handled.
- The key-value pairs of the input map are transformed into a
GoogleAnalytics_ItemVector.
- Each entry (key, value_variant) in your map becomes a distinct
GoogleAnalytics_Item within this ItemVector.
- Each such GoogleAnalytics_Item stores the original map key under a
property named "name", and the original map value (which must be a
primitive type) under a typed property (e.g., "int_value",
"double_value", "string_value").
- This resulting ItemVector is then added to the event's parameters
using the original Parameter's name.
3. **Comments:**
- Code comments within ConvertParametersToGAParams have been updated
to reflect this new processing logic.
This change aligns the behavior with specific design requirements for how
map and vector type parameters should be translated to the underlying
Windows C API for Google Analytics.
This commit incorporates detailed review feedback to align the
Windows Analytics C++ implementation (src/analytics_desktop.cc)
with common Firebase C++ SDK patterns for managing Futures and
including headers.
Key changes:
1. **Future Management for Stubbed APIs:**
- Replaced the previous Future creation mechanism (MakeFuture) for
stubbed functions (GetAnalyticsInstanceId, GetSessionId, and their
LastResult counterparts) with the standard FutureData pattern.
- Added a static FutureData instance (g_future_data), initialized in
firebase::analytics::Initialize() and cleaned up in Terminate().
- Defined an internal AnalyticsFn enum for Future function tracking.
- Stubbed Future-returning methods now use g_future_data to create
and complete Futures.
- As per feedback, these Futures are completed with error code 0
and a null error message. A LogWarning is now used to inform
developers that the called API is not supported on Desktop.
- Added checks for g_future_data initialization in these methods.
2. **Include Path Updates:**
- Corrected #include paths for common Firebase headers (app.h,
variant.h, future.h, log.h) to use full module-prefixed paths
(e.g., "app/src/include/firebase/app.h") instead of direct
"firebase/" paths.
- Removed a stale include comment.
These changes improve the consistency of the Windows Analytics stubs
with the rest of the Firebase C++ SDK.
refactor: Address further review comments for Windows Analytics
This commit incorporates additional specific feedback on the Windows
Analytics C++ implementation (src/analytics_desktop.cc):
1. **Comment Cleanup:**
- Removed a commented-out unused constant and its description.
- Removed a non-functional comment next to a namespace declaration.
- Clarified and shortened the comment in the `is_vector` handling
block within `ConvertParametersToGAParams`.
2. **Use Common `AnalyticsFn` Enum:**
- Included `analytics/src/common/analytics_common.h`.
- Removed the local definition of `AnalyticsFn` enum, now relying on
the common definition (assumed to be in
`firebase::analytics::internal`).
3. **Corrected Map Parameter to `GoogleAnalytics_Item` Conversion:**
- Critically, fixed the logic in `ConvertParametersToGAParams` for when
a `Parameter::value` is a map.
- Previously, each key-value pair from the input map was incorrectly
creating a `GoogleAnalytics_Item` with fixed property names like
"name" and "int_value".
- The logic now correctly ensures that each key-value pair from your
input map becomes a direct property in the `GoogleAnalytics_Item`,
using the original map's key as the key for the property in the
`GoogleAnalytics_Item`. For example, an entry `{"user_key": 123}`
in the input map now results in a property `{"user_key": 123}`
within the `GoogleAnalytics_Item`.
This commit incorporates the latest round of specific feedback on the
Windows Analytics C++ implementation (src/analytics_desktop.cc):
1. **Comment Cleanup:**
- Removed a large commented-out block that previously contained a
local `AnalyticsFn` enum definition.
- Removed a comment in `Initialize()` related to marking the `app`
parameter as unused.
- Removed an outdated comment in `Initialize()` that described the
function as a placeholder.
- Removed a type comment from the `ConvertParametersToGAParams()`
function signature.
- Replaced a lengthy comment in `LogEvent()` regarding C API object
lifecycle with a more concise one.
2. **Refined Map Parameter Processing Logic:**
- In `ConvertParametersToGAParams`, when handling a `Parameter` whose
value is a map, the logic for creating `GoogleAnalytics_Item`
objects from the map's entries has been clarified.
- A local boolean flag (`successfully_set_property`) is used for each
map entry to track if a value was successfully set in the
corresponding `GoogleAnalytics_Item`.
- A `GoogleAnalytics_Item` is only added to the `GoogleAnalytics_ItemVector`
if a property was successfully set. Otherwise, the item is destroyed.
This prevents empty or partially formed items (e.g., from map entries
with unsupported value types) from being included in the ItemVector.
As per review feedback, this commit moves the Windows Analytics C++ implementation file from src/analytics_desktop.cc to analytics/src/analytics_desktop.cc. The content of the file remains the same as the previous version, incorporating all prior review feedback. I attempted to run the code formatter script after the move, but the script failed due to an internal error. Formatting was not applied.
Renamed the parameter `library_path` to `library_filename` in the `VerifyAndLoadAnalyticsLibrary` function (declaration in `analytics_windows.h` and definition and usage in `analytics_windows.cc`). This change improves clarity, as the parameter is expected to be only the DLL's filename, not a full path. The full path for file operations is constructed internally within the function using _wpgmptr and this filename.
If no hash is passed in, load the DLL indiscriminantly.
github-actions
bot
added
the
tests: in-progress
✅ Integration test succeeded!Requested by @jonsimantov on commit 4cc9e6c |
github-actions
bot
added
the
tests: succeeded
firebase-workflow-trigger
bot
removed
the
tests: in-progress
jonsimantov
added
the
tests-requested: quick
github-actions
bot
added
tests: in-progress
firebase-workflow-trigger
bot
removed
the
tests: in-progress
jonsimantov
changed the title
jonsimantov
added
the
tests-requested: quick
github-actions
bot
added
tests: in-progress
firebase-workflow-trigger
bot
removed
the
tests: in-progress
I changed the code in `analytics_desktop.cc` to use `sizeof(FirebaseAnalytics_WindowsDllHash)` when constructing the vector for the DLL hash. This replaces the previously hardcoded size of 32, making the code safer and more maintainable.
I changed a LogVerbose call to LogDebug in `VerifyAndLoadAnalyticsLibrary` within `analytics/src/analytics_windows.cc`. The log message for a hash size mismatch when iterating through allowed hashes is now logged at Debug level instead of Verbose.
jonsimantov
added
tests-requested: quick
github-actions
bot
added
tests: in-progress
firebase-workflow-trigger
bot
removed
the
tests: in-progress
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR swaps out the Analytics stub on desktop for a version that wraps the GA Windows DLL.
It's designed to work dynamically, so if it can load the DLL and verify its SHA256 hash, it uses it, otherwise it transparently operates in stub mode as always. On other desktop platforms it defaults back to stub mode.
The included DLL is itself a stub, so this change is effectively a no-op (aside from logging) at the moment.
Testing
Integration tests have been manually run on Windows with and without the DLL, and confirmed to remain stubs on other desktop platforms.
Type of Change
Place an
xthe applicable box:Notes
Release Notessection ofrelease_build_files/readme.md.