Data Saver promo changes

Data Saver promo changes

Tweaks to the Data Saver promo currently shown on Chrome
Android to improve the messaging and the call-to-action.
Adds a clickable link to the settings page and makes the
enable button, renamed to "Turn on data saver," directly
enable the proxy.

BUG=455267
Committed: https://crrev.com/d4f91a8bfe7b1f6f9c8eebaeaa470459e9493407
Cr-Commit-Position: refs/heads/master@{#322480}

[megjablon, 2015-03-19 18:14:30]

Patchset #2 (id:20001) has been deleted

[megjablon, 2015-03-19 18:58:37]

@bengr: The "optimize the pages you visit" link takes the user to the settings
page where they can learn more or enable Data Saver. "Turned on from promo" UMA
still reports if the user clicks this link and then enables. If the user
directly clicks the "Turn on Data Saver" button on the promo, I also now report
to the same UMA. Do we care to differentiate in UMA if the user enables Data
Saver directly from the Promo or in the settings page linked from promo?

[megjablon, 2015-03-19 18:58:47]

Patchset #1 (id:1) has been deleted

[bengr, 2015-03-20 19:39:28]

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

[bengr, 2015-03-20 19:39:29]

https://codereview.chromium.org/1011363006/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/bandwidth/DataReductionPromoScreen.java
(right):

https://codereview.chromium.org/1011363006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/bandwidth/DataReductionPromoScreen.java:117:
TextView title2 = (TextView) findViewById(R.id.data_reduction_title_2_link);
Can we use a more descriptive name than 'title2'?

https://codereview.chromium.org/1011363006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/bandwidth/DataReductionPromoScreen.java:132:
textPaint.setColor(Color.parseColor("#27b4e7"));
Why can't this be defined in the xml?

https://codereview.chromium.org/1011363006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/bandwidth/DataReductionPromoScreen.java:139:
new SpanInfo("<link>", "</link>", clickableLearnMoreSpan)));
Is this how links are done? again, I would expect such formatting stuff to be in
the xml or grd

[megjablon, 2015-03-24 19:46:20]

Patchset #2 (id:60001) has been deleted

[megjablon, 2015-03-24 19:48:32]

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

[megjablon, 2015-03-24 19:48:33]

