UI for the Data Saver InfoBar Promo

UI for the Data Saver InfoBar Promo

For non-users of Data Saver, show a promotional InfoBar if the following are true:

The user in the DataReductionProxyInfobarPromo field trial.
The user doesn’t have the Data Reduction Proxy enabled.
The user saw the promo in a version >= M51.
The user didn’t opt out of the FRE promo card.
The current version is at least two milestones after the last promo was displayed.
The infobar has never been shown and dismissed before.
The request is HTTP.

BUG=610757
Committed: https://crrev.com/ea3202d691f6594fd34e9bbb699e866cd12a07a9
Cr-Commit-Position: refs/heads/master@{#403267}

[megjablon, 2016-05-31 21:43:09]

Description was changed from

==========
Native UI for the Data Saver Infobar Promo

For non-users of Data Saver, show a promotional InfoBar if the following are
true:

The user in the DataReductionProxyInfobarPromo field trial.
The user doesn’t have the Data Reduction Proxy enabled.
The user saw the promo in a version >= M51.
The user didn’t opt out of the FRE promo card.
The current version is at least two milestones after the last promo was
displayed.
The infobar has never been shown and dismissed before.
The request is HTTP.

BUG=610757
==========

to

==========
UI for the Data Saver Infobar Promo

For non-users of Data Saver, show a promotional InfoBar if the following are
true:

The user in the DataReductionProxyInfobarPromo field trial.
The user doesn’t have the Data Reduction Proxy enabled.
The user saw the promo in a version >= M51.
The user didn’t opt out of the FRE promo card.
The current version is at least two milestones after the last promo was
displayed.
The infobar has never been shown and dismissed before.
The request is HTTP.

BUG=610757
==========

[megjablon, 2016-06-01 00:51:16]

Patchset #1 (id:1) has been deleted

[megjablon, 2016-06-01 21:40:59]

Patchset #2 (id:40001) has been deleted

[megjablon, 2016-06-01 21:43:23]

Patchset #2 (id:60001) has been deleted

[megjablon, 2016-06-01 22:36:05]

Description was changed from

==========
UI for the Data Saver Infobar Promo

For non-users of Data Saver, show a promotional InfoBar if the following are
true:

The user in the DataReductionProxyInfobarPromo field trial.
The user doesn’t have the Data Reduction Proxy enabled.
The user saw the promo in a version >= M51.
The user didn’t opt out of the FRE promo card.
The current version is at least two milestones after the last promo was
displayed.
The infobar has never been shown and dismissed before.
The request is HTTP.

BUG=610757
==========

to

==========
UI for the Data Saver InfoBar Promo

For non-users of Data Saver, show a promotional InfoBar if the following are
true:

The user in the DataReductionProxyInfobarPromo field trial.
The user doesn’t have the Data Reduction Proxy enabled.
The user saw the promo in a version >= M51.
The user didn’t opt out of the FRE promo card.
The current version is at least two milestones after the last promo was
displayed.
The infobar has never been shown and dismissed before.
The request is HTTP.

BUG=610757
==========

[megjablon, 2016-06-10 01:10:41]

megjablon@chromium.org changed reviewers:
+ bengr@chromium.org, rajendrant@chromium.org

[megjablon, 2016-06-10 01:10:42]

PTAL, thanks!

[megjablon, 2016-06-10 01:24:57]

Patchset #4 (id:120001) has been deleted

[gone, 2016-06-10 01:32:47]

dfalcantara@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[gone, 2016-06-10 01:32:48]

Drive by infobar watchlist comment:
Any reason you're not using the already-existing mipmap-mdpi/app_icon.png and
related icons?  Those are automatically changed during build time to match
whatever channel of Chrome you're on.

[gone, 2016-06-10 01:33:54]

dfalcantara@chromium.org changed reviewers:
- dfalcantara@chromium.org

[megjablon, 2016-06-10 20:08:06]

Patchset #5 (id:160001) has been deleted

[megjablon, 2016-06-10 20:08:43]

On 2016/06/10 01:32:48, dfalcantara wrote:
> Drive by infobar watchlist comment:
> Any reason you're not using the already-existing mipmap-mdpi/app_icon.png and
> related icons?  Those are automatically changed during build time to match
> whatever channel of Chrome you're on.

Nope, just didn't know about it. Thanks for the suggestion! I've switched to
using app_icon.png.

[Raj, 2016-06-13 17:52:19]

https://chromiumcodereview.appspot.com/2022313002/diff/180001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/firstrun/DataReductionProxyFirstRunFragment.java
(right):

https://chromiumcodereview.appspot.com/2022313002/diff/180001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/DataReductionProxyFirstRunFragment.java:68:
AboutVersionStrings versionStrings = PrefServiceBridge.getInstance()
Could you pass no argument to saveFreOrSecondRunPromoDisplayed(), and have these
AboutVersionStrings variables within DataReductionPromoUtils ?

https://chromiumcodereview.appspot.com/2022313002/diff/180001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java
(right):

https://chromiumcodereview.appspot.com/2022313002/diff/180001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:32:
* button to enable the proxy.
Add something like this at the end.
The InfoBar is displayed when its not shown previously, for HTTP pages and under
certain milestone restrictions are met.

https://chromiumcodereview.appspot.com/2022313002/diff/180001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:37:
private static Bitmap sIcon;
These could be nonstatic final, similar to how other classes in this package do.

https://chromiumcodereview.appspot.com/2022313002/diff/180001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBarDelegate.java
(right):

https://chromiumcodereview.appspot.com/2022313002/diff/180001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBarDelegate.java:23:
private DataReductionPromoInfoBarDelegate() {
nit: may be empty constructor can be defined inline ?

https://chromiumcodereview.appspot.com/2022313002/diff/180001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBarDelegate.java:35:
InfoBar showDataReductionPromoInfoBar() {
/s/showDataReductionPromoInfoBar/showInfoBarPromo

https://chromiumcodereview.appspot.com/2022313002/diff/180001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoScreen.java
(right):

https://chromiumcodereview.appspot.com/2022313002/diff/180001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoScreen.java:117:
AboutVersionStrings versionStrings = PrefServiceBridge.getInstance()
Same as previous comment. Can the version be obtained in
saveFreOrSecondRunPromoDisplayed().

https://chromiumcodereview.appspot.com/2022313002/diff/180001/chrome/browser/...
File
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.cc
(right):

https://chromiumcodereview.appspot.com/2022313002/diff/180001/chrome/browser/...
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.cc:28:
~DataReductionPromoInfoBarDelegateAndroid() {}
nit: This could be inline in the header file.

https://chromiumcodereview.appspot.com/2022313002/diff/180001/chrome/browser/...
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.cc:44:
base::string16 DataReductionPromoInfoBarDelegateAndroid::GetMessageText()
Is this function needed ?

https://chromiumcodereview.appspot.com/2022313002/diff/180001/chrome/browser/...
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.cc:49:
int DataReductionPromoInfoBarDelegateAndroid::GetButtons() const {
Is this function needed ?

https://chromiumcodereview.appspot.com/2022313002/diff/180001/chrome/browser/...
File chrome/browser/ui/android/infobars/data_reduction_promo_infobar.cc (right):

https://chromiumcodereview.appspot.com/2022313002/diff/180001/chrome/browser/...
chrome/browser/ui/android/infobars/data_reduction_promo_infobar.cc:37:
DataReductionPromoInfoBar::~DataReductionPromoInfoBar() {
nit: Can this be inlined.

[bengr, 2016-06-20 15:18:33]

I'll wait for Raj to approve before I take a look.

[megjablon, 2016-06-23 23:17:30]

https://codereview.chromium.org/2022313002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/firstrun/DataReductionProxyFirstRunFragment.java
(right):

https://codereview.chromium.org/2022313002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/DataReductionProxyFirstRunFragment.java:68:
AboutVersionStrings versionStrings = PrefServiceBridge.getInstance()
On 2016/06/13 17:52:18, Raj wrote:
> Could you pass no argument to saveFreOrSecondRunPromoDisplayed(), and have
these
> AboutVersionStrings variables within DataReductionPromoUtils ?

Done.

https://codereview.chromium.org/2022313002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java
(right):

https://codereview.chromium.org/2022313002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:32:
* button to enable the proxy.
On 2016/06/13 17:52:18, Raj wrote:
> Add something like this at the end.
> The InfoBar is displayed when its not shown previously, for HTTP pages and
under
> certain milestone restrictions are met.

Done.

https://codereview.chromium.org/2022313002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:37:
private static Bitmap sIcon;
On 2016/06/13 17:52:18, Raj wrote:
> These could be nonstatic final, similar to how other classes in this package
do.

I believe the other classes are using native images, whereas this is using an
Android resource. Since we don't have the context later when the constructor is
called from native, we have to store the icon in the static launch(). If you see
another way to do this please let me know.

https://codereview.chromium.org/2022313002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBarDelegate.java
(right):

https://codereview.chromium.org/2022313002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBarDelegate.java:23:
private DataReductionPromoInfoBarDelegate() {
On 2016/06/13 17:52:18, Raj wrote:
> nit: may be empty constructor can be defined inline ?

Inlining is an optimization performed by the Java compiler.

https://codereview.chromium.org/2022313002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBarDelegate.java:35:
InfoBar showDataReductionPromoInfoBar() {
On 2016/06/13 17:52:18, Raj wrote:
> /s/showDataReductionPromoInfoBar/showInfoBarPromo

Done.

https://codereview.chromium.org/2022313002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoScreen.java
(right):

https://codereview.chromium.org/2022313002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoScreen.java:117:
AboutVersionStrings versionStrings = PrefServiceBridge.getInstance()
On 2016/06/13 17:52:18, Raj wrote:
> Same as previous comment. Can the version be obtained in
> saveFreOrSecondRunPromoDisplayed().

Done.

https://codereview.chromium.org/2022313002/diff/180001/chrome/browser/net/spd...
File
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.cc
(right):

https://codereview.chromium.org/2022313002/diff/180001/chrome/browser/net/spd...
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.cc:28:
~DataReductionPromoInfoBarDelegateAndroid() {}
On 2016/06/13 17:52:18, Raj wrote:
> nit: This could be inline in the header file.

Done.

https://codereview.chromium.org/2022313002/diff/180001/chrome/browser/net/spd...
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.cc:44:
base::string16 DataReductionPromoInfoBarDelegateAndroid::GetMessageText()
On 2016/06/13 17:52:18, Raj wrote:
> Is this function needed ?

Yes, it is abstract so it needs to be overwritten.

https://codereview.chromium.org/2022313002/diff/180001/chrome/browser/net/spd...
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.cc:49:
int DataReductionPromoInfoBarDelegateAndroid::GetButtons() const {
On 2016/06/13 17:52:18, Raj wrote:
> Is this function needed ?

Nope removed.

https://codereview.chromium.org/2022313002/diff/180001/chrome/browser/ui/andr...
File chrome/browser/ui/android/infobars/data_reduction_promo_infobar.cc (right):

https://codereview.chromium.org/2022313002/diff/180001/chrome/browser/ui/andr...
chrome/browser/ui/android/infobars/data_reduction_promo_infobar.cc:37:
DataReductionPromoInfoBar::~DataReductionPromoInfoBar() {
On 2016/06/13 17:52:18, Raj wrote:
> nit: Can this be inlined.

Style guide says not to
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts

[megjablon, 2016-06-24 23:46:55]

Patchset #7 (id:220001) has been deleted

[megjablon, 2016-06-24 23:54:38]

megjablon@chromium.org changed reviewers:
+ dfalcantara@chromium.org, pkasting@chromium.org

[megjablon, 2016-06-24 23:54:39]

pkasting: infobar_delegate.h
dfalcantara: chrome/android/* and  chrome/browser/ui/*
asvitkine: histograms.xml

[Raj, 2016-06-25 01:21:10]

lgtm % nits

https://codereview.chromium.org/2022313002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java
(right):

https://codereview.chromium.org/2022313002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:37:
private static Bitmap sIcon;
On 2016/06/23 23:17:30, megjablon wrote:
> On 2016/06/13 17:52:18, Raj wrote:
> > These could be nonstatic final, similar to how other classes in this package
> do.
> 
> I believe the other classes are using native images, whereas this is using an
> Android resource. Since we don't have the context later when the constructor
is
> called from native, we have to store the icon in the static launch(). If you
see
> another way to do this please let me know.

Acknowledged.

https://codereview.chromium.org/2022313002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBarDelegate.java
(right):

https://codereview.chromium.org/2022313002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBarDelegate.java:23:
private DataReductionPromoInfoBarDelegate() {
On 2016/06/23 23:17:30, megjablon wrote:
> On 2016/06/13 17:52:18, Raj wrote:
> > nit: may be empty constructor can be defined inline ?
> 
> Inlining is an optimization performed by the Java compiler.

Acknowledged.

https://codereview.chromium.org/2022313002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java
(right):

https://codereview.chromium.org/2022313002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:33:
* page is HTTP pages, andr certain milestone restrictions are met.
/s/ pages//
/s/andr/and/

https://codereview.chromium.org/2022313002/diff/240001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java
(right):

https://codereview.chromium.org/2022313002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:31:
* Generates an InfoBar to promote the data reduction proxy. The InfoBar contains
a message and a
nit: /s/data reduction proxy/data reduction proxy to non-users of it/

https://codereview.chromium.org/2022313002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:33:
* page is HTTP pages, andr certain milestone restrictions are met.
nit: pls add
Once the proxy is enabled from the infobar a toast message is shown.

https://codereview.chromium.org/2022313002/diff/240001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBarDelegate.java
(right):

https://codereview.chromium.org/2022313002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBarDelegate.java:11:
* Provides JNI methods for DataReductionPromoInfoBars.
nit: /s/DataReductionPromoInfoBars/DataReductionPromoInfoBar/

https://codereview.chromium.org/2022313002/diff/240001/chrome/browser/net/spd...
File
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.h
(right):

https://codereview.chromium.org/2022313002/diff/240001/chrome/browser/net/spd...
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.h:24:
// and a link, and is used only by an Android field trial.
nit: Mentioning about field trial here is not very relevant.
You could remove it or add something generic like,
and is used to promote data reduction proxy to non-users of it.

[bengr, 2016-06-27 01:54:13]

I'll take a look once you address Raj's nits.

[megjablon, 2016-06-27 17:47:22]

megjablon@chromium.org changed reviewers:
+ asvitkine@google.com
- pkasting@chromium.org

[megjablon, 2016-06-27 17:48:49]

megjablon@chromium.org changed reviewers:
+ blundell@chromium.org

[megjablon, 2016-06-27 17:48:50]

-pkasting since he's ooo
+blundell: infobar_delegate.h

[megjablon, 2016-06-27 19:57:09]

https://chromiumcodereview.appspot.com/2022313002/diff/240001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java
(right):

https://chromiumcodereview.appspot.com/2022313002/diff/240001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:31:
* Generates an InfoBar to promote the data reduction proxy. The InfoBar contains
a message and a
On 2016/06/25 01:21:10, Raj wrote:
> nit: /s/data reduction proxy/data reduction proxy to non-users of it/

Done.

https://chromiumcodereview.appspot.com/2022313002/diff/240001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:33:
* page is HTTP pages, andr certain milestone restrictions are met.
On 2016/06/25 01:21:10, Raj wrote:
> nit: pls add
> Once the proxy is enabled from the infobar a toast message is shown.

Done.

https://chromiumcodereview.appspot.com/2022313002/diff/240001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBarDelegate.java
(right):

https://chromiumcodereview.appspot.com/2022313002/diff/240001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBarDelegate.java:11:
* Provides JNI methods for DataReductionPromoInfoBars.
On 2016/06/25 01:21:10, Raj wrote:
> nit: /s/DataReductionPromoInfoBars/DataReductionPromoInfoBar/

Done.

https://chromiumcodereview.appspot.com/2022313002/diff/240001/chrome/browser/...
File
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.h
(right):

https://chromiumcodereview.appspot.com/2022313002/diff/240001/chrome/browser/...
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.h:24:
// and a link, and is used only by an Android field trial.
On 2016/06/25 01:21:10, Raj wrote:
> nit: Mentioning about field trial here is not very relevant.
> You could remove it or add something generic like,
> and is used to promote data reduction proxy to non-users of it.

Done.

[gone, 2016-06-27 20:44:58]

https://chromiumcodereview.appspot.com/2022313002/diff/280001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java
(right):

https://chromiumcodereview.appspot.com/2022313002/diff/280001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:187:
super.onButtonClicked(isPrimaryButton);
Functionality like this should handled by the DataReductionPromoInfoBarDelegate
instead.  InfoBar subclasses are (supposed to be) just for setting up the UI and
controlling what is visible or enabled (e.g.).

https://chromiumcodereview.appspot.com/2022313002/diff/280001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:202:
if (DataReductionProxySettings.getInstance().isDataReductionProxyEnabled())
return;
Ditto for this.

https://chromiumcodereview.appspot.com/2022313002/diff/280001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:204:
.dataReductionProxyUIAction(DataReductionProxyUma.ACTION_INFOBAR_DISMISSED);
"Dismiss" is an explicit closure by the user clicking on the X button: see
InfoBarDelegate::InfoBarDismissed() in infobar_delegate.h.  What are you trying
to measure?

In its current state, this clumps implicit closure (by page navigations) and
close button clicks together.  If you're just measuring times the X button was
clicked, that gets triggered via InfoBarDelegate::InfoBarDismissed().

https://chromiumcodereview.appspot.com/2022313002/diff/280001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:210:
InfoBarControlLayout mMessageLayout = layout.getMessageLayout();
getMessageLayout() exists to add subtitles (like domain names) to the message,
which gets sandwiched in between the icon and the X.

Instead, tell the InfoBarLayout to make a new one for your thing:
InfoBarControlLayout layout = layout.addControlLayout();
layout.addDescription(sText);

For future reference:
https://folio.googleplex.com/chrome-ux-specs-and-sources/Clank%20-%20Material...

https://chromiumcodereview.appspot.com/2022313002/diff/280001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBarDelegate.java
(right):

https://chromiumcodereview.appspot.com/2022313002/diff/280001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBarDelegate.java:36:
DataReductionPromoInfoBar infoBar = new DataReductionPromoInfoBar();
return new DataReductionPromoInfoBar()?

https://chromiumcodereview.appspot.com/2022313002/diff/280001/chrome/android/...
File
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/InfoBarTest.java
(right):

https://chromiumcodereview.appspot.com/2022313002/diff/280001/chrome/android/...
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/InfoBarTest.java:51:
private static final String M51_VERSION = "Chrome 51.0.0.0";
Eh, use a real branch number here, even if it isn't important overall.

51.0.2704.0

https://chromiumcodereview.appspot.com/2022313002/diff/280001/chrome/android/...
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/InfoBarTest.java:345:
assertTrue("Wrong infobar count", getInfoBars().isEmpty());
Any time you've checked for no infobars isn't testing what you're looking for:
1) You're immediately returning after the runOnUiThread call, which runs
asynchronously, so there's no time to pop up an infobar.

2) You're doing the check immediately, which is by default false because we
almost never explicitly show an infobar during tests.

Can you change maybeLaunchPromoInfoBar to return a boolean saying whether or not
the infobar should have been shown, then update everywhere?

https://chromiumcodereview.appspot.com/2022313002/diff/280001/tools/metrics/h...
File tools/metrics/histograms/histograms.xml (left):

https://chromiumcodereview.appspot.com/2022313002/diff/280001/tools/metrics/h...
tools/metrics/histograms/histograms.xml:76559: <int value="62"
label="WINDOWS_DESKTOP_SEARCH_INFOBAR_DELEGATE"/>
While you're here, can you add 63 for UPDATE_PASSWORD_INFOBAR_DELEGATE?  Someone
forgot to do it.

https://chromiumcodereview.appspot.com/2022313002/diff/280001/tools/metrics/h...
File tools/metrics/histograms/histograms.xml (right):

https://chromiumcodereview.appspot.com/2022313002/diff/280001/tools/metrics/h...
tools/metrics/histograms/histograms.xml:67769: +  <int value="12" label="InfoBar
promo dismissed (no action performed)"/>
Dismissed implies a user action.

[megjablon, 2016-06-27 22:16:44]

https://chromiumcodereview.appspot.com/2022313002/diff/280001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java
(right):

https://chromiumcodereview.appspot.com/2022313002/diff/280001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:187:
super.onButtonClicked(isPrimaryButton);
On 2016/06/27 20:44:58, dfalcantara wrote:
> Functionality like this should handled by the
DataReductionPromoInfoBarDelegate
> instead.  InfoBar subclasses are (supposed to be) just for setting up the UI
and
> controlling what is visible or enabled (e.g.).

Done.

https://chromiumcodereview.appspot.com/2022313002/diff/280001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:202:
if (DataReductionProxySettings.getInstance().isDataReductionProxyEnabled())
return;
On 2016/06/27 20:44:58, dfalcantara wrote:
> Ditto for this.

Done.

https://chromiumcodereview.appspot.com/2022313002/diff/280001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:204:
.dataReductionProxyUIAction(DataReductionProxyUma.ACTION_INFOBAR_DISMISSED);
On 2016/06/27 20:44:58, dfalcantara wrote:
> "Dismiss" is an explicit closure by the user clicking on the X button: see
> InfoBarDelegate::InfoBarDismissed() in infobar_delegate.h.  What are you
trying
> to measure?
> 
> In its current state, this clumps implicit closure (by page navigations) and
> close button clicks together.  If you're just measuring times the X button was
> clicked, that gets triggered via InfoBarDelegate::InfoBarDismissed().

Yep, I saw the InfoBarDismissed, but that doesn't cover all the cases I want. I
don't need to count explicit and implicit closure separately, but I want to
track both. We should know whether the user enabled or the snackbar was
dismissed for every snackbar shown. ACTION_INFOBAR_ENABLED +
ACTION_INFOBAR_DISMISSED should equal the total number of DRP infobars shown.

https://chromiumcodereview.appspot.com/2022313002/diff/280001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:210:
InfoBarControlLayout mMessageLayout = layout.getMessageLayout();
On 2016/06/27 20:44:58, dfalcantara wrote:
> getMessageLayout() exists to add subtitles (like domain names) to the message,
> which gets sandwiched in between the icon and the X.
> 
> Instead, tell the InfoBarLayout to make a new one for your thing:
> InfoBarControlLayout layout = layout.addControlLayout();
> layout.addDescription(sText);
> 
> For future reference:
>
https://folio.googleplex.com/chrome-ux-specs-and-sources/Clank%20-%20Material...

Done. Is there a way to increase the line spacing on this? It looks much more
compressed than the mocks I was given.

https://chromiumcodereview.appspot.com/2022313002/diff/280001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBarDelegate.java
(right):

https://chromiumcodereview.appspot.com/2022313002/diff/280001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBarDelegate.java:36:
DataReductionPromoInfoBar infoBar = new DataReductionPromoInfoBar();
On 2016/06/27 20:44:58, dfalcantara wrote:
> return new DataReductionPromoInfoBar()?

Done.

https://chromiumcodereview.appspot.com/2022313002/diff/280001/chrome/android/...
File
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/InfoBarTest.java
(right):

https://chromiumcodereview.appspot.com/2022313002/diff/280001/chrome/android/...
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/InfoBarTest.java:51:
private static final String M51_VERSION = "Chrome 51.0.0.0";
On 2016/06/27 20:44:58, dfalcantara wrote:
> Eh, use a real branch number here, even if it isn't important overall.
> 
> 51.0.2704.0

Done.

https://chromiumcodereview.appspot.com/2022313002/diff/280001/chrome/android/...
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/InfoBarTest.java:345:
assertTrue("Wrong infobar count", getInfoBars().isEmpty());
On 2016/06/27 20:44:58, dfalcantara wrote:
> Any time you've checked for no infobars isn't testing what you're looking for:
> 1) You're immediately returning after the runOnUiThread call, which runs
> asynchronously, so there's no time to pop up an infobar.
> 
> 2) You're doing the check immediately, which is by default false because we
> almost never explicitly show an infobar during tests.
> 
> Can you change maybeLaunchPromoInfoBar to return a boolean saying whether or
not
> the infobar should have been shown, then update everywhere?

Done.

https://chromiumcodereview.appspot.com/2022313002/diff/280001/tools/metrics/h...
File tools/metrics/histograms/histograms.xml (left):

https://chromiumcodereview.appspot.com/2022313002/diff/280001/tools/metrics/h...
tools/metrics/histograms/histograms.xml:76559: <int value="62"
label="WINDOWS_DESKTOP_SEARCH_INFOBAR_DELEGATE"/>
On 2016/06/27 20:44:58, dfalcantara wrote:
> While you're here, can you add 63 for UPDATE_PASSWORD_INFOBAR_DELEGATE? 
Someone
> forgot to do it.

Done.

https://chromiumcodereview.appspot.com/2022313002/diff/280001/tools/metrics/h...
File tools/metrics/histograms/histograms.xml (right):

https://chromiumcodereview.appspot.com/2022313002/diff/280001/tools/metrics/h...
tools/metrics/histograms/histograms.xml:67769: +  <int value="12" label="InfoBar
promo dismissed (no action performed)"/>
On 2016/06/27 20:44:58, dfalcantara wrote:
> Dismissed implies a user action.

That's why I have the "(no action performed)". We use this language for our
other promos too.

[megjablon, 2016-06-27 22:46:54]

Patchset #12 (id:340001) has been deleted

[gone, 2016-06-27 23:12:13]

Comments spanning both Patch Set 9 and 12.

https://chromiumcodereview.appspot.com/2022313002/diff/280001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java
(right):

https://chromiumcodereview.appspot.com/2022313002/diff/280001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:204:
.dataReductionProxyUIAction(DataReductionProxyUma.ACTION_INFOBAR_DISMISSED);
On 2016/06/27 22:16:44, megjablon wrote:
> On 2016/06/27 20:44:58, dfalcantara wrote:
> > "Dismiss" is an explicit closure by the user clicking on the X button: see
> > InfoBarDelegate::InfoBarDismissed() in infobar_delegate.h.  What are you
> trying
> > to measure?
> > 
> > In its current state, this clumps implicit closure (by page navigations) and
> > close button clicks together.  If you're just measuring times the X button
was
> > clicked, that gets triggered via InfoBarDelegate::InfoBarDismissed().
> 
> Yep, I saw the InfoBarDismissed, but that doesn't cover all the cases I want.
I
> don't need to count explicit and implicit closure separately, but I want to
> track both. We should know whether the user enabled or the snackbar was
> dismissed for every snackbar shown. ACTION_INFOBAR_ENABLED +
> ACTION_INFOBAR_DISMISSED should equal the total number of DRP infobars shown. 

You should update the histograms file, then.  It currently says that no action
was performed, which isn't correct because of the manaul closing action.

https://chromiumcodereview.appspot.com/2022313002/diff/280001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:210:
InfoBarControlLayout mMessageLayout = layout.getMessageLayout();
On 2016/06/27 22:16:44, megjablon wrote:
> On 2016/06/27 20:44:58, dfalcantara wrote:
> > getMessageLayout() exists to add subtitles (like domain names) to the
message,
> > which gets sandwiched in between the icon and the X.
> > 
> > Instead, tell the InfoBarLayout to make a new one for your thing:
> > InfoBarControlLayout layout = layout.addControlLayout();
> > layout.addDescription(sText);
> > 
> > For future reference:
> >
>
https://folio.googleplex.com/chrome-ux-specs-and-sources/Clank%20-%20Material...
> 
> Done. Is there a way to increase the line spacing on this? It looks much more
> compressed than the mocks I was given.

Eh, don't increase the spacing on it.  Their mocks should be following their
master spec, which the InfoBarLayout implements.  If you get pushback when this
lands, CC me and hwi@ about it.

https://chromiumcodereview.appspot.com/2022313002/diff/360001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java
(right):

https://chromiumcodereview.appspot.com/2022313002/diff/360001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:165:
DataReductionPromoInfoBarDelegate.onButtonClicked(isPrimaryButton,
getContext());
Don't override onButtonClicked here; instead, make
data_reduction_promo_infobar_delegate in C++ call to the Java
DataReductionPromoInfoBarDelegate via Accept()


DataReductionPromoInfoBarDelegate::Accept() {
  JNIEnv* env = base::android::AttachCurrentThread();
  Java_DataReductionPromoInfoBarDelegate_accept(env);
}

DataReductionPromoInfoBarDelegate.accept() {
    // Whatever your current onButtonClicked does, using ContextUtils for a
context to get the string
}

Kind of like what AppBannerInfoBarDelegateAndroid does.

https://chromiumcodereview.appspot.com/2022313002/diff/360001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:171:
DataReductionPromoInfoBarDelegate.onInfoBarClosed();
Given that you want to always record this, can you just add a JNI hook to
DataReductionPromoInfoBarDelegate::~DataReductionPromoInfoBarDelegate() that
calls the Java DataReductionPromoInfoBarDelegate to record that the infobar was
closed?  If you want to get fancy you could also check whether
DataReductionPromoInfoBarDelegate::Accept(),
DataReductionPromoInfoBarDelegate::Canceled(),
orDataReductionPromoInfoBarDelegate::InfoBarDismissed() were called to figure
out what the user did.

https://chromiumcodereview.appspot.com/2022313002/diff/360001/chrome/android/...
File
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/InfoBarTest.java
(right):

https://chromiumcodereview.appspot.com/2022313002/diff/360001/chrome/android/...
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/InfoBarTest.java:325:
assertFalse(DataReductionPromoInfoBar.maybeLaunchPromoInfoBar(
You should still do this on the UI thread.  I'd go as far as to call
ThreadUtils.assertOnUiThread inside of
DataReductionPromoInfoBar.maybeLaunchPromoInfoBar.

[blundell, 2016-06-28 05:39:43]

//components lgtm

[megjablon, 2016-06-29 04:43:45]

Patchset #13 (id:380001) has been deleted

[Peter Kasting, 2016-06-29 06:44:42]

pkasting@chromium.org changed reviewers:
+ pkasting@chromium.org

[Peter Kasting, 2016-06-29 06:44:44]

LGTM

https://chromiumcodereview.appspot.com/2022313002/diff/400001/chrome/browser/...
File
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.cc
(right):

https://chromiumcodereview.appspot.com/2022313002/diff/400001/chrome/browser/...
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.cc:24:
std::unique_ptr<DataReductionPromoInfoBarDelegateAndroid>(
Nit: Prefer base::MakeUnique()

https://chromiumcodereview.appspot.com/2022313002/diff/400001/chrome/browser/...
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.cc:31:
java_data_reduction_promo_delegate_() {}
Nit: Don't explicitly write out base class or member initializations that use
the default constructor

https://chromiumcodereview.appspot.com/2022313002/diff/400001/chrome/browser/...
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.cc:75:
return base::string16();
Wow, really?  Seems odd

https://chromiumcodereview.appspot.com/2022313002/diff/400001/chrome/browser/...
File
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.h
(right):

https://chromiumcodereview.appspot.com/2022313002/diff/400001/chrome/browser/...
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.h:23:
// Configures an infobar with two buttons that stays visible until it is
Nit: I don't know what "configures" means here.  Removing that word entirely
seems like it makes everything else more comprehensible.

https://chromiumcodereview.appspot.com/2022313002/diff/400001/chrome/browser/...
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.h:25:
// and two action buttons, and is used to promote data reduction proxy to
Nit: Why talk about "displaying a message and two action buttons"?  That feels
both too specific (why do callers care about how many buttons there are) and too
vague (if you're going to talk about them, say what they are).

Also, this is written very passively.

Something like this would likely be better than this whole comment block:

// An infobar that prompts the user to enable the data reduction proxy.

https://chromiumcodereview.appspot.com/2022313002/diff/400001/chrome/browser/...
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.h:50:
// ConfirmInfoBarDelegate
Nit: Trailing colon

https://chromiumcodereview.appspot.com/2022313002/diff/400001/chrome/browser/...
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.h:52:
bool Accept() override;
Nit: This goes below the next function (maintain base class declaration order). 
Keep .cc file matching.

https://chromiumcodereview.appspot.com/2022313002/diff/400001/chrome/browser/...
File chrome/browser/ui/android/infobars/data_reduction_promo_infobar.cc (right):

https://chromiumcodereview.appspot.com/2022313002/diff/400001/chrome/browser/...
chrome/browser/ui/android/infobars/data_reduction_promo_infobar.cc:16: //
DataReductionPromoInfoBar:
Nit: I usually do "// DataReductionPromoInfoBar ----" and continue out to 79
columns or so, to call out these dividers more visibly.

https://chromiumcodereview.appspot.com/2022313002/diff/400001/chrome/browser/...
chrome/browser/ui/android/infobars/data_reduction_promo_infobar.cc:42: return
std::unique_ptr<infobars::InfoBar>(
Nit: Prefer base::MakeUnique()

[megjablon, 2016-06-29 17:01:52]

https://chromiumcodereview.appspot.com/2022313002/diff/280001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java
(right):

https://chromiumcodereview.appspot.com/2022313002/diff/280001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:204:
.dataReductionProxyUIAction(DataReductionProxyUma.ACTION_INFOBAR_DISMISSED);
On 2016/06/27 23:12:13, dfalcantara wrote:
> On 2016/06/27 22:16:44, megjablon wrote:
> > On 2016/06/27 20:44:58, dfalcantara wrote:
> > > "Dismiss" is an explicit closure by the user clicking on the X button: see
> > > InfoBarDelegate::InfoBarDismissed() in infobar_delegate.h.  What are you
> > trying
> > > to measure?
> > > 
> > > In its current state, this clumps implicit closure (by page navigations)
and
> > > close button clicks together.  If you're just measuring times the X button
> was
> > > clicked, that gets triggered via InfoBarDelegate::InfoBarDismissed().
> > 
> > Yep, I saw the InfoBarDismissed, but that doesn't cover all the cases I
want.
> I
> > don't need to count explicit and implicit closure separately, but I want to
> > track both. We should know whether the user enabled or the snackbar was
> > dismissed for every snackbar shown. ACTION_INFOBAR_ENABLED +
> > ACTION_INFOBAR_DISMISSED should equal the total number of DRP infobars
shown. 
> 
> You should update the histograms file, then.  It currently says that no action
> was performed, which isn't correct because of the manaul closing action.

Ah ya I could see how that's confusing. I meant no action with the proxy.
Changed the wording.

https://chromiumcodereview.appspot.com/2022313002/diff/280001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:210:
InfoBarControlLayout mMessageLayout = layout.getMessageLayout();
On 2016/06/27 23:12:13, dfalcantara wrote:
> On 2016/06/27 22:16:44, megjablon wrote:
> > On 2016/06/27 20:44:58, dfalcantara wrote:
> > > getMessageLayout() exists to add subtitles (like domain names) to the
> message,
> > > which gets sandwiched in between the icon and the X.
> > > 
> > > Instead, tell the InfoBarLayout to make a new one for your thing:
> > > InfoBarControlLayout layout = layout.addControlLayout();
> > > layout.addDescription(sText);
> > > 
> > > For future reference:
> > >
> >
>
https://folio.googleplex.com/chrome-ux-specs-and-sources/Clank%20-%20Material...
> > 
> > Done. Is there a way to increase the line spacing on this? It looks much
more
> > compressed than the mocks I was given.
> 
> Eh, don't increase the spacing on it.  Their mocks should be following their
> master spec, which the InfoBarLayout implements.  If you get pushback when
this
> lands, CC me and hwi@ about it.

SGTM

https://chromiumcodereview.appspot.com/2022313002/diff/360001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java
(right):

https://chromiumcodereview.appspot.com/2022313002/diff/360001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:165:
DataReductionPromoInfoBarDelegate.onButtonClicked(isPrimaryButton,
getContext());
On 2016/06/27 23:12:13, dfalcantara wrote:
> Don't override onButtonClicked here; instead, make
> data_reduction_promo_infobar_delegate in C++ call to the Java
> DataReductionPromoInfoBarDelegate via Accept()
> 
> 
> DataReductionPromoInfoBarDelegate::Accept() {
>   JNIEnv* env = base::android::AttachCurrentThread();
>   Java_DataReductionPromoInfoBarDelegate_accept(env);
> }
> 
> DataReductionPromoInfoBarDelegate.accept() {
>     // Whatever your current onButtonClicked does, using ContextUtils for a
> context to get the string
> }
> 
> Kind of like what AppBannerInfoBarDelegateAndroid does.

Done.

https://chromiumcodereview.appspot.com/2022313002/diff/360001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:171:
DataReductionPromoInfoBarDelegate.onInfoBarClosed();
On 2016/06/27 23:12:13, dfalcantara wrote:
> Given that you want to always record this, can you just add a JNI hook to
> DataReductionPromoInfoBarDelegate::~DataReductionPromoInfoBarDelegate() that
> calls the Java DataReductionPromoInfoBarDelegate to record that the infobar
was
> closed?  If you want to get fancy you could also check whether
> DataReductionPromoInfoBarDelegate::Accept(),
> DataReductionPromoInfoBarDelegate::Canceled(),
> orDataReductionPromoInfoBarDelegate::InfoBarDismissed() were called to figure
> out what the user did.

Done.

https://chromiumcodereview.appspot.com/2022313002/diff/360001/chrome/android/...
File
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/InfoBarTest.java
(right):

https://chromiumcodereview.appspot.com/2022313002/diff/360001/chrome/android/...
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/InfoBarTest.java:325:
assertFalse(DataReductionPromoInfoBar.maybeLaunchPromoInfoBar(
On 2016/06/27 23:12:13, dfalcantara wrote:
> You should still do this on the UI thread.  I'd go as far as to call
> ThreadUtils.assertOnUiThread inside of
> DataReductionPromoInfoBar.maybeLaunchPromoInfoBar.

I added the @UiThreadTest to run this on the UI thread. Added assert.

https://chromiumcodereview.appspot.com/2022313002/diff/400001/chrome/browser/...
File
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.cc
(right):

https://chromiumcodereview.appspot.com/2022313002/diff/400001/chrome/browser/...
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.cc:24:
std::unique_ptr<DataReductionPromoInfoBarDelegateAndroid>(
On 2016/06/29 06:44:43, Peter Kasting (OOO) wrote:
> Nit: Prefer base::MakeUnique()

Done.

https://chromiumcodereview.appspot.com/2022313002/diff/400001/chrome/browser/...
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.cc:31:
java_data_reduction_promo_delegate_() {}
On 2016/06/29 06:44:43, Peter Kasting (OOO) wrote:
> Nit: Don't explicitly write out base class or member initializations that use
> the default constructor

Done.

https://chromiumcodereview.appspot.com/2022313002/diff/400001/chrome/browser/...
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.cc:75:
return base::string16();
On 2016/06/29 06:44:43, Peter Kasting (OOO) wrote:
> Wow, really?  Seems odd

Strings are all assigned in Java in the same location since we need to be in the
the java infobar class to add the substring.

https://chromiumcodereview.appspot.com/2022313002/diff/400001/chrome/browser/...
File
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.h
(right):

https://chromiumcodereview.appspot.com/2022313002/diff/400001/chrome/browser/...
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.h:23:
// Configures an infobar with two buttons that stays visible until it is
On 2016/06/29 06:44:44, Peter Kasting (OOO) wrote:
> Nit: I don't know what "configures" means here.  Removing that word entirely
> seems like it makes everything else more comprehensible.

Done.

https://chromiumcodereview.appspot.com/2022313002/diff/400001/chrome/browser/...
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.h:25:
// and two action buttons, and is used to promote data reduction proxy to
On 2016/06/29 06:44:43, Peter Kasting (OOO) wrote:
> Nit: Why talk about "displaying a message and two action buttons"?  That feels
> both too specific (why do callers care about how many buttons there are) and
too
> vague (if you're going to talk about them, say what they are).
> 
> Also, this is written very passively.
> 
> Something like this would likely be better than this whole comment block:
> 
> // An infobar that prompts the user to enable the data reduction proxy.

Done.

https://chromiumcodereview.appspot.com/2022313002/diff/400001/chrome/browser/...
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.h:50:
// ConfirmInfoBarDelegate
On 2016/06/29 06:44:43, Peter Kasting (OOO) wrote:
> Nit: Trailing colon

Done.

https://chromiumcodereview.appspot.com/2022313002/diff/400001/chrome/browser/...
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.h:52:
bool Accept() override;
On 2016/06/29 06:44:44, Peter Kasting (OOO) wrote:
> Nit: This goes below the next function (maintain base class declaration
order). 
> Keep .cc file matching.

Done.

https://chromiumcodereview.appspot.com/2022313002/diff/400001/chrome/browser/...
File chrome/browser/ui/android/infobars/data_reduction_promo_infobar.cc (right):

https://chromiumcodereview.appspot.com/2022313002/diff/400001/chrome/browser/...
chrome/browser/ui/android/infobars/data_reduction_promo_infobar.cc:16: //
DataReductionPromoInfoBar:
On 2016/06/29 06:44:44, Peter Kasting (OOO) wrote:
> Nit: I usually do "// DataReductionPromoInfoBar ----" and continue out to 79
> columns or so, to call out these dividers more visibly.

Done.

https://chromiumcodereview.appspot.com/2022313002/diff/400001/chrome/browser/...
chrome/browser/ui/android/infobars/data_reduction_promo_infobar.cc:42: return
std::unique_ptr<infobars::InfoBar>(
On 2016/06/29 06:44:44, Peter Kasting (OOO) wrote:
> Nit: Prefer base::MakeUnique()

Done.

[gone, 2016-06-29 17:25:08]

lgtm % comments

https://chromiumcodereview.appspot.com/2022313002/diff/420001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBarDelegate.java
(right):

https://chromiumcodereview.appspot.com/2022313002/diff/420001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBarDelegate.java:53:
public static void accept() {
I *think* these can be private static voids instead of public static voids.  JNI
does some weird magic with that.

https://chromiumcodereview.appspot.com/2022313002/diff/420001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBarDelegate.java:69:
public static void destroy() {
I'd call this onNativeDestroyed(); destroy() implies that the
DataReductionPromoInfoBarDelegate is being told to destroy itself, which doesn't
make sense in a static context.

https://chromiumcodereview.appspot.com/2022313002/diff/420001/chrome/browser/...
File
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.cc
(right):

https://chromiumcodereview.appspot.com/2022313002/diff/420001/chrome/browser/...
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.cc:53:
java_data_reduction_promo_delegate_.Reset(
Since you're not actually doing anything with the Java delegate after this
point, you could get by with vanilla static calls on the Java
DataReductionPromoInfoBarDelegate instead of creating and storing it, e.g. how
the Accept() block works.

JNIEnv* env = base::android::AttachCurrentThread();
Java_DataReductionPromoInfoBarDelegate_showPromoInfoBar(env);

[megjablon, 2016-06-29 20:11:31]

https://chromiumcodereview.appspot.com/2022313002/diff/420001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBarDelegate.java
(right):

https://chromiumcodereview.appspot.com/2022313002/diff/420001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBarDelegate.java:53:
public static void accept() {
On 2016/06/29 17:25:08, dfalcantara wrote:
> I *think* these can be private static voids instead of public static voids. 
JNI
> does some weird magic with that.

Done.

https://chromiumcodereview.appspot.com/2022313002/diff/420001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBarDelegate.java:69:
public static void destroy() {
On 2016/06/29 17:25:08, dfalcantara wrote:
> I'd call this onNativeDestroyed(); destroy() implies that the
> DataReductionPromoInfoBarDelegate is being told to destroy itself, which
doesn't
> make sense in a static context.

Done.

https://chromiumcodereview.appspot.com/2022313002/diff/420001/chrome/browser/...
File
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.cc
(right):

https://chromiumcodereview.appspot.com/2022313002/diff/420001/chrome/browser/...
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.cc:53:
java_data_reduction_promo_delegate_.Reset(
On 2016/06/29 17:25:08, dfalcantara wrote:
> Since you're not actually doing anything with the Java delegate after this
> point, you could get by with vanilla static calls on the Java
> DataReductionPromoInfoBarDelegate instead of creating and storing it, e.g. how
> the Accept() block works.
> 
> JNIEnv* env = base::android::AttachCurrentThread();
> Java_DataReductionPromoInfoBarDelegate_showPromoInfoBar(env);

Done.

[Alexei Svitkine (slow), 2016-06-29 20:15:29]

asvitkine@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Alexei Svitkine (slow), 2016-06-29 20:15:29]

lgtm

[Peter Kasting, 2016-06-29 20:48:29]

https://chromiumcodereview.appspot.com/2022313002/diff/400001/chrome/browser/...
File
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.cc
(right):

https://chromiumcodereview.appspot.com/2022313002/diff/400001/chrome/browser/...
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.cc:75:
return base::string16();
On 2016/06/29 17:01:52, megjablon wrote:
> On 2016/06/29 06:44:43, Peter Kasting (OOO) wrote:
> > Wow, really?  Seems odd
> 
> Strings are all assigned in Java in the same location since we need to be in
the
> the java infobar class to add the substring.

Ah, might be worth a comment.

[tbansal1, 2016-06-30 19:00:40]

tbansal@chromium.org changed reviewers:
+ tbansal@chromium.org

[tbansal1, 2016-06-30 19:00:42]

chrome/browser/net/spdyproxy/* lgtm % nits

https://chromiumcodereview.appspot.com/2022313002/diff/460001/chrome/browser/...
File
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.cc
(right):

https://chromiumcodereview.appspot.com/2022313002/diff/460001/chrome/browser/...
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.cc:8:
#include "chrome/browser/android/android_theme_resources.h"
Is this include necessary?

https://chromiumcodereview.appspot.com/2022313002/diff/460001/chrome/browser/...
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.cc:11:
#include "components/infobars/core/infobar_delegate.h"
components/infobars/core/infobar_delegate.h is already in .h file. So may be
skip from here.

https://chromiumcodereview.appspot.com/2022313002/diff/460001/chrome/browser/...
File
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.h
(right):

https://chromiumcodereview.appspot.com/2022313002/diff/460001/chrome/browser/...
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.h:48:
base::string16 GetMessageText() const override;
base/strings/string16.h

[megjablon, 2016-06-30 19:56:09]

https://chromiumcodereview.appspot.com/2022313002/diff/460001/chrome/browser/...
File
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.cc
(right):

https://chromiumcodereview.appspot.com/2022313002/diff/460001/chrome/browser/...
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.cc:8:
#include "chrome/browser/android/android_theme_resources.h"
On 2016/06/30 19:00:41, tbansal1 wrote:
> Is this include necessary?

Done.

https://chromiumcodereview.appspot.com/2022313002/diff/460001/chrome/browser/...
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.cc:11:
#include "components/infobars/core/infobar_delegate.h"
On 2016/06/30 19:00:41, tbansal1 wrote:
> components/infobars/core/infobar_delegate.h is already in .h file. So may be
> skip from here.

Done.

https://chromiumcodereview.appspot.com/2022313002/diff/460001/chrome/browser/...
File
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.h
(right):

https://chromiumcodereview.appspot.com/2022313002/diff/460001/chrome/browser/...
chrome/browser/net/spdyproxy/data_reduction_promo_infobar_delegate_android.h:48:
base::string16 GetMessageText() const override;
On 2016/06/30 19:00:41, tbansal1 wrote:
> base/strings/string16.h

Done.

[megjablon, 2016-06-30 19:56:20]

The CQ bit was checked by megjablon@chromium.org

[megjablon, 2016-06-30 19:56:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
rajendrant@chromium.org, blundell@chromium.org, pkasting@chromium.org,
dfalcantara@chromium.org, asvitkine@chromium.org, tbansal@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/2022313002/#ps480001 (title: ""tbansal
comments"")

[commit-bot: I haz the power, 2016-06-30 19:57:06]

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, 2016-06-30 20:26:00]

Description was changed from

==========
UI for the Data Saver InfoBar Promo

For non-users of Data Saver, show a promotional InfoBar if the following are
true:

The user in the DataReductionProxyInfobarPromo field trial.
The user doesn’t have the Data Reduction Proxy enabled.
The user saw the promo in a version >= M51.
The user didn’t opt out of the FRE promo card.
The current version is at least two milestones after the last promo was
displayed.
The infobar has never been shown and dismissed before.
The request is HTTP.

BUG=610757
==========

to

==========
UI for the Data Saver InfoBar Promo

For non-users of Data Saver, show a promotional InfoBar if the following are
true:

The user in the DataReductionProxyInfobarPromo field trial.
The user doesn’t have the Data Reduction Proxy enabled.
The user saw the promo in a version >= M51.
The user didn’t opt out of the FRE promo card.
The current version is at least two milestones after the last promo was
displayed.
The infobar has never been shown and dismissed before.
The request is HTTP.

BUG=610757
==========

[commit-bot: I haz the power, 2016-06-30 20:26:02]

Committed patchset #17 (id:480001)

[commit-bot: I haz the power, 2016-06-30 20:26:08]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-30 20:28:12]

Description was changed from

==========
UI for the Data Saver InfoBar Promo

For non-users of Data Saver, show a promotional InfoBar if the following are
true:

The user in the DataReductionProxyInfobarPromo field trial.
The user doesn’t have the Data Reduction Proxy enabled.
The user saw the promo in a version >= M51.
The user didn’t opt out of the FRE promo card.
The current version is at least two milestones after the last promo was
displayed.
The infobar has never been shown and dismissed before.
The request is HTTP.

BUG=610757
==========

to

==========
UI for the Data Saver InfoBar Promo

For non-users of Data Saver, show a promotional InfoBar if the following are
true:

The user in the DataReductionProxyInfobarPromo field trial.
The user doesn’t have the Data Reduction Proxy enabled.
The user saw the promo in a version >= M51.
The user didn’t opt out of the FRE promo card.
The current version is at least two milestones after the last promo was
displayed.
The infobar has never been shown and dismissed before.
The request is HTTP.

BUG=610757

Committed: https://crrev.com/ea3202d691f6594fd34e9bbb699e866cd12a07a9
Cr-Commit-Position: refs/heads/master@{#403267}
==========

[commit-bot: I haz the power, 2016-06-30 20:28:13]

Patchset 17 (id:??) landed as
https://crrev.com/ea3202d691f6594fd34e9bbb699e866cd12a07a9
Cr-Commit-Position: refs/heads/master@{#403267}