Allow client app choose whether custom action button is tinted

Allow client app choose whether custom action button is tinted

addCustomActionButton()'s signature is modified to pass a
drawable instead of a bitmap, and the intent data provider
will construct a tinted drawable, if the client app chooses
to tint the button.

BUG=508946
Committed: https://crrev.com/c771e9f647bf497c466499d74259583c8be80776
Cr-Commit-Position: refs/heads/master@{#339310}

[Ian Wen, 2015-07-15 21:38:52]

ianwen@chromium.org changed reviewers:
+ aurimas@chromium.org, yusufo@chromium.org

[Ian Wen, 2015-07-15 21:38:53]

aurimas@ please take a look at my change to TintedImageButton.

[aurimas (slooooooooow), 2015-07-16 18:39:16]

Could we use TintedDrawable vs regular Drawable instead? That seems like a
simpler solution.

[Yusuf, 2015-07-16 21:01:14]

https://chromiumcodereview.appspot.com/1237623013/diff/1/chrome/android/java/...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
(right):

https://chromiumcodereview.appspot.com/1237623013/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:606:
public void setCustomActionButtonShouldTint(boolean shouldTint) {
addCustomActionButton can be used for this. Let's not add another API. And in
general we avoid casts to subclasses in the manager.

[Ian Wen, 2015-07-16 22:29:37]

Yusuf and Aurimas:

Thanks for the great suggestions. All done.

[aurimas (slooooooooow), 2015-07-17 00:08:08]

https://chromiumcodereview.appspot.com/1237623013/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
(right):

https://chromiumcodereview.appspot.com/1237623013/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:140:
getToolbarManager().addCustomActionButton(mIntentDataProvider.getActionButtonIcon(),
Can we create Drawable for this button right here and pass that instead of
Bitmap and Boolean? That way CustomTabToolbar does not have to know anything
about tinting.

[Ian Wen, 2015-07-17 16:26:44]

https://chromiumcodereview.appspot.com/1237623013/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
(right):

https://chromiumcodereview.appspot.com/1237623013/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:140:
getToolbarManager().addCustomActionButton(mIntentDataProvider.getActionButtonIcon(),
On 2015/07/17 00:08:08, aurimas wrote:
> Can we create Drawable for this button right here and pass that instead of
> Bitmap and Boolean? That way CustomTabToolbar does not have to know anything
> about tinting.

Good point. Doooooooooooone.

[Yusuf, 2015-07-17 16:32:18]

lgtm for me after the nit, if Aurimas is happy.

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

https://chromiumcodereview.appspot.com/1237623013/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:83:
"android.support.customtabs.extra.ACTION_BUTTON_TINT";
lets match the name on the string and say AUTO_TINT

[Ian Wen, 2015-07-17 16:35:22]

ianwen@chromium.org changed reviewers:
+ newt@chromium.org

[Ian Wen, 2015-07-17 16:35:23]

newt@chromium.org: Could you please take a look at the widget folder? XD

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

https://chromiumcodereview.appspot.com/1237623013/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:83:
"android.support.customtabs.extra.ACTION_BUTTON_TINT";
On 2015/07/17 16:32:18, Yusuf wrote:
> lets match the name on the string and say AUTO_TINT

Done.

[aurimas (slooooooooow), 2015-07-17 16:37:16]

aurimas@chromium.org changed reviewers:
- newt@chromium.org

[aurimas (slooooooooow), 2015-07-17 16:37:17]

Lgtm after you update the CL description to match what actually happened in this
CL.

[newt (away), 2015-07-17 16:46:05]

newt@chromium.org changed reviewers:
+ newt@chromium.org

[newt (away), 2015-07-17 16:46:05]

lgtm after comment

https://chromiumcodereview.appspot.com/1237623013/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java
(right):

https://chromiumcodereview.appspot.com/1237623013/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:79:
* Extra boolean that specifies whether the custom action button should be auto
tinted. Default
What does "auto" mean here? As opposed to "manually" (?) tinted? I'd leave out
the word "auto" unless it has some specific meaning that I'm missing.

[Ian Wen, 2015-07-17 16:55:48]

https://chromiumcodereview.appspot.com/1237623013/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java
(right):

https://chromiumcodereview.appspot.com/1237623013/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:79:
* Extra boolean that specifies whether the custom action button should be auto
tinted. Default
On 2015/07/17 16:46:05, newt wrote:
> What does "auto" mean here? As opposed to "manually" (?) tinted? I'd leave out
> the word "auto" unless it has some specific meaning that I'm missing.

Good question. "Auto" tint means if the theme of the toolbar is dark, the action
button will be tinted white; otherwise it will be tinted gray.

[newt (away), 2015-07-17 17:02:35]

https://chromiumcodereview.appspot.com/1237623013/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java
(right):

https://chromiumcodereview.appspot.com/1237623013/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:79:
* Extra boolean that specifies whether the custom action button should be auto
tinted. Default
On 2015/07/17 16:55:47, Ian Wen wrote:
> On 2015/07/17 16:46:05, newt wrote:
> > What does "auto" mean here? As opposed to "manually" (?) tinted? I'd leave
out
> > the word "auto" unless it has some specific meaning that I'm missing.
> 
> Good question. "Auto" tint means if the theme of the toolbar is dark, the
action
> button will be tinted white; otherwise it will be tinted gray.

Understood. IMO, though, "TINT_ACTION_BUTTON" is a more helpful name, since it
tells you what is going to be tinted. Also, everything in a computer program
happens "auto"-matically, so the word "auto" doesn't add much as I see it. The
ultimate decision is up to you :)  Either way, you should make the variable name
match its value.

[Yusuf, 2015-07-17 17:08:52]

https://chromiumcodereview.appspot.com/1237623013/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java
(right):

https://chromiumcodereview.appspot.com/1237623013/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:79:
* Extra boolean that specifies whether the custom action button should be auto
tinted. Default
On 2015/07/17 17:02:35, newt wrote:
> On 2015/07/17 16:55:47, Ian Wen wrote:
> > On 2015/07/17 16:46:05, newt wrote:
> > > What does "auto" mean here? As opposed to "manually" (?) tinted? I'd leave
> out
> > > the word "auto" unless it has some specific meaning that I'm missing.
> > 
> > Good question. "Auto" tint means if the theme of the toolbar is dark, the
> action
> > button will be tinted white; otherwise it will be tinted gray.
> 
> Understood. IMO, though, "TINT_ACTION_BUTTON" is a more helpful name, since it
> tells you what is going to be tinted. Also, everything in a computer program
> happens "auto"-matically, so the word "auto" doesn't add much as I see it. The
> ultimate decision is up to you :)  Either way, you should make the variable
name
> match its value.

Taking votes for TINT_ACTION_BUTTON_AUTOMAGICALLY!

this or newt@'s suggested name is good for me

[Ian Wen, 2015-07-17 17:09:28]

https://chromiumcodereview.appspot.com/1237623013/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java
(right):

https://chromiumcodereview.appspot.com/1237623013/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:79:
* Extra boolean that specifies whether the custom action button should be auto
tinted. Default
On 2015/07/17 17:08:52, Yusuf wrote:
> On 2015/07/17 17:02:35, newt wrote:
> > On 2015/07/17 16:55:47, Ian Wen wrote:
> > > On 2015/07/17 16:46:05, newt wrote:
> > > > What does "auto" mean here? As opposed to "manually" (?) tinted? I'd
leave
> > out
> > > > the word "auto" unless it has some specific meaning that I'm missing.
> > > 
> > > Good question. "Auto" tint means if the theme of the toolbar is dark, the
> > action
> > > button will be tinted white; otherwise it will be tinted gray.
> > 
> > Understood. IMO, though, "TINT_ACTION_BUTTON" is a more helpful name, since
it
> > tells you what is going to be tinted. Also, everything in a computer program
> > happens "auto"-matically, so the word "auto" doesn't add much as I see it.
The
> > ultimate decision is up to you :)  Either way, you should make the variable
> name
> > match its value.
> 
> Taking votes for TINT_ACTION_BUTTON_AUTOMAGICALLY!
> 
> this or newt@'s suggested name is good for me

Ah sounds good. Done!

[Ian Wen, 2015-07-17 17:10:31]

The CQ bit was checked by ianwen@chromium.org

[Ian Wen, 2015-07-17 17:10:32]

The patchset sent to the CQ was uploaded after l-g-t-m from yusufo@chromium.org,
aurimas@chromium.org, newt@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1237623013/#ps100001 (title: " ")

[commit-bot: I haz the power, 2015-07-17 17:11:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1237623013/100001

[commit-bot: I haz the power, 2015-07-17 17:15:00]

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

[commit-bot: I haz the power, 2015-07-17 17:15:02]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Ian Wen, 2015-07-17 18:53:10]

The CQ bit was checked by ianwen@chromium.org

[Ian Wen, 2015-07-17 18:53:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
aurimas@chromium.org, yusufo@chromium.org, newt@chromium.org
Link to the patchset: https://codereview.chromium.org/1237623013/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2015-07-17 18:55:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1237623013/120001

[commit-bot: I haz the power, 2015-07-17 20:08:16]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-07-17 20:09:12]

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