asvitkine: histograms.xml
newt: chrome/*

https://codereview.chromium.org/1011363006/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/bandwidth/DataReductionPromoScreen.java
(right):

https://codereview.chromium.org/1011363006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/bandwidth/DataReductionPromoScreen.java:117:
TextView title2 = (TextView) findViewById(R.id.data_reduction_title_2_link);
On 2015/03/20 19:39:28, bengr wrote:
> Can we use a more descriptive name than 'title2'?

Done.

https://codereview.chromium.org/1011363006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/bandwidth/DataReductionPromoScreen.java:132:
textPaint.setColor(Color.parseColor("#27b4e7"));
On 2015/03/20 19:39:28, bengr wrote:
> Why can't this be defined in the xml?

Because it isn't a standalone link. This is the way you create links in text.

https://codereview.chromium.org/1011363006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/bandwidth/DataReductionPromoScreen.java:139:
new SpanInfo("<link>", "</link>", clickableLearnMoreSpan)));
On 2015/03/20 19:39:28, bengr wrote:
> Is this how links are done? again, I would expect such formatting stuff to be
in
> the xml or grd

See above.

[bengr, 2015-03-24 20:34:15]

lgtm

[bengr, 2015-03-24 20:34:42]

On 2015/03/24 20:34:15, bengr wrote:
> lgtm

Fix up the CL description.

[megjablon, 2015-03-24 20:37:28]

On 2015/03/24 20:34:42, bengr wrote:
> On 2015/03/24 20:34:15, bengr wrote:
> > lgtm
> 
> Fix up the CL description.

Done.

[newt (away), 2015-03-24 21:08:26]

lgtm after comments

https://codereview.chromium.org/1011363006/diff/80001/chrome/android/java/res...
File chrome/android/java/res/layout/data_reduction_promo_screen.xml (right):

https://codereview.chromium.org/1011363006/diff/80001/chrome/android/java/res...
chrome/android/java/res/layout/data_reduction_promo_screen.xml:63:
android:lineSpacingExtra="20dp"
use sp instead of dp so this scales when the user changes their system font
size. same below.

https://codereview.chromium.org/1011363006/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/bandwidth/DataReductionPromoScreen.java
(right):

https://codereview.chromium.org/1011363006/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/bandwidth/DataReductionPromoScreen.java:134:
textPaint.setColor(Color.parseColor("#27b4e7"));
Don't use Color.parseColor(). Instead:

    textPaint.setColor(textPaint.linkColor);

and in the XML file, you can set android:textColorLink="#27b4e7" on the
TextView.

[megjablon, 2015-03-24 21:23:11]

https://codereview.chromium.org/1011363006/diff/80001/chrome/android/java/res...
File chrome/android/java/res/layout/data_reduction_promo_screen.xml (right):

https://codereview.chromium.org/1011363006/diff/80001/chrome/android/java/res...
chrome/android/java/res/layout/data_reduction_promo_screen.xml:63:
android:lineSpacingExtra="20dp"
On 2015/03/24 21:08:26, newt wrote:
> use sp instead of dp so this scales when the user changes their system font
> size. same below.

Done.

https://codereview.chromium.org/1011363006/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/bandwidth/DataReductionPromoScreen.java
(right):

https://codereview.chromium.org/1011363006/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/bandwidth/DataReductionPromoScreen.java:134:
textPaint.setColor(Color.parseColor("#27b4e7"));
On 2015/03/24 21:08:26, newt wrote:
> Don't use Color.parseColor(). Instead:
> 
>     textPaint.setColor(textPaint.linkColor);
> 
> and in the XML file, you can set android:textColorLink="#27b4e7" on the
> TextView.

Done.

[megjablon, 2015-03-25 00:33:08]

On 2015/03/24 21:23:11, megjablon wrote:
>
https://codereview.chromium.org/1011363006/diff/80001/chrome/android/java/res...
> File chrome/android/java/res/layout/data_reduction_promo_screen.xml (right):
> 
>
https://codereview.chromium.org/1011363006/diff/80001/chrome/android/java/res...
> chrome/android/java/res/layout/data_reduction_promo_screen.xml:63:
> android:lineSpacingExtra="20dp"
> On 2015/03/24 21:08:26, newt wrote:
> > use sp instead of dp so this scales when the user changes their system font
> > size. same below.
> 
> Done.
> 
>
https://codereview.chromium.org/1011363006/diff/80001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/bandwidth/DataReductionPromoScreen.java
> (right):
> 
>
https://codereview.chromium.org/1011363006/diff/80001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/bandwidth/DataReductionPromoScreen.java:134:
> textPaint.setColor(Color.parseColor("#27b4e7"));
> On 2015/03/24 21:08:26, newt wrote:
> > Don't use Color.parseColor(). Instead:
> > 
> >     textPaint.setColor(textPaint.linkColor);
> > 
> > and in the XML file, you can set android:textColorLink="#27b4e7" on the
> > TextView.
> 
> Done.

@newt: Using android:textColorLink="#27b4e7" doesn't change the color of the
link
on my device if I am removing the underline using updateDrawState. Is there a
way
to do this in the xml?

[newt (away), 2015-03-25 00:40:43]

On 2015/03/25 00:33:08, megjablon wrote:
> On 2015/03/24 21:23:11, megjablon wrote:
> >
>
https://codereview.chromium.org/1011363006/diff/80001/chrome/android/java/res...
> > File chrome/android/java/res/layout/data_reduction_promo_screen.xml (right):
> > 
> >
>
https://codereview.chromium.org/1011363006/diff/80001/chrome/android/java/res...
> > chrome/android/java/res/layout/data_reduction_promo_screen.xml:63:
> > android:lineSpacingExtra="20dp"
> > On 2015/03/24 21:08:26, newt wrote:
> > > use sp instead of dp so this scales when the user changes their system
font
> > > size. same below.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1011363006/diff/80001/chrome/android/java/src...
> > File
> >
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/bandwidth/DataReductionPromoScreen.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/1011363006/diff/80001/chrome/android/java/src...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/bandwidth/DataReductionPromoScreen.java:134:
> > textPaint.setColor(Color.parseColor("#27b4e7"));
> > On 2015/03/24 21:08:26, newt wrote:
> > > Don't use Color.parseColor(). Instead:
> > > 
> > >     textPaint.setColor(textPaint.linkColor);
> > > 
> > > and in the XML file, you can set android:textColorLink="#27b4e7" on the
> > > TextView.
> > 
> > Done.
> 
> @newt: Using android:textColorLink="#27b4e7" doesn't change the color of the
> link
> on my device if I am removing the underline using updateDrawState. Is there a
> way
> to do this in the xml?

you need to call super.updateDrawState() or
textPaint.setColor(textPaint.linkColor);

[megjablon, 2015-03-26 18:08:56]

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

[megjablon, 2015-03-26 22:07:03]

The CQ bit was checked by megjablon@chromium.org

[megjablon, 2015-03-26 22:07:03]

The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org,
newt@chromium.org
Link to the patchset: https://codereview.chromium.org/1011363006/#ps120001
(title: "Removing uma and link color fix")

[commit-bot: I haz the power, 2015-03-26 22:07:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1011363006/120001

[commit-bot: I haz the power, 2015-03-26 22:18:32]

Committed patchset #4 (id:120001)

[commit-bot: I haz the power, 2015-03-26 22:19:31]

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