[Autofill / PWM] Defer agent / client creation [chromium/src : main]
Ian Vollick (Gerrit)
Ian Vollick voted Commit-Queue+0![]( "Open in Gerrit")
| Commit-Queue | +0 |
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
findit-for-me@appspot.gserviceaccount.com (Gerrit)
findit...@appspot.gserviceaccount.com voted Code-Coverage+1![]( "Open in Gerrit")
This change meets the code coverage requirements.
| Code-Coverage | +1 |
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
findit-for-me@appspot.gserviceaccount.com (Gerrit)
findit...@appspot.gserviceaccount.com voted Code-Coverage+1![]( "Open in Gerrit")
This change meets the code coverage requirements.
| Code-Coverage | +1 |
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Vollick (Gerrit)
Ian Vollick added 1 comment![]( "Open in Gerrit")
Hey all. I've implemented Christoph's suggestion in this CL. There's a fair bit in this patch and it doesn't yet remove the deferring agents (I kept the old version behind a flag so we could fall back if there are issues with the new behavior).
You'll notice that a lot of the CL is common with crrev.com/c/5555207. One thing I could do is land a patch that is pure plumbing to migrate callers. I'll prepare that after sending this out for review.
Another thing I could do is land crrev.com/c/5555207 first so that the CHECK in the drivers isn't conditional on the new path (since we'll be deferring creation notifications).
Plmk if you have preferences.
Related details
- Christoph Schwering
- Dave Tapuska
- Jan Keitel
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christoph Schwering (Gerrit)
Christoph Schwering added 4 comments![]( "Open in Gerrit")
Thanks for working on this!
Did you mean to rebase on crrev.com/c/5647771?
if (is_prerendering && defer_while_prerendering) {Pull the IsEnabled() call down here to enroll only clients that are acutally prerendering?
if (unsafe_render_frame()->GetWebFrame()->GetDocument().IsLoaded()) {I'm not sure what `IsLoaded()` means.
Should we instead use [`Document::ParsingState`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/document.h;l=1023;drc=90cac1911508d3d682a67c97aa62483eb712f69a) and, in the PWM case, also [`Document::DocumentReadyState`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/document.h;l=1015-1017;drc=90cac1911508d3d682a67c97aa62483eb712f69a)?
Or maybe we could pass this information as parameter of `NotifyPrerenderingDocumentActivated()` from `DocumentLoader`.
CHECK(!IsPrerendering() || !defer_creation);As above: inline the IsEnabled() call to avoid enrolling irrelevant clients?
Related details
- Dave Tapuska
- Ian Vollick
- Jan Keitel
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
findit-for-me@appspot.gserviceaccount.com (Gerrit)
findit...@appspot.gserviceaccount.com voted Code-Coverage+1![]( "Open in Gerrit")
This change meets the code coverage requirements.
| Code-Coverage | +1 |
Related details
- Dave Tapuska
- Jan Keitel
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Vollick (Gerrit)
Ian Vollick added 4 comments![]( "Open in Gerrit")
Thanks for working on this!
Did you mean to rebase on crrev.com/c/5647771?
I will, yes, and also on crrev.com/c/5647237. But it seemed simpler for me to cherry-pick changes from those other CLs rather than create a string of dependent CLs (the first two don't really depend on each other) so I'm keeping everything glommed together here for now. I could mark this as WIP while those other CLs are in review, too.
if (is_prerendering && defer_while_prerendering) {Pull the IsEnabled() call down here to enroll only clients that are acutally prerendering?
Done and made the analogous change for pwm.
if (unsafe_render_frame()->GetWebFrame()->GetDocument().IsLoaded()) {I'm not sure what `IsLoaded()` means.
Should we instead use [`Document::ParsingState`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/document.h;l=1023;drc=90cac1911508d3d682a67c97aa62483eb712f69a) and, in the PWM case, also [`Document::DocumentReadyState`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/document.h;l=1015-1017;drc=90cac1911508d3d682a67c97aa62483eb712f69a)?
Or maybe we could pass this information as parameter of `NotifyPrerenderingDocumentActivated()` from `DocumentLoader`.
I'm not sure about IsLoaded either. It seems to be based on the existence of the parser, but using IsLoadCompleted (which depends on ready state) and HasFinishedParsing (which depends on the parsing state) seems good to me, though I'd like to check with an expert. I've made this change in the latest patch.
CHECK(!IsPrerendering() || !defer_creation);As above: inline the IsEnabled() call to avoid enrolling irrelevant clients?
- Christoph Schwering
- Dave Tapuska
- Jan Keitel
Ian Vollick (Gerrit)
Ian Vollick added 1 comment![]( "Open in Gerrit")
- Christoph Schwering
- Dave Tapuska
- Jan Keitel
- Richard (Torne) Coles
- Vasilii Sukhanov
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave Tapuska (Gerrit)
Dave Tapuska voted and added 3 comments![]( "Open in Gerrit")
Votes added by Dave Tapuska
| Code-Review | +1 |
3 comments
class PostPrerenderingActivationTask : blink::WebLocalFrameObserver {public ?
// |kFinishedParsing|.backticks are the preferred C++ style as opposed to pipes.
// Unlike IsLoaded, which is based on the existance of the document's parser,"existance" is a possible misspelling of "existence".
Please fix.
Related details
- Christoph Schwering
- Ian Vollick
- Jan Keitel
- Richard (Torne) Coles
- Vasilii Sukhanov
Code-Coverage
Code-Review
Ian Vollick (Gerrit)
Ian Vollick added 4 comments![]( "Open in Gerrit")
class PostPrerenderingActivationTask : blink::WebLocalFrameObserver {Ian Vollickpublic ?
Done
bool HasFinishedParsing() const;Sadly, it looks like using this as a signal to know if the dom content loaded is incorrect.
This call causes the dom content loaded event to be fired.
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/document.cc;l=7514;drc=cc5be7150eef183a1b9a6716d42a396ab7c59733
I.e., it's only called when we successfully finish parsing.
However, we also set kFinishedParsing when parsing is cancelled.
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/document.cc;l=3685;drc=cc5be7150eef183a1b9a6716d42a396ab7c59733
I'm going to put the CL into WIP while I look at alternatives.
backticks are the preferred C++ style as opposed to pipes.
Done
// Unlike IsLoaded, which is based on the existance of the document's parser,"existance" is a possible misspelling of "existence".
Please fix.
- Christoph Schwering
- Jan Keitel
- Richard (Torne) Coles
- Vasilii Sukhanov
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
findit-for-me@appspot.gserviceaccount.com (Gerrit)
findit...@appspot.gserviceaccount.com voted Code-Coverage+1![]( "Open in Gerrit")
This change meets the code coverage requirements.
| Code-Coverage | +1 |
Related details
Christoph Schwering (Gerrit)
Christoph Schwering added 5 comments![]( "Open in Gerrit")
password_autofill_agent_->DidActivatePrerenderingDocument();Could you flip the order with the `if`? Right now, PAA::DidDispatchDOMContentLoadedEvent() is independent of AA (I think), but I expect that'll change in the future and that PAA will depend on AA having extracted forms already.
CHECK(!IsPrerendering() || !defer_creation);Ian VollickAs above: inline the IsEnabled() call to avoid enrolling irrelevant clients?
Done and made the analogous change for pwm.
Thanks! If you like to keep the shorter name, you could define a helper
```
namespace {
void ShouldDeferCreation() {
return base::FeatureList::IsEnabled(
autofill::features::kAutofillDeferDriverAgentCreationUntilActivation);
}
}
```
I think we have or had this pattern a few times in Autofill[Agent].
autofill::features::nit: remove? (also elsewhere)
bool HasFinishedParsing() const;Sadly, it looks like using this as a signal to know if the dom content loaded is incorrect.
This call causes the dom content loaded event to be fired.
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/document.cc;l=7514;drc=cc5be7150eef183a1b9a6716d42a396ab7c59733I.e., it's only called when we successfully finish parsing.
However, we also set kFinishedParsing when parsing is cancelled.
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/document.cc;l=3685;drc=cc5be7150eef183a1b9a6716d42a396ab7c59733I'm going to put the CL into WIP while I look at alternatives.
The logic could move to `PostPrerenderingActivationTask`, roughly like this:
```
class PostPrerenderingActivationTask : public blink::WebLocalFrameObserver,
public content::RenderFrameObserver {
public:
PostPrerenderingActivationTask(blink::WebLocalFrame* web_local_frame,
base::OnceCallback<void()> callback)
: blink::WebLocalFrameObserver(web_local_frame),
callback_(std::move(callback)) {}
~PostPrerenderingActivationTask() override = default;
void DidActivatePrerenderingDocument() override {
if (dcl_) aa_.DidDispatchDOMContentLoaded();
if (load_) aa_.DidFinishLoad();
delete this;
} void OnFrameDetached() override { delete this; }
void DidDispatchDOMContentLoaded() override { dcl_ = true; }
void DidFinishLoad() override { load_ = true; }private:
AutofillAgent& aa_;
bool dcl_ = false;
bool load_ = false;
};
```
We could make it `AutofillAgent`'s responsibility to pass these calls on to `PasswordAutofillAgent`.
bool IsLoaded();
// Unlike IsLoaded, which is based on the existence of the document's parser,
// this is based on the document's ready state.
bool IsLoadCompleted() const;
// Returns true if the document's parsing state has reached
// `kFinishedParsing`.
bool HasFinishedParsing() const;Edit: race condition with your comment, so the comment below is probably obsolete.
---
I wonder if we can document these more thoroughly or mention the JS events they refer to. My understanding so far is:
- `HasFinishedParsing()` == [`DOMContentLoaded`](https://developer.mozilla.org/en-US/docs/Web/API/Document/DOMContentLoaded_event) has been fired.
- `IsLoadComplete()` == [`load`](https://developer.mozilla.org/en-US/docs/Web/API/Window/load_event) has been fired.
- `IsLoaded()` == nothing?
In particular, `IsLoadComplete()` implies `HasFinishedParsing()`, but `IsLoadComplete()` may never become true for a document.
Related details
- Ian Vollick
Ian Vollick (Gerrit)
Ian Vollick added 5 comments![]( "Open in Gerrit")
password_autofill_agent_->DidActivatePrerenderingDocument();Could you flip the order with the `if`? Right now, PAA::DidDispatchDOMContentLoadedEvent() is independent of AA (I think), but I expect that'll change in the future and that PAA will depend on AA having extracted forms already.
Done
CHECK(!IsPrerendering() || !defer_creation);Ian VollickAs above: inline the IsEnabled() call to avoid enrolling irrelevant clients?
Christoph SchweringDone and made the analogous change for pwm.
Thanks! If you like to keep the shorter name, you could define a helper
```
namespace {
void ShouldDeferCreation() {
return base::FeatureList::IsEnabled(
autofill::features::kAutofillDeferDriverAgentCreationUntilActivation);
}
}
```
I think we have or had this pattern a few times in Autofill[Agent].
Done
autofill::features::Ian Vollicknit: remove? (also elsewhere)
Done
bool IsLoaded();
// Unlike IsLoaded, which is based on the existence of the document's parser,
// this is based on the document's ready state.
bool IsLoadCompleted() const;
// Returns true if the document's parsing state has reached
// `kFinishedParsing`.
bool HasFinishedParsing() const;Edit: race condition with your comment, so the comment below is probably obsolete.
---
I wonder if we can document these more thoroughly or mention the JS events they refer to. My understanding so far is:
- `HasFinishedParsing()` == [`DOMContentLoaded`](https://developer.mozilla.org/en-US/docs/Web/API/Document/DOMContentLoaded_event) has been fired.
- `IsLoadComplete()` == [`load`](https://developer.mozilla.org/en-US/docs/Web/API/Window/load_event) has been fired.
- `IsLoaded()` == nothing?
In particular, `IsLoadComplete()` implies `HasFinishedParsing()`, but `IsLoadComplete()` may never become true for a document.
I've removed all this in the latest patchset, so I think this is moot.
- Christoph Schwering
- Dave Tapuska
- Jan Keitel
- Richard (Torne) Coles
- Vasilii Sukhanov
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Vollick (Gerrit)
Ian Vollick added 2 comments![]( "Open in Gerrit")
Ian VollickThanks for working on this!
Did you mean to rebase on crrev.com/c/5647771?
I will, yes, and also on crrev.com/c/5647237. But it seemed simpler for me to cherry-pick changes from those other CLs rather than create a string of dependent CLs (the first two don't really depend on each other) so I'm keeping everything glommed together here for now. I could mark this as WIP while those other CLs are in review, too.
Resolving this since this change no longer has dependencies.
if (unsafe_render_frame()->GetWebFrame()->GetDocument().IsLoaded()) {Ian VollickI'm not sure what `IsLoaded()` means.
Should we instead use [`Document::ParsingState`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/document.h;l=1023;drc=90cac1911508d3d682a67c97aa62483eb712f69a) and, in the PWM case, also [`Document::DocumentReadyState`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/document.h;l=1015-1017;drc=90cac1911508d3d682a67c97aa62483eb712f69a)?
Or maybe we could pass this information as parameter of `NotifyPrerenderingDocumentActivated()` from `DocumentLoader`.
I'm not sure about IsLoaded either. It seems to be based on the existence of the parser, but using IsLoadCompleted (which depends on ready state) and HasFinishedParsing (which depends on the parsing state) seems good to me, though I'd like to check with an expert. I've made this change in the latest patch.
Since I'm not listening to the RFO methods rather than trying to find an equivalent signal, I think this is no longer an issue so I'm gonna resolve this thread.
Related details
- Christoph Schwering
- Dave Tapuska
- Jan Keitel
- Richard (Torne) Coles
- Vasilii Sukhanov
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
findit-for-me@appspot.gserviceaccount.com (Gerrit)
findit...@appspot.gserviceaccount.com voted Code-Coverage+1![]( "Open in Gerrit")
This change meets the code coverage requirements.
| Code-Coverage | +1 |
Related details
- Christoph Schwering
- Dave Tapuska
- Jan Keitel
- Richard (Torne) Coles
- Vasilii Sukhanov
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Vollick (Gerrit)
Ian Vollick added 1 comment![]( "Open in Gerrit")
std::move(callback_).Run(driver);I notice that the code coverage shows that these lines are not executed during the test. It looks like the passes here are bogus. I'm investigating.
Related details
- Christoph Schwering
- Dave Tapuska
- Jan Keitel
- Richard (Torne) Coles
- Vasilii Sukhanov
Code-Coverage
Code-Review
No-Unresolved-Comments
Christoph Schwering (Gerrit)
Christoph Schwering added 5 comments![]( "Open in Gerrit")
const bool defer_while_prerendering = base::FeatureList::IsEnabled(
autofill::features::kAutofillDeferDriverAgentCreationUntilActivation);nit: please add a `// TODO(crbug.com/342132628): Remove when launched.` or similar (we have or at least had this convention of having a bug explicitly mentioned with each features in autofill_features.cc)
// Called before post-prerender activation callbacks.
void DidActivatePrerenderingDocument(
bool did_dispatch_dom_content_loaded_event,
bool did_finish_load);A `RenderFrame` cannot go into prerendering mode twice over the course of its lifetime, and therefore it can be activated at most once, correct?
I'm asking because if not, I assume that `AwRenderFrameExt` needs to reset its `bool did_*`, and the `CHECK()` in `unsafe_autofill_driver()` is wrong.
// the page is activated. Applies to both autofill and password manager.nit: please add a `// TODO(crbug.com/342132628): Remove when launched.` or similar (we have or at least had this convention of having a bug explicitly mentioned with each features in autofill_features.cc)
autofill::features::kAutofillDeferDriverAgentCreationUntilActivation);nit: inline below and swap the order to enroll only prerendering clients?
- Dave Tapuska
- Ian Vollick
- Jan Keitel
- Richard (Torne) Coles
- Vasilii Sukhanov
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Vollick (Gerrit)
Ian Vollick added 5 comments![]( "Open in Gerrit")
const bool defer_while_prerendering = base::FeatureList::IsEnabled(
autofill::features::kAutofillDeferDriverAgentCreationUntilActivation);nit: please add a `// TODO(crbug.com/342132628): Remove when launched.` or similar (we have or at least had this convention of having a bug explicitly mentioned with each features in autofill_features.cc)
Sg. I've moved this to a helper (as you'd suggested for the agents) and put the TODO there. Please reopen if that doesn't fit the bill.
I notice that the code coverage shows that these lines are not executed during the test. It looks like the passes here are bogus. I'm investigating.
Looks like there were a couple of problems. First, in NavigatedMainFramePrerenderedPageActivation it seems that since the page wasn't really prerendering, we wouldn't defer so we'd happen to create the driver prior to activation. It still provides some coverage for the non-deferring case, but it is contrived in the deferring case. I've commented as such.
In ContentAutofillDriverTest_PrerenderBadMessage it looks like due to the bad message helper we don't die (I've checked that we hit the early out for prerender activation if we do this) so we can also check that activation of an actually-prerendered page also doesn't hit reset.
The other problem in ContentAutofillDriverTest_PrerenderBadMessage is that the creation observer is RAII and it falls out of scope in the if block. In this latest patch set I now stick it in a unique_ptr.
// Called before post-prerender activation callbacks.
void DidActivatePrerenderingDocument(
bool did_dispatch_dom_content_loaded_event,
bool did_finish_load);A `RenderFrame` cannot go into prerendering mode twice over the course of its lifetime, and therefore it can be activated at most once, correct?
I'm asking because if not, I assume that `AwRenderFrameExt` needs to reset its `bool did_*`, and the `CHECK()` in `unsafe_autofill_driver()` is wrong.
Yes, I think that's right. See this [transition diagram]( https://chromium.googlesource.com/chromium/src/+/HEAD/docs/render-frame-host-lifecycle-state.png) and the set of [allowed transitions](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.cc;l=16189;drc=cc5be7150eef183a1b9a6716d42a396ab7c59733).
@dtap...@chromium.org, @nhi...@chromium.org, plmk if that's wrong.
// the page is activated. Applies to both autofill and password manager.nit: please add a `// TODO(crbug.com/342132628): Remove when launched.` or similar (we have or at least had this convention of having a bug explicitly mentioned with each features in autofill_features.cc)
Done
autofill::features::kAutofillDeferDriverAgentCreationUntilActivation);nit: inline below and swap the order to enroll only prerendering clients?
- Christoph Schwering
- Dave Tapuska
findit-for-me@appspot.gserviceaccount.com (Gerrit)
findit...@appspot.gserviceaccount.com voted Code-Coverage+1![]( "Open in Gerrit")
This change meets the code coverage requirements.
| Code-Coverage | +1 |
Christoph Schwering (Gerrit)
Christoph Schwering added 1 comment![]( "Open in Gerrit")
const bool defer_while_prerendering = base::FeatureList::IsEnabled(
autofill::features::kAutofillDeferDriverAgentCreationUntilActivation);Ian Vollicknit: please add a `// TODO(crbug.com/342132628): Remove when launched.` or similar (we have or at least had this convention of having a bug explicitly mentioned with each features in autofill_features.cc)
Sg. I've moved this to a helper (as you'd suggested for the agents) and put the TODO there. Please reopen if that doesn't fit the bill.
Aah, I'm sorry, my comment ended up in the completely wrong place: I meant to leave the comment in `autofill_features.cc` above the definition of `kAutofillDeferDriverAgentCreationUntilActivation`.
Related details
- Dave Tapuska
- Ian Vollick
Ian Vollick (Gerrit)
Ian Vollick added 1 comment![]( "Open in Gerrit")
const bool defer_while_prerendering = base::FeatureList::IsEnabled(
autofill::features::kAutofillDeferDriverAgentCreationUntilActivation);Ian Vollicknit: please add a `// TODO(crbug.com/342132628): Remove when launched.` or similar (we have or at least had this convention of having a bug explicitly mentioned with each features in autofill_features.cc)
Christoph SchweringSg. I've moved this to a helper (as you'd suggested for the agents) and put the TODO there. Please reopen if that doesn't fit the bill.
Aah, I'm sorry, my comment ended up in the completely wrong place: I meant to leave the comment in `autofill_features.cc` above the definition of `kAutofillDeferDriverAgentCreationUntilActivation`.
Done. I've removed the comment from the function, though, since it'll clearly need to go once the feature is removed.
Related details
- Christoph Schwering
- Dave Tapuska
- Jan Keitel
- Richard (Torne) Coles
- Vasilii Sukhanov
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Vollick (Gerrit)
Ian Vollick voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ian Vollick
| Commit-Queue | +1 |
1 comment
// Called before post-prerender activation callbacks.
void DidActivatePrerenderingDocument(
bool did_dispatch_dom_content_loaded_event,
bool did_finish_load);Ian VollickA `RenderFrame` cannot go into prerendering mode twice over the course of its lifetime, and therefore it can be activated at most once, correct?
I'm asking because if not, I assume that `AwRenderFrameExt` needs to reset its `bool did_*`, and the `CHECK()` in `unsafe_autofill_driver()` is wrong.
Yes, I think that's right. See this [transition diagram]( https://chromium.googlesource.com/chromium/src/+/HEAD/docs/render-frame-host-lifecycle-state.png) and the set of [allowed transitions](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.cc;l=16189;drc=cc5be7150eef183a1b9a6716d42a396ab7c59733).
@dtap...@chromium.org, @nhi...@chromium.org, plmk if that's wrong.
IIUC, that resolves your question so I'm going to mark this comment as such (but please open if that's wrong).
Related details
- Christoph Schwering
- Dave Tapuska
- Jan Keitel
- Richard (Torne) Coles
- Vasilii Sukhanov
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
findit-for-me@appspot.gserviceaccount.com (Gerrit)
findit...@appspot.gserviceaccount.com voted Code-Coverage+1![]( "Open in Gerrit")
This change meets the code coverage requirements.
| Code-Coverage | +1 |
Related details
- Christoph Schwering
- Dave Tapuska
- Jan Keitel
- Richard (Torne) Coles
- Vasilii Sukhanov
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christoph Schwering (Gerrit)
Christoph Schwering added 7 comments![]( "Open in Gerrit")
Thanks a lot. Just a few more nits in the tests.
AutofillTestPrerendering() {nit: add a blank line
if (GetParam()) {nit: I think it's more readable if we have a one-liner
```
bool defer_driver_agent_creation_until_activation() const { return GetParam(); }
```
and call that instead of `GetParam()`? (also in some other test cases)
MockAutofillManager* mock = autofill_manager(rfh);
// Once the prerendered frame becomes active, we should get notified of any
// forms on the page, but we will drop other messages (eg focus) on the floor.
// If this was sent earlier, we would crash due to bad message checks
// in ContentAutofillDriver::FormsSeen.
struct {
Sequence seq;
base::RunLoop run_loop;
} on_forms_seen;
EXPECT_CALL(*mock, OnFormsSeen)
.InSequence(on_forms_seen.seq)
.WillOnce(InvokeClosure(on_forms_seen.run_loop.QuitClosure()));
if (!GetParam()) {
// If we're not deferring creation, we will get the message for the
// programmatic focus (this will be dropped on the floor, otherwise.
struct {
Sequence seq;
base::RunLoop run_loop;
} on_focus_on_form_field_impl;
EXPECT_CALL(*mock, OnFocusOnFormFieldImpl)
.InSequence(on_focus_on_form_field_impl.seq)
.WillOnce(
InvokeClosure(on_focus_on_form_field_impl.run_loop.QuitClosure()));
on_focus_on_form_field_impl.run_loop.Run();
}The purpose of `on_forms_seen`, `on_focus_on_form_field_impl` and their `Sequence`s is to avoid UB as per the [GMock documentation](https://google.github.io/googletest/gmock_cook_book.html), which says:
Setting expectations after code that exercises the mock has undefined behavior.
I suspect the new version of the test is UB.
Would it suffice to only move the `on_focus_on_form_field_impl.run_loop.Run()` call down here?
Admittedly we have tons of this kind of UB in other Autofill tests. But I think we shouldn't mix the UB-preventing style with the UB style.
INSTANTIATE_TEST_SUITE_P(All,nit: I think `ContentAutofillDriverTest` is preferable for prefix matching the tests (otherwise they're listed as `All/PrerenderingContentAutofillDriverTest.NavigatedMainFramePrerenderedPageActivation/0` etc., IIRC)
::autofill::features::nit: remove (also elsewhere)
// Called before post-prerender activation callbacks.
void DidActivatePrerenderingDocument(
bool did_dispatch_dom_content_loaded_event,
bool did_finish_load);Ian VollickA `RenderFrame` cannot go into prerendering mode twice over the course of its lifetime, and therefore it can be activated at most once, correct?
I'm asking because if not, I assume that `AwRenderFrameExt` needs to reset its `bool did_*`, and the `CHECK()` in `unsafe_autofill_driver()` is wrong.
Ian VollickYes, I think that's right. See this [transition diagram]( https://chromium.googlesource.com/chromium/src/+/HEAD/docs/render-frame-host-lifecycle-state.png) and the set of [allowed transitions](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.cc;l=16189;drc=cc5be7150eef183a1b9a6716d42a396ab7c59733).
@dtap...@chromium.org, @nhi...@chromium.org, plmk if that's wrong.
IIUC, that resolves your question so I'm going to mark this comment as such (but please open if that's wrong).
Great, thank you. Apologies for the piecewise review.
Related details
- Dave Tapuska
- Ian Vollick
- Jan Keitel
- Richard (Torne) Coles
- Vasilii Sukhanov
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jan Keitel (Gerrit)
Jan Keitel added 7 comments![]( "Open in Gerrit")
Thank you for all the work on this, Ian!
public ::testing::WithParamInterface<bool> {Optional nit: Use `base::test::WithFeatureOverride`?
factory_->AddObserver(this);Optional nit, since test: Use `ScopedObservation`?
public ::testing::WithParamInterface<bool> {Optional nit: Use `base::test::WithFeatureOverride`?
if (!ShouldDeferCreationUntilPrerenderActivation()) {
return;
}Should this be a `CHECK` instead? AFAIK this can never happen?
if (!ShouldDeferCreationUntilPrerenderActivation()) {
return;
}Same question as above: Make this a `CHECK`?
if (did_dispatch_dom_content_loaded_event) {
DidDispatchDOMContentLoadedEvent();
}
if (did_finish_load) {
DidFinishLoad();
}If I look at the code below, it seems that sending both events is overkill. IIUC `SendPasswordForm(false)` sends a superset of the forms that `SendPasswordForms(true)` sends.
Another question: Is the ordering of the two events always guaranteed? If so, how about passing an enum instead of a bool?
Related details
- Dave Tapuska
- Ian Vollick
Ian Vollick (Gerrit)
Ian Vollick added 11 comments![]( "Open in Gerrit")
Optional nit: Use `base::test::WithFeatureOverride`?
Done
nit: add a blank line
Done
nit: I think it's more readable if we have a one-liner
```
bool defer_driver_agent_creation_until_activation() const { return GetParam(); }
```
and call that instead of `GetParam()`? (also in some other test cases)
Done
MockAutofillManager* mock = autofill_manager(rfh);
// Once the prerendered frame becomes active, we should get notified of any
// forms on the page, but we will drop other messages (eg focus) on the floor.
// If this was sent earlier, we would crash due to bad message checks
// in ContentAutofillDriver::FormsSeen.
struct {
Sequence seq;
base::RunLoop run_loop;
} on_forms_seen;
EXPECT_CALL(*mock, OnFormsSeen)
.InSequence(on_forms_seen.seq)
.WillOnce(InvokeClosure(on_forms_seen.run_loop.QuitClosure()));
if (!GetParam()) {
// If we're not deferring creation, we will get the message for the
// programmatic focus (this will be dropped on the floor, otherwise.
struct {
Sequence seq;
base::RunLoop run_loop;
} on_focus_on_form_field_impl;
EXPECT_CALL(*mock, OnFocusOnFormFieldImpl)
.InSequence(on_focus_on_form_field_impl.seq)
.WillOnce(
InvokeClosure(on_focus_on_form_field_impl.run_loop.QuitClosure()));
on_focus_on_form_field_impl.run_loop.Run();
}The purpose of `on_forms_seen`, `on_focus_on_form_field_impl` and their `Sequence`s is to avoid UB as per the [GMock documentation](https://google.github.io/googletest/gmock_cook_book.html), which says:
Setting expectations after code that exercises the mock has undefined behavior.
I suspect the new version of the test is UB.
Would it suffice to only move the `on_focus_on_form_field_impl.run_loop.Run()` call down here?
Admittedly we have tons of this kind of UB in other Autofill tests. But I think we shouldn't mix the UB-preventing style with the UB style.
Good point -- thanks for flagging. In order to set up the expectations before using the mocks and wait on them later, I tried to apply the expectations right as the mocks were being created. To do this, I made it possible to observe the injector. Hopefully this addresses the concern of setting expectations after interacting with mocks, but plmk and reopen, if not or if I've got this wrong.
Optional nit, since test: Use `ScopedObservation`?
Done
nit: I think `ContentAutofillDriverTest` is preferable for prefix matching the tests (otherwise they're listed as `All/PrerenderingContentAutofillDriverTest.NavigatedMainFramePrerenderedPageActivation/0` etc., IIRC)
Done
Optional nit: Use `base::test::WithFeatureOverride`?
Done
::autofill::features::Ian Vollicknit: remove (also elsewhere)
Done
if (!ShouldDeferCreationUntilPrerenderActivation()) {
return;
}Should this be a `CHECK` instead? AFAIK this can never happen?
Done
if (!ShouldDeferCreationUntilPrerenderActivation()) {
return;
}Same question as above: Make this a `CHECK`?
Done
if (did_dispatch_dom_content_loaded_event) {
DidDispatchDOMContentLoadedEvent();
}
if (did_finish_load) {
DidFinishLoad();
}If I look at the code below, it seems that sending both events is overkill. IIUC `SendPasswordForm(false)` sends a superset of the forms that `SendPasswordForms(true)` sends.
Another question: Is the ordering of the two events always guaranteed? If so, how about passing an enum instead of a bool?
It looks like it's not quite a superset. We send rendered or parsed messages depending on the boolean parameter [here](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/content/renderer/password_autofill_agent.cc;l=1385;drc=3e02eb658e84b46ebb53667e0a1cb52fe53b5f87). But even if it was the case that one of the Did fns was a superset of the other, I think I'd still prefer to call both methods so that we don't have an unexpected breakage in the future if these functions change.
As for the ordering of events, yes, it looks like like the ordering is guaranteed, ShouldComplete() guards the fn in Document where the load finish event is triggered and is [blocked on being at kFinishedParsing](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/document.cc;l=4036;drc=3e02eb658e84b46ebb53667e0a1cb52fe53b5f87).
In my latest patch set I've switched to an enum and added some checks that the order is as expected in chrome_content_renderer_client.cc
Related details
- Christoph Schwering
- Dave Tapuska
- Jan Keitel
- Richard (Torne) Coles
- Vasilii Sukhanov
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
findit-for-me@appspot.gserviceaccount.com (Gerrit)
findit...@appspot.gserviceaccount.com voted Code-Coverage+1![]( "Open in Gerrit")
This change meets the code coverage requirements.
| Code-Coverage | +1 |
Jan Keitel (Gerrit)
Jan Keitel added 6 comments![]( "Open in Gerrit")
Thank you Ian - just a few more nits below.
base::test::ScopedFeatureList scoped_features_;Remove?
#include "autofill_agent.h"Please remove - is that `clangd` going rogue? I've recently noticed the same in my setup.
if (events_to_dispatch >=Nit, happy to be overruled: How do you feel about just doing an explicit switch, even if that duplicates the line that call `DidDispatchDOMContentLoadedEvent`? This feels a little fragile in case somebody changes the enum order.
if (did_dispatch_dom_content_loaded_event) {
DidDispatchDOMContentLoadedEvent();
}
if (did_finish_load) {
DidFinishLoad();
}Ian VollickIf I look at the code below, it seems that sending both events is overkill. IIUC `SendPasswordForm(false)` sends a superset of the forms that `SendPasswordForms(true)` sends.
Another question: Is the ordering of the two events always guaranteed? If so, how about passing an enum instead of a bool?
It looks like it's not quite a superset. We send rendered or parsed messages depending on the boolean parameter [here](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/content/renderer/password_autofill_agent.cc;l=1385;drc=3e02eb658e84b46ebb53667e0a1cb52fe53b5f87). But even if it was the case that one of the Did fns was a superset of the other, I think I'd still prefer to call both methods so that we don't have an unexpected breakage in the future if these functions change.
As for the ordering of events, yes, it looks like like the ordering is guaranteed, ShouldComplete() guards the fn in Document where the load finish event is triggered and is [blocked on being at kFinishedParsing](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/document.cc;l=4036;drc=3e02eb658e84b46ebb53667e0a1cb52fe53b5f87).
In my latest patch set I've switched to an enum and added some checks that the order is as expected in chrome_content_renderer_client.cc
Ack, you're right - let's leave the PWM calls as they are (or at least for a later cleanup by somebody on our team). Thank you for moving to the enum!
Related details
- Christoph Schwering
- Dave Tapuska
- Ian Vollick
Ian Vollick (Gerrit)
Ian Vollick added 5 comments![]( "Open in Gerrit")
base::test::ScopedFeatureList scoped_features_;Ian VollickRemove?
Done
#include "autofill_agent.h"Ian VollickRemove.
Done. Not sure how I got these strange includes.
Please remove - is that `clangd` going rogue? I've recently noticed the same in my setup.
Done. I'm not sure - I'd like to know the cause, too.
Nit, happy to be overruled: How do you feel about just doing an explicit switch, even if that duplicates the line that call `DidDispatchDOMContentLoadedEvent`? This feels a little fragile in case somebody changes the enum order.
- Christoph Schwering
- Dave Tapuska
- Jan Keitel
- Richard (Torne) Coles
- Vasilii Sukhanov
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christoph Schwering (Gerrit)
Christoph Schwering added 11 comments![]( "Open in Gerrit")
Thanks! One final round of nits from my side :-)
raw_ptr<AutofillTestPrerendering> owner_;nit: `raw_ref`?
autofill_manager_injector_observer_(this) {nit: you can initialize this one at the declaration with
```
MockAutofillManagerInjectorObserver autofill_manager_injector_observer_{this};
```
std::unique_ptr<SequenceAndRunLoop> on_forms_seen_;
std::unique_ptr<SequenceAndRunLoop> on_focus_on_form_field_impl_;Does it make a difference if these are `std::unique_ptr<SequenceAndRunLoop>` instead of just `SequenceAndRunLoop`?
autofill::features::kAutofillDeferDriverAgentCreationUntilActivation);nit: remove
std::unique_ptr<ContentAutofillDriverCreationObserver> creation_observer;nitty nit: as per go/totw/123 I think this should be `optional`. The initialization could then be come
```
creation_observer.emplace(factory(), base::BindOnce(...));
```
#include "autofill_agent.h"nit: incomplete path
autofill::features::kAutofillDeferDriverAgentCreationUntilActivation);nit: remove
default:nit: remove? We probably want the compiler to tell us when the enum changes.
CHECK(!(is_prerendering && !ShouldDeferCreationUntilPrerenderActivation()));That exclamation mark is wrong, isn't it?
Related details
- Dave Tapuska
- Ian Vollick
- Jan Keitel
- Richard (Torne) Coles
- Vasilii Sukhanov
Code-Coverage
Code-Review
No-Unresolved-Comments
Jan Keitel (Gerrit)
Jan Keitel voted and added 2 comments![]( "Open in Gerrit")
Votes added by Jan Keitel
| Code-Review | +1 |
2 comments
LGTM % Chris' comment on the switch and the PWM check. 😊
Thank you, Ian!
default:nit: remove? We probably want the compiler to tell us when the enum changes.
- Christoph Schwering
- Dave Tapuska
- Ian Vollick
- Richard (Torne) Coles
- Vasilii Sukhanov
Code-Coverage
Code-Review
findit-for-me@appspot.gserviceaccount.com (Gerrit)
findit...@appspot.gserviceaccount.com voted Code-Coverage+1![]( "Open in Gerrit")
This change meets the code coverage requirements.
- Dave Tapuska
Dave Tapuska (Gerrit)
Dave Tapuska voted Code-Review+1![]( "Open in Gerrit")
- Ian Vollick
Ian Vollick (Gerrit)
Ian Vollick added 10 comments![]( "Open in Gerrit")
raw_ptr<AutofillTestPrerendering> owner_;Ian Vollicknit: `raw_ref`?
Done
nit: you can initialize this one at the declaration with
```
MockAutofillManagerInjectorObserver autofill_manager_injector_observer_{this};
```
Done
std::unique_ptr<SequenceAndRunLoop> on_forms_seen_;
std::unique_ptr<SequenceAndRunLoop> on_focus_on_form_field_impl_;Does it make a difference if these are `std::unique_ptr<SequenceAndRunLoop>` instead of just `SequenceAndRunLoop`?
Yes, the run loops need to be constructed on the right thread. They could be optionals, though, in line with your other comment. I've updated in this latest patch set.
autofill::features::kAutofillDeferDriverAgentCreationUntilActivation);Ian Vollicknit: remove
Done
std::unique_ptr<ContentAutofillDriverCreationObserver> creation_observer;nitty nit: as per go/totw/123 I think this should be `optional`. The initialization could then be come
```
creation_observer.emplace(factory(), base::BindOnce(...));
```
Done
#include "autofill_agent.h"Ian Vollicknit: incomplete path
Jan commented on this as well (and Vasilii [noted this on a previous CL](https://chromium-review.googlesource.com/c/chromium/src/+/5647771/comment/fa3f7121_a197b4da/)). I'm not sure what's happening with these includes. It's removed now.
autofill::features::kAutofillDeferDriverAgentCreationUntilActivation);Ian Vollicknit: remove
Done
Jan Keitelnit: remove? We probably want the compiler to tell us when the enum changes.
+1
Done
CHECK(!(is_prerendering && !ShouldDeferCreationUntilPrerenderActivation()));That exclamation mark is wrong, isn't it?
Oof, you're right. Fixed (and I switched the logic to match content_autofill_driver.cc). I was concerned about this not being tripped in tests, so I checked manually in chrome (prerendering a site with passwords) and it didn't fail regardless of how I flipped the feature setting. It seems function isn't called during prerendering.
Related details
- Christoph Schwering
- Richard (Torne) Coles
- Vasilii Sukhanov
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
findit-for-me@appspot.gserviceaccount.com (Gerrit)
findit...@appspot.gserviceaccount.com voted Code-Coverage+1![]( "Open in Gerrit")
This change meets the code coverage requirements.
| Code-Coverage | +1 |
Christoph Schwering (Gerrit)
Christoph Schwering voted and added 1 comment![]( "Open in Gerrit")
Votes added by Christoph Schwering
| Code-Review | +1 |
1 comment
LGTM, thank you very much for building this!
Related details
- Ian Vollick
Richard (Torne) Coles (Gerrit)
Richard (Torne) Coles voted and added 1 comment![]( "Open in Gerrit")
Votes added by Richard (Torne) Coles
| Code-Review | +1 |
1 comment
//android_webview LGTM (sorry for slow review, was catching up after being sick)
Related details
- Ian Vollick
- Vasilii Sukhanov
Ian Vollick (Gerrit)
Ian Vollick added 1 comment![]( "Open in Gerrit")
CHECK(!(is_prerendering && !ShouldDeferCreationUntilPrerenderActivation()));Ian VollickThat exclamation mark is wrong, isn't it?
Oof, you're right. Fixed (and I switched the logic to match content_autofill_driver.cc). I was concerned about this not being tripped in tests, so I checked manually in chrome (prerendering a site with passwords) and it didn't fail regardless of how I flipped the feature setting. It seems function isn't called during prerendering.
I should say, though, that I may be missing cases. I'm only basing my comment here on my very minimal manual testing and the test cases. This could certainly be incomplete. I'm currently reviewing calls to ContentPasswordManagerDriver::GetForRenderFrameHost to check. I'm unresolving this thread while I look through this.
Related details
- Christoph Schwering
- Vasilii Sukhanov
Code-Coverage
Code-Review
No-Unresolved-Comments
Ian Vollick (Gerrit)
Ian Vollick added 1 comment![]( "Open in Gerrit")
CHECK(!(is_prerendering && !ShouldDeferCreationUntilPrerenderActivation()));Ian VollickThat exclamation mark is wrong, isn't it?
Ian VollickOof, you're right. Fixed (and I switched the logic to match content_autofill_driver.cc). I was concerned about this not being tripped in tests, so I checked manually in chrome (prerendering a site with passwords) and it didn't fail regardless of how I flipped the feature setting. It seems function isn't called during prerendering.
I should say, though, that I may be missing cases. I'm only basing my comment here on my very minimal manual testing and the test cases. This could certainly be incomplete. I'm currently reviewing calls to ContentPasswordManagerDriver::GetForRenderFrameHost to check. I'm unresolving this thread while I look through this.
I've dug through callsites. I may still be missing things, but it didn't look like this would be called during prerendering with this patch applied. I'm going to resolve this, but plmk if you'd like me to do more analysis.
Related details
- Vasilii Sukhanov
Code-Coverage
Code-Review
Vasilii Sukhanov (Gerrit)
Vasilii Sukhanov voted Code-Review+1![]( "Open in Gerrit")
- Ian Vollick
Ian Vollick (Gerrit)
Ian Vollick added 1 comment![]( "Open in Gerrit")
Thanks, all for the reviews. In this latest patchset, I've updated the autofill browsertest to have the same coverage as before in the non-deferring case (i.e., we set up expectations of zero calls prior to activation, trigger a checkpoint, and then continue to run). In the deferring case, I've added a check for the non-existence of the driver. I've also updated comments in both the autofill and password manager browsertests to note a case where the checks would be incorrect: the time after we finish checking for no-calls-prior-to-activation and before the prerendered page activation we then trigger.
PTAL when you have a moment: https://crrev.com/c/5630210/52..54
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
findit-for-me@appspot.gserviceaccount.com (Gerrit)
findit...@appspot.gserviceaccount.com voted Code-Coverage+1![]( "Open in Gerrit")
This change meets the code coverage requirements.
| Code-Coverage | +1 |
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |