Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2450843002:
Indicate that sync was disabled by administrator. (Closed)
DescriptionIndicate that sync was disabled by administrator.
This CL does the folllowing changes to the sign-in confirmation dialog:
* hides the "Personalized Google Services" section and the "sync settings" link when sync is disabled.
* changes tge button titles to [ CANCEL SIGN IN ] and [ SIGN IN ANYWAY ]
* changes the Chrome sync title to "Chrome Sync is disabled by your administrator"
* changes the body of the "Chrome Sync" section to "Your administrator has disabled syncing of your bookmarks, history, passwords, and other settings."
Screenshot with the new UI can be found here (internal like only):
https://screenshot.googleplex.com/XwOrvUgYDvS
BUG=629602
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/30237c53d233c40002cbc7ed6f04b88a525daa52
Cr-Commit-Position: refs/heads/master@{#428337}
Patch Set 1 : Prepare for review #Patch Set 2 : Get the right UI format #Patch Set 3 : Clean-up (prepare for review) #Patch Set 4 : Clean-up (prepare for review) #
Total comments: 4
Patch Set 5 : Address code review. #
Total comments: 2
Messages
Total messages: 50 (36 generated)
Description was changed from ========== Indicate that sync was disabled by administrator BUG=629602 ========== to ========== Indicate that sync was disabled by administrator BUG=629602 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== Indicate that sync was disabled by administrator BUG=629602 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Indicate that sync was disabled by administrator. This CL hides the "Personalized Google Services" section and the "sync settings" link when sync is disabled. BUG=629602 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Indicate that sync was disabled by administrator. This CL hides the "Personalized Google Services" section and the "sync settings" link when sync is disabled. BUG=629602 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Indicate that sync was disabled by administrator. This CL does the folllowing changes to the sign-in confirmation dialog: * hides the "Personalized Google Services" section and the "sync settings" link when sync is disabled. * changes tge button titles to [ CANCEL SIGN IN ] and [ SIGN IN ANYWAY ] * changes the Chrome sync title to "Chrome Sync is disabled by your administrator" * changes the body of the "Chrome Sync" section to "Your administrator has disabled syncing of your bookmarks, history, passwords, and other settings." Screenshot with the new UI can be found here (internal like only): https://screenshot.googleplex.com/KPfqGb6WiP7 BUG=629602 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
msarda@chromium.org changed reviewers: + anthonyvd@chromium.org, xiyuan@chromium.org
anthonyvd@chromium.org: Please review changes all changes in this CL. xiyuan@chromium.org: Please review changes in chrome/browser/resources/signin/sync_confirmation/*
lgtm
The CQ bit was checked by msarda@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
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by msarda@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...
Patchset #4 (id:80001) has been deleted
The CQ bit was checked by msarda@chromium.org to run a CQ dry run
Patchset #3 (id:60001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
msarda@chromium.org changed reviewers: - anthonyvd@chromium.org
Roger: Anthony is OOO and I made some substantial changes since his review as UX/PM wanted a different UI. May I ask you to review this CL?
Description was changed from ========== Indicate that sync was disabled by administrator. This CL does the folllowing changes to the sign-in confirmation dialog: * hides the "Personalized Google Services" section and the "sync settings" link when sync is disabled. * changes tge button titles to [ CANCEL SIGN IN ] and [ SIGN IN ANYWAY ] * changes the Chrome sync title to "Chrome Sync is disabled by your administrator" * changes the body of the "Chrome Sync" section to "Your administrator has disabled syncing of your bookmarks, history, passwords, and other settings." Screenshot with the new UI can be found here (internal like only): https://screenshot.googleplex.com/KPfqGb6WiP7 BUG=629602 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Indicate that sync was disabled by administrator. This CL does the folllowing changes to the sign-in confirmation dialog: * hides the "Personalized Google Services" section and the "sync settings" link when sync is disabled. * changes tge button titles to [ CANCEL SIGN IN ] and [ SIGN IN ANYWAY ] * changes the Chrome sync title to "Chrome Sync is disabled by your administrator" * changes the body of the "Chrome Sync" section to "Your administrator has disabled syncing of your bookmarks, history, passwords, and other settings." Screenshot with the new UI can be found here (internal like only): https://screenshot.googleplex.com/XwOrvUgYDvS BUG=629602 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
msarda@chromium.org changed reviewers: + rogerta@chromium.org
Roger: Anthony is OOO and I made some substantial changes since his review as UX/PM wanted a different UI. May I ask you to review this CL?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by msarda@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...
Patchset #3 (id:100001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_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 msarda@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.
lgtm https://codereview.chromium.org/2450843002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/signin/sync_confirmation/sync_confirmation.html (right): https://codereview.chromium.org/2450843002/diff/140001/chrome/browser/resourc... chrome/browser/resources/signin/sync_confirmation/sync_confirmation.html:70: <div class="message-container" id='syncSettingsLinkContainer'> Just curious why you are adding an id here and line 59, since I don't see them used. Maybe I missed something. https://codereview.chromium.org/2450843002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/signin/sync_confirmation_ui.cc (right): https://codereview.chromium.org/2450843002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/sync_confirmation_ui.cc:66: } Nit: I think I'd only set three int variables inside the if/else block, that way no need to duplicate string names and calls.
https://codereview.chromium.org/2450843002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/signin/sync_confirmation/sync_confirmation.html (right): https://codereview.chromium.org/2450843002/diff/140001/chrome/browser/resourc... chrome/browser/resources/signin/sync_confirmation/sync_confirmation.html:70: <div class="message-container" id='syncSettingsLinkContainer'> On 2016/10/27 14:18:12, Roger Tawa wrote: > Just curious why you are adding an id here and line 59, since I don't see them > used. Maybe I missed something. Good catch (I iterated over this file multiple times). Reverted these changes. https://codereview.chromium.org/2450843002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/signin/sync_confirmation_ui.cc (right): https://codereview.chromium.org/2450843002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/sync_confirmation_ui.cc:66: } On 2016/10/27 14:18:12, Roger Tawa wrote: > Nit: I think I'd only set three int variables inside the if/else block, that way > no need to duplicate string names and calls. Done.
The CQ bit was checked by msarda@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from anthonyvd@chromium.org, xiyuan@chromium.org, rogerta@chromium.org Link to the patchset: https://codereview.chromium.org/2450843002/#ps160001 (title: "Address code review.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
msarda@chromium.org changed reviewers: + pkasting@chromium.org
pkasting@chromium.org: Please review changes in chrome/browser/ui/cocoa/profiles/signin_view_controller_delegate_mac.mm chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.cc Thank you.
The CQ bit was unchecked by msarda@chromium.org
LGTM https://codereview.chromium.org/2450843002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.cc (right): https://codereview.chromium.org/2450843002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.cc:30: const int kSigninErrorDialogHeight = 164; These numbers are so horribly magic :( I hope we fix this in Harmony...
The CQ bit was checked by msarda@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2450843002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.cc (right): https://codereview.chromium.org/2450843002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.cc:30: const int kSigninErrorDialogHeight = 164; On 2016/10/27 22:14:50, Peter Kasting wrote: > These numbers are so horribly magic :( I hope we fix this in Harmony... Acknowledged.
Message was sent while issue was closed.
Description was changed from ========== Indicate that sync was disabled by administrator. This CL does the folllowing changes to the sign-in confirmation dialog: * hides the "Personalized Google Services" section and the "sync settings" link when sync is disabled. * changes tge button titles to [ CANCEL SIGN IN ] and [ SIGN IN ANYWAY ] * changes the Chrome sync title to "Chrome Sync is disabled by your administrator" * changes the body of the "Chrome Sync" section to "Your administrator has disabled syncing of your bookmarks, history, passwords, and other settings." Screenshot with the new UI can be found here (internal like only): https://screenshot.googleplex.com/XwOrvUgYDvS BUG=629602 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Indicate that sync was disabled by administrator. This CL does the folllowing changes to the sign-in confirmation dialog: * hides the "Personalized Google Services" section and the "sync settings" link when sync is disabled. * changes tge button titles to [ CANCEL SIGN IN ] and [ SIGN IN ANYWAY ] * changes the Chrome sync title to "Chrome Sync is disabled by your administrator" * changes the body of the "Chrome Sync" section to "Your administrator has disabled syncing of your bookmarks, history, passwords, and other settings." Screenshot with the new UI can be found here (internal like only): https://screenshot.googleplex.com/XwOrvUgYDvS BUG=629602 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #5 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Indicate that sync was disabled by administrator. This CL does the folllowing changes to the sign-in confirmation dialog: * hides the "Personalized Google Services" section and the "sync settings" link when sync is disabled. * changes tge button titles to [ CANCEL SIGN IN ] and [ SIGN IN ANYWAY ] * changes the Chrome sync title to "Chrome Sync is disabled by your administrator" * changes the body of the "Chrome Sync" section to "Your administrator has disabled syncing of your bookmarks, history, passwords, and other settings." Screenshot with the new UI can be found here (internal like only): https://screenshot.googleplex.com/XwOrvUgYDvS BUG=629602 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Indicate that sync was disabled by administrator. This CL does the folllowing changes to the sign-in confirmation dialog: * hides the "Personalized Google Services" section and the "sync settings" link when sync is disabled. * changes tge button titles to [ CANCEL SIGN IN ] and [ SIGN IN ANYWAY ] * changes the Chrome sync title to "Chrome Sync is disabled by your administrator" * changes the body of the "Chrome Sync" section to "Your administrator has disabled syncing of your bookmarks, history, passwords, and other settings." Screenshot with the new UI can be found here (internal like only): https://screenshot.googleplex.com/XwOrvUgYDvS BUG=629602 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/30237c53d233c40002cbc7ed6f04b88a525daa52 Cr-Commit-Position: refs/heads/master@{#428337} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/30237c53d233c40002cbc7ed6f04b88a525daa52 Cr-Commit-Position: refs/heads/master@{#428337} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
