Update top-level "connected devices" settings item |
||||||||||||||
Issue descriptionPreviously, the "network call to CryptAuth failed" and "call succeeded, but host not yet verified" cases were separated out with different UIs. UX has asked us to unify these into one state.
,
Aug 16
,
Aug 22
,
Aug 23
Additionally, we need to handle these cases: (1) User has no verified hosts - the item should be visible but no setup button. (2) Suite is disabled by policy - the item should be visible with the enterprise logo.
,
Aug 24
Could you switch the name back? I'll make separate bugs for those two items but it's difficult to track with a vague title
,
Aug 24
Issue 877334 has been merged into this issue.
,
Aug 24
Re: comment #5: It's not worth making these into two separate bugs, since fixing all the aforementioned issues can be easily achieved in one small CL.
,
Aug 27
Given that each change requires new browser tests, I plan to split them up into separate CLs.
,
Aug 28
,
Aug 29
,
Sep 5
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/51bd5c40dd8f4606fded035dba07872a089af71b commit 51bd5c40dd8f4606fded035dba07872a089af71b Author: Jordy Greenblatt <jordynass@chromium.org> Date: Tue Sep 04 19:04:40 2018 [CrOS MultiDevice] Add 'no eligible devices' page to Settings UI The original design specified that we detach/hide the MultiDevice page/ page-container in settings when no eligible host phone is found. This CL updates the UI to match the new mocks. This involved changing the settings-multidevice-page-container element so it does not hide the settings-multidevice-page and changing the settings-multidevice-page element so it matches the mocks when there are no eligible hosts. Here is a screenshot of the new case: https://screenshot.googleplex.com/vD780e3CQAD --- Note that I reverted the change of 'isHostSet' in MultiDeviceFeatureBehavior from a method to a computed Polymer property in https://chromium-review.googlesource.com/c/chromium/src/+/1191666 because this CL required that I bind to both isHostSet and pageContentData in order to be sure my bindings had the right value of isHostSet, which is in turn computed from pageContentData. I thought it would be better to switch isHostSet back to a method to avoid more a more convoluted binding dependency graph. Bug: 870127 Change-Id: I1f23a645216aa412894f2729a91603d998acf4eb Reviewed-on: https://chromium-review.googlesource.com/1196168 Commit-Queue: Jordy Greenblatt <jordynass@chromium.org> Reviewed-by: Tommy Li <tommycli@chromium.org> Reviewed-by: Kyle Horimoto <khorimoto@chromium.org> Cr-Commit-Position: refs/heads/master@{#588605} [modify] https://crrev.com/51bd5c40dd8f4606fded035dba07872a089af71b/chrome/app/settings_strings.grdp [modify] https://crrev.com/51bd5c40dd8f4606fded035dba07872a089af71b/chrome/browser/resources/settings/multidevice_page/multidevice_feature_behavior.js [modify] https://crrev.com/51bd5c40dd8f4606fded035dba07872a089af71b/chrome/browser/resources/settings/multidevice_page/multidevice_page.html [modify] https://crrev.com/51bd5c40dd8f4606fded035dba07872a089af71b/chrome/browser/resources/settings/multidevice_page/multidevice_page.js [modify] https://crrev.com/51bd5c40dd8f4606fded035dba07872a089af71b/chrome/browser/resources/settings/multidevice_page/multidevice_page_container.js [modify] https://crrev.com/51bd5c40dd8f4606fded035dba07872a089af71b/chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc [modify] https://crrev.com/51bd5c40dd8f4606fded035dba07872a089af71b/chrome/test/data/webui/settings/multidevice_page_container_tests.js [modify] https://crrev.com/51bd5c40dd8f4606fded035dba07872a089af71b/chrome/test/data/webui/settings/multidevice_page_tests.js
,
Sep 5
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/39424745964259cf783eaced301de01ce1b76b2f commit 39424745964259cf783eaced301de01ce1b76b2f Author: Jordy Greenblatt <jordynass@chromium.org> Date: Wed Sep 05 19:46:08 2018 [CrOS MultiDevice] Change Settings UI no server page to match new mocks The mocks now require that the Settings UI page when there is there is no response from the server has a disabled "Verify" button instead of the old active "Retry" button. Here are screenshots for this CL deployed: Waiting for verification: http://screen/E19zdKcijRN Waiting for server: http://screen/SSkhb7tzDkb Bug: 870127 Change-Id: If529edc1b6756de4fa6ecc5f5c8d0346e551a926 Reviewed-on: https://chromium-review.googlesource.com/1205596 Reviewed-by: Jeremy Klein <jlklein@chromium.org> Reviewed-by: Hector Carmona <hcarmona@chromium.org> Commit-Queue: Jordy Greenblatt <jordynass@chromium.org> Cr-Commit-Position: refs/heads/master@{#588973} [modify] https://crrev.com/39424745964259cf783eaced301de01ce1b76b2f/chrome/browser/resources/settings/multidevice_page/multidevice_page.html [modify] https://crrev.com/39424745964259cf783eaced301de01ce1b76b2f/chrome/browser/resources/settings/multidevice_page/multidevice_page.js [modify] https://crrev.com/39424745964259cf783eaced301de01ce1b76b2f/chrome/test/data/webui/settings/multidevice_page_tests.js
,
Sep 6
Jordy, do these need to be merged?
,
Sep 6
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f40d7aaad6b2d37eaa691e8ec01b975cc379533c commit f40d7aaad6b2d37eaa691e8ec01b975cc379533c Author: Jordy Greenblatt <jordynass@chromium.org> Date: Thu Sep 06 21:23:47 2018 [CrOS MultiDevice] Reflect policy prohibition in Settings. This CL shows a policy indicator and a disabled toggle when the suite or a specific feature (on the subpage) is prohibited by policy. Note that some unrealistic cases are not handled. This includes: 1) suite is prohibited but the mode (a.k.a. host status) is something other than "no eligible hosts" 2) the user is in the subpage but the suite is prohibited by policy. Screenshots: Suite prohibited by policy (main Settings UI page): http://screen/EbYef2RFFrf Individual feature prohibited by policy (subpage): http://screen/D3iqHQCW4B3 Bug: 870127 Change-Id: Iebeed37255e4a3dffb4f1b6376acfe5a9c26cc2d Reviewed-on: https://chromium-review.googlesource.com/1208900 Reviewed-by: Hector Carmona <hcarmona@chromium.org> Reviewed-by: Jeremy Klein <jlklein@chromium.org> Commit-Queue: Jordy Greenblatt <jordynass@chromium.org> Cr-Commit-Position: refs/heads/master@{#589303} [modify] https://crrev.com/f40d7aaad6b2d37eaa691e8ec01b975cc379533c/chrome/browser/resources/settings/multidevice_page/multidevice_feature_behavior.js [modify] https://crrev.com/f40d7aaad6b2d37eaa691e8ec01b975cc379533c/chrome/browser/resources/settings/multidevice_page/multidevice_feature_item.html [modify] https://crrev.com/f40d7aaad6b2d37eaa691e8ec01b975cc379533c/chrome/browser/resources/settings/multidevice_page/multidevice_feature_item.js [modify] https://crrev.com/f40d7aaad6b2d37eaa691e8ec01b975cc379533c/chrome/browser/resources/settings/multidevice_page/multidevice_page.html [modify] https://crrev.com/f40d7aaad6b2d37eaa691e8ec01b975cc379533c/chrome/browser/resources/settings/multidevice_page/multidevice_page.js [modify] https://crrev.com/f40d7aaad6b2d37eaa691e8ec01b975cc379533c/chrome/test/data/webui/settings/multidevice_page_tests.js
,
Sep 6
,
Sep 6
@jhawkins, yes, but they required an additional CL (in Comment 14)
,
Sep 7
This bug requires manual review: There is .grd file changes and we are only 38 days from stable. Please contact the milestone owner if you have questions. Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Sep 10
Jordy, we need a few changes to your first CL. See my comments on https://chromium-review.googlesource.com/c/chromium/src/+/1205596 for details.
,
Sep 12
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e92f4a575519fe408146ba41a09b2eb5c4d4384d commit e92f4a575519fe408146ba41a09b2eb5c4d4384d Author: Jordy Greenblatt <jordynass@chromium.org> Date: Wed Sep 12 21:50:49 2018 [CrOS MultiDevice] Simplify Settings UI modes for user. In my CL 1205596, I implemented the "waiting for server" mode in the Settings UI MultiDevice (a.k.a. Connected Devices) main page in accordance with with specs for when the device is offline. This CL corrects that so they match the "waiting for verification" mode instead. Screenshots of this CL deployed... in "waiting for server" mode: https://drive.google.com/file/d/0B1-cJJPK-cu6OFk5WDYwU29CLWxKYS1JQk44Q05xQjlycGo4/view?usp=sharing and in "waiting for verification" mode: https://drive.google.com/file/d/0B1-cJJPK-cu6NU1Fenlid3VFOWllTGZTZ0ZfemZ4eEltbXdj/view?usp=sharing Bug: 870127 Change-Id: Ie69a0e939246f9d4a6c587e359b83807435f5299 Reviewed-on: https://chromium-review.googlesource.com/1216717 Reviewed-by: Kyle Horimoto <khorimoto@chromium.org> Reviewed-by: Hector Carmona <hcarmona@chromium.org> Commit-Queue: Jordy Greenblatt <jordynass@chromium.org> Cr-Commit-Position: refs/heads/master@{#590832} [modify] https://crrev.com/e92f4a575519fe408146ba41a09b2eb5c4d4384d/chrome/app/settings_strings.grdp [modify] https://crrev.com/e92f4a575519fe408146ba41a09b2eb5c4d4384d/chrome/browser/resources/settings/multidevice_page/multidevice_page.html [modify] https://crrev.com/e92f4a575519fe408146ba41a09b2eb5c4d4384d/chrome/browser/resources/settings/multidevice_page/multidevice_page.js [modify] https://crrev.com/e92f4a575519fe408146ba41a09b2eb5c4d4384d/chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc [modify] https://crrev.com/e92f4a575519fe408146ba41a09b2eb5c4d4384d/chrome/test/data/webui/settings/multidevice_page_tests.js
,
Sep 12
Here's a summary of the rules that were executed: - OnlyMergeApprovedChange: Rule Failed -- Revision 51b4ec01cf01ccf445fc8b4adfced845d845261b was merged to refs/branch-heads/3538 branch with no merge approval from a TPM! Please explain why this change was merged to the branch!
,
Sep 12
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/51b4ec01cf01ccf445fc8b4adfced845d845261b commit 51b4ec01cf01ccf445fc8b4adfced845d845261b Author: Kyle Horimoto <khorimoto@google.com> Date: Wed Sep 12 23:57:50 2018 [CrOS MultiDevice] Add 'no eligible devices' page to Settings UI The original design specified that we detach/hide the MultiDevice page/ page-container in settings when no eligible host phone is found. This CL updates the UI to match the new mocks. This involved changing the settings-multidevice-page-container element so it does not hide the settings-multidevice-page and changing the settings-multidevice-page element so it matches the mocks when there are no eligible hosts. Here is a screenshot of the new case: https://screenshot.googleplex.com/vD780e3CQAD --- Note that I reverted the change of 'isHostSet' in MultiDeviceFeatureBehavior from a method to a computed Polymer property in https://chromium-review.googlesource.com/c/chromium/src/+/1191666 because this CL required that I bind to both isHostSet and pageContentData in order to be sure my bindings had the right value of isHostSet, which is in turn computed from pageContentData. I thought it would be better to switch isHostSet back to a method to avoid more a more convoluted binding dependency graph. TBR=jordynass@chromium.org (cherry picked from commit 51bd5c40dd8f4606fded035dba07872a089af71b) Bug: 870127 Change-Id: I1f23a645216aa412894f2729a91603d998acf4eb Reviewed-on: https://chromium-review.googlesource.com/1196168 Commit-Queue: Jordy Greenblatt <jordynass@chromium.org> Reviewed-by: Tommy Li <tommycli@chromium.org> Reviewed-by: Kyle Horimoto <khorimoto@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#588605} Reviewed-on: https://chromium-review.googlesource.com/1222160 Cr-Commit-Position: refs/branch-heads/3538@{#348} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811} [modify] https://crrev.com/51b4ec01cf01ccf445fc8b4adfced845d845261b/chrome/app/settings_strings.grdp [modify] https://crrev.com/51b4ec01cf01ccf445fc8b4adfced845d845261b/chrome/browser/resources/settings/multidevice_page/multidevice_feature_behavior.js [modify] https://crrev.com/51b4ec01cf01ccf445fc8b4adfced845d845261b/chrome/browser/resources/settings/multidevice_page/multidevice_page.html [modify] https://crrev.com/51b4ec01cf01ccf445fc8b4adfced845d845261b/chrome/browser/resources/settings/multidevice_page/multidevice_page.js [modify] https://crrev.com/51b4ec01cf01ccf445fc8b4adfced845d845261b/chrome/browser/resources/settings/multidevice_page/multidevice_page_container.js [modify] https://crrev.com/51b4ec01cf01ccf445fc8b4adfced845d845261b/chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc [modify] https://crrev.com/51b4ec01cf01ccf445fc8b4adfced845d845261b/chrome/test/data/webui/settings/multidevice_page_container_tests.js [modify] https://crrev.com/51b4ec01cf01ccf445fc8b4adfced845d845261b/chrome/test/data/webui/settings/multidevice_page_tests.js
,
Sep 12
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7b498cda415d24902fb1a3e10b345606212c4ad5 commit 7b498cda415d24902fb1a3e10b345606212c4ad5 Author: Kyle Horimoto <khorimoto@google.com> Date: Wed Sep 12 23:58:35 2018 [CrOS MultiDevice] Change Settings UI no server page to match new mocks The mocks now require that the Settings UI page when there is there is no response from the server has a disabled "Verify" button instead of the old active "Retry" button. Here are screenshots for this CL deployed: Waiting for verification: http://screen/E19zdKcijRN Waiting for server: http://screen/SSkhb7tzDkb TBR=jordynass@chromium.org (cherry picked from commit 39424745964259cf783eaced301de01ce1b76b2f) Bug: 870127 Change-Id: If529edc1b6756de4fa6ecc5f5c8d0346e551a926 Reviewed-on: https://chromium-review.googlesource.com/1205596 Reviewed-by: Jeremy Klein <jlklein@chromium.org> Reviewed-by: Hector Carmona <hcarmona@chromium.org> Commit-Queue: Jordy Greenblatt <jordynass@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#588973} Reviewed-on: https://chromium-review.googlesource.com/1222161 Reviewed-by: Kyle Horimoto <khorimoto@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#349} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811} [modify] https://crrev.com/7b498cda415d24902fb1a3e10b345606212c4ad5/chrome/browser/resources/settings/multidevice_page/multidevice_page.html [modify] https://crrev.com/7b498cda415d24902fb1a3e10b345606212c4ad5/chrome/browser/resources/settings/multidevice_page/multidevice_page.js [modify] https://crrev.com/7b498cda415d24902fb1a3e10b345606212c4ad5/chrome/test/data/webui/settings/multidevice_page_tests.js
,
Sep 12
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bcb8e7f3a8cddde7f5a28ca363c75f360d1e4a6d commit bcb8e7f3a8cddde7f5a28ca363c75f360d1e4a6d Author: Kyle Horimoto <khorimoto@google.com> Date: Wed Sep 12 23:59:29 2018 [CrOS MultiDevice] Reflect policy prohibition in Settings. This CL shows a policy indicator and a disabled toggle when the suite or a specific feature (on the subpage) is prohibited by policy. Note that some unrealistic cases are not handled. This includes: 1) suite is prohibited but the mode (a.k.a. host status) is something other than "no eligible hosts" 2) the user is in the subpage but the suite is prohibited by policy. Screenshots: Suite prohibited by policy (main Settings UI page): http://screen/EbYef2RFFrf Individual feature prohibited by policy (subpage): http://screen/D3iqHQCW4B3 TBR=jordynass@chromium.org (cherry picked from commit f40d7aaad6b2d37eaa691e8ec01b975cc379533c) Bug: 870127 Change-Id: Iebeed37255e4a3dffb4f1b6376acfe5a9c26cc2d Reviewed-on: https://chromium-review.googlesource.com/1208900 Reviewed-by: Hector Carmona <hcarmona@chromium.org> Reviewed-by: Jeremy Klein <jlklein@chromium.org> Commit-Queue: Jordy Greenblatt <jordynass@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#589303} Reviewed-on: https://chromium-review.googlesource.com/1222986 Reviewed-by: Kyle Horimoto <khorimoto@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#350} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811} [modify] https://crrev.com/bcb8e7f3a8cddde7f5a28ca363c75f360d1e4a6d/chrome/browser/resources/settings/multidevice_page/multidevice_feature_behavior.js [modify] https://crrev.com/bcb8e7f3a8cddde7f5a28ca363c75f360d1e4a6d/chrome/browser/resources/settings/multidevice_page/multidevice_feature_item.html [modify] https://crrev.com/bcb8e7f3a8cddde7f5a28ca363c75f360d1e4a6d/chrome/browser/resources/settings/multidevice_page/multidevice_feature_item.js [modify] https://crrev.com/bcb8e7f3a8cddde7f5a28ca363c75f360d1e4a6d/chrome/browser/resources/settings/multidevice_page/multidevice_page.html [modify] https://crrev.com/bcb8e7f3a8cddde7f5a28ca363c75f360d1e4a6d/chrome/browser/resources/settings/multidevice_page/multidevice_page.js [modify] https://crrev.com/bcb8e7f3a8cddde7f5a28ca363c75f360d1e4a6d/chrome/test/data/webui/settings/multidevice_page_tests.js
,
Sep 13
,
Sep 13
Here's a summary of the rules that were executed: - OnlyMergeApprovedChange: Rule Failed -- Revision 7b498cda415d24902fb1a3e10b345606212c4ad5 was merged to refs/branch-heads/3538 branch with no merge approval from a TPM! Please explain why this change was merged to the branch!
,
Sep 13
Here's a summary of the rules that were executed: - OnlyMergeApprovedChange: Rule Failed -- Revision bcb8e7f3a8cddde7f5a28ca363c75f360d1e4a6d was merged to refs/branch-heads/3538 branch with no merge approval from a TPM! Please explain why this change was merged to the branch!
,
Sep 13
Why were these changes merged to 70?
,
Sep 13
Sorry about that - I misread the Merge-Review-70 as Merge-Approved-70. Should these changes be reverted until approval is granted, or should these be remain merged?
,
Sep 13
Yes, please revert.
,
Sep 24
,
Sep 28
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Sep 28
|
||||||||||||||
►
Sign in to add a comment |
||||||||||||||
Comment 1 by khorimoto@chromium.org
, Aug 16