Added the "unmodifiable" property to chrome.bookmarks.BookmarkTreeNode.

Added the "unmodifiable" property to chrome.bookmarks.BookmarkTreeNode.

This optional property indicates the reason why this BookmarkTreeNode can't
be modified by the user nor the extension. For now the only value defined is
"managed", indicating that this bookmark was configured by the system
administrator.

If the property is not present then the bookmark is modifiable (default).

BUG=49598
R=kalman@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275387

[Joao da Silva, 2014-06-05 20:14:29]

Here's an attempt at BookmarkNodeTree.modifiable.

It's a bit unfortunate that the bookmark_api_helpers now all get the
ChromeBookmarkClient* as an arg, because it's required to determine if a node is
managed; maybe there's a better way to do this?

I also learned about tools/json_schema_compiler because the enum was't getting
initialized to X_NONE; this could be easily "fixed" in GetBookmarkTreeNode(),
where the BookmarkTreeNode is created, but I wonder if other places also create
BTNs. Let me know if the fix to the compiler makes sense :-)

PTAL, thanks!

[not at google - send to devlin, 2014-06-05 20:58:26]

looks fine, but yeah all that threading of Client everywhere is a pain.

https://codereview.chromium.org/320473002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/bookmarks/bookmark_api_helpers.h (right):

https://codereview.chromium.org/320473002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/bookmarks/bookmark_api_helpers.h:24:
ChromeBookmarkClient* client,
well one way you could "fix" this is by making this a class, not a set of utils,
which take a ChromeBookmarkClient in its constructor.. then constructing one for
each bookmarks API call.

might be overkill for this change though.

https://codereview.chromium.org/320473002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/bookmarks/bookmarks_api.cc (right):

