Makes all permanent nodes share the same node class so that visibility

Makes all permanent nodes share the same node class so that visibility
of any of the permanent nodes can be toggled.

BUG=none
TEST=none
R=noyau@chromium.org


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

[sky, 2011-12-06 22:38:33]

I'll send this around to others, but wanted to make sure you're happy with it
first.

  -Scott

[noyau (Ping after 24h), 2011-12-07 09:45:21]

http://codereview.chromium.org/8828006/diff/1/chrome/browser/bookmarks/bookma...
File chrome/browser/bookmarks/bookmark_model.cc (left):

http://codereview.chromium.org/8828006/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_model.cc:551: void
BookmarkModel::SetMobileFolderVisible(bool value) {
Maybe revisiting this method to be SetFolderVisible(BookmarkNode::Type type,
bool value)? Otherwise I'd have to create one method per type.

[noyau (Ping after 24h), 2011-12-07 10:27:56]

http://codereview.chromium.org/8828006/diff/1/chrome/browser/bookmarks/bookma...
File chrome/browser/bookmarks/bookmark_model.cc (right):

http://codereview.chromium.org/8828006/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_model.cc:108: visible_(true) {
Maybe I do not understand the code enough, but it seems to me that the Mobile
folder now defaults to visible if sync is not enabled. Your observer only
changes the state when a synchronized node is associated to the mobile folder,
and this code doesn't run when synchronization is not turned on.

In other word, I think there is a need to specify the default visibility of
permanent nodes in the constructor.

[sky, 2011-12-07 16:36:53]

I've uploaded a new snapshot.

http://codereview.chromium.org/8828006/diff/1/chrome/browser/bookmarks/bookma...
File chrome/browser/bookmarks/bookmark_model.cc (left):

http://codereview.chromium.org/8828006/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_model.cc:551: void
BookmarkModel::SetMobileFolderVisible(bool value) {
On 2011/12/07 09:45:22, noyau wrote:
> Maybe revisiting this method to be SetFolderVisible(BookmarkNode::Type type,
> bool value)? Otherwise I'd have to create one method per type.

Will do.

http://codereview.chromium.org/8828006/diff/1/chrome/browser/bookmarks/bookma...
File chrome/browser/bookmarks/bookmark_model.cc (right):

http://codereview.chromium.org/8828006/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_model.cc:108: visible_(true) {
On 2011/12/07 10:27:56, noyau wrote:
> Maybe I do not understand the code enough, but it seems to me that the Mobile
> folder now defaults to visible if sync is not enabled. Your observer only
> changes the state when a synchronized node is associated to the mobile folder,
> and this code doesn't run when synchronization is not turned on.
> 
> In other word, I think there is a need to specify the default visibility of
> permanent nodes in the constructor.

See line 733. The mobile node is special cased.

[noyau (Ping after 24h), 2011-12-07 16:49:33]

lgtm but for a misspelling of the method name.

http://codereview.chromium.org/8828006/diff/1/chrome/browser/bookmarks/bookma...
File chrome/browser/bookmarks/bookmark_model.cc (right):

http://codereview.chromium.org/8828006/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_model.cc:108: visible_(true) {
On 2011/12/07 16:36:53, sky wrote:
> On 2011/12/07 10:27:56, noyau wrote:
> > Maybe I do not understand the code enough, but it seems to me that the
Mobile
> > folder now defaults to visible if sync is not enabled. Your observer only
> > changes the state when a synchronized node is associated to the mobile
folder,
> > and this code doesn't run when synchronization is not turned on.
> > 
> > In other word, I think there is a need to specify the default visibility of
> > permanent nodes in the constructor.
> 
> See line 733. The mobile node is special cased.
Ah, I missed that line!

http://codereview.chromium.org/8828006/diff/5001/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_model.h (right):

http://codereview.chromium.org/8828006/diff/5001/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_model.h:353: void
SetPermanentNodeVisibile(BookmarkNode::Type type, bool value);
Spelling: Visibile instead of Visible.

[sky, 2011-12-07 17:06:53]

tim: bookmark_model_associator
brettw: the rest

[brettw, 2011-12-07 17:48:40]

lgtm

[sky, 2011-12-08 04:32:51]

Tim: ping

[noyau (Ping after 24h), 2011-12-08 11:13:53]

What's happening to the folder visibility if the user simply stop syncing? Will
the mobile folder visibility change? 

Minor request, would it be possible to rename UpdateMobileNodeVisibility() to
the less specific UpdatePermanentNodeVisibility()? I would make my life easier.

[sky, 2011-12-08 16:27:47]

On 2011/12/08 11:13:53, noyau wrote:
> What's happening to the folder visibility if the user simply stop syncing?
Will
> the mobile folder visibility change? 

Not sure there. 

> Minor request, would it be possible to rename UpdateMobileNodeVisibility() to
> the less specific UpdatePermanentNodeVisibility()? I would make my life
easier.

Sure. Done.

  -Scott

[noyau (Ping after 24h), 2011-12-09 15:54:34]

http://codereview.chromium.org/8828006/diff/19001/chrome/browser/bookmarks/bo...
File chrome/browser/bookmarks/bookmark_model.h (right):

http://codereview.chromium.org/8828006/diff/19001/chrome/browser/bookmarks/bo...
chrome/browser/bookmarks/bookmark_model.h:164: class BookmarkPermanentNode :
public BookmarkNode {
BookmarkPermanentNode doesn't need to be defined here, a forward declaration
would be sufficient with the definition in the implementation file.

[sky, 2011-12-09 17:57:04]

http://codereview.chromium.org/8828006/diff/19001/chrome/browser/bookmarks/bo...
File chrome/browser/bookmarks/bookmark_model.h (right):

http://codereview.chromium.org/8828006/diff/19001/chrome/browser/bookmarks/bo...
chrome/browser/bookmarks/bookmark_model.h:164: class BookmarkPermanentNode :
public BookmarkNode {
On 2011/12/09 15:54:35, noyau wrote:
> BookmarkPermanentNode doesn't need to be defined here, a forward declaration
> would be sufficient with the definition in the implementation file.

That's problematic because bookmark_bar_node_ is now of type
BookmarkPermanentNode but the getter returns BookmarkNode. Without this in the
header the compiler wouldn't be able to figure out BookmarkPermanentNode is a
BookmarkNode.

[akalin, 2011-12-09 17:58:35]

sync lgtm

[commit-bot: I haz the power, 2011-12-09 18:04:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/8828006/19001

[commit-bot: I haz the power, 2011-12-09 19:35:05]

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

[commit-bot: I haz the power, 2011-12-12 15:47:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/8828006/19001

[commit-bot: I haz the power, 2011-12-12 17:35:32]

Change committed as 114028