Managed bookmarks for supervised users

Managed bookmarks for supervised users.

Design doc: https://docs.google.com/document/d/1Ms5wHyGtVzdH0MOF46olgnTj4k9opQGYzMSrk-LJckM

Based on fhorschig's https://chromereviews.googleplex.com/108977013/

BUG=443606
Committed: https://crrev.com/1a600c7b3fdff94fa7e00656191f37fb65dd05b1
Cr-Commit-Position: refs/heads/master@{#315289}

[Marc Treib, 2014-12-18 15:52:39]

treib@chromium.org changed reviewers:
+ pam@chromium.org

[Marc Treib, 2014-12-18 15:52:40]

Hi Pam, I think this is ready for a first round of review - PTAL whenever you
have some free time. I'll add OWNERS after you're satisfied.

[Pam (message me for reviews), 2015-01-14 14:03:38]

https://codereview.chromium.org/769153007/diff/40001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/769153007/diff/40001/chrome/app/generated_res...
chrome/app/generated_resources.grd:6332: +      <message
name="IDS_FLAGS_ENABLE_SUPERVISED_USER_MANAGED_BOOKMARKS_FOLDER_NAME" desc="Name
of the flag to enable the mangaed bookmarks folder for supervised users.">
typo "mangaed"

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/chrome_bookmark_client.cc (right):

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.cc:48: bool
HasDescendantsOf(const std::vector<const BookmarkNode*>& list,
This name is a little misleading, so a quick comment would be good.

// Returns true if any node in |list| is a descendant of |root|.

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.cc:60: // Load the initial
contents of the |node| now, and assign it an unused ID.
What are "initial" contents, as opposed to any other kind?

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.cc:64: node, initial_bookmarks,
id + 1);
I suppose this object is profile-keyed and not multi-threaded, so no chance of a
problem here, right?

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.cc:235: // The |managed_node_|
can have its title updated if the user signs in or
Please explain why |managed_node_| (that is, the top-level folder) can have its
title updated, but |supervised_node_| can't. (The Managed Bookmarks folder can
have the name of the managed domain in its title.)

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.cc:278: // Start tracking the
managed bookmarks. This will detect any changes that
"managed and supervised" x 2

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/chrome_bookmark_client.h (right):

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.h:37: // Returns the
supervised_node.
This comment (and the managed_node() one above) is really pointless.

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.h:120: BookmarkPermanentNode*
managed_node_;
Somewhere in this file, maybe as comments for managed_node_ and
supervised_node_, it needs a better description of the difference between
"managed" and "supervised" bookmarks.

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/bookmarks/bookmarks_api.cc (right):

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/bookmarks/bookmarks_api.cc:212: if
(client->IsDescendantOfManagedOrSupervisedNode(node)) {
The API description for "unmodifiable" will also need to be changed. The
extensions team might want to add another reason, or might be fine with
supervised and managed overlapping.

chrome/common/extensions/api/bookmarks.json

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_bookmarks_handler.cc
(right):

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_bookmarks_handler.cc:14: const
char kKeyLink[] = "SupervisedBookmarkLink";
A little more description of where these keys apply or come from would be good.

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_bookmarks_handler.cc:23: // |key|
can be either "<ID>-<Value>" or just "<ID>".
Under what circumstances does each of these |key| formats come up? And should
the - be a : ?

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_bookmarks_handler.cc:35: // |key|
must be "<ID>-<Value>".
- --> :

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_bookmarks_handler.cc:49:
base::ListValue* FindNode(base::ListValue* root, int id) {
FindFolder would be a clearer function name. Also, function-level comment
please, since this is not completely trivial.

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_bookmarks_handler.cc:140:
links_.push_back(Link(url, name, parent_id));
I'm almost tempted to make a union for id|url, and merge ParseFolders and
ParseLinks into a single method. But maybe that's too clever for its own good.

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_bookmarks_handler.cc:154: while
(!folders.empty() && folders.size() != folders_failed.size()) {
This snippet is a bit hard to understand. A comment or two describing the
overall process would help.

Also, consider at least logging an error if there are folders that can't be
added.

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_bookmarks_handler.cc:178:
AddNodeToTree(link.parent_id, node.Pass());
Also consider logging a warning if the bookmark can't be added.

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/supervise...
File
chrome/browser/supervised_user/supervised_user_bookmarks_handler_unittest.cc
(right):

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_bookmarks_handler_unittest.cc:279:
scoped_ptr<base::DictionaryValue> settings(
Please add comments explaining what's going on here, since the various methods
it's calling aren't right here. (e.g. "Make some fake settings", "Parse the
settings into a bookmarks tree", "Compare to the expected tree constructed
directly from the hardcoded JSON above".)

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1645:
button->SetImage(views::Button::STATE_NORMAL, *image);
This pattern is different in all the rest of the methods here -- maybe it
changed since the code was first written.

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.h (right):

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.h:421: views::MenuButton*
supervised_bookmarks_button_;
It seems strange to me that the supervised bookmarks would be relegated to a
folder on the bookmarks bar. I guess that's addressed with the
bootstrapping-the-NTP ideas for the future, though.

https://codereview.chromium.org/769153007/diff/40001/components/bookmark_bar_...
File components/bookmark_bar_strings.grdp (right):

https://codereview.chromium.org/769153007/diff/40001/components/bookmark_bar_...
components/bookmark_bar_strings.grdp:41: Supervised bookmarks
This is shown even when the general managed bookmarks are active, isn't it?

Also, the bookmarks aren't supervised. What about "Recommended bookmarks" or
similar?

https://codereview.chromium.org/769153007/diff/40001/components/bookmark_bar_...
components/bookmark_bar_strings.grdp:58: Supervised Bookmarks
Ditto x 2 above

https://codereview.chromium.org/769153007/diff/40001/components/policy/core/b...
File components/policy/core/browser/managed_bookmarks_tracker.cc (right):

https://codereview.chromium.org/769153007/diff/40001/components/policy/core/b...
components/policy/core/browser/managed_bookmarks_tracker.cc:198: return false;
Adding validation to an existing set of users' data may have unintended side
effects. Will this trip on JS bookmarklets, for example? Or is anyone using the
bookmark system to push random other snippets of text to their users?

https://codereview.chromium.org/769153007/diff/40001/components/policy/core/b...
File components/policy/core/browser/managed_bookmarks_tracker.h (right):

https://codereview.chromium.org/769153007/diff/40001/components/policy/core/b...
components/policy/core/browser/managed_bookmarks_tracker.h:37:
ManagedBookmarksTracker(BookmarkModel* model,
Please add a comment explaining |is_supervised|.

[noyau (Ping after 24h), 2015-01-14 15:12:14]

Drive-by: The design doc for this feature is a little light, the client UI is
not discussed at all. As the de-facto owner of the bookmark UI on iOS I was
wondering what were your plans for that platform, and more generally for mobile.
The feature seems very similar to managed bookmarks at first glance, which is a
good thing as it means this is something we already know how to do. Feel free to
drop me a line or a calendar invite so we can discuss about it. 

Anyway, it would probably be better if this CL was split at least in two: One
part back-end, code that will be shared by all platforms (bookmark model
changes, how the data gets into the managed bookmarks, etc) and one part front
end (Icons, flags, menus, etc) that will be only used on desktop.

[Marc Treib, 2015-01-14 16:04:16]

On 2015/01/14 15:12:14, noyau wrote:
> Drive-by: The design doc for this feature is a little light, the client UI is
> not discussed at all. As the de-facto owner of the bookmark UI on iOS I was
> wondering what were your plans for that platform, and more generally for
mobile.
> The feature seems very similar to managed bookmarks at first glance, which is
a
> good thing as it means this is something we already know how to do. Feel free
to
> drop me a line or a calendar invite so we can discuss about it. 

On desktop, the UI looks exactly like for managed bookmarks (apart from a
different title and icon), i.e. a special entry in the bookmarks bar. I'm not
familiar at all with Bling, so I don't know if that is used there as well.
However, supervised users were never supported on Bling, and support for child
accounts on Bling is still at an early design stage, so I don't think we need to
worry about the Bling UI now.

> Anyway, it would probably be better if this CL was split at least in two: One
> part back-end, code that will be shared by all platforms (bookmark model
> changes, how the data gets into the managed bookmarks, etc) and one part front
> end (Icons, flags, menus, etc) that will be only used on desktop.

[Marc Treib, 2015-01-14 16:40:50]

https://codereview.chromium.org/769153007/diff/40001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/769153007/diff/40001/chrome/app/generated_res...
chrome/app/generated_resources.grd:6332: +      <message
name="IDS_FLAGS_ENABLE_SUPERVISED_USER_MANAGED_BOOKMARKS_FOLDER_NAME" desc="Name
of the flag to enable the mangaed bookmarks folder for supervised users.">
On 2015/01/14 14:03:36, Pam (also PM for reviews) wrote:
> typo "mangaed"

Done.

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/chrome_bookmark_client.cc (right):

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.cc:48: bool
HasDescendantsOf(const std::vector<const BookmarkNode*>& list,
On 2015/01/14 14:03:36, Pam (also PM for reviews) wrote:
> This name is a little misleading, so a quick comment would be good.
> 
> // Returns true if any node in |list| is a descendant of |root|.

Done.

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.cc:60: // Load the initial
contents of the |node| now, and assign it an unused ID.
On 2015/01/14 14:03:36, Pam (also PM for reviews) wrote:
> What are "initial" contents, as opposed to any other kind?

AIUI, the "initial contents" are just "whatever's already in the pref when this
thing starts up".

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.cc:64: node, initial_bookmarks,
id + 1);
On 2015/01/14 14:03:37, Pam (also PM for reviews) wrote:
> I suppose this object is profile-keyed and not multi-threaded, so no chance of
a
> problem here, right?

"this object" meaning what, exactly? LoadInitial is a static method.
In any case, this isn't actually new code, so if there were any problem, it
should have been there already.

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.cc:235: // The |managed_node_|
can have its title updated if the user signs in or
On 2015/01/14 14:03:36, Pam (also PM for reviews) wrote:
> Please explain why |managed_node_| (that is, the top-level folder) can have
its
> title updated, but |supervised_node_| can't. (The Managed Bookmarks folder can
> have the name of the managed domain in its title.)

Done.

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.cc:278: // Start tracking the
managed bookmarks. This will detect any changes that
On 2015/01/14 14:03:37, Pam (also PM for reviews) wrote:
> "managed and supervised" x 2

Done.

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/chrome_bookmark_client.h (right):

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.h:37: // Returns the
supervised_node.
On 2015/01/14 14:03:37, Pam (also PM for reviews) wrote:
> This comment (and the managed_node() one above) is really pointless.

I guess somebody wanted every public method to have a comment. But agreed; I've
removed the comments.

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.h:120: BookmarkPermanentNode*
managed_node_;
On 2015/01/14 14:03:37, Pam (also PM for reviews) wrote:
> Somewhere in this file, maybe as comments for managed_node_ and
> supervised_node_, it needs a better description of the difference between
> "managed" and "supervised" bookmarks.

Done.

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/bookmarks/bookmarks_api.cc (right):

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/bookmarks/bookmarks_api.cc:212: if
(client->IsDescendantOfManagedOrSupervisedNode(node)) {
On 2015/01/14 14:03:37, Pam (also PM for reviews) wrote:
> The API description for "unmodifiable" will also need to be changed. The
> extensions team might want to add another reason, or might be fine with
> supervised and managed overlapping.
> 
> chrome/common/extensions/api/bookmarks.json

Good catch! I've updated the description string in the bookmarks API definition,
let's see what the extensions people say.

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_bookmarks_handler.cc
(right):

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_bookmarks_handler.cc:14: const
char kKeyLink[] = "SupervisedBookmarkLink";
On 2015/01/14 14:03:37, Pam (also PM for reviews) wrote:
> A little more description of where these keys apply or come from would be
good.

Done.

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_bookmarks_handler.cc:23: // |key|
can be either "<ID>-<Value>" or just "<ID>".
On 2015/01/14 14:03:37, Pam (also PM for reviews) wrote:
> Under what circumstances does each of these |key| formats come up? And should
> the - be a : ?

Updated the comment. And yes, that should be ":", thanks.

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_bookmarks_handler.cc:35: // |key|
must be "<ID>-<Value>".
On 2015/01/14 14:03:37, Pam (also PM for reviews) wrote:
> - --> :

Done.

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_bookmarks_handler.cc:49:
base::ListValue* FindNode(base::ListValue* root, int id) {
On 2015/01/14 14:03:37, Pam (also PM for reviews) wrote:
> FindFolder would be a clearer function name. Also, function-level comment
> please, since this is not completely trivial.

Done.

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_bookmarks_handler.cc:140:
links_.push_back(Link(url, name, parent_id));
On 2015/01/14 14:03:37, Pam (also PM for reviews) wrote:
> I'm almost tempted to make a union for id|url, and merge ParseFolders and
> ParseLinks into a single method. But maybe that's too clever for its own good.

Hrm. I agree that the mostly-duplicated code isn't too pretty, but a union..
ugh. I prefer to keep it as is.

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_bookmarks_handler.cc:154: while
(!folders.empty() && folders.size() != folders_failed.size()) {
On 2015/01/14 14:03:37, Pam (also PM for reviews) wrote:
> This snippet is a bit hard to understand. A comment or two describing the
> overall process would help.
> 
> Also, consider at least logging an error if there are folders that can't be
> added.

Done.

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_bookmarks_handler.cc:178:
AddNodeToTree(link.parent_id, node.Pass());
On 2015/01/14 14:03:37, Pam (also PM for reviews) wrote:
> Also consider logging a warning if the bookmark can't be added.

Done.

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/supervise...
File
chrome/browser/supervised_user/supervised_user_bookmarks_handler_unittest.cc
(right):

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_bookmarks_handler_unittest.cc:279:
scoped_ptr<base::DictionaryValue> settings(
On 2015/01/14 14:03:37, Pam (also PM for reviews) wrote:
> Please add comments explaining what's going on here, since the various methods
> it's calling aren't right here. (e.g. "Make some fake settings", "Parse the
> settings into a bookmarks tree", "Compare to the expected tree constructed
> directly from the hardcoded JSON above".)

Done.

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1645:
button->SetImage(views::Button::STATE_NORMAL, *image);
On 2015/01/14 14:03:37, Pam (also PM for reviews) wrote:
> This pattern is different in all the rest of the methods here -- maybe it
> changed since the code was first written.

Probably, yeah - thanks, done.

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.h (right):

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.h:421: views::MenuButton*
supervised_bookmarks_button_;
On 2015/01/14 14:03:37, Pam (also PM for reviews) wrote:
> It seems strange to me that the supervised bookmarks would be relegated to a
> folder on the bookmarks bar. I guess that's addressed with the
> bootstrapping-the-NTP ideas for the future, though.

Where else would they appear? I suppose we would also show them on the special
NTP if we ever get that, but until then the bookmarks bar seems like the logical
place?

https://codereview.chromium.org/769153007/diff/40001/components/bookmark_bar_...
File components/bookmark_bar_strings.grdp (right):

https://codereview.chromium.org/769153007/diff/40001/components/bookmark_bar_...
components/bookmark_bar_strings.grdp:41: Supervised bookmarks
On 2015/01/14 14:03:37, Pam (also PM for reviews) wrote:
> This is shown even when the general managed bookmarks are active, isn't it?

Right. Leftover from Friedrich's old code where I forgot to update the comment.

> Also, the bookmarks aren't supervised. What about "Recommended bookmarks" or
> similar?

"recommended" is certainly better than "supervised", I've updated the string.
Generally, I tend to let other people do the string bike shedding :)

https://codereview.chromium.org/769153007/diff/40001/components/bookmark_bar_...
components/bookmark_bar_strings.grdp:58: Supervised Bookmarks
On 2015/01/14 14:03:37, Pam (also PM for reviews) wrote:
> Ditto x 2 above

Done.

https://codereview.chromium.org/769153007/diff/40001/components/policy/core/b...
File components/policy/core/browser/managed_bookmarks_tracker.cc (right):

https://codereview.chromium.org/769153007/diff/40001/components/policy/core/b...
components/policy/core/browser/managed_bookmarks_tracker.cc:198: return false;
On 2015/01/14 14:03:37, Pam (also PM for reviews) wrote:
> Adding validation to an existing set of users' data may have unintended side
> effects. Will this trip on JS bookmarklets, for example? Or is anyone using
the
> bookmark system to push random other snippets of text to their users?

The policy checks for valid URLs elsewhere, so this shouldn't ever get
triggered. Our code didn't (it does now), and putting an invalid URL here
resulted in a crash somewhere in the bookmarks code, which is why I added this
check.
I've replaced it with a DCHECK.

https://codereview.chromium.org/769153007/diff/40001/components/policy/core/b...
File components/policy/core/browser/managed_bookmarks_tracker.h (right):

https://codereview.chromium.org/769153007/diff/40001/components/policy/core/b...
components/policy/core/browser/managed_bookmarks_tracker.h:37:
ManagedBookmarksTracker(BookmarkModel* model,
On 2015/01/14 14:03:37, Pam (also PM for reviews) wrote:
> Please add a comment explaining |is_supervised|.

Done. I've also added a class-level comment, and updated a few other ones.

[Pam (message me for reviews), 2015-01-15 11:37:04]

LGTM, preferably with two more comments added as described below.

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/chrome_bookmark_client.cc (right):

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.cc:64: node, initial_bookmarks,
id + 1);
On 2015/01/14 16:40:49, Marc Treib wrote:
> On 2015/01/14 14:03:37, Pam (also PM for reviews) wrote:
> > I suppose this object is profile-keyed and not multi-threaded, so no chance
of
> a
> > problem here, right?
> 
> "this object" meaning what, exactly? LoadInitial is a static method.
> In any case, this isn't actually new code, so if there were any problem, it
> should have been there already.

OK, then, I suppose there's nothing to do here... apart from scanning the bug
tracker to see if there are any suspicious-looking bookmarks bugs. :) (I did a
quick search, but not a really thorough reading.)

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_bookmarks_handler.cc
(right):

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_bookmarks_handler.cc:140:
links_.push_back(Link(url, name, parent_id));
On 2015/01/14 16:40:50, Marc Treib wrote:
> On 2015/01/14 14:03:37, Pam (also PM for reviews) wrote:
> > I'm almost tempted to make a union for id|url, and merge ParseFolders and
> > ParseLinks into a single method. But maybe that's too clever for its own
good.
> 
> Hrm. I agree that the mostly-duplicated code isn't too pretty, but a union..
> ugh. I prefer to keep it as is.

Could also be a struct or class with separate url and id fields. But it's also
OK as-is.

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.h (right):

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.h:421: views::MenuButton*
supervised_bookmarks_button_;
On 2015/01/14 16:40:50, Marc Treib wrote:
> On 2015/01/14 14:03:37, Pam (also PM for reviews) wrote:
> > It seems strange to me that the supervised bookmarks would be relegated to a
> > folder on the bookmarks bar. I guess that's addressed with the
> > bootstrapping-the-NTP ideas for the future, though.
> 
> Where else would they appear? I suppose we would also show them on the special
> NTP if we ever get that, but until then the bookmarks bar seems like the
logical
> place?

I expected them to be flat on the bookmarks bar, instead of inside a folder, to
be more apparent and friendly for naive family members. Ultimately it's up to
UX, of course.

https://codereview.chromium.org/769153007/diff/60001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/chrome_bookmark_client.h (right):

https://codereview.chromium.org/769153007/diff/60001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.h:123: BookmarkPermanentNode*
managed_node_;
Something like "// The top-level managed bookmarks folder." would be good here,
and likewise below.

[Marc Treib, 2015-01-15 12:30:09]

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.h (right):

https://codereview.chromium.org/769153007/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.h:421: views::MenuButton*
supervised_bookmarks_button_;
On 2015/01/15 11:37:04, Pam (also PM for reviews) wrote:
> On 2015/01/14 16:40:50, Marc Treib wrote:
> > On 2015/01/14 14:03:37, Pam (also PM for reviews) wrote:
> > > It seems strange to me that the supervised bookmarks would be relegated to
a
> > > folder on the bookmarks bar. I guess that's addressed with the
> > > bootstrapping-the-NTP ideas for the future, though.
> > 
> > Where else would they appear? I suppose we would also show them on the
special
> > NTP if we ever get that, but until then the bookmarks bar seems like the
> logical
> > place?
> 
> I expected them to be flat on the bookmarks bar, instead of inside a folder,
to
> be more apparent and friendly for naive family members. Ultimately it's up to
> UX, of course.

FWIW, in the new bookmarks UI (which should launch Really Soon Now, at least as
an experiment), all the bookmarks do show up individually.
I think the bookmarks bar will continue to exist though, so we still might want
to change the behavior here. The current version mimics the enterprise
bookmarks. Listing the bookmarks individually would probably be a bit more work,
so I'd like to consult the UX people before actually doing that.

https://codereview.chromium.org/769153007/diff/60001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/chrome_bookmark_client.h (right):

https://codereview.chromium.org/769153007/diff/60001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.h:123: BookmarkPermanentNode*
managed_node_;
On 2015/01/15 11:37:04, Pam (also PM for reviews) wrote:
> Something like "// The top-level managed bookmarks folder." would be good
here,
> and likewise below.

Done.

[Marc Treib, 2015-01-23 11:51:12]

treib@chromium.org changed reviewers:
+ asvitkine@chromium.org, bartfab@chromium.org, oshima@chromium.org,
rohitrao@chromium.org, sky@chromium.org

[Marc Treib, 2015-01-23 11:51:13]

Now that Pam's satisfied, adding people for OWNERS review:

rohitrao for build/ios
oshima for c/a/theme (note: The icons aren't final, in particular for Mac; I'll
hopefully get the correct ones soon.)
sky for c/b/bookmarks, c/b/extensions/api/bookmarks, components/bookmarks, and
c/b/ui
bartfab for components/policy
asvitkine for tools/histograms

PTAL!

Also +nepper fyi

[noyau (Ping after 24h), 2015-01-23 12:14:52]

https://codereview.chromium.org/769153007/diff/200001/chrome/browser/bookmark...
File chrome/browser/bookmarks/chrome_bookmark_client.h (right):

https://codereview.chromium.org/769153007/diff/200001/chrome/browser/bookmark...
chrome/browser/bookmarks/chrome_bookmark_client.h:39: const BookmarkNode*
supervised_node() { return supervised_node_; }
Can we have a comment before those two methods describing the difference between
a managed_node and a supervised_node? I know there is a comment on the instance
variables, but those are buried all the way down in the private part of the
class.

https://codereview.chromium.org/769153007/diff/200001/chrome/browser/supervis...
File chrome/browser/supervised_user/supervised_user_bookmarks_handler.h (right):

https://codereview.chromium.org/769153007/diff/200001/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_bookmarks_handler.h:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
2015 on new files :)

https://codereview.chromium.org/769153007/diff/200001/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_bookmarks_handler.h:26: // Public
for testing only.
The testing class is listed as friend, does these really need to be public?

[rohitrao (ping after 24h), 2015-01-23 14:14:07]

In grit_whitelist.txt, did you intentionally only add NAME and not DESCRIPTION? 
Will iOS need the new icons in theme_resources.grd?

[Alexei Svitkine (slow), 2015-01-23 16:06:13]

I also took a look at cocoa/bookmarks, since I'm an owner of that dir.

https://codereview.chromium.org/769153007/diff/200001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.h (right):

https://codereview.chromium.org/769153007/diff/200001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.h:318: // apps
shortcut and the managed and supervised bookmarks folders.
Nit: replace the first "and" with a comma since there's a list of things now.

https://codereview.chromium.org/769153007/diff/200001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm (right):

https://codereview.chromium.org/769153007/diff/200001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:524:
!supervisedBookmarksButton_.get())
Nit: add {}'s

https://codereview.chromium.org/769153007/diff/200001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1235: if
(!supervisedBookmarksButton_.get())
This code is basically identical to -setManagedBookmarksButtonVisibility other
than which button object you're doing this to. Can you make a helper function?

https://codereview.chromium.org/769153007/diff/200001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1316: 
Nit: Remove empty line.

[Marc Treib, 2015-01-26 09:31:28]

On 2015/01/23 14:14:07, rohitrao wrote:
> In grit_whitelist.txt, did you intentionally only add NAME and not
DESCRIPTION? 
> Will iOS need the new icons in theme_resources.grd?

There is no DESCRIPTION - what I added in grit_whitelist.txt is not the flag
name from generated_resources.grd, but a string from bookmark_bar_strings.grdp.

The new icons are for desktop only. I'm not familiar with iOS, but noyau
mentioned that iOS uses the same bookmarks UI as Android, which doesn't use
special icons here.

[Marc Treib, 2015-01-26 12:27:42]

https://codereview.chromium.org/769153007/diff/200001/chrome/browser/bookmark...
File chrome/browser/bookmarks/chrome_bookmark_client.h (right):

https://codereview.chromium.org/769153007/diff/200001/chrome/browser/bookmark...
chrome/browser/bookmarks/chrome_bookmark_client.h:39: const BookmarkNode*
supervised_node() { return supervised_node_; }
On 2015/01/23 12:14:52, noyau wrote:
> Can we have a comment before those two methods describing the difference
between
> a managed_node and a supervised_node? I know there is a comment on the
instance
> variables, but those are buried all the way down in the private part of the
> class.

Done.

https://codereview.chromium.org/769153007/diff/200001/chrome/browser/supervis...
File chrome/browser/supervised_user/supervised_user_bookmarks_handler.h (right):

https://codereview.chromium.org/769153007/diff/200001/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_bookmarks_handler.h:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
On 2015/01/23 12:14:52, noyau wrote:
> 2015 on new files :)

Thanks, done!

https://codereview.chromium.org/769153007/diff/200001/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_bookmarks_handler.h:26: // Public
for testing only.
On 2015/01/23 12:14:52, noyau wrote:
> The testing class is listed as friend, does these really need to be public?

Yes, it's also used in the test outside of the actual testing class.

https://codereview.chromium.org/769153007/diff/200001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.h (right):

https://codereview.chromium.org/769153007/diff/200001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.h:318: // apps
shortcut and the managed and supervised bookmarks folders.
On 2015/01/23 16:06:13, Alexei Svitkine wrote:
> Nit: replace the first "and" with a comma since there's a list of things now.

Done.

https://codereview.chromium.org/769153007/diff/200001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm (right):

https://codereview.chromium.org/769153007/diff/200001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:524:
!supervisedBookmarksButton_.get())
On 2015/01/23 16:06:13, Alexei Svitkine wrote:
> Nit: add {}'s

Done.

https://codereview.chromium.org/769153007/diff/200001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1235: if
(!supervisedBookmarksButton_.get())
On 2015/01/23 16:06:13, Alexei Svitkine wrote:
> This code is basically identical to -setManagedBookmarksButtonVisibility other
> than which button object you're doing this to. Can you make a helper function?

It's not quite identical - for the managed button, there's also a pref to
show/hide the button. I've made a helper function with the common parts, though.

https://codereview.chromium.org/769153007/diff/200001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1316: 
On 2015/01/23 16:06:13, Alexei Svitkine wrote:
> Nit: Remove empty line.

Done.

[bartfab (slow), 2015-01-26 15:40:14]

components/policy/core/browser LGTM

However, I think that managed_bookmarks_tracker* should move somewhere else now
that handles more than just policy.

https://codereview.chromium.org/769153007/diff/240001/components/policy/core/...
File components/policy/core/browser/managed_bookmarks_tracker.cc (right):

https://codereview.chromium.org/769153007/diff/240001/components/policy/core/...
components/policy/core/browser/managed_bookmarks_tracker.cc:103: std::string
domain = get_management_domain_callback_.Run();
Nit: const.

https://codereview.chromium.org/769153007/diff/240001/components/policy/core/...
components/policy/core/browser/managed_bookmarks_tracker.cc:116: // In case the
user just signed into or out of a managed account.
Nit: s/a managed/the/ ("managed accounts" refers to enterprise accounts but you
are using this class more broadly now)

https://codereview.chromium.org/769153007/diff/240001/components/policy/core/...
components/policy/core/browser/managed_bookmarks_tracker.cc:199:
DCHECK(url->is_valid());
Nit: #include "base/logging.h"

https://codereview.chromium.org/769153007/diff/240001/components/policy/core/...
File components/policy/core/browser/managed_bookmarks_tracker.h (right):

https://codereview.chromium.org/769153007/diff/240001/components/policy/core/...
components/policy/core/browser/managed_bookmarks_tracker.h:61: // Starts
tracking the policy for updates to the managed/supervised bookmarks.
Nit: s/policy/pref/

https://codereview.chromium.org/769153007/diff/240001/components/policy/core/...
components/policy/core/browser/managed_bookmarks_tracker.h:72: base::string16
GetBookmarksFolderTitle() const;
Nit: #include "base/strings/string16.h"

https://codereview.chromium.org/769153007/diff/240001/components/policy/core/...
components/policy/core/browser/managed_bookmarks_tracker.h:84: bool
is_supervised_;
Nit: const.

https://codereview.chromium.org/769153007/diff/240001/components/policy/core/...
File components/policy/core/browser/managed_bookmarks_tracker_unittest.cc
(right):

https://codereview.chromium.org/769153007/diff/240001/components/policy/core/...
components/policy/core/browser/managed_bookmarks_tracker_unittest.cc:50: void
CreateModel() {
Nit: Please remove this method and replace all callers with something like:

CreateModel(false /* is_supervised */);

https://codereview.chromium.org/769153007/diff/240001/components/policy/core/...
components/policy/core/browser/managed_bookmarks_tracker_unittest.cc:205:
CreateModel(true);
Nit: Add /* is_supervised */

https://codereview.chromium.org/769153007/diff/240001/components/policy/core/...
components/policy/core/browser/managed_bookmarks_tracker_unittest.cc:213:
CreateModel(true);
Nit: Add /* is_supervised */

https://codereview.chromium.org/769153007/diff/240001/components/policy/core/...
components/policy/core/browser/managed_bookmarks_tracker_unittest.cc:216: //
Don't bother checking the actual contents, the non-supervised tests cover
You could parameterize the test and verify that both managed and supervised are
handled correctly. There is no way to guarantee that the code paths which handle
managed and supervised will not diverge at some point.

[Alexei Svitkine (slow), 2015-01-26 16:13:56]

lgtm

[rohitrao (ping after 24h), 2015-01-26 16:15:24]

grit_whitelist.txt LGTM

[Marc Treib, 2015-01-26 16:16:48]

https://codereview.chromium.org/769153007/diff/240001/components/policy/core/...
File components/policy/core/browser/managed_bookmarks_tracker.cc (right):

https://codereview.chromium.org/769153007/diff/240001/components/policy/core/...
components/policy/core/browser/managed_bookmarks_tracker.cc:103: std::string
domain = get_management_domain_callback_.Run();
On 2015/01/26 15:40:13, bartfab wrote:
> Nit: const.

Done.

https://codereview.chromium.org/769153007/diff/240001/components/policy/core/...
components/policy/core/browser/managed_bookmarks_tracker.cc:116: // In case the
user just signed into or out of a managed account.
On 2015/01/26 15:40:14, bartfab wrote:
> Nit: s/a managed/the/ ("managed accounts" refers to enterprise accounts but
you
> are using this class more broadly now)

Done.

https://codereview.chromium.org/769153007/diff/240001/components/policy/core/...
components/policy/core/browser/managed_bookmarks_tracker.cc:199:
DCHECK(url->is_valid());
On 2015/01/26 15:40:14, bartfab wrote:
> Nit: #include "base/logging.h"

Done.

https://codereview.chromium.org/769153007/diff/240001/components/policy/core/...
File components/policy/core/browser/managed_bookmarks_tracker.h (right):

https://codereview.chromium.org/769153007/diff/240001/components/policy/core/...
components/policy/core/browser/managed_bookmarks_tracker.h:61: // Starts
tracking the policy for updates to the managed/supervised bookmarks.
On 2015/01/26 15:40:14, bartfab wrote:
> Nit: s/policy/pref/

Done.

https://codereview.chromium.org/769153007/diff/240001/components/policy/core/...
components/policy/core/browser/managed_bookmarks_tracker.h:72: base::string16
GetBookmarksFolderTitle() const;
On 2015/01/26 15:40:14, bartfab wrote:
> Nit: #include "base/strings/string16.h"

That's already there.

https://codereview.chromium.org/769153007/diff/240001/components/policy/core/...
components/policy/core/browser/managed_bookmarks_tracker.h:84: bool
is_supervised_;
On 2015/01/26 15:40:14, bartfab wrote:
> Nit: const.

Done.

https://codereview.chromium.org/769153007/diff/240001/components/policy/core/...
File components/policy/core/browser/managed_bookmarks_tracker_unittest.cc
(right):

https://codereview.chromium.org/769153007/diff/240001/components/policy/core/...
components/policy/core/browser/managed_bookmarks_tracker_unittest.cc:50: void
CreateModel() {
On 2015/01/26 15:40:14, bartfab wrote:
> Nit: Please remove this method and replace all callers with something like:
> 
> CreateModel(false /* is_supervised */);

Done.

https://codereview.chromium.org/769153007/diff/240001/components/policy/core/...
components/policy/core/browser/managed_bookmarks_tracker_unittest.cc:205:
CreateModel(true);
On 2015/01/26 15:40:14, bartfab wrote:
> Nit: Add /* is_supervised */

Done.

https://codereview.chromium.org/769153007/diff/240001/components/policy/core/...
components/policy/core/browser/managed_bookmarks_tracker_unittest.cc:213:
CreateModel(true);
On 2015/01/26 15:40:14, bartfab wrote:
> Nit: Add /* is_supervised */

Done.

https://codereview.chromium.org/769153007/diff/240001/components/policy/core/...
components/policy/core/browser/managed_bookmarks_tracker_unittest.cc:216: //
Don't bother checking the actual contents, the non-supervised tests cover
On 2015/01/26 15:40:14, bartfab wrote:
> You could parameterize the test and verify that both managed and supervised
are
> handled correctly. There is no way to guarantee that the code paths which
handle
> managed and supervised will not diverge at some point.

Hm. It's not "the test", but "every test in this file".
I think it's fairly unlikely that managed vs supervised will diverge (or if they
do, then they likely won't use the same class anymore anyway), so I don't think
it's worth it to parameterize/duplicate the tests.

[Marc Treib, 2015-01-26 16:18:32]

On 2015/01/26 15:40:14, bartfab wrote:
> However, I think that managed_bookmarks_tracker* should move somewhere else
now
> that handles more than just policy.

True. I could move it into components/bookmarks/browser/.
Bookmarks OWNERS: WDYT?

[oshima, 2015-01-26 19:31:34]

c/a/theme lgtm

[Marc Treib, 2015-02-03 17:45:23]

Friendly ping:
sky, can you take a look at the remaining (not-yet-reviewed) bookmarks code?
Thanks!

[sky, 2015-02-03 21:28:52]

Can you remind me which files that is?

On Tue, Feb 3, 2015 at 9:45 AM, <treib@chromium.org> wrote:

> Friendly ping:
> sky, can you take a look at the remaining (not-yet-reviewed) bookmarks
> code?
> Thanks!
>
> https://codereview.chromium.org/769153007/
>

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

[Marc Treib, 2015-02-04 08:53:26]

On 2015/02/03 21:28:52, sky wrote:
> Can you remind me which files that is?

Sure! (and sorry for not doing that before)

chrome/browser/android/bookmarks/bookmarks_bridge.cc
chrome/browser/bookmarks/*
chrome/browser/extensions/api/bookmarks/*
chrome/browser/ui/views/bookmarks/*
components/bookmarks/common/bookmark_pref_names.*

It's quite a lot of files, but c/b/bookmarks/ is really the only thing with
non-trivial changes.
Thanks again!


> On Tue, Feb 3, 2015 at 9:45 AM, <mailto:treib@chromium.org> wrote:
> 
> > Friendly ping:
> > sky, can you take a look at the remaining (not-yet-reviewed) bookmarks
> > code?
> > Thanks!
> >
> > https://codereview.chromium.org/769153007/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[sky, 2015-02-04 18:36:58]

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

https://codereview.chromium.org/769153007/diff/280001/chrome/browser/bookmark...
chrome/browser/bookmarks/chrome_bookmark_client.cc:53: for (size_t i = 0; i <
list.size(); ++i) {
nit: use new for loop

https://codereview.chromium.org/769153007/diff/280001/chrome/browser/bookmark...
chrome/browser/bookmarks/chrome_bookmark_client.cc:224:
scoped_ptr<BookmarkPermanentNode> supervised(new BookmarkPermanentNode(1));
This conflicts with the id of the bookmark bar.

https://codereview.chromium.org/769153007/diff/280001/chrome/browser/bookmark...
File chrome/browser/bookmarks/chrome_bookmark_client.h (right):

https://codereview.chromium.org/769153007/diff/280001/chrome/browser/bookmark...
chrome/browser/bookmarks/chrome_bookmark_client.h:51: bool
IsDescendantOfSupervisedNode(const BookmarkNode* node);
Can these new functions (and the two above it) be const? Also, I tend to think
it best to keep this class solely about exposing/maintaining the nodes. Any
functions that need to query these should be moved into their own file.

https://codereview.chromium.org/769153007/diff/280001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/769153007/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:883: if
(supervised_bookmarks_button_->visible()) {
What happens when the supervised, managed and apps shortcut are all visible and
the window is made as narrow as possible?

https://codereview.chromium.org/769153007/diff/280001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc (right):

https://codereview.chromium.org/769153007/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc:81: bool show_pushed
= show_options == SHOW_PERMANENT_FOLDERS &&
show_pushed is rather confusing here. How about a better name?

[Marc Treib, 2015-02-05 12:57:06]

Thanks for your comments! Some addressed, some further commented on below.

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

https://codereview.chromium.org/769153007/diff/280001/chrome/browser/bookmark...
chrome/browser/bookmarks/chrome_bookmark_client.cc:53: for (size_t i = 0; i <
list.size(); ++i) {
On 2015/02/04 18:36:58, sky wrote:
> nit: use new for loop

Done.

https://codereview.chromium.org/769153007/diff/280001/chrome/browser/bookmark...
chrome/browser/bookmarks/chrome_bookmark_client.cc:224:
scoped_ptr<BookmarkPermanentNode> supervised(new BookmarkPermanentNode(1));
On 2015/02/04 18:36:58, sky wrote:
> This conflicts with the id of the bookmark bar.

Where is that defined, and what ID can I use instead?
Also, how does the conflict manifest? Everything seems to be working fine, as
far as I can tell. Sorry if I'm being dense here, I'm not very familiar with the
bookmarks code.

https://codereview.chromium.org/769153007/diff/280001/chrome/browser/bookmark...
File chrome/browser/bookmarks/chrome_bookmark_client.h (right):

https://codereview.chromium.org/769153007/diff/280001/chrome/browser/bookmark...
chrome/browser/bookmarks/chrome_bookmark_client.h:51: bool
IsDescendantOfSupervisedNode(const BookmarkNode* node);
On 2015/02/04 18:36:58, sky wrote:
> Can these new functions (and the two above it) be const? Also, I tend to think
> it best to keep this class solely about exposing/maintaining the nodes. Any
> functions that need to query these should be moved into their own file.

They can indeed be const, and now are. Thanks!

What exactly are you suggesting when you say their callers should move to their
own file? CanSetPermanentNodeTitle, CanSyncNode, and CanBeEditedByUser query
these. They're overridden from BookmarkClient, so they need to stay in this
class.

https://codereview.chromium.org/769153007/diff/280001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/769153007/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:883: if
(supervised_bookmarks_button_->visible()) {
On 2015/02/04 18:36:58, sky wrote:
> What happens when the supervised, managed and apps shortcut are all visible
and
> the window is made as narrow as possible?

We're only exposing the supervised bookmarks for child accounts right now, which
can't also be dasher accounts, so the managed and supervised folders won't show
up at the same time.
That said: Chrome's current behavior is that it won't allow the window to get so
narrow that the supervised folder wouldn't fit.

https://codereview.chromium.org/769153007/diff/280001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc (right):

https://codereview.chromium.org/769153007/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc:81: bool show_pushed
= show_options == SHOW_PERMANENT_FOLDERS &&
On 2015/02/04 18:36:58, sky wrote:
> show_pushed is rather confusing here. How about a better name?

I've renamed it to show_forced_folders which I hope is slightly better. Do you
have a better suggestion?

[sky, 2015-02-05 17:57:11]

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

https://codereview.chromium.org/769153007/diff/280001/chrome/browser/bookmark...
chrome/browser/bookmarks/chrome_bookmark_client.cc:224:
scoped_ptr<BookmarkPermanentNode> supervised(new BookmarkPermanentNode(1));
On 2015/02/05 12:57:06, Marc Treib wrote:
> On 2015/02/04 18:36:58, sky wrote:
> > This conflicts with the id of the bookmark bar.
> 
> Where is that defined, and what ID can I use instead?
> Also, how does the conflict manifest? Everything seems to be working fine, as
> far as I can tell. Sorry if I'm being dense here, I'm not very familiar with
the
> bookmarks code.

From BookmarkNode:

  // Returns an unique id for this node.
  // For bookmark nodes that are managed by the bookmark model, the IDs are
  // persisted across sessions.
  int64 id() const { return id_; }

Your patch breaks this as the supervised node and bookmark bar nodes now have
the same id and are no longer unique. Functions like
bookmarks::GetBookmarkNodeByID() now are undeterministic.

The ids are used throughout the code in random places. Biggest case may be
copy/paste that writes the id (BookmarkNodeData).

As far as how to fix it, there needs to be some way for the client to have the
model create the nodes so the nodes have the right ids. Likely the client needs
to be handed a special interface for this.

https://codereview.chromium.org/769153007/diff/280001/chrome/browser/bookmark...
File chrome/browser/bookmarks/chrome_bookmark_client.h (right):

https://codereview.chromium.org/769153007/diff/280001/chrome/browser/bookmark...
chrome/browser/bookmarks/chrome_bookmark_client.h:51: bool
IsDescendantOfSupervisedNode(const BookmarkNode* node);
On 2015/02/05 12:57:06, Marc Treib wrote:
> On 2015/02/04 18:36:58, sky wrote:
> > Can these new functions (and the two above it) be const? Also, I tend to
think
> > it best to keep this class solely about exposing/maintaining the nodes. Any
> > functions that need to query these should be moved into their own file.
> 
> They can indeed be const, and now are. Thanks!
> 
> What exactly are you suggesting when you say their callers should move to
their
> own file? CanSetPermanentNodeTitle, CanSyncNode, and CanBeEditedByUser query
> these. They're overridden from BookmarkClient, so they need to stay in this
> class.

What I meant is that these functions should move to their own file, take the
supervised/managed node and the other node as arguments. Why? The functions
don't need any state from this class (other than the nodes you get from it).
Basically, prefer non-member non-friend functions to member functions (see
effective C++ for specifics).

How about moving IsDescendantOf to bookmark_utils and changing call sites to:

bookmark_utils::IsDescendantOf(supervised_node(), node) ?

https://codereview.chromium.org/769153007/diff/280001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/769153007/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:883: if
(supervised_bookmarks_button_->visible()) {
On 2015/02/05 12:57:06, Marc Treib wrote:
> On 2015/02/04 18:36:58, sky wrote:
> > What happens when the supervised, managed and apps shortcut are all visible
> and
> > the window is made as narrow as possible?
> 
> We're only exposing the supervised bookmarks for child accounts right now,
which
> can't also be dasher accounts, so the managed and supervised folders won't
show
> up at the same time.
> That said: Chrome's current behavior is that it won't allow the window to get
so
> narrow that the supervised folder wouldn't fit.

Ok.

https://codereview.chromium.org/769153007/diff/280001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc (right):

https://codereview.chromium.org/769153007/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc:81: bool show_pushed
= show_options == SHOW_PERMANENT_FOLDERS &&
On 2015/02/05 12:57:06, Marc Treib wrote:
> On 2015/02/04 18:36:58, sky wrote:
> > show_pushed is rather confusing here. How about a better name?
> 
> I've renamed it to show_forced_folders which I hope is slightly better. Do you
> have a better suggestion?

SGTM

[Marc Treib, 2015-02-06 13:40:37]

All remaining comments addressed, hopefully :)

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

https://codereview.chromium.org/769153007/diff/280001/chrome/browser/bookmark...
chrome/browser/bookmarks/chrome_bookmark_client.cc:224:
scoped_ptr<BookmarkPermanentNode> supervised(new BookmarkPermanentNode(1));
On 2015/02/05 17:57:11, sky wrote:
> On 2015/02/05 12:57:06, Marc Treib wrote:
> > On 2015/02/04 18:36:58, sky wrote:
> > > This conflicts with the id of the bookmark bar.
> > 
> > Where is that defined, and what ID can I use instead?
> > Also, how does the conflict manifest? Everything seems to be working fine,
as
> > far as I can tell. Sorry if I'm being dense here, I'm not very familiar with
> the
> > bookmarks code.
> 
> From BookmarkNode:
> 
>   // Returns an unique id for this node.
>   // For bookmark nodes that are managed by the bookmark model, the IDs are
>   // persisted across sessions.
>   int64 id() const { return id_; }
> 
> Your patch breaks this as the supervised node and bookmark bar nodes now have
> the same id and are no longer unique. Functions like
> bookmarks::GetBookmarkNodeByID() now are undeterministic.
> 
> The ids are used throughout the code in random places. Biggest case may be
> copy/paste that writes the id (BookmarkNodeData).
> 
> As far as how to fix it, there needs to be some way for the client to have the
> model create the nodes so the nodes have the right ids. Likely the client
needs
> to be handed a special interface for this.

I've taken another look, and I don't think that's necessary: Here we create the
nodes with a temporary/"empty" ID, but we don't do anything with them, so it
should be okay if the ids aren't unique. Later, in LoadExtraNodes (specifically,
in LoadInitialContents), we do assign proper unique IDs before returning them.
I've changed the hardcoded ID to 0 and added a comment to make this clear.

https://codereview.chromium.org/769153007/diff/280001/chrome/browser/bookmark...
File chrome/browser/bookmarks/chrome_bookmark_client.h (right):

https://codereview.chromium.org/769153007/diff/280001/chrome/browser/bookmark...
chrome/browser/bookmarks/chrome_bookmark_client.h:51: bool
IsDescendantOfSupervisedNode(const BookmarkNode* node);
On 2015/02/05 17:57:11, sky wrote:
> On 2015/02/05 12:57:06, Marc Treib wrote:
> > On 2015/02/04 18:36:58, sky wrote:
> > > Can these new functions (and the two above it) be const? Also, I tend to
> think
> > > it best to keep this class solely about exposing/maintaining the nodes.
Any
> > > functions that need to query these should be moved into their own file.
> > 
> > They can indeed be const, and now are. Thanks!
> > 
> > What exactly are you suggesting when you say their callers should move to
> their
> > own file? CanSetPermanentNodeTitle, CanSyncNode, and CanBeEditedByUser query
> > these. They're overridden from BookmarkClient, so they need to stay in this
> > class.
> 
> What I meant is that these functions should move to their own file, take the
> supervised/managed node and the other node as arguments. Why? The functions
> don't need any state from this class (other than the nodes you get from it).
> Basically, prefer non-member non-friend functions to member functions (see
> effective C++ for specifics).
> 
> How about moving IsDescendantOf to bookmark_utils and changing call sites to:
> 
> bookmark_utils::IsDescendantOf(supervised_node(), node) ?

Ah, I see now, thanks! Done.

[sky, 2015-02-06 16:39:33]

Perfect. Thanks for the comment. LGTM

[Marc Treib, 2015-02-06 16:49:05]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2015-02-06 16:50:12]

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

[commit-bot: I haz the power, 2015-02-06 17:02:50]

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

[commit-bot: I haz the power, 2015-02-06 17:02:51]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[Marc Treib, 2015-02-09 09:55:44]

New patchsets have been uploaded after l-g-t-m from sky@chromium.org

[Marc Treib, 2015-02-09 11:34:00]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2015-02-09 11:35:05]

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

[commit-bot: I haz the power, 2015-02-09 11:56:35]

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

[commit-bot: I haz the power, 2015-02-09 11:56:37]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
Timed out jobs are not retried to avoid causing additional load on the builders
with large pending queues.

[Marc Treib, 2015-02-09 13:26:46]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2015-02-09 13:27:30]

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

[commit-bot: I haz the power, 2015-02-09 13:48:07]

Committed patchset #19 (id:360001)

[commit-bot: I haz the power, 2015-02-09 13:48:48]

Patchset 19 (id:??) landed as
https://crrev.com/1a600c7b3fdff94fa7e00656191f37fb65dd05b1
Cr-Commit-Position: refs/heads/master@{#315289}