Snackbar for promoting Data Saver to existing users

Snackbar for promoting Data Saver to existing users

After loading a page show a snackbar that promotes Data Saver
according to the following rules and conditions:

1) The Chrome instance should be in the promo Finch-controlled
experimental group.

2) It will only show on HTTP pages.

3) That values that the promo will be shown are configured via
a Finch param value.

4) Trigger at 100MB. Trigger again at 1GB.

5) For existing users, if they haven't yet hit 100MB or 1GB,
they should get the reminder like everyone else. If they're
past 1GB, they won't see anything.

BUG=610765
Committed: https://crrev.com/f87e8c45d2e027decba61c085266006cf630ce66
Cr-Commit-Position: refs/heads/master@{#427836}

[megjablon, 2016-10-11 22:04:47]

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

[megjablon, 2016-10-11 22:04:48]

PTAL
dfalcantara: chrome/android/*

[megjablon, 2016-10-11 22:07:25]

Description was changed from

==========
Snackbar for promoting Data Saver to exsiting users

Needs tests

BUG=610765
==========

to

==========
Snackbar for promoting Data Saver to existing users

After loading a page show a snackbar that promotes Data Saver according to the
following rules and conditions:

1) The Chrome instance should be in the promo Finch-controlled experimental
group.

2) It will only show on HTTP pages.

3) That values that the promo will be shown are configured via a Finch param
value.

4) Trigger at 100MB. Trigger again at 1GB.

5) For existing users, if they haven't yet hit 100MB or 1GB, they should get the
reminder like everyone else. If they're past 1GB, they won't see anything.

BUG=610765
==========

[megjablon, 2016-10-11 22:07:55]

Description was changed from

==========
Snackbar for promoting Data Saver to existing users

After loading a page show a snackbar that promotes Data Saver according to the
following rules and conditions:

1) The Chrome instance should be in the promo Finch-controlled experimental
group.

2) It will only show on HTTP pages.

3) That values that the promo will be shown are configured via a Finch param
value.

4) Trigger at 100MB. Trigger again at 1GB.

5) For existing users, if they haven't yet hit 100MB or 1GB, they should get the
reminder like everyone else. If they're past 1GB, they won't see anything.

BUG=610765
==========

to

==========
Snackbar for promoting Data Saver to existing users

After loading a page show a snackbar that promotes Data Saver
according to the following rules and conditions:

1) The Chrome instance should be in the promo Finch-controlled
experimental group.

2) It will only show on HTTP pages.

3) That values that the promo will be shown are configured via
a Finch param value.

4) Trigger at 100MB. Trigger again at 1GB.

5) For existing users, if they haven't yet hit 100MB or 1GB,
they should get the reminder like everyone else. If they're
past 1GB, they won't see anything.

BUG=610765
==========

[gone, 2016-10-13 01:03:11]

Haven't looked at the test, yet.

https://chromiumcodereview.appspot.com/2367403005/diff/40001/chrome/android/j...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://chromiumcodereview.appspot.com/2367403005/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:534: }
What was the reasoning for putting all this non-paint related code in
didFirstVisuallyNonEmptyPaint, again?  Could you add a comment about
it so that it's clear?

Maybe something explaining why it doesn't make sense to pop up a snackbar
saying you saved data when the user hasn't even seen anything yet?

https://chromiumcodereview.appspot.com/2367403005/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:983:
mDataReductionPromoSnackbarController = new
DataReductionPromoSnackbarController(this,
Does this need to be created if the snackbar promo isn't allowed?

https://chromiumcodereview.appspot.com/2367403005/diff/40001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPreferences.java
(right):

https://chromiumcodereview.appspot.com/2367403005/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPreferences.java:43:
// Whether the current activity is started from the snackbar promo.
Maybe use single line javadoc syntax here so it shows up in IDEs?

/** Whether the current Activity is started from the snackbar promo. */

https://chromiumcodereview.appspot.com/2367403005/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPreferences.java:59:
Bundle extras = getActivity().getIntent().getExtras();
Add another method to IntentUtils to handle this case; we keep getting Intents
from other apps that cause us to crash.  Look at how IntentUtils.safeGet* and
its ilk work.

https://chromiumcodereview.appspot.com/2367403005/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPreferences.java:75:
}
Could you integrate this into the conditional below?

