Add icons to redesigned CBD dialog.

Add icons to redesigned CBD dialog. 

Dialog with icons:
https://screenshot.googleplex.com/GQ0LeamyMkN.png

Dialog for phones with low DP (< 360dp):
https://screenshot.googleplex.com/CwuaYHVgdiF.png
(truncated spinner text still needs to be fixed)

BUG=681523
Review-Url: https://codereview.chromium.org/2652983002
Cr-Commit-Position: refs/heads/master@{#455241}
Committed: https://chromium.googlesource.com/chromium/src/+/50c18ca66848657416f8f39451ae8bdfaa0da824

[dullweber, 2017-01-24 17:19:29]

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

[commit-bot: I haz the power, 2017-01-24 17:19:43]

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-01-24 18:13:26]

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

[commit-bot: I haz the power, 2017-01-24 18:13:27]

Dry run: This issue passed the CQ dry run.

[dullweber, 2017-03-01 08:57:04]

Description was changed from

==========
Add icons to redesigned CBD dialog.

TODO: replace icon placeholders

BUG=681523
==========

to

==========
Add icons to redesigned CBD dialog. Add icons 

Dialog with icons:
https://screenshot.googleplex.com/GQ0LeamyMkN.png

Dialog for phones with low DP (< 360dp):
https://screenshot.googleplex.com/GQ0LeamyMkN.png
(truncated spinner text still needs to be fixed)

BUG=681523
==========

[dullweber, 2017-03-01 08:57:43]

Description was changed from

==========
Add icons to redesigned CBD dialog. Add icons 

Dialog with icons:
https://screenshot.googleplex.com/GQ0LeamyMkN.png

Dialog for phones with low DP (< 360dp):
https://screenshot.googleplex.com/GQ0LeamyMkN.png
(truncated spinner text still needs to be fixed)

BUG=681523
==========

to

==========
Add icons to redesigned CBD dialog. 

Dialog with icons:
https://screenshot.googleplex.com/GQ0LeamyMkN.png

Dialog for phones with low DP (< 360dp):
https://screenshot.googleplex.com/GQ0LeamyMkN.png
(truncated spinner text still needs to be fixed)

BUG=681523
==========

[dullweber, 2017-03-01 08:58:47]

dullweber@chromium.org changed reviewers:
+ msramek@chromium.org

[dullweber, 2017-03-01 08:58:47]

Hi Martin, please have a look at this cl

[dullweber, 2017-03-01 09:03:30]

Description was changed from

==========
Add icons to redesigned CBD dialog. 

Dialog with icons:
https://screenshot.googleplex.com/GQ0LeamyMkN.png

Dialog for phones with low DP (< 360dp):
https://screenshot.googleplex.com/GQ0LeamyMkN.png
(truncated spinner text still needs to be fixed)

BUG=681523
==========

to

==========
Add icons to redesigned CBD dialog. 

Dialog with icons:
https://screenshot.googleplex.com/GQ0LeamyMkN.png

Dialog for phones with low DP (< 360dp):
https://screenshot.googleplex.com/CwuaYHVgdiF
(truncated spinner text still needs to be fixed)

BUG=681523
==========

[dullweber, 2017-03-01 09:03:43]

Description was changed from

==========
Add icons to redesigned CBD dialog. 

Dialog with icons:
https://screenshot.googleplex.com/GQ0LeamyMkN.png

Dialog for phones with low DP (< 360dp):
https://screenshot.googleplex.com/CwuaYHVgdiF
(truncated spinner text still needs to be fixed)

BUG=681523
==========

to

==========
Add icons to redesigned CBD dialog. 

Dialog with icons:
https://screenshot.googleplex.com/GQ0LeamyMkN.png

Dialog for phones with low DP (< 360dp):
https://screenshot.googleplex.com/CwuaYHVgdiF.png
(truncated spinner text still needs to be fixed)

BUG=681523
==========

[msramek, 2017-03-01 15:03:15]

library_music.png looks slightly bigger than the rest in each of the five
collections. It looks correct on your screenshot though, but could you please
double-check on a different resolution as well? (Maybe try on a tablet?)

When we chatted about the file names yesterday, I admit I was quite ambiguous,
but now that I look at it, I'm definitely in favor of renaming. If the filename
described what's in the picture, that would not be a problem, but the filename
describes the purpose of the picture which is different than what we use it for.
That can lead to a lot of confusion when someone else reuses the file and
possibly changes it later. Can you please rename the pictures to match our
datatypes? The existing permission_cookie file can stay as is, since there it's
literally the same thing.

After that, LGTM!

https://codereview.chromium.org/2652983002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java
(right):

