Clean up tinted ImageView subclasses

Clean up tinted ImageView subclasses

* Introduce an ImageViewTinter that can be used by the TintedImageView
  and the TintedImageButton classes.  They're basically the same thing
  already, but because Java doesn't support extending multiple classes
  we're stuck in this hole.

* Introduce basic tests for the ImageViewTinter users.

* Make the AppMenuItemIcon a TintedImageButton so that it can be tinted
  if needed.  Also make it implement Checkable because that's basically
  what it's already doing.

* The share icon was implemented in a different way than RDS.  Make RDS
  follow what the share icon does because it's probably easier to maintain
  in the end, meaning that it now uses a AppMenuItemIcon.

* Make the RDS checkbox be tinted appropriately gray or blue to match the
  top icon row.  This is necessary because Android themeing is inconsistent
  across KK and MM: the unchecked checkbox is straight black on KK but gray
  on MM, while the checked checkbox is straight black on KK and blue on MM.

BUG=571445
Review-Url: https://codereview.chromium.org/2951133003
Cr-Commit-Position: refs/heads/master@{#482063}
Committed: https://chromium.googlesource.com/chromium/src/+/65bc4a46d509009cc15dc2ae8e316a1e215fc50b

[gone, 2017-06-21 23:13:06]

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

[commit-bot: I haz the power, 2017-06-21 23:13:19]

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-06-22 00:41:12]

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

[commit-bot: I haz the power, 2017-06-22 00:41:13]

Dry run: This issue passed the CQ dry run.

[gone, 2017-06-22 19:06:32]

Description was changed from

==========
Clean up tinted ImageView subclasses

* Introduce an ImageViewTinter that can be used by the TintedImageView
  and the TintedImageButton classes.  They're basically the same thing
  already, but because Java doesn't support extending multiple classes
  we're stuck in this hole.

* Introduce basic tests for the ImageViewTinter subclasses.

* The share icon was implemented in a different way than RDS.  Make RDS
  follow what the share icon does because it's probably easier to maintain
  in the end, meaning that it now uses a AppMenuItemIcon.

* Make the AppMenuItemIcon a TintedImageButton so that it can be tinted
  if needed.  Also make it implement Checkable because that's basically
  what it's already doing.

* Make the RDS checkbox be tinted appropriately gray or blue to match the
  top icon row.  This is necessary because Android themeing is inconsistent
  across KK and MM: the unchecked checkbox is straight black on KK but gray
  on MM, while the checked checkbox is straight black on KK and blue on MM.

BUG=571445
==========

to

==========
Clean up tinted ImageView subclasses

* Introduce an ImageViewTinter that can be used by the TintedImageView
  and the TintedImageButton classes.  They're basically the same thing
  already, but because Java doesn't support extending multiple classes
  we're stuck in this hole.

* Introduce basic tests for the ImageViewTinter users.

* The share icon was implemented in a different way than RDS.  Make RDS
  follow what the share icon does because it's probably easier to maintain
  in the end, meaning that it now uses a AppMenuItemIcon.

* Make the AppMenuItemIcon a TintedImageButton so that it can be tinted
  if needed.  Also make it implement Checkable because that's basically
  what it's already doing.

* Make the RDS checkbox be tinted appropriately gray or blue to match the
  top icon row.  This is necessary because Android themeing is inconsistent
  across KK and MM: the unchecked checkbox is straight black on KK but gray
  on MM, while the checked checkbox is straight black on KK and blue on MM.

BUG=571445
==========

[gone, 2017-06-22 19:07:44]

Description was changed from

==========
Clean up tinted ImageView subclasses

* Introduce an ImageViewTinter that can be used by the TintedImageView
  and the TintedImageButton classes.  They're basically the same thing
  already, but because Java doesn't support extending multiple classes
  we're stuck in this hole.

* Introduce basic tests for the ImageViewTinter users.

* The share icon was implemented in a different way than RDS.  Make RDS
  follow what the share icon does because it's probably easier to maintain
  in the end, meaning that it now uses a AppMenuItemIcon.

* Make the AppMenuItemIcon a TintedImageButton so that it can be tinted
  if needed.  Also make it implement Checkable because that's basically
  what it's already doing.

* Make the RDS checkbox be tinted appropriately gray or blue to match the
  top icon row.  This is necessary because Android themeing is inconsistent
  across KK and MM: the unchecked checkbox is straight black on KK but gray
  on MM, while the checked checkbox is straight black on KK and blue on MM.

BUG=571445
==========

to

==========
Clean up tinted ImageView subclasses

* Introduce an ImageViewTinter that can be used by the TintedImageView
  and the TintedImageButton classes.  They're basically the same thing
  already, but because Java doesn't support extending multiple classes
  we're stuck in this hole.

* Introduce basic tests for the ImageViewTinter users.

* Make the AppMenuItemIcon a TintedImageButton so that it can be tinted
  if needed.  Also make it implement Checkable because that's basically
  what it's already doing.

* The share icon was implemented in a different way than RDS.  Make RDS
  follow what the share icon does because it's probably easier to maintain
  in the end, meaning that it now uses a AppMenuItemIcon.

* Make the RDS checkbox be tinted appropriately gray or blue to match the
  top icon row.  This is necessary because Android themeing is inconsistent
  across KK and MM: the unchecked checkbox is straight black on KK but gray
  on MM, while the checked checkbox is straight black on KK and blue on MM.

BUG=571445
==========

[gone, 2017-06-22 19:07:44]

Description was changed from

==========
Clean up tinted ImageView subclasses

* Introduce an ImageViewTinter that can be used by the TintedImageView
  and the TintedImageButton classes.  They're basically the same thing
  already, but because Java doesn't support extending multiple classes
  we're stuck in this hole.

* Introduce basic tests for the ImageViewTinter users.

* The share icon was implemented in a different way than RDS.  Make RDS
  follow what the share icon does because it's probably easier to maintain
  in the end, meaning that it now uses a AppMenuItemIcon.

* Make the AppMenuItemIcon a TintedImageButton so that it can be tinted
  if needed.  Also make it implement Checkable because that's basically
  what it's already doing.

* Make the RDS checkbox be tinted appropriately gray or blue to match the
  top icon row.  This is necessary because Android themeing is inconsistent
  across KK and MM: the unchecked checkbox is straight black on KK but gray
  on MM, while the checked checkbox is straight black on KK and blue on MM.

BUG=571445
==========

to

==========
Clean up tinted ImageView subclasses

* Introduce an ImageViewTinter that can be used by the TintedImageView
  and the TintedImageButton classes.  They're basically the same thing
  already, but because Java doesn't support extending multiple classes
  we're stuck in this hole.

* Introduce basic tests for the ImageViewTinter users.

* Make the AppMenuItemIcon a TintedImageButton so that it can be tinted
  if needed.  Also make it implement Checkable because that's basically
  what it's already doing.

* The share icon was implemented in a different way than RDS.  Make RDS
  follow what the share icon does because it's probably easier to maintain
  in the end, meaning that it now uses a AppMenuItemIcon.

* Make the RDS checkbox be tinted appropriately gray or blue to match the
  top icon row.  This is necessary because Android themeing is inconsistent
  across KK and MM: the unchecked checkbox is straight black on KK but gray
  on MM, while the checked checkbox is straight black on KK and blue on MM.

BUG=571445
==========

[gone, 2017-06-22 19:08:32]

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

[gone, 2017-06-22 19:08:33]

This ended up a little larger than I'd intended, but a lot of it was refactoring
and adding a new test.  If you want me to try and split it up I can give it a
go.

[gone, 2017-06-22 20:59:08]

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

[gone, 2017-06-22 20:59:25]

Take 2.  PTAL.

[commit-bot: I haz the power, 2017-06-22 20:59:27]

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-06-22 22:36:14]

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

[commit-bot: I haz the power, 2017-06-22 22:36:15]

Dry run: This issue passed the CQ dry run.

[Theresa, 2017-06-23 00:42:32]

https://codereview.chromium.org/2951133003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuAdapter.java
(right):

https://codereview.chromium.org/2951133003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuAdapter.java:302:
button.getResources(), R.color.blue_mode_tint));
We should be able to use one color state list for the tinting. Something like
this in XML:

<selector xmlns:android="http://schemas.android.com/apk/res/android">
    <item android:state_checked="true" android:color="@color/light_active_color"
/>
    <item android:color="@color/light_normal_color"/>
</selector>

https://codereview.chromium.org/2951133003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuItemIcon.java
(left):

https://codereview.chromium.org/2951133003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuItemIcon.java:34:
// highlighted.
I suspect that we do this to help distinguish touch targets e.g. on the
"Share..." menu item.

https://codereview.chromium.org/2951133003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/ImageViewTinter.java
(right):

https://codereview.chromium.org/2951133003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/ImageViewTinter.java:26:
* The default style used by the Application will likely cause your Drawable to
be automatically
What is our default tint set to?

https://codereview.chromium.org/2951133003/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/widget/ImageViewTinterTest.java
(right):

https://codereview.chromium.org/2951133003/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/widget/ImageViewTinterTest.java:115:
}
super nit picky nit: Can the tests either be grouped by the pre-fix or post-fix
e.g.

testTintedImageView_attributeParsingExplicitTint
testTintedImageView_attributeParsingNullTint
testTintedImageView_setTint
testTintedImageButton_attributeParsingExplicitTint
testTintedImageButton_attributeParsingNullTint
testTintedImageButton_setTint

OR

testTintedImageView_attributeParsingExplicitTint
testTintedImageButton_attributeParsingExplicitTint
testTintedImageView_attributeParsingNullTint
testTintedImageButton_attributeParsingNullTint
testTintedImageView_setTint
testTintedImageButton_setTint

I have a slight preference for the second.

https://codereview.chromium.org/2951133003/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/widget/ImageViewTinterTest.java:176:
passed |= expectedColor == bitmap.getPixel(x, y);
Can we return early once we've found a matching pixel?

[gone, 2017-06-23 17:45:01]

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

[gone, 2017-06-23 17:45:08]

https://codereview.chromium.org/2951133003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuAdapter.java
(right):

https://codereview.chromium.org/2951133003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuAdapter.java:302:
button.getResources(), R.color.blue_mode_tint));
On 2017/06/23 00:42:31, Theresa wrote:
> We should be able to use one color state list for the tinting. Something like
> this in XML:
> 
> <selector xmlns:android="http://schemas.android.com/apk/res/android">
>     <item android:state_checked="true"
android:color="@color/light_active_color"
> />
>     <item android:color="@color/light_normal_color"/>
> </selector>

Done.

https://codereview.chromium.org/2951133003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuItemIcon.java
(left):

https://codereview.chromium.org/2951133003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuItemIcon.java:34:
// highlighted.
On 2017/06/23 00:42:31, Theresa wrote:
> I suspect that we do this to help distinguish touch targets e.g. on the
> "Share..." menu item.

Doesn't seem to help any.  I think this was to prevent double highlighting of
the share icon touch target and the share row (which includes the share title),
but the share row doesn't get highlighted.

https://codereview.chromium.org/2951133003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/ImageViewTinter.java
(right):

https://codereview.chromium.org/2951133003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/ImageViewTinter.java:26:
* The default style used by the Application will likely cause your Drawable to
be automatically
On 2017/06/23 00:42:31, Theresa wrote:
> What is our default tint set to?

It's in styles.xml, under MainTheme.  We automatically use dark_mode_tint, which
was making the update menu item's icon entirely dark gray.

https://codereview.chromium.org/2951133003/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/widget/ImageViewTinterTest.java
(right):

https://codereview.chromium.org/2951133003/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/widget/ImageViewTinterTest.java:115:
}
On 2017/06/23 00:42:31, Theresa wrote:
> super nit picky nit: Can the tests either be grouped by the pre-fix or
post-fix
> e.g.
> 
> testTintedImageView_attributeParsingExplicitTint
> testTintedImageView_attributeParsingNullTint
> testTintedImageView_setTint
> testTintedImageButton_attributeParsingExplicitTint
> testTintedImageButton_attributeParsingNullTint
> testTintedImageButton_setTint
> 
> OR
> 
> testTintedImageView_attributeParsingExplicitTint
> testTintedImageButton_attributeParsingExplicitTint
> testTintedImageView_attributeParsingNullTint
> testTintedImageButton_attributeParsingNullTint
> testTintedImageView_setTint
> testTintedImageButton_setTint
> 
> I have a slight preference for the second.

Welp, this is what happens when you add tests randomly.  Done.

https://codereview.chromium.org/2951133003/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/widget/ImageViewTinterTest.java:176:
passed |= expectedColor == bitmap.getPixel(x, y);
On 2017/06/23 00:42:31, Theresa wrote:
> Can we return early once we've found a matching pixel?

Yep, done.

[commit-bot: I haz the power, 2017-06-23 17:45:14]

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-06-23 19:09:22]

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

[commit-bot: I haz the power, 2017-06-23 19:09:22]

