android: Don't dismiss AutofillPopup on outside touch

android: Don't allow AutofillPopup on outside touch

By default, PopupWindows dismiss when the user touches outside the window. The only
way to avert this behavior when using a ListPopupWindow is to rely on a hidden API.

This manifested as the linked bug because each tap dismissed the popup and detached it from the anchor view. C++ didn't realize the popup was dismissed, and tried to update it, but there was no anchor view.

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

[Evan Stade, 2014-08-05 19:44:30]

This is ugly, suggestions for better fixes welcome.

I see two alternatives, but both are worse imo (maybe I'm not fully aware of how
bad using a hidden API is). It seems the worst case scenario is the hidden API
is removed/changed, and we get this bug again.

1) Ditch ListPopupWindow, use PopupWindow directly (essentially reimplement
ListPopupWindow)
2) Listen for dismiss, inform tell the C++ class when the popup has been
dismissed. This will fix the positioning, but will still dismiss and re-show the
popup on every tap, which is not the behavior we want.

Perhaps the best longterm resolution is to file a feature request to expose
PopupWindow's setOutsideTouchable in ListPopupWindow.

https://codereview.chromium.org/442933002/diff/1/ui/android/java/src/org/chro...
File ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java (left):

https://codereview.chromium.org/442933002/diff/1/ui/android/java/src/org/chro...
ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:102: public void
dismiss() {
this was never being called AFAICT

[aurimas (slooooooooow), 2014-08-05 21:26:50]

https://codereview.chromium.org/442933002/diff/1/ui/android/java/src/org/chro...
File ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java (left):

https://codereview.chromium.org/442933002/diff/1/ui/android/java/src/org/chro...
ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:102: public void
dismiss() {
On 2014/08/05 19:44:30, Evan Stade wrote:
> this was never being called AFAICT

We used to call setOnDismissListener(), but I am guessing it somehow got
deleting during the refactoring to use DropdownPopupWindow for both
AutofillPopup and <select> dropdowns on tablets. We can add that listener and
that one definitely gets called.

https://codereview.chromium.org/442933002/diff/1/ui/android/java/src/org/chro...
File ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java (right):

https://codereview.chromium.org/442933002/diff/1/ui/android/java/src/org/chro...
ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:108:
setForceIgnoreOutsideTouch.invoke(this, new Object[]{ true });
If we no longer dismiss the popup we have to add a bunch of code to keep it in
the right place as user scrolls and pinches to zoom. Also this means there is no
way for the user to dismiss the popup (desktop has esc key).

Could we possibly just capture touch events on the anchor view and do not send
those through?

[Evan Stade, 2014-08-05 21:29:19]

https://codereview.chromium.org/442933002/diff/1/ui/android/java/src/org/chro...
File ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java (left):

https://codereview.chromium.org/442933002/diff/1/ui/android/java/src/org/chro...
ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:102: public void
dismiss() {
On 2014/08/05 21:26:50, aurimas wrote:
> On 2014/08/05 19:44:30, Evan Stade wrote:
> > this was never being called AFAICT
> 
> We used to call setOnDismissListener(), but I am guessing it somehow got
> deleting during the refactoring to use DropdownPopupWindow for both
> AutofillPopup and <select> dropdowns on tablets. We can add that listener and
> that one definitely gets called.

yes, but that won't fix this bug.

https://codereview.chromium.org/442933002/diff/1/ui/android/java/src/org/chro...
File ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java (right):

https://codereview.chromium.org/442933002/diff/1/ui/android/java/src/org/chro...
ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:108:
setForceIgnoreOutsideTouch.invoke(this, new Object[]{ true });
On 2014/08/05 21:26:50, aurimas wrote:
> If we no longer dismiss the popup we have to add a bunch of code to keep it in
> the right place as user scrolls and pinches to zoom.

no, Chrome dismisses the popup on scroll or zoom.

> Also this means there is no
> way for the user to dismiss the popup (desktop has esc key).

Chrome dismisses the popup when the input field loses focus.

> 
> Could we possibly just capture touch events on the anchor view and do not send
> those through?

We don't want to block input events. Tapping away from the input field (e.g. on
another input) should work.

[aurimas (slooooooooow), 2014-08-05 21:43:12]

https://codereview.chromium.org/442933002/diff/1/ui/android/java/src/org/chro...
File ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java (left):

https://codereview.chromium.org/442933002/diff/1/ui/android/java/src/org/chro...
ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:102: public void
dismiss() {
On 2014/08/05 21:29:19, Evan Stade wrote:
> On 2014/08/05 21:26:50, aurimas wrote:
> > On 2014/08/05 19:44:30, Evan Stade wrote:
> > > this was never being called AFAICT
> > 
> > We used to call setOnDismissListener(), but I am guessing it somehow got
> > deleting during the refactoring to use DropdownPopupWindow for both
> > AutofillPopup and <select> dropdowns on tablets. We can add that listener
and
> > that one definitely gets called.
> 
> yes, but that won't fix this bug.

Why couldn't it? Adding the listener would call to C++ controller class to
requestHide(). That would mean that the controller would know that the popup is
gone.

Additionally you can add a boolean to track whether it was a call from C++
controller (we can to dismiss) or user tab (do not dismiss)

https://codereview.chromium.org/442933002/diff/1/ui/android/java/src/org/chro...
File ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java (right):

https://codereview.chromium.org/442933002/diff/1/ui/android/java/src/org/chro...
ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:108:
setForceIgnoreOutsideTouch.invoke(this, new Object[]{ true });
On 2014/08/05 21:29:19, Evan Stade wrote:
> On 2014/08/05 21:26:50, aurimas wrote:
> > If we no longer dismiss the popup we have to add a bunch of code to keep it
in
> > the right place as user scrolls and pinches to zoom.
> 
> no, Chrome dismisses the popup on scroll or zoom.
> 
> > Also this means there is no
> > way for the user to dismiss the popup (desktop has esc key).
> 
> Chrome dismisses the popup when the input field loses focus.
Right, but on desktop you can still have focus but no popup. In android this
change would make user tap outside of the field and the tap the field again
exactly once in the right position not to have an autofill popup.
> 
> > 
> > Could we possibly just capture touch events on the anchor view and do not
send
> > those through?
> 
> We don't want to block input events. Tapping away from the input field (e.g.
on
> another input) should work.
Anchor view is exactly of the side of the input field, so it would only get the
events that would normally go to the input field. Although now that I think of
it would break users ability to move the cursor.

[Evan Stade, 2014-08-05 21:49:15]

https://codereview.chromium.org/442933002/diff/1/ui/android/java/src/org/chro...
File ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java (left):

https://codereview.chromium.org/442933002/diff/1/ui/android/java/src/org/chro...
ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:102: public void
dismiss() {
On 2014/08/05 21:43:12, aurimas wrote:
> On 2014/08/05 21:29:19, Evan Stade wrote:
> > On 2014/08/05 21:26:50, aurimas wrote:
> > > On 2014/08/05 19:44:30, Evan Stade wrote:
> > > > this was never being called AFAICT
> > > 
> > > We used to call setOnDismissListener(), but I am guessing it somehow got
> > > deleting during the refactoring to use DropdownPopupWindow for both
> > > AutofillPopup and <select> dropdowns on tablets. We can add that listener
> and
> > > that one definitely gets called.
> > 
> > yes, but that won't fix this bug.
> 
> Why couldn't it? Adding the listener would call to C++ controller class to
> requestHide(). That would mean that the controller would know that the popup
is
> gone.
> 
> Additionally you can add a boolean to track whether it was a call from C++
> controller (we can to dismiss) or user tab (do not dismiss)

see my first comment on this review

https://codereview.chromium.org/442933002/diff/1/ui/android/java/src/org/chro...
File ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java (right):

https://codereview.chromium.org/442933002/diff/1/ui/android/java/src/org/chro...
ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:108:
setForceIgnoreOutsideTouch.invoke(this, new Object[]{ true });
On 2014/08/05 21:43:12, aurimas wrote:
> On 2014/08/05 21:29:19, Evan Stade wrote:
> > On 2014/08/05 21:26:50, aurimas wrote:
> > > If we no longer dismiss the popup we have to add a bunch of code to keep
it
> in
> > > the right place as user scrolls and pinches to zoom.
> > 
> > no, Chrome dismisses the popup on scroll or zoom.
> > 
> > > Also this means there is no
> > > way for the user to dismiss the popup (desktop has esc key).
> > 
> > Chrome dismisses the popup when the input field loses focus.
> Right, but on desktop you can still have focus but no popup. In android this
> change would make user tap outside of the field and the tap the field again
> exactly once in the right position not to have an autofill popup.

That's what you have to do on desktop as well.

> > 
> > > 
> > > Could we possibly just capture touch events on the anchor view and do not
> send
> > > those through?
> > 
> > We don't want to block input events. Tapping away from the input field (e.g.
> on
> > another input) should work.
> Anchor view is exactly of the side of the input field, so it would only get
the
> events that would normally go to the input field. Although now that I think of
> it would break users ability to move the cursor.

[aurimas (slooooooooow), 2014-08-05 22:01:41]

https://codereview.chromium.org/442933002/diff/1/ui/android/java/src/org/chro...
File ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java (left):

https://codereview.chromium.org/442933002/diff/1/ui/android/java/src/org/chro...
ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:102: public void
dismiss() {
On 2014/08/05 21:49:15, Evan Stade wrote:
> On 2014/08/05 21:43:12, aurimas wrote:
> > On 2014/08/05 21:29:19, Evan Stade wrote:
> > > On 2014/08/05 21:26:50, aurimas wrote:
> > > > On 2014/08/05 19:44:30, Evan Stade wrote:
> > > > > this was never being called AFAICT
> > > > 
> > > > We used to call setOnDismissListener(), but I am guessing it somehow got
> > > > deleting during the refactoring to use DropdownPopupWindow for both
> > > > AutofillPopup and <select> dropdowns on tablets. We can add that
listener
> > and
> > > > that one definitely gets called.
> > > 
> > > yes, but that won't fix this bug.
> > 
> > Why couldn't it? Adding the listener would call to C++ controller class to
> > requestHide(). That would mean that the controller would know that the popup
> is
> > gone.
> > 
> > Additionally you can add a boolean to track whether it was a call from C++
> > controller (we can to dismiss) or user tab (do not dismiss)
> 
> see my first comment on this review

This dismiss() override is different from setting OnDismissListener from what I
remember. I think OnDismissListener always get called, but dismiss() only in
some situations. 

SetOnDismissListener sets the listener all the way to the PopupWindow that is
owned by ListPopupWindow.

[Evan Stade, 2014-08-05 22:04:35]

https://codereview.chromium.org/442933002/diff/1/ui/android/java/src/org/chro...
File ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java (left):

https://codereview.chromium.org/442933002/diff/1/ui/android/java/src/org/chro...
ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:102: public void
dismiss() {
On 2014/08/05 22:01:41, aurimas wrote:
> On 2014/08/05 21:49:15, Evan Stade wrote:
> > On 2014/08/05 21:43:12, aurimas wrote:
> > > On 2014/08/05 21:29:19, Evan Stade wrote:
> > > > On 2014/08/05 21:26:50, aurimas wrote:
> > > > > On 2014/08/05 19:44:30, Evan Stade wrote:
> > > > > > this was never being called AFAICT
> > > > > 
> > > > > We used to call setOnDismissListener(), but I am guessing it somehow
got
> > > > > deleting during the refactoring to use DropdownPopupWindow for both
> > > > > AutofillPopup and <select> dropdowns on tablets. We can add that
> listener
> > > and
> > > > > that one definitely gets called.
> > > > 
> > > > yes, but that won't fix this bug.
> > > 
> > > Why couldn't it? Adding the listener would call to C++ controller class to
> > > requestHide(). That would mean that the controller would know that the
popup
> > is
> > > gone.
> > > 
> > > Additionally you can add a boolean to track whether it was a call from C++
> > > controller (we can to dismiss) or user tab (do not dismiss)
> > 
> > see my first comment on this review
> 
> This dismiss() override is different from setting OnDismissListener from what
I
> remember. I think OnDismissListener always get called, but dismiss() only in
> some situations. 
> 
> SetOnDismissListener sets the listener all the way to the PopupWindow that is
> owned by ListPopupWindow.

I understand that. We simply don't want the popup to dismiss. The dismiss
listener has no opportunity to prevent the dismiss.

[aurimas (slooooooooow), 2014-08-06 17:19:49]

+tedchoc who has been working with ListPopupWindow a lot. Maybe he has some
ideas.

[Ted C, 2014-08-07 01:45:23]

On 2014/08/06 17:19:49, aurimas wrote:
> +tedchoc who has been working with ListPopupWindow a lot. Maybe he has some
> ideas.

Sadly, I don't know how to work around this.  I recently got rid of the
ListPopupWindow for the omnibox suggestions as it was just causing too much
headache for these such issues.

But using reflection to get to a non-exposed API seems scary to me.  We've
done whatever possible to be SDK compliant and this doesn't seem to be a big
enough issue to justify breaking that.

I'd be ok if we could get android support for making these methods public,
but doing it in reverse sounds like a recipe for a TODO that will never be
removed.  And then we have no guarantee that this won't break quietly in
the future or will not work on X versions of android that don't attempt
to conform to this API (and if a platform doesn't support this, do we
gracefully degrade or do we crash).  If we can get it to just make this
better if we have this support, then I'll begrudgingly be ok with it...
i guess.

We continue to have problems with ListPopupWindow (ask dtrainor@ for things
that he wasn't able to do for the menu) that it might be time to justify
writing a replacement ourselves.

[Evan Stade, 2014-08-08 19:38:55]

On 2014/08/07 01:45:23, Ted C wrote:
> On 2014/08/06 17:19:49, aurimas wrote:
> > +tedchoc who has been working with ListPopupWindow a lot. Maybe he has some
> > ideas.
> 
> Sadly, I don't know how to work around this.  I recently got rid of the
> ListPopupWindow for the omnibox suggestions as it was just causing too much
> headache for these such issues.
> 
> But using reflection to get to a non-exposed API seems scary to me.  We've
> done whatever possible to be SDK compliant and this doesn't seem to be a big
> enough issue to justify breaking that.
> 
> I'd be ok if we could get android support for making these methods public,
> but doing it in reverse sounds like a recipe for a TODO that will never be
> removed.  And then we have no guarantee that this won't break quietly in
> the future 

We can add a log of some sort to the catch block if we prefer breaking noisily.
What kind of logging statement or assertion do you think is appropriate?

> or will not work on X versions of android that don't attempt
> to conform to this API (and if a platform doesn't support this, do we
> gracefully degrade or do we crash). 

We gracefully degrade, i.e. we go back to having a bug, but it's no worse than
if the bug weren't fixed.

> If we can get it to just make this
> better if we have this support, then I'll begrudgingly be ok with it...
> i guess.

I think that's what this patch does. Behavior is better if it works, no worse if
it doesn't work.

> 
> We continue to have problems with ListPopupWindow (ask dtrainor@ for things
> that he wasn't able to do for the menu) that it might be time to justify
> writing a replacement ourselves.

That sounds a little scary based on the description of ListPopupWindow
("ListPopupWindow contains a number of tricky behaviors"). Let's optimistically
assume the Android team would rather expose a little extra functionality than
have us fork the entire implementation.

[Ted C, 2014-08-11 23:10:03]

lgtm

As long as phones without this work, I see this as the only available avenue we
have to proceed.

https://codereview.chromium.org/442933002/diff/1/ui/android/java/src/org/chro...
File ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java (right):

https://codereview.chromium.org/442933002/diff/1/ui/android/java/src/org/chro...
ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:106: Method
setForceIgnoreOutsideTouch = listPopupWindowClass.getMethod(
I would just use ListPopupWindow.class instead of using reflection for that.

Just for extra sanity, can you test with an invalid method name or a different
parameter type (ones that won't match the API) just to make sure it always falls
into Exception and never Throwable (I can't think of any case it wouldn't, but I
want to be extract cautious).

https://codereview.chromium.org/442933002/diff/1/ui/android/java/src/org/chro...
ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:107:
"setForceIgnoreOutsideTouch", new Class[]{ boolean.class });
space between [] and {

(same below)

https://codereview.chromium.org/442933002/diff/1/ui/android/java/src/org/chro...
ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:110:
Log.e("AutofillPopup", "ListPopupWindow.setForceIgnoreOutsideTouch not found");
use the method where you log e as well in case we can track these issues

[Evan Stade, 2014-08-12 01:36:06]

I also added the OnDismissListener so that when the hack doesn't work, the
controller is told that the popup has gone down. So when the hack doesn't work,
tapping the input field repeatedly will still show and hide the popup, but it
will at least stay in the correct location instead of moving to (0, 0).

https://codereview.chromium.org/442933002/diff/1/ui/android/java/src/org/chro...
File ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java (right):

https://codereview.chromium.org/442933002/diff/1/ui/android/java/src/org/chro...
ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:106: Method
setForceIgnoreOutsideTouch = listPopupWindowClass.getMethod(
On 2014/08/11 23:10:03, Ted C wrote:
> I would just use ListPopupWindow.class instead of using reflection for that.

done

> 
> Just for extra sanity, can you test with an invalid method name or a different
> parameter type (ones that won't match the API) just to make sure it always
falls
> into Exception and never Throwable (I can't think of any case it wouldn't, but
I
> want to be extract cautious).

tried it, worked as expected (i.e. exhibited the bug).

https://codereview.chromium.org/442933002/diff/1/ui/android/java/src/org/chro...
ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:107:
"setForceIgnoreOutsideTouch", new Class[]{ boolean.class });
On 2014/08/11 23:10:03, Ted C wrote:
> space between [] and {
> 
> (same below)

Done.

https://codereview.chromium.org/442933002/diff/1/ui/android/java/src/org/chro...
ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:110:
Log.e("AutofillPopup", "ListPopupWindow.setForceIgnoreOutsideTouch not found");
On 2014/08/11 23:10:03, Ted C wrote:
> use the method where you log e as well in case we can track these issues

Done.

[Evan Stade, 2014-08-12 19:40:14]

On 2014/08/12 01:36:06, Evan Stade wrote:
> I also added the OnDismissListener so that when the hack doesn't work, the
> controller is told that the popup has gone down. So when the hack doesn't
work,
> tapping the input field repeatedly will still show and hide the popup, but it
> will at least stay in the correct location instead of moving to (0, 0).

this touched more files, please re-review

[Evan Stade, 2014-08-12 19:54:56]

+benm for AW

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

https://codereview.chromium.org/442933002/diff/40001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:114: } catch
(Exception e) {
this makes FindBugs say:

M D REC: Exception is caught when Exception is not thrown in
org.chromium.ui.autofill.AutofillPopup.filterAndShow(AutofillSuggestion[],
boolean)

Can we ignore this warning?

[Ted C, 2014-08-13 01:27:48]

still lgtm for me

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

https://codereview.chromium.org/442933002/diff/40001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:114: } catch
(Exception e) {
On 2014/08/12 19:54:56, Evan Stade wrote:
> this makes FindBugs say:
> 
> M D REC: Exception is caught when Exception is not thrown in
> org.chromium.ui.autofill.AutofillPopup.filterAndShow(AutofillSuggestion[],
> boolean)
> 
> Can we ignore this warning?

You'll need to add it to:
./build/android/findbugs_filter/findbugs_known_bugs.txt

We really try to avoid adding new exemptions, but we don't want some internal
issue to cause a crash here (but at the same time it could mean the popup is in
an invalid state and lead to more unexpected craziness...but I'm less worried of
the later case in this particular instance).

Looks like you missed a comment from my last change where you should be using
Log.e(<tag>, <message>, <exception>); just in case we can see it in logcat ever.

[Evan Stade, 2014-08-13 19:15:22]

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

https://codereview.chromium.org/442933002/diff/40001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/autofill/AutofillPopup.java:114: } catch
(Exception e) {
On 2014/08/13 01:27:48, Ted C wrote:
> On 2014/08/12 19:54:56, Evan Stade wrote:
> > this makes FindBugs say:
> > 
> > M D REC: Exception is caught when Exception is not thrown in
> > org.chromium.ui.autofill.AutofillPopup.filterAndShow(AutofillSuggestion[],
> > boolean)
> > 
> > Can we ignore this warning?
> 
> You'll need to add it to:
> ./build/android/findbugs_filter/findbugs_known_bugs.txt
> 
> We really try to avoid adding new exemptions, but we don't want some internal
> issue to cause a crash here (but at the same time it could mean the popup is
in
> an invalid state and lead to more unexpected craziness...but I'm less worried
of
> the later case in this particular instance).

I think we could get aroudn filing an exemption by doing

} catch(RuntimeException e) {
  Log ...
} catch (Exception e){ 
  same log ...
}

I assume filing an exemption is preferable though.

> 
> Looks like you missed a comment from my last change where you should be using
> Log.e(<tag>, <message>, <exception>); just in case we can see it in logcat
ever.

Ah, now I understand what your last comment meant... my brain's parse of it
involved adding the method name to the tag.

[benm (inactive), 2014-08-18 11:13:56]

reflection in ui/ makes me sad :-(

Has anyone filed an internal bug with the Android framework team (or at least
spoken with them) to understand any adverse effects of using this hidden API?
The documentation states that it's only used in "special circumstances"...

[Evan Stade, 2014-08-18 18:16:48]

On 2014/08/18 11:13:56, benm wrote:
> reflection in ui/ makes me sad :-(
> 
> Has anyone filed an internal bug with the Android framework team (or at least
> spoken with them) to understand any adverse effects of using this hidden API?
> The documentation states that it's only used in "special circumstances"...

no, but I read the code. I do plan to file a bug to see if they would expose
this API or something similar.

[Evan Stade, 2014-08-19 22:24:37]

ping benm

[benm (inactive), 2014-08-20 13:30:43]

On 2014/08/19 22:24:37, Evan Stade wrote:
> ping benm

What would be the cost of fixing this by not using ListPopupWindow? It seems
like that's what we really want to do to fix this properly.

[Evan Stade, 2014-08-20 20:24:17]

On 2014/08/20 13:30:43, benm wrote:
> On 2014/08/19 22:24:37, Evan Stade wrote:
> > ping benm
> 
> What would be the cost of fixing this by not using ListPopupWindow? It seems
> like that's what we really want to do to fix this properly.

Reimplementing ListPopupWindow seems risky because "ListPopupWindow contains a
number of tricky behaviors" according to the docs. Besides being risky, that
file is 1200 lines long.

[Evan Stade, 2014-08-21 17:32:32]

This CL has been ready to go for a week; +sgurun for android_webview/ review
from local timezone.

[benm (inactive), 2014-08-21 17:40:16]

On 2014/08/21 17:32:32, Evan Stade wrote:
> This CL has been ready to go for a week; +sgurun for android_webview/ review
> from local timezone.

Just for the record, I don't agree with reflection on this. Chrome is supposed
to be SDK clean, and reflection isn't normally allowed.

I thought Ted said he recently replaced ListPopupWindow with something else
earlier in the thread.

[Evan Stade, 2014-08-21 17:43:24]

On 2014/08/21 17:40:16, benm wrote:
> On 2014/08/21 17:32:32, Evan Stade wrote:
> > This CL has been ready to go for a week; +sgurun for android_webview/ review
> > from local timezone.
> 
> Just for the record, I don't agree with reflection on this. Chrome is supposed
> to be SDK clean, and reflection isn't normally allowed.
> 
> I thought Ted said he recently replaced ListPopupWindow with something else
> earlier in the thread.

He said "We continue to have problems with ListPopupWindow (ask dtrainor@ for
things
that he wasn't able to do for the menu) that it might be time to justify
writing a replacement ourselves." He also said "As long as phones without this
work,
I see this as the only available avenue we have to proceed."

[sgurun-gerrit only, 2014-08-21 17:46:39]

On 2014/08/21 17:40:16, benm wrote:
> On 2014/08/21 17:32:32, Evan Stade wrote:
> > This CL has been ready to go for a week; +sgurun for android_webview/ review
> > from local timezone.
> 
> Just for the record, I don't agree with reflection on this. Chrome is supposed
> to be SDK clean, and reflection isn't normally allowed.
> 
> I thought Ted said he recently replaced ListPopupWindow with something else
> earlier in the thread.

I'd like to second Ben's earlier question: Is this the mechanism that the
android frameworks team suggests?

[benm (inactive), 2014-08-21 17:49:23]

On 2014/08/21 17:43:24, Evan Stade wrote:
> On 2014/08/21 17:40:16, benm wrote:
> > On 2014/08/21 17:32:32, Evan Stade wrote:
> > > This CL has been ready to go for a week; +sgurun for android_webview/
review
> > > from local timezone.
> > 
> > Just for the record, I don't agree with reflection on this. Chrome is
supposed
> > to be SDK clean, and reflection isn't normally allowed.
> > 
> > I thought Ted said he recently replaced ListPopupWindow with something else
> > earlier in the thread.
> 
> He said "We continue to have problems with ListPopupWindow (ask dtrainor@ for
> things
> that he wasn't able to do for the menu) that it might be time to justify
> writing a replacement ourselves." He also said "As long as phones without this
> work,
> I see this as the only available avenue we have to proceed."

Also "I recently got rid of the
ListPopupWindow for the omnibox suggestions as it was just causing too much
headache for these such issues."

I interpreted this to mean that it was replaced by something rather than deleted
entirely, but maybe that's not a correct interpretation.

[Evan Stade, 2014-08-21 17:59:31]

On 2014/08/21 17:46:39, sgurun wrote:
> On 2014/08/21 17:40:16, benm wrote:
> > On 2014/08/21 17:32:32, Evan Stade wrote:
> > > This CL has been ready to go for a week; +sgurun for android_webview/
review
> > > from local timezone.
> > 
> > Just for the record, I don't agree with reflection on this. Chrome is
supposed
> > to be SDK clean, and reflection isn't normally allowed.
> > 
> > I thought Ted said he recently replaced ListPopupWindow with something else
> > earlier in the thread.
> 
> I'd like to second Ben's earlier question: Is this the mechanism that the
> android frameworks team suggests?

Do you have a contact on that team we could CC here?

[sgurun-gerrit only, 2014-08-21 18:04:25]

On 2014/08/21 17:59:31, Evan Stade wrote:
> On 2014/08/21 17:46:39, sgurun wrote:
> > On 2014/08/21 17:40:16, benm wrote:
> > > On 2014/08/21 17:32:32, Evan Stade wrote:
> > > > This CL has been ready to go for a week; +sgurun for android_webview/
> review
> > > > from local timezone.
> > > 
> > > Just for the record, I don't agree with reflection on this. Chrome is
> supposed
> > > to be SDK clean, and reflection isn't normally allowed.
> > > 
> > > I thought Ted said he recently replaced ListPopupWindow with something
else
> > > earlier in the thread.
> > 
> > I'd like to second Ben's earlier question: Is this the mechanism that the
> > android frameworks team suggests?
> 
> Do you have a contact on that team we could CC here?

I will try to find a person. If they cannot comment here due to lack of chromium
account, then maybe I will create an internal bug.

[sgurun-gerrit only, 2014-08-21 23:05:23]

On 2014/08/21 18:04:25, sgurun wrote:
> On 2014/08/21 17:59:31, Evan Stade wrote:
> > On 2014/08/21 17:46:39, sgurun wrote:
> > > On 2014/08/21 17:40:16, benm wrote:
> > > > On 2014/08/21 17:32:32, Evan Stade wrote:
> > > > > This CL has been ready to go for a week; +sgurun for android_webview/
> > review
> > > > > from local timezone.
> > > > 
> > > > Just for the record, I don't agree with reflection on this. Chrome is
> > supposed
> > > > to be SDK clean, and reflection isn't normally allowed.
> > > > 
> > > > I thought Ted said he recently replaced ListPopupWindow with something
> else
> > > > earlier in the thread.
> > > 
> > > I'd like to second Ben's earlier question: Is this the mechanism that the
> > > android frameworks team suggests?
> > 
> > Do you have a contact on that team we could CC here?
> 
> I will try to find a person. If they cannot comment here due to lack of
chromium
> account, then maybe I will create an internal bug.

based on discussion with frameworks team: aw lgtm

[Evan Stade, 2014-08-22 01:11:10]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-08-22 01:12:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/estade@chromium.org/442933002/100001

[commit-bot: I haz the power, 2014-08-22 02:03:06]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[commit-bot: I haz the power, 2014-08-22 02:16:54]

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

[commit-bot: I haz the power, 2014-08-22 02:16:57]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[aurimas (slooooooooow), 2014-08-22 03:59:31]

The CQ bit was checked by aurimas@chromium.org

[commit-bot: I haz the power, 2014-08-22 04:00:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/estade@chromium.org/442933002/100001

[commit-bot: I haz the power, 2014-08-22 04:55:28]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[commit-bot: I haz the power, 2014-08-22 05:49:43]

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

[commit-bot: I haz the power, 2014-08-22 05:49:44]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[benm (inactive), 2014-08-22 09:20:12]

On 2014/08/22 05:49:44, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   android_dbg_tests_recipe on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

Thanks for following up with the Android team! :-)

[Evan Stade, 2014-08-22 18:07:49]

On 2014/08/22 09:20:12, benm wrote:
> On 2014/08/22 05:49:44, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   android_dbg_tests_recipe on tryserver.chromium.linux
> >
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
> 
> Thanks for following up with the Android team! :-)

Thanks for caring about Chrome.

[Evan Stade, 2014-08-22 18:15:33]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-08-22 18:16:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/estade@chromium.org/442933002/100001

[commit-bot: I haz the power, 2014-08-22 19:11:32]

Committed patchset #6 (100001) as 291463