New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 887784 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Oct 3
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug


Participants' hotlists:
Better-Together-Launch-Blockers


Sign in to add a comment

Get Rid of settings-multidevice-page-container element

Project Member Reported by jordynass@chromium.org, Sep 21

Issue description

Background:

Initially we introduced this element to allow the MultiDevice settings section to dynamically hide itself when there were no eligible hosts. Since then, the specs have changed so that the section no longer needs to hide itself in that case. So now the only reasons to hide the section are the cases handled by the general settings framework (flag off, guest session) or when the Chromebook is not enrolled in CryptAuth. Because we will be enrolling Chromebooks automatically when their flag is on, the only case that's not covered by the general settings framework is when a user opens the setting page between the time they go through the Setup Flow and when their Chromebook gets its response from CryptAuth.

The CryptAuth response is expected to take on the order of one second or less so the remaining problem case is when the user is offline. We could add a string to clarify this case or we could simply treat it the same as the 'no eligible devices' case since it's likely to be an infrequent case anyway.


Immediate Impetus:

Settings has recently pushed back against this design because no other settings page has that pattern (dynamically appearing/hiding) and it is at odds with some of their existing patterns, e.g. they use an immutable registry of subpage routes.

Also, a few bugs have popped up, e.g. 883893, 876905, that are likely to go away if we don't rely on the additional layer of the settings-multidevice-page-container handling the page's visibility dynamically.


Plan:

Get rid of the intermediate settings-multidevice-page-container layer altogether.
 
Owner: jordynass@chromium.org
Status: Started (was: Untriaged)
Labels: -Pri-2 Pri-1
Cc: jordynass@chromium.org jessejames@chromium.org lesliewatkins@chromium.org jhawkins@chromium.org
 Issue 876905  has been merged into this issue.
Cc: nohle@chromium.org
 Issue 883893  has been merged into this issue.
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 3

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/562605f1860d0e0c997343ce04bde1511ef0efa4

commit 562605f1860d0e0c997343ce04bde1511ef0efa4
Author: Jordy Greenblatt <jordynass@chromium.org>
Date: Wed Oct 03 03:15:34 2018

[CrOS MultiDevice] Remove page container infrastructure in Settings UI

Settings has strongly urged us to move away from dynamically attaching
and detaching the MultiDevice page from Settings UI. After this push,
I looked into the costs of that and with khorimoto@ decided they were
small enough to warrant the refactor given the rarity of this
functionality in practice. Note that MultiDevice settings flag already
controls whether to allow the page at a higher level so that does not
rely on the dynamic attachment ability provided by the
multidevice-page-container.

An additional incentive is to prevent a few display bugs (see the bugs
merged into 887784 for details) that are very likely to be irrelevant
once this infrastructure is removed.

Screenshots from the cases that cause the MultiDevice page to detach in
the HEAD version:
http://screen/BW2npVh2Ocw
http://screen/pKLechGvcCp

Bug:  887784 
Change-Id: Iaefb8bff4ae300c483645e2558b8a95083e2bc47
Reviewed-on: https://chromium-review.googlesource.com/c/1258211
Commit-Queue: Jordy Greenblatt <jordynass@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596107}
[modify] https://crrev.com/562605f1860d0e0c997343ce04bde1511ef0efa4/chrome/browser/resources/settings/basic_page/basic_page.html
[modify] https://crrev.com/562605f1860d0e0c997343ce04bde1511ef0efa4/chrome/browser/resources/settings/basic_page/basic_page.js
[modify] https://crrev.com/562605f1860d0e0c997343ce04bde1511ef0efa4/chrome/browser/resources/settings/multidevice_page/BUILD.gn
[modify] https://crrev.com/562605f1860d0e0c997343ce04bde1511ef0efa4/chrome/browser/resources/settings/multidevice_page/multidevice_page.html
[modify] https://crrev.com/562605f1860d0e0c997343ce04bde1511ef0efa4/chrome/browser/resources/settings/multidevice_page/multidevice_page.js
[delete] https://crrev.com/762df375cb5cde323ba6681a38775778939dbddd/chrome/browser/resources/settings/multidevice_page/multidevice_page_container.html
[delete] https://crrev.com/762df375cb5cde323ba6681a38775778939dbddd/chrome/browser/resources/settings/multidevice_page/multidevice_page_container.js
[modify] https://crrev.com/562605f1860d0e0c997343ce04bde1511ef0efa4/chrome/browser/resources/settings/settings_resources.grd
[modify] https://crrev.com/562605f1860d0e0c997343ce04bde1511ef0efa4/chrome/test/data/webui/settings/cr_settings_browsertest.js
[delete] https://crrev.com/762df375cb5cde323ba6681a38775778939dbddd/chrome/test/data/webui/settings/multidevice_page_container_tests.js
[modify] https://crrev.com/562605f1860d0e0c997343ce04bde1511ef0efa4/chrome/test/data/webui/settings/multidevice_page_tests.js

Status: Fixed (was: Started)

Sign in to add a comment