Inherit default text color from MainTheme.

Inherit default text color from parent themes.

This change removes default_text_color being
hard-coded in ui components everywhere.

Most controls are created as part of the their parent Themes (MainTheme and DownloadsActivityTheme for this cl)
they will automatically get android:textColor set to default_text_color.

BUG=654062
Committed: https://crrev.com/e4f0a51143b3db3643b7bac31d0be4d08642fe4e
Cr-Commit-Position: refs/heads/master@{#424599}

[bhallam, 2016-09-22 21:54:36]

Description was changed from

==========
Inherit default text color from MainTheme.

This change removes default_text_color being
hard-coded in ui components everywhere.
As all controls are created as part of the MainTheme,
they will automatically get android:textColor set to default_text_color.

BUG=0
==========

to

==========
Inherit default text color from MainTheme.

This change removes default_text_color being
hard-coded in ui components everywhere.
As all controls are created as part of the MainTheme,
they will automatically get android:textColor set to default_text_color.

BUG=0
==========

[bhallam, 2016-09-22 21:54:36]

bhallam@amazon.com changed reviewers:
+ dfalcantara@chromium.org

[gone, 2016-09-27 17:32:39]

Have you tested all the places where this blanket change takes effect? 
Downloads and Payments controls don't inherit from MainTheme.

[bhallam, 2016-09-28 00:50:14]

Description was changed from

==========
Inherit default text color from MainTheme.

This change removes default_text_color being
hard-coded in ui components everywhere.
As all controls are created as part of the MainTheme,
they will automatically get android:textColor set to default_text_color.

BUG=0
==========

to

==========
Inherit default text color from parent themes.

This change removes default_text_color being
hard-coded in ui components everywhere.

Most controls are created as part of the their parent Themes (MainTheme and
DownloadsActivityTheme for this cl)
they will automatically get android:textColor set to default_text_color.

BUG=0
==========

[bhallam, 2016-09-28 01:51:54]

On 2016/09/27 17:32:39, dfalcantara (slow) wrote:
> Have you tested all the places where this blanket change takes effect? 
> Downloads and Payments controls don't inherit from MainTheme.

Hi dfalcantara!
I was able to test it on the Downloads Activity and there was no difference
between the two changes.
(Strangely enough there isnt a stark visual difference between
default_text_color used v/s the android:textColor inherited from
Theme.AppCompat.

And i have modified on the previous revision of
chrome/android/java/res/layout/payment_request_header
so as to reuse the default text style for payment text views.

[gone, 2016-09-30 00:11:22]

Yeah, the color difference is (annoyingly) subtle, but there (yay black versus
not-entirely black).

At a high level, I'd be fine with this, but would you mind holding off on this
until branch point next Thursday afternoon?  I'm afraid that this touches enough
UI in a subtle way that anything wrong would go unnoticed for a while.  I'm
planning on compiling it locally and taking a look myself in the meantime.

[bhallam, 2016-10-04 18:03:11]

On 2016/09/30 00:11:22, dfalcantara (slow) wrote:
> Yeah, the color difference is (annoyingly) subtle, but there (yay black versus
> not-entirely black).
> 
> At a high level, I'd be fine with this, but would you mind holding off on this
> until branch point next Thursday afternoon?  I'm afraid that this touches
enough
> UI in a subtle way that anything wrong would go unnoticed for a while.  I'm
> planning on compiling it locally and taking a look myself in the meantime.


Thanks dfalcantra!
I will await your feedback on those changes.

[gone, 2016-10-07 21:42:17]

Yeah, didn't take long to spot unintentional side effects from your change.  I
filed 654062 to see what UX wants to do here.

https://chromiumcodereview.appspot.com/2365733002/diff/20001/chrome/android/j...
File chrome/android/java/res/values-v17/styles.xml (right):

https://chromiumcodereview.appspot.com/2365733002/diff/20001/chrome/android/j...
chrome/android/java/res/values-v17/styles.xml:26: <item
name="android:textColor">@color/default_text_color</item>
This impacts every bit of text everywhere that is inheriting from Android's
default theme and not explicitly setting default_text_color themselves.

[gone, 2016-10-07 22:05:48]

Er, intentional side effects, I guess?  I still don't think it makes a huge
difference, and cleans up a bit of the code fairly nicely, but we'll see what UX
says.

[bhallam, 2016-10-07 22:39:18]

On 2016/10/07 22:05:48, dfalcantara wrote:
> Er, intentional side effects, I guess?  I still don't think it makes a huge
> difference, and cleans up a bit of the code fairly nicely, but we'll see what
UX
> says.

Yes, I had an idea that it would affect ui controls where the default text color
is not specified;
and I agree that my motivation for this CL is to drive a central way to provide
attribute values through parent themes and styles
than explicit definitions as part of layouts for the controls.

I really appreciate you helping me getting more traction through the Chromium UX
team.

[bhallam, 2016-10-08 01:52:11]

Description was changed from

==========
Inherit default text color from parent themes.

This change removes default_text_color being
hard-coded in ui components everywhere.

Most controls are created as part of the their parent Themes (MainTheme and
DownloadsActivityTheme for this cl)
they will automatically get android:textColor set to default_text_color.

BUG=0
==========

to

==========
Inherit default text color from parent themes.

This change removes default_text_color being
hard-coded in ui components everywhere.

Most controls are created as part of the their parent Themes (MainTheme and
DownloadsActivityTheme for this cl)
they will automatically get android:textColor set to default_text_color.

BUG=654062
==========

[gone, 2016-10-10 18:22:55]

Yeah, thanks for looking into this.  A few years ago, we had a different
internal bug called "Unify Fifty Shades of Grey" to try and address similar
problems, but more greys crept in since then, I guess.

Overall l-g-t-m, but I'm not too keen on the conditional change.

https://chromiumcodereview.appspot.com/2365733002/diff/40001/chrome/android/j...
File chrome/android/java/res/values-v17/styles.xml (right):

https://chromiumcodereview.appspot.com/2365733002/diff/40001/chrome/android/j...
chrome/android/java/res/values-v17/styles.xml:59: <item
name="android:textColor">@color/default_text_color</item>
This should go under the FullscreenWhiteDialog section instead.

https://chromiumcodereview.appspot.com/2365733002/diff/40001/chrome/android/j...
File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java
(right):

https://chromiumcodereview.appspot.com/2365733002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:283:
view.setBackgroundColor(isItemSelected ? mBackgroundHighlightColor : null);
I was having some issues getting this to pass findbugs locally when I ran it;
you'll likely hit problems with the android_clang_dbg bot.

https://chromiumcodereview.appspot.com/2365733002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:285:
@ColorInt final int textColor = isEnabled(position)
nit: this one should probably be called textColorId

https://chromiumcodereview.appspot.com/2365733002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:289:
@ColorRes final int textColorRes = ApiCompatibilityUtils.getColor(
nit: which would let this be called textColor

https://chromiumcodereview.appspot.com/2365733002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:292:
view.setTextColor(isItemSelected ? Color.WHITE : textColorRes);
Not a big fan of getting rid of the outer position == mSelectedItem conditional;
it makes this whole block harder to parse and makes us fetch the color when we
didn't have to before.

[bhallam, 2016-10-11 02:15:19]

Hi dfalcantara
I am addressing all your nits and concerns in the next revision to the CL.

https://chromiumcodereview.appspot.com/2365733002/diff/40001/chrome/android/j...
File chrome/android/java/res/values-v17/styles.xml (right):

https://chromiumcodereview.appspot.com/2365733002/diff/40001/chrome/android/j...
chrome/android/java/res/values-v17/styles.xml:59: <item
name="android:textColor">@color/default_text_color</item>
On 2016/10/10 18:22:55, dfalcantara wrote:
> This should go under the FullscreenWhiteDialog section instead.

Done.

https://chromiumcodereview.appspot.com/2365733002/diff/40001/chrome/android/j...
File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java
(right):

https://chromiumcodereview.appspot.com/2365733002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:283:
view.setBackgroundColor(isItemSelected ? mBackgroundHighlightColor : null);
On 2016/10/10 18:22:55, dfalcantara wrote:
> I was having some issues getting this to pass findbugs locally when I ran it;
> you'll likely hit problems with the android_clang_dbg bot.
Done

https://chromiumcodereview.appspot.com/2365733002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:285:
@ColorInt final int textColor = isEnabled(position)
On 2016/10/10 18:22:55, dfalcantara wrote:
> nit: this one should probably be called textColorId

Done

https://chromiumcodereview.appspot.com/2365733002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:289:
@ColorRes final int textColorRes = ApiCompatibilityUtils.getColor(
On 2016/10/10 18:22:55, dfalcantara wrote:
> nit: which would let this be called textColor

Done.

https://chromiumcodereview.appspot.com/2365733002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:292:
view.setTextColor(isItemSelected ? Color.WHITE : textColorRes);
On 2016/10/10 18:22:55, dfalcantara wrote:
> Not a big fan of getting rid of the outer position == mSelectedItem
conditional;
> it makes this whole block harder to parse and makes us fetch the color when we
> didn't have to before.

Done.

[gone, 2016-10-11 19:53:50]

https://codereview.chromium.org/2365733002/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java
(right):

https://codereview.chromium.org/2365733002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:288:
@ColorInt final int textColorId = isEnabled(position)
You set the annotations backwards; the colorId is the @ColorRes and the
textColor is the packed AARRGGBB @ColorInt

[bhallam, 2016-10-11 20:03:57]

https://codereview.chromium.org/2365733002/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java
(right):

https://codereview.chromium.org/2365733002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:288:
@ColorInt final int textColorId = isEnabled(position)
On 2016/10/11 19:53:50, dfalcantara wrote:
> You set the annotations backwards; the colorId is the @ColorRes and the
> textColor is the packed AARRGGBB @ColorInt

Thanks! Done.

[gone, 2016-10-11 22:47:39]

lgtm, thanks!

[bhallam, 2016-10-11 22:53:56]

The CQ bit was checked by bhallam@amazon.com

[commit-bot: I haz the power, 2016-10-11 22:54:32]

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, 2016-10-11 23:45:00]

Description was changed from

==========
Inherit default text color from parent themes.

This change removes default_text_color being
hard-coded in ui components everywhere.

Most controls are created as part of the their parent Themes (MainTheme and
DownloadsActivityTheme for this cl)
they will automatically get android:textColor set to default_text_color.

BUG=654062
==========

to

==========
Inherit default text color from parent themes.

This change removes default_text_color being
hard-coded in ui components everywhere.

Most controls are created as part of the their parent Themes (MainTheme and
DownloadsActivityTheme for this cl)
they will automatically get android:textColor set to default_text_color.

BUG=654062
==========

[commit-bot: I haz the power, 2016-10-11 23:45:01]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-11 23:48:13]

Description was changed from

==========
Inherit default text color from parent themes.

This change removes default_text_color being
hard-coded in ui components everywhere.

Most controls are created as part of the their parent Themes (MainTheme and
DownloadsActivityTheme for this cl)
they will automatically get android:textColor set to default_text_color.

BUG=654062
==========

to

==========
Inherit default text color from parent themes.

This change removes default_text_color being
hard-coded in ui components everywhere.

Most controls are created as part of the their parent Themes (MainTheme and
DownloadsActivityTheme for this cl)
they will automatically get android:textColor set to default_text_color.

BUG=654062

Committed: https://crrev.com/e4f0a51143b3db3643b7bac31d0be4d08642fe4e
Cr-Commit-Position: refs/heads/master@{#424599}
==========

[commit-bot: I haz the power, 2016-10-11 23:48:14]

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