Use larger vector graphics for special tiles and fix selection ring.

Use larger vector graphics for special tiles and fix selection ring.

BUG=656015
Review-Url: https://codereview.chromium.org/2843973002
Cr-Commit-Position: refs/heads/master@{#469635}
Committed: https://chromium.googlesource.com/chromium/src/+/403845af284556f00645e2790618722fb99f9d34

[Finnur, 2017-04-26 15:51:54]

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

[commit-bot: I haz the power, 2017-04-26 15:52:12]

Dry run: CQ is trying da patch.

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

[Finnur, 2017-04-26 15:54:15]

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

[Finnur, 2017-04-26 15:54:15]

This addresses some of the UI concerns Chris had.

Specifically, regarding the size of the special tile images, selection ring and
the blue checkmark image (redundand outline and missing drop shadow, etc).

[commit-bot: I haz the power, 2017-04-26 15:55:51]

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

[commit-bot: I haz the power, 2017-04-26 15:55:52]

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...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)

[Theresa, 2017-04-26 18:04:31]

https://codereview.chromium.org/2843973002/diff/1/chrome/android/java/res/dra...
File chrome/android/java/res/drawable/ic_collections_grey600_48dp.xml (right):

https://codereview.chromium.org/2843973002/diff/1/chrome/android/java/res/dra...
chrome/android/java/res/drawable/ic_collections_grey600_48dp.xml:5: 
We already have an ic_collections_grey.xml that's grey_google_grey_600. Can we
share?

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

https://codereview.chromium.org/2843973002/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/photo_picker_bitmap_view.xml:41:
android:background="@drawable/checkmark_blue"
We don't gain any APK size savings since the verify_checkmark.png is still used
other places. What do we gain by making this a vector drawable?

Same question about ic_photo_camera.png.

[Finnur, 2017-04-26 20:17:05]

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

[commit-bot: I haz the power, 2017-04-26 20:18:08]

Dry run: CQ is trying da patch.

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

[Finnur, 2017-04-26 20:30:45]

https://codereview.chromium.org/2843973002/diff/1/chrome/android/java/res/dra...
File chrome/android/java/res/drawable/ic_collections_grey600_48dp.xml (right):

https://codereview.chromium.org/2843973002/diff/1/chrome/android/java/res/dra...
chrome/android/java/res/drawable/ic_collections_grey600_48dp.xml:5: 
The impetus for this change is larger icons for the special tiles. Really dumb
question: Is this as simple as using ic_collections_grey.xml and doubling the
size specified in xml? If I were at my desk I'd try, but I'm not (and I'd like
to get one round of discussion in before your EOD). :)

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

https://codereview.chromium.org/2843973002/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/photo_picker_bitmap_view.xml:41:
android:background="@drawable/checkmark_blue"
The impetus for this change is not efficiency. There was a PNG supplied by the
designer and, following your suggestion, I switched to use the verify_checkmark
drawable. The designer balked at the results, because that change made the
checkmark grow a white border outline, that it didn't use to have, and loose the
dropshadow. Hence the vector xml.

[Finnur, 2017-04-26 20:31:26]

Forgot to mention (although it is obvious) that there's no code change, just
replying for a round of comments.

[Theresa, 2017-04-26 20:47:17]

https://codereview.chromium.org/2843973002/diff/1/chrome/android/java/res/dra...
File chrome/android/java/res/drawable/ic_collections_grey600_48dp.xml (right):

https://codereview.chromium.org/2843973002/diff/1/chrome/android/java/res/dra...
chrome/android/java/res/drawable/ic_collections_grey600_48dp.xml:5: 
On 2017/04/26 20:30:44, Finnur wrote:
> The impetus for this change is larger icons for the special tiles. Really dumb
> question: Is this as simple as using ic_collections_grey.xml and doubling the
> size specified in xml? If I were at my desk I'd try, but I'm not (and I'd like
> to get one round of discussion in before your EOD). :)

You should be able to display ic_collections_grey.xml in an Image View height &
width = 48dp and scaleType = center (or maybe center_crop).

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

https://codereview.chromium.org/2843973002/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/photo_picker_bitmap_view.xml:41:
android:background="@drawable/checkmark_blue"
On 2017/04/26 20:30:44, Finnur wrote:
> The impetus for this change is not efficiency. There was a PNG supplied by the
> designer and, following your suggestion, I switched to use the
verify_checkmark
> drawable. The designer balked at the results, because that change made the
> checkmark grow a white border outline, that it didn't use to have, and loose
the
> dropshadow. Hence the vector xml.

