Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2749703003:
Adding mediator for Sign-in promo (Closed)
|
Created:
3 years, 9 months ago by jlebel Modified:
3 years, 8 months ago CC:
chromium-reviews, ios-reviews+chrome_chromium.org, ios-reviews_chromium.org, pkl (ping after 24h if needed), noyau+watch_chromium.org, marq+watch_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding mediator for Sign-in promo
Creating SigninPromoView and SigninPromoViewMediator.
Moving SiginPromoItem from settings/cells to authentication.
SigninPromoView can display cold start sign-in promo (with no known
profile), or warm start sign-in promo (to be able to sign-in without
typing password). This class is now used by SiginPromoCell and will be
used in the bookmarks view and any other place.
SigninPromoViewMediator can configure a SigninPromoView instance
according to the value it contains. An instance of this class is
required by the initializer of SigninPromoItem.
-[SigninPromoItem configureCell:] uses the SigninPromoViewMediator
instance to configuring the SigninPromoCell cell.
Now, SigninPromoCell contains only a SigninPromoView instance.
TODO: the Chrome/Chromium icon is missing for the cold start view.
Screenshot: https://drive.google.com/file/d/0ByXziH_JVCGJcGo5djJvR1BCUjA/view
Related to: crrev.com/2747893002
BUG=661794
Review-Url: https://codereview.chromium.org/2749703003
Cr-Commit-Position: refs/heads/master@{#460337}
Committed: https://chromium.googlesource.com/chromium/src/+/bb883994c62cbdbb2d449d886190d97e24548fcc
Patch Set 1 #
Total comments: 33
Patch Set 2 : Visual constraints #
Total comments: 106
Patch Set 3 : Fixing Louis' comments #
Total comments: 45
Patch Set 4 : . #
Total comments: 17
Patch Set 5 : . #
Total comments: 6
Patch Set 6 : . #Patch Set 7 : Typo #Patch Set 8 : Removing useless test #Patch Set 9 : Update tests #Dependent Patchsets: Messages
Total messages: 58 (32 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Adding mediator BUG= ========== to ========== Adding mediator for Sign-in promo Creating SigninPromoView and SigninPromoViewMediator. Moving SiginPromoItem from settings/cells to authentication. SigninPromoView can display cold start sign-in promo (with no known profile), or warm start sign-in promo (to be able to sign-in without typing password). This class is now used by SiginPromoCell and will be used in the bookmarks view and any other place. SigninPromoViewMediator can configure a SigninPromoView instance according to the value it contains. An instance of this class is required by the initializer of SigninPromoItem. -[SigninPromoItem configureCell:] uses the SigninPromoViewMediator instance to configuring the SigninPromoCell cell. Now, SigninPromoCell contains only a SigninPromoView instance. TODO: the Chrome/Chromium icon is missing for the cold start view. Related to: crrev.com/2747893002 BUG=661794 ==========
Description was changed from ========== Adding mediator for Sign-in promo Creating SigninPromoView and SigninPromoViewMediator. Moving SiginPromoItem from settings/cells to authentication. SigninPromoView can display cold start sign-in promo (with no known profile), or warm start sign-in promo (to be able to sign-in without typing password). This class is now used by SiginPromoCell and will be used in the bookmarks view and any other place. SigninPromoViewMediator can configure a SigninPromoView instance according to the value it contains. An instance of this class is required by the initializer of SigninPromoItem. -[SigninPromoItem configureCell:] uses the SigninPromoViewMediator instance to configuring the SigninPromoCell cell. Now, SigninPromoCell contains only a SigninPromoView instance. TODO: the Chrome/Chromium icon is missing for the cold start view. Related to: crrev.com/2747893002 BUG=661794 ========== to ========== Adding mediator for Sign-in promo Creating SigninPromoView and SigninPromoViewMediator. Moving SiginPromoItem from settings/cells to authentication. SigninPromoView can display cold start sign-in promo (with no known profile), or warm start sign-in promo (to be able to sign-in without typing password). This class is now used by SiginPromoCell and will be used in the bookmarks view and any other place. SigninPromoViewMediator can configure a SigninPromoView instance according to the value it contains. An instance of this class is required by the initializer of SigninPromoItem. -[SigninPromoItem configureCell:] uses the SigninPromoViewMediator instance to configuring the SigninPromoCell cell. Now, SigninPromoCell contains only a SigninPromoView instance. TODO: the Chrome/Chromium icon is missing for the cold start view. Screenshot: https://drive.google.com/file/d/0ByXziH_JVCGJcGo5djJvR1BCUjA/view Related to: crrev.com/2747893002 BUG=661794 ==========
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
jlebel@chromium.org changed reviewers: + lpromero@chromium.org, msarda@chromium.org
hello Mihai, Louis, Can you review this patch related to Sign-in promo? I'm adding a Sign-in promo view to share it in different places and a mediator to be able to setup the view. Thanks,
https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_item.mm (right): https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_item.mm:42: [_signinPromoViewConfigurator configureSigninPromoView:cell.signinPromoView]; Should the configuration also configure the textLabel of the cell's signinPromoView? https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_item.mm:65: [_signinPromoView.topAnchor constraintEqualToAnchor:contentView.topAnchor], Is the goal of these constraints to have the same size for _signinPromoView and contentView? If so, then consider using: Consider using function AddSameSizeConstraint (see https://cs.chromium.org/chromium/src/ios/chrome/browser/ui/uikit_ui_util.mm?r... ). https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_item.mm:75: // Implements -layoutSubviews as per instructions in documentation for I suppose this comment is not needed (but probably Louis knows better). https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_item.mm:78: [super layoutSubviews]; I think calling [super layoutSubviews] here is unnecessary the following reasons: * the bounds of this SigninPromoCell will not change after this call (self.bounds are the same whether [super layoutSubviews] is called or not) * This means that the parentWidth is the same are the same whether [super layoutSubviews] is called or not * This means that _signinPromoView.textLabel.preferredMaxLayoutWidth is the same whether [super layoutSubviews] is called or not https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view.h (right): https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:16: @property(nonatomic, readonly) BOOL coldStart; Avoid using bool and prefer using the enum SigninPromoViewMode below. Also, why is this a public property of the protocol? https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:24: SigninPromoViewColdStartMode, I think we should drop this enum entirely and pass a ChromeIdentity (this one https://cs.chromium.org/chromium/src/ios/public/provider/chrome/browser/signi...) to the SigninPromoViewConfigurator. Basically, if |ChromeIdentity| is no-nil, then it is similar to cold start. Basically, if |ChromeIdentity| is no-nil, then it is similar to warm start. I think this change will actually leads to a big change in the code (sorry for that). To get image for a profile, consider using https://cs.chromium.org/chromium/src/ios/public/provider/chrome/browser/signi... (I think for now using the cached image should be fine). Note that an identity may have a NULL user full name and image. It is very rare that an identity has a null email (this in general means there was an error fetching this info). https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:27: SigninPromoViewWarmStartMode, I would not name this |coldStart| or |warmStart| as this makes me thing about the application being started after it was killed or just foregrounded. https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:49: @property(nonatomic, readonly) MDCFlatButton* secondaryButton; Nit: Blank line to ease code review in the code review tool (unfortunately there is not highlight in code review, compared to regular IDE). https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:51: // |secondaryButton|. Available to be able to compute Nit: s/Available to be able to compute.../Used to compute the preferred max layout of the text label? https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view.mm (right): https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:29: @implementation SigninPromoView { Louis: May I ask you to do an in-depth review of this class? It has been a while since I reviewed iOS cells and views code and things may have changed. https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:109: [_imageView.topAnchor constraintEqualToAnchor:self.topAnchor I have to say I find this constraints code very hard to read. Could we use use visual format syntax to get the same results - see https://developer.apple.com/library/content/documentation/UserExperience/Conc... https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:160: [NSLayoutConstraint deactivateConstraints:_coldStartConstraints]; Why is this needed? https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h (right): https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h:17: @property(nonatomic, copy) NSString* userFullName; Mediator should also take a ChromeIdentity object (not username, email and image). https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view_mediator.mm (right): https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view_mediator.mm:28: return !_userFullName; userFullName of a valid identity is often NULL (the user may have not set it). I assume we should default to the email in this case (please double check with Eli or UX). https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/settings/cells/signin_promo_item.mm (left): https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/cells/signin_promo_item.mm:64: @implementation SigninPromoCell I think I am misunderstanding the initial goal of this class and what your plans are for code placement and the design of this class (sorry for this question, but I'd like to understand the code structure to do a better code review): * this class SigninPromoItem (and the corresponding tests and cell) was added only 3 days ago (in CL https://codereview.chromium.org/2743633002 and was never really used * this class is now moved and completely rewritten (or is the code the same and codereview tool does not work well) * the new screenshots and the old screenshots are similar, but have different cell sizes (https://drive.google.com/file/d/0ByXziH_JVCGJcGo5djJvR1BCUjA/view vs https://drive.google.com/file/d/0ByXziH_JVCGJQzU1WlZ6bEVIT1E/view I guess I'd have the following question: Do you plan to move / redo this code around again, or is this CL the final class structure for the promo UI?
Patchset #2 (id:140001) has been deleted
Thanks for your input. https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_item.mm (right): https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_item.mm:42: [_signinPromoViewConfigurator configureSigninPromoView:cell.signinPromoView]; On 2017/03/16 22:15:31, msarda wrote: > Should the configuration also configure the textLabel of the cell's > signinPromoView? I can but this is a fixed value, and the value is not related to the data related to the profile. This looks fine to me. https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_item.mm:65: [_signinPromoView.topAnchor constraintEqualToAnchor:contentView.topAnchor], On 2017/03/16 22:15:31, msarda wrote: > Is the goal of these constraints to have the same size for _signinPromoView and > contentView? If so, then consider using: > Consider using function AddSameSizeConstraint (see > https://cs.chromium.org/chromium/src/ios/chrome/browser/ui/uikit_ui_util.mm?r... > ). Done. https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_item.mm:75: // Implements -layoutSubviews as per instructions in documentation for On 2017/03/16 22:15:31, msarda wrote: > I suppose this comment is not needed (but probably Louis knows better). I've done exactly the same implementation than those methods: https://cs.chromium.org/search/?q=%22Implements+-layoutSubviews+as+per+instru... So I'm guessing I need the same comment. But I don't mind removing it. https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_item.mm:78: [super layoutSubviews]; On 2017/03/16 22:15:31, msarda wrote: > I think calling [super layoutSubviews] here is unnecessary the following > reasons: > * the bounds of this SigninPromoCell will not change after this call > (self.bounds are the same whether [super layoutSubviews] is called or not) > * This means that the parentWidth is the same are the same whether [super > layoutSubviews] is called or not > * This means that _signinPromoView.textLabel.preferredMaxLayoutWidth is the same > whether [super layoutSubviews] is called or not -[MDCollectionViewCell layoutSubviews layoutSubviews] does exist (and -[UICollectionViewCell layoutSubviews] too). So I think it is a good idea to call it. The only gain would be speed optimisation? https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view.h (right): https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:16: @property(nonatomic, readonly) BOOL coldStart; On 2017/03/16 22:15:31, msarda wrote: > Avoid using bool and prefer using the enum SigninPromoViewMode below. Also, why > is this a public property of the protocol? Done. https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:24: SigninPromoViewColdStartMode, On 2017/03/16 22:15:31, msarda wrote: > I think we should drop this enum entirely and pass a ChromeIdentity (this one > https://cs.chromium.org/chromium/src/ios/public/provider/chrome/browser/signi...) > to the SigninPromoViewConfigurator. > Basically, if |ChromeIdentity| is no-nil, then it is similar to cold start. > Basically, if |ChromeIdentity| is no-nil, then it is similar to warm start. > > I think this change will actually leads to a big change in the code (sorry for > that). > > To get image for a profile, consider using > https://cs.chromium.org/chromium/src/ios/public/provider/chrome/browser/signi... > (I think for now using the cached image should be fine). > > Note that an identity may have a NULL user full name and image. It is very rare > that an identity has a null email (this in general means there was an error > fetching this info). The view should not know anything related to the profile. The point of the mediator is to have an abstraction layer. https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:27: SigninPromoViewWarmStartMode, On 2017/03/16 22:15:31, msarda wrote: > I would not name this |coldStart| or |warmStart| as this makes me thing about > the application being started after it was killed or just foregrounded. I'm not sure what could be a better name. Do you prefer: SigninPromoViewNoProfileMode, SigninPromoViewProfileFoundMode, ? https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:49: @property(nonatomic, readonly) MDCFlatButton* secondaryButton; On 2017/03/16 22:15:31, msarda wrote: > Nit: Blank line to ease code review in the code review tool (unfortunately there > is not highlight in code review, compared to regular IDE). Done. https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:51: // |secondaryButton|. Available to be able to compute On 2017/03/16 22:15:31, msarda wrote: > Nit: s/Available to be able to compute.../Used to compute the preferred max > layout of the text label? Done. https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view.mm (right): https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:109: [_imageView.topAnchor constraintEqualToAnchor:self.topAnchor On 2017/03/16 22:15:31, msarda wrote: > I have to say I find this constraints code very hard to read. Could we use use > visual format syntax to get the same results - see > https://developer.apple.com/library/content/documentation/UserExperience/Conc... Done. https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:160: [NSLayoutConstraint deactivateConstraints:_coldStartConstraints]; On 2017/03/16 22:15:31, msarda wrote: > Why is this needed? Done. https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h (right): https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h:17: @property(nonatomic, copy) NSString* userFullName; On 2017/03/16 22:15:31, msarda wrote: > Mediator should also take a ChromeIdentity object (not username, email and > image). No, that's the point of the TODO. Plus that would mess up the point of MaterialCellCatalogViewController. We would have to deal with profiles. https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view_mediator.mm (right): https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view_mediator.mm:28: return !_userFullName; On 2017/03/16 22:15:31, msarda wrote: > userFullName of a valid identity is often NULL (the user may have not set it). I > assume we should default to the email in this case (please double check with Eli > or UX). Yes, you are right. This should be done with email, not with the full name. https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/settings/cells/signin_promo_item.mm (left): https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/cells/signin_promo_item.mm:64: @implementation SigninPromoCell On 2017/03/16 22:15:30, msarda wrote: > I think I am misunderstanding the initial goal of this class and what your plans > are for code placement and the design of this class (sorry for this question, > but I'd like to understand the code structure to do a better code review): > * this class SigninPromoItem (and the corresponding tests and cell) was added > only 3 days ago (in CL https://codereview.chromium.org/2743633002 and was never > really used > * this class is now moved and completely rewritten (or is the code the same and > codereview tool does not work well) > * the new screenshots and the old screenshots are similar, but have different > cell sizes (https://drive.google.com/file/d/0ByXziH_JVCGJcGo5djJvR1BCUjA/view vs > https://drive.google.com/file/d/0ByXziH_JVCGJQzU1WlZ6bEVIT1E/view > > I guess I'd have the following question: Do you plan to move / redo this code > around again, or is this CL the final class structure for the promo UI? > I adjusted my design before finishing the previous patch. I asked Louis if it was ok to land this previous patch as it was. So I did. In this patch I just split this class. The UI went into SigninPromoView. The cell didn't change. I guess I will have to update this view one more time to add the close button, when I will have to use this SigninPromoView for the bookmark view. But as far as I know, there is no big design changes, and I don't plan to add a big design changes (unless the UI mocks is totally changed). I'm impressed that you were able to see that the cell is 5px shorter on the new screenshot than on the old screenshot. But everything is a bit shorter on the new screenshot. Something wrong happened to the new screenshot. You can see also the difference on the header of the view "Cell Catalog". So the issue is definitely on png of the screenshot and not on the implementation I've done. Except the fact it is not the same iphone (SE vs 7), the 2 implementations are exactly the same. There is also another difference, there is only the warm start cell on the old screenshot. The new screenshot implements also the cold start cell. I've checked again after changing the constraints, the UI is still exactly the same.
https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_item.mm (right): https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_item.mm:42: [_signinPromoViewConfigurator configureSigninPromoView:cell.signinPromoView]; On 2017/03/21 17:22:28, jlebel wrote: > On 2017/03/16 22:15:31, msarda wrote: > > Should the configuration also configure the textLabel of the cell's > > signinPromoView? > > I can but this is a fixed value, and the value is not related to the data > related to the profile. This looks fine to me. OK, I see (basically this is not something configurable, so there is nothing to be configured here). https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_item.mm:78: [super layoutSubviews]; On 2017/03/21 17:22:28, jlebel wrote: > On 2017/03/16 22:15:31, msarda wrote: > > I think calling [super layoutSubviews] here is unnecessary the following > > reasons: > > * the bounds of this SigninPromoCell will not change after this call > > (self.bounds are the same whether [super layoutSubviews] is called or not) > > * This means that the parentWidth is the same are the same whether [super > > layoutSubviews] is called or not > > * This means that _signinPromoView.textLabel.preferredMaxLayoutWidth is the > same > > whether [super layoutSubviews] is called or not > > -[MDCollectionViewCell layoutSubviews layoutSubviews] does exist (and > -[UICollectionViewCell layoutSubviews] too). > So I think it is a good idea to call it. The only gain would be speed > optimisation? Ok, I've added a comment on the next patch. I'd like Louis's opinion about this as well. https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view.h (right): https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:24: SigninPromoViewColdStartMode, On 2017/03/21 17:22:29, jlebel wrote: > On 2017/03/16 22:15:31, msarda wrote: > > I think we should drop this enum entirely and pass a ChromeIdentity (this one > > > https://cs.chromium.org/chromium/src/ios/public/provider/chrome/browser/signi...) > > to the SigninPromoViewConfigurator. > > Basically, if |ChromeIdentity| is no-nil, then it is similar to cold start. > > Basically, if |ChromeIdentity| is no-nil, then it is similar to warm start. > > > > I think this change will actually leads to a big change in the code (sorry for > > that). > > > > To get image for a profile, consider using > > > https://cs.chromium.org/chromium/src/ios/public/provider/chrome/browser/signi... > > (I think for now using the cached image should be fine). > > > > Note that an identity may have a NULL user full name and image. It is very > rare > > that an identity has a null email (this in general means there was an error > > fetching this info). > > The view should not know anything related to the profile. The point of the > mediator is to > have an abstraction layer. For completeness sake: ChromeIdentity and ChromeIdentityService do not depend on the profile. I now understand that the intent for the view is to very simple and not depend at all on the model. Thank you for clarifying this. https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h (right): https://codereview.chromium.org/2749703003/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h:17: @property(nonatomic, copy) NSString* userFullName; On 2017/03/21 17:22:29, jlebel wrote: > On 2017/03/16 22:15:31, msarda wrote: > > Mediator should also take a ChromeIdentity object (not username, email and > > image). > > No, that's the point of the TODO. Plus that would mess up the point of > MaterialCellCatalogViewController. We would have to deal with profiles. ChromeIdentity and ChromeIdentityService do not depend on profile. My understanding (which is very limited for now) is that the mediator is supposed to configure the cell and observe all necessary objects to do that. I think Louis needs to do a pass over this CL as I probably do not understand the roles of the mediator, cell, view etc. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/BUILD.gn (right): https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/BUILD.gn:90: "signin_promo_item.h", Note that authentication_arc depends on the //ios/public/provider/chrome/browser/signin which includes ChromeIdentity and ChromeIdentityService etc. If the intent for this SigninPromoCell is never depend on the model, then I suggest we move them to a new and different target - maybe something like 'signin_promo_cell' that does not depend on //ios/public/provider/chrome/browser/signin https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_item.h (right): https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_item.h:22: signinPromoViewConfigurator; Optional nit: s/signinPromoViewConfigurator/configurator https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_item.h:25: signinPromoViewConfigurator: Nit: s/signinPromoViewConfigurator/configurator for better code alignment. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_item.h:32: // Cell representation for SigninPromoItem. The cell contains only a s/. The cell contains ../, containing a single SigninPromoView https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_item.mm (right): https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_item.mm:55: [self addSubviews]; I think calling this method from the constructor is risky. Take the case that somebody subclasses SigninPromoCell and adds its own method named addSubviews (this is not impossible as adding subviews is common in a cell). When the SigninPromoCellSubclass initializes the superclass, it will end up calling addSubviews of the subclass and that will call a function of an partially initialized object. Seeing that the method addSubviews is very small and trivial (and never called apart from the constructor), I would suggest inlining it here. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_item.mm:62: Optional nit: Kill empty line. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_item.mm:71: [super layoutSubviews]; Louis: Please take a look at this double call of layoutSubviews and let us know if this is a common pattern for MDC (I have personally never seen this before). https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view.h (right): https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:25: SigninPromoViewWarmStartMode, I think the only difference between warm and cold modes are the fact that the secondaryButton is hidden (see my comment concerning the image below). If so, then please rename these to: * SigninPromoHideSecondaryButton * SigninPromoShowSecondaryButton Another more simple option would be to add a method setSecondaryButtonHidden:(BOOL)hidden that hides or shows the secondary button. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:30: // + "Cold Start" mode displays the chrome/chomium icon in the image view, and I think the imageView should be configured by the mediator, so it is unnecessary to specify it here. Basically, this class displays the image that is passed to it. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:36: // - the image for |imageView|, using -[SigninPromoView setProfileImage:] I think the mediator should always set the image this view should display. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:51: // label. s/of the text label./of |textLabel| https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:59: - (void)setProfileImage:(UIImage*)image; I think the owner should always set the image (not only for the profileImage case). https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view.mm (right): https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:28: const CGFloat kChromeImageFixedSize = 24; I expect the profile image and the chrome image should have the same size. I think there is a problem in the mocks. If this is the case, then we can use a single size for images and have it be the same constraint in both modes. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:40: NSArray<NSLayoutConstraint*>* _coldStartConstraints; s/_coldStartConstraints/_secondaryButtonVisibleConstraints https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:41: NSArray<NSLayoutConstraint*>* _warmStartConstraints; s/_coldStartConstraints/_secondaryButtonHiddenConstraints https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:62: [self setViewConstraints]; Consider calling [self setMode:SigninPromoViewColdStartMode] (one line instead of 2). https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:130: // Constraints in shared between modes. Constraints shared between modes. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:133: @"kVerticalPaddingkButtonVerticalPadding-[primaryButton(kButtonHeight)]"; Personal pref: Consider removing the @ on this second line. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:142: formatString = @"H:|-kHorizontalPadding-[primaryButton]-kHorizontalPadding-|"; You can put all these visual constraints in a single dictionary and apply them once. This makes the code more compact and it easier to read - for example take a look at https://cs.chromium.org/chromium/src/ios/chrome/browser/ui/authentication/sig... https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:143: [NSLayoutConstraint I think this code would be so much simpler to read if we did the following: * Add an anonymous namespace function that takes an array of NSStrings (the layout format), the metrics and the views. We could name this LayoutConstraintsFromVisualFormat( * returns an NSArray of NSLayoutConstraint Then here we coud be: _coldStartConstraints = LayoutConstraintsFromVisualFormat( @{ @"V:|-kVerticalPaddingx2-[imageView]-kVerticalPadding-[textLabel]-" "kVerticalPaddingkButtonVerticalPadding-[primaryButton(kButtonHeight)]", @"H:|-kHorizontalPadding-[primaryButton]-kHorizontalPadding-|", @"V:[primaryButton]-kVerticalPaddingkButtonVerticalPadding-|", @"H:[imageView(kChromeImageFixedSize)]", @"V:[imageView(kChromeImageFixedSize)]" }, metrics, views); https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:202: _mode = SigninPromoViewWarmStartMode; I think this line needs to be removed. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:221: - (void)activateColdMode { DCHECK(_mode, COLD); https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:222: // TODO(jlebel) Needs to set the chrome/chromium icon in |imageView|. Please remove this TODO as it is the job of the configuration to set the right image all the time. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:228: - (void)activateWarmMode { DCHECK(_mode, WARM); https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h (right): https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h:17: @property(nonatomic, copy) NSString* userFullName; My understanding is that the mediator mediates between the model and the view. If so, then the mediator should use a ChromeIdentity and the ChromeIdentityService. Let's wait for Louis' review though.
Sorry, I haven’t had time to finish yet, especially to look at all the previous comments. Here is a first batch. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/BUILD.gn (right): https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/BUILD.gn:90: "signin_promo_item.h", On 2017/03/22 12:18:38, msarda wrote: > Note that authentication_arc depends on the > //ios/public/provider/chrome/browser/signin which includes ChromeIdentity and > ChromeIdentityService etc. If the intent for this SigninPromoCell is never > depend on the model, then I suggest we move them to a new and different target - > maybe something like 'signin_promo_cell' that does not depend on > //ios/public/provider/chrome/browser/signin Can you create a authentication_ui target in this file that will contain only the UI layer (items, cells, views, view controllers)? This is the target that Showcase should depend on for example. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h (right): https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h:17: @property(nonatomic, copy) NSString* userFullName; On 2017/03/22 12:18:39, msarda wrote: > My understanding is that the mediator mediates between the model and the view. > If so, then the mediator should use a ChromeIdentity and the > ChromeIdentityService. > > Let's wait for Louis' review though. Right, the mediator is not part of the UI layer, and it speaks both the model and UI language. You can init it with a ChromeIdentity for example, and it sources strings to the UI when asked. Mediator is usually an actor, not just a data holder. It handles the translation from ChromeIdentity to strings/images. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/settings/material_cell_catalog_view_controller.mm (right): https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/material_cell_catalog_view_controller.mm:18: #import "ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h" You shouldn't need the mediator here. You need to create an object responding to the SigninPromoViewConfigurator protocol that returns sample data.
https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_item.h (right): https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_item.h:14: Remove empty line. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_item.h:22: signinPromoViewConfigurator; On 2017/03/22 12:18:38, msarda wrote: > Optional nit: s/signinPromoViewConfigurator/configurator +1 https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_item.h:25: signinPromoViewConfigurator: On 2017/03/22 12:18:38, msarda wrote: > Nit: s/signinPromoViewConfigurator/configurator for better code alignment. I would even just get rid of this initializer. People can initialize the item then set the configurator? Otherwise, please make the configurator readonly. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_item.h:32: // Cell representation for SigninPromoItem. The cell contains only a On 2017/03/22 12:18:38, msarda wrote: > s/. The cell contains ../, containing a single SigninPromoView Disagreeing. The current comment is "more clearer", as we saw in the Tech Writing 101 training :) https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_item.mm (right): https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_item.mm:32: - (instancetype)initWithType:(NSInteger)type { No need for this runtime check. The compiler already covers us. (It was not the case in earlier SDKs I think.) https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_item.mm:55: [self addSubviews]; On 2017/03/22 12:18:38, msarda wrote: > I think calling this method from the constructor is risky. Take the case that > somebody subclasses SigninPromoCell and adds its own method named addSubviews > (this is not impossible as adding subviews is common in a cell). When the > SigninPromoCellSubclass initializes the superclass, it will end up calling > addSubviews of the subclass and that will call a function of an partially > initialized object. > > Seeing that the method addSubviews is very small and trivial (and never called > apart from the constructor), I would suggest inlining it here. Agreed. We have some instances of cells that use such helper methods, I should have pushed back back then. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_item.mm:71: [super layoutSubviews]; On 2017/03/22 12:18:38, msarda wrote: > Louis: Please take a look at this double call of layoutSubviews and let us know > if this is a common pattern for MDC (I have personally never seen this before). Correct, this is common pattern, as explained in the previous comment. The reason is that this cell needs to support self-sizing, with labels of unlimited number of lines. https://www.objc.io/issues/3-views/advanced-auto-layout-toolbox/#intrinsic-co... https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_item.mm:74: // changes, for instance on screen rotation. Did you check in the material_cell_catalog that rotations (back and forth) update correctly the layout and never clip the text? Consider testing with the Double Length Strings pseudo language: https://developer.apple.com/library/content/documentation/MacOSX/Conceptual/B... https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view.h (right): https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:6: #define IOS_CHROME_BROWSER_UI_AUTHENTICATION_SIGN_PROMO_VIEW_H_ Since this view has quite some logic, can you add tests? https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:56: // Changes the image in |imageView| with a circular image. Should be called Can you be more explicit with the "circular"? The image that is passed can be whatever, but this method will make it circular. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:59: - (void)setProfileImage:(UIImage*)image; On 2017/03/22 12:18:39, msarda wrote: > I think the owner should always set the image (not only for the profileImage > case). The Chrome logo can stay inside this class, to me. (BTW< did you check that the asset is branded? i.e. Blue for Chromium, colored for Chrome?) https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view.mm (right): https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:28: const CGFloat kChromeImageFixedSize = 24; On 2017/03/22 12:18:39, msarda wrote: > I expect the profile image and the chrome image should have the same size. I > think there is a problem in the mocks. > > If this is the case, then we can use a single size for images and have it be the > same constraint in both modes. That would be simpler for us. Try to push back to UX for either a larger chrome logo, or an asset with padding? Otherwise, we can handle it in code by creating an image with padding. Or with constraints, but it's less readable and encapsulated. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:34: SigninPromoViewMode _mode; These 5 ivars are not needed, as they are generated by the @synthesize below. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:40: NSArray<NSLayoutConstraint*>* _coldStartConstraints; On 2017/03/22 12:18:39, msarda wrote: > s/_coldStartConstraints/_secondaryButtonVisibleConstraints Actually, I like the abstraction the names give. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:50: - (instancetype)initWithCoder:(NSCoder*)aDecoder { No need for this (see similar comment above or below, not sure). https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:62: [self setViewConstraints]; On 2017/03/22 12:18:39, msarda wrote: > Consider calling [self setMode:SigninPromoViewColdStartMode] (one line instead > of 2). Ha! Mihai, you advised against calling methods in init in another file :) Personally, I just put all the code within init, no -addSubviews, -setDefaultViewStyling, etc. But looks good to me. Either keep the previous calls and add -setMode:, or move everything inline in init. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:133: @"kVerticalPaddingkButtonVerticalPadding-[primaryButton(kButtonHeight)]"; On 2017/03/22 12:18:39, msarda wrote: > Personal pref: Consider removing the @ on this second line. Agreed, I thought it was a second string. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:186: options:NSLayoutFormatAlignAllCenterX Isn't tis option already done with line 138? https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:234: - (void)setProfileImage:(UIImage*)image { Can you add a comment in the header that this is a no-op if the mode is COLD? Because the following will just drop the image: view.profileImage = <...> view.mode = WARM It needs to be: view.mode = WARM view.profileImage = <...> https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:249: - (CGFloat)horizontalPadding { Doesn't seem used. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h (right): https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h:14: // TODO(jlebel): Create a subclass that monitor profile notification. monitors presubmit will also ask you to change the TODO format to TODO(crbug.com/xxxxx) https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view_mediator_unittest.mm (right): https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view_mediator_unittest.mm:23: _signinPromoView = OCMStrictClassMock([SigninPromoView class]); Why not just a real SigninPromoView? You don't really care which methods are called, you really care that once configured, the title is this, the image is that, etc. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view_mediator_unittest.mm:88: SigninPromoViewMediator* _mediator; C++ class, so use trailing underscore and snake case for data members. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view_mediator_unittest.mm:104: EXPECT_TRUE(_mediator.coldStart); I don;t see this property on the mediator. Where is it defined? Seems the tests are out of date. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/settings/cells/BUILD.gn (right): https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/cells/BUILD.gn:40: "//ios/chrome/app/strings", Is this still needed? Probably. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/cells/BUILD.gn:45: "//ui/base", Is this still needed? Probably. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/settings/material_cell_catalog_view_controller.mm (right): https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/material_cell_catalog_view_controller.mm:78: ItemTypeWarmStartSigninPromo, BTW, what happened to the hot version?
Patchset #3 (id:180001) has been deleted
The CQ bit was checked by jlebel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #3 (id:200001) has been deleted
Thanks for your input. I changed |configurator| property of SigninPromoItem to be strong (instead of weak). Also I need noticed that string files are not part of this patch: ios/chrome/app/strings/ios_chromium_strings.grd ios/chrome/app/strings/ios_google_chrome_strings.grd I'm not sure how those files work. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/BUILD.gn (right): https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/BUILD.gn:90: "signin_promo_item.h", On 2017/03/22 16:18:49, lpromero wrote: > On 2017/03/22 12:18:38, msarda wrote: > > Note that authentication_arc depends on the > > //ios/public/provider/chrome/browser/signin which includes ChromeIdentity and > > ChromeIdentityService etc. If the intent for this SigninPromoCell is never > > depend on the model, then I suggest we move them to a new and different target > - > > maybe something like 'signin_promo_cell' that does not depend on > > //ios/public/provider/chrome/browser/signin > > Can you create a authentication_ui target in this file that will contain only > the UI layer (items, cells, views, view controllers)? > This is the target that Showcase should depend on for example. If I create authentication_ui target with all views and view controllers, I will have to include //ios/public/provider/chrome/browser/signin since the rest do mix UI and model API. I need to put only my item, cell and view to be able to remove //ios/public/provider/chrome/browser/signin I don't use Showcase, I use "Cell Catalog". https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_item.h (right): https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_item.h:14: On 2017/03/24 10:36:19, lpromero wrote: > Remove empty line. Done. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_item.h:22: signinPromoViewConfigurator; On 2017/03/24 10:36:19, lpromero wrote: > On 2017/03/22 12:18:38, msarda wrote: > > Optional nit: s/signinPromoViewConfigurator/configurator > > +1 Done. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_item.h:25: signinPromoViewConfigurator: On 2017/03/22 12:18:38, msarda wrote: > Nit: s/signinPromoViewConfigurator/configurator for better code alignment. Done. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_item.h:32: // Cell representation for SigninPromoItem. The cell contains only a On 2017/03/24 10:36:19, lpromero wrote: > On 2017/03/22 12:18:38, msarda wrote: > > s/. The cell contains ../, containing a single SigninPromoView > > Disagreeing. The current comment is "more clearer", as we saw in the Tech > Writing 101 training :) Acknowledged. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_item.mm (right): https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_item.mm:32: - (instancetype)initWithType:(NSInteger)type { On 2017/03/24 10:36:19, lpromero wrote: > No need for this runtime check. The compiler already covers us. (It was not the > case in earlier SDKs I think.) Done. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_item.mm:55: [self addSubviews]; On 2017/03/24 10:36:19, lpromero wrote: > On 2017/03/22 12:18:38, msarda wrote: > > I think calling this method from the constructor is risky. Take the case that > > somebody subclasses SigninPromoCell and adds its own method named addSubviews > > (this is not impossible as adding subviews is common in a cell). When the > > SigninPromoCellSubclass initializes the superclass, it will end up calling > > addSubviews of the subclass and that will call a function of an partially > > initialized object. > > > > Seeing that the method addSubviews is very small and trivial (and never called > > apart from the constructor), I would suggest inlining it here. > > Agreed. We have some instances of cells that use such helper methods, I should > have pushed back back then. Done. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_item.mm:62: On 2017/03/22 12:18:38, msarda wrote: > Optional nit: Kill empty line. Done. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_item.mm:71: [super layoutSubviews]; On 2017/03/24 10:36:19, lpromero wrote: > On 2017/03/22 12:18:38, msarda wrote: > > Louis: Please take a look at this double call of layoutSubviews and let us > know > > if this is a common pattern for MDC (I have personally never seen this > before). > > Correct, this is common pattern, as explained in the previous comment. The > reason is that this cell needs to support self-sizing, with labels of unlimited > number of lines. > https://www.objc.io/issues/3-views/advanced-auto-layout-toolbox/#intrinsic-co... Acknowledged. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_item.mm:74: // changes, for instance on screen rotation. On 2017/03/24 10:36:19, lpromero wrote: > Did you check in the material_cell_catalog that rotations (back and forth) > update correctly the layout and never clip the text? Consider testing with the > Double Length Strings pseudo language: > https://developer.apple.com/library/content/documentation/MacOSX/Conceptual/B... Nice tip! But it works like a charm (everything is centered). https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view.h (right): https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:6: #define IOS_CHROME_BROWSER_UI_AUTHENTICATION_SIGN_PROMO_VIEW_H_ On 2017/03/24 10:36:20, lpromero wrote: > Since this view has quite some logic, can you add tests? Done. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:25: SigninPromoViewWarmStartMode, On 2017/03/22 12:18:38, msarda wrote: > I think the only difference between warm and cold modes are the fact that the > secondaryButton is hidden (see my comment concerning the image below). > If so, then please rename these to: > * SigninPromoHideSecondaryButton > * SigninPromoShowSecondaryButton > > Another more simple option would be to add a method > setSecondaryButtonHidden:(BOOL)hidden that hides or shows the secondary button. Since I would like to keep the chrome icon inside the view, the mode is more than just showing/hiding the secondary button. I suggest to rename it to SigninPromoViewColdStateMode/SigninPromoViewWarmStateMode. Does it ok with you? https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:30: // + "Cold Start" mode displays the chrome/chomium icon in the image view, and On 2017/03/22 12:18:39, msarda wrote: > I think the imageView should be configured by the mediator, so it is unnecessary > to specify it here. Basically, this class displays the image that is passed to > it. As Louis mention it, it is nicer to keep it in the view. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:36: // - the image for |imageView|, using -[SigninPromoView setProfileImage:] On 2017/03/22 12:18:38, msarda wrote: > I think the mediator should always set the image this view should display. Acknowledged. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:51: // label. On 2017/03/22 12:18:38, msarda wrote: > s/of the text label./of |textLabel| Done. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:56: // Changes the image in |imageView| with a circular image. Should be called On 2017/03/24 10:36:19, lpromero wrote: > Can you be more explicit with the "circular"? The image that is passed can be > whatever, but this method will make it circular. Done. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:59: - (void)setProfileImage:(UIImage*)image; On 2017/03/24 10:36:20, lpromero wrote: > On 2017/03/22 12:18:39, msarda wrote: > > I think the owner should always set the image (not only for the profileImage > > case). > > The Chrome logo can stay inside this class, to me. (BTW< did you check that the > asset is branded? i.e. Blue for Chromium, colored for Chrome?) Yes, I have the asset with all colors. I will add them in another patch. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view.mm (right): https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:28: const CGFloat kChromeImageFixedSize = 24; On 2017/03/24 10:36:20, lpromero wrote: > On 2017/03/22 12:18:39, msarda wrote: > > I expect the profile image and the chrome image should have the same size. I > > think there is a problem in the mocks. > > > > If this is the case, then we can use a single size for images and have it be > the > > same constraint in both modes. > > That would be simpler for us. Try to push back to UX for either a larger chrome > logo, or an asset with padding? Otherwise, we can handle it in code by creating > an image with padding. Or with constraints, but it's less readable and > encapsulated. I don't understand my code already handles the 2 sizes. I'm not sure why should we push back for juste a size problem. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:34: SigninPromoViewMode _mode; On 2017/03/24 10:36:20, lpromero wrote: > These 5 ivars are not needed, as they are generated by the @synthesize below. Done. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:40: NSArray<NSLayoutConstraint*>* _coldStartConstraints; On 2017/03/24 10:36:20, lpromero wrote: > On 2017/03/22 12:18:39, msarda wrote: > > s/_coldStartConstraints/_secondaryButtonVisibleConstraints > > Actually, I like the abstraction the names give. Acknowledged. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:41: NSArray<NSLayoutConstraint*>* _warmStartConstraints; On 2017/03/22 12:18:39, msarda wrote: > s/_coldStartConstraints/_secondaryButtonHiddenConstraints Acknowledged. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:50: - (instancetype)initWithCoder:(NSCoder*)aDecoder { On 2017/03/24 10:36:20, lpromero wrote: > No need for this (see similar comment above or below, not sure). Done. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:62: [self setViewConstraints]; On 2017/03/24 10:36:20, lpromero wrote: > On 2017/03/22 12:18:39, msarda wrote: > > Consider calling [self setMode:SigninPromoViewColdStartMode] (one line instead > > of 2). > > Ha! Mihai, you advised against calling methods in init in another file :) > Personally, I just put all the code within init, no -addSubviews, > -setDefaultViewStyling, etc. But looks good to me. Either keep the previous > calls and add -setMode:, or move everything inline in init. I can't call -[SigninPromoView setMode:] in the init. It would return without doing anything. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:130: // Constraints in shared between modes. On 2017/03/22 12:18:39, msarda wrote: > Constraints shared between modes. Done. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:133: @"kVerticalPaddingkButtonVerticalPadding-[primaryButton(kButtonHeight)]"; On 2017/03/24 10:36:20, lpromero wrote: > On 2017/03/22 12:18:39, msarda wrote: > > Personal pref: Consider removing the @ on this second line. > > Agreed, I thought it was a second string. Done. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:142: formatString = @"H:|-kHorizontalPadding-[primaryButton]-kHorizontalPadding-|"; On 2017/03/22 12:18:39, msarda wrote: > You can put all these visual constraints in a single dictionary and apply them > once. This makes the code more compact and it easier to read - for example take > a look at > https://cs.chromium.org/chromium/src/ios/chrome/browser/ui/authentication/sig... Done. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:143: [NSLayoutConstraint On 2017/03/22 12:18:39, msarda wrote: > I think this code would be so much simpler to read if we did the following: > * Add an anonymous namespace function that takes an array of NSStrings (the > layout format), the metrics and the views. We could name this > LayoutConstraintsFromVisualFormat( > * returns an NSArray of NSLayoutConstraint > > Then here we coud be: > _coldStartConstraints = LayoutConstraintsFromVisualFormat( > @{ > @"V:|-kVerticalPaddingx2-[imageView]-kVerticalPadding-[textLabel]-" > > "kVerticalPaddingkButtonVerticalPadding-[primaryButton(kButtonHeight)]", > @"H:|-kHorizontalPadding-[primaryButton]-kHorizontalPadding-|", > @"V:[primaryButton]-kVerticalPaddingkButtonVerticalPadding-|", > @"H:[imageView(kChromeImageFixedSize)]", > @"V:[imageView(kChromeImageFixedSize)]" > }, > metrics, views); I will do that in another patch. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:186: options:NSLayoutFormatAlignAllCenterX On 2017/03/24 10:36:20, lpromero wrote: > Isn't tis option already done with line 138? I didn't know it applied to the secondary button even it was not used in the visual constraint from line 138. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:202: _mode = SigninPromoViewWarmStartMode; On 2017/03/22 12:18:39, msarda wrote: > I think this line needs to be removed. Done. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:221: - (void)activateColdMode { On 2017/03/22 12:18:39, msarda wrote: > DCHECK(_mode, COLD); Done. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:222: // TODO(jlebel) Needs to set the chrome/chromium icon in |imageView|. On 2017/03/22 12:18:39, msarda wrote: > Please remove this TODO as it is the job of the configuration to set the right > image all the time. I'm going to follow Louis suggestion and keep it that way. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:228: - (void)activateWarmMode { On 2017/03/22 12:18:39, msarda wrote: > DCHECK(_mode, WARM); Done. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:234: - (void)setProfileImage:(UIImage*)image { On 2017/03/24 10:36:20, lpromero wrote: > Can you add a comment in the header that this is a no-op if the mode is COLD? > Because the following will just drop the image: > view.profileImage = <...> > view.mode = WARM > > It needs to be: > view.mode = WARM > view.profileImage = <...> Should I rephrase: "Should be called only with the "Warm State" mode. The image is not changed when "Cold State" mode."? https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:249: - (CGFloat)horizontalPadding { On 2017/03/24 10:36:20, lpromero wrote: > Doesn't seem used. It is used. It is explained in the header why this property exists: "Horizontal padding used for |textLabel|, |primaryButton| and |secondaryButton|. Used to compute the preferred max layout of |textLabel|." https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h (right): https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h:14: // TODO(jlebel): Create a subclass that monitor profile notification. On 2017/03/24 10:36:20, lpromero wrote: > monitors > presubmit will also ask you to change the TODO format to TODO(crbug.com/xxxxx) Done. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h:17: @property(nonatomic, copy) NSString* userFullName; On 2017/03/22 16:18:49, lpromero wrote: > On 2017/03/22 12:18:39, msarda wrote: > > My understanding is that the mediator mediates between the model and the view. > > If so, then the mediator should use a ChromeIdentity and the > > ChromeIdentityService. > > > > Let's wait for Louis' review though. > > Right, the mediator is not part of the UI layer, and it speaks both the model > and UI language. You can init it with a ChromeIdentity for example, and it > sources strings to the UI when asked. > > Mediator is usually an actor, not just a data holder. It handles the translation > from ChromeIdentity to strings/images. The ProfileSigninPromoViewMediator will be able to handle a ChromeIdentity. See crrev.com/2750403003 https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view_mediator_unittest.mm (right): https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view_mediator_unittest.mm:23: _signinPromoView = OCMStrictClassMock([SigninPromoView class]); On 2017/03/24 10:36:20, lpromero wrote: > Why not just a real SigninPromoView? You don't really care which methods are > called, you really care that once configured, the title is this, the image is > that, etc. I have a better control about what is done on SigninPromoView. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view_mediator_unittest.mm:88: SigninPromoViewMediator* _mediator; On 2017/03/24 10:36:20, lpromero wrote: > C++ class, so use trailing underscore and snake case for data members. Done. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view_mediator_unittest.mm:104: EXPECT_TRUE(_mediator.coldStart); On 2017/03/24 10:36:20, lpromero wrote: > I don;t see this property on the mediator. Where is it defined? Seems the tests > are out of date. Done. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/settings/cells/BUILD.gn (right): https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/cells/BUILD.gn:40: "//ios/chrome/app/strings", On 2017/03/24 10:36:20, lpromero wrote: > Is this still needed? Probably. Needed for "ios/chrome/grit/ios_chromium_strings.h" https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/cells/BUILD.gn:45: "//ui/base", On 2017/03/24 10:36:20, lpromero wrote: > Is this still needed? Probably. Needed for ui/base/l10n/l10n_util.h. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/settings/material_cell_catalog_view_controller.mm (right): https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/material_cell_catalog_view_controller.mm:18: #import "ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h" On 2017/03/22 16:18:49, lpromero wrote: > You shouldn't need the mediator here. You need to create an object responding to > the SigninPromoViewConfigurator protocol that returns sample data. I need to alloc a SigninPromoViewMediator instance. How would I do that? https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/material_cell_catalog_view_controller.mm:78: ItemTypeWarmStartSigninPromo, On 2017/03/24 10:36:20, lpromero wrote: > BTW, what happened to the hot version? It will be done by the current code.
https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view.mm (right): https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:143: [NSLayoutConstraint On 2017/03/24 20:59:09, jlebel wrote: > On 2017/03/22 12:18:39, msarda wrote: > > I think this code would be so much simpler to read if we did the following: > > * Add an anonymous namespace function that takes an array of NSStrings (the > > layout format), the metrics and the views. We could name this > > LayoutConstraintsFromVisualFormat( > > * returns an NSArray of NSLayoutConstraint > > > > Then here we coud be: > > _coldStartConstraints = LayoutConstraintsFromVisualFormat( > > @{ > > @"V:|-kVerticalPaddingx2-[imageView]-kVerticalPadding-[textLabel]-" > > > > "kVerticalPaddingkButtonVerticalPadding-[primaryButton(kButtonHeight)]", > > @"H:|-kHorizontalPadding-[primaryButton]-kHorizontalPadding-|", > > @"V:[primaryButton]-kVerticalPaddingkButtonVerticalPadding-|", > > @"H:[imageView(kChromeImageFixedSize)]", > > @"V:[imageView(kChromeImageFixedSize)]" > > }, > > metrics, views); > > I will do that in another patch. I have a strong preference for this as I think it makes the code more readable. If you want to do this in a future CL, please add a TODO for it. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h (right): https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h:17: @property(nonatomic, copy) NSString* userFullName; On 2017/03/24 20:59:09, jlebel wrote: > On 2017/03/22 16:18:49, lpromero wrote: > > On 2017/03/22 12:18:39, msarda wrote: > > > My understanding is that the mediator mediates between the model and the > view. > > > If so, then the mediator should use a ChromeIdentity and the > > > ChromeIdentityService. > > > > > > Let's wait for Louis' review though. > > > > Right, the mediator is not part of the UI layer, and it speaks both the model > > and UI language. You can init it with a ChromeIdentity for example, and it > > sources strings to the UI when asked. > > > > Mediator is usually an actor, not just a data holder. It handles the > translation > > from ChromeIdentity to strings/images. > > The ProfileSigninPromoViewMediator will be able to handle a ChromeIdentity. > See crrev.com/2750403003 I do not understand the interest of having a SigninPromoViewMediator and a ProfileSigninPromoViewMediator in folder ios/chrome/browser/ui/authentication/ - there should be only one here and it must use the ChromeIdentity objects. If there is a need for a specialized SigninPromoViewMediator in the cell catalog, then this class should be moved in that folder and only used there (I want us to avoid subclassing for the mediator as this adds unnecessary complexity that we'll have to maintain in the future). https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view.h (right): https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:22: SigninPromoViewColdStateMode, I think this should be SigninPromoViewModeColdState and SigninPromoViewModeWarmState See https://google.github.io/styleguide/objcguide.xml?showone=Variable_Names#Vari... https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view.mm (right): https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:119: NSMutableArray* constraints = [NSMutableArray array]; Optional nit: I think that using a local variable for the constraints is not really needed (this also leads to arrays being copied below). I would suggest removing constraints and using _coldStateConstraints or _warmStateConstraints here. https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:120: formatString = Personal pref: I do not like reusing a previously used variable: - it is error-prone in the future, if somebody removes the line here and forgets to remove it below - the name does not really mean anything I would rename it to primaryButtonVerticalConstraint https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:128: // TODO(crbug.com/2749703003): Remove this rule once chrome image is added. Unexpected character: https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:129: formatString = @"V:[imageView(kChromeImageFixedSize)]"; Same here: imageViewVerticalSizeConstraint https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:140: formatString = @"V:[primaryButton]-kButtonVerticalPaddingx2-[" s/buttonsVerticalConstraint https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:149: formatString = Same here: secondaryButtonHorizontalConstraint https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:184: DCHECK(_mode == SigninPromoViewColdStateMode); Prefer DCHECK_EQSigninPromoViewColdStateMode, _mode) as it has better logging. https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:191: DCHECK(_mode == SigninPromoViewWarmStateMode); DCHECK_EQ https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:197: - (void)setProfileImage:(UIImage*)image { I think this method should never be called in the cold mode. Change the if to a DCHECK: DCHECK_EQ(WarmMode, _mode). https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h (right): https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h:14: // TODO(crbug.com/2749703003): Create a subclass that monitor profile s/monitor/monitors https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view_mediator.mm (right): https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view_mediator.mm:32: [signinPromoView.primaryButton From our conversation, I understood the following: * the SigninPromoView will have a cold state and a warm state * the cold state will be entirely configured by the view * the warm state will be configured by the mediator Is this correct? If my understanding is correct, then the mediator should not set the primary button title. https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view_mediator.mm:46: forState:UIControlStateNormal]; Optional nit: UIImage* image = _userImage; if (!image) { image = ios::GetChromeBrowserProvider()->->GetSigninResourcesProvider()->GetDefaultAvatar(); } [signinPromoView setProfileImage:image];
For strings files, the changes should show up in git status, as they are in this repo: https://cs.chromium.org/chromium/src/ios/chrome/app/strings/ios_chromium_stri... https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/BUILD.gn (right): https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/BUILD.gn:90: "signin_promo_item.h", On 2017/03/24 20:59:08, jlebel wrote: > On 2017/03/22 16:18:49, lpromero wrote: > > On 2017/03/22 12:18:38, msarda wrote: > > > Note that authentication_arc depends on the > > > //ios/public/provider/chrome/browser/signin which includes ChromeIdentity > and > > > ChromeIdentityService etc. If the intent for this SigninPromoCell is never > > > depend on the model, then I suggest we move them to a new and different > target > > - > > > maybe something like 'signin_promo_cell' that does not depend on > > > //ios/public/provider/chrome/browser/signin > > > > Can you create a authentication_ui target in this file that will contain only > > the UI layer (items, cells, views, view controllers)? > > This is the target that Showcase should depend on for example. > > If I create authentication_ui target with all views and view controllers, I will > have to include //ios/public/provider/chrome/browser/signin since the rest do > mix UI and model API. > I need to put only my item, cell and view to be able to remove > //ios/public/provider/chrome/browser/signin > > I don't use Showcase, I use "Cell Catalog". > Right, I meant that the authentication_ui will ultimately contain these. In the meantime, they contain as much UI as possible, which does not include VC yet, as it has not been decoupled. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view.mm (right): https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:186: options:NSLayoutFormatAlignAllCenterX On 2017/03/24 20:59:09, jlebel wrote: > On 2017/03/24 10:36:20, lpromero wrote: > > Isn't tis option already done with line 138? > > I didn't know it applied to the secondary button even it was not used in the > visual constraint from line 138. Oh good catch. Line 138 might not set this then. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:249: - (CGFloat)horizontalPadding { On 2017/03/24 20:59:09, jlebel wrote: > On 2017/03/24 10:36:20, lpromero wrote: > > Doesn't seem used. > > It is used. It is explained in the header why this property exists: > "Horizontal padding used for |textLabel|, |primaryButton| and |secondaryButton|. > Used to compute the preferred max layout of |textLabel|." Sounds good. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h (right): https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h:17: @property(nonatomic, copy) NSString* userFullName; On 2017/03/27 09:34:16, msarda wrote: > On 2017/03/24 20:59:09, jlebel wrote: > > On 2017/03/22 16:18:49, lpromero wrote: > > > On 2017/03/22 12:18:39, msarda wrote: > > > > My understanding is that the mediator mediates between the model and the > > view. > > > > If so, then the mediator should use a ChromeIdentity and the > > > > ChromeIdentityService. > > > > > > > > Let's wait for Louis' review though. > > > > > > Right, the mediator is not part of the UI layer, and it speaks both the > model > > > and UI language. You can init it with a ChromeIdentity for example, and it > > > sources strings to the UI when asked. > > > > > > Mediator is usually an actor, not just a data holder. It handles the > > translation > > > from ChromeIdentity to strings/images. > > > > The ProfileSigninPromoViewMediator will be able to handle a ChromeIdentity. > > See crrev.com/2750403003 > > I do not understand the interest of having a SigninPromoViewMediator and a > ProfileSigninPromoViewMediator in folder ios/chrome/browser/ui/authentication/ - > there should be only one here and it must use the ChromeIdentity objects. > > If there is a need for a specialized SigninPromoViewMediator in the cell > catalog, then this class should be moved in that folder and only used there (I > want us to avoid subclassing for the mediator as this adds unnecessary > complexity that we'll have to maintain in the future). Agreed. The cell catalog should not add complexity. https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view_mediator_unittest.mm (right): https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view_mediator_unittest.mm:23: _signinPromoView = OCMStrictClassMock([SigninPromoView class]); On 2017/03/24 20:59:09, jlebel wrote: > On 2017/03/24 10:36:20, lpromero wrote: > > Why not just a real SigninPromoView? You don't really care which methods are > > called, you really care that once configured, the title is this, the image is > > that, etc. > > I have a better control about what is done on SigninPromoView. You mean that the view takes the configurator data as recommendations and might not honor them? https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/settings/material_cell_catalog_view_controller.mm (right): https://codereview.chromium.org/2749703003/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/material_cell_catalog_view_controller.mm:18: #import "ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h" On 2017/03/24 20:59:10, jlebel wrote: > On 2017/03/22 16:18:49, lpromero wrote: > > You shouldn't need the mediator here. You need to create an object responding > to > > the SigninPromoViewConfigurator protocol that returns sample data. > > I need to alloc a SigninPromoViewMediator instance. How would I do that? Can't you just use here: @interface MyConfigurator : NSObject<SigninPromoViewConfigurator> @end that would just implement the protocol? No need to loop the mediator in at all. https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/BUILD.gn (right): https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/BUILD.gn:105: source_set("authentication_ui_arc") { Just call it authentication_ui, as there is no ambiguity. https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_item.h (right): https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_item.h:20: @property(nonatomic, readonly) id<SigninPromoViewConfigurator> configurator; Probably still needs to be weak (but readonly), to avoid cycles. https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_item.mm (right): https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_item.mm:35: l10n_util::GetNSString(IDS_IOS_SIGNIN_PROMO_SETTINGS); Since this is not settable by items clients, I suggest moving it to the cell directly, when creating the signInPromoView. https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_item.mm:49: _signinPromoView = [[SigninPromoView alloc] initWithFrame:self.bounds]; Nit: I prefer to let the creator of the view set the translatesAutorisizingMasksIntoConstraints instead of the view itself. Can you remove it from the view and add it here? https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view.h (right): https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:30: // + "Cold State" mode displays the chrome/chomium icon in the image view, and chromium https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view.mm (right): https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:182: // TODO(crbug.com/2749703003) Needs to set the chrome/chromium icon in Chrome/Chromium https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:182: // TODO(crbug.com/2749703003) Needs to set the chrome/chromium icon in Not a crbug. Here and in several places. https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:197: - (void)setProfileImage:(UIImage*)image { On 2017/03/27 09:34:16, msarda wrote: > I think this method should never be called in the cold mode. Change the if to a > DCHECK: > DCHECK_EQ(WarmMode, _mode). If you DCHECK, you need to remove the early return then.
Hello Mihai and Louis, Thanks for your inputs. https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/BUILD.gn (right): https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/BUILD.gn:105: source_set("authentication_ui_arc") { On 2017/03/27 13:14:46, lpromero wrote: > Just call it authentication_ui, as there is no ambiguity. Done. https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_item.h (right): https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_item.h:20: @property(nonatomic, readonly) id<SigninPromoViewConfigurator> configurator; On 2017/03/27 13:14:46, lpromero wrote: > Probably still needs to be weak (but readonly), to avoid cycles. So you want MaterialCellCatalogViewController to retain the configurator? The cells needs its configurator. The configurator doesn't need the cell. https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_item.mm (right): https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_item.mm:35: l10n_util::GetNSString(IDS_IOS_SIGNIN_PROMO_SETTINGS); On 2017/03/27 13:14:46, lpromero wrote: > Since this is not settable by items clients, I suggest moving it to the cell > directly, when creating the signInPromoView. The issue is then the item will not be usable for bookmark? https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_item.mm:49: _signinPromoView = [[SigninPromoView alloc] initWithFrame:self.bounds]; On 2017/03/27 13:14:46, lpromero wrote: > Nit: I prefer to let the creator of the view set the > translatesAutorisizingMasksIntoConstraints instead of the view itself. Can you > remove it from the view and add it here? Done. https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view.h (right): https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:22: SigninPromoViewColdStateMode, On 2017/03/27 09:34:16, msarda wrote: > I think this should be SigninPromoViewModeColdState and > SigninPromoViewModeWarmState > > See > https://google.github.io/styleguide/objcguide.xml?showone=Variable_Names#Vari... Done. https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:30: // + "Cold State" mode displays the chrome/chomium icon in the image view, and On 2017/03/27 13:14:46, lpromero wrote: > chromium Done. https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view.mm (right): https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:119: NSMutableArray* constraints = [NSMutableArray array]; On 2017/03/27 09:34:16, msarda wrote: > Optional nit: I think that using a local variable for the constraints is not > really needed (this also leads to arrays being copied below). I would suggest > removing constraints and using _coldStateConstraints or _warmStateConstraints > here. My issue about using _coldStateConstraints and _warmStateConstraints is that I will have to make them mutable. This would imply that contraints can be changed. But this will be solved with crrev.com/2778073002 https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:120: formatString = On 2017/03/27 09:34:16, msarda wrote: > Personal pref: I do not like reusing a previously used variable: > - it is error-prone in the future, if somebody removes the line here and forgets > to remove it below > - the name does not really mean anything > > I would rename it to primaryButtonVerticalConstraint Done. https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:128: // TODO(crbug.com/2749703003): Remove this rule once chrome image is added. On 2017/03/27 09:34:16, msarda wrote: > Unexpected character: Done. https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:129: formatString = @"V:[imageView(kChromeImageFixedSize)]"; On 2017/03/27 09:34:16, msarda wrote: > Same here: imageViewVerticalSizeConstraint Done. https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:140: formatString = @"V:[primaryButton]-kButtonVerticalPaddingx2-[" On 2017/03/27 09:34:16, msarda wrote: > s/buttonsVerticalConstraint Done. https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:149: formatString = On 2017/03/27 09:34:16, msarda wrote: > Same here: secondaryButtonHorizontalConstraint Done. https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:182: // TODO(crbug.com/2749703003) Needs to set the chrome/chromium icon in On 2017/03/27 13:14:46, lpromero wrote: > Chrome/Chromium Done. https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:182: // TODO(crbug.com/2749703003) Needs to set the chrome/chromium icon in On 2017/03/27 13:14:46, lpromero wrote: > Not a crbug. Here and in several places. Done. https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:184: DCHECK(_mode == SigninPromoViewColdStateMode); On 2017/03/27 09:34:16, msarda wrote: > Prefer DCHECK_EQSigninPromoViewColdStateMode, _mode) as it has better logging. Done. https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:191: DCHECK(_mode == SigninPromoViewWarmStateMode); On 2017/03/27 09:34:16, msarda wrote: > DCHECK_EQ Done. https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:197: - (void)setProfileImage:(UIImage*)image { On 2017/03/27 13:14:46, lpromero wrote: > On 2017/03/27 09:34:16, msarda wrote: > > I think this method should never be called in the cold mode. Change the if to > a > > DCHECK: > > DCHECK_EQ(WarmMode, _mode). > > If you DCHECK, you need to remove the early return then. The if() makes more sense to me. But I can change it to a DCHECK(). https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h (right): https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h:14: // TODO(crbug.com/2749703003): Create a subclass that monitor profile On 2017/03/27 09:34:16, msarda wrote: > s/monitor/monitors Done. https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view_mediator.mm (right): https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view_mediator.mm:32: [signinPromoView.primaryButton On 2017/03/27 09:34:17, msarda wrote: > From our conversation, I understood the following: > * the SigninPromoView will have a cold state and a warm state > * the cold state will be entirely configured by the view > * the warm state will be configured by the mediator > Is this correct? > > If my understanding is correct, then the mediator should not set the primary > button title. Done. https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view_mediator.mm:46: forState:UIControlStateNormal]; On 2017/03/27 09:34:16, msarda wrote: > Optional nit: > UIImage* image = _userImage; > if (!image) { > image = > ios::GetChromeBrowserProvider()->->GetSigninResourcesProvider()->GetDefaultAvatar(); > } > [signinPromoView setProfileImage:image]; Done.
https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view.mm (right): https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:119: NSMutableArray* constraints = [NSMutableArray array]; On 2017/03/27 21:34:42, jlebel wrote: > On 2017/03/27 09:34:16, msarda wrote: > > Optional nit: I think that using a local variable for the constraints is not > > really needed (this also leads to arrays being copied below). I would suggest > > removing constraints and using _coldStateConstraints or _warmStateConstraints > > here. > > My issue about using _coldStateConstraints and _warmStateConstraints is that I > will have to make them mutable. This would imply that contraints can be changed. > But this will be solved with crrev.com/2778073002 Acknowledged. https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:197: - (void)setProfileImage:(UIImage*)image { On 2017/03/27 21:34:43, jlebel wrote: > On 2017/03/27 13:14:46, lpromero wrote: > > On 2017/03/27 09:34:16, msarda wrote: > > > I think this method should never be called in the cold mode. Change the if > to > > a > > > DCHECK: > > > DCHECK_EQ(WarmMode, _mode). > > > > If you DCHECK, you need to remove the early return then. > > The if() makes more sense to me. But I can change it to a DCHECK(). DCHECK_EQ is better than the if IMHO, for the following reason: If the caller does the following calls: [signinPromoView setProfileImage:profileImage]; [signinPromoView setMode:Warm]; - With the if, tthere is no error, but the image is not set correctly. - With the DCHECK, in dev, we get a crash in debug and we know we need to fix it. https://codereview.chromium.org/2749703003/diff/240001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view.h (right): https://codereview.chromium.org/2749703003/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:24: // password. The user will need to enter more than his/her password. s/password/credentials https://codereview.chromium.org/2749703003/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:35: // The owner is in charge to set: This comment is now only available for the warm state (the cold state is configured internally): // For the warm state, the user is in charge to set: https://codereview.chromium.org/2749703003/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:37: // (only for "Warm State") Remove "(only for "Warm State")" https://codereview.chromium.org/2749703003/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:50: // |secondaryButton|. Used to compute the preferred max layout of |textLabel|. Is this the max "layout" or "width"? https://codereview.chromium.org/2749703003/diff/240001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view.mm (right): https://codereview.chromium.org/2749703003/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:203: if (SigninPromoViewModeColdState == _mode) { DCHECK_EQ is better than the if IMHO, for the following reason: If the caller does the following calls: [signinPromoView setProfileImage:profileImage]; [signinPromoView setMode:Warm]; - With the if, tthere is no error, but the image is not set correctly. - With the DCHECK, in dev, we get a crash in debug and we know we need to fix it. https://codereview.chromium.org/2749703003/diff/240001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h (right): https://codereview.chromium.org/2749703003/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h:17: @property(nonatomic, copy) NSString* userFullName; I think the following comment from a previous patch was missed: msarda's comment: "I do not understand the interest of having a SigninPromoViewMediator and a ProfileSigninPromoViewMediator in folder ios/chrome/browser/ui/authentication/ - there should be only one here and it must use the ChromeIdentity objects. If there is a need for a specialized SigninPromoViewMediator in the cell catalog, then this class should be moved in that folder and only used there (I want us to avoid subclassing for the mediator as this adds unnecessary complexity that we'll have to maintain in the future)." lpromero's response: "Agreed. The cell catalog should not add complexity."
https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view.mm (right): https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:119: NSMutableArray* constraints = [NSMutableArray array]; On 2017/03/27 21:34:42, jlebel wrote: > On 2017/03/27 09:34:16, msarda wrote: > > Optional nit: I think that using a local variable for the constraints is not > > really needed (this also leads to arrays being copied below). I would suggest > > removing constraints and using _coldStateConstraints or _warmStateConstraints > > here. > > My issue about using _coldStateConstraints and _warmStateConstraints is that I > will have to make them mutable. This would imply that contraints can be changed. > But this will be solved with crrev.com/2778073002 Acknowledged. https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:197: - (void)setProfileImage:(UIImage*)image { On 2017/03/27 21:34:43, jlebel wrote: > On 2017/03/27 13:14:46, lpromero wrote: > > On 2017/03/27 09:34:16, msarda wrote: > > > I think this method should never be called in the cold mode. Change the if > to > > a > > > DCHECK: > > > DCHECK_EQ(WarmMode, _mode). > > > > If you DCHECK, you need to remove the early return then. > > The if() makes more sense to me. But I can change it to a DCHECK(). DCHECK_EQ is better than the if IMHO, for the following reason: If the caller does the following calls: [signinPromoView setProfileImage:profileImage]; [signinPromoView setMode:Warm]; - With the if, tthere is no error, but the image is not set correctly. - With the DCHECK, in dev, we get a crash in debug and we know we need to fix it. https://codereview.chromium.org/2749703003/diff/240001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view.h (right): https://codereview.chromium.org/2749703003/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:24: // password. The user will need to enter more than his/her password. s/password/credentials https://codereview.chromium.org/2749703003/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:35: // The owner is in charge to set: This comment is now only available for the warm state (the cold state is configured internally): // For the warm state, the user is in charge to set: https://codereview.chromium.org/2749703003/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:37: // (only for "Warm State") Remove "(only for "Warm State")" https://codereview.chromium.org/2749703003/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:50: // |secondaryButton|. Used to compute the preferred max layout of |textLabel|. Is this the max "layout" or "width"? https://codereview.chromium.org/2749703003/diff/240001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view.mm (right): https://codereview.chromium.org/2749703003/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:203: if (SigninPromoViewModeColdState == _mode) { DCHECK_EQ is better than the if IMHO, for the following reason: If the caller does the following calls: [signinPromoView setProfileImage:profileImage]; [signinPromoView setMode:Warm]; - With the if, tthere is no error, but the image is not set correctly. - With the DCHECK, in dev, we get a crash in debug and we know we need to fix it. https://codereview.chromium.org/2749703003/diff/240001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h (right): https://codereview.chromium.org/2749703003/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h:17: @property(nonatomic, copy) NSString* userFullName; I think the following comment from a previous patch was missed: msarda's comment: "I do not understand the interest of having a SigninPromoViewMediator and a ProfileSigninPromoViewMediator in folder ios/chrome/browser/ui/authentication/ - there should be only one here and it must use the ChromeIdentity objects. If there is a need for a specialized SigninPromoViewMediator in the cell catalog, then this class should be moved in that folder and only used there (I want us to avoid subclassing for the mediator as this adds unnecessary complexity that we'll have to maintain in the future)." lpromero's response: "Agreed. The cell catalog should not add complexity."
+1 for the refactoring to remove the need for a mediator subclass, and removing the mediator from the catalog. https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_item.h (right): https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_item.h:20: @property(nonatomic, readonly) id<SigninPromoViewConfigurator> configurator; On 2017/03/27 21:34:42, jlebel wrote: > On 2017/03/27 13:14:46, lpromero wrote: > > Probably still needs to be weak (but readonly), to avoid cycles. > > So you want MaterialCellCatalogViewController to retain the configurator? The > cells needs its configurator. The configurator doesn't need the cell. MaterialCellCatalog shouldn't be a driver for architectural decisions. Lets imagine we use SigninPromoItem in Settings. The Settings view controller would create and own the mediator, and pass a weak ref to the cells (in practice there is just one cell, but conceptually, you could have several). I see the configurator as a delegate. https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view.mm (right): https://codereview.chromium.org/2749703003/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:197: - (void)setProfileImage:(UIImage*)image { On 2017/03/27 21:34:43, jlebel wrote: > On 2017/03/27 13:14:46, lpromero wrote: > > On 2017/03/27 09:34:16, msarda wrote: > > > I think this method should never be called in the cold mode. Change the if > to > > a > > > DCHECK: > > > DCHECK_EQ(WarmMode, _mode). > > > > If you DCHECK, you need to remove the early return then. > > The if() makes more sense to me. But I can change it to a DCHECK(). What I mean is that if you DCHECK, the Chromium style guide says you shouldn't handle the case where it fails: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.chromium.org/2749703003/diff/240001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view.h (right): https://codereview.chromium.org/2749703003/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:50: // |secondaryButton|. Used to compute the preferred max layout of |textLabel|. On 2017/03/28 08:12:51, msarda wrote: > Is this the max "layout" or "width"? Both: the preferred max layout width :) https://codereview.chromium.org/2749703003/diff/240001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/settings/material_cell_catalog_view_controller.mm (right): https://codereview.chromium.org/2749703003/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/material_cell_catalog_view_controller.mm:484: SigninPromoViewMediator* signinPromoViewMediator = Put the autorelease here. https://codereview.chromium.org/2749703003/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/material_cell_catalog_view_controller.mm:495: [[SigninPromoViewMediator alloc] init]; Put the autoroelease here.
lgtm onc ecomments are addressed. Refactoring of the mediator can happen in a separate CL/
lgtm
Thanks, https://codereview.chromium.org/2749703003/diff/240001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view.h (right): https://codereview.chromium.org/2749703003/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:24: // password. On 2017/03/28 08:12:51, msarda wrote: > The user will need to enter more than his/her password. > s/password/credentials Done. https://codereview.chromium.org/2749703003/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:35: // The owner is in charge to set: On 2017/03/28 08:12:51, msarda wrote: > This comment is now only available for the warm state (the cold state is > configured internally): > > // For the warm state, the user is in charge to set: Done. https://codereview.chromium.org/2749703003/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:37: // (only for "Warm State") On 2017/03/28 08:12:51, msarda wrote: > Remove "(only for "Warm State")" Done. https://codereview.chromium.org/2749703003/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:50: // |secondaryButton|. Used to compute the preferred max layout of |textLabel|. On 2017/03/28 08:12:51, msarda wrote: > Is this the max "layout" or "width"? Done. https://codereview.chromium.org/2749703003/diff/240001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view.mm (right): https://codereview.chromium.org/2749703003/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.mm:203: if (SigninPromoViewModeColdState == _mode) { On 2017/03/28 08:12:52, msarda wrote: > DCHECK_EQ is better than the if IMHO, for the following reason: > > If the caller does the following calls: > [signinPromoView setProfileImage:profileImage]; > [signinPromoView setMode:Warm]; > - With the if, tthere is no error, but the image is not set correctly. > - With the DCHECK, in dev, we get a crash in debug and we know we need to fix > it. Done. https://codereview.chromium.org/2749703003/diff/240001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h (right): https://codereview.chromium.org/2749703003/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h:17: @property(nonatomic, copy) NSString* userFullName; On 2017/03/28 08:12:52, msarda wrote: > I think the following comment from a previous patch was missed: > msarda's comment: > "I do not understand the interest of having a SigninPromoViewMediator and a > ProfileSigninPromoViewMediator in folder ios/chrome/browser/ui/authentication/ - > there should be only one here and it must use the ChromeIdentity objects. > > If there is a need for a specialized SigninPromoViewMediator in the cell > catalog, then this class should be moved in that folder and only used there (I > want us to avoid subclassing for the mediator as this adds unnecessary > complexity that we'll have to maintain in the future)." > > lpromero's response: > "Agreed. The cell catalog should not add complexity." Acknowledged. Will be done in crrev.com/2750403003 https://codereview.chromium.org/2749703003/diff/240001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/settings/material_cell_catalog_view_controller.mm (right): https://codereview.chromium.org/2749703003/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/material_cell_catalog_view_controller.mm:484: SigninPromoViewMediator* signinPromoViewMediator = On 2017/03/28 09:43:15, lpromero wrote: > Put the autorelease here. Done. https://codereview.chromium.org/2749703003/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/material_cell_catalog_view_controller.mm:495: [[SigninPromoViewMediator alloc] init]; On 2017/03/28 09:43:15, lpromero wrote: > Put the autoroelease here. Done.
The CQ bit was checked by jlebel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2749703003/diff/260001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view.h (right): https://codereview.chromium.org/2749703003/diff/260001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:24: // without entering theircredentials. s/theircredentials/their credentials https://codereview.chromium.org/2749703003/diff/260001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:35: // For the warm state, the owner is in charge to set: s/is in charge to set/ should set https://codereview.chromium.org/2749703003/diff/260001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:57: // cropped first). Should be called only with the "Warm State" mode. The image This comment needs to be fixed as the implementation was changed: s/ Should be called only with the "Warm State" mode./Must only be called in "Warm State" mode. Remove "The image is not changed when "Cold State" mode."
https://codereview.chromium.org/2749703003/diff/260001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/authentication/signin_promo_view.h (right): https://codereview.chromium.org/2749703003/diff/260001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:24: // without entering theircredentials. On 2017/03/28 12:49:17, msarda wrote: > s/theircredentials/their credentials Done. https://codereview.chromium.org/2749703003/diff/260001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:35: // For the warm state, the owner is in charge to set: On 2017/03/28 12:49:16, msarda wrote: > s/is in charge to set/ should set Done. https://codereview.chromium.org/2749703003/diff/260001/ios/chrome/browser/ui/... ios/chrome/browser/ui/authentication/signin_promo_view.h:57: // cropped first). Should be called only with the "Warm State" mode. The image On 2017/03/28 12:49:17, msarda wrote: > This comment needs to be fixed as the implementation was changed: > s/ Should be called only with the "Warm State" mode./Must only be called in > "Warm State" mode. > > Remove "The image is not changed when "Cold State" mode." Done.
The CQ bit was checked by jlebel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msarda@chromium.org, lpromero@chromium.org Link to the patchset: https://codereview.chromium.org/2749703003/#ps280001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by jlebel@chromium.org
The CQ bit was checked by jlebel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msarda@chromium.org, lpromero@chromium.org Link to the patchset: https://codereview.chromium.org/2749703003/#ps300001 (title: "Typo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by jlebel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msarda@chromium.org, lpromero@chromium.org Link to the patchset: https://codereview.chromium.org/2749703003/#ps290017 (title: "Removing useless test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jlebel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msarda@chromium.org, lpromero@chromium.org Link to the patchset: https://codereview.chromium.org/2749703003/#ps330001 (title: "Update tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by jlebel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 330001, "attempt_start_ts": 1490777382183080,
"parent_rev": "9cd67cdf097cd50ffc37536579f671ac2e263034", "commit_rev":
"bb883994c62cbdbb2d449d886190d97e24548fcc"}
Message was sent while issue was closed.
Description was changed from ========== Adding mediator for Sign-in promo Creating SigninPromoView and SigninPromoViewMediator. Moving SiginPromoItem from settings/cells to authentication. SigninPromoView can display cold start sign-in promo (with no known profile), or warm start sign-in promo (to be able to sign-in without typing password). This class is now used by SiginPromoCell and will be used in the bookmarks view and any other place. SigninPromoViewMediator can configure a SigninPromoView instance according to the value it contains. An instance of this class is required by the initializer of SigninPromoItem. -[SigninPromoItem configureCell:] uses the SigninPromoViewMediator instance to configuring the SigninPromoCell cell. Now, SigninPromoCell contains only a SigninPromoView instance. TODO: the Chrome/Chromium icon is missing for the cold start view. Screenshot: https://drive.google.com/file/d/0ByXziH_JVCGJcGo5djJvR1BCUjA/view Related to: crrev.com/2747893002 BUG=661794 ========== to ========== Adding mediator for Sign-in promo Creating SigninPromoView and SigninPromoViewMediator. Moving SiginPromoItem from settings/cells to authentication. SigninPromoView can display cold start sign-in promo (with no known profile), or warm start sign-in promo (to be able to sign-in without typing password). This class is now used by SiginPromoCell and will be used in the bookmarks view and any other place. SigninPromoViewMediator can configure a SigninPromoView instance according to the value it contains. An instance of this class is required by the initializer of SigninPromoItem. -[SigninPromoItem configureCell:] uses the SigninPromoViewMediator instance to configuring the SigninPromoCell cell. Now, SigninPromoCell contains only a SigninPromoView instance. TODO: the Chrome/Chromium icon is missing for the cold start view. Screenshot: https://drive.google.com/file/d/0ByXziH_JVCGJcGo5djJvR1BCUjA/view Related to: crrev.com/2747893002 BUG=661794 Review-Url: https://codereview.chromium.org/2749703003 Cr-Commit-Position: refs/heads/master@{#460337} Committed: https://chromium.googlesource.com/chromium/src/+/bb883994c62cbdbb2d449d886190... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:330001) as https://chromium.googlesource.com/chromium/src/+/bb883994c62cbdbb2d449d886190... |
