Changes the visibility of the 'mobile' node based on whether

Changes the visibility of the 'mobile' node based on whether there is a node on the sync side.

BUG=102714
TEST=none

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

[sky, 2011-12-02 23:48:46]

This is relative to http://codereview.chromium.org/8772064/

[akalin, 2011-12-03 00:17:05]

http://codereview.chromium.org/8786006/diff/1/chrome/browser/sync/glue/bookma...
File chrome/browser/sync/glue/bookmark_model_associator.cc (right):

http://codereview.chromium.org/8786006/diff/1/chrome/browser/sync/glue/bookma...
chrome/browser/sync/glue/bookmark_model_associator.cc:191: if
(!bookmark_model_->IsLoaded())
hmm does the visibility get updated eventually when the model gets loaded?

http://codereview.chromium.org/8786006/diff/1/chrome/browser/sync/glue/bookma...
chrome/browser/sync/glue/bookmark_model_associator.cc:251:
dirty_associations_sync_ids_.erase(sync_id);
should we make the mobile folder invisible here?  not sure exactly how it works

[sky, 2011-12-03 00:23:06]

http://codereview.chromium.org/8786006/diff/1/chrome/browser/sync/glue/bookma...
File chrome/browser/sync/glue/bookmark_model_associator.cc (right):

http://codereview.chromium.org/8786006/diff/1/chrome/browser/sync/glue/bookma...
chrome/browser/sync/glue/bookmark_model_associator.cc:191: if
(!bookmark_model_->IsLoaded())
On 2011/12/03 00:17:05, akalin wrote:
> hmm does the visibility get updated eventually when the model gets loaded?

I would assume Associate is run at that point. But I don't know for sure.

http://codereview.chromium.org/8786006/diff/1/chrome/browser/sync/glue/bookma...
chrome/browser/sync/glue/bookmark_model_associator.cc:251:
dirty_associations_sync_ids_.erase(sync_id);
On 2011/12/03 00:17:05, akalin wrote:
> should we make the mobile folder invisible here?  not sure exactly how it
works

I'm not sure either.

[sky, 2011-12-03 00:25:32]

Tim, if you could review this one that would be great.

Thanks,

  -Scott

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

Adding Yaron the review list.

  -Scott

[Yaron, 2011-12-05 18:40:12]

lgtm

http://codereview.chromium.org/8786006/diff/1/chrome/browser/sync/glue/bookma...
File chrome/browser/sync/glue/bookmark_model_associator.cc (right):

http://codereview.chromium.org/8786006/diff/1/chrome/browser/sync/glue/bookma...
chrome/browser/sync/glue/bookmark_model_associator.cc:191: if
(!bookmark_model_->IsLoaded())
On 2011/12/03 00:23:07, sky wrote:
> On 2011/12/03 00:17:05, akalin wrote:
> > hmm does the visibility get updated eventually when the model gets loaded?
> 
> I would assume Associate is run at that point. But I don't know for sure.

Hmm. It looks like BookmarkChangeProcessor::Loaded has a NOTREACHED() which I
guess implies that we shouldn't have bookmark sync without having previously
loaded the bookmark model?

I also see a few DCHECK(bookmark_model_->IsLoaded()) in here.

http://codereview.chromium.org/8786006/diff/1/chrome/browser/sync/glue/bookma...
chrome/browser/sync/glue/bookmark_model_associator.cc:251:
dirty_associations_sync_ids_.erase(sync_id);
On 2011/12/03 00:23:07, sky wrote:
> On 2011/12/03 00:17:05, akalin wrote:
> > should we make the mobile folder invisible here?  not sure exactly how it
> works
> 
> I'm not sure either.

Nope. We should leave it the way it was.

[sky, 2011-12-05 23:35:29]

Diffs between last patch:
Added kCreateMobileBookmarksFolder that indicates whether we should force
creation of mobile folder.
Changed BookmarkModelAssociator::UpdateMobileNodeVisibility to use id_map_
directly. Calling GetSyncIdForTaggedNode was causing test failures (write lock
was being grabbed while lock already held).
There's a temporary workaround for a bug on trunk that causes all sync tests to
crash. I won't land this, but I need it to make sure my try runs succeed.
Changed BookmarkModelAssociator to DCHECK model is loaded.

http://codereview.chromium.org/8786006/diff/10001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/8786006/diff/10001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:213: 0,
This is a temporary workaround so sync tests work. I won't land it. jwd is in
the process of reverting, at which point I can remove this and try again.

[Yaron, 2011-12-06 00:26:46]

http://codereview.chromium.org/8786006/diff/10001/net/tools/testserver/chromi...
File net/tools/testserver/chromiumsync.py (left):

http://codereview.chromium.org/8786006/diff/10001/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync.py:397: PermanentItem('synced_bookmarks',
name='Mobile Bookmarks',
Hmm. Why is this changed?

[sky, 2011-12-06 00:33:13]

http://codereview.chromium.org/8786006/diff/10001/net/tools/testserver/chromi...
File net/tools/testserver/chromiumsync.py (left):

http://codereview.chromium.org/8786006/diff/10001/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync.py:397: PermanentItem('synced_bookmarks',
name='Mobile Bookmarks',
On 2011/12/06 00:26:46, Yaron wrote:
> Hmm. Why is this changed?

I added it in http://codereview.chromium.org/8759017/ . It was necessary when we
required the mobile folder to exist. Now that that isn't the case, this
shouldn't be required. At least that's my take on it. It could be I'm wrong. Let
me know.