https://codereview.chromium.org/2652983002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:200:
public int getIcon() {
nit: Doc for public functions please.

[dullweber, 2017-03-02 09:02:24]

On 2017/03/01 15:03:15, msramek wrote:
> library_music.png looks slightly bigger than the rest in each of the five
> collections. It looks correct on your screenshot though, but could you please
> double-check on a different resolution as well? (Maybe try on a tablet?)
> 
> When we chatted about the file names yesterday, I admit I was quite ambiguous,
> but now that I look at it, I'm definitely in favor of renaming. If the
filename
> described what's in the picture, that would not be a problem, but the filename
> describes the purpose of the picture which is different than what we use it
for.
> That can lead to a lot of confusion when someone else reuses the file and
> possibly changes it later. Can you please rename the pictures to match our
> datatypes? The existing permission_cookie file can stay as is, since there
it's
> literally the same thing.
> 
> After that, LGTM!
> 
>
https://codereview.chromium.org/2652983002/diff/20001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java
> (right):
> 
>
https://codereview.chromium.org/2652983002/diff/20001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:200:
> public int getIcon() {
> nit: Doc for public functions please.


Yes, the library_music file is too big. But it is the icon for media licenses
and therefor not used yet. That's why it didn't show up wrong on the screenshot.

I removed it for now and I will add it again when the option will actually be
there.

I renamed the files.

[dullweber, 2017-03-02 09:04:09]

dullweber@chromium.org changed reviewers:
+ twellington@chromium.org

[dullweber, 2017-03-02 09:04:09]

twellington@chromium.org: Please review these changes

[dullweber, 2017-03-02 10:11:14]

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

[commit-bot: I haz the power, 2017-03-02 10:11:39]

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-03-02 10:44:56]

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

[commit-bot: I haz the power, 2017-03-02 10:44:57]

Dry run: This issue passed the CQ dry run.

[Theresa, 2017-03-02 16:17:48]

form_data.png looks like bookmark_edit_normal.png - can we use the existing
asset and rename it?

For naming in general, these look like material icons so it would be ideal to
name them something like ic_collections_grey600_24dp.png (for example; not sure
if these are 24dp or36dp) so that it's easy for others to find them in the
future. We've shifted away from naming assets by the feature they're used in and
toward more standard naming (we have yet to formalize this but we're working on
it). Ping me or email me if you want help finding the material icon names.

Also, please run tools/resources/optimize-png-files.sh on each new png if you
haven't already.

https://codereview.chromium.org/2652983002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java
(right):

https://codereview.chromium.org/2652983002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:172:
// TODO(dullweber): Cookie icon has a slightly wrong grey.
When you're ready to address this TODO, this can be fixed by tinting the asset
-- checkout TintedImageView.

[dullweber, 2017-03-02 16:58:51]

On 2017/03/02 16:17:48, Theresa wrote:
> form_data.png looks like bookmark_edit_normal.png - can we use the existing
> asset and rename it?
> 
> For naming in general, these look like material icons so it would be ideal to
> name them something like ic_collections_grey600_24dp.png (for example; not
sure
> if these are 24dp or36dp) so that it's easy for others to find them in the
> future. We've shifted away from naming assets by the feature they're used in
and
> toward more standard naming (we have yet to formalize this but we're working
on
> it). Ping me or email me if you want help finding the material icon names.
> 
> Also, please run tools/resources/optimize-png-files.sh on each new png if you
> haven't already.
> 
>
https://codereview.chromium.org/2652983002/diff/40001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java
> (right):
> 
>
https://codereview.chromium.org/2652983002/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:172:
> // TODO(dullweber): Cookie icon has a slightly wrong grey.
> When you're ready to address this TODO, this can be fixed by tinting the asset
> -- checkout TintedImageView.

Yes, these are material design icons, I got them directly from icons/. I will
change them back to their original name. 

bookmark_edit_normal.png is nearly the same icon as form_data.png, it is the
same resolution but a little bit smaller and also has a darker grey. Is it ok if
I add the new one or should I try to use the existing one? 

I can try to use the TintedImageView for the cookies icon, otherwise I would
wait for Max to get an icon with the right color. I couldn't find it on icons/

Thanks for the suggestion with optimize-png-files.sh. I will run this on every
file.

[Theresa, 2017-03-02 17:14:59]

On 2017/03/02 16:58:51, dullweber wrote:
> On 2017/03/02 16:17:48, Theresa wrote:
> > form_data.png looks like bookmark_edit_normal.png - can we use the existing
> > asset and rename it?
> > 
> > For naming in general, these look like material icons so it would be ideal
to
> > name them something like ic_collections_grey600_24dp.png (for example; not
> sure
> > if these are 24dp or36dp) so that it's easy for others to find them in the
> > future. We've shifted away from naming assets by the feature they're used in
> and
> > toward more standard naming (we have yet to formalize this but we're working
> on
> > it). Ping me or email me if you want help finding the material icon names.
> > 
> > Also, please run tools/resources/optimize-png-files.sh on each new png if
you
> > haven't already.
> > 
> >
>
https://codereview.chromium.org/2652983002/diff/40001/chrome/android/java/src...
> > File
> >
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/2652983002/diff/40001/chrome/android/java/src...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:172:
> > // TODO(dullweber): Cookie icon has a slightly wrong grey.
> > When you're ready to address this TODO, this can be fixed by tinting the
asset
> > -- checkout TintedImageView.
> 
> Yes, these are material design icons, I got them directly from icons/. I will
> change them back to their original name. 
> 
> bookmark_edit_normal.png is nearly the same icon as form_data.png, it is the
> same resolution but a little bit smaller and also has a darker grey. Is it ok
if
> I add the new one or should I try to use the existing one? 
> 
> I can try to use the TintedImageView for the cookies icon, otherwise I would
> wait for Max to get an icon with the right color. I couldn't find it on icons/
> 
> Thanks for the suggestion with optimize-png-files.sh. I will run this on every
> file.

Each asset we add increases our APK size so we strongly prefer to re-use assets
when possible. It looks like bookmark_edit_normal and form_data are both 36px x
36px, so I think the intention is that they are the same asset. I'd be okay with
the bookmark edit matching the material form_data asset. They should also
ideally be the same shade of grey. We have an ongoing effort to reduce the 126*
shades of grey we use in Chrome. Can you please check with UX to see what color
they'd like to converge on?

* this is stat may be slightly out of date but it's a close approximation.

[dullweber, 2017-03-06 16:08:24]

On 2017/03/02 17:14:59, Theresa wrote:
> On 2017/03/02 16:58:51, dullweber wrote:
> > On 2017/03/02 16:17:48, Theresa wrote:
> > > form_data.png looks like bookmark_edit_normal.png - can we use the
existing
> > > asset and rename it?
> > > 
> > > For naming in general, these look like material icons so it would be ideal
> to
> > > name them something like ic_collections_grey600_24dp.png (for example; not
> > sure
> > > if these are 24dp or36dp) so that it's easy for others to find them in the
> > > future. We've shifted away from naming assets by the feature they're used
in
> > and
> > > toward more standard naming (we have yet to formalize this but we're
working
> > on
> > > it). Ping me or email me if you want help finding the material icon names.
> > > 
> > > Also, please run tools/resources/optimize-png-files.sh on each new png if
> you
> > > haven't already.
> > > 
> > >
> >
>
https://codereview.chromium.org/2652983002/diff/40001/chrome/android/java/src...
> > > File
> > >
> >
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2652983002/diff/40001/chrome/android/java/src...
> > >
> >
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:172:
> > > // TODO(dullweber): Cookie icon has a slightly wrong grey.
> > > When you're ready to address this TODO, this can be fixed by tinting the
> asset
> > > -- checkout TintedImageView.
> > 
> > Yes, these are material design icons, I got them directly from icons/. I
will
> > change them back to their original name. 
> > 
> > bookmark_edit_normal.png is nearly the same icon as form_data.png, it is the
> > same resolution but a little bit smaller and also has a darker grey. Is it
ok
> if
> > I add the new one or should I try to use the existing one? 
> > 
> > I can try to use the TintedImageView for the cookies icon, otherwise I would
> > wait for Max to get an icon with the right color. I couldn't find it on
icons/
> > 
> > Thanks for the suggestion with optimize-png-files.sh. I will run this on
every
> > file.
> 
> Each asset we add increases our APK size so we strongly prefer to re-use
assets
> when possible. It looks like bookmark_edit_normal and form_data are both 36px
x
> 36px, so I think the intention is that they are the same asset. I'd be okay
with
> the bookmark edit matching the material form_data asset. They should also
> ideally be the same shade of grey. We have an ongoing effort to reduce the
126*
> shades of grey we use in Chrome. Can you please check with UX to see what
color
> they'd like to converge on?
> 
> * this is stat may be slightly out of date but it's a close approximation.

I changed the files back to their original name and optimized them. The -r
switch for a revision didn't work for me, so I added an option to directly
specify git commits to optimize-png-files.sh. I tried to use the number from
Cr-Commit-Position, I don't know if I did something wrong or it just doesn't
work anymore?

I looked at TintedImageView and it looks like a good solution, but I have no
idea what would be the best approach to replace the default PreferenceImageView
with it? The relevant Layout seems to be this one:
https://cs.chromium.org/chromium/src/third_party/android_tools/sdk/platforms/...
I could make a copy and replace the construction of it but I don't know if that
is the right way to do this?

[Theresa, 2017-03-06 16:27:18]

On 2017/03/06 16:08:24, dullweber wrote:
> I changed the files back to their original name and optimized them. The -r
> switch for a revision didn't work for me, so I added an option to directly
> specify git commits to optimize-png-files.sh. I tried to use the number from
> Cr-Commit-Position, I don't know if I did something wrong or it just doesn't
> work anymore?

I've never tried to use that parameter before, so I'm not sure. I always do:
$  tools/resources/optimize-png-files.sh <filename>

> I looked at TintedImageView and it looks like a good solution, but I have no
> idea what would be the best approach to replace the default
PreferenceImageView
> with it? The relevant Layout seems to be this one:
>
https://cs.chromium.org/chromium/src/third_party/android_tools/sdk/platforms/...
> I could make a copy and replace the construction of it but I don't know if
that
> is the right way to do this?

While it is possible to set custom layouts for preferences, I think that it
would be easier to use the Preference#setIcon(Drawable icon) method Instead
(sorry, my initial suggestion was a bit off). You can construct and store a
tinted drawable and use that in the call to mCheckbox.setIcon().

To construct a TintedDrawable, look at
TintedDrawable#constructTintedDrawable(Resources res, int drawableId, int
tintColorId):
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