if (mFromPromo) {
    statusChange = mIsEnabled
            ? DataReductionProxyUma.ACTION_SNACKBAR_LINK_CLICKED
            : DataReductionProxyUma.ACTION_SNACKBAR_LINK_CLICKED_DISABLED);
} else if (mWasEnabledAtCreation) {
    ...
} else {
    ...
}

https://chromiumcodereview.appspot.com/2367403005/diff/40001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java
(right):

https://chromiumcodereview.appspot.com/2367403005/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:41:
* snackbar.
Can you clean this sentence up a bit?  It's repeated throughout the file and
really, really hard to parse.  Splitting it into two would make sense since
these are javadocs, anyway.

/** Key used to save the content length when the snackbar promo was last shown.
*/

/** Key used to track whether the user has set the content length on the first
upgrade, or install of, the version that shows the snackbar. */

Also, why do you need to specify "first upgrade or install to"?  Can the user
get to the point where this boolean is recorded but not have the snackbar shown
to them?

https://chromiumcodereview.appspot.com/2367403005/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:154:
* Saves a shared pref indicating the content length when the last the snackbar
promo was
the last the

You might as well just flesh out the javadoc for the key itself and then leave
this as

/** See {@link #SHARED_PREF_DISPLAYED_SNACKBAR_PROMO_CONTENT_LENGTH}. */

That's let you avoid copy-pasting the same sentences everywhere.

https://chromiumcodereview.appspot.com/2367403005/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:166:
* has not been displayed before, returns -1,
Maybe

/**
 * Returns the content length from when the promo snackbar was last displayed.
 *
 * @return The content length, or -1 if the promo has not been displayed before.
 */

https://chromiumcodereview.appspot.com/2367403005/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:168:
* @return The content length when the last the snackbar promo was displayed.
the last the

https://chromiumcodereview.appspot.com/2367403005/diff/40001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/snackbar/DataReductionPromoSnackbarController.java
(right):

https://chromiumcodereview.appspot.com/2367403005/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/DataReductionPromoSnackbarController.java:27:
// for.
Sentence needs to be reworked, too.

https://chromiumcodereview.appspot.com/2367403005/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/DataReductionPromoSnackbarController.java:36:
private final int[] mPromoDataSavingsMB;
nit: newline here

https://chromiumcodereview.appspot.com/2367403005/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/DataReductionPromoSnackbarController.java:92:
promoDataSavingMB * BYTES_IN_MB).replace(".0", "\u00A0");
What's this unicode character for?

https://chromiumcodereview.appspot.com/2367403005/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/DataReductionPromoSnackbarController.java:95:
.concat(" ").concat(dataSavedString),
You can't just concatenate these two strings because different languages will
change the ordering of the words.  Look into one of the templated strings in the
GRD files; maybe IDS_DOWNLOAD_MANAGER_UI_SPACE_USED_GB would give you some
ideas?

https://chromiumcodereview.appspot.com/2367403005/diff/40001/chrome/android/j...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://chromiumcodereview.appspot.com/2367403005/diff/40001/chrome/android/j...
chrome/android/java/strings/android_chrome_strings.grd:892: Chrome just saved
you
See comment above about concatenating strings.

[megjablon, 2016-10-24 22:37:30]

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

[commit-bot: I haz the power, 2016-10-24 22:38:13]

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, 2016-10-24 22:41:57]

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

[commit-bot: I haz the power, 2016-10-24 22:41:58]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[megjablon, 2016-10-24 23:19:51]

Patchset #6 (id:100001) has been deleted

[megjablon, 2016-10-24 23:26:31]

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

[megjablon, 2016-10-24 23:26:40]

Patchset #5 (id:80001) has been deleted

