NTP Zombie Code Slayer Part IV: Most Visited

NTP Zombie Code Slayer Part IV: Most Visited

This is where things get complicated.

NTP4 had a most visited pane.  It showed your top sites.
So does the current NTP; so I'm trying to remove everything
that deals with most visited tiles on NTP4 without accidentally
breaking the same named thing on the current NTP.

Here's what used to exist that I'm trying to remove:
http://bostandjiev.com/Content/Google/chrome.jpg

Here's what currently exists that I'm trying not to break:
http://cdn.ghacks.net/wp-content/uploads/2013/04/chrome-new-tab-page.jpg

BUG=329637
Committed: https://crrev.com/8d57a0152613119d7e6369838fd5838fb7773e68
Cr-Commit-Position: refs/heads/master@{#330910}

[Dan Beam, 2015-05-16 01:35:24]

dbeam@chromium.org changed reviewers:
+ huangs@chromium.org

[Dan Beam, 2015-05-16 01:37:47]

depends on episode III:
https://codereview.chromium.org/1140813003/

huangs@: chrome/browser/search (and as FYI)
estade@: everything else

[Dan Beam, 2015-05-16 01:53:14]

dbeam@chromium.org changed reviewers:
+ samarth@chromium.org
- huangs@chromium.org

[Dan Beam, 2015-05-16 01:54:33]

actually, -huangs@, +samarth@ for fuller approval (and huangs@ is out on Monday)

[samarth, 2015-05-18 16:41:53]

lgtm for c/b/search

https://codereview.chromium.org/1129903008/diff/1/chrome/browser/search/thumb...
File chrome/browser/search/thumbnail_source.cc (right):

https://codereview.chromium.org/1129903008/diff/1/chrome/browser/search/thumb...
chrome/browser/search/thumbnail_source.cc:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
nit: Does the date need to be update for a plain move?

[Dan Beam, 2015-05-18 17:45:22]

dbeam@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Dan Beam, 2015-05-18 17:45:24]

+rdevlin.cronin@ for chrome/browser/extensions

https://codereview.chromium.org/1129903008/diff/1/chrome/browser/search/thumb...
File chrome/browser/search/thumbnail_source.cc (right):

https://codereview.chromium.org/1129903008/diff/1/chrome/browser/search/thumb...
chrome/browser/search/thumbnail_source.cc:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
On 2015/05/18 16:41:52, samarth wrote:
> nit: Does the date need to be update for a plain move?

nah, it doesn't.  reverted.

[Dan Beam, 2015-05-18 17:51:23]

dbeam@chromium.org changed reviewers:
+ sky@chromium.org

[Dan Beam, 2015-05-18 17:51:25]

+sky@ for chrome/test/base macro removal (now unused)

[Dan Beam, 2015-05-18 19:55:15]

dbeam@chromium.org changed reviewers:
- estade@chromium.org

[Dan Beam, 2015-05-18 19:56:12]

dbeam@chromium.org changed reviewers:
+ xiyuan@chromium.org

[Dan Beam, 2015-05-18 19:57:25]

-estade@, +xiyuan@ for chrome/browser/resources, chrome/browser/ui/webui

[xiyuan, 2015-05-18 20:29:13]

https://codereview.chromium.org/1129903008/diff/40001/chrome/browser/resource...
File chrome/browser/resources/ntp4/page_list_view.js (right):

https://codereview.chromium.org/1129903008/diff/40001/chrome/browser/resource...
chrome/browser/resources/ntp4/page_list_view.js:172:
assert(loadTimeData.getBoolean('showApps'));
ChromeOS (and Win8 metro) does not show apps page because there is an ash app
list. Probably should keep the "if (apps)".

[sky, 2015-05-18 21:36:51]

chrome/test/base/web_ui_browser_test.h LGTM

[Devlin, 2015-05-19 15:56:14]

chrome/browser/extensions/extension_service.cc lgtm.  Feel free to TBR simple
#include changes, if you like. :)

[Dan Beam, 2015-05-19 17:11:12]

https://codereview.chromium.org/1129903008/diff/40001/chrome/browser/resource...
File chrome/browser/resources/ntp4/page_list_view.js (right):

https://codereview.chromium.org/1129903008/diff/40001/chrome/browser/resource...
chrome/browser/resources/ntp4/page_list_view.js:172:
assert(loadTimeData.getBoolean('showApps'));
On 2015/05/18 20:29:12, xiyuan wrote:
> ChromeOS (and Win8 metro) does not show apps page because there is an ash app
> list. Probably should keep the "if (apps)".

