Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23314003:
Add support for datalist to text input element on Android (Closed)
|
Created:
7 years, 4 months ago by keishi Modified:
7 years, 1 month ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd support for datalist to text input element on Android
We want to have a darker color divider between the datalist suggestions and autofill suggestions.
The ListView instance is constructed inside ListPopupWindow so we can't override ListView.drawDivider(), so we draw the divider inside the list item view.
BUG=242455
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=234194
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=234378
Patch Set 1 #Patch Set 2 : #Patch Set 3 : Created custom drawable #
Total comments: 23
Patch Set 4 : Calculate 44dp+1px #Patch Set 5 : Rebased #
Total comments: 14
Patch Set 6 : Fixed nits #Patch Set 7 : Removed change to runtime_features.cc #Patch Set 8 : Added R.java change #Patch Set 9 : #
Messages
Total messages: 29 (0 generated)
Here is a screenshot of the patch in action. The "Tom"s are datalist suggestions and "Tony" is the autofill suggestion. https://docs.google.com/file/d/0BwRi59l_ri74VXJhWWV5em1CeDA/edit?usp=sharing
newt@, could you take a look at this too since it is UI related? I've tried using drawable for the divider instead of a view, but I wasn't able to draw the divider on top of the blue highlight color.
datalist support everywhere! :) Here's an approach that should work pretty well: - Create a class called BottomBorderDrawable that extends Drawable and draws a border at the bottom of its canvas. (Here's an example of creating a custom Drawable: http://www.vogella.com/articles/AndroidDrawables/article.html#drawables_custom2) - Add a method to set the BottomBorderDrawable's border width and color. - At runtime, set the background of the LinearLayout in autofill_text.xml to an instance of BottomBorderDrawable (you can't set it in xml), and change the width and color as needed. What do you think?
On 2013/09/06 23:40:32, newt wrote: > datalist support everywhere! :) > > Here's an approach that should work pretty well: > - Create a class called BottomBorderDrawable that extends Drawable and draws a > border at the bottom of its canvas. (Here's an example of creating a custom > Drawable: > http://www.vogella.com/articles/AndroidDrawables/article.html#drawables_custom2) > - Add a method to set the BottomBorderDrawable's border width and color. > - At runtime, set the background of the LinearLayout in autofill_text.xml to an > instance of BottomBorderDrawable (you can't set it in xml), and change the width > and color as needed. > > What do you think? Thanks for the advice! I was able to create a custom drawable and remove the View.
https://codereview.chromium.org/23314003/diff/9001/ui/android/java/res/layout... File ui/android/java/res/layout/autofill_text.xml (right): https://codereview.chromium.org/23314003/diff/9001/ui/android/java/res/layout... ui/android/java/res/layout/autofill_text.xml:14: <LinearLayout android:id="@+id/autofill_menu_text" these two linearlayouts can be combined into one https://codereview.chromium.org/23314003/diff/9001/ui/android/java/src/org/ch... File ui/android/java/src/org/chromium/ui/autofill/AutofillDividerDrawable.java (right): https://codereview.chromium.org/23314003/diff/9001/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/autofill/AutofillDividerDrawable.java:16: private Paint paint; mPaint https://codereview.chromium.org/23314003/diff/9001/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/autofill/AutofillDividerDrawable.java:25: RectF rect = new RectF(0, 0, width, 1); avoid allocating memory while drawing: store this rect as a member variable and update its dimensions when drawing (or in onBoundsChange()). Also, the height should be a constant dp (not px). You'll need to multiply by density (getContext().getResources().density) to get dp from px. I think you'll need to pass in density to the constructor of this class, since the context isn't available here. https://codereview.chromium.org/23314003/diff/9001/ui/android/java/src/org/ch... File ui/android/java/src/org/chromium/ui/autofill/AutofillListAdapter.java (right): https://codereview.chromium.org/23314003/diff/9001/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/autofill/AutofillListAdapter.java:10: import android.graphics.drawable.GradientDrawable; unused https://codereview.chromium.org/23314003/diff/9001/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/autofill/AutofillListAdapter.java:65: layout.setPaddingRelative(layout.getPaddingStart(), dividerWidth, getPaddingX() returns user-specific padding plus internal padding (for scroll bars, etc). As a result, you should avoid setting the padding to the current getPadding value. I'd just write: layout.setPadding(0, dividerWidthPx, 0, 0); Also, dividerWidthPx should be calculated as: int dividerWidthDp = 1; // or whatever dividerWidthPx = getContext().getResources().density * dividerWidthDp; https://codereview.chromium.org/23314003/diff/9001/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/autofill/AutofillListAdapter.java:68: divider.setColor(dividerColor); and add divider.setHeight() here https://codereview.chromium.org/23314003/diff/9001/ui/android/java/src/org/ch... File ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java (right): https://codereview.chromium.org/23314003/diff/9001/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:113: getListView().setDividerHeight(0); should this live in the constructor instead? https://codereview.chromium.org/23314003/diff/9001/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:221: AutofillListAdapter adapter = (AutofillListAdapter)parent.getAdapter(); space after cast https://codereview.chromium.org/23314003/diff/9001/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:222: int listIndex = mSuggestions.indexOf(adapter.getItem(position)); could this ever return -1? I'd handle that case, if so.
https://codereview.chromium.org/23314003/diff/9001/ui/android/java/res/layout... File ui/android/java/res/layout/autofill_text.xml (right): https://codereview.chromium.org/23314003/diff/9001/ui/android/java/res/layout... ui/android/java/res/layout/autofill_text.xml:14: <LinearLayout android:id="@+id/autofill_menu_text" On 2013/09/09 20:20:42, newt wrote: > these two linearlayouts can be combined into one I thought that maybe by using two LinearLayouts I can get the total height to be 44dp+1px(divider width). https://codereview.chromium.org/23314003/diff/9001/ui/android/java/src/org/ch... File ui/android/java/src/org/chromium/ui/autofill/AutofillListAdapter.java (right): https://codereview.chromium.org/23314003/diff/9001/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/autofill/AutofillListAdapter.java:65: layout.setPaddingRelative(layout.getPaddingStart(), dividerWidth, On 2013/09/09 20:20:42, newt wrote: > getPaddingX() returns user-specific padding plus internal padding (for scroll > bars, etc). As a result, you should avoid setting the padding to the current > getPadding value. I'd just write: > > layout.setPadding(0, dividerWidthPx, 0, 0); > > Also, dividerWidthPx should be calculated as: > > int dividerWidthDp = 1; // or whatever > dividerWidthPx = getContext().getResources().density * dividerWidthDp; The default ListView seems to have a default dividerWidth of 1px. I've checked divider_horizontal_dark_opaque.9.png in the SDK and it is a 3x3 9-patch for every dpi. https://codereview.chromium.org/23314003/diff/9001/ui/android/java/src/org/ch... File ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java (right): https://codereview.chromium.org/23314003/diff/9001/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:113: getListView().setDividerHeight(0); On 2013/09/09 20:20:42, newt wrote: > should this live in the constructor instead? I've tried and getListView() returns null in the constructor.
https://codereview.chromium.org/23314003/diff/9001/ui/android/java/res/layout... File ui/android/java/res/layout/autofill_text.xml (right): https://codereview.chromium.org/23314003/diff/9001/ui/android/java/res/layout... ui/android/java/res/layout/autofill_text.xml:10: android:layout_width="fill_parent" "fill_parent" -> "match_parent" everywhere https://codereview.chromium.org/23314003/diff/9001/ui/android/java/res/layout... ui/android/java/res/layout/autofill_text.xml:14: <LinearLayout android:id="@+id/autofill_menu_text" On 2013/09/10 09:19:56, keishi1 wrote: > On 2013/09/09 20:20:42, newt wrote: > > these two linearlayouts can be combined into one > > I thought that maybe by using two LinearLayouts I can get the total height to be > 44dp+1px(divider width). If you need to calculate this value (44dp+1px), you can do it in Java code. Or just use 45dp. Alternatively, you could also rid of minHeight, change layout_height to "wrap_content", and add padding to make the LinearLayout the size you want. This will work better anyway if the user changes their system font size. Something like: <LinearLayout android:id="@+id/autofill_menu_text" android:layout_width="match_parent" android:layout_height="wrap_content" android:orientation="vertical" android:paddingTop="6dp" android:paddingBottom="6dp" > <TextView android:id="@+id/autofill_label" ... though this may not center the text the way you'd like. https://codereview.chromium.org/23314003/diff/9001/ui/android/java/src/org/ch... File ui/android/java/src/org/chromium/ui/autofill/AutofillListAdapter.java (right): https://codereview.chromium.org/23314003/diff/9001/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/autofill/AutofillListAdapter.java:65: layout.setPaddingRelative(layout.getPaddingStart(), dividerWidth, On 2013/09/10 09:19:56, keishi1 wrote: > On 2013/09/09 20:20:42, newt wrote: > > getPaddingX() returns user-specific padding plus internal padding (for scroll > > bars, etc). As a result, you should avoid setting the padding to the current > > getPadding value. I'd just write: > > > > layout.setPadding(0, dividerWidthPx, 0, 0); > > > > Also, dividerWidthPx should be calculated as: > > > > int dividerWidthDp = 1; // or whatever > > dividerWidthPx = getContext().getResources().density * dividerWidthDp; > > The default ListView seems to have a default dividerWidth of 1px. > I've checked divider_horizontal_dark_opaque.9.png in the SDK and it is a 3x3 > 9-patch for every dpi. divider_horizontal_dark_opaque.9.png is just a single pixel that gets stretched to whatever size is needed. You can override the divider color and/or height using ListView.setDivider() and ListView.setDividerHeight(). I think you should override the color, so the color of the dividers between adjacent elements matches the color of the extra divider between the two groups. https://codereview.chromium.org/23314003/diff/9001/ui/android/java/src/org/ch... File ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java (right): https://codereview.chromium.org/23314003/diff/9001/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:113: getListView().setDividerHeight(0); On 2013/09/10 09:19:56, keishi1 wrote: > On 2013/09/09 20:20:42, newt wrote: > > should this live in the constructor instead? > > I've tried and getListView() returns null in the constructor. ah, the ListView probably isn't constructed until it's needed. this is fine then.
https://codereview.chromium.org/23314003/diff/9001/ui/android/java/res/layout... File ui/android/java/res/layout/autofill_text.xml (right): https://codereview.chromium.org/23314003/diff/9001/ui/android/java/res/layout... ui/android/java/res/layout/autofill_text.xml:14: <LinearLayout android:id="@+id/autofill_menu_text" On 2013/09/10 17:45:10, newt wrote: > On 2013/09/10 09:19:56, keishi1 wrote: > > On 2013/09/09 20:20:42, newt wrote: > > > these two linearlayouts can be combined into one > > > > I thought that maybe by using two LinearLayouts I can get the total height to > be > > 44dp+1px(divider width). > If you need to calculate this value (44dp+1px), you can do it in Java code. Or > just use 45dp. I implemented 44dp + 1px. > Alternatively, you could also rid of minHeight, change layout_height to > "wrap_content", and add padding to make the LinearLayout the size you want. This > will work better anyway if the user changes their system font size. Something > like: > > <LinearLayout > android:id="@+id/autofill_menu_text" > android:layout_width="match_parent" > android:layout_height="wrap_content" > android:orientation="vertical" > android:paddingTop="6dp" > android:paddingBottom="6dp" > > > <TextView android:id="@+id/autofill_label" > ... > > though this may not center the text the way you'd like. The sublabel may or may not exist but we still want the same height for all items so I think we can't do this. https://codereview.chromium.org/23314003/diff/9001/ui/android/java/src/org/ch... File ui/android/java/src/org/chromium/ui/autofill/AutofillDividerDrawable.java (right): https://codereview.chromium.org/23314003/diff/9001/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/autofill/AutofillDividerDrawable.java:16: private Paint paint; On 2013/09/09 20:20:42, newt wrote: > mPaint Done. https://codereview.chromium.org/23314003/diff/9001/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/autofill/AutofillDividerDrawable.java:25: RectF rect = new RectF(0, 0, width, 1); On 2013/09/09 20:20:42, newt wrote: > avoid allocating memory while drawing: store this rect as a member variable and > update its dimensions when drawing (or in onBoundsChange()). > > Also, the height should be a constant dp (not px). You'll need to multiply by > density (getContext().getResources().density) to get dp from px. I think you'll > need to pass in density to the constructor of this class, since the context > isn't available here. Done. https://codereview.chromium.org/23314003/diff/9001/ui/android/java/src/org/ch... File ui/android/java/src/org/chromium/ui/autofill/AutofillListAdapter.java (right): https://codereview.chromium.org/23314003/diff/9001/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/autofill/AutofillListAdapter.java:10: import android.graphics.drawable.GradientDrawable; On 2013/09/09 20:20:42, newt wrote: > unused Done. https://codereview.chromium.org/23314003/diff/9001/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/autofill/AutofillListAdapter.java:65: layout.setPaddingRelative(layout.getPaddingStart(), dividerWidth, On 2013/09/10 17:45:10, newt wrote: > On 2013/09/10 09:19:56, keishi1 wrote: > > On 2013/09/09 20:20:42, newt wrote: > > > getPaddingX() returns user-specific padding plus internal padding (for > scroll > > > bars, etc). As a result, you should avoid setting the padding to the current > > > getPadding value. I'd just write: > > > > > > layout.setPadding(0, dividerWidthPx, 0, 0); > > > > > > Also, dividerWidthPx should be calculated as: > > > > > > int dividerWidthDp = 1; // or whatever > > > dividerWidthPx = getContext().getResources().density * dividerWidthDp; > > > > The default ListView seems to have a default dividerWidth of 1px. > > I've checked divider_horizontal_dark_opaque.9.png in the SDK and it is a 3x3 > > 9-patch for every dpi. > > divider_horizontal_dark_opaque.9.png is just a single pixel that gets stretched > to whatever size is needed. You can override the divider color and/or height > using ListView.setDivider() and ListView.setDividerHeight(). I think you should > override the color, so the color of the dividers between adjacent elements > matches the color of the extra divider between the two groups. I'm not sure what you are getting at. We don't use the ListView divider at all. ListView uses a single drawable to draw all dividers, so we can't change dividers unless we subclass ListView. We can't use a subclassed ListView because we the ListView is constructed by the ListPopupWindow. https://codereview.chromium.org/23314003/diff/9001/ui/android/java/src/org/ch... File ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java (right): https://codereview.chromium.org/23314003/diff/9001/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:221: AutofillListAdapter adapter = (AutofillListAdapter)parent.getAdapter(); On 2013/09/09 20:20:42, newt wrote: > space after cast Done. https://codereview.chromium.org/23314003/diff/9001/ui/android/java/src/org/ch... ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:222: int listIndex = mSuggestions.indexOf(adapter.getItem(position)); On 2013/09/09 20:20:42, newt wrote: > could this ever return -1? I'd handle that case, if so. It shouldn't return -1. Added an assertion.
It has been a while. Could you please take a look again. Thanks.
a few nits, then lgtm. sorry I lost track of this. thanks! https://codereview.chromium.org/23314003/diff/24001/ui/android/java/res/layou... File ui/android/java/res/layout/autofill_text.xml (right): https://codereview.chromium.org/23314003/diff/24001/ui/android/java/res/layou... ui/android/java/res/layout/autofill_text.xml:10: android:layout_width="match_parent" since you're not actually using the layout_width and layout_height values defined here, it'd be nice to remove them. (this may cause a compile error, in which case don't worry about it) https://codereview.chromium.org/23314003/diff/24001/ui/android/java/res/layou... ui/android/java/res/layout/autofill_text.xml:22: android:layout_marginStart="10dp" nit: put attributes in this order: android:id android:layout_width android:layout_height android:layout_* android:* and put android:id on its own line (not on same line as "TextView") https://codereview.chromium.org/23314003/diff/24001/ui/android/java/src/org/c... File ui/android/java/src/org/chromium/ui/autofill/AutofillDividerDrawable.java (right): https://codereview.chromium.org/23314003/diff/24001/ui/android/java/src/org/c... ui/android/java/src/org/chromium/ui/autofill/AutofillDividerDrawable.java:18: private RectF mDividerRect; mDividerRect always has integer coordinates, so I'd use Rect instead of RectF https://codereview.chromium.org/23314003/diff/24001/ui/android/java/src/org/c... File ui/android/java/src/org/chromium/ui/autofill/AutofillListAdapter.java (right): https://codereview.chromium.org/23314003/diff/24001/ui/android/java/src/org/c... ui/android/java/src/org/chromium/ui/autofill/AutofillListAdapter.java:61: if (mSeparators.contains(position)) { do you want the same divider height for dark dividers as for normal dividers?
Thanks! https://codereview.chromium.org/23314003/diff/24001/ui/android/java/res/layou... File ui/android/java/res/layout/autofill_text.xml (right): https://codereview.chromium.org/23314003/diff/24001/ui/android/java/res/layou... ui/android/java/res/layout/autofill_text.xml:10: android:layout_width="match_parent" On 2013/10/03 16:34:51, newt wrote: > since you're not actually using the layout_width and layout_height values > defined here, it'd be nice to remove them. (this may cause a compile error, in > which case don't worry about it) Done. https://codereview.chromium.org/23314003/diff/24001/ui/android/java/res/layou... ui/android/java/res/layout/autofill_text.xml:22: android:layout_marginStart="10dp" On 2013/10/03 16:34:51, newt wrote: > nit: put attributes in this order: > > android:id > android:layout_width > android:layout_height > android:layout_* > android:* > > and put android:id on its own line (not on same line as "TextView") Done. https://codereview.chromium.org/23314003/diff/24001/ui/android/java/src/org/c... File ui/android/java/src/org/chromium/ui/autofill/AutofillDividerDrawable.java (right): https://codereview.chromium.org/23314003/diff/24001/ui/android/java/src/org/c... ui/android/java/src/org/chromium/ui/autofill/AutofillDividerDrawable.java:18: private RectF mDividerRect; On 2013/10/03 16:34:51, newt wrote: > mDividerRect always has integer coordinates, so I'd use Rect instead of RectF Done. https://codereview.chromium.org/23314003/diff/24001/ui/android/java/src/org/c... File ui/android/java/src/org/chromium/ui/autofill/AutofillListAdapter.java (right): https://codereview.chromium.org/23314003/diff/24001/ui/android/java/src/org/c... ui/android/java/src/org/chromium/ui/autofill/AutofillListAdapter.java:61: if (mSeparators.contains(position)) { On 2013/10/03 16:34:51, newt wrote: > do you want the same divider height for dark dividers as for normal dividers? Yes. Same height just darker color.
lgtm with some nits https://codereview.chromium.org/23314003/diff/24001/ui/android/java/src/org/c... File ui/android/java/src/org/chromium/ui/autofill/AutofillDividerDrawable.java (right): https://codereview.chromium.org/23314003/diff/24001/ui/android/java/src/org/c... ui/android/java/src/org/chromium/ui/autofill/AutofillDividerDrawable.java:5: nit: extra blank line https://codereview.chromium.org/23314003/diff/24001/ui/android/java/src/org/c... ui/android/java/src/org/chromium/ui/autofill/AutofillDividerDrawable.java:53: return 255; can you add comment or perhaps use a constant for this? https://codereview.chromium.org/23314003/diff/24001/ui/android/java/src/org/c... ui/android/java/src/org/chromium/ui/autofill/AutofillDividerDrawable.java:55: nit: extra line
Thanks https://codereview.chromium.org/23314003/diff/24001/ui/android/java/src/org/c... File ui/android/java/src/org/chromium/ui/autofill/AutofillDividerDrawable.java (right): https://codereview.chromium.org/23314003/diff/24001/ui/android/java/src/org/c... ui/android/java/src/org/chromium/ui/autofill/AutofillDividerDrawable.java:5: On 2013/11/06 17:33:56, Miguel Garcia wrote: > nit: extra blank line Done. https://codereview.chromium.org/23314003/diff/24001/ui/android/java/src/org/c... ui/android/java/src/org/chromium/ui/autofill/AutofillDividerDrawable.java:53: return 255; On 2013/11/06 17:33:56, Miguel Garcia wrote: > can you add comment or perhaps use a constant for this? I found a constant already defined android.graphics.PixelFormat.OPAQUE https://codereview.chromium.org/23314003/diff/24001/ui/android/java/src/org/c... ui/android/java/src/org/chromium/ui/autofill/AutofillDividerDrawable.java:55: On 2013/11/06 17:33:56, Miguel Garcia wrote: > nit: extra line Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/keishi@chromium.org/23314003/45001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/keishi@chromium.org/23314003/155001
Sorry for I got bad news for ya. Compile failed with a clobber build on android_aosp. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_ao... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
+primiano could you review the change to R.java? Thanks.
LGTM. Thanks for the cc.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/keishi@chromium.org/23314003/375001
The commit queue went berserk retrying too often for a seemingly flaky test on builder android_dbg: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
On 2013/11/08 01:57:58, I haz the power (commit-bot) wrote: > The commit queue went berserk retrying too often for a > seemingly flaky test on builder android_dbg: > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... Can anyone tell me why this would fail android_dbg? It passes when I try locally on My Nexus 7. C 151s Main ******************************************************************************** C 151s Main Detailed Logs C 151s Main ******************************************************************************** C 151s Main [CRASH] org.chromium.chrome.browser.autofill.AutofillTest#testAutofillClickFirstSuggestion: C 151s Main java.lang.NoSuchMethodError: android.view.View.setBackground C 151s Main [CRASH] org.chromium.chrome.browser.autofill.AutofillTest#testAutofillWithDifferentNumberSuggestions: C 151s Main java.lang.NoSuchMethodError: android.view.View.setBackground C 151s Main ********************************************************************************
On 2013/11/08 08:08:17, keishi1 wrote: > On 2013/11/08 01:57:58, I haz the power (commit-bot) wrote: > > The commit queue went berserk retrying too often for a > > seemingly flaky test on builder android_dbg: > > > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... > > > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... > > > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... > > > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... > > > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... > > Can anyone tell me why this would fail android_dbg? It passes when I try locally > on My Nexus 7. > > C 151s Main > ******************************************************************************** > C 151s Main Detailed Logs > C 151s Main > ******************************************************************************** > C 151s Main [CRASH] > org.chromium.chrome.browser.autofill.AutofillTest#testAutofillClickFirstSuggestion: > C 151s Main java.lang.NoSuchMethodError: android.view.View.setBackground > C 151s Main [CRASH] > org.chromium.chrome.browser.autofill.AutofillTest#testAutofillWithDifferentNumberSuggestions: > C 151s Main java.lang.NoSuchMethodError: android.view.View.setBackground > C 151s Main > ******************************************************************************** View.setBackground() was added in a recent version of Android, and isn't available on ICS devices. Looks like the bot has an ICS device attached. It's a good thing the bot found this, and not our users! :) To fix: use ApiCompatibilityUtils.setBackgroundForView() (that's base/android/java/src/org/chromium/base/ApiCompatibilityUtils.java). You can grep to find a couple examples in the codebase.
On 2013/11/08 17:18:56, newt wrote: > On 2013/11/08 08:08:17, keishi1 wrote: > > On 2013/11/08 01:57:58, I haz the power (commit-bot) wrote: > > > The commit queue went berserk retrying too often for a > > > seemingly flaky test on builder android_dbg: > > > > > > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... > > > > > > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... > > > > > > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... > > > > > > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... > > > > > > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... > > > > Can anyone tell me why this would fail android_dbg? It passes when I try > locally > > on My Nexus 7. > > > > C 151s Main > > > ******************************************************************************** > > C 151s Main Detailed Logs > > C 151s Main > > > ******************************************************************************** > > C 151s Main [CRASH] > > > org.chromium.chrome.browser.autofill.AutofillTest#testAutofillClickFirstSuggestion: > > C 151s Main java.lang.NoSuchMethodError: android.view.View.setBackground > > C 151s Main [CRASH] > > > org.chromium.chrome.browser.autofill.AutofillTest#testAutofillWithDifferentNumberSuggestions: > > C 151s Main java.lang.NoSuchMethodError: android.view.View.setBackground > > C 151s Main > > > ******************************************************************************** > > View.setBackground() was added in a recent version of Android, and isn't > available on ICS devices. Looks like the bot has an ICS device attached. It's a > good thing the bot found this, and not our users! :) > > To fix: use ApiCompatibilityUtils.setBackgroundForView() (that's > base/android/java/src/org/chromium/base/ApiCompatibilityUtils.java). You can > grep to find a couple examples in the codebase. Thanks! I would have never suspected that.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/keishi@chromium.org/23314003/765001
Message was sent while issue was closed.
Change committed as 234194
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/keishi@chromium.org/23314003/765001
Message was sent while issue was closed.
Change committed as 234378 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
