[iOS][PRDBD] Sets strings for "Manage other data" in QuickDeleteMediator [chromium/src : main]
Angela Novakovic (Gerrit)
Angela Novakovic added 2 comments![]( "Open in Gerrit")
I am writing the unittest for this CL and having issues with verifying that the string changes when the DSE changes, so I would like to know if I set it up correctly here. 😊
- (NSString*)manageOtherDataSubtitle {This will be updated in the following CL. It will take in account when _templateURLService ||
_templateURLService->GetDefaultSearchProvider() are nil. In that case, a separate set of string will be displayed.
if(!_templateURLService ||
!_templateURLService->GetDefaultSearchProvider()){
_identityManager->HasPrimaryAccount(signin::ConsentLevel::kSignin)? string1 : string2 }
Related details
- Noémie St-Onge
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. |
Noémie St-Onge (Gerrit)
Noémie St-Onge added 15 comments![]( "Open in Gerrit")
This change isn't guarded by the feature flag, so landing it would have an immediate impact on the current feature, we want to avoid that
_searchEngineObserver.reset();We don't need to reset an already null pointer
_searchEngineObserver.reset();Same, we don't need to reset an already null pointer
// update the strings for `Manage other data`.`"Manage other data"`, backticks are used for symbols
return _templateURLService &&
_templateURLService->GetDefaultSearchProvider() &&
_templateURLService->GetDefaultSearchProvider()->GetEngineType(
_templateURLService->search_terms_data()) == SEARCH_ENGINE_GOOGLE;This code is run quite often (every time we update the title and subtitle), should we instead store the result in `init` and update it whenever `-searchEngineChanged` is called?
// Sets the title for `Manage other data` section.```suggestion
// Sets the title for the "Manage other data" section.
```
// Sets the subtitle for `Manage other data` section.```suggestion
// Sets the subtitle for the "Manage other data" section.
```
- (NSString*)manageOtherDataSubtitle {This will be updated in the following CL. It will take in account when _templateURLService ||
_templateURLService->GetDefaultSearchProvider() are nil. In that case, a separate set of string will be displayed.
if(!_templateURLService ||
!_templateURLService->GetDefaultSearchProvider()){
_identityManager->HasPrimaryAccount(signin::ConsentLevel::kSignin)? string1 : string2 }
Acknowledged
? l10n_util::GetNSString(IDS_SETTINGS_MANAGE_PASSWORDS_SUB_LABEL)Passwords?
// These are the expected strings when templateURLService is a nullptr.`` `templateURLService` ``
NSString* _manageOtherDataTitle;
NSString* _manageOtherDataSubtitle;ivars should come with a comment
// Sets the subtitle for the `Manage other data` section.`"Manage other data"`
// Sets the title for the `Manage other data` section.`"Manage other data"`
}
- (void)setManageOtherDataSubtitle:(NSString*)manageOtherDataSubtitle {skip a line between methods
Related details
- Angela Novakovic
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. |
Angela Novakovic (Gerrit)
Angela Novakovic voted and added 15 comments![]( "Open in Gerrit")
Votes added by Angela Novakovic
| Commit-Queue | +1 |
This change isn't guarded by the feature flag, so landing it would have an immediate impact on the current feature, we want to avoid that
Done
We don't need to reset an already null pointer
Done
Same, we don't need to reset an already null pointer
Done
`"Manage other data"`, backticks are used for symbols
Done
return _templateURLService &&
_templateURLService->GetDefaultSearchProvider() &&
_templateURLService->GetDefaultSearchProvider()->GetEngineType(
_templateURLService->search_terms_data()) == SEARCH_ENGINE_GOOGLE;This code is run quite often (every time we update the title and subtitle), should we instead store the result in `init` and update it whenever `-searchEngineChanged` is called?
Done
```suggestion
// Sets the title for the "Manage other data" section.
```
Done
// Sets the subtitle for `Manage other data` section.```suggestion
// Sets the subtitle for the "Manage other data" section.
```
Done
? l10n_util::GetNSString(IDS_SETTINGS_MANAGE_PASSWORDS_SUB_LABEL)Angela NovakovicPasswords?
The name for the string is a bit confusing, but it is "Passwords can be deleted in their management settings" which is what the subtitle should be when DSE is Google and user is not signed in.
// if(!_templateURLService ||
// !_templateURLService->GetDefaultSearchProvider()){
// _identityManager->HasPrimaryAccount(signin::ConsentLevel::kSignin)?
// l10n_util::GetNSString(IDS_IOS_CLEAR_BROWSING_DATA_MANAGE_OTHER_DATA_SUBTITLE_WHEN_DSE_NULL)
// : l10n_util::GetNSString(IDS_SETTINGS_MANAGE_PASSWORDS_SUB_LABEL)}Will uncomment this block once the CL with the new string has been merged. 😊
// These are the expected strings when templateURLService is a nullptr.Angela Novakovic`` `templateURLService` ``
Done
NSString* _manageOtherDataTitle;
NSString* _manageOtherDataSubtitle;ivars should come with a comment
Done
// Sets the subtitle for the `Manage other data` section.Angela Novakovic`"Manage other data"`
Done
// Sets the title for the `Manage other data` section.Angela Novakovic`"Manage other data"`
Done
}
- (void)setManageOtherDataSubtitle:(NSString*)manageOtherDataSubtitle {skip a line between methods
- Noémie St-Onge
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. |
Noémie St-Onge (Gerrit)
Noémie St-Onge added 20 comments![]( "Open in Gerrit")
A following CL will add the unit tests for this CL.I highly encourage adding tests in the same CL as the change when possible. It's fine to do it in a follow-up when adding the tests would be a big CL in itself
# keep-sorted start
"+ios/chrome/browser/saved_tab_groups/model",
# keep-sorted endWhy are we removing lines 2 and 4?
templateURLService:templateURLServiceDo we really need the TemplateURLService here? Isn't it just needed for Quick Delete Browsing Data?
// This boolean return true if the default seach engine is Google.`returns`, but even then, a variable doesn't "return" something
BOOL _theDefaultSearchEngineIsGoogle;we can remove `the` from the variable name
if (IsPasswordRemovalFromDeleteBrowsingDataEnabled()) {I think it would be simpler to just guard the calls to `-setManageOtherDataTitle` and `-setManageOtherDataTitle`. It's fine to set the service and observer when the flag is off since it is temporary
_theDefaultSearchEngineIsGoogle = [self isGoogleTheDefaultSearchEngine];Calling a function on `self` while `self` is being constructed is discouraged. We should look to convert `-isGoogleTheDefaultSearchEngine` to a non-member function declared in the anonymous namespace
if ([self isGoogleTheDefaultSearchEngine]) {we should use the ivar instead
[self isGoogleTheDefaultSearchEngine];We should update the ivar here. `-setManageOtherDataTitle` and `-setsetManageOtherDataSubtitle` are reading `_theDefaultSearchEngineIsGoogle`, so an update to the DSE wouldn't have been taken into account
```suggestion
_theDefaultSearchEngineIsGoogle = [self isGoogleTheDefaultSearchEngine];
```
// Sets whether the user's default search engine is Google.`Returns` as it doesn't set anything
// Sets the title for the "Manage other data" section.Table view sections can have a title, so this could be misleading. I would use the term `cell` instead
// Sets the title for the "Manage other data" section.nit: This function doesn't actually "set" the title, it returns it. The view controller will use the received string to set the title of the cell. Same for the subtitle function
// Sets the subtitle for the "Manage other data" section.`cell`
// if(!_templateURLService ||
// !_templateURLService->GetDefaultSearchProvider()){
// _identityManager->HasPrimaryAccount(signin::ConsentLevel::kSignin)?
// l10n_util::GetNSString(IDS_IOS_CLEAR_BROWSING_DATA_MANAGE_OTHER_DATA_SUBTITLE_WHEN_DSE_NULL)
// : l10n_util::GetNSString(IDS_SETTINGS_MANAGE_PASSWORDS_SUB_LABEL)}Will uncomment this block once the CL with the new string has been merged. 😊
this would be a great use case for chained CLs!
CHECK(!IsPasswordRemovalFromDeleteBrowsingDataEnabled());This is going to crash. This is a great example of why having the tests in the same CL is preferred, the tests would have caught that
```suggestion
CHECK(IsPasswordRemovalFromDeleteBrowsingDataEnabled());
```
CHECK(!IsPasswordRemovalFromDeleteBrowsingDataEnabled());```suggestion
CHECK(IsPasswordRemovalFromDeleteBrowsingDataEnabled());
```
// Sets the subtitle for the "Manage other data" section.```suggestion
// Sets the ViewController with the subtitle for the "Manage other data" cell.
```
// Sets the title for the "Manage other data" section.I would follow the same phrasing as the other comments:
```suggestion
// Sets the ViewController with the title for the "Manage other data" cell.
```
Otherwise, it sounds like this function will actually set the title itself, which can or can't be true as it will depend on the view controller's implementation
// No-op: This ViewController doesn't show `Manage Other Data`.```suggestion
// No-op: This ViewController doesn't show the "Manage Other Data" cell.
```
// No-op: This ViewController doesn't show `Manage Other Data`.```suggestion
// No-op: This ViewController doesn't show the "Manage Other Data" cell.
```
Related details
- Angela Novakovic
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. |
Angela Novakovic (Gerrit)
Angela Novakovic added 21 comments![]( "Open in Gerrit")
I highly encourage adding tests in the same CL as the change when possible. It's fine to do it in a follow-up when adding the tests would be a big CL in itself
Done
# keep-sorted start
"+ios/chrome/browser/saved_tab_groups/model",
# keep-sorted endWhy are we removing lines 2 and 4?
I didn't realize that these were added to all deps in the codebase! Have put them back.
Do we really need the TemplateURLService here? Isn't it just needed for Quick Delete Browsing Data?
Correct, we do not need it. I think I've made a mistake when updating my code.
// This boolean return true if the default seach engine is Google.`returns`, but even then, a variable doesn't "return" something
Done
we can remove `the` from the variable name
Done
if (IsPasswordRemovalFromDeleteBrowsingDataEnabled()) {I think it would be simpler to just guard the calls to `-setManageOtherDataTitle` and `-setManageOtherDataTitle`. It's fine to set the service and observer when the flag is off since it is temporary
Done
_theDefaultSearchEngineIsGoogle = [self isGoogleTheDefaultSearchEngine];Calling a function on `self` while `self` is being constructed is discouraged. We should look to convert `-isGoogleTheDefaultSearchEngine` to a non-member function declared in the anonymous namespace
Done
we should use the ivar instead
Done
We should update the ivar here. `-setManageOtherDataTitle` and `-setsetManageOtherDataSubtitle` are reading `_theDefaultSearchEngineIsGoogle`, so an update to the DSE wouldn't have been taken into account
```suggestion
_theDefaultSearchEngineIsGoogle = [self isGoogleTheDefaultSearchEngine];
```
Done
// Sets whether the user's default search engine is Google.`Returns` as it doesn't set anything
Done
CHECK(IsPasswordRemovalFromDeleteBrowsingDataEnabled());This was removed, as this can be reached even when the feature flag is disabled.
// Sets the title for the "Manage other data" section.nit: This function doesn't actually "set" the title, it returns it. The view controller will use the received string to set the title of the cell. Same for the subtitle function
Done
// Sets the title for the "Manage other data" section.Table view sections can have a title, so this could be misleading. I would use the term `cell` instead
Done
// Sets the subtitle for the "Manage other data" section.Angela Novakovic`cell`
Done
// if(!_templateURLService ||
// !_templateURLService->GetDefaultSearchProvider()){
// _identityManager->HasPrimaryAccount(signin::ConsentLevel::kSignin)?
// l10n_util::GetNSString(IDS_IOS_CLEAR_BROWSING_DATA_MANAGE_OTHER_DATA_SUBTITLE_WHEN_DSE_NULL)
// : l10n_util::GetNSString(IDS_SETTINGS_MANAGE_PASSWORDS_SUB_LABEL)}Noémie St-OngeWill uncomment this block once the CL with the new string has been merged. 😊
this would be a great use case for chained CLs!
Absolutely, I only realized after I had created this CL that I needed an additional string. 😊 Is there a way to chain a CL on another CL after the fact?
CHECK(!IsPasswordRemovalFromDeleteBrowsingDataEnabled());This is going to crash. This is a great example of why having the tests in the same CL is preferred, the tests would have caught that
```suggestion
CHECK(IsPasswordRemovalFromDeleteBrowsingDataEnabled());
```
Done
CHECK(!IsPasswordRemovalFromDeleteBrowsingDataEnabled());Angela Novakovic```suggestion
CHECK(IsPasswordRemovalFromDeleteBrowsingDataEnabled());
```
Done
// Sets the subtitle for the "Manage other data" section.```suggestion
// Sets the ViewController with the subtitle for the "Manage other data" cell.
```
Done
// Sets the title for the "Manage other data" section.I would follow the same phrasing as the other comments:
```suggestion
// Sets the ViewController with the title for the "Manage other data" cell.
```Otherwise, it sounds like this function will actually set the title itself, which can or can't be true as it will depend on the view controller's implementation
Done
// No-op: This ViewController doesn't show `Manage Other Data`.```suggestion
// No-op: This ViewController doesn't show the "Manage Other Data" cell.
```
Done
// No-op: This ViewController doesn't show `Manage Other Data`.```suggestion
// No-op: This ViewController doesn't show the "Manage Other Data" cell.
```
- Noémie St-Onge
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. |
Noémie St-Onge (Gerrit)
Noémie St-Onge added 12 comments![]( "Open in Gerrit")
# keep-sorted start
"+ios/chrome/browser/saved_tab_groups/model",
"+ios/chrome/browser/search_engines/model",
# keep-sorted endIndentation is off
raw_ptr<TemplateURLService> templateURLService) {`template_url_service` as this is C++
raw_ptr<TemplateURLService> templateURLService) {Use `TemplateURLService*`, `raw_ptr` should only be used for class and struct fields
https://chromium.googlesource.com/chromium/src/+/HEAD/base/memory/raw_ptr.md
if (_templateURLService) {
_searchEngineObserver = std::make_unique<SearchEngineObserverBridge>(
self, templateURLService);
}
_defaultSearchEngineIsGoogle =
IsGoogleTheDefaultSearchEngine(_templateURLService);There is a lot of duplicated code between the 2 initializers, and we're now also duplicating a bit less trivial logic. Could you look into refactoring these? I think having a private designated initializer and making the existing initializers convenience initializers would make sense. This can be done in another CL
if (_defaultSearchEngineIsGoogle) {If the DSE is null and the user goes from being signed in to signed out (or vice-versa), we wouldn't update the subtitle, which is wrong. This case should be covered in a test
// update the strings for "Manage other data".```suggestion
// update the strings for the "Manage other data" cell.
```
CHECK(IsPasswordRemovalFromDeleteBrowsingDataEnabled());This was removed, as this can be reached even when the feature flag is disabled.
Acknowledged
// Returns the title for the "Manage other data" cell.```suggestion
// Returns the title for the "Manage other data" cell. The title depends on the user's default search engine.
```
// Returns the subtitle for the "Manage other data" cell.```suggestion
// Returns the subtitle for the "Manage other data" cell. The subtitle depends on the user's default search engine and sign-in status.
```
if (!_templateURLService ||
!_templateURLService->GetDefaultSearchProvider()) {
return _identityManager->HasPrimaryAccount(signin::ConsentLevel::kSignin)
? l10n_util::GetNSString(
IDS_IOS_CLEAR_BROWSING_DATA_MANAGE_OTHER_DATA_SUBTITLE_UNKNOWN_DSE)
: l10n_util::GetNSString(
IDS_SETTINGS_MANAGE_PASSWORDS_SUB_LABEL);
}I would add a comment here to explain that this is handling the case where the DSE is unknown
// No-op: This ViewController doesn't show the "Manage Other Data" cell.`"Manage other data" cell` to be consistent
// No-op: This ViewController doesn't show the "Manage Other Data" cell.`"Manage other data" cell` to be consistent
Related details
- Angela Novakovic
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. |
Angela Novakovic (Gerrit)
Angela Novakovic added 11 comments![]( "Open in Gerrit")
# keep-sorted start
"+ios/chrome/browser/saved_tab_groups/model",
"+ios/chrome/browser/search_engines/model",
# keep-sorted endAngela NovakovicIndentation is off
Done
Use `TemplateURLService*`, `raw_ptr` should only be used for class and struct fields
https://chromium.googlesource.com/chromium/src/+/HEAD/base/memory/raw_ptr.md
Done
`template_url_service` as this is C++
Done
if (_templateURLService) {
_searchEngineObserver = std::make_unique<SearchEngineObserverBridge>(
self, templateURLService);
}
_defaultSearchEngineIsGoogle =
IsGoogleTheDefaultSearchEngine(_templateURLService);There is a lot of duplicated code between the 2 initializers, and we're now also duplicating a bit less trivial logic. Could you look into refactoring these? I think having a private designated initializer and making the existing initializers convenience initializers would make sense. This can be done in another CL
Will do in a following CL. 😊
If the DSE is null and the user goes from being signed in to signed out (or vice-versa), we wouldn't update the subtitle, which is wrong. This case should be covered in a test
Done
```suggestion
// update the strings for the "Manage other data" cell.
```
Done
// Returns the title for the "Manage other data" cell.```suggestion
// Returns the title for the "Manage other data" cell. The title depends on the user's default search engine.
```
Done
// Returns the subtitle for the "Manage other data" cell.```suggestion
// Returns the subtitle for the "Manage other data" cell. The subtitle depends on the user's default search engine and sign-in status.
```
Done
if (!_templateURLService ||
!_templateURLService->GetDefaultSearchProvider()) {
return _identityManager->HasPrimaryAccount(signin::ConsentLevel::kSignin)
? l10n_util::GetNSString(
IDS_IOS_CLEAR_BROWSING_DATA_MANAGE_OTHER_DATA_SUBTITLE_UNKNOWN_DSE)
: l10n_util::GetNSString(
IDS_SETTINGS_MANAGE_PASSWORDS_SUB_LABEL);
}I would add a comment here to explain that this is handling the case where the DSE is unknown
Done
// No-op: This ViewController doesn't show the "Manage Other Data" cell.`"Manage other data" cell` to be consistent
Done
// No-op: This ViewController doesn't show the "Manage Other Data" cell.`"Manage other data" cell` to be consistent
- Noémie St-Onge
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. |
Noémie St-Onge (Gerrit)
Noémie St-Onge added 11 comments![]( "Open in Gerrit")
if (_templateURLService) {
_searchEngineObserver = std::make_unique<SearchEngineObserverBridge>(
self, templateURLService);
}
_defaultSearchEngineIsGoogle =
IsGoogleTheDefaultSearchEngine(_templateURLService);Angela NovakovicThere is a lot of duplicated code between the 2 initializers, and we're now also duplicating a bit less trivial logic. Could you look into refactoring these? I think having a private designated initializer and making the existing initializers convenience initializers would make sense. This can be done in another CL
Will do in a following CL. 😊
Acknowledged
if (!_templateURLService ||
!_templateURLService->GetDefaultSearchProvider() ||
_defaultSearchEngineIsGoogle) {
[_consumer setManageOtherDataSubtitle:[self manageOtherDataSubtitle]];
}I think it's fine to just call `[_consumer setManageOtherDataSubtitle:[self manageOtherDataSubtitle]];` here.
Assuming you did it that way to prevent an unnecessary consumer update - by having the condition, you are:
1) Essentially duplicating logic from `-manageOtherDataSubtitle`, and duplicated logic means 2 places to maintain, which brings risk and is to be avoided in general.
2) Potentially querying the `_templateURLService` twice in a row (once in the condition and once in `-manageOtherDataSubtitle`. So we would potentially save a call to `-setManageOtherDataSubtitle`, by risking to call `_templateURLService->GetDefaultSearchProvider()` twice, which isn't better.
// Sets the subtitle when the default search engine is null.`Set`
[OCMockObject niceMockForProtocol:@protocol(QuickDeleteConsumer)];What's the reason for this change?
// data" cell when the default search engine (DSE) is Google when the feature`and`?
// data" cell when the default search engine (DSE) is Google when the feature
// 'kPasswordRemovalFromDeleteBrowsingData` is enabled.Actually, since these are all tests that require the feature to be enabled, and don't come with a "feature disabled" version, I think we can remove the flag mention from the comments. Enabling the flag in the tests should speak for itself
base::test::ScopedFeatureList feature_list;
feature_list.InitAndEnableFeature(kPasswordRemovalFromDeleteBrowsingData);You don't need the second line to enable a feature. `ScopedFeatureList` has this constructor available https://source.chromium.org/chromium/chromium/src/+/main:base/test/scoped_feature_list.h;l=91;drc=4b18d0c047503e82fdfc53c8b57a397458b18958
Please update other occurences
TemplateURLData non_google_provider_data;
non_google_provider_data.SetURL("https://www.nongoogle.com/?q={searchTerms}");
non_google_provider_data.suggestions_url =
"https://www.nongoogle.com/suggest/?q={searchTerms}";
auto* non_google_provider = template_url_service_->Add(
std::make_unique<TemplateURL>(non_google_provider_data));
template_url_service_->SetUserSelectedDefaultSearchProvider(
non_google_provider);This is repeated a few times, let's create a helper function
// Verifies that the consumer receives the correct subtitle for the "Manage
// Other Data" cell when the user is signed in and the DSE is Google when the`"Manage other data" cell` for consistency. Please also update all other occurrences
mediator_ = [[QuickDeleteMediator alloc] initWithPrefs:profile_->GetPrefs()
browsingDataCounterWrapperProducer:
fake_browsing_data_counter_wrapper_producer_
identityManager:identity_manager_
browsingDataRemover:browsing_data_remover_
discoverFeedService:discover_feed_service_
templateURLService:nullptr
canPerformRadialWipeAnimation:NO
uiBlockerTarget:scene_state_
featureEngagementTracker:tracker_];Why not just call `CreateMediator()`? We can add an argument to the function if it's just that we need to change the `templateURLService`
Related details
- Angela Novakovic
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. |
Angela Novakovic (Gerrit)
Angela Novakovic added 11 comments![]( "Open in Gerrit")
_searchEngineObserver = std::make_unique<SearchEngineObserverBridge>(
self, templateURLService);Is there a possibility that the user on iPad might change their DSE and cause a crash here? Since it would call the `searchEngineChanged` method and result in reaching the CHECK() lines in `ManageOtherDataTitle` and `ManageOtherDataSubtitle`. Perhaps, we should add a check to see if the flag is on and change:
`if(_templateURLService)` to `if(_templateURLService && IsPasswordRemovalFromDeleteBrowsingDataEnabled())`
What do you think?
if (!_templateURLService ||
!_templateURLService->GetDefaultSearchProvider() ||
_defaultSearchEngineIsGoogle) {
[_consumer setManageOtherDataSubtitle:[self manageOtherDataSubtitle]];
}I think it's fine to just call `[_consumer setManageOtherDataSubtitle:[self manageOtherDataSubtitle]];` here.
Assuming you did it that way to prevent an unnecessary consumer update - by having the condition, you are:
1) Essentially duplicating logic from `-manageOtherDataSubtitle`, and duplicated logic means 2 places to maintain, which brings risk and is to be avoided in general.
2) Potentially querying the `_templateURLService` twice in a row (once in the condition and once in `-manageOtherDataSubtitle`. So we would potentially save a call to `-setManageOtherDataSubtitle`, by risking to call `_templateURLService->GetDefaultSearchProvider()` twice, which isn't better.
Yes, that was my intent, but I do see now that it makes more sense to do it this way.
// Sets the subtitle when the default search engine is null.Angela Novakovic`Set`
Done
[OCMockObject niceMockForProtocol:@protocol(QuickDeleteConsumer)];What's the reason for this change?
This change allows me to not setup a OCMStub for setManageOtherDataTitle or setManageOtherDataSubtitle. If I do set them up, I need to set OCMExpect for both title and subtitle when testing these to avoid crashes. In addition, if my OCMExpect is the same as the OCMStub, then the test says that the method was never call for the OCMExpect.
An example is for TestTitleWhenDSEIsGoogle, where I check that the correct title is displayed. As the DSE is google to start with, the OCMStub is already set to the correct title, so when I use OCMExpect to check that the correct title (same as in OCMStub) is displayed, it fails.
? Same for other occurrences below
I thought we could remove this line between the feature flag setup and CreateMediator().
// data" cell when the default search engine (DSE) is Google when the featureAngela Novakovic`and`?
Acknowledged
// data" cell when the default search engine (DSE) is Google when the feature
// 'kPasswordRemovalFromDeleteBrowsingData` is enabled.Actually, since these are all tests that require the feature to be enabled, and don't come with a "feature disabled" version, I think we can remove the flag mention from the comments. Enabling the flag in the tests should speak for itself
Done
base::test::ScopedFeatureList feature_list;
feature_list.InitAndEnableFeature(kPasswordRemovalFromDeleteBrowsingData);You don't need the second line to enable a feature. `ScopedFeatureList` has this constructor available https://source.chromium.org/chromium/chromium/src/+/main:base/test/scoped_feature_list.h;l=91;drc=4b18d0c047503e82fdfc53c8b57a397458b18958
Please update other occurences
Done
TemplateURLData non_google_provider_data;
non_google_provider_data.SetURL("https://www.nongoogle.com/?q={searchTerms}");
non_google_provider_data.suggestions_url =
"https://www.nongoogle.com/suggest/?q={searchTerms}";
auto* non_google_provider = template_url_service_->Add(
std::make_unique<TemplateURL>(non_google_provider_data));
template_url_service_->SetUserSelectedDefaultSearchProvider(
non_google_provider);This is repeated a few times, let's create a helper function
Done
// Verifies that the consumer receives the correct subtitle for the "Manage
// Other Data" cell when the user is signed in and the DSE is Google when the`"Manage other data" cell` for consistency. Please also update all other occurrences
Done
mediator_ = [[QuickDeleteMediator alloc] initWithPrefs:profile_->GetPrefs()
browsingDataCounterWrapperProducer:
fake_browsing_data_counter_wrapper_producer_
identityManager:identity_manager_
browsingDataRemover:browsing_data_remover_
discoverFeedService:discover_feed_service_
templateURLService:nullptr
canPerformRadialWipeAnimation:NO
uiBlockerTarget:scene_state_
featureEngagementTracker:tracker_];Why not just call `CreateMediator()`? We can add an argument to the function if it's just that we need to change the `templateURLService`
I was considering this, but I think when I did try to implement it the first time, I didn't do it properly, so it looked less elegant. Now, I've improve upon it 😊
Related details
- Noémie St-Onge
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. |
Noémie St-Onge (Gerrit)
Noémie St-Onge added 2 comments![]( "Open in Gerrit")
_searchEngineObserver = std::make_unique<SearchEngineObserverBridge>(
self, templateURLService);Is there a possibility that the user on iPad might change their DSE and cause a crash here? Since it would call the `searchEngineChanged` method and result in reaching the CHECK() lines in `ManageOtherDataTitle` and `ManageOtherDataSubtitle`. Perhaps, we should add a check to see if the flag is on and change:
`if(_templateURLService)` to `if(_templateURLService && IsPasswordRemovalFromDeleteBrowsingDataEnabled())`
What do you think?
Good catch! I would add the flag check in `searchEngineChanged` though, like we've done in other places. Calls to `setManageOtherDataTitle` and `setManageOtherDataSubtitle` should be guarded by a flag check
Angela Novakovic? Same for other occurrences below
I thought we could remove this line between the feature flag setup and CreateMediator().
IMO, separating the feature changes (and other similar setups) from the actual test helps with readability. But there's no strict guidance on this so I'll leave it up to you
Related details
- Angela Novakovic
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. |
Angela Novakovic (Gerrit)
Angela Novakovic added 2 comments![]( "Open in Gerrit")
_searchEngineObserver = std::make_unique<SearchEngineObserverBridge>(
self, templateURLService);Noémie St-OngeIs there a possibility that the user on iPad might change their DSE and cause a crash here? Since it would call the `searchEngineChanged` method and result in reaching the CHECK() lines in `ManageOtherDataTitle` and `ManageOtherDataSubtitle`. Perhaps, we should add a check to see if the flag is on and change:
`if(_templateURLService)` to `if(_templateURLService && IsPasswordRemovalFromDeleteBrowsingDataEnabled())`
What do you think?
Good catch! I would add the flag check in `searchEngineChanged` though, like we've done in other places. Calls to `setManageOtherDataTitle` and `setManageOtherDataSubtitle` should be guarded by a flag check
Done
Angela Novakovic? Same for other occurrences below
Noémie St-OngeI thought we could remove this line between the feature flag setup and CreateMediator().
IMO, separating the feature changes (and other similar setups) from the actual test helps with readability. But there's no strict guidance on this so I'll leave it up to you
Agree. I think I had more in mind to minimize the space between code. 😊
Related details
- Noémie St-Onge
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noémie St-Onge (Gerrit)
Noémie St-Onge added 14 comments![]( "Open in Gerrit")
// This boolean tells if user's default search engine is Google.nit
```suggestion
// This boolean tells if the user's default search engine is Google.
```
if (IsPasswordRemovalFromDeleteBrowsingDataEnabled()) {
_defaultSearchEngineIsGoogle =
IsGoogleTheDefaultSearchEngine(_templateURLService);
[_consumer setManageOtherDataTitle:[self manageOtherDataTitle]];
[_consumer setManageOtherDataSubtitle:[self manageOtherDataSubtitle]];
}in the init methods, we set `_defaultSearchEngineIsGoogle` regardless of the flag's state. I think it would make more sense to be consistent and to just guard the calls to `setManageOtherDataTitle` and `setManageOtherDataSubtitle` like we did elsewhere
if (!_templateURLService ||
!_templateURLService->GetDefaultSearchProvider()) {nit: instead of having a `_defaultSearchEngineIsGoogle` variable and having to query the service here again to check for a null DSE, we could have an enum with the 3 options: google, null and other and store that in an ivar
return _identityManager->HasPrimaryAccount(signin::ConsentLevel::kSignin)nit: we could create a `isSignedIn` local variable to improve readability
identity_manager_ = IdentityManagerFactory::GetForProfile(profile_.get());
browsing_data_remover_ =
BrowsingDataRemoverFactory::GetForProfile(profile_.get());
discover_feed_service_ =
DiscoverFeedServiceFactory::GetForProfile(profile_.get());Why do we need these changes?
tracker_ =Same, this change doesn't seem needed
raw_ptr<history::HistoryService> history_service_;
scoped_refptr<password_manager::MockPasswordStoreInterface> password_store_;
FakeBrowsingDataCounterWrapperProducer*
fake_browsing_data_counter_wrapper_producer_;
SceneState* scene_state_;Any reasons for re-ordering these?
void setDSEToNonGoogle() {In C++, only the first letter of the acronym is capitalized (same for the other functions below)
void setDSEToNonGoogle() {C++ functions start with a capital letter (same for the other functions below)
AuthenticationService* auth_service() {
return AuthenticationServiceFactory::GetForProfile(profile_.get());
}Functions usually go above ivars. Also, other services that need to used in the tests have an ivar (e.g., `history_service_`). Is there a reason for doing this differently for the auth service?
// Tests that `setPasswordsSelection` is not called when the feature
// `kPasswordRemovalFromDeleteBrowsingData` is enabled.Sorry if my comment wasn't clear, but I think mentioning the feature in tests like this one is actually useful because we are comparing the behaviour with and without the flag. Mentioning the feature helps understanding what's being tested. Otherwise, testing that "something is not called" without more context can be confusing to the reader.
Whereas with tests like `TestStringsWhenDSEIsGoogleAndUserIsSignedOut`, where we test something that's brand new an only apply when the feature is enabled, mentioning "when X feature is enabled" kind of gives the impression that we would have a test case testing the opposite
CreateMediator(template_url_service_);To prevent having to pass the service every time, we have to options:
1) Give a default `nullptr` value to the `template_url_service` argument of `CreateMediator`. The mediator is expected to be working with a null `TemplateURLService` [[ref](https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/settings/ui_bundled/clear_browsing_data/coordinator/quick_delete_mediator.h;l=5?q=quick_delete_medi&sq=#:~:text=%60templateURLService%60%20can,//%20be%20null%20if%20not%20needed.)], so we could leave it as null for tests that don't require it.
2) Add another `CreateMediator` (without any arguments), that will simply call `CreateMediator(template_url_service_)`
verifyStringsWhenDSEisGoogleAndUserIsSignedOut();We should instead declare a stub for `[consumer_ setManageOtherDataTitle:]` and`[consumer_ setManageOtherDataSubtitle:]`. We shouldn't be checking those in every test
// Set OCMExpect for when DSE is Google.
verifyStringsWhenDSEisGoogleAndUserIsSignedOut();We don't have to validate the initial state every time
Related details
- Angela Novakovic
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. |
Angela Novakovic (Gerrit)
Angela Novakovic added 14 comments![]( "Open in Gerrit")
// This boolean tells if user's default search engine is Google.nit
```suggestion
// This boolean tells if the user's default search engine is Google.
```
Done
if (IsPasswordRemovalFromDeleteBrowsingDataEnabled()) {
_defaultSearchEngineIsGoogle =
IsGoogleTheDefaultSearchEngine(_templateURLService);
[_consumer setManageOtherDataTitle:[self manageOtherDataTitle]];
[_consumer setManageOtherDataSubtitle:[self manageOtherDataSubtitle]];
}in the init methods, we set `_defaultSearchEngineIsGoogle` regardless of the flag's state. I think it would make more sense to be consistent and to just guard the calls to `setManageOtherDataTitle` and `setManageOtherDataSubtitle` like we did elsewhere
Done
if (!_templateURLService ||
!_templateURLService->GetDefaultSearchProvider()) {nit: instead of having a `_defaultSearchEngineIsGoogle` variable and having to query the service here again to check for a null DSE, we could have an enum with the 3 options: google, null and other and store that in an ivar
Done
return _identityManager->HasPrimaryAccount(signin::ConsentLevel::kSignin)nit: we could create a `isSignedIn` local variable to improve readability
Done
identity_manager_ = IdentityManagerFactory::GetForProfile(profile_.get());
browsing_data_remover_ =
BrowsingDataRemoverFactory::GetForProfile(profile_.get());
discover_feed_service_ =
DiscoverFeedServiceFactory::GetForProfile(profile_.get());Why do we need these changes?
We do not need them. I had a different idea at the time. I am reverting this change.
Same, this change doesn't seem needed
Done
raw_ptr<history::HistoryService> history_service_;
scoped_refptr<password_manager::MockPasswordStoreInterface> password_store_;
FakeBrowsingDataCounterWrapperProducer*
fake_browsing_data_counter_wrapper_producer_;
SceneState* scene_state_;Any reasons for re-ordering these?
I was trying to put it in somewhat of an order and group the raw_ptrs together.
C++ functions start with a capital letter (same for the other functions below)
Done
In C++, only the first letter of the acronym is capitalized (same for the other functions below)
Done
AuthenticationService* auth_service() {
return AuthenticationServiceFactory::GetForProfile(profile_.get());
}Functions usually go above ivars. Also, other services that need to used in the tests have an ivar (e.g., `history_service_`). Is there a reason for doing this differently for the auth service?
Done
// Tests that `setPasswordsSelection` is not called when the feature
// `kPasswordRemovalFromDeleteBrowsingData` is enabled.Sorry if my comment wasn't clear, but I think mentioning the feature in tests like this one is actually useful because we are comparing the behaviour with and without the flag. Mentioning the feature helps understanding what's being tested. Otherwise, testing that "something is not called" without more context can be confusing to the reader.
Whereas with tests like `TestStringsWhenDSEIsGoogleAndUserIsSignedOut`, where we test something that's brand new an only apply when the feature is enabled, mentioning "when X feature is enabled" kind of gives the impression that we would have a test case testing the opposite
Done
To prevent having to pass the service every time, we have to options:
1) Give a default `nullptr` value to the `template_url_service` argument of `CreateMediator`. The mediator is expected to be working with a null `TemplateURLService` [[ref](https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/settings/ui_bundled/clear_browsing_data/coordinator/quick_delete_mediator.h;l=5?q=quick_delete_medi&sq=#:~:text=%60templateURLService%60%20can,//%20be%20null%20if%20not%20needed.)], so we could leave it as null for tests that don't require it.2) Add another `CreateMediator` (without any arguments), that will simply call `CreateMediator(template_url_service_)`
Done
We should instead declare a stub for `[consumer_ setManageOtherDataTitle:]` and`[consumer_ setManageOtherDataSubtitle:]`. We shouldn't be checking those in every test
Discussed offline - reached conclusion that switching to a non-strict OCM Protocol is the better approach in this case to avoid repeated code.
// Set OCMExpect for when DSE is Google.
verifyStringsWhenDSEisGoogleAndUserIsSignedOut();We don't have to validate the initial state every time
- Noémie St-Onge
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noémie St-Onge (Gerrit)
Noémie St-Onge added 17 comments![]( "Open in Gerrit")
// Default Search Engine States.```suggestion
// Default search engine states.
```
typedef NS_ENUM(NSInteger, DefaultSearchEngine) {```suggestion
typedef NS_ENUM(NSInteger, DefaultSearchEngineState) {
```
typedef NS_ENUM(NSInteger, DefaultSearchEngine) {
DefaultSearchEngineIsGoogle,
DefaultSearchEngineIsNotGoogle,
DefaultSearchEngineIsNull,
};Prefer C++ enums go/bling-best-practices#ns-enum-vs-c-enums
NSInteger SetDefaultSearchEngine(TemplateURLService* template_url_service) {This function doesn't "set" anything. We can use "get" instead
NSInteger SetDefaultSearchEngine(TemplateURLService* template_url_service) {I would keep the word "state". It otherwise sounds like we are setting the search engine in the model. Same for the comment
```suggestion
NSInteger SetDefaultSearchEngineState(TemplateURLService* template_url_service) {
```
NSInteger _defaultSearchEngineStatus;Use the enum as the type
// When the default search engine changes, the consumer is called in order to
// update the strings for the "Manage other data" cell.We shouldn't document protocol methods that we're implementing because they're already (or at least should be) documented in the protocol / observer class. You can add comments in the body though
#import "ios/chrome/browser/shared/model/application_context/application_context.h"Do we need this import? If not, please also clean up the build file
// other data" cell are given to the consumer when the DSE is not Google. The
// string doesn't change based on the sign-in status.This doesn't add value IMO. If anything, I feel like it might be confusing to talk about the sign-in status here while there's nothing in this function that relates to it.
CreateMediator(template_url_service_);The purpose of having a default value for an argument is so that we can change the value when needed, without having to pass an argument each time given that the value is the same in most cases. Here, we have more `CreateMediator(template_url_service_)` calls than `CreateMediator()` calls, so the default value isn't really helping us. If we want to pass a valid template URL service in most cases, then we should go with the option of having 2 `CreateMediator` methods: one that takes is a parameter and another that call the first one with the `template_url_service_` as argument
// Set OCMExpect for when DSE is Google.```suggestion
// Set expectations on `consumer_` for when the DSE is Google and the user is signed out.
```
Same for other similar comments
mediator_.consumer = consumer_;
EXPECT_OCMOCK_VERIFY(consumer_);skip line to be consistent with other test cases. Same for other occurrences below
// Verifies that the consumer receives the correct title and subtitle for thewe're only checking the subtitle here because we are setting the expectation after a **change** to the sign-in status. So this is basically testing that the consumer is updated on sign-in status updates, which ends up being really similar to `TestStringsWhenSignInStatusChangesAndDSEIsNull`. `TestStringsWhenDSEIsNullAndUserIsSignedIn` and `TestStringsWhenSignInStatusChangesAndDSEIsNull` are actually the exact same.
To test the various DSE/sign-in status combinations, we can set the states **before** creating the mediator, and then check that the title and subtitle are correct after setting the consumer.
Then, to test that the consumer is updated when there are changes later on to the DSE or sign-in status, we can have 1 test for each and check that the expected methods are being called:
- For a DSE change:
- `OCMExpect([consumer_ setManageOtherDataTitle:[OCMArg any]])`
- `OCMExpect([consumer_ setManageOtherDataSubtitle:[OCMArg any]])`
- For a sign-in status change:
- `OCMReject([consumer_ setManageOtherDataTitle:[OCMArg any]])`
- `OCMExpect([consumer_ setManageOtherDataSubtitle:[OCMArg any]])`
// Verify that Google is the default search provider.```suggestion
// Verify that Google is the default search provider initially.
```
VerifyStringsWhenDseIsNotGoogle();
// Change the default search provider to a non-Google one.skip line for consistency
VerifyStringsWhenDseIsGoogleAndUserIsSignedOut();
// Change the default search provider back to Google.skip line for consistency
// Create the mediator with a nullptr for the templateURLService.```suggestion
// Create the mediator with a null template URL service. This will result in getting a null DSE.
```
Related details
- Angela Novakovic
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. |
Angela Novakovic (Gerrit)
Angela Novakovic added 17 comments![]( "Open in Gerrit")
```suggestion
// Default search engine states.
```
Done
typedef NS_ENUM(NSInteger, DefaultSearchEngine) {Angela Novakovic```suggestion
typedef NS_ENUM(NSInteger, DefaultSearchEngineState) {
```
Done
typedef NS_ENUM(NSInteger, DefaultSearchEngine) {
DefaultSearchEngineIsGoogle,
DefaultSearchEngineIsNotGoogle,
DefaultSearchEngineIsNull,
};Angela NovakovicPrefer C++ enums go/bling-best-practices#ns-enum-vs-c-enums
Done
NSInteger SetDefaultSearchEngine(TemplateURLService* template_url_service) {I would keep the word "state". It otherwise sounds like we are setting the search engine in the model. Same for the comment
```suggestion
NSInteger SetDefaultSearchEngineState(TemplateURLService* template_url_service) {
```
Done
NSInteger SetDefaultSearchEngine(TemplateURLService* template_url_service) {This function doesn't "set" anything. We can use "get" instead
Done
Use the enum as the type
Done
// When the default search engine changes, the consumer is called in order to
// update the strings for the "Manage other data" cell.We shouldn't document protocol methods that we're implementing because they're already (or at least should be) documented in the protocol / observer class. You can add comments in the body though
Done
#import "ios/chrome/browser/shared/model/application_context/application_context.h"Do we need this import? If not, please also clean up the build file
We need it for `FakeSystemIdentityManager* system_identity_manager =
FakeSystemIdentityManager::FromSystemIdentityManager(
GetApplicationContext()->GetSystemIdentityManager());`
// other data" cell are given to the consumer when the DSE is not Google. The
// string doesn't change based on the sign-in status.This doesn't add value IMO. If anything, I feel like it might be confusing to talk about the sign-in status here while there's nothing in this function that relates to it.
Done
CreateMediator(template_url_service_);The purpose of having a default value for an argument is so that we can change the value when needed, without having to pass an argument each time given that the value is the same in most cases. Here, we have more `CreateMediator(template_url_service_)` calls than `CreateMediator()` calls, so the default value isn't really helping us. If we want to pass a valid template URL service in most cases, then we should go with the option of having 2 `CreateMediator` methods: one that takes is a parameter and another that call the first one with the `template_url_service_` as argument
If there were more calls that needed `TemplateURLService` to be null, then the first option would have been better, or would it still have been fine if I had set the default value to be `template_url_service_` instead of nullptr?
```suggestion
// Set expectations on `consumer_` for when the DSE is Google and the user is signed out.
```Same for other similar comments
Done
mediator_.consumer = consumer_;
EXPECT_OCMOCK_VERIFY(consumer_);skip line to be consistent with other test cases. Same for other occurrences below
Done
// Verifies that the consumer receives the correct title and subtitle for thewe're only checking the subtitle here because we are setting the expectation after a **change** to the sign-in status. So this is basically testing that the consumer is updated on sign-in status updates, which ends up being really similar to `TestStringsWhenSignInStatusChangesAndDSEIsNull`. `TestStringsWhenDSEIsNullAndUserIsSignedIn` and `TestStringsWhenSignInStatusChangesAndDSEIsNull` are actually the exact same.
To test the various DSE/sign-in status combinations, we can set the states **before** creating the mediator, and then check that the title and subtitle are correct after setting the consumer.
Then, to test that the consumer is updated when there are changes later on to the DSE or sign-in status, we can have 1 test for each and check that the expected methods are being called:
- For a DSE change:
- `OCMExpect([consumer_ setManageOtherDataTitle:[OCMArg any]])`
- `OCMExpect([consumer_ setManageOtherDataSubtitle:[OCMArg any]])`
- For a sign-in status change:
- `OCMReject([consumer_ setManageOtherDataTitle:[OCMArg any]])`
- `OCMExpect([consumer_ setManageOtherDataSubtitle:[OCMArg any]])`
This is very clear! Thank you! 😊
For DSE and sign-in change, since we just want to verify that the appropriate methods are called/rejected, we do not care about the arguments given to `setManageOtherDataTitle` and `setManageOtherDataSubtitle` as we've already tested that the consumer receives the correct strings based on the DSE and sign-in state, right?
// Verify that Google is the default search provider.```suggestion
// Verify that Google is the default search provider initially.
```
Done
VerifyStringsWhenDseIsNotGoogle();
// Change the default search provider to a non-Google one.Angela Novakovicskip line for consistency
Done
VerifyStringsWhenDseIsGoogleAndUserIsSignedOut();
// Change the default search provider back to Google.Angela Novakovicskip line for consistency
Done
// Create the mediator with a nullptr for the templateURLService.```suggestion
// Create the mediator with a null template URL service. This will result in getting a null DSE.
```
- Noémie St-Onge
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. |
Noémie St-Onge (Gerrit)
Noémie St-Onge added 2 comments![]( "Open in Gerrit")
CreateMediator(template_url_service_);Angela NovakovicThe purpose of having a default value for an argument is so that we can change the value when needed, without having to pass an argument each time given that the value is the same in most cases. Here, we have more `CreateMediator(template_url_service_)` calls than `CreateMediator()` calls, so the default value isn't really helping us. If we want to pass a valid template URL service in most cases, then we should go with the option of having 2 `CreateMediator` methods: one that takes is a parameter and another that call the first one with the `template_url_service_` as argument
If there were more calls that needed `TemplateURLService` to be null, then the first option would have been better, or would it still have been fine if I had set the default value to be `template_url_service_` instead of nullptr?
we can't set an ivar as the default value, but if it was a static variable, then, yes, we could have used it as the default value
// Verifies that the consumer receives the correct title and subtitle for theAngela Novakovicwe're only checking the subtitle here because we are setting the expectation after a **change** to the sign-in status. So this is basically testing that the consumer is updated on sign-in status updates, which ends up being really similar to `TestStringsWhenSignInStatusChangesAndDSEIsNull`. `TestStringsWhenDSEIsNullAndUserIsSignedIn` and `TestStringsWhenSignInStatusChangesAndDSEIsNull` are actually the exact same.
To test the various DSE/sign-in status combinations, we can set the states **before** creating the mediator, and then check that the title and subtitle are correct after setting the consumer.
Then, to test that the consumer is updated when there are changes later on to the DSE or sign-in status, we can have 1 test for each and check that the expected methods are being called:
- For a DSE change:
- `OCMExpect([consumer_ setManageOtherDataTitle:[OCMArg any]])`
- `OCMExpect([consumer_ setManageOtherDataSubtitle:[OCMArg any]])`
- For a sign-in status change:
- `OCMReject([consumer_ setManageOtherDataTitle:[OCMArg any]])`
- `OCMExpect([consumer_ setManageOtherDataSubtitle:[OCMArg any]])`
This is very clear! Thank you! 😊
For DSE and sign-in change, since we just want to verify that the appropriate methods are called/rejected, we do not care about the arguments given to `setManageOtherDataTitle` and `setManageOtherDataSubtitle` as we've already tested that the consumer receives the correct strings based on the DSE and sign-in state, right?
For the expected calls, we can pass the expected string instead of `[OCMArg any]` to validate that the `isSignedIn` & `_defaultSearchEngineState` have been updated, but, IMO, it would be redundant to also test the consumer update with every DSE/Sign-in status combination
Related details
- Angela Novakovic
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. |
Angela Novakovic (Gerrit)
Angela Novakovic added 3 comments![]( "Open in Gerrit")
Please let me know if any modification is needed 😊
CreateMediator(template_url_service_);Angela NovakovicThe purpose of having a default value for an argument is so that we can change the value when needed, without having to pass an argument each time given that the value is the same in most cases. Here, we have more `CreateMediator(template_url_service_)` calls than `CreateMediator()` calls, so the default value isn't really helping us. If we want to pass a valid template URL service in most cases, then we should go with the option of having 2 `CreateMediator` methods: one that takes is a parameter and another that call the first one with the `template_url_service_` as argument
Noémie St-OngeIf there were more calls that needed `TemplateURLService` to be null, then the first option would have been better, or would it still have been fine if I had set the default value to be `template_url_service_` instead of nullptr?
we can't set an ivar as the default value, but if it was a static variable, then, yes, we could have used it as the default value
Acknowledged
// Verifies that the consumer receives the correct title and subtitle for theAngela Novakovicwe're only checking the subtitle here because we are setting the expectation after a **change** to the sign-in status. So this is basically testing that the consumer is updated on sign-in status updates, which ends up being really similar to `TestStringsWhenSignInStatusChangesAndDSEIsNull`. `TestStringsWhenDSEIsNullAndUserIsSignedIn` and `TestStringsWhenSignInStatusChangesAndDSEIsNull` are actually the exact same.
To test the various DSE/sign-in status combinations, we can set the states **before** creating the mediator, and then check that the title and subtitle are correct after setting the consumer.
Then, to test that the consumer is updated when there are changes later on to the DSE or sign-in status, we can have 1 test for each and check that the expected methods are being called:
- For a DSE change:
- `OCMExpect([consumer_ setManageOtherDataTitle:[OCMArg any]])`
- `OCMExpect([consumer_ setManageOtherDataSubtitle:[OCMArg any]])`
- For a sign-in status change:
- `OCMReject([consumer_ setManageOtherDataTitle:[OCMArg any]])`
- `OCMExpect([consumer_ setManageOtherDataSubtitle:[OCMArg any]])`
Noémie St-OngeThis is very clear! Thank you! 😊
For DSE and sign-in change, since we just want to verify that the appropriate methods are called/rejected, we do not care about the arguments given to `setManageOtherDataTitle` and `setManageOtherDataSubtitle` as we've already tested that the consumer receives the correct strings based on the DSE and sign-in state, right?
For the expected calls, we can pass the expected string instead of `[OCMArg any]` to validate that the `isSignedIn` & `_defaultSearchEngineState` have been updated, but, IMO, it would be redundant to also test the consumer update with every DSE/Sign-in status combination
Perfect. I've done that. I've only left `OCMReject` with `[OCMArg any]` as I would not expect any calls to be made to change the title when the sign-in status changes.
Related details
- Noémie St-Onge
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noémie St-Onge (Gerrit)
Noémie St-Onge added 11 comments![]( "Open in Gerrit")
// Get the user's default search engine state.`Gets` or `Returns`
if (!template_url_service ||
!template_url_service->GetDefaultSearchProvider()) {
return DefaultSearchEngineState::kNull;
}
if (template_url_service->GetDefaultSearchProvider()->GetEngineType(
template_url_service->search_terms_data()) == SEARCH_ENGINE_GOOGLE) {
return DefaultSearchEngineState::kGoogle;
} else {
return DefaultSearchEngineState::kNotGoogle;
}Same as in https://chromium-review.googlesource.com/c/chromium/src/+/7489686/comment/bd04d030_d3ee0c20/. Please remove this function and the enum if the other CL ends up landing first
```suggestion
if (!template_url_service ||
!template_url_service->GetDefaultSearchProvider()) {
return DefaultSearchEngineState::kNull;
}
if (template_url_service->GetDefaultSearchProvider()->GetEngineType(
template_url_service->search_terms_data()) == SEARCH_ENGINE_GOOGLE) {
return DefaultSearchEngineState::kGoogle;
}
return DefaultSearchEngineState::kNotGoogle;
```
DefaultSearchEngineState _defaultSearchEngineStatus;Let's stick with the word `state` for consistency (same in the comment)
// Used to get the user's current sign-in status.This is not used to "get" the status, it is just storing the status
BOOL shouldShowFooter = _isSignedIn;
[_consumer setShouldShowFooter:shouldShowFooter];Let's just pass `_isSignedIn` to `setShouldShowFooter`. The `shouldShowFooter` variable doesn't add value IMO
void CreateMediator(raw_ptr<TemplateURLService> template_url_service) {Add a comment
void CreateMediator() { CreateMediator(template_url_service_); }Add a comment
Let's keep this empty line for consistency with other test cases
TEST_F(QuickDeleteMediatorTest, TestStringsWhenDseIsGoogleAndUserIsSignedOut) {I think this test and the other ones that check the strings for a given DSE/Sign-in status combination could be grouped into a single parametrized test case https://engdoc.corp.google.com/eng/doc/devguide/testing/parameterized-testing.md?cl=head&polyglot=cpp
This would look something like this:
```
struct ManageOtherDataTestParams {
std::string test_name;
DefaultSearchEngineState dse_state;
bool signed_in;
int expected_title_id;
int expected_subtitle_id;
};
class QuickDeleteMediatorManageOtherDataTest
: public QuickDeleteMediatorTest,
public ::testing::WithParamInterface<ManageOtherDataTestParams> {};
TEST_P(QuickDeleteMediatorManageOtherDataTest, TestStrings) {
base::test::ScopedFeatureList feature_list(
kPasswordRemovalFromDeleteBrowsingData);
const ManageOtherDataTestParams& params = GetParam();// Set up DSE.
switch (params.dse_state) {
case DefaultSearchEngineState::kNull:
CreateMediator(/*template_url_service=*/nullptr);
break;
case DefaultSearchEngineState::kGoogle:
CreateMediator(); // Google is default in test env.
break;
case DefaultSearchEngineState::kNotGoogle:
SetDseToNonGoogle();
CreateMediator();
break;
}
// Set up sign-in status.
if (params.signed_in) {
auth_service_->SignIn(fake_identity_,
signin_metrics::AccessPoint::kSettings);
}
OCMExpect([consumer_ setManageOtherDataTitle:l10n_util::GetNSString(
params.expected_title_id)]);
OCMExpect(
[consumer_ setManageOtherDataSubtitle:l10n_util::GetNSString(
params.expected_subtitle_id)]);
mediator_.consumer = consumer_;
EXPECT_OCMOCK_VERIFY(consumer_);
}
INSTANTIATE_TEST_SUITE_P(
AllVariants,
QuickDeleteMediatorManageOtherDataTest,
::testing::ValuesIn<ManageOtherDataTestParams>({
{.test_name = "Google_SignedOut",
.dse_state = DefaultSearchEngineState::kGoogle,
.signed_in = false,
.expected_title_id = IDS_SETTINGS_MANAGE_OTHER_GOOGLE_DATA_LABEL,
.expected_subtitle_id = IDS_SETTINGS_MANAGE_PASSWORDS_SUB_LABEL},
{.test_name = "Google_SignedIn",
.dse_state = DefaultSearchEngineState::kGoogle,
.signed_in = true,
.expected_title_id = IDS_SETTINGS_MANAGE_OTHER_GOOGLE_DATA_LABEL,
.expected_subtitle_id = IDS_SETTINGS_MANAGE_OTHER_DATA_SUB_LABEL}, {.test_name = "NonGoogle",
.dse_state = DefaultSearchEngineState::kNotGoogle,
.signed_in = false,
.expected_title_id = IDS_SETTINGS_MANAGE_OTHER_DATA_LABEL,
.expected_subtitle_id = IDS_SETTINGS_MANAGE_OTHER_DATA_SUB_LABEL}, {.test_name = "NullDSE_SignedOut",
.dse_state = DefaultSearchEngineState::kNull,
.signed_in = false,
.expected_title_id = IDS_SETTINGS_MANAGE_OTHER_DATA_LABEL,
.expected_subtitle_id = IDS_SETTINGS_MANAGE_PASSWORDS_SUB_LABEL}, {.test_name = "NullDSE_SignedIn",
.dse_state = DefaultSearchEngineState::kNull,
.signed_in = true,
.expected_title_id = IDS_SETTINGS_MANAGE_OTHER_DATA_LABEL,
.expected_subtitle_id =
IDS_IOS_CLEAR_BROWSING_DATA_MANAGE_OTHER_DATA_SUBTITLE_UNKNOWN_DSE},
}),
[](const ::testing::TestParamInfo<ManageOtherDataTestParams>& info) {
return info.param.test_name;
});
```// "Manage other data" cell when the user's sign-in status changes and the DSE
// is Google.nit: I would remove this part from the comment as it sounds like an important setup of this test. It is going to impact the expected string, but here the scope is checking a sign-in status change
OCMReject([consumer_ setManageOtherDataTitle:[OCMArg any]]);I would add a comment like: `A change in the sign-in status doesn't impact the title`
Related details
- Angela Novakovic
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. |
Angela Novakovic (Gerrit)
Angela Novakovic added 11 comments![]( "Open in Gerrit")
// Get the user's default search engine state.Angela Novakovic`Gets` or `Returns`
Done
if (!template_url_service ||
!template_url_service->GetDefaultSearchProvider()) {
return DefaultSearchEngineState::kNull;
}
if (template_url_service->GetDefaultSearchProvider()->GetEngineType(
template_url_service->search_terms_data()) == SEARCH_ENGINE_GOOGLE) {
return DefaultSearchEngineState::kGoogle;
} else {
return DefaultSearchEngineState::kNotGoogle;
}Same as in https://chromium-review.googlesource.com/c/chromium/src/+/7489686/comment/bd04d030_d3ee0c20/. Please remove this function and the enum if the other CL ends up landing first
```suggestion
if (!template_url_service ||
!template_url_service->GetDefaultSearchProvider()) {
return DefaultSearchEngineState::kNull;
}
if (template_url_service->GetDefaultSearchProvider()->GetEngineType(
template_url_service->search_terms_data()) == SEARCH_ENGINE_GOOGLE) {
return DefaultSearchEngineState::kGoogle;
}
return DefaultSearchEngineState::kNotGoogle;
```
Done
DefaultSearchEngineState _defaultSearchEngineStatus;Let's stick with the word `state` for consistency (same in the comment)
Done
This is not used to "get" the status, it is just storing the status
Done
BOOL shouldShowFooter = _isSignedIn;
[_consumer setShouldShowFooter:shouldShowFooter];Let's just pass `_isSignedIn` to `setShouldShowFooter`. The `shouldShowFooter` variable doesn't add value IMO
Agree, I have removed it.
void CreateMediator(raw_ptr<TemplateURLService> template_url_service) {Angela NovakovicAdd a comment
Done
void CreateMediator() { CreateMediator(template_url_service_); }Angela NovakovicAdd a comment
Done
Let's keep this empty line for consistency with other test cases
Done
TEST_F(QuickDeleteMediatorTest, TestStringsWhenDseIsGoogleAndUserIsSignedOut) {Done
// "Manage other data" cell when the user's sign-in status changes and the DSE
// is Google.nit: I would remove this part from the comment as it sounds like an important setup of this test. It is going to impact the expected string, but here the scope is checking a sign-in status change
Done
OCMReject([consumer_ setManageOtherDataTitle:[OCMArg any]]);I would add a comment like: `A change in the sign-in status doesn't impact the title`
- Noémie St-Onge
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noémie St-Onge (Gerrit)
Noémie St-Onge added 7 comments![]( "Open in Gerrit")
namespace {
using quick_delete_util::DefaultSearchEngineState;
} // namespaceno need to wrap the using declaration in a namespace
// Creates the QuickDeleteMediator. `template_url_service` can be a nullptr.```suggestion
// Creates a QuickDeleteMediator with a given `template_url_service`. `template_url_service` can be a nullptr.
```
// Creates the QuickDeleteMediator with a default value of
// `template_url_service_` for the `templateURLService`.```suggestion
// Creates a QuickDeleteMediator with a valid template URL service.
```
void VerifyStringsWhenDseIsGoogleAndUserIsSignedOut() {This function is now only used once. I would remove it as I feel like it would be clearer to see the OCMExpects directly in the test
void VerifyStringsWhenDseIsNotGoogle() {Same, this function is now only used once. I would remove it as I feel like it would be clearer to see the OCMExpects directly in the test
struct ManageOtherDataTestParams {Add comment
class QuickDeleteMediatorManageOtherDataTestAdd comment
Related details
- Angela Novakovic
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. |
Angela Novakovic (Gerrit)
Angela Novakovic added 7 comments![]( "Open in Gerrit")
namespace {
using quick_delete_util::DefaultSearchEngineState;
} // namespaceno need to wrap the using declaration in a namespace
Done
// Creates the QuickDeleteMediator. `template_url_service` can be a nullptr.```suggestion
// Creates a QuickDeleteMediator with a given `template_url_service`. `template_url_service` can be a nullptr.
```
Done
// Creates the QuickDeleteMediator with a default value of
// `template_url_service_` for the `templateURLService`.```suggestion
// Creates a QuickDeleteMediator with a valid template URL service.
```
Done
void VerifyStringsWhenDseIsGoogleAndUserIsSignedOut() {This function is now only used once. I would remove it as I feel like it would be clearer to see the OCMExpects directly in the test
Done
Same, this function is now only used once. I would remove it as I feel like it would be clearer to see the OCMExpects directly in the test
Done
struct ManageOtherDataTestParams {Angela NovakovicAdd comment
Done
class QuickDeleteMediatorManageOtherDataTestAngela NovakovicAdd comment
Done
Related details
- Noémie St-Onge
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |