Remove NOTIFICATION_HISTORY_URL_VISITED

Remove NOTIFICATION_HISTORY_URL_VISITED

Port all client of the NOTIFICATION_HISTORY_URL_VISITED to instead be
HistoryServiceObserver or HistoryBackendObserver depending on the thread
they want the notification to be delivered.

Delete the notification and the notification details types.

BUG=373326
TBR=sky@chromium.org

Committed: https://crrev.com/bb1e5752f4c39a37892097460afce447226c457c
Cr-Commit-Position: refs/heads/master@{#300253}

[sdefresne, 2014-10-14 14:48:17]

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

[sdefresne, 2014-10-14 14:48:17]

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

[sdefresne, 2014-10-14 15:10:37]

sdefresne@chromium.org changed reviewers:
+ atwilson@chromium.org, bauerb@chromium.org, brettw@chromium.org,
kalman@chromium.org, pkasting@chromium.org, sky@chromium.org

[sdefresne, 2014-10-14 15:15:03]

This is a followup to https://codereview.chromium.org/631253002/ that refactor
client code to use the observer interface instead of the notification center.

droger: please take a look at the CL in its globality

Since this is refactoring, once it has been LGTM-ed, I plan to TBR the OWNERS
approval, so if you're in the below this line, you're there for ownership of
part of the code.

bauerb: as OWNERS of //chrome/browser/android/provider
atwilson: as OWNERS of //chrome/browser/sync/glue
pkasting: as OWNERS of //chrome/browser/autocomplete,
//chrome/browser/search_engines, //chrome/browser/ui/cocoa
sky: as OWNERS of //chrome/browser/chrome_notification_types.h
kalman: as OWNERS of //chrome/browser/extensions/api/history

[droger, 2014-10-14 15:15:24]

https://codereview.chromium.org/651193002/diff/1/chrome/browser/android/provi...
File chrome/browser/android/provider/chrome_browser_provider.cc (right):

https://codereview.chromium.org/651193002/diff/1/chrome/browser/android/provi...
chrome/browser/android/provider/chrome_browser_provider.cc:1625: JNIEnv* env =
AttachCurrentThread();
Is it possible to share some code with OnURLVisited()?

https://codereview.chromium.org/651193002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/history/history_api.cc (right):

https://codereview.chromium.org/651193002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/history/history_api.cc:170:
DCHECK(history_service_ == history_service);
DHCEK_EQ

https://codereview.chromium.org/651193002/diff/1/chrome/browser/search_engine...
File chrome/browser/search_engines/chrome_template_url_service_client.h (right):

https://codereview.chromium.org/651193002/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/chrome_template_url_service_client.h:11: #include
"content/public/browser/notification_registrar.h"
Remove these includes.

https://codereview.chromium.org/651193002/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/chrome_template_url_service_client.h:14: class
Profile;
Remove.

https://codereview.chromium.org/651193002/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/chrome_template_url_service_client.h:40:
base::Time visit_time) override;
You need includes or forward declarations for these types.

https://codereview.chromium.org/651193002/diff/1/chrome/browser/search_engine...
File chrome/browser/search_engines/template_url_service_test_util.cc (right):

https://codereview.chromium.org/651193002/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/template_url_service_test_util.cc:115:
DCHECK(profile()->CreateHistoryService(false, false));
Is it OK if this code is not called in release?

Or did you mean instead:
bool created = profile()->CreateHistoryService(false, false);
DCHECK(created);

https://codereview.chromium.org/651193002/diff/1/chrome/browser/ui/cocoa/hist...
File chrome/browser/ui/cocoa/history_menu_bridge.mm (right):

https://codereview.chromium.org/651193002/diff/1/chrome/browser/ui/cocoa/hist...
chrome/browser/ui/cocoa/history_menu_bridge.mm:171: need_recreate_ = true;
Should we share the code with Observe() for this?

[Andrew T Wilson (Slow), 2014-10-14 15:20:04]

sync lgtm

[sdefresne, 2014-10-14 16:02:05]

https://codereview.chromium.org/651193002/diff/1/chrome/browser/android/provi...
File chrome/browser/android/provider/chrome_browser_provider.cc (right):

