Commit 16d4186c authored by Jordy Greenblatt's avatar Jordy Greenblatt Committed by Commit Bot

SettingsSplit: Remove unnecessary deps of os-settings on pageVisibility

This is a CL to minimize os-settings's dependency on pageVisibility.
This includes removing some references to non-existent paths as in [1]
and replacing any pageVisibility path that only depends on guest mode
in OS Settings with an explicit check on whether the machine is in
guest mode from loadTimeData.

This doesn't completely eliminate the pageVisibility fork [2] bug
because the people-page still provides a pageVisibility subpath to sync-
settings, but it removes the vast majority of the dependencies on the
browser settings' pageVisibility. The people-page issue will be handled
in a later CL to avoid bloating.

LOGGED IN SCREENSHOTS:

Screenshots for OS Settings logged into a GAIA account with this change
deployed.

Page 1 http://screen/Tcbdf04X2Q4
Page 2 http://screen/9t80k4pRAGj
Page 3 http://screen/dRCw3iZcQUU

Compare to the top level 'settings-section's in UX mock [1] (modulo
reordering and renaming).

GUEST MODE SCREENSHOTS:

As per specs sent out by jessejames@, in guest mode, the visible
sections[2] are Network, Bluetooth, Device, Search engine, Date and
time, Privacy and security, Languages and input, Files, Printing, and
Accessibility. Compare this with screenshots of guest mode with this
change deployed:

Page 1 http://screen/AriC9twWUi3
Page 2 http://screen/qnrUthAru3G

[1] https://chromium-review.googlesource.com/c/chromium/src/+/1716338
[2] http://crbug.com/974906

[1] https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZQLWgsMW9gCl/files/MCEJu8Y2hyDScYKiC7u07no3e21TaxzYd_w
[2] There is a possibility that 'People' may be shown in guest mode
    depending on the outcome of a UX design discussion on the correct
    place for Kerberos settings, but this CL is working under the
    assumption that it is not shown. This can easily be changed after.