[tim (not reviewing), 2011-12-06 01:00:57]

The sync functionality LGTM, thanks.

http://codereview.chromium.org/8786006/diff/10001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_bookmark_unittest.cc (right):

http://codereview.chromium.org/8786006/diff/10001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_bookmark_unittest.cc:300:
switches::kCreateMobileBookmarksFolder);
Hmm.. so we don't cover the not-enabled cases now?

http://codereview.chromium.org/8786006/diff/10001/net/tools/testserver/chromi...
File net/tools/testserver/chromiumsync.py (left):

http://codereview.chromium.org/8786006/diff/10001/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync.py:397: PermanentItem('synced_bookmarks',
name='Mobile Bookmarks',
On 2011/12/06 00:33:13, sky wrote:
> On 2011/12/06 00:26:46, Yaron wrote:
> > Hmm. Why is this changed?
> 
> I added it in http://codereview.chromium.org/8759017/ . It was necessary when
we
> required the mobile folder to exist. Now that that isn't the case, this
> shouldn't be required. At least that's my take on it. It could be I'm wrong.
Let
> me know.

This would be necessary if any of our integration tests covered mobile bookmarks
cases.  I'm not sure that we have any that do?

[sky, 2011-12-06 01:02:13]

http://codereview.chromium.org/8786006/diff/10001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_bookmark_unittest.cc (right):

http://codereview.chromium.org/8786006/diff/10001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_bookmark_unittest.cc:300:
switches::kCreateMobileBookmarksFolder);
On 2011/12/06 01:00:57, timsteele wrote:
> Hmm.. so we don't cover the not-enabled cases now?

That's right, but that was the case before these set of changes too.

[Yaron, 2011-12-06 01:31:54]

On 2011/12/06 01:02:13, sky wrote:
>
http://codereview.chromium.org/8786006/diff/10001/chrome/browser/sync/profile...
> File chrome/browser/sync/profile_sync_service_bookmark_unittest.cc (right):
> 
>
http://codereview.chromium.org/8786006/diff/10001/chrome/browser/sync/profile...
> chrome/browser/sync/profile_sync_service_bookmark_unittest.cc:300:
> switches::kCreateMobileBookmarksFolder);
> On 2011/12/06 01:00:57, timsteele wrote:
> > Hmm.. so we don't cover the not-enabled cases now?
> 
> That's right, but that was the case before these set of changes too.

lgtm

[akalin, 2011-12-06 02:53:40]

I think tim+yaron is enough for this CL; removed myself as reviewer.

On 2011/12/06 01:31:54, Yaron wrote:
> On 2011/12/06 01:02:13, sky wrote:
> >
>
http://codereview.chromium.org/8786006/diff/10001/chrome/browser/sync/profile...
> > File chrome/browser/sync/profile_sync_service_bookmark_unittest.cc (right):
> > 
> >
>
http://codereview.chromium.org/8786006/diff/10001/chrome/browser/sync/profile...
> > chrome/browser/sync/profile_sync_service_bookmark_unittest.cc:300:
> > switches::kCreateMobileBookmarksFolder);
> > On 2011/12/06 01:00:57, timsteele wrote:
> > > Hmm.. so we don't cover the not-enabled cases now?
> > 
> > That's right, but that was the case before these set of changes too.
> 
> lgtm

[akalin, 2011-12-06 03:00:23]

adding myself as reviewer for net/tools stuff

net/tools stuff lgtm

On 2011/12/06 02:53:40, akalin wrote:
> I think tim+yaron is enough for this CL; removed myself as reviewer.
> 
> On 2011/12/06 01:31:54, Yaron wrote:
> > On 2011/12/06 01:02:13, sky wrote:
> > >
> >
>
http://codereview.chromium.org/8786006/diff/10001/chrome/browser/sync/profile...
> > > File chrome/browser/sync/profile_sync_service_bookmark_unittest.cc
(right):
> > > 
> > >
> >
>
http://codereview.chromium.org/8786006/diff/10001/chrome/browser/sync/profile...
> > > chrome/browser/sync/profile_sync_service_bookmark_unittest.cc:300:
> > > switches::kCreateMobileBookmarksFolder);
> > > On 2011/12/06 01:00:57, timsteele wrote:
> > > > Hmm.. so we don't cover the not-enabled cases now?
> > > 
> > > That's right, but that was the case before these set of changes too.
> > 
> > lgtm

[akalin, 2011-12-06 03:03:47]

http://codereview.chromium.org/8786006/diff/10001/net/tools/testserver/chromi...
File net/tools/testserver/chromiumsync.py (left):

http://codereview.chromium.org/8786006/diff/10001/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync.py:397: PermanentItem('synced_bookmarks',
name='Mobile Bookmarks',
On 2011/12/06 01:00:57, timsteele wrote:
> On 2011/12/06 00:33:13, sky wrote:
> > On 2011/12/06 00:26:46, Yaron wrote:
> > > Hmm. Why is this changed?
> > 
> > I added it in http://codereview.chromium.org/8759017/ . It was necessary
when
> we
> > required the mobile folder to exist. Now that that isn't the case, this
> > shouldn't be required. At least that's my take on it. It could be I'm wrong.
> Let
> > me know.
> 
> This would be necessary if any of our integration tests covered mobile
bookmarks
> cases.  I'm not sure that we have any that do?

Yeah, I guess we don't have integration tests that need this, so I'm okay with
omitting it.