IPH - connect data saver previews

IPH - connect data saver previews

Adds the IPH that points to the data saver preview infobar.  This
includes three main changes:
- Adding support for determining which kind of infobar is showing in
Java.
- Hooking up the info bar show code to log an event saying it was shown.
- Hooking up the info bar show code to a function which can pick (if
any) a feature and string to show which will point to that infobar.

BUG=710648
Review-Url: https://codereview.chromium.org/2815653003
Cr-Commit-Position: refs/heads/master@{#467935}
Committed: https://chromium.googlesource.com/chromium/src/+/a8c329d4466035b680575a794744e469641e7b9b

[David Trainor- moved to gerrit, 2017-04-12 08:36:16]

dtrainor@chromium.org changed reviewers:
+ dfalcantara@chromium.org, nyquist@chromium.org

[David Trainor- moved to gerrit, 2017-04-12 08:36:17]

wip... haven't tested or compiled.  just seeing how i could do this.

Dan I'm adding you for your advice on if this is the best way to hook this up. 
Happy to talk tomorrow about it.  Thanks!

https://codereview.chromium.org/2815653003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/textbubble/ViewAnchoredTextBubble.java
(right):

https://codereview.chromium.org/2815653003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/textbubble/ViewAnchoredTextBubble.java:21:
public class ViewAnchoredTextBubble extends TextBubble
Might be worth pulling changes to this file out to another CL.

[David Trainor- moved to gerrit, 2017-04-12 08:41:20]

https://codereview.chromium.org/2815653003/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java
(right):

https://codereview.chromium.org/2815653003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java:170:
return nativeGetInfoBarIdentifier(mNativeInfoBarPtr);
TODO(me): Figure out how frequent this is and if we care.

https://codereview.chromium.org/2815653003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java:210:
private native int nativeGetInfoBarIdentifier(long nativeInfoBarAndroid);
Oops!  Make this actually talk to something :/.

https://codereview.chromium.org/2815653003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java
(right):

https://codereview.chromium.org/2815653003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:267:
showFeatureEngagementForInfoBar(infoBar);
Hmm this doesn't mean the infobar is showing.  Would it make sense to have a way
to listen to the layout for when infobars become visible (I could also listen to
the view?)?

https://codereview.chromium.org/2815653003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:271:
* Notifies that an infobar's View ({@link InfoBar#getView}) has changed. If the
infobar is
So in this case I guess the iph would just get dismissed... I think that's ok. 
I can try to test this scenario out tomorrow night.

[gone, 2017-04-12 17:12:44]

https://codereview.chromium.org/2815653003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java
(right):

https://codereview.chromium.org/2815653003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:267:
showFeatureEngagementForInfoBar(infoBar);
On 2017/04/12 08:41:20, David Trainor-ping if over 24h wrote:
> Hmm this doesn't mean the infobar is showing.  Would it make sense to have a
way
> to listen to the layout for when infobars become visible (I could also listen
to
> the view?)?

Maybe see when InfoBarAnimationListener fires?

https://codereview.chromium.org/2815653003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:271:
* Notifies that an infobar's View ({@link InfoBar#getView}) has changed. If the
infobar is
On 2017/04/12 08:41:20, David Trainor-ping if over 24h wrote:
> So in this case I guess the iph would just get dismissed... I think that's ok.

> I can try to test this scenario out tomorrow night.

Maybe.  This might happen when something like translate is going through
multiple phases, which means it's probably fine to dismiss the IPH anyway?

[David Trainor- moved to gerrit, 2017-04-17 23:33:40]

https://codereview.chromium.org/2815653003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainerLayout.java
(right):

https://codereview.chromium.org/2815653003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainerLayout.java:64:
public class InfoBarContainerLayout extends FrameLayout {
This is sad, but it's the only way I could get Item out.  Would you prefer
@VisibleForTesting?  I could also expose this through a listener interface in
InfoBarContainer and cast to an InfoBar there to make it a bit cleaner since
that is already public.

[gone, 2017-04-18 21:05:19]

https://codereview.chromium.org/2815653003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/IPHInfoBarSupport.java
(right):

https://codereview.chromium.org/2815653003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/IPHInfoBarSupport.java:27:
*/
Wasn't there a rule about capitalizing only the first letter of an acronym
(Url)?

https://codereview.chromium.org/2815653003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/IPHInfoBarSupport.java:62:
Profile profile = Profile.getLastUsedProfile();
You sure this is the right profile?  I'm worried you might be pulling the one
for the wrong tab.

https://codereview.chromium.org/2815653003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/IPHInfoBarSupport.java:75:
// Clean up any old infobar if necessary.
you're not cleaning up an infobar here.

https://codereview.chromium.org/2815653003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/IPHInfoBarSupport.java:89:
mCurrentState = new PopupState();
does it make sense to put any of these in a constructor call?

https://codereview.chromium.org/2815653003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java
(right):

https://codereview.chromium.org/2815653003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:140:
private final IPHInfoBarSupport mIPHSupport;
javadoc?

https://codereview.chromium.org/2815653003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:228:
public void setAnimationListener(InfoBarAnimationListener listener) {
rename the function?  set implies one

https://codereview.chromium.org/2815653003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainerLayout.java
(right):

https://codereview.chromium.org/2815653003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainerLayout.java:64:
public class InfoBarContainerLayout extends FrameLayout {
On 2017/04/17 23:33:40, David Trainor-ping if over 24h wrote:
> This is sad, but it's the only way I could get Item out.  Would you prefer
> @VisibleForTesting?  I could also expose this through a listener interface in
> InfoBarContainer and cast to an InfoBar there to make it a bit cleaner since
> that is already public.

Should be fine.  Constructor's still package protected anyway.

https://codereview.chromium.org/2815653003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainerLayout.java:703:
private ObserverList<InfoBarAnimationListener> mAnimationListeners = new
ObserverList<>();
private final

https://codereview.chromium.org/2815653003/diff/20001/chrome/browser/previews...
File chrome/browser/previews/previews_infobar_tab_helper.cc (right):

https://codereview.chromium.org/2815653003/diff/20001/chrome/browser/previews...
chrome/browser/previews/previews_infobar_tab_helper.cc:82:
feature_engagement_tracker::FeatureEngagementTracker* tracker =
Maybe this should be called after the infobar delegate is created?

https://codereview.chromium.org/2815653003/diff/20001/components/feature_enga...
File components/feature_engagement_tracker/internal/event_constants.cc (right):

https://codereview.chromium.org/2815653003/diff/20001/components/feature_enga...
components/feature_engagement_tracker/internal/event_constants.cc:1: ///
Copyright 2017 The Chromium Authors. All rights reserved.
Thou shalt count to three slashes, no more, no less.  Three shall be the number
thou shalt count, neither count thou two, excepting that thou then proceed to
three.  Five is right out.

https://codereview.chromium.org/2815653003/diff/20001/components/feature_enga...
File components/feature_engagement_tracker/internal/feature_constants.cc
(right):

https://codereview.chromium.org/2815653003/diff/20001/components/feature_enga...
components/feature_engagement_tracker/internal/feature_constants.cc:18:
base::FEATURE_DISABLED_BY_DEFAULT};
Should this be alphabetized?

https://codereview.chromium.org/2815653003/diff/20001/components/feature_enga...
File components/feature_engagement_tracker/internal/feature_list.cc (right):

https://codereview.chromium.org/2815653003/diff/20001/components/feature_enga...
components/feature_engagement_tracker/internal/feature_list.cc:15:
&kIPHDataSaverPreview};
Ditto for alphabetization.

[nyquist, 2017-04-18 23:09:04]

This looks very reasonable to me regarding the interaction with the component. I
don't feel strongly about exactly when you trigger the event.

https://codereview.chromium.org/2815653003/diff/20001/components/feature_enga...
File
components/feature_engagement_tracker/public/android/java/src/org/chromium/components/feature_engagement_tracker/EventConstants.java
(right):

https://codereview.chromium.org/2815653003/diff/20001/components/feature_enga...
components/feature_engagement_tracker/public/android/java/src/org/chromium/components/feature_engagement_tracker/EventConstants.java:12:
public static final String DATA_SAVER_PREVIEW_OPENED =
"data_saver_preview_opened";
Since these are defined directly in the public subdir for Java, should we move
the .cc files for the constants for C++ to public as well?

https://codereview.chromium.org/2815653003/diff/20001/components/feature_enga...
File components/feature_engagement_tracker/public/feature_constants.h (right):

https://codereview.chromium.org/2815653003/diff/20001/components/feature_enga...
components/feature_engagement_tracker/public/feature_constants.h:22: extern
const base::Feature kIPHDataSaverPreview;
I guess dfalcantara@ would want these ordered as well?

[David Trainor- moved to gerrit, 2017-04-25 05:01:06]

https://codereview.chromium.org/2815653003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/IPHInfoBarSupport.java
(right):

https://codereview.chromium.org/2815653003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/IPHInfoBarSupport.java:62:
Profile profile = Profile.getLastUsedProfile();
On 2017/04/18 21:05:18, ping past 24hrs (dfalcantara) wrote:
> You sure this is the right profile?  I'm worried you might be pulling the one
> for the wrong tab.

Good point.  I'll pull from the tab thanks!

https://codereview.chromium.org/2815653003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java
(right):

https://codereview.chromium.org/2815653003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:140:
private final IPHInfoBarSupport mIPHSupport;
On 2017/04/18 21:05:19, slow (dfalcantara) wrote:
> javadoc?

Done.

https://codereview.chromium.org/2815653003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:228:
public void setAnimationListener(InfoBarAnimationListener listener) {
On 2017/04/18 21:05:19, slow (dfalcantara) wrote:
> rename the function?  set implies one

Done.

https://codereview.chromium.org/2815653003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainerLayout.java
(right):

https://codereview.chromium.org/2815653003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainerLayout.java:703:
private ObserverList<InfoBarAnimationListener> mAnimationListeners = new
ObserverList<>();
On 2017/04/18 21:05:19, slow (dfalcantara) wrote:
> private final

Done.

https://codereview.chromium.org/2815653003/diff/20001/chrome/browser/previews...
File chrome/browser/previews/previews_infobar_tab_helper.cc (right):

https://codereview.chromium.org/2815653003/diff/20001/chrome/browser/previews...
chrome/browser/previews/previews_infobar_tab_helper.cc:82:
feature_engagement_tracker::FeatureEngagementTracker* tracker =
On 2017/04/18 21:05:19, slow (dfalcantara) wrote:
> Maybe this should be called after the infobar delegate is created?

Ideally we want the precondition events to be fired before we query them to see
if we should show the IPH dialog.  In this case, it's a little odd.  We're
saying, "Log that we've shown a preview."  Then in the infobar we're effectively
asking, "Did we show a preview and should I show?"  It's odd because the event
and the triggering condition are almost the same here.

https://codereview.chromium.org/2815653003/diff/20001/components/feature_enga...
File components/feature_engagement_tracker/internal/event_constants.cc (right):

https://codereview.chromium.org/2815653003/diff/20001/components/feature_enga...
components/feature_engagement_tracker/internal/event_constants.cc:1: ///
Copyright 2017 The Chromium Authors. All rights reserved.
On 2017/04/18 21:05:19, slow (dfalcantara) wrote:
> Thou shalt count to three slashes, no more, no less.  Three shall be the
number
> thou shalt count, neither count thou two, excepting that thou then proceed to
> three.  Five is right out.

'Tis but a scratch!

https://codereview.chromium.org/2815653003/diff/20001/components/feature_enga...
File components/feature_engagement_tracker/internal/feature_constants.cc
(right):

https://codereview.chromium.org/2815653003/diff/20001/components/feature_enga...
components/feature_engagement_tracker/internal/feature_constants.cc:18:
base::FEATURE_DISABLED_BY_DEFAULT};
On 2017/04/18 21:05:19, slow (dfalcantara) wrote:
> Should this be alphabetized?

Left the demo mode feature above since the header broke that out separately. 
Then ended up just erasing the dummy feature.

https://codereview.chromium.org/2815653003/diff/20001/components/feature_enga...
File components/feature_engagement_tracker/internal/feature_list.cc (right):

https://codereview.chromium.org/2815653003/diff/20001/components/feature_enga...
components/feature_engagement_tracker/internal/feature_list.cc:15:
&kIPHDataSaverPreview};
On 2017/04/18 21:05:19, slow (dfalcantara) wrote:
> Ditto for alphabetization.

Acknowledged.

https://codereview.chromium.org/2815653003/diff/20001/components/feature_enga...
File
components/feature_engagement_tracker/public/android/java/src/org/chromium/components/feature_engagement_tracker/EventConstants.java
(right):

https://codereview.chromium.org/2815653003/diff/20001/components/feature_enga...
components/feature_engagement_tracker/public/android/java/src/org/chromium/components/feature_engagement_tracker/EventConstants.java:12:
public static final String DATA_SAVER_PREVIEW_OPENED =
"data_saver_preview_opened";
On 2017/04/18 23:09:04, nyquist wrote:
> Since these are defined directly in the public subdir for Java, should we move
> the .cc files for the constants for C++ to public as well?

I'm fine doing that.  We can move them over along with FeatureConstants in one
CL (or if Shakti's CL does it I'll wait for his to land).

https://codereview.chromium.org/2815653003/diff/20001/components/feature_enga...
File components/feature_engagement_tracker/public/feature_constants.h (right):

https://codereview.chromium.org/2815653003/diff/20001/components/feature_enga...
components/feature_engagement_tracker/public/feature_constants.h:22: extern
const base::Feature kIPHDataSaverPreview;
On 2017/04/18 23:09:04, nyquist wrote:
> I guess dfalcantara@ would want these ordered as well?

See cc comment.  Removed dummy and ended up just leaving this at the bottom
since Demo was pulled out separately.

[David Trainor- moved to gerrit, 2017-04-25 05:17:16]

The CQ bit was checked by dtrainor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 05:17:38]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-25 05:21:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-25 05:21:24]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[gone, 2017-04-25 20:49:09]