[dullweber, 2017-03-06 16:29:16]

On 2017/03/06 16:27:18, Theresa wrote:
> While it is possible to set custom layouts for preferences, I think that it
> would be easier to use the Preference#setIcon(Drawable icon) method Instead
> (sorry, my initial suggestion was a bit off). You can construct and store a
> tinted drawable and use that in the call to mCheckbox.setIcon().
> 
> To construct a TintedDrawable, look at
> TintedDrawable#constructTintedDrawable(Resources res, int drawableId, int
> tintColorId):
>
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

That sounds much better, thanks :)

[dullweber, 2017-03-07 09:57:13]

I use tinted images for the icons in other colors now.
I also removed form_data.png and replaced it with the existing
bookmark_edit_normal.png

[Theresa, 2017-03-07 16:14:39]

lgtm

[dullweber, 2017-03-07 17:00:33]

dullweber@chromium.org changed reviewers:
+ scottmg@chromium.org

[dullweber, 2017-03-07 17:00:34]

scottmg@chromium.org: Please review the changes in
tools/resources/optimize-png-files.sh.

The -r parameter to select a revision didn't work for me. "git svn find-rev
r$OPTARG" never returns when I pass the number in Cr-Commit-Position. Am I doing
this right?
I added an option to pass a git commit directly to fix this.

