Make Autofill work again on iPads running iOS 9.

Make Autofill work again on iPads running iOS 9.

Add the Autofill view to the keyboard view instead of reusing UIWebView's inputAccessoryView. Also, don't show the navigation buttons since the iPad keyboard displays them in the new shortcut bar.

BUG=518906
Committed: https://crrev.com/e6aa06d091e602066d9da16d4dc4ae1f87860d4e
Cr-Commit-Position: refs/heads/master@{#344849}

[Justin Donnelly, 2015-08-19 20:09:31]

jdonnelly@chromium.org changed reviewers:
+ justincohen@chromium.org, rohitrao@chromium.org

[Justin Donnelly, 2015-08-19 20:09:31]

This is ready for a full review.

[Justin Donnelly, 2015-08-19 20:23:32]

justincohen, regarding the issue you showed me where the autofill view isn't
shown when you detach the keyboard: registering again for keyboardDidChangeFrame
notifications solves this. The view sometimes still doesn't show or is slightly
misaligned if you drag a split keyboard around but it always shows up again in
the correct position after you stop dragging. This seems reasonable to me and
keeps the implementation here pretty simple.

[justincohen, 2015-08-19 20:34:19]

https://codereview.chromium.org/1305433003/diff/1/ios/chrome/browser/autofill...
File ios/chrome/browser/autofill/form_input_accessory_view.mm (right):

https://codereview.chromium.org/1305433003/diff/1/ios/chrome/browser/autofill...
ios/chrome/browser/autofill/form_input_accessory_view.mm:124:
_customView.reset([customView retain]);
Why do we retain this at all

https://codereview.chromium.org/1305433003/diff/1/ios/chrome/browser/autofill...
File ios/chrome/browser/autofill/form_input_accessory_view_controller.mm
(right):

https://codereview.chromium.org/1305433003/diff/1/ios/chrome/browser/autofill...
ios/chrome/browser/autofill/form_input_accessory_view_controller.mm:287: [[self
getKeyboardView] addSubview:_customAccessoryView];
Is there a way we can DCHECK if something changes in UIKIt and -getKeyboardView
no longer works?

[Justin Donnelly, 2015-08-20 16:38:02]

Three things here:

- DCHECK on the keyboardView
- Don't show the autofill view until the first time suggestions are available,
as discussed yesterday.
- Unregister for keyboard notifications when the user switches tabs to prevent
the non-visible tabs from trying to show their suggestions.

Unfortunately, the last thing there requires a downstream change:
https://chromereviews.googleplex.com/243187013. One small upside at least is
that this prevents the old code path from having all the non-shown tabs
constantly updating their keyboard accessory even though it's not visible.

https://codereview.chromium.org/1305433003/diff/1/ios/chrome/browser/autofill...
File ios/chrome/browser/autofill/form_input_accessory_view.mm (right):

https://codereview.chromium.org/1305433003/diff/1/ios/chrome/browser/autofill...
ios/chrome/browser/autofill/form_input_accessory_view.mm:124:
_customView.reset([customView retain]);
On 2015/08/19 20:34:19, justincohen wrote:
> Why do we retain this at all

Don't know. My quick guess would be that something in here used to communicate
with the view but no longer does. How about I circle back around to this in
another CL. I'd prefer to touch as little as possible here.

https://codereview.chromium.org/1305433003/diff/1/ios/chrome/browser/autofill...
File ios/chrome/browser/autofill/form_input_accessory_view_controller.mm
(right):

https://codereview.chromium.org/1305433003/diff/1/ios/chrome/browser/autofill...
ios/chrome/browser/autofill/form_input_accessory_view_controller.mm:287: [[self
getKeyboardView] addSubview:_customAccessoryView];
On 2015/08/19 20:34:19, justincohen wrote:
> Is there a way we can DCHECK if something changes in UIKIt and
-getKeyboardView
> no longer works?

Done.

[justincohen, 2015-08-20 17:29:16]

LGTM with nits.

https://codereview.chromium.org/1305433003/diff/20001/ios/chrome/browser/auto...
File ios/chrome/browser/autofill/form_input_accessory_view.mm (right):

https://codereview.chromium.org/1305433003/diff/20001/ios/chrome/browser/auto...
ios/chrome/browser/autofill/form_input_accessory_view.mm:124:
_customView.reset([customView retain]);
Can we create a bug to clean this up in M47?

https://codereview.chromium.org/1305433003/diff/20001/ios/chrome/browser/auto...
File ios/chrome/browser/autofill/form_input_accessory_view_controller.mm
(right):

https://codereview.chromium.org/1305433003/diff/20001/ios/chrome/browser/auto...
ios/chrome/browser/autofill/form_input_accessory_view_controller.mm:276: -
(void)showCustomInputAccessoryView:(UIView*)view {
DCHECK(view)

https://codereview.chromium.org/1305433003/diff/20001/ios/chrome/browser/auto...
ios/chrome/browser/autofill/form_input_accessory_view_controller.mm:286: //
suggestions have been trigger yet, don't show the custom view.
triggered

https://codereview.chromium.org/1305433003/diff/20001/ios/chrome/browser/auto...
ios/chrome/browser/autofill/form_input_accessory_view_controller.mm:305:
DCHECK(keyboardView);
I'm curious what keyboardView is if you have a physical keyboard attached (you
can fake this in the simulator).

I'm guessing the view exists, it's just layed out just beneath the bottom of the
rootViewController's bounds.

[Justin Donnelly, 2015-08-20 20:24:28]

I don't know how I missed this until now but the autofill view is still being
shown after getting suggestions and then tapping into the omnibox. I'm trying to
figure out how I can some notification of this state and hide the autofill view.

Suggestions?

https://codereview.chromium.org/1305433003/diff/20001/ios/chrome/browser/auto...
File ios/chrome/browser/autofill/form_input_accessory_view.mm (right):

https://codereview.chromium.org/1305433003/diff/20001/ios/chrome/browser/auto...
ios/chrome/browser/autofill/form_input_accessory_view.mm:124:
_customView.reset([customView retain]);
On 2015/08/20 17:29:16, justincohen wrote:
> Can we create a bug to clean this up in M47?

Done: http://crbug.com/523046

https://codereview.chromium.org/1305433003/diff/20001/ios/chrome/browser/auto...
File ios/chrome/browser/autofill/form_input_accessory_view_controller.mm
(right):

https://codereview.chromium.org/1305433003/diff/20001/ios/chrome/browser/auto...
ios/chrome/browser/autofill/form_input_accessory_view_controller.mm:276: -
(void)showCustomInputAccessoryView:(UIView*)view {
On 2015/08/20 17:29:16, justincohen wrote:
> DCHECK(view)

Done.

https://codereview.chromium.org/1305433003/diff/20001/ios/chrome/browser/auto...
ios/chrome/browser/autofill/form_input_accessory_view_controller.mm:286: //
suggestions have been trigger yet, don't show the custom view.
On 2015/08/20 17:29:16, justincohen wrote:
> triggered

Done.

https://codereview.chromium.org/1305433003/diff/20001/ios/chrome/browser/auto...
ios/chrome/browser/autofill/form_input_accessory_view_controller.mm:305:
DCHECK(keyboardView);
On 2015/08/20 17:29:16, justincohen wrote:
> I'm curious what keyboardView is if you have a physical keyboard attached (you
> can fake this in the simulator).

I actually do most of my testing in the simulator with a hardware keyboard.

> I'm guessing the view exists, it's just layed out just beneath the bottom of
the
> rootViewController's bounds.

Right, except that the QuickType bar portion of the keyboard view is shown when
available even when the keyboard part of the keyboard view is out of bounds.

[rohitrao (ping after 24h), 2015-08-20 20:51:25]

On 2015/08/20 20:24:28, Justin Donnelly wrote:
> I don't know how I missed this until now but the autofill view is still being
> shown after getting suggestions and then tapping into the omnibox. I'm trying
to
> figure out how I can some notification of this state and hide the autofill
view.
> 
> Suggestions?

locationBarDidBecomeFirstResponder is plumbed all the way up through to
BrowserViewController.  Does that help?

[Justin Donnelly, 2015-08-21 14:52:38]

Last round, I hope. Based on justincohen's suggestion, use text input editing
notifications to get rid of the autofill view when any other text input tries to
show the keyboard.

Justin, last night I described this as "a pretty big hammer for a little nail"
meaning that a very general notification mechanism seemed overkill to cover just
the case where the user taps on the omnibox. But on further reflection, I think
this is exactly what we want. It means that any text input, current (omnibox,
settings, card unmask dialog) *or future* will be guaranteed not to have the
autofill view show up for it.

It also allows me to get rid of an earlier bit of code that checked to see if a
view was being presented. That's not necessary since if the view contains a text
input (both card unmask dialog or settings, maybe others?) the new notification
will get rid of the autofill view.

I'll keep testing today to make sure there are no more surprises, but so far
this seems to work well. Please take another look.

[justincohen, 2015-08-21 14:54:03]

still LGTM.

[Justin Donnelly, 2015-08-21 15:42:09]

On 2015/08/21 14:54:03, justincohen wrote:
> still LGTM.

Testing continues to go well.

Rohit, any feedback before I submit this?

[Justin Donnelly, 2015-08-21 17:03:43]

The CQ bit was checked by jdonnelly@chromium.org

[commit-bot: I haz the power, 2015-08-21 17:05:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1305433003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1305433003/60001

[commit-bot: I haz the power, 2015-08-21 17:05:03]

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

[commit-bot: I haz the power, 2015-08-21 17:05:04]

No L-G-T-M from a valid reviewer yet. Only full committers are accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[rohitrao (ping after 24h), 2015-08-21 17:18:25]

https://codereview.chromium.org/1305433003/diff/60001/ios/chrome/browser/auto...
File ios/chrome/browser/autofill/form_input_accessory_view_controller.mm
(right):

https://codereview.chromium.org/1305433003/diff/60001/ios/chrome/browser/auto...
ios/chrome/browser/autofill/form_input_accessory_view_controller.mm:98: //
Heuristics are used to compute this information. It returns false if the
Just to be clear, this was simply a comment fix, right?

[Justin Donnelly, 2015-08-21 17:21:50]

https://codereview.chromium.org/1305433003/diff/60001/ios/chrome/browser/auto...
File ios/chrome/browser/autofill/form_input_accessory_view_controller.mm
(right):

https://codereview.chromium.org/1305433003/diff/60001/ios/chrome/browser/auto...
ios/chrome/browser/autofill/form_input_accessory_view_controller.mm:98: //
Heuristics are used to compute this information. It returns false if the
On 2015/08/21 17:18:25, rohitrao wrote:
> Just to be clear, this was simply a comment fix, right?

Correct.

[rohitrao (ping after 24h), 2015-08-21 20:12:25]

lgtm

[Justin Donnelly, 2015-08-21 20:13:10]

The CQ bit was checked by jdonnelly@chromium.org

[Justin Donnelly, 2015-08-21 20:13:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
justincohen@chromium.org
Link to the patchset: https://codereview.chromium.org/1305433003/#ps80001
(title: " ")

[commit-bot: I haz the power, 2015-08-21 20:13:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1305433003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1305433003/80001

[commit-bot: I haz the power, 2015-08-21 20:20:55]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-08-21 20:22:04]

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