lgtm

[David Trainor- moved to gerrit, 2017-04-26 00:08:15]

The CQ bit was checked by dtrainor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 00:09:15]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-26 00:24:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-26 00:24:22]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[David Trainor- moved to gerrit, 2017-04-26 23:48:39]

Description was changed from

==========
IPH - connect data saver previews

Adds the IPH that points to the data saver preview infobar.  This
includes three main changes:
- Adding support for determining which kind of infobar is showing in
Java.
- Hooking up the data saver preview show code to the NotifyEvent() code
in the feature tracker.
- Hooking up the info bar show code to a function which can pick (if
any) a feature and string to show which will point to that infobar.

BUG=710648
==========

to

==========
IPH - connect data saver previews

Adds the IPH that points to the data saver preview infobar.  This
includes three main changes:
- Adding support for determining which kind of infobar is showing in
Java.
- Hooking up the info bar show code to log an event saying it was shown.
- Hooking up the info bar show code to a function which can pick (if
any) a feature and string to show which will point to that infobar.

BUG=710648
==========

[David Trainor- moved to gerrit, 2017-04-26 23:49:10]

The CQ bit was checked by dtrainor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 23:50:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-27 01:27:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-27 01:27:01]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[nyquist, 2017-04-27 05:47:17]

lgtm

