Add icon to the Data Saver main menu footer and update to spec

Add icon to the Data Saver main menu footer and update to spec

Add icon to the Data Saver footer, update the strings to the new spec,
and add a divider.

BUG=702799
Review-Url: https://codereview.chromium.org/2795653006
Cr-Commit-Position: refs/heads/master@{#463350}
Committed: https://chromium.googlesource.com/chromium/src/+/35113f638165a3c3e79b10b397684ad93c539315

[megjablon, 2017-04-04 23:16:17]

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

[megjablon, 2017-04-04 23:16:18]

PTAL, thanks!

[Ted C, 2017-04-05 17:32:13]

I was thinking that we could split the icon into two separate things (one being
the peak-y graph section that we could tint either gray or blue), then the mask
with the lines and highlight and low light.  I don't know if it would save us
anything though (might be worth asking Bruno to see for a quick comparison).

Also, did you run these through the png crunching tools?

https://codereview.chromium.org/2795653006/diff/1/chrome/android/java/res/lay...
File chrome/android/java/res/layout/data_reduction_main_menu_footer.xml (right):

https://codereview.chromium.org/2795653006/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/data_reduction_main_menu_footer.xml:13: <View
style="@style/Divider"/>
Instead of the outer view being a linearlayout, can it be a framelayout?

I wonder if this layout could be flatten a bit to something like:

<FrameLayout
   width=match_parent
   height=wrap_content>
   <Divider>
   <Image
       layout_marginTop="divider_height"
       height=match_parent
       width=<explicit width in dp>
       gravity/center-ing bits />
   <LinearLayout
       layout_marginStart="image width in dp"
       layout_marginTop="divider_height"
       height=wrap_content
       width=match_parent>
       <Text-y things>
   </LinearLayout>
</FrameLayout>

I think the layout system should be able to handle the alignment we are looking
without the nested layouts...i think.  But oh have I been wrong in the past.

[megjablon, 2017-04-06 22:00:34]

Patchset #2 (id:20001) has been deleted

[megjablon, 2017-04-06 22:01:34]

Got new assets that split up the background and chart. I ran them through the
png tool this time too :)

https://codereview.chromium.org/2795653006/diff/1/chrome/android/java/res/lay...
File chrome/android/java/res/layout/data_reduction_main_menu_footer.xml (right):

https://codereview.chromium.org/2795653006/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/data_reduction_main_menu_footer.xml:13: <View
style="@style/Divider"/>
On 2017/04/05 17:32:13, Ted C wrote:
> Instead of the outer view being a linearlayout, can it be a framelayout?
> 
> I wonder if this layout could be flatten a bit to something like:
> 
> <FrameLayout
>    width=match_parent
>    height=wrap_content>
>    <Divider>
>    <Image
>        layout_marginTop="divider_height"
>        height=match_parent
>        width=<explicit width in dp>
>        gravity/center-ing bits />
>    <LinearLayout
>        layout_marginStart="image width in dp"
>        layout_marginTop="divider_height"
>        height=wrap_content
>        width=match_parent>
>        <Text-y things>
>    </LinearLayout>
> </FrameLayout>
> 
> I think the layout system should be able to handle the alignment we are
looking
> without the nested layouts...i think.  But oh have I been wrong in the past.

I played around with this a little, but I'd like to keep all the content in a
layout that has the AppMenuItem style so that if the padding changes the item
stays aligned with those in the menu. The divider needs to go above that so even
if I switch layout types, I would want another layout to wrap all the content in
below the divider. If you're not worried about keeping the AppMenuItem style
then I can do this.

[Ted C, 2017-04-07 18:19:10]

It looks like the graph portion of the image is still the same size as the grid.
 I was thinking it would be cropped to the exact size and we could bottom align
it in the layer list declaration.

https://codereview.chromium.org/2795653006/diff/40001/chrome/android/java/res...
File chrome/android/java/res/layout/data_reduction_main_menu_footer.xml (right):