https://codereview.chromium.org/651193002/diff/1/chrome/browser/android/provi...
chrome/browser/android/provider/chrome_browser_provider.cc:1625: JNIEnv* env =
AttachCurrentThread();
On 2014/10/14 15:15:24, droger wrote:
> Is it possible to share some code with OnURLVisited()?

Done.

https://codereview.chromium.org/651193002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/history/history_api.cc (right):

https://codereview.chromium.org/651193002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/history/history_api.cc:170:
DCHECK(history_service_ == history_service);
On 2014/10/14 15:15:24, droger wrote:
> DHCEK_EQ

Done.

https://codereview.chromium.org/651193002/diff/1/chrome/browser/search_engine...
File chrome/browser/search_engines/chrome_template_url_service_client.h (right):

https://codereview.chromium.org/651193002/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/chrome_template_url_service_client.h:11: #include
"content/public/browser/notification_registrar.h"
On 2014/10/14 15:15:24, droger wrote:
> Remove these includes.

Done.

https://codereview.chromium.org/651193002/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/chrome_template_url_service_client.h:14: class
Profile;
On 2014/10/14 15:15:24, droger wrote:
> Remove.

Done.

https://codereview.chromium.org/651193002/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/chrome_template_url_service_client.h:40:
base::Time visit_time) override;
On 2014/10/14 15:15:24, droger wrote:
> You need includes or forward declarations for these types.

They are forward-declared / included in
"components/history/core/browser/history_service_observer.h". Do I really need
to re-foward-declare / include them?

https://codereview.chromium.org/651193002/diff/1/chrome/browser/search_engine...
File chrome/browser/search_engines/template_url_service_test_util.cc (right):

https://codereview.chromium.org/651193002/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/template_url_service_test_util.cc:115:
DCHECK(profile()->CreateHistoryService(false, false));
On 2014/10/14 15:15:24, droger wrote:
> Is it OK if this code is not called in release?
> 
> Or did you mean instead:
> bool created = profile()->CreateHistoryService(false, false);
> DCHECK(created);

Done.

https://codereview.chromium.org/651193002/diff/1/chrome/browser/ui/cocoa/hist...
File chrome/browser/ui/cocoa/history_menu_bridge.mm (right):

https://codereview.chromium.org/651193002/diff/1/chrome/browser/ui/cocoa/hist...
chrome/browser/ui/cocoa/history_menu_bridge.mm:171: need_recreate_ = true;
On 2014/10/14 15:15:24, droger wrote:
> Should we share the code with Observe() for this?

Done.

[not at google - send to devlin, 2014-10-14 16:06:41]

lgtm

https://codereview.chromium.org/651193002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/history/history_api.cc (right):

https://codereview.chromium.org/651193002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/history/history_api.cc:140: const
content::Source<Profile> source = content::Source<Profile>(profile);
Remove this alias now?

https://codereview.chromium.org/651193002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/history/history_api.cc:148:
history_service_->RemoveObserver(this);
Incidentally - have a look at ScopedObserver.

https://code.google.com/p/chromium/codesearch#chromium/src/base/scoped_observ...

It helps a little with this boilerplate.

https://codereview.chromium.org/651193002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/history/history_api.cc:155: case
chrome::NOTIFICATION_HISTORY_URLS_DELETED:
Now that there's only 1 case, simpler to convert it to a DCHECK_EQ.

https://codereview.chromium.org/651193002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/history/history_api.h (right):

https://codereview.chromium.org/651193002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/history/history_api.h:33: explicit
HistoryEventRouter(Profile* profile,
explicit no longer needed.

[droger, 2014-10-14 16:11:09]

lgtm

https://codereview.chromium.org/651193002/diff/1/chrome/browser/search_engine...
File chrome/browser/search_engines/chrome_template_url_service_client.h (right):

https://codereview.chromium.org/651193002/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/chrome_template_url_service_client.h:40:
base::Time visit_time) override;
On 2014/10/14 16:02:05, sdefresne wrote:
> On 2014/10/14 15:15:24, droger wrote:
> > You need includes or forward declarations for these types.
> 
> They are forward-declared / included in
> "components/history/core/browser/history_service_observer.h". Do I really need
> to re-foward-declare / include them?

Ok, you are probably right.

https://codereview.chromium.org/651193002/diff/20001/chrome/browser/android/p...
File chrome/browser/android/provider/chrome_browser_provider.h (right):

https://codereview.chromium.org/651193002/diff/20001/chrome/browser/android/p...
chrome/browser/android/provider/chrome_browser_provider.h:184: void
OnHistoryChanged();
Do we need a comment for this function?

[Peter Kasting, 2014-10-14 16:40:54]

LGTM with question

https://codereview.chromium.org/651193002/diff/20001/chrome/browser/search_en...
File chrome/browser/search_engines/chrome_template_url_service_client.cc
(right):

https://codereview.chromium.org/651193002/diff/20001/chrome/browser/search_en...
chrome/browser/search_engines/chrome_template_url_service_client.cc:14: //
Register as HistoryServiceObserver.
Nit: This comment doesn't add anything to the code, I'd remove it.

https://codereview.chromium.org/651193002/diff/20001/chrome/browser/search_en...
chrome/browser/search_engines/chrome_template_url_service_client.cc:25:
history_service_->RemoveObserver(this);
This change adds a lifetime ordering requirement that the history service always
outlive this class.  Is that guaranteed?

[sdefresne, 2014-10-14 16:49:42]

https://codereview.chromium.org/651193002/diff/20001/chrome/browser/search_en...
File chrome/browser/search_engines/chrome_template_url_service_client.cc
(right):

https://codereview.chromium.org/651193002/diff/20001/chrome/browser/search_en...
chrome/browser/search_engines/chrome_template_url_service_client.cc:14: //
Register as HistoryServiceObserver.
On 2014/10/14 16:40:53, Peter Kasting wrote:
> Nit: This comment doesn't add anything to the code, I'd remove it.

Done.

https://codereview.chromium.org/651193002/diff/20001/chrome/browser/search_en...
chrome/browser/search_engines/chrome_template_url_service_client.cc:25:
history_service_->RemoveObserver(this);
On 2014/10/14 16:40:53, Peter Kasting wrote:
> This change adds a lifetime ordering requirement that the history service
always
> outlive this class.  Is that guaranteed?

Yes. ChromeTemplateURLServiceClient is owned by TemplateURLService which is a
KeyedService and the TemplateURLServiceFactory has a dependency on
HistoryServiceFactory, so the TemplateURLService outlives the HistoryService.

[Peter Kasting, 2014-10-14 16:50:40]

https://codereview.chromium.org/651193002/diff/20001/chrome/browser/search_en...
File chrome/browser/search_engines/chrome_template_url_service_client.cc
(right):

https://codereview.chromium.org/651193002/diff/20001/chrome/browser/search_en...
chrome/browser/search_engines/chrome_template_url_service_client.cc:25:
history_service_->RemoveObserver(this);
On 2014/10/14 16:49:42, sdefresne wrote:
> On 2014/10/14 16:40:53, Peter Kasting wrote:
> > This change adds a lifetime ordering requirement that the history service
> always
> > outlive this class.  Is that guaranteed?
> 
> Yes. ChromeTemplateURLServiceClient is owned by TemplateURLService which is a
> KeyedService and the TemplateURLServiceFactory has a dependency on
> HistoryServiceFactory, so the TemplateURLService outlives the HistoryService.

OK.  I would not be opposed to a comment that explicitly spells that out.

[brettw, 2014-10-14 18:16:57]

lgtm

https://codereview.chromium.org/651193002/diff/40001/chrome/browser/android/p...
File chrome/browser/android/provider/chrome_browser_provider.h (right):

https://codereview.chromium.org/651193002/diff/40001/chrome/browser/android/p...
chrome/browser/android/provider/chrome_browser_provider.h:200: Profile*
profile_;
Can you add a note above this block of profile-related stuff about the lifetime
of this class relative to the profile stuff, so it's clear why these raw
pointers are OK?

[Bernhard Bauer, 2014-10-15 12:24:52]

android LGTM

https://codereview.chromium.org/651193002/diff/40001/chrome/browser/android/p...
File chrome/browser/android/provider/chrome_browser_provider.cc (right):

https://codereview.chromium.org/651193002/diff/40001/chrome/browser/android/p...
chrome/browser/android/provider/chrome_browser_provider.cc:1184:
history_service_->RemoveObserver(this);
Maybe you could use a ScopedObserver to take care of this, if you don't need the
history service otherwise.

[sdefresne, 2014-10-17 13:01:37]