[commit-bot: I haz the power, 2016-10-24 23:27:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[megjablon, 2016-10-24 23:59:00]

https://codereview.chromium.org/2367403005/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://codereview.chromium.org/2367403005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:534: }
On 2016/10/13 01:03:10, dfalcantara (slow 10.24) wrote:
> What was the reasoning for putting all this non-paint related code in
> didFirstVisuallyNonEmptyPaint, again?  Could you add a comment about
> it so that it's clear?
> 
> Maybe something explaining why it doesn't make sense to pop up a snackbar
> saying you saved data when the user hasn't even seen anything yet?

Done.

https://codereview.chromium.org/2367403005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:983:
mDataReductionPromoSnackbarController = new
DataReductionPromoSnackbarController(this,
On 2016/10/13 01:03:10, dfalcantara (slow 10.24) wrote:
> Does this need to be created if the snackbar promo isn't allowed?

Done.

https://codereview.chromium.org/2367403005/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPreferences.java
(right):

https://codereview.chromium.org/2367403005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPreferences.java:43:
// Whether the current activity is started from the snackbar promo.
On 2016/10/13 01:03:11, dfalcantara (slow 10.24) wrote:
> Maybe use single line javadoc syntax here so it shows up in IDEs?
> 
> /** Whether the current Activity is started from the snackbar promo. */

Done.

https://codereview.chromium.org/2367403005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPreferences.java:59:
Bundle extras = getActivity().getIntent().getExtras();
On 2016/10/13 01:03:11, dfalcantara (slow 10.24) wrote:
> Add another method to IntentUtils to handle this case; we keep getting Intents
> from other apps that cause us to crash.  Look at how IntentUtils.safeGet* and
> its ilk work.

Using the existing safeGetBooleanExtra

https://codereview.chromium.org/2367403005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPreferences.java:75:
}
On 2016/10/13 01:03:11, dfalcantara (slow 10.24) wrote:
> Could you integrate this into the conditional below?
> 
> if (mFromPromo) {
>     statusChange = mIsEnabled
>             ? DataReductionProxyUma.ACTION_SNACKBAR_LINK_CLICKED
>             : DataReductionProxyUma.ACTION_SNACKBAR_LINK_CLICKED_DISABLED);
> } else if (mWasEnabledAtCreation) {
>     ...
> } else {
>     ...
> }

Done.

https://codereview.chromium.org/2367403005/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java
(right):

https://codereview.chromium.org/2367403005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:41:
* snackbar.
On 2016/10/13 01:03:11, dfalcantara (slow 10.24) wrote:
> Can you clean this sentence up a bit?  It's repeated throughout the file and
> really, really hard to parse.  Splitting it into two would make sense since
> these are javadocs, anyway.
> 
> /** Key used to save the content length when the snackbar promo was last
shown.
> */
> 
> /** Key used to track whether the user has set the content length on the first
> upgrade, or install of, the version that shows the snackbar. */
> 
> Also, why do you need to specify "first upgrade or install to"?  Can the user
> get to the point where this boolean is recorded but not have the snackbar
shown
> to them?

Done.

Yes. Added a comment. If the finch config says we are showing the snackbar for
100MB and 1GB, but the user has already saved 1GB when installing M56 then this
will initialize the starting content length to the value that the user has saved
already (which would be greater than 1GB) and they would not see the snackbars.

https://codereview.chromium.org/2367403005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:154:
* Saves a shared pref indicating the content length when the last the snackbar
promo was
On 2016/10/13 01:03:11, dfalcantara (slow 10.24) wrote:
> the last the
> 
> You might as well just flesh out the javadoc for the key itself and then leave
> this as
> 
> /** See {@link #SHARED_PREF_DISPLAYED_SNACKBAR_PROMO_CONTENT_LENGTH}. */
> 
> That's let you avoid copy-pasting the same sentences everywhere.

Done.

https://codereview.chromium.org/2367403005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:166:
* has not been displayed before, returns -1,
On 2016/10/13 01:03:11, dfalcantara (slow 10.24) wrote:
> Maybe
> 
> /**
>  * Returns the content length from when the promo snackbar was last displayed.
>  *
>  * @return The content length, or -1 if the promo has not been displayed
before.
>  */

Done.

https://codereview.chromium.org/2367403005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:168:
* @return The content length when the last the snackbar promo was displayed.
On 2016/10/13 01:03:11, dfalcantara (slow 10.24) wrote:
> the last the

Done.

https://codereview.chromium.org/2367403005/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/snackbar/DataReductionPromoSnackbarController.java
(right):

https://codereview.chromium.org/2367403005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/DataReductionPromoSnackbarController.java:27:
// for.
On 2016/10/13 01:03:11, dfalcantara (slow 10.24) wrote:
> Sentence needs to be reworked, too.

Done.

https://codereview.chromium.org/2367403005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/DataReductionPromoSnackbarController.java:36:
private final int[] mPromoDataSavingsMB;
On 2016/10/13 01:03:11, dfalcantara (slow 10.24) wrote:
> nit: newline here

