Abstract history dependencies on bookmarks through HistoryClient

Abstract history dependencies on bookmarks through HistoryClient

Add methods to abstract BookmarkService and BookmarkModel methods used
by HistoryService through the HistoryClient interface.

BUG=371825
TBR=sky

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

[Jiang Jiang, 2014-05-19 21:51:49]

https://codereview.chromium.org/285233012/diff/30001/chrome/browser/history/c...
File chrome/browser/history/chrome_history_client.h (right):

https://codereview.chromium.org/285233012/diff/30001/chrome/browser/history/c...
chrome/browser/history/chrome_history_client.h:9: #include
"chrome/browser/history/history_service.h"
Forward declare is enough.

https://codereview.chromium.org/285233012/diff/30001/chrome/browser/history/c...
chrome/browser/history/chrome_history_client.h:23: HistoryService*
history_service() {
Should be a const?

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

https://codereview.chromium.org/285233012/diff/30001/chrome/browser/history/h...
chrome/browser/history/history_service.h:560: history::HistoryClient*
history_client() { return history_client_; }
Should be a const?

https://codereview.chromium.org/285233012/diff/30001/chrome/browser/history/h...
File chrome/browser/history/history_service_factory.cc (right):

https://codereview.chromium.org/285233012/diff/30001/chrome/browser/history/h...
chrome/browser/history/history_service_factory.cc:79: return NULL;
Isn't it leaking |history_client|?

[sdefresne, 2014-05-23 10:02:49]

Please have a look.

https://codereview.chromium.org/285233012/diff/30001/chrome/browser/history/c...
File chrome/browser/history/chrome_history_client.h (right):

https://codereview.chromium.org/285233012/diff/30001/chrome/browser/history/c...
chrome/browser/history/chrome_history_client.h:9: #include
"chrome/browser/history/history_service.h"
On 2014/05/19 21:51:50, Jiang wrote:
> Forward declare is enough.

Done.

https://codereview.chromium.org/285233012/diff/30001/chrome/browser/history/c...
chrome/browser/history/chrome_history_client.h:23: HistoryService*
history_service() {
On 2014/05/19 21:51:50, Jiang wrote:
> Should be a const?

Done.

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

https://codereview.chromium.org/285233012/diff/30001/chrome/browser/history/h...
chrome/browser/history/history_service.h:560: history::HistoryClient*
history_client() { return history_client_; }
On 2014/05/19 21:51:50, Jiang wrote:
> Should be a const?

Done.

https://codereview.chromium.org/285233012/diff/30001/chrome/browser/history/h...
File chrome/browser/history/history_service_factory.cc (right):

https://codereview.chromium.org/285233012/diff/30001/chrome/browser/history/h...
chrome/browser/history/history_service_factory.cc:79: return NULL;
On 2014/05/19 21:51:50, Jiang wrote:
> Isn't it leaking |history_client|?

It was originally leaking the history_service. Addressed the leak nonetheless.

[blundell, 2014-05-23 11:37:55]

https://codereview.chromium.org/285233012/diff/50001/chrome/browser/history/h...
File chrome/browser/history/history_service_factory.cc (right):

https://codereview.chromium.org/285233012/diff/50001/chrome/browser/history/h...
chrome/browser/history/history_service_factory.cc:76:
scoped_ptr<ChromeHistoryClient> history_client(
There's no reason to make this change. The HistoryServiceFactory owns the
returned instance.

[sdefresne, 2014-05-23 13:59:29]

sky: Please have a look (blundell is OOO).

https://codereview.chromium.org/285233012/diff/50001/chrome/browser/history/h...
File chrome/browser/history/history_service_factory.cc (right):

https://codereview.chromium.org/285233012/diff/50001/chrome/browser/history/h...
chrome/browser/history/history_service_factory.cc:76:
scoped_ptr<ChromeHistoryClient> history_client(
On 2014/05/23 11:37:55, blundell wrote:
> There's no reason to make this change. The HistoryServiceFactory owns the
> returned instance.

Done.

[sky, 2014-05-23 15:50:50]

As I reviewed all the bookmark changes, I think it's only fair to share the
load. How about getting brettw (sorry) to review these?
sky->brettw

[blundell, 2014-05-26 07:16:58]

Would it be possible to split this into two CLs:
- an initial CL that introduces HistoryClient, ChromeHistoryClient, and changes
the ownership model of HistoryService so that it's owned by ChromeHistoryClient
- a followup CL that does the bookmarks abstraction?

Thanks,

Colin

https://codereview.chromium.org/285233012/diff/70001/components/history.gypi
File components/history.gypi (right):

https://codereview.chromium.org/285233012/diff/70001/components/history.gypi#...
components/history.gypi:34:
'history/core/browser/history_client_mock_bookmarks.cc',
We're moving to a structure where the support code lives in
//components/history/core/test (cf. discussions around //components/bookmarks).

https://codereview.chromium.org/285233012/diff/70001/components/history/core/...
File components/history/core/browser/history_client.h (right):

https://codereview.chromium.org/285233012/diff/70001/components/history/core/...
components/history/core/browser/history_client.h:21: // This class abstracts
operations that depends on the embedder's environment,
s/depends/depend

https://codereview.chromium.org/285233012/diff/70001/components/history/core/...
components/history/core/browser/history_client.h:28: virtual void
BlockTillBookmarksLoaded();
s/Till/Until

https://codereview.chromium.org/285233012/diff/70001/components/history/core/...
components/history/core/browser/history_client.h:32: // If not on the main
thread, then you must call BlockTillLoaded first.
s/you must call BlockTillLoaded/BlockTillLoaded must be called

[sdefresne, 2014-05-26 16:34:02]

I've split the CL as requested. The first CL that just introduce HistoryClient
and the history component can be found at
https://codereview.chromium.org/305443004/. The current CL is based on that
other CL.

Please have another look once we agree on the first CL.

https://codereview.chromium.org/285233012/diff/70001/components/history.gypi
File components/history.gypi (right):

https://codereview.chromium.org/285233012/diff/70001/components/history.gypi#...
components/history.gypi:34:
'history/core/browser/history_client_mock_bookmarks.cc',
On 2014/05/26 07:16:58, blundell wrote:
> We're moving to a structure where the support code lives in
> //components/history/core/test (cf. discussions around
//components/bookmarks).

Done.

https://codereview.chromium.org/285233012/diff/70001/components/history/core/...
File components/history/core/browser/history_client.h (right):

https://codereview.chromium.org/285233012/diff/70001/components/history/core/...
components/history/core/browser/history_client.h:21: // This class abstracts
operations that depends on the embedder's environment,
On 2014/05/26 07:16:58, blundell wrote:
> s/depends/depend

Done.

https://codereview.chromium.org/285233012/diff/70001/components/history/core/...
components/history/core/browser/history_client.h:28: virtual void
BlockTillBookmarksLoaded();
On 2014/05/26 07:16:58, blundell wrote:
> s/Till/Until

Done.

https://codereview.chromium.org/285233012/diff/70001/components/history/core/...
components/history/core/browser/history_client.h:32: // If not on the main
thread, then you must call BlockTillLoaded first.
On 2014/05/26 07:16:58, blundell wrote:
> s/you must call BlockTillLoaded/BlockTillLoaded must be called

Done.

[sdefresne, 2014-05-30 15:16:45]

PTAL

[blundell, 2014-06-02 09:39:30]

Looks really good, only significant comment is on testing_profile.cc

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

https://codereview.chromium.org/285233012/diff/110001/chrome/browser/history/...
chrome/browser/history/chrome_history_client.h:14: // depends on Chrome
environment.
s/depends/depend

https://codereview.chromium.org/285233012/diff/110001/chrome/browser/history/...
File chrome/browser/history/expire_history_backend.h (right):

https://codereview.chromium.org/285233012/diff/110001/chrome/browser/history/...
chrome/browser/history/expire_history_backend.h:254: // Returns the
HistoryClient, blocking until it is loaded. This may return
s/it is loaded/bookmarks are loaded ?

https://codereview.chromium.org/285233012/diff/110001/chrome/browser/history/...
chrome/browser/history/expire_history_backend.h:303: // Use GetHistoryClient to
access this, which makes sure the data is loaded.
s/data/bookmarks ?

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

https://codereview.chromium.org/285233012/diff/110001/chrome/test/base/testin...
chrome/test/base/testing_profile.cc:524:
history_service->history_backend_->bookmark_service_ = bookmark_service;
I think that you should create and wire up a ChromeHistoryServiceClient here to
keep the setup as it was before.

[sdefresne, 2014-06-02 12:59:11]

PTAL

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

https://codereview.chromium.org/285233012/diff/110001/chrome/browser/history/...
chrome/browser/history/chrome_history_client.h:14: // depends on Chrome
environment.
On 2014/06/02 09:39:30, blundell wrote:
> s/depends/depend

Done.

https://codereview.chromium.org/285233012/diff/110001/chrome/browser/history/...
File chrome/browser/history/expire_history_backend.h (right):

https://codereview.chromium.org/285233012/diff/110001/chrome/browser/history/...
chrome/browser/history/expire_history_backend.h:254: // Returns the
HistoryClient, blocking until it is loaded. This may return
On 2014/06/02 09:39:30, blundell wrote:
> s/it is loaded/bookmarks are loaded ?

Done.

https://codereview.chromium.org/285233012/diff/110001/chrome/browser/history/...
chrome/browser/history/expire_history_backend.h:303: // Use GetHistoryClient to
access this, which makes sure the data is loaded.
On 2014/06/02 09:39:30, blundell wrote:
> s/data/bookmarks ?

Done.

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

https://codereview.chromium.org/285233012/diff/110001/chrome/test/base/testin...
chrome/test/base/testing_profile.cc:524:
history_service->history_backend_->bookmark_service_ = bookmark_service;
On 2014/06/02 09:39:30, blundell wrote:
> I think that you should create and wire up a ChromeHistoryServiceClient here
to
> keep the setup as it was before.

Done.

[blundell, 2014-06-02 13:02:35]

LGTM with question

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

https://codereview.chromium.org/285233012/diff/130001/chrome/test/base/testin...
chrome/test/base/testing_profile.cc:516:
BookmarkModelFactory::GetForProfile(profile));
Is this needed? i.e., is it different from what ChromeHistoryClientFactory would
do otherwise?

[sdefresne, 2014-06-03 10:42:03]

brettw → FYI

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

https://codereview.chromium.org/285233012/diff/130001/chrome/test/base/testin...
chrome/test/base/testing_profile.cc:516:
BookmarkModelFactory::GetForProfile(profile));
On 2014/06/02 13:02:36, blundell wrote:
> Is this needed? i.e., is it different from what ChromeHistoryClientFactory
would
> do otherwise?

Yes this is required, since ChromeHistoryClientFactory returns NULL for a
testing profile. And if I change it to returns non-NULL, then other tests
asserts as we try to create a ChromeHistoryClient when there is no BookmarkModel
created (ie. BookmarkModelFactory returns NULL for a testing profile).

[sdefresne, 2014-06-03 12:21:09]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2014-06-03 12:23:04]

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

[sdefresne, 2014-06-03 13:08:15]

TBR=jcivelli for OWNERS of //chrome/test/base

[commit-bot: I haz the power, 2014-06-03 13:50:06]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[sdefresne, 2014-06-04 14:05:17]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2014-06-04 14:06:36]

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

[commit-bot: I haz the power, 2014-06-04 14:39:41]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-06-04 14:43:54]

Change committed as 274824