Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2779693003:
Adding Finch and flag for Sign-in promo (Closed)
|
Created:
3 years, 8 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, arv+watch_chromium.org, marq+watch_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding Finch and flag for Sign-in promo
BUG=661794
Review-Url: https://codereview.chromium.org/2779693003
Cr-Commit-Position: refs/heads/master@{#460374}
Committed: https://chromium.googlesource.com/chromium/src/+/2cf54d71895488bec927412bc9634a51a2981d57
Patch Set 1 : . #
Total comments: 8
Patch Set 2 : . #
Total comments: 4
Patch Set 3 : . #Patch Set 4 : Merge #Patch Set 5 : Merge #Patch Set 6 : Fixing a fix made at the wrong place #
Messages
Total messages: 31 (19 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Adding Finch and flag for Sign-in promo BUG= ========== to ========== Adding Finch and flag for Sign-in promo BUG=661794 ==========
jlebel@chromium.org changed reviewers: + sdefresne@chromium.org
Hello Sylvain, Can you review this patch to add flag and finch for the sign in promo feature that I'm implementing? Thanks,
https://codereview.chromium.org/2779693003/diff/40001/components/signin/core/... File components/signin/core/common/signin_switches.cc (right): https://codereview.chromium.org/2779693003/diff/40001/components/signin/core/... components/signin/core/common/signin_switches.cc:38: // Enables sign-in promo. Please sort alphabetically. https://codereview.chromium.org/2779693003/diff/40001/components/signin/core/... components/signin/core/common/signin_switches.cc:41: // Disables sign-in promo. Ditto. https://codereview.chromium.org/2779693003/diff/40001/components/signin/core/... File components/signin/core/common/signin_switches.h (right): https://codereview.chromium.org/2779693003/diff/40001/components/signin/core/... components/signin/core/common/signin_switches.h:27: Remove the blank line and sort alphabetically with the other global variables. https://codereview.chromium.org/2779693003/diff/40001/ios/chrome/browser/expe... File ios/chrome/browser/experimental_flags.mm (right): https://codereview.chromium.org/2779693003/diff/40001/ios/chrome/browser/expe... ios/chrome/browser/experimental_flags.mm:288: // This call activates the field trial, if needed, so it must come before any This comment contradict the code. Code should either be moved before the two early returns or the comment should be removed.
Thanks, https://codereview.chromium.org/2779693003/diff/40001/components/signin/core/... File components/signin/core/common/signin_switches.cc (right): https://codereview.chromium.org/2779693003/diff/40001/components/signin/core/... components/signin/core/common/signin_switches.cc:38: // Enables sign-in promo. On 2017/03/29 10:37:07, sdefresne wrote: > Please sort alphabetically. Done. https://codereview.chromium.org/2779693003/diff/40001/components/signin/core/... components/signin/core/common/signin_switches.cc:41: // Disables sign-in promo. On 2017/03/29 10:37:07, sdefresne wrote: > Ditto. Done. https://codereview.chromium.org/2779693003/diff/40001/components/signin/core/... File components/signin/core/common/signin_switches.h (right): https://codereview.chromium.org/2779693003/diff/40001/components/signin/core/... components/signin/core/common/signin_switches.h:27: On 2017/03/29 10:37:07, sdefresne wrote: > Remove the blank line and sort alphabetically with the other global variables. Done. https://codereview.chromium.org/2779693003/diff/40001/ios/chrome/browser/expe... File ios/chrome/browser/experimental_flags.mm (right): https://codereview.chromium.org/2779693003/diff/40001/ios/chrome/browser/expe... ios/chrome/browser/experimental_flags.mm:288: // This call activates the field trial, if needed, so it must come before any On 2017/03/29 10:37:07, sdefresne wrote: > This comment contradict the code. Code should either be moved before the two > early returns or the comment should be removed. Done.
sdefresne@chromium.org changed reviewers: + msarda@chromium.org
lgtm => msarda@chromium.org for components/signin OWNERS
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-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
LGTM with nits. Please address them before landing this CL. https://codereview.chromium.org/2779693003/diff/60001/ios/chrome/browser/abou... File ios/chrome/browser/about_flags.mm (right): https://codereview.chromium.org/2779693003/diff/60001/ios/chrome/browser/abou... ios/chrome/browser/about_flags.mm:235: // Populate command line flag for Suggestions UI display. s/Suggestions UI/Sign-in promo https://codereview.chromium.org/2779693003/diff/60001/ios/chrome/browser/expe... File ios/chrome/browser/experimental_flags.mm (right): https://codereview.chromium.org/2779693003/diff/60001/ios/chrome/browser/expe... ios/chrome/browser/experimental_flags.mm:287: // This call activates the field trial. This has to be done only if the flag Remove this comment as it is not relevant (it rested after a copy/paste).
The CQ bit was checked by jlebel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sdefresne@chromium.org, msarda@chromium.org Link to the patchset: https://codereview.chromium.org/2779693003/#ps80001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks https://codereview.chromium.org/2779693003/diff/60001/ios/chrome/browser/abou... File ios/chrome/browser/about_flags.mm (right): https://codereview.chromium.org/2779693003/diff/60001/ios/chrome/browser/abou... ios/chrome/browser/about_flags.mm:235: // Populate command line flag for Suggestions UI display. On 2017/03/29 11:41:35, msarda wrote: > s/Suggestions UI/Sign-in promo Done. https://codereview.chromium.org/2779693003/diff/60001/ios/chrome/browser/expe... File ios/chrome/browser/experimental_flags.mm (right): https://codereview.chromium.org/2779693003/diff/60001/ios/chrome/browser/expe... ios/chrome/browser/experimental_flags.mm:287: // This call activates the field trial. This has to be done only if the flag On 2017/03/29 11:41:35, msarda wrote: > Remove this comment as it is not relevant (it rested after a copy/paste). Done.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jlebel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sdefresne@chromium.org, msarda@chromium.org Link to the patchset: https://codereview.chromium.org/2779693003/#ps100001 (title: "Merge")
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: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by jlebel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sdefresne@chromium.org, msarda@chromium.org Link to the patchset: https://codereview.chromium.org/2779693003/#ps140001 (title: "Fixing a fix made at the wrong place")
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": 140001, "attempt_start_ts": 1490790262515040,
"parent_rev": "b3e89a5e0e9a950e87ec0fafdf3b981fda7ccd40", "commit_rev":
"2cf54d71895488bec927412bc9634a51a2981d57"}
Message was sent while issue was closed.
Description was changed from ========== Adding Finch and flag for Sign-in promo BUG=661794 ========== to ========== Adding Finch and flag for Sign-in promo BUG=661794 Review-Url: https://codereview.chromium.org/2779693003 Cr-Commit-Position: refs/heads/master@{#460374} Committed: https://chromium.googlesource.com/chromium/src/+/2cf54d71895488bec927412bc963... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001) as https://chromium.googlesource.com/chromium/src/+/2cf54d71895488bec927412bc963... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
