Revert "Revert 84829 - Initial implementation of "Synced Bookmarks" folder."

Revert "Revert 84829 - Initial implementation of "Synced Bookmarks" folder."

Second attempt to land this change. Fixes memory leak (82186), 
deleted bookmarks (82273) and crash on add bookmarks in windows (82349).

Original desscription:
Mostly ensures that sycned bookmarks are correctly treated as an immutable
folder. Some of the UI-bits maybe incomplete. For example, if
enable-synced-bookmarks-folder is set, then synced bookmarks will appear in the
bookmark manager page and some components of the UI but it's not on the bookmark
bar or anything like that. This change also ensures that the synced bookmark
folder matches a sync folder if one is available.


BUG=
TEST=test bookmark addition/moving around in combination with sync


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

[Yaron, 2011-05-12 00:39:55]

@sky:
Just one comment I wanted to ask for your opinion on at the get-go

I'll add other OWNERS (for the little ui-bits) once the core bookmark code is
approved.

Note that I looked at breaking this up for the second attempt. The only thing
that could nicely be parceled off is the addition of the switch, the proto
change and download_updates_command. I believe the rest of the logic is pretty
intertwined and I think I added the minimal assertions necessary in the UI code
to ensure sanity.

http://codereview.chromium.org/7012005/diff/5001/chrome/browser/ui/views/book...
File chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc (right):

http://codereview.chromium.org/7012005/diff/5001/chrome/browser/ui/views/book...
chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc:492:
DCHECK(root_node->child_count() == 3);
Are we concerned about the case of you going back in time with your chrome? So
namely if the flag becomes unset after it was once set, this could be wrong.
Should I assert that it's >= 2 and <= 3?

[sky, 2011-05-12 15:37:19]

http://codereview.chromium.org/7012005/diff/5001/chrome/browser/ui/views/book...
File chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc (right):

http://codereview.chromium.org/7012005/diff/5001/chrome/browser/ui/views/book...
chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc:492:
DCHECK(root_node->child_count() == 3);
On 2011/05/12 00:39:55, Yaron wrote:
> Are we concerned about the case of you going back in time with your chrome? So
> namely if the flag becomes unset after it was once set, this could be wrong.
> Should I assert that it's >= 2 and <= 3?

We generally show a warning to the user if they attempt to move a profile from
an older version to a newer version. As far as I know each subsystem that cares
shows a warning. Try opening the latest version of chrome then an earlier
version, say 10, to see the warning.

[sky, 2011-05-12 16:29:13]

I don't quite get what you've scoped behind the command line flag. For example,
the codec always reads/writes the synced node.

  -Scott

http://codereview.chromium.org/7012005/diff/5002/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_codec.cc (right):

http://codereview.chromium.org/7012005/diff/5002/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_codec.cc:154: Value* synced_folder_value;
Move this to where it's used, 171ish.

http://codereview.chromium.org/7012005/diff/5002/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_codec.cc:167: // It's not a critical error yet
if we can't deserialize the synced bookmark
This comment isn't quite right. For backward compatibility we can never assume
there is a synced folder.

http://codereview.chromium.org/7012005/diff/5002/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_codec_unittest.cc (right):

http://codereview.chromium.org/7012005/diff/5002/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_codec_unittest.cc:295:
TEST_F(BookmarkCodecTest, CanDecodeModelWithoutSyncedBookmarks) {
I assume we aren't going to have the switch forever, at which point this test
won't really be testing what we want it to. Additionally this is testing with
the current encoder, when we really want the old one. So, instead of this, save
a file to disk created with the last version (no synced folder) and then have
the test read that back in and make sure its ok.

http://codereview.chromium.org/7012005/diff/5002/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_model.cc (right):

http://codereview.chromium.org/7012005/diff/5002/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_model.cc:596: root_.Add(synced_node_, 2);
How come we don't always add the node to the model?

http://codereview.chromium.org/7012005/diff/5002/chrome/browser/history/starr...
File chrome/browser/history/starred_url_database.cc (right):

http://codereview.chromium.org/7012005/diff/5002/chrome/browser/history/starr...
chrome/browser/history/starred_url_database.cc:27: //                     
other, 4 for synced.
At one point bookmarks were saved in the db (this file), and not in the json
file. Anyone that stills has their bookmarks in the db is NOT going to have the
synced folder. Hopefully the only you should have to change in this file is the
call to encoder.Encode.

[Yaron, 2011-05-12 18:19:22]

Some initial responses. I still need to update the unit test as per your
request, but I wanted to address the higher-level question about the
command-line flag.

I think you're right that I was confused in what it's guarding. We could take
two approaches with the flag:
1) Completely guard this folder by the flag (i.e. unset, there are no changes)
2) Only control the visual display with this flag.

The reason I'd like to do the latter is because of sync. If, for example, on
your desktop you get the new chrome and you add bookmarks to this folder and it
syncs to chrome on another track on your laptop, then I think sync will get
angry and stop working. So I think it makes sense to have this new folder work
properly under the hood, and have the flag only control its visibility. There's
still a slightly oddity in how it interacts with sync, because the sync server
doesn't yet guarantee that it'll provide you with this permanent item unless you
request it, but if the sync server says that there are bookmarks under this
folder, we should sync those in to our local model, even if we're suppressing
the UI.

DOes that make sense and sound reasonable?

http://codereview.chromium.org/7012005/diff/5001/chrome/browser/ui/views/book...
File chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc (right):

http://codereview.chromium.org/7012005/diff/5001/chrome/browser/ui/views/book...
chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc:492:
DCHECK(root_node->child_count() == 3);
On 2011/05/12 15:37:19, sky wrote:
> On 2011/05/12 00:39:55, Yaron wrote:
> > Are we concerned about the case of you going back in time with your chrome?
So
> > namely if the flag becomes unset after it was once set, this could be wrong.
> > Should I assert that it's >= 2 and <= 3?
> 
> We generally show a warning to the user if they attempt to move a profile from
> an older version to a newer version. As far as I know each subsystem that
cares
> shows a warning. Try opening the latest version of chrome then an earlier
> version, say 10, to see the warning.

I see. Well, I changed it to allow 2-3 or nodes which  seems strictly safer. I'm
trying to make the flag control simply whether the synced bookmarks are visible
in the ui, but in the model you could have 3 even if the flag is unset.
Actually, in that case, should I add a guard to CreateNodes to only add Synced
if the command line flag is set?

http://codereview.chromium.org/7012005/diff/5002/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_codec.cc (right):

http://codereview.chromium.org/7012005/diff/5002/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_codec.cc:154: Value* synced_folder_value;
On 2011/05/12 16:29:13, sky wrote:
> Move this to where it's used, 171ish.

Done.

http://codereview.chromium.org/7012005/diff/5002/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_codec.cc:167: // It's not a critical error yet
if we can't deserialize the synced bookmark
On 2011/05/12 16:29:13, sky wrote:
> This comment isn't quite right. For backward compatibility we can never assume
> there is a synced folder.

So how did we ever make the switch to Other Bookmarks being required, or was it
always there? I didn't know if several releases down the line we could make it a
required filed. Updated comment.

http://codereview.chromium.org/7012005/diff/5002/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_model.cc (right):

http://codereview.chromium.org/7012005/diff/5002/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_model.cc:596: root_.Add(synced_node_, 2);
On 2011/05/12 16:29:13, sky wrote:
> How come we don't always add the node to the model?

Because that caused it to show in the UI. Perhaps I should just guard it at the
higher layer?

http://codereview.chromium.org/7012005/diff/5002/chrome/browser/history/starr...
File chrome/browser/history/starred_url_database.cc (right):

http://codereview.chromium.org/7012005/diff/5002/chrome/browser/history/starr...
chrome/browser/history/starred_url_database.cc:27: //                     
other, 4 for synced.
On 2011/05/12 16:29:13, sky wrote:
> At one point bookmarks were saved in the db (this file), and not in the json
> file. Anyone that stills has their bookmarks in the db is NOT going to have
the
> synced folder. Hopefully the only you should have to change in this file is
the
> call to encoder.Encode.

So we never actually write to this DB anymore, you're saying I should revert
everything except encode, including updating the enum? Well actually, I need to
update the enum for encode to make sense. Comment included below..

http://codereview.chromium.org/7012005/diff/5002/chrome/browser/history/starr...
chrome/browser/history/starred_url_database.cc:465: 
Looks like I should just revert this part?

[Yaron, 2011-05-12 20:17:46]

http://codereview.chromium.org/7012005/diff/7003/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_codec_unittest.cc (right):

http://codereview.chromium.org/7012005/diff/7003/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_codec_unittest.cc:313:
ExpectIDsUnique(&decoded_model);
My test is failing on this assertion. I thought that ids should be unique
throughout the tree and this should be a valid check. The problem is that the
synced bookmarks folder is given id 3, but that already exists in the serialized
tree. This may not be a problem in practice (you wouldn't make edits to an
immutable folder, and lookups for the permanent folders are done with special
accessors) but seems bad to leave around. Should I handle this by reassigning a
decoded id of 3 with a new id or only construct the synced node after decoding?

[sky, 2011-05-12 21:02:20]

On Thu, May 12, 2011 at 11:19 AM,  <yfriedman@chromium.org> wrote:
> Some initial responses. I still need to update the unit test as per your
> request, but I wanted to address the higher-level question about the
> command-line flag.
>
> I think you're right that I was confused in what it's guarding. We could
> take
> two approaches with the flag:
> 1) Completely guard this folder by the flag (i.e. unset, there are no
> changes)
> 2) Only control the visual display with this flag.
> The reason I'd like to do the latter is because of sync. If, for example, on
> your desktop you get the new chrome and you add bookmarks to this folder and
> it
> syncs to chrome on another track on your laptop, then I think sync will get
> angry and stop working. So I think it makes sense to have this new folder
> work
> properly under the hood, and have the flag only control its visibility.
> There's
> still a slightly oddity in how it interacts with sync, because the sync
> server
> doesn't yet guarantee that it'll provide you with this permanent item unless
> you
> request it, but if the sync server says that there are bookmarks under this
> folder, we should sync those in to our local model, even if we're
> suppressing
> the UI.
>
> DOes that make sense and sound reasonable?

Yes. I would also say that if there is anything in the synced folder,
I would force it to be visible regardless of the flag. I would say the
only thing the command line should do is to force the synced folder to
be visible if empty.

>
http://codereview.chromium.org/7012005/diff/5001/chrome/browser/ui/views/book...
> File chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc (right):
>
>
http://codereview.chromium.org/7012005/diff/5001/chrome/browser/ui/views/book...
> chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc:492:
> DCHECK(root_node->child_count() == 3);
> On 2011/05/12 15:37:19, sky wrote:
>>
>> On 2011/05/12 00:39:55, Yaron wrote:
>> > Are we concerned about the case of you going back in time with your
>
> chrome? So
>>
>> > namely if the flag becomes unset after it was once set, this could
>
> be wrong.
>>
>> > Should I assert that it's >= 2 and <= 3?
>
>> We generally show a warning to the user if they attempt to move a
>
> profile from
>>
>> an older version to a newer version. As far as I know each subsystem
>
> that cares
>>
>> shows a warning. Try opening the latest version of chrome then an
>
> earlier
>>
>> version, say 10, to see the warning.
>
> I see. Well, I changed it to allow 2-3 or nodes which  seems strictly
> safer. I'm trying to make the flag control simply whether the synced
> bookmarks are visible in the ui, but in the model you could have 3 even
> if the flag is unset. Actually, in that case, should I add a guard to
> CreateNodes to only add Synced if the command line flag is set?

I'm not sure what effects that would have on sync. Lets say you ran
once with the flag set, added a couple of things to the new folder,
then ran without the flag. Wouldn't that end up clobbering the synced
folder on exit?

>
http://codereview.chromium.org/7012005/diff/5002/chrome/browser/bookmarks/boo...
> File chrome/browser/bookmarks/bookmark_codec.cc (right):
>
>
http://codereview.chromium.org/7012005/diff/5002/chrome/browser/bookmarks/boo...
> chrome/browser/bookmarks/bookmark_codec.cc:154: Value*
> synced_folder_value;
> On 2011/05/12 16:29:13, sky wrote:
>>
>> Move this to where it's used, 171ish.
>
> Done.
>
>
http://codereview.chromium.org/7012005/diff/5002/chrome/browser/bookmarks/boo...
> chrome/browser/bookmarks/bookmark_codec.cc:167: // It's not a critical
> error yet if we can't deserialize the synced bookmark
> On 2011/05/12 16:29:13, sky wrote:
>>
>> This comment isn't quite right. For backward compatibility we can
>
> never assume
>>
>> there is a synced folder.
>
> So how did we ever make the switch to Other Bookmarks being required, or
> was it always there? I didn't know if several releases down the line we
> could make it a required filed. Updated comment.

