Topsites become keyedService based.

Topsites become keyedService based.

TopSites was created and owned by profile.

This patch creates a TopSitesServicefactory which is keyedService.
So now instead of calling GetTopSites() from profile, can get the TopSites from
TopSitesFactory.

BUG=435501
Committed: https://crrev.com/30f03390995328247e3642aa5cd2cc9aae781b45
Cr-Commit-Position: refs/heads/master@{#313468}

[Jitu( very slow this week), 2014-12-19 12:58:05]

jitendra.ks@samsung.com changed reviewers:
+ sdefresne@chromium.org

[Jitu( very slow this week), 2014-12-19 12:58:06]

PTAL

Is this you want to be. Please let me know any suggestions.

Thanks

[sdefresne, 2014-12-19 15:11:42]

I've made lots of comments, but I still think this is going into the right
direction.

The CL contains some noise due to renaming TopSites to TopSitesProvider and
TopSitesImpl to TopSitesService. Can you revert the renaming to make the change
smaller and easier to review?

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/c...
File chrome/browser/history/chrome_history_client_factory.cc (right):

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/c...
chrome/browser/history/chrome_history_client_factory.cc:38:
DependsOn(BookmarkModelFactory::GetInstance());
You have to add an explicit dependency on TopSitesServiceFactory here so that
the KeyedService are build/destroyed in correct order. Please also add that
dependency to the factories of all other KeyedService that ends up using
TopSitesServiceFactory.

DependsOn(TopSitesServiceFactory::GetInstance());

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/h...
File chrome/browser/history/history_backend.cc (left):

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_backend.cc:33: #include
"chrome/browser/history/top_sites.h"
Looks like it is unused, remove.

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/h...
File chrome/browser/history/history_service.cc (left):

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_service.cc:39: #include
"chrome/browser/history/top_sites.h"
Looks like it is unused, remove.

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
File chrome/browser/history/top_sites_database.cc (right):

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
chrome/browser/history/top_sites_database.cc:12: #include
"chrome/browser/history/top_sites_provider.h"
Looks like it is unused, remove.

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
File chrome/browser/history/top_sites_provider.cc (right):

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
chrome/browser/history/top_sites_provider.cc:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
Do not change copyright notice when renaming files.

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
chrome/browser/history/top_sites_provider.cc:2: // Use of this source code is
governed by a BSD-style license that can be
Why is this file not marked as moved but as new?

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
chrome/browser/history/top_sites_provider.cc:34: TopSitesProvider*
TopSitesProvider::Create(Profile* profile,
Remove this method, it should instead be the responsibility of the
TopSitesServiceFactory.

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
chrome/browser/history/top_sites_provider.cc:41:
TopSitesProvider::TopSitesProvider() {
You define the constructor in the header and the implementation files. This
won't work.

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
File chrome/browser/history/top_sites_provider.h (left):

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
chrome/browser/history/top_sites_provider.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
Do not change copyright notice when renaming files.

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
chrome/browser/history/top_sites_provider.h:38: class TopSites
This class must be the KeyedService, not TopSitesService.

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
File chrome/browser/history/top_sites_provider.h (right):

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
chrome/browser/history/top_sites_provider.h:38: class TopSitesProvider : public
base::RefCountedThreadSafe<TopSitesProvider>,
Why did you rename this class from TopSites to TopSitesProvider. I don't see the
benefit. If you really think this make the code easier to understand, please do
it as a separate change list to keep this one manageable.

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
chrome/browser/history/top_sites_provider.h:38: class TopSitesProvider : public
base::RefCountedThreadSafe<TopSitesProvider>,
Why did you rename this class from TopSites to TopSitesProvider. I don't see the
benefit. If you really think this make the code easier to understand, please do
it as a separate change list to keep this one manageable.

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
chrome/browser/history/top_sites_provider.h:41: TopSitesProvider() {}
Do not inline complex constructors.

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
File chrome/browser/history/top_sites_service.h (left):

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
chrome/browser/history/top_sites_service.h:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
Do not change copyright notice when moving files.

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
File chrome/browser/history/top_sites_service.h (right):

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
chrome/browser/history/top_sites_service.h:49: class TopSitesService : public
KeyedService, public TopSitesProvider {
TopSitesImpl should not be the KeyedService, it should be TopSites.

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
File chrome/browser/history/top_sites_service_factory.cc (right):

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
chrome/browser/history/top_sites_service_factory.cc:28: "TopsitesService",
s/TopsitesService/TopSitesService/

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
chrome/browser/history/top_sites_service_factory.cc:37: TopSitesService*
provider = NULL;
s/NULL/nullptr/

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
File chrome/browser/history/top_sites_service_factory.h (right):

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
chrome/browser/history/top_sites_service_factory.h:8: #include
"base/memory/singleton.h"
You can remove this include and instead forward-declare DefaultSingletonTraits:

template <typename T>
struct DefaultSingletonTraits;

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
chrome/browser/history/top_sites_service_factory.h:9: #include
"chrome/browser/history/top_sites_service.h"
TopSitesImpl is an implementation details and client code should still use
TopSites (or TopSitesProvider as you called it), so instead include
"chrome/browser/history/top_sites_provider.h".

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
chrome/browser/history/top_sites_service_factory.h:14: // Topsites.
s/Topsites/TopSites/ (or TopSitesProvider).

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
chrome/browser/history/top_sites_service_factory.h:15: class
TopSitesServiceFactory : public BrowserContextKeyedServiceFactory {
Since TopSitesProvider is a base::RefCountedThreadSafe, you need to make this
factory a RefcountedBrowserContextKeyedServiceFactory.

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
chrome/browser/history/top_sites_service_factory.h:18: static
history::TopSitesService* GetForProfile(Profile* profile);
s/TopSitesService/TopSitesProvider/

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
chrome/browser/history/top_sites_service_factory.h:19: 
You'll need another method GetForProfileIfExists() to mirror the
Profile::GetTopSitesWithoutCreating() method.

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
chrome/browser/history/top_sites_service_factory.h:28: // virtual
content::BrowserContext* GetBrowserContextToUse(
Dead-code, remove.

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/profiles/...
File chrome/browser/profiles/profile.h (right):

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/profiles/...
chrome/browser/profiles/profile.h:60: namespace history {
Remove that forward-declaration.

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:41: #include
"chrome/browser/history/top_sites_provider.h"
Remove those three includes.

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:827: // if (top_sites_service_.get())
Dead code, remove.

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.h (right):

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.h:257: // scoped_refptr<history::TopSites>
top_sites_;  // For history and thumbnails.
Dead code, remove.

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/ntp/suggestions_source_top_sites.cc (right):

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/ntp/suggestions_source_top_sites.cc:14: #include
"chrome/browser/history/top_sites_provider.h"
Appears to be unused, remove.

https://codereview.chromium.org/815983002/diff/20001/chrome/test/base/testing...
File chrome/test/base/testing_profile.cc (left):

https://codereview.chromium.org/815983002/diff/20001/chrome/test/base/testing...
chrome/test/base/testing_profile.cc:574: top_sites_ = history::TopSites::Create(
Remove top_sites_ property from TestingProfile() and instead register a custom
testing factory here, see
RefcountedBrowserContextKeyedServiceFactory::SetTestingFactory().

https://codereview.chromium.org/815983002/diff/20001/chrome/test/base/testing...
chrome/test/base/testing_profile.cc:578: void
TestingProfile::SetTopSites(history::TopSites* top_sites) {
You'll have to remove this method and instead change the only caller (in
chrome/browser/autocomplete/zero_suggest_provider_unittest.cc) to set a custom
factory for TopSitesServiceFactory, see
RefcountedBrowserContextKeyedServiceFactory::SetTestingFactory().

https://codereview.chromium.org/815983002/diff/20001/chrome/test/base/testing...
chrome/test/base/testing_profile.cc:797: history::TopSites*
TestingProfile::GetTopSites() {
Remove.

https://codereview.chromium.org/815983002/diff/20001/chrome/test/base/testing...
chrome/test/base/testing_profile.cc:801: history::TopSites*
TestingProfile::GetTopSitesWithoutCreating() {
Remove.

[Jitu( very slow this week), 2014-12-23 09:28:56]

Patchset #3 (id:40001) has been deleted

[Jitu( very slow this week), 2014-12-23 09:31:19]

Patchset #3 (id:60001) has been deleted

[Jitu( very slow this week), 2014-12-23 10:44:49]

Thanks ... 
I am taking care all your comments

Just some doubt

chrome/browser/history/top_sites_service_factory.h:15: class
TopSitesServiceFactory : public BrowserContextKeyedServiceFactory {
Since TopSitesProvider is a base::RefCountedThreadSafe, you need to make this
factory a RefcountedBrowserContextKeyedServiceFactory.

Q1.
So here i have to make this factory as
RefcountedBrowserContextKeyedServiceFactory.

Q2.
As TopSitesImpl is type TopSites so TopSitesImpl should not be keyedService.
So now i need to make TopSites as keyedService. right ?? Or
RefcountedKeyedService .

Can you please make me understand this.

[sdefresne, 2014-12-23 10:48:02]

On 2014/12/23 at 10:44:49, jitendra.ks wrote:
> Thanks ... 
> I am taking care all your comments
> 
> Just some doubt
> 
> chrome/browser/history/top_sites_service_factory.h:15: class
> TopSitesServiceFactory : public BrowserContextKeyedServiceFactory {
> Since TopSitesProvider is a base::RefCountedThreadSafe, you need to make this
> factory a RefcountedBrowserContextKeyedServiceFactory.
> 
> Q1.
> So here i have to make this factory as
RefcountedBrowserContextKeyedServiceFactory.

Yes.

> Q2.
> As TopSitesImpl is type TopSites so TopSitesImpl should not be keyedService.
> So now i need to make TopSites as keyedService. right ?? Or
RefcountedKeyedService .

Yes, TopSites must inherit from RefcountedKeyedService (and you can remove the
inheritance from base::RefCountedThreadSafe<TopSites> since
RecountedKeyedService already inherit from base::RefCountedThreadSafe).

> Can you please make me understand this.

[Jitu( very slow this week), 2014-12-24 13:10:18]

Patchset #5 (id:120001) has been deleted

[Jitu( very slow this week), 2014-12-24 13:31:39]

Patchset #6 (id:160001) has been deleted

[Jitu( very slow this week), 2014-12-24 14:50:22]

Patchset #5 (id:140001) has been deleted

[Jitu( very slow this week), 2014-12-24 15:03:24]

Patchset #5 (id:180001) has been deleted

[Jitu( very slow this week), 2014-12-26 06:20:46]

Patchset #5 (id:190001) has been deleted

[Jitu( very slow this week), 2014-12-26 06:21:07]

Patchset #4 (id:100001) has been deleted

[Jitu( very slow this week), 2014-12-26 06:53:16]

Patchset #5 (id:220001) has been deleted

[Jitu( very slow this week), 2014-12-26 07:31:16]

Patchset #5 (id:240001) has been deleted

[Jitu( very slow this week), 2014-12-26 08:05:18]

Patchset #5 (id:260001) has been deleted

[Jitu( very slow this week), 2014-12-26 08:57:55]

Patchset #5 (id:280001) has been deleted

[Jitu( very slow this week), 2014-12-26 09:05:28]

Patchset #5 (id:300001) has been deleted

[Jitu( very slow this week), 2014-12-26 12:37:16]

Patchset #4 (id:170048) has been deleted

[Jitu( very slow this week), 2014-12-26 14:04:29]

PTAL

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/c...
File chrome/browser/history/chrome_history_client_factory.cc (right):

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/c...
chrome/browser/history/chrome_history_client_factory.cc:38:
DependsOn(BookmarkModelFactory::GetInstance());
On 2014/12/19 15:11:40, sdefresne wrote:
> You have to add an explicit dependency on TopSitesServiceFactory here so that
> the KeyedService are build/destroyed in correct order. Please also add that
> dependency to the factories of all other KeyedService that ends up using
> TopSitesServiceFactory.
> 
> DependsOn(TopSitesServiceFactory::GetInstance());

Done.

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/h...
File chrome/browser/history/history_backend.cc (left):

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_backend.cc:33: #include
"chrome/browser/history/top_sites.h"
On 2014/12/19 15:11:40, sdefresne wrote:
> Looks like it is unused, remove.

Done.

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/h...
File chrome/browser/history/history_service.cc (left):

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_service.cc:39: #include
"chrome/browser/history/top_sites.h"
On 2014/12/19 15:11:40, sdefresne wrote:
> Looks like it is unused, remove.

Done.

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
File chrome/browser/history/top_sites_database.cc (right):

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
chrome/browser/history/top_sites_database.cc:12: #include
"chrome/browser/history/top_sites_provider.h"
On 2014/12/19 15:11:40, sdefresne wrote:
> Looks like it is unused, remove.

Done.

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
File chrome/browser/history/top_sites_service.h (right):

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
chrome/browser/history/top_sites_service.h:49: class TopSitesService : public
KeyedService, public TopSitesProvider {
On 2014/12/19 15:11:41, sdefresne wrote:
> TopSitesImpl should not be the KeyedService, it should be TopSites.

Done.

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
File chrome/browser/history/top_sites_service_factory.cc (right):

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
chrome/browser/history/top_sites_service_factory.cc:28: "TopsitesService",
On 2014/12/19 15:11:41, sdefresne wrote:
> s/TopsitesService/TopSitesService/

Done.

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
chrome/browser/history/top_sites_service_factory.cc:37: TopSitesService*
provider = NULL;
On 2014/12/19 15:11:41, sdefresne wrote:
> s/NULL/nullptr/

Done.

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
File chrome/browser/history/top_sites_service_factory.h (right):

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
chrome/browser/history/top_sites_service_factory.h:8: #include
"base/memory/singleton.h"
On 2014/12/19 15:11:41, sdefresne wrote:
> You can remove this include and instead forward-declare
DefaultSingletonTraits:
> 
> template <typename T>
> struct DefaultSingletonTraits;

Done.

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
chrome/browser/history/top_sites_service_factory.h:9: #include
"chrome/browser/history/top_sites_service.h"
On 2014/12/19 15:11:41, sdefresne wrote:
> TopSitesImpl is an implementation details and client code should still use
> TopSites (or TopSitesProvider as you called it), so instead include
> "chrome/browser/history/top_sites_provider.h".

Done.

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
chrome/browser/history/top_sites_service_factory.h:14: // Topsites.
On 2014/12/19 15:11:42, sdefresne wrote:
> s/Topsites/TopSites/ (or TopSitesProvider).

Done.

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
chrome/browser/history/top_sites_service_factory.h:14: // Topsites.
On 2014/12/19 15:11:42, sdefresne wrote:
> s/Topsites/TopSites/ (or TopSitesProvider).

Done.

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
chrome/browser/history/top_sites_service_factory.h:15: class
TopSitesServiceFactory : public BrowserContextKeyedServiceFactory {
On 2014/12/19 15:11:42, sdefresne wrote:
> Since TopSitesProvider is a base::RefCountedThreadSafe, you need to make this
> factory a RefcountedBrowserContextKeyedServiceFactory.

Done.

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
chrome/browser/history/top_sites_service_factory.h:19: 
On 2014/12/19 15:11:42, sdefresne wrote:
> You'll need another method GetForProfileIfExists() to mirror the
> Profile::GetTopSitesWithoutCreating() method.

Done.

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/history/t...
chrome/browser/history/top_sites_service_factory.h:28: // virtual
content::BrowserContext* GetBrowserContextToUse(
On 2014/12/19 15:11:41, sdefresne wrote:
> Dead-code, remove.

Done.

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/profiles/...
File chrome/browser/profiles/profile.h (right):

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/profiles/...
chrome/browser/profiles/profile.h:60: namespace history {
On 2014/12/19 15:11:42, sdefresne wrote:
> Remove that forward-declaration.

Done.

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:41: #include
"chrome/browser/history/top_sites_provider.h"
On 2014/12/19 15:11:42, sdefresne wrote:
> Remove those three includes.

Done.

Added for testing the code forgot to remove :(

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:827: // if (top_sites_service_.get())
On 2014/12/19 15:11:42, sdefresne wrote:
> Dead code, remove.

Done.

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.h (right):

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.h:257: // scoped_refptr<history::TopSites>
top_sites_;  // For history and thumbnails.
On 2014/12/19 15:11:42, sdefresne wrote:
> Dead code, remove.

Done.

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/ntp/suggestions_source_top_sites.cc (right):

https://codereview.chromium.org/815983002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/ntp/suggestions_source_top_sites.cc:14: #include
"chrome/browser/history/top_sites_provider.h"
On 2014/12/19 15:11:42, sdefresne wrote:
> Appears to be unused, remove.

Done.

https://codereview.chromium.org/815983002/diff/20001/chrome/test/base/testing...
File chrome/test/base/testing_profile.cc (left):

https://codereview.chromium.org/815983002/diff/20001/chrome/test/base/testing...
chrome/test/base/testing_profile.cc:574: top_sites_ = history::TopSites::Create(
On 2014/12/19 15:11:42, sdefresne wrote:
> Remove top_sites_ property from TestingProfile() and instead register a custom
> testing factory here, see
> RefcountedBrowserContextKeyedServiceFactory::SetTestingFactory().

Done.

https://codereview.chromium.org/815983002/diff/20001/chrome/test/base/testing...
chrome/test/base/testing_profile.cc:578: void
TestingProfile::SetTopSites(history::TopSites* top_sites) {
On 2014/12/19 15:11:42, sdefresne wrote:
> You'll have to remove this method and instead change the only caller (in
> chrome/browser/autocomplete/zero_suggest_provider_unittest.cc) to set a custom
> factory for TopSitesServiceFactory, see
> RefcountedBrowserContextKeyedServiceFactory::SetTestingFactory().

Done.

https://codereview.chromium.org/815983002/diff/20001/chrome/test/base/testing...
chrome/test/base/testing_profile.cc:797: history::TopSites*
TestingProfile::GetTopSites() {
On 2014/12/19 15:11:42, sdefresne wrote:
> Remove.

Done.

https://codereview.chromium.org/815983002/diff/20001/chrome/test/base/testing...
chrome/test/base/testing_profile.cc:801: history::TopSites*
TestingProfile::GetTopSitesWithoutCreating() {
On 2014/12/19 15:11:42, sdefresne wrote:
> Remove.

Done.

[sdefresne, 2014-12-29 09:48:14]

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/android/...
File chrome/browser/android/dev_tools_manager_delegate_android.cc (right):

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/android/...
chrome/browser/android/dev_tools_manager_delegate_android.cc:343: if
(top_sites.get()) {
nit: "if (top_sites)" should work, here and everywhere else.

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/android/...
File chrome/browser/android/most_visited_sites.cc (right):

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.cc:385: if (top_sites.get() == NULL)
nit: "if (!top_sites)" should work here and everywhere else.

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/autocomp...
File chrome/browser/autocomplete/zero_suggest_provider_unittest.cc (right):

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/autocomp...
chrome/browser/autocomplete/zero_suggest_provider_unittest.cc:241:
static_cast<FakeEmptyTopSites*>(top_sites)->mv_callback.Run(urls);
nit: "static_cast<FakeEmptyTopSites*>(top_sites.get())" here and everywhere
else.

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/history/...
File chrome/browser/history/chrome_history_client.h (right):

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/history/...
chrome/browser/history/chrome_history_client.h:49: // The TopSites object is
owned by the Profile (see
You can remove that comment since 1. you're using a scoped_refptr, 2. TopSites
is a KeyedService and the proper dependency is declared between the factories so
the lifetime are enforced.

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/history/...
File chrome/browser/history/top_sites_impl_unittest.cc (right):

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/history/...
chrome/browser/history/top_sites_impl_unittest.cc:239:
TopSitesServiceFactory::GetForProfile(profile_));
nit: TopSitesServiceFactory::GetForProfile(profile_).get()

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/history/...
File chrome/browser/history/top_sites_service_factory.h (right):

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/history/...
chrome/browser/history/top_sites_service_factory.h:17: class
TopSitesServiceFactory
nit: rename this class to TopSitesFactory

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/history/...
chrome/browser/history/top_sites_service_factory.h:30: static
scoped_refptr<RefcountedKeyedService> BuildProfileForTesting(
I don't think those two method needs to be 1. method, 2. part of the public
interface. I would just make them free function in testing_profile.cc and remove
them from there.

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

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:96: scoped_refptr<history::TopSites>
top_sites_provider =
nit: rename top_sites_provider to top_sites

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/thumbnai...
File chrome/browser/thumbnails/thumbnail_service_impl.cc (right):

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_service_impl.cc:49:
ThumbnailServiceImpl::ThumbnailServiceImpl(Profile* profile)
You need to add the dependency on TopSitesServiceFactory to
ThumbnailServiceFactory.

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/thumbnai...
File chrome/browser/thumbnails/thumbnail_service_unittest.cc (right):

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_service_unittest.cc:55: MockProfile() :
mock_top_sites_(new MockTopSites(this)) {
Remove mock_top_sites_ property.

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_service_unittest.cc:58: history::TopSites*
GetTopSites() { return mock_top_sites_.get(); }
Remove this method. Instead use TopSitesServiceFactory::SetTestingFactory() to
register a MockTopSites as the TopSites to use for this MockProfile.

You can probably do this in MockProfile constructor:

MockProfile() {
  TopSitesServiceFactory::SetTestingProfile(this, BuildMockTopSites);
}

with BuildMockTopSites defined as:

scoped_refptr<RefcountedKeyedService> BuildMockTopSites(content::BrowserContext*
context) {
  return scoped_refptr<RefcountedKeyedService>(new
MockTopSites(static_cast<Profile*>(context));
}

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_service_unittest.cc:65:
scoped_refptr<MockTopSites> mock_top_sites_;
Remove this property.

https://codereview.chromium.org/815983002/diff/400001/chrome/test/base/testin...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/815983002/diff/400001/chrome/test/base/testin...
chrome/test/base/testing_profile.cc:592:
&TopSitesServiceFactory::CreateCustomTopSitesServiceFactory(&top_sites));
Please move this testing factory function
(TopSitesServiceFactory::CreateCustomTopSitesServiceFactory) into the anonymous
namespace at the beginning of the file. It does not need to be a static function
of TopSitesServiceFactory, it can just be a free function.

https://codereview.chromium.org/815983002/diff/400001/chrome/test/base/testin...
chrome/test/base/testing_profile.cc:597: this,
&TopSitesServiceFactory::BuildProfileForTesting(this));
Please move this testing factory function
(TopSitesServiceFactory::BuildProfileForTesting) into the anonymous namespace at
the beginning of the file. It does not need to be a static function of
TopSitesServiceFactory, it can just be a free function.

[Jitu( very slow this week), 2014-12-29 15:26:50]

Patchset #9 (id:420001) has been deleted

[Jitu( very slow this week), 2014-12-29 15:35:06]

Patchset #9 (id:440001) has been deleted

[Jitu( very slow this week), 2014-12-30 10:01:23]

Patchset #9 (id:460001) has been deleted

[Jitu( very slow this week), 2014-12-30 10:09:03]

Fixed all the review comments...

PTAL

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/android/...
File chrome/browser/android/dev_tools_manager_delegate_android.cc (right):

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/android/...
chrome/browser/android/dev_tools_manager_delegate_android.cc:343: if
(top_sites.get()) {
On 2014/12/29 09:48:14, sdefresne wrote:
> nit: "if (top_sites)" should work, here and everywhere else.

Done.

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/android/...
File chrome/browser/android/most_visited_sites.cc (right):

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.cc:385: if (top_sites.get() == NULL)
On 2014/12/29 09:48:14, sdefresne wrote:
> nit: "if (!top_sites)" should work here and everywhere else.

Done.

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/autocomp...
File chrome/browser/autocomplete/zero_suggest_provider_unittest.cc (right):

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/autocomp...
chrome/browser/autocomplete/zero_suggest_provider_unittest.cc:241:
static_cast<FakeEmptyTopSites*>(top_sites)->mv_callback.Run(urls);
On 2014/12/29 09:48:14, sdefresne wrote:
> nit: "static_cast<FakeEmptyTopSites*>(top_sites.get())" here and everywhere
> else.

Done.

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/history/...
File chrome/browser/history/chrome_history_client.h (right):

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/history/...
chrome/browser/history/chrome_history_client.h:49: // The TopSites object is
owned by the Profile (see
On 2014/12/29 09:48:14, sdefresne wrote:
> You can remove that comment since 1. you're using a scoped_refptr, 2. TopSites
> is a KeyedService and the proper dependency is declared between the factories
so
> the lifetime are enforced.

Done.

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/history/...
File chrome/browser/history/top_sites_impl_unittest.cc (right):

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/history/...
chrome/browser/history/top_sites_impl_unittest.cc:239:
TopSitesServiceFactory::GetForProfile(profile_));
On 2014/12/29 09:48:14, sdefresne wrote:
> nit: TopSitesServiceFactory::GetForProfile(profile_).get()

Done.

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/history/...
File chrome/browser/history/top_sites_service_factory.h (right):

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/history/...
chrome/browser/history/top_sites_service_factory.h:30: static
scoped_refptr<RefcountedKeyedService> BuildProfileForTesting(
On 2014/12/29 09:48:14, sdefresne wrote:
> I don't think those two method needs to be 1. method, 2. part of the public
> interface. I would just make them free function in testing_profile.cc and
remove
> them from there.

Done.

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

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:96: scoped_refptr<history::TopSites>
top_sites_provider =
On 2014/12/29 09:48:14, sdefresne wrote:
> nit: rename top_sites_provider to top_sites

Done.

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/thumbnai...
File chrome/browser/thumbnails/thumbnail_service_impl.cc (right):

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_service_impl.cc:49:
ThumbnailServiceImpl::ThumbnailServiceImpl(Profile* profile)
On 2014/12/29 09:48:14, sdefresne wrote:
> You need to add the dependency on TopSitesServiceFactory to
> ThumbnailServiceFactory.

Done.

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/thumbnai...
File chrome/browser/thumbnails/thumbnail_service_unittest.cc (right):

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_service_unittest.cc:55: MockProfile() :
mock_top_sites_(new MockTopSites(this)) {
On 2014/12/29 09:48:14, sdefresne wrote:
> Remove mock_top_sites_ property.

Done.

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_service_unittest.cc:58: history::TopSites*
GetTopSites() { return mock_top_sites_.get(); }
On 2014/12/29 09:48:14, sdefresne wrote:
> Remove this method. Instead use TopSitesServiceFactory::SetTestingFactory() to
> register a MockTopSites as the TopSites to use for this MockProfile.
> 
> You can probably do this in MockProfile constructor:
> 
> MockProfile() {
>   TopSitesServiceFactory::SetTestingProfile(this, BuildMockTopSites);
> }
> 
> with BuildMockTopSites defined as:
> 
> scoped_refptr<RefcountedKeyedService>
BuildMockTopSites(content::BrowserContext*
> context) {
>   return scoped_refptr<RefcountedKeyedService>(new
> MockTopSites(static_cast<Profile*>(context));
> }

Done.

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_service_unittest.cc:65:
scoped_refptr<MockTopSites> mock_top_sites_;
On 2014/12/29 09:48:14, sdefresne wrote:
> Remove this property.

Done.

https://codereview.chromium.org/815983002/diff/400001/chrome/test/base/testin...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/815983002/diff/400001/chrome/test/base/testin...
chrome/test/base/testing_profile.cc:592:
&TopSitesServiceFactory::CreateCustomTopSitesServiceFactory(&top_sites));
On 2014/12/29 09:48:14, sdefresne wrote:
> Please move this testing factory function
> (TopSitesServiceFactory::CreateCustomTopSitesServiceFactory) into the
anonymous
> namespace at the beginning of the file. It does not need to be a static
function
> of TopSitesServiceFactory, it can just be a free function.

Done.

https://codereview.chromium.org/815983002/diff/400001/chrome/test/base/testin...
chrome/test/base/testing_profile.cc:597: this,
&TopSitesServiceFactory::BuildProfileForTesting(this));
On 2014/12/29 09:48:14, sdefresne wrote:
> Please move this testing factory function
> (TopSitesServiceFactory::BuildProfileForTesting) into the anonymous namespace
at
> the beginning of the file. It does not need to be a static function of
> TopSitesServiceFactory, it can just be a free function.

Done.

[Jitu( very slow this week), 2015-01-06 10:49:01]

jitendra.ks@samsung.com changed reviewers:
+ bauerb@chromium.org, cpu@chromium.org

[Jitu( very slow this week), 2015-01-06 10:49:02]

bauerb@chromium.org: Please review changes in 

cpu@chromium.org: Please review changes in

[Bernhard Bauer, 2015-01-06 11:10:31]

The CL description seems to be outdated.

Also, FTR: It's customary to wait for an LGTM from the main reviewer before
adding OWNERS reviewers, to avoid everyone having to look at all the files.

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/android/...
File chrome/browser/android/dev_tools_manager_delegate_android.cc (right):

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/android/...
chrome/browser/android/dev_tools_manager_delegate_android.cc:343: if
(top_sites.get()) {
On 2014/12/30 10:09:02, Slow this week wrote:
> On 2014/12/29 09:48:14, sdefresne wrote:
> > nit: "if (top_sites)" should work, here and everywhere else.
> 
> Done.

This is still not fixed in a *lot* of other places.

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

https://codereview.chromium.org/815983002/diff/480001/chrome/browser/history/...
chrome/browser/history/top_sites_factory.cc:42:
scoped_refptr<RefcountedKeyedService>
Can you move this above the constructor and destructor, so it matches the order
in the header?

https://codereview.chromium.org/815983002/diff/480001/chrome/browser/history/...
File chrome/browser/history/top_sites_impl_unittest.cc (right):

https://codereview.chromium.org/815983002/diff/480001/chrome/browser/history/...
chrome/browser/history/top_sites_impl_unittest.cc:375: new
TestTopSitesObserver(profile_.get(), TopSitesFactory::GetForProfile(profile_));
Please fit this into 80 columns.

https://codereview.chromium.org/815983002/diff/480001/chrome/browser/thumbnai...
File chrome/browser/thumbnails/thumbnail_service_unittest.cc (right):

https://codereview.chromium.org/815983002/diff/480001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_service_unittest.cc:67:
TopSitesFactory::GetInstance()::SetTestingFactory(this,
Looks like the indentation got messed up here?

https://codereview.chromium.org/815983002/diff/480001/chrome/test/base/testin...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/815983002/diff/480001/chrome/test/base/testin...
chrome/test/base/testing_profile.cc:254: return
scoped_refptr<history::TopSites>(top_sites);
If this scoped_refptr<history::TopSites> can be automatically converted to the
scoped_refptr<RefcountedKeyedService> declared as the return type, could you use
make_scoped_refptr here and have it convert from
scoped_refptr<history::TopSitesImpl>?

https://codereview.chromium.org/815983002/diff/480001/chrome/test/base/testin...
chrome/test/base/testing_profile.cc:626: scoped_refptr<history::TopSites>
top_site =
Nit: |top_sites|

[Bernhard Bauer, 2015-01-06 11:11:13]

Also, Rietveld is a bit overzealous in parsing my comment. Not LGTM in this
state.

[Jitu( very slow this week), 2015-01-07 05:30:31]

On 2015/01/06 11:10:31, Bernhard Bauer wrote:
> The CL description seems to be outdated.
> 
> Also, FTR: It's customary to wait for an LGTM from the main reviewer before
> adding OWNERS reviewers, to avoid everyone having to look at all the files.

Thanks Bauer for suggestion , i will take care.

[Jitu( very slow this week), 2015-01-08 09:38:25]

PTAL

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

https://codereview.chromium.org/815983002/diff/480001/chrome/browser/history/...
chrome/browser/history/top_sites_factory.cc:42:
scoped_refptr<RefcountedKeyedService>
On 2015/01/06 11:10:30, Bernhard Bauer wrote:
> Can you move this above the constructor and destructor, so it matches the
order
> in the header?

Done.

https://codereview.chromium.org/815983002/diff/480001/chrome/browser/history/...
File chrome/browser/history/top_sites_impl_unittest.cc (right):

https://codereview.chromium.org/815983002/diff/480001/chrome/browser/history/...
chrome/browser/history/top_sites_impl_unittest.cc:375: new
TestTopSitesObserver(profile_.get(), TopSitesFactory::GetForProfile(profile_));
On 2015/01/06 11:10:30, Bernhard Bauer wrote:
> Please fit this into 80 columns.

Done.

https://codereview.chromium.org/815983002/diff/480001/chrome/test/base/testin...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/815983002/diff/480001/chrome/test/base/testin...
chrome/test/base/testing_profile.cc:626: scoped_refptr<history::TopSites>
top_site =
On 2015/01/06 11:10:31, Bernhard Bauer wrote:
> Nit: |top_sites|

Done.

[Bernhard Bauer, 2015-01-08 10:22:05]

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/android/...
File chrome/browser/android/dev_tools_manager_delegate_android.cc (right):

https://codereview.chromium.org/815983002/diff/400001/chrome/browser/android/...
chrome/browser/android/dev_tools_manager_delegate_android.cc:343: if
(top_sites.get()) {
On 2015/01/06 11:10:30, Bernhard Bauer wrote:
> On 2014/12/30 10:09:02, Slow this week wrote:
> > On 2014/12/29 09:48:14, sdefresne wrote:
> > > nit: "if (top_sites)" should work, here and everywhere else.
> > 
> > Done.
> 
> This is still not fixed in a *lot* of other places.

Still not fixed in some places. These are really not that difficult to spot;
just do a pass over the CL in Rietveld (you can install
https://chrome.google.com/webstore/detail/rietveld-usability-toolki/nmljjlfbn...
for inline diffs so you can just scroll down the page) or in a local `git diff`.
It's somewhat disappointing that you as the author seem to be spending less
effort on this CL than the reviewer.

https://codereview.chromium.org/815983002/diff/480001/chrome/test/base/testin...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/815983002/diff/480001/chrome/test/base/testin...
chrome/test/base/testing_profile.cc:254: return
scoped_refptr<history::TopSites>(top_sites);
On 2015/01/06 11:10:31, Bernhard Bauer wrote:
> If this scoped_refptr<history::TopSites> can be automatically converted to the
> scoped_refptr<RefcountedKeyedService> declared as the return type, could you
use
> make_scoped_refptr here and have it convert from
> scoped_refptr<history::TopSitesImpl>?

Just make_scoped_refptr; The compiler will pick the right template.

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/android/...
File chrome/browser/android/provider/chrome_browser_provider.cc (right):

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/android/...
chrome/browser/android/provider/chrome_browser_provider.cc:1578: if
(top_sites_.get())
Remove the .get().

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/autocomp...
File chrome/browser/autocomplete/zero_suggest_provider.cc (right):

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/autocomp...
chrome/browser/autocomplete/zero_suggest_provider.cc:322: if (ts.get()) {
Remove this.

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/extensio...
File chrome/browser/extensions/api/top_sites/top_sites_api.cc (right):

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/extensio...
chrome/browser/extensions/api/top_sites/top_sites_api.cc:24: if (ts.get() ==
NULL)
Remove .get() and add the negation operator again.

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/history/...
File chrome/browser/history/chrome_history_client.h (right):

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/history/...
chrome/browser/history/chrome_history_client.h:28: history::TopSites*
top_sites);
This should take a const scoped_refptr<>& now.

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/history/...
File chrome/browser/history/chrome_history_client_factory.cc (right):

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/history/...
chrome/browser/history/chrome_history_client_factory.cc:51: profile,
top_sites.get());
Remove the .get() when ChromeHistoryClient can take a scoped_refptr.

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

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/history/...
chrome/browser/history/top_sites_factory.cc:37: return
make_scoped_refptr<history::TopSites>(top_sites);
Does a simple make_scoped_refptr() work? If it doesn't, you can just call the
scoped_refptr constructor instead of the make_scoped_refptr method, as that is
shorter.

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/history/...
File chrome/browser/history/top_sites_impl_unittest.cc (right):

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/history/...
chrome/browser/history/top_sites_impl_unittest.cc:376: profile_.get(),
Indentation: four spaces more than the previous line.

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/thumbnai...
File chrome/browser/thumbnails/thumbnail_service_impl.cc (right):

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_service_impl.cc:50: :
top_sites_(TopSitesFactory::GetForProfile(profile).get()),
Remove the .get().

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_service_impl.cc:60: if (local_ptr.get() ==
NULL)
Can you also change these to `!local`?

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/ui/tabs/...
File chrome/browser/ui/tabs/tab_strip_model_utils.cc (right):

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_model_utils.cc:14: history::TopSites*
top_sites,
Why this change? There is no need to pass in a pointer to a mutable object, and
passing in a raw pointer makes the ownership model unclearer.

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/global_menu_bar_x11.cc (right):

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/global_menu_bar_x11.cc:431: if (top_sites_.get())
{
Remove .get().

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/global_menu_bar_x11.cc:571:
DCHECK(top_sites_.get());
Remove the .get().

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/most_visited_handler.cc (right):

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/most_visited_handler.cc:88: if (ts.get()) {
Remove .get().

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/most_visited_handler.cc:97:
content::Source<history::TopSites>(ts.get()));
Remove .get().

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/most_visited_handler.cc:144: if (ts.get())
Remove .get().

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/most_visited_handler.cc:158: if (ts.get()) {
Remove .get().

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/most_visited_handler.cc:185: if (ts.get())
Remove .get().

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/most_visited_handler.cc:195: if (ts.get())
Remove .get().

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/most_visited_handler.cc:263: if (ts.get())
Remove .get().

https://codereview.chromium.org/815983002/diff/500001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/815983002/diff/500001/chrome/chrome_browser.g...
chrome/chrome_browser.gypi:1540: 'browser/history/top_sites_factory.h',
Keep these sorted alphabetically: c before h.

https://codereview.chromium.org/815983002/diff/500001/chrome/test/base/testin...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/815983002/diff/500001/chrome/test/base/testin...
chrome/test/base/testing_profile.cc:231: profile, top_sites.get());
Remove the .get() when ChromeHistoryClient takes a scoped_refptr.

https://codereview.chromium.org/815983002/diff/500001/chrome/test/base/testin...
chrome/test/base/testing_profile.cc:254: return
make_scoped_refptr<history::TopSites>(top_sites);
Just make_scoped_refptr.

[sdefresne, 2015-01-08 10:41:02]

Some more comments.

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

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/history/...
chrome/browser/history/top_sites_factory.cc:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
nit: you'll only land this is 2015, so please use 2015 in new files.

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/history/...
chrome/browser/history/top_sites_factory.cc:12: using namespace history;
nit: we prefer not to use "using namespace" unless it improves the clarity. I
don't think it is required here (especially since you mostly use fully qualified
types history::TopSites, ...).

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/history/...
chrome/browser/history/top_sites_factory.cc:34: content::BrowserContext*
profile) const {
nit: s/profile/context/

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/history/...
chrome/browser/history/top_sites_factory.cc:42: "TopSitesService",
nit: the value passed there is usually the name of the returned class, thus use
"TopSites" here.

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

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/history/...
chrome/browser/history/top_sites_factory.h:22: static
scoped_refptr<history::TopSites> GetForProfileIfExists(
nit: please comment this method.

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/history/...
File chrome/browser/history/top_sites_impl.h (right):

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/history/...
chrome/browser/history/top_sites_impl.h:85: // RefcountedKeyedService:
nit: add a blank line before.

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/thumbnai...
File chrome/browser/thumbnails/thumbnail_service_unittest.cc (right):

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_service_unittest.cc:73:
top_sites->AddKnownURL(url, score);
AddKnownURL is not a method of history::TopSites, so you need as cast here:

  static_cast<MockTopSites*>(top_sites.get())->AddKnownURL(url, score);

https://codereview.chromium.org/815983002/diff/500001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/815983002/diff/500001/chrome/chrome_browser.g...
chrome/chrome_browser.gypi:1540: 'browser/history/top_sites_factory.h',
Keep file name sorted (.cc before .h).

https://codereview.chromium.org/815983002/diff/500001/chrome/test/base/testin...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/815983002/diff/500001/chrome/test/base/testin...
chrome/test/base/testing_profile.cc:249: scoped_refptr<RefcountedKeyedService>
BuildProfileForTesting(
Rename this method BuildTopSites

https://codereview.chromium.org/815983002/diff/500001/chrome/test/base/testin...
File chrome/test/base/testing_profile.h (right):

https://codereview.chromium.org/815983002/diff/500001/chrome/test/base/testin...
chrome/test/base/testing_profile.h:192: void CreateTopSitesService();
nit: leave this method named CreateTopSites() and keep the original comment as
it is still true.

[Bernhard Bauer, 2015-01-08 10:45:14]

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

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/history/...
chrome/browser/history/top_sites_factory.cc:12: using namespace history;
On 2015/01/08 10:41:01, sdefresne wrote:
> nit: we prefer not to use "using namespace" unless it improves the clarity. I
> don't think it is required here (especially since you mostly use fully
qualified
> types history::TopSites, ...).

Good catch! The Google C++ style guide is actually even stricter about it:

"You may not use a using-directive to make all names from a namespace
available."
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespaces).

[Jitu( very slow this week), 2015-01-12 11:30:09]

PTAL

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/android/...
File chrome/browser/android/provider/chrome_browser_provider.cc (right):

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/android/...
chrome/browser/android/provider/chrome_browser_provider.cc:1578: if
(top_sites_.get())
On 2015/01/08 10:22:04, Bernhard Bauer wrote:
> Remove the .get().

Done.

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/autocomp...
File chrome/browser/autocomplete/zero_suggest_provider.cc (right):

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/autocomp...
chrome/browser/autocomplete/zero_suggest_provider.cc:322: if (ts.get()) {
On 2015/01/08 10:22:04, Bernhard Bauer wrote:
> Remove this.

Done.

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/extensio...
File chrome/browser/extensions/api/top_sites/top_sites_api.cc (right):

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/extensio...
chrome/browser/extensions/api/top_sites/top_sites_api.cc:24: if (ts.get() ==
NULL)
On 2015/01/08 10:22:04, Bernhard Bauer wrote:
> Remove .get() and add the negation operator again.

Done.

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/history/...
File chrome/browser/history/chrome_history_client.h (right):

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/history/...
chrome/browser/history/chrome_history_client.h:28: history::TopSites*
top_sites);
On 2015/01/08 10:22:05, Bernhard Bauer wrote:
> This should take a const scoped_refptr<>& now.

Done.

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/history/...
File chrome/browser/history/chrome_history_client_factory.cc (right):

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/history/...
chrome/browser/history/chrome_history_client_factory.cc:51: profile,
top_sites.get());
On 2015/01/08 10:22:05, Bernhard Bauer wrote:
> Remove the .get() when ChromeHistoryClient can take a scoped_refptr.

Done.

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

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/history/...
chrome/browser/history/top_sites_factory.cc:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
On 2015/01/08 10:41:01, sdefresne wrote:
> nit: you'll only land this is 2015, so please use 2015 in new files.

Done.

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/history/...
chrome/browser/history/top_sites_factory.cc:12: using namespace history;
On 2015/01/08 10:41:01, sdefresne wrote:
> nit: we prefer not to use "using namespace" unless it improves the clarity. I
> don't think it is required here (especially since you mostly use fully
qualified
> types history::TopSites, ...).

Done.

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/history/...
chrome/browser/history/top_sites_factory.cc:34: content::BrowserContext*
profile) const {
On 2015/01/08 10:41:01, sdefresne wrote:
> nit: s/profile/context/

Done.

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/history/...
chrome/browser/history/top_sites_factory.cc:37: return
make_scoped_refptr<history::TopSites>(top_sites);
On 2015/01/08 10:22:05, Bernhard Bauer wrote:
> Does a simple make_scoped_refptr() work? If it doesn't, you can just call the
> scoped_refptr constructor instead of the make_scoped_refptr method, as that is
> shorter.

I think
return make_scoped_refptr(top_sites) work here.

Are you suggesting to do change like this

scoped_refptr<TopSitesImpl> top_sites =  make_scoped_refptr(new
TopSitesImpl(static_cast<Profile*>(profile));

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/history/...
chrome/browser/history/top_sites_factory.cc:42: "TopSitesService",
On 2015/01/08 10:41:01, sdefresne wrote:
> nit: the value passed there is usually the name of the returned class, thus
use
> "TopSites" here.

Done.

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/history/...
chrome/browser/history/top_sites_factory.cc:42: "TopSitesService",
On 2015/01/08 10:41:01, sdefresne wrote:
> nit: the value passed there is usually the name of the returned class, thus
use
> "TopSites" here.

Done.

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

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/history/...
chrome/browser/history/top_sites_factory.h:22: static
scoped_refptr<history::TopSites> GetForProfileIfExists(
On 2015/01/08 10:41:01, sdefresne wrote:
> nit: please comment this method.

Done.

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/history/...
File chrome/browser/history/top_sites_impl.h (right):

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/history/...
chrome/browser/history/top_sites_impl.h:85: // RefcountedKeyedService:
On 2015/01/08 10:41:02, sdefresne wrote:
> nit: add a blank line before.

Done.

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/history/...
File chrome/browser/history/top_sites_impl_unittest.cc (right):

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/history/...
chrome/browser/history/top_sites_impl_unittest.cc:376: profile_.get(),
On 2015/01/08 10:22:05, Bernhard Bauer wrote:
> Indentation: four spaces more than the previous line.

Done.

seems git cl format not worked here :(

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/thumbnai...
File chrome/browser/thumbnails/thumbnail_service_impl.cc (right):

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_service_impl.cc:50: :
top_sites_(TopSitesFactory::GetForProfile(profile).get()),
On 2015/01/08 10:22:05, Bernhard Bauer wrote:
> Remove the .get().

Done.

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_service_impl.cc:60: if (local_ptr.get() ==
NULL)
On 2015/01/08 10:22:05, Bernhard Bauer wrote:
> Can you also change these to `!local`?

Done.

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/thumbnai...
File chrome/browser/thumbnails/thumbnail_service_unittest.cc (right):

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_service_unittest.cc:73:
top_sites->AddKnownURL(url, score);
On 2015/01/08 10:41:02, sdefresne wrote:
> AddKnownURL is not a method of history::TopSites, so you need as cast here:
> 
>   static_cast<MockTopSites*>(top_sites.get())->AddKnownURL(url, score);

Done.

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/ui/tabs/...
File chrome/browser/ui/tabs/tab_strip_model_utils.cc (right):

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_model_utils.cc:14: history::TopSites*
top_sites,
On 2015/01/08 10:22:05, Bernhard Bauer wrote:
> Why this change? There is no need to pass in a pointer to a mutable object,
and
> passing in a raw pointer makes the ownership model unclearer.

Done.

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/global_menu_bar_x11.cc (right):

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/global_menu_bar_x11.cc:431: if (top_sites_.get())
{
On 2015/01/08 10:22:05, Bernhard Bauer wrote:
> Remove .get().

Done.

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/global_menu_bar_x11.cc:571:
DCHECK(top_sites_.get());
On 2015/01/08 10:22:05, Bernhard Bauer wrote:
> Remove the .get().

Done.

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/most_visited_handler.cc (right):

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/most_visited_handler.cc:88: if (ts.get()) {
On 2015/01/08 10:22:05, Bernhard Bauer wrote:
> Remove .get().

Done.

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/most_visited_handler.cc:97:
content::Source<history::TopSites>(ts.get()));
On 2015/01/08 10:22:05, Bernhard Bauer wrote:
> Remove .get().

Done.

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/most_visited_handler.cc:144: if (ts.get())
On 2015/01/08 10:22:05, Bernhard Bauer wrote:
> Remove .get().

Done.

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/most_visited_handler.cc:158: if (ts.get()) {
On 2015/01/08 10:22:05, Bernhard Bauer wrote:
> Remove .get().

Done.

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/most_visited_handler.cc:185: if (ts.get())
On 2015/01/08 10:22:05, Bernhard Bauer wrote:
> Remove .get().

Done.

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/most_visited_handler.cc:195: if (ts.get())
On 2015/01/08 10:22:05, Bernhard Bauer wrote:
> Remove .get().

Done.

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/most_visited_handler.cc:263: if (ts.get())
On 2015/01/08 10:22:05, Bernhard Bauer wrote:
> Remove .get().

Done.

https://codereview.chromium.org/815983002/diff/500001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/815983002/diff/500001/chrome/chrome_browser.g...
chrome/chrome_browser.gypi:1540: 'browser/history/top_sites_factory.h',
On 2015/01/08 10:22:05, Bernhard Bauer wrote:
> Keep these sorted alphabetically: c before h.

Done.

https://codereview.chromium.org/815983002/diff/500001/chrome/test/base/testin...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/815983002/diff/500001/chrome/test/base/testin...
chrome/test/base/testing_profile.cc:231: profile, top_sites.get());
On 2015/01/08 10:22:05, Bernhard Bauer wrote:
> Remove the .get() when ChromeHistoryClient takes a scoped_refptr.

Done.

https://codereview.chromium.org/815983002/diff/500001/chrome/test/base/testin...
chrome/test/base/testing_profile.cc:249: scoped_refptr<RefcountedKeyedService>
BuildProfileForTesting(
On 2015/01/08 10:41:02, sdefresne wrote:
> Rename this method BuildTopSites

Done.

https://codereview.chromium.org/815983002/diff/500001/chrome/test/base/testin...
chrome/test/base/testing_profile.cc:254: return
make_scoped_refptr<history::TopSites>(top_sites);
On 2015/01/08 10:22:05, Bernhard Bauer wrote:
> Just make_scoped_refptr.

Done.

https://codereview.chromium.org/815983002/diff/500001/chrome/test/base/testin...
File chrome/test/base/testing_profile.h (right):

https://codereview.chromium.org/815983002/diff/500001/chrome/test/base/testin...
chrome/test/base/testing_profile.h:192: void CreateTopSitesService();
On 2015/01/08 10:41:02, sdefresne wrote:
> nit: leave this method named CreateTopSites() and keep the original comment as
> it is still true.

Done.

[Bernhard Bauer, 2015-01-12 14:32:23]

Almost there.

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

https://codereview.chromium.org/815983002/diff/500001/chrome/browser/history/...
chrome/browser/history/top_sites_factory.cc:37: return
make_scoped_refptr<history::TopSites>(top_sites);
On 2015/01/12 11:30:07, Slow this week wrote:
> On 2015/01/08 10:22:05, Bernhard Bauer wrote:
> > Does a simple make_scoped_refptr() work? If it doesn't, you can just call
the
> > scoped_refptr constructor instead of the make_scoped_refptr method, as that
is
> > shorter.
> 
> I think
> return make_scoped_refptr(top_sites) work here.
> 
> Are you suggesting to do change like this
> 
> scoped_refptr<TopSitesImpl> top_sites =  make_scoped_refptr(new
> TopSitesImpl(static_cast<Profile*>(profile));

That would be a possible alternative, but what you did is exactly what I was
refering to.

https://codereview.chromium.org/815983002/diff/520001/chrome/browser/history/...
File chrome/browser/history/chrome_history_client.h (right):

https://codereview.chromium.org/815983002/diff/520001/chrome/browser/history/...
chrome/browser/history/chrome_history_client.h:26: explicit ChromeHistoryClient(
Could you remove the "explicit" here?

https://codereview.chromium.org/815983002/diff/520001/chrome/browser/ui/tabs/...
File chrome/browser/ui/tabs/tab_strip_model_utils.h (right):

https://codereview.chromium.org/815983002/diff/520001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_model_utils.h:21: history::TopSites& top_sites,
This should be a const reference. The version on trunk had it exactly right.

[Jitu( very slow this week), 2015-01-12 14:55:15]

PTAL

https://codereview.chromium.org/815983002/diff/520001/chrome/browser/history/...
File chrome/browser/history/chrome_history_client.h (right):

https://codereview.chromium.org/815983002/diff/520001/chrome/browser/history/...
chrome/browser/history/chrome_history_client.h:26: explicit ChromeHistoryClient(
On 2015/01/12 14:32:23, Bernhard Bauer wrote:
> Could you remove the "explicit" here?

Done.

https://codereview.chromium.org/815983002/diff/520001/chrome/browser/ui/tabs/...
File chrome/browser/ui/tabs/tab_strip_model_utils.h (right):

https://codereview.chromium.org/815983002/diff/520001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_model_utils.h:21: history::TopSites& top_sites,
On 2015/01/12 14:32:23, Bernhard Bauer wrote:
> This should be a const reference. The version on trunk had it exactly right.

Done.

[Bernhard Bauer, 2015-01-12 15:52:50]

https://codereview.chromium.org/815983002/diff/540001/chrome/browser/ui/tabs/...
File chrome/browser/ui/tabs/tab_strip_model_utils.cc (right):

https://codereview.chromium.org/815983002/diff/540001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_model_utils.cc:14: history::TopSites&
top_sites,
Here also.

[sdefresne, 2015-01-12 17:24:24]

This is looking better.

I still have some comments, and a suggestion. I think that you can remove both
call to TopSitesFactory::SetTestingFactory(). I've put the explanation in the
comments. Can you please try them?

BTW, you may want to rebase your CL on top of the tree as it no longer apply
cleanly (I wanted to send it to the try bots).

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

https://codereview.chromium.org/815983002/diff/540001/chrome/browser/history/...
chrome/browser/history/top_sites_factory.h:22: // Get the TopSites service for
|profile|, but do not create if it doesn't
nit: but do not create it if it doesn't

https://codereview.chromium.org/815983002/diff/540001/chrome/browser/history/...
File chrome/browser/history/top_sites_impl.cc (right):

https://codereview.chromium.org/815983002/diff/540001/chrome/browser/history/...
chrome/browser/history/top_sites_impl.cc:447:
HistoryServiceFactory::GetForProfile(profile_, Profile::EXPLICIT_ACCESS);
nit: Profile::EXPLICIT access has been removed with
https://codereview.chromium.org/839193002, you should now use
ServiceAccessType::EXPLICIT_ACCESS.

https://codereview.chromium.org/815983002/diff/540001/chrome/browser/history/...
chrome/browser/history/top_sites_impl.cc:633:
DCHECK(!BrowserThread::IsThreadInitialized(BrowserThread::UI) ||
nit: can you removed this DCHECK? This is a guarantee of
RefcountedBrowserContextKeyedServiceFactory. Moreover,
//components/history/core/browser (were I'd like to move most of this file)
won't have access to content::BrowserThread.

https://codereview.chromium.org/815983002/diff/540001/chrome/browser/ui/tabs/...
File chrome/browser/ui/tabs/tab_strip_model_utils.cc (right):

https://codereview.chromium.org/815983002/diff/540001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_model_utils.cc:14: history::TopSites&
top_sites,
Please just revert changes to this file.

https://codereview.chromium.org/815983002/diff/540001/chrome/test/base/testin...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/815983002/diff/540001/chrome/test/base/testin...
chrome/test/base/testing_profile.cc:249: scoped_refptr<RefcountedKeyedService>
BuildTopSites(
It looks like TopSitesFactory::BuildInstanceForProfile() has the same
implementation, and since TopSitesFactory does not overrides
ServiceIsNULLForTesting() then I think the default implementation of
TopSitesFactory::GetForProfile() will work with a TestingProfile. This means
that you could just remove this method and TestingProfile::CreateTopSites().

Can you try to change TestingProfile::CreateTopSites() to do nothing (i.e. has a
{} implementation) and see if all unit and browser tests pass? I think they
should, in which case you can simplify this file.

[loverszhao, 2015-01-13 07:37:20]

On 2015/01/12 17:24:24, sdefresne wrote:
> This is looking better.
> 
> I still have some comments, and a suggestion. I think that you can remove both
> call to TopSitesFactory::SetTestingFactory(). I've put the explanation in the
> comments. Can you please try them?
> 
> BTW, you may want to rebase your CL on top of the tree as it no longer apply
> cleanly (I wanted to send it to the try bots).
> 
>
https://codereview.chromium.org/815983002/diff/540001/chrome/browser/history/...
> File chrome/browser/history/top_sites_factory.h (right):
> 
>
https://codereview.chromium.org/815983002/diff/540001/chrome/browser/history/...
> chrome/browser/history/top_sites_factory.h:22: // Get the TopSites service for
> |profile|, but do not create if it doesn't
> nit: but do not create it if it doesn't
> 
>
https://codereview.chromium.org/815983002/diff/540001/chrome/browser/history/...
> File chrome/browser/history/top_sites_impl.cc (right):
> 
>
https://codereview.chromium.org/815983002/diff/540001/chrome/browser/history/...
> chrome/browser/history/top_sites_impl.cc:447:
> HistoryServiceFactory::GetForProfile(profile_, Profile::EXPLICIT_ACCESS);
> nit: Profile::EXPLICIT access has been removed with
> https://codereview.chromium.org/839193002, you should now use
> ServiceAccessType::EXPLICIT_ACCESS.
> 
>
https://codereview.chromium.org/815983002/diff/540001/chrome/browser/history/...
> chrome/browser/history/top_sites_impl.cc:633:
> DCHECK(!BrowserThread::IsThreadInitialized(BrowserThread::UI) ||
> nit: can you removed this DCHECK? This is a guarantee of
> RefcountedBrowserContextKeyedServiceFactory. Moreover,
> //components/history/core/browser (were I'd like to move most of this file)
> won't have access to content::BrowserThread.
> 
>
https://codereview.chromium.org/815983002/diff/540001/chrome/browser/ui/tabs/...
> File chrome/browser/ui/tabs/tab_strip_model_utils.cc (right):
> 
>
https://codereview.chromium.org/815983002/diff/540001/chrome/browser/ui/tabs/...
> chrome/browser/ui/tabs/tab_strip_model_utils.cc:14: history::TopSites&
> top_sites,
> Please just revert changes to this file.
> 
>
https://codereview.chromium.org/815983002/diff/540001/chrome/test/base/testin...
> File chrome/test/base/testing_profile.cc (right):
> 
>
https://codereview.chromium.org/815983002/diff/540001/chrome/test/base/testin...
> chrome/test/base/testing_profile.cc:249: scoped_refptr<RefcountedKeyedService>
> BuildTopSites(
> It looks like TopSitesFactory::BuildInstanceForProfile() has the same
> implementation, and since TopSitesFactory does not overrides
> ServiceIsNULLForTesting() then I think the default implementation of
> TopSitesFactory::GetForProfile() will work with a TestingProfile. This means
> that you could just remove this method and TestingProfile::CreateTopSites().
> 
> Can you try to change TestingProfile::CreateTopSites() to do nothing (i.e. has
a
> {} implementation) and see if all unit and browser tests pass? I think they
> should, in which case you can simplify this file.

Ley me try .

[Jitu( very slow this week), 2015-01-14 13:03:27]

After doing the change and rebase i am getting below errors

../../chrome/browser/ui/browser.cc:1892:42: error: no viable conversion from
'scoped_refptr<history::TopSites>' to 'const history::TopSites'
  chrome::GetOpenUrls(*tab_strip_model_, top_sites, &open_urls);
                                         ^~~~~~~~~
../../chrome/browser/history/top_sites.h:40:7: note: candidate constructor (the
implicit copy constructor) not viable: no known conversion from
'scoped_refptr<history::TopSites>' to 'const history::TopSites &' for 1st
argument
class TopSites : public RefcountedKeyedService,
      ^
../../base/memory/ref_counted.h:347:3: note: candidate function
  operator Testable() const { return ptr_ ? &scoped_refptr::ptr_ : nullptr; }
  ^
../../chrome/browser/ui/tabs/tab_strip_model_utils.h:21:43: note: passing
argument to parameter 'top_sites' here
                 const history::TopSites& top_sites,
                                          ^
1 error generated.


Any suggestion ??

[sdefresne, 2015-01-14 21:24:14]

On 2015/01/14 at 13:03:27, jitendra.ks wrote:
> After doing the change and rebase i am getting below errors
> 
> ../../chrome/browser/ui/browser.cc:1892:42: error: no viable conversion from
'scoped_refptr<history::TopSites>' to 'const history::TopSites'
>   chrome::GetOpenUrls(*tab_strip_model_, top_sites, &open_urls);
>                                          ^~~~~~~~~
> ../../chrome/browser/history/top_sites.h:40:7: note: candidate constructor
(the implicit copy constructor) not viable: no known conversion from
'scoped_refptr<history::TopSites>' to 'const history::TopSites &' for 1st
argument
> class TopSites : public RefcountedKeyedService,
>       ^
> ../../base/memory/ref_counted.h:347:3: note: candidate function
>   operator Testable() const { return ptr_ ? &scoped_refptr::ptr_ : nullptr; }
>   ^
> ../../chrome/browser/ui/tabs/tab_strip_model_utils.h:21:43: note: passing
argument to parameter 'top_sites' here
>                  const history::TopSites& top_sites,
>                                           ^
> 1 error generated.
> 
> 
> Any suggestion ??

chrome::GetOpenUrls() second parameter is of type "const history::TopSites&", so
just dereference the scoped_refptr<> when calling this method. There is a null
check before, so you know that the dereferencing is valid:

  chrome::GetOpenUrls(*tab_strip_model_, *top_sites.get(), &open_urls);

[sdefresne, 2015-01-14 21:31:48]

https://codereview.chromium.org/815983002/diff/540001/chrome/test/base/testin...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/815983002/diff/540001/chrome/test/base/testin...
chrome/test/base/testing_profile.cc:249: scoped_refptr<RefcountedKeyedService>
BuildTopSites(
On 2015/01/12 at 17:24:24, sdefresne wrote:
> It looks like TopSitesFactory::BuildInstanceForProfile() has the same
implementation, and since TopSitesFactory does not overrides
ServiceIsNULLForTesting() then I think the default implementation of
TopSitesFactory::GetForProfile() will work with a TestingProfile. This means
that you could just remove this method and TestingProfile::CreateTopSites().
> 
> Can you try to change TestingProfile::CreateTopSites() to do nothing (i.e. has
a {} implementation) and see if all unit and browser tests pass? I think they
should, in which case you can simplify this file.

Please also test keeping TestingProfile::CreateTopSites() and overriding
TopSitesFactory::ServiceIsNULLForTesting() to return true.

[Jitu( very slow this week), 2015-01-19 10:11:37]

On 2015/01/14 21:31:48, sdefresne wrote:
>
https://codereview.chromium.org/815983002/diff/540001/chrome/test/base/testin...
> File chrome/test/base/testing_profile.cc (right):
> 
>
https://codereview.chromium.org/815983002/diff/540001/chrome/test/base/testin...
> chrome/test/base/testing_profile.cc:249: scoped_refptr<RefcountedKeyedService>
> BuildTopSites(
> On 2015/01/12 at 17:24:24, sdefresne wrote:
> > It looks like TopSitesFactory::BuildInstanceForProfile() has the same
> implementation, and since TopSitesFactory does not overrides
> ServiceIsNULLForTesting() then I think the default implementation of
> TopSitesFactory::GetForProfile() will work with a TestingProfile. This means
> that you could just remove this method and TestingProfile::CreateTopSites().
> > 
> > Can you try to change TestingProfile::CreateTopSites() to do nothing (i.e.
has
> a {} implementation) and see if all unit and browser tests pass? I think they
> should, in which case you can simplify this file.
> 
> Please also test keeping TestingProfile::CreateTopSites() and overriding
> TopSitesFactory::ServiceIsNULLForTesting() to return true.

Both the cases i have tried ... it passes most of the cases.
But still some testcases are failed due to

FATAL:top_sites_backend.cc(82)] Check failed: !db_.

I am checking more on it.

Thanks

[sdefresne, 2015-01-21 17:57:24]

> Both the cases i have tried ... it passes most of the cases.
> But still some testcases are failed due to
> 
> FATAL:top_sites_backend.cc(82)] Check failed: !db_.
> 
> I am checking more on it.
> 
> Thanks

So I did try to debug the unit tests, and I think your DCHECK was due to two
bugs:
1. TopSitesImpl::Shutdown() was never called
2. TestingProfile::CreateTopSites() did not properly wait for the old TopSites
to cleanup

I've uploaded https://codereview.chromium.org/861273002 to fix those two issues.
I also had to fix some errors that I pointed in your CL, namely:
1. incorrect order between function and class definition
2. conflict during rebasing
3. incorrect parameters to GetForProfile (instance instead of pointer to
instance)

In the future, could you please ensure that you have uploaded the most recent
version of your CL after rebasing on top of HEAD, and that you have checked it
compiles cleanly. Thanks.

I've send my fixed CL to the try bots. I can land it with a Patch-By: line
attributing the changes to you if you want or if you prefer, I can let you
incorporate the changes in your CL and land this one once it has been approved.

https://codereview.chromium.org/815983002/diff/560001/chrome/browser/autocomp...
File chrome/browser/autocomplete/zero_suggest_provider_unittest.cc (right):

https://codereview.chromium.org/815983002/diff/560001/chrome/browser/autocomp...
chrome/browser/autocomplete/zero_suggest_provider_unittest.cc:98: 
You need to override "void ShutdownOnUIThread()" in this class since you don't
implement it in "TopSites".

https://codereview.chromium.org/815983002/diff/560001/chrome/browser/autocomp...
chrome/browser/autocomplete/zero_suggest_provider_unittest.cc:185:
TopSitesFactory::GetInstance()->SetTestingFactory(profile_,
You need to use "&profile_" here and below since "profile_" is a
"TestingProfile" not a "TestingProfile*".

https://codereview.chromium.org/815983002/diff/560001/chrome/browser/history/...
File chrome/browser/history/chrome_history_client.h (right):

https://codereview.chromium.org/815983002/diff/560001/chrome/browser/history/...
chrome/browser/history/chrome_history_client.h:29:
ChromeHistoryClient(bookmarks::BookmarkModel* bookmark_model,
Upstream code has been changed and no longer require the "TopSites", so you can
remove this change after fetch/rebase.

https://codereview.chromium.org/815983002/diff/560001/chrome/browser/history/...
File chrome/browser/history/chrome_history_client_factory.cc (right):

https://codereview.chromium.org/815983002/diff/560001/chrome/browser/history/...
chrome/browser/history/chrome_history_client_factory.cc:39:
DependsOn(TopSitesFactory::GetInstance());
"ChromeHistoryClient" no longer needs the "TopSites" so you can remove this
dependency.

https://codereview.chromium.org/815983002/diff/560001/chrome/browser/history/...
File chrome/browser/history/expire_history_backend_unittest.cc (right):

https://codereview.chromium.org/815983002/diff/560001/chrome/browser/history/...
chrome/browser/history/expire_history_backend_unittest.cc:231:
TopSitesFactory::GetForProfile(profile_);
You need to use "&profile_" here and below since "profile_" is a
"TestingProfile" not a "TestingProfile*".

https://codereview.chromium.org/815983002/diff/560001/chrome/browser/history/...
chrome/browser/history/expire_history_backend_unittest.cc:232:
top_sites_->SetPageThumbnail(url_row1.url(), thumbnail, score);
You need to use "top_sites" here and below since you removed the "top_sites_"
member variable and instead used a local variable "top_sites".

https://codereview.chromium.org/815983002/diff/560001/chrome/browser/history/...
chrome/browser/history/expire_history_backend_unittest.cc:232:
top_sites_->SetPageThumbnail(url_row1.url(), thumbnail, score);
You need to use "top_sites" here and below since you removed the "top_sites_"
member variable and instead used a local variable "top_sites".

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

https://codereview.chromium.org/815983002/diff/560001/chrome/browser/history/...
chrome/browser/history/top_sites_factory.h:30: protected:
style: prefer to make overridden methods "private" when you don't call them in
the child class (which is the case here).

https://codereview.chromium.org/815983002/diff/560001/chrome/browser/history/...
File chrome/browser/history/top_sites_impl_unittest.cc (right):

https://codereview.chromium.org/815983002/diff/560001/chrome/browser/history/...
chrome/browser/history/top_sites_impl_unittest.cc:241:
TopSitesFactory::GetForProfile(profile_).get());
This line won't compile, it need to be:

return static_cast<TopSitesImpl*>(
    TopSitesFactory::GetForProfile(profile_.get()).get());

https://codereview.chromium.org/815983002/diff/560001/chrome/browser/history/...
chrome/browser/history/top_sites_impl_unittest.cc:376: new
TestTopSitesObserver(profile_.get(), TopSitesFactory::GetForProfile(profile_));
This line is incorrectly formatted, it should have been:

top_sites_observer_.reset(new TopSitesObserver(
    profile_.get(), TopSitesFactory::GetForProfile(profile())));

However, this line has been removed upstream as the "TestTopSitesObserver" was
unnecessary. Please fetch and rebase your patch on top of HEAD (code changes
often) and then re-upload your CL.

https://codereview.chromium.org/815983002/diff/560001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.cc (left):

https://codereview.chromium.org/815983002/diff/560001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:828: top_sites_->Shutdown();
This line was the only call to "TopSites::Shutdown()". Now that "TopSites" is a
"RefcountedKeyedService" then the "DependencyManager" will invoke the
"TopSites::ShutdownOnUIThread()" method instead, so you should remove the
"TopSites::Shutdown()" method and move the implementation into
"TopSites::ShutdownOnUIThread()" (whose only real implementation is in
"TopSitesImpl").

This is what was causing the "DCHECK(!db_);" to be reached in "TopSitesBackend"
destructor since the "db_" member is deleted in
"TopSitesBackend::ShutdownDBOnDBThread" which is invoked from
"TopSitesBackend::Shutdown" itself called from "TopSitesImpl::Shutdown" (which
is no longer called).

https://codereview.chromium.org/815983002/diff/560001/chrome/browser/thumbnai...
File chrome/browser/thumbnails/thumbnail_service_unittest.cc (right):

https://codereview.chromium.org/815983002/diff/560001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_service_unittest.cc:17:
scoped_refptr<RefcountedKeyedService> BuildMockTopSites(
This function won't compile as "MockTopSites" is not visible at that point. You
need to move the method between the definition of "MockTopSites" class and
"MockProfile" class. BTW, the method also need to be commented, I'd suggest:

// Testing factory that build a |MockTopSites| instance.
scoped_refptr<RefcountedKeyedService> BuildMockTopSites(

https://codereview.chromium.org/815983002/diff/560001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_service_unittest.cc:23: }  // namespace
nit: you can also put "MockTopSites" and "MockProfile" into the anonymous
namespace

namespace {

class MockTopSites : public history::TopSitesImpl {
  // ...
};

scoped_refptr<RefcountedKeyedService> BuildMockTopSites(
    content::BrowserContext* profile) {
  // ...
}

class MockProfile : public TestingProfile {
  // ...
};

}  // namespace

https://codereview.chromium.org/815983002/diff/560001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_service_unittest.cc:67:
TopSitesFactory::GetInstance()::SetTestingFactory(this, BuildMockTopSites);
This won't compile, it should be:

TopSitesFactory::GetInstance()->SetTestingFactory(this, BuildMockTopSites);

https://codereview.chromium.org/815983002/diff/560001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_service_unittest.cc:107:
TopSitesFactory::GetForProfile(profile);
You need to use "&profile" here and below since "profile" is a "MockProfile" not
a "MockProfile*".

https://codereview.chromium.org/815983002/diff/560001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/global_menu_bar_x11.cc (right):

https://codereview.chromium.org/815983002/diff/560001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/global_menu_bar_x11.cc:436: registrar_.Add(this,
chrome::NOTIFICATION_TOP_SITES_CHANGED,
This code has changed upstream, please fetch, rebase and re-upload after fixing
the conflict, something along the lines of:

// Register as TopSitesObserver so that we can update ourselves when the
// TopSites changes.
scoped_observer_.Add(top_sites_.get());

https://codereview.chromium.org/815983002/diff/560001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/most_visited_handler.cc (right):

https://codereview.chromium.org/815983002/diff/560001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/most_visited_handler.cc:96: registrar_.Add(this,
chrome::NOTIFICATION_TOP_SITES_CHANGED,
Same here, you have a conflict with upstream that now uses "TopSitesObserver"
instead of notification.

https://codereview.chromium.org/815983002/diff/560001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/most_visited_handler.cc:96: registrar_.Add(this,
chrome::NOTIFICATION_TOP_SITES_CHANGED,
Same here, you have a conflict with upstream that now uses "TopSitesObserver"
instead of notification.

https://codereview.chromium.org/815983002/diff/560001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/suggestions_page_handler.cc (right):

https://codereview.chromium.org/815983002/diff/560001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/suggestions_page_handler.cc:83: registrar_.Add(this,
chrome::NOTIFICATION_TOP_SITES_CHANGED,
Same here, you have a conflict with upstream that now uses "TopSitesObserver"
instead of notification.

https://codereview.chromium.org/815983002/diff/560001/chrome/test/base/testin...
File chrome/test/base/testing_profile.cc (left):

https://codereview.chromium.org/815983002/diff/560001/chrome/test/base/testin...
chrome/test/base/testing_profile.cc:502: DestroyTopSites();
Here too we need to cleanup if an instance of "TopSites" has been created,
however, it is not possible to call "TopSitesFactory::GetForProfileIfExists()"
after the call to
"BrowserContextDependencyManager::DestroyBrowserContextServices()" since the
current profile would have been marked as destroyed and you'll hit "DCHECK". So
instead we need to remember whether or not a "TopSites" instance had been create
and conditionally do the cleanup.

https://codereview.chromium.org/815983002/diff/560001/chrome/test/base/testin...
chrome/test/base/testing_profile.cc:585: if (top_sites_.get()) {
Once the "TopSites::ShutdownOnUIThread" is correctly called, then you'll see
that tests fails due to concurrent access to the top sites database. This is
caused by improper cleanup of the previous "TopSites" instance before creating
the new one. If the "base::MessageLoop::current()->RunUntilIdle();" call is
removed, then previous "TopSites" won't have the time to cleanup and unlock the
database, and the new one will fails all access to it.

So this method need to be added back but should instead use
"TopSitesFactory::GetForProfileIfExists()" to check if an instance of "TopSites"
has been created or not and should use "TopSitesFactory::SetTestingFactory()"
with a null factory to delete the old instance.

https://codereview.chromium.org/815983002/diff/560001/chrome/test/base/testin...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/815983002/diff/560001/chrome/test/base/testin...
chrome/test/base/testing_profile.cc:468: CreateTopSites();
This is incorrect, we do not want to unconditionally create a "TopSites".

https://codereview.chromium.org/815983002/diff/560001/chrome/test/base/testin...
chrome/test/base/testing_profile.cc:586: //
TopSitesFactory::GetInstance()->SetTestingFactory(this, BuildTopSites);
If you comment this line, then the "BuildTopSites" function ends up not being
used which is flagged as a warning, and since warning are considered error will
cause the build to fail. Please either keep the code or remove it when
uploading, but first run the tests locally before committing to an
implementation.

[Jitu( very slow this week), 2015-01-22 05:33:33]

Thanks sdefresne ,

As my linux machine was screwed up(some issues) , i was trying building the code
in trybots.
In another patch https://codereview.chromium.org/802153005/ 

Where i have resolved all the compile related issues and also overrided
ShutdownDBOnDBThread() in topsitesimpl file.

As per your suggestions i ll be doing the changes and upload the patch today.

[Jitu( very slow this week), 2015-01-22 11:30:12]

PTAL...

Thanks

[sdefresne, 2015-01-22 19:05:05]

This is getting better. I think you'll have something ready once you fix those
last comments.

https://codereview.chromium.org/815983002/diff/580001/chrome/browser/autocomp...
File chrome/browser/autocomplete/zero_suggest_provider_unittest.cc (right):

https://codereview.chromium.org/815983002/diff/580001/chrome/browser/autocomp...
chrome/browser/autocomplete/zero_suggest_provider_unittest.cc:93: void
ShutdownOnUIThread() override {}
style: add a comment before this method saying

// RefcountedKeyedService:

https://codereview.chromium.org/815983002/diff/580001/chrome/browser/autocomp...
chrome/browser/autocomplete/zero_suggest_provider_unittest.cc:185:
TopSitesFactory::GetInstance()->SetTestingFactory(&profile_,
Please add a call to "profile_.DestroyTopSites();" before setting the test
factory.

https://codereview.chromium.org/815983002/diff/580001/chrome/browser/history/...
File chrome/browser/history/top_sites.cc (right):

https://codereview.chromium.org/815983002/diff/580001/chrome/browser/history/...
chrome/browser/history/top_sites.cc:8: #include
"chrome/browser/history/top_sites_impl.h"
Remove this unused #include.

https://codereview.chromium.org/815983002/diff/580001/chrome/browser/history/...
File chrome/browser/history/top_sites.h (right):

https://codereview.chromium.org/815983002/diff/580001/chrome/browser/history/...
chrome/browser/history/top_sites.h:46: static TopSites* Create(Profile* profile,
const base::FilePath& db_name);
Remove this method since it has no implementation.

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

https://codereview.chromium.org/815983002/diff/580001/chrome/browser/history/...
chrome/browser/history/top_sites_factory.cc:33: void
TopSitesFactory::ShutdownForProfile(Profile* profile) {
This is unused, remove.

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

https://codereview.chromium.org/815983002/diff/580001/chrome/browser/history/...
chrome/browser/history/top_sites_factory.h:9: #include
"chrome/browser/history/top_sites.h"
nit: you don't need this #include, instead you can forward-declare TopSites in
the history namespace.

https://codereview.chromium.org/815983002/diff/580001/chrome/browser/history/...
chrome/browser/history/top_sites_factory.h:9: #include
"chrome/browser/history/top_sites.h"
nit: you don't need this #include, instead you can forward-declare TopSites in
the history namespace.

https://codereview.chromium.org/815983002/diff/580001/chrome/browser/history/...
chrome/browser/history/top_sites_factory.h:30: static void
ShutdownForProfile(Profile* profile);
This is unused, remove.

https://codereview.chromium.org/815983002/diff/580001/chrome/browser/history/...
chrome/browser/history/top_sites_factory.h:36: 
style: remove empty line

https://codereview.chromium.org/815983002/diff/580001/chrome/browser/history/...
chrome/browser/history/top_sites_factory.h:39: friend struct
DefaultSingletonTraits<TopSitesFactory>;
style: friend should go before methods

https://codereview.chromium.org/815983002/diff/580001/chrome/test/base/testin...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/815983002/diff/580001/chrome/test/base/testin...
chrome/test/base/testing_profile.cc:128: TopSitesFactory* top_sites_factory =
TopSitesFactory::GetInstance();
This does not compile on Windows, and I don't really understand why. You can fix
the compilation failure by rewriting the code as:

return !!TopSitesFactory::GetInstance()->GetForProfileIfExists(profile);

[Bernhard Bauer, 2015-01-22 21:23:27]

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

https://codereview.chromium.org/815983002/diff/580001/chrome/browser/history/...
chrome/browser/history/top_sites_factory.h:39: friend struct
DefaultSingletonTraits<TopSitesFactory>;
Friend declarations come first in the private section.

https://codereview.chromium.org/815983002/diff/580001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/815983002/diff/580001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1892: chrome::GetOpenUrls(*tab_strip_model_,
*top_sites.get(), &open_urls);
Just dereferencing the scoped_refptr (not the raw pointer returned by .get()) is
sufficient.

https://codereview.chromium.org/815983002/diff/580001/chrome/test/base/testin...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/815983002/diff/580001/chrome/test/base/testin...
chrome/test/base/testing_profile.cc:128: TopSitesFactory* top_sites_factory =
TopSitesFactory::GetInstance();
On 2015/01/22 19:05:05, sdefresne wrote:
> This does not compile on Windows, and I don't really understand why. You can
fix
> the compilation failure by rewriting the code as:
> 
> return !!TopSitesFactory::GetInstance()->GetForProfileIfExists(profile);

In any case, the .get() and the comparison to nullptr isn't necessary.

https://codereview.chromium.org/815983002/diff/580001/chrome/test/base/testin...
chrome/test/base/testing_profile.cc:520: // Remember whether a TopSites has beed
created for the current profile,
Nit: "has been", or "had been" (below you use the latter, so it should be
consistent).

[Jitu( very slow this week), 2015-01-23 11:36:53]

PTAL

https://codereview.chromium.org/815983002/diff/580001/chrome/browser/autocomp...
File chrome/browser/autocomplete/zero_suggest_provider_unittest.cc (right):

https://codereview.chromium.org/815983002/diff/580001/chrome/browser/autocomp...
chrome/browser/autocomplete/zero_suggest_provider_unittest.cc:93: void
ShutdownOnUIThread() override {}
On 2015/01/22 19:05:05, sdefresne wrote:
> style: add a comment before this method saying
> 
> // RefcountedKeyedService:

Done.

https://codereview.chromium.org/815983002/diff/580001/chrome/browser/autocomp...
chrome/browser/autocomplete/zero_suggest_provider_unittest.cc:185:
TopSitesFactory::GetInstance()->SetTestingFactory(&profile_,
On 2015/01/22 19:05:05, sdefresne wrote:
> Please add a call to "profile_.DestroyTopSites();" before setting the test
> factory.

Done.

https://codereview.chromium.org/815983002/diff/580001/chrome/browser/history/...
File chrome/browser/history/top_sites.cc (right):

https://codereview.chromium.org/815983002/diff/580001/chrome/browser/history/...
chrome/browser/history/top_sites.cc:8: #include
"chrome/browser/history/top_sites_impl.h"
On 2015/01/22 19:05:05, sdefresne wrote:
> Remove this unused #include.

Done.

https://codereview.chromium.org/815983002/diff/580001/chrome/browser/history/...
File chrome/browser/history/top_sites.h (right):

https://codereview.chromium.org/815983002/diff/580001/chrome/browser/history/...
chrome/browser/history/top_sites.h:46: static TopSites* Create(Profile* profile,
const base::FilePath& db_name);
On 2015/01/22 19:05:05, sdefresne wrote:
> Remove this method since it has no implementation.

Done.

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

https://codereview.chromium.org/815983002/diff/580001/chrome/browser/history/...
chrome/browser/history/top_sites_factory.cc:33: void
TopSitesFactory::ShutdownForProfile(Profile* profile) {
On 2015/01/22 19:05:05, sdefresne wrote:
> This is unused, remove.

Done.

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

https://codereview.chromium.org/815983002/diff/580001/chrome/browser/history/...
chrome/browser/history/top_sites_factory.h:9: #include
"chrome/browser/history/top_sites.h"
On 2015/01/22 19:05:05, sdefresne wrote:
> nit: you don't need this #include, instead you can forward-declare TopSites in
> the history namespace.

Done.

https://codereview.chromium.org/815983002/diff/580001/chrome/browser/history/...
chrome/browser/history/top_sites_factory.h:30: static void
ShutdownForProfile(Profile* profile);
On 2015/01/22 19:05:05, sdefresne wrote:
> This is unused, remove.

Done.

https://codereview.chromium.org/815983002/diff/580001/chrome/browser/history/...
chrome/browser/history/top_sites_factory.h:36: 
On 2015/01/22 19:05:05, sdefresne wrote:
> style: remove empty line

Done.

https://codereview.chromium.org/815983002/diff/580001/chrome/browser/history/...
chrome/browser/history/top_sites_factory.h:39: friend struct
DefaultSingletonTraits<TopSitesFactory>;
On 2015/01/22 19:05:05, sdefresne wrote:
> style: friend should go before methods

Done.

https://codereview.chromium.org/815983002/diff/580001/chrome/browser/history/...
chrome/browser/history/top_sites_factory.h:39: friend struct
DefaultSingletonTraits<TopSitesFactory>;
On 2015/01/22 21:23:27, Bernhard Bauer wrote:
> Friend declarations come first in the private section.

Done.

https://codereview.chromium.org/815983002/diff/580001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/815983002/diff/580001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1892: chrome::GetOpenUrls(*tab_strip_model_,
*top_sites.get(), &open_urls);
On 2015/01/22 21:23:27, Bernhard Bauer wrote:
> Just dereferencing the scoped_refptr (not the raw pointer returned by .get())
is
> sufficient.

Done.

https://codereview.chromium.org/815983002/diff/580001/chrome/test/base/testin...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/815983002/diff/580001/chrome/test/base/testin...
chrome/test/base/testing_profile.cc:520: // Remember whether a TopSites has beed
created for the current profile,
On 2015/01/22 21:23:27, Bernhard Bauer wrote:
> Nit: "has been", or "had been" (below you use the latter, so it should be
> consistent).

Done.

[sdefresne, 2015-01-23 13:25:46]

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

https://codereview.chromium.org/815983002/diff/600001/chrome/browser/history/...
chrome/browser/history/top_sites_factory.h:9: #include
"chrome/browser/profiles/profile.h"
nit: you can remove this #include and instead forward-declare the "Profile"
class.

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

https://codereview.chromium.org/815983002/diff/600001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:96: scoped_refptr<history::TopSites>
top_sites =
Add a dependency on TopSitesFactory to InstantServiceFactory (sorry, I missed
that one in the previous reviews).

[Jitu( very slow this week), 2015-01-23 13:52:52]

PTAL

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

https://codereview.chromium.org/815983002/diff/600001/chrome/browser/history/...
chrome/browser/history/top_sites_factory.h:9: #include
"chrome/browser/profiles/profile.h"
On 2015/01/23 13:25:46, sdefresne wrote:
> nit: you can remove this #include and instead forward-declare the "Profile"
> class.

Done.

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

https://codereview.chromium.org/815983002/diff/600001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:96: scoped_refptr<history::TopSites>
top_sites =
On 2015/01/23 13:25:46, sdefresne wrote:
> Add a dependency on TopSitesFactory to InstantServiceFactory (sorry, I missed
> that one in the previous reviews).

Done.

[sdefresne, 2015-01-23 14:03:43]

lgtm

[Bernhard Bauer, 2015-01-23 14:34:07]

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

https://codereview.chromium.org/815983002/diff/620001/chrome/browser/history/...
chrome/browser/history/top_sites_factory.h:42: TopSitesFactory();
Constructors and descructors should come before other methods. See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...
for the full order.

[Jitu( very slow this week), 2015-01-24 07:10:58]

Patchset #17 (id:640001) has been deleted

[Jitu( very slow this week), 2015-01-24 11:48:23]

PTAL

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

https://codereview.chromium.org/815983002/diff/620001/chrome/browser/history/...
chrome/browser/history/top_sites_factory.h:42: TopSitesFactory();
On 2015/01/23 14:34:06, Bernhard Bauer wrote:
> Constructors and descructors should come before other methods. See
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...
> for the full order.

Done.

[Bernhard Bauer, 2015-01-26 09:39:35]

lgtm

[Jitu( very slow this week), 2015-01-26 10:21:39]

Dear Cpu, 

Can you PTAL.

[Jitu( very slow this week), 2015-01-26 10:22:01]

Dear Cpu, 

Can you PTAL.

[sdefresne, 2015-01-26 10:38:33]

On 2015/01/26 at 10:22:01, jitendra.ks wrote:
> Dear Cpu, 
> 
> Can you PTAL.

jitendra.ks: I assume that you're asking cpu to review this CL because he is an
OWNERS for some touched code. It is generally a good idea to let the person know
which part of the CL they should review when you are only asking for them to
review the part the own.

[cpu_(ooo_6.6-7.5), 2015-01-26 19:50:28]

I am sorry, what in particular should I be looking at?

 I am not the expert on  android, autocomplete, devtools, extensions, histroy,
thumbnails, etc.

[sdefresne, 2015-01-26 21:20:48]

sdefresne@chromium.org changed reviewers:
+ asargent@chromium.org, benwells@chromium.org, erg@chromium.org,
jcivelli@chromium.org, jered@chromium.org, mpearson@chromium.org,
msw@chromium.org, pfeldman@chromium.org

[sdefresne, 2015-01-26 21:28:54]

sdefresne@chromium.org changed reviewers:
- asargent@chromium.org

[sdefresne, 2015-01-26 21:28:56]

jitendra.ks: when selecting OWNERS using "git cl owners" try to pick the OWNERS
that is the most specific (i.e. in the deepest directory), and add a line in the
message indicating what each OWNERS should review.

cpu: I've looked at the OWNERS for the different directories, and added one
OWNERS for each directory, but there are still some files that don't have OWNERS
except the OWNERS at //chrome level, can you review just those files, they are:
  - chrome/browser/jumplist_win.cc
  - chrome/browser/thumbnails/*

benwells: can you do an OWNERS review of
  - chrome/browser/extensions/*
  - chrome/browser/ui/app_list/test*/

erg: can you do an OWNERS review of
  - chrome/browser/profiles/*

mpearson: can you do an OWNERS review of
  - chrome/browser/autocomplete/*

msw: can you do an OWNERS review of
  - chrome/browser/ui/*

jcivelli: can you do an OWNERS review of
  - chrome/test/base/testing_profile.*

pfeldman: can you do an OWNERS review of
  - chrome/browser/devtools/*

jered: can you do an OWNERS review of
  - chrome/browser/search/*

[Elliot Glaysher, 2015-01-26 21:33:04]

I <3 seeing things removed from Profile.

lgtm

[Mark P, 2015-01-26 22:49:37]

chrome/browser/autocomplete/... lgtm

[cpu_(ooo_6.6-7.5), 2015-01-27 00:42:22]

thanks sdefresne, that helps a lot

lgtm on 

  - chrome/browser/jumplist_win.cc
  - chrome/browser/thumbnails/

please wait on the other owners for their files.

[Jay Civelli, 2015-01-27 00:58:23]

https://codereview.chromium.org/815983002/diff/680001/chrome/test/base/testin...
File chrome/test/base/testing_profile.h (right):

https://codereview.chromium.org/815983002/diff/680001/chrome/test/base/testin...
chrome/test/base/testing_profile.h:16: #include
"components/keyed_service/core/refcounted_keyed_service.h"
Is this include needed?

[benwells, 2015-01-27 04:20:41]

c/b/ui/app_list lgtm. i assume that is what you wanted me to look at.

[sdefresne, 2015-01-27 09:55:27]

https://codereview.chromium.org/815983002/diff/680001/chrome/test/base/testin...
File chrome/test/base/testing_profile.h (right):

https://codereview.chromium.org/815983002/diff/680001/chrome/test/base/testin...
chrome/test/base/testing_profile.h:16: #include
"components/keyed_service/core/refcounted_keyed_service.h"
On 2015/01/27 at 00:58:23, Jay Civelli wrote:
> Is this include needed?

Good catch. It looks like the implementation uses a
scoped_refptr<RefcountedKeyedService>, so I'd say it can be moved to
testing_profile.cc

[sdefresne, 2015-01-27 09:57:46]

On 2015/01/27 at 04:20:41, benwells wrote:
> c/b/ui/app_list lgtm. i assume that is what you wanted me to look at.

benwells: Thank you for c/b/ui/app_list review. Could you also review
chrome/browser/extensions/? They are the two directory we need OWNERS approval
from you.

[Jitu( very slow this week), 2015-01-27 10:50:40]

Dear Jay Civelli,

Fixed the comments for testing_profile.h

PTAL

https://codereview.chromium.org/815983002/diff/680001/chrome/test/base/testin...
File chrome/test/base/testing_profile.h (right):

https://codereview.chromium.org/815983002/diff/680001/chrome/test/base/testin...
chrome/test/base/testing_profile.h:16: #include
"components/keyed_service/core/refcounted_keyed_service.h"
On 2015/01/27 00:58:23, Jay Civelli wrote:
> Is this include needed?

Done.

[dgozman, 2015-01-27 11:01:16]

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

[dgozman, 2015-01-27 11:01:18]

chrome/browser/devtools lgtm

[Jay Civelli, 2015-01-27 16:33:54]

LGTM for chrome/test/base

[benwells, 2015-01-27 21:29:00]

c/b/extensions lgtm

[Jitu( very slow this week), 2015-01-28 08:57:36]

The CQ bit was checked by jitendra.ks@samsung.com

[commit-bot: I haz the power, 2015-01-28 08:58:19]

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

[commit-bot: I haz the power, 2015-01-28 09:47:48]

Committed patchset #19 (id:700001)

[commit-bot: I haz the power, 2015-01-28 09:48:39]

Patchset 19 (id:??) landed as
https://crrev.com/30f03390995328247e3642aa5cd2cc9aae781b45
Cr-Commit-Position: refs/heads/master@{#313468}