https://codereview.chromium.org/2795653006/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/data_reduction_main_menu_footer.xml:24:
style="@style/AppMenuItem" >
I think even if we want to keep AppMenuItem, we should still make
DataReductionMainMenuFooter and this LinearLayout into FrameLayouts.

Here you would set a marginTop to 1dp which is the divider height.  We could
pull out a size dimen for this and use it in styles.xml for Divider and allow it
to be reused here.

Then if we just change layout_width of the image view to be a specific value,
set the marginStart of the inner linearlayout to match, you should be able to
reduce from 3 nested LinearLayouts to having just one as the child.  The number
of layout passes required to do that should be fewer and it shouldn't make it
too hard to grok IMO.

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

https://codereview.chromium.org/2795653006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionMainMenuFooter.java:75:
icon.setColorFilter(new ColorMatrixColorFilter(matrix));
from TintedDrawable, we use:

setColorFilter(<color>, PorterDuff.Mode.SRC_IN);

I'm wondering if we could do that purely for consistency (where we tint to a
particular color).

Ahhh...it looks like the highlight is still on the graph portion.  I was
thinking that would be in the grid overlay (a semi-transparent white line) that
would be plopped on top.  Without getting new assets, I suspect this is indeed
the right thing to do without using a specific color to change.

[megjablon, 2017-04-07 20:03:26]

https://codereview.chromium.org/2795653006/diff/40001/chrome/android/java/res...
File chrome/android/java/res/layout/data_reduction_main_menu_footer.xml (right):

https://codereview.chromium.org/2795653006/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/data_reduction_main_menu_footer.xml:24:
style="@style/AppMenuItem" >
On 2017/04/07 18:19:10, Ted C wrote:
> I think even if we want to keep AppMenuItem, we should still make
> DataReductionMainMenuFooter and this LinearLayout into FrameLayouts.
> 
> Here you would set a marginTop to 1dp which is the divider height.  We could
> pull out a size dimen for this and use it in styles.xml for Divider and allow
it
> to be reused here.
> 
> Then if we just change layout_width of the image view to be a specific value,
> set the marginStart of the inner linearlayout to match, you should be able to
> reduce from 3 nested LinearLayouts to having just one as the child.  The
number
> of layout passes required to do that should be fewer and it shouldn't make it
> too hard to grok IMO.

Done. PTAL to make sure I understood correctly.

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

https://codereview.chromium.org/2795653006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionMainMenuFooter.java:75:
icon.setColorFilter(new ColorMatrixColorFilter(matrix));
On 2017/04/07 18:19:10, Ted C wrote:
> from TintedDrawable, we use:
> 
> setColorFilter(<color>, PorterDuff.Mode.SRC_IN);
> 
> I'm wondering if we could do that purely for consistency (where we tint to a
> particular color).
> 
> Ahhh...it looks like the highlight is still on the graph portion.  I was
> thinking that would be in the grid overlay (a semi-transparent white line)
that
> would be plopped on top.  Without getting new assets, I suspect this is indeed
> the right thing to do without using a specific color to change.

Are we ok with keeping this as is or should I try to get different assets?

[Ted C, 2017-04-07 20:22:29]

