[AF] Add and implement GetIntersectionObserverInfo in AutofillAgent [chromium/src : main]
Fernando Ramirez (Gerrit)
Fernando Ramirez added 1 comment![]( "Open in Gerrit")
Related details
- Olivia Saul
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivia Saul (Gerrit)
Olivia Saul added 5 comments![]( "Open in Gerrit")
(There's also a merge conflict apparently)
std::move(callback).Run(false);It makes sense that we'd want to return `false` here, but it's more of a "something went wrong" rather than a "hey the thing is not visible", right? Should the param maybe be renamed to something more along those lines? Perhaps not `success`, but do you know what I mean?
FastForwardBy(base::Milliseconds(800));Is it possible to change all these `800`s to use `kMinimumVisibleDuration`? Otherwise, if we change `kMinimumVisibleDuration` to 801, all these tests fail, and that's probably not intended.
HeapTaskRunnerTimer<FormController> visibility_timer_;
Member<IntersectionObserver> observer_;
base::OnceCallback<void(bool)> visibility_callback_;Please add comment lines to describe what all three of these are. They're just vague enough to not imply they're for our specific use case.
}nit: These appear to be out of order with the .h file...but it looks like this was already a pre-existing problem? If so, I guess disregard...
Related details
- Fernando Ramirez
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 4 comments![]( "Open in Gerrit")
std::move(callback).Run(false);It makes sense that we'd want to return `false` here, but it's more of a "something went wrong" rather than a "hey the thing is not visible", right? Should the param maybe be renamed to something more along those lines? Perhaps not `success`, but do you know what I mean?
Running the callback with `false` here means the element wasn't found. Based on [Christoph's previous comment](https://chromium-review.git.corp.google.com/c/chromium/src/+/7814161/comment/8fdc41be_6100880f/), this can happen because the element is going to be passed from the browser, which is asynchronous. By the time `AutofillAgent::GetIntersectionObserverInfo()` is called, the element may be gone which technically means it's not visible, no?
Is it possible to change all these `800`s to use `kMinimumVisibleDuration`? Otherwise, if we change `kMinimumVisibleDuration` to 801, all these tests fail, and that's probably not intended.
Yup good idea, instead of hard coding these to `800` I'll use `kMinimumVisibleDuration`.
HeapTaskRunnerTimer<FormController> visibility_timer_;
Member<IntersectionObserver> observer_;
base::OnceCallback<void(bool)> visibility_callback_;Please add comment lines to describe what all three of these are. They're just vague enough to not imply they're for our specific use case.
Done. Also added a comment above `MonitorVisibility`, `VisibilityTimerFired`, and the new static variable for `kMinimumVisibleDuration`.
nit: These appear to be out of order with the .h file...but it looks like this was already a pre-existing problem? If so, I guess disregard...
`GetDeliveryBehavior`, `Deliver`, `GetExecutionContext`, and `MonitorVisibility` are in order. But yeah I did notice the rest were out of order so I just decided to try to group all these functions together.
Also, I added comments for what parameters we're setting for the intersection observer.
Related details
- Olivia Saul
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. |
Olivia Saul (Gerrit)
Olivia Saul added 5 comments![]( "Open in Gerrit")
// intersecting the viewport and is visible.Add extra text please:
*`is_visible` will also return `false` if the field with ID `field_id` could not be found.*
std::move(callback).Run(false);Fernando RamirezIt makes sense that we'd want to return `false` here, but it's more of a "something went wrong" rather than a "hey the thing is not visible", right? Should the param maybe be renamed to something more along those lines? Perhaps not `success`, but do you know what I mean?
Running the callback with `false` here means the element wasn't found. Based on [Christoph's previous comment](https://chromium-review.git.corp.google.com/c/chromium/src/+/7814161/comment/8fdc41be_6100880f/), this can happen because the element is going to be passed from the browser, which is asynchronous. By the time `AutofillAgent::GetIntersectionObserverInfo()` is called, the element may be gone which technically means it's not visible, no?
Yeah, and that's all fine, I just found myself feeling that returning "no, not visible" implicitly meant "but I did find the field", which isn't true. The more I think about it, though, the more I *don't* want to name it something like `field_found_and_visible` instead of simply `is_visible`, so I added a request to clarify this in the function header comment instead. Thanks!
// The IntersectionObserver used to monitor the visibility of the form
// control.nit: `the form control` -> `a form control, if desired`
// at least `kMinimumVisibleDuration`.Please list what conditions it could return `false`.
FormController(Document& document);
~FormController() override;**Note:** I think the auto-attached fix is wrong...please be careful.
~ ~ ~
Please fix this WARNING reported by ClangTidy: check: google-explicit-constructor
single-argument constructors must be marked ...
check: google-explicit-constructor
single-argument constructors must be marked explicit to avoid unintentional implicit conversions (https://clang.llvm.org/extra/clang-tidy/checks/google/explicit-constructor.html)
(Note: You can add `Skip-Clang-Tidy-Checks: google-explicit-constructor` footer to the CL description to skip the check)
(Lint observed on `android-clang-tidy-rel`, but not on `linux-clang-tidy-rel`)
Related details
- Fernando Ramirez
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 4 comments![]( "Open in Gerrit")
Add extra text please:
*`is_visible` will also return `false` if the field with ID `field_id` could not be found.*
Done
// The IntersectionObserver used to monitor the visibility of the form
// control.nit: `the form control` -> `a form control, if desired`
Done
// at least `kMinimumVisibleDuration`.Please list what conditions it could return `false`.
Here we wouldn't ever call `false` which reminds me that you mentioned maybe we shouldn't use a boolean callback. Should we instead use a void callback?
I've updated the comment for now.
FormController(Document& document);
~FormController() override;**Note:** I think the auto-attached fix is wrong...please be careful.
~ ~ ~
Please fix this WARNING reported by ClangTidy: check: google-explicit-constructor
single-argument constructors must be marked ...
check: google-explicit-constructor
single-argument constructors must be marked explicit to avoid unintentional implicit conversions (https://clang.llvm.org/extra/clang-tidy/checks/google/explicit-constructor.html)
(Note: You can add `Skip-Clang-Tidy-Checks: google-explicit-constructor` footer to the CL description to skip the check)
(Lint observed on `android-clang-tidy-rel`, but not on `linux-clang-tidy-rel`)
- Olivia Saul
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. |
Olivia Saul (Gerrit)
Olivia Saul voted and added 3 comments![]( "Open in Gerrit")
Votes added by Olivia Saul
| Code-Review | +1 |
3 comments
LGTM provided the last two comments are addressed (`explicit` fix and ensuring a false callback is documented).
// at least `kMinimumVisibleDuration`.Fernando RamirezPlease list what conditions it could return `false`.
Here we wouldn't ever call `false` which reminds me that you mentioned maybe we shouldn't use a boolean callback. Should we instead use a void callback?
I've updated the comment for now.
I'm sorry, I think I'm getting my callbacks mixed up in my head. web_document.cc's `MonitorFormFieldVisibility(~)` contains `std::move(callback).Run(false);` on line 533. That's the one I meant to leave this comment about.
FormController(Document& document);
~FormController() override;Fernando Ramirez**Note:** I think the auto-attached fix is wrong...please be careful.
~ ~ ~
Please fix this WARNING reported by ClangTidy: check: google-explicit-constructor
single-argument constructors must be marked ...
check: google-explicit-constructor
single-argument constructors must be marked explicit to avoid unintentional implicit conversions (https://clang.llvm.org/extra/clang-tidy/checks/google/explicit-constructor.html)
(Note: You can add `Skip-Clang-Tidy-Checks: google-explicit-constructor` footer to the CL description to skip the check)
(Lint observed on `android-clang-tidy-rel`, but not on `linux-clang-tidy-rel`)
Yeahh I had just ignored that for now lol
Oh it still requires a fix -- constructors with a single argument have to have `explicit` in front of them [as a Google C++ standard](https://google.github.io/styleguide/cppguide.html#Implicit_Conversions). But the auto-generated fix seems to delete that constructor and tag the destructor with `explicit` which is not the right thing at all, lol.
Related details
- Fernando Ramirez
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 2 comments![]( "Open in Gerrit")
// at least `kMinimumVisibleDuration`.Fernando RamirezPlease list what conditions it could return `false`.
Olivia SaulHere we wouldn't ever call `false` which reminds me that you mentioned maybe we shouldn't use a boolean callback. Should we instead use a void callback?
I've updated the comment for now.
I'm sorry, I think I'm getting my callbacks mixed up in my head. web_document.cc's `MonitorFormFieldVisibility(~)` contains `std::move(callback).Run(false);` on line 533. That's the one I meant to leave this comment about.
No worries, I also left a comment that we return `false` if the document is null (line 528).
FormController(Document& document);
~FormController() override;Fernando Ramirez**Note:** I think the auto-attached fix is wrong...please be careful.
~ ~ ~
Please fix this WARNING reported by ClangTidy: check: google-explicit-constructor
single-argument constructors must be marked ...
check: google-explicit-constructor
single-argument constructors must be marked explicit to avoid unintentional implicit conversions (https://clang.llvm.org/extra/clang-tidy/checks/google/explicit-constructor.html)
(Note: You can add `Skip-Clang-Tidy-Checks: google-explicit-constructor` footer to the CL description to skip the check)
(Lint observed on `android-clang-tidy-rel`, but not on `linux-clang-tidy-rel`)
Olivia SaulYeahh I had just ignored that for now lol
Oh it still requires a fix -- constructors with a single argument have to have `explicit` in front of them [as a Google C++ standard](https://google.github.io/styleguide/cppguide.html#Implicit_Conversions). But the auto-generated fix seems to delete that constructor and tag the destructor with `explicit` which is not the right thing at all, lol.
Ohh sorry I misunderstood! I've added the `explicit` keyword to the constructor.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fernando Ramirez (Gerrit)
Fernando Ramirez added 1 comment![]( "Open in Gerrit")
Hi Stefan, PTAL. This is the implementation we aligned on in go/autofill-omnibox-intersection-observer-visibility-tracking. A separate CL was created since significant changes were involved to the original CL.
Related details
- Stefan Zager
Code-Coverage
Code-Owners
Code-Review
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")
visibility_callback_ = std::move(callback);Should this be:
```
if (visibility_callback_) {
std::move(visibility_callback_).Run(false);
}
visibility_callback_ = std::move(callback);
```
?
And maybe `visibility_timer_.Stop();` for good measure?
Related details
- Fernando Ramirez
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 1 comment![]( "Open in Gerrit")
Should this be:
```
if (visibility_callback_) {
std::move(visibility_callback_).Run(false);
}
visibility_callback_ = std::move(callback);
```?
And maybe `visibility_timer_.Stop();` for good measure?
Good catch, without this the first callback would never be run if we begin observing a new element (which shouldn't happen for our feature but it's good to generalize for future use cases). I also added a comment above to document this.
Related details
- Stefan Zager
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stefan Zager (Gerrit)
Stefan Zager voted and added 1 comment![]( "Open in Gerrit")
- Fernando Ramirez
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fernando Ramirez (Gerrit)
Fernando Ramirez added 1 comment![]( "Open in Gerrit")
- Christoph Schwering
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |