Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2830543003:
Re-fact TranslateMenuHelper. (Closed)
DescriptionCode refactoring for TranslateMenuHelper.
1, Improve menu performance by reuse convertView.
2, Simplify menu list loading.
3, Move menu entity definition to a separate class.
Use this helper to build translate overflow menu.
Patched issue/2820423003 to test the menu texts.
Screenshot:
https://screenshot.googleplex.com/kbqTJchhJrc
BUG=703887
Review-Url: https://codereview.chromium.org/2830543003
Cr-Commit-Position: refs/heads/master@{#467199}
Committed: https://chromium.googlesource.com/chromium/src/+/c36577e5a5e9f4e7a7f089bc7b457fa77f30578b
Patch Set 1 #
Total comments: 10
Patch Set 2 : fix #Patch Set 3 : Merge branch 'trans-menu-strings' into trans-menu #
Total comments: 3
Patch Set 4 : fix #Patch Set 5 : Merge branch 'master' into trans-menu #
Total comments: 6
Patch Set 6 : simplify logic #Patch Set 7 : remove both target and source #
Total comments: 4
Patch Set 8 : fix #Patch Set 9 : fix #
Dependent Patchsets: Messages
Total messages: 27 (15 generated)
Description was changed from ========== Re-fact TranslateMenuHelper. 1, Imporve pefermance by reuse convertView. 2, Simplify menu list loading. 3, Move menu entity definition to a new class. Use this helper to build translate overlow menu. Patched issue/2820423003 to test the menu. Screenshot: https://screenshot.googleplex.com/kbqTJchhJrc BUG=703887 ========== to ========== Re-fact TranslateMenuHelper. 1, Imporve pefermance by reuse convertView. 2, Simplify menu list loading. 3, Move menu entity definition to a new class. Use this helper to build translate overlow menu. Patched issue/2820423003 to test the menu. Screenshot: https://screenshot.googleplex.com/kbqTJchhJrc BUG=703887 ==========
googleo@chromium.org changed reviewers: + dfalcantara@chromium.org
Description was changed from ========== Re-fact TranslateMenuHelper. 1, Imporve pefermance by reuse convertView. 2, Simplify menu list loading. 3, Move menu entity definition to a new class. Use this helper to build translate overlow menu. Patched issue/2820423003 to test the menu. Screenshot: https://screenshot.googleplex.com/kbqTJchhJrc BUG=703887 ========== to ========== Code refactoring for TranslateMenuHelper. 1, Improve menu performance by reuse convertView. 2, Simplify menu list loading. 3, Move menu entity definition to a separate class. Use this helper to build translate overflow menu. Patched issue/2820423003 to test the menu texts. Screenshot: https://screenshot.googleplex.com/kbqTJchhJrc BUG=703887 ==========
Hope it works for you, Dan. Thanks.
I mentioned in some other CL how to set up your git branches so that your CLs depend on each other but don't need to be smashed together. Probably got lost, but if you do this it should work: # your string CL branch tracks master git checkout -tb string_branch master # download your existing CL into that branch (just one way to do this) git cl patch 2820423003 # this CL tracks the string CL git checkout -tb refactoring_branch string_branch # download this CL into that branch git cl patch 2830543003 # uploads this CL and marks it as dependent on the other one git cl upload https://codereview.chromium.org/2830543003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java (right): https://codereview.chromium.org/2830543003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:63: .setOnClickListener(new OnClickListener() { can you pull this out into a separate function that creates the OnClickListener and returns it? this is indented too deeply for my liking. https://codereview.chromium.org/2830543003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:122: return; case VALID_CASE_1: case VALID_CASE_2: case VALID_CASE_3: return; default: assert false; asserting in default this prevents adding a new switch you forgot to account for https://codereview.chromium.org/2830543003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenu.java (right): https://codereview.chromium.org/2830543003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenu.java:41: public final String mCode; using a string for what is effectively an enum is inefficient. convert them into ints. look up the @IntDef pattern used elsewhere for the best way to do it. that said: why don't you just pass in the int res ID? R.string.translate_always_text (and the other ones) are ints already. https://codereview.chromium.org/2830543003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java (right): https://codereview.chromium.org/2830543003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java:151: if (convertView == null) { if (menuItemView == null) you already set menuItemView to it earlier. https://codereview.chromium.org/2830543003/diff/1/chrome/android/java/strings... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2830543003/diff/1/chrome/android/java/strings... chrome/android/java/strings/android_chrome_strings.grd:1566: <message name="IDS_TRANSLATE_INFOBAR_MORE_LANGUAGE" meaning="Android" desc="Menu item for selecting another target language to translate. [CHAR-LIMIT=64]"> where did you get this "meaning" thing? i think it was a bad copy from somewhere else that was doing things incorrectly :/ delete it here and the other translate string on 1563
Thanks for your review, Dan. All comments are replied. I will update the android_chrome_strings.grd changes in another CL then merge it here. BTW, is it too late to run the steps you mentioned to manage the 2 dependency CLs? If yes, I will use it next time. Currently I manage the 2 branches manually with git-merge. I believe it should work fine but it's not so smart as your steps. https://codereview.chromium.org/2830543003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java (right): https://codereview.chromium.org/2830543003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:122: return; On 2017/04/19 23:21:13, slow (dfalcantara) wrote: > case VALID_CASE_1: > case VALID_CASE_2: > case VALID_CASE_3: > return; > default: > assert false; > > asserting in default this prevents adding a new switch you forgot to account for Done. https://codereview.chromium.org/2830543003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenu.java (right): https://codereview.chromium.org/2830543003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenu.java:41: public final String mCode; On 2017/04/19 23:21:13, slow (dfalcantara) wrote: > using a string for what is effectively an enum is inefficient. convert them > into ints. look up the @IntDef pattern used elsewhere for the best way to do > it. > > that said: why don't you just pass in the int res ID? > R.string.translate_always_text (and the other ones) are ints already. Thanks, that's also what I thought. My original data structure was {type:int, resId:int} with a getString() method. And Marti mentioned we are going to load the language list in the same popup menu. Then it is not compatible with the structure. Language menu item needs an ISO code as Key and resId is useless at all in that case. WDYT? https://codereview.chromium.org/2830543003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java (right): https://codereview.chromium.org/2830543003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java:151: if (convertView == null) { On 2017/04/19 23:21:13, slow (dfalcantara) wrote: > if (menuItemView == null) > > you already set menuItemView to it earlier. Done. https://codereview.chromium.org/2830543003/diff/1/chrome/android/java/strings... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2830543003/diff/1/chrome/android/java/strings... chrome/android/java/strings/android_chrome_strings.grd:1566: <message name="IDS_TRANSLATE_INFOBAR_MORE_LANGUAGE" meaning="Android" desc="Menu item for selecting another target language to translate. [CHAR-LIMIT=64]"> On 2017/04/19 23:21:13, slow (dfalcantara) wrote: > where did you get this "meaning" thing? i think it was a bad copy from > somewhere else that was doing things incorrectly :/ > > delete it here and the other translate string on 1563 I copied it from just the line above. Will remove them all in my changes.
https://codereview.chromium.org/2830543003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenu.java (right): https://codereview.chromium.org/2830543003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenu.java:41: public final String mCode; On 2017/04/20 01:17:21, Leo wrote: > On 2017/04/19 23:21:13, slow (dfalcantara) wrote: > > using a string for what is effectively an enum is inefficient. convert them > > into ints. look up the @IntDef pattern used elsewhere for the best way to do > > it. > > > > that said: why don't you just pass in the int res ID? > > R.string.translate_always_text (and the other ones) are ints already. > > Thanks, that's also what I thought. My original data structure was {type:int, > resId:int} with a getString() method. > > And Marti mentioned we are going to load the language list in the same popup > menu. Then it is not compatible with the structure. Language menu item needs an > ISO code as Key and resId is useless at all in that case. > > WDYT? what would this structure look like? I'm not seeing an mKey here. https://codereview.chromium.org/2830543003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java (right): https://codereview.chromium.org/2830543003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:62: ((ImageButton) content.findViewById(R.id.translate_infobar_menu_button)) don't bother casting to ImageButton... I think the OnClickListener can be set on a regular View too. https://codereview.chromium.org/2830543003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:124: case TranslateMenu.NOT_THIS_LANGUAGE: you still need to return here so it doesn't run into the default assert failure case https://codereview.chromium.org/2830543003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenu.java (right): https://codereview.chromium.org/2830543003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenu.java:54: switch (mCode) { surprised this works in java 7, but I still can't imagine it being very efficient.
Thanks for the review, Dan. I have convert all string var to int via another code refactor. Please take a look.
https://codereview.chromium.org/2830543003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenu.java (right): https://codereview.chromium.org/2830543003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenu.java:36: public static final int ITEM_CHECKBOX_OPTION = 3; no newline. https://codereview.chromium.org/2830543003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java (right): https://codereview.chromium.org/2830543003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java:158: private String getTitleById(int itemId) { Add comments explaining each case. https://codereview.chromium.org/2830543003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java:177: case TranslateMenu.MENU_TARGET_LANGUAGE: Sorry for flip-flopping here, but it was hard to know what you wanted until everything was in this CL. The reason we use language code strings is because the native-side Translate backend code downloads an updated list of languages that doesn't get pushed to the Java side to re-create the language menu. This resulted in off-by-one errors when using numbers for several milestones, causing languages to be incorrectly translated. I *think* we addressed this by sticking with language code strings. I would strongly suggest against switching to ints for language codes. Just go back to comparing strings.
Thanks for the review Dan, PTAL. I removed index manipulation by introducing MenuType into Adapter. After adding language code in MenuItem, getting text logic is much clearer now. https://codereview.chromium.org/2830543003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenu.java (right): https://codereview.chromium.org/2830543003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenu.java:36: public static final int ITEM_CHECKBOX_OPTION = 3; On 2017/04/22 00:47:52, slow (dfalcantara) wrote: > no newline. Acknowledged. https://codereview.chromium.org/2830543003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java (right): https://codereview.chromium.org/2830543003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java:158: private String getTitleById(int itemId) { On 2017/04/22 00:47:52, slow (dfalcantara) wrote: > Add comments explaining each case. After introducing a method in TranslationOptions, the logic here should be clear without extra comments. https://codereview.chromium.org/2830543003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java:177: case TranslateMenu.MENU_TARGET_LANGUAGE: On 2017/04/22 00:47:52, slow (dfalcantara) wrote: > Sorry for flip-flopping here, but it was hard to know what you wanted until > everything was in this CL. > > The reason we use language code strings is because the native-side Translate > backend code downloads an updated list of languages that doesn't get pushed to > the Java side to re-create the language menu. This resulted in off-by-one > errors when using numbers for several milestones, causing languages to be > incorrectly translated. I *think* we addressed this by sticking with language > code strings. > > I would strongly suggest against switching to ints for language codes. Just go > back to comparing strings. Thanks, I remove the index manipulation by binding the code with MenuItem. It should make new logic much clearer.
The CQ bit was checked by googleo@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: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
Remove both target and source in the language popup list. Original UI used Spinner as language popup list, so it is necessary to keep source/target language in source/target list to show selection in "Language Panel". Now we remove language panel step and use menu popup list to replace spinner. It doesn't make sense to keep source or target any more.
lgtm https://codereview.chromium.org/2830543003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateOptions.java (right): https://codereview.chromium.org/2830543003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateOptions.java:229: public String getRepresentationByCode(String languageCode) { 1) getRepresentationFromCode 2) you need javadoc for public methods https://codereview.chromium.org/2830543003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java (right): https://codereview.chromium.org/2830543003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java:168: if (menuType == mMenuType) { for simple conditionals in java, you can put it on one line: if (menuType == mMenuType) return;
The CQ bit was checked by googleo@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...
All fixed. Thanks https://codereview.chromium.org/2830543003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateOptions.java (right): https://codereview.chromium.org/2830543003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateOptions.java:229: public String getRepresentationByCode(String languageCode) { On 2017/04/24 20:12:19, slow (dfalcantara) wrote: > 1) getRepresentationFromCode > 2) you need javadoc for public methods Done. https://codereview.chromium.org/2830543003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java (right): https://codereview.chromium.org/2830543003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java:168: if (menuType == mMenuType) { On 2017/04/24 20:12:19, slow (dfalcantara) wrote: > for simple conditionals in java, you can put it on one line: > > if (menuType == mMenuType) return; Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by googleo@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/2830543003/#ps160001 (title: "fix")
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": 160001, "attempt_start_ts": 1493166931780730,
"parent_rev": "4166ab857753a66e9ba6aea475ada0f14b0971d8", "commit_rev":
"c36577e5a5e9f4e7a7f089bc7b457fa77f30578b"}
Message was sent while issue was closed.
Description was changed from ========== Code refactoring for TranslateMenuHelper. 1, Improve menu performance by reuse convertView. 2, Simplify menu list loading. 3, Move menu entity definition to a separate class. Use this helper to build translate overflow menu. Patched issue/2820423003 to test the menu texts. Screenshot: https://screenshot.googleplex.com/kbqTJchhJrc BUG=703887 ========== to ========== Code refactoring for TranslateMenuHelper. 1, Improve menu performance by reuse convertView. 2, Simplify menu list loading. 3, Move menu entity definition to a separate class. Use this helper to build translate overflow menu. Patched issue/2820423003 to test the menu texts. Screenshot: https://screenshot.googleplex.com/kbqTJchhJrc BUG=703887 Review-Url: https://codereview.chromium.org/2830543003 Cr-Commit-Position: refs/heads/master@{#467199} Committed: https://chromium.googlesource.com/chromium/src/+/c36577e5a5e9f4e7a7f089bc7b45... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/c36577e5a5e9f4e7a7f089bc7b45... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