On 2017/04/07 20:03:26, megjablon wrote:
>
https://codereview.chromium.org/2795653006/diff/40001/chrome/android/java/res...
> File chrome/android/java/res/layout/data_reduction_main_menu_footer.xml
(right):
> 
>
https://codereview.chromium.org/2795653006/diff/40001/chrome/android/java/res...
> chrome/android/java/res/layout/data_reduction_main_menu_footer.xml:24:
> style="@style/AppMenuItem" >
> On 2017/04/07 18:19:10, Ted C wrote:
> > I think even if we want to keep AppMenuItem, we should still make
> > DataReductionMainMenuFooter and this LinearLayout into FrameLayouts.
> > 
> > Here you would set a marginTop to 1dp which is the divider height.  We could
> > pull out a size dimen for this and use it in styles.xml for Divider and
allow
> it
> > to be reused here.
> > 
> > Then if we just change layout_width of the image view to be a specific
value,
> > set the marginStart of the inner linearlayout to match, you should be able
to
> > reduce from 3 nested LinearLayouts to having just one as the child.  The
> number
> > of layout passes required to do that should be fewer and it shouldn't make
it
> > too hard to grok IMO.
> 
> Done. PTAL to make sure I understood correctly.
> 
>
https://codereview.chromium.org/2795653006/diff/40001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionMainMenuFooter.java
> (right):
> 
>
https://codereview.chromium.org/2795653006/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionMainMenuFooter.java:75:
> icon.setColorFilter(new ColorMatrixColorFilter(matrix));
> On 2017/04/07 18:19:10, Ted C wrote:
> > from TintedDrawable, we use:
> > 
> > setColorFilter(<color>, PorterDuff.Mode.SRC_IN);
> > 
> > I'm wondering if we could do that purely for consistency (where we tint to a
> > particular color).
> > 
> > Ahhh...it looks like the highlight is still on the graph portion.  I was
> > thinking that would be in the grid overlay (a semi-transparent white line)
> that
> > would be plopped on top.  Without getting new assets, I suspect this is
indeed
> > the right thing to do without using a specific color to change.
> 
> Are we ok with keeping this as is or should I try to get different assets?

We are more closely tracking asset size changes, so the main thing I would
suggest trying is just trimming the largest graph asset to drop the empty space
at the top.  Then compare the sizes against it and the previous version.  If
it does get smaller, then we should trim them all of the empty region.

I wouldn't necessarily work on getting new ones that split out the highlight
stroke into the overlay yet as that could be done later and would require
involvement from UX (I would just use gimp locally to zealous autocrop the
png myself, crush it again, and compare sizes).

BTW...lgtm with the current code.  I think a quick test on the image size and
potentially an update there is all that is left.

[megjablon, 2017-04-08 00:36:15]

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

[commit-bot: I haz the power, 2017-04-08 00:36:46]

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

[megjablon, 2017-04-08 00:43:14]

On 2017/04/07 20:22:29, Ted C wrote:
> On 2017/04/07 20:03:26, megjablon wrote:
> >
>
https://codereview.chromium.org/2795653006/diff/40001/chrome/android/java/res...
> > File chrome/android/java/res/layout/data_reduction_main_menu_footer.xml
> (right):
> > 
> >
>
https://codereview.chromium.org/2795653006/diff/40001/chrome/android/java/res...
> > chrome/android/java/res/layout/data_reduction_main_menu_footer.xml:24:
> > style="@style/AppMenuItem" >
> > On 2017/04/07 18:19:10, Ted C wrote:
> > > I think even if we want to keep AppMenuItem, we should still make
> > > DataReductionMainMenuFooter and this LinearLayout into FrameLayouts.
> > > 
> > > Here you would set a marginTop to 1dp which is the divider height.  We
could
> > > pull out a size dimen for this and use it in styles.xml for Divider and
> allow
> > it
> > > to be reused here.
> > > 
> > > Then if we just change layout_width of the image view to be a specific
> value,
> > > set the marginStart of the inner linearlayout to match, you should be able
> to
> > > reduce from 3 nested LinearLayouts to having just one as the child.  The
> > number
> > > of layout passes required to do that should be fewer and it shouldn't make
> it
> > > too hard to grok IMO.
> > 
> > Done. PTAL to make sure I understood correctly.
> > 
> >
>
https://codereview.chromium.org/2795653006/diff/40001/chrome/android/java/src...
> > File
> >
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionMainMenuFooter.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/2795653006/diff/40001/chrome/android/java/src...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionMainMenuFooter.java:75:
> > icon.setColorFilter(new ColorMatrixColorFilter(matrix));
> > On 2017/04/07 18:19:10, Ted C wrote:
> > > from TintedDrawable, we use:
> > > 
> > > setColorFilter(<color>, PorterDuff.Mode.SRC_IN);
> > > 
> > > I'm wondering if we could do that purely for consistency (where we tint to
a
> > > particular color).
> > > 
> > > Ahhh...it looks like the highlight is still on the graph portion.  I was
> > > thinking that would be in the grid overlay (a semi-transparent white line)
> > that
> > > would be plopped on top.  Without getting new assets, I suspect this is
> indeed
> > > the right thing to do without using a specific color to change.
> > 
> > Are we ok with keeping this as is or should I try to get different assets?
> 
> We are more closely tracking asset size changes, so the main thing I would
> suggest trying is just trimming the largest graph asset to drop the empty
space
> at the top.  Then compare the sizes against it and the previous version.  If
> it does get smaller, then we should trim them all of the empty region.
> 
> I wouldn't necessarily work on getting new ones that split out the highlight
> stroke into the overlay yet as that could be done later and would require
> involvement from UX (I would just use gimp locally to zealous autocrop the
> png myself, crush it again, and compare sizes).
> 
> BTW...lgtm with the current code.  I think a quick test on the image size and
> potentially an update there is all that is left.

It didn't save a ton, but it saved a few bytes by cropping them. The layer-list
stretched the graph and using a height attribute didn't apply to it on my Ice
Cream Sandwich device, so I used a padding at the top of the item.

[commit-bot: I haz the power, 2017-04-08 01:35:18]

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

[commit-bot: I haz the power, 2017-04-08 01:35:19]

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

[megjablon, 2017-04-08 01:55:39]

Patchset #4 (id:80001) has been deleted

[megjablon, 2017-04-08 01:55:49]

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

[megjablon, 2017-04-08 01:56:03]

Patchset #4 (id:100001) has been deleted

[commit-bot: I haz the power, 2017-04-08 01:56:12]

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

[megjablon, 2017-04-08 01:56:58]

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

[commit-bot: I haz the power, 2017-04-08 01:57:23]

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

[megjablon, 2017-04-08 02:06:31]

Patchset #4 (id:120001) has been deleted

[megjablon, 2017-04-08 02:13:09]

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

[commit-bot: I haz the power, 2017-04-08 02:13:33]

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

[commit-bot: I haz the power, 2017-04-08 02:50:31]

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

[commit-bot: I haz the power, 2017-04-08 02:50:32]

Dry run: This issue passed the CQ dry run.

[Ted C, 2017-04-10 16:48:48]

https://codereview.chromium.org/2795653006/diff/140001/chrome/android/java/re...
File chrome/android/java/res/drawable/data_reduction_main_menu_icon.xml (right):

https://codereview.chromium.org/2795653006/diff/140001/chrome/android/java/re...
chrome/android/java/res/drawable/data_reduction_main_menu_icon.xml:10:
android:layout_gravity="bottom"
I "think" this would need to be android:gravity="bottom", but I don't believe
you need this.

https://codereview.chromium.org/2795653006/diff/140001/chrome/android/java/re...
chrome/android/java/res/drawable/data_reduction_main_menu_icon.xml:11:
android:top="8dp" />
if you did android:bottom="0dp", would that work instead of giving a top offset?

[megjablon, 2017-04-10 18:00:35]

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

[megjablon, 2017-04-10 18:00:42]

https://codereview.chromium.org/2795653006/diff/140001/chrome/android/java/re...
File chrome/android/java/res/drawable/data_reduction_main_menu_icon.xml (right):

https://codereview.chromium.org/2795653006/diff/140001/chrome/android/java/re...
chrome/android/java/res/drawable/data_reduction_main_menu_icon.xml:10:
android:layout_gravity="bottom"
On 2017/04/10 16:48:48, Ted C wrote:
> I "think" this would need to be android:gravity="bottom", but I don't believe
> you need this.

See below

