Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years, 5 months ago by Ian Wen Modified:
5 years, 5 months ago Reviewers:
aurimas (slooooooooow), Yusuf, newt (away) CC:
chromium-reviews, ianwen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow 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}
Patch Set 1 #
Total comments: 1
Patch Set 2 : respond to aurimas's comment #
Total comments: 2
Patch Set 3 : pass drawable instead of bitmap #
Total comments: 2
Patch Set 4 : rename extra #
Total comments: 5
Patch Set 5 : rename extra #2 #Patch Set 6 : #Patch Set 7 : rebase #
Created: 5 years, 5 months ago
Depends on Patchset: Messages
Total messages: 28 (9 generated)
ianwen@chromium.org changed reviewers: + aurimas@chromium.org, yusufo@chromium.org
aurimas@ please take a look at my change to TintedImageButton.
Could we use TintedDrawable vs regular Drawable instead? That seems like a simpler solution.
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.
Yusuf and Aurimas: Thanks for the great suggestions. All done.
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.
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.
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
ianwen@chromium.org changed reviewers: + newt@chromium.org
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@chromium.org changed reviewers: - newt@chromium.org
Lgtm after you update the CL description to match what actually happened in this CL.
newt@chromium.org changed reviewers: + newt@chromium.org
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.
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.
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.
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
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!
The CQ bit was checked by ianwen@chromium.org
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: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1237623013/100001
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by ianwen@chromium.org
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1237623013/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/c771e9f647bf497c466499d74259583c8be80776 Cr-Commit-Position: refs/heads/master@{#339310} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