https://codereview.chromium.org/2815653003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/IPHInfoBarSupport.java
(right):

https://codereview.chromium.org/2815653003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/IPHInfoBarSupport.java:92:
if (!mTracker.shouldTriggerHelpUI(params.feature)) return;
Optional nit: empty line after early return?

https://codereview.chromium.org/2815653003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/IPHInfoBarSupport.java:124:
switch (infoBar.getInfoBarIdentifier()) {
Nit: huh. Seems like you need a default case here?

https://codereview.chromium.org/2815653003/diff/80001/chrome/browser/previews...
File chrome/browser/previews/previews_infobar_tab_helper.cc (right):

https://codereview.chromium.org/2815653003/diff/80001/chrome/browser/previews...
chrome/browser/previews/previews_infobar_tab_helper.cc:9: #include
"chrome/browser/feature_engagement_tracker/feature_engagement_tracker_factory.h"
Nit: Are you sure you need to edit this file?

https://codereview.chromium.org/2815653003/diff/80001/components/feature_enga...
File
components/feature_engagement_tracker/public/android/java/src/org/chromium/components/feature_engagement_tracker/EventConstants.java
(right):

https://codereview.chromium.org/2815653003/diff/80001/components/feature_enga...
components/feature_engagement_tracker/public/android/java/src/org/chromium/components/feature_engagement_tracker/EventConstants.java:32:
* The data server preview infobar was shown.
Nit: s/server/saver/

[David Trainor- moved to gerrit, 2017-04-28 00:57:47]

https://codereview.chromium.org/2815653003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/IPHInfoBarSupport.java
(right):

https://codereview.chromium.org/2815653003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/IPHInfoBarSupport.java:92:
if (!mTracker.shouldTriggerHelpUI(params.feature)) return;
On 2017/04/27 05:47:17, nyquist (nychthemeron ping) wrote:
> Optional nit: empty line after early return?

Done.

https://codereview.chromium.org/2815653003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/IPHInfoBarSupport.java:124:
switch (infoBar.getInfoBarIdentifier()) {
On 2017/04/27 05:47:17, nyquist (nychthemeron ping) wrote:
> Nit: huh. Seems like you need a default case here?

Boo.  Darnit findbugs.

https://codereview.chromium.org/2815653003/diff/80001/chrome/browser/previews...
File chrome/browser/previews/previews_infobar_tab_helper.cc (right):

https://codereview.chromium.org/2815653003/diff/80001/chrome/browser/previews...
chrome/browser/previews/previews_infobar_tab_helper.cc:9: #include
"chrome/browser/feature_engagement_tracker/feature_engagement_tracker_factory.h"
On 2017/04/27 05:47:17, nyquist (nychthemeron ping) wrote:
> Nit: Are you sure you need to edit this file?

D'oh!

https://codereview.chromium.org/2815653003/diff/80001/components/feature_enga...
File
components/feature_engagement_tracker/public/android/java/src/org/chromium/components/feature_engagement_tracker/EventConstants.java
(right):

https://codereview.chromium.org/2815653003/diff/80001/components/feature_enga...
components/feature_engagement_tracker/public/android/java/src/org/chromium/components/feature_engagement_tracker/EventConstants.java:32:
* The data server preview infobar was shown.
On 2017/04/27 05:47:17, nyquist (nychthemeron ping) wrote:
> Nit: s/server/saver/

It was supposed to be server (the server-side preview, although I admit the
comment didn't make sense), but now it's on for all data saver things \o/!  Will
change.

[David Trainor- moved to gerrit, 2017-04-28 07:28:56]

The CQ bit was checked by dtrainor@chromium.org

[David Trainor- moved to gerrit, 2017-04-28 07:28:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org, nyquist@chromium.org
Link to the patchset: https://codereview.chromium.org/2815653003/#ps100001
(title: "Addressed comments")

[commit-bot: I haz the power, 2017-04-28 07:29:11]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-28 08:45:08]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493364536179340,
"parent_rev": "f32671c0f445b58612ebba5c7a72de7c7981e8e9", "commit_rev":
"f60cd166ef7ed04cbc3b23ea0522326df6dc5007"}

[commit-bot: I haz the power, 2017-04-28 08:45:25]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493364536179340,
"parent_rev": "fa3fcf6eb2ec5bfb3504ab2c118bc8406fd5ef57", "commit_rev":
"a8c329d4466035b680575a794744e469641e7b9b"}

[commit-bot: I haz the power, 2017-04-28 08:45:32]

Description was changed from

==========
IPH - connect data saver previews

Adds the IPH that points to the data saver preview infobar.  This
includes three main changes:
- Adding support for determining which kind of infobar is showing in
Java.
- Hooking up the info bar show code to log an event saying it was shown.
- Hooking up the info bar show code to a function which can pick (if
any) a feature and string to show which will point to that infobar.

BUG=710648
==========

to

==========
IPH - connect data saver previews

Adds the IPH that points to the data saver preview infobar.  This
includes three main changes:
- Adding support for determining which kind of infobar is showing in
Java.
- Hooking up the info bar show code to log an event saying it was shown.
- Hooking up the info bar show code to a function which can pick (if
any) a feature and string to show which will point to that infobar.

BUG=710648

Review-Url: https://codereview.chromium.org/2815653003
Cr-Commit-Position: refs/heads/master@{#467935}
Committed:
https://chromium.googlesource.com/chromium/src/+/a8c329d4466035b680575a794744...
==========

[commit-bot: I haz the power, 2017-04-28 08:45:33]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/a8c329d4466035b680575a794744...