Introduce proximity_auth component.

Introduce proximity_auth component.

This component handles the protocol between Chrome and an associated phone for
the Easy Unlock and Easy Sign-in features.

The system is currently implemented as a Chrome packaged app, and this is the
first step in migrating this functionality to native code. We will gradually
reimplement the system and switch from the app to this component once the
refactoring is complete.

BUG=385719
Committed: https://crrev.com/a72d976919a59d83ebe2883ff623050802a24488
Cr-Commit-Position: refs/heads/master@{#292704}

[Tim Song, 2014-08-27 01:32:25]

tengs@chromium.org changed reviewers:
+ isherman@chromium.org

[Ilya Sherman, 2014-08-27 03:18:34]

isherman@chromium.org changed reviewers:
+ blundell@chromium.org

[Ilya Sherman, 2014-08-27 03:18:34]

+The one, the only, Dr. Blundell.

I really think we should go ahead and introduce gyp files, and set ourselves as
the owners for them.  For the components_tests.gyp file, I think we should just
allow anyone to approve edits to that file.  Colin, WDYT?

https://codereview.chromium.org/505373004/diff/1/components/proximity_auth/pr...
File components/proximity_auth/proximity_auth_system.h (right):

https://codereview.chromium.org/505373004/diff/1/components/proximity_auth/pr...
components/proximity_auth/proximity_auth_system.h:15: // remote devices (ie.
phones), this object will handle the connection,
nit: "ie." -> "i.e."

https://codereview.chromium.org/505373004/diff/1/components/proximity_auth/pr...
File components/proximity_auth/proximity_auth_system_unittest.cc (right):

https://codereview.chromium.org/505373004/diff/1/components/proximity_auth/pr...
components/proximity_auth/proximity_auth_system_unittest.cc:21: EXPECT_EQ(2u,
returned_list.size());
nit: This should be an ASSERT_EQ, as otherwise the test could crash on the
following lines.

https://codereview.chromium.org/505373004/diff/1/components/proximity_auth/re...
File components/proximity_auth/remote_device.h (right):

https://codereview.chromium.org/505373004/diff/1/components/proximity_auth/re...
components/proximity_auth/remote_device.h:17: typedef std::vector<RemoteDevice>
RemoteDeviceList;
nit: I prefer spelling out the full type name to using typedefs, but we can use
typedefs in this component if you strongly prefer them.

[blundell, 2014-08-27 13:40:12]

On 2014/08/27 03:18:34, Ilya Sherman wrote:
> +The one, the only, Dr. Blundell.
> 
> I really think we should go ahead and introduce gyp files, and set ourselves
as
> the owners for them.  For the components_tests.gyp file, I think we should
just
> allow anyone to approve edits to that file.  Colin, WDYT?
> 

Do you mean introduce proximity_auth.gypi, or move from components having .gypi
files to them having their own .gyp files? For the former, I totally agree. For
the latter, I'd be really wary of slowdown on regenerating the project.

components_tests.gyp is already the way you state :), although it doesn't seem
like many people know that/take advantage of it.

>
https://codereview.chromium.org/505373004/diff/1/components/proximity_auth/pr...
> File components/proximity_auth/proximity_auth_system.h (right):
> 
>
https://codereview.chromium.org/505373004/diff/1/components/proximity_auth/pr...
> components/proximity_auth/proximity_auth_system.h:15: // remote devices (ie.
> phones), this object will handle the connection,
> nit: "ie." -> "i.e."
> 
>
https://codereview.chromium.org/505373004/diff/1/components/proximity_auth/pr...
> File components/proximity_auth/proximity_auth_system_unittest.cc (right):
> 
>
https://codereview.chromium.org/505373004/diff/1/components/proximity_auth/pr...
> components/proximity_auth/proximity_auth_system_unittest.cc:21: EXPECT_EQ(2u,
> returned_list.size());
> nit: This should be an ASSERT_EQ, as otherwise the test could crash on the
> following lines.
> 
>
https://codereview.chromium.org/505373004/diff/1/components/proximity_auth/re...
> File components/proximity_auth/remote_device.h (right):
> 
>
https://codereview.chromium.org/505373004/diff/1/components/proximity_auth/re...
> components/proximity_auth/remote_device.h:17: typedef
std::vector<RemoteDevice>
> RemoteDeviceList;
> nit: I prefer spelling out the full type name to using typedefs, but we can
use
> typedefs in this component if you strongly prefer them.

[blundell, 2014-08-27 13:41:58]

https://codereview.chromium.org/505373004/diff/1/components/proximity_auth/DEPS
File components/proximity_auth/DEPS (right):

https://codereview.chromium.org/505373004/diff/1/components/proximity_auth/DE...
components/proximity_auth/DEPS:2: "+base"
this isn't necessary (everything is allowed to depend on base).

https://codereview.chromium.org/505373004/diff/1/components/proximity_auth/OW...
File components/proximity_auth/OWNERS (right):

