Introduce HistoryClient interface

Introduce HistoryClient interface

Introduce the HistoryClient interface that will abstract the embedder for
the history component. The implementation in //chrome ChromeHistoryClient
owns the HistoryService and implements the KeyedService interface.

BUG=371825
TBR=sky, zea

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273846

[sdefresne, 2014-05-26 16:31:06]

As asked in https://codereview.chromium.org/285233012, this is the CL that just
introduce HistoryClient and change the ownership.

Please take a look.

[blundell, 2014-05-27 09:56:38]

https://codereview.chromium.org/305443004/diff/1/chrome/browser/history/chrom...
File chrome/browser/history/chrome_history_client.h (right):

https://codereview.chromium.org/305443004/diff/1/chrome/browser/history/chrom...
chrome/browser/history/chrome_history_client.h:18: class ChromeHistoryClient :
public HistoryClient,
should have a class comment.

https://codereview.chromium.org/305443004/diff/1/chrome/browser/history/chrom...
chrome/browser/history/chrome_history_client.h:23: HistoryService*
history_service() const {
const methods should return const pointers (or it shouldn't be a const method,
one of the two).

https://codereview.chromium.org/305443004/diff/1/chrome/browser/history/histo...
File chrome/browser/history/history_service.h (right):

https://codereview.chromium.org/305443004/diff/1/chrome/browser/history/histo...
chrome/browser/history/history_service.h:95: // Must call Init after
construction.
add a comment about expected lifetime.

https://codereview.chromium.org/305443004/diff/1/chrome/browser/history/histo...
chrome/browser/history/history_service.h:566: history::HistoryClient*
history_client() const { return history_client_; }
same comment about const.

https://codereview.chromium.org/305443004/diff/1/chrome/browser/history/histo...
chrome/browser/history/history_service.h:1025: // The HistoryClient; may be NULL
in tests.
add a comment about expected lifetime.

https://codereview.chromium.org/305443004/diff/1/chrome/browser/history/histo...
File chrome/browser/history/history_service_factory.cc (right):

https://codereview.chromium.org/305443004/diff/1/chrome/browser/history/histo...
chrome/browser/history/history_service_factory.cc:26: ChromeHistoryClient*
history_client = static_cast<ChromeHistoryClient*>(
If you're keeping HistoryServiceFactory, I think you might as well keep
HistoryService as a KeyedService, have a ChromeHistoryClientFactory, and have
FaviconService depend on ChromeHistoryClient (at the factory level) to ensure
that the lifetimes work out as desired.

https://codereview.chromium.org/305443004/diff/1/chrome/browser/sync/glue/boo...
File chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc (right):

https://codereview.chromium.org/305443004/diff/1/chrome/browser/sync/glue/boo...
chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc:60: class
HistoryClientMock : public history::TestHistoryClient,
I think the need for this change will go away if you make the change I
suggested.

https://codereview.chromium.org/305443004/diff/1/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service_typed_url_unittest.cc (right):

https://codereview.chromium.org/305443004/diff/1/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service_typed_url_unittest.cc:115: class
HistoryClientMock : public history::TestHistoryClient,
same

https://codereview.chromium.org/305443004/diff/1/components/history.gypi
File components/history.gypi (right):

https://codereview.chromium.org/305443004/diff/1/components/history.gypi#newc...
components/history.gypi:9: 'type': 'static_library',
i think this has to be type None until it has source files in it.

https://codereview.chromium.org/305443004/diff/1/components/history.gypi#newc...
components/history.gypi:31: 'history/core/browser/test_history_client.cc',
history/core/test

[sdefresne, 2014-05-28 17:10:41]

PTAL

https://codereview.chromium.org/305443004/diff/1/chrome/browser/history/chrom...
File chrome/browser/history/chrome_history_client.h (right):

https://codereview.chromium.org/305443004/diff/1/chrome/browser/history/chrom...
chrome/browser/history/chrome_history_client.h:18: class ChromeHistoryClient :
public HistoryClient,
On 2014/05/27 09:56:38, blundell wrote:
> should have a class comment.

Done.

https://codereview.chromium.org/305443004/diff/1/chrome/browser/history/chrom...
chrome/browser/history/chrome_history_client.h:23: HistoryService*
history_service() const {
On 2014/05/27 09:56:38, blundell wrote:
> const methods should return const pointers (or it shouldn't be a const method,
> one of the two).

Done.

https://codereview.chromium.org/305443004/diff/1/chrome/browser/history/histo...
File chrome/browser/history/history_service.h (right):

https://codereview.chromium.org/305443004/diff/1/chrome/browser/history/histo...
chrome/browser/history/history_service.h:95: // Must call Init after
construction.
On 2014/05/27 09:56:38, blundell wrote:
> add a comment about expected lifetime.

Done.

https://codereview.chromium.org/305443004/diff/1/chrome/browser/history/histo...
chrome/browser/history/history_service.h:566: history::HistoryClient*
history_client() const { return history_client_; }
On 2014/05/27 09:56:38, blundell wrote:
> same comment about const.

Done.

https://codereview.chromium.org/305443004/diff/1/chrome/browser/history/histo...
chrome/browser/history/history_service.h:1025: // The HistoryClient; may be NULL
in tests.
On 2014/05/27 09:56:38, blundell wrote:
> add a comment about expected lifetime.

Done.

https://codereview.chromium.org/305443004/diff/1/chrome/browser/history/histo...
File chrome/browser/history/history_service_factory.cc (right):

https://codereview.chromium.org/305443004/diff/1/chrome/browser/history/histo...
chrome/browser/history/history_service_factory.cc:26: ChromeHistoryClient*
history_client = static_cast<ChromeHistoryClient*>(
On 2014/05/27 09:56:38, blundell wrote:
> If you're keeping HistoryServiceFactory, I think you might as well keep
> HistoryService as a KeyedService, have a ChromeHistoryClientFactory, and have
> FaviconService depend on ChromeHistoryClient (at the factory level) to ensure
> that the lifetimes work out as desired.

Done with %s/FaviconService/HistoryService/g.

https://codereview.chromium.org/305443004/diff/1/chrome/browser/sync/glue/boo...
File chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc (right):

https://codereview.chromium.org/305443004/diff/1/chrome/browser/sync/glue/boo...
chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc:60: class
HistoryClientMock : public history::TestHistoryClient,
On 2014/05/27 09:56:38, blundell wrote:
> I think the need for this change will go away if you make the change I
> suggested.

Done.

https://codereview.chromium.org/305443004/diff/1/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service_typed_url_unittest.cc (right):

https://codereview.chromium.org/305443004/diff/1/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service_typed_url_unittest.cc:115: class
HistoryClientMock : public history::TestHistoryClient,
On 2014/05/27 09:56:38, blundell wrote:
> same

Done.

https://codereview.chromium.org/305443004/diff/1/components/history.gypi
File components/history.gypi (right):

https://codereview.chromium.org/305443004/diff/1/components/history.gypi#newc...
components/history.gypi:9: 'type': 'static_library',
On 2014/05/27 09:56:38, blundell wrote:
> i think this has to be type None until it has source files in it.

Done.

https://codereview.chromium.org/305443004/diff/1/components/history.gypi#newc...
components/history.gypi:31: 'history/core/browser/test_history_client.cc',
On 2014/05/27 09:56:38, blundell wrote:
> history/core/test

Done.

[blundell, 2014-05-30 12:19:03]

LGTM!

https://codereview.chromium.org/305443004/diff/60001/chrome/browser/history/h...
File chrome/browser/history/history_service.h (right):

https://codereview.chromium.org/305443004/diff/60001/chrome/browser/history/h...
chrome/browser/history/history_service.h:97: // must be valid for the whole life
time of |HistoryService|.
nit: s/life time/lifetime

https://codereview.chromium.org/305443004/diff/80001/chrome/browser/history/c...
File chrome/browser/history/chrome_history_client_factory.h (right):

https://codereview.chromium.org/305443004/diff/80001/chrome/browser/history/c...
chrome/browser/history/chrome_history_client_factory.h:14: // Singleton that
owns all HistoryClient and associates them with Profiles.
s/HistoryClient/ChromeHistoryClients

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

https://codereview.chromium.org/305443004/diff/80001/chrome/browser/history/h...
chrome/browser/history/history_service.h:1028: // The history client, may be
null when testing. The object should outlive
s/should/should otherwise

[sdefresne, 2014-05-30 12:42:14]

https://codereview.chromium.org/305443004/diff/80001/chrome/browser/history/c...
File chrome/browser/history/chrome_history_client_factory.h (right):

https://codereview.chromium.org/305443004/diff/80001/chrome/browser/history/c...
chrome/browser/history/chrome_history_client_factory.h:14: // Singleton that
owns all HistoryClient and associates them with Profiles.
On 2014/05/30 12:19:04, blundell wrote:
> s/HistoryClient/ChromeHistoryClients

Done.

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

https://codereview.chromium.org/305443004/diff/80001/chrome/browser/history/h...
chrome/browser/history/history_service.h:1028: // The history client, may be
null when testing. The object should outlive
On 2014/05/30 12:19:04, blundell wrote:
> s/should/should otherwise

Done.

[sdefresne, 2014-05-30 12:54:56]

TBR=sky for OWNERS of //chrome/test and //chrome/browser/DEPS
TBR=zea for OWNERS of //chrome/browser/sync

[sdefresne, 2014-05-30 12:57:02]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2014-05-30 12:58:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sdefresne@chromium.org/305443004/120001

[commit-bot: I haz the power, 2014-05-30 15:43:19]

Change committed as 273846