Add serialization to PairingRegistry.

Add serialization to PairingRegistry.

To implement this, PairingRegistry maintains a queue of pending requests. Each
public method wraps the original callback inside an interstitial callback that
runs the next pending request in addition to invoking the original callback.

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

[Jamie, 2013-07-18 00:15:07]

PTAL. I haven't implemented the dtor behaviour we discussed to ensure that the
callback is invoked even in this case. I can either add that to this CL once
we've agreed on the general outline, or as a follow-up.

https://codereview.chromium.org/19714002/diff/1/remoting/protocol/pairing_reg...
File remoting/protocol/pairing_registry.h (right):

https://codereview.chromium.org/19714002/diff/1/remoting/protocol/pairing_reg...
remoting/protocol/pairing_registry.h:139: void
SavePairingAndScheduleNext(Pairing pairing,
I'm not convinced by the naming of these methods. They don't schedule the next
request in the queue directly (this happens because they are provided with a
suitable callback), but that's the key difference between them and the
corresponding public methods. I'm open to suggestions for a better naming
convention.

[Lambros, 2013-07-18 20:34:51]

https://codereview.chromium.org/19714002/diff/1/remoting/protocol/pairing_reg...
File remoting/protocol/pairing_registry.h (right):

https://codereview.chromium.org/19714002/diff/1/remoting/protocol/pairing_reg...
remoting/protocol/pairing_registry.h:188: bool servicing_request_;
Suggestion: if you keep the currently-serviced request on the queue, and only
pop it off after completion, I think you won't need the separate flag - you can
just test the queue for emptiness.

https://codereview.chromium.org/19714002/diff/1/remoting/protocol/pairing_reg...
File remoting/protocol/pairing_registry_unittest.cc (right):

https://codereview.chromium.org/19714002/diff/1/remoting/protocol/pairing_reg...
remoting/protocol/pairing_registry_unittest.cc:208:
registry->ClearAllPairings(base::Bind(ExpectSaveSuccess));
optional: I know I originally wrote this myself in the test cases I added, but I
don't think base::Bind makes sense here - we're not binding anything. Maybe
remove it, or replace it with an explicit Callback ctor:
SaveCallback(ExpectSaveSuccess) ?

https://codereview.chromium.org/19714002/diff/1/remoting/protocol/pairing_reg...
remoting/protocol/pairing_registry_unittest.cc:215:
mock_delegate->RunCallback();
I think we should verify the number of times this gets called. Otherwise a
PairingRegistry that fails to queue some requests might still pass this test as
the expectations would not be triggered.
Something like this:

for (int i = 0; i < ??; i++) {
  ASSERT_TRUE(mock_delegate->HasCallback());
  mock_delegate->RunCallback();
}
EXPECT_FALSE(mock_delegate->HasCallback());

Is there a better alternative that doesn't involve knowing how many callbacks
there should be?

https://codereview.chromium.org/19714002/diff/1/remoting/protocol/protocol_mo...
File remoting/protocol/protocol_mock_objects.cc (right):

https://codereview.chromium.org/19714002/diff/1/remoting/protocol/protocol_mo...
remoting/protocol/protocol_mock_objects.cc:61: DCHECK(load_callback_.is_null());
Use ASSERT_TRUE here and elsewhere instead of DCHECK.

https://codereview.chromium.org/19714002/diff/1/remoting/protocol/protocol_mo...
remoting/protocol/protocol_mock_objects.cc:85: load_callback_ =
base::Bind(base::Bind(callback), pairings_json_);
optional: Remove inner base::Bind ?

https://codereview.chromium.org/19714002/diff/1/remoting/protocol/protocol_mo...
remoting/protocol/protocol_mock_objects.cc:89: DCHECK(load_callback_.is_null() ^
save_callback_.is_null());
Split this up, otherwise the test output won't tell you whether they were both
set or both unset.

Maybe something like:

if (have load callback) {
  EXPECT_TRUE(save is null);
  ...
} else if (have save callback) {
  EXPECT_TRUE(load is null);
  ...
} else {
  ADD_FAILURE() << "RunCallback() called without any callbacks set";
}

https://codereview.chromium.org/19714002/diff/1/remoting/protocol/protocol_mo...
File remoting/protocol/protocol_mock_objects.h (right):

https://codereview.chromium.org/19714002/diff/1/remoting/protocol/protocol_mo...
remoting/protocol/protocol_mock_objects.h:227: // manually. For unit tests that
need to verify correct behaviour under
optional: I think it's strange to have the negotiating authenticator run one set
of callbacks automatically, but not another set. I'd prefer all or nothing.
Consider updating the negotiating authenticator tests.

https://codereview.chromium.org/19714002/diff/1/remoting/protocol/protocol_mo...
remoting/protocol/protocol_mock_objects.h:236: return !load_callback_.is_null()
|| !save_callback_.is_null();
nit: Move to .cc file? I think clang might trigger on inlined methods that are
"too complex"?
Also, should be const.

[Jamie, 2013-07-18 22:44:56]

PTAL. Note that I've also removed the private PairingRegistry methods that I
added purely to be queuable closures as you suggested off-line. The queued
closures now refer to the delegate directly.

https://codereview.chromium.org/19714002/diff/1/remoting/protocol/pairing_reg...
File remoting/protocol/pairing_registry.h (right):

https://codereview.chromium.org/19714002/diff/1/remoting/protocol/pairing_reg...
remoting/protocol/pairing_registry.h:188: bool servicing_request_;
On 2013/07/18 20:34:51, Lambros wrote:
> Suggestion: if you keep the currently-serviced request on the queue, and only
> pop it off after completion, I think you won't need the separate flag - you
can
> just test the queue for emptiness.

Done.

https://codereview.chromium.org/19714002/diff/1/remoting/protocol/pairing_reg...
File remoting/protocol/pairing_registry_unittest.cc (right):

https://codereview.chromium.org/19714002/diff/1/remoting/protocol/pairing_reg...
remoting/protocol/pairing_registry_unittest.cc:208:
registry->ClearAllPairings(base::Bind(ExpectSaveSuccess));
On 2013/07/18 20:34:51, Lambros wrote:
> optional: I know I originally wrote this myself in the test cases I added, but
I
> don't think base::Bind makes sense here - we're not binding anything. Maybe
> remove it, or replace it with an explicit Callback ctor:
> SaveCallback(ExpectSaveSuccess) ?

Bind is needed to wrap a bare function. See callback.h for details.

https://codereview.chromium.org/19714002/diff/1/remoting/protocol/pairing_reg...
remoting/protocol/pairing_registry_unittest.cc:215:
mock_delegate->RunCallback();
On 2013/07/18 20:34:51, Lambros wrote:
> I think we should verify the number of times this gets called. Otherwise a
> PairingRegistry that fails to queue some requests might still pass this test
as
> the expectations would not be triggered.
> Something like this:
> 
> for (int i = 0; i < ??; i++) {
>   ASSERT_TRUE(mock_delegate->HasCallback());
>   mock_delegate->RunCallback();
> }
> EXPECT_FALSE(mock_delegate->HasCallback());
> 
> Is there a better alternative that doesn't involve knowing how many callbacks
> there should be?

This test shouldn't care how many *delegate* callbacks are made, but you're
correct that we need to verify that the correct number of pairing registry
callbacks were made. I've added a simple counter for that purpose and cleaned up
another unit test that was using a boolean and a string for a similar purpose.

https://codereview.chromium.org/19714002/diff/1/remoting/protocol/protocol_mo...
File remoting/protocol/protocol_mock_objects.cc (right):

https://codereview.chromium.org/19714002/diff/1/remoting/protocol/protocol_mo...
remoting/protocol/protocol_mock_objects.cc:61: DCHECK(load_callback_.is_null());
On 2013/07/18 20:34:51, Lambros wrote:
> Use ASSERT_TRUE here and elsewhere instead of DCHECK.

Done.

https://codereview.chromium.org/19714002/diff/1/remoting/protocol/protocol_mo...
remoting/protocol/protocol_mock_objects.cc:89: DCHECK(load_callback_.is_null() ^
save_callback_.is_null());
On 2013/07/18 20:34:51, Lambros wrote:
> Split this up, otherwise the test output won't tell you whether they were both
> set or both unset.
> 
> Maybe something like:
> 
> if (have load callback) {
>   EXPECT_TRUE(save is null);
>   ...
> } else if (have save callback) {
>   EXPECT_TRUE(load is null);
>   ...
> } else {
>   ADD_FAILURE() << "RunCallback() called without any callbacks set";
> }

Done.

https://codereview.chromium.org/19714002/diff/1/remoting/protocol/protocol_mo...
File remoting/protocol/protocol_mock_objects.h (right):

https://codereview.chromium.org/19714002/diff/1/remoting/protocol/protocol_mo...
remoting/protocol/protocol_mock_objects.h:227: // manually. For unit tests that
need to verify correct behaviour under
On 2013/07/18 20:34:51, Lambros wrote:
> optional: I think it's strange to have the negotiating authenticator run one
set
> of callbacks automatically, but not another set. I'd prefer all or nothing.
> Consider updating the negotiating authenticator tests.

I think it makes sense because the focus of the tests is different. Here, we're
specifically testing the asynchronous nature of the pairing registry, so we need
to be able to control when the callbacks fire. The the negotiating authenticator
test, the focus is on the overall flow, so there's no need to add to the overall
complexity.

https://codereview.chromium.org/19714002/diff/1/remoting/protocol/protocol_mo...
remoting/protocol/protocol_mock_objects.h:236: return !load_callback_.is_null()
|| !save_callback_.is_null();
On 2013/07/18 20:34:51, Lambros wrote:
> nit: Move to .cc file? I think clang might trigger on inlined methods that are
> "too complex"?
> Also, should be const.

I've made it const, but I think this is simple enough to be inlined, and clang
agrees :)

[Lambros, 2013-07-19 01:54:21]

lgtm with comments

https://codereview.chromium.org/19714002/diff/8001/remoting/protocol/pairing_...
File remoting/protocol/pairing_registry.cc (right):

https://codereview.chromium.org/19714002/diff/8001/remoting/protocol/pairing_...
remoting/protocol/pairing_registry.cc:278: request.Run();
|request| temporary not needed anymore?

https://codereview.chromium.org/19714002/diff/8001/remoting/protocol/protocol...
File remoting/protocol/protocol_mock_objects.cc (right):

https://codereview.chromium.org/19714002/diff/8001/remoting/protocol/protocol...
remoting/protocol/protocol_mock_objects.cc:89: if (!load_callback_.is_null()) {
You only need one if..elif..else.. chain here. In my previous comment I wrote
ellipses (...) as placeholders intending for you to fill in the code that
actually runs the callbacks. Sorry for not being clearer :)

[Jamie, 2013-07-19 02:10:13]

fyi

https://codereview.chromium.org/19714002/diff/8001/remoting/protocol/pairing_...
File remoting/protocol/pairing_registry.cc (right):

https://codereview.chromium.org/19714002/diff/8001/remoting/protocol/pairing_...
remoting/protocol/pairing_registry.cc:278: request.Run();
On 2013/07/19 01:54:21, Lambros wrote:
> |request| temporary not needed anymore?

I think it's a bit clearer than running it together on one line.

https://codereview.chromium.org/19714002/diff/8001/remoting/protocol/protocol...
File remoting/protocol/protocol_mock_objects.cc (right):

https://codereview.chromium.org/19714002/diff/8001/remoting/protocol/protocol...
remoting/protocol/protocol_mock_objects.cc:89: if (!load_callback_.is_null()) {
On 2013/07/19 01:54:21, Lambros wrote:
> You only need one if..elif..else.. chain here. In my previous comment I wrote
> ellipses (...) as placeholders intending for you to fill in the code that
> actually runs the callbacks. Sorry for not being clearer :)

Done.

[commit-bot: I haz the power, 2013-07-19 02:10:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/19714002/12002

[commit-bot: I haz the power, 2013-07-19 09:01:39]

Change committed as 212553