[Sync] Remove --create-mobile-bookmarks-folder

[Sync] Remove --create-mobile-bookmarks-folder

Instead, check defined(OS_ANDROID) and add {create,expect}_mobile_bookmarks_folder switches.

BUG=113723
TEST=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=124065

[akalin, 2012-02-24 01:49:02]

+zea for review

+yaron, +qsr FYI

[akalin, 2012-02-27 19:47:15]

ping!

On 2012/02/24 01:49:02, akalin wrote:
> +zea for review
> 
> +yaron, +qsr FYI

[Nicolas Zea, 2012-02-27 20:03:12]

http://codereview.chromium.org/9453036/diff/1001/chrome/browser/sync/engine/d...
File chrome/browser/sync/engine/download_updates_command.cc (right):

http://codereview.chromium.org/9453036/diff/1001/chrome/browser/sync/engine/d...
chrome/browser/sync/engine/download_updates_command.cc:44:
get_updates->set_include_syncable_bookmarks(include_syncable_bookmarks_);
So is this always included in the getupdates if it already exists? Since
presumably if we've synced some mobile bookmarks we want all clients to download
them.

http://codereview.chromium.org/9453036/diff/1001/chrome/browser/sync/engine/d...
File chrome/browser/sync/engine/download_updates_command.h (right):

http://codereview.chromium.org/9453036/diff/1001/chrome/browser/sync/engine/d...
chrome/browser/sync/engine/download_updates_command.h:43: explicit
DownloadUpdatesCommand(bool include_syncable_bookmarks);
Comment about the parameter.

http://codereview.chromium.org/9453036/diff/1001/chrome/browser/sync/glue/boo...
File chrome/browser/sync/glue/bookmark_model_associator.cc (right):

http://codereview.chromium.org/9453036/diff/1001/chrome/browser/sync/glue/boo...
chrome/browser/sync/glue/bookmark_model_associator.cc:4: 
remove extra newline

http://codereview.chromium.org/9453036/diff/1001/chrome/browser/sync/glue/boo...
File chrome/browser/sync/glue/bookmark_model_associator.h (right):

http://codereview.chromium.org/9453036/diff/1001/chrome/browser/sync/glue/boo...
chrome/browser/sync/glue/bookmark_model_associator.h:43: bool
create_mobile_bookmarks_folder);
Comment about this param

[akalin, 2012-02-27 22:04:18]

PTAL

http://codereview.chromium.org/9453036/diff/1001/chrome/browser/sync/engine/d...
File chrome/browser/sync/engine/download_updates_command.cc (right):

http://codereview.chromium.org/9453036/diff/1001/chrome/browser/sync/engine/d...
chrome/browser/sync/engine/download_updates_command.cc:44:
get_updates->set_include_syncable_bookmarks(include_syncable_bookmarks_);
On 2012/02/27 20:03:12, nzea wrote:
> So is this always included in the getupdates if it already exists? Since
> presumably if we've synced some mobile bookmarks we want all clients to
download
> them.

I don't think so.  This was the behavior before but for some reason it was
reverted.  sky or yaron may know more about it.

http://codereview.chromium.org/9453036/diff/1001/chrome/browser/sync/engine/d...
File chrome/browser/sync/engine/download_updates_command.h (right):

http://codereview.chromium.org/9453036/diff/1001/chrome/browser/sync/engine/d...
chrome/browser/sync/engine/download_updates_command.h:43: explicit
DownloadUpdatesCommand(bool include_syncable_bookmarks);
On 2012/02/27 20:03:12, nzea wrote:
> Comment about the parameter.

Done.

http://codereview.chromium.org/9453036/diff/1001/chrome/browser/sync/glue/boo...
File chrome/browser/sync/glue/bookmark_model_associator.cc (right):

http://codereview.chromium.org/9453036/diff/1001/chrome/browser/sync/glue/boo...
chrome/browser/sync/glue/bookmark_model_associator.cc:4: 
On 2012/02/27 20:03:12, nzea wrote:
> remove extra newline

Done.

http://codereview.chromium.org/9453036/diff/1001/chrome/browser/sync/glue/boo...
File chrome/browser/sync/glue/bookmark_model_associator.h (right):

http://codereview.chromium.org/9453036/diff/1001/chrome/browser/sync/glue/boo...
chrome/browser/sync/glue/bookmark_model_associator.h:43: bool
create_mobile_bookmarks_folder);
On 2012/02/27 20:03:12, nzea wrote:
> Comment about this param

Done.

[Nicolas Zea, 2012-02-27 22:10:38]

LGTM, although may be good to get Yaron/sky/someone to confirm this is safe to
do.

[akalin, 2012-02-27 22:14:42]

+yaron, wanna take a look?

[Yaron, 2012-02-27 22:15:01]

http://codereview.chromium.org/9453036/diff/1001/chrome/browser/sync/engine/d...
File chrome/browser/sync/engine/download_updates_command.cc (right):

http://codereview.chromium.org/9453036/diff/1001/chrome/browser/sync/engine/d...
chrome/browser/sync/engine/download_updates_command.cc:44:
get_updates->set_include_syncable_bookmarks(include_syncable_bookmarks_);
On 2012/02/27 22:04:18, akalin wrote:
> On 2012/02/27 20:03:12, nzea wrote:
> > So is this always included in the getupdates if it already exists? Since
> > presumably if we've synced some mobile bookmarks we want all clients to
> download
> > them.
> 
> I don't think so.  This was the behavior before but for some reason it was
> reverted.  sky or yaron may know more about it.

It's a misnomer. This flag tells the sync server to _create_ the mobile
permanent folder if it's not already created. If it's already been created then
anybody will retrieve it.

[Yaron, 2012-02-27 22:17:01]

http://codereview.chromium.org/9453036/diff/10002/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer.cc (right):

http://codereview.chromium.org/9453036/diff/10002/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer.cc:148: const bool kIncludeSyncableBookmarks =
true;
You could start renaming this to kCreateMobileBookmarksFolder and propagating
that throughout. I assume it's also safe to rename the proto-field name, right?
(i.e. there's a unique id and the actual name isn't important)

[akalin, 2012-02-27 22:29:44]

Renamed proto field (should be safe, although server-side may need changes,
which i'll do after this gets checked in).

PTAL.

http://codereview.chromium.org/9453036/diff/10002/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer.cc (right):

http://codereview.chromium.org/9453036/diff/10002/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer.cc:148: const bool kIncludeSyncableBookmarks =
true;
On 2012/02/27 22:17:01, Yaron wrote:
> You could start renaming this to kCreateMobileBookmarksFolder and propagating
> that throughout. I assume it's also safe to rename the proto-field name,
right?
> (i.e. there's a unique id and the actual name isn't important)

Done.

[Nicolas Zea, 2012-02-27 22:35:22]

http://codereview.chromium.org/9453036/diff/10007/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/download_updates_command.h (right):

http://codereview.chromium.org/9453036/diff/10007/chrome/browser/sync/engine/...
chrome/browser/sync/engine/download_updates_command.h:44: // create the mobile
bookmarks folder if it's not already created.
Since the folder is actually called "synced bookmarks", how about renaming to
that and having the description be something like
"controls whether or not to tell the server to create the synced bookmarks
permanent item if it's not already created".

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

http://codereview.chromium.org/9453036/diff/10007/chrome/browser/sync/glue/bo...
chrome/browser/sync/glue/bookmark_model_associator.cc:190: bool
create_mobile_bookmarks_folder)
rename to expect_synced_bookmarks_folder (since we're not creating it here)

[akalin, 2012-02-27 22:56:44]

PTAL

http://codereview.chromium.org/9453036/diff/10007/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/download_updates_command.h (right):

http://codereview.chromium.org/9453036/diff/10007/chrome/browser/sync/engine/...
chrome/browser/sync/engine/download_updates_command.h:44: // create the mobile
bookmarks folder if it's not already created.
On 2012/02/27 22:35:23, nzea wrote:
> Since the folder is actually called "synced bookmarks", how about renaming to
> that and having the description be something like
> "controls whether or not to tell the server to create the synced bookmarks
> permanent item if it's not already created". 

actually, the folder is called "Mobile Bookmarks" (I checked on my phone).  The
proto had 'synced bookmarks' only because it was checked in before Chrome on
Android was public.

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

http://codereview.chromium.org/9453036/diff/10007/chrome/browser/sync/glue/bo...
chrome/browser/sync/glue/bookmark_model_associator.cc:190: bool
create_mobile_bookmarks_folder)
On 2012/02/27 22:35:23, nzea wrote:
> rename to expect_synced_bookmarks_folder (since we're not creating it here)

Done.

[Nicolas Zea, 2012-02-27 23:25:31]

LGTM

http://codereview.chromium.org/9453036/diff/12004/chrome/browser/sync/glue/bo...
File chrome/browser/sync/glue/bookmark_model_associator.h (right):

http://codereview.chromium.org/9453036/diff/12004/chrome/browser/sync/glue/bo...
chrome/browser/sync/glue/bookmark_model_associator.h:40: // expect the mobile
bookmarks top-level folder to be created.
top-level folder -> permanent item (not really top level)

[akalin, 2012-02-28 00:10:48]

committing soonish

http://codereview.chromium.org/9453036/diff/12004/chrome/browser/sync/glue/bo...
File chrome/browser/sync/glue/bookmark_model_associator.h (right):

http://codereview.chromium.org/9453036/diff/12004/chrome/browser/sync/glue/bo...
chrome/browser/sync/glue/bookmark_model_associator.h:40: // expect the mobile
bookmarks top-level folder to be created.
On 2012/02/27 23:25:31, nzea wrote:
> top-level folder -> permanent item (not really top level)

Done.

[commit-bot: I haz the power, 2012-02-28 00:11:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/9453036/14001

[Yaron, 2012-02-28 00:30:51]

lgtm

[akalin, 2012-02-28 00:32:24]

On 2012/02/28 00:30:51, Yaron wrote:
> lgtm

sorry, didn't mean to not wait for you. :/ committing soonish starting from now!

[commit-bot: I haz the power, 2012-02-28 05:52:29]

Can't apply patch for file
chrome/browser/sync/engine/download_updates_command.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/sync/engine/download_updates_command.cc
Hunk #1 FAILED at 12.
Hunk #2 succeeded at 25 (offset -2 lines).
Hunk #3 succeeded at 40 with fuzz 2 (offset -2 lines).
1 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/sync/engine/download_updates_command.cc.rej

[commit-bot: I haz the power, 2012-02-28 21:19:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/9453036/16001

[commit-bot: I haz the power, 2012-02-28 22:31:49]

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

[commit-bot: I haz the power, 2012-02-28 22:45:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/9453036/16001