Done.

https://codereview.chromium.org/2367403005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/DataReductionPromoSnackbarController.java:92:
promoDataSavingMB * BYTES_IN_MB).replace(".0", "\u00A0");
On 2016/10/13 01:03:11, dfalcantara (slow 10.24) wrote:
> What's this unicode character for?

Removed. Fixed using the templated strings.

https://codereview.chromium.org/2367403005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/DataReductionPromoSnackbarController.java:95:
.concat(" ").concat(dataSavedString),
On 2016/10/13 01:03:11, dfalcantara (slow 10.24) wrote:
> You can't just concatenate these two strings because different languages will
> change the ordering of the words.  Look into one of the templated strings in
the
> GRD files; maybe IDS_DOWNLOAD_MANAGER_UI_SPACE_USED_GB would give you some
> ideas?

Ah thank you so much for this. Fixed.

https://codereview.chromium.org/2367403005/diff/40001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2367403005/diff/40001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:892: Chrome just saved
you
On 2016/10/13 01:03:11, dfalcantara (slow 10.24) wrote:
> See comment above about concatenating strings.

Done.

[commit-bot: I haz the power, 2016-10-25 01:03:09]

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

[commit-bot: I haz the power, 2016-10-25 01:03:10]

Dry run: This issue passed the CQ dry run.

[gone, 2016-10-25 17:17:06]

lgtm, just a nit

https://codereview.chromium.org/2367403005/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/snackbar/DataReductionPromoSnackbarController.java
(right):