https://codereview.chromium.org/2795653006/diff/140001/chrome/android/java/re...
chrome/android/java/res/drawable/data_reduction_main_menu_icon.xml:11:
android:top="8dp" />
On 2017/04/10 16:48:48, Ted C wrote:
> if you did android:bottom="0dp", would that work instead of giving a top
offset?

Setting bottom offset doesn't work. The layer-list stretches the item to fill
the space, so this still allows the graph to be drawn to the top.

Just found this in the developer guide:

"All drawable items are scaled to fit the size of the containing View, by
default. Thus, placing your images in a layer list at different positions might
increase the size of the View and some images scale as appropriate. To avoid
scaling items in the list, use a <bitmap> element inside the <item> element to
specify the drawable and define the gravity to something that does not scale,
such as "center"."

https://developer.android.com/guide/topics/resources/drawable-resource.html#L...

So I moved it into a bitmap.

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

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

[Ted C, 2017-04-10 18:09:20]

On 2017/04/10 18:00:42, megjablon wrote:
>
https://codereview.chromium.org/2795653006/diff/140001/chrome/android/java/re...
> File chrome/android/java/res/drawable/data_reduction_main_menu_icon.xml
(right):
> 
>
https://codereview.chromium.org/2795653006/diff/140001/chrome/android/java/re...
> chrome/android/java/res/drawable/data_reduction_main_menu_icon.xml:10:
> android:layout_gravity="bottom"
> On 2017/04/10 16:48:48, Ted C wrote:
> > I "think" this would need to be android:gravity="bottom", but I don't
believe
> > you need this.
> 
> See below
> 
>
https://codereview.chromium.org/2795653006/diff/140001/chrome/android/java/re...
> chrome/android/java/res/drawable/data_reduction_main_menu_icon.xml:11:
> android:top="8dp" />
> On 2017/04/10 16:48:48, Ted C wrote:
> > if you did android:bottom="0dp", would that work instead of giving a top
> offset?
> 
> Setting bottom offset doesn't work. The layer-list stretches the item to fill
> the space, so this still allows the graph to be drawn to the top.
> 
> Just found this in the developer guide:
> 
> "All drawable items are scaled to fit the size of the containing View, by
> default. Thus, placing your images in a layer list at different positions
might
> increase the size of the View and some images scale as appropriate. To avoid
> scaling items in the list, use a <bitmap> element inside the <item> element to
> specify the drawable and define the gravity to something that does not scale,
> such as "center"."
> 
>
https://developer.android.com/guide/topics/resources/drawable-resource.html#L...
> 
> So I moved it into a bitmap.

cool... still lgtm

[commit-bot: I haz the power, 2017-04-10 18:57:59]

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

[commit-bot: I haz the power, 2017-04-10 18:58:00]

Dry run: This issue passed the CQ dry run.

[megjablon, 2017-04-10 18:58:53]

The CQ bit was checked by megjablon@chromium.org

[commit-bot: I haz the power, 2017-04-10 18:59:44]

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

[commit-bot: I haz the power, 2017-04-10 19:11:33]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1491850733229820,
"parent_rev": "a4392f1f23606ecc67b9de879c4cade685227163", "commit_rev":
"35113f638165a3c3e79b10b397684ad93c539315"}

[commit-bot: I haz the power, 2017-04-10 19:12:24]

Description was changed from

==========
Add icon to the Data Saver main menu footer and update to spec

Add icon to the Data Saver footer, update the strings to the new spec,
and add a divider.

BUG=702799
==========

to

==========
Add icon to the Data Saver main menu footer and update to spec

Add icon to the Data Saver footer, update the strings to the new spec,
and add a divider.

BUG=702799

Review-Url: https://codereview.chromium.org/2795653006
Cr-Commit-Position: refs/heads/master@{#463350}
Committed:
https://chromium.googlesource.com/chromium/src/+/35113f638165a3c3e79b10b39768...
==========

[commit-bot: I haz the power, 2017-04-10 19:12:25]

Committed patchset #5 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/35113f638165a3c3e79b10b39768...