Patchset #10 (id:180001) has been deleted

[sdefresne, 2014-10-17 13:01:47]

Patchset #9 (id:160001) has been deleted

[sdefresne, 2014-10-17 13:01:54]

Patchset #8 (id:140001) has been deleted

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

Patchset #7 (id:120001) has been deleted

[sdefresne, 2014-10-17 13:02:12]

Patchset #4 (id:60001) has been deleted

[sdefresne, 2014-10-17 13:02:20]

Patchset #4 (id:80001) has been deleted

[sdefresne, 2014-10-17 13:02:28]

Patchset #4 (id:100001) has been deleted

[sdefresne, 2014-10-17 13:02:44]

Patchset #4 (id:100001) has been deleted

[sdefresne, 2014-10-17 13:05:03]

Patchset #4 (id:200001) has been deleted

[sdefresne, 2014-10-17 13:35:48]

Patchset #4 (id:220001) has been deleted

[sdefresne, 2014-10-17 13:35:59]

Patchset #4 (id:220001) has been deleted

[sdefresne, 2014-10-17 14:32:39]

pkasting: could you take another look as the issue you pointed about lifetime
was correct and I had to introduce a Shutdown() method to deal with it.

sky: ping for //chrome/browser/chrome_notification_types.h

https://codereview.chromium.org/651193002/diff/20001/chrome/browser/android/p...
File chrome/browser/android/provider/chrome_browser_provider.h (right):

https://codereview.chromium.org/651193002/diff/20001/chrome/browser/android/p...
chrome/browser/android/provider/chrome_browser_provider.h:184: void
OnHistoryChanged();
On 2014/10/14 16:11:09, droger wrote:
> Do we need a comment for this function?

Done.

https://codereview.chromium.org/651193002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/history/history_api.cc (right):

https://codereview.chromium.org/651193002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/history/history_api.cc:140: const
content::Source<Profile> source = content::Source<Profile>(profile);
On 2014/10/14 16:06:41, kalman wrote:
> Remove this alias now?

Done.

https://codereview.chromium.org/651193002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/history/history_api.cc:148:
history_service_->RemoveObserver(this);
On 2014/10/14 16:06:41, kalman wrote:
> Incidentally - have a look at ScopedObserver.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/scoped_observ...
> 
> It helps a little with this boilerplate.

Done.

https://codereview.chromium.org/651193002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/history/history_api.cc:155: case
chrome::NOTIFICATION_HISTORY_URLS_DELETED:
On 2014/10/14 16:06:41, kalman wrote:
> Now that there's only 1 case, simpler to convert it to a DCHECK_EQ.

Done.

https://codereview.chromium.org/651193002/diff/20001/chrome/browser/search_en...
File chrome/browser/search_engines/chrome_template_url_service_client.cc
(right):

https://codereview.chromium.org/651193002/diff/20001/chrome/browser/search_en...
chrome/browser/search_engines/chrome_template_url_service_client.cc:25:
history_service_->RemoveObserver(this);
On 2014/10/14 16:50:39, Peter Kasting wrote:
> On 2014/10/14 16:49:42, sdefresne wrote:
> > On 2014/10/14 16:40:53, Peter Kasting wrote:
> > > This change adds a lifetime ordering requirement that the history service
> > always
> > > outlive this class.  Is that guaranteed?
> > 
> > Yes. ChromeTemplateURLServiceClient is owned by TemplateURLService which is
a
> > KeyedService and the TemplateURLServiceFactory has a dependency on
> > HistoryServiceFactory, so the TemplateURLService outlives the
HistoryService.
> 
> OK.  I would not be opposed to a comment that explicitly spells that out.

My comment was correct but my analysis wasn't.

ChromeTemplateURLServiceClient currently outlives HistoryService. I've
introduced a Shutdown() method that will be called from the
TemplateUTLServiceFactory::Shutdown() to remove the observer while the
HistoryService is still alive.

https://codereview.chromium.org/651193002/diff/40001/chrome/browser/android/p...
File chrome/browser/android/provider/chrome_browser_provider.cc (right):

https://codereview.chromium.org/651193002/diff/40001/chrome/browser/android/p...
chrome/browser/android/provider/chrome_browser_provider.cc:1184:
history_service_->RemoveObserver(this);
On 2014/10/15 12:24:52, Bernhard Bauer wrote:
> Maybe you could use a ScopedObserver to take care of this, if you don't need
the
> history service otherwise.

