[AF] Add and implement ObserveFieldVisibility in AutofillAgent [chromium/src : main]
Fernando Ramirez (Gerrit)
Fernando Ramirez added 1 comment![]( "Open in Gerrit")
Fernando RamirezJust updating that Fernando and I had an offline chat about the mojo structure, so I expect a new version of the CL to be uploaded when he has had a chance to update it.
Hi Nasko, could you PTAL at the updated implementation? Instead of using an unbounded callback I use a `PendingRemote`.
Related details
- Christoph Schwering
- Nasko Oskov
- Stefan Zager
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christoph Schwering (Gerrit)
Christoph Schwering added 1 comment![]( "Open in Gerrit")
// Starts observing the visibility of the field identified by `field_id`.
// The `observer` will be notified when the field becomes visible (e.g.,
// meets the 800ms visibility condition).
ObserveFieldVisibility(FieldRendererId field_id,
pending_remote<AutofillVisibilityObserver> observer);Could you document why we use `pending_remote`?
IIUC the differences of `pending_remote` compared to a response callback are that `pending_remote`
- supports multiple methods in the interface (obviously)
- supports any number of method calls (whereas a response callback must be called exactly once)
- supports cancellation by the browser process
- `pending_remote` needs more boilerplate code
We don't use the first and third though, and we actually do guarantee to call the method exactly once, don't we? Is the motivation for the `pending_remote` to defensively handle the case where (due to a bug) AutofillAgent does *not* call the callback? Or am I missing the real point?
Related details
- Fernando Ramirez
- Nasko Oskov
- Stefan Zager
Nasko Oskov (Gerrit)
Nasko Oskov added 2 comments![]( "Open in Gerrit")
The shape of the IPC surface looks good. Thanks for working through this. I am happy to proceed this way, but won't +1 until the rest of owners are happy and we are sure we are proceeding with this approach.
// Starts observing the visibility of the field identified by `field_id`.
// The `observer` will be notified when the field becomes visible (e.g.,
// meets the 800ms visibility condition).
ObserveFieldVisibility(FieldRendererId field_id,
pending_remote<AutofillVisibilityObserver> observer);Could you document why we use `pending_remote`?
IIUC the differences of `pending_remote` compared to a response callback are that `pending_remote`
- supports multiple methods in the interface (obviously)
- supports any number of method calls (whereas a response callback must be called exactly once)
- supports cancellation by the browser process
- `pending_remote` needs more boilerplate code
We don't use the first and third though, and we actually do guarantee to call the method exactly once, don't we? Is the motivation for the `pending_remote` to defensively handle the case where (due to a bug) AutofillAgent does *not* call the callback? Or am I missing the real point?
Yes, the goal here is to avoid having hanging callbacks across processes.
Related details
- Fernando Ramirez
- Stefan Zager
Fernando Ramirez (Gerrit)
Fernando Ramirez added 5 comments![]( "Open in Gerrit")
Fernando RamirezJust updating that Fernando and I had an offline chat about the mojo structure, so I expect a new version of the CL to be uploaded when he has had a chance to update it.
Hi Nasko, could you PTAL at the updated implementation? Instead of using an unbounded callback I use a `PendingRemote`.
Closing this comment based on https://chromium-review.git.corp.google.com/c/chromium/src/+/7870348/comments/a7495b8f_c87a14b2.
GetIntersectionObserverInfo(FieldRendererId field_id)Fernando RamirezFYI for the mojo reviewer: this message seems a bit unusual in that it has a response, but by design the response may be delayed arbitrarily long. It may not even fire until page unload. At a glance, this seems different from other examples of mojo messages with a response:
https://source.chromium.org/search?q=%22%3D%3E%22%20f:.*%5C.mojom$&sq=
Fernando Ramirez@na...@chromium.org, hopefully my explanation of why the response may be delayed is made clear in https://chromium-review.git.corp.google.com/c/chromium/src/+/7870348/comment/3570d57e_c3b205e4/
I have updated my implementation. This comment is no longer relevant.
// Starts observing the visibility of the field identified by `field_id`.
// The `observer` will be notified when the field becomes visible (e.g.,
// meets the 800ms visibility condition).
ObserveFieldVisibility(FieldRendererId field_id,
pending_remote<AutofillVisibilityObserver> observer);Nasko OskovCould you document why we use `pending_remote`?
IIUC the differences of `pending_remote` compared to a response callback are that `pending_remote`
- supports multiple methods in the interface (obviously)
- supports any number of method calls (whereas a response callback must be called exactly once)
- supports cancellation by the browser process
- `pending_remote` needs more boilerplate code
We don't use the first and third though, and we actually do guarantee to call the method exactly once, don't we? Is the motivation for the `pending_remote` to defensively handle the case where (due to a bug) AutofillAgent does *not* call the callback? Or am I missing the real point?
Yes, the goal here is to avoid having hanging callbacks across processes.
and we actually do guarantee to call the method exactly once, don't we?
Correct, this is called at most once (exactly once if the field becomes visible, and zero times if it does not). As opposed to the response callback, we don't have to call it with a fallback value, like `false`, if the user never scrolls down or closes the tab.
I added a code comment for why we use a `pending_remote` too.
// Note: We use a `pending_remote<AutofillVisibilityObserver>` instead of a
// standard Mojo response callback (e.g., `=> (bool is_visible)`) because the
// visibility condition depends on user interaction (scrolling) and may be
// delayed indefinitely. Since standard Mojo response callbacks are expected
// to return promptly, we avoid using an unbounded callback here in favor of a
// dedicated Mojo observer interface.Just double checking if this all looks good to you based on our offline discussion @na...@chromium.org
base::OnceCallback<void(bool)> callback) {Fernando RamirezLike in the previous version, this callback must be called. IIUC this CL doesn't guarantee it. Am I missing something?
Olivia SaulNo, you are correct the callback is never called when the credit card number field is never fully visible for at least 800ms. This can happen because:
- The user never scrolls down to the credit card number field
- The user scrolls past the element too quickly
- The user closes the tab
Could we use `ScopedClosureRunner` similar to the original CL? Could this ensure the callback is called with `false` in these cases?
Christoph SchweringDrive-by: How come this callback must be called? Is there a contract or something that has to be fulfilled because it's in AutofillAgent-land? In my design I just sort of figured it was okay if the user never scrolled down and the callback was never called, similar to how we use callbacks for UI bubbles ("here's a callback IF the user accepts the dialog").
Stefan ZagerHow come this callback must be called?
It's a Mojo requirement. I can't find it in the documentation right now, but the generated mojo code is includes this
```
class AutofillAgent_TriggerFormExtractionWithResponse_ProxyToResponder : public ::mojo::internal::ProxyToResponder {
public:
~AutofillAgent_TriggerFormExtractionWithResponse_ProxyToResponder() {
#if DCHECK_IS_ON()
if (responder_) {
responder_->IsConnectedAsync(base::BindOnce(&OnIsConnectedComplete));
}
#endif
}private:
#if DCHECK_IS_ON()
static void OnIsConnectedComplete(bool connected) {
DCHECK(!connected)
<< "AutofillAgent::TriggerFormExtractionWithResponseCallback was destroyed without "
<< "first either being run or its corresponding binding being closed. "
<< "It is an error to drop response callbacks which still correspond "
<< "to an open interface pipe.";
}
#endif
};
```Could we use ScopedClosureRunner similar to the original CL?
We can't use it directly. In the original CL, we used the ScopedClosureRunner to delete an object, which owned the response callback and whose destructor called that response callback (IIRC). That pattern would work here, too, I think.
Alternatively, we can use base::SplitOnceCallback to in AutofillAgent. That'd roughly look like this:
```
auto [cb1, cb2] = base::SplitCallback(std::move(callback));
element.GetDocument().MonitorFormFieldVisibility(element, std::move(cb1));
base::SequencedTaskRunner::GetCurrentDefault()->PostDelayedTask(
FROM_HERE, base::BindOnce(std::move(cb2), false), base::Seconds(2));
// Or a timer, but IIUC calling `cb2` would be safe:
// some_timer_.Start(FROM_HERE, base::Seconds(1),
// base::BindOnce(std::move(cb2), false));
```
Fernando RamirezI'm no expert, but I find it unusual to use a mojo message with a responder in this way, when the response may be delayed indefinitely. In other examples I have seen the response is typically sent promptly. Would it make sense to split this into two distinct mojo messages, `MonitorVisibiltiy` and `ReportVisible`?
Christoph SchweringI incorporated the use of `ScopedClosureRunner` for `MonitorVisibiltiy`. This should follow the same approach as the original CL (crrev.com/c/7814161). This ensures the callback is always called, is my understanding here correct?
Nasko OskovYes, I think this is correct.
Would it make sense to split this into two distinct mojo messages, `MonitorVisibiltiy` and `ReportVisible`?
I agree this feels a bit weird. I'm not aware of guidelines for this specific case. I think the general recommendation is to use response callbacks. Let's check with the Mojo reviewer.
Fernando RamirezIPC reviewers usually look at the security implications of the design, not as much on the functional side.
That said, most interfaces do indeed follow the pattern of having API respond as soon as possible while any future changes use observers.
I'm not versed enough in what this feature is trying to accomplish to be able to give more concrete examples of how to proceed. I would suggest looking at other interfaces for inspiration.
Fernando Ramirezwhile any future changes use observers.
So we should be able to proceed with our approach since we use an observer?
I'm not versed enough in what this feature is trying to accomplish to be able to give more concrete examples of how to proceed.
We want to be able show an "Autofill payment" chip on the omnibox as an alternative entry point for payments, but only if the credit card number field is actively visible to the user.
Unlike existing methods like `CheckViewAreaVisible`, which perform immediate checks, `GetIntersectionObserverInfo` is a monitoring request. It uses an `IntersectionObserver` to ensure the field has been visible for at least 800ms.
Because this mojo callback response depends on a user scrolling and may never be met, we use `ScopedClosureRunner` to guarantee that the mojo callback is eventually executed with `false` upon destruction, satisfying the requirement that response callbacks must be invoked.
> I would suggest looking at other interfaces for inspiration.I couldn't find examples that use response callbacks in `autofill_agent.mojom`. I checked in both `autofill_agent.cc` and `password_autofill_agent.cc`.
Updated my implementation. This comment thread is no longer relevant.
Related details
- Christoph Schwering
- Nasko Oskov
- Stefan Zager
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christoph Schwering (Gerrit)
Christoph Schwering added 3 comments![]( "Open in Gerrit")
// Starts observing the visibility of the field identified by `field_id`.
// The `observer` will be notified when the field becomes visible (e.g.,
// meets the 800ms visibility condition).
ObserveFieldVisibility(FieldRendererId field_id,
pending_remote<AutofillVisibilityObserver> observer);Nasko OskovCould you document why we use `pending_remote`?
IIUC the differences of `pending_remote` compared to a response callback are that `pending_remote`
- supports multiple methods in the interface (obviously)
- supports any number of method calls (whereas a response callback must be called exactly once)
- supports cancellation by the browser process
- `pending_remote` needs more boilerplate code
We don't use the first and third though, and we actually do guarantee to call the method exactly once, don't we? Is the motivation for the `pending_remote` to defensively handle the case where (due to a bug) AutofillAgent does *not* call the callback? Or am I missing the real point?
Fernando RamirezYes, the goal here is to avoid having hanging callbacks across processes.
and we actually do guarantee to call the method exactly once, don't we?
Correct, this is called at most once (exactly once if the field becomes visible, and zero times if it does not). As opposed to the response callback, we don't have to call it with a fallback value, like `false`, if the user never scrolls down or closes the tab.
I added a code comment for why we use a `pending_remote` too.
With the `pending_remote`, a second `ObserveFieldVisibility()` call cancels the first one's subscription without explicitly informing the first one about it. I suppose the caller can use `set_disconnect_handler()` and/or with a timeout to address that? (Could you document that?)
An alternative with less boilerplate code may be a timer in `AutofillAgent` that destroys `form_element_intersection_observer_` after 5 seconds or so. (The timeout could be a parameter of `ObserveFieldVisibility()`.) Are there reasons why the `pending_remote` is preferable?
// delayed indefinitely. Since standard Mojo response callbacks are expected
// to return promptly, we avoid using an unbounded callback here in favor of aWhat does "promptly" mean exactly? I guess it's not "synchronously".
I assume issue is that with the response callback, it is called at the latest when `~AutofillAgent()` destroys `form_element_intersection_observer_`? AFAIK that's technically not an issue. Or is it?
// to return promptly, we avoid using an unbounded callback here in favor of aWhat's an unbounded callback?
Do you mean a dropped callback (= the callback is never called)? `WebElement::MonitorVisibility()` does guarantee to call the callback -- even if it may be very late (when we destroy `form_element_intersection_observer_`).
Related details
- Fernando Ramirez
- Nasko Oskov
- Stefan Zager
Fernando Ramirez (Gerrit)
Fernando Ramirez added 3 comments![]( "Open in Gerrit")
// Starts observing the visibility of the field identified by `field_id`.
// The `observer` will be notified when the field becomes visible (e.g.,
// meets the 800ms visibility condition).
ObserveFieldVisibility(FieldRendererId field_id,
pending_remote<AutofillVisibilityObserver> observer);Nasko OskovCould you document why we use `pending_remote`?
IIUC the differences of `pending_remote` compared to a response callback are that `pending_remote`
- supports multiple methods in the interface (obviously)
- supports any number of method calls (whereas a response callback must be called exactly once)
- supports cancellation by the browser process
- `pending_remote` needs more boilerplate code
We don't use the first and third though, and we actually do guarantee to call the method exactly once, don't we? Is the motivation for the `pending_remote` to defensively handle the case where (due to a bug) AutofillAgent does *not* call the callback? Or am I missing the real point?
Fernando RamirezYes, the goal here is to avoid having hanging callbacks across processes.
Christoph Schweringand we actually do guarantee to call the method exactly once, don't we?
Correct, this is called at most once (exactly once if the field becomes visible, and zero times if it does not). As opposed to the response callback, we don't have to call it with a fallback value, like `false`, if the user never scrolls down or closes the tab.
I added a code comment for why we use a `pending_remote` too.
With the `pending_remote`, a second `ObserveFieldVisibility()` call cancels the first one's subscription without explicitly informing the first one about it. I suppose the caller can use `set_disconnect_handler()` and/or with a timeout to address that? (Could you document that?)
An alternative with less boilerplate code may be a timer in `AutofillAgent` that destroys `form_element_intersection_observer_` after 5 seconds or so. (The timeout could be a parameter of `ObserveFieldVisibility()`.) Are there reasons why the `pending_remote` is preferable?
With the pending_remote, a second ObserveFieldVisibility() call cancels the first one's subscription without explicitly informing the first one about it. I suppose the caller can use set_disconnect_handler() and/or with a timeout to address that? (Could you document that?)
Correct, but for the omnibox project we wouldn't ever call a second `ObserveFieldVisibility()` because of this check: https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/ui/payments/omnibox_autofill_delegate.cc;l=123-126;drc=f068323b695d2ce5b559a6cb375a84f82fea51a4.
Still, I documented that the caller can use `set_disconnect_handler()`.
Are there reasons why the pending_remote is preferable?
@na...@chromium.org probably has a better answer to this.
// delayed indefinitely. Since standard Mojo response callbacks are expected
// to return promptly, we avoid using an unbounded callback here in favor of aWhat does "promptly" mean exactly? I guess it's not "synchronously".
I assume issue is that with the response callback, it is called at the latest when `~AutofillAgent()` destroys `form_element_intersection_observer_`? AFAIK that's technically not an issue. Or is it?
From my understanding, you're right, it's technically not an issue.
But @na...@chromium.org recommends using a remote rather than an unbounded callback, even if the callback would eventually return when `form_element_intersection_observer_` is destroyed.
// to return promptly, we avoid using an unbounded callback here in favor of aWhat's an unbounded callback?
Do you mean a dropped callback (= the callback is never called)? `WebElement::MonitorVisibility()` does guarantee to call the callback -- even if it may be very late (when we destroy `form_element_intersection_observer_`).
What's an unbounded callback?
A callback that can be executed at a completely arbitrary time in the future.
WebElement::MonitorVisibility() does guarantee to call the callback -- even if it may be very late (when we destroy form_element_intersection_observer_).
@na...@chromium.org mentioned to me that this is not usually how we design features, callbacks should return relatively quickly.
Related details
- Christoph Schwering
- Nasko Oskov
- Stefan Zager
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christoph Schwering (Gerrit)
Christoph Schwering voted and added 3 comments![]( "Open in Gerrit")
Votes added by Christoph Schwering
| Code-Review | +1 |
3 comments
LGTM, thanks for the perseverance, Fernando :)
for the omnibox project we wouldn't ever call a second `ObserveFieldVisibility()`
My point is that *another* caller of ObserveFieldVisibility() would implicitly cancel OmniboxAutofillDelegate's observation without OmniboxAutofillDelegate learning about this, which would be difficult to debug. So OmniboxAutofillDelegate may want to set a set_disconnect_handler() (but that's for a future CL, of course).
// to return promptly, we avoid using an unbounded callback here in favor of aFernando RamirezWhat's an unbounded callback?
Do you mean a dropped callback (= the callback is never called)? `WebElement::MonitorVisibility()` does guarantee to call the callback -- even if it may be very late (when we destroy `form_element_intersection_observer_`).
What's an unbounded callback?
A callback that can be executed at a completely arbitrary time in the future.
WebElement::MonitorVisibility() does guarantee to call the callback -- even if it may be very late (when we destroy form_element_intersection_observer_).
@na...@chromium.org mentioned to me that this is not usually how we design features, callbacks should return relatively quickly.
What's an unbounded callback?
A callback that can be executed at a completely arbitrary time in the future.
I think I haven't seen the expression "unbounded callback" in Chromium before. "Unbound" and "callback" sounds to me like it's referring unbound parameters.
I suppose just "callback" or "response callback" would be clearer.
Related details
- Fernando Ramirez
- Nasko Oskov
- Stefan Zager
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fernando Ramirez (Gerrit)
Fernando Ramirez added 3 comments![]( "Open in Gerrit")
LGTM, thanks for the perseverance, Fernando :)
No problem, and thank you for the bearing with my throughout the review process! This is my first time working with renderer related code so all of this is pretty new to me 😄
// Starts observing the visibility of the field identified by `field_id`.
// The `observer` will be notified when the field becomes visible (e.g.,
// meets the 800ms visibility condition).
ObserveFieldVisibility(FieldRendererId field_id,
pending_remote<AutofillVisibilityObserver> observer);I see what you're saying now. Sounds good, I'll handle this in a separate CL.
// to return promptly, we avoid using an unbounded callback here in favor of aFernando RamirezWhat's an unbounded callback?
Do you mean a dropped callback (= the callback is never called)? `WebElement::MonitorVisibility()` does guarantee to call the callback -- even if it may be very late (when we destroy `form_element_intersection_observer_`).
Christoph SchweringWhat's an unbounded callback?
A callback that can be executed at a completely arbitrary time in the future.
WebElement::MonitorVisibility() does guarantee to call the callback -- even if it may be very late (when we destroy form_element_intersection_observer_).
@na...@chromium.org mentioned to me that this is not usually how we design features, callbacks should return relatively quickly.
What's an unbounded callback?
A callback that can be executed at a completely arbitrary time in the future.
I think I haven't seen the expression "unbounded callback" in Chromium before. "Unbound" and "callback" sounds to me like it's referring unbound parameters.
I suppose just "callback" or "response callback" would be clearer.
Ah I was using terminology used by @na...@chromium.org in our chat as that's what he referred to. Maybe it's not a common expression.
I've updated the comment to use "response callback" rather than "unbound callback".
Related details
- Nasko Oskov
- Stefan Zager
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nasko Oskov (Gerrit)
Nasko Oskov added 1 comment![]( "Open in Gerrit")
void OnFieldBecameVisible();Shouldn't this be `override`? Also, it comes from the mojo interface, but this class hasn't added inheritance from the mojo interface. I'm not sure I follow how this works as I also don't see what code calls ObserveFieldVisibility.
Related details
- Fernando Ramirez
- Stefan Zager
Fernando Ramirez (Gerrit)
Fernando Ramirez added 1 comment![]( "Open in Gerrit")
void OnFieldBecameVisible();Shouldn't this be `override`? Also, it comes from the mojo interface, but this class hasn't added inheritance from the mojo interface. I'm not sure I follow how this works as I also don't see what code calls ObserveFieldVisibility.
`OnFieldBecameVisible` is not an override because `OmniboxAutofillDelegate` is a core browser class that doesn't implement mojo interfaces directly.
Instead, this is handled in `ContentAutofillDriver`, which translates the incoming mojo notification back to this C++ callback.
This call to the driver is implemented by @yis...@google.com in crrev.com/c/7944884 like this:
```
manager.driver().ObserveFieldVisibility(
/*field_id=*/trigger_field_global_id_,
/*callback=*/
base::BindOnce(&OmniboxAutofillDelegate::OnFieldBecameVisible,
weak_ptr_factory_.GetWeakPtr()));
```
Related details
- Nasko Oskov
- Stefan Zager
- Yishui Liu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stefan Zager (Gerrit)
Stefan Zager added 7 comments![]( "Open in Gerrit")
interface AutofillVisibilityObserver {There are no implementations of this as yet, I assume that will be in a follow-up? Any particular reason why this method couldn't just be added to `AutofillDriver`, defined in `autofill_driver.mojom`?
base::OnceCallback<void(bool)> callback) const;I don't think `const` is correct here.
class TimeDelta;This is a bit weird -- `MonitorVisibility` takes a concrete (non-pointer non-reference) `TimeDelta` argument, so this header must be able to access the class definition for `TimeDelta`. Seems like this should either be an `#include` (following the rule of "Include What You Use"), or this advance declaration should be superfluous.
element->GetDocument().GetTaskRunner(TaskType::kUserInteraction),This should be the default task runner; this task shouldn't compete with input events.
observer_ = IntersectionObserver::Create(This overload of `Create` is meant only to support construction from javascript. For blink-internal usage like this, you should use:
This implies that `VisibilityObserver` need not inherit from `IntersectionObserverDelegate`. This probable makes some of the new `#include`'s unnecessary.
DeliverResult(false);This doesn't seem possible.
visibility_timer_.Stop();Should this be moved outside the `if` clause?
Related details
- Fernando Ramirez
- Nasko Oskov
- Yishui Liu
Nasko Oskov (Gerrit)
Nasko Oskov added 1 comment![]( "Open in Gerrit")
void OnFieldBecameVisible();Fernando RamirezShouldn't this be `override`? Also, it comes from the mojo interface, but this class hasn't added inheritance from the mojo interface. I'm not sure I follow how this works as I also don't see what code calls ObserveFieldVisibility.
`OnFieldBecameVisible` is not an override because `OmniboxAutofillDelegate` is a core browser class that doesn't implement mojo interfaces directly.
Instead, this is handled in `ContentAutofillDriver`, which translates the incoming mojo notification back to this C++ callback.
This call to the driver is implemented by @yis...@google.com in crrev.com/c/7944884 like this:
```
manager.driver().ObserveFieldVisibility(
/*field_id=*/trigger_field_global_id_,
/*callback=*/
base::BindOnce(&OmniboxAutofillDelegate::OnFieldBecameVisible,
weak_ptr_factory_.GetWeakPtr()));
```
I am not sure I follow. When you have a mojo interface, the receiving end is usually a class that inherits the mojo interface definition and provides the receiving method implementation. In this CL's case, some class must provide the `OnFieldBecameVisible` implementation and I do not see it. Therefore this CL does not seem to actually have working implementation of the functionality of receiving the call from the renderer process.
If you look at szager@'s comment he raises similar point - you have not implemented the interface you are adding and you should, as this is what we expect when adding mojo API surface - implementation is in the same CL so it can be reviewed for correctness.
Related details
- Fernando Ramirez
- Yishui Liu
Christoph Schwering (Gerrit)
Christoph Schwering voted and added 1 comment![]( "Open in Gerrit")
Votes added by Christoph Schwering
| Code-Review | +1 |
1 comment
interface AutofillVisibilityObserver {There are no implementations of this as yet, I assume that will be in a follow-up? Any particular reason why this method couldn't just be added to `AutofillDriver`, defined in `autofill_driver.mojom`?
Making `AutofillDriver` the `AutofillVisibilityObserver` sounds wrong to me because then we can have only one `AutofillVisibilityObserver` at a time.
Stefan Zager (Gerrit)
Stefan Zager added 1 comment![]( "Open in Gerrit")
interface AutofillVisibilityObserver {Christoph SchweringThere are no implementations of this as yet, I assume that will be in a follow-up? Any particular reason why this method couldn't just be added to `AutofillDriver`, defined in `autofill_driver.mojom`?
Making `AutofillDriver` the `AutofillVisibilityObserver` sounds wrong to me because then we can have only one `AutofillVisibilityObserver` at a time.
I don't think that's true, as long as the `FieldRendererId` is passed through.
Christoph Schwering (Gerrit)
Christoph Schwering added 1 comment![]( "Open in Gerrit")
interface AutofillVisibilityObserver {Christoph SchweringThere are no implementations of this as yet, I assume that will be in a follow-up? Any particular reason why this method couldn't just be added to `AutofillDriver`, defined in `autofill_driver.mojom`?
Stefan ZagerMaking `AutofillDriver` the `AutofillVisibilityObserver` sounds wrong to me because then we can have only one `AutofillVisibilityObserver` at a time.
I don't think that's true, as long as the `FieldRendererId` is passed through.
If `[Content]AutofillDriver` *is* the `AutofillVisiblityObserver`, how could it inform different of `AutofillDriver::ObserveFieldVisibility()` when a field became visible?
I think the straightforward API is to make `mojom::PendingRemote<AutofillVisibilityObserver>` a parameter of `ContentAutofillDriver::ObserveFieldVisibility()`. Then it's the callers job to implement `AutofillVisiblityObserver` to their liking.
(Currently, `AutofillAgent` of course only supports one simultaneous observer. But IMO the API should be so that we can support multiple, so that we only need to make small changes to `AutofillAgent` when some other part of the code wants to observe field visibility.)
Yishui Liu (Gerrit)
Yishui Liu added 1 comment![]( "Open in Gerrit")
interface AutofillVisibilityObserver {Christoph SchweringThere are no implementations of this as yet, I assume that will be in a follow-up? Any particular reason why this method couldn't just be added to `AutofillDriver`, defined in `autofill_driver.mojom`?
Stefan ZagerMaking `AutofillDriver` the `AutofillVisibilityObserver` sounds wrong to me because then we can have only one `AutofillVisibilityObserver` at a time.
Christoph SchweringI don't think that's true, as long as the `FieldRendererId` is passed through.
If `[Content]AutofillDriver` *is* the `AutofillVisiblityObserver`, how could it inform different of `AutofillDriver::ObserveFieldVisibility()` when a field became visible?
I think the straightforward API is to make `mojom::PendingRemote<AutofillVisibilityObserver>` a parameter of `ContentAutofillDriver::ObserveFieldVisibility()`. Then it's the callers job to implement `AutofillVisiblityObserver` to their liking.
(Currently, `AutofillAgent` of course only supports one simultaneous observer. But IMO the API should be so that we can support multiple, so that we only need to make small changes to `AutofillAgent` when some other part of the code wants to observe field visibility.)
I think we need to put this interface in `components/autofill/core/common/mojom/autofill_types.mojom` if we want to implement it `AutofillOmniboxDelegate`, since both `autofill_driver.mojom` and `autofill_agent.mojom` are under `components/autofill/content/` which is not allowed to be included in [components/autofill/core files](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/DEPS;l=15?q=components%2Fautofill%2FDEPS).
Related details
- Fernando Ramirez
Fernando Ramirez (Gerrit)
Fernando Ramirez added 8 comments![]( "Open in Gerrit")
Fernando RamirezShouldn't this be `override`? Also, it comes from the mojo interface, but this class hasn't added inheritance from the mojo interface. I'm not sure I follow how this works as I also don't see what code calls ObserveFieldVisibility.
Nasko Oskov`OnFieldBecameVisible` is not an override because `OmniboxAutofillDelegate` is a core browser class that doesn't implement mojo interfaces directly.
Instead, this is handled in `ContentAutofillDriver`, which translates the incoming mojo notification back to this C++ callback.
This call to the driver is implemented by @yis...@google.com in crrev.com/c/7944884 like this:
```
manager.driver().ObserveFieldVisibility(
/*field_id=*/trigger_field_global_id_,
/*callback=*/
base::BindOnce(&OmniboxAutofillDelegate::OnFieldBecameVisible,
weak_ptr_factory_.GetWeakPtr()));
```
I am not sure I follow. When you have a mojo interface, the receiving end is usually a class that inherits the mojo interface definition and provides the receiving method implementation. In this CL's case, some class must provide the `OnFieldBecameVisible` implementation and I do not see it. Therefore this CL does not seem to actually have working implementation of the functionality of receiving the call from the renderer process.
If you look at szager@'s comment he raises similar point - you have not implemented the interface you are adding and you should, as this is what we expect when adding mojo API surface - implementation is in the same CL so it can be reviewed for correctness.
Ah I understand now, I have added the proper override for this method.
interface AutofillVisibilityObserver {Like @yis...@google.com [pointed out](https://chromium-review.git.corp.google.com/c/chromium/src/+/7870348/comment/0f21bb4e_93b8e873/), since `autofill_agent.mojom` is within the `components/autofill/content` directory, [we cannot include](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/DEPS;l=15;drc=ae572bef3647189819f1371c5da4841b25afa124) this header within `omnibox_autofill_delegate.h`.
`autofill_types.mojom` is already included in `autofill_agent.h` and is within the `components/autofill/core` directory so it can be included in `omnibox_autofill_delegate.h`. That's why `AutofillVisibilityObserver` was moved to this file. I'm happy to also move this interface to a different file.
I don't think `const` is correct here.
Done
This is a bit weird -- `MonitorVisibility` takes a concrete (non-pointer non-reference) `TimeDelta` argument, so this header must be able to access the class definition for `TimeDelta`. Seems like this should either be an `#include` (following the rule of "Include What You Use"), or this advance declaration should be superfluous.
I've added the include for this header and removed the forward declaration.
element->GetDocument().GetTaskRunner(TaskType::kUserInteraction),This should be the default task runner; this task shouldn't compete with input events.
Gotcha, `kInternalDefault` looks to be the default: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/platform/task_type.h;l=197-198;drc=5650b3485eaa8443ee812a44056a1b88b4ee0ce1
This overload of `Create` is meant only to support construction from javascript. For blink-internal usage like this, you should use:
This implies that `VisibilityObserver` need not inherit from `IntersectionObserverDelegate`. This probable makes some of the new `#include`'s unnecessary.
Got it, using this new `Create` function takes in a `Param` object. An extra param that I didn't know of before is `root`. Looking at this comment, it seems like providing `nullptr` to root would observe the top-level frame's viewport: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/intersection_observer/intersection_observer.h;l=163-167;drc=5650b3485eaa8443ee812a44056a1b88b4ee0ce1
This doesn't seem possible.
You're right, this function is only called within `WebElement::MonitorVisibility` when it should have both an `element_` and an `observer_`.
Should this be moved outside the `if` clause?
- Christoph Schwering
- Nasko Oskov
- Stefan Zager
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christoph Schwering (Gerrit)
Christoph Schwering voted Code-Review+1![]( "Open in Gerrit")
- Fernando Ramirez
- Nasko Oskov
- Stefan Zager
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stefan Zager (Gerrit)
Stefan Zager added 1 comment![]( "Open in Gerrit")
interface AutofillVisibilityObserver {Christoph SchweringThere are no implementations of this as yet, I assume that will be in a follow-up? Any particular reason why this method couldn't just be added to `AutofillDriver`, defined in `autofill_driver.mojom`?
Stefan ZagerMaking `AutofillDriver` the `AutofillVisibilityObserver` sounds wrong to me because then we can have only one `AutofillVisibilityObserver` at a time.
Christoph SchweringI don't think that's true, as long as the `FieldRendererId` is passed through.
Yishui LiuIf `[Content]AutofillDriver` *is* the `AutofillVisiblityObserver`, how could it inform different of `AutofillDriver::ObserveFieldVisibility()` when a field became visible?
I think the straightforward API is to make `mojom::PendingRemote<AutofillVisibilityObserver>` a parameter of `ContentAutofillDriver::ObserveFieldVisibility()`. Then it's the callers job to implement `AutofillVisiblityObserver` to their liking.
(Currently, `AutofillAgent` of course only supports one simultaneous observer. But IMO the API should be so that we can support multiple, so that we only need to make small changes to `AutofillAgent` when some other part of the code wants to observe field visibility.)
I think we need to put this interface in `components/autofill/core/common/mojom/autofill_types.mojom` if we want to implement it `AutofillOmniboxDelegate`, since both `autofill_driver.mojom` and `autofill_agent.mojom` are under `components/autofill/content/` which is not allowed to be included in [components/autofill/core files](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/DEPS;l=15?q=components%2Fautofill%2FDEPS).
`AutofillAgent` and `AutofillDriver` form a conjoined pair; the first one handles browser-to-renderer messages and the second one handles renderer-to-browser messages. It makes sense to me that the "start observing this element for visibility" message would be sent from the browser via AutofillAgent mojo, and the "this element is now visible" would be reported back via AutofillDriver mojo. Am I missing something?
Related details
- Fernando Ramirez
- Nasko Oskov
Stefan Zager (Gerrit)
Stefan Zager added 1 comment![]( "Open in Gerrit")
interface AutofillVisibilityObserver {Christoph SchweringThere are no implementations of this as yet, I assume that will be in a follow-up? Any particular reason why this method couldn't just be added to `AutofillDriver`, defined in `autofill_driver.mojom`?
Stefan ZagerMaking `AutofillDriver` the `AutofillVisibilityObserver` sounds wrong to me because then we can have only one `AutofillVisibilityObserver` at a time.
Christoph SchweringI don't think that's true, as long as the `FieldRendererId` is passed through.
Yishui LiuIf `[Content]AutofillDriver` *is* the `AutofillVisiblityObserver`, how could it inform different of `AutofillDriver::ObserveFieldVisibility()` when a field became visible?
I think the straightforward API is to make `mojom::PendingRemote<AutofillVisibilityObserver>` a parameter of `ContentAutofillDriver::ObserveFieldVisibility()`. Then it's the callers job to implement `AutofillVisiblityObserver` to their liking.
(Currently, `AutofillAgent` of course only supports one simultaneous observer. But IMO the API should be so that we can support multiple, so that we only need to make small changes to `AutofillAgent` when some other part of the code wants to observe field visibility.)
Stefan ZagerI think we need to put this interface in `components/autofill/core/common/mojom/autofill_types.mojom` if we want to implement it `AutofillOmniboxDelegate`, since both `autofill_driver.mojom` and `autofill_agent.mojom` are under `components/autofill/content/` which is not allowed to be included in [components/autofill/core files](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/DEPS;l=15?q=components%2Fautofill%2FDEPS).
`AutofillAgent` and `AutofillDriver` form a conjoined pair; the first one handles browser-to-renderer messages and the second one handles renderer-to-browser messages. It makes sense to me that the "start observing this element for visibility" message would be sent from the browser via AutofillAgent mojo, and the "this element is now visible" would be reported back via AutofillDriver mojo. Am I missing something?
To be clear: I don't see the need for `AutofillVisibilityObserver` as a new stand-alone interface. It seems like it should be possible and straightforward to route the renderer->browswer message to `OmniboxAutofillDelegate` via `AutofillDriver`, without the extra type complexity and lifetime issues.