Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2766053002:
[refactor] Fix autofill features for payments when the form is inside an OOPIF (Closed)
|
Created:
3 years, 9 months ago by EhsanK Modified:
3 years, 8 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, jam, creis+watch_chromium.org, rouslan+autofill_chromium.org, rogerm+autofillwatch_chromium.org, sebsg+autofillwatch_chromium.org, nasko+codewatch_chromium.org, browser-components-watch_chromium.org, mathp+autofillwatch_chromium.org, darin-cc_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org, kolos1, site-isolation-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[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}
Committed: https://chromium.googlesource.com/chromium/src/+/27ca69b1e049f9f86e15f34420eeef224335f075
Patch Set 1 : Shortest Fix - No refactoring #Patch Set 2 : [refactor] Fix Payments Autofill Client in OOPIFs #Patch Set 3 : Remove RenderWidgetOwnerDelegate::RenderWidgetFocusChangeComplete() #Patch Set 4 : Move a method from WebWidgetClient to WebFrameClient #
Total comments: 7
Patch Set 5 : Using Node& instead of Node* #
Total comments: 24
Patch Set 6 : Rebased #Patch Set 7 : Adrressing creis@'s comments #Patch Set 8 : Adding a test #Patch Set 9 : Rebased (post blink refactor) #Patch Set 10 : Rebased #Patch Set 11 : Move logic to WebAutofillClient #Patch Set 12 : Fixing tests #
Total comments: 15
Patch Set 13 : Addressing vabr@'s comments #
Total comments: 1
Messages
Total messages: 67 (42 generated)
Description was changed from ========== Notify RenderFrames as opposed to RenderWidgetOwnerDelegate when element focus completes Currently, the call to RenderWidget::FocusChangeComplete is forwarded to |owner_delegate_| which is nullptr for RenderWidgets belonging to OOPIFs. This regresses some autofill features such as payments. This CL will reroute this call through RenderFrame so that RenderViewObservers will be notified about element focus change in OOPIFs. BUG=703800 ========== to ========== [refactor] Fix Payments Autofill Client in OOPIFs 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=703800, 433486 ==========
Description was changed from ========== [refactor] Fix Payments Autofill Client in OOPIFs 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=703800, 433486 ========== to ========== [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=703800, 433486 ==========
Description was changed from ========== [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=703800, 433486 ========== to ========== [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=703800, 583347, 433486 ==========
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ekaramad@chromium.org changed reviewers: + creis@chromium.org, dcheng@chromium.org, vabr@chromium.org
creis@chromium.org: Please review changes in content/ vabr@chromium.org: Please review changes in components/ dcheng@chromium.org: Please review changes in dcheng/ Also, do we need to add a test with this CL given that the refactor actually fixes the issue with OOPIFs? In that case, Vaclav, could you please point me to the right direction? I think I will need some credit card info stored somewhere?
Since this is just calling through to RenderFrame, should we just be plumbing this through WebFrameClient or a Mojo interface?
On 2017/03/28 21:35:11, dcheng wrote: > Since this is just calling through to RenderFrame, should we just be plumbing > this through WebFrameClient or a Mojo interface? Between WebFrameClient and WebWidgetClient I agree WebFrameClient might make more sense. I am not quite clear on mojo interface suggestion. AutofillAgent I believe is the ultimate consumer of these events which implements a mojom but is implemented inside components. Are you suggesting moving that module to blink/modules?
> mailto:vabr@chromium.org: Please review changes in components/ components/* LGTM, thank you for removing the legacy code! Vaclav
On 2017/03/28 21:53:14, EhsanK wrote: > On 2017/03/28 21:35:11, dcheng wrote: > > Since this is just calling through to RenderFrame, should we just be plumbing > > this through WebFrameClient or a Mojo interface? > > Between WebFrameClient and WebWidgetClient I agree WebFrameClient might make > more sense. I am not quite clear on mojo interface suggestion. AutofillAgent I > believe is the ultimate consumer of these events which implements a mojom but is > implemented inside components. Are you suggesting moving that module to > blink/modules? I guess just using WebFrameClient might be good enough for now. I'm not entirely sure what the plan for autofill is: long-term I could see a Source/modules/autofill which can use //components/autofill, but that doesn't exist today (obviously). +vabr, do you know if there's any plans around this area, especially given the Onion Soup work?
> +vabr, do you know if there's any plans around this area, especially given the > Onion Soup work? We still do want to migrate autofill and passwords code from //content to Blink and to browser, as appropriate. The "to Blink" part would be part of Onion Soup. kolos@ has been looking into this last year. Our only issue is time and resources, but because this is blocking an increasing number of issues on our side, I expect us to find time soon. Cheers, Vaclav
Thank you all, PTAL.
https://codereview.chromium.org/2766053002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right): https://codereview.chromium.org/2766053002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/ChromeClientImpl.cpp:1089: if (!mouseDownNode) Can this actually be null? https://codereview.chromium.org/2766053002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/ChromeClientImpl.cpp:1103: webframe->autofillClient()->textFieldDidReceiveKeyDown( It seems like this is plumbed through WebAutofillClient -- maybe we can do that for the mouse down stuff too, since it's autofill specific?
Thanks Daniel. PTAL. https://codereview.chromium.org/2766053002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right): https://codereview.chromium.org/2766053002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/ChromeClientImpl.cpp:1089: if (!mouseDownNode) On 2017/04/03 19:17:04, dcheng wrote: > Can this actually be null? Thanks. Apparently it can't. Code search returns the two calls to be from EventHandler and GestureManager and in both cases we only call this method if HitTestResult.innerNode() is not nullptr. https://codereview.chromium.org/2766053002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/ChromeClientImpl.cpp:1103: webframe->autofillClient()->textFieldDidReceiveKeyDown( On 2017/04/03 19:17:04, dcheng wrote: > It seems like this is plumbed through WebAutofillClient -- maybe we can do that > for the mouse down stuff too, since it's autofill specific? This is a good suggestion. However, the call right now goes to PageClickTracker and then through the PageClickListener interface to the AutofillAgent which so happens to be the WebAutofillClient. That being said, similarly, I could also add a method to WebAutofillClient for FocusChangeComplete and then remove the PageClickTracker and PageClickListener completely. However, there is a bunch of tests involved and it takes some time. I am happy to work on it, either in this CL or a followup one if vabr@ is also happy with the change.
https://codereview.chromium.org/2766053002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right): https://codereview.chromium.org/2766053002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/ChromeClientImpl.cpp:1103: webframe->autofillClient()->textFieldDidReceiveKeyDown( On 2017/04/04 18:01:49, EhsanK wrote: > On 2017/04/03 19:17:04, dcheng wrote: > > It seems like this is plumbed through WebAutofillClient -- maybe we can do > that > > for the mouse down stuff too, since it's autofill specific? > > This is a good suggestion. However, the call right now goes to PageClickTracker > and then through the PageClickListener interface to the AutofillAgent which so > happens to be the WebAutofillClient. > > That being said, similarly, I could also add a method to WebAutofillClient for > FocusChangeComplete and then remove the PageClickTracker and PageClickListener > completely. However, there is a bunch of tests involved and it takes some time. > I am happy to work on it, either in this CL or a followup one if vabr@ is also > happy with the change. vabr@, does this make sense to you?
Sorry for the delay! I think we do need tests for this, since there appears to be some bugs in the patch. https://codereview.chromium.org/2766053002/diff/80001/content/public/renderer... File content/public/renderer/render_frame_observer.h (right): https://codereview.chromium.org/2766053002/diff/80001/content/public/renderer... content/public/renderer/render_frame_observer.h:122: // required animations (if any) have finished. nit: Fix double space before "have" https://codereview.chromium.org/2766053002/diff/80001/content/public/renderer... content/public/renderer/render_frame_observer.h:126: virtual void DidCompleteLeftMouseDownOrGestureTapInNode( This name is a bit awkward, but I'm ok with it if you need to be this specific. That said, it looks like it's not hooked up. :) https://codereview.chromium.org/2766053002/diff/80001/content/public/renderer... File content/public/renderer/render_view_observer.h (right): https://codereview.chromium.org/2766053002/diff/80001/content/public/renderer... content/public/renderer/render_view_observer.h:72: virtual void FocusChangeComplete() {} This looks like it might be unused after we remove it from the two autofill classes. Can we remove it, too? https://codereview.chromium.org/2766053002/diff/80001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2766053002/diff/80001/content/renderer/render... content/renderer/render_frame_impl.cc:1095: void RenderFrameImpl::didCompleteLeftMouseDownOrGestureTapInNode( These should move down with the WebFrameClient overrides as well. https://codereview.chromium.org/2766053002/diff/80001/content/renderer/render... content/renderer/render_frame_impl.cc:2452: if (frame_ && frame_ == render_view_->webview()->focusedFrame()) { Can these methods be called when frame_ is null? Many of the methods in this class just have a DCHECK(frame_). (Also see my question in RenderWidget about whether we can only call this for focused frames in the first place, possibly eliminating the need for this check.) https://codereview.chromium.org/2766053002/diff/80001/content/renderer/render... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/2766053002/diff/80001/content/renderer/render... content/renderer/render_frame_impl.h:253: const blink::WebNode& node) override; This should be in the WebFrameClient overrides section around line 693 (as should overrideFlashEmbedWithHTML). https://codereview.chromium.org/2766053002/diff/80001/content/renderer/render... content/renderer/render_frame_impl.h:271: // Called by render widget to notify about a change in focused element. nit: RenderWidget Might want to clarify that this is called on every frame, if we stick with that decision. https://codereview.chromium.org/2766053002/diff/80001/content/renderer/render... content/renderer/render_frame_impl.h:274: // Called by render widget to notify about handling a left mouse down or nit: RenderWidget https://codereview.chromium.org/2766053002/diff/80001/content/renderer/render... content/renderer/render_frame_impl.h:276: // at the local root and is never null. I'm not clear on this explanation. Are you saying it's called on the local root's RenderFrameImpl, and not on the frame |node| is in? That's a bit confusing (especially next to DidCompleteFocusChange, which is apparently called on every frame), so it's worth clarifying. https://codereview.chromium.org/2766053002/diff/80001/content/renderer/render... content/renderer/render_frame_impl.h:277: void DidCompleteLeftMouseDownOrGestureTapInNode(const blink::WebNode& node); This isn't implemented or called. Is it stale code, or would compile or tests have caught it? https://codereview.chromium.org/2766053002/diff/80001/content/renderer/render... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2766053002/diff/80001/content/renderer/render... content/renderer/render_widget.cc:976: render_frame.DidCompleteFocusChange(); How often is this called? I'm mildly concerned about having to notify every frame in a potentially large page every time the focus changes, which seems like it could be quite frequent. It looks like most of those frames just ignore the call anyway-- only the focused frame will react. Would it be sufficient to only notify the focused frame from here?
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Charlie. PTAL.
We are waiting to decide whether moving the logic to WebAutofillClient in this
CL makes more sense. So maybe the changes in RenderFrame{Impl,Observer} will be
undone.
That being said, I guess writing a test is independent of what we will do next
so meanwhile, I will get started on that.
https://codereview.chromium.org/2766053002/diff/80001/content/public/renderer...
File content/public/renderer/render_frame_observer.h (right):
https://codereview.chromium.org/2766053002/diff/80001/content/public/renderer...
content/public/renderer/render_frame_observer.h:122: // required animations (if
any) have finished.
On 2017/04/05 20:13:44, Charlie Reis wrote:
> nit: Fix double space before "have"
Done. Thanks!
https://codereview.chromium.org/2766053002/diff/80001/content/public/renderer...
content/public/renderer/render_frame_observer.h:126: virtual void
DidCompleteLeftMouseDownOrGestureTapInNode(
On 2017/04/05 20:13:44, Charlie Reis wrote:
> This name is a bit awkward, but I'm ok with it if you need to be this
specific.
> That said, it looks like it's not hooked up. :)
Yes I was not sure how it should be named. In reality the method was called when
there is a mouse down or tap. The name inside blink is onMouseDown but I figured
that might be a bit incorrect given that it is not necessarily on mouse down.
But I am open to any suggestion regarding the naming (that is if this method
survives the patch given that there is a possibility of moving the logic to
WebAutofillClient).
Also, this is implemented in PageClickTracker which is one of the
RenderFrameObservers in this patch; and should get called from
RenderFrameImpl::didCompleteLeftMouseDownOrGestureTapInNode().
https://codereview.chromium.org/2766053002/diff/80001/content/public/renderer...
File content/public/renderer/render_view_observer.h (right):
https://codereview.chromium.org/2766053002/diff/80001/content/public/renderer...
content/public/renderer/render_view_observer.h:72: virtual void
FocusChangeComplete() {}
On 2017/04/05 20:13:44, Charlie Reis wrote:
> This looks like it might be unused after we remove it from the two autofill
> classes. Can we remove it, too?
Yes thanks. I forgot to remove it.
https://codereview.chromium.org/2766053002/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):
https://codereview.chromium.org/2766053002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:1095: void
RenderFrameImpl::didCompleteLeftMouseDownOrGestureTapInNode(
On 2017/04/05 20:13:44, Charlie Reis wrote:
> These should move down with the WebFrameClient overrides as well.
Done. Both for this method and the one above. Thanks!
https://codereview.chromium.org/2766053002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:2452: if (frame_ && frame_ ==
render_view_->webview()->focusedFrame()) {
On 2017/04/05 20:13:44, Charlie Reis wrote:
> Can these methods be called when frame_ is null? Many of the methods in this
> class just have a DCHECK(frame_).
You are right. WasHidden() only calls GetWebFrame() which contains a
DCHECK(frame_). This method is similar to that (i.e., called from RenderWidget
as observer call).
> (Also see my question in RenderWidget about whether we can only call this for
> focused frames in the first place, possibly eliminating the need for this
> check.)
Yes that would eliminate the if block, but I guess coding-wise, that changes how
things have been layered so far, in that we would have to refer to RenderView
and RenderFrame from inside RenderWidget.
https://codereview.chromium.org/2766053002/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.h (right):
https://codereview.chromium.org/2766053002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.h:253: const blink::WebNode& node) override;
On 2017/04/05 20:13:45, Charlie Reis wrote:
> This should be in the WebFrameClient overrides section around line 693 (as
> should overrideFlashEmbedWithHTML).
Thanks. I moved both. Also deleted the comment above overrideFlasEmbedWithHTML
as exactly the first sentence in the comment for
WebFrameClient::overrideFlashEmbedWithHTML.
https://codereview.chromium.org/2766053002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.h:271: // Called by render widget to notify
about a change in focused element.
On 2017/04/05 20:13:45, Charlie Reis wrote:
> nit: RenderWidget
>
> Might want to clarify that this is called on every frame, if we stick with
that
> decision.
Done. Thanks!
https://codereview.chromium.org/2766053002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.h:274: // Called by render widget to notify
about handling a left mouse down or
On 2017/04/05 20:13:45, Charlie Reis wrote:
> nit: RenderWidget
Done.
https://codereview.chromium.org/2766053002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.h:276: // at the local root and is never
null.
On 2017/04/05 20:13:45, Charlie Reis wrote:
> I'm not clear on this explanation. Are you saying it's called on the local
> root's RenderFrameImpl, and not on the frame |node| is in? That's a bit
> confusing (especially next to DidCompleteFocusChange, which is apparently
called
> on every frame), so it's worth clarifying.
Sorry about this code and comment. I forgot to remove this from previous patch.
The logic is now in WebFrameClient (RenderFrameImpl) and does not go to
WebWidgetClient and then to all RenderFrameImpls. Now we directly call
WebFrameClient::didCompleteLeftMouseDownOrGestureTapInNode().
https://codereview.chromium.org/2766053002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.h:277: void
DidCompleteLeftMouseDownOrGestureTapInNode(const blink::WebNode& node);
On 2017/04/05 20:13:45, Charlie Reis wrote:
> This isn't implemented or called. Is it stale code, or would compile or tests
> have caught it?
Stale code from previous patch. Apologies. Thanks!
https://codereview.chromium.org/2766053002/diff/80001/content/renderer/render...
File content/renderer/render_widget.cc (right):
https://codereview.chromium.org/2766053002/diff/80001/content/renderer/render...
content/renderer/render_widget.cc:976: render_frame.DidCompleteFocusChange();
On 2017/04/05 20:13:45, Charlie Reis wrote:
> How often is this called? I'm mildly concerned about having to notify every
> frame in a potentially large page every time the focus changes, which seems
like
> it could be quite frequent.
I think quite often and basically whenever we focus an element.
>
> It looks like most of those frames just ignore the call anyway-- only the
> focused frame will react. Would it be sufficient to only notify the focused
> frame from here?
I would appreciate adding a legal way to directly communicate with
RenderFrame(s) or at least the RenderView from here. But unfortunately this does
not seem to be the way things are intended in RenderWidget as there is no notion
of RenderView, RenderFrame, or focused frame anywhere in this file.
Adding knowledge of frame tree or some frames to RenderWidget solves some
problems such as the pointer to focused_pepper_plugin_ we keep here or handling
page focus in OOPIFs (https://crbug.com/643727 and https://crbug.com/689777).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
creis@chromium.org changed reviewers: + kenrb@chromium.org
[+kenrb] Thanks. One question for Daniel and Ken about tracking focus in RenderWidget. Let me know what you decide about moving the logic to WebAutofillClient, too (once we hear back from vabr@). https://codereview.chromium.org/2766053002/diff/80001/content/public/renderer... File content/public/renderer/render_frame_observer.h (right): https://codereview.chromium.org/2766053002/diff/80001/content/public/renderer... content/public/renderer/render_frame_observer.h:126: virtual void DidCompleteLeftMouseDownOrGestureTapInNode( On 2017/04/06 21:31:27, EhsanK wrote: > On 2017/04/05 20:13:44, Charlie Reis wrote: > > This name is a bit awkward, but I'm ok with it if you need to be this > specific. > > That said, it looks like it's not hooked up. :) > > Yes I was not sure how it should be named. In reality the method was called when > there is a mouse down or tap. The name inside blink is onMouseDown but I figured > that might be a bit incorrect given that it is not necessarily on mouse down. > But I am open to any suggestion regarding the naming (that is if this method > survives the patch given that there is a possibility of moving the logic to > WebAutofillClient). > > Also, this is implemented in PageClickTracker which is one of the > RenderFrameObservers in this patch; and should get called from > RenderFrameImpl::didCompleteLeftMouseDownOrGestureTapInNode(). Ah, my mistake! I was reading it wrong. https://codereview.chromium.org/2766053002/diff/80001/content/renderer/render... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2766053002/diff/80001/content/renderer/render... content/renderer/render_widget.cc:976: render_frame.DidCompleteFocusChange(); On 2017/04/06 21:31:28, EhsanK wrote: > On 2017/04/05 20:13:45, Charlie Reis wrote: > > How often is this called? I'm mildly concerned about having to notify every > > frame in a potentially large page every time the focus changes, which seems > like > > it could be quite frequent. > I think quite often and basically whenever we focus an element. > > > > It looks like most of those frames just ignore the call anyway-- only the > > focused frame will react. Would it be sufficient to only notify the focused > > frame from here? > I would appreciate adding a legal way to directly communicate with > RenderFrame(s) or at least the RenderView from here. But unfortunately this does > not seem to be the way things are intended in RenderWidget as there is no notion > of RenderView, RenderFrame, or focused frame anywhere in this file. > > Adding knowledge of frame tree or some frames to RenderWidget solves some > problems such as the pointer to focused_pepper_plugin_ we keep here or handling > page focus in OOPIFs (https://crbug.com/643727 and https://crbug.com/689777). Daniel and Ken, what are your thoughts on this? RenderWidget already has a list of render_frames_ that it uses for notifying observers. Would it make sense to give it a notion of the focused frame so that it can handle cases like this without iterating over all frames?
Hi, and sorry I missed the question for me last week; my answer is below. (I did not follow all the comment threads once I approved the CL. It's always better to flag such questions in the beginning of the message, like creis@ did just now.) Still LGTM, and thanks for your work on this! Vaclav https://codereview.chromium.org/2766053002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right): https://codereview.chromium.org/2766053002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/ChromeClientImpl.cpp:1103: webframe->autofillClient()->textFieldDidReceiveKeyDown( On 2017/04/05 16:59:33, dcheng wrote: > On 2017/04/04 18:01:49, EhsanK wrote: > > On 2017/04/03 19:17:04, dcheng wrote: > > > It seems like this is plumbed through WebAutofillClient -- maybe we can do > > that > > > for the mouse down stuff too, since it's autofill specific? > > > > This is a good suggestion. However, the call right now goes to > PageClickTracker > > and then through the PageClickListener interface to the AutofillAgent which so > > happens to be the WebAutofillClient. > > > > That being said, similarly, I could also add a method to WebAutofillClient for > > FocusChangeComplete and then remove the PageClickTracker and PageClickListener > > completely. However, there is a bunch of tests involved and it takes some > time. > > I am happy to work on it, either in this CL or a followup one if vabr@ is also > > happy with the change. > > vabr@, does this make sense to you? Indeed, this makes sense to me, based on this signal being autofill-specific.
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Patchset #11 (id:200001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thank you all! PTAL. vabr@: Could you please review the latest patch? Note: I moved the signals needed to WebAutofillClient but removing PageClickTracker (and PageClickListener) proved a bit hard due to 'page_click_tracker_browsertest.cc'. I guess if you agree I could work on removing the classes and moving the tests into a separate (or new) file. I suggest doing it in another CL unless you find substantial issues with the current patch. https://codereview.chromium.org/2766053002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right): https://codereview.chromium.org/2766053002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/ChromeClientImpl.cpp:1103: webframe->autofillClient()->textFieldDidReceiveKeyDown( On 2017/04/10 06:48:27, vabr (Chromium) wrote: > On 2017/04/05 16:59:33, dcheng wrote: > > On 2017/04/04 18:01:49, EhsanK wrote: > > > On 2017/04/03 19:17:04, dcheng wrote: > > > > It seems like this is plumbed through WebAutofillClient -- maybe we can do > > > that > > > > for the mouse down stuff too, since it's autofill specific? > > > > > > This is a good suggestion. However, the call right now goes to > > PageClickTracker > > > and then through the PageClickListener interface to the AutofillAgent which > so > > > happens to be the WebAutofillClient. > > > > > > That being said, similarly, I could also add a method to WebAutofillClient > for > > > FocusChangeComplete and then remove the PageClickTracker and > PageClickListener > > > completely. However, there is a bunch of tests involved and it takes some > > time. > > > I am happy to work on it, either in this CL or a followup one if vabr@ is > also > > > happy with the change. > > > > vabr@, does this make sense to you? > > Indeed, this makes sense to me, based on this signal being autofill-specific. So I will go ahead and work on the refactor within in this CL. Thanks!
Thanks for the changes! The current CL LGTM, with some comments below. You plan to separate the PageClickTracker removal into different CLs SGTM. One step at a time is a good strategy. Thanks for doing this work! Vaclav https://codereview.chromium.org/2766053002/diff/240001/chrome/renderer/autofi... File chrome/renderer/autofill/page_click_tracker_browsertest.cc (right): https://codereview.chromium.org/2766053002/diff/240001/chrome/renderer/autofi... chrome/renderer/autofill/page_click_tracker_browsertest.cc:54: page_click_tracker_ = While your code is correct, it takes some mental effort to understand the ownership transfer (page_clicl_tracker_ starts owning the object but becomes just weak). Please consider making the code clearer like this: auto page_click_tracker = base::MakeUnique<PageClickTracker>(...); page_click_tracker_weak_ = page_click_tracker.get(); autofill_agent_->set_page_click_tracker_for_testing(std::move(page_click_tracker)); https://codereview.chromium.org/2766053002/diff/240001/chrome/renderer/autofi... chrome/renderer/autofill/page_click_tracker_browsertest.cc:91: PageClickTracker* page_click_tracker_; nit: Please rename to page_click_tracker_weak_ to clarify that this is not an owning pointer. https://codereview.chromium.org/2766053002/diff/240001/components/autofill/co... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/2766053002/diff/240001/components/autofill/co... components/autofill/content/renderer/autofill_agent.cc:767: page_click_tracker_->DidCompleteFocusChangeInFrame(); Please add a comment saying that page_click_tracker_ needs to be notified after is_generation_popup_possibly_visible_ is updated (see also https://codereview.chromium.org/1184913002#msg6). https://codereview.chromium.org/2766053002/diff/240001/components/autofill/co... File components/autofill/content/renderer/autofill_agent.h (right): https://codereview.chromium.org/2766053002/diff/240001/components/autofill/co... components/autofill/content/renderer/autofill_agent.h:88: PageClickTracker* page_click_tracker) { Please change the declaration to require unique_ptr as argument. (See my comments above.) https://codereview.chromium.org/2766053002/diff/240001/components/autofill/co... components/autofill/content/renderer/autofill_agent.h:279: // This object must be constructed after AutofillAgent so that password This comment, explained in https://codereview.chromium.org/1184913002#msg6, is now obsolete. It meant to say: it is important that FocusChangeComplete() is first called on AutofillAgent, and then on PageClickTracker. You now fixed the code to ensure this dependency by AA::DidCompleteFocusChangeInFrame calling PCT::DidCompleteFocusChangeInFrame at its own end. So you can drop the comment here and add the one I proposed inside DidCompleteFocusChangeInFrame.
Curious if you have any thoughts about my comment. Maybe it can't be helped for this particular case though... https://codereview.chromium.org/2766053002/diff/240001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2766053002/diff/240001/content/renderer/rende... content/renderer/render_view_impl.cc:1317: autofill_client->DidCompleteFocusChangeInFrame(); It seems a bit awkward that we're in content, but we need to call back down into Blink, to call back up into the autofill code. I don't really have a better suggestion at the moment, but this doesn't seem quite right either.
I'm not sure who you were asking, dcheng@, but here is my response just in case :). https://codereview.chromium.org/2766053002/diff/240001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2766053002/diff/240001/content/renderer/rende... content/renderer/render_view_impl.cc:1317: autofill_client->DidCompleteFocusChangeInFrame(); On 2017/04/18 04:41:50, dcheng (OOO through May 2) wrote: > It seems a bit awkward that we're in content, but we need to call back down into > Blink, to call back up into the autofill code. I don't really have a better > suggestion at the moment, but this doesn't seem quite right either. I don't immediately see a better option now, and I have one reason against worrying about this :) -- we have two big refactorings planned for autofill content layer: one being part of Blink Onion Soup, which will move a lot of stuff inside Blink, and pushing the rest to the browser process. This will leave autofill's content layer likely empty, and the call here, if it survives, will end up either really calling Blink, or talking to browser via Mojo.
Thanks Vaclav! I will investigate the tests next to see how they can be converted. Filed a tracking bug for it as well. PTAL. https://codereview.chromium.org/2766053002/diff/240001/chrome/renderer/autofi... File chrome/renderer/autofill/page_click_tracker_browsertest.cc (right): https://codereview.chromium.org/2766053002/diff/240001/chrome/renderer/autofi... chrome/renderer/autofill/page_click_tracker_browsertest.cc:54: page_click_tracker_ = On 2017/04/14 06:44:24, vabr (Chromium) wrote: > While your code is correct, it takes some mental effort to understand the > ownership transfer (page_clicl_tracker_ starts owning the object but becomes > just weak). Please consider making the code clearer like this: > > auto page_click_tracker = base::MakeUnique<PageClickTracker>(...); > page_click_tracker_weak_ = page_click_tracker.get(); > autofill_agent_->set_page_click_tracker_for_testing(std::move(page_click_tracker)); Thanks! Sorry for the vagueness in the code. But I noticed that we do not even need a local reference to PageClickTracker in the test class since it is never used. I removed the reference to it in this patch. https://codereview.chromium.org/2766053002/diff/240001/chrome/renderer/autofi... chrome/renderer/autofill/page_click_tracker_browsertest.cc:91: PageClickTracker* page_click_tracker_; On 2017/04/14 06:44:24, vabr (Chromium) wrote: > nit: Please rename to page_click_tracker_weak_ to clarify that this is not an > owning pointer. Thanks! Removed as explained in comment above. https://codereview.chromium.org/2766053002/diff/240001/components/autofill/co... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/2766053002/diff/240001/components/autofill/co... components/autofill/content/renderer/autofill_agent.cc:767: page_click_tracker_->DidCompleteFocusChangeInFrame(); On 2017/04/14 06:44:24, vabr (Chromium) wrote: > Please add a comment saying that page_click_tracker_ needs to be notified after > is_generation_popup_possibly_visible_ is updated (see also > https://codereview.chromium.org/1184913002#msg6). Acknowledged. https://codereview.chromium.org/2766053002/diff/240001/components/autofill/co... File components/autofill/content/renderer/autofill_agent.h (right): https://codereview.chromium.org/2766053002/diff/240001/components/autofill/co... components/autofill/content/renderer/autofill_agent.h:88: PageClickTracker* page_click_tracker) { On 2017/04/14 06:44:24, vabr (Chromium) wrote: > Please change the declaration to require unique_ptr as argument. (See my > comments above.) Thanks! Noted. Done. https://codereview.chromium.org/2766053002/diff/240001/components/autofill/co... components/autofill/content/renderer/autofill_agent.h:279: // This object must be constructed after AutofillAgent so that password On 2017/04/14 06:44:24, vabr (Chromium) wrote: > This comment, explained in https://codereview.chromium.org/1184913002#msg6, is > now obsolete. It meant to say: it is important that FocusChangeComplete() is > first called on AutofillAgent, and then on PageClickTracker. > > You now fixed the code to ensure this dependency by > AA::DidCompleteFocusChangeInFrame calling PCT::DidCompleteFocusChangeInFrame at > its own end. > > So you can drop the comment here and add the one I proposed inside > DidCompleteFocusChangeInFrame. Done. Thanks for clarification! https://codereview.chromium.org/2766053002/diff/240001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2766053002/diff/240001/content/renderer/rende... content/renderer/render_view_impl.cc:1317: autofill_client->DidCompleteFocusChangeInFrame(); On 2017/04/18 07:17:02, vabr (Chromium) wrote: > On 2017/04/18 04:41:50, dcheng (OOO through May 2) wrote: > > It seems a bit awkward that we're in content, but we need to call back down > into > > Blink, to call back up into the autofill code. I don't really have a better > > suggestion at the moment, but this doesn't seem quite right either. > > I don't immediately see a better option now, and I have one reason against > worrying about this :) -- we have two big refactorings planned for autofill > content layer: one being part of Blink Onion Soup, which will move a lot of > stuff inside Blink, and pushing the rest to the browser process. This will leave > autofill's content layer likely empty, and the call here, if it survives, will > end up either really calling Blink, or talking to browser via Mojo. I am not quite sure. There is no apparent content/ interface which represents the AutofillAgent so either the blink object or a Render(Frame|View)Observer call seemed to be the way. I think as vabr@ suggested a forthcoming refactoring should solve this problem;perhaps by moving the logic for DidCompleteFocusChangeInFrame() from component/ to blink/. On a separate note, this method (OnScrollFocusedEditable...) needs some fixing to support OOPIFs as well (currently a blocker for TDI). I believe this is never called on an OOPIF right now. I don't know what the design for that would look like, but I guess it might be possible to directly call the autofill agent about this completion as well.
On 2017/04/18 07:17:02, vabr (Chromium) wrote: > I'm not sure who you were asking, dcheng@, but here is my response just in case > :). It was an open-ended question, so all responses welcome =) > > https://codereview.chromium.org/2766053002/diff/240001/content/renderer/rende... > File content/renderer/render_view_impl.cc (right): > > https://codereview.chromium.org/2766053002/diff/240001/content/renderer/rende... > content/renderer/render_view_impl.cc:1317: > autofill_client->DidCompleteFocusChangeInFrame(); > On 2017/04/18 04:41:50, dcheng (OOO through May 2) wrote: > > It seems a bit awkward that we're in content, but we need to call back down > into > > Blink, to call back up into the autofill code. I don't really have a better > > suggestion at the moment, but this doesn't seem quite right either. > > I don't immediately see a better option now, and I have one reason against > worrying about this :) -- we have two big refactorings planned for autofill > content layer: one being part of Blink Onion Soup, which will move a lot of > stuff inside Blink, and pushing the rest to the browser process. This will leave > autofill's content layer likely empty, and the call here, if it survives, will > end up either really calling Blink, or talking to browser via Mojo. I'm not so convinced that onion soup will solve this in the near term; it looks like some of this plumbing is tied to compositor callbacks, and I think onion souping that will be non-trivial Nonetheless, I don't see an immediate solution (and I think it's a slight improvement that it's clear when things are exposed for autofill only) so LGTM
Thanks for the replies, they LGTM. However, it seems like the newest patch set was not uploaded? Cheers, Vaclav
content/ LGTM assuming it's basically the same in the latest (missing) patchset. I appreciate that the RenderFrameObserver changes are gone, and hopefully the upcoming refactor will make some of this a bit easier. Thanks! https://codereview.chromium.org/2766053002/diff/240001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2766053002/diff/240001/content/renderer/rende... content/renderer/render_widget.cc:992: if (focused && focused->AutofillClient()) Yeah, it feels weird here too to have RenderWidget treat autofill as a special case in FocusChangeComplete. But given vabr's comment about an upcoming refactor, I won't block on it.
Description was changed from ========== [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=703800, 583347, 433486 ========== to ========== [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 ==========
Thank you all! Sorry I missed the actual upload part for the last patch. I will try to CQ today. https://codereview.chromium.org/2766053002/diff/240001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2766053002/diff/240001/content/renderer/rende... content/renderer/render_widget.cc:992: if (focused && focused->AutofillClient()) On 2017/04/19 20:39:29, Charlie Reis wrote: > Yeah, it feels weird here too to have RenderWidget treat autofill as a special > case in FocusChangeComplete. But given vabr's comment about an upcoming > refactor, I won't block on it. Yes it is only used for autofill client. I think it is possible to remove this method if that is more favored but we should note that it is being called from RenderWidgetInputHandler: https://cs.chromium.org/chromium/src/content/renderer/input/render_widget_inp... I am not sure about the coding style here but from the list of reference in render_widget_input_handler.cc I assumed it is not OK to add any 'public/web' includes since there is none there. https://codereview.chromium.org/2766053002/diff/260001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2766053002/diff/260001/content/renderer/rende... content/renderer/render_widget.cc:1816: FocusChangeComplete(); This is yet another case where we come from blink to content to call autofill. But this is also nontrivial given the platform dependent code.
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by ekaramad@chromium.org
The CQ bit was checked by ekaramad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org, creis@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2766053002/#ps260001 (title: "Addressing vabr@'s comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1492701814864200,
"parent_rev": "f4ecb9e37e9e2d59e32b8b96f23ac4a1e33b9552", "commit_rev":
"27ca69b1e049f9f86e15f34420eeef224335f075"}
Message was sent while issue was closed.
Description was changed from ========== [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 ========== to ========== [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} Committed: https://chromium.googlesource.com/chromium/src/+/27ca69b1e049f9f86e15f34420ee... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:260001) as https://chromium.googlesource.com/chromium/src/+/27ca69b1e049f9f86e15f34420ee... |
