Show Ash like <select> popup on Android tablets

Show Ash like <select> popup on Android tablets

We should show Ash like <select> popups on Android tablets so the UI is closer to Chrome OS.

The new popup will only apply to <select size=1> as <select multiple> will be shown as a listbox like the Desktop versions.

BUG=314713
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268062

[keishi, 2014-04-10 15:18:02]

I've talked to the ui team (sgabriel@ and roma@) and agreed that we do not want
to introduce a completely new popup UI just for Android tablets, and that we
should make the UI on Android tablets look like Chrome OS with a touch screen.

Here is the initial design doc:
https://docs.google.com/a/google.com/document/d/1KL2EtVzIHSEH2RGp024JcVJDsXJQ...
Here is the screenshot with this patch applied:
https://drive.google.com/file/d/0B4aiM9jljUy8Vjl0NXlma0ZDWG8/edit?usp=sharing

[Ted C, 2014-04-12 01:10:27]

I think you might want to take a peek at how AutofillDialog works, as I think
these should behave somewhat similarly (adding aurimas@ in case there are any
pointers).

Looks like you might have switched your hdpi and xhdpi assets.

Also, I would comment on the bug about your open questions for the designs and
get them to comment as I'm not sure it will happen in the doc.

https://codereview.chromium.org/231953003/diff/60001/content/browser/android/...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/231953003/diff/60001/content/browser/android/...
content/browser/android/content_view_core_impl.cc:519: const gfx::Rect& bounds,
const std::vector<MenuItem>& items,
this formatting wasn't correct before, it should actually have a param on each
line since it doesn't all fit on one.

https://codereview.chromium.org/231953003/diff/60001/content/browser/android/...
File content/browser/android/content_view_core_impl.h (right):

https://codereview.chromium.org/231953003/diff/60001/content/browser/android/...
content/browser/android/content_view_core_impl.h:250: double item_font_size,
In my opinion, we should not use the font size but should use something that is
well suited for a touch platform.

https://codereview.chromium.org/231953003/diff/60001/content/public/android/j...
File content/public/android/java/res/drawable/select_popup_item_selector.xml
(right):

https://codereview.chromium.org/231953003/diff/60001/content/public/android/j...
content/public/android/java/res/drawable/select_popup_item_selector.xml:2:
<selector xmlns:android="http://schemas.android.com/apk/res/android">
missing copyright above this.

https://codereview.chromium.org/231953003/diff/60001/content/public/android/j...
content/public/android/java/res/drawable/select_popup_item_selector.xml:4: <item
android:state_pressed="true">
Actually, I wouldn't recommend using your own selection drawable.  I think using
the default android version is better and more compliant with the platform.

https://codereview.chromium.org/231953003/diff/60001/content/public/android/j...
File content/public/android/java/res/layout/select_popup_dialog.xml (right):

https://codereview.chromium.org/231953003/diff/60001/content/public/android/j...
content/public/android/java/res/layout/select_popup_dialog.xml:2: 
missing the copyright

https://codereview.chromium.org/231953003/diff/60001/content/public/android/j...
File content/public/android/java/res/layout/select_popup_item.xml (right):

https://codereview.chromium.org/231953003/diff/60001/content/public/android/j...
content/public/android/java/res/layout/select_popup_item.xml:11:
android:minHeight="38dp"
minHeight should be 48dp per android touch guidelines.  We should also ensure
it's at least 48dp wide (and in reality, we should make the minimum something
more obvious like 100dp).

https://codereview.chromium.org/231953003/diff/60001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/231953003/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2291:
public RenderCoordinates renderCoordinates() {
This method already exists:
getRenderCoordinates

https://codereview.chromium.org/231953003/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2413:
double item_font_size, boolean multiple,
camel cased param names in java

Should update the javadoc as well

https://codereview.chromium.org/231953003/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2427:
item_font_size, multiple,
camel casing here too...and indenting should be 8 from the start of the previous
line.

And if all params fit on the second line together, then put them all there
instead of 3 and 3.

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

https://codereview.chromium.org/231953003/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/SelectPopupAdapter.java:55:
public void setTextSize(float textSize) {
all public methods should have javadocs (like is this in px or dp)  is the min
height for the items in the list or the list itself?

https://codereview.chromium.org/231953003/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/SelectPopupAdapter.java:82:
if (mPadding >= 0)
braces are required in java if the conditional and statement don't all fit on a
single line.

https://codereview.chromium.org/231953003/diff/60001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/input/SelectPopupDialog.java
(right):

https://codereview.chromium.org/231953003/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/SelectPopupDialog.java:36:
public class SelectPopupDialog implements OnItemClickListener,
With the amount of tablet specific checks, I think you should extract a tablet
version and have a common interface to interact with both styles of views.

https://codereview.chromium.org/231953003/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/SelectPopupDialog.java:37:
DialogInterface.OnCancelListener,
same about indenting, needs to be 8 from the start of the previous line.

https://codereview.chromium.org/231953003/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/SelectPopupDialog.java:64:
mListBoxPopup = new Dialog(mContext,
android.R.style.Theme_Translucent_NoTitleBar);
Instead of using a regular Dialog, we should be using a ListPopupWindow

https://codereview.chromium.org/231953003/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/SelectPopupDialog.java:72:
public void onClick(View v) {
needs @Override

https://codereview.chromium.org/231953003/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/SelectPopupDialog.java:109:
int[] selected) {
indenting

https://codereview.chromium.org/231953003/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/SelectPopupDialog.java:111:
if (!DeviceUtils.isTablet(mContext))
needs braces

https://codereview.chromium.org/231953003/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/SelectPopupDialog.java:117:
renderCoordinates.fromLocalCssToPix((float)itemFontSize));
I'm not sure we want to scale these based on page scale.  We should get feedback
from a designer, but this seems odd to me.

And you need spaces between casts and the param name.

https://codereview.chromium.org/231953003/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/SelectPopupDialog.java:194:
view = adapter.getView(i, view, fakeParent);
Hmm..I think you might be able to pass null as the parent (especially if you
don't use it).

https://codereview.chromium.org/231953003/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/SelectPopupDialog.java:197:
LinearLayout.LayoutParams.WRAP_CONTENT);
java indenting

https://codereview.chromium.org/231953003/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/SelectPopupDialog.java:200:
int width = view.getMeasuredWidth();
this could just be

maxWidth = Math.max(maxWidth, view.getMeasuredWidth());

https://codereview.chromium.org/231953003/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/SelectPopupDialog.java:215:
MeasureSpec.makeMeasureSpec(width, MeasureSpec.EXACTLY),
these lines should be +4 indented

https://codereview.chromium.org/231953003/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/SelectPopupDialog.java:234:
* reused.
align this with "The rect" above

https://codereview.chromium.org/231953003/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/SelectPopupDialog.java:237:
if (!DeviceUtils.isTablet(mContext)) {
I would make this a one liner since it is compact and easy to read.

if (!DeviceUtils.isTablet(mContext)) return;

[keishi, 2014-04-15 14:41:07]

I've rewritten the patch using ListPopupWindow. PTAL.

> Also, I would comment on the bug about your open questions for the designs and
> get them to comment as I'm not sure it will happen in the doc.

I haven't updated the document but I had those questions answered over VC.

https://codereview.chromium.org/231953003/diff/60001/content/browser/android/...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/231953003/diff/60001/content/browser/android/...
content/browser/android/content_view_core_impl.cc:519: const gfx::Rect& bounds,
const std::vector<MenuItem>& items,
On 2014/04/12 01:10:27, Ted C wrote:
> this formatting wasn't correct before, it should actually have a param on each
> line since it doesn't all fit on one.

Done.

https://codereview.chromium.org/231953003/diff/60001/content/browser/android/...
File content/browser/android/content_view_core_impl.h (right):

https://codereview.chromium.org/231953003/diff/60001/content/browser/android/...
content/browser/android/content_view_core_impl.h:250: double item_font_size,
On 2014/04/12 01:10:27, Ted C wrote:
> In my opinion, we should not use the font size but should use something that
is
> well suited for a touch platform.

I've asked sgabriel@ if he wants the popup font size to be bigger when zoomed in
and he said yes. But I understand your concert so I am going to ignore font size
for this patch and asked him again.

https://codereview.chromium.org/231953003/diff/60001/content/public/android/j...
File content/public/android/java/res/drawable/select_popup_item_selector.xml
(right):

https://codereview.chromium.org/231953003/diff/60001/content/public/android/j...
content/public/android/java/res/drawable/select_popup_item_selector.xml:4: <item
android:state_pressed="true">
On 2014/04/12 01:10:27, Ted C wrote:
> Actually, I wouldn't recommend using your own selection drawable.  I think
using
> the default android version is better and more compliant with the platform.
> 

Done.

https://codereview.chromium.org/231953003/diff/60001/content/public/android/j...
File content/public/android/java/res/layout/select_popup_item.xml (right):

https://codereview.chromium.org/231953003/diff/60001/content/public/android/j...
content/public/android/java/res/layout/select_popup_item.xml:11:
android:minHeight="38dp"
On 2014/04/12 01:10:27, Ted C wrote:
> minHeight should be 48dp per android touch guidelines.  We should also ensure
> it's at least 48dp wide (and in reality, we should make the minimum something
> more obvious like 100dp).

Done.

https://codereview.chromium.org/231953003/diff/60001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/231953003/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2291:
public RenderCoordinates renderCoordinates() {
On 2014/04/12 01:10:27, Ted C wrote:
> This method already exists:
> getRenderCoordinates

Done.

https://codereview.chromium.org/231953003/diff/60001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/input/SelectPopupDialog.java
(right):

https://codereview.chromium.org/231953003/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/SelectPopupDialog.java:64:
mListBoxPopup = new Dialog(mContext,
android.R.style.Theme_Translucent_NoTitleBar);
On 2014/04/12 01:10:27, Ted C wrote:
> Instead of using a regular Dialog, we should be using a ListPopupWindow

Done.

https://codereview.chromium.org/231953003/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/SelectPopupDialog.java:194:
view = adapter.getView(i, view, fakeParent);
On 2014/04/12 01:10:27, Ted C wrote:
> Hmm..I think you might be able to pass null as the parent (especially if you
> don't use it).

Done.

https://codereview.chromium.org/231953003/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/SelectPopupDialog.java:197:
LinearLayout.LayoutParams.WRAP_CONTENT);
On 2014/04/12 01:10:27, Ted C wrote:
> java indenting

Done.

https://codereview.chromium.org/231953003/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/SelectPopupDialog.java:200:
int width = view.getMeasuredWidth();
On 2014/04/12 01:10:27, Ted C wrote:
> this could just be
> 
> maxWidth = Math.max(maxWidth, view.getMeasuredWidth());

Done.

[aurimas (slooooooooow), 2014-04-16 01:36:13]

It is a good attempt at implementing this feature, however most of the code is a
simple copy of AutofillPopup.java[1].
I would really like to to reuse AutofillPopup.java instead. Can we create a
DropdownPopup.java that extracts all the common code between AutofillPopup and
SingleSelectPopupAdapter?

Additionally, I would keep the style/looks of the popup closer to Android: keep
the separators between items, use the same background drawable (not the new blue
drawable), etc...

Thoughts?

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/ui/android/java/sr...

[keishi, 2014-04-16 15:17:06]

On 2014/04/16 01:36:13, aurimas wrote:
> It is a good attempt at implementing this feature, however most of the code is
a
> simple copy of AutofillPopup.java[1].
> I would really like to to reuse AutofillPopup.java instead. Can we create a
> DropdownPopup.java that extracts all the common code between AutofillPopup and
> SingleSelectPopupAdapter?
> 
> Additionally, I would keep the style/looks of the popup closer to Android:
keep
> the separators between items, use the same background drawable (not the new
blue
> drawable), etc...
> 
> Thoughts?
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/android/java/sr...

I made the select popup share the same appearance and code for autofill popup.

[aurimas (slooooooooow), 2014-04-16 17:36:48]

Please take a look at my comments.

You will have to land the drawables (pngs) in a separate CL and dcommit it
because rietveld corrupts them.

https://codereview.chromium.org/231953003/diff/100001/content/public/android/...
File content/public/android/java/res/layout/select_popup_item.xml (right):

https://codereview.chromium.org/231953003/diff/100001/content/public/android/...
content/public/android/java/res/layout/select_popup_item.xml:3: <TextView
xmlns:android="http://schemas.android.com/apk/res/android"
You should be able to reuse ui/android/java/res/layout/dropdown_item.xml. If you
set the label only it should look the same as this.

https://codereview.chromium.org/231953003/diff/100001/content/public/android/...
File content/public/android/java/res/values-v17/styles.xml (right):

https://codereview.chromium.org/231953003/diff/100001/content/public/android/...
content/public/android/java/res/values-v17/styles.xml:8: <resources
xmlns:android="http://schemas.android.com/apk/res/android">
This is not needed. Please revert this change.

https://codereview.chromium.org/231953003/diff/100001/content/public/android/...
File content/public/android/java/res/values/dimens.xml (right):

https://codereview.chromium.org/231953003/diff/100001/content/public/android/...
content/public/android/java/res/values/dimens.xml:11: <dimen
name="select_popup_horizontal_margin">3dp</dimen>
All these seem to be unused?

https://codereview.chromium.org/231953003/diff/100001/content/public/android/...
File content/public/android/java/resource_map/org/chromium/content/R.java
(right):

https://codereview.chromium.org/231953003/diff/100001/content/public/android/...
content/public/android/java/resource_map/org/chromium/content/R.java:27: public
static int select_popup_border_width;
These seem to be unused?

https://codereview.chromium.org/231953003/diff/100001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/231953003/diff/100001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2395:
private void showSelectPopup(Rect bounds, String[] items, int[] enabled, boolean
multiple,
Can we move most of the code in this method to SelectPopup.java? SelectPopup can
then deside if it wants to show dialog or a popup and do all the set-up for the
popup items. ContentViewCore looks like a wrong place to keep this code.

https://codereview.chromium.org/231953003/diff/100001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2419:
for (int i = 0; i < popupItems.size(); i++) {
We are copying items for the second time now (line 2403 and this). Please make
sure we only do it once.

https://codereview.chromium.org/231953003/diff/100001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2444:
if (mSelectPopupDialog != null) {
This can we simplified when we have SelectPopup that handles both Dialog and
Popup.

https://codereview.chromium.org/231953003/diff/100001/ui/android/java/res/val...
File ui/android/java/res/values-v17/styles.xml (right):

https://codereview.chromium.org/231953003/diff/100001/ui/android/java/res/val...
ui/android/java/res/values-v17/styles.xml:10: <style name="DropdownPopupWindow"
parent="@android:style/Widget.ListPopupWindow">
Change the <tab> back to spaces.

https://codereview.chromium.org/231953003/diff/100001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java (right):

https://codereview.chromium.org/231953003/diff/100001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:75: public void
show() {
Move this method to DropdownPopupWindow.java so that all classes extending it
get it?

https://codereview.chromium.org/231953003/diff/100001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:85: public void
show(AutofillSuggestion[] suggestions) {
Please rename this to filterAndShow

[aurimas (slooooooooow), 2014-04-16 17:37:53]

Also, please attach screenshots of how this new popup looks after this CL to the
associated bug.

[keishi, 2014-04-17 12:25:40]

Here are the screenshots:
https://drive.google.com/file/d/0B4aiM9jljUy8M29TclY0ZW9YdWs/edit?usp=sharing
https://drive.google.com/file/d/0B4aiM9jljUy8MlJ2Z2JLTUFMSmM/edit?usp=sharing

https://codereview.chromium.org/231953003/diff/100001/content/public/android/...
File content/public/android/java/res/layout/select_popup_item.xml (right):

https://codereview.chromium.org/231953003/diff/100001/content/public/android/...
content/public/android/java/res/layout/select_popup_item.xml:3: <TextView
xmlns:android="http://schemas.android.com/apk/res/android"
On 2014/04/16 17:36:48, aurimas wrote:
> You should be able to reuse ui/android/java/res/layout/dropdown_item.xml. If
you
> set the label only it should look the same as this.

This file wasn't used. Removed.

https://codereview.chromium.org/231953003/diff/100001/content/public/android/...
File content/public/android/java/res/values-v17/styles.xml (right):

https://codereview.chromium.org/231953003/diff/100001/content/public/android/...
content/public/android/java/res/values-v17/styles.xml:8: <resources
xmlns:android="http://schemas.android.com/apk/res/android">
On 2014/04/16 17:36:48, aurimas wrote:
> This is not needed. Please revert this change.

Done.

https://codereview.chromium.org/231953003/diff/100001/content/public/android/...
File content/public/android/java/res/values/dimens.xml (right):

https://codereview.chromium.org/231953003/diff/100001/content/public/android/...
content/public/android/java/res/values/dimens.xml:11: <dimen
name="select_popup_horizontal_margin">3dp</dimen>
On 2014/04/16 17:36:48, aurimas wrote:
> All these seem to be unused?

Done.

https://codereview.chromium.org/231953003/diff/100001/content/public/android/...
File content/public/android/java/resource_map/org/chromium/content/R.java
(right):

https://codereview.chromium.org/231953003/diff/100001/content/public/android/...
content/public/android/java/resource_map/org/chromium/content/R.java:27: public
static int select_popup_border_width;
On 2014/04/16 17:36:48, aurimas wrote:
> These seem to be unused?

Done.

https://codereview.chromium.org/231953003/diff/100001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/231953003/diff/100001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2395:
private void showSelectPopup(Rect bounds, String[] items, int[] enabled, boolean
multiple,
On 2014/04/16 17:36:48, aurimas wrote:
> Can we move most of the code in this method to SelectPopup.java? SelectPopup
can
> then deside if it wants to show dialog or a popup and do all the set-up for
the
> popup items. ContentViewCore looks like a wrong place to keep this code.

Done.

https://codereview.chromium.org/231953003/diff/100001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2419:
for (int i = 0; i < popupItems.size(); i++) {
On 2014/04/16 17:36:48, aurimas wrote:
> We are copying items for the second time now (line 2403 and this). Please make
> sure we only do it once.

Done.

https://codereview.chromium.org/231953003/diff/100001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2444:
if (mSelectPopupDialog != null) {
On 2014/04/16 17:36:48, aurimas wrote:
> This can we simplified when we have SelectPopup that handles both Dialog and
> Popup.

Done.

https://codereview.chromium.org/231953003/diff/100001/ui/android/java/res/val...
File ui/android/java/res/values-v17/styles.xml (right):

https://codereview.chromium.org/231953003/diff/100001/ui/android/java/res/val...
ui/android/java/res/values-v17/styles.xml:10: <style name="DropdownPopupWindow"
parent="@android:style/Widget.ListPopupWindow">
On 2014/04/16 17:36:48, aurimas wrote:
> Change the <tab> back to spaces.

Done.

https://codereview.chromium.org/231953003/diff/100001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java (right):

https://codereview.chromium.org/231953003/diff/100001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:75: public void
show() {
On 2014/04/16 17:36:48, aurimas wrote:
> Move this method to DropdownPopupWindow.java so that all classes extending it
> get it?
Done, but I'm not sure if this is necessary for select popups that don't coexist
with the keyboard.

https://codereview.chromium.org/231953003/diff/100001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:85: public void
show(AutofillSuggestion[] suggestions) {
On 2014/04/16 17:36:48, aurimas wrote:
> Please rename this to filterAndShow

Done.

[aurimas (slooooooooow), 2014-04-17 15:55:43]

LGTM

tedchoc: please take a quick pass to make sure that you agree.

[Ted C, 2014-04-18 01:28:05]

overall seems nice to me.  a little cleanups and it looks good to go

https://codereview.chromium.org/231953003/diff/80001/content/public/android/j...
File content/public/android/java/res/layout/select_popup_item.xml (right):

https://codereview.chromium.org/231953003/diff/80001/content/public/android/j...
content/public/android/java/res/layout/select_popup_item.xml:1: <?xml
version="1.0" encoding="utf-8"?>
add copyright

https://codereview.chromium.org/231953003/diff/80001/content/public/android/j...
content/public/android/java/res/layout/select_popup_item.xml:7:
android:text="@+id/label"
This doesn't look right...you don't really want text to be here do you?

https://codereview.chromium.org/231953003/diff/80001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/input/SingleSelectPopup.java
(right):

https://codereview.chromium.org/231953003/diff/80001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/SingleSelectPopup.java:1:
// Copyright 2012 The Chromium Authors. All rights reserved.
2014

https://codereview.chromium.org/231953003/diff/190001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/231953003/diff/190001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2399:
if (DeviceUtils.isTablet(mContext) && !multiple) {
I have a slight preference to try to make ContentViewCore completely unaware of
the select popup type that should be created, but it's pretty isolated here so
it is not too bad.

https://codereview.chromium.org/231953003/diff/190001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/SelectPopupDropdown.java
(right):

https://codereview.chromium.org/231953003/diff/190001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/SelectPopupDropdown.java:25:
private DropdownPopupWindow mDropdownPopupWindow = null;
= null isn't necessary as it's the default.

https://codereview.chromium.org/231953003/diff/190001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/SelectPopupDropdown.java:27:
private final ContentViewCore mContentViewCore;
I like to put final above non-final.

https://codereview.chromium.org/231953003/diff/190001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right):

https://codereview.chromium.org/231953003/diff/190001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:36: for (int i = 0; i <
items.size(); i++) {
can you share this with the below constructor?

You could use getCount() and getItem() of the array adapter to make it a shared
function.

https://codereview.chromium.org/231953003/diff/190001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/DropdownItem.java (right):

https://codereview.chromium.org/231953003/diff/190001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/DropdownItem.java:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
2014

https://codereview.chromium.org/231953003/diff/190001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/DropdownItem.java:11: public String
getLabel();
you don't need public on methods as that is implied in interfaces

also, javadoc since they are public

https://codereview.chromium.org/231953003/diff/190001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/DropdownPopupWindow.java (right):

https://codereview.chromium.org/231953003/diff/190001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/DropdownPopupWindow.java:1: // Copyright
2013 The Chromium Authors. All rights reserved.
2014

https://codereview.chromium.org/231953003/diff/190001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/DropdownPopupWindow.java:19: * The Autofill
suggestion popup that lists relevant suggestions.
the javadocs of this file need to be updated as they reference Autofill a lot

https://codereview.chromium.org/231953003/diff/190001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/DropdownPopupWindow.java:25: private View
mAnchorView;
this looks like it can be final

https://codereview.chromium.org/231953003/diff/190001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/DropdownPopupWindow.java:36: * @param
autofillCallback A object that handles the calls to the native
AutofillPopupView.
remove

https://codereview.chromium.org/231953003/diff/190001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/DropdownPopupWindow.java:47:
mViewAndroidDelegate.setAnchorViewPosition(mAnchorView, mAnchorX, mAnchorY,
mAnchorWidth,
why do you need to call this before these values are initialized?  If it needs
to be initialized to 0s, then I would just use those explicitly as this is a bit
confusing since these variables haven't been set yet.

[keishi, 2014-04-18 03:46:18]

I fixed the handling of enabled items and <optgroup> items.

https://codereview.chromium.org/231953003/diff/190001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/SelectPopupDropdown.java
(right):

https://codereview.chromium.org/231953003/diff/190001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/SelectPopupDropdown.java:25:
private DropdownPopupWindow mDropdownPopupWindow = null;
On 2014/04/18 01:28:05, Ted C wrote:
> = null isn't necessary as it's the default.

Done.

https://codereview.chromium.org/231953003/diff/190001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/SelectPopupDropdown.java:27:
private final ContentViewCore mContentViewCore;
On 2014/04/18 01:28:05, Ted C wrote:
> I like to put final above non-final.

Done.

https://codereview.chromium.org/231953003/diff/190001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right):

https://codereview.chromium.org/231953003/diff/190001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:36: for (int i = 0; i <
items.size(); i++) {
On 2014/04/18 01:28:05, Ted C wrote:
> can you share this with the below constructor?
> 
> You could use getCount() and getItem() of the array adapter to make it a
shared
> function.

Done.

https://codereview.chromium.org/231953003/diff/190001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/DropdownItem.java (right):

https://codereview.chromium.org/231953003/diff/190001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/DropdownItem.java:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
On 2014/04/18 01:28:05, Ted C wrote:
> 2014

Done.

https://codereview.chromium.org/231953003/diff/190001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/DropdownItem.java:11: public String
getLabel();
On 2014/04/18 01:28:05, Ted C wrote:
> you don't need public on methods as that is implied in interfaces
> 
> also, javadoc since they are public

Done.

https://codereview.chromium.org/231953003/diff/190001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/DropdownPopupWindow.java (right):

https://codereview.chromium.org/231953003/diff/190001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/DropdownPopupWindow.java:1: // Copyright
2013 The Chromium Authors. All rights reserved.
On 2014/04/18 01:28:05, Ted C wrote:
> 2014

Done.

https://codereview.chromium.org/231953003/diff/190001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/DropdownPopupWindow.java:19: * The Autofill
suggestion popup that lists relevant suggestions.
On 2014/04/18 01:28:05, Ted C wrote:
> the javadocs of this file need to be updated as they reference Autofill a lot

Done.

https://codereview.chromium.org/231953003/diff/190001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/DropdownPopupWindow.java:25: private View
mAnchorView;
On 2014/04/18 01:28:05, Ted C wrote:
> this looks like it can be final

Done.

https://codereview.chromium.org/231953003/diff/190001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/DropdownPopupWindow.java:36: * @param
autofillCallback A object that handles the calls to the native
AutofillPopupView.
On 2014/04/18 01:28:05, Ted C wrote:
> remove

Done.

https://codereview.chromium.org/231953003/diff/190001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/DropdownPopupWindow.java:47:
mViewAndroidDelegate.setAnchorViewPosition(mAnchorView, mAnchorX, mAnchorY,
mAnchorWidth,
On 2014/04/18 01:28:05, Ted C wrote:
> why do you need to call this before these values are initialized?  If it needs
> to be initialized to 0s, then I would just use those explicitly as this is a
bit
> confusing since these variables haven't been set yet.

I don't know why this was in AutofillPopup. Removed.

[keishi, 2014-04-18 03:49:29]

mkosiba@ could you review the files below? Thanks.
android_webview/java/src/org/chromium/android_webview/AwAutofillManagerDelegate.java
ui/android/java/resource_map/org/chromium/ui/R.java

[keishi, 2014-04-22 10:55:40]

Separated out the adding of png files
https://codereview.chromium.org/246233004/

[keishi, 2014-04-22 10:56:11]

mkosiba@ and tedchoc@ please take a look. Thanks.

[Ted C, 2014-04-22 17:00:41]

lgtm

[mkosiba (inactive), 2014-04-23 12:28:27]

since you're changing the names in R.java you'll also need to update the android
glue layer
(https://android.googlesource.com/platform/frameworks/webview/+/chromium-dev/c...)
to keep the android_aosp bot compiling.

the full set of instructions for contributing code to Android are here:
http://dev.chromium.org/developers/how-tos/build-instructions-android-webview
I realize that's a lot for a simple rename so if you want to save time just
checkout the branch directly (git clone
https://android.googlesource.com/platform/frameworks/webview), do the rename,
send me the patch and I'll get it uploaded, reviewed and submitted.

[mkosiba (inactive), 2014-04-23 12:29:50]

On 2014/04/23 12:28:27, mkosiba wrote:
> checkout the branch directly (git clone
> https://android.googlesource.com/platform/frameworks/webview), do the rename,
> send me the patch and I'll get it uploaded, reviewed and submitted.

oh, and you need to base your patch on the chromium-dev branch, so

  git clone https://and...orks/webview
  cd webview
  git checkout chromium-dev

[keishi, 2014-04-24 02:19:24]

On 2014/04/23 12:29:50, mkosiba wrote:
> On 2014/04/23 12:28:27, mkosiba wrote:
> > checkout the branch directly (git clone
> > https://android.googlesource.com/platform/frameworks/webview), do the
rename,
> > send me the patch and I'll get it uploaded, reviewed and submitted.

Thanks. I've uploaded the patch to https://codereview.chromium.org/256433005/

[mkosiba (inactive), 2014-04-24 13:45:57]

sorry, I assumed the AOSP bot errors were only related to the rename but it
looks like you're referencing new resources from Java code too. In this case you
need to add the relevant resource names to the R.java file and update
https://codereview.chromium.org/256433005/ to:
 1) contain copies of the new resources you're adding
 2) initialize the new ids in the ResourceProvider

I'm currently working on making the webview resource story closer to chrome's so
hopefully you're one of the last people to have to deal with this.

https://codereview.chromium.org/231953003/diff/230001/ui/android/java/resourc...
File ui/android/java/resource_map/org/chromium/ui/R.java (right):

https://codereview.chromium.org/231953003/diff/230001/ui/android/java/resourc...
ui/android/java/resource_map/org/chromium/ui/R.java:55: public static int
dropdown_text;
you need to add dropdown_item (per AOSP bot error)

https://codereview.chromium.org/231953003/diff/230001/ui/android/java/resourc...
ui/android/java/resource_map/org/chromium/ui/R.java:73: public static int
dropdown_text_height;
you need to add these fields:
 - dropdown_item_height
 - dropdown_item_divider_height

[keishi, 2014-04-25 06:40:56]

On 2014/04/24 13:45:57, mkosiba wrote:
> sorry, I assumed the AOSP bot errors were only related to the rename but it
> looks like you're referencing new resources from Java code too. In this case
you
> need to add the relevant resource names to the R.java file and update
> https://codereview.chromium.org/256433005/ to:
>  1) contain copies of the new resources you're adding
>  2) initialize the new ids in the ResourceProvider
> 
> I'm currently working on making the webview resource story closer to chrome's
so
> hopefully you're one of the last people to have to deal with this.
> 
>
https://codereview.chromium.org/231953003/diff/230001/ui/android/java/resourc...
> File ui/android/java/resource_map/org/chromium/ui/R.java (right):
> 
>
https://codereview.chromium.org/231953003/diff/230001/ui/android/java/resourc...
> ui/android/java/resource_map/org/chromium/ui/R.java:55: public static int
> dropdown_text;
> you need to add dropdown_item (per AOSP bot error)
> 
>
https://codereview.chromium.org/231953003/diff/230001/ui/android/java/resourc...
> ui/android/java/resource_map/org/chromium/ui/R.java:73: public static int
> dropdown_text_height;
> you need to add these fields:
>  - dropdown_item_height
>  - dropdown_item_divider_height

I don't think I am adding new resources, only renaming them. I had forgotten
that I renamed dropdown_text to dropdown_item so I fixed that in this CL and in
https://codereview.chromium.org/256433005/.

[mkosiba (inactive), 2014-04-25 17:42:57]

On 2014/04/25 06:40:56, keishi1 wrote:
> On 2014/04/24 13:45:57, mkosiba wrote:
> > sorry, I assumed the AOSP bot errors were only related to the rename but it
> > looks like you're referencing new resources from Java code too. In this case
> you
> > need to add the relevant resource names to the R.java file and update
> > https://codereview.chromium.org/256433005/ to:
> >  1) contain copies of the new resources you're adding
> >  2) initialize the new ids in the ResourceProvider
> > 
> > I'm currently working on making the webview resource story closer to
chrome's
> so
> > hopefully you're one of the last people to have to deal with this.
> > 
> >
>
https://codereview.chromium.org/231953003/diff/230001/ui/android/java/resourc...
> > File ui/android/java/resource_map/org/chromium/ui/R.java (right):
> > 
> >
>
https://codereview.chromium.org/231953003/diff/230001/ui/android/java/resourc...
> > ui/android/java/resource_map/org/chromium/ui/R.java:55: public static int
> > dropdown_text;
> > you need to add dropdown_item (per AOSP bot error)
> > 
> >
>
https://codereview.chromium.org/231953003/diff/230001/ui/android/java/resourc...
> > ui/android/java/resource_map/org/chromium/ui/R.java:73: public static int
> > dropdown_text_height;
> > you need to add these fields:
> >  - dropdown_item_height
> >  - dropdown_item_divider_height
> 
> I don't think I am adding new resources, only renaming them. I had forgotten
> that I renamed dropdown_text to dropdown_item so I fixed that in this CL and
in
> https://codereview.chromium.org/256433005/.

right. got confused by the rename.
you missed a couple of things in the Android CL (renaming files, not updating
some fields).
I'm fixing those up and should upload+commit it to Android once I get it to
compile.

You seem to have some test failures on one of the 'regular' android bots though.

[keishi, 2014-05-02 00:23:48]

On 2014/04/25 17:42:57, mkosiba wrote:
> On 2014/04/25 06:40:56, keishi1 wrote:
> > On 2014/04/24 13:45:57, mkosiba wrote:
> > > sorry, I assumed the AOSP bot errors were only related to the rename but
it
> > > looks like you're referencing new resources from Java code too. In this
case
> > you
> > > need to add the relevant resource names to the R.java file and update
> > > https://codereview.chromium.org/256433005/ to:
> > >  1) contain copies of the new resources you're adding
> > >  2) initialize the new ids in the ResourceProvider
> > > 
> > > I'm currently working on making the webview resource story closer to
> chrome's
> > so
> > > hopefully you're one of the last people to have to deal with this.
> > > 
> > >
> >
>
https://codereview.chromium.org/231953003/diff/230001/ui/android/java/resourc...
> > > File ui/android/java/resource_map/org/chromium/ui/R.java (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/231953003/diff/230001/ui/android/java/resourc...
> > > ui/android/java/resource_map/org/chromium/ui/R.java:55: public static int
> > > dropdown_text;
> > > you need to add dropdown_item (per AOSP bot error)
> > > 
> > >
> >
>
https://codereview.chromium.org/231953003/diff/230001/ui/android/java/resourc...
> > > ui/android/java/resource_map/org/chromium/ui/R.java:73: public static int
> > > dropdown_text_height;
> > > you need to add these fields:
> > >  - dropdown_item_height
> > >  - dropdown_item_divider_height
> > 
> > I don't think I am adding new resources, only renaming them. I had forgotten
> > that I renamed dropdown_text to dropdown_item so I fixed that in this CL and
> in
> > https://codereview.chromium.org/256433005/.
> 
> right. got confused by the rename.
> you missed a couple of things in the Android CL (renaming files, not updating
> some fields).
> I'm fixing those up and should upload+commit it to Android once I get it to
> compile.
> 
> You seem to have some test failures on one of the 'regular' android bots
though.

I fixed the test failures.

[mkosiba (inactive), 2014-05-02 13:13:21]

On 2014/05/02 00:23:48, keishi1 wrote:
> On 2014/04/25 17:42:57, mkosiba wrote:
> > On 2014/04/25 06:40:56, keishi1 wrote:
> > > On 2014/04/24 13:45:57, mkosiba wrote:
> > > > sorry, I assumed the AOSP bot errors were only related to the rename but
> it
> > > > looks like you're referencing new resources from Java code too. In this
> case
> > > you
> > > > need to add the relevant resource names to the R.java file and update
> > > > https://codereview.chromium.org/256433005/ to:
> > > >  1) contain copies of the new resources you're adding
> > > >  2) initialize the new ids in the ResourceProvider
> > > > 
> > > > I'm currently working on making the webview resource story closer to
> > chrome's
> > > so
> > > > hopefully you're one of the last people to have to deal with this.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/231953003/diff/230001/ui/android/java/resourc...
> > > > File ui/android/java/resource_map/org/chromium/ui/R.java (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/231953003/diff/230001/ui/android/java/resourc...
> > > > ui/android/java/resource_map/org/chromium/ui/R.java:55: public static
int
> > > > dropdown_text;
> > > > you need to add dropdown_item (per AOSP bot error)
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/231953003/diff/230001/ui/android/java/resourc...
> > > > ui/android/java/resource_map/org/chromium/ui/R.java:73: public static
int
> > > > dropdown_text_height;
> > > > you need to add these fields:
> > > >  - dropdown_item_height
> > > >  - dropdown_item_divider_height
> > > 
> > > I don't think I am adding new resources, only renaming them. I had
forgotten
> > > that I renamed dropdown_text to dropdown_item so I fixed that in this CL
and
> > in
> > > https://codereview.chromium.org/256433005/.
> > 
> > right. got confused by the rename.
> > you missed a couple of things in the Android CL (renaming files, not
updating
> > some fields).
> > I'm fixing those up and should upload+commit it to Android once I get it to
> > compile.
> > 
> > You seem to have some test failures on one of the 'regular' android bots
> though.
> 
> I fixed the test failures.

Sweet! To get AOSP to pass you need to make one change to your CL:
 1) open android_webview/buildbot/aosp_manifest.xml
 2) find the line with <project path="frameworks/webview" in it
 3) change 'revision' to d36825d5b76e90f1ef4f54676cdf34b3bacd043b
 4) upload new patchset, commit via CQ.

[keishi, 2014-05-02 15:48:54]

On 2014/05/02 13:13:21, mkosiba wrote:
> On 2014/05/02 00:23:48, keishi1 wrote:
> > On 2014/04/25 17:42:57, mkosiba wrote:
> > > On 2014/04/25 06:40:56, keishi1 wrote:
> > > > On 2014/04/24 13:45:57, mkosiba wrote:
> > > > > sorry, I assumed the AOSP bot errors were only related to the rename
but
> > it
> > > > > looks like you're referencing new resources from Java code too. In
this
> > case
> > > > you
> > > > > need to add the relevant resource names to the R.java file and update
> > > > > https://codereview.chromium.org/256433005/ to:
> > > > >  1) contain copies of the new resources you're adding
> > > > >  2) initialize the new ids in the ResourceProvider
> > > > > 
> > > > > I'm currently working on making the webview resource story closer to
> > > chrome's
> > > > so
> > > > > hopefully you're one of the last people to have to deal with this.
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/231953003/diff/230001/ui/android/java/resourc...
> > > > > File ui/android/java/resource_map/org/chromium/ui/R.java (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/231953003/diff/230001/ui/android/java/resourc...
> > > > > ui/android/java/resource_map/org/chromium/ui/R.java:55: public static
> int
> > > > > dropdown_text;
> > > > > you need to add dropdown_item (per AOSP bot error)
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/231953003/diff/230001/ui/android/java/resourc...
> > > > > ui/android/java/resource_map/org/chromium/ui/R.java:73: public static
> int
> > > > > dropdown_text_height;
> > > > > you need to add these fields:
> > > > >  - dropdown_item_height
> > > > >  - dropdown_item_divider_height
> > > > 
> > > > I don't think I am adding new resources, only renaming them. I had
> forgotten
> > > > that I renamed dropdown_text to dropdown_item so I fixed that in this CL
> and
> > > in
> > > > https://codereview.chromium.org/256433005/.
> > > 
> > > right. got confused by the rename.
> > > you missed a couple of things in the Android CL (renaming files, not
> updating
> > > some fields).
> > > I'm fixing those up and should upload+commit it to Android once I get it
to
> > > compile.
> > > 
> > > You seem to have some test failures on one of the 'regular' android bots
> > though.
> > 
> > I fixed the test failures.
> 
> Sweet! To get AOSP to pass you need to make one change to your CL:
>  1) open android_webview/buildbot/aosp_manifest.xml
>  2) find the line with <project path="frameworks/webview" in it
>  3) change 'revision' to d36825d5b76e90f1ef4f54676cdf34b3bacd043b
>  4) upload new patchset, commit via CQ.

Done. I've changed aosp_manifest.xml

[mkosiba (inactive), 2014-05-02 15:51:28]

lgtm

[keishi, 2014-05-03 02:23:14]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2014-05-03 02:23:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/keishi@chromium.org/231953003/440001

[commit-bot: I haz the power, 2014-05-03 02:24:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-03 02:24:14]

Failed to apply patch for
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
  Hunk #4 FAILED at 1083.
  Hunk #5 succeeded at 1360 (offset 12 lines).
  Hunk #6 succeeded at 1375 (offset 12 lines).
  Hunk #7 succeeded at 1414 (offset 12 lines).
  Hunk #8 succeeded at 1475 (offset 12 lines).
  Hunk #9 FAILED at 1614.
  Hunk #10 FAILED at 1654.
  Hunk #11 succeeded at 1867 (offset 15 lines).
  Hunk #12 succeeded at 2330 (offset 15 lines).
  Hunk #13 succeeded at 2356 (offset 15 lines).
  3 out of 13 hunks FAILED -- saving rejects to file
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java.rej

Patch:      
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
Index:
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
diff --git
a/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
b/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
index
40661f1caf9c7d53fcc92e2032619dec8ccb6767..6abe554673da8f80e75816655a005ada2748ef69
100644
---
a/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
+++
b/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
@@ -64,7 +64,9 @@ import org.chromium.content.browser.input.ImeAdapter;
 import
org.chromium.content.browser.input.ImeAdapter.AdapterInputConnectionFactory;
 import org.chromium.content.browser.input.InputMethodManagerWrapper;
 import org.chromium.content.browser.input.InsertionHandleController;
+import org.chromium.content.browser.input.SelectPopup;
 import org.chromium.content.browser.input.SelectPopupDialog;
+import org.chromium.content.browser.input.SelectPopupDropdown;
 import org.chromium.content.browser.input.SelectPopupItem;
 import org.chromium.content.browser.input.SelectionHandleController;
 import org.chromium.content.common.ContentSwitches;
@@ -227,7 +229,7 @@ public class ContentViewCore
     private ZoomControlsDelegate mZoomControlsDelegate;
 
     private PopupZoomer mPopupZoomer;
-    private SelectPopupDialog mSelectPopupDialog;
+    private SelectPopup mSelectPopup;
 
     private Runnable mFakeMouseMoveRunnable = null;
 
@@ -627,14 +629,14 @@ public class ContentViewCore
             public void didNavigateMainFrame(String url, String baseUrl,
                     boolean isNavigationToDifferentPage, boolean
isNavigationInPage) {
                 if (!isNavigationToDifferentPage) return;
-                hidePopupDialog();
+                hidePopups();
                 resetScrollInProgress();
                 resetGestureDetection();
             }
 
             @Override
             public void renderProcessGone(boolean wasOomProtected) {
-                hidePopupDialog();
+                hidePopups();
                 resetScrollInProgress();
                 // No need to reset gesture detection as the detector will have
                 // been destroyed in the RenderWidgetHostView.
@@ -1081,36 +1083,48 @@ public class ContentViewCore
      * @see View#onTouchEvent(MotionEvent)
      */
     public boolean onTouchEvent(MotionEvent event) {
-        MotionEvent offset = createOffsetMotionEvent(event);
+        TraceEvent.begin("onTouchEvent");
+        try {
+            cancelRequestToScrollFocusedEditableNodeIntoView();
+
+            final int eventAction = event.getActionMasked();
+
+            // Only these actions have any effect on gesture detection.  Other
+            // actions have no corresponding WebTouchEvent type and may confuse
the
+            // touch pipline, so we ignore them entirely.
+            if (eventAction != MotionEvent.ACTION_DOWN
+                    && eventAction != MotionEvent.ACTION_UP
+                    && eventAction != MotionEvent.ACTION_CANCEL
+                    && eventAction != MotionEvent.ACTION_MOVE
+                    && eventAction != MotionEvent.ACTION_POINTER_DOWN
+                    && eventAction != MotionEvent.ACTION_POINTER_UP) {
+                return false;
+            }
 
-        cancelRequestToScrollFocusedEditableNodeIntoView();
+            if (mNativeContentViewCore == 0) return false;
 
-        final int eventAction = offset.getActionMasked();
+            // A zero offset is quite common, in which case the unnecessary
copy should be avoided.
+            MotionEvent offset = null;
+            if (mCurrentTouchOffsetX != 0 || mCurrentTouchOffsetY != 0) {
+                offset = createOffsetMotionEvent(event);
+                event = offset;
+            }
 
-        // Only these actions have any effect on gesture detection.  Other
-        // actions have no corresponding WebTouchEvent type and may confuse the
-        // touch pipline, so we ignore them entirely.
-        if (eventAction != MotionEvent.ACTION_DOWN
-                && eventAction != MotionEvent.ACTION_UP
-                && eventAction != MotionEvent.ACTION_CANCEL
-                && eventAction != MotionEvent.ACTION_MOVE
-                && eventAction != MotionEvent.ACTION_POINTER_DOWN
-                && eventAction != MotionEvent.ACTION_POINTER_UP) {
-            return false;
+            final int pointerCount = event.getPointerCount();
+            final boolean consumed = nativeOnTouchEvent(mNativeContentViewCore,
event,
+                    event.getEventTime(), eventAction,
+                    pointerCount, event.getHistorySize(),
event.getActionIndex(),
+                    event.getX(), event.getY(),
+                    pointerCount > 1 ? event.getX(1) : 0,
+                    pointerCount > 1 ? event.getY(1) : 0,
+                    event.getPointerId(0), pointerCount > 1 ?
event.getPointerId(1) : -1,
+                    event.getTouchMajor(), pointerCount > 1 ?
event.getTouchMajor(1) : 0);
+
+            if (offset != null) offset.recycle();
+            return consumed;
+        } finally {
+            TraceEvent.end("onTouchEvent");
         }
-
-        if (mNativeContentViewCore == 0) return false;
-        final int pointerCount = offset.getPointerCount();
-        boolean consumed = nativeOnTouchEvent(mNativeContentViewCore, offset,
-                offset.getEventTime(), eventAction,
-                pointerCount, offset.getHistorySize(), offset.getActionIndex(),
-                offset.getX(), offset.getY(),
-                pointerCount > 1 ? offset.getX(1) : 0,
-                pointerCount > 1 ? offset.getY(1) : 0,
-                offset.getPointerId(0), pointerCount > 1 ?
offset.getPointerId(1) : -1,
-                offset.getTouchMajor(), pointerCount > 1 ?
offset.getTouchMajor(1) : 0);
-        offset.recycle();
-        return consumed;
     }
 
     public void setIgnoreRemainingTouchEvents() {
@@ -1346,7 +1360,7 @@ public class ContentViewCore
      */
     public void onHide() {
         assert mNativeContentViewCore != 0;
-        hidePopupDialog();
+        hidePopups();
         setInjectedAccessibility(false);
         nativeOnHide(mNativeContentViewCore);
     }
@@ -1361,7 +1375,7 @@ public class ContentViewCore
         return mContentSettings;
     }
 
-    private void hidePopupDialog() {
+    private void hidePopups() {
         hideSelectPopup();
         hideHandles();
         hideSelectActionBar();
@@ -1400,7 +1414,7 @@ public class ContentViewCore
     @SuppressLint("MissingSuperCall")
     public void onDetachedFromWindow() {
         setInjectedAccessibility(false);
-        hidePopupDialog();
+        hidePopups();
         mZoomControlsDelegate.dismissZoomPicker();
         unregisterAccessibilityContentObserver();
 
@@ -1461,7 +1475,6 @@ public class ContentViewCore
             mInputMethodManagerWrapper.restartInput(mContainerView);
         }
         mContainerViewInternals.super_onConfigurationChanged(newConfig);
-
         // To request layout has side effect, but it seems OK as it only happen
in
         // onConfigurationChange and layout has to be changed in most case.
         mContainerView.requestLayout();
@@ -1613,25 +1626,27 @@ public class ContentViewCore
     public boolean onHoverEvent(MotionEvent event) {
         TraceEvent.begin("onHoverEvent");
         MotionEvent offset = createOffsetMotionEvent(event);
+        try {
+            if (mBrowserAccessibilityManager != null) {
+                return mBrowserAccessibilityManager.onHoverEvent(offset);
+            }
 
-        if (mBrowserAccessibilityManager != null) {
-            return mBrowserAccessibilityManager.onHoverEvent(offset);
-        }
+            // Work around Android bug where the x, y coordinates of a hover
exit
+            // event are incorrect when touch exploration is on.
+            if (mTouchExplorationEnabled && offset.getAction() ==
MotionEvent.ACTION_HOVER_EXIT) {
+                return true;
+            }
 
-        // Work around Android bug where the x, y coordinates of a hover exit
-        // event are incorrect when touch exploration is on.
-        if (mTouchExplorationEnabled && offset.getAction() ==
MotionEvent.ACTION_HOVER_EXIT) {
+            mContainerView.removeCallbacks(mFakeMouseMoveRunnable);
+            if (mNativeContentViewCore != 0) {
+                nativeSendMouseMoveEvent(mNativeContentViewCore,
offset.getEventTime(),
+                        offset.getX(), offset.getY());
+            }
             return true;
+        } finally {
+            offset.recycle();
+            TraceEvent.end("onHoverEvent");
         }
-
-        mContainerView.removeCallbacks(mFakeMouseMoveRunnable);
-        if (mNativeContentViewCore != 0) {
-            nativeSendMouseMoveEvent(mNativeContentViewCore,
offset.getEventTime(),
-                    offset.getX(), offset.getY());
-        }
-        TraceEvent.end("onHo…
(message too large)

[keishi, 2014-05-03 05:46:33]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2014-05-03 05:47:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/keishi@chromium.org/231953003/460001

[commit-bot: I haz the power, 2014-05-03 10:18:07]

Change committed as 268062