Changes BookmarkModelAssociator not to fail if mobile folder doesn't

Changes BookmarkModelAssociator not to fail if mobile folder doesn't
exist. This is a regression from
http://codereview.chromium.org/8759017

BUG=106238
TEST=none
R=akalin@chromium.org,yfriedman@chromium.org


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

[sky, 2011-12-03 00:07:39]

Let me know if this isn't the right thing to do here.

[akalin, 2011-12-03 00:12:20]

LGTM

I think this is fine to fix the regression, but we'll have to handle properly
the case of the mobile node being created while a desktop browser is open. 
Yaron?

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

http://codereview.chromium.org/8790008/diff/1/chrome/browser/sync/glue/bookma...
chrome/browser/sync/glue/bookmark_model_associator.cc:373:
AssociateTaggedPermanentNode(bookmark_model_->mobile_node(),
#include compiler specific and use ignore_result

http://codereview.chromium.org/8790008/diff/1/chrome/browser/sync/glue/bookma...
chrome/browser/sync/glue/bookmark_model_associator.cc:526:
GetSyncIdForTaggedNode(kMobileBookmarksTag, &mobile_bookmarks_id);
here, too

[sky, 2011-12-03 00:20:55]

Added ignore_result and WARN_UNUSED

[sky, 2011-12-03 00:24:08]

Yaron doesn't seem to be around. I'm going to schedule this for commit to fix
the regression. Yaron, if you don't think this is right let me know and I'll do
a followup patch.

[commit-bot: I haz the power, 2011-12-03 00:24:31]

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

[akalin, 2011-12-03 00:25:30]

LGTM

[Yaron, 2011-12-03 01:32:02]

On 2011/12/03 00:25:30, akalin wrote:
> LGTM

LGTM

From http://codereview.chromium.org/8759017, it looks like we'll always create
the mobile folder (because of
http://codereview.chromium.org/8759017/diff/6001/chrome/browser/sync/engine/d...).
I'm not sure that's desirable but this is an improvement, regardless.

[commit-bot: I haz the power, 2011-12-03 02:44:00]

Change committed as 112851

[sky, 2011-12-05 05:33:23]

On 2011/12/03 01:32:02, Yaron wrote:
> On 2011/12/03 00:25:30, akalin wrote:
> > LGTM
> 
> LGTM
> 
> From http://codereview.chromium.org/8759017, it looks like we'll always create
> the mobile folder (because of
>
http://codereview.chromium.org/8759017/diff/6001/chrome/browser/sync/engine/d...).
> I'm not sure that's desirable but this is an improvement, regardless.

I don't know enough about the backend side. Is there some way to change things
so that we don't trigger creation?

  -Scott

[Yaron, 2011-12-05 18:49:23]

On 2011/12/05 05:33:23, sky wrote:
> On 2011/12/03 01:32:02, Yaron wrote:
> > On 2011/12/03 00:25:30, akalin wrote:
> > > LGTM
> > 
> > LGTM
> > 
> > From http://codereview.chromium.org/8759017, it looks like we'll always
create
> > the mobile folder (because of
> >
>
http://codereview.chromium.org/8759017/diff/6001/chrome/browser/sync/engine/d...).
> > I'm not sure that's desirable but this is an improvement, regardless.
> 
> I don't know enough about the backend side. Is there some way to change things
> so that we don't trigger creation?
> 
>   -Scott

That's what get_updates->set_include_syncable_bookmarks(true) is supposed to
control. It tells the server that it should create the permanent item if it
doesn't already exist. That's why we had it flag-guarded off. I think having the
kEnableSyncedBookmarksFolder switch control only that proto field might give the
desired effect

[sky, 2011-12-05 22:01:22]

On Mon, Dec 5, 2011 at 10:49 AM,  <yfriedman@chromium.org> wrote:
> On 2011/12/05 05:33:23, sky wrote:
>>
>> On 2011/12/03 01:32:02, Yaron wrote:
>> > On 2011/12/03 00:25:30, akalin wrote:
>> > > LGTM
>> >
>> > LGTM
>> >
>> > From http://codereview.chromium.org/8759017, it looks like we'll always
>
> create
>>
>> > the mobile folder (because of
>> >
>
>
>
http://codereview.chromium.org/8759017/diff/6001/chrome/browser/sync/engine/d...).
>>
>> > I'm not sure that's desirable but this is an improvement, regardless.
>
>
>> I don't know enough about the backend side. Is there some way to change
>> things
>> so that we don't trigger creation?
>
>
>>   -Scott
>
>
> That's what get_updates->set_include_syncable_bookmarks(true) is supposed to
> control. It tells the server that it should create the permanent item if it
> doesn't already exist. That's why we had it flag-guarded off. I think having
> the
> kEnableSyncedBookmarksFolder switch control only that proto field might give
> the
> desired effect

I'll put it behind a flag and rename it kCreateMobileBookmarksFolder.
The only place that will check it is in this code. I'll merge it with
my other cl. SG?

  -Scott

[yfriedman, 2011-12-05 22:42:17]

sgtm

On Mon, Dec 5, 2011 at 2:00 PM, Scott Violet <sky@chromium.org> wrote:

> On Mon, Dec 5, 2011 at 10:49 AM,  <yfriedman@chromium.org> wrote:
> > On 2011/12/05 05:33:23, sky wrote:
> >>
> >> On 2011/12/03 01:32:02, Yaron wrote:
> >> > On 2011/12/03 00:25:30, akalin wrote:
> >> > > LGTM
> >> >
> >> > LGTM
> >> >
> >> > From http://codereview.chromium.org/8759017, it looks like we'll
> always
> >
> > create
> >>
> >> > the mobile folder (because of
> >> >
> >
> >
> >
>
http://codereview.chromium.org/8759017/diff/6001/chrome/browser/sync/engine/d...
> ).
> >>
> >> > I'm not sure that's desirable but this is an improvement, regardless.
> >
> >
> >> I don't know enough about the backend side. Is there some way to change
> >> things
> >> so that we don't trigger creation?
> >
> >
> >>   -Scott
> >
> >
> > That's what get_updates->set_include_syncable_bookmarks(true) is
> supposed to
> > control. It tells the server that it should create the permanent item if
> it
> > doesn't already exist. That's why we had it flag-guarded off. I think
> having
> > the
> > kEnableSyncedBookmarksFolder switch control only that proto field might
> give
> > the
> > desired effect
>
> I'll put it behind a flag and rename it kCreateMobileBookmarksFolder.
> The only place that will check it is in this code. I'll merge it with
> my other cl. SG?
>
>  -Scott
>