Refactor syncapi.h

Fragment syncapi.h into sync/internal_api/*

This commit splits syncapi.cc and syncapi.h into many files.  Most of these files have been moved to the newly created chrome/browser/sync/internal_api.

Each of the following classes now have their own .cc and .h files:
    - BaseNode
    - ReadNode
    - WriteNode
    - BaseTransaction
    - ReadTransaction
    - WriteTransaction
    - UserShare
    - SyncManager

Functions formerly declared at file-scope in syncapi.cc and shared among several classes are now declared in engine/syncapi_internal.h.  We intend to use DEPS rules to prevent these functions from being included in non-syncapi classes.

Test classes closely related to syncapi.h classes have been moved from engine to internal_api.

This change necessarily touches the #include lists for lots of files and some of the sources lists in .gyp.

This change should have no effect on program behaviour.

BUG=19878
TEST=

[rlarocque, 2011-08-15 22:16:46]

Here's part 1 of the syncapi refactor.  I'm sending out this review for early
feedback only.  The code isn't ready to be committed as is.  

See the description for a list of TODO items.

[tim (not reviewing), 2011-08-15 22:53:55]

#happydance

Still looking. Are you planning on making the dir changes as part of the same
CL, or a follow-up?

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/read...
File chrome/browser/sync/engine/read_transaction.h (right):

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/read...
chrome/browser/sync/engine/read_transaction.h:12: }  // namespace
tracked_objects
Well, you can remove this since it is inline below...

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/read...
chrome/browser/sync/engine/read_transaction.h:19: // parameter.
Think you can remove this todo.

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/read...
chrome/browser/sync/engine/read_transaction.h:46: } // namespace sync_api
Perhaps we should change this to be 
browser_sync::internal_api once it lives in the internal_api folder?

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/sync...
File chrome/browser/sync/engine/sync_manager.h (right):

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/sync_manager.h:23: }
// namespace base

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/sync_manager.h:32: }
}  // namespace sessions
}  // namespace browser_sync

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/sync_manager.h:38: 
// namespace sync_notifier

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/sync_manager.h:41: }
namespace sync_pb

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/sync...
File chrome/browser/sync/engine/syncapi.h (right):

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncapi.h:5: // This file defines the "sync API", an
interface to the syncer
we could throw this comment into a readme in /internal_api

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncapi.h:42: // FIXME: This include list is a
temporary hack to ensure a smooth
s/FIXME.../"TODO(rlarocque): Bug 19878. This include list..."

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/sync...
File chrome/browser/sync/engine/syncapi_functions.cc (right):

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncapi_functions.cc:17: static const
FilePath::CharType kBookmarkSyncUserSettingsDatabase[] =
is this actually used anywhere?

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncapi_functions.cc:19: 
nit - extra newline

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncapi_functions.cc:56: 
nit - extra newline

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/sync...
File chrome/browser/sync/engine/syncapi_functions.h (right):

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncapi_functions.h:18: 
nit - extra newline

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/sync...
File chrome/browser/sync/engine/syncapi_internal.h (right):

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncapi_internal.h:9: // the internal sync_api.  They
are not to be used by clients of the API.
Hmm. Ideally these would stay under /engine/ then, so we can enforce proper DEPS
rules.  Although "internal" is about to get overloaded :\

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/writ...
File chrome/browser/sync/engine/write_transaction.h (right):

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/writ...
chrome/browser/sync/engine/write_transaction.h:14: }
end namespace decls with // namespace foo

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/writ...
chrome/browser/sync/engine/write_transaction.h:17: class Location;
same ns comment as before

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/writ...
chrome/browser/sync/engine/write_transaction.h:22: // TODO(akalin): Make
ReadTransaction/WriteTransaction take a Location
remove todo

http://codereview.chromium.org/7633077/diff/1/chrome/chrome.gyp
File chrome/chrome.gyp (right):

http://codereview.chromium.org/7633077/diff/1/chrome/chrome.gyp#newcode519
chrome/chrome.gyp:519: 'browser/sync/engine/write_node.cc',
nit- although it wasn't true before, this should be alphabetized

[rlarocque, 2011-08-16 18:12:46]

The next code dump should respond to these code changes, among other things.

I've got a branch in my git tree organized as follows:
1. the changes in this review
2. the changes that remove the syncapi.h hack and move a bunch of files
3. the changes made in response to this review

I'm guessing this code review tool will show the whole branch as one squashed
change.  If you want to see any individual commit, ping me and I'll e-mail you a
diff.

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/read...
File chrome/browser/sync/engine/read_transaction.h (right):

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/read...
chrome/browser/sync/engine/read_transaction.h:12: }  // namespace
tracked_objects
Remove the forward declaration?  I don't see how that would work.

On 2011/08/15 22:53:55, timsteele wrote:
> Well, you can remove this since it is inline below...

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/read...
chrome/browser/sync/engine/read_transaction.h:19: // parameter.
On 2011/08/15 22:53:55, timsteele wrote:
> Think you can remove this todo.

Done.

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/read...
chrome/browser/sync/engine/read_transaction.h:46: } // namespace sync_api
I was thinking about that, but then I figured this merge will be hard enough as
is.  Maybe we can open another ticket for it.  

On 2011/08/15 22:53:55, timsteele wrote:
> Perhaps we should change this to be 
> browser_sync::internal_api once it lives in the internal_api folder?

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/sync...
File chrome/browser/sync/engine/sync_manager.h (right):

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/sync_manager.h:23: }
On 2011/08/15 22:53:55, timsteele wrote:
> // namespace base

Done.

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/sync_manager.h:32: }
On 2011/08/15 22:53:55, timsteele wrote:
> }  // namespace sessions
> }  // namespace browser_sync

Done.

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/sync_manager.h:38: 
On 2011/08/15 22:53:55, timsteele wrote:
> // namespace sync_notifier

Done.

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/sync_manager.h:41: }
On 2011/08/15 22:53:55, timsteele wrote:
> namespace sync_pb

Done.

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/sync...
File chrome/browser/sync/engine/syncapi.h (right):

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncapi.h:42: // FIXME: This include list is a
temporary hack to ensure a smooth
Done, sort of.  This file will be deleted in the final version of this diff.

I'll keep that in mind for the next time I write a FIXME, though.  

On 2011/08/15 22:53:55, timsteele wrote:
> s/FIXME.../"TODO(rlarocque): Bug 19878. This include list..."

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/sync...
File chrome/browser/sync/engine/syncapi_functions.cc (right):

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncapi_functions.cc:17: static const
FilePath::CharType kBookmarkSyncUserSettingsDatabase[] =
Apparently not.  I'm surprised the compiler didn't warn about this.  I think
it's supposed to when -Wunused-variables is turned on.  

I've removed the line.  

On 2011/08/15 22:53:55, timsteele wrote:
> is this actually used anywhere?

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncapi_functions.cc:19: 
On 2011/08/15 22:53:55, timsteele wrote:
> nit - extra newline

Done.

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncapi_functions.cc:56: 
On 2011/08/15 22:53:55, timsteele wrote:
> nit - extra newline

Done.

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/sync...
File chrome/browser/sync/engine/syncapi_internal.h (right):

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncapi_internal.h:9: // the internal sync_api.  They
are not to be used by clients of the API.
On 2011/08/15 22:53:55, timsteele wrote:
> Hmm. Ideally these would stay under /engine/ then, so we can enforce proper
DEPS
> rules.  Although "internal" is about to get overloaded :\

Done.

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/writ...
File chrome/browser/sync/engine/write_transaction.h (right):

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/writ...
chrome/browser/sync/engine/write_transaction.h:14: }
On 2011/08/15 22:53:55, timsteele wrote:
> end namespace decls with // namespace foo

Done.

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/writ...
chrome/browser/sync/engine/write_transaction.h:17: class Location;
Still don't understand it.

On 2011/08/15 22:53:55, timsteele wrote:
> same ns comment as before

http://codereview.chromium.org/7633077/diff/1/chrome/browser/sync/engine/writ...
chrome/browser/sync/engine/write_transaction.h:22: // TODO(akalin): Make
ReadTransaction/WriteTransaction take a Location
On 2011/08/15 22:53:55, timsteele wrote:
> remove todo

Done.

http://codereview.chromium.org/7633077/diff/1/chrome/chrome.gyp
File chrome/chrome.gyp (right):

http://codereview.chromium.org/7633077/diff/1/chrome/chrome.gyp#newcode519
chrome/chrome.gyp:519: 'browser/sync/engine/write_node.cc',
On 2011/08/15 22:53:55, timsteele wrote:
> nit- although it wasn't true before, this should be alphabetized

Done.

[rlarocque, 2011-08-16 18:28:14]

Here is the second patch set.  It includes responses to Tim's review comments,
and the file move operations promised in the original description of this
review.  

This is the final version of the patch.  With your approval, I intend to squash
this into a single mega-commit, rebase, then have it committed.

[Nicolas Zea, 2011-08-16 19:39:07]

Some comments. Also, I wonder if the README is really necessary? Tim?

http://codereview.chromium.org/7633077/diff/7001/chrome/browser/sync/engine/n...
File chrome/browser/sync/engine/net/server_connection_manager.cc (left):

http://codereview.chromium.org/7633077/diff/7001/chrome/browser/sync/engine/n...
chrome/browser/sync/engine/net/server_connection_manager.cc:16: #include
"chrome/browser/sync/engine/syncapi.h"
Excellent (here and elsewhere).

http://codereview.chromium.org/7633077/diff/7001/chrome/browser/sync/engine/n...
File chrome/browser/sync/engine/nigori_util.cc (right):

http://codereview.chromium.org/7633077/diff/7001/chrome/browser/sync/engine/n...
chrome/browser/sync/engine/nigori_util.cc:13: #include
"chrome/browser/sync/internal_api/write_transaction.h"
Don't think this writetransaction is necessary (all the transactions here are
syncable::writetransactions)

http://codereview.chromium.org/7633077/diff/7001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncapi_internal.cc (right):

http://codereview.chromium.org/7633077/diff/7001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncapi_internal.cc:15: // global helper function:
FIXME move this
FIXME...->TODO(rlarocque): move this

http://codereview.chromium.org/7633077/diff/7001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncapi_internal.cc:16:
sync_pb::PasswordSpecificsData* DecryptPasswordSpecifics(
I think all three of these methods can actually be part of basenode.

http://codereview.chromium.org/7633077/diff/7001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncapi_internal.cc:50: const
sync_pb::EntitySpecifics& left,
fix indent

http://codereview.chromium.org/7633077/diff/7001/chrome/browser/sync/glue/aut...
File chrome/browser/sync/glue/autofill_change_processor.h (right):

http://codereview.chromium.org/7633077/diff/7001/chrome/browser/sync/glue/aut...
chrome/browser/sync/glue/autofill_change_processor.h:19: #include
"chrome/browser/sync/internal_api/write_transaction.h"
can forward declare nodes/transactions here instead of including (in this and
most other datatype specific header files)

http://codereview.chromium.org/7633077/diff/7001/chrome/browser/sync/internal...
File chrome/browser/sync/internal_api/syncapi_functions.cc (right):

http://codereview.chromium.org/7633077/diff/7001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/syncapi_functions.cc:20: std::string
PassphraseRequiredReasonToString(
I think this should be in sync_manager.h (where passphraserequiredreason is
defined)

http://codereview.chromium.org/7633077/diff/7001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/syncapi_functions.cc:38: bool
InitialSyncEndedForTypes(syncable::ModelTypeSet types,
This one too.

http://codereview.chromium.org/7633077/diff/7001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/syncapi_functions.cc:55: syncable::ModelTypeSet
GetEncryptedTypes(
This can be in basetransaction.h

[tim (not reviewing), 2011-08-16 20:08:03]

Getting there! Few more comments

I think we should add DEPS rules on internal_api from engine/ /syncable
/protocol /notifier /sessions

I guess the README is weird... maybe just leave it on SyncManager.

Also, there's a bunch of stuff that was already pulled out of syncapi in /engine
that could go in internal_api:

change_reorder_buffer*
configure_reason*
*model_safe_worker*
http_post_provider_*

You can do it in a follow up change if you prefer.

http://codereview.chromium.org/7633077/diff/7001/chrome/browser/sync/syncable...
File chrome/browser/sync/syncable/syncable.cc (right):

http://codereview.chromium.org/7633077/diff/7001/chrome/browser/sync/syncable...
chrome/browser/sync/syncable/syncable.cc:42: #include
"chrome/browser/sync/internal_api/read_transaction.h"
hrm... this looks odd.  internal_api::ReadTransaction is intended to be a
wrapper around syncable::ReadTransaction.

In fact, I'd like to add a restriction on internal_api/ includes from syncable/.

[rlarocque, 2011-08-16 22:19:19]

I'll upload another code dump shortly.  

In response to Tim's latest comments:
* I've updated the DEPS files for engine, syncable, etc.
* I don't think the README text belongs in sync_manager.h.  The first few lines
are as follows:

> This file defines the "sync API", an interface to the syncer
> backend that exposes (1) the core functionality of maintaining a consistent
> local snapshot of a hierarchical object set; (2) a means to transactionally
> access and modify those objects; (3) a means to control client/server...

I don't think this is an accurate description of SyncManager.  

* I'd prefer to make those additional moves in a follow-up commit.  The longer
it takes to commit this one, the greater the risk of nasty merge conflicts.

http://codereview.chromium.org/7633077/diff/7001/chrome/browser/sync/engine/n...
File chrome/browser/sync/engine/nigori_util.cc (right):

http://codereview.chromium.org/7633077/diff/7001/chrome/browser/sync/engine/n...
chrome/browser/sync/engine/nigori_util.cc:13: #include
"chrome/browser/sync/internal_api/write_transaction.h"
On 2011/08/16 19:39:07, nzea wrote:
> Don't think this writetransaction is necessary (all the transactions here are
> syncable::writetransactions)

Done.

http://codereview.chromium.org/7633077/diff/7001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncapi_internal.cc (right):

http://codereview.chromium.org/7633077/diff/7001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncapi_internal.cc:15: // global helper function:
FIXME move this
On 2011/08/16 19:39:07, nzea wrote:
> FIXME...->TODO(rlarocque): move this

Deleted.  (I already moved the file and should have deleted this line when I
did.)

http://codereview.chromium.org/7633077/diff/7001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncapi_internal.cc:16:
sync_pb::PasswordSpecificsData* DecryptPasswordSpecifics(
On 2011/08/16 19:39:07, nzea wrote:
> I think all three of these methods can actually be part of basenode.

Sadly, no.  
* DecryptPasswordSpecifics is used in BaseNode, SyncManager and WriteNode.
* IsNameServerIllegalAfterTrimming is used in BaseNode and WriteNode.
* AreSpecificsEqual is used in SyncManager and WriteNode (but not BaseNode).  

It might be possible to refactor to fix this.  I haven't investigated that
option because I didn't want to make any code changes in this commit.

http://codereview.chromium.org/7633077/diff/7001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncapi_internal.cc:50: const
sync_pb::EntitySpecifics& left,
On 2011/08/16 19:39:07, nzea wrote:
> fix indent

Done.

http://codereview.chromium.org/7633077/diff/7001/chrome/browser/sync/glue/aut...
File chrome/browser/sync/glue/autofill_change_processor.h (right):

http://codereview.chromium.org/7633077/diff/7001/chrome/browser/sync/glue/aut...
chrome/browser/sync/glue/autofill_change_processor.h:19: #include
"chrome/browser/sync/internal_api/write_transaction.h"
On 2011/08/16 19:39:07, nzea wrote:
> can forward declare nodes/transactions here instead of including (in this and
> most other datatype specific header files)

Fixed here and in many other glue/*.h files.  Let me know if you find any that I
missed in the next patch set.

http://codereview.chromium.org/7633077/diff/7001/chrome/browser/sync/internal...
File chrome/browser/sync/internal_api/syncapi_functions.cc (right):

http://codereview.chromium.org/7633077/diff/7001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/syncapi_functions.cc:20: std::string
PassphraseRequiredReasonToString(
On 2011/08/16 19:39:07, nzea wrote:
> I think this should be in sync_manager.h (where passphraserequiredreason is
> defined)

Done.

http://codereview.chromium.org/7633077/diff/7001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/syncapi_functions.cc:38: bool
InitialSyncEndedForTypes(syncable::ModelTypeSet types,
On 2011/08/16 19:39:07, nzea wrote:
> This one too.

Done.

http://codereview.chromium.org/7633077/diff/7001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/syncapi_functions.cc:55: syncable::ModelTypeSet
GetEncryptedTypes(
On 2011/08/16 19:39:07, nzea wrote:
> This can be in basetransaction.h

Done.

Now I can delete this file.  Woo-hoo!

http://codereview.chromium.org/7633077/diff/7001/chrome/browser/sync/syncable...
File chrome/browser/sync/syncable/syncable.cc (right):

http://codereview.chromium.org/7633077/diff/7001/chrome/browser/sync/syncable...
chrome/browser/sync/syncable/syncable.cc:42: #include
"chrome/browser/sync/internal_api/read_transaction.h"
On 2011/08/16 20:08:03, timsteele wrote:
> hrm... this looks odd.  internal_api::ReadTransaction is intended to be a
> wrapper around syncable::ReadTransaction.
> 
> In fact, I'd like to add a restriction on internal_api/ includes from
syncable/.

Done.  (both removing the include and adding a DEPS rule)

[rlarocque, 2011-08-16 22:25:34]

The latest patch is now up.

[Nicolas Zea, 2011-08-16 22:31:10]

On Tue, Aug 16, 2011 at 3:19 PM, <rlarocque@chromium.org> wrote:
>
> Sadly, no.
> * DecryptPasswordSpecifics is used in BaseNode, SyncManager and
> WriteNode.
> * IsNameServerIllegalAfterTrimmi**ng is used in BaseNode and WriteNode.
> * AreSpecificsEqual is used in SyncManager and WriteNode (but not
> BaseNode).
>
> It might be possible to refactor to fix this.  I haven't investigated
> that option because I didn't want to make any code changes in this
> commit.
>
>
Note that both SyncManager and WriteNode already depend on BaseNode
(WriteNode inherits from BaseNode after all), so it should be fine. But if
you prefer to leave that for a separate patch that works too. Rest LGTM with
Tim's go-ahead.

[tim (not reviewing), 2011-08-16 22:47:57]

Looks great! Thanks!
LGTM

[tim (not reviewing), 2011-08-16 22:50:49]

On 2011/08/16 22:47:57, timsteele wrote:
> Looks great! Thanks!
> LGTM

Should have added - Nicolas is sending this at the try bots for a test run...
once that's green (or there is plausible deniability for the redness :p) you can
check the commit box and watch the fireworks.

[Nicolas Zea, 2011-08-16 23:09:25]

I've uploaded the review at http://codereview.chromium.org/7624009. You can
keep your eye on the try job status there.

On Tue, Aug 16, 2011 at 3:50 PM, <tim@chromium.org> wrote:

> On 2011/08/16 22:47:57, timsteele wrote:
>
>> Looks great! Thanks!
>> LGTM
>>
>
> Should have added - Nicolas is sending this at the try bots for a test
> run...
> once that's green (or there is plausible deniability for the redness :p)
> you can
> check the commit box and watch the fireworks.
>
>
>
http://codereview.chromium.**org/7633077/<http://codereview.chromium.org/7633...
>

[Nicolas Zea, 2011-08-16 23:19:49]

Looks like the integration tests aren't compiling. Make sure you've tested
the following targets: unit_tests, sync_unit_tests, and
sync_integration_tests.

-Nicolas

On Tue, Aug 16, 2011 at 4:09 PM, Nicolas Zea <zea@chromium.org> wrote:

> I've uploaded the review at http://codereview.chromium.org/7624009. You
> can keep your eye on the try job status there.
>
>
> On Tue, Aug 16, 2011 at 3:50 PM, <tim@chromium.org> wrote:
>
>> On 2011/08/16 22:47:57, timsteele wrote:
>>
>>> Looks great! Thanks!
>>> LGTM
>>>
>>
>> Should have added - Nicolas is sending this at the try bots for a test
>> run...
>> once that's green (or there is plausible deniability for the redness :p)
>> you can
>> check the commit box and watch the fireworks.
>>
>>
>>
http://codereview.chromium.**org/7633077/<http://codereview.chromium.org/7633...
>>
>
>

[rlarocque1, 2011-08-16 23:23:53]

Minor complaint: The description of the changelog is no longer accurate.
 syncapi_functions.{h,cc} have been deleted.  Will there be a chance to edit
this before committing?

On Tue, Aug 16, 2011 at 4:09 PM, Nicolas Zea <zea@chromium.org> wrote:

> I've uploaded the review at http://codereview.chromium.org/7624009. You
> can keep your eye on the try job status there.
>
>
> On Tue, Aug 16, 2011 at 3:50 PM, <tim@chromium.org> wrote:
>
>> On 2011/08/16 22:47:57, timsteele wrote:
>>
>>> Looks great! Thanks!
>>> LGTM
>>>
>>
>> Should have added - Nicolas is sending this at the try bots for a test
>> run...
>> once that's green (or there is plausible deniability for the redness :p)
>> you can
>> check the commit box and watch the fireworks.
>>
>>
>>
http://codereview.chromium.**org/7633077/<http://codereview.chromium.org/7633...
>>
>
>

[Nicolas Zea, 2011-08-16 23:27:09]

Yeah, you can just edit your own changelog and I'll pull things from there
to retry the try bots.

On Tue, Aug 16, 2011 at 4:23 PM, Richard Larocque <rlarocque@google.com>wrote:

> Minor complaint: The description of the changelog is no longer accurate.
>  syncapi_functions.{h,cc} have been deleted.  Will there be a chance to edit
> this before committing?
>
> On Tue, Aug 16, 2011 at 4:09 PM, Nicolas Zea <zea@chromium.org> wrote:
>
>> I've uploaded the review at http://codereview.chromium.org/7624009. You
>> can keep your eye on the try job status there.
>>
>>
>> On Tue, Aug 16, 2011 at 3:50 PM, <tim@chromium.org> wrote:
>>
>>> On 2011/08/16 22:47:57, timsteele wrote:
>>>
>>>> Looks great! Thanks!
>>>> LGTM
>>>>
>>>
>>> Should have added - Nicolas is sending this at the try bots for a test
>>> run...
>>> once that's green (or there is plausible deniability for the redness :p)
>>> you can
>>> check the commit box and watch the fireworks.
>>>
>>>
>>>
http://codereview.chromium.**org/7633077/<http://codereview.chromium.org/7633...
>>>
>>
>>
>

[Nicolas Zea, 2011-08-18 17:16:50]

This has landed in 95693.

[Nicolas Zea, 2011-08-18 17:39:31]

On 2011/08/18 17:16:50, nzea wrote:
> This has landed in 95693.

On second thought, I mean 97238!

[rlarocque, 2011-08-18 20:18:32]

On 2011/08/16 20:08:03, timsteele wrote:
> I think we should add DEPS rules on internal_api from engine/ /syncable
> /protocol /notifier /sessions
> 
...
> 
> Also, there's a bunch of stuff that was already pulled out of syncapi in
/engine
> that could go in internal_api:
> 
> change_reorder_buffer*
> configure_reason*
> *model_safe_worker*
> http_post_provider_*
> 
> You can do it in a follow up change if you prefer.
> 

Now that we've got some of the code committed, I'm taking another look at Tim's
comments.

The first commit added DEPS rules for syncable, protocol, notifier, and
sessions.  However, I did not add a rule for engine.  There are three files that
would have failed checkdeps if I had done so:
* engine/all_status.h: Requires access to  sync_api::SyncManager::Status.
* engine/nigori_util.h: Requires access to sync_api::WriteNode
* engine/change_reorder_buffer.h: Requires access to a bunch of different
internal_api headers, but will soon be moved to that directory itself (more on
that later).

It might be possible to fix these issues by moving nigori_util and all_status to
syncapi, but I'm not sure that's the right move.  

As for moving some other syncapi files to internal_api, I'm running into some
issues with that:
* change_reorder_buffer: Actually, this one is OK.
* configure_reason: SyncScheduler needs it to implement "void
SyncScheduler::ScheduleConfig(const ModelTypeBitSet& types,
sync_api::ConfigureReason reason)".  I don't see any obvious way to move
configure_reason.h to internal_api without violating the rule that files in
engine should not include from internal_api.
* model_safe_worker: ModelChangingSyncerCommand needs access to ModelSafeWorker.
 SyncScheduler needs access to ModelSafeWorkerRegistrar.  Once again, I don't
see a way to move the file without violating the rule that engine should not
include from internal_api.
* http_post_provider_*: These classes are used by
SyncAPIServerConnectionManager.  I might be able to make this move work if we
also move engine/net/syncapi_server_connection_manager.* to internal_api.  Does
that sound like a good idea?