Dry run: This issue passed the CQ dry run.

[Theresa, 2017-06-23 20:46:57]

lgtm % nit on color state XML

https://codereview.chromium.org/2951133003/diff/80001/chrome/android/java/res...
File chrome/android/java/res/color/checkbox_tint.xml (right):

https://codereview.chromium.org/2951133003/diff/80001/chrome/android/java/res...
chrome/android/java/res/color/checkbox_tint.xml:7: <item
android:state_checked="true" android:color="@color/blue_mode_tint" />
nit: is there a benefit to using blue_mode_tint instead of
@color/light_active_color?

[gone, 2017-06-23 21:19:08]

https://codereview.chromium.org/2951133003/diff/80001/chrome/android/java/res...
File chrome/android/java/res/color/checkbox_tint.xml (right):

https://codereview.chromium.org/2951133003/diff/80001/chrome/android/java/res...
chrome/android/java/res/color/checkbox_tint.xml:7: <item
android:state_checked="true" android:color="@color/blue_mode_tint" />
On 2017/06/23 20:46:56, Theresa wrote:
> nit: is there a benefit to using blue_mode_tint instead of
> @color/light_active_color?

Nada.  Fixed.

[gone, 2017-06-23 21:20:59]

The CQ bit was checked by dfalcantara@chromium.org

[gone, 2017-06-23 21:21:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
twellington@chromium.org
Link to the patchset: https://codereview.chromium.org/2951133003/#ps100001
(title: "Comments")

[commit-bot: I haz the power, 2017-06-23 21:21:21]

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-06-23 22:15:38]

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

[commit-bot: I haz the power, 2017-06-23 22:15:39]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[gone, 2017-06-23 22:25:22]

The CQ bit was checked by dfalcantara@chromium.org

[commit-bot: I haz the power, 2017-06-23 22:25: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-06-23 23:11:47]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1498256722381370,
"parent_rev": "de93e30d4aa72f1c1a5c88e2aafba0021697b351", "commit_rev":
"65bc4a46d509009cc15dc2ae8e316a1e215fc50b"}

[commit-bot: I haz the power, 2017-06-23 23:12:03]

Description was changed from

==========
Clean up tinted ImageView subclasses

* Introduce an ImageViewTinter that can be used by the TintedImageView
  and the TintedImageButton classes.  They're basically the same thing
  already, but because Java doesn't support extending multiple classes
  we're stuck in this hole.

* Introduce basic tests for the ImageViewTinter users.

* Make the AppMenuItemIcon a TintedImageButton so that it can be tinted
  if needed.  Also make it implement Checkable because that's basically
  what it's already doing.

* The share icon was implemented in a different way than RDS.  Make RDS
  follow what the share icon does because it's probably easier to maintain
  in the end, meaning that it now uses a AppMenuItemIcon.

* Make the RDS checkbox be tinted appropriately gray or blue to match the
  top icon row.  This is necessary because Android themeing is inconsistent
  across KK and MM: the unchecked checkbox is straight black on KK but gray
  on MM, while the checked checkbox is straight black on KK and blue on MM.

BUG=571445
==========

to

==========
Clean up tinted ImageView subclasses

* Introduce an ImageViewTinter that can be used by the TintedImageView
  and the TintedImageButton classes.  They're basically the same thing
  already, but because Java doesn't support extending multiple classes
  we're stuck in this hole.

* Introduce basic tests for the ImageViewTinter users.

* Make the AppMenuItemIcon a TintedImageButton so that it can be tinted
  if needed.  Also make it implement Checkable because that's basically
  what it's already doing.

* The share icon was implemented in a different way than RDS.  Make RDS
  follow what the share icon does because it's probably easier to maintain
  in the end, meaning that it now uses a AppMenuItemIcon.

* Make the RDS checkbox be tinted appropriately gray or blue to match the
  top icon row.  This is necessary because Android themeing is inconsistent
  across KK and MM: the unchecked checkbox is straight black on KK but gray
  on MM, while the checked checkbox is straight black on KK and blue on MM.

BUG=571445

Review-Url: https://codereview.chromium.org/2951133003
Cr-Commit-Position: refs/heads/master@{#482063}
Committed:
https://chromium.googlesource.com/chromium/src/+/65bc4a46d509009cc15dc2ae8e31...
==========

[commit-bot: I haz the power, 2017-06-23 23:12:05]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/65bc4a46d509009cc15dc2ae8e31...