From 1f8acc51c0c5d9fea66917884b927eff8f3e570e Mon Sep 17 00:00:00 2001 From: dbeam Date: Thu, 3 Mar 2016 14:28:10 -0800 Subject: NTP4/apps page: fix page stickiness R=estade@chromium.org BUG=591225 Review URL: https://codereview.chromium.org/1757673002 Cr-Commit-Position: refs/heads/master@{#379115} --- chrome/browser/resources/ntp4/page_list_view.js | 36 ++++-------- .../browser/ui/webui/ntp/app_launcher_handler.cc | 51 +++++++++++++---- chrome/browser/ui/webui/ntp/app_launcher_handler.h | 22 +++++++- .../browser/ui/webui/ntp/new_tab_page_handler.cc | 61 --------------------- chrome/browser/ui/webui/ntp/new_tab_page_handler.h | 64 ---------------------- chrome/browser/ui/webui/ntp/new_tab_ui.cc | 4 +- chrome/browser/ui/webui/ntp/ntp_resource_cache.cc | 4 +- chrome/chrome_browser_ui.gypi | 2 - 8 files changed, 75 insertions(+), 169 deletions(-) delete mode 100644 chrome/browser/ui/webui/ntp/new_tab_page_handler.cc delete mode 100644 chrome/browser/ui/webui/ntp/new_tab_page_handler.h diff --git a/chrome/browser/resources/ntp4/page_list_view.js b/chrome/browser/resources/ntp4/page_list_view.js index 69027b9..efb31fe4 100644 --- a/chrome/browser/resources/ntp4/page_list_view.js +++ b/chrome/browser/resources/ntp4/page_list_view.js @@ -108,14 +108,8 @@ cr.define('ntp', function() { trash: undefined, /** - * The type of page that is currently shown. The value is a numerical ID. - * @type {number} - */ - shownPage: 0, - - /** - * The index of the page that is currently shown, within the page type. - * For example if the third Apps page is showing, this will be 2. + * The index of the page that is currently shown. For example if the third + * page is showing, this will be 2. * @type {number} */ shownPageIndex: 0, @@ -166,7 +160,6 @@ cr.define('ntp', function() { if (this.pageSwitcherEnd) ntp.initializePageSwitcher(this.pageSwitcherEnd); - this.shownPage = loadTimeData.getInteger('shown_page_type'); this.shownPageIndex = loadTimeData.getInteger('shown_page_index'); // TODO(dbeam): remove showApps and everything that says if (apps). @@ -450,8 +443,7 @@ cr.define('ntp', function() { app.replaceAppData(appData); } else if (opt_highlight) { page.insertAndHighlightApp(appData); - this.setShownPage_(loadTimeData.getInteger('apps_page_id'), - appData.page_index); + this.setShownPage_(appData.page_index); } else { page.insertApp(appData, false); } @@ -488,11 +480,11 @@ cr.define('ntp', function() { /** * Updates the hidden state of the app launcher promo based on the page * shown and load data content. + * @private */ updateAppLauncherPromoHiddenState_: function() { $('app-launcher-promo').hidden = - !loadTimeData.getBoolean('showAppLauncherPromo') || - this.shownPage != loadTimeData.getInteger('apps_page_id'); + !loadTimeData.getBoolean('showAppLauncherPromo'); }, /** @@ -504,8 +496,7 @@ cr.define('ntp', function() { this.tilePages.length - 1)); this.cardSlider.setCards(Array.prototype.slice.call(this.tilePages), pageNo); - if (this.shownPage == loadTimeData.getInteger('apps_page_id') && - loadTimeData.getBoolean('showApps')) { + if (loadTimeData.getBoolean('showApps')) { this.cardSlider.selectCardByValue( this.appsPages[Math.min(this.shownPageIndex, this.appsPages.length - 1)]); @@ -632,12 +623,11 @@ cr.define('ntp', function() { // Don't change shownPage until startup is done (and page changes actually // reflect user actions). if (!this.isStartingUp_()) { - if (page.classList.contains('apps-page')) { - this.setShownPage_(loadTimeData.getInteger('apps_page_id'), - this.getAppsPageIndex(page)); - } else { + // TODO(dbeam): is this ever false? + if (page.classList.contains('apps-page')) + this.setShownPage_(this.getAppsPageIndex(page)); + else console.error('unknown page selected'); - } } // Update the active dot @@ -650,15 +640,13 @@ cr.define('ntp', function() { /** * Saves/updates the newly selected page to open when first loading the NTP. - * @param {number} shownPage The new shown page type. * @param {number} shownPageIndex The new shown page index. * @private */ - setShownPage_: function(shownPage, shownPageIndex) { + setShownPage_: function(shownPageIndex) { assert(shownPageIndex >= 0); - this.shownPage = shownPage; this.shownPageIndex = shownPageIndex; - chrome.send('pageSelected', [this.shownPage, this.shownPageIndex]); + chrome.send('pageSelected', [this.shownPageIndex]); this.updateAppLauncherPromoHiddenState_(); }, diff --git a/chrome/browser/ui/webui/ntp/app_launcher_handler.cc b/chrome/browser/ui/webui/ntp/app_launcher_handler.cc index 29d62a7..c3adbc2 100644 --- a/chrome/browser/ui/webui/ntp/app_launcher_handler.cc +++ b/chrome/browser/ui/webui/ntp/app_launcher_handler.cc @@ -48,6 +48,7 @@ #include "chrome/common/web_application_info.h" #include "chrome/grit/generated_resources.h" #include "components/favicon_base/favicon_types.h" +#include "components/pref_registry/pref_registry_syncable.h" #include "components/prefs/pref_service.h" #include "components/prefs/scoped_user_pref_update.h" #include "content/public/browser/notification_service.h" @@ -79,6 +80,16 @@ using extensions::ExtensionSystem; namespace { +// The purpose of this enum is to track which page on the NTP is showing. +// The lower 10 bits of kNtpShownPage are used for the index within the page +// group, and the rest of the bits are used for the page group ID (defined +// here). +static const int kPageIdOffset = 10; +enum { + INDEX_MASK = (1 << kPageIdOffset) - 1, + APPS_PAGE_ID = 2 << kPageIdOffset, +}; + void RecordAppLauncherPromoHistogram( apps::AppLauncherPromoHistogramValues value) { DCHECK_LT(value, apps::APP_LAUNCHER_PROMO_MAX); @@ -86,11 +97,6 @@ void RecordAppLauncherPromoHistogram( "Apps.AppLauncherPromo", value, apps::APP_LAUNCHER_PROMO_MAX); } -// This is used to avoid a DCHECK due to an unhandled WebUI callback. The -// JavaScript used to switch between pages sends "pageSelected" which is used -// in the context of the NTP for recording metrics we don't need here. -void NoOpCallback(const base::ListValue* args) {} - } // namespace AppLauncherHandler::AppInstallInfo::AppInstallInfo() {} @@ -206,6 +212,20 @@ void AppLauncherHandler::CreateAppInfo( value->SetString("app_launch_ordinal", app_launch_ordinal.ToInternalValue()); } +// static +void AppLauncherHandler::GetLocalizedValues(Profile* profile, + base::DictionaryValue* values) { + PrefService* prefs = profile->GetPrefs(); + int shown_page = prefs->GetInteger(prefs::kNtpShownPage); + values->SetInteger("shown_page_index", shown_page & INDEX_MASK); +} + +// static +void AppLauncherHandler::RegisterProfilePrefs( + user_prefs::PrefRegistrySyncable* registry) { + registry->RegisterIntegerPref(prefs::kNtpShownPage, APPS_PAGE_ID); +} + void AppLauncherHandler::RegisterMessages() { registrar_.Add(this, chrome::NOTIFICATION_APP_INSTALLED_TO_NTP, content::Source(web_ui()->GetWebContents())); @@ -251,12 +271,14 @@ void AppLauncherHandler::RegisterMessages() { base::Bind(&AppLauncherHandler::HandleGenerateAppForLink, base::Unretained(this))); web_ui()->RegisterMessageCallback("stopShowingAppLauncherPromo", - base::Bind(&AppLauncherHandler::StopShowingAppLauncherPromo, + base::Bind(&AppLauncherHandler::HandleStopShowingAppLauncherPromo, base::Unretained(this))); web_ui()->RegisterMessageCallback("onLearnMore", - base::Bind(&AppLauncherHandler::OnLearnMore, + base::Bind(&AppLauncherHandler::HandleOnLearnMore, + base::Unretained(this))); + web_ui()->RegisterMessageCallback("pageSelected", + base::Bind(&AppLauncherHandler::HandlePageSelected, base::Unretained(this))); - web_ui()->RegisterMessageCallback("pageSelected", base::Bind(&NoOpCallback)); } void AppLauncherHandler::Observe(int type, @@ -710,7 +732,7 @@ void AppLauncherHandler::HandleGenerateAppForLink(const base::ListValue* args) { &cancelable_task_tracker_); } -void AppLauncherHandler::StopShowingAppLauncherPromo( +void AppLauncherHandler::HandleStopShowingAppLauncherPromo( const base::ListValue* args) { #if defined(ENABLE_APP_LIST) g_browser_process->local_state()->SetBoolean( @@ -719,10 +741,19 @@ void AppLauncherHandler::StopShowingAppLauncherPromo( #endif } -void AppLauncherHandler::OnLearnMore(const base::ListValue* args) { +void AppLauncherHandler::HandleOnLearnMore(const base::ListValue* args) { RecordAppLauncherPromoHistogram(apps::APP_LAUNCHER_PROMO_LEARN_MORE); } +void AppLauncherHandler::HandlePageSelected(const base::ListValue* args) { + double index_double; + CHECK(args->GetDouble(0, &index_double)); + int index = static_cast(index_double); + + PrefService* prefs = Profile::FromWebUI(web_ui())->GetPrefs(); + prefs->SetInteger(prefs::kNtpShownPage, APPS_PAGE_ID | index); +} + void AppLauncherHandler::OnFaviconForApp( scoped_ptr install_info, const favicon_base::FaviconImageResult& image_result) { diff --git a/chrome/browser/ui/webui/ntp/app_launcher_handler.h b/chrome/browser/ui/webui/ntp/app_launcher_handler.h index d95e43d..ff958fc 100644 --- a/chrome/browser/ui/webui/ntp/app_launcher_handler.h +++ b/chrome/browser/ui/webui/ntp/app_launcher_handler.h @@ -33,6 +33,10 @@ namespace favicon_base { struct FaviconImageResult; } +namespace user_prefs { +class PrefRegistrySyncable; +} + // The handler for Javascript messages related to the "apps" view. class AppLauncherHandler : public content::WebUIMessageHandler, @@ -50,6 +54,13 @@ class AppLauncherHandler ExtensionService* service, base::DictionaryValue* value); + // Registers values (strings etc.) for the page. + static void GetLocalizedValues(Profile* profile, + base::DictionaryValue* values); + + // Register per-profile preferences. + static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry); + // WebUIMessageHandler: void RegisterMessages() override; @@ -120,9 +131,14 @@ class AppLauncherHandler // page_index]. void HandleGenerateAppForLink(const base::ListValue* args); - // Other registered message callbacks with unused |args|. - void StopShowingAppLauncherPromo(const base::ListValue* args); - void OnLearnMore(const base::ListValue* args); + // Handles "stopShowingAppLauncherPromo" message with unused |args|. + void HandleStopShowingAppLauncherPromo(const base::ListValue* args); + + // Handles "learnMore" message with unused |args|. + void HandleOnLearnMore(const base::ListValue* args); + + // Handles "pageSelected" message with |args| containing [page_index]. + void HandlePageSelected(const base::ListValue* args); private: struct AppInstallInfo { diff --git a/chrome/browser/ui/webui/ntp/new_tab_page_handler.cc b/chrome/browser/ui/webui/ntp/new_tab_page_handler.cc deleted file mode 100644 index 4cfd6e3..0000000 --- a/chrome/browser/ui/webui/ntp/new_tab_page_handler.cc +++ /dev/null @@ -1,61 +0,0 @@ -// Copyright (c) 2012 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "chrome/browser/ui/webui/ntp/new_tab_page_handler.h" - -#include "base/bind.h" -#include "base/bind_helpers.h" -#include "base/memory/scoped_ptr.h" -#include "chrome/browser/browser_process.h" -#include "chrome/browser/profiles/profile.h" -#include "chrome/browser/ui/webui/ntp/new_tab_ui.h" -#include "chrome/common/pref_names.h" -#include "components/browser_sync/browser/profile_sync_service.h" -#include "components/pref_registry/pref_registry_syncable.h" -#include "components/prefs/pref_service.h" -#include "content/public/browser/web_ui.h" - -NewTabPageHandler::NewTabPageHandler() : page_switch_count_(0) { -} - -NewTabPageHandler::~NewTabPageHandler() {} - -void NewTabPageHandler::RegisterMessages() { - web_ui()->RegisterMessageCallback("pageSelected", - base::Bind(&NewTabPageHandler::HandlePageSelected, - base::Unretained(this))); -} - -void NewTabPageHandler::HandlePageSelected(const base::ListValue* args) { - page_switch_count_++; - - double page_id_double; - CHECK(args->GetDouble(0, &page_id_double)); - int page_id = static_cast(page_id_double); - - double index_double; - CHECK(args->GetDouble(1, &index_double)); - int index = static_cast(index_double); - - PrefService* prefs = Profile::FromWebUI(web_ui())->GetPrefs(); - prefs->SetInteger(prefs::kNtpShownPage, page_id | index); -} - -// static -void NewTabPageHandler::RegisterProfilePrefs( - user_prefs::PrefRegistrySyncable* registry) { - // TODO(estade): should be syncable. - registry->RegisterIntegerPref(prefs::kNtpShownPage, APPS_PAGE_ID); -} - -// static -void NewTabPageHandler::GetLocalizedValues(Profile* profile, - base::DictionaryValue* values) { - values->SetInteger("apps_page_id", APPS_PAGE_ID); - - PrefService* prefs = profile->GetPrefs(); - int shown_page = prefs->GetInteger(prefs::kNtpShownPage); - values->SetInteger("shown_page_type", shown_page & ~INDEX_MASK); - values->SetInteger("shown_page_index", shown_page & INDEX_MASK); -} diff --git a/chrome/browser/ui/webui/ntp/new_tab_page_handler.h b/chrome/browser/ui/webui/ntp/new_tab_page_handler.h deleted file mode 100644 index 715b25e..0000000 --- a/chrome/browser/ui/webui/ntp/new_tab_page_handler.h +++ /dev/null @@ -1,64 +0,0 @@ -// Copyright (c) 2012 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#ifndef CHROME_BROWSER_UI_WEBUI_NTP_NEW_TAB_PAGE_HANDLER_H_ -#define CHROME_BROWSER_UI_WEBUI_NTP_NEW_TAB_PAGE_HANDLER_H_ - -#include - -#include "base/macros.h" -#include "base/memory/weak_ptr.h" -#include "base/values.h" -#include "content/public/browser/web_ui_message_handler.h" - -class PrefRegistrySimple; -class Profile; - -namespace user_prefs { -class PrefRegistrySyncable; -} - -// Handler for general New Tab Page functionality that does not belong in a -// more specialized handler. -class NewTabPageHandler : public content::WebUIMessageHandler, - public base::SupportsWeakPtr { - public: - NewTabPageHandler(); - - // Register NTP per-profile preferences. - static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry); - - // Registers values (strings etc.) for the page. - static void GetLocalizedValues(Profile* profile, - base::DictionaryValue* values); - - private: - ~NewTabPageHandler() override; - - // WebUIMessageHandler implementation. - void RegisterMessages() override; - - // Callback for "pageSelected". - void HandlePageSelected(const base::ListValue* args); - - // Tracks the number of times the user has switches pages (for UMA). - size_t page_switch_count_; - - // The purpose of this enum is to track which page on the NTP is showing. - // The lower 10 bits of kNtpShownPage are used for the index within the page - // group, and the rest of the bits are used for the page group ID (defined - // here). - static const int kPageIdOffset = 10; - enum { - INDEX_MASK = (1 << kPageIdOffset) - 1, - APPS_PAGE_ID = 2 << kPageIdOffset, - LAST_PAGE_ID = APPS_PAGE_ID, - }; - static const int kHistogramEnumerationMax = - (LAST_PAGE_ID >> kPageIdOffset) + 1; - - DISALLOW_COPY_AND_ASSIGN(NewTabPageHandler); -}; - -#endif // CHROME_BROWSER_UI_WEBUI_NTP_NEW_TAB_PAGE_HANDLER_H_ diff --git a/chrome/browser/ui/webui/ntp/new_tab_ui.cc b/chrome/browser/ui/webui/ntp/new_tab_ui.cc index 9fc2d3b..cb1bdfc 100644 --- a/chrome/browser/ui/webui/ntp/new_tab_ui.cc +++ b/chrome/browser/ui/webui/ntp/new_tab_ui.cc @@ -17,7 +17,6 @@ #include "chrome/browser/ui/webui/ntp/app_launcher_handler.h" #include "chrome/browser/ui/webui/ntp/core_app_launcher_handler.h" #include "chrome/browser/ui/webui/ntp/favicon_webui_handler.h" -#include "chrome/browser/ui/webui/ntp/new_tab_page_handler.h" #include "chrome/browser/ui/webui/ntp/new_tab_page_sync_handler.h" #include "chrome/browser/ui/webui/ntp/ntp_resource_cache.h" #include "chrome/browser/ui/webui/ntp/ntp_resource_cache_factory.h" @@ -93,7 +92,6 @@ NewTabUI::NewTabUI(content::WebUI* web_ui) if (!profile->IsOffTheRecord()) { web_ui->AddMessageHandler(new MetricsHandler()); web_ui->AddMessageHandler(new FaviconWebUIHandler()); - web_ui->AddMessageHandler(new NewTabPageHandler()); web_ui->AddMessageHandler(new CoreAppLauncherHandler()); web_ui->AddMessageHandler(new NewTabPageSyncHandler()); @@ -208,7 +206,7 @@ void NewTabUI::OnShowBookmarkBarChanged() { void NewTabUI::RegisterProfilePrefs( user_prefs::PrefRegistrySyncable* registry) { CoreAppLauncherHandler::RegisterProfilePrefs(registry); - NewTabPageHandler::RegisterProfilePrefs(registry); + AppLauncherHandler::RegisterProfilePrefs(registry); } // static diff --git a/chrome/browser/ui/webui/ntp/ntp_resource_cache.cc b/chrome/browser/ui/webui/ntp/ntp_resource_cache.cc index 48064ef..5161f9b 100644 --- a/chrome/browser/ui/webui/ntp/ntp_resource_cache.cc +++ b/chrome/browser/ui/webui/ntp/ntp_resource_cache.cc @@ -30,7 +30,7 @@ #include "chrome/browser/ui/bookmarks/bookmark_bar_constants.h" #include "chrome/browser/ui/sync/sync_promo_ui.h" #include "chrome/browser/ui/webui/app_launcher_login_handler.h" -#include "chrome/browser/ui/webui/ntp/new_tab_page_handler.h" +#include "chrome/browser/ui/webui/ntp/app_launcher_handler.h" #include "chrome/browser/ui/webui/ntp/new_tab_ui.h" #include "chrome/common/chrome_switches.h" #include "chrome/common/pref_names.h" @@ -479,7 +479,7 @@ void NTPResourceCache::CreateNewTabHTML() { load_time_data.SetBoolean("canShowAppInfoDialog", CanShowAppInfoDialog()); - NewTabPageHandler::GetLocalizedValues(profile_, &load_time_data); + AppLauncherHandler::GetLocalizedValues(profile_, &load_time_data); AppLauncherLoginHandler::GetLocalizedValues(profile_, &load_time_data); webui::SetLoadTimeDataDefaults(app_locale, &load_time_data); diff --git a/chrome/chrome_browser_ui.gypi b/chrome/chrome_browser_ui.gypi index a5cca15..6bd841f 100644 --- a/chrome/chrome_browser_ui.gypi +++ b/chrome/chrome_browser_ui.gypi @@ -1880,8 +1880,6 @@ 'browser/ui/webui/ntp/core_app_launcher_handler.h', 'browser/ui/webui/ntp/favicon_webui_handler.cc', 'browser/ui/webui/ntp/favicon_webui_handler.h', - 'browser/ui/webui/ntp/new_tab_page_handler.cc', - 'browser/ui/webui/ntp/new_tab_page_handler.h', 'browser/ui/webui/ntp/new_tab_page_sync_handler.cc', 'browser/ui/webui/ntp/new_tab_page_sync_handler.h', 'browser/ui/webui/ntp/new_tab_ui.cc', -- cgit v1.1