https://codereview.chromium.org/2367403005/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/DataReductionPromoSnackbarController.java:90:
if (dataSavingInBytes > 0 && dataSavingInBytes >= promoDataSavingMB *
BYTES_PER_MEGABYTE
nit: Pull out promoDataSavingMB * BYTES_PER_MEGABYTE; it's repeated several
times in the block.

[megjablon, 2016-10-25 17:42:00]

https://codereview.chromium.org/2367403005/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/snackbar/DataReductionPromoSnackbarController.java
(right):

https://codereview.chromium.org/2367403005/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/DataReductionPromoSnackbarController.java:90:
if (dataSavingInBytes > 0 && dataSavingInBytes >= promoDataSavingMB *
BYTES_PER_MEGABYTE
On 2016/10/25 17:17:06, dfalcantara (slow 10.24) wrote:
> nit: Pull out promoDataSavingMB * BYTES_PER_MEGABYTE; it's repeated several
> times in the block.

Done.

[megjablon, 2016-10-25 17:44:48]

megjablon@chromium.org changed reviewers:
+ rkaplow@chromium.org, tbansal@chromium.org

[megjablon, 2016-10-25 17:44:49]

rkaplow: histograms.xml
tbansal: chrome/browser/* and components/*

[tbansal1, 2016-10-25 18:10:05]

https://codereview.chromium.org/2367403005/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/snackbar/DataReductionPromoSnackbarController.java
(right):

https://codereview.chromium.org/2367403005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/DataReductionPromoSnackbarController.java:91:
if (dataSavingInBytes > 0 && dataSavingInBytes >= promoDataSavingBytes
Would not this trigger if mPromoDataSavingsMB[i] was set to -1 in the try-catch
above?

https://codereview.chromium.org/2367403005/diff/140001/chrome/browser/net/spd...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc
(right):

https://codereview.chromium.org/2367403005/diff/140001/chrome/browser/net/spd...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:122:
return Settings()->GetHttpOriginalContentLength() -
If only saved bytes are needed, then is it possible to just expose that from
settings?

https://codereview.chromium.org/2367403005/diff/140001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc
(right):

https://codereview.chromium.org/2367403005/diff/140001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:156:
DCHECK(data_reduction_proxy_service_->compression_stats());
This DCHECK is unnecessary since if
data_reduction_proxy_service_->compression_stats() is null, then Chrome is
anyways going to crash on next line (both on release and debug builds).

Same comment on Line 163 below.

[megjablon, 2016-10-25 20:36:11]

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

[commit-bot: I haz the power, 2016-10-25 20:36:42]

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, 2016-10-25 21:38:04]

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

[commit-bot: I haz the power, 2016-10-25 21:38:05]

Dry run: This issue passed the CQ dry run.

[megjablon, 2016-10-25 22:14:19]

https://codereview.chromium.org/2367403005/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/snackbar/DataReductionPromoSnackbarController.java
(right):

https://codereview.chromium.org/2367403005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/DataReductionPromoSnackbarController.java:91:
if (dataSavingInBytes > 0 && dataSavingInBytes >= promoDataSavingBytes
On 2016/10/25 18:10:04, tbansal1 wrote:
> Would not this trigger if mPromoDataSavingsMB[i] was set to -1 in the
try-catch
> above?

Good catch! dataSavingInBytes > 0 was meant to be promoDataSavingMB > 0.

https://codereview.chromium.org/2367403005/diff/140001/chrome/browser/net/spd...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc
(right):

https://codereview.chromium.org/2367403005/diff/140001/chrome/browser/net/spd...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:122:
return Settings()->GetHttpOriginalContentLength() -
On 2016/10/25 18:10:04, tbansal1 wrote:
> If only saved bytes are needed, then is it possible to just expose that from
> settings?

I implemented it this way so it was clear what content length saved meant.
Changed so this is passed from settings though.

https://codereview.chromium.org/2367403005/diff/140001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc
(right):

https://codereview.chromium.org/2367403005/diff/140001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:156:
DCHECK(data_reduction_proxy_service_->compression_stats());
On 2016/10/25 18:10:04, tbansal1 wrote:
> This DCHECK is unnecessary since if
> data_reduction_proxy_service_->compression_stats() is null, then Chrome is
> anyways going to crash on next line (both on release and debug builds).
> 
> Same comment on Line 163 below.

Done.

[tbansal1, 2016-10-25 23:15:14]

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

https://codereview.chromium.org/2367403005/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:190:
public static void saveSnackbarPromoInitWithStartingContentLength(long
contentLength) {
Is the argument content length or saved bytes?

https://codereview.chromium.org/2367403005/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/snackbar/DataReductionPromoSnackbarController.java
(right):

https://codereview.chromium.org/2367403005/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/DataReductionPromoSnackbarController.java:31:
private static final String CLEAR_DATA_REDUCITON_PROXY_DATA_SAVINGS_SWITCH =
"REDUCITON"
typo

https://codereview.chromium.org/2367403005/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/DataReductionPromoSnackbarController.java:56:
variationParamValue = variationParamValue.replace(" ", "");
I wish most of this code was in C++ (except may be the one that actually shows
the snackbar and UI components), so it would be easier to port to other
platforms, and test it.

https://codereview.chromium.org/2367403005/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/DataReductionPromoSnackbarController.java:83:
if
(!DataReductionPromoUtils.hasSnackbarPromoBeenInitWithStartingContentLength()) {
IIUC, here we should be comparing the saved bytes in the pref (for existing
users)  with the contents in  mPromoDataSavingsMB. Is that correct?

[megjablon, 2016-10-25 23:49:57]

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

https://codereview.chromium.org/2367403005/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:190:
public static void saveSnackbarPromoInitWithStartingContentLength(long
contentLength) {
On 2016/10/25 23:15:13, tbansal1 wrote:
> Is the argument content length or saved bytes?

Done.

https://codereview.chromium.org/2367403005/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/snackbar/DataReductionPromoSnackbarController.java
(right):

https://codereview.chromium.org/2367403005/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/DataReductionPromoSnackbarController.java:31:
private static final String CLEAR_DATA_REDUCITON_PROXY_DATA_SAVINGS_SWITCH =
On 2016/10/25 23:15:13, tbansal1 wrote:
> "REDUCITON"
> typo

Done.

https://codereview.chromium.org/2367403005/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/DataReductionPromoSnackbarController.java:56:
variationParamValue = variationParamValue.replace(" ", "");
On 2016/10/25 23:15:13, tbansal1 wrote:
> I wish most of this code was in C++ (except may be the one that actually shows
> the snackbar and UI components), so it would be easier to port to other
> platforms, and test it.

I think right now it makes the most sense to have it in java, because we really
haven't been making any other efforts to promote on other platforms. Adding new
snackbars for other platforms will not be trivial and means this should probably
live in a new shared class for UI if we decide to do that, which we don't
currently have. I'd rather not move this over, given I do have test coverage for
it in java, but if you feel strongly I can.

https://codereview.chromium.org/2367403005/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/DataReductionPromoSnackbarController.java:83:
if
(!DataReductionPromoUtils.hasSnackbarPromoBeenInitWithStartingContentLength()) {
On 2016/10/25 23:15:13, tbansal1 wrote:
> IIUC, here we should be comparing the saved bytes in the pref (for existing
> users)  with the contents in  mPromoDataSavingsMB. Is that correct?

That is done below. Initializing the pref here prevents users that have already
saved X MB of data from seeing a promo for that amount. For example, if a user
has saved 1 GB, and we are showing the promo for 100 MB and 10 GB, then on
upgrade or first use this will set the pref to 1GB. The code below will then not
show the promo for 100 MB, but when the user reaches 10 GB of savings the promo
will show.

[tbansal1, 2016-10-26 00:09:30]

lgtm % nit.

https://codereview.chromium.org/2367403005/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/snackbar/DataReductionPromoSnackbarController.java
(right):

https://codereview.chromium.org/2367403005/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/DataReductionPromoSnackbarController.java:56:
variationParamValue = variationParamValue.replace(" ", "");
On 2016/10/25 23:49:57, megjablon wrote:
> On 2016/10/25 23:15:13, tbansal1 wrote:
> > I wish most of this code was in C++ (except may be the one that actually
shows
> > the snackbar and UI components), so it would be easier to port to other
> > platforms, and test it.
> 
> I think right now it makes the most sense to have it in java, because we
really
> haven't been making any other efforts to promote on other platforms. Adding
new
> snackbars for other platforms will not be trivial and means this should
probably
> live in a new shared class for UI if we decide to do that, which we don't
> currently have. I'd rather not move this over, given I do have test coverage
for
> it in java, but if you feel strongly I can.

Acknowledged.

https://codereview.chromium.org/2367403005/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java
(right):

https://codereview.chromium.org/2367403005/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:170:
* @return The content length, or -1 if the promo has not been displayed before.
nit: "The content length"
update the comment.

[megjablon, 2016-10-26 18:22:57]

https://codereview.chromium.org/2367403005/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java
(right):

https://codereview.chromium.org/2367403005/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:170:
* @return The content length, or -1 if the promo has not been displayed before.
On 2016/10/26 00:09:30, tbansal1 wrote:
> nit: "The content length"
> update the comment.

Done.

[megjablon, 2016-10-26 18:31:59]

megjablon@chromium.org changed reviewers:
- rkaplow@chromium.org

[megjablon, 2016-10-26 18:34:04]

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

[megjablon, 2016-10-26 18:34:47]

asvitkine: histograms.xml

[tbansal1, 2016-10-26 18:36:23]

still lgtm but one minor nit.

https://codereview.chromium.org/2367403005/diff/220001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java
(right):

https://codereview.chromium.org/2367403005/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:49:
* Key used to save the saved bytes when the snackbar promo was last shown.
One last nit: Can you update the comment to say that this pref also contains the
bytes saved for data saver users that had data saver turned on when this pref
was added. And, that doing that prevents us from showing promo to existing
users.

[Alexei Svitkine (slow), 2016-10-26 18:40:16]

https://codereview.chromium.org/2367403005/diff/220001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2367403005/diff/220001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:68702: +   
Sync.AuthServerRejectedTokenAgeLong     Records the origin of the icon used
Is this change intentional?

[megjablon, 2016-10-26 18:50:08]

https://codereview.chromium.org/2367403005/diff/220001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java
(right):

https://codereview.chromium.org/2367403005/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoUtils.java:49:
* Key used to save the saved bytes when the snackbar promo was last shown.
On 2016/10/26 18:36:23, tbansal1 wrote:
> One last nit: Can you update the comment to say that this pref also contains
the
> bytes saved for data saver users that had data saver turned on when this pref
> was added. And, that doing that prevents us from showing promo to existing
> users.

Done.

https://codereview.chromium.org/2367403005/diff/220001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2367403005/diff/220001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:68702: +   
Sync.AuthServerRejectedTokenAgeLong     Records the origin of the icon used
On 2016/10/26 18:40:16, Alexei Svitkine (slow) wrote:
> Is this change intentional?

Nope. Not sure how that ended up there. Removed.

[Alexei Svitkine (slow), 2016-10-26 18:56:50]

lgtm % comment

https://codereview.chromium.org/2367403005/diff/240001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2367403005/diff/240001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:8744: +<histogram
name="DataReductionProxy.SnackbarPromo" units="MB">
The name of this histogram doesn't indicate that it's logging the content
length.

I would change it to make that more clear - for example:
DataReductionProxy.SnackbarPromo.ContentLength

[megjablon, 2016-10-26 19:04:55]

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

[commit-bot: I haz the power, 2016-10-26 19:05:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[megjablon, 2016-10-26 19:34:59]

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

[megjablon, 2016-10-26 19:35:41]

https://codereview.chromium.org/2367403005/diff/240001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2367403005/diff/240001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:8744: +<histogram
name="DataReductionProxy.SnackbarPromo" units="MB">
On 2016/10/26 18:56:50, Alexei Svitkine (slow) wrote:
> The name of this histogram doesn't indicate that it's logging the content
> length.
> 
> I would change it to make that more clear - for example:
> DataReductionProxy.SnackbarPromo.ContentLength

Done.

[commit-bot: I haz the power, 2016-10-26 19:36:11]

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, 2016-10-26 19:42:41]

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

[commit-bot: I haz the power, 2016-10-26 19:42:42]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_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_...)

[megjablon, 2016-10-26 19:58:40]

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

[commit-bot: I haz the power, 2016-10-26 19:59:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[megjablon, 2016-10-26 21:12:13]

The CQ bit was unchecked by megjablon@chromium.org

[megjablon, 2016-10-26 21:12:13]

The CQ bit was checked by megjablon@chromium.org

[megjablon, 2016-10-26 21:12:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org, tbansal@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/2367403005/#ps280001
(title: "rename "data saving" to "data savings" for consistency ")

[commit-bot: I haz the power, 2016-10-26 21:12:54]

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-10-26 22:00:50]

Description was changed from

==========
Snackbar for promoting Data Saver to existing users

After loading a page show a snackbar that promotes Data Saver
according to the following rules and conditions:

1) The Chrome instance should be in the promo Finch-controlled
experimental group.

2) It will only show on HTTP pages.

3) That values that the promo will be shown are configured via
a Finch param value.

4) Trigger at 100MB. Trigger again at 1GB.

5) For existing users, if they haven't yet hit 100MB or 1GB,
they should get the reminder like everyone else. If they're
past 1GB, they won't see anything.

BUG=610765
==========

to

==========
Snackbar for promoting Data Saver to existing users

After loading a page show a snackbar that promotes Data Saver
according to the following rules and conditions:

1) The Chrome instance should be in the promo Finch-controlled
experimental group.

2) It will only show on HTTP pages.

3) That values that the promo will be shown are configured via
a Finch param value.

4) Trigger at 100MB. Trigger again at 1GB.

5) For existing users, if they haven't yet hit 100MB or 1GB,
they should get the reminder like everyone else. If they're
past 1GB, they won't see anything.

BUG=610765
==========

[commit-bot: I haz the power, 2016-10-26 22:00:51]

Committed patchset #13 (id:280001)

[commit-bot: I haz the power, 2016-10-26 22:03:34]

Description was changed from

==========
Snackbar for promoting Data Saver to existing users

After loading a page show a snackbar that promotes Data Saver
according to the following rules and conditions:

1) The Chrome instance should be in the promo Finch-controlled
experimental group.

2) It will only show on HTTP pages.

3) That values that the promo will be shown are configured via
a Finch param value.

4) Trigger at 100MB. Trigger again at 1GB.

5) For existing users, if they haven't yet hit 100MB or 1GB,
they should get the reminder like everyone else. If they're
past 1GB, they won't see anything.

BUG=610765
==========

to

==========
Snackbar for promoting Data Saver to existing users

After loading a page show a snackbar that promotes Data Saver
according to the following rules and conditions:

1) The Chrome instance should be in the promo Finch-controlled
experimental group.

2) It will only show on HTTP pages.

3) That values that the promo will be shown are configured via
a Finch param value.

4) Trigger at 100MB. Trigger again at 1GB.

5) For existing users, if they haven't yet hit 100MB or 1GB,
they should get the reminder like everyone else. If they're
past 1GB, they won't see anything.

BUG=610765

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

[commit-bot: I haz the power, 2016-10-26 22:03:36]

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