Merge duplicate code in invalidation helper.

Merge duplicate code in invalidation helper.

There is code duplication in:
  components/sync_driver/invalidation_helper
  sync/tools/invalidation_helper

This patch merged those files by removing duplicate code in
sync/tools/invalidation_helper.

BUG=528259
Committed: https://crrev.com/de7123741b492a12f35a0cbd7b3e36ff3839a3e6
Cr-Commit-Position: refs/heads/master@{#349849}

Committed: https://crrev.com/61dd5d4b919569d5cda3bb100a41b20e9053b6ce
Cr-Commit-Position: refs/heads/master@{#349930}

[Jitu( very slow this week), 2015-09-09 08:08:10]

jitendra.ks@samsung.com changed reviewers:
+ droger@chromium.org

[Jitu( very slow this week), 2015-09-09 08:08:11]

PTAL

[droger, 2015-09-09 08:29:08]

droger@chromium.org changed reviewers:
+ zea@chromium.org

[droger, 2015-09-09 08:29:08]

Code looks good, although the dependency of //sync on //components looks wrong
to me.

zea: Can you confirm if this is indeed the architecture we want? Maybe the
reason why this code was duplicated is because of this dependency on
//components?

https://codereview.chromium.org/1310323009/diff/1/sync/sync.gyp
File sync/sync.gyp (right):

https://codereview.chromium.org/1310323009/diff/1/sync/sync.gyp#newcode54
sync/sync.gyp:54: '../components/components.gyp:invalidation_public',
Can we remove this?

https://codereview.chromium.org/1310323009/diff/1/sync/util/invalidation_help...
File sync/util/invalidation_helper.h (right):

https://codereview.chromium.org/1310323009/diff/1/sync/util/invalidation_help...
sync/util/invalidation_helper.h:8: #include
"components/invalidation/public/invalidation_util.h"
Is this include necessary?

If we can't remove, can we replace by:

namespace invalidation {
class ObjectId;
}

[Jitu( very slow this week), 2015-09-09 09:59:04]

https://codereview.chromium.org/1310323009/diff/1/sync/util/invalidation_help...
File sync/util/invalidation_helper.h (right):

https://codereview.chromium.org/1310323009/diff/1/sync/util/invalidation_help...
sync/util/invalidation_helper.h:8: #include
"components/invalidation/public/invalidation_util.h"
On 2015/09/09 08:29:08, droger wrote:
> Is this include necessary?
> 
> If we can't remove, can we replace by:
> 
> namespace invalidation {
> class ObjectId;
> }

Including ObjectId is look good.

But how about "ObjectIdSet"

[droger, 2015-09-09 10:59:15]

https://codereview.chromium.org/1310323009/diff/1/sync/util/invalidation_help...
File sync/util/invalidation_helper.h (right):

https://codereview.chromium.org/1310323009/diff/1/sync/util/invalidation_help...
sync/util/invalidation_helper.h:8: #include
"components/invalidation/public/invalidation_util.h"
On 2015/09/09 09:59:04, jitu wrote:
> On 2015/09/09 08:29:08, droger wrote:
> > Is this include necessary?
> > 
> > If we can't remove, can we replace by:
> > 
> > namespace invalidation {
> > class ObjectId;
> > }
> 
> Including ObjectId is look good.
> 
> But how about "ObjectIdSet"

Right, we have to include the file, at least in the .cc. You can keep it as is
then.

[Jitu( very slow this week), 2015-09-10 12:07:47]

On 2015/09/09 10:59:15, droger wrote:
>
https://codereview.chromium.org/1310323009/diff/1/sync/util/invalidation_help...
> File sync/util/invalidation_helper.h (right):
> 
>
https://codereview.chromium.org/1310323009/diff/1/sync/util/invalidation_help...
> sync/util/invalidation_helper.h:8: #include
> "components/invalidation/public/invalidation_util.h"
> On 2015/09/09 09:59:04, jitu wrote:
> > On 2015/09/09 08:29:08, droger wrote:
> > > Is this include necessary?
> > > 
> > > If we can't remove, can we replace by:
> > > 
> > > namespace invalidation {
> > > class ObjectId;
> > > }
> > 
> > Including ObjectId is look good.
> > 
> > But how about "ObjectIdSet"
> 
> Right, we have to include the file, at least in the .cc. You can keep it as is
> then.

Dear David,

In trybots it is failing in some Bots, As i checked i have added the
dependencies in gyp files.
But not getting why it is failing.

One more thing, i have added invalidation_helper as part of "sync_core" target.
But as per comment given above that target, it should not depends on chrome or
components.

Could you please help me in this.

[droger, 2015-09-10 12:15:05]

It looks like these are dependencies issues in gyp/GN.

Before tackling those, lets first sort out the overall architecture, because I
am not convinced that it is fine for //sync/util to depend on //components.

zea: can you advise here?

[Nicolas Zea, 2015-09-14 19:17:32]

On 2015/09/10 12:15:05, droger wrote:
> It looks like these are dependencies issues in gyp/GN.
> 
> Before tackling those, lets first sort out the overall architecture, because I
> am not convinced that it is fine for //sync/util to depend on //components.
> 
> zea: can you advise here?

Agree that sync/util shouldn't really be depending on components/. sync/tools is
fine. Would moving this to components/sync_driver work? sync_client.cc and
sync_listen_notifications.cc might also need to be moved into
components/sync_driver/tools.

[droger, 2015-09-15 13:10:35]

On 2015/09/14 19:17:32, Nicolas Zea wrote:
> On 2015/09/10 12:15:05, droger wrote:
> > It looks like these are dependencies issues in gyp/GN.
> > 
> > Before tackling those, lets first sort out the overall architecture, because
I
> > am not convinced that it is fine for //sync/util to depend on //components.
> > 
> > zea: can you advise here?
> 
> Agree that sync/util shouldn't really be depending on components/. sync/tools
is
> fine. Would moving this to components/sync_driver work? sync_client.cc and
> sync_listen_notifications.cc might also need to be moved into
> components/sync_driver/tools.

Would this be a separate target?

If sync/tools is allowed to depend on components, then we could just put the
file in the main sync_driver target in //components/sync_driver, where both
sync/tools and other sync_driver code can use it.

[Nicolas Zea, 2015-09-15 20:09:50]

Hmm, good question. Both of those are their own target really. Is that a problem
for components/? Having some of sync/ allow dependencies on components/ seems
inconsistent/error prone

[droger, 2015-09-15 20:59:32]

On 2015/09/15 20:09:50, Nicolas Zea wrote:
> Hmm, good question. Both of those are their own target really. Is that a
problem
> for components/? Having some of sync/ allow dependencies on components/ seems
> inconsistent/error prone

It's already the case that sync/tools depends on components (it depends on
invalidations).
This is not what we are trying to solve, and the goal of this CL is not to
remove this dependency.

What we want here is sharing the duplicated code that is in sync/tools and
components/sync_driver, because it is causing duplicated symbols and breaks the
link.
Putting it in sync/util (as done here) introduces a dependency on components in
sync/util, which looks bad.
I suggest putting it in sync_driver directly.

If we want to also remove the other dependencies of sync/tools on components,
maybe we could move more files, or move the whole sync/tools to components, but
again this is not the primary goal here.

[Nicolas Zea, 2015-09-16 17:55:16]

Right, I'm fine with the tools to components/.

[Jitu( very slow this week), 2015-09-18 13:06:49]

Patchset #2 (id:20001) has been deleted

[Jitu( very slow this week), 2015-09-18 13:55:26]

PTAL!

[droger, 2015-09-18 14:01:01]

LGTM for me.
I can look at whether it's possible to move //sync/tools to
//components/sync_tools as a followup if needed.

[Nicolas Zea, 2015-09-18 20:37:09]

LGTM, thanks! Moving sync/tools would be nice, but certainly isn't a high
priority.

[Jitu( very slow this week), 2015-09-19 04:36:30]

The CQ bit was checked by jitendra.ks@samsung.com

[commit-bot: I haz the power, 2015-09-19 04:36:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1310323009/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1310323009/40001

[commit-bot: I haz the power, 2015-09-19 04:41:20]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2015-09-19 04:42:10]

Patchset 2 (id:??) landed as
https://crrev.com/de7123741b492a12f35a0cbd7b3e36ff3839a3e6
Cr-Commit-Position: refs/heads/master@{#349849}

[Peter Kasting, 2015-09-19 07:13:30]

A revert of this CL (patchset #2 id:40001) has been created in
https://codereview.chromium.org/1357143002/ by pkasting@chromium.org.

The reason for reverting is: Broke gn gen; see e.g.
http://build.chromium.org/p/chromium.linux/builders/Linux%20Builder/builds/56....

[droger, 2015-09-21 07:17:11]

I don't understand why this passed the CQ.

https://codereview.chromium.org/1310323009/diff/40001/sync/tools/BUILD.gn
File sync/tools/BUILD.gn (right):

https://codereview.chromium.org/1310323009/diff/40001/sync/tools/BUILD.gn#new...
sync/tools/BUILD.gn:20: "//sync",
Looks like there is a missing dependency on //components/sync_driver.

[droger, 2015-09-21 07:18:08]

https://codereview.chromium.org/1310323009/diff/40001/sync/tools/BUILD.gn
File sync/tools/BUILD.gn (right):

https://codereview.chromium.org/1310323009/diff/40001/sync/tools/BUILD.gn#new...
sync/tools/BUILD.gn:34: "//components/invalidation/impl",
And probably here too.

[Jitu( very slow this week), 2015-09-21 13:03:22]

PTAL !

https://codereview.chromium.org/1310323009/diff/40001/sync/tools/BUILD.gn
File sync/tools/BUILD.gn (right):

https://codereview.chromium.org/1310323009/diff/40001/sync/tools/BUILD.gn#new...
sync/tools/BUILD.gn:20: "//sync",
On 2015/09/21 07:17:11, droger wrote:
> Looks like there is a missing dependency on //components/sync_driver.

Done.

https://codereview.chromium.org/1310323009/diff/40001/sync/tools/BUILD.gn#new...
sync/tools/BUILD.gn:34: "//components/invalidation/impl",
On 2015/09/21 07:18:07, droger wrote:
> And probably here too.

I think only adding in above place is fine, as already this target depends on
:common

[droger, 2015-09-21 13:11:13]

LGTM with the nit below.

https://codereview.chromium.org/1310323009/diff/40001/sync/tools/BUILD.gn
File sync/tools/BUILD.gn (right):

https://codereview.chromium.org/1310323009/diff/40001/sync/tools/BUILD.gn#new...
sync/tools/BUILD.gn:34: "//components/invalidation/impl",
On 2015/09/21 13:03:22, jitu wrote:
> On 2015/09/21 07:18:07, droger wrote:
> > And probably here too.
> 
> I think only adding in above place is fine, as already this target depends on
> :common

You are right that it will compile.

However, the recommendation is to always explicitly list the depedencies that
are needed, and not rely on transitive dependencies.
Since sync_listen_notifications.cc has includes in sync_driver, the dependency
should be added here too.

The benefit of this, is that if somebody removes the dependency in :common in
the future, this target will not break.


This is similar to the practice for #includes directives: we always include what
we use and don't rely on transitive #includes.

[Jitu( very slow this week), 2015-09-21 14:43:27]

The CQ bit was checked by jitendra.ks@samsung.com

[Jitu( very slow this week), 2015-09-21 14:43:28]

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org,
droger@chromium.org
Link to the patchset: https://codereview.chromium.org/1310323009/#ps80001
(title: " ")

[commit-bot: I haz the power, 2015-09-21 14:43:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1310323009/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1310323009/80001

[commit-bot: I haz the power, 2015-09-21 15:32:16]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-09-21 15:32:58]

Patchset 4 (id:??) landed as
https://crrev.com/61dd5d4b919569d5cda3bb100a41b20e9053b6ce
Cr-Commit-Position: refs/heads/master@{#349930}