Refactor sending NOTIFICATION_HISTORY_URL_VISITED

Refactor sending NOTIFICATION_HISTORY_URL_VISITED

Refactor //chrome/browser/history code sending the notification
NOTIFICATION_HISTORY_URL_VISITED so that the notification is not
send from the core of the component.

Introduce HistoryServiceObserver to follow the observer pattern
instead of notification and use this pattern in the core history
code.

Introduce HistoryBackendObserver to follow the observer pattern
instead of notification in the history thread.

ChromeHistoryClient forward the HistoryServiceObserver events to
the notification system so that porting the clients can be on a
case by case basis and does not block the componentization work.

BUG=373326
Committed: https://crrev.com/c8eb77ed61f9c44efac034bd6bd3a0a6ba553cfb
Cr-Commit-Position: refs/heads/master@{#299719}

[sdefresne, 2014-10-07 14:35:44]

sdefresne@chromium.org changed reviewers:
+ blundell@chromium.org, brettw@chromium.org

[sdefresne, 2014-10-07 14:36:47]

blundell: can you take a look?

brettw: I think this is just simple refactoring and fit in the temporary owners
agreement => FYI

[brettw, 2014-10-07 16:47:39]

I only looked at a few things but this seems reasonable.

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

https://codereview.chromium.org/631253002/diff/1/chrome/browser/history/histo...
chrome/browser/history/history_backend.h:132: const base::Time& visit_time) = 0;
I usually pass times by copy, since a time is really an int64 and copying is
usually cheaper than dereferencing the reference. You can keep it this way if
you really like, though.

[blundell, 2014-10-08 09:59:14]

LGTM with questions/nits

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

https://codereview.chromium.org/631253002/diff/1/chrome/browser/history/chrom...
chrome/browser/history/chrome_history_client.cc:98:
scoped_ptr<history::URLVisitedDetails> details(
This should have a TODO and bug reference about removing this notification.

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

https://codereview.chromium.org/631253002/diff/1/chrome/browser/history/histo...
chrome/browser/history/history_backend_unittest.cc:257:
details.PassAs<history::HistoryDetails>());
nit: I don't think PassAs is necessary anymore.

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

https://codereview.chromium.org/631253002/diff/1/chrome/browser/history/histo...
chrome/browser/history/history_service_factory.cc:81:
history_client->SetHistoryService(history_service.get());
Will we be able to remove this once everything is converted to observers? If so,
add a TODO to that effect. Circular references between keyed services are kind
of nasty, as you know :).

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

https://codereview.chromium.org/631253002/diff/1/components/history/core/brow...
components/history/core/browser/history_service_observer.h:17: class
HistoryServiceObserver {
Nit: I prefer the pattern of HistoryService::Observer. Did you have a strong
reason for separating it out?

[sdefresne, 2014-10-09 09:38:16]

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

https://codereview.chromium.org/631253002/diff/1/chrome/browser/history/chrom...
chrome/browser/history/chrome_history_client.cc:98:
scoped_ptr<history::URLVisitedDetails> details(
On 2014/10/08 09:59:13, blundell wrote:
> This should have a TODO and bug reference about removing this notification.

Done.

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

https://codereview.chromium.org/631253002/diff/1/chrome/browser/history/histo...
chrome/browser/history/history_backend.h:132: const base::Time& visit_time) = 0;
On 2014/10/07 16:47:39, brettw wrote:
> I usually pass times by copy, since a time is really an int64 and copying is
> usually cheaper than dereferencing the reference. You can keep it this way if
> you really like, though.

Apparently not using const reference is the most common pattern. Done.

$ git grep 'const base::Time&' | wc -l
    1057
$ git grep 'base::Time' | grep -v 'const base::Time&' | wc -l
   21682

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

https://codereview.chromium.org/631253002/diff/1/chrome/browser/history/histo...
chrome/browser/history/history_backend_unittest.cc:257:
details.PassAs<history::HistoryDetails>());
On 2014/10/08 09:59:13, blundell wrote:
> nit: I don't think PassAs is necessary anymore.

Done.

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

https://codereview.chromium.org/631253002/diff/1/chrome/browser/history/histo...
chrome/browser/history/history_service_factory.cc:81:
history_client->SetHistoryService(history_service.get());
On 2014/10/08 09:59:13, blundell wrote:
> Will we be able to remove this once everything is converted to observers? If
so,
> add a TODO to that effect. Circular references between keyed services are kind
> of nasty, as you know :).

Done.

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

https://codereview.chromium.org/631253002/diff/1/components/history/core/brow...
components/history/core/browser/history_service_observer.h:17: class
HistoryServiceObserver {
On 2014/10/08 09:59:13, blundell wrote:
> Nit: I prefer the pattern of HistoryService::Observer. Did you have a strong
> reason for separating it out?

As discussed offline, will do this in a followup CL.

[sdefresne, 2014-10-09 09:39:34]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2014-10-09 09:40:43]

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

[commit-bot: I haz the power, 2014-10-09 10:11:27]

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

[commit-bot: I haz the power, 2014-10-09 10:11:28]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[sdefresne, 2014-10-13 17:31:32]

sdefresne@chromium.org changed reviewers:
+ phajdan.jr@chromium.org, zea@chromium.org

[sdefresne, 2014-10-13 17:31:33]

phajdan.jr@chromium.org: Please review changes in //chrome/test/base

zea@chromium.org: Please review changes in //chrome/browser/sync
Note I had to introduce the HistoryBackendObserver just for that code since it
expected the notification to be delivered in the history thread instead of the
UI thread (where the rest of the code expected it).

[sdefresne, 2014-10-13 17:32:32]

On 2014/10/13 17:31:33, sdefresne wrote:
> mailto:phajdan.jr@chromium.org: Please review changes in //chrome/test/base
> 
> mailto:zea@chromium.org: Please review changes in //chrome/browser/sync
> Note I had to introduce the HistoryBackendObserver just for that code since it
> expected the notification to be delivered in the history thread instead of the
> UI thread (where the rest of the code expected it).

Oh, and ignore the bots, I forgot that the CL is based on a CL not submitted
upstream and thus the patch is failing.

[Paweł Hajdan Jr., 2014-10-14 15:15:20]

chrome/test/base LGTM

[Nicolas Zea, 2014-10-14 17:46:28]

sync LGTM

[sdefresne, 2014-10-15 17:26:18]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2014-10-15 17:27:40]

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

[commit-bot: I haz the power, 2014-10-15 18:15:53]

Committed patchset #9 (id:580001)

[commit-bot: I haz the power, 2014-10-15 18:16:51]

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