I didn't realize that the spec had a blue check w/out a white border. It's fine
to introduce a new asset if that's the case.

I'm still wondering about ic_photo_camera, though. Does that have properties not
captured in the PNG. In general, we are trying really hard to avoid one-off
designs and solutions and that includes assets that are displayed.

[commit-bot: I haz the power, 2017-04-26 21:30:54]

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

[commit-bot: I haz the power, 2017-04-26 21:30:55]

Dry run: This issue passed the CQ dry run.

[Finnur, 2017-04-27 15:02:26]

https://codereview.chromium.org/2843973002/diff/1/chrome/android/java/res/dra...
File chrome/android/java/res/drawable/ic_collections_grey600_48dp.xml (right):

https://codereview.chromium.org/2843973002/diff/1/chrome/android/java/res/dra...
chrome/android/java/res/drawable/ic_collections_grey600_48dp.xml:5: 
When I did this initially, I used an ImageView (for the Collections icon) and a
TextView (for the Gallery label). This errored out in a pre-submit script with a
note saying I should use a TextView with an image drawable instead.

Can you advise how I can satisfy both the review comment (use an ImageView) and
the pre-submit script (use a TextView)?

For fun, I tried sizing the TextView, but that just increases the size of the
view and doesn't scale up the drawable.

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

https://codereview.chromium.org/2843973002/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/photo_picker_bitmap_view.xml:41:
android:background="@drawable/checkmark_blue"
I'll probably have an answer when I see your feedback on the other question.

[Theresa, 2017-04-27 15:45:28]

https://codereview.chromium.org/2843973002/diff/1/chrome/android/java/res/dra...
File chrome/android/java/res/drawable/ic_collections_grey600_48dp.xml (right):

https://codereview.chromium.org/2843973002/diff/1/chrome/android/java/res/dra...
chrome/android/java/res/drawable/ic_collections_grey600_48dp.xml:5: 
On 2017/04/27 15:02:26, Finnur wrote:
> When I did this initially, I used an ImageView (for the Collections icon) and
a
> TextView (for the Gallery label). This errored out in a pre-submit script with
a
> note saying I should use a TextView with an image drawable instead.
> 
> Can you advise how I can satisfy both the review comment (use an ImageView)
and
> the pre-submit script (use a TextView)?
> 
> For fun, I tried sizing the TextView, but that just increases the size of the
> view and doesn't scale up the drawable.

Try calling setBounds() on the vector drawable.

If that doesn't work, then change ic_collections_grey.xml and check that the
other use looks correct. The one other use is in settings. I *think* the
ImageView probably already has properties set to scale correctly, but if not we
can try setting properties on the ImageView directly or bounds on that drawable.

If we can't get it to work, I'll acquiesce, but the whole point of vectors is
crisp scaling so it seems like a shame to have different XML files for different
sizes.

[Theresa, 2017-04-27 15:49:49]

https://codereview.chromium.org/2843973002/diff/1/chrome/android/java/res/dra...
File chrome/android/java/res/drawable/ic_collections_grey600_48dp.xml (right):

https://codereview.chromium.org/2843973002/diff/1/chrome/android/java/res/dra...
chrome/android/java/res/drawable/ic_collections_grey600_48dp.xml:5: 
On 2017/04/27 15:45:27, Theresa wrote:
> On 2017/04/27 15:02:26, Finnur wrote:
> > When I did this initially, I used an ImageView (for the Collections icon)
and
> a
> > TextView (for the Gallery label). This errored out in a pre-submit script
with
> a
> > note saying I should use a TextView with an image drawable instead.
> > 
> > Can you advise how I can satisfy both the review comment (use an ImageView)
> and
> > the pre-submit script (use a TextView)?
> > 
> > For fun, I tried sizing the TextView, but that just increases the size of
the
> > view and doesn't scale up the drawable.
> 
> Try calling setBounds() on the vector drawable.
> 
> If that doesn't work, then change ic_collections_grey.xml and check that the
> other use looks correct. The one other use is in settings. I *think* the
> ImageView probably already has properties set to scale correctly, but if not
we
> can try setting properties on the ImageView directly or bounds on that
drawable.
> 
> If we can't get it to work, I'll acquiesce, but the whole point of vectors is
> crisp scaling so it seems like a shame to have different XML files for
different
> sizes.

Also, what was the exact error. We may be able to suppress it and add a comment
explaining the suppression.