https://codereview.chromium.org/320473002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/bookmarks/bookmarks_api.cc:244:
BookmarkModelFactory::GetChromeBookmarkClientForProfile(profile)) {
kinda weird that BookmarkModelFactory is a BrowserContextKeyedServiceFactory yet
deals in Profiles...

[not at google - send to devlin, 2014-06-05 20:58:37]

lgtm if you want to ignore my suggestiong

[Joao da Silva, 2014-06-05 21:04:17]

I'd like to fix both suggestions but focus this CL on the API change only, if
you don't mind.

https://codereview.chromium.org/320473002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/bookmarks/bookmark_api_helpers.h (right):

https://codereview.chromium.org/320473002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/bookmarks/bookmark_api_helpers.h:24:
ChromeBookmarkClient* client,
On 2014/06/05 20:58:26, kalman wrote:
> well one way you could "fix" this is by making this a class, not a set of
utils,
> which take a ChromeBookmarkClient in its constructor.. then constructing one
for
> each bookmarks API call.
> 
> might be overkill for this change though.

I think I'll leave this change as-is to be focused just on the API.

WDYT of moving these 4 helpers to the ChromeBookmarkClient itself?

https://codereview.chromium.org/320473002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/bookmarks/bookmarks_api.cc (right):

https://codereview.chromium.org/320473002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/bookmarks/bookmarks_api.cc:244:
BookmarkModelFactory::GetChromeBookmarkClientForProfile(profile)) {
On 2014/06/05 20:58:26, kalman wrote:
> kinda weird that BookmarkModelFactory is a BrowserContextKeyedServiceFactory
yet
> deals in Profiles...

The main getter is GetForProfile(), so I used this name for consistency.

I'm guessing this is fallout from the refactoring where the ProfileKeyedService
APIs were componentized. I'd leave renaming these methods to another CL, if you
don't mind.

[not at google - send to devlin, 2014-06-05 22:21:33]

https://codereview.chromium.org/320473002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/bookmarks/bookmark_api_helpers.h (right):

https://codereview.chromium.org/320473002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/bookmarks/bookmark_api_helpers.h:24:
ChromeBookmarkClient* client,
On 2014/06/05 21:04:16, Joao da Silva wrote:
> On 2014/06/05 20:58:26, kalman wrote:
> > well one way you could "fix" this is by making this a class, not a set of
> utils,
> > which take a ChromeBookmarkClient in its constructor.. then constructing one
> for
> > each bookmarks API call.
> > 
> > might be overkill for this change though.
> 
> I think I'll leave this change as-is to be focused just on the API.
> 
> WDYT of moving these 4 helpers to the ChromeBookmarkClient itself?

The functions which deal with Extension-generated objects (like
BookmarkTreeNode) don't really belong on ChromeBookmarkClient. Better for
ChromeBookmarkClient to be ignorant of anything extensiony.

https://codereview.chromium.org/320473002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/bookmarks/bookmarks_api.cc (right):

https://codereview.chromium.org/320473002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/bookmarks/bookmarks_api.cc:244:
BookmarkModelFactory::GetChromeBookmarkClientForProfile(profile)) {
On 2014/06/05 21:04:16, Joao da Silva wrote:
> On 2014/06/05 20:58:26, kalman wrote:
> > kinda weird that BookmarkModelFactory is a BrowserContextKeyedServiceFactory
> yet
> > deals in Profiles...
> 
> The main getter is GetForProfile(), so I used this name for consistency.
> 
> I'm guessing this is fallout from the refactoring where the
ProfileKeyedService
> APIs were componentized. I'd leave renaming these methods to another CL, if
you
> don't mind.

I didn't mean to imply that it was something to do with your change, sorry.

[Joao da Silva, 2014-06-05 22:28:23]

https://codereview.chromium.org/320473002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/bookmarks/bookmark_api_helpers.h (right):

https://codereview.chromium.org/320473002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/bookmarks/bookmark_api_helpers.h:24:
ChromeBookmarkClient* client,
On 2014/06/05 22:21:32, kalman wrote:
> On 2014/06/05 21:04:16, Joao da Silva wrote:
> > On 2014/06/05 20:58:26, kalman wrote:
> > > well one way you could "fix" this is by making this a class, not a set of
> > utils,
> > > which take a ChromeBookmarkClient in its constructor.. then constructing
one
> > for
> > > each bookmarks API call.
> > > 
> > > might be overkill for this change though.
> > 
> > I think I'll leave this change as-is to be focused just on the API.
> > 
> > WDYT of moving these 4 helpers to the ChromeBookmarkClient itself?
> 
> The functions which deal with Extension-generated objects (like
> BookmarkTreeNode) don't really belong on ChromeBookmarkClient. Better for
> ChromeBookmarkClient to be ignorant of anything extensiony.

Makes sense.

These helpers are only used by the implementations of bookmarks and
bookmarkManagerPrivate APIs, and all of them inherit from BookmarksFunction.
BookmarksFunction already has some protected helpers, so WDYT of removing this
file and moving these helpers into methods of that class?

[not at google - send to devlin, 2014-06-05 22:29:10]

https://codereview.chromium.org/320473002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/bookmarks/bookmark_api_helpers.h (right):

https://codereview.chromium.org/320473002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/bookmarks/bookmark_api_helpers.h:24:
ChromeBookmarkClient* client,
On 2014/06/05 22:28:23, Joao da Silva wrote:
> On 2014/06/05 22:21:32, kalman wrote:
> > On 2014/06/05 21:04:16, Joao da Silva wrote:
> > > On 2014/06/05 20:58:26, kalman wrote:
> > > > well one way you could "fix" this is by making this a class, not a set
of
> > > utils,
> > > > which take a ChromeBookmarkClient in its constructor.. then constructing
> one
> > > for
> > > > each bookmarks API call.
> > > > 
> > > > might be overkill for this change though.
> > > 
> > > I think I'll leave this change as-is to be focused just on the API.
> > > 
> > > WDYT of moving these 4 helpers to the ChromeBookmarkClient itself?
> > 
> > The functions which deal with Extension-generated objects (like
> > BookmarkTreeNode) don't really belong on ChromeBookmarkClient. Better for
> > ChromeBookmarkClient to be ignorant of anything extensiony.
> 
> Makes sense.
> 
> These helpers are only used by the implementations of bookmarks and
> bookmarkManagerPrivate APIs, and all of them inherit from BookmarksFunction.
> BookmarksFunction already has some protected helpers, so WDYT of removing this
> file and moving these helpers into methods of that class?

SVGTM.

[Joao da Silva, 2014-06-06 10:51:03]

Committed patchset #2 manually as r275387 (presubmit successful).