[iOS][PRDBD] Implement QuickDeleteOtherDataViewController [chromium/src : main]
Angela Novakovic (Gerrit)
Angela Novakovic added 13 comments![]( "Open in Gerrit")
[iOS][PRDBD] Implements QuickDeleteOtherDataVCAngela NovakovicIt is an observation of mine that people don't use third person singular in the title (Git does not have the sense of a title, however, this first line is used as Gerrit CL's title.) I found it make the title simpler. Where is `QuickDeleteOtherDataVC` coming from?
`QuickDeleteOtherDataVC` is the view controller of `QuickDeleteOtherDataCoordinator`. I've abbreviated ViewController as VC several times in my CL titles, but in this case as I do have space, I do agree that it gives more clarity to write the whole name. 😊
This CL implements the QuickDeleteOtherDataViewController to displayAngela NovakovicIf `QuickDeleteOtherDataVC` is referring to `QuickDeleteOtherDataViewController`, then the abbreviation at the end is not suitable. You can either use the full name, or use a descriptive name, like the view controller for .... The partial abbreviation of using "VC" for "ViewController" can cause readability issues.
Done
Fixed: 464551506, 464552107
Bug:482036587Angela NovakovicSo, this CL fixes 464551506, 464552107, as well as 482036587 (I normally don't see a CL that fixes this much issues). If it is, then they can put into one line. Also, please add a space after `Bug:`. Having a space is better for readability, and also consistency is important. In two lines, there is `Fixed:<space>` and immediately after that `Bug:<no space>`.
This CL doesn't fix 482036587, it mentions a TODO with this bug number.
// The accessibility identifier for the passwords and passkeys cell in the quickAngela NovakovicIn the screenshot, it only shows "Passwords". The three identifiers defined below, two match the screenshot. If "kQuickDeleteOtherDataPasswordIdentifier" is to follow the same patter, it should be "...PasswordsIde...", with the "s".
It is better to put a blank line in-between them. I know that you are putting them together to indicate they are used in the same file. But from a reader's perspective, they are not related, so this reason of this grouping isn't obvious, and adding a line makes the code easier to read and also follows the established pattern above those newly added lines.
Done
NSString* const kQuickDeleteOtherDataPasswordIdentifier =
@"QuickDeleteOtherDataPasswordsIdentifier";Angela Novakovic"kQuickDeleteOtherDataPasswordIdentifier" uses "Password", while its value uses "Passwords".
Done
// Open quick delete browsing data page.Angela Novakovic"Open" -> "Opens"
Done
grey_accessibilityTrait(UIAccessibilityTraitButton), nil);Angela NovakovicA quick question: since kQuickDeleteManageOtherDataCellIdentifier can already uniquely identify the element, is the `grey_accessibilityTrait` needed here? If the intention is to also verify the accessibility trait for an element, it should be done in a unit test. Unit tests are much faster and can verify in depth.
You are correct, it is not needed.
- (AppLaunchConfiguration)appConfigurationForTestCase {
AppLaunchConfiguration config;
config.relaunch_policy = NoForceRelaunchAndResetState;
config.additional_args.push_back(std::string("--") +
syncer::kSyncShortNudgeDelayForTest);
config.features_enabled.push_back(kPasswordRemovalFromDeleteBrowsingData);
return config;
}Angela NovakovicDo you plan to add more EG test with the feature disabled for some tests? It is generally a good idea to test the UI with a feature flag on and off to make sure the feature flag can be enabled and disabled, and the app still functions as expected. It may not be in this file.
I was not planning on adding tests for when the feature flag is disabled as this view is new and cannot be accessed when the feature flag is disabled (it is accessed via the "Manage other data" cell which is only available when the feature flag is enabled).
Happy to explore this if you think that there should be a test for the "Other Data" page flow for when the feature flag is disabled. 😊
- (void)signIn {Angela NovakovicPlease double check whether this `signin` method is used.
I've added it too soon indeed. It will be needed in another CL. I have removed it as it is not needed here.
// Open quick delete other data page.Angela Novakovic"Opens"? Please also review comments after this line.
Done
// Tests that the "Search History" cell is visible.
- (void)testSearchHistoryVisibility {
// Open quick delete other data page.
[self openQuickDeleteOtherDataPage];
[[EarlGrey selectElementWithMatcher:SearchHistoryCellMatcher()]
assertWithMatcher:grey_sufficientlyVisible()];
}
// Tests that the "My activity" cell is visible.
- (void)testMyActivityCellVisibility {
// Open quick delete other data page.
[self openQuickDeleteOtherDataPage];
[[EarlGrey selectElementWithMatcher:MyActivityCellMatcher()]
assertWithMatcher:grey_sufficientlyVisible()];
}
// Tests that the footer is visible.
- (void)testFooterVisibility {
// Open quick delete other data page.
[self openQuickDeleteOtherDataPage];
[[EarlGrey selectElementWithMatcher:FooterMatcher()]
assertWithMatcher:grey_sufficientlyVisible()];
}Angela NovakovicWhat is the reason to have three tests when all three elements should be visible at the same time?
That is currently accurate, however, later the cells will not be visible at the same time if the DSE is not Google or the user is not signed in.
I will put them together as one test at this time and modify in a following CL. 😊
#pragma mark - UIViewControllerAngela Novakovic`UIViewController` is not suitable here. If you were to say, this section is about life cycle management, then use that. Just listing `UIViewController` is not clear about the intention.
This seems to be norm in other files. I also see that in some files, we omit the pragma mark, and in other we have a different pragma mark after the `viewDidLoad` such as `#pragma mark - LegacyChromeTableViewController`. In this case, we are using the `UITableViewDiffableDataSource`, so perhaps putting that as a pragma mark before loadModel might help?
What would you suggest in this case? 😊
createCellWithTitle:@"Passwords"
subtitle:@"Change this"Angela NovakovicWe don't normally put hard coded string in code with the intention of merging it.
I can wait to have the other CL finalized and reviewed before merging this one alternatively. 😊
Related details
- Leo Zhao
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. |
Leo Zhao (Gerrit)
Leo Zhao added 2 comments![]( "Open in Gerrit")
- (AppLaunchConfiguration)appConfigurationForTestCase {
AppLaunchConfiguration config;
config.relaunch_policy = NoForceRelaunchAndResetState;
config.additional_args.push_back(std::string("--") +
syncer::kSyncShortNudgeDelayForTest);
config.features_enabled.push_back(kPasswordRemovalFromDeleteBrowsingData);
return config;
}Angela NovakovicDo you plan to add more EG test with the feature disabled for some tests? It is generally a good idea to test the UI with a feature flag on and off to make sure the feature flag can be enabled and disabled, and the app still functions as expected. It may not be in this file.
I was not planning on adding tests for when the feature flag is disabled as this view is new and cannot be accessed when the feature flag is disabled (it is accessed via the "Manage other data" cell which is only available when the feature flag is enabled).
Happy to explore this if you think that there should be a test for the "Other Data" page flow for when the feature flag is disabled. 😊
What I ment was, currently, `kPasswordRemovalFromDeleteBrowsingData` is disabled by default. Here, the EG tests run with it enabled. What happens if this feature is disabled, which is the condition that the app is running? You can add a test with the feature disabled, then open the tools menu, and verify other buttons are there, however, the button to open the quick delete browsing data page isn't there.
#pragma mark - UIViewControllerAngela Novakovic`UIViewController` is not suitable here. If you were to say, this section is about life cycle management, then use that. Just listing `UIViewController` is not clear about the intention.
This seems to be norm in other files. I also see that in some files, we omit the pragma mark, and in other we have a different pragma mark after the `viewDidLoad` such as `#pragma mark - LegacyChromeTableViewController`. In this case, we are using the `UITableViewDiffableDataSource`, so perhaps putting that as a pragma mark before loadModel might help?
What would you suggest in this case? 😊
UIViewController is the root class of all view classes. `#pragma mark - LegacyChromeTableViewController.` is saying, this section contains overrides of `LegacyChromeTableViewController`. Having `#pragma mark - UIViewController` does not add extra information. You can add life cycle section, and put viewDidLoad in there, and also init, viewWillAppear, etc. Then you can create another section and put loadModels in there since that is not a life cycle method of UIViewController.
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")
- (AppLaunchConfiguration)appConfigurationForTestCase {
AppLaunchConfiguration config;
config.relaunch_policy = NoForceRelaunchAndResetState;
config.additional_args.push_back(std::string("--") +
syncer::kSyncShortNudgeDelayForTest);
config.features_enabled.push_back(kPasswordRemovalFromDeleteBrowsingData);
return config;
}Angela NovakovicDo you plan to add more EG test with the feature disabled for some tests? It is generally a good idea to test the UI with a feature flag on and off to make sure the feature flag can be enabled and disabled, and the app still functions as expected. It may not be in this file.
Leo ZhaoI was not planning on adding tests for when the feature flag is disabled as this view is new and cannot be accessed when the feature flag is disabled (it is accessed via the "Manage other data" cell which is only available when the feature flag is enabled).
Happy to explore this if you think that there should be a test for the "Other Data" page flow for when the feature flag is disabled. 😊
What I ment was, currently, `kPasswordRemovalFromDeleteBrowsingData` is disabled by default. Here, the EG tests run with it enabled. What happens if this feature is disabled, which is the condition that the app is running? You can add a test with the feature disabled, then open the tools menu, and verify other buttons are there, however, the button to open the quick delete browsing data page isn't there.
I understand now! We could do that, but this is already done in `QuickDeleteBrowsingDataTestCase` in `- (void)testManageOtherDataCellVisibility`. We verify whether the `Manage other data` cell is visible when the feature is enabled vs disabled. If the feature is disabled, the user won't see this cell/button and won't be able to reach `QuickDeleteOtherDataViewController`. Re-testing this in `QuickDeleteOtherDataTestCase` seems repetitive, but please correct me if I am misunderstanding! 😊
#pragma mark - UIViewControllerAngela Novakovic`UIViewController` is not suitable here. If you were to say, this section is about life cycle management, then use that. Just listing `UIViewController` is not clear about the intention.
Leo ZhaoThis seems to be norm in other files. I also see that in some files, we omit the pragma mark, and in other we have a different pragma mark after the `viewDidLoad` such as `#pragma mark - LegacyChromeTableViewController`. In this case, we are using the `UITableViewDiffableDataSource`, so perhaps putting that as a pragma mark before loadModel might help?
What would you suggest in this case? 😊
UIViewController is the root class of all view classes. `#pragma mark - LegacyChromeTableViewController.` is saying, this section contains overrides of `LegacyChromeTableViewController`. Having `#pragma mark - UIViewController` does not add extra information. You can add life cycle section, and put viewDidLoad in there, and also init, viewWillAppear, etc. Then you can create another section and put loadModels in there since that is not a life cycle method of UIViewController.
Okay, so from what I understand, adding the `#pragma mark - UIViewController` in this case does not provide more information, unless we have more methods of the life cycle such as viewDidLoad, innit, viewWillAppear, viewWillDisappear... as such, in this scenario, for now, it might be better to not add the pragma mark.
Also, the `loadModel` method is not part of the life cycle, thus would require a separate pragma mark.
Related details
- Leo Zhao
- 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. |
Leo Zhao (Gerrit)
Leo Zhao voted Code-Review+1![]( "Open in Gerrit")
- Angela Novakovic
- 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 23 comments![]( "Open in Gerrit")
Made a first pass, haven't looked at everything yet
New View: https://screenshot.googleplex.com/3kUTPVT5Dfkeha6.pngShouldn't the left navigation bar button be a back button? Also, shouldn't the string be "Passwords and passkeys"?
The figma also shows `Search history` (with a lower case `h`), we should validate we UX
extern NSString* const kQuickDeleteOtherDataPasswordsIdentifier;```suggestion
extern NSString* const kQuickDeleteOtherDataPasswordsAndPasskeysIdentifier;
```
@"QuickDeleteOtherDataPasswordsIdentifier";```suggestion
@"QuickDeleteOtherDataPasswordsAndPasskeysIdentifier";
```
id<GREYMatcher> quickDeleteOtherDataPageTitleMatcher() {C++ functions start with a capital letter
```suggestion
id<GREYMatcher> QuickDeleteOtherDataPageTitleMatcher() {
```
// Opens quick delete browsing data page.`Open` was fine, we use the imperative mood for implementation comments. Same for other occurrences
go/c-readability-advice#implementation-comment-conventions
// Tests that when user taps on the "Manage other data" cell, a new view, the
// Quick Delete Other Data page, opens.```suggestion
// Tests that tapping the "Manage other data" cell opens the Quick Delete Other Data page.
```
// Opens quick delete browsing data page.```suggestion
// Open the quick delete browsing data page.
```
[[EarlGrey selectElementWithMatcher:ManageOtherDataCellMatcher()]
assertWithMatcher:grey_sufficientlyVisible()];I don't think we need to check the visibility prior to tapping as tapping an element that's not visible will fail anyway
#import <XCTest/XCTest.h>
#import "base/strings/sys_string_conversions.h"
#import "components/browsing_data/core/browsing_data_utils.h"
#import "components/browsing_data/core/pref_names.h"
#import "components/signin/public/base/signin_metrics.h"
#import "components/strings/grit/components_strings.h"
#import "components/sync/base/command_line_switches.h"
#import "ios/chrome/browser/authentication/test/signin_earl_grey.h"
#import "ios/chrome/browser/authentication/test/signin_earl_grey_ui_test_util.h"
#import "ios/chrome/browser/metrics/model/metrics_app_interface.h"
#import "ios/chrome/browser/settings/ui_bundled/clear_browsing_data/public/features.h"
#import "ios/chrome/browser/settings/ui_bundled/clear_browsing_data/public/quick_delete_constants.h"
#import "ios/chrome/browser/signin/model/fake_system_identity.h"
#import "ios/chrome/grit/ios_strings.h"
#import "ios/chrome/test/earl_grey/chrome_actions.h"
#import "ios/chrome/test/earl_grey/chrome_earl_grey.h"
#import "ios/chrome/test/earl_grey/chrome_earl_grey_ui.h"
#import "ios/chrome/test/earl_grey/chrome_matchers.h"
#import "ios/chrome/test/earl_grey/chrome_test_case.h"
#import "ios/chrome/test/earl_grey/chrome_xcui_actions.h"
#import "ios/testing/earl_grey/earl_grey_test.h"
#import "ui/base/l10n/l10n_util_mac.h"There seems to be unnecessary imports here
using chrome_test_util::BrowsingDataButtonMatcher;
using chrome_test_util::ButtonWithAccessibilityLabel;
using chrome_test_util::ClearBrowsingDataView;we usually put using declarations outside of the namespace
id<GREYMatcher> quickDeleteBrowsingDataPageTitleMatcher() {C++ functions start with a capital letter
```suggestion
id<GREYMatcher> QuickDeleteBrowsingDataPageTitleMatcher() {
```
id<GREYMatcher> quickDeleteOtherDataPageTitleMatcher() {C++ functions start with a capital letter
```suggestion
id<GREYMatcher> QuickDeleteBrowsingDataPageTitleMatcher() {
```
config.relaunch_policy = NoForceRelaunchAndResetState;This is the default value, we can remove
config.additional_args.push_back(std::string("--") +
syncer::kSyncShortNudgeDelayForTest);Why do we need this?
// Opens Quick Delete Other Data page.```suggestion
// Opens the Quick Delete Other Data page.
```
Please also apply to other occurrences
- (void)openQuickDeleteOtherDataPage {Doesn't have to be an instance method, let's move it to the anonymous namespace
// Tests the cancel button dismisses the other data page.```suggestion
// Tests that the cancel button dismisses the Quick Delete Other Data page.
```
- (void)testPageNavigationCancelButton {Doesn't have to be an instance method, let's move it to the anonymous namespace
// Taps cancel button.Given that comments should be complete sentences, we shouldn't skip words like `the`
```suggestion
// Tap the cancel button.
```
// Ensures the other data page is closed while the browsing data page is still`Quick Delete Other Data page`
// Ensures the other data page is closed while the browsing data page is still`Quick Delete Browsing Data page`
// Tests the dismissal of the other data page when user swipes down.```suggestion
// Tests the dismissal of the Quick Delete Other Data page when swiped down.
```
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 23 comments![]( "Open in Gerrit")
Votes added by Angela Novakovic
| Commit-Queue | +1 |
23 comments
New View: https://screenshot.googleplex.com/3kUTPVT5Dfkeha6.pngShouldn't the left navigation bar button be a back button? Also, shouldn't the string be "Passwords and passkeys"?
The figma also shows `Search history` (with a lower case `h`), we should validate we UX
No, you are absolutely right. I will update these. The current string is only a placeholder.
extern NSString* const kQuickDeleteOtherDataPasswordsIdentifier;```suggestion
extern NSString* const kQuickDeleteOtherDataPasswordsAndPasskeysIdentifier;
```
Done
@"QuickDeleteOtherDataPasswordsIdentifier";Angela Novakovic```suggestion
@"QuickDeleteOtherDataPasswordsAndPasskeysIdentifier";
```
Done
id<GREYMatcher> quickDeleteOtherDataPageTitleMatcher() {C++ functions start with a capital letter
```suggestion
id<GREYMatcher> QuickDeleteOtherDataPageTitleMatcher() {
```
Done
`Open` was fine, we use the imperative mood for implementation comments. Same for other occurrences
go/c-readability-advice#implementation-comment-conventions
Acknowledged
// Tests that when user taps on the "Manage other data" cell, a new view, the
// Quick Delete Other Data page, opens.```suggestion
// Tests that tapping the "Manage other data" cell opens the Quick Delete Other Data page.
```
Done
```suggestion
// Open the quick delete browsing data page.
```
Done
[[EarlGrey selectElementWithMatcher:ManageOtherDataCellMatcher()]
assertWithMatcher:grey_sufficientlyVisible()];I don't think we need to check the visibility prior to tapping as tapping an element that's not visible will fail anyway
Done
#import <XCTest/XCTest.h>
#import "base/strings/sys_string_conversions.h"
#import "components/browsing_data/core/browsing_data_utils.h"
#import "components/browsing_data/core/pref_names.h"
#import "components/signin/public/base/signin_metrics.h"
#import "components/strings/grit/components_strings.h"
#import "components/sync/base/command_line_switches.h"
#import "ios/chrome/browser/authentication/test/signin_earl_grey.h"
#import "ios/chrome/browser/authentication/test/signin_earl_grey_ui_test_util.h"
#import "ios/chrome/browser/metrics/model/metrics_app_interface.h"
#import "ios/chrome/browser/settings/ui_bundled/clear_browsing_data/public/features.h"
#import "ios/chrome/browser/settings/ui_bundled/clear_browsing_data/public/quick_delete_constants.h"
#import "ios/chrome/browser/signin/model/fake_system_identity.h"
#import "ios/chrome/grit/ios_strings.h"
#import "ios/chrome/test/earl_grey/chrome_actions.h"
#import "ios/chrome/test/earl_grey/chrome_earl_grey.h"
#import "ios/chrome/test/earl_grey/chrome_earl_grey_ui.h"
#import "ios/chrome/test/earl_grey/chrome_matchers.h"
#import "ios/chrome/test/earl_grey/chrome_test_case.h"
#import "ios/chrome/test/earl_grey/chrome_xcui_actions.h"
#import "ios/testing/earl_grey/earl_grey_test.h"
#import "ui/base/l10n/l10n_util_mac.h"There seems to be unnecessary imports here
Done
using chrome_test_util::BrowsingDataButtonMatcher;
using chrome_test_util::ButtonWithAccessibilityLabel;
using chrome_test_util::ClearBrowsingDataView;we usually put using declarations outside of the namespace
Done
id<GREYMatcher> quickDeleteBrowsingDataPageTitleMatcher() {C++ functions start with a capital letter
```suggestion
id<GREYMatcher> QuickDeleteBrowsingDataPageTitleMatcher() {
```
Done
id<GREYMatcher> quickDeleteOtherDataPageTitleMatcher() {C++ functions start with a capital letter
```suggestion
id<GREYMatcher> QuickDeleteBrowsingDataPageTitleMatcher() {
```
Acknowledged
config.relaunch_policy = NoForceRelaunchAndResetState;This is the default value, we can remove
Done
config.additional_args.push_back(std::string("--") +
syncer::kSyncShortNudgeDelayForTest);Why do we need this?
Done
```suggestion
// Opens the Quick Delete Other Data page.
```
Please also apply to other occurrences
Done
Doesn't have to be an instance method, let's move it to the anonymous namespace
Done
- (AppLaunchConfiguration)appConfigurationForTestCase {
AppLaunchConfiguration config;
config.relaunch_policy = NoForceRelaunchAndResetState;
config.additional_args.push_back(std::string("--") +
syncer::kSyncShortNudgeDelayForTest);
config.features_enabled.push_back(kPasswordRemovalFromDeleteBrowsingData);
return config;
}Angela NovakovicDo you plan to add more EG test with the feature disabled for some tests? It is generally a good idea to test the UI with a feature flag on and off to make sure the feature flag can be enabled and disabled, and the app still functions as expected. It may not be in this file.
Leo ZhaoI was not planning on adding tests for when the feature flag is disabled as this view is new and cannot be accessed when the feature flag is disabled (it is accessed via the "Manage other data" cell which is only available when the feature flag is enabled).
Happy to explore this if you think that there should be a test for the "Other Data" page flow for when the feature flag is disabled. 😊
Angela NovakovicWhat I ment was, currently, `kPasswordRemovalFromDeleteBrowsingData` is disabled by default. Here, the EG tests run with it enabled. What happens if this feature is disabled, which is the condition that the app is running? You can add a test with the feature disabled, then open the tools menu, and verify other buttons are there, however, the button to open the quick delete browsing data page isn't there.
I understand now! We could do that, but this is already done in `QuickDeleteBrowsingDataTestCase` in `- (void)testManageOtherDataCellVisibility`. We verify whether the `Manage other data` cell is visible when the feature is enabled vs disabled. If the feature is disabled, the user won't see this cell/button and won't be able to reach `QuickDeleteOtherDataViewController`. Re-testing this in `QuickDeleteOtherDataTestCase` seems repetitive, but please correct me if I am misunderstanding! 😊
Done
// Tests the cancel button dismisses the other data page.```suggestion
// Tests that the cancel button dismisses the Quick Delete Other Data page.
```
Done
- (void)testPageNavigationCancelButton {Doesn't have to be an instance method, let's move it to the anonymous namespace
This is not a method, but a test, to see that the cancel button correctly dismisses the page. Now, it has been changed to verify that the "Back" button dismisses the page.
Based on that, I think it should be kept here, but please correct me if I am wrong! 😊
Given that comments should be complete sentences, we shouldn't skip words like `the`
```suggestion
// Tap the cancel button.
```
Done
// Ensures the other data page is closed while the browsing data page is still`Quick Delete Other Data page`
Done
// Ensures the other data page is closed while the browsing data page is still`Quick Delete Browsing Data page`
Done
// Tests the dismissal of the other data page when user swipes down.```suggestion
// Tests the dismissal of the Quick Delete Other Data page when swiped down.
```
- Leo Zhao
- 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 16 comments![]( "Open in Gerrit")
@"QuickDeleteOtherDataPasswordsIdentifier";```suggestion
@"QuickDeleteOtherDataPasswordsAndPasskeysIdentifier";
```
id<GREYMatcher> ElementIsSelectedMatcher(bool selected) {The cleanup of existing code like this could have been done in a separate CL, especially given the many lines that are now changed in this file which are unrelated to this specific CL. I'm okay with leaving things as is, but something to take into consideration for future CLs.
Note: added code should follow the style guide even if the rest of the file might not always
// Opens the Quick Delete from the three dot menu for the specified window.`Opens Quick Delete` was fine here since it is not followed by `page` or something similar
// Tests that tapping the "Manage other data" cell, opens the Quick Delete Otherno need for a comma here
void openQuickDeleteOtherDataPage() {`OpenQuickDeleteOtherDataPage`
- (void)setUp {
[super setUp];
}
- (void)tearDownHelper {
[super tearDownHelper];
}
These don't add anything as they're only calling the base class' implementation. Please remove them
- (void)testPageNavigationCancelButton {Angela NovakovicDoesn't have to be an instance method, let's move it to the anonymous namespace
This is not a method, but a test, to see that the cancel button correctly dismisses the page. Now, it has been changed to verify that the "Back" button dismisses the page.
Based on that, I think it should be kept here, but please correct me if I am wrong! 😊
Oh, sorry, I must have read that too quickly. It should indeed stay in the class
- (void)testPageDismissal {```suggestion
- (void)testPageDismissalViaDownSwipe {
- ```
enum class SectionIdentifier {
kPasswordsAndPasskeys,
kGoogleAccountData,
kFooter,
};
// Item identifiers in the "Other data" page.
enum class ItemIdentifier {
kPasswordsAndPasskeys,
kSearchHistory,
kMyActivity,
kFooter,
};You can remove the `class` when for section and item identifiers to allow implicit conversion to integer
go/bling-best-practices#ns-enum-vs-c-enums
UIImage* GetBrandedGoogleServicesSymbol() {Isn't the symbol only going to be branded when BUILDFLAG(IOS_USE_BRANDED_ASSETS) is true?
NSArray<NSNumber*>* GoogleDataItemIdentifiers() {```suggestion
NSArray<NSNumber*>* GoogleAccountDataItemIdentifiers() {
```
// Set the arrow icon type of the right side of the cell.`Sets`
- (void)setArrowTypeWithCell:(UITableViewCell*)cell```suggestion
- (void)setAccessoryViewForCell:(UITableViewCell*)cell
- ```
// Creates a cell for the "Other data" page table view.we can omit naming the page when in the class that represents this page. It could make the reader think that we're talking about another page
- (UIImage*)iconForItemIdentifier:(ItemIdentifier)itemIdentifier {Can be put in the anonymous namespace
Related details
- Angela Novakovic
- Leo Zhao
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 15 comments![]( "Open in Gerrit")
@"QuickDeleteOtherDataPasswordsIdentifier";Angela Novakovic```suggestion
@"QuickDeleteOtherDataPasswordsAndPasskeysIdentifier";
```
Done
id<GREYMatcher> ElementIsSelectedMatcher(bool selected) {The cleanup of existing code like this could have been done in a separate CL, especially given the many lines that are now changed in this file which are unrelated to this specific CL. I'm okay with leaving things as is, but something to take into consideration for future CLs.
Note: added code should follow the style guide even if the rest of the file might not always
Acknowledged
// Opens the Quick Delete from the three dot menu for the specified window.`Opens Quick Delete` was fine here since it is not followed by `page` or something similar
Done
// Tests that tapping the "Manage other data" cell, opens the Quick Delete Otherno need for a comma here
Done
void openQuickDeleteOtherDataPage() {Angela Novakovic`OpenQuickDeleteOtherDataPage`
Done
- (void)setUp {
[super setUp];
}
- (void)tearDownHelper {
[super tearDownHelper];
}
These don't add anything as they're only calling the base class' implementation. Please remove them
Done
- (void)testPageDismissal {Angela Novakovic```suggestion
- (void)testPageDismissalViaDownSwipe {
- ```
Done
enum class SectionIdentifier {
kPasswordsAndPasskeys,
kGoogleAccountData,
kFooter,
};
// Item identifiers in the "Other data" page.
enum class ItemIdentifier {
kPasswordsAndPasskeys,
kSearchHistory,
kMyActivity,
kFooter,
};You can remove the `class` when for section and item identifiers to allow implicit conversion to integer
go/bling-best-practices#ns-enum-vs-c-enums
Done
Isn't the symbol only going to be branded when BUILDFLAG(IOS_USE_BRANDED_ASSETS) is true?
Discussed offline.
```suggestion
NSArray<NSNumber*>* GoogleAccountDataItemIdentifiers() {
```
Done
// Set the arrow icon type of the right side of the cell.Angela Novakovic`Sets`
Done
- (void)setArrowTypeWithCell:(UITableViewCell*)cellAngela Novakovic```suggestion
- (void)setAccessoryViewForCell:(UITableViewCell*)cell
- ```
Done
// Creates a cell for the "Other data" page table view.we can omit naming the page when in the class that represents this page. It could make the reader think that we're talking about another page
Done
- (UIImage*)iconForItemIdentifier:(ItemIdentifier)itemIdentifier {Can be put in the anonymous namespace
- 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")
[_baseNavigationController popViewControllerAnimated:YES];I don't think we want to do this here because `popViewControllerAnimated` will just pop whatever pushed view controller that is now on top of the navigation stack.
So, if there was an intermediate view controller between the Browsing Data and Other Data view controllers, and we tapped the back button on the Other Data view controller, the system would dismiss the Other Data view controller and then this line would pop the intermediate view controller
If you want to try it out, update the end of the `start` method with:
```
- (void)start {
...
_viewController = [[QuickDeleteOtherDataViewController alloc]
initWithStyle:ChromeTableViewStyle()];
_viewController.quickDeleteOtherDataHandler =
self.quickDeleteOtherDataHandler;
QuickDeleteOtherDataViewController* vc = [[QuickDeleteOtherDataViewController alloc]
initWithStyle:ChromeTableViewStyle()];
vc.quickDeleteOtherDataHandler = self.quickDeleteOtherDataHandler;
[_baseNavigationController pushViewController:_viewController animated:NO];
[_baseNavigationController pushViewController:vc animated:NO];
}
```
// Returns a matcher for the passwords and passkeys cell.`"Passwords and passkeys" cell` to be consistent. Please also apply to other occurrences
// Returns a matcher for the search history cell.`"Search history" cell` to be consistent. Please also apply to other occurrences
// Returns a matcher for the "my activity" cell.`"My Activity" cell` to be consistent. Please also apply to other occurrences
[items addObject:@((ItemIdentifier::kSearchHistoryIdentifier))];I don't think we need the double parentheses around the section and item identifiers `@(ItemIdentifier::kSearchHistoryIdentifier)` should be fine
return items;Why not just return `@[@(ItemIdentifier::kSearchHistoryIdentifier), @(ItemIdentifier::kMyActivityIdentifier)]`?
UIImage* IconForItemIdentifier(ItemIdentifier itemIdentifier) {C++ function, so `item_identifier`
[snapshot appendItemsWithIdentifiers:GoogleAccountDataItemIdentifiers()nit: we can directly pass `@[@(ItemIdentifier::kSearchHistoryIdentifier), @(ItemIdentifier::kMyActivityIdentifier)]`, like you did above for `appendSectionsWithIdentifiers`. `GoogleAccountDataItemIdentifiers` was only used here anyway, we probably don't need a helper function for it
- (void)didMoveToParentViewController:(UIViewController*)parent {
[super didMoveToParentViewController:parent];
if (!parent) {
[self.quickDeleteOtherDataHandler hideQuickDeleteOtherDataPage];
}
}This function doesn't belong in the `UITableViewDelegate` pragma mark
// Sets the arrow icon type of the right side of the cell.`accessory type`, and we can remove the `right side` mention as it is implied https://developer.apple.com/documentation/uikit/uitableviewcell/accessorytype-swift.property?language=objc
initWithImage:DefaultAccessorySymbolConfigurationWithRegularWeight(
kExternalLinkSymbol)];This symbol is grey in the mocks, but blue in the screenshot provided in the CL description
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")
[_baseNavigationController popViewControllerAnimated:YES];I don't think we want to do this here because `popViewControllerAnimated` will just pop whatever pushed view controller that is now on top of the navigation stack.
So, if there was an intermediate view controller between the Browsing Data and Other Data view controllers, and we tapped the back button on the Other Data view controller, the system would dismiss the Other Data view controller and then this line would pop the intermediate view controller
If you want to try it out, update the end of the `start` method with:
```
- (void)start {
..._viewController = [[QuickDeleteOtherDataViewController alloc]
initWithStyle:ChromeTableViewStyle()];
_viewController.quickDeleteOtherDataHandler =
self.quickDeleteOtherDataHandler;
QuickDeleteOtherDataViewController* vc = [[QuickDeleteOtherDataViewController alloc]
initWithStyle:ChromeTableViewStyle()];
vc.quickDeleteOtherDataHandler = self.quickDeleteOtherDataHandler;
[_baseNavigationController pushViewController:_viewController animated:NO];
[_baseNavigationController pushViewController:vc animated:NO];
}
```
Nice! I tried it and I see clearly what you meant! 😊 Thank you!
// Returns a matcher for the passwords and passkeys cell.`"Passwords and passkeys" cell` to be consistent. Please also apply to other occurrences
Done
`"Search history" cell` to be consistent. Please also apply to other occurrences
Done
`"My Activity" cell` to be consistent. Please also apply to other occurrences
Done
[items addObject:@((ItemIdentifier::kSearchHistoryIdentifier))];I don't think we need the double parentheses around the section and item identifiers `@(ItemIdentifier::kSearchHistoryIdentifier)` should be fine
Done
Why not just return `@[@(ItemIdentifier::kSearchHistoryIdentifier), @(ItemIdentifier::kMyActivityIdentifier)]`?
Done
UIImage* IconForItemIdentifier(ItemIdentifier itemIdentifier) {Angela NovakovicC++ function, so `item_identifier`
Done
[snapshot appendItemsWithIdentifiers:GoogleAccountDataItemIdentifiers()nit: we can directly pass `@[@(ItemIdentifier::kSearchHistoryIdentifier), @(ItemIdentifier::kMyActivityIdentifier)]`, like you did above for `appendSectionsWithIdentifiers`. `GoogleAccountDataItemIdentifiers` was only used here anyway, we probably don't need a helper function for it
Done
- (void)didMoveToParentViewController:(UIViewController*)parent {
[super didMoveToParentViewController:parent];
if (!parent) {
[self.quickDeleteOtherDataHandler hideQuickDeleteOtherDataPage];
}
}This function doesn't belong in the `UITableViewDelegate` pragma mark
Done
// Sets the arrow icon type of the right side of the cell.`accessory type`, and we can remove the `right side` mention as it is implied https://developer.apple.com/documentation/uikit/uitableviewcell/accessorytype-swift.property?language=objc
Done
initWithImage:DefaultAccessorySymbolConfigurationWithRegularWeight(
kExternalLinkSymbol)];This symbol is grey in the mocks, but blue in the screenshot provided in the CL description
- Alexis Hétu
- 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. |
Alexis Hétu (Gerrit)
Alexis Hétu added 2 comments![]( "Open in Gerrit")
We don't usually merge white space only changes
switch (itemIdentifier) {
case ItemIdentifier::kPasswordsAndPasskeysIdentifier: {
cell = [self
createCellWithTitle:l10n_util::GetNSString(
IDS_SETTINGS_PASSWORDS_AND_PASSKEYS)
subtitle:
l10n_util::GetNSString(
IDS_SETTINGS_MANAGE_IN_GOOGLE_PASSWORD_MANAGER)
icon:IconForItemIdentifier(itemIdentifier)
accessibilityIdentifier:
kQuickDeleteOtherDataPasswordsAndPasskeysIdentifier];
[self setAccessoryTypeForCell:cell
itemIdentifier:ItemIdentifier::
kPasswordsAndPasskeysIdentifier];
return cell;
}
case ItemIdentifier::kSearchHistoryIdentifier: {
cell = [self
createCellWithTitle:l10n_util::GetNSString(
IDS_SETTINGS_SEARCH_HISTORY)
// TODO(crbug.com/482036587) Replace the below
// placeholder string with the subtitle variable coming
// from the QuickDeleteOtherDataConsumer.
subtitle:@"Change this"
icon:IconForItemIdentifier(itemIdentifier)
accessibilityIdentifier:kQuickDeleteOtherDataSearchHistoryIdentifier];
[self setAccessoryTypeForCell:cell
itemIdentifier:ItemIdentifier::kSearchHistoryIdentifier];
return cell;
}
case ItemIdentifier::kMyActivityIdentifier: {
cell = [self
createCellWithTitle:l10n_util::GetNSString(
IDS_SETTINGS_MY_ACTIVITY)
subtitle:
l10n_util::GetNSString(
IDS_SETTINGS_MANAGE_IN_YOUR_GOOGLE_ACCOUNT)
icon:IconForItemIdentifier(itemIdentifier)
accessibilityIdentifier:kQuickDeleteOtherDataMyActivityIdentifier];
[self setAccessoryTypeForCell:cell
itemIdentifier:ItemIdentifier::kMyActivityIdentifier];
return cell;
}
}These look very similar. You already added the `IconForItemIdentifier` helper to get the icon from the `itemIdentifier`. If you had a utility for each argument (title, subtitle and accessibilityIdentifier), you wouldn't need the large switch statement with mostly duplicated code in each case here.
Related details
- Angela Novakovic
- 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. |
Alexis Hétu (Gerrit)
Alexis Hétu added 2 comments![]( "Open in Gerrit")
enum SectionIdentifier {
kPasswordsAndPasskeysSection,nit:
```
typedef NS_ENUM(NSInteger, SectionIdentifier) {
kPasswordsAndPasskeysSection = 0,
```
enum ItemIdentifier {
kPasswordsAndPasskeysIdentifier,nit:
```
typedef NS_ENUM(NSInteger, ItemIdentifier) {
kPasswordsAndPasskeysIdentifier = 0,
```
Angela Novakovic (Gerrit)
Angela Novakovic added 4 comments![]( "Open in Gerrit")
We don't usually merge white space only changes
Acknowledged
nit:
```
typedef NS_ENUM(NSInteger, SectionIdentifier) {
kPasswordsAndPasskeysSection = 0,
```
I will keep the current format to follow go/bling-best-practices#ns-enum-vs-c-enums .
nit:
```
typedef NS_ENUM(NSInteger, ItemIdentifier) {
kPasswordsAndPasskeysIdentifier = 0,
```
Same as the section identifiers. I will follow go/bling-best-practices#ns-enum-vs-c-enums for this new file.
switch (itemIdentifier) {These look very similar. You already added the `IconForItemIdentifier` helper to get the icon from the `itemIdentifier`. If you had a utility for each argument (title, subtitle and accessibilityIdentifier), you wouldn't need the large switch statement with mostly duplicated code in each case here.
- Alexis Hétu
- Filipa Senra
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Filipa Senra (Gerrit)
Filipa Senra voted and added 5 comments![]( "Open in Gerrit")
Votes added by Filipa Senra
| Code-Review | +1 |
5 comments
Took a look, but not super in depth as I think the other reviewers are doing that.
// Tests the cancel button dismisses the Quick Delete Browsing Data page.nit: that
// Ensure the Quick Delete Browsing Data page is closed while quick deletenit: the
[_mediator disconnect];nit: is this new line needed?
}nit: should you have NOTREACHED after the switch statments?
Related details
- Alexis Hétu
- 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. |
Alexis Hétu (Gerrit)
Alexis Hétu voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alexis Hétu
| Code-Review | +1 |
1 comment
- 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 5 comments![]( "Open in Gerrit")
Took a look, but not super in depth as I think the other reviewers are doing that.
Thank you Filipa! Indeed they are! 😊
// Tests the cancel button dismisses the Quick Delete Browsing Data page.Angela Novakovicnit: that
Done
// Ensure the Quick Delete Browsing Data page is closed while quick deleteAngela Novakovicnit: the
Done
nit: is this new line needed?
Done
nit: should you have NOTREACHED after the switch statments?
- Alexis Hétu
- Filipa Senra
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexis Hétu (Gerrit)
Alexis Hétu voted Code-Review+1![]( "Open in Gerrit")
- Angela Novakovic
- Filipa Senra
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |