Skip to content

Commit

Permalink
[MV] Refactors NTP Most Visited tiles into cr_components (part 1)
Browse files Browse the repository at this point in the history
This CL refactors the NTP Most Visited tiles which are largely
duplicated between the 1P and the 3P NTP into cr_components. It also
migrates the <cr-most-visited> to Mojo JS modules. The 3P NTP will
start using this new component in a follow up CL.

In addition to migrating the UI and handler code, this CL makes the
following changes to the MV and as a result the NTP:
- Allows MV tiles to handle its own visibility and theming for better
  encapsulation of that logic.
- Splits the browser interface for observing changes in the MV info
  into two separate callbacks, one for MV settings and the other MV
  tiles. This avoids sending stale MV settings to the UI and will
  simplify the browser side code in the future.
- NTP customize dialog will longer observe changes to the MV settings.
  the customize module will query those settings once shown.

Bug: 1211534
Change-Id: Ie5e836cdd5ca04342b58f97cb779a75ef19cea27
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2919142
Commit-Queue: Moe Ahmadi <mahmadi@chromium.org>
Reviewed-by: Tibor Goldschwendt <tiborg@chromium.org>
Reviewed-by: dpapad <dpapad@chromium.org>
Reviewed-by: Ken Rockot <rockot@google.com>
Reviewed-by: Alex Gough <ajgo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#888254}
  • Loading branch information
Moe Ahmadi authored and Chromium LUCI CQ committed Jun 2, 2021
1 parent 909aa2c commit 122826c
Show file tree
Hide file tree
Showing 42 changed files with 1,253 additions and 627 deletions.
1 change: 1 addition & 0 deletions chrome/browser/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -2365,6 +2365,7 @@ static_library("browser") {
"//ui/web_dialogs",
"//ui/webui",
"//ui/webui/resources/cr_components/customize_themes:mojom",
"//ui/webui/resources/cr_components/most_visited:mojom",
]
if (is_chromeos_ash) {
sources += [
Expand Down
4 changes: 4 additions & 0 deletions chrome/browser/chrome_browser_interface_binders.cc
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@
#include "media/base/media_switches.h"
#include "media/mojo/mojom/speech_recognition_service.mojom.h"
#include "mojo/public/cpp/bindings/self_owned_receiver.h"
#include "ui/webui/resources/cr_components/most_visited/most_visited.mojom.h"
#endif // defined(OS_ANDROID)

#if defined(OS_WIN) || defined(OS_MAC) || defined(OS_LINUX) || \
Expand Down Expand Up @@ -657,6 +658,9 @@ void PopulateChromeWebUIFrameBinders(
RegisterWebUIControllerInterfaceBinder<
new_tab_page::mojom::PageHandlerFactory, NewTabPageUI>(map);

RegisterWebUIControllerInterfaceBinder<
most_visited::mojom::MostVisitedPageHandlerFactory, NewTabPageUI>(map);

RegisterWebUIControllerInterfaceBinder<history_clusters::mojom::PageHandler,
MemoriesUI>(map);

Expand Down
23 changes: 5 additions & 18 deletions chrome/browser/resources/new_tab_page/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,9 @@ js_library("lazy_load") {
deps = [
":customize_dialog",
":middle_slot_promo",
":most_visited",
":voice_search_overlay",
"modules/dummy:module",
"//ui/webui/resources/cr_components/most_visited",
]
}

Expand Down Expand Up @@ -107,21 +107,6 @@ js_library("middle_slot_promo") {
]
}

js_library("most_visited") {
deps = [
":i18n_setup",
":new_tab_page_proxy",
":window_proxy",
"//third_party/polymer/v3_0/components-chromium/polymer:polymer_bundled",
"//ui/webui/resources/cr_elements/cr_action_menu:cr_action_menu.m",
"//ui/webui/resources/cr_elements/cr_dialog:cr_dialog.m",
"//ui/webui/resources/cr_elements/cr_toast:cr_toast.m",
"//ui/webui/resources/js:assert.m",
"//ui/webui/resources/js:cr.m",
"//ui/webui/resources/js/cr/ui:focus_outline_manager.m",
]
}

js_library("customize_dialog") {
deps = [
":customize_backgrounds",
Expand Down Expand Up @@ -260,7 +245,6 @@ html_to_js("web_components_local") {
"logo.js",
"middle_slot_promo.js",
"mini_page.js",
"most_visited.js",
"iframe.js",
"voice_search_overlay.js",
"customize_modules.js",
Expand Down Expand Up @@ -313,7 +297,6 @@ preprocess_if_expr("preprocess_gen") {
in_files = [
"app.js",
"middle_slot_promo.js",
"most_visited.js",
"customize_dialog.js",
"voice_search_overlay.js",
"customize_backgrounds.js",
Expand Down Expand Up @@ -491,18 +474,22 @@ if (optimize_webui) {
"shared.rollup.js",
]
excludes = [
"chrome://resources/cr_components/most_visited/most_visited.mojom-lite.js",
"chrome://resources/js/cr.m.js",
"chrome://resources/mojo/mojo/public/js/bindings.js",
"chrome://resources/mojo/mojo/public/js/mojo_bindings_lite.js",
"chrome://resources/mojo/mojo/public/mojom/base/big_buffer.mojom-lite.js",
"chrome://resources/mojo/mojo/public/mojom/base/string16.mojom-lite.js",
"chrome://resources/mojo/mojo/public/mojom/base/text_direction.mojom-lite.js",
"chrome://resources/mojo/mojo/public/mojom/base/text_direction.mojom-webui.js",
"chrome://resources/mojo/mojo/public/mojom/base/time.mojom-lite.js",
"chrome://resources/mojo/mojo/public/mojom/base/time.mojom-webui.js",
"chrome://resources/mojo/mojo/public/mojom/base/unguessable_token.mojom-lite.js",
"chrome://resources/mojo/skia/public/mojom/skcolor.mojom-lite.js",
"chrome://resources/mojo/skia/public/mojom/skcolor.mojom-webui.js",
"chrome://resources/mojo/url/mojom/origin.mojom-lite.js",
"chrome://resources/mojo/url/mojom/url.mojom-lite.js",
"chrome://resources/mojo/url/mojom/url.mojom-webui.js",
"new_tab_page.mojom-lite.js",
"realbox/realbox.mojom-lite.js",
"promo_browser_command.mojom-lite.js",
Expand Down
22 changes: 8 additions & 14 deletions chrome/browser/resources/new_tab_page/app.html
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
<style include="cr-shared-style">
:host {
--ntp-theme-shortcut-background-color: rgb(229, 231, 232);
--ntp-theme-text-color: var(--google-grey-800);
--ntp-theme-text-shadow: none;
--ntp-one-google-bar-height: 56px;
Expand All @@ -21,7 +20,6 @@

@media (prefers-color-scheme: dark) {
:host {
--ntp-theme-shortcut-background-color: var(--google-grey-refresh-100);
--ntp-theme-text-color: white;
}
}
Expand Down Expand Up @@ -75,10 +73,10 @@
visibility: visible;
}

ntp-most-visited[dark] {
--icon-button-color-active: var(--google-grey-refresh-300);
--icon-button-color: white;
--tile-hover-color: rgba(255, 255, 255, .1);
cr-most-visited {
--most-visited-focus-shadow: var(--ntp-focus-shadow);
--most-visited-text-color: var(--ntp-theme-text-color);
--most-visited-text-shadow: var(--ntp-theme-text-shadow);
}

/* ~ because the dom-if results in a template between the middle-slot-promo
Expand Down Expand Up @@ -229,10 +227,8 @@
position: fixed;
}
</style>
<div id="content"
style="--ntp-theme-text-color: [[rgbaOrInherit_(theme_.shortcutTextColor)]];
--ntp-theme-shortcut-background-color:
[[rgbaOrInherit_(theme_.shortcutBackgroundColor)]];
<div id="content" style="
--ntp-theme-text-color: [[rgbaOrInherit_(theme_.textColor)]];
--ntp-logo-color: [[rgbaOrInherit_(logoColor_)]];">
<template is="dom-if" if="[[lazyRender_]]">
<ntp-iframe id="oneGoogleBar" src="[[oneGoogleBarIframePath_]]"
Expand All @@ -253,10 +249,8 @@
<dom-if if="[[lazyRender_]]" on-dom-change="onLazyRendered_">
<template>
<template is="dom-if" if="[[shortcutsEnabled_]]">
<ntp-most-visited id="mostVisited" dark$="[[theme_.isDark]]"
use-white-add-icon$="[[theme_.shortcutUseWhiteAddIcon]]"
use-title-pill$="[[theme_.shortcutUseTitlePill]]">
</ntp-most-visited>
<cr-most-visited id="mostVisited" theme="[[theme_.mostVisited]]">
</cr-most-visited>
</template>
<template is="dom-if" if="[[middleSlotPromoEnabled_]]">
<ntp-middle-slot-promo
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/resources/new_tab_page/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -806,7 +806,7 @@ class AppElement extends mixinBehaviors
case $$(this, 'ntp-realbox'):
recordClick(NtpElement.kRealbox);
return;
case $$(this, 'ntp-most-visited'):
case $$(this, 'cr-most-visited'):
recordClick(NtpElement.kMostVisited);
return;
case $$(this, 'ntp-middle-slot-promo'):
Expand Down
28 changes: 8 additions & 20 deletions chrome/browser/resources/new_tab_page/customize_shortcuts.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,37 +41,25 @@ class CustomizeShortcutsElement extends mixinBehaviors

constructor() {
super();
const {callbackRouter, handler} = NewTabPageProxy.getInstance();
/** @private {!newTabPage.mojom.PageCallbackRouter} */
this.callbackRouter_ = callbackRouter;
/** @private {newTabPage.mojom.PageHandlerRemote} */
const {handler} = NewTabPageProxy.getInstance();
/** @private {!newTabPage.mojom.PageHandlerRemote} */
this.pageHandler_ = handler;
/** @private {?number} */
this.setMostVisitedInfoListenerId_ = null;
this.pageHandler_.getMostVisitedSettings().then(
({customLinksEnabled, shortcutsVisible}) => {
this.customLinksEnabled_ = customLinksEnabled;
this.hide_ = !shortcutsVisible;
});
}

/** @override */
connectedCallback() {
super.connectedCallback();
this.setMostVisitedInfoListenerId_ =
this.callbackRouter_.setMostVisitedInfo.addListener(info => {
this.customLinksEnabled_ = info.customLinksEnabled;
this.hide_ = !info.visible;
});
this.pageHandler_.updateMostVisitedInfo();
FocusOutlineManager.forDocument(document);
}

/** @override */
disconnectedCallback() {
super.disconnectedCallback();
this.callbackRouter_.removeListener(
assert(this.setMostVisitedInfoListenerId_));
}

apply() {
this.pageHandler_.setMostVisitedSettings(
this.customLinksEnabled_, /* visible= */ !this.hide_);
this.customLinksEnabled_, /* shortcutsVisible= */ !this.hide_);
}

/**
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/resources/new_tab_page/lazy_load.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,5 @@

import './customize_dialog.js';
import './middle_slot_promo.js';
import './most_visited.js';
import './voice_search_overlay.js';
import 'chrome://resources/cr_components/most_visited/most_visited.js';
2 changes: 2 additions & 0 deletions chrome/browser/resources/new_tab_page/new_tab_page_proxy.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import 'chrome://resources/mojo/mojo/public/mojom/base/time.mojom-lite.js';
import 'chrome://resources/mojo/skia/public/mojom/skcolor.mojom-lite.js';
import 'chrome://resources/mojo/url/mojom/url.mojom-lite.js';

import 'chrome://resources/cr_components/most_visited/most_visited.mojom-lite.js';

import './realbox/realbox.mojom-lite.js';
import './new_tab_page.mojom-lite.js';

Expand Down
3 changes: 3 additions & 0 deletions chrome/browser/ui/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -599,6 +599,7 @@ static_library("ui") {
"//ui/web_dialogs",
"//ui/webui",
"//ui/webui/resources/cr_components/customize_themes:mojom",
"//ui/webui/resources/cr_components/most_visited:mojom",
"//v8:v8_version",
]

Expand Down Expand Up @@ -1341,6 +1342,8 @@ static_library("ui") {
"webui/commander/commander_handler.h",
"webui/commander/commander_ui.cc",
"webui/commander/commander_ui.h",
"webui/cr_components/most_visited/most_visited_handler.cc",
"webui/cr_components/most_visited/most_visited_handler.h",
"webui/customize_themes/chrome_customize_themes_handler.cc",
"webui/customize_themes/chrome_customize_themes_handler.h",
"webui/devtools_ui.cc",
Expand Down
1 change: 1 addition & 0 deletions chrome/browser/ui/webui/cr_components/most_visited/OWNERS
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
file://chrome/browser/ui/webui/new_tab_page/OWNERS
Loading

0 comments on commit 122826c

Please sign in to comment.