[android] Autofill popup behavior fixes.

[android] Autofill popup behavior fixes.

- Don't hide autofill popup on browser height change on Android.

  Android keyboard can show or hide a strip of suggestions when user
  switches between input fields. If the input field triggers showing the
  autofill popup, the keyboard resize will hide it, which causes a UI
  flash.

- Remove redundant Views code to hide autofill popup on window resize.

  Browser process now has a cross-platform way to listen to window
  resize and close the autofill popup. Removed Views-only resize
  listener was also firing on window move. Removing it means that moving
  the window without resizing it will not close the autofill popup. The
  popup stays attached to the field during window move, however.

- Let autofill know when a password popup may be showing. This enables
  autofill agent to hide the popup when the page scrolls.

- Re-enable showing username suggestions on first click on Android,
  which now behaves on par with desktop.

BUG=430318
Committed: https://crrev.com/2f5993f9a83beb7c92dbaa31847643d9cd41bf13
Cr-Commit-Position: refs/heads/master@{#313633}

[please use gerrit instead, 2015-01-23 00:09:59]

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

[please use gerrit instead, 2015-01-23 00:10:00]

Garrett, PTAL.

[Evan Stade, 2015-01-23 02:16:13]

On 2015/01/23 00:10:00, Rouslan Solomakhin wrote:
> Garrett, PTAL.

nit: this never hides the popup on resize, correct? The description makes it
sound like it will still hide on some resizes.

Did we originally add hide-on-resize specifically due to the android keyboard?

[Evan Stade, 2015-01-23 02:16:34]

estade@chromium.org changed reviewers:
+ estade@chromium.org

[please use gerrit instead, 2015-01-23 21:55:58]

On 2015/01/23 02:16:13, Evan Stade wrote:
> On 2015/01/23 00:10:00, Rouslan Solomakhin wrote:
> > Garrett, PTAL.
> 
> nit: this never hides the popup on resize, correct? The description makes it
> sound like it will still hide on some resizes.
> 
> Did we originally add hide-on-resize specifically due to the android keyboard?


Chrome on desktop still hides the popup because of
AutofillPopupBaseView::OnWidgetBoundsChanged() on Views. Android does not hide
the popup on resize after this patch.

We originally added hide-on-resize for http://crbug.com/401752, which was about
popup being in the wrong place after orientation change. This happened because
AutofillAgent::TextFieldDidChangeImpl() was showing the popup after orientation
change. I digged deeper into the problem and found that AutofillAgent::Hide()
was being called from RenderFrameImpl::didChangeScrollOffset() after orientation
change, but it did not hide the popup, because "is_popup_possibly_visible_" was
false. That's because AutofillAgent::TextFieldDidChangeImpl() did not set
"is_popup_possibly_visible_" to true. The clean fix is to make sure that
"is_popup_possibly_visible_" is correctly set to true in
AutofillAgent::TextFieldDidChangeImpl(). That's what I've done in this patch.

Note, if orientation change did not scroll the webpage, the popup is not hidden.
It stays attached to the input field, however.

I've just tested this on Mac to find out that it needs a hide-on-resize
mechanism. Let me make hide-on-resize behavior Mac-only and upload a 2nd patch
set. Please stand by...

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

Garrett & Evan, PTAL Patch Set 2.

[Evan Stade, 2015-01-23 22:28:00]

https://codereview.chromium.org/870833002/diff/20001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/chrome_autofill_client.cc (right):

https://codereview.chromium.org/870833002/diff/20001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/chrome_autofill_client.cc:224: // Views hides
autofill popup in AutofillPopupBaseView::OnWidgetBoundsChanged.
maybe you can delete that code on views so we don't have two ways of doing the
same thing?

[please use gerrit instead, 2015-01-23 22:28:34]

On 2015/01/23 22:28:00, Evan Stade wrote:
>
https://codereview.chromium.org/870833002/diff/20001/chrome/browser/ui/autofi...
> File chrome/browser/ui/autofill/chrome_autofill_client.cc (right):
> 
>
https://codereview.chromium.org/870833002/diff/20001/chrome/browser/ui/autofi...
> chrome/browser/ui/autofill/chrome_autofill_client.cc:224: // Views hides
> autofill popup in AutofillPopupBaseView::OnWidgetBoundsChanged.
> maybe you can delete that code on views so we don't have two ways of doing the
> same thing?

On it...

[please use gerrit instead, 2015-01-26 18:47:23]

Patchset #3 (id:40001) has been deleted

[please use gerrit instead, 2015-01-26 18:47:30]

Patchset #3 (id:60001) has been deleted

[please use gerrit instead, 2015-01-26 18:51:14]

Evan & Garrett, PTAL Patch Set 3.

https://codereview.chromium.org/870833002/diff/20001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/chrome_autofill_client.cc (right):

https://codereview.chromium.org/870833002/diff/20001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/chrome_autofill_client.cc:224: // Views hides
autofill popup in AutofillPopupBaseView::OnWidgetBoundsChanged.
On 2015/01/23 22:28:00, Evan Stade wrote:
> maybe you can delete that code on views so we don't have two ways of doing the
> same thing?

Done.

[please use gerrit instead, 2015-01-26 19:41:43]

To clarify Patch Set 3: I've removed the code in Views that was closing autofill
popup when the window resizes. The MainFrameWasResized callback is
cross-platform, so it is preferred. Curiously, Views was calling WidgetResized
also when the window moved. Removing this callback now does not close the popup
when the window moves. This is OK, because the popup stays glued to the window
in the correct position---attached to the input field.

The Views implementation of autofill popup used to listen to WidgetResized event
by observing Widget events. The popup also registered accelerators with the
focus manager of the widget. Now that WidgetResized is no longer observer,
there's no need for the autofill popup to observe Widget events. Autofill popup
no longer takes Widget as a constructor argument. Instead, autofill popup takes
a focus manager in the constructor and uses it directly. This should make for
cleaner API, in my opinion. If you think that the change is too disruptive or
fear that the weak pointer to the focus manager may become stale, then I can
change the popup back to using a Widget and getting a fresh focus manager from
there every time.

[Evan Stade, 2015-01-26 20:21:53]

estade@chromium.org changed reviewers:
+ sky@chromium.org

[Evan Stade, 2015-01-26 20:21:54]

lgtm but I would appreciate if Scott could take a look at the FocusManager
stuff.

https://codereview.chromium.org/870833002/diff/80001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_popup_controller_interactive_uitest.cc
(right):

https://codereview.chromium.org/870833002/diff/80001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_popup_controller_interactive_uitest.cc:106:
gfx::Rect new_bounds = browser()->window()->GetBounds();
seems like you should leave this test as is (perhaps change expectations to
reflect new expected behavior) and add another test for the resize behavior

https://codereview.chromium.org/870833002/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/autofill/autofill_popup_base_view.cc (right):

https://codereview.chromium.org/870833002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/autofill_popup_base_view.cc:101:
focus_manager_->UnregisterAccelerators(this);
Seems like this could cause problems during shutdown in particular. Maybe sky@
knows if this is safe.

[sky, 2015-01-26 22:26:53]

https://codereview.chromium.org/870833002/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/autofill/autofill_popup_base_view.cc (right):

https://codereview.chromium.org/870833002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/autofill_popup_base_view.cc:101:
focus_manager_->UnregisterAccelerators(this);
On 2015/01/26 20:21:54, Evan Stade wrote:
> Seems like this could cause problems during shutdown in particular. Maybe sky@
> knows if this is safe.

Is focus_manager_ from another widget? If so, I agree, there may be timing
issues.

[please use gerrit instead, 2015-01-26 23:49:03]

Please see my replies inline.

https://codereview.chromium.org/870833002/diff/80001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_popup_controller_interactive_uitest.cc
(right):

https://codereview.chromium.org/870833002/diff/80001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_popup_controller_interactive_uitest.cc:106:
gfx::Rect new_bounds = browser()->window()->GetBounds();
On 2015/01/26 20:21:54, Evan Stade wrote:
> seems like you should leave this test as is (perhaps change expectations to
> reflect new expected behavior) and add another test for the resize behavior

Changed expectations and added another test. The two tests are now called
DoNotHidePopupOnWindowMove and HidePopupOnWindowResize.

https://codereview.chromium.org/870833002/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/autofill/autofill_popup_base_view.cc (right):

https://codereview.chromium.org/870833002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/autofill_popup_base_view.cc:101:
focus_manager_->UnregisterAccelerators(this);
On 2015/01/26 22:26:53, sky wrote:
> On 2015/01/26 20:21:54, Evan Stade wrote:
> > Seems like this could cause problems during shutdown in particular. Maybe
sky@
> > knows if this is safe.
> 
> Is focus_manager_ from another widget? If so, I agree, there may be timing
> issues.

An instance of FocusManager is created when initializing a Widget and deleted in
the destructor of the Widget.

1) You cannot show an autofill popup for an unitialized Widget.

2) AutofillPopupBaseView was using a Widget (observing_widget_) to get the
FocusManager before this patch, so both observing_widget_ and its FocusManager
survive past the lifetime of AutofillPopupBaseView.

(1) and (2) together show that AutofillPopupBaseView is created after
FocusManager creation and deleted before FocusManager deletion. Therefore, this
change should be safe.

[Garrett Casto, 2015-01-26 23:50:20]

lgtm

[Evan Stade, 2015-01-27 00:01:43]

https://codereview.chromium.org/870833002/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/autofill/autofill_popup_base_view.cc (right):

https://codereview.chromium.org/870833002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/autofill_popup_base_view.cc:101:
focus_manager_->UnregisterAccelerators(this);
On 2015/01/26 23:49:03, Rouslan Solomakhin wrote:
> On 2015/01/26 22:26:53, sky wrote:
> > On 2015/01/26 20:21:54, Evan Stade wrote:
> > > Seems like this could cause problems during shutdown in particular. Maybe
> sky@
> > > knows if this is safe.
> > 
> > Is focus_manager_ from another widget? If so, I agree, there may be timing
> > issues.
> 
> An instance of FocusManager is created when initializing a Widget and deleted
in
> the destructor of the Widget.
> 
> 1) You cannot show an autofill popup for an unitialized Widget.
> 
> 2) AutofillPopupBaseView was using a Widget (observing_widget_) to get the
> FocusManager before this patch, so both observing_widget_ and its FocusManager
> survive past the lifetime of AutofillPopupBaseView.
> 
> (1) and (2) together show that AutofillPopupBaseView is created after
> FocusManager creation and deleted before FocusManager deletion. Therefore,
this
> change should be safe.

