Page gets zoomed in when user taps URL bar after clearing text in an input field

When ContentView is not in focus, we don't have to call scrollFocusedEditableNodeIntoView.

Issue occurs due to speed switching of focus.

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

[AKVT, 2014-05-29 13:51:13]

PTAL this change.

[aurimas (slooooooooow), 2014-05-29 17:46:20]

On 2014/05/29 13:51:13, AJITH KUMAR V wrote:
> PTAL this change.

Please respond to the bug before we start reviewing this CL.

[AKVT, 2014-06-02 05:58:58]

On 2014/05/29 17:46:20, aurimas wrote:
> On 2014/05/29 13:51:13, AJITH KUMAR V wrote:
> > PTAL this change.
> 
> Please respond to the bug before we start reviewing this CL.

I have uploaded the video of reproduction in bug 378765. PTAL

[jdduke (slow), 2014-06-02 14:41:03]

On 2014/06/02 05:58:58, AJITH KUMAR V wrote:
> On 2014/05/29 17:46:20, aurimas wrote:
> > On 2014/05/29 13:51:13, AJITH KUMAR V wrote:
> > > PTAL this change.
> > 
> > Please respond to the bug before we start reviewing this CL.
> 
> I have uploaded the video of reproduction in bug 378765. PTAL

I'm sorry, but I still don't see the video attached to crbug.com/378765.

[AKVT, 2014-06-02 15:24:53]

On 2014/06/02 14:41:03, jdduke wrote:
> On 2014/06/02 05:58:58, AJITH KUMAR V wrote:
> > On 2014/05/29 17:46:20, aurimas wrote:
> > > On 2014/05/29 13:51:13, AJITH KUMAR V wrote:
> > > > PTAL this change.
> > > 
> > > Please respond to the bug before we start reviewing this CL.
> > 
> > I have uploaded the video of reproduction in bug 378765. PTAL
> 
> I'm sorry, but I still don't see the video attached to crbug.com/378765.

@jdduke, Due to size restriction, earlier change was not visible. Now I uploaded
the video with 4 chunks. PTAL in bug 378765.

[jdduke (slow), 2014-06-02 16:08:47]

The patch description is a bit confusing.  It refers to
"undoScrollFocusedEditableNodeIntoViewIfNeeded", but we no longer have that
method.  Is it possible your downstream version of ContentViewCore is different
than that in upstream Chromium, hence our inability to repro this error?

[AKVT, 2014-06-02 16:19:20]

On 2014/06/02 16:08:47, jdduke wrote:
> The patch description is a bit confusing.  It refers to
> "undoScrollFocusedEditableNodeIntoViewIfNeeded", but we no longer have that
> method.  Is it possible your downstream version of ContentViewCore is
different
> than that in upstream Chromium, hence our inability to repro this error?

Sorry for the confusion. I have updated the patch description. I am able to
reproduce the issue in ContentShell based on Chrome37 as well by following above
steps.

[jdduke (slow), 2014-06-02 16:42:42]

On 2014/06/02 16:19:20, AJITH KUMAR V wrote:
> On 2014/06/02 16:08:47, jdduke wrote:
> > The patch description is a bit confusing.  It refers to
> > "undoScrollFocusedEditableNodeIntoViewIfNeeded", but we no longer have that
> > method.  Is it possible your downstream version of ContentViewCore is
> different
> > than that in upstream Chromium, hence our inability to repro this error?
> 
> Sorry for the confusion. I have updated the patch description. I am able to
> reproduce the issue in ContentShell based on Chrome37 as well by following
above
> steps.

Thanks.  This change seems reasonable to me.  I wonder if we also want to
suppress the |scrollFocusedEditableNodeIntoView()| call that occurs in
|updateAfterSizeChanged()|, as that too can be delayed by which time the user
may have switched focus? If so, you might put a call to
cancelRequestToScrollFocusedEditableNodeIntoView() next to the
|hideImeIfNeeded()| call in onFocusChanged(false). Aurimas, thoughts?

[AKVT, 2014-06-02 16:52:37]

