In-product help flags should be available cross-platform |
|||
Issue descriptionIn chrome://flags, the in-product demo mode is only available on Android, but it should be available for all platforms.
,
Jun 5 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/31093e7189d7a055b362bc8f325ef96099ee1b20 commit 31093e7189d7a055b362bc8f325ef96099ee1b20 Author: Catherine Chung <catherinechung@google.com> Date: Mon Jun 05 19:00:11 2017 Refactored IPH demo mode to be run cross-platform. Current code only enables IPH demo mode on Android, restricting testing on other platforms. This feature needs to be cross-platform to be used with deferred onboarding (new tab) on Desktop Chrome. Things that were changed in this CL: * Makes the whole feature engagement tracker available to all platforms * Updated documentation to reflect this change * Enables demo mode in chrome://flags to be cross-platform * Change in feature_engagement_tracker_impl.cc was necessary to make it work for Windows' pathnames. Bug: 729138 Change-Id: I76b382e004ea9b73bd7fb2df26aa83ff7856422e Reviewed-on: https://chromium-review.googlesource.com/518933 Commit-Queue: Catherine Chung <catherinechung@google.com> Reviewed-by: Tommy Nyquist <nyquist@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Reviewed-by: Robert Liao <robliao@chromium.org> Cr-Commit-Position: refs/heads/master@{#477045} [modify] https://crrev.com/31093e7189d7a055b362bc8f325ef96099ee1b20/chrome/browser/BUILD.gn [modify] https://crrev.com/31093e7189d7a055b362bc8f325ef96099ee1b20/chrome/browser/about_flags.cc [modify] https://crrev.com/31093e7189d7a055b362bc8f325ef96099ee1b20/chrome/browser/flag_descriptions.cc [modify] https://crrev.com/31093e7189d7a055b362bc8f325ef96099ee1b20/chrome/browser/flag_descriptions.h [modify] https://crrev.com/31093e7189d7a055b362bc8f325ef96099ee1b20/components/BUILD.gn [modify] https://crrev.com/31093e7189d7a055b362bc8f325ef96099ee1b20/components/feature_engagement_tracker/README.md [modify] https://crrev.com/31093e7189d7a055b362bc8f325ef96099ee1b20/components/feature_engagement_tracker/internal/feature_engagement_tracker_impl.cc [modify] https://crrev.com/31093e7189d7a055b362bc8f325ef96099ee1b20/components/feature_engagement_tracker/internal/system_time_provider_unittest.cc
,
Jun 5 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/527967f74f2505aabedb6eba72ac5c22abc5ccc7 commit 527967f74f2505aabedb6eba72ac5c22abc5ccc7 Author: Dirk Pranke <dpranke@chromium.org> Date: Mon Jun 05 22:39:21 2017 Revert "Refactored IPH demo mode to be run cross-platform." This reverts commit 31093e7189d7a055b362bc8f325ef96099ee1b20. Reason for revert: This change adds static initializers to the build, which is not allowed. It broke the `sizes` step as a result: https://luci-milo.appspot.com/buildbot/chromium/Linux%20x64/41189 https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium%2FLinux_x64%2F41189%2F%2B%2Frecipes%2Fsteps%2Fsizes%2F0%2Fstdout Original change's description: > Refactored IPH demo mode to be run cross-platform. > > Current code only enables IPH demo mode on Android, restricting testing > on other platforms. > > This feature needs to be cross-platform to be used with deferred onboarding > (new tab) on Desktop Chrome. > > Things that were changed in this CL: > * Makes the whole feature engagement tracker available to all platforms > * Updated documentation to reflect this change > * Enables demo mode in chrome://flags to be cross-platform > * Change in feature_engagement_tracker_impl.cc was necessary to make it > work for Windows' pathnames. > > Bug: 729138 > Change-Id: I76b382e004ea9b73bd7fb2df26aa83ff7856422e > Reviewed-on: https://chromium-review.googlesource.com/518933 > Commit-Queue: Catherine Chung <catherinechung@google.com> > Reviewed-by: Tommy Nyquist <nyquist@chromium.org> > Reviewed-by: Scott Violet <sky@chromium.org> > Reviewed-by: Robert Liao <robliao@chromium.org> > Cr-Commit-Position: refs/heads/master@{#477045} TBR=sky@chromium.org,nyquist@chromium.org,robliao@chromium.org,erikchen@chromium.org,catherinechung@google.com NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 729138 Change-Id: Ia6b18d2b797eacea75ec16349f9f42d32ea3f0e8 Reviewed-on: https://chromium-review.googlesource.com/524289 Commit-Queue: Dirk Pranke <dpranke@chromium.org> Reviewed-by: Dirk Pranke <dpranke@chromium.org> Cr-Commit-Position: refs/heads/master@{#477106} [modify] https://crrev.com/527967f74f2505aabedb6eba72ac5c22abc5ccc7/chrome/browser/BUILD.gn [modify] https://crrev.com/527967f74f2505aabedb6eba72ac5c22abc5ccc7/chrome/browser/about_flags.cc [modify] https://crrev.com/527967f74f2505aabedb6eba72ac5c22abc5ccc7/chrome/browser/flag_descriptions.cc [modify] https://crrev.com/527967f74f2505aabedb6eba72ac5c22abc5ccc7/chrome/browser/flag_descriptions.h [modify] https://crrev.com/527967f74f2505aabedb6eba72ac5c22abc5ccc7/components/BUILD.gn [modify] https://crrev.com/527967f74f2505aabedb6eba72ac5c22abc5ccc7/components/feature_engagement_tracker/README.md [modify] https://crrev.com/527967f74f2505aabedb6eba72ac5c22abc5ccc7/components/feature_engagement_tracker/internal/feature_engagement_tracker_impl.cc [modify] https://crrev.com/527967f74f2505aabedb6eba72ac5c22abc5ccc7/components/feature_engagement_tracker/internal/system_time_provider_unittest.cc
,
Jun 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/832c0a2f3b0f11b51c6509302b365269c2f815a0 commit 832c0a2f3b0f11b51c6509302b365269c2f815a0 Author: Tommy Nyquist <nyquist@chromium.org> Date: Fri Jun 09 19:20:42 2017 Never store base::Feature as key to std::map or std::set. In the feature engagement tracker base::Feature was sometimes used as the key to std::map and std::set members. That was all fine until the base::Feature definition for in-product help features was changed to constexpr in https://chromium-review.googlesource.com/c/525053/ . That lead to each instance of a feature not necessarily having the same address. Luckily, up until now, the feature engagement tracker has only been used on Android, and particularly, it has only ever been used from Java. The Java version of the API only accepts Java String objects as representations of features, and there is a mapping from the String to the base::Feature* in the JNI bridge. This means that all usages from Java use the same pointer to a feature. In addition, that feature map was initialized from the same place as where other configuration objects, etc., got their feature pointers from, so in practice it all worked. This CL changes all members to instead always use the feature name stored as std::string as the unique identifier, similarly to the experiment/variations code in //base. BUG=729791, 729138 Change-Id: I7169e8c5344ba5922de8cf1bd04d7097264d5412 Reviewed-on: https://chromium-review.googlesource.com/527506 Commit-Queue: Tommy Nyquist <nyquist@chromium.org> Reviewed-by: David Trainor <dtrainor@chromium.org> Cr-Commit-Position: refs/heads/master@{#478367} [modify] https://crrev.com/832c0a2f3b0f11b51c6509302b365269c2f815a0/components/feature_engagement_tracker/internal/availability_model_impl.cc [modify] https://crrev.com/832c0a2f3b0f11b51c6509302b365269c2f815a0/components/feature_engagement_tracker/internal/availability_model_impl.h [modify] https://crrev.com/832c0a2f3b0f11b51c6509302b365269c2f815a0/components/feature_engagement_tracker/internal/availability_model_impl_unittest.cc [modify] https://crrev.com/832c0a2f3b0f11b51c6509302b365269c2f815a0/components/feature_engagement_tracker/internal/availability_store.cc [modify] https://crrev.com/832c0a2f3b0f11b51c6509302b365269c2f815a0/components/feature_engagement_tracker/internal/availability_store.h [modify] https://crrev.com/832c0a2f3b0f11b51c6509302b365269c2f815a0/components/feature_engagement_tracker/internal/availability_store_unittest.cc [modify] https://crrev.com/832c0a2f3b0f11b51c6509302b365269c2f815a0/components/feature_engagement_tracker/internal/chrome_variations_configuration.cc [modify] https://crrev.com/832c0a2f3b0f11b51c6509302b365269c2f815a0/components/feature_engagement_tracker/internal/chrome_variations_configuration.h [modify] https://crrev.com/832c0a2f3b0f11b51c6509302b365269c2f815a0/components/feature_engagement_tracker/internal/chrome_variations_configuration_unittest.cc [modify] https://crrev.com/832c0a2f3b0f11b51c6509302b365269c2f815a0/components/feature_engagement_tracker/internal/configuration.h [modify] https://crrev.com/832c0a2f3b0f11b51c6509302b365269c2f815a0/components/feature_engagement_tracker/internal/editable_configuration.cc [modify] https://crrev.com/832c0a2f3b0f11b51c6509302b365269c2f815a0/components/feature_engagement_tracker/internal/editable_configuration.h [modify] https://crrev.com/832c0a2f3b0f11b51c6509302b365269c2f815a0/components/feature_engagement_tracker/internal/feature_config_condition_validator_unittest.cc [modify] https://crrev.com/832c0a2f3b0f11b51c6509302b365269c2f815a0/components/feature_engagement_tracker/internal/once_condition_validator.cc [modify] https://crrev.com/832c0a2f3b0f11b51c6509302b365269c2f815a0/components/feature_engagement_tracker/internal/once_condition_validator.h [modify] https://crrev.com/832c0a2f3b0f11b51c6509302b365269c2f815a0/components/feature_engagement_tracker/internal/single_invalid_configuration.cc [modify] https://crrev.com/832c0a2f3b0f11b51c6509302b365269c2f815a0/components/feature_engagement_tracker/internal/single_invalid_configuration.h [modify] https://crrev.com/832c0a2f3b0f11b51c6509302b365269c2f815a0/components/feature_engagement_tracker/internal/stats.cc
,
Jun 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d4d24365f0994915ea469e96656d49deeef9dbcb commit d4d24365f0994915ea469e96656d49deeef9dbcb Author: Catherine Chung <catherinechung@google.com> Date: Sat Jun 10 02:09:39 2017 Refactored IPH demo mode to be run cross-platform. Current code only enables IPH demo mode on Android, restricting testing on other platforms. This feature needs to be cross-platform to be used with deferred onboarding (new tab) on Desktop Chrome. Things that were changed in this CL: * Makes the whole feature engagement tracker available to all platforms * Updated documentation to reflect this change * Enables demo mode in chrome://flags to be cross-platform * Change in feature_engagement_tracker_impl.cc was necessary to make it work for Windows' pathnames. * Moved some code from feature_list.cc to feature_list.h to resolve the static initializers bug * Fixed an issue with the SystemTimeProviderTest * Changed flag names from kIPH... to kInProductHelp... Original CL: https://chromium-review.googlesource.com/c/518933/ Revert CL: https://chromium-review.googlesource.com/c/524289/ This CL re-lands the original CL. NOTE: There were no changes made during this revert, since the bug was fixed in https://chromium-review.googlesource.com/c/525053/. Bug: 729138 , 729791 Change-Id: I395287ff1b433708e97762a536910a4750827b54 Reviewed-on: https://chromium-review.googlesource.com/526360 Commit-Queue: Catherine Chung <catherinechung@google.com> Reviewed-by: Scott Violet <sky@chromium.org> Reviewed-by: Tommy Nyquist <nyquist@chromium.org> Reviewed-by: Robert Liao <robliao@chromium.org> Cr-Commit-Position: refs/heads/master@{#478499} [modify] https://crrev.com/d4d24365f0994915ea469e96656d49deeef9dbcb/chrome/browser/BUILD.gn [modify] https://crrev.com/d4d24365f0994915ea469e96656d49deeef9dbcb/chrome/browser/about_flags.cc [modify] https://crrev.com/d4d24365f0994915ea469e96656d49deeef9dbcb/chrome/browser/flag_descriptions.cc [modify] https://crrev.com/d4d24365f0994915ea469e96656d49deeef9dbcb/chrome/browser/flag_descriptions.h [modify] https://crrev.com/d4d24365f0994915ea469e96656d49deeef9dbcb/components/BUILD.gn [modify] https://crrev.com/d4d24365f0994915ea469e96656d49deeef9dbcb/components/feature_engagement_tracker/README.md [modify] https://crrev.com/d4d24365f0994915ea469e96656d49deeef9dbcb/components/feature_engagement_tracker/internal/feature_engagement_tracker_impl.cc [modify] https://crrev.com/d4d24365f0994915ea469e96656d49deeef9dbcb/components/feature_engagement_tracker/internal/system_time_provider_unittest.cc [modify] https://crrev.com/d4d24365f0994915ea469e96656d49deeef9dbcb/components/feature_engagement_tracker/public/feature_list.cc [modify] https://crrev.com/d4d24365f0994915ea469e96656d49deeef9dbcb/components/feature_engagement_tracker/public/feature_list.h
,
Jun 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ba7195e2875e1e36a709caa5c0dabaa934e01c1a commit ba7195e2875e1e36a709caa5c0dabaa934e01c1a Author: Tommy Nyquist <nyquist@chromium.org> Date: Tue Jun 13 02:45:12 2017 Make FeatureEngagementTracker work correctly across all platforms. As part of ensuring that there were no extra static initializers because of how the feature engagement tracker features were used in about_flags.cc, the features were changed to defined as constexpr instead of extern const. This lead to every instance of a base::Feature to be a unique instance, which when later used at different places would lead to different instances of the conceptually same feature being passed to base::FeatureList::IsEnabled(...). That in turn would check whether this was the only unique instance in FeatureList::CheckFeatureIdentity(...), which in these cases it would not be. This effectively made the feature engagement tracker unusable on all platforms except for on Android, where the features were used through a Java shim wrapper, which had a single unique instance of each feature. This CL fixes this by making all the base::Feature instances extern const again. That however, lead to another issue where the DEFINE_VARIATION_PARAM(...) macro no longer could read the name from the base::Feature without causing a lot of static initializers, so the name of the feature is now instead passed in. Also, in the about_flags.cc array of features itself, there was an issue with trying to extract the name of the kIPHDemoMode, which also lead to static initializers. Following how this is done in non-Android platforms, this CL makes it an inline string instead. This CL does not clean up all the other bad examples of usages of .name in FEATURE_WITH_PARAMS_VALUE_TYPE macro usages for OS_ANDROID. Lastly, since this code is now cross-platform it should be treated as such, so this CL ensures that all In-Product Help features are only defined on the platform they are used on. To ensure platforms with no usages of In-Product Help so far work correctly, dummy entries are added to the two arrays (kAllFeatures[] and kIPHDemoModeChoiceVariations[]) that refer to them. Lastly, the header guard was incorrect for the feature_list.h file, so that is cleaned up. BUG= 729138 , 729791 Change-Id: Ie98bd4636dd076e2f02c2035236448096d7a55c1 Reviewed-on: https://chromium-review.googlesource.com/531659 Commit-Queue: Tommy Nyquist <nyquist@chromium.org> Reviewed-by: Robert Liao <robliao@chromium.org> Reviewed-by: Alexei Svitkine <asvitkine@chromium.org> Cr-Commit-Position: refs/heads/master@{#478872} [modify] https://crrev.com/ba7195e2875e1e36a709caa5c0dabaa934e01c1a/chrome/browser/about_flags.cc [modify] https://crrev.com/ba7195e2875e1e36a709caa5c0dabaa934e01c1a/components/feature_engagement_tracker/public/BUILD.gn [add] https://crrev.com/ba7195e2875e1e36a709caa5c0dabaa934e01c1a/components/feature_engagement_tracker/public/feature_constants.cc [modify] https://crrev.com/ba7195e2875e1e36a709caa5c0dabaa934e01c1a/components/feature_engagement_tracker/public/feature_constants.h [modify] https://crrev.com/ba7195e2875e1e36a709caa5c0dabaa934e01c1a/components/feature_engagement_tracker/public/feature_list.cc [modify] https://crrev.com/ba7195e2875e1e36a709caa5c0dabaa934e01c1a/components/feature_engagement_tracker/public/feature_list.h
,
Jun 13 2017
Unless something unexpected happens, ba7195e2875e1e36a709caa5c0dabaa934e01c1a should be the last commit necessary to fix this issue.
,
Jul 13 2017
,
Jul 17 2017
|
|||
►
Sign in to add a comment |
|||
Comment 1 by bugdroid1@chromium.org
, Jun 5 2017