do you even need to pass the focus manager? You can't do something like
GetWidget()->ParentWidget()->GetFocusManager()?

[please use gerrit instead, 2015-01-27 19:56:22]

Patchset #5 (id:120001) has been deleted

[please use gerrit instead, 2015-01-27 19:56:32]

Patchset #4 (id:100001) has been deleted

[please use gerrit instead, 2015-01-27 20:02:41]

Evan, please see my reply inline.

https://codereview.chromium.org/870833002/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/autofill/autofill_popup_base_view.cc (right):

https://codereview.chromium.org/870833002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/autofill_popup_base_view.cc:101:
focus_manager_->UnregisterAccelerators(this);
On 2015/01/27 00:01:43, Evan Stade wrote:
> On 2015/01/26 23:49:03, Rouslan Solomakhin wrote:
> > On 2015/01/26 22:26:53, sky wrote:
> > > On 2015/01/26 20:21:54, Evan Stade wrote:
> > > > Seems like this could cause problems during shutdown in particular.
Maybe
> > sky@
> > > > knows if this is safe.
> > > 
> > > Is focus_manager_ from another widget? If so, I agree, there may be timing
> > > issues.
> > 
> > An instance of FocusManager is created when initializing a Widget and
deleted
> in
> > the destructor of the Widget.
> > 
> > 1) You cannot show an autofill popup for an unitialized Widget.
> > 
> > 2) AutofillPopupBaseView was using a Widget (observing_widget_) to get the
> > FocusManager before this patch, so both observing_widget_ and its
FocusManager
> > survive past the lifetime of AutofillPopupBaseView.
> > 
> > (1) and (2) together show that AutofillPopupBaseView is created after
> > FocusManager creation and deleted before FocusManager deletion. Therefore,
> this
> > change should be safe.
> 
> do you even need to pass the focus manager? You can't do something like
> GetWidget()->ParentWidget()->GetFocusManager()?