https://codereview.chromium.org/505373004/diff/1/components/proximity_auth/OW...
components/proximity_auth/OWNERS:2: isherman@chromium.org
nit: alphabetical order.

https://codereview.chromium.org/505373004/diff/1/components/proximity_auth/ru...
File components/proximity_auth/run_all_unittests.cc (right):

https://codereview.chromium.org/505373004/diff/1/components/proximity_auth/ru...
components/proximity_auth/run_all_unittests.cc:11: class ProximityAuthTestSuite
: public base::TestSuite {
This isn't necessary. Instead, you'll just add your unittests to
components_tests.gyp.

[Tim Song, 2014-08-27 19:38:31]

Patchset #3 (id:40001) has been deleted

[Tim Song, 2014-08-27 19:40:06]

https://codereview.chromium.org/505373004/diff/1/components/proximity_auth/DEPS
File components/proximity_auth/DEPS (right):

https://codereview.chromium.org/505373004/diff/1/components/proximity_auth/DE...
components/proximity_auth/DEPS:2: "+base"
On 2014/08/27 13:41:57, blundell wrote:
> this isn't necessary (everything is allowed to depend on base).

Done.

https://codereview.chromium.org/505373004/diff/1/components/proximity_auth/OW...
File components/proximity_auth/OWNERS (right):

https://codereview.chromium.org/505373004/diff/1/components/proximity_auth/OW...
components/proximity_auth/OWNERS:2: isherman@chromium.org
On 2014/08/27 13:41:58, blundell wrote:
> nit: alphabetical order.

Done.

https://codereview.chromium.org/505373004/diff/1/components/proximity_auth/pr...
File components/proximity_auth/proximity_auth_system.h (right):

https://codereview.chromium.org/505373004/diff/1/components/proximity_auth/pr...
components/proximity_auth/proximity_auth_system.h:15: // remote devices (ie.
phones), this object will handle the connection,
On 2014/08/27 03:18:33, Ilya Sherman wrote:
> nit: "ie." -> "i.e."

Done.

https://codereview.chromium.org/505373004/diff/1/components/proximity_auth/pr...
File components/proximity_auth/proximity_auth_system_unittest.cc (right):

https://codereview.chromium.org/505373004/diff/1/components/proximity_auth/pr...
components/proximity_auth/proximity_auth_system_unittest.cc:21: EXPECT_EQ(2u,
returned_list.size());
On 2014/08/27 03:18:33, Ilya Sherman wrote:
> nit: This should be an ASSERT_EQ, as otherwise the test could crash on the
> following lines.

Done.

https://codereview.chromium.org/505373004/diff/1/components/proximity_auth/re...
File components/proximity_auth/remote_device.h (right):

https://codereview.chromium.org/505373004/diff/1/components/proximity_auth/re...
components/proximity_auth/remote_device.h:17: typedef std::vector<RemoteDevice>
RemoteDeviceList;
On 2014/08/27 03:18:33, Ilya Sherman wrote:
> nit: I prefer spelling out the full type name to using typedefs, but we can
use
> typedefs in this component if you strongly prefer them.

Done.

https://codereview.chromium.org/505373004/diff/1/components/proximity_auth/ru...
File components/proximity_auth/run_all_unittests.cc (right):

https://codereview.chromium.org/505373004/diff/1/components/proximity_auth/ru...
components/proximity_auth/run_all_unittests.cc:11: class ProximityAuthTestSuite
: public base::TestSuite {
On 2014/08/27 13:41:58, blundell wrote:
> This isn't necessary. Instead, you'll just add your unittests to
> components_tests.gyp.

I would strongly like to have a separate executable for these tests for a couple
of reasons:

1. There will be over a dozen test suites, and it would be unwieldy to prefix
everything so they can easily be filtered.

2. components_unittests depends on a lot of libraries that we don't need (e.g.
WebKit), so the compile and link times are unnecessarily long.

I created a separate target in components_tests.gyp for these tests. Let me know
if you agree with this reasoning.

[Ilya Sherman, 2014-08-27 19:44:48]

https://codereview.chromium.org/505373004/diff/1/components/proximity_auth/ru...
File components/proximity_auth/run_all_unittests.cc (right):

https://codereview.chromium.org/505373004/diff/1/components/proximity_auth/ru...
components/proximity_auth/run_all_unittests.cc:11: class ProximityAuthTestSuite
: public base::TestSuite {
On 2014/08/27 19:40:06, Tim Song wrote:
> On 2014/08/27 13:41:58, blundell wrote:
> > This isn't necessary. Instead, you'll just add your unittests to
> > components_tests.gyp.
> 
> I would strongly like to have a separate executable for these tests for a
couple
> of reasons:
> 
> 1. There will be over a dozen test suites, and it would be unwieldy to prefix
> everything so they can easily be filtered.
> 
> 2. components_unittests depends on a lot of libraries that we don't need (e.g.
> WebKit), so the compile and link times are unnecessarily long.
> 
> I created a separate target in components_tests.gyp for these tests. Let me
know
> if you agree with this reasoning.

My understanding is that the difficulty with adding new test suites is that we
have to update all the bots to be aware of these test suites.