Bug: 987349, 974906
Change-Id: I7f384437f9008742926eb7cf9d4d623bd8415591
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1724828Reviewed-by: default avatarMay Lippert <maybelle@chromium.org>
Reviewed-by: default avatarHector Carmona <hcarmona@chromium.org>
Commit-Queue: Jordy Greenblatt <jordynass@chromium.org>
Cr-Commit-Position: refs/heads/master@{#682854}
parent 1cc0aa84
......@@ -20,7 +20,6 @@ js_library("smb_shares_page") {
js_library("os_files_page") {
deps = [
"../..:page_visibility",
"../..:route",
]
}
......@@ -16,16 +16,14 @@
<div route-path="default">
<settings-toggle-button
pref="{{prefs.gdata.disabled}}"
label="$i18n{disconnectGoogleDriveAccount}"
hidden="[[!pageVisibility.googleDrive]]">
label="$i18n{disconnectGoogleDriveAccount}">
</settings-toggle-button>
<cr-link-row id="smbShares" class="hr" on-click="onTapSmbShares_"
hidden="[[!pageVisibility.smbShares]]"
label="$i18n{smbSharesTitle}"></cr-link-row>
label="$i18n{smbSharesTitle}">
</cr-link-row>
</div>
<template is="dom-if" route-path="/smbShares">
<settings-subpage
hidden="[[!pageVisibility.smbShares]]"
associated-control="[[$$('#smbShares')]]"
page-title="$i18n{smbSharesTitle}">
<settings-smb-shares-page prefs="[[prefs]]">
......
......@@ -19,12 +19,6 @@ Polymer({
notify: true,
},
/**
* Dictionary defining page visibility.
* @type {!DownloadsPageVisibility}
*/
pageVisibility: Object,
/** @private {!Map<string, string>} */
focusConfig_: {
type: Object,
......
......@@ -47,7 +47,7 @@ Polymer({
/**
* Dictionary defining page visibility.
* @type {!PeoplePageVisibility}
* @type {!PageVisibility}
*/
pageVisibility: Object,
......
......@@ -21,7 +21,6 @@ js_library("os_powerwash_dialog") {
js_library("os_reset_page") {
deps = [
"../..:page_visibility",
"//ui/webui/resources/js:assert",
"//ui/webui/resources/js:cr",
"//ui/webui/resources/js/cr/ui:focus_without_ink",
......
......@@ -12,7 +12,6 @@ js_type_check("closure_compile") {
js_library("os_settings_menu") {
deps = [
"../..:page_visibility",
"../..:route",
"//third_party/polymer/v1_0/components-chromium/iron-collapse:iron-collapse-extracted",
"//third_party/polymer/v1_0/components-chromium/paper-ripple:paper-ripple-extracted",
......
......@@ -148,21 +148,21 @@
</div>
</a>
<a id="multidevice" href="/multidevice"
hidden="[[!pageVisibility.multidevice]]">
hidden="[[isGuestMode_]]">
<div class="item">
<iron-icon icon="settings:multidevice-better-together-suite">
</iron-icon>
$i18n{multidevicePageTitle}
</div>
</a>
<a id="people" href="/people" hidden="[[!pageVisibility.people]]">
<a id="people" href="/people" hidden="[[isGuestMode_]]">
<div class="item">
<iron-icon icon="cr:person"></iron-icon>
$i18n{peoplePageTitle}
</div>
</a>
<a id="personalization" href="/personalization"
hidden="[[!pageVisibility.personalization]]">
hidden="[[isGuestMode_]]">
<div class="item">
<iron-icon icon="settings:palette"></iron-icon>
$i18n{personalizationPageTitle}
......@@ -205,14 +205,12 @@
</div>
</a>
<cr-button id="advancedButton" aria-active-attribute="aria-expanded"
on-click="onAdvancedButtonToggle_"
hidden="[[!pageVisibility.advancedSettings]]">
on-click="onAdvancedButtonToggle_">
<span>$i18n{advancedPageTitle}</span>
<iron-icon icon="[[arrowState_(advancedOpened)]]">
</iron-icon>
</cr-button>
<iron-collapse id="advancedSubmenu" opened="[[advancedOpened]]"
hidden="[[!pageVisibility.advancedSettings]]">
<iron-collapse id="advancedSubmenu" opened="[[advancedOpened]]">
<iron-selector id="subMenu" selectable="a" attr-for-selected="href"
role="navigation" on-click="onLinkClick_">
<a href="/dateTime">
......@@ -233,7 +231,7 @@
$i18n{osLanguagesPageTitle}
</div>
</a>
<a href="/files">
<a href="/files" hidden="[[isGuestMode_]]">
<div class="item">
<iron-icon icon="cr:file-download"></iron-icon>
$i18n{filesPageTitle}
......@@ -251,7 +249,7 @@
$i18n{a11yPageTitle}
</div>
</a>
<a id="reset" href="/reset" hidden="[[!pageVisibility.reset]]">
<a id="reset" href="/reset" hidden="[[isGuestMode_]]">
<div class="item">
<iron-icon icon="settings:restore"></iron-icon>
$i18n{resetPageTitle}
......
......@@ -19,10 +19,14 @@ Polymer({
},
/**
* Dictionary defining page visibility.
* @type {!PageVisibility}
* Whether the user is in guest mode.
* @private{boolean}
*/
pageVisibility: Object,
isGuestMode_: {
type: Boolean,
value: loadTimeData.getBoolean('isGuest'),
readOnly: true,
},
},
/** @param {!settings.Route} newRoute */
......
......@@ -35,6 +35,16 @@ Polymer({
/** @type {!AndroidAppsInfo|undefined} */
androidAppsInfo: Object,
/**
* Whether the user is in guest mode.
* @private{boolean}
*/
isGuestMode_: {
type: Boolean,
value: loadTimeData.getBoolean('isGuest'),
readOnly: true,
},
/**
* Dictionary defining page visibility.
* @type {!PageVisibility}
......@@ -159,12 +169,10 @@ Polymer({
settings.getSearchManager().search(query, assert(this.$$('#basicPage'))),
];
if (this.pageVisibility.advancedSettings !== false) {
whenSearchDone.push(
this.$$('#advancedPageTemplate').get().then(function(advancedPage) {
return settings.getSearchManager().search(query, advancedPage);
}));
}
whenSearchDone.push(
this.$$('#advancedPageTemplate').get().then(function(advancedPage) {
return settings.getSearchManager().search(query, advancedPage);
}));
return Promise.all(whenSearchDone).then(function(requests) {
// Combine the SearchRequests results to a single SearchResult object.
......@@ -199,18 +207,6 @@ Polymer({
this.androidAppsInfo = info;
},
/**
* Returns true in case Android apps settings needs to be created. It is not
* created in case ARC++ is not allowed for the current profile.
* @return {boolean}
* @private
*/
shouldCreateAndroidAppsSection_: function() {
const visibility = /** @type {boolean|undefined} */ (
this.get('pageVisibility.androidApps'));
return this.showAndroidApps && this.showPage_(visibility);
},
/**
* Returns true in case Android apps settings should be shown. It is not
* shown in case we don't have the Play Store app and settings app is not
......
......@@ -123,8 +123,7 @@
align="$i18n{textdirection}">
<div class="drawer-content">
<template is="dom-if" id="drawerTemplate">
<os-settings-menu page-visibility="[[pageVisibility_]]"
show-apps="[[showApps_]]"
<os-settings-menu show-apps="[[showApps_]]"
show-android-apps="[[showAndroidApps_]]"
show-crostini="[[showCrostini_]]"
show-plugin-vm="[[showPluginVm_]]"
......
......@@ -19,7 +19,7 @@
on-click="navigateToChangePicture_">
</cr-link-row>
<cr-link-row class="hr" id="wallpaperButton"
hidden="[[!pageVisibility.setWallpaper]]"
hidden="[[!showWallpaperRow_]]"
on-click="openWallpaperManager_" label="$i18n{setWallpaper}"
sub-label="$i18n{openWallpaperApp}"
disabled="[[isWallpaperPolicyControlled_]]" external>
......
......@@ -13,11 +13,8 @@ Polymer({
is: 'settings-personalization-page',
properties: {
/**
* Dictionary defining page visibility.
* @type {!PersonalizationPageVisibility}
*/
pageVisibility: Object,
/** @private */
showWallpaperRow_: {type: Boolean, value: true},
/** @private */
isWallpaperPolicyControlled_: {type: Boolean, value: true},
......@@ -47,8 +44,7 @@ Polymer({
ready: function() {
this.browserProxy_.isWallpaperSettingVisible().then(
isWallpaperSettingVisible => {
assert(this.pageVisibility);
this.pageVisibility.setWallpaper = isWallpaperSettingVisible;
this.showWallpaperRow_ = isWallpaperSettingVisible;
});
this.browserProxy_.isWallpaperPolicyControlled().then(
isPolicyControlled => {
......
......@@ -22,7 +22,6 @@
* multidevice: (boolean|undefined),
* onStartup: (boolean|undefined),
* people: (boolean|undefined|PeoplePageVisibility),
* personalization: (boolean|undefined|PersonalizationPageVisibility),
* printing: (boolean|undefined),
* privacy: (boolean|undefined|PrivacyPageVisibility),
* reset:(boolean|undefined|ResetPageVisibility),
......@@ -67,13 +66,6 @@ let DownloadsPageVisibility;
*/
let PeoplePageVisibility;
/**
* @typedef {{
* setWallpaper: boolean,
* }}
*/
let PersonalizationPageVisibility;
/**
* @typedef {{
* contentProtectionAttestation: boolean,
......@@ -132,7 +124,6 @@ cr.define('settings', function() {
multidevice: false,
autofill: false,
people: false,
personalization: false,
onStartup: false,
reset: false,
appearance: {
......@@ -181,10 +172,6 @@ cr.define('settings', function() {
googleAccounts: showOSSettings,
manageUsers: showOSSettings,
},
personalization: {
// Personalization is in OS settings only, so section always shows.
setWallpaper: true,
},
onStartup: true,
reset: {
powerwash: showOSSettings,
......
......@@ -118,29 +118,4 @@ suite('OSSettingsMenuReset', function() {
// BASIC has no sub page selected.
assertFalse(!!selector.selected);
});
test('pageVisibility', function() {
function assertPageVisibility(shown) {
assertEquals(shown, !settingsMenu.$$('#advancedButton').hidden);
assertEquals(shown, !settingsMenu.$$('#advancedSubmenu').hidden);
assertEquals(shown, !settingsMenu.$$('#multidevice').hidden);
assertEquals(shown, !settingsMenu.$$('#people').hidden);
assertEquals(shown, !settingsMenu.$$('#reset').hidden);
}
// Menu items are shown by default.
assertPageVisibility(true);
// Set the visibility of the pages under test to "false".
settingsMenu.pageVisibility = Object.assign(settings.pageVisibility, {
advancedSettings: false,
multidevice: false,
people: false,
reset: false
});
Polymer.dom.flush();
// Now the menu items should be hidden.
assertPageVisibility(false);
});
});
......@@ -101,9 +101,8 @@ suite('PersonalizationHandler', function() {
await personalizationBrowserProxy.whenCalled('openWallpaperManager');
});
test('wallpaperSettingVisible', async () => {
personalizationPage.set('pageVisibility.setWallpaper', false);
await personalizationBrowserProxy.whenCalled('isWallpaperSettingVisible');
test('wallpaperSettingVisible', function() {
personalizationPage.showWallpaperRow_ = false;
Polymer.dom.flush();
assertTrue(personalizationPage.$$('#wallpaperButton').hidden);
});
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment