Refactored PairingRegistry::Delegate such that it can retrieve/modify for a single client.

Refactored PairingRegistry::Delegate such that it can retrieve/modify for a single client.

The delegate implementation for Linux now keeps a separate JSON file per each pairing under "paired-clients" in the config directory.

This CL also makes the delegate completely synchronous moving jumping between task runners to the parent PairingRegistry.

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

[alexeypa (please no reviews), 2013-07-29 23:44:49]

PTAL.

[Jamie, 2013-07-30 21:35:07]

https://chromiumcodereview.appspot.com/21128006/diff/1/remoting/host/pairing_...
File remoting/host/pairing_registry_delegate_linux.cc (right):

https://chromiumcodereview.appspot.com/21128006/diff/1/remoting/host/pairing_...
remoting/host/pairing_registry_delegate_linux.cc:23: const char
kRegistryDirectory[] = "pairing-registry";
I think paired-clients is sufficient for the directory name ("registry" doesn't
make it as clear what the contents are used for).

https://chromiumcodereview.appspot.com/21128006/diff/1/remoting/host/pairing_...
remoting/host/pairing_registry_delegate_linux.cc:156: "");
Would it make sense to make the extraction of the secret optional in
CreateFromJson? It would avoid duplicating it in every delegate.

https://chromiumcodereview.appspot.com/21128006/diff/1/remoting/host/pairing_...
remoting/host/pairing_registry_delegate_linux.cc:173:
base::DeleteFile(pairing_file, false);
DeleteFile can fail. I think we should try to continue deleting the other files
in this case, but pass false to the callback.

https://chromiumcodereview.appspot.com/21128006/diff/1/remoting/host/pairing_...
remoting/host/pairing_registry_delegate_linux.cc:250:
task_runner->PostTask(FROM_HERE, base::Bind(callback, success));
Would it be worth moving these Run*CallbackOnThread methods into the
PairingRegistry so that they don't need to be duplicated in all delegates?

https://chromiumcodereview.appspot.com/21128006/diff/1/remoting/host/pairing_...
File remoting/host/pairing_registry_delegate_linux_unittest.cc (right):

https://chromiumcodereview.appspot.com/21128006/diff/1/remoting/host/pairing_...
remoting/host/pairing_registry_delegate_linux_unittest.cc:26: // Load the
pairings again to make sure we get what we've just written.
s/pairings/pairing/

https://chromiumcodereview.appspot.com/21128006/diff/1/remoting/host/pairing_...
remoting/host/pairing_registry_delegate_linux_unittest.cc:60:
load_delegate->SetRegistryPathForTesting(temp_registry);
I think we need more unit tests now that the interface has more methods (and the
implementation is more complex). For example, we should verify LoadAllPairings
when we've just created the directory (should be empty), LoadPairing with an
unknown client id when the registry is not empty. At a minimum, there should be
a unit test per interface method.

If you want to get started on the Windows implementation, then it's fine to
leave this for now, and I'll implement them.

https://chromiumcodereview.appspot.com/21128006/diff/1/remoting/protocol/pair...
File remoting/protocol/pairing_registry.h (right):

https://chromiumcodereview.appspot.com/21128006/diff/1/remoting/protocol/pair...
remoting/protocol/pairing_registry.h:51: scoped_ptr<base::Value> EncodeJson()
const;
These methods don't deal with JSON. It might make sense to push the JSON
handling into them, or perhaps just rename them something like ToValue and
FromValue.

https://chromiumcodereview.appspot.com/21128006/diff/1/remoting/protocol/pair...
remoting/protocol/pairing_registry.h:89: // block.
Since you've removed the SanitizePairings method, I assume that that is now the
responsibility of the Delegate? If so, please add a comment to that effect.

https://chromiumcodereview.appspot.com/21128006/diff/1/remoting/protocol/pair...
File remoting/protocol/pairing_registry_unittest.cc (right):

https://chromiumcodereview.appspot.com/21128006/diff/1/remoting/protocol/pair...
remoting/protocol/pairing_registry_unittest.cc:86: PairingRegistry::Pairing
pairing_2 = registry->CreatePairing("my_client");
I think a lot of these DrainCallbacks are redundant. You only really need one at
the end of each test, perhaps followed by an appropriate EXPECT_EQ for
callback_count_. The only case where you might want more than one is to get a
better sense for where the test fails by using multiple EXPECT_EQ statements (as
we do in this test).

https://chromiumcodereview.appspot.com/21128006/diff/1/remoting/protocol/prot...
File remoting/protocol/protocol_mock_objects.cc (right):

https://chromiumcodereview.appspot.com/21128006/diff/1/remoting/protocol/prot...
remoting/protocol/protocol_mock_objects.cc:109: "");
The test that verifies that the shared secret is not returned is now only
testing this mock. I think it was better when the pairing registry took care of
that, meaning that it could be more easily unit tested.

https://chromiumcodereview.appspot.com/21128006/diff/1/remoting/protocol/prot...
remoting/protocol/protocol_mock_objects.cc:147: base::Closure callback =
pending_callback_;
Nit: From the point of view of limiting the scope of variables as much as
possible, this can be moved inside the loop, which can instead be conditional on
pending_callback_.

[alexeypa (please no reviews), 2013-07-31 21:31:24]

PTAL.

https://chromiumcodereview.appspot.com/21128006/diff/1/remoting/host/pairing_...
File remoting/host/pairing_registry_delegate_linux.cc (right):

https://chromiumcodereview.appspot.com/21128006/diff/1/remoting/host/pairing_...
remoting/host/pairing_registry_delegate_linux.cc:23: const char
kRegistryDirectory[] = "pairing-registry";
On 2013/07/30 21:35:07, Jamie wrote:
> I think paired-clients is sufficient for the directory name ("registry"
doesn't
> make it as clear what the contents are used for).

Done.

https://chromiumcodereview.appspot.com/21128006/diff/1/remoting/host/pairing_...
remoting/host/pairing_registry_delegate_linux.cc:156: "");
On 2013/07/30 21:35:07, Jamie wrote:
> Would it make sense to make the extraction of the secret optional in
> CreateFromJson? It would avoid duplicating it in every delegate.

Done.

https://chromiumcodereview.appspot.com/21128006/diff/1/remoting/host/pairing_...
remoting/host/pairing_registry_delegate_linux.cc:173:
base::DeleteFile(pairing_file, false);
On 2013/07/30 21:35:07, Jamie wrote:
> DeleteFile can fail. I think we should try to continue deleting the other
files
> in this case, but pass false to the callback.

Done.

https://chromiumcodereview.appspot.com/21128006/diff/1/remoting/host/pairing_...
remoting/host/pairing_registry_delegate_linux.cc:250:
task_runner->PostTask(FROM_HERE, base::Bind(callback, success));
On 2013/07/30 21:35:07, Jamie wrote:
> Would it be worth moving these Run*CallbackOnThread methods into the
> PairingRegistry so that they don't need to be duplicated in all delegates?

I made PairingRegistry::Delegate a synchronous class and moved all threading to
PairingRegistry.

https://chromiumcodereview.appspot.com/21128006/diff/1/remoting/host/pairing_...
File remoting/host/pairing_registry_delegate_linux_unittest.cc (right):

https://chromiumcodereview.appspot.com/21128006/diff/1/remoting/host/pairing_...
remoting/host/pairing_registry_delegate_linux_unittest.cc:26: // Load the
pairings again to make sure we get what we've just written.
On 2013/07/30 21:35:07, Jamie wrote:
> s/pairings/pairing/

Done.

https://chromiumcodereview.appspot.com/21128006/diff/1/remoting/host/pairing_...
remoting/host/pairing_registry_delegate_linux_unittest.cc:60:
load_delegate->SetRegistryPathForTesting(temp_registry);
On 2013/07/30 21:35:07, Jamie wrote:
> I think we need more unit tests now that the interface has more methods (and
the
> implementation is more complex). For example, we should verify LoadAllPairings
> when we've just created the directory (should be empty), LoadPairing with an
> unknown client id when the registry is not empty. At a minimum, there should
be
> a unit test per interface method.
> 
> If you want to get started on the Windows implementation, then it's fine to
> leave this for now, and I'll implement them.

Done.

https://chromiumcodereview.appspot.com/21128006/diff/1/remoting/protocol/pair...
File remoting/protocol/pairing_registry.h (right):

https://chromiumcodereview.appspot.com/21128006/diff/1/remoting/protocol/pair...
remoting/protocol/pairing_registry.h:51: scoped_ptr<base::Value> EncodeJson()
const;
On 2013/07/30 21:35:07, Jamie wrote:
> These methods don't deal with JSON. It might make sense to push the JSON
> handling into them, or perhaps just rename them something like ToValue and
> FromValue.

Done.

https://chromiumcodereview.appspot.com/21128006/diff/1/remoting/protocol/pair...
remoting/protocol/pairing_registry.h:89: // block.
On 2013/07/30 21:35:07, Jamie wrote:
> Since you've removed the SanitizePairings method, I assume that that is now
the
> responsibility of the Delegate? If so, please add a comment to that effect.

I resurrected SanitizePairings().

https://chromiumcodereview.appspot.com/21128006/diff/1/remoting/protocol/pair...
File remoting/protocol/pairing_registry_unittest.cc (right):

https://chromiumcodereview.appspot.com/21128006/diff/1/remoting/protocol/pair...
remoting/protocol/pairing_registry_unittest.cc:86: PairingRegistry::Pairing
pairing_2 = registry->CreatePairing("my_client");
On 2013/07/30 21:35:07, Jamie wrote:
> I think a lot of these DrainCallbacks are redundant. You only really need one
at
> the end of each test, perhaps followed by an appropriate EXPECT_EQ for
> callback_count_. The only case where you might want more than one is to get a
> better sense for where the test fails by using multiple EXPECT_EQ statements
(as
> we do in this test).

Done.

https://chromiumcodereview.appspot.com/21128006/diff/1/remoting/protocol/prot...
File remoting/protocol/protocol_mock_objects.cc (right):

https://chromiumcodereview.appspot.com/21128006/diff/1/remoting/protocol/prot...
remoting/protocol/protocol_mock_objects.cc:109: "");
On 2013/07/30 21:35:07, Jamie wrote:
> The test that verifies that the shared secret is not returned is now only
> testing this mock. I think it was better when the pairing registry took care
of
> that, meaning that it could be more easily unit tested.

Done.

https://chromiumcodereview.appspot.com/21128006/diff/1/remoting/protocol/prot...
remoting/protocol/protocol_mock_objects.cc:147: base::Closure callback =
pending_callback_;
On 2013/07/30 21:35:07, Jamie wrote:
> Nit: From the point of view of limiting the scope of variables as much as
> possible, this can be moved inside the loop, which can instead be conditional
on
> pending_callback_.

Done.

[alexeypa (please no reviews), 2013-08-01 19:06:24]

ping

[Jamie, 2013-08-01 22:44:04]

LGTM with nits and one extra test request.

https://chromiumcodereview.appspot.com/21128006/diff/19001/remoting/protocol/...
File remoting/protocol/pairing_registry.cc (right):

https://chromiumcodereview.appspot.com/21128006/diff/19001/remoting/protocol/...
remoting/protocol/pairing_registry.cc:267: LOG(WARNING) << "A pairing entry is
not a disctionary.";
s/disctionary/dictionary/

https://chromiumcodereview.appspot.com/21128006/diff/19001/remoting/protocol/...
File remoting/protocol/pairing_registry.h (right):

https://chromiumcodereview.appspot.com/21128006/diff/19001/remoting/protocol/...
remoting/protocol/pairing_registry.h:94: // Deletes all pairings in persistent
storage. Must not block.
I think these are all allowed to block now, correct? If so, please update the
comment.

https://chromiumcodereview.appspot.com/21128006/diff/19001/remoting/protocol/...
remoting/protocol/pairing_registry.h:140: // Lets the tests to override task
posting to make all callbacks synchronous.
Nit: Either "Allows the tests to override..." or "Lets the tests override..."

https://chromiumcodereview.appspot.com/21128006/diff/27001/remoting/host/pair...
File remoting/host/pairing_registry_delegate.h (right):

https://chromiumcodereview.appspot.com/21128006/diff/27001/remoting/host/pair...
remoting/host/pairing_registry_delegate.h:23: // returned by
CreatePairingRegistryDelegate().
Please add something to this comment to explain what the task runner is used
for.

https://chromiumcodereview.appspot.com/21128006/diff/27001/remoting/host/pair...
File remoting/host/pairing_registry_delegate_linux_unittest.cc (right):

https://chromiumcodereview.appspot.com/21128006/diff/27001/remoting/host/pair...
remoting/host/pairing_registry_delegate_linux_unittest.cc:20: // subdirectory to
ensure that the AddPairing method creates the parent
s/AddPairing/Save/ (or LinuxPairingRegistryDelegate::Save)

https://chromiumcodereview.appspot.com/21128006/diff/27001/remoting/host/pair...
remoting/host/pairing_registry_delegate_linux_unittest.cc:44:
TEST_F(PairingRegistryDelegateLinuxTest, Delete) {
Can you add a test for DeleteAll as well (since it's still a deletion test, you
can merge it into this one if you like).

[alexeypa (please no reviews), 2013-08-01 23:07:42]

https://chromiumcodereview.appspot.com/21128006/diff/19001/remoting/protocol/...
File remoting/protocol/pairing_registry.cc (right):

https://chromiumcodereview.appspot.com/21128006/diff/19001/remoting/protocol/...
remoting/protocol/pairing_registry.cc:267: LOG(WARNING) << "A pairing entry is
not a disctionary.";
On 2013/08/01 22:44:04, Jamie wrote:
> s/disctionary/dictionary/

Fixed by patch #4

https://chromiumcodereview.appspot.com/21128006/diff/19001/remoting/protocol/...
File remoting/protocol/pairing_registry.h (right):

https://chromiumcodereview.appspot.com/21128006/diff/19001/remoting/protocol/...
remoting/protocol/pairing_registry.h:94: // Deletes all pairings in persistent
storage. Must not block.
On 2013/08/01 22:44:04, Jamie wrote:
> I think these are all allowed to block now, correct? If so, please update the
> comment.

Done.

https://chromiumcodereview.appspot.com/21128006/diff/19001/remoting/protocol/...
remoting/protocol/pairing_registry.h:140: // Lets the tests to override task
posting to make all callbacks synchronous.
On 2013/08/01 22:44:04, Jamie wrote:
> Nit: Either "Allows the tests to override..." or "Lets the tests override..."

Done.

https://chromiumcodereview.appspot.com/21128006/diff/27001/remoting/host/pair...
File remoting/host/pairing_registry_delegate.h (right):

https://chromiumcodereview.appspot.com/21128006/diff/27001/remoting/host/pair...
remoting/host/pairing_registry_delegate.h:23: // returned by
CreatePairingRegistryDelegate().
On 2013/08/01 22:44:04, Jamie wrote:
> Please add something to this comment to explain what the task runner is used
> for.

Done.

https://chromiumcodereview.appspot.com/21128006/diff/27001/remoting/host/pair...
File remoting/host/pairing_registry_delegate_linux_unittest.cc (right):

https://chromiumcodereview.appspot.com/21128006/diff/27001/remoting/host/pair...
remoting/host/pairing_registry_delegate_linux_unittest.cc:20: // subdirectory to
ensure that the AddPairing method creates the parent
On 2013/08/01 22:44:04, Jamie wrote:
> s/AddPairing/Save/ (or LinuxPairingRegistryDelegate::Save)

Done.

https://chromiumcodereview.appspot.com/21128006/diff/27001/remoting/host/pair...
remoting/host/pairing_registry_delegate_linux_unittest.cc:44:
TEST_F(PairingRegistryDelegateLinuxTest, Delete) {
On 2013/08/01 22:44:04, Jamie wrote:
> Can you add a test for DeleteAll as well (since it's still a deletion test,
you
> can merge it into this one if you like).

Done.

[commit-bot: I haz the power, 2013-08-01 23:09:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/21128006/36002

[commit-bot: I haz the power, 2013-08-02 01:35:21]

Change committed as 215187