Componentize TopSites, TopSitesBackend, TopSitesDatabase

Componentize TopSites, TopSitesBackend, TopSitesDatabase

TopSites prepopulated pages are not injected by the embedder via the
TopSitesFactory. The method TopSites::GetPrepopulatedSites() now does return
the sites favicon, thumbnail and color in addition to the URL and title.

TopSitesImplTest now can inject prepopulated pages to the TopSitesImpl thus
enabling the test TopSitesImplTest.BlacklistingWithPrepopulated on Android.

Change TopSites so that it does not inherit from content::NotificationObserver
as it does not need to listen to notification and move the inheritance to
TopSitesImpl (until the code is changed to use observer pattern instead of
notifications).

Remove dependency of TopSitesBackend on content::BrowserThread by injecting a
base::SingleThreadedTaskRunner reference to the corresponding thread via the
TopSitesImpl::Init() method.

Introduce //components/test/data/history and move pristine SQL files used by
the different unit tests there, helping remove dependency of the unit tests
on //chrome.

Move top_sites.{cc,h}, top_sites_database.{cc,h} into
//components/history/core/browser.

BUG=380144, 380157
TBR=sky, dgozman, samarth

Committed: https://crrev.com/0da3bc0e26f768834feca7a5e1c0c70f4c88e453
Cr-Commit-Position: refs/heads/master@{#313740}

[sdefresne, 2015-01-26 23:00:56]

sdefresne@chromium.org changed reviewers:
+ droger@chromium.org

[sdefresne, 2015-01-26 23:00:57]

Please take a look.

[droger, 2015-01-27 10:08:37]

only nits

https://codereview.chromium.org/870063002/diff/1/chrome/browser/history/top_s...
File chrome/browser/history/top_sites_factory.cc (right):

https://codereview.chromium.org/870063002/diff/1/chrome/browser/history/top_s...
chrome/browser/history/top_sites_factory.cc:85: for (size_t i = 0; i <
arraysize(kRawPrepopulatedPages); ++i) {
optional nit:
use some temporary variable here for clarity:
const RawPrepopulatedPage& page = kRawPrepopulatedPages[i];

https://codereview.chromium.org/870063002/diff/1/chrome/browser/history/top_s...
File chrome/browser/history/top_sites_impl.cc (right):

https://codereview.chromium.org/870063002/diff/1/chrome/browser/history/top_s...
chrome/browser/history/top_sites_impl.cc:249: if (url ==
prepopulated_page.url.url) {
should this be something like:
prepopulated_page.most_visited.url

"url.url" looks weird.

https://codereview.chromium.org/870063002/diff/1/chrome/browser/history/top_s...
File chrome/browser/history/top_sites_impl.h (right):

https://codereview.chromium.org/870063002/diff/1/chrome/browser/history/top_s...
chrome/browser/history/top_sites_impl.h:85: PrepopulatedPageList
GetPrepopulatedPages() override;
Could you return a const reference here?

https://codereview.chromium.org/870063002/diff/1/chrome/browser/history/top_s...
File chrome/browser/history/top_sites_impl_unittest.cc (right):

https://codereview.chromium.org/870063002/diff/1/chrome/browser/history/top_s...
chrome/browser/history/top_sites_impl_unittest.cc:225: PrepopulatedPageList
GetPrepopulatedPages() {
Return a const reference?

https://codereview.chromium.org/870063002/diff/1/chrome/browser/history/top_s...
chrome/browser/history/top_sites_impl_unittest.cc:243: void EmptyCallback() {}
Optional:
use base::DoNothing() instead.

https://codereview.chromium.org/870063002/diff/1/chrome/browser/history/top_s...
chrome/browser/history/top_sites_impl_unittest.cc:346:
profile_->DestroyTopSites();
Maybe add a comment here explaining why you need to destroy the top sites.
Or rename the method to ResetTopSites() maybe.

https://codereview.chromium.org/870063002/diff/1/components/history/core/brow...
File components/history/core/browser/top_sites.h (right):

https://codereview.chromium.org/870063002/diff/1/components/history/core/brow...
components/history/core/browser/top_sites.h:44: MostVisitedURL url;  // The
prepopulated page URL and title.
Should this be:
MostVisitedURL most_visited;
or
MostVisitedURL most_visited_url;

to avoid the "url.url" elsewhere.

[sdefresne, 2015-01-27 13:55:16]

PTAL

https://codereview.chromium.org/870063002/diff/1/chrome/browser/history/top_s...
File chrome/browser/history/top_sites_factory.cc (right):

https://codereview.chromium.org/870063002/diff/1/chrome/browser/history/top_s...
chrome/browser/history/top_sites_factory.cc:85: for (size_t i = 0; i <
arraysize(kRawPrepopulatedPages); ++i) {
On 2015/01/27 10:08:36, droger wrote:
> optional nit:
> use some temporary variable here for clarity:
> const RawPrepopulatedPage& page = kRawPrepopulatedPages[i];

Done.

https://codereview.chromium.org/870063002/diff/1/chrome/browser/history/top_s...
File chrome/browser/history/top_sites_impl.cc (right):

https://codereview.chromium.org/870063002/diff/1/chrome/browser/history/top_s...
chrome/browser/history/top_sites_impl.cc:249: if (url ==
prepopulated_page.url.url) {
On 2015/01/27 at 10:08:36, droger wrote:
> should this be something like:
> prepopulated_page.most_visited.url
> 
> "url.url" looks weird.

Changed to prepopulated.most_visited.url (though there are many other
occurrences of MostVisitedURL variables named url, leading to url.url in the
history code).

https://codereview.chromium.org/870063002/diff/1/chrome/browser/history/top_s...
File chrome/browser/history/top_sites_impl.h (right):

https://codereview.chromium.org/870063002/diff/1/chrome/browser/history/top_s...
chrome/browser/history/top_sites_impl.h:85: PrepopulatedPageList
GetPrepopulatedPages() override;
On 2015/01/27 at 10:08:36, droger wrote:
> Could you return a const reference here?

GetPrepopulatedPages() is a virtual method defined on TopSites pure virtual
interface. Using a const reference in the interface would force a particular
implementation. Returning a copy does not make the code worse than before since
it was already returning a copy.

https://codereview.chromium.org/870063002/diff/1/chrome/browser/history/top_s...
chrome/browser/history/top_sites_impl.h:85: PrepopulatedPageList
GetPrepopulatedPages() override;
On 2015/01/27 at 10:08:36, droger wrote:
> Could you return a const reference here?

GetPrepopulatedPages() is a virtual method defined on TopSites pure virtual
interface. Using a const reference in the interface would force a particular
implementation. Returning a copy does not make the code worse than before since
it was already returning a copy.

https://codereview.chromium.org/870063002/diff/1/chrome/browser/history/top_s...
File chrome/browser/history/top_sites_impl_unittest.cc (right):

https://codereview.chromium.org/870063002/diff/1/chrome/browser/history/top_s...
chrome/browser/history/top_sites_impl_unittest.cc:225: PrepopulatedPageList
GetPrepopulatedPages() {
On 2015/01/27 at 10:08:36, droger wrote:
> Return a const reference?

I can't since top_sites()->GetPrepopulatedPages() does not return a const
reference. That would be returning a const reference to a local variable I
think, which is incorrect.

https://codereview.chromium.org/870063002/diff/1/chrome/browser/history/top_s...
chrome/browser/history/top_sites_impl_unittest.cc:243: void EmptyCallback() {}
On 2015/01/27 at 10:08:36, droger wrote:
> Optional:
> use base::DoNothing() instead.

Not used.

https://codereview.chromium.org/870063002/diff/1/chrome/browser/history/top_s...
chrome/browser/history/top_sites_impl_unittest.cc:346:
profile_->DestroyTopSites();
On 2015/01/27 at 10:08:37, droger wrote:
> Maybe add a comment here explaining why you need to destroy the top sites.
> Or rename the method to ResetTopSites() maybe.

Done.

https://codereview.chromium.org/870063002/diff/1/components/history/core/brow...
File components/history/core/browser/top_sites.h (right):

https://codereview.chromium.org/870063002/diff/1/components/history/core/brow...
components/history/core/browser/top_sites.h:44: MostVisitedURL url;  // The
prepopulated page URL and title.
On 2015/01/27 at 10:08:37, droger wrote:
> Should this be:
> MostVisitedURL most_visited;
> or
> MostVisitedURL most_visited_url;
> 
> to avoid the "url.url" elsewhere.

Done.

[droger, 2015-01-27 15:43:18]

lgtm

[sdefresne, 2015-01-27 18:13:07]

sdefresne@chromium.org changed reviewers:
+ bauerb@chromium.org, bengr@chromium.org, dgozman@chromium.org,
finnur@chromium.org, phajdan.jr@chromium.org, pkasting@chromium.org,
sky@chromium.org

[sdefresne, 2015-01-27 18:13:08]

bauerb: please review as OWNERS of
- chrome/browser/android/
- chrome/browser/ui/webui/

pkasting: please review as OWNERS of
- chrome/browser/autocomplete
- chrome/browser/ui/browser.cc
- chrome/browser/ui/tabs/
- chrome/browser/ui/views/

dgozman: please review as OWNERS of
- chrome/browser/devtools/

samarth: please review as OWNERS of
- chrome/browser/search/
- chrome/browser/ui/search/

bengr: please review as OWNERS of
- chrome/browser/precache/

finnur: please review as OWNERS of
- chrome/browser/extensions/api/top_sites/

phajdan.jr: please review as OWNERS of
- chrome/test/base/testing_profile.cc

sky: please review as OWNERS of
- chrome/browser/jumplist_win.cc
- chrome/browser/thumbnails/
- chrome/tools/profiles/thumbnail-inl.h

[sdefresne, 2015-01-27 18:14:01]

sdefresne@chromium.org changed reviewers:
+ samarth@chromium.org

[sdefresne, 2015-01-27 18:14:02]

+ samarth for OWNERS of chrome/browser/search and chrome/browser/ui/search

[Bernhard Bauer, 2015-01-27 18:33:50]

Android and Web UI LGTM.

BTW, TBR'ing the mechanical part of this change (renaming includes) is perfectly
acceptable, provided the overall change has been LGTM'd.

[bengr, 2015-01-27 19:03:04]

On 2015/01/27 18:33:50, Bernhard Bauer wrote:
> Android and Web UI LGTM.
> 
> BTW, TBR'ing the mechanical part of this change (renaming includes) is
perfectly
> acceptable, provided the overall change has been LGTM'd.

Precache lgtm

[Peter Kasting, 2015-01-27 19:18:19]

On 2015/01/27 18:13:08, sdefresne wrote:
> pkasting: please review as OWNERS of
> - chrome/browser/autocomplete
> - chrome/browser/ui/browser.cc
> - chrome/browser/ui/tabs/
> - chrome/browser/ui/views/

LGTM.  Feel free to TBR changes that are trivial mechanical ones (e.g. changing
an #include).

[sdefresne, 2015-01-28 09:45:11]

sdefresne@chromium.org changed reviewers:
+ bsalomon@google.com

[sdefresne, 2015-01-28 09:45:40]

bsalomon@: need OWNERS approval for adding a new dependency on third_party/skia
from components/history

[Finnur, 2015-01-28 11:46:12]

> chrome/browser/extensions/api/top_sites/ 

LGTM

[Paweł Hajdan Jr., 2015-01-28 13:13:23]

chrome/test/base LGTM

[bsalomon, 2015-01-28 14:54:44]

On 2015/01/28 09:45:40, sdefresne wrote:
> bsalomon@: need OWNERS approval for adding a new dependency on
third_party/skia
> from components/history

lgtm

[bsalomon, 2015-01-28 14:54:46]

On 2015/01/28 09:45:40, sdefresne wrote:
> bsalomon@: need OWNERS approval for adding a new dependency on
third_party/skia
> from components/history

lgtm

[sdefresne, 2015-01-29 16:41:56]

TBR-ing OWNERS of the remaining files impacted by mechanical changes (file
move).

[sdefresne, 2015-01-29 16:42:07]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2015-01-29 16:42:19]

The CQ bit was unchecked by sdefresne@chromium.org

[sdefresne, 2015-01-29 17:21:09]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2015-01-29 17:22:05]

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

[commit-bot: I haz the power, 2015-01-29 18:27:08]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-01-29 18:28:13]

Patchset 9 (id:??) landed as
https://crrev.com/0da3bc0e26f768834feca7a5e1c0c70f4c88e453
Cr-Commit-Position: refs/heads/master@{#313740}