Rename FocusChangeComplete to ImeShownAnimationsComplete.

Rename FocusChangeComplete to ImeShownAnimationsComplete.

ImeShownAnimationsComplete is a better name because this method can be
called even when focus did not change. The method is always called when
it is safe to show the autofill popup, because the page has finished
scrolling and zooming the active input element into view after showing
the virtual keyboard (if any).

BUG=430318

[please use gerrit instead, 2015-01-29 01:08:00]

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

[please use gerrit instead, 2015-01-29 01:08:00]

Evan, PTAL.

[Evan Stade, 2015-01-29 01:10:10]

I'm not the right reviewer for this. content/ owner should take a look.

[please use gerrit instead, 2015-01-29 01:15:48]

rouslan@chromium.org changed reviewers:
+ sievers@chromium.org
- estade@chromium.org

[please use gerrit instead, 2015-01-29 01:15:49]

Daniel, PTAL.

[no sievers, 2015-01-29 22:01:56]

https://codereview.chromium.org/886663002/diff/1/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/886663002/diff/1/content/renderer/render_view...
content/renderer/render_view_impl.cc:1422: void
RenderViewImpl::OnScrollFocusedEditableNodeIntoRect(
Actually here it looks like (based on the generic naming of this method and the
ViewMsg and it's description) the previous name is better.

https://codereview.chromium.org/886663002/diff/1/content/renderer/render_view...
content/renderer/render_view_impl.cc:3715: ImeShownAnimationsComplete();
Also here: this is for example for double-tap zoom, right?
So why 'IMEAnimation'?

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

Hi Daniel,

Please see my comments inline.

I think the confusion here stems from InputMsg_ScrollFocusedEditableNodeIntoRect
message name. This message is sent after tapping an input field, even if it was
already focused before. Tapping an input field will bring up IME (virtual
keyboard), then zoom and scroll the tapped field into legible size and position
above the keyboard. The tapped input field is focused at this time, which is
where the InputMsg gets its name.

However, the animations can be shown even for an already focused field that was
tapped again. Focus would not be changing here: already focused element was
tapped. The tap will show the keyboard and will zoom and scroll the input into
legible size and position. Since the focus is not changing, it does not make
sense to name the method FocusChangeComplete.

Given these descriptions of the events, I think that ImeShownAnimationsComplete
is a better name than FocusChangeComplete. What do you think?

--Rouslan

https://codereview.chromium.org/886663002/diff/1/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/886663002/diff/1/content/renderer/render_view...
content/renderer/render_view_impl.cc:1422: void
RenderViewImpl::OnScrollFocusedEditableNodeIntoRect(
On 2015/01/29 22:01:56, sievers wrote:
> Actually here it looks like (based on the generic naming of this method and
the
> ViewMsg and it's description) the previous name is better.

OnScrollFocusedEditableNodeIntoRect is called in the browser after showing the
IME (virtual keyboard). The IME is shown in response to tapping an input field.
The purpose of this method is to zoom in and move the input field into legible
size above the newly shown keyboard.

https://codereview.chromium.org/886663002/diff/1/content/renderer/render_view...
content/renderer/render_view_impl.cc:3715: ImeShownAnimationsComplete();
On 2015/01/29 22:01:56, sievers wrote:
> Also here: this is for example for double-tap zoom, right?
> So why 'IMEAnimation'?

DidCompletePageScaleAnimation is not four double-tap zoom. It's for the zoom and
scroll that happens when you tap on an input field. This animation is initiated
in webview()->scrollFocusedNodeIntoRect() on line 1435. The animation happens in
the compositor. The compositor then calls DidCompletePageScaleAnimation here.

[no sievers, 2015-01-30 19:22:13]

On 2015/01/29 22:24:24, Rouslan Solomakhin wrote:
> Hi Daniel,
> 
> Please see my comments inline.
> 
> I think the confusion here stems from
InputMsg_ScrollFocusedEditableNodeIntoRect
> message name. This message is sent after tapping an input field, even if it
was
> already focused before. Tapping an input field will bring up IME (virtual
> keyboard), then zoom and scroll the tapped field into legible size and
position
> above the keyboard. The tapped input field is focused at this time, which is
> where the InputMsg gets its name.
> 
> However, the animations can be shown even for an already focused field that
was
> tapped again. Focus would not be changing here: already focused element was
> tapped. The tap will show the keyboard and will zoom and scroll the input into
> legible size and position. Since the focus is not changing, it does not make
> sense to name the method FocusChangeComplete.
> 
> Given these descriptions of the events, I think that
ImeShownAnimationsComplete
> is a better name than FocusChangeComplete. What do you think?
> 
> --Rouslan
> 
>
https://codereview.chromium.org/886663002/diff/1/content/renderer/render_view...
> File content/renderer/render_view_impl.cc (right):
> 
>
https://codereview.chromium.org/886663002/diff/1/content/renderer/render_view...
> content/renderer/render_view_impl.cc:1422: void
> RenderViewImpl::OnScrollFocusedEditableNodeIntoRect(
> On 2015/01/29 22:01:56, sievers wrote:
> > Actually here it looks like (based on the generic naming of this method and
> the
> > ViewMsg and it's description) the previous name is better.
> 
> OnScrollFocusedEditableNodeIntoRect is called in the browser after showing the
> IME (virtual keyboard). The IME is shown in response to tapping an input
field.
> The purpose of this method is to zoom in and move the input field into legible
> size above the newly shown keyboard.
> 
>
https://codereview.chromium.org/886663002/diff/1/content/renderer/render_view...
> content/renderer/render_view_impl.cc:3715: ImeShownAnimationsComplete();
> On 2015/01/29 22:01:56, sievers wrote:
> > Also here: this is for example for double-tap zoom, right?
> > So why 'IMEAnimation'?
> 
> DidCompletePageScaleAnimation is not four double-tap zoom. It's for the zoom
and
> scroll that happens when you tap on an input field. This animation is
initiated
> in webview()->scrollFocusedNodeIntoRect() on line 1435. The animation happens
in
> the compositor. The compositor then calls DidCompletePageScaleAnimation here.

See WebViewImpl::animateDoubleTapZoom() and other call sites for
startPageScaleAnimation().

Am not convinced yet that this is all IME-related :)

Also if that was the case, why rename one part only and leave the rest with the
more generic names (referring to 'focus')?
Shouldn't we rename all functions and IPC messages then if we want to be more
specific?

[please use gerrit instead, 2015-01-31 02:04:37]

How about PageScaleAnimationComplete?

The core of the issue is that this event may not be caused by a focus change, so
FocusChangeComplete can be confusing.

[no sievers, 2015-02-04 21:56:52]

On 2015/01/31 02:04:37, Rouslan Solomakhin wrote:
> How about PageScaleAnimationComplete?
> 
> The core of the issue is that this event may not be caused by a focus change,
so
> FocusChangeComplete can be confusing.

[no sievers, 2015-02-04 22:02:05]

On 2015/02/04 21:56:52, sievers wrote:
> On 2015/01/31 02:04:37, Rouslan Solomakhin wrote:
> > How about PageScaleAnimationComplete?
> > 
> > The core of the issue is that this event may not be caused by a focus
change,
> so
> > FocusChangeComplete can be confusing.

Sorry for the delay. Focus change still seems to be the common denominator among
all the affected code that I see.
In particular the two implementations for the observer callback that you are
basically talking about:

AutofillAgent::FocusChangeComplete() is indeed concerned with 'focus change': it
retrieves the focused element and calls
password_generation_agent_->FocusedNodeHasChanged()

PageClickTracker::FocusChangeComplete() calls FormControlElementClicked() and
also seems to be concerned with 'focus change completed' as it resets
was_focused_before_now_ after passing the previous value.

[please use gerrit instead, 2015-02-10 18:45:00]

Fair enough. I will close the issue for now and think about better naming and/or
implementation.

[no sievers, 2015-02-10 19:01:59]

On 2015/02/10 18:45:00, Rouslan Solomakhin wrote:
> Fair enough. I will close the issue for now and think about better naming
and/or
> implementation.

Yes, feel free to come up with a more comprehensive scheme. I agree that in the
current form it's all a bit vague in terms of what is really happening (or
supposed to happen). But it then might involve changing all the layers to
improve it.