nah, this code can't event run on ChromeOS.  ChromeOS-specific code is next up
on the chopping block.

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

[xiyuan, 2015-05-19 17:13:01]

lgtm

https://codereview.chromium.org/1129903008/diff/40001/chrome/browser/resource...
File chrome/browser/resources/ntp4/page_list_view.js (right):

https://codereview.chromium.org/1129903008/diff/40001/chrome/browser/resource...
chrome/browser/resources/ntp4/page_list_view.js:172:
assert(loadTimeData.getBoolean('showApps'));
On 2015/05/19 17:11:12, Dan Beam wrote:
> On 2015/05/18 20:29:12, xiyuan wrote:
> > ChromeOS (and Win8 metro) does not show apps page because there is an ash
app
> > list. Probably should keep the "if (apps)".
> 
> nah, this code can't event run on ChromeOS.  ChromeOS-specific code is next up
> on the chopping block.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

Okay. Thanks for double checking.

[Dan Beam, 2015-05-19 21:25:46]

dbeam@chromium.org changed reviewers:
+ gab@chromium.org

[Dan Beam, 2015-05-19 21:25:47]

+gab@ for components/pref_registry DEPS addition to chrome/browser/history/DEPS

[sky, 2015-05-19 21:33:36]

history changes LGTM - Can you elaborate on why the pages get most visited in a
different way?

[Dan Beam, 2015-05-19 21:51:31]

On 2015/05/19 21:33:36, sky wrote:
> history changes LGTM - Can you elaborate on why the pages get most visited in
a
> different way?

Done.

https://codereview.chromium.org/1129903008/diff/80001/chrome/browser/history/...
File chrome/browser/history/top_sites_factory.cc (right):

https://codereview.chromium.org/1129903008/diff/80001/chrome/browser/history/...
chrome/browser/history/top_sites_factory.cc:94:
prefs::kNtpMostVisitedURLsBlacklist, prepopulated_page_list,
this is the only class that needs to know how blacklisting works

https://codereview.chromium.org/1129903008/diff/80001/chrome/browser/search/i...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/1129903008/diff/80001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:170: top_sites->AddBlacklistedURL(url);
both pages interact with the blacklist in the same way

https://codereview.chromium.org/1129903008/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/most_visited_handler.cc (left):

https://codereview.chromium.org/1129903008/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/most_visited_handler.cc:282:
ts->AddBlacklistedURL(url);
using history::TopSites::*Blacklist* functions and
TopSitesFactory::GetForProfile()

https://codereview.chromium.org/1129903008/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/most_visited_handler.cc:293:
registry->RegisterDictionaryPref(prefs::kNtpMostVisitedURLsBlacklist);
but topsites was counting on NTP4 to register a pref for it

[Dan Beam, 2015-05-19 22:11:47]

The CQ bit was checked by dbeam@chromium.org