I've tried using the focus manager of the widget created in DoShow(), but
requestAutocomplete dialog behavior broke: pressing Enter to select a profile
will attempt to submit the dialog instead; pressing Esc to dismiss a profile
suggestion will dismiss the dialog instead. Thus the focus manager needs to come
from a top-level widget.

[please use gerrit instead, 2015-01-27 20:28:37]

On 2015/01/27 20:02:41, Rouslan Solomakhin wrote:
> Evan, please see my reply inline.
> 
>
https://codereview.chromium.org/870833002/diff/80001/chrome/browser/ui/views/...
> File chrome/browser/ui/views/autofill/autofill_popup_base_view.cc (right):
> 
>
https://codereview.chromium.org/870833002/diff/80001/chrome/browser/ui/views/...
> chrome/browser/ui/views/autofill/autofill_popup_base_view.cc:101:
> focus_manager_->UnregisterAccelerators(this);
> On 2015/01/27 00:01:43, Evan Stade wrote:
> > On 2015/01/26 23:49:03, Rouslan Solomakhin wrote:
> > > On 2015/01/26 22:26:53, sky wrote:
> > > > On 2015/01/26 20:21:54, Evan Stade wrote:
> > > > > Seems like this could cause problems during shutdown in particular.
> Maybe
> > > sky@
> > > > > knows if this is safe.
> > > > 
> > > > Is focus_manager_ from another widget? If so, I agree, there may be
timing
> > > > issues.
> > > 
> > > An instance of FocusManager is created when initializing a Widget and
> deleted
> > in
> > > the destructor of the Widget.
> > > 
> > > 1) You cannot show an autofill popup for an unitialized Widget.
> > > 
> > > 2) AutofillPopupBaseView was using a Widget (observing_widget_) to get the
> > > FocusManager before this patch, so both observing_widget_ and its
> FocusManager
> > > survive past the lifetime of AutofillPopupBaseView.
> > > 
> > > (1) and (2) together show that AutofillPopupBaseView is created after
> > > FocusManager creation and deleted before FocusManager deletion. Therefore,
> > this
> > > change should be safe.
> > 
> > do you even need to pass the focus manager? You can't do something like
> > GetWidget()->ParentWidget()->GetFocusManager()?
> 
> I've tried using the focus manager of the widget created in DoShow(), but
> requestAutocomplete dialog behavior broke: pressing Enter to select a profile
> will attempt to submit the dialog instead; pressing Esc to dismiss a profile
> suggestion will dismiss the dialog instead. Thus the focus manager needs to
come
> from a top-level widget.

Using GetWidget()->GetTopLevelWidget()->GetFocusManager() also breaks
requestAutocomplete dialog. I guess the top-level widget for the autofill popup
is not the browser window.

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

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

[please use gerrit instead, 2015-01-28 20:54:35]

Nasko, OWNERS PTAL content/.

[sky, 2015-01-28 21:30:03]

LGTM

[please use gerrit instead, 2015-01-28 22:33:31]

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

[please use gerrit instead, 2015-01-28 22:35:03]

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

[please use gerrit instead, 2015-01-28 22:35:04]

David, OWNERS PTAL content/.

[davidben, 2015-01-28 22:47:36]

content/ lgtm

[Evan Stade, 2015-01-28 23:54:10]

lgtm

[please use gerrit instead, 2015-01-28 23:56:05]

The CQ bit was checked by rouslan@chromium.org

[commit-bot: I haz the power, 2015-01-28 23:57:09]

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

[commit-bot: I haz the power, 2015-01-29 00:18:59]

Committed patchset #4 (id:140001)

[commit-bot: I haz the power, 2015-01-29 00:20:08]

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