The json form came after we decided on the other folder.

>
http://codereview.chromium.org/7012005/diff/5002/chrome/browser/bookmarks/boo...
> File chrome/browser/bookmarks/bookmark_model.cc (right):
>
>
http://codereview.chromium.org/7012005/diff/5002/chrome/browser/bookmarks/boo...
> chrome/browser/bookmarks/bookmark_model.cc:596: root_.Add(synced_node_,
> 2);
> On 2011/05/12 16:29:13, sky wrote:
>>
>> How come we don't always add the node to the model?
>
> Because that caused it to show in the UI. Perhaps I should just guard it
> at the higher layer?

I think so. See earlier comments.

>
http://codereview.chromium.org/7012005/diff/5002/chrome/browser/history/starr...
> File chrome/browser/history/starred_url_database.cc (right):
>
>
http://codereview.chromium.org/7012005/diff/5002/chrome/browser/history/starr...
> chrome/browser/history/starred_url_database.cc:27: //
>  other, 4 for synced.
> On 2011/05/12 16:29:13, sky wrote:
>>
>> At one point bookmarks were saved in the db (this file), and not in
>
> the json
>>
>> file. Anyone that stills has their bookmarks in the db is NOT going to
>
> have the
>>
>> synced folder. Hopefully the only you should have to change in this
>
> file is the
>>
>> call to encoder.Encode.
>
> So we never actually write to this DB anymore, you're saying I should
> revert everything except encode, including updating the enum? Well
> actually, I need to update the enum for encode to make sense. Comment
> included below..

Yes.

>
http://codereview.chromium.org/7012005/diff/5002/chrome/browser/history/starr...
> chrome/browser/history/starred_url_database.cc:465:
> Looks like I should just revert this part?
>
> http://codereview.chromium.org/7012005/
>

Yes. I'm hoping the only thing you need is line 662 new. Well, and
probably creating the synced_node.

  -Scott

[sky, 2011-05-12 21:14:32]

http://codereview.chromium.org/7012005/diff/7003/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_codec.cc (right):

