New issue
Advanced search Search tips

Issue 712754 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

Remove PageClickTracker and PageClickListener

Project Member Reported by ekaramad@chromium.org, Apr 18 2017

Issue description

PageClickTracker and PageClickListener should be removed and the logic for tracking page focus and clicks should live in AutofillAgent (WebAutofillClient).
 
We should start by moving the logic to WebAutofillClient and then remove the classes after the page_click_tracker_browsertest.cc is converted into some other test file.
Project Member

Comment 2 by bugdroid1@chromium.org, Apr 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/27ca69b1e049f9f86e15f34420eeef224335f075

commit 27ca69b1e049f9f86e15f34420eeef224335f075
Author: ekaramad <ekaramad@chromium.org>
Date: Thu Apr 20 18:34:29 2017

[refactor] Fix autofill features for payments when the form is inside an OOPIF

Currently, clicking into a credit card information <input> field inside an
OOPIF does not pop up the autofill window. The root cause is simply because
the call to RenderWidget::FocusChangeComplete() is dropped for OOPIFs.
Moreover, for the main frame, this call is forwarded throught
RenderViewObserver calls to the corresponding PageClickTracker.

On the other hand, both PageClickTracker and AutofillAgent use a internal
Legacy class to observe RenderView for updates in element focus and mouse down
down inside a node. This is unfortunate given that the actual classes are
RenderFrameObservers.

This CL will make the following changes:

A) FocusChangeComplete:
This call is now forwarded to the observers of the RenderWidget which are
RenderFrames. They will then notify their own observers, e.g., AutofillAgent
and PageClickTracker. In line with this, the Legacy classes inside these
classes will be removed. Also, there is no longer a need for
RenderViewImpl::RenderWidgetFocusChangeComplete.

B) MouseDown:
Currently, in response to a left mouse button down or a gesture tap, ChromeClient
notifies the WebViewClient (RenderViewImpl). This is solely used in
PageClickTracker. This CL will remove the call from WebViewClient to the
WebWidgetClient (RenderWidget) which similarly to FocusChangeComplete, will be
forwarded to the observer RenderFrames and eventually the RenderFrameObservers.

The change in A) will also fix a bug with credit card payment autofill in
OOPIFs.

BUG=712754, 703800 , 583347,  433486 

Review-Url: https://codereview.chromium.org/2766053002
Cr-Commit-Position: refs/heads/master@{#466078}

[modify] https://crrev.com/27ca69b1e049f9f86e15f34420eeef224335f075/chrome/browser/autofill/autofill_interactive_uitest.cc
[modify] https://crrev.com/27ca69b1e049f9f86e15f34420eeef224335f075/chrome/renderer/autofill/page_click_tracker_browsertest.cc
[modify] https://crrev.com/27ca69b1e049f9f86e15f34420eeef224335f075/components/autofill/content/renderer/autofill_agent.cc
[modify] https://crrev.com/27ca69b1e049f9f86e15f34420eeef224335f075/components/autofill/content/renderer/autofill_agent.h
[modify] https://crrev.com/27ca69b1e049f9f86e15f34420eeef224335f075/components/autofill/content/renderer/page_click_tracker.cc
[modify] https://crrev.com/27ca69b1e049f9f86e15f34420eeef224335f075/components/autofill/content/renderer/page_click_tracker.h
[modify] https://crrev.com/27ca69b1e049f9f86e15f34420eeef224335f075/content/public/renderer/render_view_observer.h
[modify] https://crrev.com/27ca69b1e049f9f86e15f34420eeef224335f075/content/renderer/render_view_impl.cc
[modify] https://crrev.com/27ca69b1e049f9f86e15f34420eeef224335f075/content/renderer/render_view_impl.h
[modify] https://crrev.com/27ca69b1e049f9f86e15f34420eeef224335f075/content/renderer/render_widget.cc
[modify] https://crrev.com/27ca69b1e049f9f86e15f34420eeef224335f075/content/renderer/render_widget_owner_delegate.h
[modify] https://crrev.com/27ca69b1e049f9f86e15f34420eeef224335f075/third_party/WebKit/Source/core/input/EventHandler.cpp
[modify] https://crrev.com/27ca69b1e049f9f86e15f34420eeef224335f075/third_party/WebKit/Source/core/input/GestureManager.cpp
[modify] https://crrev.com/27ca69b1e049f9f86e15f34420eeef224335f075/third_party/WebKit/Source/core/page/ChromeClient.h
[modify] https://crrev.com/27ca69b1e049f9f86e15f34420eeef224335f075/third_party/WebKit/Source/web/ChromeClientImpl.cpp
[modify] https://crrev.com/27ca69b1e049f9f86e15f34420eeef224335f075/third_party/WebKit/Source/web/ChromeClientImpl.h
[modify] https://crrev.com/27ca69b1e049f9f86e15f34420eeef224335f075/third_party/WebKit/public/web/WebAutofillClient.h
[modify] https://crrev.com/27ca69b1e049f9f86e15f34420eeef224335f075/third_party/WebKit/public/web/WebViewClient.h

Project Member

Comment 3 by bugdroid1@chromium.org, Apr 28 2017

Labels: merge-merged-3071
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ca19e7c02d4e92fbf88d3d52b25d1e5760e408eb

commit ca19e7c02d4e92fbf88d3d52b25d1e5760e408eb
Author: EhsanK <ekaramad@chromium.org>
Date: Fri Apr 28 21:46:12 2017

[refactor] Fix autofill features for payments when the form is inside an OOPIF

Currently, clicking into a credit card information <input> field inside an
OOPIF does not pop up the autofill window. The root cause is simply because
the call to RenderWidget::FocusChangeComplete() is dropped for OOPIFs.
Moreover, for the main frame, this call is forwarded throught
RenderViewObserver calls to the corresponding PageClickTracker.

On the other hand, both PageClickTracker and AutofillAgent use a internal
Legacy class to observe RenderView for updates in element focus and mouse down
down inside a node. This is unfortunate given that the actual classes are
RenderFrameObservers.

This CL will make the following changes:

A) FocusChangeComplete:
This call is now forwarded to the observers of the RenderWidget which are
RenderFrames. They will then notify their own observers, e.g., AutofillAgent
and PageClickTracker. In line with this, the Legacy classes inside these
classes will be removed. Also, there is no longer a need for
RenderViewImpl::RenderWidgetFocusChangeComplete.

B) MouseDown:
Currently, in response to a left mouse button down or a gesture tap, ChromeClient
notifies the WebViewClient (RenderViewImpl). This is solely used in
PageClickTracker. This CL will remove the call from WebViewClient to the
WebWidgetClient (RenderWidget) which similarly to FocusChangeComplete, will be
forwarded to the observer RenderFrames and eventually the RenderFrameObservers.

The change in A) will also fix a bug with credit card payment autofill in
OOPIFs.

BUG=712754, 703800 , 583347,  433486 

Review-Url: https://codereview.chromium.org/2766053002
Cr-Commit-Position: refs/heads/master@{#466078}
(cherry picked from commit 27ca69b1e049f9f86e15f34420eeef224335f075)

Review-Url: https://codereview.chromium.org/2853623002 .
Cr-Commit-Position: refs/branch-heads/3071@{#304}
Cr-Branched-From: a106f0abbf69dad349d4aaf4bcc4f5d376dd2377-refs/heads/master@{#464641}

[modify] https://crrev.com/ca19e7c02d4e92fbf88d3d52b25d1e5760e408eb/chrome/browser/autofill/autofill_interactive_uitest.cc
[modify] https://crrev.com/ca19e7c02d4e92fbf88d3d52b25d1e5760e408eb/chrome/renderer/autofill/page_click_tracker_browsertest.cc
[modify] https://crrev.com/ca19e7c02d4e92fbf88d3d52b25d1e5760e408eb/components/autofill/content/renderer/autofill_agent.cc
[modify] https://crrev.com/ca19e7c02d4e92fbf88d3d52b25d1e5760e408eb/components/autofill/content/renderer/autofill_agent.h
[modify] https://crrev.com/ca19e7c02d4e92fbf88d3d52b25d1e5760e408eb/components/autofill/content/renderer/page_click_tracker.cc
[modify] https://crrev.com/ca19e7c02d4e92fbf88d3d52b25d1e5760e408eb/components/autofill/content/renderer/page_click_tracker.h
[modify] https://crrev.com/ca19e7c02d4e92fbf88d3d52b25d1e5760e408eb/content/public/renderer/render_view_observer.h
[modify] https://crrev.com/ca19e7c02d4e92fbf88d3d52b25d1e5760e408eb/content/renderer/render_view_impl.cc
[modify] https://crrev.com/ca19e7c02d4e92fbf88d3d52b25d1e5760e408eb/content/renderer/render_view_impl.h
[modify] https://crrev.com/ca19e7c02d4e92fbf88d3d52b25d1e5760e408eb/content/renderer/render_widget.cc
[modify] https://crrev.com/ca19e7c02d4e92fbf88d3d52b25d1e5760e408eb/content/renderer/render_widget_owner_delegate.h
[modify] https://crrev.com/ca19e7c02d4e92fbf88d3d52b25d1e5760e408eb/third_party/WebKit/Source/core/input/EventHandler.cpp
[modify] https://crrev.com/ca19e7c02d4e92fbf88d3d52b25d1e5760e408eb/third_party/WebKit/Source/core/input/GestureManager.cpp
[modify] https://crrev.com/ca19e7c02d4e92fbf88d3d52b25d1e5760e408eb/third_party/WebKit/Source/core/page/ChromeClient.h
[modify] https://crrev.com/ca19e7c02d4e92fbf88d3d52b25d1e5760e408eb/third_party/WebKit/Source/web/ChromeClientImpl.cpp
[modify] https://crrev.com/ca19e7c02d4e92fbf88d3d52b25d1e5760e408eb/third_party/WebKit/Source/web/ChromeClientImpl.h
[modify] https://crrev.com/ca19e7c02d4e92fbf88d3d52b25d1e5760e408eb/third_party/WebKit/public/web/WebAutofillClient.h
[modify] https://crrev.com/ca19e7c02d4e92fbf88d3d52b25d1e5760e408eb/third_party/WebKit/public/web/WebViewClient.h

Project Member

Comment 4 by bugdroid1@chromium.org, Nov 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3538c8ab740281d197e6b3fe68f2ef317d444c66

commit 3538c8ab740281d197e6b3fe68f2ef317d444c66
Author: Ehsan Karamad <ekaramad@chromium.org>
Date: Mon Nov 20 16:31:40 2017

[refactor] Remove PageClickTracker and PageClickListener

This CL removes the mentioned classes from the autofill client implementation.
The PageClickTracker logic used to observe RenderView to get information about
user clicks and gesture taps inside form elements. Since AutofillAgent is now
a RenderFrameObserver, most of the notifications are directly received from
the RenderFrame of the owner WebLocalFrame, or from inside blink through
WebAutofillClient interface. Therefore, PageClickTracker code is no longer
necessary.

Furthermore, currently PageClickTracker is simply a state machine to determine
clicks inside a form control element where all its inputs come from
AutofillAgent and its only listener (PageClickListener) is the same
AutofillAgent.

To support the current tests for PageClickTracker, a few test only state
variables are added to AutofillAgent. Eventually, the tests should be diligently
modified to remove the need for such variables.

Bug: 712754
Change-Id: I44e16ba0b10909f84377bb5e7c78a5266786e898
Reviewed-on: https://chromium-review.googlesource.com/773450
Commit-Queue: Ehsan Karamad <ekaramad@chromium.org>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517842}
[modify] https://crrev.com/3538c8ab740281d197e6b3fe68f2ef317d444c66/chrome/renderer/autofill/form_autocomplete_browsertest.cc
[add] https://crrev.com/3538c8ab740281d197e6b3fe68f2ef317d444c66/chrome/renderer/autofill/form_control_click_detection_browsertest.cc
[delete] https://crrev.com/d53d8b9b8b3d58d57ce08e089e6fb4273bb325ab/chrome/renderer/autofill/page_click_tracker_browsertest.cc
[modify] https://crrev.com/3538c8ab740281d197e6b3fe68f2ef317d444c66/chrome/renderer/autofill/password_autofill_agent_browsertest.cc
[modify] https://crrev.com/3538c8ab740281d197e6b3fe68f2ef317d444c66/chrome/test/BUILD.gn
[modify] https://crrev.com/3538c8ab740281d197e6b3fe68f2ef317d444c66/components/autofill/content/renderer/BUILD.gn
[modify] https://crrev.com/3538c8ab740281d197e6b3fe68f2ef317d444c66/components/autofill/content/renderer/autofill_agent.cc
[modify] https://crrev.com/3538c8ab740281d197e6b3fe68f2ef317d444c66/components/autofill/content/renderer/autofill_agent.h
[delete] https://crrev.com/d53d8b9b8b3d58d57ce08e089e6fb4273bb325ab/components/autofill/content/renderer/page_click_listener.h
[delete] https://crrev.com/d53d8b9b8b3d58d57ce08e089e6fb4273bb325ab/components/autofill/content/renderer/page_click_tracker.cc
[delete] https://crrev.com/d53d8b9b8b3d58d57ce08e089e6fb4273bb325ab/components/autofill/content/renderer/page_click_tracker.h

Status: Assigned (was: Available)
Cc: -vabr@chromium.org
vabr going hobby only -> reducing involvement.
Please contact me directly in urgent matters.

Sign in to add a comment