Implement showing the opt in UI in attempt otp filling tool [chromium/src : main]
Trang Mai (Gerrit)
Trang Mai added 1 comment![]( "Open in Gerrit")
The test failures are related to the change.
Related details
- Stephan Drab
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. |
Stephan Drab (Gerrit)
Stephan Drab added 1 comment![]( "Open in Gerrit")
The test failures are related to the change.
- Trang Mai
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. |
Salma Aly (Gerrit)
Salma Aly added 1 comment![]( "Open in Gerrit")
// Consume the context. The service clears its state and stops observing.
std::optional<autofill::ActorLoginContext> context =
tool_delegate()
.GetActorOneTimeTokenFillingService()
.ConsumeLoginContext();
if (context.has_value() &&
IsActorLoginFlow(GetTargetTab(), trigger_fields_, *context)) {
// Verified sign-in journey: proceed with silent OTP filling.
// TODO(crbug.com/504573041): Implement
} else {
// No recent login, origin mismatch, untracked frame, or sequence broken
// by too many navigations: require confirmation UI (Post-MVP).
// TODO(crbug.com/504573041): Implement
}This logic is about whether we will show a confirmation UI or not. I think this should come after we show/check the opt-in UI.
Therefore, I think we should move this AFTER the opt-in UI check. Currently in line 236 we call retrieve, instead we should do this check here (or maybe optimise it? run the `IsActorLoginFlow` which the dialog do it's thing? can be an optimisation in another CL), the `IsActorLoginFlow` check will call retrieve. Also keep in mind that in https://chromium-review.git.corp.google.com/c/chromium/src/+/8026692 this logic is being changed 😄 so one of you will suffer while solving merge conflicts 😐
Related details
- Stephan Drab
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. |
Salma Aly (Gerrit)
Salma Aly added 1 comment![]( "Open in Gerrit")
// Consume the context. The service clears its state and stops observing.
std::optional<autofill::ActorLoginContext> context =
tool_delegate()
.GetActorOneTimeTokenFillingService()
.ConsumeLoginContext();
if (context.has_value() &&
IsActorLoginFlow(GetTargetTab(), trigger_fields_, *context)) {
// Verified sign-in journey: proceed with silent OTP filling.
// TODO(crbug.com/504573041): Implement
} else {
// No recent login, origin mismatch, untracked frame, or sequence broken
// by too many navigations: require confirmation UI (Post-MVP).
// TODO(crbug.com/504573041): Implement
}This logic is about whether we will show a confirmation UI or not. I think this should come after we show/check the opt-in UI.
Therefore, I think we should move this AFTER the opt-in UI check. Currently in line 236 we call retrieve, instead we should do this check here (or maybe optimise it? run the `IsActorLoginFlow` which the dialog do it's thing? can be an optimisation in another CL), the `IsActorLoginFlow` check will call retrieve. Also keep in mind that in https://chromium-review.git.corp.google.com/c/chromium/src/+/8026692 this logic is being changed 😄 so one of you will suffer while solving merge conflicts 😐
you can see in https://chromium-review.git.corp.google.com/c/chromium/src/+/8034083 the retrieve is called after the check
Let me know if a GVC to discuss this is a good idea :)
Stephan Drab (Gerrit)
Stephan Drab added 2 comments![]( "Open in Gerrit")
Stephan DrabThe test failures are related to the change.
The test ran fine on my local machine, investigating... 🤔
Should be fixed now.
// Consume the context. The service clears its state and stops observing.
std::optional<autofill::ActorLoginContext> context =
tool_delegate()
.GetActorOneTimeTokenFillingService()
.ConsumeLoginContext();
if (context.has_value() &&
IsActorLoginFlow(GetTargetTab(), trigger_fields_, *context)) {
// Verified sign-in journey: proceed with silent OTP filling.
// TODO(crbug.com/504573041): Implement
} else {
// No recent login, origin mismatch, untracked frame, or sequence broken
// by too many navigations: require confirmation UI (Post-MVP).
// TODO(crbug.com/504573041): Implement
}Salma AlyThis logic is about whether we will show a confirmation UI or not. I think this should come after we show/check the opt-in UI.
Therefore, I think we should move this AFTER the opt-in UI check. Currently in line 236 we call retrieve, instead we should do this check here (or maybe optimise it? run the `IsActorLoginFlow` which the dialog do it's thing? can be an optimisation in another CL), the `IsActorLoginFlow` check will call retrieve. Also keep in mind that in https://chromium-review.git.corp.google.com/c/chromium/src/+/8026692 this logic is being changed 😄 so one of you will suffer while solving merge conflicts 😐
you can see in https://chromium-review.git.corp.google.com/c/chromium/src/+/8034083 the retrieve is called after the check
Let me know if a GVC to discuss this is a good idea :)
That makes sense, changed it. Thanks for the heads up for Oleksandr's change!
Related details
- Salma Aly
- Trang Mai
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Salma Aly (Gerrit)
Salma Aly added 1 comment![]( "Open in Gerrit")
// Consume the context. The service clears its state and stops observing.
std::optional<autofill::ActorLoginContext> context =
tool_delegate()
.GetActorOneTimeTokenFillingService()
.ConsumeLoginContext();
if (context.has_value() &&
IsActorLoginFlow(GetTargetTab(), trigger_fields_, *context)) {
// Verified sign-in journey: proceed with silent OTP filling.
// TODO(crbug.com/504573041): Implement
} else {
// No recent login, origin mismatch, untracked frame, or sequence broken
// by too many navigations: require confirmation UI (Post-MVP).
// TODO(crbug.com/504573041): Implement
}Salma AlyThis logic is about whether we will show a confirmation UI or not. I think this should come after we show/check the opt-in UI.
Therefore, I think we should move this AFTER the opt-in UI check. Currently in line 236 we call retrieve, instead we should do this check here (or maybe optimise it? run the `IsActorLoginFlow` which the dialog do it's thing? can be an optimisation in another CL), the `IsActorLoginFlow` check will call retrieve. Also keep in mind that in https://chromium-review.git.corp.google.com/c/chromium/src/+/8026692 this logic is being changed 😄 so one of you will suffer while solving merge conflicts 😐
Stephan Drabyou can see in https://chromium-review.git.corp.google.com/c/chromium/src/+/8034083 the retrieve is called after the check
Let me know if a GVC to discuss this is a good idea :)
That makes sense, changed it. Thanks for the heads up for Oleksandr's change!
- Stephan Drab
- Trang Mai
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Bokan (Gerrit)
David Bokan added 2 comments![]( "Open in Gerrit")
base::Time dismissal_timestamp =
autofill::prefs::GetAutofillGmailOtpFillingActivationDismissalTimestamp(
prefs);
if (dismissal_timestamp != base::Time() &&
(base::Time::Now() - dismissal_timestamp < base::Days(90))) {
LOG(INFO) << "Within cool off period => Don't show opt-in card";
std::move(callback).Run(
MakeResult(mojom::ActionResultCode::kFormFillingAutofillUnavailable,
/*requires_page_stabilization=*/false,
"Within 90-day cool-off period."));
return;
}
LOG(INFO) << "Outside of cool off period => Show Gmail OTP opt-in card";
tool_delegate().RequestToShowGmailOtpOptInDialog(
base::BindOnce(&AttemptOtpFillingTool::OnGmailOtpOptInResponse,
weak_factory_.GetWeakPtr(), std::move(callback)));This seems like it would fit better in the Validate method (we're checking a precondition of using the tool). Invoke should only contain the actual tool logic and only be called one we know all preconditions have been met.
LOG(INFO) << "Retrieved Gmail OTP is empty (tool timeout)";Chrome shouldn't have any LOGs in production. If you want/need this use VLOG instead. That said, most logging should happen through the actor journal, e.g. https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/actor/tools/navigate_tool.cc;l=127;drc=63ec5d57642d302b1f2be8207a6672edd989c034
Related details
- Stephan Drab
- Trang Mai
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. |
Trang Mai (Gerrit)
Trang Mai added 2 comments![]( "Open in Gerrit")
// TODO(crbug.com/511944669): In the case of !is_permission_granted(), theThis bug is marked as fixed, did you mean to leave some other bug number here?
if (response && response->is_permission_granted() &&If the dialog fails to display or if a Mojo channel error occurs, this would be false and we'll SetAutofillGmailOtpFillingActivationDismissalTimestamp even though the user did not see the dialog. Maybe we need to handle that case differently.
Related details
- Stephan Drab
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. |
Stephan Drab (Gerrit)
Stephan Drab added 4 comments![]( "Open in Gerrit")
base::Time dismissal_timestamp =
autofill::prefs::GetAutofillGmailOtpFillingActivationDismissalTimestamp(
prefs);
if (dismissal_timestamp != base::Time() &&
(base::Time::Now() - dismissal_timestamp < base::Days(90))) {
LOG(INFO) << "Within cool off period => Don't show opt-in card";
std::move(callback).Run(
MakeResult(mojom::ActionResultCode::kFormFillingAutofillUnavailable,
/*requires_page_stabilization=*/false,
"Within 90-day cool-off period."));
return;
}
LOG(INFO) << "Outside of cool off period => Show Gmail OTP opt-in card";
tool_delegate().RequestToShowGmailOtpOptInDialog(
base::BindOnce(&AttemptOtpFillingTool::OnGmailOtpOptInResponse,
weak_factory_.GetWeakPtr(), std::move(callback)));This seems like it would fit better in the Validate method (we're checking a precondition of using the tool). Invoke should only contain the actual tool logic and only be called one we know all preconditions have been met.
Hi David, thanks for the hint! I changed the implementation accordingly and updated the tests.
// TODO(crbug.com/511944669): In the case of !is_permission_granted(), theThis bug is marked as fixed, did you mean to leave some other bug number here?
Removed the comment altogether
If the dialog fails to display or if a Mojo channel error occurs, this would be false and we'll SetAutofillGmailOtpFillingActivationDismissalTimestamp even though the user did not see the dialog. Maybe we need to handle that case differently.
You're right. I had the same thought and added error handling for this and changed this implementation already!
LOG(INFO) << "Retrieved Gmail OTP is empty (tool timeout)";Chrome shouldn't have any LOGs in production. If you want/need this use VLOG instead. That said, most logging should happen through the actor journal, e.g. https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/actor/tools/navigate_tool.cc;l=127;drc=63ec5d57642d302b1f2be8207a6672edd989c034
- Dave Tapuska
- Trang Mai
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Trang Mai (Gerrit)
Trang Mai voted and added 2 comments![]( "Open in Gerrit")
Votes added by Trang Mai
| Code-Review | +1 |
2 comments
response->is_permission_granted())
.Add("is_error_reason", response->is_error_reason())`response` can be null, so response->is_permission_granted() and response->is_error_reason() can cause NPE
bool opt_in_permission_granted = response->get_permission_granted();Please fix this WARNING reported by autoreview issue finding: This seems redundant. The `if` block above (lines 228-245) already handles the case where permission is NOT granted. If execution reaches here, `response->get_permission_granted()` must be true. The `if (!opt_in_permission_granted)` block on line 257 will never be entered.
Related details
- Dave Tapuska
- Stephan Drab
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. |
Dave Tapuska (Gerrit)
Dave Tapuska voted and added 3 comments![]( "Open in Gerrit")
Votes added by Dave Tapuska
| Code-Review | +1 |
3 comments
std::vector<mojom::JournalDetailsPtr> journal_details = {});Every call used here, passes the details in, so there is no need to pass a default initializer. Also you can then use the -forward mojom header for the actor_types if you do that.
#include "chrome/common/actor_webui.mojom.h"-forward? (we try not to include the full mojom.h headers in headers).
bool within_cool_off_period = time_since_last_dismissal < base::Days(90);constants should be labeled constants in the annonymous namespace.
Related details
- Stephan Drab
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. |
Stephan Drab (Gerrit)
Stephan Drab added 5 comments![]( "Open in Gerrit")
std::vector<mojom::JournalDetailsPtr> journal_details = {});Every call used here, passes the details in, so there is no need to pass a default initializer. Also you can then use the -forward mojom header for the actor_types if you do that.
Thanks! Done
-forward? (we try not to include the full mojom.h headers in headers).
Thanks for the hint! Done
bool within_cool_off_period = time_since_last_dismissal < base::Days(90);constants should be labeled constants in the annonymous namespace.
Done
response->is_permission_granted())
.Add("is_error_reason", response->is_error_reason())`response` can be null, so response->is_permission_granted() and response->is_error_reason() can cause NPE
Done
bool opt_in_permission_granted = response->get_permission_granted();Please fix this WARNING reported by autoreview issue finding: This seems redundant. The `if` block above (lines 228-245) already handles the case where permission is NOT granted. If execution reaches here, `response->get_permission_granted()` must be true. The `if (!opt_in_permission_granted)` block on line 257 will never be entered.
I think this is not true. The block above checks using `is_....` whether the response is a permission granted response. If is't a permission granted response, the method `get_permission_granted` retrieves the value of the permission granted field (`true` or `false`). See unit tests `ValidateOptInPermissionGrantedCallbackSucceedsAndEnablesOptInAndResetsDismissalTimestamp` and `ValidateOptInPermissionDeniedCallbackFailsAndDoesNotEnableOptInAndUpdatesDismissalTimestamp` that successfully execute the two code paths.
Related details
- Dave Tapuska
- Trang Mai
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Battré (Gerrit)
Dominic Battré added 6 comments![]( "Open in Gerrit")
Looking good, but maybe you can take a pass as the tests.
ValidateOptInPermissionGrantedCallbackSucceedsAndEnablesOptInAndResetsDismissalTimestamp) {lol
YouCertainlyCannotDenyYourJavaBackgroundFromPreviousProjects_ITypicallyUseAShorterTestCaseNamenAndPutTheDescriptionInACommentAsThatIsEasierToRead_AlsoTheGrammaticalStructureIsABitHardToParseAndIWonderIfOurToolingDoesWellWithThis_YouCertainlyCannotDisableThisTestAndWriteACLTitleWithTheTestNameThatIsLessThan72Characters :-)
EXPECT_CALL(delegate(), RequestToShowGmailOtpOptInDialog(_))nit: you can drop this if you don't validate any parameters.
TEST_F(
AttemptOtpFillingToolTest,
ValidateOptInPermissionGrantedCallbackSucceedsAndEnablesOptInAndResetsDismissalTimestamp) {
EXPECT_CALL(delegate(), RequestToShowGmailOtpOptInDialog(_))
.WillOnce(base::test::RunOnceCallback<0>(
webui::mojom::GmailOtpOptInResult::NewPermissionGranted(true)));
AttemptOtpFillingTool tool(TaskId(1), delegate(), mock_tab().GetHandle(),
{PageTarget(gfx::Point(10, 10))},
/*for_signin=*/true);
autofill::prefs::SetAutofillGmailOtpFillingEnabled(prefs(), false);
base::test::TestFuture<mojom::ActionResultPtr> future;
tool.Validate(future.GetCallback());
mojom::ActionResultPtr result = future.Take();
EXPECT_EQ(mojom::ActionResultCode::kOk, result->code);
EXPECT_TRUE(autofill::prefs::IsAutofillGmailOtpFillingEnabled(prefs()));
EXPECT_EQ(
base::Time(),
autofill::prefs::GetAutofillGmailOtpFillingActivationDismissalTimestamp(
prefs()));
}I think that a few modifications would make the tests easier to maintain in the future. Here is a suggestion.
```suggestion
namespace {
using autofill::prefs::SetAutofillGmailOtpFillingEnabled;
using autofill::prefs::IsAutofillGmailOtpFillingEnabled;
using autofill::prefs::GetAutofillGmailOtpFillingActivationDismissalTimestamp;
using base::test::TestFuture;
using webui::mojom::GmailOtpOptInResult;
}
// Test the case that user grants permission to access GMailOTPs during
// the validation of the tool. The decision should be persisted in prefs.
TEST_F(AttemptOtpFillingToolTest, UserApprovesGmailOtpOptIn) {
// Simulate that request to opt in to GmailOTPs is approved.
EXPECT_CALL(delegate(), RequestToShowGmailOtpOptInDialog)
.WillOnce(base::test::RunOnceCallback<0>(
GmailOtpOptInResult::NewPermissionGranted(true)));
// Disable GMailOtp setting.
SetAutofillGmailOtpFillingEnabled(prefs(), false);
// Simulate use of calling the AttemptOtpFillingTool.
AttemptOtpFillingTool tool(TaskId(1), delegate(), mock_tab().GetHandle(),
{PageTarget(gfx::Point(10, 10))},
/*for_signin=*/true);
TestFuture<mojom::ActionResultPtr> future;
tool.Validate(future.GetCallback());
// Validate expectations.
mojom::ActionResultPtr result = future.Take();
EXPECT_EQ(mojom::ActionResultCode::kOk, result->code);
EXPECT_TRUE(IsAutofillGmailOtpFillingEnabled(prefs()));
EXPECT_EQ(
base::Time(),
GetAutofillGmailOtpFillingActivationDismissalTimestamp(prefs()));
}
```
ValidateOptInPermissionDeniedCallbackFailsAndDoesNotEnableOptInAndUpdatesDismissalTimestamp) {FindingTheDeltaToThePreviousStringIsABitLikeWhereIsWaldo
Related details
- Dave Tapuska
- Stephan Drab
- Trang Mai
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. |
Stephan Drab (Gerrit)
Stephan Drab added 5 comments![]( "Open in Gerrit")
ValidateOptInPermissionGrantedCallbackSucceedsAndEnablesOptInAndResetsDismissalTimestamp) {Stephan Drablol
YouCertainlyCannotDenyYourJavaBackgroundFromPreviousProjects_ITypicallyUseAShorterTestCaseNamenAndPutTheDescriptionInACommentAsThatIsEasierToRead_AlsoTheGrammaticalStructureIsABitHardToParseAndIWonderIfOurToolingDoesWellWithThis_YouCertainlyCannotDisableThisTestAndWriteACLTitleWithTheTestNameThatIsLessThan72Characters :-)
Thanks for the comment! 😂
I changed the test names to the pattern `When_Given` and omits the `Then` part go/unit-testing-practices?polyglot=cpp#behavior-testing without strictly enforcing the 72 char limit.
EXPECT_CALL(delegate(), RequestToShowGmailOtpOptInDialog(_))nit: you can drop this if you don't validate any parameters.
OK, but I need this for
- the test setup to call the callback with `NewPermissionGranted(true)` and
- the assertion that this method is in fact called.
If I comment this out, the test takes quite long because it seems to wait for the callback.
Am I missing something?
TEST_F(
AttemptOtpFillingToolTest,
ValidateOptInPermissionGrantedCallbackSucceedsAndEnablesOptInAndResetsDismissalTimestamp) {
EXPECT_CALL(delegate(), RequestToShowGmailOtpOptInDialog(_))
.WillOnce(base::test::RunOnceCallback<0>(
webui::mojom::GmailOtpOptInResult::NewPermissionGranted(true)));
AttemptOtpFillingTool tool(TaskId(1), delegate(), mock_tab().GetHandle(),
{PageTarget(gfx::Point(10, 10))},
/*for_signin=*/true);
autofill::prefs::SetAutofillGmailOtpFillingEnabled(prefs(), false);
base::test::TestFuture<mojom::ActionResultPtr> future;
tool.Validate(future.GetCallback());
mojom::ActionResultPtr result = future.Take();
EXPECT_EQ(mojom::ActionResultCode::kOk, result->code);
EXPECT_TRUE(autofill::prefs::IsAutofillGmailOtpFillingEnabled(prefs()));
EXPECT_EQ(
base::Time(),
autofill::prefs::GetAutofillGmailOtpFillingActivationDismissalTimestamp(
prefs()));
}Thanks for the hint. I added a few more namespace usages and used a bit less comments in the test.
ValidateOptInPermissionDeniedCallbackFailsAndDoesNotEnableOptInAndUpdatesDismissalTimestamp) {Stephan DrabFindingTheDeltaToThePreviousStringIsABitLikeWhereIsWaldo
Done
Nit: new lines after tests
- Dave Tapuska
- Dominic Battré
- Trang Mai
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Battré (Gerrit)
Dominic Battré added 9 comments![]( "Open in Gerrit")
EXPECT_CALL(delegate(), RequestToShowGmailOtpOptInDialog(_))Stephan Drabnit: you can drop this if you don't validate any parameters.
OK, but I need this for
- the test setup to call the callback with `NewPermissionGranted(true)` and
- the assertion that this method is in fact called.
If I comment this out, the test takes quite long because it seems to wait for the callback.
Am I missing something?
Maybe I got this wrong. My proposal was to write
```
EXPECT_CALL(delegate(), RequestToShowGmailOtpOptInDialog)
```
instead of
```
EXPECT_CALL(delegate(), RequestToShowGmailOtpOptInDialog(_))
```
It's possible that you need the `_` for the `RunOnceCallback<0>`?
// remove the dismissal timestampnit: period at the end.
EXPECT_CALL(delegate(), RequestToShowGmailOtpOptInDialog(_)).Times(0);
EXPECT_CALL(delegate().mock_otp_service(), RetrieveOtp(_, _, _)).Times(0);```suggestion
EXPECT_CALL(delegate(), RequestToShowGmailOtpOptInDialog).Times(0);
EXPECT_CALL(delegate().mock_otp_service(), RetrieveOtp).Times(0);
```
}Check here that `GetAutofillGmailOtpFillingActivationDismissalTimestamp(prefs())` did not change?
EXPECT_CALL(delegate(), RequestToShowGmailOtpOptInDialog(_));```suggestion
EXPECT_CALL(delegate(), RequestToShowGmailOtpOptInDialog);
```
TEST_F(AttemptOtpFillingToolTest, TimeOfUseValidation_TabWentAway) {Do you want to be consistent with adding comments to the tests?
Here I can conclude it but for "NoLastObservation" it's not obvious anymore. I'd need to check the enum to understand what this is about.
[](tabs::TabHandle, const std::vector<autofill::FieldGlobalId>&,
base::OnceCallback<void(
base::expected<std::string,
one_time_tokens::OneTimeTokenRetrievalError>)>
cb) { std::move(cb).Run("123456"); });Can you simplify this to
```
RunOnceCallback<2>("123456")
```
?
Similarly below
.WillOnce([](tabs::TabHandle, const std::vector<autofill::FieldGlobalId>&,
const std::string&, base::OnceCallback<void(bool)> cb) {
std::move(cb).Run(true);
});```suggestion
.WillOnce(RunOnceCallback<2>(true)));
```
?
Related details
- Dave Tapuska
- Stephan Drab
- Trang Mai
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. |
Stephan Drab (Gerrit)
Stephan Drab added 8 comments![]( "Open in Gerrit")
EXPECT_CALL(delegate(), RequestToShowGmailOtpOptInDialog(_))Stephan Drabnit: you can drop this if you don't validate any parameters.
Dominic BattréOK, but I need this for
- the test setup to call the callback with `NewPermissionGranted(true)` and
- the assertion that this method is in fact called.
If I comment this out, the test takes quite long because it seems to wait for the callback.
Am I missing something?
Maybe I got this wrong. My proposal was to write
```
EXPECT_CALL(delegate(), RequestToShowGmailOtpOptInDialog)
```
instead of
```
EXPECT_CALL(delegate(), RequestToShowGmailOtpOptInDialog(_))
```It's possible that you need the `_` for the `RunOnceCallback<0>`?
Ah, that works! Done
nit: period at the end.
Done
EXPECT_CALL(delegate(), RequestToShowGmailOtpOptInDialog(_)).Times(0);
EXPECT_CALL(delegate().mock_otp_service(), RetrieveOtp(_, _, _)).Times(0);```suggestion
EXPECT_CALL(delegate(), RequestToShowGmailOtpOptInDialog).Times(0);
EXPECT_CALL(delegate().mock_otp_service(), RetrieveOtp).Times(0);
```
Done
Check here that `GetAutofillGmailOtpFillingActivationDismissalTimestamp(prefs())` did not change?
Done
EXPECT_CALL(delegate(), RequestToShowGmailOtpOptInDialog(_));Stephan Drab```suggestion
EXPECT_CALL(delegate(), RequestToShowGmailOtpOptInDialog);
```
Done
TEST_F(AttemptOtpFillingToolTest, TimeOfUseValidation_TabWentAway) {Do you want to be consistent with adding comments to the tests?
Here I can conclude it but for "NoLastObservation" it's not obvious anymore. I'd need to check the enum to understand what this is about.
I did add tests for those methods for code that already existed, but I figured I wouldn't add comments, since I didn't have such a good overview of what this should do. But you're right, out of consistency, the comments make sense. Added them.
[](tabs::TabHandle, const std::vector<autofill::FieldGlobalId>&,
base::OnceCallback<void(
base::expected<std::string,
one_time_tokens::OneTimeTokenRetrievalError>)>
cb) { std::move(cb).Run("123456"); });Can you simplify this to
```
RunOnceCallback<2>("123456")
```
?Similarly below
I also fixed the other occurrences. Done
.WillOnce([](tabs::TabHandle, const std::vector<autofill::FieldGlobalId>&,
const std::string&, base::OnceCallback<void(bool)> cb) {
std::move(cb).Run(true);
});```suggestion
.WillOnce(RunOnceCallback<2>(true)));
```
?
- Dave Tapuska
- Dominic Battré
- Trang Mai
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave Tapuska (Gerrit)
Dave Tapuska added 1 comment![]( "Open in Gerrit")
#include "chrome/common/actor_webui.mojom.h"actor_webui.mojom-forward.h
Related details
- Dominic Battré
- Stephan Drab
- Trang Mai
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. |
Dave Tapuska (Gerrit)
Dave Tapuska voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Dominic Battré
- Stephan Drab
- Trang Mai
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. |
Stephan Drab (Gerrit)
Stephan Drab added 1 comment![]( "Open in Gerrit")
#include "chrome/common/actor_webui.mojom.h"Stephan Drabactor_webui.mojom-forward.h
Done
Related details
- Dave Tapuska
- Dominic Battré
- Trang Mai
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Battré (Gerrit)
Dominic Battré voted and added 7 comments![]( "Open in Gerrit")
Votes added by Dominic Battré
| Code-Review | +1 |
7 comments
"Gmail OTP disabled and within 90-day cool-off period "I would recommend dropping this number (as the constant could change) or using base::StringPrintf to add it do the string.
// persisted prefs did not changePeriod at the end.
TEST_F(AttemptOtpFillingToolTest, TimeOfUseValidation_TabWentAway) {Stephan DrabDo you want to be consistent with adding comments to the tests?
Here I can conclude it but for "NoLastObservation" it's not obvious anymore. I'd need to check the enum to understand what this is about.
I did add tests for those methods for code that already existed, but I figured I wouldn't add comments, since I didn't have such a good overview of what this should do. But you're right, out of consistency, the comments make sense. Added them.
Thanks, by doing the research once, you safe the work for everybody in the future.
// observation is null.do you want to document what this means?
// Time of use validation returns kFormFillingFieldNotFound when trigger fieldsplural or singular?
// Invoke fails with kOtpFillFailure when the result of"Invoke()"? That makes reading the sentence easier because it's not obvious that this is a function name. Same below.
Related details
- Dave Tapuska
- Stephan Drab
- Trang Mai
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. |
Stephan Drab (Gerrit)
Stephan Drab added 6 comments![]( "Open in Gerrit")
"Gmail OTP disabled and within 90-day cool-off period "I would recommend dropping this number (as the constant could change) or using base::StringPrintf to add it do the string.
Done
// persisted prefs did not changeStephan DrabPeriod at the end.
Done
do you want to document what this means?
Done
// Time of use validation returns kFormFillingFieldNotFound when trigger fieldsStephan Drabplural or singular?
Done
// Invoke fails with kOtpFillFailure when the result of"Invoke()"? That makes reading the sentence easier because it's not obvious that this is a function name. Same below.
Done
// retrieving the OTP is false.Stephan Drabfilling
Done
Related details
- Dave Tapuska
- Dominic Battré
- Trang Mai
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Battré (Gerrit)
Dominic Battré voted Code-Review+1![]( "Open in Gerrit")
- Dave Tapuska
- Stephan Drab
- Trang Mai
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |