From 08e40f2bfd2ad9edde6304a9f70a6ac6e5f0b26a Mon Sep 17 00:00:00 2001 From: Tibor Goldschwendt Date: Tue, 2 Mar 2021 21:20:15 +0000 Subject: [PATCH] [ntp][modules] Allow disabling individual modules in customize dialog This CL adds various controls to the customize dialog that lets the user * Disable (hide) all modules (screenshot/8RYAASu4r9mCNPm), or * Customize modules, which means the user can disable each module individually (screenshot/5xLiE8u5M8A4YXt). The disabled state was introduced in crrev/c/2705963. This CL merges the disabled state with visibility (previous model where either all or no modules were shown) to make the model more manageable. Hereby, visible is mapped to the user can customize and hidden is mapped to all modules disabled. However, if visibility is managed by a policy, then visible is mapped to all modules enabled (screenshot/Bq2mYcVEbXiE5xX) since this is more in line with what the policy promises. Note that native code is not aware of the full set of modules and only keeps a list of disabled modules. Therefore, disabling all modules is accomplished by setting a flag rather then setting each module as disabled. Due to the merge of disabled state and visibility this CL has the nice side effect that we no longer instantiate hidden modules. (cherry picked from commit 21b31fdcd845f65fea49ec9c7c420a66688b6c8a) Change-Id: I9ffbf5c8ee4f8e6c7ca90fe267908418c1bc9f4d Fixed: 1177527 Fixed: 1148098 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2713395 Commit-Queue: Tibor Goldschwendt Reviewed-by: Esmael Elmoslimany Reviewed-by: Robert Kaplow Reviewed-by: Alex Gough Cr-Original-Commit-Position: refs/heads/master@{#858216} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2727083 Commit-Queue: Rubber Stamper Auto-Submit: Tibor Goldschwendt Bot-Commit: Rubber Stamper Cr-Commit-Position: refs/branch-heads/4430@{#60} Cr-Branched-From: e5ce7dc4f7518237b3d9bb93cccca35d25216cbe-refs/heads/master@{#857950} --- chrome/app/generated_resources.grd | 7 +- ...P_CUSTOMIZE_CUSTOMIZE_CARDS_LABEL.png.sha1 | 1 + ...TP_CUSTOMIZE_HIDE_ALL_CARDS_LABEL.png.sha1 | 1 + ..._NTP_CUSTOMIZE_SHOW_MODULES_LABEL.png.sha1 | 1 - .../browser/resources/new_tab_page/BUILD.gn | 16 ++ .../browser/resources/new_tab_page/app.html | 8 +- chrome/browser/resources/new_tab_page/app.js | 81 +++++---- .../new_tab_page/customize_modules.html | 76 +++++--- .../new_tab_page/customize_modules.js | 92 ++++++++-- .../new_tab_page/modules/module_registry.js | 19 +- .../ui/webui/new_tab_page/new_tab_page.mojom | 11 +- .../new_tab_page/new_tab_page_handler.cc | 27 +-- .../webui/new_tab_page/new_tab_page_handler.h | 3 +- .../new_tab_page_handler_unittest.cc | 2 +- .../ui/webui/new_tab_page/new_tab_page_ui.cc | 3 +- .../test/data/webui/new_tab_page/app_test.js | 31 ++-- .../new_tab_page/customize_modules_test.js | 169 +++++++++++++++--- .../new_tab_page/metrics_test_support.js | 8 + .../modules/module_registry_test.js | 4 +- chrome/test/data/webui/test_browser_proxy.js | 17 +- .../histogram_suffixes_list.xml | 1 + .../new_tab_page/histograms.xml | 15 ++ 22 files changed, 447 insertions(+), 146 deletions(-) create mode 100644 chrome/app/generated_resources_grd/IDS_NTP_CUSTOMIZE_CUSTOMIZE_CARDS_LABEL.png.sha1 create mode 100644 chrome/app/generated_resources_grd/IDS_NTP_CUSTOMIZE_HIDE_ALL_CARDS_LABEL.png.sha1 delete mode 100644 chrome/app/generated_resources_grd/IDS_NTP_CUSTOMIZE_SHOW_MODULES_LABEL.png.sha1 diff --git a/chrome/app/generated_resources.grd b/chrome/app/generated_resources.grd index dabe252bb0f93..2408c0f76f85a 100644 --- a/chrome/app/generated_resources.grd +++ b/chrome/app/generated_resources.grd @@ -5563,8 +5563,11 @@ Keep your key file in a safe place. You will need it to create new versions of y Shortcuts are curated by you - - Show cards + + Hide all cards + + + Customize cards Current theme you have installed diff --git a/chrome/app/generated_resources_grd/IDS_NTP_CUSTOMIZE_CUSTOMIZE_CARDS_LABEL.png.sha1 b/chrome/app/generated_resources_grd/IDS_NTP_CUSTOMIZE_CUSTOMIZE_CARDS_LABEL.png.sha1 new file mode 100644 index 0000000000000..c9dd72c7479a7 --- /dev/null +++ b/chrome/app/generated_resources_grd/IDS_NTP_CUSTOMIZE_CUSTOMIZE_CARDS_LABEL.png.sha1 @@ -0,0 +1 @@ +e4e2afd525563f13c23de2ca491d2b8620fa7224 \ No newline at end of file diff --git a/chrome/app/generated_resources_grd/IDS_NTP_CUSTOMIZE_HIDE_ALL_CARDS_LABEL.png.sha1 b/chrome/app/generated_resources_grd/IDS_NTP_CUSTOMIZE_HIDE_ALL_CARDS_LABEL.png.sha1 new file mode 100644 index 0000000000000..c9dd72c7479a7 --- /dev/null +++ b/chrome/app/generated_resources_grd/IDS_NTP_CUSTOMIZE_HIDE_ALL_CARDS_LABEL.png.sha1 @@ -0,0 +1 @@ +e4e2afd525563f13c23de2ca491d2b8620fa7224 \ No newline at end of file diff --git a/chrome/app/generated_resources_grd/IDS_NTP_CUSTOMIZE_SHOW_MODULES_LABEL.png.sha1 b/chrome/app/generated_resources_grd/IDS_NTP_CUSTOMIZE_SHOW_MODULES_LABEL.png.sha1 deleted file mode 100644 index 0898f55e6e3bd..0000000000000 --- a/chrome/app/generated_resources_grd/IDS_NTP_CUSTOMIZE_SHOW_MODULES_LABEL.png.sha1 +++ /dev/null @@ -1 +0,0 @@ -59d3f9b4ee27a36182130da99366271599fba2a6 \ No newline at end of file diff --git a/chrome/browser/resources/new_tab_page/BUILD.gn b/chrome/browser/resources/new_tab_page/BUILD.gn index f113a6f865e98..a7de034c3732a 100644 --- a/chrome/browser/resources/new_tab_page/BUILD.gn +++ b/chrome/browser/resources/new_tab_page/BUILD.gn @@ -125,6 +125,7 @@ js_library("customize_dialog") { deps = [ ":customize_backgrounds", ":customize_dialog_types", + ":customize_modules", ":customize_shortcuts", ":utils", "//third_party/polymer/v3_0/components-chromium/polymer:polymer_bundled", @@ -160,6 +161,21 @@ js_library("customize_shortcuts") { ] } +js_library("customize_modules") { + deps = [ + ":browser_proxy", + "modules:module_registry", + "//third_party/polymer/v3_0/components-chromium/polymer:polymer_bundled", + "//ui/webui/resources/cr_elements/cr_button:cr_button.m", + "//ui/webui/resources/cr_elements/cr_radio_button:cr_radio_button.m", + "//ui/webui/resources/cr_elements/cr_radio_group:cr_radio_group.m", + "//ui/webui/resources/cr_elements/cr_toggle:cr_toggle.m", + "//ui/webui/resources/cr_elements/policy:cr_policy_indicator.m", + "//ui/webui/resources/js:assert.m", + "//ui/webui/resources/js:load_time_data.m", + ] +} + js_library("voice_search_overlay") { deps = [ "//third_party/polymer/v3_0/components-chromium/polymer:polymer_bundled", diff --git a/chrome/browser/resources/new_tab_page/app.html b/chrome/browser/resources/new_tab_page/app.html index 2a536d041ac03..d1ccf195fb5d4 100644 --- a/chrome/browser/resources/new_tab_page/app.html +++ b/chrome/browser/resources/new_tab_page/app.html @@ -102,8 +102,7 @@ } :host(:not([promo-and-modules-loaded_])) ntp-middle-slot-promo, - :host(:not([promo-and-modules-loaded_])) ntp-module-wrapper, - :host(:not([modules-visible_])) ntp-module-wrapper { + :host(:not([promo-and-modules-loaded_])) ntp-module-wrapper { display: none; } @@ -287,7 +286,10 @@ on-dom-change="onModulesLoaded_"> + on-disable-module="onDisableModule_" + hidden="[[moduleDisabled_(item.id, + dismissedModules_.*, + disabledModules_)]]"> loadTimeData.getBoolean('modulesEnabled'), - reflectToAttribute: true, - }, - - /** @private */ - modulesVisible_: { - type: Boolean, - reflectToAttribute: true, - }, + modulesVisibilityDetermined_: Boolean, /** @private */ middleSlotPromoLoaded_: Boolean, @@ -235,11 +225,12 @@ class AppElement extends PolymerElement { }, /** @private */ - modulesLoadedAndVisible_: { + modulesLoadedAndVisibilityDetermined_: { type: Boolean, - computed: `computeModulesLoadedAndVisible_(promoAndModulesLoaded_, - modulesVisible_)`, - observer: 'onModulesLoadedAndVisibleChange_', + computed: `computeModulesLoadedAndVisibilityDetermined_( + promoAndModulesLoaded_, + modulesVisibilityDetermined_)`, + observer: 'onModulesLoadedAndVisibilityDeterminedChange_', }, /** @@ -252,9 +243,21 @@ class AppElement extends PolymerElement { /** @private {!Array} */ moduleDescriptors_: Object, + /** @private {!Array} */ + dismissedModules_: { + type: Array, + value: () => [], + }, + + /** @private {!{all: boolean, ids: !Array}} */ + disabledModules_: { + type: Object, + value: () => ({all: true, ids: []}), + }, + /** * Data about the most recently removed module. - * @type {?{element: !Element, message: string, undo: function()}} + * @type {?{message: string, undo: function()}} * @private */ removedModuleData_: { @@ -276,7 +279,7 @@ class AppElement extends PolymerElement { /** @private {?number} */ this.setThemeListenerId_ = null; /** @private {?number} */ - this.setModulesVisibleListenerId_ = null; + this.setDisabledModulesListenerId_ = null; /** @private {!EventTracker} */ this.eventTracker_ = new EventTracker(); this.loadOneGoogleBar_(); @@ -301,11 +304,12 @@ class AppElement extends PolymerElement { performance.measure('theme-set'); this.theme_ = theme; }); - this.setModulesVisibleListenerId_ = - this.callbackRouter_.setModulesVisible.addListener(visible => { - this.modulesVisible_ = visible; + this.setDisabledModulesListenerId_ = + this.callbackRouter_.setDisabledModules.addListener((all, ids) => { + this.disabledModules_ = {all, ids}; + this.modulesVisibilityDetermined_ = true; }); - this.pageHandler_.updateModulesVisible(); + this.pageHandler_.updateDisabledModules(); this.eventTracker_.add(window, 'message', (event) => { /** @type {!Object} */ const data = event.data; @@ -344,6 +348,8 @@ class AppElement extends PolymerElement { disconnectedCallback() { super.disconnectedCallback(); this.callbackRouter_.removeListener(assert(this.setThemeListenerId_)); + this.callbackRouter_.removeListener( + assert(this.setDisabledModulesListenerId_)); this.eventTracker_.removeAll(); } @@ -531,8 +537,8 @@ class AppElement extends PolymerElement { * @return {boolean} * @private */ - computeModulesLoadedAndVisible_() { - return this.promoAndModulesLoaded_ && this.modulesVisible_; + computeModulesLoadedAndVisibilityDetermined_() { + return this.promoAndModulesLoaded_ && this.modulesVisibilityDetermined_; } /** @private */ @@ -628,9 +634,15 @@ class AppElement extends PolymerElement { } /** @private */ - onModulesLoadedAndVisibleChange_() { - if (this.modulesLoadedAndVisible_) { + onModulesLoadedAndVisibilityDeterminedChange_() { + if (this.modulesLoadedAndVisibilityDetermined_) { this.pageHandler_.onModulesRendered(BrowserProxy.getInstance().now()); + this.moduleDescriptors_.forEach(({id}) => { + chrome.metricsPrivate.recordBoolean( + `NewTabPage.Modules.EnabledOnNTPLoad.${id}`, + !this.disabledModules_.all && + !this.disabledModules_.ids.includes(id)); + }); } } @@ -871,14 +883,16 @@ class AppElement extends PolymerElement { const id = $$(this, '#modules').itemForElement(e.target).id; const restoreCallback = e.detail.restoreCallback; this.removedModuleData_ = { - element: /** @type {!Element} */ (e.target), message: e.detail.message, undo: () => { + this.splice('dismissedModules_', this.dismissedModules_.indexOf(id), 1); restoreCallback(); this.pageHandler_.onRestoreModule(id); }, }; - this.removedModuleData_.element.hidden = true; + if (!this.dismissedModules_.includes(id)) { + this.push('dismissedModules_', id); + } // Notify the user. $$(this, '#removeModuleToast').show(); @@ -894,7 +908,6 @@ class AppElement extends PolymerElement { const descriptor = /** @type {!ModuleDescriptor} */ ( $$(this, '#modules').itemForElement(e.target)); this.removedModuleData_ = { - element: /** @type {!Element} */ (e.target), message: loadTimeData.getStringF('disableModuleToastMessage', descriptor.name), undo: () => { @@ -906,7 +919,6 @@ class AppElement extends PolymerElement { }, }; - this.removedModuleData_.element.hidden = true; this.pageHandler_.setModuleDisabled(descriptor.id, true); $$(this, '#removeModuleToast').show(); chrome.metricsPrivate.recordSparseHashable( @@ -915,13 +927,22 @@ class AppElement extends PolymerElement { 'NewTabPage.Modules.Disabled.ModuleRequest', descriptor.id); } + /** + * @param {string} id + * @return {boolean} + * @private + */ + moduleDisabled_(id) { + return this.disabledModules_.all || this.dismissedModules_.includes(id) || + this.disabledModules_.ids.includes(id); + } + /** * @private */ onUndoRemoveModuleButtonClick_() { // Restore the module. this.removedModuleData_.undo(); - this.removedModuleData_.element.hidden = false; // Notify the user. $$(this, '#removeModuleToast').hide(); diff --git a/chrome/browser/resources/new_tab_page/customize_modules.html b/chrome/browser/resources/new_tab_page/customize_modules.html index 6c5e293af4e7f..3203247dcc8d9 100644 --- a/chrome/browser/resources/new_tab_page/customize_modules.html +++ b/chrome/browser/resources/new_tab_page/customize_modules.html @@ -5,31 +5,51 @@ #show { align-items: center; + display: flex; + margin-bottom: 24px; + margin-inline-start: 14px; + margin-top: 14px; + } + + cr-radio-button { + height: 20px; + padding: 0; + } + + cr-radio-button + cr-radio-button { + margin-top: 31px; + } + + #show cr-policy-indicator { + --cr-icon-size: 48px; + margin-inline-start: 48px; + } + + #toggles { border: 1px solid var(--ntp-border-color); border-radius: 4px; box-sizing: border-box; - display: flex; - height: 64px; + margin-inline-end: 51px; + margin-inline-start: 50px; max-width: 544px; - width: 100%; } - :host([selected_]) #show { - background-color: var(--ntp-selected-background-color); - border-color: var(--ntp-selected-border-color); - color: var(--ntp-selected-border-color); + .toggle-row { + align-items: center; + display: flex; + height: 52px; } - #showTitleContainer { - flex-grow: 1; - margin-inline-start: 24px; + .toggle-row + .toggle-row { + border-top: 1px solid var(--ntp-border-color); } - #showTitle { - font-weight: bold; + .toggle-name { + flex-grow: 1; + margin-inline-start: 24px; } - cr-policy-indicator { + .toggle-row cr-policy-indicator { margin-inline-end: 24px; } @@ -37,15 +57,31 @@ margin-inline-end: 20px; } -
-
-
$i18n{showModules}
-
+ + + $i18n{hideAllCards} + + + $i18n{customizeCards} + + - - +
+
+
diff --git a/chrome/browser/resources/new_tab_page/customize_modules.js b/chrome/browser/resources/new_tab_page/customize_modules.js index 9f38d64f45380..e77caf9e90df5 100644 --- a/chrome/browser/resources/new_tab_page/customize_modules.js +++ b/chrome/browser/resources/new_tab_page/customize_modules.js @@ -2,10 +2,11 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -import './mini_page.js'; import 'chrome://resources/cr_elements/cr_icons_css.m.js'; import 'chrome://resources/cr_elements/cr_toggle/cr_toggle.m.js'; import 'chrome://resources/cr_elements/cr_button/cr_button.m.js'; +import 'chrome://resources/cr_elements/cr_radio_group/cr_radio_group.m.js'; +import 'chrome://resources/cr_elements/cr_radio_button/cr_radio_button.m.js'; import 'chrome://resources/cr_elements/policy/cr_policy_indicator.m.js'; import {assert} from 'chrome://resources/js/assert.m.js'; @@ -13,6 +14,7 @@ import {loadTimeData} from 'chrome://resources/js/load_time_data.m.js'; import {html, PolymerElement} from 'chrome://resources/polymer/v3_0/polymer/polymer_bundled.min.js'; import {BrowserProxy} from './browser_proxy.js'; +import {ModuleRegistry} from './modules/module_registry.js'; /** Element that lets the user configure modules settings. */ class CustomizeModulesElement extends PolymerElement { @@ -26,10 +28,13 @@ class CustomizeModulesElement extends PolymerElement { static get properties() { return { - /** @private */ + /** + * If true, modules are customizable. If false, all modules are hidden. + * @private + */ show_: { type: Boolean, - reflectToAttribute: true, + observer: 'onShowChange_', }, /** @private */ @@ -38,11 +43,21 @@ class CustomizeModulesElement extends PolymerElement { value: () => loadTimeData.getBoolean('modulesVisibleManagedByPolicy'), }, - /** @private */ - selected_: { - type: Boolean, - reflectToAttribute: true, - computed: 'computeSelected_(show_, showManagedByPolicy_)', + /** + * @private { + * !Array<{ + * name: string, + * id: string, + * checked: boolean, + * initiallyChecked: boolean, + * disabled: boolean, + * }> + * } + */ + modules_: { + type: Array, + value: () => ModuleRegistry.getInstance().getDescriptors().map( + d => ({name: d.name, id: d.id, checked: true, hidden: false})), }, }; } @@ -50,25 +65,31 @@ class CustomizeModulesElement extends PolymerElement { constructor() { super(); /** @private {?number} */ - this.setModulesVisibleListenerId_ = null; + this.setDisabledModulesListenerId_ = null; } /** @override */ connectedCallback() { super.connectedCallback(); - this.setModulesVisibleListenerId_ = - BrowserProxy.getInstance().callbackRouter.setModulesVisible.addListener( - visible => { - this.show_ = visible; + this.setDisabledModulesListenerId_ = + BrowserProxy.getInstance() + .callbackRouter.setDisabledModules.addListener((all, ids) => { + this.show_ = !all; + this.modules_.forEach(({id}, i) => { + const checked = !all && !ids.includes(id); + this.set(`modules_.${i}.checked`, checked); + this.set(`modules_.${i}.initiallyChecked`, checked); + this.set(`modules_.${i}.disabled`, ids.includes(id)); + }); }); - BrowserProxy.getInstance().handler.updateModulesVisible(); + BrowserProxy.getInstance().handler.updateDisabledModules(); } /** @override */ disconnectedCallback() { super.disconnectedCallback(); BrowserProxy.getInstance().callbackRouter.removeListener( - assert(this.setModulesVisibleListenerId_)); + assert(this.setDisabledModulesListenerId_)); } /** @override */ @@ -85,15 +106,50 @@ class CustomizeModulesElement extends PolymerElement { } apply() { - BrowserProxy.getInstance().handler.setModulesVisible(this.show_); + const handler = BrowserProxy.getInstance().handler; + handler.setModulesVisible(this.show_); + this.modules_ + .filter(({checked, initiallyChecked}) => checked !== initiallyChecked) + .forEach(({id, checked}) => { + // Don't set disabled status of a module if we are in hide all mode to + // preserve the status for the next time we go into customize mode. + if (this.show_) { + handler.setModuleDisabled(id, !checked); + } + const base = `NewTabPage.Modules.${checked ? 'Enabled' : 'Disabled'}`; + chrome.metricsPrivate.recordSparseHashable(base, id); + chrome.metricsPrivate.recordSparseHashable(`${base}.Customize`, id); + }); + } + + /** + * @param {!CustomEvent<{value: string}>} e + * @private + */ + onShowRadioSelectionChanged_(e) { + this.show_ = e.detail.value === 'customize'; + } + + /** @private */ + onShowChange_() { + this.modules_.forEach( + (m, i) => this.set(`modules_.${i}.checked`, this.show_ && !m.disabled)); + } + + /** + * @return {string} + * @private + */ + radioSelection_() { + return this.show_ ? 'customize' : 'hide'; } /** * @return {boolean} * @private */ - computeSelected_() { - return this.show_ && !this.showManagedByPolicy_; + moduleToggleDisabled_() { + return this.showManagedByPolicy_ || !this.show_; } } diff --git a/chrome/browser/resources/new_tab_page/modules/module_registry.js b/chrome/browser/resources/new_tab_page/modules/module_registry.js index ac7d1647a4835..3f41bdd28a4fe 100644 --- a/chrome/browser/resources/new_tab_page/modules/module_registry.js +++ b/chrome/browser/resources/new_tab_page/modules/module_registry.js @@ -17,6 +17,11 @@ export class ModuleRegistry { this.descriptors_ = []; } + /** @return {!Array} */ + getDescriptors() { + return this.descriptors_; + } + /** * Registers modules via their descriptors. * @param {!Array} descriptors @@ -34,9 +39,17 @@ export class ModuleRegistry { * @return {!Promise>} */ async initializeModules(timeout) { - const disabledIds = - (await BrowserProxy.getInstance().handler.getDisabledModules()) - .moduleIds; + // Capture updateDisabledModules -> setDisabledModules round trip in a + // promise for convenience. + const disabledIds = await new Promise((resolve, _) => { + const callbackRouter = BrowserProxy.getInstance().callbackRouter; + const listenerId = + callbackRouter.setDisabledModules.addListener((all, ids) => { + callbackRouter.removeListener(listenerId); + resolve(all ? this.descriptors_.map(({id}) => id) : ids); + }); + BrowserProxy.getInstance().handler.updateDisabledModules(); + }); await Promise.all( this.descriptors_.filter(d => disabledIds.indexOf(d.id) < 0) .map(d => d.initialize(timeout))); diff --git a/chrome/browser/ui/webui/new_tab_page/new_tab_page.mojom b/chrome/browser/ui/webui/new_tab_page/new_tab_page.mojom index 572a65c4f92e4..1783caf2084c7 100644 --- a/chrome/browser/ui/webui/new_tab_page/new_tab_page.mojom +++ b/chrome/browser/ui/webui/new_tab_page/new_tab_page.mojom @@ -322,12 +322,10 @@ interface PageHandler { OnRestoreModule(string module_id); // If |visible| the modules will be shown. SetModulesVisible(bool visible); - // Triggers a call to |SetModulesVisible| with the current visibility setting. - UpdateModulesVisible(); // Disables module with ID |module_id| if |disabled|. Enables otherwise. SetModuleDisabled(string module_id, bool disabled); - // Returns IDs of disabled modules. - GetDisabledModules() => (array module_ids); + // Triggers a call to |SetDisabledModules|. + UpdateDisabledModules(); // ======= METRICS ======= // Logs that |tiles| were displayed / updated at |time|. The first instance of @@ -398,6 +396,7 @@ interface Page { SetMostVisitedInfo(MostVisitedInfo info); // Sets the current theme. SetTheme(Theme theme); - // Updates the module visibility. - SetModulesVisible(bool visible); + // Disables the modules in |ids|. If |all|, disables all modules and passes an + // empty list for |ids|. + SetDisabledModules(bool all, array ids); }; diff --git a/chrome/browser/ui/webui/new_tab_page/new_tab_page_handler.cc b/chrome/browser/ui/webui/new_tab_page/new_tab_page_handler.cc index 74087fe248baa..a1c923bcf04cc 100644 --- a/chrome/browser/ui/webui/new_tab_page/new_tab_page_handler.cc +++ b/chrome/browser/ui/webui/new_tab_page/new_tab_page_handler.cc @@ -631,12 +631,7 @@ void NewTabPageHandler::OnRestoreModule(const std::string& module_id) { void NewTabPageHandler::SetModulesVisible(bool visible) { profile_->GetPrefs()->SetBoolean(prefs::kNtpModulesVisible, visible); - UpdateModulesVisible(); -} - -void NewTabPageHandler::UpdateModulesVisible() { - page_->SetModulesVisible( - profile_->GetPrefs()->GetBoolean(prefs::kNtpModulesVisible)); + UpdateDisabledModules(); } void NewTabPageHandler::SetModuleDisabled(const std::string& module_id, @@ -647,17 +642,23 @@ void NewTabPageHandler::SetModuleDisabled(const std::string& module_id, } else { update->EraseListValue(base::Value(module_id)); } + UpdateDisabledModules(); } -void NewTabPageHandler::GetDisabledModules( - GetDisabledModulesCallback callback) { - const auto* module_ids_value = - profile_->GetPrefs()->GetList(prefs::kNtpDisabledModules); +void NewTabPageHandler::UpdateDisabledModules() { std::vector module_ids; - for (const auto& id : *module_ids_value) { - module_ids.push_back(id.GetString()); + // If the module visibility is managed by policy we either disable all modules + // (if invisible) or no modules (if visible). + if (!profile_->GetPrefs()->IsManagedPreference(prefs::kNtpModulesVisible)) { + const auto* module_ids_value = + profile_->GetPrefs()->GetList(prefs::kNtpDisabledModules); + for (const auto& id : *module_ids_value) { + module_ids.push_back(id.GetString()); + } } - std::move(callback).Run(std::move(module_ids)); + page_->SetDisabledModules( + !profile_->GetPrefs()->GetBoolean(prefs::kNtpModulesVisible), + std::move(module_ids)); } void NewTabPageHandler::OnPromoDataUpdated() { diff --git a/chrome/browser/ui/webui/new_tab_page/new_tab_page_handler.h b/chrome/browser/ui/webui/new_tab_page/new_tab_page_handler.h index 1b3613d62cfbe..865b53850a5fb 100644 --- a/chrome/browser/ui/webui/new_tab_page/new_tab_page_handler.h +++ b/chrome/browser/ui/webui/new_tab_page/new_tab_page_handler.h @@ -100,9 +100,8 @@ class NewTabPageHandler : public new_tab_page::mojom::PageHandler, void OnDismissModule(const std::string& module_id) override; void OnRestoreModule(const std::string& module_id) override; void SetModulesVisible(bool visible) override; - void UpdateModulesVisible() override; void SetModuleDisabled(const std::string& module_id, bool disabled) override; - void GetDisabledModules(GetDisabledModulesCallback callback) override; + void UpdateDisabledModules() override; void OnAppRendered(double time) override; void OnMostVisitedTilesRendered( std::vector tiles, diff --git a/chrome/browser/ui/webui/new_tab_page/new_tab_page_handler_unittest.cc b/chrome/browser/ui/webui/new_tab_page/new_tab_page_handler_unittest.cc index 000baa0aff785..242d755a8bf90 100644 --- a/chrome/browser/ui/webui/new_tab_page/new_tab_page_handler_unittest.cc +++ b/chrome/browser/ui/webui/new_tab_page/new_tab_page_handler_unittest.cc @@ -44,7 +44,7 @@ class MockPage : public new_tab_page::mojom::Page { MOCK_METHOD1(SetMostVisitedInfo, void(new_tab_page::mojom::MostVisitedInfoPtr)); MOCK_METHOD1(SetTheme, void(new_tab_page::mojom::ThemePtr)); - MOCK_METHOD1(SetModulesVisible, void(bool)); + MOCK_METHOD2(SetDisabledModules, void(bool, const std::vector&)); MOCK_METHOD1(AutocompleteResultChanged, void(search::mojom::AutocompleteResultPtr)); MOCK_METHOD3(AutocompleteMatchImageAvailable, diff --git a/chrome/browser/ui/webui/new_tab_page/new_tab_page_ui.cc b/chrome/browser/ui/webui/new_tab_page/new_tab_page_ui.cc index 2871d2e5169cc..f15a8f628992a 100644 --- a/chrome/browser/ui/webui/new_tab_page/new_tab_page_ui.cc +++ b/chrome/browser/ui/webui/new_tab_page/new_tab_page_ui.cc @@ -144,7 +144,8 @@ content::WebUIDataSource* CreateNewTabPageUiHtmlSource(Profile* profile) { {"defaultThemeLabel", IDS_NTP_CUSTOMIZE_DEFAULT_LABEL}, {"hideShortcuts", IDS_NTP_CUSTOMIZE_HIDE_SHORTCUTS_LABEL}, {"hideShortcutsDesc", IDS_NTP_CUSTOMIZE_HIDE_SHORTCUTS_DESC}, - {"showModules", IDS_NTP_CUSTOMIZE_SHOW_MODULES_LABEL}, + {"hideAllCards", IDS_NTP_CUSTOMIZE_HIDE_ALL_CARDS_LABEL}, + {"customizeCards", IDS_NTP_CUSTOMIZE_CUSTOMIZE_CARDS_LABEL}, {"mostVisited", IDS_NTP_CUSTOMIZE_MOST_VISITED_LABEL}, {"myShortcuts", IDS_NTP_CUSTOMIZE_MY_SHORTCUTS_LABEL}, {"noBackground", IDS_NTP_CUSTOMIZE_NO_BACKGROUND_LABEL}, diff --git a/chrome/test/data/webui/new_tab_page/app_test.js b/chrome/test/data/webui/new_tab_page/app_test.js index acf7d8467bafe..a3b63a7fc823f 100644 --- a/chrome/test/data/webui/new_tab_page/app_test.js +++ b/chrome/test/data/webui/new_tab_page/app_test.js @@ -22,7 +22,7 @@ suite('NewTabPageAppTest', () => { let testProxy; /** @type {FakeMetricsPrivate} */ - let fakeMetricsPrivate; + let metrics; /** * @implements {BackgroundManager} @@ -65,10 +65,11 @@ suite('NewTabPageAppTest', () => { BackgroundManager.instance_ = backgroundManager; const moduleRegistry = TestBrowserProxy.fromClass(ModuleRegistry); moduleResolver = new PromiseResolver(); + moduleRegistry.setResultFor('getDescriptors', []); moduleRegistry.setResultFor('initializeModules', moduleResolver.promise); ModuleRegistry.instance_ = moduleRegistry; - fakeMetricsPrivate = new FakeMetricsPrivate(); - chrome.metricsPrivate = fakeMetricsPrivate; + metrics = new FakeMetricsPrivate(); + chrome.metricsPrivate = metrics; app = document.createElement('ntp-app'); document.body.appendChild(app); @@ -467,16 +468,18 @@ suite('NewTabPageAppTest', () => { .dispatchEvent(new Event( 'ntp-middle-slot-promo-loaded', {bubbles: true, composed: true})); - testProxy.callbackRouterRemote.setModulesVisible(visible); + testProxy.callbackRouterRemote.setDisabledModules(!visible, ['bar']); await flushTasks(); // Wait for module descriptor resolution. // Assert. const modules = app.shadowRoot.querySelectorAll('ntp-module-wrapper'); assertEquals(2, modules.length); + assertEquals(1, testProxy.handler.getCallCount('onModulesRendered')); + const histogram = 'NewTabPage.Modules.EnabledOnNTPLoad'; + assertEquals(1, metrics.count(`${histogram}.foo`, visible)); + assertEquals(1, metrics.count(`${histogram}.bar`, false)); assertEquals( - visible ? 1 : 0, - testProxy.handler.getCallCount('onModulesRendered')); - assertEquals(1, testProxy.handler.getCallCount('updateModulesVisible')); + 1, testProxy.handler.getCallCount('updateDisabledModules')); }); }); @@ -563,12 +566,9 @@ suite('NewTabPageAppTest', () => { 'hello "bar"', $$(app, '#removeModuleToastMessage').textContent.trim()); assertNotStyle($$(app, '#undoRemoveModuleButton'), 'display', 'none'); + assertEquals(1, metrics.count('NewTabPage.Modules.Disabled', 'foo')); assertEquals( - 1, fakeMetricsPrivate.count('NewTabPage.Modules.Disabled', 'foo')); - assertEquals( - 1, - fakeMetricsPrivate.count( - 'NewTabPage.Modules.Disabled.ModuleRequest', 'foo')); + 1, metrics.count('NewTabPage.Modules.Disabled.ModuleRequest', 'foo')); // Act. $$(app, '#undoRemoveModuleButton').click(); @@ -576,11 +576,8 @@ suite('NewTabPageAppTest', () => { // Assert. assertFalse($$(app, '#removeModuleToast').open); - assertEquals( - 1, fakeMetricsPrivate.count('NewTabPage.Modules.Enabled', 'foo')); - assertEquals( - 1, - fakeMetricsPrivate.count('NewTabPage.Modules.Enabled.Toast', 'foo')); + assertEquals(1, metrics.count('NewTabPage.Modules.Enabled', 'foo')); + assertEquals(1, metrics.count('NewTabPage.Modules.Enabled.Toast', 'foo')); }); }); }); diff --git a/chrome/test/data/webui/new_tab_page/customize_modules_test.js b/chrome/test/data/webui/new_tab_page/customize_modules_test.js index 49ded7340b251..c5cdb3dd8c2d3 100644 --- a/chrome/test/data/webui/new_tab_page/customize_modules_test.js +++ b/chrome/test/data/webui/new_tab_page/customize_modules_test.js @@ -4,10 +4,20 @@ import 'chrome://new-tab-page/lazy_load.js'; -import {$$, BrowserProxy} from 'chrome://new-tab-page/new_tab_page.js'; +import {$$, BrowserProxy, ModuleDescriptor, ModuleRegistry} from 'chrome://new-tab-page/new_tab_page.js'; import {loadTimeData} from 'chrome://resources/js/load_time_data.m.js'; +import {FakeMetricsPrivate} from 'chrome://test/new_tab_page/metrics_test_support.js'; import {assertNotStyle, assertStyle, createTestProxy} from 'chrome://test/new_tab_page/test_support.js'; -import {flushTasks} from 'chrome://test/test_util.m.js'; +import {TestBrowserProxy} from 'chrome://test/test_browser_proxy.m.js'; + +/** + * @param {!HTMLElement} host + * @param {string} selector + * @return {!Array} + */ +function queryAll(host, selector) { + return Array.from(host.shadowRoot.querySelectorAll(selector)); +} suite('NewTabPageCustomizeModulesTest', () => { /** @@ -16,46 +26,159 @@ suite('NewTabPageCustomizeModulesTest', () => { */ let testProxy; + /** + * @implements {ModuleRegistry} + * @extends {TestBrowserProxy} + */ + let moduleRegistry; + + /** @type {FakeMetricsPrivate} */ + let metrics; + + /** + * @param {boolean} allDisabled + * @param {!Array} modules + * @return {!Promise} + */ + async function createCustomizeModules(allDisabled, modules) { + moduleRegistry.setResultFor( + 'getDescriptors', + modules.map( + ({id, name}) => new ModuleDescriptor(id, name, 1, () => null))); + const customizeModules = document.createElement('ntp-customize-modules'); + document.body.appendChild(customizeModules); + testProxy.callbackRouterRemote.setDisabledModules( + allDisabled, + modules.filter(({disabled}) => disabled).map(({id}) => id)); + await testProxy.callbackRouterRemote.$.flushForTesting(); + return customizeModules; + } + setup(() => { PolymerTest.clearBody(); testProxy = createTestProxy(); BrowserProxy.instance_ = testProxy; + moduleRegistry = TestBrowserProxy.fromClass(ModuleRegistry); + ModuleRegistry.instance_ = moduleRegistry; + metrics = new FakeMetricsPrivate(); + chrome.metricsPrivate = metrics; + loadTimeData.overrideValues({modulesVisibleManagedByPolicy: false}); }); [true, false].forEach(visible => { - test('toggle show calls handler', async () => { - // Arrange. - const customizeModules = document.createElement('ntp-customize-modules'); - document.body.appendChild(customizeModules); - testProxy.callbackRouterRemote.setModulesVisible(visible); + const mode = visible ? 'customize' : 'hide'; + test(`creating element shows correct config in ${mode} mode`, async () => { + // Arrange & Act. + const customizeModules = await createCustomizeModules(!visible, [ + {id: 'foo', name: 'foo name', disabled: false}, + {id: 'bar', name: 'bar name', disabled: false}, + {id: 'baz', name: 'baz name', disabled: true}, + ]); - // setModulesVisible sets visible to true, but customizeModules' show_ - // isn't immediately updated after. Using flushForTesting ensures - // everything from the callback is done updating before proceeding. - await testProxy.callbackRouterRemote.$.flushForTesting(); + // Assert. + assertEquals(visible, customizeModules.$.customizeButton.checked); + assertEquals(!visible, customizeModules.$.hideButton.checked); + const names = queryAll(customizeModules, '.toggle-name'); + assertEquals(3, names.length); + assertEquals('foo name', names[0].textContent.trim()); + assertEquals('bar name', names[1].textContent.trim()); + assertEquals('baz name', names[2].textContent.trim()); + const toggles = queryAll(customizeModules, 'cr-toggle'); + assertEquals(3, toggles.length); + assertEquals(!visible, toggles[0].disabled); + assertEquals(!visible, toggles[1].disabled); + assertEquals(!visible, toggles[2].disabled); + assertEquals(visible, toggles[0].checked); + assertEquals(visible, toggles[1].checked); + assertFalse(toggles[2].checked); + queryAll(customizeModules, 'cr-policy-indicator').forEach((el) => { + assertStyle(el, 'display', 'none'); + }); + }); + + test(`toggle in ${mode} mode sets module status`, async () => { + // Arrange. + const customizeModules = await createCustomizeModules(!visible, [ + {id: 'foo', name: 'foo name', disabled: false}, + {id: 'bar', name: 'bar name', disabled: false}, + {id: 'baz', name: 'baz name', disabled: true}, + ]); // Act. - customizeModules.$.showToggle.click(); + $$(customizeModules, `#${visible ? 'hide' : 'customize'}Button`).click(); customizeModules.apply(); // Assert. - assertEquals( - !visible, await testProxy.handler.whenCalled('setModulesVisible')); - assertStyle( - $$(customizeModules, 'cr-policy-indicator'), 'display', 'none'); + assertEquals(!visible, testProxy.handler.getArgs('setModulesVisible')[0]); + const toggles = queryAll(customizeModules, 'cr-toggle'); + assertEquals(3, toggles.length); + assertEquals(visible, toggles[0].disabled); + assertEquals(visible, toggles[1].disabled); + assertEquals(visible, toggles[2].disabled); + assertEquals(!visible, toggles[0].checked); + assertEquals(!visible, toggles[1].checked); + assertFalse(toggles[2].checked); + const visibleEnabled = visible ? 'Enabled' : 'Disabled'; + const hideEnabled = !visible ? 'Enabled' : 'Disabled'; + const base = 'NewTabPage.Modules'; + assertEquals(2, metrics.count(`${base}.${hideEnabled}`)); + assertEquals(2, metrics.count(`${base}.${hideEnabled}.Customize`)); + assertEquals(0, metrics.count(`${base}.${visibleEnabled}`)); + assertEquals(0, metrics.count(`${base}.${visibleEnabled}.Customize`)); + }); + + test(`policy disables UI in ${mode} mode`, async () => { + // Act. + loadTimeData.overrideValues({modulesVisibleManagedByPolicy: true}); + const customizeModules = await createCustomizeModules(!visible, [ + {id: 'foo', name: 'foo name', disabled: false}, + {id: 'bar', name: 'bar name', disabled: false}, + {id: 'baz', name: 'baz name', disabled: true}, + ]); + + // Assert. + assertTrue($$(customizeModules, 'cr-radio-group').disabled); + const toggles = queryAll(customizeModules, 'cr-toggle'); + assertEquals(3, toggles.length); + assertTrue(toggles[0].disabled); + assertTrue(toggles[1].disabled); + assertTrue(toggles[2].disabled); + assertEquals(visible, toggles[0].checked); + assertEquals(visible, toggles[1].checked); + assertFalse(toggles[2].checked); + queryAll(customizeModules, 'cr-policy-indicator').forEach((el) => { + assertNotStyle(el, 'display', 'none'); + }); }); }); - test('policy disables show toggle', () => { + test('toggling disables, enables module', async () => { + // Arrange. + const customizeModules = await createCustomizeModules(false, [ + {id: 'foo', name: 'foo name', disabled: false}, + {id: 'bar', name: 'bar name', disabled: false}, + {id: 'baz', name: 'baz name', disabled: true}, + ]); + // Act. - loadTimeData.overrideValues({modulesVisibleManagedByPolicy: true}); - const customizeModules = document.createElement('ntp-customize-modules'); - document.body.appendChild(customizeModules); + const toggles = queryAll(customizeModules, 'cr-toggle'); + toggles[0].click(); + toggles[2].click(); + customizeModules.apply(); // Assert. - assertTrue(customizeModules.$.showToggle.disabled); - assertNotStyle( - $$(customizeModules, 'cr-policy-indicator'), 'display', 'none'); + const base = 'NewTabPage.Modules'; + assertEquals(2, testProxy.handler.getCallCount('setModuleDisabled')); + assertDeepEquals( + ['foo', true], testProxy.handler.getArgs('setModuleDisabled')[0]); + assertDeepEquals( + ['baz', false], testProxy.handler.getArgs('setModuleDisabled')[1]); + assertEquals(1, metrics.count(`${base}.Disabled`)); + assertEquals(1, metrics.count(`${base}.Disabled.Customize`)); + assertEquals(1, metrics.count(`${base}.Disabled`, 'foo')); + assertEquals(1, metrics.count(`${base}.Enabled`)); + assertEquals(1, metrics.count(`${base}.Enabled.Customize`)); + assertEquals(1, metrics.count(`${base}.Enabled`, 'baz')); }); }); diff --git a/chrome/test/data/webui/new_tab_page/metrics_test_support.js b/chrome/test/data/webui/new_tab_page/metrics_test_support.js index ea81fc31b6a86..059c3b34e0ac3 100644 --- a/chrome/test/data/webui/new_tab_page/metrics_test_support.js +++ b/chrome/test/data/webui/new_tab_page/metrics_test_support.js @@ -36,6 +36,14 @@ export class FakeMetricsPrivate { this.get_(metricName).push(value); } + /** + * @param {string} metricName + * @param {boolean} value + */ + recordBoolean(metricName, value) { + this.get_(metricName).push(value); + } + /** * @param {string} metricName * @return {!Map} diff --git a/chrome/test/data/webui/new_tab_page/modules/module_registry_test.js b/chrome/test/data/webui/new_tab_page/modules/module_registry_test.js index 63e0a45e6ed41..d191c8f6efbf8 100644 --- a/chrome/test/data/webui/new_tab_page/modules/module_registry_test.js +++ b/chrome/test/data/webui/new_tab_page/modules/module_registry_test.js @@ -30,16 +30,16 @@ suite('NewTabPageModulesModuleRegistryTest', () => { new ModuleDescriptor('baz', 'bla', 300, () => bazModuleResolver.promise), new ModuleDescriptor('buz', 'blo', 400, () => Promise.resolve(fooModule)), ]); - testProxy.handler.setResultFor( - 'getDisabledModules', Promise.resolve({moduleIds: ['buz']})); // Act. const modulesPromise = ModuleRegistry.getInstance().initializeModules(0); + testProxy.callbackRouterRemote.setDisabledModules(false, ['buz']); // Delayed promise resolution to test async module instantiation. bazModuleResolver.resolve(bazModule); const modules = await modulesPromise; // Assert. + assertEquals(1, testProxy.handler.getCallCount('updateDisabledModules')); assertEquals(2, modules.length); assertEquals('foo', modules[0].id); assertEquals(100, modules[0].heightPx); diff --git a/chrome/test/data/webui/test_browser_proxy.js b/chrome/test/data/webui/test_browser_proxy.js index 587dabc755d61..49358e67091aa 100644 --- a/chrome/test/data/webui/test_browser_proxy.js +++ b/chrome/test/data/webui/test_browser_proxy.js @@ -7,7 +7,7 @@ /** * @typedef {{resolver: !PromiseResolver, - * callCount: number, + * args: !Array<*>, * resultMapper: (!Function|undefined)}} */ let MethodData; @@ -105,7 +105,7 @@ let MethodData; */ methodCalled(methodName, opt_arg) { const methodData = this.resolverMap_.get(methodName); - methodData.callCount += 1; + methodData.args.push(opt_arg); this.resolverMap_.set(methodName, methodData); methodData.resolver.resolve(opt_arg); } @@ -167,7 +167,16 @@ let MethodData; * @return {number} */ getCallCount(methodName) { - return this.getMethodData_(methodName).callCount; + return this.getMethodData_(methodName).args.length; + } + + /** + * Returns the arguments of calls made to |method|. + * @param {string} methodName + * @return {!Array<*>} + */ + getArgs(methodName) { + return this.getMethodData_(methodName).args; } /** @@ -212,6 +221,6 @@ let MethodData; */ createMethodData_(methodName) { this.resolverMap_.set( - methodName, {resolver: new PromiseResolver(), callCount: 0}); + methodName, {resolver: new PromiseResolver(), args: []}); } } diff --git a/tools/metrics/histograms/histograms_xml/histogram_suffixes_list.xml b/tools/metrics/histograms/histograms_xml/histogram_suffixes_list.xml index 90dc1d7e57279..930ce07c43839 100644 --- a/tools/metrics/histograms/histograms_xml/histogram_suffixes_list.xml +++ b/tools/metrics/histograms/histograms_xml/histogram_suffixes_list.xml @@ -11391,6 +11391,7 @@ reviews. Googlers can read more about this at go/gwsq-gerrit. + diff --git a/tools/metrics/histograms/histograms_xml/new_tab_page/histograms.xml b/tools/metrics/histograms/histograms_xml/new_tab_page/histograms.xml index b04d328b8f1af..35b70610a453c 100644 --- a/tools/metrics/histograms/histograms_xml/new_tab_page/histograms.xml +++ b/tools/metrics/histograms/histograms_xml/new_tab_page/histograms.xml @@ -804,6 +804,7 @@ reviews. Googlers can read more about this at go/gwsq-gerrit. + @@ -822,6 +823,19 @@ reviews. Googlers can read more about this at go/gwsq-gerrit. + + danielms@google.com + tiborg@chromium.org + chrome-desktop-ntp@google.com + + Logs for each NTP module whether it was enabled after the NTP has + instantiated the modules. Only logged on the 1P NTP. Note that even if the + user has Google as their default search engine, Incognito and Guest mode + NTPs are not considered 1P and don't log this histogram. + + + danielms@google.com @@ -835,6 +849,7 @@ reviews. Googlers can read more about this at go/gwsq-gerrit. +