You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Modify Analytics on desktop to use a verified DLL on Windows (although it is currently still a stub). (#1731)
* Here's the updated commit message:
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.
* feat: Integrate Firebase logging for Windows Analytics
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.
* refactor: Update event parameter handling for map and vector types
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.
* refactor: Align Windows Analytics stubs with SDK patterns
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.
* Here's the plan:
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`.
* refactor: Address final review comments for Windows Analytics
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.
* chore: Relocate analytics_desktop.cc to analytics/src/
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.
* Clean up generated code.
* Update desktop code to fix issues.
* Copy Analytics DLL from SDK directory.
* Add wide string version of SetAnalyticsLibraryPath and export
"publicly".
* Update log messages.
* Format code.
* Update paths and remove old stub.
* Update some nomenclature.
* Add a SHA256 hash to verify on load.
* Fix build error.
* Add hash constant.
* Update layout.
* refactor: Rename library_path to library_filename for clarity
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.
* Tweak errors and format code.
* Revert to stubs on load fail.
* Remove big comment.
* Don't include analytics_windows.h except on Windows.
If no hash is passed in, load the DLL indiscriminantly.
* Add copyright notice.
* Fixed log include.
* Fix comment.
* refactor: Extract GetExecutablePath helper in analytics_windows.cc
I've refactored `VerifyAndLoadAnalyticsLibrary` in `analytics_windows.cc`
to improve readability and modularity by extracting the logic for
obtaining the executable's full path into a new static helper
function `GetExecutablePath()`.
- The new `GetExecutablePath()` function encapsulates the prioritized
use of `_get_wpgmptr()`, fallback to `_get_pgmptr()`, and the
necessary `MultiByteToWideChar` conversion, along with all
associated error handling and logging. It returns `std::wstring`.
- `VerifyAndLoadAnalyticsLibrary` now calls `GetExecutablePath()` and
checks its return value before proceeding with path manipulation.
- This change makes `VerifyAndLoadAnalyticsLibrary` shorter and easier
to understand.
All previous security enhancements and fixes are maintained.
* refactor: Update log.h include path and extract GetExecutablePath helper
This commit includes two main changes:
1. Updated the include path for `log.h` in `analytics_windows.cc` and
`analytics_desktop.cc` from `app/src/include/firebase/log.h` to
`app/src/log.h` for consistency.
2. Refactored `VerifyAndLoadAnalyticsLibrary` in `analytics_windows.cc`
to improve readability and modularity by extracting the logic for
obtaining the executable's full path into a new static helper
function `GetExecutablePath()`.
- The new `GetExecutablePath()` function encapsulates the prioritized
use of `_get_wpgmptr()`, fallback to `_get_pgmptr()`, and the
necessary `MultiByteToWideChar` conversion, along with all
associated error handling and logging. It returns `std::wstring`.
- `VerifyAndLoadAnalyticsLibrary` now calls `GetExecutablePath()` and
checks its return value before proceeding with path manipulation.
- This change makes `VerifyAndLoadAnalyticsLibrary` shorter and easier
to understand.
All previous security enhancements and fixes are maintained.
* Updated formatting and errors.
* Factor out log tag.
* Format code.
* Fix syntax error.
* Refactor GetExecutablePath with two-tier buffer strategy
This commit updates the GetExecutablePath function in
analytics_windows.cc to use a two-tier buffer sizing strategy
as per revised requirements.
The function first attempts to retrieve the executable path using
a buffer of MAX_PATH + 1 characters. If this fails due to an
insufficient buffer (ERROR_INSUFFICIENT_BUFFER), it makes a
second attempt with a larger buffer of 65536 characters.
This approach replaces the previous iterative buffer resizing
logic. Additionally, explicit try/catch blocks for std::bad_alloc
have been removed in accordance with codebase guidelines. Error
handling for GetModuleFileNameW API calls is maintained.
* Clean up logic for getting executable path.
* Format code.
* Fix nesting.
* Fix nesting again.
* Use LoadLibraryW because LoadLibraryExW is failing.
* Fix build error.
* Add a warning.
* Add an info message.
* Fix error.
* Change message to debug.
* Move functions.
* Delete DLL file with stubs.
* Silence an unneeded error.
* Revert "Delete DLL file with stubs."
This reverts commit 7bfd4e3.
* Change file format and move the hash out of non-Windows platforms.
* Fix: Use sizeof for Analytics DLL hash array
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.
* Refactor: Adjust log level for hash mismatch in analytics_windows
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.
* Format code.
* Updated Analytics Windows to allow multiple DLL hashes, and add usage of Options struct. (#1744)
* Update script and code to handle known hashes for multiple DLLs.
* Fixed issues on Windows with multi-dll-hash code.
* Fix: Link Crypt32.lib for firebase_analytics on Windows
Add Crypt32.lib to the linked libraries for the firebase_analytics
target on Windows. This resolves a linker error LNK2019 for
_CryptBinaryToStringA@20, which was occurring in builds that
included analytics_windows.obj (e.g., firebase_analytics_test).
While CryptBinaryToStringA is not directly called in
analytics_windows.cc, the linker indicates that analytics_windows.obj
requires this symbol, possibly due to an indirect dependency or
other build system interactions. Linking Crypt32.lib provides the
necessary symbol.
* Fix: Link Crypt32.lib for firebase_analytics_test on Windows
I've added Crypt32.lib to the dependencies of the firebase_analytics_test
target in `analytics/tests/CMakeLists.txt` when building on Windows.
This is to resolve a persistent LNK2019 error for
_CryptBinaryToStringA@20, which occurs during the linking phase of
firebase_analytics_test.obj. This change directly links the
required system library at the test executable level.
* Update DLL.
* Remove unused file.
* Remove extra file.
* Initialize Analytics C SDK with AppOptions on desktop
This change updates the desktop analytics initialization to use the
newly required Options struct for the Windows C API.
- In `analytics/src/analytics_desktop.cc`:
- `firebase::analytics::Initialize(const App& app)` now retrieves
`app_id` and `package_name` from `app.options()`.
- It calls `GoogleAnalytics_Options_Create()` to create the options struct.
- Populates `app_id`, `package_name`, and sets a default for
`analytics_collection_enabled_at_first_launch`.
- Calls `GoogleAnalytics_Initialize()` with the populated options struct.
- String lifetimes for `app_id` and `package_name` are handled by
creating local `std::string` copies before passing their `c_str()`
to the C API.
* Update stub generation.
* Format code.
* Fix build issues.
* Update Analytics to use new options struct. (#1745)
* Initialize Analytics C SDK with AppOptions on desktop
This change updates the desktop analytics initialization to use the
newly required Options struct for the Windows C API.
- In `analytics/src/analytics_desktop.cc`:
- `firebase::analytics::Initialize(const App& app)` now retrieves
`app_id` and `package_name` from `app.options()`.
- It calls `GoogleAnalytics_Options_Create()` to create the options struct.
- Populates `app_id`, `package_name`, and sets a default for
`analytics_collection_enabled_at_first_launch`.
- Calls `GoogleAnalytics_Initialize()` with the populated options struct.
- String lifetimes for `app_id` and `package_name` are handled by
creating local `std::string` copies before passing their `c_str()`
to the C API.
* Format code.
* Fix build issues.
---------
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
* Format code.
* Only display Analytics warnings if we are not in stub mode.
* Format code.
* Address review comments and add missing copyright notices.
* Remove extraneous commented out code.
* Add another known hash to the list.
---------
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
* Update analytics_desktop.cc
---------
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
Co-authored-by: a-maurice <[email protected]>
0 commit comments