On 2014/06/02 16:42:42, jdduke wrote:
> On 2014/06/02 16:19:20, AJITH KUMAR V wrote:
> > On 2014/06/02 16:08:47, jdduke wrote:
> > > The patch description is a bit confusing.  It refers to
> > > "undoScrollFocusedEditableNodeIntoViewIfNeeded", but we no longer have
that
> > > method.  Is it possible your downstream version of ContentViewCore is
> > different
> > > than that in upstream Chromium, hence our inability to repro this error?
> > 
> > Sorry for the confusion. I have updated the patch description. I am able to
> > reproduce the issue in ContentShell based on Chrome37 as well by following
> above
> > steps.
> 
> Thanks.  This change seems reasonable to me.  I wonder if we also want to
> suppress the |scrollFocusedEditableNodeIntoView()| call that occurs in
> |updateAfterSizeChanged()|, as that too can be delayed by which time the user
> may have switched focus? If so, you might put a call to
> cancelRequestToScrollFocusedEditableNodeIntoView() next to the
> |hideImeIfNeeded()| call in onFocusChanged(false). Aurimas, thoughts?

@jdduke Thanks for the review. I shall update same changes to
updateAfterSizeChanged function as well to block
scrollFocusedEditableNodeIntoView() and call
cancelRequestToScrollFocusedEditableNodeIntoView() when the ContentView loses
focus. I will upload as next patch set.

[jdduke (slow), 2014-06-02 16:55:15]

On 2014/06/02 16:52:37, AJITH KUMAR V wrote:
> On 2014/06/02 16:42:42, jdduke wrote:
> > On 2014/06/02 16:19:20, AJITH KUMAR V wrote:
> > > On 2014/06/02 16:08:47, jdduke wrote:
> > > > The patch description is a bit confusing.  It refers to
> > > > "undoScrollFocusedEditableNodeIntoViewIfNeeded", but we no longer have
> that
> > > > method.  Is it possible your downstream version of ContentViewCore is
> > > different
> > > > than that in upstream Chromium, hence our inability to repro this error?
> > > 
> > > Sorry for the confusion. I have updated the patch description. I am able
to
> > > reproduce the issue in ContentShell based on Chrome37 as well by following
> > above
> > > steps.
> > 
> > Thanks.  This change seems reasonable to me.  I wonder if we also want to
> > suppress the |scrollFocusedEditableNodeIntoView()| call that occurs in
> > |updateAfterSizeChanged()|, as that too can be delayed by which time the
user
> > may have switched focus? If so, you might put a call to
> > cancelRequestToScrollFocusedEditableNodeIntoView() next to the
> > |hideImeIfNeeded()| call in onFocusChanged(false). Aurimas, thoughts?
> 
> @jdduke Thanks for the review. I shall update same changes to
> updateAfterSizeChanged function as well to block
> scrollFocusedEditableNodeIntoView() and call
> cancelRequestToScrollFocusedEditableNodeIntoView() when the ContentView loses
> focus. I will upload as next patch set.

The latter should be sufficient, you shouldn't need to do both.

[jdduke (slow), 2014-06-02 16:56:17]

On 2014/06/02 16:55:15, jdduke wrote:
> On 2014/06/02 16:52:37, AJITH KUMAR V wrote:
> > @jdduke Thanks for the review. I shall update same changes to
> > updateAfterSizeChanged function as well to block
> > scrollFocusedEditableNodeIntoView() and call
> > cancelRequestToScrollFocusedEditableNodeIntoView() when the ContentView
loses
> > focus. I will upload as next patch set.
> 
> The latter should be sufficient, you shouldn't need to do both.

Hmm, unless we don't get the focus change call for the omnibox, I'm not actually
sure what the sequence is there...

[AKVT, 2014-06-02 17:11:01]

@jdduke, Updated the changes. PTAL

[jdduke (slow), 2014-06-02 17:15:49]

lgtm but please wait for sign-off from either aurimas@ or tedchoc@.

[AKVT, 2014-06-02 17:38:25]

On 2014/06/02 17:15:49, jdduke wrote:
> lgtm but please wait for sign-off from either aurimas@ or tedchoc@.

Thanks jdduke. @aurimas & tedchoc PTAL this change.

[Ted C, 2014-06-02 21:25:46]

lgtm

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

https://codereview.chromium.org/304743007/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1576:
cancelRequestToScrollFocusedEditableNodeIntoView();
I agree with jdduke@ that this should be sufficient, but the above check won't
hurt.

[AKVT, 2014-06-03 05:25:28]

The CQ bit was checked by ajith.v@samsung.com

[commit-bot: I haz the power, 2014-06-03 05:26:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ajith.v@samsung.com/304743007/20001

[commit-bot: I haz the power, 2014-06-03 08:12:44]

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

[commit-bot: I haz the power, 2014-06-03 13:17:30]

Change committed as 274502