http://codereview.chromium.org/7012005/diff/7003/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_codec.cc:172:
DecodeNode(*static_cast<DictionaryValue*>(synced_folder_value), NULL,
Here's where the bug is that leads to duplicate ids. BookmarkCodec::DecodeNode
tracks the unique id, but you only call it if you found the synced node. This
means we don't realize the id given to the synced node is already in use.

http://codereview.chromium.org/7012005/diff/7003/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_codec_unittest.cc (right):

http://codereview.chromium.org/7012005/diff/7003/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_codec_unittest.cc:313:
ExpectIDsUnique(&decoded_model);
On 2011/05/12 20:17:46, Yaron wrote:
> My test is failing on this assertion. I thought that ids should be unique
> throughout the tree and this should be a valid check. The problem is that the
> synced bookmarks folder is given id 3, but that already exists in the
serialized
> tree. This may not be a problem in practice (you wouldn't make edits to an
> immutable folder, and lookups for the permanent folders are done with special
> accessors) but seems bad to leave around. Should I handle this by reassigning
a
> decoded id of 3 with a new id or only construct the synced node after
decoding?

Good thing I had you write this test. I've added a comment where the bug is.

[Yaron, 2011-05-13 21:39:03]

On 2011/05/12 21:14:32, sky wrote:
>
http://codereview.chromium.org/7012005/diff/7003/chrome/browser/bookmarks/boo...
> File chrome/browser/bookmarks/bookmark_codec.cc (right):
> 
>
http://codereview.chromium.org/7012005/diff/7003/chrome/browser/bookmarks/boo...
> chrome/browser/bookmarks/bookmark_codec.cc:172:
> DecodeNode(*static_cast<DictionaryValue*>(synced_folder_value), NULL,
> Here's where the bug is that leads to duplicate ids. BookmarkCodec::DecodeNode
> tracks the unique id, but you only call it if you found the synced node. This
> means we don't realize the id given to the synced node is already in use.
> 
>
http://codereview.chromium.org/7012005/diff/7003/chrome/browser/bookmarks/boo...
> File chrome/browser/bookmarks/bookmark_codec_unittest.cc (right):
> 
>
http://codereview.chromium.org/7012005/diff/7003/chrome/browser/bookmarks/boo...
> chrome/browser/bookmarks/bookmark_codec_unittest.cc:313:
> ExpectIDsUnique(&decoded_model);
> On 2011/05/12 20:17:46, Yaron wrote:
> > My test is failing on this assertion. I thought that ids should be unique
> > throughout the tree and this should be a valid check. The problem is that
the
> > synced bookmarks folder is given id 3, but that already exists in the
> serialized
> > tree. This may not be a problem in practice (you wouldn't make edits to an
> > immutable folder, and lookups for the permanent folders are done with
special
> > accessors) but seems bad to leave around. Should I handle this by
reassigning
> a
> > decoded id of 3 with a new id or only construct the synced node after
> decoding?
> 
> Good thing I had you write this test. I've added a comment where the bug is.

I wanted to send this off to try-bots, but I'm still cleaning up some of the
view-related code. Looks like I also have a test or two to fix in the core model
code.

[sky, 2011-05-17 15:59:07]

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_codec.cc (right):

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_codec.cc:167: // them to exist in order to
backwards-compatible with older versions of
'to backwards' -> 'to be backwards'

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_codec.cc:177: ids_valid_ = false;
Can we special case this to avoid reassigning ids for everyone. The reason I
suggest this is that extensions use the id. We should attempt to keep the id
unique if we can.

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_model.cc (right):

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_model.cc:66: bool BookmarkNode::IsVisible()
const {
Don't we want it visible if not empty too?

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_model.cc:277: if (node == bookmark_bar_node_
|| node == other_node_ ||
is_permanent_node

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

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_model.h:118: bool IsVisible() const;
Can you add a TODO to remove this when synced is no longer a command line flag.

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_model_unittest.cc (right):

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_model_unittest.cc:1052:
switches::kEnableSyncedBookmarksFolder)) {
Spacing is off. Also, don't leak this flag across all tests. Set it here, and
unset it in Teardown so that you know it gets unset.

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_utils.cc (right):

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_utils.cc:487: if (nodes.size() < max_count &&
I don't think you want this inner if, do you?
Also, shouldn't you be checking IsVisible?

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/bookmarks/rec...
File chrome/browser/bookmarks/recently_used_folders_combo_model.cc (right):

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/bookmarks/rec...
chrome/browser/bookmarks/recently_used_folders_combo_model.cc:34: if
(node->parent() != model->GetBookmarkBarNode() &&
is_permanent_node

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_bookmark_helpers.cc (right):

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_bookmark_helpers.cc:95: if (node ==
model->root_node() ||
is_permanent_node

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_bookmarks_module.cc (right):

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_bookmarks_module.cc:524: if (node ==
model->root_node() ||
is_permanent_node

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_bookmarks_module.cc:619: if (node ==
model->root_node() ||
is_permanent_node

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/history/histo...
File chrome/browser/history/history_types.h (right):

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/history/histo...
chrome/browser/history/history_types.h:285: // The synced folder.
Add after OTHER

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/history/starr...
File chrome/browser/history/starred_url_database.cc (right):

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/history/starr...
chrome/browser/history/starred_url_database.cc:27: //                     
other, 4 for synced.
This will never be in the database, so you can remove it.

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/history/starr...
chrome/browser/history/starred_url_database.cc:70: entry->type =
history::StarredEntry::SYNCED;
Remove this.

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/history/starr...
chrome/browser/history/starred_url_database.cc:219: case
history::StarredEntry::SYNCED:
Remove this.

[Yaron, 2011-05-17 19:17:32]

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_codec.cc (right):

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_codec.cc:167: // them to exist in order to
backwards-compatible with older versions of
On 2011/05/17 15:59:08, sky wrote:
> 'to backwards' -> 'to be backwards'

Done.

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_codec.cc:177: ids_valid_ = false;
On 2011/05/17 15:59:08, sky wrote:
> Can we special case this to avoid reassigning ids for everyone. The reason I
> suggest this is that extensions use the id. We should attempt to keep the id
> unique if we can.

Ok, I made it only reassign the synced folder if nothing else would be
reassigned.

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_model.cc (right):

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_model.cc:66: bool BookmarkNode::IsVisible()
const {
On 2011/05/17 15:59:08, sky wrote:
> Don't we want it visible if not empty too?

I discussed this with Srikanth and the plan is to keep this as is for now
(namely, only make it visible if the flag is set).

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_model.cc:277: if (node == bookmark_bar_node_
|| node == other_node_ ||
On 2011/05/17 15:59:08, sky wrote:
> is_permanent_node

Done.

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

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_model.h:118: bool IsVisible() const;
On 2011/05/17 15:59:08, sky wrote:
> Can you add a TODO to remove this when synced is no longer a command line
flag.

Done.

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_model_unittest.cc (right):

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_model_unittest.cc:1052:
switches::kEnableSyncedBookmarksFolder)) {
On 2011/05/17 15:59:08, sky wrote:
> Spacing is off. Also, don't leak this flag across all tests. Set it here, and
> unset it in Teardown so that you know it gets unset.

That's what I wanted to do, but I can't actually see how to unset it. Are you
familiar with the API? I suppose I could change the "hasSwitch" call to
something like GetSwitchValue and make sure it's "true". Then tearDown could
call AppendSwitchASCII with "false". Unless you know of an easier way?

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_utils.cc (right):

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_utils.cc:487: if (nodes.size() < max_count &&
On 2011/05/17 15:59:08, sky wrote:
> I don't think you want this inner if, do you?
> Also, shouldn't you be checking IsVisible?

Woah. Not sure what happened there. Fixed. And ya, added IsVisible guard.

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/bookmarks/rec...
File chrome/browser/bookmarks/recently_used_folders_combo_model.cc (right):

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/bookmarks/rec...
chrome/browser/bookmarks/recently_used_folders_combo_model.cc:34: if
(node->parent() != model->GetBookmarkBarNode() &&
On 2011/05/17 15:59:08, sky wrote:
> is_permanent_node

Done. Good call on all these. Should be easier to add another node if we ever
need to in the future :)

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_bookmarks_module.cc (right):

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_bookmarks_module.cc:524: if (node ==
model->root_node() ||
On 2011/05/17 15:59:08, sky wrote:
> is_permanent_node

Done.

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_bookmarks_module.cc:619: if (node ==
model->root_node() ||
On 2011/05/17 15:59:08, sky wrote:
> is_permanent_node

Done.

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/history/histo...
File chrome/browser/history/history_types.h (right):

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/history/histo...
chrome/browser/history/history_types.h:285: // The synced folder.
On 2011/05/17 15:59:08, sky wrote:
> Add after OTHER

Done.

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/history/starr...
File chrome/browser/history/starred_url_database.cc (right):

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/history/starr...
chrome/browser/history/starred_url_database.cc:27: //                     
other, 4 for synced.
On 2011/05/17 15:59:08, sky wrote:
> This will never be in the database, so you can remove it.

Done.

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/history/starr...
chrome/browser/history/starred_url_database.cc:70: entry->type =
history::StarredEntry::SYNCED;
On 2011/05/17 15:59:08, sky wrote:
> Remove this.

Done.

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/history/starr...
chrome/browser/history/starred_url_database.cc:219: case
history::StarredEntry::SYNCED:
On 2011/05/17 15:59:08, sky wrote:
> Remove this.

Done.

[sky, 2011-05-17 20:00:05]

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_model.cc (right):

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_model.cc:66: bool BookmarkNode::IsVisible()
const {
On 2011/05/17 19:17:33, Yaron wrote:
> On 2011/05/17 15:59:08, sky wrote:
> > Don't we want it visible if not empty too?
> 
> I discussed this with Srikanth and the plan is to keep this as is for now
> (namely, only make it visible if the flag is set).

If we do that, couldn't that lead to weird states? For example, lets say you
enable the flag, bookmark google in the synced folder, unset the flag, relaunch
chrome, go to google, click the star, then the edit button. The sync folder
won't be shown, right? This scenario may cause logic problems in the editor and
other places.

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_model_unittest.cc (right):

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_model_unittest.cc:1052:
switches::kEnableSyncedBookmarksFolder)) {
On 2011/05/17 19:17:33, Yaron wrote:
> On 2011/05/17 15:59:08, sky wrote:
> > Spacing is off. Also, don't leak this flag across all tests. Set it here,
and
> > unset it in Teardown so that you know it gets unset.
> 
> That's what I wanted to do, but I can't actually see how to unset it. Are you
> familiar with the API? I suppose I could change the "hasSwitch" call to
> something like GetSwitchValue and make sure it's "true". Then tearDown could
> call AppendSwitchASCII with "false". Unless you know of an easier way?

In the constructor do:

original_command_line_(*CommandLine::ForCurrentProcess())

In Teardown do:
*CommandLine::ForCurrentProcess() = original_command_line_;

http://codereview.chromium.org/7012005/diff/14001/chrome/browser/bookmarks/bo...
File chrome/browser/bookmarks/bookmark_codec.cc (right):

http://codereview.chromium.org/7012005/diff/14001/chrome/browser/bookmarks/bo...
chrome/browser/bookmarks/bookmark_codec.cc:183: 
remove empty line.

http://codereview.chromium.org/7012005/diff/14001/chrome/browser/bookmarks/bo...
File chrome/browser/bookmarks/bookmark_utils.cc (right):

http://codereview.chromium.org/7012005/diff/14001/chrome/browser/bookmarks/bo...
chrome/browser/bookmarks/bookmark_utils.cc:483: if (nodes.size() < max_count &&
model->synced_node()->IsVisible() &&
I think you want this outside the if loop on 479. Meaning it should be at the
same nesting level as 479.

[Yaron, 2011-05-17 21:38:37]

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_model.cc (right):

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_model.cc:66: bool BookmarkNode::IsVisible()
const {
On 2011/05/17 20:00:05, sky wrote:
> On 2011/05/17 19:17:33, Yaron wrote:
> > On 2011/05/17 15:59:08, sky wrote:
> > > Don't we want it visible if not empty too?
> > 
> > I discussed this with Srikanth and the plan is to keep this as is for now
> > (namely, only make it visible if the flag is set).
> 
> If we do that, couldn't that lead to weird states? For example, lets say you
> enable the flag, bookmark google in the synced folder, unset the flag,
relaunch
> chrome, go to google, click the star, then the edit button. The sync folder
> won't be shown, right? This scenario may cause logic problems in the editor
and
> other places.

I think we'd be ok with some inconsistency with toggling the flag on and off,
since it's not really a user-supported option, but I am concerned about the
potential for a crash. Unsurprisingly, the situation you mentioned did crash so
I fixed it (see the diff in boomark_tree_model). Regardless, Srikanth said it's
fine to make it visible if it has children. I made the change and updated the
test.

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_model_unittest.cc (right):

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_model_unittest.cc:1052:
switches::kEnableSyncedBookmarksFolder)) {
On 2011/05/17 20:00:05, sky wrote:
> On 2011/05/17 19:17:33, Yaron wrote:
> > On 2011/05/17 15:59:08, sky wrote:
> > > Spacing is off. Also, don't leak this flag across all tests. Set it here,
> and
> > > unset it in Teardown so that you know it gets unset.
> > 
> > That's what I wanted to do, but I can't actually see how to unset it. Are
you
> > familiar with the API? I suppose I could change the "hasSwitch" call to
> > something like GetSwitchValue and make sure it's "true". Then tearDown could
> > call AppendSwitchASCII with "false". Unless you know of an easier way?
> 
> In the constructor do:
> 
> original_command_line_(*CommandLine::ForCurrentProcess())
> 
> In Teardown do:
> *CommandLine::ForCurrentProcess() = original_command_line_;

Done.

http://codereview.chromium.org/7012005/diff/14001/chrome/browser/bookmarks/bo...
File chrome/browser/bookmarks/bookmark_codec.cc (right):

http://codereview.chromium.org/7012005/diff/14001/chrome/browser/bookmarks/bo...
chrome/browser/bookmarks/bookmark_codec.cc:183: 
On 2011/05/17 20:00:05, sky wrote:
> remove empty line.

Done.

http://codereview.chromium.org/7012005/diff/14001/chrome/browser/bookmarks/bo...
File chrome/browser/bookmarks/bookmark_utils.cc (right):

http://codereview.chromium.org/7012005/diff/14001/chrome/browser/bookmarks/bo...
chrome/browser/bookmarks/bookmark_utils.cc:483: if (nodes.size() < max_count &&
model->synced_node()->IsVisible() &&
On 2011/05/17 20:00:05, sky wrote:
> I think you want this outside the if loop on 479. Meaning it should be at the
> same nesting level as 479.

Done.

[Yaron, 2011-05-17 21:40:13]

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_model.cc (right):

http://codereview.chromium.org/7012005/diff/4007/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_model.cc:66: bool BookmarkNode::IsVisible()
const {
On 2011/05/17 21:38:37, Yaron wrote:
> On 2011/05/17 20:00:05, sky wrote:
> > On 2011/05/17 19:17:33, Yaron wrote:
> > > On 2011/05/17 15:59:08, sky wrote:
> > > > Don't we want it visible if not empty too?
> > > 
> > > I discussed this with Srikanth and the plan is to keep this as is for now
> > > (namely, only make it visible if the flag is set).
> > 
> > If we do that, couldn't that lead to weird states? For example, lets say you
> > enable the flag, bookmark google in the synced folder, unset the flag,
> relaunch
> > chrome, go to google, click the star, then the edit button. The sync folder
> > won't be shown, right? This scenario may cause logic problems in the editor
> and
> > other places.
> 
> I think we'd be ok with some inconsistency with toggling the flag on and off,
> since it's not really a user-supported option, but I am concerned about the
> potential for a crash. Unsurprisingly, the situation you mentioned did crash
so
> I fixed it (see the diff in boomark_tree_model). Regardless, Srikanth said
it's
> fine to make it visible if it has children. I made the change and updated the
> test.

Err, it's in bookmark_editor_gtk.cc

[sky, 2011-05-18 18:45:05]

LGTM

I didn't look at any of the sync code. I'm assuming Tim is doing that.

  -Scott

[Yaron, 2011-05-18 18:56:44]

Thanks for the review!

Adding pinkerton and erg for OWNERS over the ui/cocoa and ui/gtk folders

[Elliot Glaysher, 2011-05-18 21:39:29]

http://codereview.chromium.org/7012005/diff/14002/chrome/browser/ui/gtk/bookm...
File chrome/browser/ui/gtk/bookmarks/bookmark_editor_gtk.cc (right):

http://codereview.chromium.org/7012005/diff/14002/chrome/browser/ui/gtk/bookm...
chrome/browser/ui/gtk/bookmarks/bookmark_editor_gtk.cc:317: GtkTreeIter*
selected_iter = NULL;
I am pretty sure the memory handling here is incorrect. selected_iter is storage
space to write into and is usually stack allocated (see gtk docs). I don't see
it being allocated anywhere in the bookmark_tree_model.cc

[Yaron, 2011-05-18 23:29:01]

http://codereview.chromium.org/7012005/diff/14002/chrome/browser/ui/gtk/bookm...
File chrome/browser/ui/gtk/bookmarks/bookmark_editor_gtk.cc (right):

http://codereview.chromium.org/7012005/diff/14002/chrome/browser/ui/gtk/bookm...
chrome/browser/ui/gtk/bookmarks/bookmark_editor_gtk.cc:317: GtkTreeIter*
selected_iter = NULL;
On 2011/05/18 21:39:29, Elliot Glaysher wrote:
> I am pretty sure the memory handling here is incorrect. selected_iter is
storage
> space to write into and is usually stack allocated (see gtk docs). I don't see
> it being allocated anywhere in the bookmark_tree_model.cc

Done.

[Elliot Glaysher, 2011-05-18 23:51:41]

http://codereview.chromium.org/7012005/diff/13003/chrome/browser/ui/gtk/bookm...
File chrome/browser/ui/gtk/bookmarks/bookmark_editor_gtk.cc (right):

http://codereview.chromium.org/7012005/diff/13003/chrome/browser/ui/gtk/bookm...
chrome/browser/ui/gtk/bookmarks/bookmark_editor_gtk.cc:333: if (selected_id &&
selected_iter.user_data != NULL) {
I don't understand this modified line. I don't see where user_data gets set or
why you can rely on this.

[Yaron, 2011-05-19 00:05:14]

http://codereview.chromium.org/7012005/diff/13003/chrome/browser/ui/gtk/bookm...
File chrome/browser/ui/gtk/bookmarks/bookmark_editor_gtk.cc (right):

http://codereview.chromium.org/7012005/diff/13003/chrome/browser/ui/gtk/bookm...
chrome/browser/ui/gtk/bookmarks/bookmark_editor_gtk.cc:333: if (selected_id &&
selected_iter.user_data != NULL) {
On 2011/05/18 23:51:42, Elliot Glaysher wrote:
> I don't understand this modified line. I don't see where user_data gets set or
> why you can rely on this.

It gets set within bookmark_utils::AddToTreeStore which calls
bookmark_utils::AddToTreeStoreAt which copies iter to  selected_iter.
(http://google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/chrome/browser/ui/gtk/bo...)


iter's user_data field gets set in AddSingleNodeToTreeStore, presumably in one
of the gtk calls:
http://google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/chrome/browser/ui/gtk/bo...

The way I discovered this was that before we wouldn't show Synced Bookmarks even
if it had children. So if the current page was bookmarked, I hit the star, and
clicked edit, it would crash on a check for user_data being non-null. Since this
was technically fixed another way (we no longer hide nodes if they have
children), this isn't strictly necessary but I thought it was safer to include
it. I can remove it if you think that's better.

[Elliot Glaysher, 2011-05-19 18:13:34]

On Wed, May 18, 2011 at 5:05 PM,  <yfriedman@chromium.org> wrote:
> I can remove it if you think that's better.

Grepping through all our bookmark code, we never set user_data. My
apologies if I was unclear: my point wasn't that user_data was never
set, but that we never set it and are relying on internal gtk
behaviour and that scares the heck out of me. The GtkTreeIter struct
internals aren't documented on
http://developer.gnome.org/gtk/2.24/GtkTreeModel.html#GtkTreeIter-struct
(though the purpose of the user_data field is documented in what I
believe to be a non-normative paragraph). It would probably be better
to drop it.

> http://codereview.chromium.org/7012005/
>

[Yaron, 2011-05-19 21:44:42]

On 2011/05/19 18:13:34, Elliot Glaysher wrote:
> On Wed, May 18, 2011 at 5:05 PM,  <mailto:yfriedman@chromium.org> wrote:
> > I can remove it if you think that's better.
> 
> Grepping through all our bookmark code, we never set user_data. My
> apologies if I was unclear: my point wasn't that user_data was never
> set, but that we never set it and are relying on internal gtk
> behaviour and that scares the heck out of me. The GtkTreeIter struct
> internals aren't documented on
> http://developer.gnome.org/gtk/2.24/GtkTreeModel.html#GtkTreeIter-struct
> (though the purpose of the user_data field is documented in what I
> believe to be a non-normative paragraph). It would probably be better
> to drop it.
> 
> > http://codereview.chromium.org/7012005/
> >

Ok, done.

[Elliot Glaysher, 2011-05-19 22:00:20]

On 2011/05/19 21:44:42, Yaron wrote:
> On 2011/05/19 18:13:34, Elliot Glaysher wrote:
> > On Wed, May 18, 2011 at 5:05 PM,  <mailto:yfriedman@chromium.org> wrote:
> > > I can remove it if you think that's better.
> > 
> > Grepping through all our bookmark code, we never set user_data. My
> > apologies if I was unclear: my point wasn't that user_data was never
> > set, but that we never set it and are relying on internal gtk
> > behaviour and that scares the heck out of me. The GtkTreeIter struct
> > internals aren't documented on
> > http://developer.gnome.org/gtk/2.24/GtkTreeModel.html#GtkTreeIter-struct
> > (though the purpose of the user_data field is documented in what I
> > believe to be a non-normative paragraph). It would probably be better
> > to drop it.
> > 
> > > http://codereview.chromium.org/7012005/
> > >
> 
> Ok, done.

LGTM (I assume there are no changes to those files now since they aren't on the
list)

[tim (not reviewing), 2011-05-20 16:43:09]

Bit tough to follow through the review so forgive me if this is already in
there, but what's the behavior if the synced node exists in the sync db (due to
previous run with --enable), and you restart without the flag?

http://codereview.chromium.org/7012005/diff/24002/chrome/browser/sync/glue/bo...
File chrome/browser/sync/glue/bookmark_model_associator.cc (right):

http://codereview.chromium.org/7012005/diff/24002/chrome/browser/sync/glue/bo...
chrome/browser/sync/glue/bookmark_model_associator.cc:385:
DCHECK(synced_bookmarks_sync_id != sync_api::kInvalidId);
DCHECK_NE?

[Yaron, 2011-05-20 17:04:25]

In any case, if the sync node comes down from the server, we'll sync any
bookmarks in it. That's because we treat the sync node as always present in the
bookmark model. The only trick we do is that we suppress the Sync node if it's
empty and the flag is unset. If, however, sync brings back nodes from the
server, then we will expose the Synced bookmarks folder in the ui, regardless of
the flag setting. Actually, I haven't tested that happening while the browser is
running. I'll do that now.

http://codereview.chromium.org/7012005/diff/24002/chrome/browser/sync/glue/bo...
File chrome/browser/sync/glue/bookmark_model_associator.cc (right):

http://codereview.chromium.org/7012005/diff/24002/chrome/browser/sync/glue/bo...
chrome/browser/sync/glue/bookmark_model_associator.cc:385:
DCHECK(synced_bookmarks_sync_id != sync_api::kInvalidId);
On 2011/05/20 16:43:09, timsteele wrote:
> DCHECK_NE?

I was just being consistent with the above code but I changed all of them since
the failure message would probably be more useful.

[sky, 2011-05-20 17:52:43]

And by suppress I believe you mean, not surface the synced folder in
the UI (editor, manager, bubble...).

  -Scott

On Fri, May 20, 2011 at 10:04 AM,  <yfriedman@chromium.org> wrote:
> In any case, if the sync node comes down from the server, we'll sync any
> bookmarks in it. That's because we treat the sync node as always present in
> the
> bookmark model. The only trick we do is that we suppress the Sync node if
> it's
> empty and the flag is unset. If, however, sync brings back nodes from the
> server, then we will expose the Synced bookmarks folder in the ui,
> regardless of
> the flag setting. Actually, I haven't tested that happening while the
> browser is
> running. I'll do that now.
>
>
>
http://codereview.chromium.org/7012005/diff/24002/chrome/browser/sync/glue/bo...
> File chrome/browser/sync/glue/bookmark_model_associator.cc (right):
>
>
http://codereview.chromium.org/7012005/diff/24002/chrome/browser/sync/glue/bo...
> chrome/browser/sync/glue/bookmark_model_associator.cc:385:
> DCHECK(synced_bookmarks_sync_id != sync_api::kInvalidId);
> On 2011/05/20 16:43:09, timsteele wrote:
>>
>> DCHECK_NE?
>
> I was just being consistent with the above code but I changed all of
> them since the failure message would probably be more useful.
>
> http://codereview.chromium.org/7012005/
>

[Yaron, 2011-05-20 18:07:23]

On 2011/05/20 17:52:43, sky wrote:
> And by suppress I believe you mean, not surface the synced folder in
> the UI (editor, manager, bubble...).
> 
>   -Scott
> 
> On Fri, May 20, 2011 at 10:04 AM,  <mailto:yfriedman@chromium.org> wrote:
> > In any case, if the sync node comes down from the server, we'll sync any
> > bookmarks in it. That's because we treat the sync node as always present in
> > the
> > bookmark model. The only trick we do is that we suppress the Sync node if
> > it's
> > empty and the flag is unset. If, however, sync brings back nodes from the
> > server, then we will expose the Synced bookmarks folder in the ui,
> > regardless of
> > the flag setting. Actually, I haven't tested that happening while the
> > browser is
> > running. I'll do that now.
> >
> >
> >
>
http://codereview.chromium.org/7012005/diff/24002/chrome/browser/sync/glue/bo...
> > File chrome/browser/sync/glue/bookmark_model_associator.cc (right):
> >
> >
>
http://codereview.chromium.org/7012005/diff/24002/chrome/browser/sync/glue/bo...
> > chrome/browser/sync/glue/bookmark_model_associator.cc:385:
> > DCHECK(synced_bookmarks_sync_id != sync_api::kInvalidId);
> > On 2011/05/20 16:43:09, timsteele wrote:
> >>
> >> DCHECK_NE?
> >
> > I was just being consistent with the above code but I changed all of
> > them since the failure message would probably be more useful.
> >
> > http://codereview.chromium.org/7012005/
> >

Yep, that's correct

[Yaron, 2011-05-20 22:08:54]

On 2011/05/20 18:07:23, Yaron wrote:
> On 2011/05/20 17:52:43, sky wrote:
> > And by suppress I believe you mean, not surface the synced folder in
> > the UI (editor, manager, bubble...).
> > 
> >   -Scott
> > 
> > On Fri, May 20, 2011 at 10:04 AM,  <mailto:yfriedman@chromium.org> wrote:
> > > In any case, if the sync node comes down from the server, we'll sync any
> > > bookmarks in it. That's because we treat the sync node as always present
in
> > > the
> > > bookmark model. The only trick we do is that we suppress the Sync node if
> > > it's
> > > empty and the flag is unset. If, however, sync brings back nodes from the
> > > server, then we will expose the Synced bookmarks folder in the ui,
> > > regardless of
> > > the flag setting. Actually, I haven't tested that happening while the
> > > browser is
> > > running. I'll do that now.
> > >
> > >
> > >
> >
>
http://codereview.chromium.org/7012005/diff/24002/chrome/browser/sync/glue/bo...
> > > File chrome/browser/sync/glue/bookmark_model_associator.cc (right):
> > >
> > >
> >
>
http://codereview.chromium.org/7012005/diff/24002/chrome/browser/sync/glue/bo...
> > > chrome/browser/sync/glue/bookmark_model_associator.cc:385:
> > > DCHECK(synced_bookmarks_sync_id != sync_api::kInvalidId);
> > > On 2011/05/20 16:43:09, timsteele wrote:
> > >>
> > >> DCHECK_NE?
> > >
> > > I was just being consistent with the above code but I changed all of
> > > them since the failure message would probably be more useful.
> > >
> > > http://codereview.chromium.org/7012005/
> > >
> 
> Yep, that's correct

I tested with side-by-side chromes with and without
enable-synced-bookmarks-folder. It seems to work well now and without crashes,
including a chrome started without the flag, then getting a synced bookmark
folder pushed down from the new chrome syncing. Subsequent additions and
deletions seemed to work too.

As a sanity check, I looked at running sync_integration_tests with
enable-synced-bookmarks-folder as a command-line flag, but the python test
server doesn't create the new folder so the test fails quickly. Tim, do you know
where it is/how to modify it?

[tim (not reviewing), 2011-05-20 23:04:05]

On 2011/05/20 22:08:54, Yaron wrote:
> On 2011/05/20 18:07:23, Yaron wrote:
> > On 2011/05/20 17:52:43, sky wrote:
> > > And by suppress I believe you mean, not surface the synced folder in
> > > the UI (editor, manager, bubble...).
> > > 
> > >   -Scott
> > > 
> > > On Fri, May 20, 2011 at 10:04 AM,  <mailto:yfriedman@chromium.org> wrote:
> > > > In any case, if the sync node comes down from the server, we'll sync any
> > > > bookmarks in it. That's because we treat the sync node as always present
> in
> > > > the
> > > > bookmark model. The only trick we do is that we suppress the Sync node
if
> > > > it's
> > > > empty and the flag is unset. If, however, sync brings back nodes from
the
> > > > server, then we will expose the Synced bookmarks folder in the ui,
> > > > regardless of
> > > > the flag setting. Actually, I haven't tested that happening while the
> > > > browser is
> > > > running. I'll do that now.
> > > >
> > > >
> > > >
> > >
> >
>
http://codereview.chromium.org/7012005/diff/24002/chrome/browser/sync/glue/bo...
> > > > File chrome/browser/sync/glue/bookmark_model_associator.cc (right):
> > > >
> > > >
> > >
> >
>
http://codereview.chromium.org/7012005/diff/24002/chrome/browser/sync/glue/bo...
> > > > chrome/browser/sync/glue/bookmark_model_associator.cc:385:
> > > > DCHECK(synced_bookmarks_sync_id != sync_api::kInvalidId);
> > > > On 2011/05/20 16:43:09, timsteele wrote:
> > > >>
> > > >> DCHECK_NE?
> > > >
> > > > I was just being consistent with the above code but I changed all of
> > > > them since the failure message would probably be more useful.
> > > >
> > > > http://codereview.chromium.org/7012005/
> > > >
> > 
> > Yep, that's correct
> 
> I tested with side-by-side chromes with and without
> enable-synced-bookmarks-folder. It seems to work well now and without crashes,
> including a chrome started without the flag, then getting a synced bookmark
> folder pushed down from the new chrome syncing. Subsequent additions and
> deletions seemed to work too.
> 
> As a sanity check, I looked at running sync_integration_tests with
> enable-synced-bookmarks-folder as a command-line flag, but the python test
> server doesn't create the new folder so the test fails quickly. Tim, do you
know
> where it is/how to modify it?

Yup, it's in net/tools/testserver/chromiumsync.py
LGTM

[Yaron, 2011-05-21 01:13:46]

Thanks Tim! So with a minor change to have it always return the synced bookmarks
folder, both:

xvfb-run out/Debug/sync_integration_tests  --gtest_filter='*Bookmark*'
xvfb-run out/Debug/sync_integration_tests  --gtest_filter='*Bookmark*'
--enable-synced-bookmarks-folder

are passing. I think all that's left is an OWNERs stamp from Mike for the change
to cocoa. I've sent it off to try bots for another verification

[pink (ping after 24hrs), 2011-05-24 16:09:18]

LGTM on mac stuff

[commit-bot: I haz the power, 2011-05-26 16:57:14]

Presubmit check for 7012005-29004 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Run the command: svn pset svn:eol-style LF \
  chrome/test/data/bookmarks/model_without_sync.json

Presubmit checks took 1.1s to calculate.

[commit-bot: I haz the power, 2011-05-26 17:13:11]

Presubmit check for 7012005-29004 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Run the command: svn pset svn:eol-style LF \
  chrome/test/data/bookmarks/model_without_sync.json

Presubmit checks took 1.2s to calculate.

[commit-bot: I haz the power, 2011-05-26 17:15:07]

Presubmit check for 7012005-37001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Run the command: svn pset svn:eol-style LF \
  chrome/test/data/bookmarks/model_without_sync.json

Presubmit checks took 1.2s to calculate.

[M-A Ruel, 2011-05-26 17:27:43]

On 2011/05/26 17:15:07, commit-bot wrote:
> Presubmit check for 7012005-37001 failed and returned exit status 1.
> 
> Running presubmit commit checks ...
> 
> ** Presubmit Messages **
> If this change has an associated bug, add BUG=[bug number].
> 
> ** Presubmit ERRORS **
> Run the command: svn pset svn:eol-style LF \
>   chrome/test/data/bookmarks/model_without_sync.json
> 
> Presubmit checks took 1.2s to calculate.

I fixed the commit bot and enqueued your job again.

[commit-bot: I haz the power, 2011-05-26 17:28:13]

Presubmit check for 7012005-37001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Run the command: svn pset svn:eol-style LF \
  chrome/test/data/bookmarks/model_without_sync.json

Presubmit checks took 1.4s to calculate.

[M-A Ruel, 2011-05-26 17:29:16]

Ugh, updating the right config file this time.

On 2011/05/26 17:28:13, commit-bot wrote:
> Presubmit check for 7012005-37001 failed and returned exit status 1.
> 
> Running presubmit commit checks ...
> 
> ** Presubmit Messages **
> If this change has an associated bug, add BUG=[bug number].
> 
> ** Presubmit ERRORS **
> Run the command: svn pset svn:eol-style LF \
>   chrome/test/data/bookmarks/model_without_sync.json
> 
> Presubmit checks took 1.4s to calculate.

[commit-bot: I haz the power, 2011-05-26 17:36:28]

Try job failure for 7012005-37001 on win:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2011-05-26 17:57:41]

Try job failure for 7012005-37001 on win:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[M-A Ruel, 2011-05-26 18:13:34]

On 2011/05/26 17:57:41, commit-bot wrote:
> Try job failure for 7012005-37001 on win:
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

http://codereview.chromium.org/7077006/ was killing the slaves hard. I fixed the
slaves I found and enqueued this change again...

[commit-bot: I haz the power, 2011-05-26 18:21:24]

Try job failure for 7012005-37001 on win:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2011-05-26 22:07:25]

Change committed as 86902