Limit access to ChromeBookmarkClient to bookmarks code

Limit access to ChromeBookmarkClient to bookmarks code

ChromeBookmarkClient and BookmarkClient are implementation detail of the
bookmark component. Their goal is to abstract embedder differences. Thus
they should only be used by BookmarkModel.

- Change initialization of the two BookmarkPermanentNode managed_node
  and supervised_node to still be controlled by ChromeBookmarkClient but
  make the nodes accessible through BookmarkModel.

- Move CanSynNode(), CanBeEditedByUser(), IsPermanentNodeVisible() and
  PreferTouchIcon() to be defined in BookmarkModel as they do not depend
  on the embedder (the only difference depends on OS_IOS preprocessor
  macro).

- Add a method RecordAction() to BookmarkModel forwarding to the client
  and remove the client() accessor to prevent external code to access
  it.

- Change all client of ChromeBookmarkClient to instead use BookmarkModel
  as all the required API are now available on BookmarkModel.

BUG=359565, 383583

[sdefresne, 2015-06-23 14:04:27]

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

[sdefresne, 2015-06-23 14:04:28]

Please take a look.

Note that this CL depends, in order, on those two other CLs:
- https://codereview.chromium.org/1198373002
- https://codereview.chromium.org/1205603002

[sdefresne, 2015-06-23 21:54:29]

ping

[sky, 2015-06-23 22:03:53]

https://codereview.chromium.org/1203713002/diff/20001/components/bookmarks/br...
File components/bookmarks/browser/bookmark_client.h (right):

https://codereview.chromium.org/1203713002/diff/20001/components/bookmarks/br...
components/bookmarks/browser/bookmark_client.h:72: // task will be invoked in
the Profile's IO task runner.
Document what callback is.

https://codereview.chromium.org/1203713002/diff/20001/components/bookmarks/br...
File components/bookmarks/browser/bookmark_model.h (right):

https://codereview.chromium.org/1203713002/diff/20001/components/bookmarks/br...
components/bookmarks/browser/bookmark_model.h:102: // The top-level managed
bookmarks foled defined by an enterprise policy. This
I don't think these should be here. These are loaded externally and not part of
the core bookmarkmodel.

https://codereview.chromium.org/1203713002/diff/20001/components/bookmarks/br...
components/bookmarks/browser/bookmark_model.h:311: // Returns true if |node|
should sync.
Same comment for all of these. They are policy decisions and not part of the
core model.

[sdefresne, 2015-06-23 22:13:05]

https://codereview.chromium.org/1203713002/diff/20001/components/bookmarks/br...
File components/bookmarks/browser/bookmark_model.h (right):

https://codereview.chromium.org/1203713002/diff/20001/components/bookmarks/br...
components/bookmarks/browser/bookmark_model.h:102: // The top-level managed
bookmarks foled defined by an enterprise policy. This
On 2015/06/23 at 22:03:53, sky wrote:
> I don't think these should be here. These are loaded externally and not part
of the core bookmarkmodel.

Were would you recommend putting them? Should I create a new
ManagedBookmarkModel with just those two and put it into
//components/bookmarks/manager/? I cannot leave them in ChromeBookmarkClient
(without causing code duplication) since the client will not be shared between
//ios/chrome/browser and //chrome/browser.

With my patches they are still created and managed by the embedder through the
client (this is the purpose of the |callback| parameter to
HistoryClient::GetLoadExtraNodesCallback => allow the client to set
|managed_node_| and |supervised_node_|), but accessor are provided through the
BookmarkModel for convenience.

[sky, 2015-06-24 02:30:36]

As I mentioned, I don't want any of these functions to live on
BookmarkModel. I'm ok with them living in a separate file that is
shared between the necessary pieces, but not a subclass (BookmarkModel
is intended to be subclassed). I'm ok with the helper living in the
comonents directory, but would like it in a separate target that isn't
part of the bookmarks target.

  -Scott

On Tue, Jun 23, 2015 at 3:13 PM,  <sdefresne@chromium.org> wrote:
>
>
https://codereview.chromium.org/1203713002/diff/20001/components/bookmarks/br...
> File components/bookmarks/browser/bookmark_model.h (right):
>
>
https://codereview.chromium.org/1203713002/diff/20001/components/bookmarks/br...
> components/bookmarks/browser/bookmark_model.h:102: // The top-level
> managed bookmarks foled defined by an enterprise policy. This
> On 2015/06/23 at 22:03:53, sky wrote:
>>
>> I don't think these should be here. These are loaded externally and
>
> not part of the core bookmarkmodel.
>
> Were would you recommend putting them? Should I create a new
> ManagedBookmarkModel with just those two and put it into
> //components/bookmarks/manager/? I cannot leave them in
> ChromeBookmarkClient (without causing code duplication) since the client
> will not be shared between //ios/chrome/browser and //chrome/browser.
>
> With my patches they are still created and managed by the embedder
> through the client (this is the purpose of the |callback| parameter to
> HistoryClient::GetLoadExtraNodesCallback => allow the client to set
> |managed_node_| and |supervised_node_|), but accessor are provided
> through the BookmarkModel for convenience.
>
> https://codereview.chromium.org/1203713002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.