[Dan Beam, 2015-05-19 22:11:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
samarth@chromium.org, xiyuan@chromium.org, rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1129903008/#ps80001
(title: "pref registration")

[Dan Beam, 2015-05-19 22:12:06]

The CQ bit was unchecked by dbeam@chromium.org

[beaudoin, 2015-05-20 18:53:06]

On 2015/05/19 21:51:31, Dan Beam wrote:
> On 2015/05/19 21:33:36, sky wrote:
> > history changes LGTM - Can you elaborate on why the pages get most visited
in
> a
> > different way?
> 
> Done.
> 
>
https://codereview.chromium.org/1129903008/diff/80001/chrome/browser/history/...
> File chrome/browser/history/top_sites_factory.cc (right):
> 
>
https://codereview.chromium.org/1129903008/diff/80001/chrome/browser/history/...
> chrome/browser/history/top_sites_factory.cc:94:
> prefs::kNtpMostVisitedURLsBlacklist, prepopulated_page_list,
> this is the only class that needs to know how blacklisting works
> 
>
https://codereview.chromium.org/1129903008/diff/80001/chrome/browser/search/i...
> File chrome/browser/search/instant_service.cc (right):
> 
>
https://codereview.chromium.org/1129903008/diff/80001/chrome/browser/search/i...
> chrome/browser/search/instant_service.cc:170:
top_sites->AddBlacklistedURL(url);
> both pages interact with the blacklist in the same way
> 
>
https://codereview.chromium.org/1129903008/diff/80001/chrome/browser/ui/webui...
> File chrome/browser/ui/webui/ntp/most_visited_handler.cc (left):
> 
>
https://codereview.chromium.org/1129903008/diff/80001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/ntp/most_visited_handler.cc:282:
> ts->AddBlacklistedURL(url);
> using history::TopSites::*Blacklist* functions and
> TopSitesFactory::GetForProfile()
> 
>
https://codereview.chromium.org/1129903008/diff/80001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/ntp/most_visited_handler.cc:293:
> registry->RegisterDictionaryPref(prefs::kNtpMostVisitedURLsBlacklist);
> but topsites was counting on NTP4 to register a pref for it

Just a quick drive-by, you should make sure your clean-up doesn't break:
  chrome-search://local-ntp/local-ntp.html
which is still used as a fallback for some users.

[gab, 2015-05-20 18:58:49]

https://codereview.chromium.org/1129903008/diff/80001/chrome/browser/history/...
File chrome/browser/history/top_sites_factory.h (right):

https://codereview.chromium.org/1129903008/diff/80001/chrome/browser/history/...
chrome/browser/history/top_sites_factory.h:50: void RegisterProfilePrefs(
Where is this called from? I don't see any new callers in this CL?

[Dan Beam, 2015-05-20 19:16:00]

https://codereview.chromium.org/1129903008/diff/80001/chrome/browser/history/...
File chrome/browser/history/top_sites_factory.h (right):

https://codereview.chromium.org/1129903008/diff/80001/chrome/browser/history/...
chrome/browser/history/top_sites_factory.h:50: void RegisterProfilePrefs(
On 2015/05/20 18:58:49, gab wrote:
> Where is this called from? I don't see any new callers in this CL?

note the "override"

https://code.google.com/p/chromium/codesearch#chromium/src/components/keyed_s...

[gab, 2015-05-20 19:18:07]

On 2015/05/20 19:16:00, Dan Beam wrote:
>
https://codereview.chromium.org/1129903008/diff/80001/chrome/browser/history/...
> File chrome/browser/history/top_sites_factory.h (right):
> 
>
https://codereview.chromium.org/1129903008/diff/80001/chrome/browser/history/...
> chrome/browser/history/top_sites_factory.h:50: void RegisterProfilePrefs(
> On 2015/05/20 18:58:49, gab wrote:
> > Where is this called from? I don't see any new callers in this CL?
> 
> note the "override"
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/keyed_s...

Ah, sorry, lgtm

[Dan Beam, 2015-05-20 21:28:30]

The CQ bit was checked by dbeam@chromium.org

[Dan Beam, 2015-05-20 21:28:30]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
rdevlin.cronin@chromium.org, samarth@chromium.org, sky@chromium.org,
gab@chromium.org
Link to the patchset: https://codereview.chromium.org/1129903008/#ps100001
(title: "asdf")

[commit-bot: I haz the power, 2015-05-20 21:29:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1129903008/100001

[commit-bot: I haz the power, 2015-05-20 23:43:24]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-05-20 23:44:21]

Patchset 6 (id:??) landed as
https://crrev.com/1fc345cd960f7efd4e6b07d1f050615239d82d60
Cr-Commit-Position: refs/heads/master@{#330845}

[Dan Beam, 2015-05-21 02:34:29]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1151563005/ by dbeam@chromium.org.

The reason for reverting is: Duplicate pref registration on Android somehow..

[Dan Beam, 2015-05-21 04:20:10]

dbeam@chromium.org changed reviewers:
+ newt@chromium.org

[Dan Beam, 2015-05-21 04:20:11]

+newt for chrome/browser/android

[Yaron, 2015-05-21 04:24:39]

yfriedman@chromium.org changed reviewers:
+ yfriedman@chromium.org

[Yaron, 2015-05-21 04:24:41]

lgtm

[Dan Beam, 2015-05-21 07:08:11]

The CQ bit was checked by dbeam@chromium.org

[Dan Beam, 2015-05-21 07:08:13]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
rdevlin.cronin@chromium.org, samarth@chromium.org, sky@chromium.org,
gab@chromium.org
Link to the patchset: https://codereview.chromium.org/1129903008/#ps120001
(title: "android fix")

[commit-bot: I haz the power, 2015-05-21 07:08:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1129903008/120001

[commit-bot: I haz the power, 2015-05-21 07:12:29]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-05-21 07:13:20]

Patchset 7 (id:??) landed as
https://crrev.com/8d57a0152613119d7e6369838fd5838fb7773e68
Cr-Commit-Position: refs/heads/master@{#330910}