[EasyUnlock] Port Connection class to native code.

[EasyUnlock] Port Connection class to native code.

BUG=410639
TEST=components_unittests
R=tengs@chromium.org, cbentzel@chromium.org

Committed: https://crrev.com/06c6a9a72eaf2cc5fbcc429127bd2e3f4dfce75d
Cr-Commit-Position: refs/heads/master@{#294114}

[Ilya Sherman, 2014-09-04 01:21:12]

Chris, please stamp the DEPS addition.

Tim, PTAL at everything else :)

[cbentzel, 2014-09-04 02:12:08]

LGTM for the DEPS change.

[Tim Song, 2014-09-05 00:30:34]

https://codereview.chromium.org/538843002/diff/1/components/proximity_auth/co...
File components/proximity_auth/connection.cc (right):

https://codereview.chromium.org/538843002/diff/1/components/proximity_auth/co...
components/proximity_auth/connection.cc:76: is_sending_message_ = false;
move this line up in case an observer wants to send another message in response.

https://codereview.chromium.org/538843002/diff/1/components/proximity_auth/co...
components/proximity_auth/connection.cc:96: // TODO(tengs): Clear buffer after a
timeout as a guard.
We can remove this TODO. If the phone sends garbage to us, we probably have
bigger problems anyways.

https://codereview.chromium.org/538843002/diff/1/components/proximity_auth/co...
File components/proximity_auth/connection.h (right):

https://codereview.chromium.org/538843002/diff/1/components/proximity_auth/co...
components/proximity_auth/connection.h:37: // Sends a message to the remote
device.
Also make a note that OnSendCompleted will be called for all observers upon
completion with success or failure.

https://codereview.chromium.org/538843002/diff/1/components/proximity_auth/co...
components/proximity_auth/connection.h:70: void
OnBytesReceived(scoped_refptr<net::IOBuffer> io_buffer,
Use vectors for the buffers so we don't have to think about the length.

https://codereview.chromium.org/538843002/diff/1/components/proximity_auth/co...
components/proximity_auth/connection.h:74: virtual void
CancelConnectionAttempt() = 0;
This can be public or just delete it since we don't actually use it anywhere in
the app.

https://codereview.chromium.org/538843002/diff/1/components/proximity_auth/co...
components/proximity_auth/connection.h:87: virtual scoped_ptr<PermitMessage>
DeserializePermitMessage();
Are these functions temporarily exposed until the permit message is implemented?
It doesn't seem like they really need to be mocked.

https://codereview.chromium.org/538843002/diff/1/components/proximity_auth/co...
File components/proximity_auth/connection_observer.h (right):

https://codereview.chromium.org/538843002/diff/1/components/proximity_auth/co...
components/proximity_auth/connection_observer.h:26: const PermitMessage&
message) = 0;
Mention that |message| shouldn't be kept around (eg. for binding to a posted
task). 

Do we want to use a scoped_refptr in Connection instead to avoid these potential
problems?

https://codereview.chromium.org/538843002/diff/1/components/proximity_auth/co...
File components/proximity_auth/connection_unittest.cc (right):

https://codereview.chromium.org/538843002/diff/1/components/proximity_auth/co...
components/proximity_auth/connection_unittest.cc:83: TEST(ConnectionTest,
IsConnected) {
Prefix this test name with "ProximityAuth" or something similar so we can have a
gtest_filter that can run all the unit tests.

https://codereview.chromium.org/538843002/diff/1/components/proximity_auth/pe...
File components/proximity_auth/permit_message.cc (right):

https://codereview.chromium.org/538843002/diff/1/components/proximity_auth/pe...
components/proximity_auth/permit_message.cc:63: scoped_ptr<PermitMessage>
PermitMessage::FromBytesImpl(const char* bytes,
Why not put this in the namespace above?

https://codereview.chromium.org/538843002/diff/1/components/proximity_auth/pe...
File components/proximity_auth/permit_message.h (right):

https://codereview.chromium.org/538843002/diff/1/components/proximity_auth/pe...
components/proximity_auth/permit_message.h:13: class PermitMessage {
WireMessage would be a better name for this class.

https://codereview.chromium.org/538843002/diff/1/components/proximity_auth/pe...
components/proximity_auth/permit_message.h:19: static bool
IsCompleteMessage(const char* bytes, int size);
Let's use a vector<uint8_t> as the shared data type for byte buffers, so we can
avoid memory and addressing issues.

[Ilya Sherman, 2014-09-05 01:02:12]

https://codereview.chromium.org/538843002/diff/1/components/proximity_auth/co...
File components/proximity_auth/connection.cc (right):

https://codereview.chromium.org/538843002/diff/1/components/proximity_auth/co...
components/proximity_auth/connection.cc:76: is_sending_message_ = false;
On 2014/09/05 00:30:33, Tim Song wrote:
> move this line up in case an observer wants to send another message in
response.

Done, though that seems a little fragile, since observers can't be guaranteed
that someone else won't preempt them in sending the message.  Maybe
SendMessage() should return a boolean for success vs. failure?

https://codereview.chromium.org/538843002/diff/1/components/proximity_auth/co...
components/proximity_auth/connection.cc:96: // TODO(tengs): Clear buffer after a
timeout as a guard.
On 2014/09/05 00:30:33, Tim Song wrote:
> We can remove this TODO. If the phone sends garbage to us, we probably have
> bigger problems anyways.

Done.

https://codereview.chromium.org/538843002/diff/1/components/proximity_auth/co...
File components/proximity_auth/connection.h (right):

https://codereview.chromium.org/538843002/diff/1/components/proximity_auth/co...
components/proximity_auth/connection.h:37: // Sends a message to the remote
device.
On 2014/09/05 00:30:33, Tim Song wrote:
> Also make a note that OnSendCompleted will be called for all observers upon
> completion with success or failure.

Done.

https://codereview.chromium.org/538843002/diff/1/components/proximity_auth/co...
components/proximity_auth/connection.h:70: void
OnBytesReceived(scoped_refptr<net::IOBuffer> io_buffer,
On 2014/09/05 00:30:34, Tim Song wrote:
> Use vectors for the buffers so we don't have to think about the length.

Used strings (see also my response to your related comment for the wire
message).

https://codereview.chromium.org/538843002/diff/1/components/proximity_auth/co...
components/proximity_auth/connection.h:74: virtual void
CancelConnectionAttempt() = 0;
On 2014/09/05 00:30:34, Tim Song wrote:
> This can be public or just delete it since we don't actually use it anywhere
in
> the app.

Done.

https://codereview.chromium.org/538843002/diff/1/components/proximity_auth/co...
components/proximity_auth/connection.h:87: virtual scoped_ptr<PermitMessage>
DeserializePermitMessage();
On 2014/09/05 00:30:33, Tim Song wrote:
> Are these functions temporarily exposed until the permit message is
implemented?
> It doesn't seem like they really need to be mocked.

I'd prefer to test the connection code independently of the message parsing
code; so, no, I'm intending to leave these in place.

https://codereview.chromium.org/538843002/diff/1/components/proximity_auth/co...
File components/proximity_auth/connection_observer.h (right):

https://codereview.chromium.org/538843002/diff/1/components/proximity_auth/co...
components/proximity_auth/connection_observer.h:26: const PermitMessage&
message) = 0;
On 2014/09/05 00:30:34, Tim Song wrote:
> Mention that |message| shouldn't be kept around (eg. for binding to a posted
> task). 
> 
> Do we want to use a scoped_refptr in Connection instead to avoid these
potential
> problems?

I'm not sure what the concern is.  It's pretty much never safe to assume that
you can save a local reference to a variable passed in by const-ref.  On the
other hand, if the observer makes a copy of the message, then what's wrong with
keeping it around?

I'm quite opposed to using reference counting unless we have to, as it makes
memory ownership quite hard to keep track of.  Since wire messages should be
fairly cheap to copy, I'd prefer to just copy the data when necessary.

https://codereview.chromium.org/538843002/diff/1/components/proximity_auth/co...
File components/proximity_auth/connection_unittest.cc (right):

https://codereview.chromium.org/538843002/diff/1/components/proximity_auth/co...
components/proximity_auth/connection_unittest.cc:83: TEST(ConnectionTest,
IsConnected) {
On 2014/09/05 00:30:34, Tim Song wrote:
> Prefix this test name with "ProximityAuth" or something similar so we can have
a
> gtest_filter that can run all the unit tests.

Done.

https://codereview.chromium.org/538843002/diff/1/components/proximity_auth/pe...
File components/proximity_auth/permit_message.cc (right):

https://codereview.chromium.org/538843002/diff/1/components/proximity_auth/pe...
components/proximity_auth/permit_message.cc:63: scoped_ptr<PermitMessage>
PermitMessage::FromBytesImpl(const char* bytes,
On 2014/09/05 00:30:34, Tim Song wrote:
> Why not put this in the namespace above?

I've since decided that this method isn't needed; dropping it instead.

https://codereview.chromium.org/538843002/diff/1/components/proximity_auth/pe...
File components/proximity_auth/permit_message.h (right):

https://codereview.chromium.org/538843002/diff/1/components/proximity_auth/pe...
components/proximity_auth/permit_message.h:13: class PermitMessage {
On 2014/09/05 00:30:34, Tim Song wrote:
> WireMessage would be a better name for this class.

Done.

https://codereview.chromium.org/538843002/diff/1/components/proximity_auth/pe...
components/proximity_auth/permit_message.h:19: static bool
IsCompleteMessage(const char* bytes, int size);
On 2014/09/05 00:30:34, Tim Song wrote:
> Let's use a vector<uint8_t> as the shared data type for byte buffers, so we
can
> avoid memory and addressing issues.

It looks like I was mistaken about how strings handle null terminators -- they
do not treat them as a special character -- so I'll use strings instead if
you're happy with that.

[Tim Song, 2014-09-05 03:10:34]

LGTM

https://codereview.chromium.org/538843002/diff/1/components/proximity_auth/co...
File components/proximity_auth/connection.cc (right):

https://codereview.chromium.org/538843002/diff/1/components/proximity_auth/co...
components/proximity_auth/connection.cc:76: is_sending_message_ = false;
On 2014/09/05 01:02:11, Ilya Sherman wrote:
> On 2014/09/05 00:30:33, Tim Song wrote:
> > move this line up in case an observer wants to send another message in
> response.
> 
> Done, though that seems a little fragile, since observers can't be guaranteed
> that someone else won't preempt them in sending the message.  Maybe
> SendMessage() should return a boolean for success vs. failure?

Alternative, expose a is_sending_message() function.

https://codereview.chromium.org/538843002/diff/1/components/proximity_auth/co...
File components/proximity_auth/connection_observer.h (right):

https://codereview.chromium.org/538843002/diff/1/components/proximity_auth/co...
components/proximity_auth/connection_observer.h:26: const PermitMessage&
message) = 0;
On 2014/09/05 01:02:11, Ilya Sherman wrote:
> On 2014/09/05 00:30:34, Tim Song wrote:
> > Mention that |message| shouldn't be kept around (eg. for binding to a posted
> > task). 
> > 
> > Do we want to use a scoped_refptr in Connection instead to avoid these
> potential
> > problems?
> 
> I'm not sure what the concern is.  It's pretty much never safe to assume that
> you can save a local reference to a variable passed in by const-ref.  On the
> other hand, if the observer makes a copy of the message, then what's wrong
with
> keeping it around?
> 
> I'm quite opposed to using reference counting unless we have to, as it makes
> memory ownership quite hard to keep track of.  Since wire messages should be
> fairly cheap to copy, I'd prefer to just copy the data when necessary.

It's easy to make such a mistake with Bind, since you're not explicitly storing
the reference. The SecureMessage decryption is asynchronous, so we have to be
careful not to bind members of the reference to the WireMessage. I suppose a
comment won't really help you avoid that mistake though.

https://codereview.chromium.org/538843002/diff/1/components/proximity_auth/pe...
File components/proximity_auth/permit_message.h (right):

https://codereview.chromium.org/538843002/diff/1/components/proximity_auth/pe...
components/proximity_auth/permit_message.h:19: static bool
IsCompleteMessage(const char* bytes, int size);
On 2014/09/05 01:02:12, Ilya Sherman wrote:
> On 2014/09/05 00:30:34, Tim Song wrote:
> > Let's use a vector<uint8_t> as the shared data type for byte buffers, so we
> can
> > avoid memory and addressing issues.
> 
> It looks like I was mistaken about how strings handle null terminators -- they
> do not treat them as a special character -- so I'll use strings instead if
> you're happy with that.

Strings are good.

[Ilya Sherman, 2014-09-09 22:18:01]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-09-09 22:19:38]

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

[commit-bot: I haz the power, 2014-09-09 22:25:02]

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

[commit-bot: I haz the power, 2014-09-09 22:25:03]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/53671)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Ilya Sherman, 2014-09-09 22:53:25]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-09-09 23:06:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/isherman@chromium.org/538843002/40001

[Ilya Sherman, 2014-09-09 23:41:00]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-09-09 23:51:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/isherman@chromium.org/538843002/60001

[commit-bot: I haz the power, 2014-09-10 00:31:59]

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

[commit-bot: I haz the power, 2014-09-10 00:32:00]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ilya Sherman, 2014-09-10 00:36:49]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-09-10 00:43:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/isherman@chromium.org/538843002/60001

[commit-bot: I haz the power, 2014-09-10 01:48:10]

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

[commit-bot: I haz the power, 2014-09-10 01:48:11]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ilya Sherman, 2014-09-10 01:57:00]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-09-10 01:59:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/isherman@chromium.org/538843002/80001

[commit-bot: I haz the power, 2014-09-10 02:43:00]

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

[commit-bot: I haz the power, 2014-09-10 02:43:01]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Ilya Sherman, 2014-09-10 04:23:19]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-09-10 04:24:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/isherman@chromium.org/538843002/100001

[commit-bot: I haz the power, 2014-09-10 04:33:54]

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

[commit-bot: I haz the power, 2014-09-10 04:33:54]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)

[Ilya Sherman, 2014-09-10 04:46:31]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-09-10 04:48:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/isherman@chromium.org/538843002/100001

[commit-bot: I haz the power, 2014-09-10 05:49:23]

Committed patchset #6 (id:100001) as 9454034127efa9cd94aaa15f4cb331f55c728326

[commit-bot: I haz the power, 2014-09-10 05:51:42]

Patchset 6 (id:??) landed as
https://crrev.com/06c6a9a72eaf2cc5fbcc429127bd2e3f4dfce75d
Cr-Commit-Position: refs/heads/master@{#294114}