[scottmg, 2017-03-07 17:12:52]

tools rs lgtm

[Theresa, 2017-03-07 21:13:14]

The CQ bit was checked by twellington@chromium.org

[Theresa, 2017-03-07 21:13:15]

The patchset sent to the CQ was uploaded after l-g-t-m from msramek@chromium.org
Link to the patchset: https://codereview.chromium.org/2652983002/#ps80001
(title: "use tinting for cookies and form_data icon")

[commit-bot: I haz the power, 2017-03-07 21:14:22]

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-03-07 22:09:45]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488921194885250,
"parent_rev": "d8f77d07a1ef0935866b363b1bd0370954d3a3b8", "commit_rev":
"50c18ca66848657416f8f39451ae8bdfaa0da824"}

[commit-bot: I haz the power, 2017-03-07 22:10:26]

Description was changed from

==========
Add icons to redesigned CBD dialog. 

Dialog with icons:
https://screenshot.googleplex.com/GQ0LeamyMkN.png

Dialog for phones with low DP (< 360dp):
https://screenshot.googleplex.com/CwuaYHVgdiF.png
(truncated spinner text still needs to be fixed)

BUG=681523
==========

to

==========
Add icons to redesigned CBD dialog. 

Dialog with icons:
https://screenshot.googleplex.com/GQ0LeamyMkN.png

Dialog for phones with low DP (< 360dp):
https://screenshot.googleplex.com/CwuaYHVgdiF.png
(truncated spinner text still needs to be fixed)

BUG=681523

Review-Url: https://codereview.chromium.org/2652983002
Cr-Commit-Position: refs/heads/master@{#455241}
Committed:
https://chromium.googlesource.com/chromium/src/+/50c18ca66848657416f8f39451ae...
==========

[commit-bot: I haz the power, 2017-03-07 22:10:27]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/50c18ca66848657416f8f39451ae...