[Finnur, 2017-05-04 14:57:09]

Patchset #3 (id:40001) has been deleted

[Finnur, 2017-05-04 14:57:40]

Patchset #3 (id:60001) has been deleted

[Finnur, 2017-05-04 15:00:54]

Patchset #3 (id:80001) has been deleted

[Finnur, 2017-05-04 15:03:44]

Patchset #3 (id:100001) has been deleted

[Finnur, 2017-05-04 15:09:13]

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

[commit-bot: I haz the power, 2017-05-04 15:09:31]

Dry run: CQ is trying da patch.

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

[Finnur, 2017-05-04 15:16:32]

PTAL.

SetBounds does not seem to increase the size of the image inside the compound
drawable.

I tried changing the pre-existing xml to be 48dp, as you suggested, hoping the
other codepath would do the right thing. Unfortunately, the image in Settings
then increased in size and any scaling down to 24dp, that I tried, was ignored
when it came to setting the icon.

Switching to an ImageView, however, does allow me to scale the image properly
before showing it, so I've removed the extra xml files.

[Theresa, 2017-05-04 15:27:14]

Looks good overall, thanks! Just a couple of minor comments.

https://codereview.chromium.org/2843973002/diff/120001/chrome/android/java/re...
File chrome/android/java/res/layout/photo_picker_bitmap_view.xml (right):

https://codereview.chromium.org/2843973002/diff/120001/chrome/android/java/re...
chrome/android/java/res/layout/photo_picker_bitmap_view.xml:35: <View
I think this and the View below should be ImageViews unless that doesn't work
for some reason.

https://codereview.chromium.org/2843973002/diff/120001/chrome/android/java/re...
File chrome/android/java/res/values/colors.xml (right):

https://codereview.chromium.org/2843973002/diff/120001/chrome/android/java/re...
chrome/android/java/res/values/colors.xml:32: <color
name="white_alpha_40">#64FFFFFF</color>
Is white_alpha_50 close enough (can we avoid adding another shade of white)?

[Finnur, 2017-05-04 15:47:23]

Done. PTAL.

If this looks OK feel free to check the CQ box. Thanks!

[Theresa, 2017-05-04 15:57:24]

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

[Theresa, 2017-05-04 15:57:24]

lgtm

+mikecase for build/android OWNERS review

[Finnur, 2017-05-04 16:05:59]

Thanks!

Mike, if this looks OK, feel free to check the CQ box! :)

[mikecase (-- gone --), 2017-05-05 01:13:50]

The CQ bit was checked by mikecase@chromium.org

[mikecase (-- gone --), 2017-05-05 01:13:51]

lgtm

[commit-bot: I haz the power, 2017-05-05 01:14:28]

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-05-05 03:05:20]

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

[commit-bot: I haz the power, 2017-05-05 03:05:21]

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...)

[Finnur, 2017-05-05 10:21:57]

Theresa, I've moved a couple of lines to avoid the DeadStore report. 

I don't think this is controversial so I'm checking in, but happy to address any
concerns in a follow-up.

[Finnur, 2017-05-05 10:22:03]

The CQ bit was checked by finnur@chromium.org

[Finnur, 2017-05-05 10:22:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
twellington@chromium.org, mikecase@chromium.org
Link to the patchset: https://codereview.chromium.org/2843973002/#ps160001
(title: "Fix unused variable issue")

[commit-bot: I haz the power, 2017-05-05 10:22:19]

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-05-05 12:19:00]

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

[commit-bot: I haz the power, 2017-05-05 12:19:01]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Finnur, 2017-05-05 12:21:35]

The CQ bit was checked by finnur@chromium.org

[commit-bot: I haz the power, 2017-05-05 12:21:48]

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-05-05 13:16:22]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1493986895958250,
"parent_rev": "9924ab42f638a24c64ec0dc9c450bf6bbf294e3a", "commit_rev":
"403845af284556f00645e2790618722fb99f9d34"}

[commit-bot: I haz the power, 2017-05-05 13:16:33]

Description was changed from

==========
Use larger vector graphics for special tiles and fix selection ring.

BUG=656015
==========

to

==========
Use larger vector graphics for special tiles and fix selection ring.

BUG=656015

Review-Url: https://codereview.chromium.org/2843973002
Cr-Commit-Position: refs/heads/master@{#469635}
Committed:
https://chromium.googlesource.com/chromium/src/+/403845af284556f00645e2790618...
==========

[commit-bot: I haz the power, 2017-05-05 13:16:34]

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