Remove dependency of HistoryService on WebHistoryServiceFactory

Remove dependency of HistoryService on WebHistoryServiceFactory

Change HistoryService::ExpireLocalAndRemoteHistoryBetween() to
receive the history::WebHistoryService instance to use in order
to remove a dependency of HistoryService on Profile.

It is not possible to pass the WebHistoryService instance to the
HistoryService constructor and add a dependency between the two
factories due to http://crbug.com/171406 (as WebHistoryService
depends on a KeyedService calling Profile::GetRequestContext()).

BUG=453790
Committed: https://crrev.com/f3b63c0bab333186be5848b3e0afc72d417d14cf
Cr-Commit-Position: refs/heads/master@{#315038}

[sdefresne, 2015-02-05 15:00:57]

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

[sdefresne, 2015-02-05 15:00:57]

The is a re-land of https://codereview.chromium.org/891763002.

The original CL was reverted as it blocked automatic revert of
https://codereview.chromium.org/889553003/ that introduced a crash during sync.

I've updated the CL to work even if WebHistoryService is still in
chrome/browser/history. I've also moved the ExpireLocalAndRemoveHistory method
to be a free function so that it is symmetry between HistoryService and
WebHistoryService.

droger: can you take a look?
bauerb: can you review //chrome/browser/browsing_data/ as OWNERS?

[droger, 2015-02-05 16:16:14]

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

https://codereview.chromium.org/901803005/diff/1/chrome/browser/history/histo...
chrome/browser/history/history_service.h:78: void
ExpireLocalAndRemoteHistoryBetween(HistoryService* local_history,
This looks like a mistake, the function is also declared in history_util.

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

https://codereview.chromium.org/901803005/diff/1/chrome/browser/history/histo...
chrome/browser/history/history_utils.h:2: // Use of this source code is governed
by a BSD-style license that can be
Nit: I think utility files are usually called "util", not "utils".

https://codereview.chromium.org/901803005/diff/1/chrome/browser/history/histo...
chrome/browser/history/history_utils.h:34: 
Remove on blank line.

[Bernhard Bauer, 2015-02-05 16:23:20]

So, what's the advantage of the symmetry here? If the method is declared as a
friend of the HistoryService, that suggests closer ties to it than to
WebHistoryService, which breaks the symmetry anyway.

Alternatively, what's the reason why HistoryService::ExpireHistoryBetween has to
be private? This gigantic list of friend declarations in HistoryService seems a
bit code-smelly to me.

[sdefresne, 2015-02-06 13:19:19]

On 2015/02/05 16:23:20, Bernhard Bauer wrote:
> So, what's the advantage of the symmetry here? If the method is declared as a
> friend of the HistoryService, that suggests closer ties to it than to
> WebHistoryService, which breaks the symmetry anyway.

In retrospect not much, so I've changed my CL back to what was reverted (i.e.
leave the methode defined on HistoryService taking a WebHistoryService as
parameter).

> Alternatively, what's the reason why HistoryService::ExpireHistoryBetween has
to
> be private? This gigantic list of friend declarations in HistoryService seems
a
> bit code-smelly to me.

The problem was not HistoryService::ExpireHistoryBetween, but
delete_directive_handler_ which is used for synchronisation (and that is oddly a
member of HistoryService and not WebHistoryService).

But as I said, I removed the free method, which remove the need for the friend
declaration. I've created http://crbug.com/456087 to investigate this.

Could you both (droger, bauerb) please take another look?

[Bernhard Bauer, 2015-02-06 13:41:17]

LGTM

Thank you, that's a lot nicer. The CL description needs to be updated though.

[sdefresne, 2015-02-06 13:45:26]

On 2015/02/06 13:41:17, Bernhard Bauer wrote:
> LGTM
> 
> Thank you, that's a lot nicer. The CL description needs to be updated though.

Thank you, done.

[droger, 2015-02-06 14:19:08]

lgtm

[sdefresne, 2015-02-06 14:19:41]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2015-02-06 14:20:37]

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

[commit-bot: I haz the power, 2015-02-06 14:23:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-06 14:23:07]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...)
  ios_rel_device_ninja_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[sdefresne, 2015-02-06 14:31:11]

New patchsets have been uploaded after l-g-t-m from
bauerb@chromium.org,droger@chromium.org

[sdefresne, 2015-02-06 14:31:15]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2015-02-06 14:31:34]

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

[commit-bot: I haz the power, 2015-02-06 15:42:27]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-02-06 15:43:15]

Patchset 3 (id:??) landed as
https://crrev.com/f3b63c0bab333186be5848b3e0afc72d417d14cf
Cr-Commit-Position: refs/heads/master@{#315038}