New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 729138 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Email to this user bounced
Closed: Jul 2017
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Feature



Sign in to add a comment

In-product help flags should be available cross-platform

Project Member Reported by catherinechung@google.com, Jun 2 2017

Issue description

In chrome://flags, the in-product demo mode is only available on Android, but it should be available for all platforms.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d7c47ce705c55d3622759c990a073b5ef4459655

commit d7c47ce705c55d3622759c990a073b5ef4459655
Author: Tommy Nyquist <nyquist@chromium.org>
Date: Mon Jun 05 17:05:54 2017

Fix leak in AvailabilityStoreTest.StorageDirectory

The AvailabilityStoreTest.StorageDirectory test kicks off a pipeline,
just to ensure that the storage directory is correctly set in the
underlying database. However, with the test infrastructure in use,
it does not correctly clean up the pipeline of callbacks.

This means that it leads to leaks, which was caught by the
linux_chromium_asan_rel_ng bot as soon as the feature_engagement_tracker
was starting to be run on the linux platform.
This is an example of a build where the issue appears:
https://luci-milo.appspot.com/buildbot/tryserver.chromium.linux/linux_chromium_asan_rel_ng/385034
which is a dry-run of this CL:
https://chromium-review.googlesource.com/c/518933/

This CL therefore finishes the pipeline by telling the test that
the next step in the pipeline was unsuccessful, effectively killing
off the whole pipline and cleaning up the allocated memory.

BUG= 729138 

Change-Id: Ic1820dc6ba710159c6454771da5420c6a0efd162
Reviewed-on: https://chromium-review.googlesource.com/523665
Reviewed-by: Matthew Jones <mdjones@chromium.org>
Commit-Queue: Tommy Nyquist <nyquist@chromium.org>
Cr-Commit-Position: refs/heads/master@{#477003}
[modify] https://crrev.com/d7c47ce705c55d3622759c990a073b5ef4459655/components/feature_engagement_tracker/internal/availability_store_unittest.cc

Project Member

Comment 2 by bugdroid1@chromium.org, 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

Project Member

Comment 3 by bugdroid1@chromium.org, 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

Project Member

Comment 4 by bugdroid1@chromium.org, 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

Project Member

Comment 5 by bugdroid1@chromium.org, 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

Project Member

Comment 6 by bugdroid1@chromium.org, 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

Unless something unexpected happens, ba7195e2875e1e36a709caa5c0dabaa934e01c1a should be the last commit necessary to fix this issue.
Project Member

Comment 8 by sheriffbot@chromium.org, Jul 13 2017

Labels: Hotlist-Google
Status: Fixed (was: Started)

Sign in to add a comment