Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2806273002:
Make the Translate Overflow Menu using ListPopupWindow (Closed)
|
Created:
3 years, 8 months ago by Marti Wong Modified:
3 years, 8 months ago CC:
chromium-reviews, dfalcantara+watch_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake the Overflow Menu using ListPopupWindow for the new Translate infobar.
BUG=709964
Review-Url: https://codereview.chromium.org/2806273002
Cr-Commit-Position: refs/heads/master@{#464350}
Committed: https://chromium.googlesource.com/chromium/src/+/4ce69f8079d73522380aa9c7ddb1847fdd940a99
Patch Set 1 #
Total comments: 6
Patch Set 2 : Use ListPopupWindow instead of spinner #
Total comments: 15
Patch Set 3 : Fix according to comments #Patch Set 4 : Fix according to comments #
Total comments: 9
Patch Set 5 : fix according the comments #
Messages
Total messages: 34 (15 generated)
To use this spinner, you need to:
import ...TranslateMenuSpinnerHelper;
import ...TranslateMenuSpinnerHelper.TranslateSpinnerElement;
implements AdapterView.OnItemSelectedListener
Example of creating the spinner:
List<TranslateSpinnerElement> list = new
ArrayList<TranslateSpinnerElement>();
for (int i = 0; i < 8; i++) {
if (i == 3) {
// creating a divider line
list.add(new TranslateMenuSpinnerHelper
.TranslateSpinnerElement("", null));
}
else
list.add(new TranslateSpinnerElement(
"Title:" + i, "id:" + i));
}
mTranslateMenuSpinnerHelper = new
TranslateMenuSpinnerHelper(
context,
(Spinner) findViewById(R.id.SPINNER_ID), list,
this
);
mSpinner = (Spinner)
findViewById(R.id.R.id.SPINNER_ID);
}
To popup the spinner:
mSpinner.performClick();
When a spinner item is clicked:
public void onSpinnerItemSelected(String code) {
// do something
}
Description was changed from ========== Translate Spinner Menu BUG= ========== to ========== Make the Overflow Menu using Spinner (for the new Translate infobar) BUG=709964 ==========
martiw@chromium.org changed reviewers: + dfalcantara@chromium.org, googleo@chromium.org
PTAL. thanks. If you think this approach is not good enough, we may discuss first before reviewing. by the way, translate_menu_spinner_divider.xml is copied from bookmark_divider.xml I am not sure if I could just reuse bookmark_divider.xml for the POSITION_NONE in TranslateMenuSpinnerHelper.java I make that because there must be a selected item in spinner. I am afraid the selected spinner item will have a different bg color in future android update so I create the POSITION_NONE item and make it the default selected item and hide it. I am open to discuss and modify it if there is other better way.
I'm mainly confused about why you're using a spinner, which has a very specific styling: it shows the selected item and a popup menu on top of it. https://developer.android.com/guide/topics/ui/controls/spinner.html What you need is a regular popup-y menu. https://codereview.chromium.org/2806273002/diff/1/chrome/android/java/res/lay... File chrome/android/java/res/layout/translate_menu_spinner_divider.xml (right): https://codereview.chromium.org/2806273002/diff/1/chrome/android/java/res/lay... chrome/android/java/res/layout/translate_menu_spinner_divider.xml:15: android:background="#ffD6D6D6" /> 1) Where's this color defined in the mock? 2) New colors should go into colors.xml. 3) Don't mix lowercase and uppercase. 4) You don't actually need the first ff. https://codereview.chromium.org/2806273002/diff/1/chrome/android/java/res/lay... File chrome/android/java/res/layout/translate_menu_spinner_item.xml (right): https://codereview.chromium.org/2806273002/diff/1/chrome/android/java/res/lay... chrome/android/java/res/layout/translate_menu_spinner_item.xml:9: android:layout_height="?android:attr/listPreferredItemHeightSmall" > newline here https://codereview.chromium.org/2806273002/diff/1/chrome/android/java/res/lay... chrome/android/java/res/layout/translate_menu_spinner_item.xml:17: android:singleLine="true" /> newline here
On 2017/04/10 20:00:16, dfalcantara (overloaded) wrote: > I'm mainly confused about why you're using a spinner, which has a very specific > styling: it shows the selected item and a popup menu on top of it. > > https://developer.android.com/guide/topics/ui/controls/spinner.html > > What you need is a regular popup-y menu. > > https://codereview.chromium.org/2806273002/diff/1/chrome/android/java/res/lay... > File chrome/android/java/res/layout/translate_menu_spinner_divider.xml (right): > > https://codereview.chromium.org/2806273002/diff/1/chrome/android/java/res/lay... > chrome/android/java/res/layout/translate_menu_spinner_divider.xml:15: > android:background="#ffD6D6D6" /> > 1) Where's this color defined in the mock? > 2) New colors should go into colors.xml. > 3) Don't mix lowercase and uppercase. > 4) You don't actually need the first ff. > > https://codereview.chromium.org/2806273002/diff/1/chrome/android/java/res/lay... > File chrome/android/java/res/layout/translate_menu_spinner_item.xml (right): > > https://codereview.chromium.org/2806273002/diff/1/chrome/android/java/res/lay... > chrome/android/java/res/layout/translate_menu_spinner_item.xml:9: > android:layout_height="?android:attr/listPreferredItemHeightSmall" > > newline here > > https://codereview.chromium.org/2806273002/diff/1/chrome/android/java/res/lay... > chrome/android/java/res/layout/translate_menu_spinner_item.xml:17: > android:singleLine="true" /> > newline here Thanks for the prompt review Dan! I am going to change the spinner to ListPopUpWindow in this CL. I thought that I need to calculate and set height (needs Activity) when using ListPopUpWindow. It was a misunderstanding. I will address all your comments as well.
Description was changed from ========== Make the Overflow Menu using Spinner (for the new Translate infobar) BUG=709964 ========== to ========== Make the Overflow Menu using ListPopupWindow for the new Translate infobar. BUG=709964 ==========
I assume there is a dummy view object (invisible) located at the bottom right corner of the infobar. This is used as the anchor for the ListPopupWindow. Attached the 2 demo videos below: In my demos, I have some hardcoded overflow menu items, when clicking on "More languages", it will show another menu (by replacing the menu items inside the adapter); when clicking on other items, the menu will dismiss. - Demo on a android api 17 device: https://drive.google.com/open?id=0B1O0Z7eoZMuGb3pWbkZ0U21tNXc The menu item "clicking" background is supposed to be a light gray color. but it will mistakenly become blue color sometimes (randomly). still investigating. I suspect this is a android issue. - Demo on Pixel with Android O https://drive.google.com/open?id=0B1O0Z7eoZMuGUzZ1X3BRRm5KLWM Let's discuss tomorrow. thanks~ https://codereview.chromium.org/2806273002/diff/1/chrome/android/java/res/lay... File chrome/android/java/res/layout/translate_menu_spinner_divider.xml (right): https://codereview.chromium.org/2806273002/diff/1/chrome/android/java/res/lay... chrome/android/java/res/layout/translate_menu_spinner_divider.xml:15: android:background="#ffD6D6D6" /> On 2017/04/10 20:00:16, overloaded wrote: > 1) Where's this color defined in the mock? > 2) New colors should go into colors.xml. > 3) Don't mix lowercase and uppercase. > 4) You don't actually need the first ff. Color added to colors.xml. thanks (according to the static mock from Bruno, the divider color is e0e0e0) https://codereview.chromium.org/2806273002/diff/1/chrome/android/java/res/lay... File chrome/android/java/res/layout/translate_menu_spinner_item.xml (right): https://codereview.chromium.org/2806273002/diff/1/chrome/android/java/res/lay... chrome/android/java/res/layout/translate_menu_spinner_item.xml:9: android:layout_height="?android:attr/listPreferredItemHeightSmall" > On 2017/04/10 20:00:16, overloaded wrote: > newline here Done. https://codereview.chromium.org/2806273002/diff/1/chrome/android/java/res/lay... chrome/android/java/res/layout/translate_menu_spinner_item.xml:17: android:singleLine="true" /> On 2017/04/10 20:00:16, overloaded wrote: > newline here Done. https://codereview.chromium.org/2806273002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java (right): https://codereview.chromium.org/2806273002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java:27: public static final String EMPTY_STRING = ""; It seems I might better to set this to private.
On 2017/04/11 14:46:19, Marti Wong wrote: > I assume there is a dummy view object (invisible) located at the bottom right > corner of the infobar. This is used as the anchor for the ListPopupWindow. > > Attached the 2 demo videos below: > > In my demos, I have some hardcoded overflow menu items, when clicking on "More > languages", it will show another menu (by replacing the menu items inside the > adapter); when clicking on other items, the menu will dismiss. > > - Demo on a android api 17 device: > https://drive.google.com/open?id=0B1O0Z7eoZMuGb3pWbkZ0U21tNXc > The menu item "clicking" background is supposed to be a light gray color. but > it will mistakenly become blue color sometimes (randomly). still investigating. > I suspect this is a android issue. > > - Demo on Pixel with Android O > https://drive.google.com/open?id=0B1O0Z7eoZMuGUzZ1X3BRRm5KLWM > > Let's discuss tomorrow. thanks~ > > https://codereview.chromium.org/2806273002/diff/1/chrome/android/java/res/lay... > File chrome/android/java/res/layout/translate_menu_spinner_divider.xml (right): > > https://codereview.chromium.org/2806273002/diff/1/chrome/android/java/res/lay... > chrome/android/java/res/layout/translate_menu_spinner_divider.xml:15: > android:background="#ffD6D6D6" /> > On 2017/04/10 20:00:16, overloaded wrote: > > 1) Where's this color defined in the mock? > > 2) New colors should go into colors.xml. > > 3) Don't mix lowercase and uppercase. > > 4) You don't actually need the first ff. > > Color added to colors.xml. thanks > (according to the static mock from Bruno, the divider color is e0e0e0) > > https://codereview.chromium.org/2806273002/diff/1/chrome/android/java/res/lay... > File chrome/android/java/res/layout/translate_menu_spinner_item.xml (right): > > https://codereview.chromium.org/2806273002/diff/1/chrome/android/java/res/lay... > chrome/android/java/res/layout/translate_menu_spinner_item.xml:9: > android:layout_height="?android:attr/listPreferredItemHeightSmall" > > On 2017/04/10 20:00:16, overloaded wrote: > > newline here > > Done. > > https://codereview.chromium.org/2806273002/diff/1/chrome/android/java/res/lay... > chrome/android/java/res/layout/translate_menu_spinner_item.xml:17: > android:singleLine="true" /> > On 2017/04/10 20:00:16, overloaded wrote: > > newline here > > Done. > > https://codereview.chromium.org/2806273002/diff/20001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java > (right): > > https://codereview.chromium.org/2806273002/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java:27: > public static final String EMPTY_STRING = ""; > It seems I might better to set this to private. Looks alright, functionality-wise. BTW, recording videos is a lot cleaner if you do something like this: > adb shell screenrecord /sdcard/demo.mp4 > (do everything you need to do on the device) > adb pull /sdcard/demo.mp4 It records it directly on the device.
Looks a lot cleaner, too, but it's hard to tell what's actually going on until it's called from somewhere. Any reason why you said you're using an invisible anchor? The anchor should be the menu button on the infobar, itself. https://codereview.chromium.org/2806273002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java (right): https://codereview.chromium.org/2806273002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java:27: public static final String EMPTY_STRING = ""; On 2017/04/11 14:46:18, Marti Wong wrote: > It seems I might better to set this to private. Yeah, no reason this should be exposed. https://codereview.chromium.org/2806273002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java:35: * Add a divider menu item to list Periods at the end everywhere.
https://codereview.chromium.org/2806273002/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/translate_menu_divider.xml (right): https://codereview.chromium.org/2806273002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/translate_menu_divider.xml:9: android:layout_height="17dp" > this should be wrap_content https://codereview.chromium.org/2806273002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/translate_menu_divider.xml:14: android:layout_gravity="center_vertical" add 8dp top and bottom margins here https://codereview.chromium.org/2806273002/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/translate_menu_item.xml (right): https://codereview.chromium.org/2806273002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/translate_menu_item.xml:6: <LinearLayout xmlns:android="http://schemas.android.com/apk/res/android" Why is this a LinearLayout? You have one thing in it. https://codereview.chromium.org/2806273002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java (right): https://codereview.chromium.org/2806273002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java:140: public class TranslateMenuAdapter extends ArrayAdapter<TranslateMenuElement> { can this be static final? https://codereview.chromium.org/2806273002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java:185: public static class TranslateMenuElement { can this be final?
PTAL again. thanks! https://codereview.chromium.org/2806273002/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/translate_menu_divider.xml (right): https://codereview.chromium.org/2806273002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/translate_menu_divider.xml:9: android:layout_height="17dp" > On 2017/04/11 22:45:05, overloaded wrote: > this should be wrap_content Done. https://codereview.chromium.org/2806273002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/translate_menu_divider.xml:14: android:layout_gravity="center_vertical" On 2017/04/11 22:45:05, overloaded wrote: > add 8dp top and bottom margins here no margin needed according to static specs. thanks for reminding me to check. https://codereview.chromium.org/2806273002/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/translate_menu_item.xml (right): https://codereview.chromium.org/2806273002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/translate_menu_item.xml:6: <LinearLayout xmlns:android="http://schemas.android.com/apk/res/android" On 2017/04/11 22:45:05, overloaded wrote: > Why is this a LinearLayout? You have one thing in it. Changed to FrameLayout. thanks https://codereview.chromium.org/2806273002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java (right): https://codereview.chromium.org/2806273002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java:27: public static final String EMPTY_STRING = ""; On 2017/04/11 20:35:22, overloaded wrote: > On 2017/04/11 14:46:18, Marti Wong wrote: > > It seems I might better to set this to private. > > Yeah, no reason this should be exposed. Done. https://codereview.chromium.org/2806273002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java:35: * Add a divider menu item to list On 2017/04/11 20:35:22, overloaded wrote: > Periods at the end everywhere. Done. https://codereview.chromium.org/2806273002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java:140: public class TranslateMenuAdapter extends ArrayAdapter<TranslateMenuElement> { On 2017/04/11 22:45:05, overloaded wrote: > can this be static final? Done. https://codereview.chromium.org/2806273002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java:185: public static class TranslateMenuElement { On 2017/04/11 22:45:05, overloaded wrote: > can this be final? Done. https://codereview.chromium.org/2806273002/diff/60001/chrome/android/java/res... File chrome/android/java/res/layout/translate_menu_item.xml (right): https://codereview.chromium.org/2806273002/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/translate_menu_item.xml:9: android:layout_height="48dp" > height is 48dp according to the static specs. https://codereview.chromium.org/2806273002/diff/60001/chrome/android/java/res... File chrome/android/java/res/values/colors.xml (right): https://codereview.chromium.org/2806273002/diff/60001/chrome/android/java/res... chrome/android/java/res/values/colors.xml:208: <color name="translate_overflow_menu_divider_color">#e0e0e0</color> color is e0e0e0 according to the static specs. https://codereview.chromium.org/2806273002/diff/60001/chrome/android/java/res... File chrome/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/2806273002/diff/60001/chrome/android/java/res... chrome/android/java/res/values/dimens.xml:105: <dimen name="infobar_translate_menu_width">260dp</dimen> menu width is 260dp according to the static specs. https://codereview.chromium.org/2806273002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java (right): https://codereview.chromium.org/2806273002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java:46: * @param code Code of the menu item change the name from id to code after discussing with Leo. "code" makes more sense in our case. https://codereview.chromium.org/2806273002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java:60: public static void addSourceLanguageMenuItem( after discussing to Leo, I divided LANGUAGE_MENU_ITEM into SOURCE_LANGUAGE_MENU_ITEM and TARGET_LANGUAGE_MENU_ITEM so that callback function can distinguished between 2 and act accordingly.
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.
lgtm https://codereview.chromium.org/2806273002/diff/60001/chrome/android/java/res... File chrome/android/java/res/layout/translate_menu_item.xml (right): https://codereview.chromium.org/2806273002/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/translate_menu_item.xml:9: android:layout_height="48dp" > On 2017/04/12 02:13:42, Marti Wong wrote: > height is 48dp according to the static specs. @dimen/min_touch_target_size? https://codereview.chromium.org/2806273002/diff/60001/chrome/android/java/res... File chrome/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/2806273002/diff/60001/chrome/android/java/res... chrome/android/java/res/values/dimens.xml:105: <dimen name="infobar_translate_menu_width">260dp</dimen> On 2017/04/12 02:13:42, Marti Wong wrote: > menu width is 260dp according to the static specs. that doesn't work for all languages, does it? it should be dynamic to handle longer ones.
On 2017/04/12 17:17:33, overloaded (dfalcantara) wrote: > lgtm > > https://codereview.chromium.org/2806273002/diff/60001/chrome/android/java/res... > File chrome/android/java/res/layout/translate_menu_item.xml (right): > > https://codereview.chromium.org/2806273002/diff/60001/chrome/android/java/res... > chrome/android/java/res/layout/translate_menu_item.xml:9: > android:layout_height="48dp" > > On 2017/04/12 02:13:42, Marti Wong wrote: > > height is 48dp according to the static specs. > > @dimen/min_touch_target_size? > > https://codereview.chromium.org/2806273002/diff/60001/chrome/android/java/res... > File chrome/android/java/res/values/dimens.xml (right): > > https://codereview.chromium.org/2806273002/diff/60001/chrome/android/java/res... > chrome/android/java/res/values/dimens.xml:105: <dimen > name="infobar_translate_menu_width">260dp</dimen> > On 2017/04/12 02:13:42, Marti Wong wrote: > > menu width is 260dp according to the static specs. > > that doesn't work for all languages, does it? it should be dynamic to handle > longer ones. Hi Dan, Thanks for the review. Do you think I should fix the width problem in this CL or next CL? To make set the width dynamically, I think I need to.. 1. do what computeMinWidthRequiredForValues (from InfoBarControlLayout.java) does in the helper class. 2. get the ChromeActivity from Infobar->InfobarContainer->Tab to prevent the menu from wider than the screen. Is that right? Thanks! Marti
On 2017/04/13 00:40:05, Marti Wong wrote: > On 2017/04/12 17:17:33, overloaded (dfalcantara) wrote: > > lgtm > > > > > https://codereview.chromium.org/2806273002/diff/60001/chrome/android/java/res... > > File chrome/android/java/res/layout/translate_menu_item.xml (right): > > > > > https://codereview.chromium.org/2806273002/diff/60001/chrome/android/java/res... > > chrome/android/java/res/layout/translate_menu_item.xml:9: > > android:layout_height="48dp" > > > On 2017/04/12 02:13:42, Marti Wong wrote: > > > height is 48dp according to the static specs. > > > > @dimen/min_touch_target_size? > > > > > https://codereview.chromium.org/2806273002/diff/60001/chrome/android/java/res... > > File chrome/android/java/res/values/dimens.xml (right): > > > > > https://codereview.chromium.org/2806273002/diff/60001/chrome/android/java/res... > > chrome/android/java/res/values/dimens.xml:105: <dimen > > name="infobar_translate_menu_width">260dp</dimen> > > On 2017/04/12 02:13:42, Marti Wong wrote: > > > menu width is 260dp according to the static specs. > > > > that doesn't work for all languages, does it? it should be dynamic to handle > > longer ones. > > Hi Dan, > > Thanks for the review. > Do you think I should fix the width problem in this CL or next CL? > > To make set the width dynamically, I think I need to.. > 1. do what computeMinWidthRequiredForValues (from InfoBarControlLayout.java) > does in the helper class. > 2. get the ChromeActivity from Infobar->InfobarContainer->Tab to prevent the > menu from wider than the screen. > Is that right? > > Thanks! > Marti Follow-up is fine. You hopefully don't need to do anything that crazy... I think the first thing you'd need to try is to not set an explicit width on the menu.
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/2806273002/#ps80001 (title: "fix according the comments")
thanks. committing. https://codereview.chromium.org/2806273002/diff/60001/chrome/android/java/res... File chrome/android/java/res/layout/translate_menu_item.xml (right): https://codereview.chromium.org/2806273002/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/translate_menu_item.xml:9: android:layout_height="48dp" > On 2017/04/12 17:17:33, overloaded (dfalcantara) wrote: > On 2017/04/12 02:13:42, Marti Wong wrote: > > height is 48dp according to the static specs. > > @dimen/min_touch_target_size? Thanks for point this out. It seems attr/listPreferredItemHeightSmall (which is also 48dp) is more meaningful here. It is also used in AppMenu and bookmark_popup_item. https://codereview.chromium.org/2806273002/diff/60001/chrome/android/java/res... File chrome/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/2806273002/diff/60001/chrome/android/java/res... chrome/android/java/res/values/dimens.xml:105: <dimen name="infobar_translate_menu_width">260dp</dimen> On 2017/04/12 17:17:33, overloaded (dfalcantara) wrote: > On 2017/04/12 02:13:42, Marti Wong wrote: > > menu width is 260dp according to the static specs. > > that doesn't work for all languages, does it? it should be dynamic to handle > longer ones. I will do the followup in my next CL for this width issue. thanks!
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: linux_chromium_chromeos_ozone_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
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: linux_chromium_chromeos_ozone_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
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": 80001, "attempt_start_ts": 1492071341353660,
"parent_rev": "d5fe9bf25fa23fb35b7bea2580e821b3ca8cbc79", "commit_rev":
"4ce69f8079d73522380aa9c7ddb1847fdd940a99"}
Message was sent while issue was closed.
Description was changed from ========== Make the Overflow Menu using ListPopupWindow for the new Translate infobar. BUG=709964 ========== to ========== Make the Overflow Menu using ListPopupWindow for the new Translate infobar. BUG=709964 Review-Url: https://codereview.chromium.org/2806273002 Cr-Commit-Position: refs/heads/master@{#464350} Committed: https://chromium.googlesource.com/chromium/src/+/4ce69f8079d73522380aa9c7ddb1... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/4ce69f8079d73522380aa9c7ddb1... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
