Fix componentization of chrome/browser/bookmarks

Fix componentization of chrome/browser/bookmarks

Add new class ManagedBookmarkService into //components/bookmarks/managed
responsible for managed and supervised bookmarks creation and policy
enforcement.

BookmarkClient delegates policy decisions to ManagedBookmarkService (and
use sane default if there is no ManagedBookmarkService instance). Remove
all direct access to ChromeBookmarkClient and instead direct queries to
ManagedBookmarkService.

Move prefs registration to a static method of BookmarkModel so that iOS
can reuse it.

BUG=383583, 359565
Committed: https://crrev.com/57ec39ec14d7ebf9747d3855e2c13cfb76235e28
Cr-Commit-Position: refs/heads/master@{#339639}

[sdefresne, 2015-07-10 16:30:44]

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

[sdefresne, 2015-07-10 16:30:45]

Please take a look.

This is the implementation I proposed in a comment of
https://codereview.chromium.org/1203713002. All the policy related information
is either in //chrome or //components/bookmarks/managed.

[tfarina, 2015-07-10 17:29:05]

tfarina@chromium.org changed reviewers:
+ tfarina@chromium.org

[tfarina, 2015-07-10 17:29:06]

https://codereview.chromium.org/1233673002/diff/1/components/bookmarks/browse...
File components/bookmarks/browser/bookmark_model.cc (right):

https://codereview.chromium.org/1233673002/diff/1/components/bookmarks/browse...
components/bookmarks/browser/bookmark_model.cc:126: void
BookmarkModel::RegisterProfilePrefs(
Why does this needs to me a static method on BookmarkModel?

We already have a helper function for this in bookmark_utils.h, see
https://code.google.com/p/chromium/codesearch#chromium/src/components/bookmar...

Can you use that instead? If not, why?

[tfarina, 2015-07-10 17:30:52]

https://codereview.chromium.org/1233673002/diff/1/components/bookmarks/browse...
File components/bookmarks/browser/bookmark_model.h (right):

https://codereview.chromium.org/1233673002/diff/1/components/bookmarks/browse...
components/bookmarks/browser/bookmark_model.h:79: static void
RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
By the way, I would do that separately, since it does not have anything to do
with Managed stuff, besides the Supervised pref. So I think that can be done in
another CL. Would you be OK to put that in another CL?

[sdefresne, 2015-07-16 07:23:33]

https://codereview.chromium.org/1233673002/diff/1/components/bookmarks/browse...
File components/bookmarks/browser/bookmark_model.cc (right):

https://codereview.chromium.org/1233673002/diff/1/components/bookmarks/browse...
components/bookmarks/browser/bookmark_model.cc:126: void
BookmarkModel::RegisterProfilePrefs(
On 2015/07/10 at 17:29:06, tfarina wrote:
> Why does this needs to me a static method on BookmarkModel?

Does not have to be a static method on BookmarkModel.

> We already have a helper function for this in bookmark_utils.h, see
https://code.google.com/p/chromium/codesearch#chromium/src/components/bookmar...
> 
> Can you use that instead? If not, why?

Thank you for pointing to that method. I'll see if I can put it there (and check
how I can have the method called only once).

https://codereview.chromium.org/1233673002/diff/1/components/bookmarks/browse...
File components/bookmarks/browser/bookmark_model.h (right):

https://codereview.chromium.org/1233673002/diff/1/components/bookmarks/browse...
components/bookmarks/browser/bookmark_model.h:79: static void
RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
On 2015/07/10 at 17:30:52, tfarina wrote:
> By the way, I would do that separately, since it does not have anything to do
with Managed stuff, besides the Supervised pref. So I think that can be done in
another CL. Would you be OK to put that in another CL?

Make sense, removed from this CL and will do in a followup CL.

[sky, 2015-07-20 15:53:20]

https://codereview.chromium.org/1233673002/diff/20001/chrome/browser/bookmark...
File chrome/browser/bookmarks/chrome_bookmark_client.cc (right):

https://codereview.chromium.org/1233673002/diff/20001/chrome/browser/bookmark...
chrome/browser/bookmarks/chrome_bookmark_client.cc:45: bool
IsPermanentNodeValid(
I was a bit confused by Valid in the name. How about IsPermanentNode?

https://codereview.chromium.org/1233673002/diff/20001/chrome/browser/bookmark...
chrome/browser/bookmarks/chrome_bookmark_client.cc:75:
managed_bookmark_service_->BookmarkModelCreated(model);
As managed_bookmark_service_ is a BookmarkModelObserver can it override
BookmarkModelLoaded rather than explicitly calling BookmarkModelCreated?

https://codereview.chromium.org/1233673002/diff/20001/chrome/browser/bookmark...
File chrome/browser/bookmarks/managed_bookmark_service_unittest.cc (right):

https://codereview.chromium.org/1233673002/diff/20001/chrome/browser/bookmark...
chrome/browser/bookmarks/managed_bookmark_service_unittest.cc:18: #include
"components/bookmarks/managed/managed_bookmark_service.h"
nit: this should be the first include (line 5 old).

https://codereview.chromium.org/1233673002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/bookmarks/bookmark_api_helpers.h (right):

https://codereview.chromium.org/1233673002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/bookmarks/bookmark_api_helpers.h:37: void
AddNodeFoldersOnly(
How about a description?

https://codereview.chromium.org/1233673002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bubble_controller.h (right):

https://codereview.chromium.org/1233673002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_bubble_controller.h:29:
bookmarks::ManagedBookmarkService* managed_;  // weak
managedBookmarkService_, like you have in
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.h .

https://codereview.chromium.org/1233673002/diff/20001/components/bookmarks/ma...
File components/bookmarks/managed/managed_bookmark_service.cc (right):

https://codereview.chromium.org/1233673002/diff/20001/components/bookmarks/ma...
components/bookmarks/managed/managed_bookmark_service.cc:54: for (const auto&
loader : *loaders.get()) {
nit: no {}

https://codereview.chromium.org/1233673002/diff/20001/components/bookmarks/ma...
components/bookmarks/managed/managed_bookmark_service.cc:105:
&LoadNodeInitialContent, base::Passed(&managed),
As you always call LoadNodeInitialContext is there a reason to pass around a
callback? It makes the code harder to follow.

[sdefresne, 2015-07-20 17:43:40]

Thank you for the review. Please take another look.

https://codereview.chromium.org/1233673002/diff/20001/chrome/browser/bookmark...
File chrome/browser/bookmarks/chrome_bookmark_client.cc (right):

https://codereview.chromium.org/1233673002/diff/20001/chrome/browser/bookmark...
chrome/browser/bookmarks/chrome_bookmark_client.cc:45: bool
IsPermanentNodeValid(
On 2015/07/20 at 15:53:19, sky wrote:
> I was a bit confused by Valid in the name. How about IsPermanentNode?

Done.

https://codereview.chromium.org/1233673002/diff/20001/chrome/browser/bookmark...
chrome/browser/bookmarks/chrome_bookmark_client.cc:75:
managed_bookmark_service_->BookmarkModelCreated(model);
On 2015/07/20 at 15:53:19, sky wrote:
> As managed_bookmark_service_ is a BookmarkModelObserver can it override
BookmarkModelLoaded rather than explicitly calling BookmarkModelCreated?

ManagedBookmarkService needs to use the BookmarkModel before it has started
loading (as it returns a callback that will be invoked during BookmarkModel
loading). It is thus not possible to listen to BookmarkModelLoaded and remove
this call as the event will happen too late.

Another option is to change the interface of GetLoadExtraNodesCallback() to
receive the BookmarkModel and then we can remove this Init() method and
ManagedBookmarkService::BookmarkModelCreated().

https://codereview.chromium.org/1233673002/diff/20001/chrome/browser/bookmark...
File chrome/browser/bookmarks/managed_bookmark_service_unittest.cc (right):

https://codereview.chromium.org/1233673002/diff/20001/chrome/browser/bookmark...
chrome/browser/bookmarks/managed_bookmark_service_unittest.cc:18: #include
"components/bookmarks/managed/managed_bookmark_service.h"
On 2015/07/20 at 15:53:19, sky wrote:
> nit: this should be the first include (line 5 old).

Done.

https://codereview.chromium.org/1233673002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/bookmarks/bookmark_api_helpers.h (right):

https://codereview.chromium.org/1233673002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/bookmarks/bookmark_api_helpers.h:37: void
AddNodeFoldersOnly(
On 2015/07/20 at 15:53:19, sky wrote:
> How about a description?

I only changed the first parameter of this method to be a ManagedBookmarkService
instead of a ChromeBookmarkClient, I may not be the most qualified person to add
a description. I've tried to add one, but not sure if it is correct.

https://codereview.chromium.org/1233673002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bubble_controller.h (right):

https://codereview.chromium.org/1233673002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_bubble_controller.h:29:
bookmarks::ManagedBookmarkService* managed_;  // weak
On 2015/07/20 at 15:53:19, sky wrote:
> managedBookmarkService_, like you have in
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.h .

Done.

https://codereview.chromium.org/1233673002/diff/20001/components/bookmarks/ma...
File components/bookmarks/managed/managed_bookmark_service.cc (right):

https://codereview.chromium.org/1233673002/diff/20001/components/bookmarks/ma...
components/bookmarks/managed/managed_bookmark_service.cc:54: for (const auto&
loader : *loaders.get()) {
On 2015/07/20 at 15:53:19, sky wrote:
> nit: no {}

Done.

https://codereview.chromium.org/1233673002/diff/20001/components/bookmarks/ma...
components/bookmarks/managed/managed_bookmark_service.cc:105:
&LoadNodeInitialContent, base::Passed(&managed),
On 2015/07/20 at 15:53:20, sky wrote:
> As you always call LoadNodeInitialContext is there a reason to pass around a
callback? It makes the code harder to follow.

I've used objects instead. Is it clearer now?

[sky, 2015-07-20 20:10:56]

LGTM

https://codereview.chromium.org/1233673002/diff/20001/chrome/browser/bookmark...
File chrome/browser/bookmarks/chrome_bookmark_client.cc (right):

https://codereview.chromium.org/1233673002/diff/20001/chrome/browser/bookmark...
chrome/browser/bookmarks/chrome_bookmark_client.cc:75:
managed_bookmark_service_->BookmarkModelCreated(model);
On 2015/07/20 17:43:40, sdefresne wrote:
> On 2015/07/20 at 15:53:19, sky wrote:
> > As managed_bookmark_service_ is a BookmarkModelObserver can it override
> BookmarkModelLoaded rather than explicitly calling BookmarkModelCreated?
> 
> ManagedBookmarkService needs to use the BookmarkModel before it has started
> loading (as it returns a callback that will be invoked during BookmarkModel
> loading). It is thus not possible to listen to BookmarkModelLoaded and remove
> this call as the event will happen too late.
> 
> Another option is to change the interface of GetLoadExtraNodesCallback() to
> receive the BookmarkModel and then we can remove this Init() method and
> ManagedBookmarkService::BookmarkModelCreated().

Ok, leave what you have.

https://codereview.chromium.org/1233673002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/bookmarks/bookmark_api_helpers.h (right):

https://codereview.chromium.org/1233673002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/bookmarks/bookmark_api_helpers.h:37: void
AddNodeFoldersOnly(
On 2015/07/20 17:43:40, sdefresne wrote:
> On 2015/07/20 at 15:53:19, sky wrote:
> > How about a description?
> 
> I only changed the first parameter of this method to be a
ManagedBookmarkService
> instead of a ChromeBookmarkClient, I may not be the most qualified person to
add
> a description. I've tried to add one, but not sure if it is correct.

I realize you only change the args. Feel free to remove your comment and we'll
get someone else to add one at a later date.

https://codereview.chromium.org/1233673002/diff/20001/components/bookmarks/ma...
File components/bookmarks/managed/managed_bookmark_service.cc (right):

https://codereview.chromium.org/1233673002/diff/20001/components/bookmarks/ma...
components/bookmarks/managed/managed_bookmark_service.cc:105:
&LoadNodeInitialContent, base::Passed(&managed),
On 2015/07/20 17:43:40, sdefresne wrote:
> On 2015/07/20 at 15:53:20, sky wrote:
> > As you always call LoadNodeInitialContext is there a reason to pass around a
> callback? It makes the code harder to follow.
> 
> I've used objects instead. Is it clearer now?

Yes, thanks!

[sdefresne, 2015-07-21 08:12:04]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2015-07-21 08:12:05]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1233673002/#ps60001
(title: "Fix "gn check" and compilation on Mac")

[commit-bot: I haz the power, 2015-07-21 08:12:17]

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

[commit-bot: I haz the power, 2015-07-21 09:05:22]

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

[commit-bot: I haz the power, 2015-07-21 09:05:23]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[sdefresne, 2015-07-21 12:08:27]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2015-07-21 12:08:33]

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

[commit-bot: I haz the power, 2015-07-21 12:14:22]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-07-21 12:15:36]

Patchset 4 (id:??) landed as
https://crrev.com/57ec39ec14d7ebf9747d3855e2c13cfb76235e28
Cr-Commit-Position: refs/heads/master@{#339639}