Test Project, not intended to be merged. [chromium/src : main]
Leo Zhao (Gerrit)
Leo Zhao added 1 comment![]( "Open in Gerrit")
This CL is for the step "Now's a good time to upload a CL with what you have so far so you can get some early feedback." from Bling Onboarding Template.
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sourav Uttam Sinha (Gerrit)
Sourav Uttam Sinha added 7 comments![]( "Open in Gerrit")
Great work! I'd also recommend adding some sort of test (unit/EG).
<message name="IDS_TEST_PROJECT_NOT_SIGNEDIN" desc="A message to indicate the user is not signed in" meaning="Used to info the user that he or she is not signed in.">Did you mean inform*?
<message name="IDS_TEST_PROJECT_SIGNEDIN" desc="A message to indicate the user is signed in" meaning="Used to info the user that he or she is signed in.">Same as above
@property(nonatomic, strong) TestProjectCoordinator* testProjectCoordinator;Add a comment
BASE_FEATURE(kIOSAlternativeNewTabAction, base::FEATURE_ENABLED_BY_DEFAULT);This should be DISABLED_BY_DEFAULT as mentioned in the comment above. To enable it for testing purposes, you can go to chrome://flags and enable it manually.
include_rules = [Is this file necessary? If you don't plan to include any rules, you can remove it.
if (signedIn) {
self.statusLabel.text = l10n_util::GetNSString(IDS_TEST_PROJECT_SIGNEDIN);
} else {
self.statusLabel.text =
l10n_util::GetNSString(IDS_TEST_PROJECT_NOT_SIGNEDIN);
}Nit:
self.statusLabel.text =
signedIn ? l10n_util::GetNSString(IDS_TEST_PROJECT_SIGNEDIN)
: l10n_util::GetNSString(IDS_TEST_PROJECT_NOT_SIGNEDIN);
Related details
- Leo Zhao
- Sebastien S-G
- Tommy Martino
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leo Zhao (Gerrit)
Leo Zhao added 6 comments![]( "Open in Gerrit")
<message name="IDS_TEST_PROJECT_NOT_SIGNEDIN" desc="A message to indicate the user is not signed in" meaning="Used to info the user that he or she is not signed in.">Leo ZhaoDid you mean inform*?
Done
<message name="IDS_TEST_PROJECT_SIGNEDIN" desc="A message to indicate the user is signed in" meaning="Used to info the user that he or she is signed in.">Leo ZhaoSame as above
Done
@property(nonatomic, strong) TestProjectCoordinator* testProjectCoordinator;Leo ZhaoAdd a comment
Done
BASE_FEATURE(kIOSAlternativeNewTabAction, base::FEATURE_ENABLED_BY_DEFAULT);This should be DISABLED_BY_DEFAULT as mentioned in the comment above. To enable it for testing purposes, you can go to chrome://flags and enable it manually.
Done
Is this file necessary? If you don't plan to include any rules, you can remove it.
Done
if (signedIn) {
self.statusLabel.text = l10n_util::GetNSString(IDS_TEST_PROJECT_SIGNEDIN);
} else {
self.statusLabel.text =
l10n_util::GetNSString(IDS_TEST_PROJECT_NOT_SIGNEDIN);
}Nit:
self.statusLabel.text =
signedIn ? l10n_util::GetNSString(IDS_TEST_PROJECT_SIGNEDIN)
: l10n_util::GetNSString(IDS_TEST_PROJECT_NOT_SIGNEDIN);
- Sourav Uttam Sinha
- Tommy Martino
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leo Zhao (Gerrit)
Leo Zhao removed Sebastien S-G from this change![]( "Open in Gerrit")
- Sebastien S-G
Related details
- Sourav Uttam Sinha
- Tommy Martino
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leo Zhao (Gerrit)
Leo Zhao added 1 comment![]( "Open in Gerrit")
Related details
- Sourav Uttam Sinha
- Tommy Martino
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tommy Martino (Gerrit)
Tommy Martino added 16 comments![]( "Open in Gerrit")
<message name="IDS_TEST_PROJECT_NOT_SIGNEDIN" desc="A message to indicate the user is not signed in" meaning="Used to inform the user that he or she is not signed in.">`meaning` is optional and not necessary/useful here. I would remove it.
See here for more context on when you *would* want to use it: https://www.chromium.org/developers/design-documents/ui-localization/#use-message-meanings-to-disambiguate-strings
// The coordinator to show the user's auth status.
@property(nonatomic, strong) TestProjectCoordinator* testProjectCoordinator;We don't use private (in `.mm`) properties anymore. There are a lot in this file, because this file is huge and really old, but for new ones we should just use ivars. (I think maybe the tutorial you're following also needs to be updated to say "add an ivar" rather than "add a property")
You can add this below `_credentialImportCoordinator`.
Ref: go/bling-best-practices#properties-and-instance-variables
_testProjectCoordinator = [[TestProjectCoordinator alloc]Is there a reason we need to create this as soon as `BrowserCoordinator` is created, or could we defer that work and only create it when `showPopupView` is called?
// Show a popup view to indicate whether a user has signed in.nit: `Shows` (for consistency with the other commands in this file)
UINavigationController* _rootController;ChromeCoordinator has a `baseNavigationController` property, so this is redundant.
TestProjectMediator* _mediator;As a rule, Objective-C ivars (and classes, functions, protocols, etc) should always have documentation comments.
The Obj-C style guide is stricter than the C++ style guide in this sense; in C++ it is OK to omit comments when they add no value, while in Obj-C we generally still require *something* to be written. (I mention this distinction because you're going to see lots of places in the code where C++ classes have ivars or even methods without comments.)
https://google.github.io/styleguide/objcguide.html#declaration-comments
@synthesize baseNavigationController = _baseNavigationController;As with `_rootController`, I don't think you actually need to declare or explicitly set this (the parent class initializer should take care of it)
TestProjectViewController* vc = [[TestProjectViewController alloc] init];As a rule, we prefer longer, more descriptive variable names and avoid abbreviations (except in common cases like "HTML"). I would call this `viewController`.
AuthenticationServiceFactory::GetForProfile(
browser->GetProfile()->GetOriginalProfile());We have to be careful about using `GetOriginalProfile`, because it means that if we're in incognito it will behave like a non-incognito profile. I like to always leave a comment explaining why we believe it's safe.
completion:^{
[weakSelf updateUserSigninStatus];
}];Is it important that updateUserSigninStatus be called only after the VC is presented? Could we just trigger this synchronously?
[self.baseViewController dismissViewControllerAnimated:YES completion:nil];This dismisses the UI, but the coordinator continues to exist. It would be better to do this work in `stop` and manage the lifetime in tandem with the parent coordinator.
When the VC indicates (via the button tap) that it should be dismissed, we would signal to the parent coordinator (probably via a new delegate protocol). The parent coordinator would then both call `stop` on this coordinator and clean up the references to it.
// The authentication service to observe for sign-in status changes.
@property(nonatomic, assign) AuthenticationService* authService;Same as in the coordinator, this should be an instance variable instead of a property.
DCHECK(authService);We mostly prefer `CHECK` rather than `DCHECK`, except in specific circumstances. (This changed a year or two ago; we used to use a lot more `DCHECK`s and you'll still find them in the codebase.)
The difference between `CHECK` and `DCHECK` is that `CHECK` will induce a crash on all builds, while `DCHECK` will only induce a crash on debug builds (and I think Canary as well).
We use `DCHECK` when the condition would be too expensive to compute in the real production app. We'll also use it when we have a specific reason we don't want the app to crash in production, but still would like to get a dump when it happens locally (e.g., sometimes we'll do this for a bug that's hard to reproduce but not super severe).
In your case, if `authService` is null here, we *will* crash on line 36 below, and so making this a DCHECK doesn't even save a crash in production. It's actually better to crash here in a controlled way than to let the app keep running and eventually hit a use-after-free. Thus, this should be a `CHECK`.
id<GREYMatcher> newTabButtonMatcher =
grey_accessibilityID(kToolbarNewTabButtonIdentifier);
GREYCondition* isButtonInteractable = [GREYCondition
conditionWithName:@"Wait for New Tab Button to be Interactable"
block:^BOOL {
NSError* error = nil;
[[EarlGrey selectElementWithMatcher:newTabButtonMatcher]
assertWithMatcher:grey_interactable()
error:&error];
return error == nil;
}];
NSTimeInterval customTimeout = 5.0;
BOOL buttonBecameInteractable =
[isButtonInteractable waitWithTimeout:customTimeout pollInterval:0.1];
GREYAssertTrue(
buttonBecameInteractable,
@"New Tab Button did not become interactable within %f seconds",
customTimeout);You should hopefully never need to do this level of synchronization/testing/waiting/etc by hand; we have lots of useful utilities in the [ChromeEarlGrey](https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/test/earl_grey/chrome_earl_grey.h) file.
In this case I think `waitForMatcher` with a matcher for `grey_allOf(newTabButtonMatcher, grey_interactable())` should do the trick.
timeout:base::Seconds(5)];Prefer to use one of our predefined timeout intervals, e.g. `kWaitForUIElementTimeout`
self.view.backgroundColor = [UIColor whiteColor];In general, prefer to use semantic color names from [this file](https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/common/ui/colors/semantic_color_names.h).
`kBackgroundColor` would be a good choice here.
Related details
- Leo Zhao
- Sourav Uttam Sinha
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |