[Android] Show autofill popup after animation.

[Android] Show autofill popup after animation.

The renderer shows autofill popups immediately upon click or focus
change. If the browser shows an IME (virtual keyboard), then it can zoom
and scroll the webpage, which will hide the popup. This behavior causes
a "flash" of autofill popup on platforms with IME (ChromeOS, Windows 8,
and Android). This patch mostly fixes Android and paves the way for
fixing Windows 8 and ChromeOS.

The fix involves roughly 5 parts.

1) If the platform does not support a virtual keyboard (Mac, Linux,
   Windows 7), the renderer shows the autofill popup immediately.

2) If a virtual keyboard is supported, but disabled (ChromeOS without a
   virtual keyboard and Android with a hardware keyboard), then the
   browser notifies the renderer that the autofill popup can be shown.

3) If the virtual keyboard is already showing, then the browser notifies
   the renderer that the autofill popup can be shown.

     A corner case is the Android keyboard, which can change its own
     size when switching between input fields, thus resizing the webpage
     and hiding the autofill popup. The mitigation is to hide the
     autofill popup only if the input field moves due to the
     resize. This mitigation is not in this patch.

4) If the input field is already in a good position (visible on screen
   and legible size) after the keyboard is shown, then there will be no
   zoom and scroll animations. Then the renderer can show the autofill
   popup.

5) If the input field is zooming and scrolling into a good position
   (visible on screen and legible size) after the keyboard is shown,
   then the renderer waits until the compositor notifies it that the
   this animation has finished. Then the renderer can show the autofill
   popup.

BUG=430318, 440161
Committed: https://crrev.com/f7ebd883699112506893fc0500a65c5a10e563ec
Cr-Commit-Position: refs/heads/master@{#312521}

[please use gerrit instead, 2014-11-21 03:48:20]

Patchset #1 (id:1) has been deleted

[please use gerrit instead, 2014-11-21 03:48:32]

Patchset #1 (id:20001) has been deleted

[please use gerrit instead, 2014-11-21 03:48:44]

Patchset #1 (id:40001) has been deleted

[please use gerrit instead, 2014-11-21 03:48:53]

Patchset #1 (id:60001) has been deleted

[please use gerrit instead, 2014-11-21 03:49:04]

Patchset #1 (id:80001) has been deleted

[please use gerrit instead, 2014-11-21 03:49:19]

Patchset #1 (id:100001) has been deleted

[please use gerrit instead, 2014-11-21 03:49:32]

Patchset #1 (id:120001) has been deleted

[please use gerrit instead, 2014-11-21 03:49:42]

Patchset #1 (id:140001) has been deleted

[please use gerrit instead, 2014-11-21 03:49:50]

Patchset #1 (id:160001) has been deleted

[please use gerrit instead, 2014-11-21 03:49:59]

Patchset #1 (id:150035) has been deleted

[please use gerrit instead, 2014-11-21 03:50:13]

Patchset #1 (id:190001) has been deleted

[please use gerrit instead, 2014-11-21 03:50:22]

Patchset #1 (id:210001) has been deleted

[please use gerrit instead, 2014-11-21 03:50:30]

Patchset #1 (id:230001) has been deleted

[please use gerrit instead, 2014-11-21 04:35:34]

Patchset #1 (id:250001) has been deleted

[please use gerrit instead, 2014-11-22 02:18:21]

rouslan@chromium.org changed reviewers:
+ aelias@chromium.org

[please use gerrit instead, 2014-11-22 02:18:21]

Alexandre: What do you think about these compositor changes? I'm getting
cc_unittests failures, which I will investigate.

Garrett: This patch pretty much does what we need: it does not break desktop
behavior and fixes behavior on Android. It's kind of hacky, though, because I
haven't yet figured out how to receive actual compositor events on desktop.

[aelias_OOO_until_Jul13, 2014-11-22 03:38:11]

https://codereview.chromium.org/715733002/diff/270001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/715733002/diff/270001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.h:483: // If set, then page scale animation has
completed, but not committed yet.
A page scale animation isn't explicitly involved in a commit after creation, so
this phrasing is a bit strange.  I'd say ", but we haven't notified the client
about it yet (this is delayed until after commit to give the scroll offset time
to fully stabilize)"

https://codereview.chromium.org/715733002/diff/270001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.h:484: bool
did_complete_scale_animation_before_commit_;
Please move this next to "pending_page_scale_animation_".

https://codereview.chromium.org/715733002/diff/270001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/715733002/diff/270001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.h:102: virtual void
DidCompletePageScaleAnimationBeforeCommit() = 0;
I don't think it's needed to say "BeforeCommit" in all these places, it's just
unnecessarily verbose.  The methods are called as soon as the page scale
animation completes, which is when one would expect it to be called.  That just
happens to be before commit.

https://codereview.chromium.org/715733002/diff/270001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/715733002/diff/270001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:1342: void
ThreadProxy::DidCompletePageScaleAnimationBeforeCommit() {
This is called from the impl thread, so it should have "OnImplThread" in the
name.

https://codereview.chromium.org/715733002/diff/270001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:1343:
layer_tree_host()->DidCompletePageScaleAnimationBeforeCommit();
You shouldn't directly access the LayerTreeHost from the impl thread, it must
only be accessed from the main thread.  You should post a task on the
main-thread that calls this instead.

https://codereview.chromium.org/715733002/diff/270001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/715733002/diff/270001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:238: #if
!defined(OS_ANDROID)
Why?  Shouldn't we hide on resize on Android, if we hide on scroll?  I assume
this is fixing a problem you've observed; please explain and maybe we can come
up with a cleaner fix.

https://codereview.chromium.org/715733002/diff/270001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.h (right):

https://codereview.chromium.org/715733002/diff/270001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.h:68: void
WillAnimatePageScale(bool will_animate) override;
FocusChangeComplete(bool will_animate) would be a better name.

https://codereview.chromium.org/715733002/diff/270001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/715733002/diff/270001/content/renderer/render...
content/renderer/render_view_impl.cc:1513: FOR_EACH_OBSERVER(RenderViewObserver,
observers_,
It's brittle to tie the autofill showing up at all to an optional
scroll-into-view action that might be suppressed before we ever get here.  And
it also leads you to adding "#if !defined(OS_ANDROID)" in
AutofillAgent::FocusedNodeChanged, which is something we prefer to avoid.

Please make autofill observer notified based on a mandatory event that happens
on every form field focus instead (might need to introduce something like
ViewMsg_FocusChangeComplete for that).  And store will_animate_ in a member
variable that defaults to false in this method.

[please use gerrit instead, 2014-11-24 20:22:32]

Alexandre: Please see my comments inline.

https://codereview.chromium.org/715733002/diff/270001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/715733002/diff/270001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.h:483: // If set, then page scale animation has
completed, but not committed yet.
On 2014/11/22 03:38:11, aelias wrote:
> A page scale animation isn't explicitly involved in a commit after creation,
so
> this phrasing is a bit strange.  I'd say ", but we haven't notified the client
> about it yet (this is delayed until after commit to give the scroll offset
time
> to fully stabilize)"

Done.

https://codereview.chromium.org/715733002/diff/270001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.h:484: bool
did_complete_scale_animation_before_commit_;
On 2014/11/22 03:38:11, aelias wrote:
> Please move this next to "pending_page_scale_animation_".

Done.

https://codereview.chromium.org/715733002/diff/270001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/715733002/diff/270001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.h:102: virtual void
DidCompletePageScaleAnimationBeforeCommit() = 0;
On 2014/11/22 03:38:11, aelias wrote:
> I don't think it's needed to say "BeforeCommit" in all these places, it's just
> unnecessarily verbose.  The methods are called as soon as the page scale
> animation completes, which is when one would expect it to be called.  That
just
> happens to be before commit.

Done.

https://codereview.chromium.org/715733002/diff/270001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/715733002/diff/270001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:1342: void
ThreadProxy::DidCompletePageScaleAnimationBeforeCommit() {
On 2014/11/22 03:38:11, aelias wrote:
> This is called from the impl thread, so it should have "OnImplThread" in the
> name.

Done.

https://codereview.chromium.org/715733002/diff/270001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:1343:
layer_tree_host()->DidCompletePageScaleAnimationBeforeCommit();
On 2014/11/22 03:38:11, aelias wrote:
> You shouldn't directly access the LayerTreeHost from the impl thread, it must
> only be accessed from the main thread.  You should post a task on the
> main-thread that calls this instead.

Done.

https://codereview.chromium.org/715733002/diff/270001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/715733002/diff/270001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:238: #if
!defined(OS_ANDROID)
On 2014/11/22 03:38:11, aelias wrote:
> Why?  Shouldn't we hide on resize on Android, if we hide on scroll?  I assume
> this is fixing a problem you've observed; please explain and maybe we can come
> up with a cleaner fix.

This is to fix the autofill popup momentarily showing and then hiding when
switching between input fields.

When you switch between input fields, the popup shows up immediately, because
there is no animation. However, the software keyboard then resizes itself to
show/hide personalized suggestions [1]. These personalized suggestions show up
for the username field, but not for the password field, for example. The
keyboard resize causes the render to resize as well. The resize of the renderer
then causes the autofill popup to hide itself immediately after it showed up.

[1] http://www.pcmag.com/article2/0,2817,2455227,00.asp

https://codereview.chromium.org/715733002/diff/270001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/715733002/diff/270001/content/renderer/render...
content/renderer/render_view_impl.cc:1513: FOR_EACH_OBSERVER(RenderViewObserver,
observers_,
On 2014/11/22 03:38:11, aelias wrote:
> It's brittle to tie the autofill showing up at all to an optional
> scroll-into-view action that might be suppressed before we ever get here.  And
> it also leads you to adding "#if !defined(OS_ANDROID)" in
> AutofillAgent::FocusedNodeChanged, which is something we prefer to avoid.
> 
> Please make autofill observer notified based on a mandatory event that happens
> on every form field focus instead (might need to introduce something like
> ViewMsg_FocusChangeComplete for that).  And store will_animate_ in a member
> variable that defaults to false in this method.

FocusChangeComplete event is a good idea. On Android this can be invoked either
from here or from DidCompletePageScaleAnimation. I am working on figuring out
where to issue this event on desktop platforms. Let me know if you have
suggestions.

[please use gerrit instead, 2014-11-24 20:22:39]

Patchset #2 (id:290001) has been deleted

[aelias_OOO_until_Jul13, 2014-11-25 01:26:35]

https://codereview.chromium.org/715733002/diff/270001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/715733002/diff/270001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:238: #if
!defined(OS_ANDROID)
On 2014/11/24 20:22:31, Rouslan Solomakhin wrote:
> On 2014/11/22 03:38:11, aelias wrote:
> > Why?  Shouldn't we hide on resize on Android, if we hide on scroll?  I
assume
> > this is fixing a problem you've observed; please explain and maybe we can
come
> > up with a cleaner fix.
> 
> This is to fix the autofill popup momentarily showing and then hiding when
> switching between input fields.
> 
> When you switch between input fields, the popup shows up immediately, because
> there is no animation. However, the software keyboard then resizes itself to
> show/hide personalized suggestions [1]. These personalized suggestions show up
> for the username field, but not for the password field, for example. The
> keyboard resize causes the render to resize as well. The resize of the
renderer
> then causes the autofill popup to hide itself immediately after it showed up.
> 
> [1] http://www.pcmag.com/article2/0,2817,2455227,00.asp

OK, then should we not hide on resize on all platforms, if it's safe to do so?

https://codereview.chromium.org/715733002/diff/270001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/715733002/diff/270001/content/renderer/render...
content/renderer/render_view_impl.cc:1513: FOR_EACH_OBSERVER(RenderViewObserver,
observers_,
On 2014/11/24 20:22:31, Rouslan Solomakhin wrote:
> On 2014/11/22 03:38:11, aelias wrote:
> > It's brittle to tie the autofill showing up at all to an optional
> > scroll-into-view action that might be suppressed before we ever get here. 
And
> > it also leads you to adding "#if !defined(OS_ANDROID)" in
> > AutofillAgent::FocusedNodeChanged, which is something we prefer to avoid.
> > 
> > Please make autofill observer notified based on a mandatory event that
happens
> > on every form field focus instead (might need to introduce something like
> > ViewMsg_FocusChangeComplete for that).  And store will_animate_ in a member
> > variable that defaults to false in this method.
> 
> FocusChangeComplete event is a good idea. On Android this can be invoked
either
> from here or from DidCompletePageScaleAnimation. I am working on figuring out
> where to issue this event on desktop platforms. Let me know if you have
> suggestions.

To be clear, what I'm suggesting is that we should have the sequence with
guaranteed ordering:
1. Focus
2. ScrollIntoView (optional)
3. FocusComplete
4. DidCompletePageScaleAnimation (optional)

The first three events are very close to each other in time.  On ScrollIntoView,
we would set a flag if a page scale animation was issued, and on FocusComplete,
we would either show autofill immediately if that flag is false, or wait until
DidCompletePageScaleAnimation if true.  This means we wouldn't need any
platform-specific logic aside from not triggering the page scale animation.

I don't know the focus code well enough to offer a very specific suggestion of
how it should be added, although I think it would probably be in the browser
process, who sends messages to trigger the first two -- we could send another
message right after that.

[please use gerrit instead, 2014-12-02 00:30:50]

Patchset #3 (id:330001) has been deleted

[please use gerrit instead, 2014-12-12 19:11:00]

Patchset #2 (id:310001) has been deleted

[please use gerrit instead, 2014-12-12 19:11:13]

Patchset #2 (id:350001) has been deleted

[please use gerrit instead, 2014-12-12 19:11:26]

Patchset #2 (id:370001) has been deleted

[please use gerrit instead, 2014-12-12 19:11:37]

Patchset #2 (id:390001) has been deleted

[please use gerrit instead, 2014-12-12 19:11:49]

Patchset #2 (id:410001) has been deleted

[please use gerrit instead, 2014-12-12 19:12:00]

Patchset #2 (id:430001) has been deleted

[please use gerrit instead, 2014-12-12 19:12:11]

Patchset #2 (id:450001) has been deleted

[please use gerrit instead, 2014-12-12 19:12:20]

Patchset #2 (id:470001) has been deleted

[please use gerrit instead, 2014-12-12 19:12:29]

Patchset #2 (id:490001) has been deleted

[please use gerrit instead, 2014-12-12 20:23:25]

Patchset #2 (id:510001) has been deleted

[please use gerrit instead, 2014-12-12 22:56:48]

Patchset #2 (id:530001) has been deleted

[please use gerrit instead, 2014-12-12 22:57:03]

Patchset #2 (id:550001) has been deleted

[please use gerrit instead, 2014-12-12 22:57:17]

Patchset #2 (id:570001) has been deleted

[please use gerrit instead, 2014-12-12 23:11:16]

Patchset #2 (id:590001) has been deleted

[please use gerrit instead, 2014-12-12 23:11:25]

Patchset #2 (id:590001) has been deleted

[please use gerrit instead, 2014-12-12 23:11:42]

Patchset #2 (id:610001) has been deleted

[please use gerrit instead, 2014-12-12 23:11:54]

Patchset #2 (id:630001) has been deleted

[please use gerrit instead, 2014-12-12 23:12:06]

Patchset #2 (id:650001) has been deleted

[please use gerrit instead, 2014-12-12 23:25:23]

Patchset #2 (id:670001) has been deleted

[please use gerrit instead, 2014-12-12 23:25:28]

Patchset #2 (id:670001) has been deleted

[please use gerrit instead, 2014-12-15 18:29:15]

Patchset #2 (id:690001) has been deleted

[please use gerrit instead, 2014-12-15 18:29:24]

Patchset #2 (id:710001) has been deleted

[please use gerrit instead, 2014-12-15 18:39:54]

rouslan@chromium.org changed reviewers:
+ gcasto@chromium.org

[please use gerrit instead, 2014-12-15 18:39:55]

Alexandre: PTAL Patch Set 2.

I was able to get rid of platform #ifdefs. Also I fixed the Chrome OS virtual
keyboard behavior.

I'm yet to figure out how to fix linking errors for ChromeOS and browser_tests
everywhere. Also I need to add tests for the new behavior.

Garrett: Patch Set 2 should fix all popup misbehavior on Android. You may want
to double-check the patch in your environment.

https://codereview.chromium.org/715733002/diff/270001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/715733002/diff/270001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:238: #if
!defined(OS_ANDROID)
On 2014/11/25 01:26:34, aelias wrote:
> On 2014/11/24 20:22:31, Rouslan Solomakhin wrote:
> > On 2014/11/22 03:38:11, aelias wrote:
> > > Why?  Shouldn't we hide on resize on Android, if we hide on scroll?  I
> assume
> > > this is fixing a problem you've observed; please explain and maybe we can
> come
> > > up with a cleaner fix.
> > 
> > This is to fix the autofill popup momentarily showing and then hiding when
> > switching between input fields.
> > 
> > When you switch between input fields, the popup shows up immediately,
because
> > there is no animation. However, the software keyboard then resizes itself to
> > show/hide personalized suggestions [1]. These personalized suggestions show
up
> > for the username field, but not for the password field, for example. The
> > keyboard resize causes the render to resize as well. The resize of the
> renderer
> > then causes the autofill popup to hide itself immediately after it showed
up.
> > 
> > [1] http://www.pcmag.com/article2/0,2817,2455227,00.asp
> 
> OK, then should we not hide on resize on all platforms, if it's safe to do so?

I solved the issue slightly differently that does not require ignoring resize.
Switching between input fields now waits for resize or a timeout before showing
the popup.

https://codereview.chromium.org/715733002/diff/270001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/715733002/diff/270001/content/renderer/render...
content/renderer/render_view_impl.cc:1513: FOR_EACH_OBSERVER(RenderViewObserver,
observers_,
On 2014/11/25 01:26:34, aelias wrote:
> On 2014/11/24 20:22:31, Rouslan Solomakhin wrote:
> > On 2014/11/22 03:38:11, aelias wrote:
> > > It's brittle to tie the autofill showing up at all to an optional
> > > scroll-into-view action that might be suppressed before we ever get here. 
> And
> > > it also leads you to adding "#if !defined(OS_ANDROID)" in
> > > AutofillAgent::FocusedNodeChanged, which is something we prefer to avoid.
> > > 
> > > Please make autofill observer notified based on a mandatory event that
> happens
> > > on every form field focus instead (might need to introduce something like
> > > ViewMsg_FocusChangeComplete for that).  And store will_animate_ in a
member
> > > variable that defaults to false in this method.
> > 
> > FocusChangeComplete event is a good idea. On Android this can be invoked
> either
> > from here or from DidCompletePageScaleAnimation. I am working on figuring
out
> > where to issue this event on desktop platforms. Let me know if you have
> > suggestions.
> 
> To be clear, what I'm suggesting is that we should have the sequence with
> guaranteed ordering:
> 1. Focus
> 2. ScrollIntoView (optional)
> 3. FocusComplete
> 4. DidCompletePageScaleAnimation (optional)
> 
> The first three events are very close to each other in time.  On
ScrollIntoView,
> we would set a flag if a page scale animation was issued, and on
FocusComplete,
> we would either show autofill immediately if that flag is false, or wait until
> DidCompletePageScaleAnimation if true.  This means we wouldn't need any
> platform-specific logic aside from not triggering the page scale animation.
> 
> I don't know the focus code well enough to offer a very specific suggestion of
> how it should be added, although I think it would probably be in the browser
> process, who sends messages to trigger the first two -- we could send another
> message right after that.

The step #2 (FocusComplete) is not necessary, because there's already an event
that happens at the very end: OnHandledGestureEvent/OnHandledClickEvent/etc.
Also, we don't have all of the required information on whether
DidCompletePageScaleAnimation will happen at the end of the focus event in
renderer, because browser makes some decisions as well. The solution is for the
browser to let the renderer know whether it will initiate the page scale
animation or not. See
https://docs.google.com/document/d/1-5bhgEYhjHeJaPBYZZTLA0goc13epj1r_7bU-zFRP...
for a diagram.

[please use gerrit instead, 2014-12-15 19:18:06]

If you're OK with the overall approach, perhaps I should start splitting the CL
into more digestible parts.

[aelias_OOO_until_Jul13, 2014-12-15 21:59:15]

OK, the FocusChangedComplete stuff looks OK, thanks!

https://codereview.chromium.org/715733002/diff/270001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/715733002/diff/270001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:238: #if
!defined(OS_ANDROID)
On 2014/12/15 18:39:55, Rouslan Solomakhin wrote:
> I solved the issue slightly differently that does not require ignoring resize.
> Switching between input fields now waits for resize or a timeout before
showing
> the popup.

Hmm, I don't like the new timeout stuff in ContentViewCore either, sorry.

Going back to first principles on this, I think the underlying reason to hide on
resize is that if the page contents size changes due to the resize (particularly
due to horizontal resizes on desktop), then the form field is likely to move
around.  Whereas with Android keyboard that's rarely an issue because the resize
is purely vertical.  But, even for vertical resizes, there are corner cases like
fixed-position where the form field does move around.

So, I'd suggest that when a resize and its corresponding layout happens, check
to see if the form field node has moved or changed size, and only if so then
hide the autofill popup.

https://codereview.chromium.org/715733002/diff/730001/content/public/renderer...
File content/public/renderer/render_view_observer.h (right):

https://codereview.chromium.org/715733002/diff/730001/content/public/renderer...
content/public/renderer/render_view_observer.h:100: // Can be called from
browser, renderer, or compositor.
Please add comment "This indicates that animations to scroll the focused element
into view (if any) have completed.  May be called more than once for a single
focus."

[please use gerrit instead, 2014-12-16 02:07:16]

Patchset #3 (id:750001) has been deleted

[please use gerrit instead, 2014-12-16 02:07:25]

Patchset #3 (id:770001) has been deleted

[please use gerrit instead, 2014-12-16 02:17:08]

A small comment inline.

https://codereview.chromium.org/715733002/diff/270001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/715733002/diff/270001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:238: #if
!defined(OS_ANDROID)
On 2014/12/15 21:59:14, aelias wrote:
> Going back to first principles on this, I think the underlying reason to hide
on
> resize is that if the page contents size changes due to the resize
(particularly
> due to horizontal resizes on desktop), then the form field is likely to move
> around.  Whereas with Android keyboard that's rarely an issue because the
resize
> is purely vertical.  But, even for vertical resizes, there are corner cases
like
> fixed-position where the form field does move around.
> 
> So, I'd suggest that when a resize and its corresponding layout happens, check
> to see if the form field node has moved or changed size, and only if so then
> hide the autofill popup.

I like the first-principles approach and have cooked up something promising
here. (Will be sharing later.) There is one interesting affect of not having the
timeout when switching between fields: Some webpages run javascript which
changes input element position on focus change. This is such a cornercase that
I'm not worried about it, though. This can be resolved when a "real" fix for
comes along. (I assume the real fix will the in-compositor implementation of
autofill popups, but that's way down the road.)

[aelias_OOO_until_Jul13, 2014-12-16 02:35:52]

https://codereview.chromium.org/715733002/diff/270001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/715733002/diff/270001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:238: #if
!defined(OS_ANDROID)
On 2014/12/16 02:17:08, Rouslan Solomakhin wrote:
> Some webpages run javascript which
> changes input element position on focus change. This is such a cornercase that
> I'm not worried about it, though. This can be resolved when a "real" fix for
> comes along. (I assume the real fix will the in-compositor implementation of
> autofill popups, but that's way down the road.)

Assuming that Javascript usually will do that change synchronously, maybe the
immediate answer to that one is to wait until the event processing completes
before showing autofill.

[please use gerrit instead, 2014-12-16 20:05:49]

On 2014/12/16 02:35:52, aelias wrote:
>
https://codereview.chromium.org/715733002/diff/270001/components/autofill/con...
> File components/autofill/content/renderer/autofill_agent.cc (right):
> 
>
https://codereview.chromium.org/715733002/diff/270001/components/autofill/con...
> components/autofill/content/renderer/autofill_agent.cc:238: #if
> !defined(OS_ANDROID)
> On 2014/12/16 02:17:08, Rouslan Solomakhin wrote:
> > Some webpages run javascript which
> > changes input element position on focus change. This is such a cornercase
that
> > I'm not worried about it, though. This can be resolved when a "real" fix for
> > comes along. (I assume the real fix will the in-compositor implementation of
> > autofill popups, but that's way down the road.)
> 
> Assuming that Javascript usually will do that change synchronously, maybe the
> immediate answer to that one is to wait until the event processing completes
> before showing autofill.

My investigations show that we already do that.

I've only noticed an issue with asynchronous JavaScript on twitter's mobile
signup page (https://mobile.twitter.com/signup). If you type in a username
that's already taken and tap on the password field, then JavaScript will
asynchronously check whether the username is taken and show a message "Username
has already been taken." Showing this message will move the password field,
which hides the password generation popup. I think that (1) this is such a tiny
corner-case that we should not worry about it at all;  (2) moving around the
focused element is a bad idea for JavaScript in the first place.

[aelias_OOO_until_Jul13, 2014-12-16 20:10:57]

OK, sounds reasonable.  Let me know when you have a new patch up.

[please use gerrit instead, 2014-12-16 21:12:05]

Patchset #3 (id:790001) has been deleted

[please use gerrit instead, 2014-12-16 21:15:12]

Patchset #3 (id:810001) has been deleted

[please use gerrit instead, 2014-12-16 21:17:42]

Alexandre: PTAL Patch Set 3.

Since RenderViewObserver::Resized() was used only by AutofillAgent to hide the
popup, I renamed it to RenderVieObserver::FocusedElementMovedOnResize() and made
it trigger only when the bounds of the focused element have changed due to
viewport resize.

https://codereview.chromium.org/715733002/diff/730001/content/public/renderer...
File content/public/renderer/render_view_observer.h (right):

https://codereview.chromium.org/715733002/diff/730001/content/public/renderer...
content/public/renderer/render_view_observer.h:100: // Can be called from
browser, renderer, or compositor.
On 2014/12/15 21:59:15, aelias wrote:
> Please add comment "This indicates that animations to scroll the focused
element
> into view (if any) have completed.  May be called more than once for a single
> focus."

Done.

[aelias_OOO_until_Jul13, 2014-12-16 23:15:49]

This looks pretty clean to me now, thanks.  Please just add test coverage and
fix the try failures and then we can add OWNERS for landing.  (I personally
won't insist on splitting into smaller parts, personally I'm fine with large-ish
patches.)

https://codereview.chromium.org/715733002/diff/830001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/715733002/diff/830001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:245: void
AutofillAgent::OrientationChangeEvent() {
Can we delete this one?  It seems redundant with the resize mechanism.

[please use gerrit instead, 2014-12-16 23:39:22]

Adding test coverage and fixing test failures.

https://codereview.chromium.org/715733002/diff/830001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/715733002/diff/830001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:245: void
AutofillAgent::OrientationChangeEvent() {
On 2014/12/16 23:15:49, aelias wrote:
> Can we delete this one?  It seems redundant with the resize mechanism.

Evan is removing it in https://codereview.chromium.org/809623005/.

[please use gerrit instead, 2014-12-18 20:18:37]

Patchset #4 (id:850001) has been deleted

[please use gerrit instead, 2014-12-18 20:18:51]

Patchset #4 (id:870001) has been deleted

[please use gerrit instead, 2014-12-18 22:44:58]

Patchset #4 (id:890001) has been deleted

[please use gerrit instead, 2014-12-19 00:12:13]

Patchset #4 (id:910001) has been deleted

[please use gerrit instead, 2014-12-19 00:32:41]

Patchset #4 (id:930001) has been deleted

[please use gerrit instead, 2014-12-19 21:50:44]

Patchset #4 (id:950001) has been deleted

[please use gerrit instead, 2014-12-21 01:36:17]

Patchset #4 (id:970001) has been deleted

[please use gerrit instead, 2014-12-21 19:51:31]

Patchset #4 (id:990001) has been deleted

[please use gerrit instead, 2014-12-22 04:49:59]

Patchset #4 (id:1010001) has been deleted

[please use gerrit instead, 2014-12-22 04:50:11]

Patchset #4 (id:1030001) has been deleted

[please use gerrit instead, 2014-12-22 05:09:48]

Patchset #4 (id:1050001) has been deleted

[please use gerrit instead, 2014-12-22 05:09:53]

Patchset #4 (id:1050001) has been deleted

[please use gerrit instead, 2014-12-22 05:10:03]

Patchset #4 (id:1070001) has been deleted

[please use gerrit instead, 2014-12-22 05:10:14]

Patchset #4 (id:1090001) has been deleted

[please use gerrit instead, 2014-12-22 21:46:32]

Patchset #5 (id:1130001) has been deleted

[please use gerrit instead, 2014-12-22 21:46:44]

Patchset #5 (id:1150001) has been deleted

[please use gerrit instead, 2014-12-22 22:31:18]

Patchset #5 (id:1170001) has been deleted

[please use gerrit instead, 2014-12-22 23:19:40]

Patchset #5 (id:1190001) has been deleted

[please use gerrit instead, 2014-12-23 00:23:03]

Patchset #5 (id:1210001) has been deleted

[please use gerrit instead, 2014-12-23 02:17:29]

Patchset #5 (id:1230001) has been deleted

[please use gerrit instead, 2014-12-23 02:17:44]

Patchset #5 (id:1250001) has been deleted

[please use gerrit instead, 2014-12-23 02:47:14]

Patchset #5 (id:1270001) has been deleted

[please use gerrit instead, 2014-12-23 02:48:52]

Patchset #5 (id:1290001) has been deleted

[please use gerrit instead, 2014-12-23 17:13:25]

Patchset #4 (id:1110001) has been deleted

[please use gerrit instead, 2014-12-24 00:49:06]

Patchset #4 (id:1310001) has been deleted

[please use gerrit instead, 2014-12-24 00:56:43]

Alexandre: PTAL cc/ in Patch Set 4. 

I've added test coverage in layer_tree_host_impl_unittest.cc. Let me know if you
can think of more tests to add.

Other cc/ OWNERS: Feel free to take a look instead of Alexandre, while he is out
of office.

[please use gerrit instead, 2014-12-24 00:59:13]

rouslan@chromium.org changed reviewers:
- gcasto@chromium.org

[please use gerrit instead, 2014-12-24 01:03:44]

rouslan@chromium.org changed reviewers:
+ gcasto@chromium.org

[please use gerrit instead, 2014-12-24 01:03:45]

Garrett: PTAL autofill in Patch Set 4:

  chrome/renderer/autofill/page_click_tracker_browsertest.cc
  chrome/renderer/autofill/password_generation_agent_browsertest.cc
  components/autofill/content/renderer/autofill_agent.h
  components/autofill/content/renderer/autofill_agent.cc
  components/autofill/content/renderer/form_autofill_util.h
  components/autofill/content/renderer/form_autofill_util.cc
  components/autofill/content/renderer/page_click_tracker.h
  components/autofill/content/renderer/page_click_tracker.cc

Other autofill OWNERS: Feel free to take a look instead of Garrett, while he is
out of office.

[please use gerrit instead, 2014-12-24 01:06:31]

rouslan@chromium.org changed reviewers:
+ newt@chromium.org

[please use gerrit instead, 2014-12-24 01:06:33]

Newton: PTAL AutofillPopupWithKeyboardTest.java in Patch Set 4:


chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java

[please use gerrit instead, 2014-12-24 01:11:32]

rouslan@chromium.org changed reviewers:
+ jbauman@chromium.org

[please use gerrit instead, 2014-12-24 01:11:33]

John: PTAL render_widget_compositor Patch Set 4:

 content/renderer/gpu/render_widget_compositor.h
 content/renderer/gpu/render_widget_compositor.cc

Other render_widget_compositor OWNERs: Feel free to take a look instead of John,
while he is out of office.

[please use gerrit instead, 2014-12-24 01:15:42]

rouslan@chromium.org changed reviewers:
+ msw@chromium.org

[please use gerrit instead, 2014-12-24 01:15:43]

Mike: PTAL textfield in Patch Set 4:

 ui/views/controls/textfield/textfield.h
 ui/views/controls/textfield/textfield.cc

Other textfield OWNERs: Feel free to take a look instead of Mike, while he is
out of office.

[please use gerrit instead, 2014-12-24 01:20:21]

rouslan@chromium.org changed reviewers:
+ benm@chromium.org

[please use gerrit instead, 2014-12-24 01:20:22]

Ben: PTAL android in Patch Set 4:

 android_webview/browser/hardware_renderer.h

content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java

content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java

content/public/android/javatests/src/org/chromium/content/browser/input/AdapterInputConnectionTest.java

Other android OWNERs: Feel free to take a look instead of Ben, while he is out
of office.

[please use gerrit instead, 2014-12-24 01:25:02]

rouslan@chromium.org changed reviewers:
+ kevers@chromium.org

[please use gerrit instead, 2014-12-24 01:25:03]

Kevin, PTAL keyboard_controller in Patch Set 4:

 ui/keyboard/keyboard_controller.cc

Other keyboard_controller OWNERs, feel free to take a look instead of Kevin,
while he is out of office.

[please use gerrit instead, 2014-12-24 01:29:19]

rouslan@chromium.org changed reviewers:
+ wfh@chromium.org

[please use gerrit instead, 2014-12-24 01:29:20]

Will: PTAL view_messages in Patch Set 4:

 content/common/view_messages.h

Other view_messages OWNERs, feel free to take a look instead of Will, while he
is out of office.

[please use gerrit instead, 2014-12-24 01:32:48]

rouslan@chromium.org changed reviewers:
+ yukawa@chromium.org

[please use gerrit instead, 2014-12-24 01:32:49]

Yohei, PTAL ime in Patch Set 4:

 ui/base/ime/dummy_input_method.h
 ui/base/ime/dummy_input_method.cc
 ui/base/ime/dummy_text_input_client.h
 ui/base/ime/dummy_text_input_client.cc
 ui/base/ime/input_method.h
 ui/base/ime/input_method_base.h
 ui/base/ime/input_method_base.cc
 ui/base/ime/input_method_chromeos_unittest.cc
 ui/base/ime/mock_input_method.h
 ui/base/ime/mock_input_method.cc
 ui/base/ime/remote_input_method_win.cc
 ui/base/ime/text_input_client.h


Other ime OWNERs, feel free to take a look instead of Yohei, while he is out of
office.

[Will Harris, 2014-12-24 01:36:05]

new IPC message in content/common/view_messages.h LGTM

[please use gerrit instead, 2014-12-24 01:37:14]

rouslan@chromium.org changed reviewers:
+ vollick@chromium.org

[please use gerrit instead, 2014-12-24 01:37:15]

Ian, PTAL compositor in Patch Set 4:

 ui/compositor/compositor.cc
 ui/compositor/compositor.h

Other compositor OWNERs, feel free to take a look instead of Ian, while he is
out of office.

[please use gerrit instead, 2014-12-24 01:42:34]

rouslan@chromium.org changed reviewers:
+ yukishiino@chromium.org

[please use gerrit instead, 2014-12-24 01:42:35]

Yuki, PTAL input_method_bridge in Patch Set 4:

 ui/views/ime/input_method_bridge.h
 ui/views/ime/input_method_bridge.cc

[please use gerrit instead, 2014-12-24 01:45:54]

rouslan@chromium.org changed reviewers:
+ aa@chromium.org

[please use gerrit instead, 2014-12-24 01:45:55]

Aaron, PTAL mojo in Patch Set 4:

  mojo/services/html_viewer/weblayertreeview_impl.h

[please use gerrit instead, 2014-12-24 01:50:01]

rouslan@chromium.org changed reviewers:
+ nasko@chromium.org

[please use gerrit instead, 2014-12-24 01:50:02]

Nasko, PTAL content in Patch Set 4:

 content/public/renderer/render_view_observer.h
 content/public/test/render_view_test.h
 content/public/test/render_view_test.cc
 content/renderer/render_view_impl.h
 content/renderer/render_view_impl.cc
 content/renderer/render_widget.h
 content/renderer/render_widget.cc
 content/test/web_layer_tree_view_impl_for_testing.h

Other content OWNERs, feel free to take a look instead of Nasko, while he is out
of office.

[please use gerrit instead, 2014-12-24 01:52:32]

rouslan@chromium.org changed reviewers:
+ thakis@chromium.org

[please use gerrit instead, 2014-12-24 01:52:34]

Nico, PTAL prefix_selector in Patch Set 4:

 ui/views/controls/prefix_selector.h
 ui/views/controls/prefix_selector.cc

[please use gerrit instead, 2014-12-24 01:55:05]

Sorry for the "spam," everyone. This bug-fix requires piping events through
multiple layers, which creates the need to touch many files. Please let me know
if you would prefer if I split your part of the review into a separate CL. Happy
Holidays!

[yukawa, 2014-12-24 03:09:49]

lgtm

Happy Holidays!

It's OK with me to add some methods to ui/base/ime/input_method.h and
ui/base/ime/text_input_client.h to

https://codereview.chromium.org/715733002/diff/1330001/ui/base/ime/input_meth...
File ui/base/ime/input_method.h (right):

https://codereview.chromium.org/715733002/diff/1330001/ui/base/ime/input_meth...
ui/base/ime/input_method.h:166: virtual void SetSupportsOnScreenKeyboard(bool
supported) = 0;
I'm still unsure why we need setter for this property.  Why is this mutable?

https://codereview.chromium.org/715733002/diff/1330001/ui/base/ime/input_meth...
ui/base/ime/input_method.h:170: virtual bool SupportsOnScreenKeyboard() const =
0;
Related to the above comment, if the return value of this method may change at
run-time, maybe we may want to clarify with method name and/or comments.

https://codereview.chromium.org/715733002/diff/1330001/ui/base/ime/text_input...
File ui/base/ime/text_input_client.h (right):

https://codereview.chromium.org/715733002/diff/1330001/ui/base/ime/text_input...
ui/base/ime/text_input_client.h:182: virtual void OnKeyboardBoundsUnchanged() =
0;
Sorry, I'm not sure if this comment is clear to other Chromium developers in
terms of when this method may/should/must be called back.  For example, I guess
we can detect the timing "when the virtual keyboard was requested to be shown,
but was already showing", and call this method at that moment, but I have a
feeling that calling it on Windows causes some unexpected side effects at
somewhere else.  If we shouldn't call this method under certain conditions,
maybe we can clarify it with method name and/or comments.

[yukawa, 2014-12-24 03:12:07]

Oops, sorry, previous mail was my misoperation. not lgtm yet.

[yukawa, 2014-12-24 03:21:32]

Adding new methods to some methods to ui/base/ime/input_method.h and
ui/base/ime/text_input_client.h is OK with me.  But I also want to minimize the
risk of misuse of them.  Can you create another CL that copies
ui/base/ime/input_method.h and
ui/base/ime/text_input_client.h from this CL with including yukishino@ and
shuchen@ in the reviewer list?  I'm happy to discuss what we can do there.

Happy Holidays!

[Yuki, 2014-12-24 06:08:24]

https://codereview.chromium.org/715733002/diff/1330001/ui/keyboard/keyboard_c...
File ui/keyboard/keyboard_controller.cc (right):

https://codereview.chromium.org/715733002/diff/1330001/ui/keyboard/keyboard_c...
ui/keyboard/keyboard_controller.cc:482:
proxy_->GetInputMethod()->GetTextInputClient()->OnKeyboardBoundsUnchanged();
Why the text input client want to be notified this event?
Which class implements OnKeyboardBoundsUnchanged?

Could you show your design plan at CL description?
I need more clues to figure out your design.

[Yuki, 2014-12-24 06:08:50]

yukishiino@chromium.org changed reviewers:
+ shuchen@chromium.org

[please use gerrit instead, 2014-12-24 18:25:50]

Thank you for the prompt responses. The overarching theme seems to be confusion
due to lack of design description. As Yohei and Yuki requested, I've moved the
aura part of the review into a separate CL with an improved design description:

https://codereview.chromium.org/826713002/

The following files will be reviewed in this separate CL:

content/browser/renderer_host/render_widget_host_view_aura.h
content/browser/renderer_host/render_widget_host_view_aura.cc
content/common/view_messages.h
ui/base/ime/dummy_input_method.h
ui/base/ime/dummy_input_method.cc
ui/base/ime/dummy_text_input_client.h
ui/base/ime/dummy_text_input_client.cc
ui/base/ime/input_method.h
ui/base/ime/input_method_base.h
ui/base/ime/input_method_base.cc
ui/base/ime/input_method_chromeos_unittest.cc
ui/base/ime/mock_input_method.h
ui/base/ime/mock_input_method.cc
ui/base/ime/remote_input_method_win.cc
ui/base/ime/text_input_client.h
ui/keyboard/keyboard_controller.cc
ui/views/controls/prefix_selector.h
ui/views/controls/prefix_selector.cc
ui/views/controls/textfield/textfield.h
ui/views/controls/textfield/textfield.cc
ui/views/ime/input_method_bridge.h
ui/views/ime/input_method_bridge.cc

[please use gerrit instead, 2014-12-24 18:27:58]

rouslan@chromium.org changed reviewers:
- shuchen@chromium.org, thakis@chromium.org, wfh@chromium.org,
yukawa@chromium.org, yukishiino@chromium.org

[aelias_OOO_until_Jul13, 2015-01-05 19:30:59]

aelias@chromium.org changed reviewers:
+ wfh@chromium.org, yukawa@chromium.org

[aelias_OOO_until_Jul13, 2015-01-05 19:31:01]

lgtm, thanks.

[nasko, 2015-01-05 19:38:01]

Doesn't this CL depend on the ChromeOS one you pointed to
(https://codereview.chromium.org/826713002/)? It is the one that contains the
IPC message the browser side will send to the renderer.

One nit while reviewing.

https://codereview.chromium.org/715733002/diff/1350001/content/browser/render...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/715733002/diff/1350001/content/browser/render...
content/browser/renderer_host/compositor_impl_android.cc:651: void
CompositorImpl::DidCompletePageScaleAnimation() {}
Why add an empty method here instead of inlining it in the header?

[please use gerrit instead, 2015-01-05 20:23:21]

Patchset #6 (id:1370001) has been deleted

[please use gerrit instead, 2015-01-05 20:28:25]

Nasko, PTAL Patch Set 6.

This patch here is now completely independent of the ChromeOS patch and vice
versa. The result is that this patch fixes the issue only Android. The ChromeOS
patch will fix the issue on ChromeOS, after this patch lands. (Windows 8 fix
still needs to be written.)

https://codereview.chromium.org/715733002/diff/1350001/content/browser/render...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/715733002/diff/1350001/content/browser/render...
content/browser/renderer_host/compositor_impl_android.cc:651: void
CompositorImpl::DidCompletePageScaleAnimation() {}
On 2015/01/05 19:38:01, nasko wrote:
> Why add an empty method here instead of inlining it in the header?

Good catch! Inlined.

[nasko, 2015-01-05 21:49:23]

In general, we are trying to move away from RenderView to RenderFrame. It isn't
as easy to do it in this change, but please keep it in mind in the future and
structure code to avoid using RenderView.

LGTM

https://codereview.chromium.org/715733002/diff/1390001/content/public/test/re...
File content/public/test/render_view_test.cc (right):

https://codereview.chromium.org/715733002/diff/1390001/content/public/test/re...
content/public/test/render_view_test.cc:338: void
RenderViewTest::SimulateFocusChangeCompleteMessageReceived() {
Is this method needed without the IPC message? Or will it be used in the future
when all the changes have landed?

[Garrett Casto, 2015-01-06 20:01:09]

https://codereview.chromium.org/715733002/diff/1390001/components/autofill/co...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/715733002/diff/1390001/components/autofill/co...
components/autofill/content/renderer/autofill_agent.cc:217: element_.reset();
Is this safe? |element_| is mostly used to display information after
communication with the browser. It seems like this could race with that IPC and
possibly end up null. Admittedly while the current code wouldn't crash it would
be pointing to the wrong value which isn't ideal either. Philosophically this
seems to make more sense, but it seems like we need to add null checks
everywhere |element_| is used.

https://codereview.chromium.org/715733002/diff/1390001/components/autofill/co...
File components/autofill/content/renderer/page_click_tracker.cc (right):

https://codereview.chromium.org/715733002/diff/1390001/components/autofill/co...
components/autofill/content/renderer/page_click_tracker.cc:118:
&focused_element).Contains(x, y)) {
Was the old code incorrect when the page was zoomed? If so, would you mind
adding a test for this now?

https://codereview.chromium.org/715733002/diff/1390001/components/autofill/co...
File components/autofill/content/renderer/page_click_tracker.h (right):

https://codereview.chromium.org/715733002/diff/1390001/components/autofill/co...
components/autofill/content/renderer/page_click_tracker.h:59:
base::WeakPtrFactory<PageClickTracker> weak_ptr_factory_;
This is unnecessary now.

[please use gerrit instead, 2015-01-09 22:45:03]

Patchset #7 (id:1410001) has been deleted

[please use gerrit instead, 2015-01-09 23:07:50]

Patchset #7 (id:1430001) has been deleted

[please use gerrit instead, 2015-01-13 01:01:59]

Patchset #7 (id:1450001) has been deleted

[please use gerrit instead, 2015-01-13 02:12:38]

Patchset #7 (id:1470001) has been deleted

[please use gerrit instead, 2015-01-13 02:14:56]

Garrett, PTAL Patch Set 7.

Nasko, please see my answer to your question inline.

https://codereview.chromium.org/715733002/diff/1390001/components/autofill/co...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/715733002/diff/1390001/components/autofill/co...
components/autofill/content/renderer/autofill_agent.cc:217: element_.reset();
On 2015/01/06 20:01:09, Garrett Casto wrote:
> Is this safe? |element_| is mostly used to display information after
> communication with the browser. It seems like this could race with that IPC
and
> possibly end up null. Admittedly while the current code wouldn't crash it
would
> be pointing to the wrong value which isn't ideal either. Philosophically this
> seems to make more sense, but it seems like we need to add null checks
> everywhere |element_| is used.

On a second look, nulling |element_| is not required. I removed this change.

https://codereview.chromium.org/715733002/diff/1390001/components/autofill/co...
File components/autofill/content/renderer/page_click_tracker.cc (right):

https://codereview.chromium.org/715733002/diff/1390001/components/autofill/co...
components/autofill/content/renderer/page_click_tracker.cc:118:
&focused_element).Contains(x, y)) {
On 2015/01/06 20:01:09, Garrett Casto wrote:
> Was the old code incorrect when the page was zoomed? If so, would you mind
> adding a test for this now?

Added a test. The old code was incorrect when the page was "scaled" using a
pinch movement of your fingers on a touch screen (for example). Zoom was OK.

You can experience the bug in Chrome on Android when you try to use the username
field on https://form870.appspot.com/smoke.html with a zoomed in page. Zoom the
page using a two-finger pinch on your Android device screen. The old code does
not trigger the username suggestion popup when you tap the username. The new
code triggers it.

https://codereview.chromium.org/715733002/diff/1390001/components/autofill/co...
File components/autofill/content/renderer/page_click_tracker.h (right):

https://codereview.chromium.org/715733002/diff/1390001/components/autofill/co...
components/autofill/content/renderer/page_click_tracker.h:59:
base::WeakPtrFactory<PageClickTracker> weak_ptr_factory_;
On 2015/01/06 20:01:09, Garrett Casto wrote:
> This is unnecessary now.

Removed.

https://codereview.chromium.org/715733002/diff/1390001/content/public/test/re...
File content/public/test/render_view_test.cc (right):

https://codereview.chromium.org/715733002/diff/1390001/content/public/test/re...
content/public/test/render_view_test.cc:338: void
RenderViewTest::SimulateFocusChangeCompleteMessageReceived() {
On 2015/01/05 21:49:23, nasko wrote:
> Is this method needed without the IPC message? Or will it be used in the
future
> when all the changes have landed?

Upon further investigation (which took a bit longer than I expected), this
method is not necessary. Instead of simulating the event here, I can get the
actual event by (1) changing SimulateMouseClick to send MouseUp after MouseDown
and (2) registering a user gesture after loading  a testing web page. The user
gesture unlocks the "show-ime" event, which autofill agent now uses to determine
when it's OK to show the autofill popup. The user gesture that I've decided to
use is a mouse click on a dummy button on the webpage. This does not alter the
result of the browser tests.

[please use gerrit instead, 2015-01-13 02:22:21]

rouslan@chromium.org changed reviewers:
- jbauman@chromium.org, kevers@chromium.org, msw@chromium.org,
yukawa@chromium.org

[please use gerrit instead, 2015-01-13 02:22:22]

Newton, ping for review of
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java.

Ben, ping for review of:
android_webview/browser/hardware_renderer.h
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
content/public/android/javatests/src/org/chromium/content/browser/input/AdapterInputConnectionTest.java

Ian, ping for review of ui/compositor/compositor.h.

Aaron, ping for review of mojo/services/html_viewer/weblayertreeview_impl.h.

[please use gerrit instead, 2015-01-14 22:48:58]

rouslan@chromium.org changed reviewers:
- benm@chromium.org

[please use gerrit instead, 2015-01-14 22:53:09]

rouslan@chromium.org changed reviewers:
+ jdduke@chromium.org, torne@chromium.org

[please use gerrit instead, 2015-01-14 22:53:10]

Jared, PTAL in Patch Set 7:

content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
content/public/android/javatests/src/org/chromium/content/browser/input/AdapterInputConnectionTest.java


Richard, PTAL in Patch Set 7:

android_webview/browser/hardware_renderer.h

[jdduke (slow), 2015-01-14 22:59:49]

jdduke@chromium.org changed reviewers:
+ aurimas@chromium.org

[jdduke (slow), 2015-01-14 22:59:51]

> Jared, PTAL in Patch Set 7:
> 
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
>
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
>
content/public/android/javatests/src/org/chromium/content/browser/input/AdapterInputConnectionTest.java
> 
> 

Owner lgtm, but I'd like aurimas@ to take a look at those changes as well.

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

https://codereview.chromium.org/715733002/diff/1490001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1573:
mWebContents.scrollFocusedEditableNodeIntoView();
Do we still need this logic?

[please use gerrit instead, 2015-01-14 23:04:02]

Aurimas, PTAL in Patch Set 7:

content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
content/public/android/javatests/src/org/chromium/content/browser/input/AdapterInputConnectionTest.java

[Ian Vollick, 2015-01-14 23:06:42]

On 2015/01/14 23:04:02, Rouslan Solomakhin wrote:
> Aurimas, PTAL in Patch Set 7:
> 
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
>
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
>
content/public/android/javatests/src/org/chromium/content/browser/input/AdapterInputConnectionTest.java

ui/compositor lgtm.

[please use gerrit instead, 2015-01-14 23:09:27]

Jared, please see my reply inline. Let me know if I misunderstood your comment.

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

https://codereview.chromium.org/715733002/diff/1490001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1573:
mWebContents.scrollFocusedEditableNodeIntoView();
On 2015/01/14 22:59:51, jdduke wrote:
> Do we still need this logic?

This logic scrolls the focused element into view after the virtual keyboard
animates onto the screen. Without this logic, the focused element may end up
behind the keyboard. My changes do not affect this logic. It's still necessary.

[Garrett Casto, 2015-01-14 23:25:01]

components/autofill/... and chrome/renderer/autofill/... LGTM

[aurimas (slooooooooow), 2015-01-15 00:50:14]

aurimas@chromium.org changed reviewers:
+ yukawa@chromium.org

[aurimas (slooooooooow), 2015-01-15 00:50:16]

On 2015/01/14 at 23:04:02, rouslan wrote:
> Aurimas, PTAL in Patch Set 7:
> 
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
>
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
>
content/public/android/javatests/src/org/chromium/content/browser/input/AdapterInputConnectionTest.java


content/public/android/java/src/org/chromium/content/browser/* looks reasonable
to me - LTGM

https://codereview.chromium.org/715733002/diff/1490001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
(right):

https://codereview.chromium.org/715733002/diff/1490001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:298:
== Configuration.KEYBOARD_NOKEYS) {
Does KEYBOARD_NOKEYS catch bluetooth keyboards?

[please use gerrit instead, 2015-01-15 01:09:13]

Aurimas, please see my reply inline.

https://codereview.chromium.org/715733002/diff/1490001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
(right):

https://codereview.chromium.org/715733002/diff/1490001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:298:
== Configuration.KEYBOARD_NOKEYS) {
On 2015/01/15 00:50:16, aurimas wrote:
> Does KEYBOARD_NOKEYS catch bluetooth keyboards?

I've tested with a wired USB keyboard only. (I hooked up the USB keyboard to my
tablet.) I assume that bluetooth keyboards behave the same as USB keyboards. If
you have a bluetooth keyboard handy to test this patch, please be my guest :-)

[aurimas (slooooooooow), 2015-01-15 02:26:47]

On 2015/01/15 at 01:09:13, rouslan wrote:
> Aurimas, please see my reply inline.
> 
>
https://codereview.chromium.org/715733002/diff/1490001/content/public/android...
> File
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
(right):
> 
>
https://codereview.chromium.org/715733002/diff/1490001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:298:
== Configuration.KEYBOARD_NOKEYS) {
> On 2015/01/15 00:50:16, aurimas wrote:
> > Does KEYBOARD_NOKEYS catch bluetooth keyboards?
> 
> I've tested with a wired USB keyboard only. (I hooked up the USB keyboard to
my tablet.) I assume that bluetooth keyboards behave the same as USB keyboards.
If you have a bluetooth keyboard handy to test this patch, please be my guest
:-)

Verified it locally, it works fine with a bluetooth keyboard.

[Aaron Boodman, 2015-01-15 04:02:06]

lgtm

[yukawa, 2015-01-15 06:31:45]

lgtm (as the aura part is stripped out)

[Torne, 2015-01-15 10:20:35]

android_webview LGTM

[newt (away), 2015-01-15 17:36:45]

https://codereview.chromium.org/715733002/diff/1490001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java
(right):

https://codereview.chromium.org/715733002/diff/1490001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:60:
new AutofillTestHelper().setProfile(new AutofillProfile("",
"https://www.example.com",
These objects all live on the UI thread. You should access them only on the UI
thread or risk flakiness down the road. Like this:

    ThreadUtils.runOnUiThreadBlocking(new Runnable() {
        @Override
        public void run() {
            ...
        }
    });

(This is an unending source of flakiness in our existing tests, which follow
this rule only haphazardly.)

https://codereview.chromium.org/715733002/diff/1490001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:80:
public boolean isSatisfied() {
This should run on the UI thread, too. Also, lines 70, 85-88, 93

https://codereview.chromium.org/715733002/diff/1490001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:89:
assertTrue("Autofill Popup anchor view was never added.",
this message doesn't seem quite right

https://codereview.chromium.org/715733002/diff/1490001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:108:
viewCore.onSizeChanged(viewCore.getViewportWidthPix(),
UI thread, etc. You get the point :)

[please use gerrit instead, 2015-01-20 19:34:28]

Patchset #9 (id:1530001) has been deleted

[please use gerrit instead, 2015-01-20 19:34:41]

Patchset #8 (id:1510001) has been deleted

[please use gerrit instead, 2015-01-20 19:38:03]

Newton, PTAL Patch Set 8.

https://codereview.chromium.org/715733002/diff/1490001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java
(right):

https://codereview.chromium.org/715733002/diff/1490001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:60:
new AutofillTestHelper().setProfile(new AutofillProfile("",
"https://www.example.com",
On 2015/01/15 17:36:45, newt wrote:
> These objects all live on the UI thread. You should access them only on the UI
> thread or risk flakiness down the road. Like this:
> 
>     ThreadUtils.runOnUiThreadBlocking(new Runnable() {
>         @Override
>         public void run() {
>             ...
>         }
>     });
> 
> (This is an unending source of flakiness in our existing tests, which follow
> this rule only haphazardly.)

AutofillTestHelper.setProfile() handles threading correctly already. It posts a
task on UI thread and then waits for it on the originating thread.

https://codereview.chromium.org/715733002/diff/1490001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:80:
public boolean isSatisfied() {
On 2015/01/15 17:36:45, newt wrote:
> This should run on the UI thread, too. Also, lines 70, 85-88, 93

Done.

https://codereview.chromium.org/715733002/diff/1490001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:89:
assertTrue("Autofill Popup anchor view was never added.",
On 2015/01/15 17:36:45, newt wrote:
> this message doesn't seem quite right

Done.

https://codereview.chromium.org/715733002/diff/1490001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:108:
viewCore.onSizeChanged(viewCore.getViewportWidthPix(),
On 2015/01/15 17:36:45, newt wrote:
> UI thread, etc. You get the point :)

Done.

[newt (away), 2015-01-20 21:28:10]

AutofillPopupWithKeyboardTest.java looks better. Just some suggestions for
making it shorter and easier to read.

https://codereview.chromium.org/715733002/diff/1550001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java
(right):

https://codereview.chromium.org/715733002/diff/1550001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:65:
class Info {
Another option would be to use AtomicReferences instead of declaring this Info
class, e.g.

    final AtomicReference<ContentViewCore> viewCoreRef = new
AtomicReference<ContentViewCore>();
    ...
    ThreadUtils.runOnUiThreadBlocking(new Runnable() {
        @Override
        public void run() {
            viewCoreRef.set(getActivity().getActiveContentViewCore());
            ...
        }
    });

Either way is fine. Sadly, neither is especially concise.

https://codereview.chromium.org/715733002/diff/1550001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:163:
private void waitForKeyboardShownOnUiThreadBlocking() throws
InterruptedException {
you could add a "visible" parameter to this method and use it for both the
keyboard shown and keyboard hidden cases. e.g.

    private void waitForKeyboardVisibility(boolean isVisible) {
        ...
    }

https://codereview.chromium.org/715733002/diff/1550001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:164:
assertTrue("Keyboard was never shown", CriteriaHelper.pollForCriteria(new
Criteria() {
use pollForUIThreadCriteria() here, then you can simplify
isKeyboardShowingOnUiThreadBlocking() and get rid of the try/catch here

https://codereview.chromium.org/715733002/diff/1550001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:179:
private void notifyViewHeightReducedOnUiThreadBlocking() {
I'd remove "OnUiThreadBlocking" from these methods since they should *not* be
called on the UI thread (which is what that naming scheme suggests to me).

https://codereview.chromium.org/715733002/diff/1550001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:192:
private Boolean isKeyboardShowingOnUiThreadBlocking()
This should return "boolean" (not Boolean). Same below.

https://codereview.chromium.org/715733002/diff/1550001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:206:
return ThreadUtils.runOnUiThreadBlocking(new Callable<View>() {
I'd use runOnUiThreadBlockingNoException(). Then you don't have to add a
"throws" clause or worry about catching exceptions when you call this method.
Same below.

[please use gerrit instead, 2015-01-21 01:27:46]

Patchset #9 (id:1570001) has been deleted

[please use gerrit instead, 2015-01-21 01:31:45]

Newton, PTAL Patch Set 9.

https://codereview.chromium.org/715733002/diff/1550001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java
(right):

https://codereview.chromium.org/715733002/diff/1550001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:65:
class Info {
On 2015/01/20 21:28:09, newt wrote:
> Another option would be to use AtomicReferences instead of declaring this Info
> class, e.g.
> 
>     final AtomicReference<ContentViewCore> viewCoreRef = new
> AtomicReference<ContentViewCore>();
>     ...
>     ThreadUtils.runOnUiThreadBlocking(new Runnable() {
>         @Override
>         public void run() {
>             viewCoreRef.set(getActivity().getActiveContentViewCore());
>             ...
>         }
>     });
> 
> Either way is fine. Sadly, neither is especially concise.

Done, that saves 2 lines :-).

https://codereview.chromium.org/715733002/diff/1550001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:163:
private void waitForKeyboardShownOnUiThreadBlocking() throws
InterruptedException {
On 2015/01/20 21:28:10, newt wrote:
> you could add a "visible" parameter to this method and use it for both the
> keyboard shown and keyboard hidden cases. e.g.
> 
>     private void waitForKeyboardVisibility(boolean isVisible) {
>         ...
>     }

Done.

https://codereview.chromium.org/715733002/diff/1550001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:164:
assertTrue("Keyboard was never shown", CriteriaHelper.pollForCriteria(new
Criteria() {
On 2015/01/20 21:28:09, newt wrote:
> use pollForUIThreadCriteria() here, then you can simplify
> isKeyboardShowingOnUiThreadBlocking() and get rid of the try/catch here

Done.

https://codereview.chromium.org/715733002/diff/1550001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:179:
private void notifyViewHeightReducedOnUiThreadBlocking() {
On 2015/01/20 21:28:09, newt wrote:
> I'd remove "OnUiThreadBlocking" from these methods since they should *not* be
> called on the UI thread (which is what that naming scheme suggests to me).

Done.

https://codereview.chromium.org/715733002/diff/1550001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:192:
private Boolean isKeyboardShowingOnUiThreadBlocking()
On 2015/01/20 21:28:09, newt wrote:
> This should return "boolean" (not Boolean). Same below.

No longer relevant, as this method got inlined.

https://codereview.chromium.org/715733002/diff/1550001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:206:
return ThreadUtils.runOnUiThreadBlocking(new Callable<View>() {
On 2015/01/20 21:28:09, newt wrote:
> I'd use runOnUiThreadBlockingNoException(). Then you don't have to add a
> "throws" clause or worry about catching exceptions when you call this method.
> Same below.

No longer necessary, as this got inlined.

[newt (away), 2015-01-21 01:41:27]

AutofillPopupWithKeyboardTest.java lgtm

https://codereview.chromium.org/715733002/diff/1590001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java
(right):

https://codereview.chromium.org/715733002/diff/1590001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:122:
// Wait until the keyboard is showing and notify the ContentViewCore that its
height was changed
Nit: use javadoc style comment /** */

[please use gerrit instead, 2015-01-21 01:45:36]

The CQ bit was checked by rouslan@chromium.org

[please use gerrit instead, 2015-01-21 01:46:06]

https://codereview.chromium.org/715733002/diff/1590001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java
(right):

https://codereview.chromium.org/715733002/diff/1590001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:122:
// Wait until the keyboard is showing and notify the ContentViewCore that its
height was changed
On 2015/01/21 01:41:27, newt wrote:
> Nit: use javadoc style comment /** */

Done.

[commit-bot: I haz the power, 2015-01-21 01:48:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/715733002/1610001

[commit-bot: I haz the power, 2015-01-21 14:18:57]

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

[commit-bot: I haz the power, 2015-01-21 14:19:02]

Failed to apply patch for
components/autofill/content/renderer/autofill_agent.cc:
While running git apply --index -3 -p1;
  error: patch failed:
components/autofill/content/renderer/autofill_agent.cc:228
  Falling back to three-way merge...
  Applied patch to 'components/autofill/content/renderer/autofill_agent.cc' with
conflicts.
  U components/autofill/content/renderer/autofill_agent.cc

Patch:       components/autofill/content/renderer/autofill_agent.cc
Index: components/autofill/content/renderer/autofill_agent.cc
diff --git a/components/autofill/content/renderer/autofill_agent.cc
b/components/autofill/content/renderer/autofill_agent.cc
index
6afbd4ca8dd79d0d60c860e2d41e584424cf114d..7b27b8fff54b9cb813bde16cfe22f1f5b8f80387
100644
--- a/components/autofill/content/renderer/autofill_agent.cc
+++ b/components/autofill/content/renderer/autofill_agent.cc
@@ -49,6 +49,7 @@
 
 using blink::WebAutofillClient;
 using blink::WebConsoleMessage;
+using blink::WebDocument;
 using blink::WebElement;
 using blink::WebElementCollection;
 using blink::WebFormControlElement;
@@ -214,12 +215,6 @@ void AutofillAgent::FocusedNodeChanged(const WebNode& node)
{
   if (node.document().frame() != render_frame()->GetWebFrame())
     return;
 
-  if (password_generation_agent_ &&
-      password_generation_agent_->FocusedNodeHasChanged(node)) {
-    is_popup_possibly_visible_ = true;
-    return;
-  }
-
   WebElement web_element = node.toConst<WebElement>();
 
   if (!web_element.document().frame())
@@ -228,16 +223,28 @@ void AutofillAgent::FocusedNodeChanged(const WebNode&
node) {
   const WebInputElement* element = toWebInputElement(&web_element);
 
   if (!element || !element->isEnabled() || element->isReadOnly() ||
-      !element->isTextField() || element->isPasswordField())
+      !element->isTextField())
     return;
 
   element_ = *element;
 }
 
-void AutofillAgent::Resized() {
+void AutofillAgent::FocusedElementMovedOnResize() {
   HidePopup();
 }
 
+void AutofillAgent::FocusChangeComplete() {
+  WebDocument doc = render_frame()->GetWebFrame()->document();
+  WebElement focused_element;
+  if (!doc.isNull())
+    focused_element = doc.focusedElement();
+
+  if (!focused_element.isNull() && password_generation_agent_ &&
+      password_generation_agent_->FocusedNodeHasChanged(focused_element)) {
+    is_popup_possibly_visible_ = true;
+  }
+}
+
 void AutofillAgent::didRequestAutocomplete(
     const WebFormElement& form) {
   DCHECK_EQ(form.document().frame(), render_frame()->GetWebFrame());
@@ -316,12 +323,6 @@ void AutofillAgent::FormControlElementClicked(
           switches::kEnableSingleClickAutofill)) {
     // Show full suggestions when clicking on an already-focused form field. On
     // the initial click (not focused yet), only show password suggestions.
-#if defined(OS_ANDROID)
-    // TODO(gcasto): Remove after crbug.com/430318 has been fixed.
-    if (!was_focused)
-      return;
-#endif
-
     options.show_full_suggestion_list =
         options.show_full_suggestion_list || was_focused;
     options.show_password_suggestions_only = !was_focused;
@@ -785,8 +786,12 @@ void
AutofillAgent::LegacyAutofillAgent::FocusedNodeChanged(
   agent_->FocusedNodeChanged(node);
 }
 
-void AutofillAgent::LegacyAutofillAgent::Resized() {
-  agent_->Resized();
+void AutofillAgent::LegacyAutofillAgent::FocusedElementMovedOnResize() {
+  agent_->FocusedElementMovedOnResize();
+}
+
+void AutofillAgent::LegacyAutofillAgent::FocusChangeComplete() {
+  agent_->FocusChangeComplete();
 }
 
 }  // namespace autofill

[commit-bot: I haz the power, 2015-01-21 14:19:45]

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

[please use gerrit instead, 2015-01-22 00:53:51]

The CQ bit was checked by rouslan@chromium.org

[commit-bot: I haz the power, 2015-01-22 00:54:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/715733002/1630001

[commit-bot: I haz the power, 2015-01-22 01:54:45]

Committed patchset #11 (id:1630001)

[commit-bot: I haz the power, 2015-01-22 01:56:19]

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