Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2783523002:
Create the customized TabLayout for the new translate UI (Closed)
|
Created:
3 years, 9 months ago by Marti Wong Modified:
3 years, 8 months ago CC:
chromium-reviews, agrieve+watch_chromium.org, dfalcantara+watch_chromium.org, ramyasharma Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreate the customized TabLayout for the new translate UI
2 classes are created:
- translateTabLayout.java, a customized TabLayout class
- translateTabContent.java, a class for managing the content of a Tab
Demo video of the TranslateTabLayout:
https://drive.google.com/file/d/0B1O0Z7eoZMuGcDdqejkyMmpEZHc/view
BUG=705311
Review-Url: https://codereview.chromium.org/2783523002
Cr-Commit-Position: refs/heads/master@{#460669}
Committed: https://chromium.googlesource.com/chromium/src/+/fac29ec2345d1f010a70ea8a462010a6f803e4e2
Patch Set 1 #Patch Set 2 : Fix the wrong path in java_sources.gni #Patch Set 3 : Fix the wrong path in java_sources.gni #
Total comments: 27
Patch Set 4 : modify according to comments #
Total comments: 2
Patch Set 5 : Use layout XML and use Framelayout. #
Total comments: 17
Patch Set 6 : modify according to comments #
Total comments: 5
Patch Set 7 : modify according to comments #
Messages
Total messages: 53 (35 generated)
Description was changed from ========== Create the customized TabLayout for the new translate UI 2 classes are created: - translateTabLayout.java, a customized TabLayout class - translateTabContent.java, a class for managing the content of a Tab BUG=705311 ========== to ========== Create the customized TabLayout for the new translate UI 2 classes are created: - translateTabLayout.java, a customized TabLayout class - translateTabContent.java, a class for managing the content of a Tab Demo video of the TranslateTabLayout: https://drive.google.com/file/d/0B1O0Z7eoZMuGcDdqejkyMmpEZHc/view BUG=705311 ==========
martiw@chromium.org changed reviewers: + dfalcantara@chromium.org, googleo@chromium.org
The CQ bit was checked by martiw@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
The CQ bit was checked by martiw@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by martiw@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
FYI: You need to hit "Publish+Mail" to send emails out to reviewers, not just edit the issue.
https://codereview.chromium.org/2783523002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabContent.java (right): https://codereview.chromium.org/2783523002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabContent.java:22: * The content of the tab shown in the TranslateTabLayout end with a period. consistently end all of your comments with periods. https://codereview.chromium.org/2783523002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabContent.java:25: private int mIconWidth; private final https://codereview.chromium.org/2783523002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabContent.java:28: Did you consider using an XML layout for this? It'd considerably shrink the amount of code you're trying to land and make it easier to understand. https://codereview.chromium.org/2783523002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabContent.java:77: public void setText(CharSequence tabTitle) { javadocs for all public methods https://codereview.chromium.org/2783523002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabContent.java:78: if (mTextView != null) { Why would this ever be null? https://codereview.chromium.org/2783523002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabContent.java:85: if (mTextView != null && mProgressBar != null) { Same questions about nullity. https://codereview.chromium.org/2783523002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java (right): https://codereview.chromium.org/2783523002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java:13: /** end with period. second sentence is unnecessary; it's implied by the fact that it's inside the TranslateCompactInfoBar. https://codereview.chromium.org/2783523002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java:18: // When true, it will block the touch events from propagating to children consistently end comments with periods everywhere. https://codereview.chromium.org/2783523002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java:47: public void addTabs(CharSequence tab1, CharSequence tab2) { Can you use something like this instead? It's more flexible. public void addTabs(CharSequence... titles) { for (CharSequence title : titles) addTabWithTitle(Title); } https://codereview.chromium.org/2783523002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java:67: public void addTabByText(CharSequence tabTitle) { addTabWithTitle makes more sense https://codereview.chromium.org/2783523002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java:106: if (tabPos >= getTabCount()) { Be consistent about your checks. You're not checking if tabPos < 0 like with selectTab on line 82. https://codereview.chromium.org/2783523002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java:122: mTabsTouchEventDisabled = true; // disable the touch event of tabs. Do you need this variable? Can you just check if mTabShowingProgressBar != null? https://codereview.chromium.org/2783523002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java:166: throwUnsupportedTabException(); Can you just throw an IllegalArgumentException?
The CQ bit was checked by martiw@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL again. Thanks Dan for your prompt response. I will do the layout XML in my next CL. p.s. I will hit the "Publish+Mail" next time. https://codereview.chromium.org/2783523002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabContent.java (right): https://codereview.chromium.org/2783523002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabContent.java:22: * The content of the tab shown in the TranslateTabLayout On 2017/03/28 20:32:39, dfalcantara (load balance plz) wrote: > end with a period. consistently end all of your comments with periods. Done. https://codereview.chromium.org/2783523002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabContent.java:25: private int mIconWidth; On 2017/03/28 20:32:39, dfalcantara (load balance plz) wrote: > private final Done. https://codereview.chromium.org/2783523002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabContent.java:28: On 2017/03/28 20:32:39, dfalcantara (load balance plz) wrote: > Did you consider using an XML layout for this? It'd considerably shrink the > amount of code you're trying to land and make it easier to understand. Agree. I will add a todo here and make the XML layout in my next CL. https://codereview.chromium.org/2783523002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabContent.java:77: public void setText(CharSequence tabTitle) { On 2017/03/28 20:32:39, dfalcantara (load balance plz) wrote: > javadocs for all public methods Done. https://codereview.chromium.org/2783523002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabContent.java:78: if (mTextView != null) { On 2017/03/28 20:32:39, dfalcantara (load balance plz) wrote: > Why would this ever be null? Yah, I should remove this null check. thanks Dan! https://codereview.chromium.org/2783523002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabContent.java:85: if (mTextView != null && mProgressBar != null) { On 2017/03/28 20:32:39, dfalcantara (load balance plz) wrote: > Same questions about nullity. Done. https://codereview.chromium.org/2783523002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java (right): https://codereview.chromium.org/2783523002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java:13: /** On 2017/03/28 20:32:40, dfalcantara (load balance plz) wrote: > end with period. > > second sentence is unnecessary; it's implied by the fact that it's inside the > TranslateCompactInfoBar. Thanks. removed https://codereview.chromium.org/2783523002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java:18: // When true, it will block the touch events from propagating to children On 2017/03/28 20:32:40, dfalcantara (load balance plz) wrote: > consistently end comments with periods everywhere. Done. https://codereview.chromium.org/2783523002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java:47: public void addTabs(CharSequence tab1, CharSequence tab2) { On 2017/03/28 20:32:40, dfalcantara (load balance plz) wrote: > Can you use something like this instead? It's more flexible. > > public void addTabs(CharSequence... titles) { > for (CharSequence title : titles) addTabWithTitle(Title); > } Thanks for the tip! Done https://codereview.chromium.org/2783523002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java:67: public void addTabByText(CharSequence tabTitle) { On 2017/03/28 20:32:40, dfalcantara (load balance plz) wrote: > addTabWithTitle makes more sense Done. https://codereview.chromium.org/2783523002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java:106: if (tabPos >= getTabCount()) { On 2017/03/28 20:32:39, dfalcantara (load balance plz) wrote: > Be consistent about your checks. You're not checking if tabPos < 0 like with > selectTab on line 82. Done. https://codereview.chromium.org/2783523002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java:122: mTabsTouchEventDisabled = true; // disable the touch event of tabs. On 2017/03/28 20:32:40, dfalcantara (load balance plz) wrote: > Do you need this variable? Can you just check if mTabShowingProgressBar != > null? Done. https://codereview.chromium.org/2783523002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java:166: throwUnsupportedTabException(); On 2017/03/28 20:32:39, dfalcantara (load balance plz) wrote: > Can you just throw an IllegalArgumentException? Done.
https://codereview.chromium.org/2783523002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabContent.java (right): https://codereview.chromium.org/2783523002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabContent.java:28: On 2017/03/28 23:46:21, Marti Wong wrote: > On 2017/03/28 20:32:39, dfalcantara (load balance plz) wrote: > > Did you consider using an XML layout for this? It'd considerably shrink the > > amount of code you're trying to land and make it easier to understand. > > Agree. I will add a todo here and make the XML layout in my next CL. Why wait for a new CL? That'd effectively remove the need for most of this class.
On 2017/03/28 23:54:43, dfalcantara (load balance plz) wrote: > https://codereview.chromium.org/2783523002/diff/40001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabContent.java > (right): > > https://codereview.chromium.org/2783523002/diff/40001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabContent.java:28: > > On 2017/03/28 23:46:21, Marti Wong wrote: > > On 2017/03/28 20:32:39, dfalcantara (load balance plz) wrote: > > > Did you consider using an XML layout for this? It'd considerably shrink the > > > amount of code you're trying to land and make it easier to understand. > > > > Agree. I will add a todo here and make the XML layout in my next CL. > > Why wait for a new CL? That'd effectively remove the need for most of this > class. Hi Dan, Just wanted to unblock others by submitting this CL faster. Anyway, I am actually working on the XML layout now. Please wait for a while before reviewing again. :) Thanks as always!
On 2017/03/29 00:02:58, Marti Wong wrote: > On 2017/03/28 23:54:43, dfalcantara (load balance plz) wrote: > > > https://codereview.chromium.org/2783523002/diff/40001/chrome/android/java/src... > > File > > > chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabContent.java > > (right): > > > > > https://codereview.chromium.org/2783523002/diff/40001/chrome/android/java/src... > > > chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabContent.java:28: > > > > On 2017/03/28 23:46:21, Marti Wong wrote: > > > On 2017/03/28 20:32:39, dfalcantara (load balance plz) wrote: > > > > Did you consider using an XML layout for this? It'd considerably shrink > the > > > > amount of code you're trying to land and make it easier to understand. > > > > > > Agree. I will add a todo here and make the XML layout in my next CL. > > > > Why wait for a new CL? That'd effectively remove the need for most of this > > class. > > Hi Dan, > Just wanted to unblock others by submitting this CL faster. > Anyway, I am actually working on the XML layout now. > Please wait for a while before reviewing again. :) > Thanks as always! If you have other changes depending on a particular CL, you can set up git branches so that they can be marked as dependent on each other. This is one way to do it: * git checkout -tb first_cl origin/master # Makes a branch that is called "first_cl" that points at master * git cl patch 2783523002 # Downloads this particular patch into that branch * git checkout -tb second_cl first_cl # Makes a new branch called "second_cl" that points at "first_cl", which includes the patch From here, if you do a git cl upload from the "second_cl" branch, then it'll upload and say that the patch is dependent on 2783523002. If you do a gclient sync, you need to do this: * git checkout first_cl * gclient sync * git checkout second_cl * git rebase
On 2017/03/29 00:11:45, dfalcantara (load balance plz) wrote: > On 2017/03/29 00:02:58, Marti Wong wrote: > > On 2017/03/28 23:54:43, dfalcantara (load balance plz) wrote: > > > > > > https://codereview.chromium.org/2783523002/diff/40001/chrome/android/java/src... > > > File > > > > > > chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabContent.java > > > (right): > > > > > > > > > https://codereview.chromium.org/2783523002/diff/40001/chrome/android/java/src... > > > > > > chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabContent.java:28: > > > > > > On 2017/03/28 23:46:21, Marti Wong wrote: > > > > On 2017/03/28 20:32:39, dfalcantara (load balance plz) wrote: > > > > > Did you consider using an XML layout for this? It'd considerably shrink > > the > > > > > amount of code you're trying to land and make it easier to understand. > > > > > > > > Agree. I will add a todo here and make the XML layout in my next CL. > > > > > > Why wait for a new CL? That'd effectively remove the need for most of this > > > class. > > > > Hi Dan, > > Just wanted to unblock others by submitting this CL faster. > > Anyway, I am actually working on the XML layout now. > > Please wait for a while before reviewing again. :) > > Thanks as always! > > If you have other changes depending on a particular CL, you can set up > git branches so that they can be marked as dependent on each other. > > This is one way to do it: > * git checkout -tb first_cl origin/master # Makes a branch that is called > "first_cl" that points at master > * git cl patch 2783523002 # Downloads this particular patch > into that branch > * git checkout -tb second_cl first_cl # Makes a new branch called > "second_cl" that points at "first_cl", which includes the patch > > From here, if you do a git cl upload from the "second_cl" branch, then it'll > upload and say that the patch is dependent on 2783523002. > > If you do a gclient sync, you need to do this: > * git checkout first_cl > * gclient sync > * git checkout second_cl > * git rebase Get it. Thanks for the tip!
https://codereview.chromium.org/2783523002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabContent.java (right): https://codereview.chromium.org/2783523002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabContent.java:24: public class TranslateTabContent extends RelativeLayout { This could probably be just a plain FrameLayout or ViewSwitcher.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by martiw@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL my CL uses XML layout and FrameLayout now thanks a lot! https://codereview.chromium.org/2783523002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabContent.java (right): https://codereview.chromium.org/2783523002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabContent.java:24: public class TranslateTabContent extends RelativeLayout { On 2017/03/29 00:55:16, dfalcantara (load balance plz) wrote: > This could probably be just a plain FrameLayout or ViewSwitcher. Done. FrameLayout is used.
Nothing major to change. https://codereview.chromium.org/2783523002/diff/80001/chrome/android/java/res... File chrome/android/java/res/layout/infobar_translate_tab_content.xml (right): https://codereview.chromium.org/2783523002/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/infobar_translate_tab_content.xml:18: android:visibility="visible"/> I prefer keeping a space between the " and the />, like you do for the ProgressBar. Same above, for the parent. https://codereview.chromium.org/2783523002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabContent.java (right): https://codereview.chromium.org/2783523002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabContent.java:52: attrs, R.styleable.TabLayout, defStyleAttr, R.style.Widget_Design_TabLayout); Can you set these styles in the XML themselves? There are attributes for it. https://codereview.chromium.org/2783523002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabContent.java:57: // Set text color using tabLayout's ColorStateList. {@link TabLayout} Worth adding a comment about why you need to do this. https://codereview.chromium.org/2783523002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java (right): https://codereview.chromium.org/2783523002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java:23: private Context mContext; Can these be final? https://codereview.chromium.org/2783523002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java:102: if (mTabShowingProgressBar != null) { Might as well combine this with the above conditional like you do for selectTab. https://codereview.chromium.org/2783523002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java:146: return (tab.getCustomView() != null && tab.getCustomView() instanceof TranslateTabContent); you don't need to null check; instanceof automatically returns false if tab.getCustomView() is null. https://codereview.chromium.org/2783523002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java:170: throw new IllegalArgumentException( Ah, I meant that you could probably just inline this instead of making a function for it. You don't really need a custom message because it'll be obvious what went wrong if it gets thrown. Up to you, though. https://codereview.chromium.org/2783523002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java:171: "Only Tab with TranslateTabContent as its CustomView is allowed to add."); is allowed to add -> can be added
The CQ bit was checked by martiw@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL again. thanks! https://codereview.chromium.org/2783523002/diff/80001/chrome/android/java/res... File chrome/android/java/res/layout/infobar_translate_tab_content.xml (right): https://codereview.chromium.org/2783523002/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/infobar_translate_tab_content.xml:18: android:visibility="visible"/> On 2017/03/29 17:04:10, dfalcantara (load balance plz) wrote: > I prefer keeping a space between the " and the />, like you do for the > ProgressBar. Same above, for the parent. Done. https://codereview.chromium.org/2783523002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabContent.java (right): https://codereview.chromium.org/2783523002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabContent.java:52: attrs, R.styleable.TabLayout, defStyleAttr, R.style.Widget_Design_TabLayout); On 2017/03/29 17:04:10, dfalcantara (load balance plz) wrote: > Can you set these styles in the XML themselves? There are attributes for it. Done. Thanks for the advice. The code is so much cleaner now. https://codereview.chromium.org/2783523002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabContent.java:57: // Set text color using tabLayout's ColorStateList. On 2017/03/29 17:04:10, dfalcantara (load balance plz) wrote: > {@link TabLayout} > > Worth adding a comment about why you need to do this. Done. This function is changed to setTextColor Added a comment at the parent explaining why calling this function. https://codereview.chromium.org/2783523002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java (right): https://codereview.chromium.org/2783523002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java:23: private Context mContext; On 2017/03/29 17:04:10, dfalcantara (load balance plz) wrote: > Can these be final? Done. Yes, it can be final :) https://codereview.chromium.org/2783523002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java:25: private int mDefStyleAttr; mAttrs and mDefStyleAttr no longer needed after setting textAppearance in XML https://codereview.chromium.org/2783523002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java:102: if (mTabShowingProgressBar != null) { On 2017/03/29 17:04:10, dfalcantara (load balance plz) wrote: > Might as well combine this with the above conditional like you do for selectTab. Done. https://codereview.chromium.org/2783523002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java:146: return (tab.getCustomView() != null && tab.getCustomView() instanceof TranslateTabContent); On 2017/03/29 17:04:10, dfalcantara (load balance plz) wrote: > you don't need to null check; instanceof automatically returns false if > tab.getCustomView() is null. Done. https://codereview.chromium.org/2783523002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java:170: throw new IllegalArgumentException( On 2017/03/29 17:04:10, dfalcantara (load balance plz) wrote: > Ah, I meant that you could probably just inline this instead of making a > function for it. You don't really need a custom message because it'll be > obvious what went wrong if it gets thrown. Up to you, though. Done. Agree, it can make the code cleaner. I removed the "return" after throwing IllegalArgumentException() as well. https://codereview.chromium.org/2783523002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java:171: "Only Tab with TranslateTabContent as its CustomView is allowed to add."); On 2017/03/29 17:04:11, dfalcantara (load balance plz) wrote: > is allowed to add -> can be added Message removed. https://codereview.chromium.org/2783523002/diff/100001/chrome/android/java/re... File chrome/android/java/res/layout/infobar_translate_tab_content.xml (right): https://codereview.chromium.org/2783523002/diff/100001/chrome/android/java/re... chrome/android/java/res/layout/infobar_translate_tab_content.xml:18: android:textAppearance="@style/TextAppearance.Design.Tab" Seems I cannot get the item tabTextAppearance from style Base.Widget.Design.TabLayout directly. So I use the style TextAppearance.Design.Tab instead.
lgtm % comments I forgot some stuff, but address those and you should be good to go. https://codereview.chromium.org/2783523002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java (right): https://codereview.chromium.org/2783523002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java:21: private Tab mTabShowingProgressBar = null; nit: Java references default to null so you don't need to initialize it. https://codereview.chromium.org/2783523002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java:23: private final Context mContext; nit: Can you check if getContext() can be used instead of keeping a reference to mContext? I forgot TabLayout was a View and had that call available.
modified Thanks https://codereview.chromium.org/2783523002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java (right): https://codereview.chromium.org/2783523002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java:21: private Tab mTabShowingProgressBar = null; On 2017/03/29 23:53:13, dfalcantara (load balance plz) wrote: > nit: Java references default to null so you don't need to initialize it. Done. https://codereview.chromium.org/2783523002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateTabLayout.java:23: private final Context mContext; On 2017/03/29 23:53:13, dfalcantara (load balance plz) wrote: > nit: Can you check if getContext() can be used instead of keeping a reference to > mContext? I forgot TabLayout was a View and had that call available. Done.
The CQ bit was checked by martiw@chromium.org to run a CQ dry run
The CQ bit was unchecked by martiw@chromium.org
The CQ bit was checked by martiw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2783523002/#ps120001 (title: "modify according to comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity)
The CQ bit was checked by martiw@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by martiw@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by martiw@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1490850402152230,
"parent_rev": "b443952729c145d91777734b31a4e83ca2ba8ef7", "commit_rev":
"fac29ec2345d1f010a70ea8a462010a6f803e4e2"}
Message was sent while issue was closed.
Description was changed from ========== Create the customized TabLayout for the new translate UI 2 classes are created: - translateTabLayout.java, a customized TabLayout class - translateTabContent.java, a class for managing the content of a Tab Demo video of the TranslateTabLayout: https://drive.google.com/file/d/0B1O0Z7eoZMuGcDdqejkyMmpEZHc/view BUG=705311 ========== to ========== Create the customized TabLayout for the new translate UI 2 classes are created: - translateTabLayout.java, a customized TabLayout class - translateTabContent.java, a class for managing the content of a Tab Demo video of the TranslateTabLayout: https://drive.google.com/file/d/0B1O0Z7eoZMuGcDdqejkyMmpEZHc/view BUG=705311 Review-Url: https://codereview.chromium.org/2783523002 Cr-Commit-Position: refs/heads/master@{#460669} Committed: https://chromium.googlesource.com/chromium/src/+/fac29ec2345d1f010a70ea8a4620... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/fac29ec2345d1f010a70ea8a4620... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