Done.

https://codereview.chromium.org/651193002/diff/40001/chrome/browser/android/p...
File chrome/browser/android/provider/chrome_browser_provider.h (right):

https://codereview.chromium.org/651193002/diff/40001/chrome/browser/android/p...
chrome/browser/android/provider/chrome_browser_provider.h:200: Profile*
profile_;
On 2014/10/14 18:16:57, brettw wrote:
> Can you add a note above this block of profile-related stuff about the
lifetime
> of this class relative to the profile stuff, so it's clear why these raw
> pointers are OK?

Done.

[Peter Kasting, 2014-10-17 20:41:03]

https://codereview.chromium.org/651193002/diff/240001/chrome/browser/search_e...
File chrome/browser/search_engines/chrome_template_url_service_client.cc
(right):

https://codereview.chromium.org/651193002/diff/240001/chrome/browser/search_e...
chrome/browser/search_engines/chrome_template_url_service_client.cc:27: //
KeyedService with a dependency on HistoryService. Since it outlive the
Nit: outlives

This seems weird, though.  If TemplateURLService depends on HistoryService,
doesn't that mean HistoryService should be created first and destroyed last? 
Why would TemplateURLService outlive HistoryService?

https://codereview.chromium.org/651193002/diff/240001/chrome/browser/search_e...
chrome/browser/search_engines/chrome_template_url_service_client.cc:29: // steps
shutdown of the KeyedService so that we can remove the observer
Nit: two steps -> two-phase

https://codereview.chromium.org/651193002/diff/240001/chrome/browser/search_e...
chrome/browser/search_engines/chrome_template_url_service_client.cc:30: //
before the Historyservice becomes invalid.
Nit: the Historyservice -> |history_service_|

https://codereview.chromium.org/651193002/diff/240001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/history_menu_bridge.h (right):

https://codereview.chromium.org/651193002/diff/240001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/history_menu_bridge.h:136: //
history::HistoryServiceObserver:
Nit: Please place overrides for classes in the same order as the classes are
declared in lines 60-63

[sdefresne, 2014-10-18 17:31:47]

https://codereview.chromium.org/651193002/diff/240001/chrome/browser/search_e...
File chrome/browser/search_engines/chrome_template_url_service_client.cc
(right):

https://codereview.chromium.org/651193002/diff/240001/chrome/browser/search_e...
chrome/browser/search_engines/chrome_template_url_service_client.cc:27: //
KeyedService with a dependency on HistoryService. Since it outlive the
On 2014/10/17 20:41:02, Peter Kasting wrote:
> Nit: outlives
> 
> This seems weird, though.  If TemplateURLService depends on HistoryService,
> doesn't that mean HistoryService should be created first and destroyed last? 
> Why would TemplateURLService outlive HistoryService?

You're right. I don't know why I though the opposite on Friday. I'll remove this
Shutdown() method and add a comment along the lines of my previous reply. Sorry
for the noise.

https://codereview.chromium.org/651193002/diff/240001/chrome/browser/search_e...
chrome/browser/search_engines/chrome_template_url_service_client.cc:29: // steps
shutdown of the KeyedService so that we can remove the observer
On 2014/10/17 20:41:02, Peter Kasting wrote:
> Nit: two steps -> two-phase

Acknowledged.

https://codereview.chromium.org/651193002/diff/240001/chrome/browser/search_e...
chrome/browser/search_engines/chrome_template_url_service_client.cc:30: //
before the Historyservice becomes invalid.
On 2014/10/17 20:41:03, Peter Kasting wrote:
> Nit: the Historyservice -> |history_service_|

Acknowledged.

[sdefresne, 2014-10-20 12:03:32]

I've clarified the comment for ChromeTemplateURLClient but didn't make any code
change. Thank you all for the reviews.

sky: TBR the removal of the enum value declaration

[sdefresne, 2014-10-20 12:03:39]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2014-10-20 12:03:50]

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

[commit-bot: I haz the power, 2014-10-20 12:31:25]

Committed patchset #5 (id:260001)

[commit-bot: I haz the power, 2014-10-20 12:32:12]

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

[sky, 2014-10-20 19:54:36]

LGTM