Converting BookmarkModel and HistoryService to ProfileKeyedServices. This just performs the initial…

Converting BookmarkModel and HistoryService to ProfileKeyedServices. This just performs the initial conversion. Separate CLs to take care of the removal of profile_->Get<Serivce> will follow.

BUG=97804, 112525
TEST=no new, passes existing unittests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=138867

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=141294

[rpetterson, 2012-05-18 22:32:44]

erg - profiles
sky - history, bookmarks, test

http://codereview.chromium.org/10399087/diff/5003/chrome/browser/bookmarks/bo...
File chrome/browser/bookmarks/bookmark_expanded_state_tracker.cc (right):

http://codereview.chromium.org/10399087/diff/5003/chrome/browser/bookmarks/bo...
chrome/browser/bookmarks/bookmark_expanded_state_tracker.cc:16: BookmarkModel*
bookmark_model)
We need to pass this in because this is initialized during load and calling
GetForProfile won't work yet, resulting in problems when AddObserver is called
on bookmark_model.

[Elliot Glaysher, 2012-05-18 22:42:04]

profile lgtm

[sky, 2012-05-21 14:23:06]

http://codereview.chromium.org/10399087/diff/5003/chrome/browser/bookmarks/bo...
File chrome/browser/bookmarks/bookmark_expanded_state_tracker.cc (right):

http://codereview.chromium.org/10399087/diff/5003/chrome/browser/bookmarks/bo...
chrome/browser/bookmarks/bookmark_expanded_state_tracker.cc:25: void
BookmarkExpandedStateTracker::Cleanup() {
Move this to BookmarkModelBeingDeleted

http://codereview.chromium.org/10399087/diff/5003/chrome/browser/prefs/browse...
File chrome/browser/prefs/browser_prefs.cc (right):

http://codereview.chromium.org/10399087/diff/5003/chrome/browser/prefs/browse...
chrome/browser/prefs/browser_prefs.cc:10: #include
"chrome/browser/bookmarks/bookmark_model.h"
Can you remove some includes now?

http://codereview.chromium.org/10399087/diff/5003/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (left):

http://codereview.chromium.org/10399087/diff/5003/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:537: // HistoryService may call into the
BookmarkModel, as such we need to
How is this handled now?

[rpetterson, 2012-05-21 18:01:49]

http://codereview.chromium.org/10399087/diff/5003/chrome/browser/bookmarks/bo...
File chrome/browser/bookmarks/bookmark_expanded_state_tracker.cc (right):

http://codereview.chromium.org/10399087/diff/5003/chrome/browser/bookmarks/bo...
chrome/browser/bookmarks/bookmark_expanded_state_tracker.cc:25: void
BookmarkExpandedStateTracker::Cleanup() {
On 2012/05/21 14:23:06, sky wrote:
> Move this to BookmarkModelBeingDeleted

Done.

http://codereview.chromium.org/10399087/diff/5003/chrome/browser/prefs/browse...
File chrome/browser/prefs/browser_prefs.cc (right):

http://codereview.chromium.org/10399087/diff/5003/chrome/browser/prefs/browse...
chrome/browser/prefs/browser_prefs.cc:10: #include
"chrome/browser/bookmarks/bookmark_model.h"
On 2012/05/21 14:23:06, sky wrote:
> Can you remove some includes now?

Done.

http://codereview.chromium.org/10399087/diff/5003/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (left):

http://codereview.chromium.org/10399087/diff/5003/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:537: // HistoryService may call into the
BookmarkModel, as such we need to
On 2012/05/21 14:23:06, sky wrote:
> How is this handled now?

HistoryService is marked as being dependent on the BookmarkModel. This means
that when shutdown occurs, we'll delete HistoryService before the BookmarkModel
based on the dependency graph.

In order to make sure the cleanup is performed in the correct order, when we
call Shutdown on the HistoryService, it will also call the BookmarkModel's
Shutdown function (see HistoryService::ShutdownOnUIThread
(http://codereview.chromium.org/10399087/patch/5003/7012). The BookmarkModel's
Shutdown method will also get called again when it is deleted, but releasing
that signal again shouldn't matter if history has already been deleted.

By putting the signal code in BookmarkModel::Shutdown, we ensure it gets called
whether there is a history service or not. If this is unnecessary, I can move it
back to being in a Cleanup method.

[sky, 2012-05-21 20:37:17]

LGTM

http://codereview.chromium.org/10399087/diff/5003/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (left):

http://codereview.chromium.org/10399087/diff/5003/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:537: // HistoryService may call into the
BookmarkModel, as such we need to
On 2012/05/21 18:01:49, rpetterson wrote:
> On 2012/05/21 14:23:06, sky wrote:
> > How is this handled now?
> 
> HistoryService is marked as being dependent on the BookmarkModel. This means
> that when shutdown occurs, we'll delete HistoryService before the
BookmarkModel
> based on the dependency graph.
> 
> In order to make sure the cleanup is performed in the correct order, when we
> call Shutdown on the HistoryService, it will also call the BookmarkModel's
> Shutdown function (see HistoryService::ShutdownOnUIThread
> (http://codereview.chromium.org/10399087/patch/5003/7012). The BookmarkModel's
> Shutdown method will also get called again when it is deleted, but releasing
> that signal again shouldn't matter if history has already been deleted.
> 
> By putting the signal code in BookmarkModel::Shutdown, we ensure it gets
called
> whether there is a history service or not. If this is unnecessary, I can move
it
> back to being in a Cleanup method.

What you have is fine. I just wanted to make sure I understood how it's working.

[commit-bot: I haz the power, 2012-05-24 00:30:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlp@chromium.org/10399087/17001

[commit-bot: I haz the power, 2012-05-24 05:23:41]

Try job failure for 10399087-17001 (retry) on win_rel for step "update".
It's a second try, previously, steps "browser_tests, unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-05-24 18:16:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlp@chromium.org/10399087/17001

[commit-bot: I haz the power, 2012-05-24 19:46:54]

Change committed as 138867

[commit-bot: I haz the power, 2012-06-08 20:58:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlp@chromium.org/10399087/55001

[commit-bot: I haz the power, 2012-06-08 21:54:26]

Change committed as 141294