FWIW, I've generally found the compile and link times for components_unittests
to be fairly reasonable, though I agree that if we could have separate targets
without too much difficulty, that would be even better.

[blundell, 2014-08-28 07:49:37]

On 2014/08/27 19:44:48, Ilya Sherman wrote:
>
https://codereview.chromium.org/505373004/diff/1/components/proximity_auth/ru...
> File components/proximity_auth/run_all_unittests.cc (right):
> 
>
https://codereview.chromium.org/505373004/diff/1/components/proximity_auth/ru...
> components/proximity_auth/run_all_unittests.cc:11: class
ProximityAuthTestSuite
> : public base::TestSuite {
> On 2014/08/27 19:40:06, Tim Song wrote:
> > On 2014/08/27 13:41:58, blundell wrote:
> > > This isn't necessary. Instead, you'll just add your unittests to
> > > components_tests.gyp.
> > 
> > I would strongly like to have a separate executable for these tests for a
> couple
> > of reasons:
> > 
> > 1. There will be over a dozen test suites, and it would be unwieldy to
prefix
> > everything so they can easily be filtered.
> > 
> > 2. components_unittests depends on a lot of libraries that we don't need
(e.g.
> > WebKit), so the compile and link times are unnecessarily long.
> > 
> > I created a separate target in components_tests.gyp for these tests. Let me
> know
> > if you agree with this reasoning.
> 
> My understanding is that the difficulty with adding new test suites is that we
> have to update all the bots to be aware of these test suites.
> 
> FWIW, I've generally found the compile and link times for components_unittests
> to be fairly reasonable, though I agree that if we could have separate targets
> without too much difficulty, that would be even better.

components_unittests are quick to run as well as to compile and link. If you
want to be able to easily run only the proximity_auth-related unittests, then I
would indeed suggest prefixing the tests. TBH, I've never found
compiling/linking/running of components_unittests to be a pain point in all the
work I've done with components.

[Tim Song, 2014-08-28 19:01:53]

I moved the test to components_unittests, but I still feel that a separate
target is the right approach.

On my machine, it takes about 7 seconds to do an incremental build of
components_unittests, and 2 seconds for the separate target. This may not seem
like much, but for TDD, this can have a huge impact on iteration speed. Building
from scratch is an order of magnitude faster with a separate target.

[Ilya Sherman, 2014-08-28 19:09:30]

LGTM, thanks :)

[blundell, 2014-08-29 10:15:46]

LGTM with nits

As Ilya alluded to, it's not scalable from an infra POV to have each component
have a separate target for its unittests.

https://chromiumcodereview.appspot.com/505373004/diff/100001/components/proxi...
File components/proximity_auth.gypi (right):

https://chromiumcodereview.appspot.com/505373004/diff/100001/components/proxi...
components/proximity_auth.gypi:10: 'include_dirs': [
you should have a //base dependency here.

https://chromiumcodereview.appspot.com/505373004/diff/100001/components/proxi...
File components/proximity_auth/BUILD.gn (right):

https://chromiumcodereview.appspot.com/505373004/diff/100001/components/proxi...
components/proximity_auth/BUILD.gn:10: ]
and here.

https://chromiumcodereview.appspot.com/505373004/diff/100001/components/proxi...
File components/proximity_auth/DEPS (right):

https://chromiumcodereview.appspot.com/505373004/diff/100001/components/proxi...
components/proximity_auth/DEPS:1: include_rules = [
You can kill this file.

[Tim Song, 2014-08-29 18:20:38]

https://chromiumcodereview.appspot.com/505373004/diff/100001/components/proxi...
File components/proximity_auth.gypi (right):

https://chromiumcodereview.appspot.com/505373004/diff/100001/components/proxi...
components/proximity_auth.gypi:10: 'include_dirs': [
On 2014/08/29 10:15:46, blundell wrote:
> you should have a //base dependency here.

Done.

https://chromiumcodereview.appspot.com/505373004/diff/100001/components/proxi...
File components/proximity_auth/BUILD.gn (right):

https://chromiumcodereview.appspot.com/505373004/diff/100001/components/proxi...
components/proximity_auth/BUILD.gn:10: ]
On 2014/08/29 10:15:46, blundell wrote:
> and here.

Done.

https://chromiumcodereview.appspot.com/505373004/diff/100001/components/proxi...
File components/proximity_auth/DEPS (right):

https://chromiumcodereview.appspot.com/505373004/diff/100001/components/proxi...
components/proximity_auth/DEPS:1: include_rules = [
On 2014/08/29 10:15:46, blundell wrote:
> You can kill this file.

Done.

[Ilya Sherman, 2014-08-29 18:55:06]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-08-29 18:57:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tengs@chromium.org/505373004/160001

[commit-bot: I haz the power, 2014-08-29 22:57:08]

Committed patchset #8 (id:160001) as d5e2173a79ace6313deeceadcb8dae3d55708fcd

[commit-bot: I haz the power, 2014-09-10 03:11:18]

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