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 <amaurice@google.com>
0 commit comments