Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2880333003:
Adding ChromeIdentity in ShowSigninCommand. (Closed)
|
Created:
3 years, 7 months ago by jlebel Modified:
3 years, 7 months ago CC:
chromium-reviews, marq+watch_chromium.org, ios-reviews+chrome_chromium.org, noyau+watch_chromium.org, ios-reviews_chromium.org, pkl (ping after 24h if needed) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding ChromeIdentity in ShowSigninCommand.
The chrome identity used to setup the SigninPromoView should be the one
used for the sign-in using the operation
AUTHENTICATION_OPERATION_SIGNIN_PROMO_CONTINUE_AS.
BUG=661794
Review-Url: https://codereview.chromium.org/2880333003
Cr-Commit-Position: refs/heads/master@{#473606}
Committed: https://chromium.googlesource.com/chromium/src/+/c1fe1f6a3c9e8e165f915a3f3545a894b6b90abe
Patch Set 1 #
Total comments: 8
Patch Set 2 : . #
Total comments: 4
Patch Set 3 : Comment + DCHECK #
Total comments: 2
Patch Set 4 : DCHECK fix #Patch Set 5 : Merge #Patch Set 6 : Fixing the build #
Dependent Patchsets: Messages
Total messages: 31 (19 generated)
Description was changed from ========== Using the right chrome identity BUG=661794 ========== to ========== Adding ChromeIdentity in ShowSigninCommand. BUG=661794 ==========
Description was changed from ========== Adding ChromeIdentity in ShowSigninCommand. BUG=661794 ========== to ========== Adding ChromeIdentity in ShowSigninCommand. The chrome identity used to setup the SigninPromoView should be the one used for the sign-in using the operation AUTHENTICATION_OPERATION_SIGNIN_PROMO_CONTINUE_AS. BUG=661794 ==========
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
jlebel@chromium.org changed reviewers: + msarda@chromium.org
Hello Mihai, Can you review this patch about adding the chrome identity in the command? Thanks,
https://codereview.chromium.org/2880333003/diff/60001/ios/chrome/app/main_con... File ios/chrome/app/main_controller.mm (right): https://codereview.chromium.org/2880333003/diff/60001/ios/chrome/app/main_con... ios/chrome/app/main_controller.mm:2122: DCHECK(!identity); Remove this new DCHECK (see comment below). https://codereview.chromium.org/2880333003/diff/60001/ios/chrome/app/main_con... ios/chrome/app/main_controller.mm:2128: case AUTHENTICATION_OPERATION_SIGNIN_PROMO_CONTINUE_AS: I think we should remove AUTHENTICATION_OPERATION_SIGNIN_PROMO_CONTINUE_AS and just use AUTHENTICATION_OPERATION_SIGNIN instead. https://codereview.chromium.org/2880333003/diff/60001/ios/chrome/browser/ui/a... File ios/chrome/browser/ui/authentication/signin_promo_view_mediator.mm (right): https://codereview.chromium.org/2880333003/diff/60001/ios/chrome/browser/ui/a... ios/chrome/browser/ui/authentication/signin_promo_view_mediator.mm:209: initWithOperation:AUTHENTICATION_OPERATION_SIGNIN_PROMO_CONTINUE_AS Use AUTHENTICATION_OPERATION_SIGNIN. https://codereview.chromium.org/2880333003/diff/60001/ios/chrome/browser/ui/a... ios/chrome/browser/ui/authentication/signin_promo_view_mediator.mm:213: callback:nil]; Remove "callback" (it is nil by default).
https://codereview.chromium.org/2880333003/diff/60001/ios/chrome/app/main_con... File ios/chrome/app/main_controller.mm (right): https://codereview.chromium.org/2880333003/diff/60001/ios/chrome/app/main_con... ios/chrome/app/main_controller.mm:2122: DCHECK(!identity); On 2017/05/16 13:33:53, msarda wrote: > Remove this new DCHECK (see comment below). Done. https://codereview.chromium.org/2880333003/diff/60001/ios/chrome/app/main_con... ios/chrome/app/main_controller.mm:2128: case AUTHENTICATION_OPERATION_SIGNIN_PROMO_CONTINUE_AS: On 2017/05/16 13:33:53, msarda wrote: > I think we should remove AUTHENTICATION_OPERATION_SIGNIN_PROMO_CONTINUE_AS and > just use AUTHENTICATION_OPERATION_SIGNIN instead. Done. https://codereview.chromium.org/2880333003/diff/60001/ios/chrome/browser/ui/a... File ios/chrome/browser/ui/authentication/signin_promo_view_mediator.mm (right): https://codereview.chromium.org/2880333003/diff/60001/ios/chrome/browser/ui/a... ios/chrome/browser/ui/authentication/signin_promo_view_mediator.mm:209: initWithOperation:AUTHENTICATION_OPERATION_SIGNIN_PROMO_CONTINUE_AS On 2017/05/16 13:33:53, msarda wrote: > Use AUTHENTICATION_OPERATION_SIGNIN. Done. https://codereview.chromium.org/2880333003/diff/60001/ios/chrome/browser/ui/a... ios/chrome/browser/ui/authentication/signin_promo_view_mediator.mm:213: callback:nil]; On 2017/05/16 13:33:53, msarda wrote: > Remove "callback" (it is nil by default). -[ShowSigninCommand initWithOperation:identity:accessPoint:promoAction:] doesn't exist. I'm not sure it is relevant to create this method since it is used only once. I would prefer to keep it that way if it is ok with you.
jlebel@chromium.org changed reviewers: + sdefresne@chromium.org
Hello Sylvain, Can you review this patch? Thanks,
https://codereview.chromium.org/2880333003/diff/80001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/commands/show_signin_command.h (right): https://codereview.chromium.org/2880333003/diff/80001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/commands/show_signin_command.h:59: // AUTHENTICATION_OPERATION_SIGNIN_PROMO_CONTINUE_AS (|identity should not be You are removing the "AUTHENTICATION_OPERATION_SIGNIN_PROMO_CONTINUE_AS" enumeration value, so this comment should be rewritten without mention of the enum value or the enum value should not be removed. https://codereview.chromium.org/2880333003/diff/80001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/commands/show_signin_command.mm (right): https://codereview.chromium.org/2880333003/diff/80001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/commands/show_signin_command.mm:34: _operation = operation; Can you add a DCHECK to ensure the invariant that is described in the comment? Something like this: DCHECK( (identity == nil && operation != AUTHENTICATION_OPERATION_SIGNIN_PROMO_CONTINUE_AS) || (identity != nil && operation == AUTHENTICATION_OPERATION_SIGNIN_PROMO_CONTINUE_AS));
LGTM pending changes asked by Sylvain.
https://codereview.chromium.org/2880333003/diff/80001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/commands/show_signin_command.h (right): https://codereview.chromium.org/2880333003/diff/80001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/commands/show_signin_command.h:59: // AUTHENTICATION_OPERATION_SIGNIN_PROMO_CONTINUE_AS (|identity should not be On 2017/05/17 08:49:01, sdefresne wrote: > You are removing the "AUTHENTICATION_OPERATION_SIGNIN_PROMO_CONTINUE_AS" > enumeration value, so this comment should be rewritten without mention of the > enum value or the enum value should not be removed. Done. https://codereview.chromium.org/2880333003/diff/80001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/commands/show_signin_command.mm (right): https://codereview.chromium.org/2880333003/diff/80001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/commands/show_signin_command.mm:34: _operation = operation; On 2017/05/17 08:49:01, sdefresne wrote: > Can you add a DCHECK to ensure the invariant that is described in the comment? > Something like this: > > DCHECK( > (identity == nil && operation != > AUTHENTICATION_OPERATION_SIGNIN_PROMO_CONTINUE_AS) || > (identity != nil && operation == > AUTHENTICATION_OPERATION_SIGNIN_PROMO_CONTINUE_AS)); 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
https://codereview.chromium.org/2880333003/diff/100001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/commands/show_signin_command.mm (right): https://codereview.chromium.org/2880333003/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/commands/show_signin_command.mm:34: DCHECK(operation != AUTHENTICATION_OPERATION_SIGNIN && identity != nil); I think this is incorrect as it will fail if operation == AUTHENTICATION_OPERATION_SIGNIN and identity == nil which is supported. I think you want "operation != AUTHENTICATION_OPERATION_SIGNIN implies identity == nil". This can be "A => B" can be encoded as "(not A) or B" so, I think the correct DCHECK is: DCHECK(operation == AUTHENTICATION_OPERATION_SIGNIN || identity == nil);
Yes, you are right! Sorry... :( https://codereview.chromium.org/2880333003/diff/100001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/commands/show_signin_command.mm (right): https://codereview.chromium.org/2880333003/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/commands/show_signin_command.mm:34: DCHECK(operation != AUTHENTICATION_OPERATION_SIGNIN && identity != nil); On 2017/05/17 12:45:55, sdefresne wrote: > I think this is incorrect as it will fail if operation == > AUTHENTICATION_OPERATION_SIGNIN and identity == nil which is supported. I think > you want "operation != AUTHENTICATION_OPERATION_SIGNIN implies identity == nil". > This can be "A => B" can be encoded as "(not A) or B" so, I think the correct > DCHECK is: > > DCHECK(operation == AUTHENTICATION_OPERATION_SIGNIN || identity == nil); Done.
lgtm On 2017/05/17 13:11:06, jlebel wrote: > Yes, you are right! Sorry... :( No need to be sorry. Those boolean identities are always tricky. > https://codereview.chromium.org/2880333003/diff/100001/ios/chrome/browser/ui/... > File ios/chrome/browser/ui/commands/show_signin_command.mm (right): > > https://codereview.chromium.org/2880333003/diff/100001/ios/chrome/browser/ui/... > ios/chrome/browser/ui/commands/show_signin_command.mm:34: DCHECK(operation != > AUTHENTICATION_OPERATION_SIGNIN && identity != nil); > On 2017/05/17 12:45:55, sdefresne wrote: > > I think this is incorrect as it will fail if operation == > > AUTHENTICATION_OPERATION_SIGNIN and identity == nil which is supported. I > think > > you want "operation != AUTHENTICATION_OPERATION_SIGNIN implies identity == > nil". > > This can be "A => B" can be encoded as "(not A) or B" so, I think the correct > > DCHECK is: > > > > DCHECK(operation == AUTHENTICATION_OPERATION_SIGNIN || identity == nil); > > 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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, sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/2880333003/#ps160001 (title: "Fixing the build")
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": 160001, "attempt_start_ts": 1495472288090140,
"parent_rev": "b969575157e63be01ba165a4f1bcb7f6cc12b483", "commit_rev":
"c1fe1f6a3c9e8e165f915a3f3545a894b6b90abe"}
Message was sent while issue was closed.
Description was changed from ========== Adding ChromeIdentity in ShowSigninCommand. The chrome identity used to setup the SigninPromoView should be the one used for the sign-in using the operation AUTHENTICATION_OPERATION_SIGNIN_PROMO_CONTINUE_AS. BUG=661794 ========== to ========== Adding ChromeIdentity in ShowSigninCommand. The chrome identity used to setup the SigninPromoView should be the one used for the sign-in using the operation AUTHENTICATION_OPERATION_SIGNIN_PROMO_CONTINUE_AS. BUG=661794 Review-Url: https://codereview.chromium.org/2880333003 Cr-Commit-Position: refs/heads/master@{#473606} Committed: https://chromium.googlesource.com/chromium/src/+/c1fe1f6a3c9e8e165f915a3f3545... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:160001) as https://chromium.googlesource.com/chromium/src/+/c1fe1f6a3c9e8e165f915a3f3545... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
