Move sync/notifier to components/invalidation

Move sync/notifier to components/invalidation

Moves all code remaining in sync/notifier to components/invalidation.
Updates gyp files, DEPS, and #includes accordingly.

In terms of program behavior, this should be one big no-op.  This CL
contains no non-trivial code changes.

In terms of the build system, this is a significant change.  Symbols
that were previously exported through sync_notifier and related targets
have moved into the components/invalidation related targets.  Some
SYNC_EXPORT macros have been replaced with INVALIDATION_EXPORT, which is
significant since INVALIDATION_EXPORT is currently a no-op but
SYNC_EXPORT has meaning under some build configurations.

Unlike most other files in sync/notifier, invalidation_util.{cc,h} was
moved to sync/internal_api/public/base.  This is so it could be
referenced from sync/internal_api/public/base/invalidation.cc.  This is
a slight regression, but it should be fixed in the next CL when we move
all invalidations-related code out of sync/internal_api.

TBR=rogerta,benwells
BUG=259559
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283840

[rlarocque, 2014-07-14 22:47:54]

This CL is the biggest and second last step towards making invalidations build
separately from sync.

I know this might look pretty scary, but the actual number of changes is not
that large once you enable rename detection.  I recommend downloading the patch
to your local repo so you can do 'git diff -M50' to filter out the renames.

The current version is slightly out of date.  I intend to rebase it on top of
https://codereview.chromium.org/394503003/.  However, I don't foresee any major
changes other than a few merge conflicts.  

pavely: Please review components/invalidation and chrome/browser/invalidation.
maniscalco: Please review sync/ and components/sync_driver.

[rlarocque, 2014-07-14 23:32:40]

Patch updated.

+bartfab for chrome/browser/policy and chrome/browser/chromeos/policy.
+hashimoto for drive_notification_manager.
+bauerb for chrome/browser/ui/webui.

Also, I forgot to assign an reviewer for components/components_tests.gypi,
chrome/chrome_tests_unit.gyp, and components/invalidation.gypi.  Pavel, could
you review those files as well?

By the way, I may start TBRing reviewers if the commit of this CL is blocked
solely on the review of trivial changes.  Let me know if you'd prefer not to be
TBR'ed.

[rlarocque, 2014-07-14 23:33:41]

Oops, forgot to actually add those new reviewers.  Let's try this again.

+bartfab for chrome/browser/policy and chrome/browser/chromeos/policy.
+hashimoto for drive_notification_manager.
+bauerb for chrome/browser/ui/webui.

[bartfab (slow), 2014-07-15 09:40:11]

chrome/browser/policy and chrome/browser/chromeos/policy LGTM

[hashimoto, 2014-07-15 09:43:04]

chrome/browser/chromeos/drive lgtm

[Bernhard Bauer, 2014-07-15 10:03:15]

Web UI LGTM

[rlarocque, 2014-07-16 17:18:59]

maniscalco, pavely: Friendly ping.

I could have TBR'ed the other reviewers, but I don't think it would be
reasonable to TBR you two in this case.

I realize this patch is big and the review could take a while.  Let me know if
there's anything I can do to make this easier.

[rlarocque, 2014-07-16 17:24:54]

Adding a few more reviewers for some files I missed.

+miket for chrome/browser/extensions/api
+rogerta for google_apis.gyp

PS: The change to google_apis is not obviously related.  There's an explanation
in crbug.com/392532.  Feel free to contact me if it's not clear.

[rlarocque, 2014-07-16 17:26:16]

Adding a few more reviewers for some files I missed.

+miket for chrome/browser/extensions/api
+rogerta for google_apis.gyp

PS: The change to google_apis is not obviously related.  There's an explanation
in crbug.com/392532.  Feel free to contact me if it's not clear.

[maniscalco, 2014-07-16 17:44:46]

Glad you pinged me.  Sorry for the delay.  Here's my feedback after looking at
sync/ and components/sync_driver.

https://codereview.chromium.org/387733004/diff/140001/google_apis/google_apis...
File google_apis/google_apis.gyp (right):

https://codereview.chromium.org/387733004/diff/140001/google_apis/google_apis...
google_apis/google_apis.gyp:24:
'../base/third_party/dynamic_annotations/dynamic_annotations.gyp:dynamic_annotations',
nit: sort order

https://codereview.chromium.org/387733004/diff/140001/sync/internal_api/publi...
File sync/internal_api/public/base/invalidation.h (right):

https://codereview.chromium.org/387733004/diff/140001/sync/internal_api/publi...
sync/internal_api/public/base/invalidation.h:38: Invalidation(const
Invalidation& other);
Not changing in this CL, but do we need our own copy ctor?  I looked at the
implementation and I don't see how it's different from what the compiler would
provide.  Just curious.

https://codereview.chromium.org/387733004/diff/140001/sync/internal_api/publi...
sync/internal_api/public/base/invalidation.h:40: Invalidation& operator=(const
Invalidation& other);
Similar to my question about the cctor, do we need to provide an assignment
operator?  Is the compiler-provided one not what we want here?

If we do need one, consider adding a comment here mentioning why and another
comment down where the class member variables are defined reminding future
maintainer that when they add data members they must update the cctor and oper=.

https://codereview.chromium.org/387733004/diff/140001/sync/sync_internal_api....
File sync/sync_internal_api.gypi (right):

https://codereview.chromium.org/387733004/diff/140001/sync/sync_internal_api....
sync/sync_internal_api.gypi:17:
'../third_party/cacheinvalidation/cacheinvalidation.gyp:cacheinvalidation',
nit: would be nice to sort the dependencies.

https://codereview.chromium.org/387733004/diff/140001/sync/sync_tests.gypi
File sync/sync_tests.gypi (right):

https://codereview.chromium.org/387733004/diff/140001/sync/sync_tests.gypi#ne...
sync/sync_tests.gypi:173:
'../third_party/cacheinvalidation/cacheinvalidation.gyp:cacheinvalidation',
nit: is this the right sort order?

https://codereview.chromium.org/387733004/diff/140001/sync/sync_tests.gypi#ne...
sync/sync_tests.gypi:179:
'../third_party/cacheinvalidation/cacheinvalidation.gyp:cacheinvalidation',
nit: is this the right sort order?

[pavely, 2014-07-16 20:21:55]

lgtm. My only question is the same that Nick asked, about ctor and assignment
operator.

[rlarocque, 2014-07-16 21:06:36]

On 2014/07/16 20:21:55, pavely wrote:
> lgtm. My only question is the same that Nick asked, about ctor and assignment
> operator.

Thanks a lot.  I really appreciate the thoughtful reviews.

I'm re-defining the copy and assignment operators because the Windows build
seems to require it.  There seems to be some bad interaction between exported
classes and the default operators.  This makes some intuitive sense.  If the
compiler tries to define the copy constructor in every .o where its referenced
(and where else would it put the thing if unless we defined it in a .cc file?),
and it tries to export those symbols from each of those .o files, I could see
how the linker gets confused when it tries to put together these libraries. 
Also, the sync build is excessively componentized, and this may contribute to
the problem.  

I've found that explicitly defining these functions in the .cc file makes the
linker errors go away.  For now, that's good enough for me.  I hope to
re-investigate the issue when I get to work on refactoring the sync build.

I'll upload a new CL that addresses Nick's comments and adds some comments to
the spurious-looking definitions later today.

[rlarocque, 2014-07-16 22:27:22]

Patch updated.  PTAL.

[maniscalco, 2014-07-16 22:31:47]

On 2014/07/16 22:27:22, rlarocque wrote:
> Patch updated.  PTAL.

sync/ and components/sync_driver lgtm.  Thanks for explaining the cctor/oper=
thing in the bug.

[rlarocque, 2014-07-17 00:56:20]

Status update:

With every rebase, I worry that there's a good chance I'll introduce new bugs
into this CL.  For that reason, I'd like to get this committed soon.

I just tripped over the newly added GN build for components/invalidation.  I've
got it working well enough for now, but there's a few TODOs we'll need to
resolve before we can get it to link.  I think that's acceptable because the GN
build was already a few TODOs short of being link-able before this patch was
applied.

Nick may have found a solution for the constructor definition problem, but it's
probably going to have to wait for a follow-up patch.

I've got some trybot runs in progress right now.  If they look green, I'll try
to commit this later tonight, using TBRs as necessary.

[rlarocque, 2014-07-17 00:59:46]

+benwells to replace miket on the review of chrome/browser/extensions, since
Mike is currently on vacation.

rogerta is also on vacation, which is unfortunate since he's the only owner of
google_apis.  I guess I'll have to TBR him.

[rlarocque, 2014-07-17 17:18:59]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-07-17 17:20:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rlarocque@chromium.org/387733004/200001

[commit-bot: I haz the power, 2014-07-17 19:29:04]

Change committed as 283840