Avoid creating new CheckedTextViews in AlertDialog for <select>

Avoid creating new CheckedTextViews in AlertDialog for <select>

BUG=649795
Committed: https://crrev.com/de19ad74627c0db106df590c7fd3a22d7bc0e5e7
Cr-Commit-Position: refs/heads/master@{#422690}

[wychen, 2016-09-30 23:14:59]

Patchset #1 (id:1) has been deleted

[wychen, 2016-09-30 23:15:41]

wychen@chromium.org changed reviewers:
+ aelias@chromium.org, tedchoc@chromium.org

[wychen, 2016-09-30 23:15:42]

PTAL

[Ted C, 2016-10-03 21:55:47]

nice find!

https://codereview.chromium.org/2381063003/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/SelectPopupAdapter.java
(right):

https://codereview.chromium.org/2381063003/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/SelectPopupAdapter.java:74:
mDrawableStack.add(view.getCheckMarkDrawable());
instead of needing a stack, I wonder if we could use this:
https://developer.android.com/reference/android/view/View.html#setTag(int,
java.lang.Object)

Something like:

view.setTag(android.R.id.icon, view.getCheckMarkDrawable());

Whether we should define our own tag ID or not, I'm not super sure.  But then
the views are fully self contained and we can pull it out of the view that owned
it initially in the else below.  seems "slightly" better to me.  what do you
think?

[wychen, 2016-10-04 00:19:36]

https://codereview.chromium.org/2381063003/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/SelectPopupAdapter.java
(right):

https://codereview.chromium.org/2381063003/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/SelectPopupAdapter.java:74:
mDrawableStack.add(view.getCheckMarkDrawable());
On 2016/10/03 21:55:47, Ted C wrote:
> instead of needing a stack, I wonder if we could use this:
> https://developer.android.com/reference/android/view/View.html#setTag(int,
> java.lang.Object)
> 
> Something like:
> 
> view.setTag(android.R.id.icon, view.getCheckMarkDrawable());
> 
> Whether we should define our own tag ID or not, I'm not super sure.  But then
> the views are fully self contained and we can pull it out of the view that
owned
> it initially in the else below.  seems "slightly" better to me.  what do you
> think?

Done.

Self-contained views are much better! Thanks for the pointer. If we need to use
ID, I think it's safer to create a new ID, but we might be able to use the one
without ID.

https://developer.android.com/reference/android/view/View.html#setTag(java.la...

[Ted C, 2016-10-04 00:25:40]

lgtm

[wychen, 2016-10-04 02:48:49]

One more side note: the Stack implementation had some artifacts. In <select
multiple>, when some boxes are checked, and the list is several screens long,
partial animations can be seen if scrolling up and down very fast. This is gone
after using tag.

[wychen, 2016-10-04 02:49:13]

The CQ bit was checked by wychen@chromium.org

[commit-bot: I haz the power, 2016-10-04 02:49:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-04 03:34:14]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-10-04 03:36:28]

Description was changed from

==========
Avoid creating new CheckedTextViews in AlertDialog for <select>

BUG=649795
==========

to

==========
Avoid creating new CheckedTextViews in AlertDialog for <select>

BUG=649795

Committed: https://crrev.com/de19ad74627c0db106df590c7fd3a22d7bc0e5e7
Cr-Commit-Position: refs/heads/master@{#422690}
==========

[commit-bot: I haz the power, 2016-10-04 03:36:28]

Patchset 2 (id:??) landed as
https://crrev.com/de19ad74627c0db106df590c7fd3a22d7bc0e5e7
Cr-Commit-Position: refs/heads/master@{#422690}