sync: Implement fake server reflection blocking

sync: Implement fake server reflection blocking

Implements reflection blocking for the in-process fake sync server.

The notifications delivery system used in the real world will not
deliver notifications of new or modified items to the client that
originated that change.  This feature is known by the names
"reflection blocking" or "squelching".

Most of out integration tests expect reflection blocking to be
disabled, so we can't turn this on in all tests.  The default
is to leave it off.  Tests that want to enable reflection blocking
must override "TestUsesSelfNotifications" in their test framework
class.

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

[rlarocque, 2014-05-21 17:26:01]

Hi Patrick,

I was planning to refactor some integration tests, but I needed this
functionality to verify that I was doing it correctly.

Sorry if this comes as a surprise or conflicts with your existing work.  I can
keep this patch to myself if it conflicts with your current efforts.

Otherwise, please review.

[pval...(no longer on Chromium), 2014-05-21 17:37:03]

This patch is definitely welcome. Thanks for adding this.

https://codereview.chromium.org/296743005/diff/1/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/fake_server_invalidation_service.h
(right):

https://codereview.chromium.org/296743005/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/fake_server_invalidation_service.h:55: void
SetSelfNotificationsEnabled(bool enabled);
How about two methods: EnableSelfNotifications() and DisableSelfNotifications()?
I see that the call point maybe lends itself to one method, but having two
methods seems like a cleaner API.

Alternatively, could we just add self_notify as a constructor arg? It doesn't
look like we'll need to change the value over the lifetime of the service.

[rlarocque, 2014-05-21 17:50:36]

https://codereview.chromium.org/296743005/diff/1/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/fake_server_invalidation_service.h
(right):

https://codereview.chromium.org/296743005/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/fake_server_invalidation_service.h:55: void
SetSelfNotificationsEnabled(bool enabled);
On 2014/05/21 17:37:03, pvalenzuela wrote:
> How about two methods: EnableSelfNotifications() and
DisableSelfNotifications()?
> I see that the call point maybe lends itself to one method, but having two
> methods seems like a cleaner API.
> 
> Alternatively, could we just add self_notify as a constructor arg? It doesn't
> look like we'll need to change the value over the lifetime of the service.

Yes, I was mainly concerned about the call site when I wrote it this way.  I
could change it to two methods.  That might also allow me to avoid any decisions
about whether or not to use the NOTIFY_ALL / NOTIFY_OTHERS enums declared in the
p2p invalidator code vs. a bool.  Probably a good idea.

As for the constructor approach, that's the route I took with the P2P
invalidaton.  I think I was partly forced into it because of the way its members
are initialized.

The BrowserContextKeyedServiceFactory's SetTestingFactoryAndUse() takes a
function pointer, not a Closure.  So we can't bind arguments to it.  If we want
to have constructor arguments, we need to define dummy functions to wrap the
alternatives.

You can see how this turned out for the P2P case in lines 145-166 of
sync_test.cc.

I'm leaning towards separate methods.  Let me know if you'd prefer the
constructor approach.

[pval...(no longer on Chromium), 2014-05-21 19:46:58]

lgtm

https://codereview.chromium.org/296743005/diff/1/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/fake_server_invalidation_service.h
(right):

https://codereview.chromium.org/296743005/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/fake_server_invalidation_service.h:55: void
SetSelfNotificationsEnabled(bool enabled);
On 2014/05/21 17:50:36, rlarocque wrote:
> On 2014/05/21 17:37:03, pvalenzuela wrote:
> > How about two methods: EnableSelfNotifications() and
> DisableSelfNotifications()?
> > I see that the call point maybe lends itself to one method, but having two
> > methods seems like a cleaner API.
> > 
> > Alternatively, could we just add self_notify as a constructor arg? It
doesn't
> > look like we'll need to change the value over the lifetime of the service.
> 
> Yes, I was mainly concerned about the call site when I wrote it this way.  I
> could change it to two methods.  That might also allow me to avoid any
decisions
> about whether or not to use the NOTIFY_ALL / NOTIFY_OTHERS enums declared in
the
> p2p invalidator code vs. a bool.  Probably a good idea.
> 
> As for the constructor approach, that's the route I took with the P2P
> invalidaton.  I think I was partly forced into it because of the way its
members
> are initialized.
> 
> The BrowserContextKeyedServiceFactory's SetTestingFactoryAndUse() takes a
> function pointer, not a Closure.  So we can't bind arguments to it.  If we
want
> to have constructor arguments, we need to define dummy functions to wrap the
> alternatives.
> 
> You can see how this turned out for the P2P case in lines 145-166 of
> sync_test.cc.
> 
> I'm leaning towards separate methods.  Let me know if you'd prefer the
> constructor approach.

separate methods sgtm

[rlarocque, 2014-05-21 19:50:41]

On 2014/05/21 19:46:58, pvalenzuela wrote:
> lgtm
> 
>
https://codereview.chromium.org/296743005/diff/1/chrome/browser/sync/test/int...
> File chrome/browser/sync/test/integration/fake_server_invalidation_service.h
> (right):
> 
>
https://codereview.chromium.org/296743005/diff/1/chrome/browser/sync/test/int...
> chrome/browser/sync/test/integration/fake_server_invalidation_service.h:55:
void
> SetSelfNotificationsEnabled(bool enabled);
> On 2014/05/21 17:50:36, rlarocque wrote:
> > On 2014/05/21 17:37:03, pvalenzuela wrote:
> > > How about two methods: EnableSelfNotifications() and
> > DisableSelfNotifications()?
> > > I see that the call point maybe lends itself to one method, but having two
> > > methods seems like a cleaner API.
> > > 
> > > Alternatively, could we just add self_notify as a constructor arg? It
> doesn't
> > > look like we'll need to change the value over the lifetime of the service.
> > 
> > Yes, I was mainly concerned about the call site when I wrote it this way.  I
> > could change it to two methods.  That might also allow me to avoid any
> decisions
> > about whether or not to use the NOTIFY_ALL / NOTIFY_OTHERS enums declared in
> the
> > p2p invalidator code vs. a bool.  Probably a good idea.
> > 
> > As for the constructor approach, that's the route I took with the P2P
> > invalidaton.  I think I was partly forced into it because of the way its
> members
> > are initialized.
> > 
> > The BrowserContextKeyedServiceFactory's SetTestingFactoryAndUse() takes a
> > function pointer, not a Closure.  So we can't bind arguments to it.  If we
> want
> > to have constructor arguments, we need to define dummy functions to wrap the
> > alternatives.
> > 
> > You can see how this turned out for the P2P case in lines 145-166 of
> > sync_test.cc.
> > 
> > I'm leaning towards separate methods.  Let me know if you'd prefer the
> > constructor approach.
> 
> separate methods sgtm

Thanks.  That change is implemented in patch set 2.

I think your approval is enough to allow me to CQ this.  Let's find out.

[rlarocque, 2014-05-21 19:50:48]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-05-21 19:58:19]

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

[commit-bot: I haz the power, 2014-05-21 19:58:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-21 19:58:21]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[rlarocque, 2014-05-21 19:59:58]

Darn.  I'll need some help with this one.

Nick, let's see if your new and improved committer stamp works.

[maniscalco, 2014-05-22 17:59:58]

lgtm

Rubber stamp LGTM.

[rlarocque, 2014-05-22 18:08:32]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-05-22 18:10:55]

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

[commit-bot: I haz the power, 2014-05-22 21:30:17]

Change committed as 272321