[EasyUnlock] Move DataWithTimestamp out of ForegroundEidGenerator so it can be shared.

[EasyUnlock] Move DataWithTimestamp out of ForegroundEidGenerator so it can be shared.

This CL also fixes the log which coverts an EID to a hex string.

Review-Url: https://codereview.chromium.org/2847233003
Cr-Commit-Position: refs/heads/master@{#468485}
Committed: https://chromium.googlesource.com/chromium/src/+/a9983eadb02b514a528a14120090bf88794d24e8

[Tim Song, 2017-04-28 22:11:00]

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

[Kyle Horimoto, 2017-04-28 22:19:17]

https://codereview.chromium.org/2847233003/diff/1/components/cryptauth/backgr...
File components/cryptauth/background_eid_generator.cc (right):

https://codereview.chromium.org/2847233003/diff/1/components/cryptauth/backgr...
components/cryptauth/background_eid_generator.cc:49: ss << "\n  " <<
eid.start_timestamp_ms << ": " << eid.DataInHex() << ",";
nit: Can you add parentheses around each EidDataWithTimestamp for clarity to
make them look like tuples?

https://codereview.chromium.org/2847233003/diff/1/components/cryptauth/backgr...
components/cryptauth/background_eid_generator.cc:51: ss << "\n]";
nit: Remove the final comma.

https://codereview.chromium.org/2847233003/diff/1/components/cryptauth/eid_da...
File components/cryptauth/eid_data_with_timestamp.h (right):

https://codereview.chromium.org/2847233003/diff/1/components/cryptauth/eid_da...
components/cryptauth/eid_data_with_timestamp.h:14: struct EidDataWithTimestamp {
Can you change the name back to DataWithTimestamp? This type is used for for
advertisement service data and scan filters (which are not technically EIDs) as
well.

https://codereview.chromium.org/2847233003/diff/1/components/cryptauth/raw_ei...
File components/cryptauth/raw_eid_generator.h (right):

https://codereview.chromium.org/2847233003/diff/1/components/cryptauth/raw_ei...
components/cryptauth/raw_eid_generator.h:25: static std::string
EidToDebugString(const std::string& eid);
I think you forgot to upload the .cc file (or you just forgot to implement this
function).

https://codereview.chromium.org/2847233003/diff/1/components/cryptauth/raw_ei...
File components/cryptauth/raw_eid_generator_impl.cc (right):

https://codereview.chromium.org/2847233003/diff/1/components/cryptauth/raw_ei...
components/cryptauth/raw_eid_generator_impl.cc:7: #include <cstring>
Remove.

https://codereview.chromium.org/2847233003/diff/1/components/proximity_auth/b...
File
components/proximity_auth/bluetooth_low_energy_connection_finder_unittest.cc
(right):

https://codereview.chromium.org/2847233003/diff/1/components/proximity_auth/b...
components/proximity_auth/bluetooth_low_energy_connection_finder_unittest.cc:54:
const int64_t kEidPeriodMs = 60 * 1000 * 15;
nit: Use a constant kEidPeriodMinutes = 15 and use
base::TimeDelta::FromMinutes() when you use it.

[Tim Song, 2017-04-29 01:07:44]

Patchset #2 (id:20001) has been deleted

[Tim Song, 2017-04-29 01:07:51]

https://codereview.chromium.org/2847233003/diff/1/components/cryptauth/backgr...
File components/cryptauth/background_eid_generator.cc (right):

https://codereview.chromium.org/2847233003/diff/1/components/cryptauth/backgr...
components/cryptauth/background_eid_generator.cc:49: ss << "\n  " <<
eid.start_timestamp_ms << ": " << eid.DataInHex() << ",";
On 2017/04/28 22:19:16, Kyle Horimoto wrote:
> nit: Can you add parentheses around each EidDataWithTimestamp for clarity to
> make them look like tuples?

Done.

https://codereview.chromium.org/2847233003/diff/1/components/cryptauth/backgr...
components/cryptauth/background_eid_generator.cc:51: ss << "\n]";
On 2017/04/28 22:19:16, Kyle Horimoto wrote:
> nit: Remove the final comma.

Done.

https://codereview.chromium.org/2847233003/diff/1/components/cryptauth/eid_da...
File components/cryptauth/eid_data_with_timestamp.h (right):

https://codereview.chromium.org/2847233003/diff/1/components/cryptauth/eid_da...
components/cryptauth/eid_data_with_timestamp.h:14: struct EidDataWithTimestamp {
On 2017/04/28 22:19:16, Kyle Horimoto wrote:
> Can you change the name back to DataWithTimestamp? This type is used for for
> advertisement service data and scan filters (which are not technically EIDs)
as
> well.

Done.

https://codereview.chromium.org/2847233003/diff/1/components/cryptauth/raw_ei...
File components/cryptauth/raw_eid_generator.h (right):

https://codereview.chromium.org/2847233003/diff/1/components/cryptauth/raw_ei...
components/cryptauth/raw_eid_generator.h:25: static std::string
EidToDebugString(const std::string& eid);
On 2017/04/28 22:19:16, Kyle Horimoto wrote:
> I think you forgot to upload the .cc file (or you just forgot to implement
this
> function).

Sorry, I didn't mean for this file to be in the CL.

https://codereview.chromium.org/2847233003/diff/1/components/cryptauth/raw_ei...
File components/cryptauth/raw_eid_generator_impl.cc (right):

https://codereview.chromium.org/2847233003/diff/1/components/cryptauth/raw_ei...
components/cryptauth/raw_eid_generator_impl.cc:7: #include <cstring>
On 2017/04/28 22:19:17, Kyle Horimoto wrote:
> Remove.

Done.

https://codereview.chromium.org/2847233003/diff/1/components/proximity_auth/b...
File
components/proximity_auth/bluetooth_low_energy_connection_finder_unittest.cc
(right):

https://codereview.chromium.org/2847233003/diff/1/components/proximity_auth/b...
components/proximity_auth/bluetooth_low_energy_connection_finder_unittest.cc:54:
const int64_t kEidPeriodMs = 60 * 1000 * 15;
On 2017/04/28 22:19:17, Kyle Horimoto wrote:
> nit: Use a constant kEidPeriodMinutes = 15 and use
> base::TimeDelta::FromMinutes() when you use it.

It seems cleaner to use milliseconds directly as I would need to do this
conversion quite a few times in the code otherwise.

[Kyle Horimoto, 2017-04-30 03:17:49]

LGTM w/ nits

https://codereview.chromium.org/2847233003/diff/1/components/proximity_auth/b...
File
components/proximity_auth/bluetooth_low_energy_connection_finder_unittest.cc
(right):

https://codereview.chromium.org/2847233003/diff/1/components/proximity_auth/b...
components/proximity_auth/bluetooth_low_energy_connection_finder_unittest.cc:54:
const int64_t kEidPeriodMs = 60 * 1000 * 15;
On 2017/04/29 01:07:50, Tim Song wrote:
> On 2017/04/28 22:19:17, Kyle Horimoto wrote:
> > nit: Use a constant kEidPeriodMinutes = 15 and use
> > base::TimeDelta::FromMinutes() when you use it.
> 
> It seems cleaner to use milliseconds directly as I would need to do this
> conversion quite a few times in the code otherwise.

Alright, can you add a "15 minutes" comment then?

https://codereview.chromium.org/2847233003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.cc
(right):

https://codereview.chromium.org/2847233003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.cc:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
Did you mean to change this file as part of the CL? This seems orthogonal to the
purpose of this one - can you submit it separately?

Same with easy_unlock_service_signin_chromeos.cc.

https://codereview.chromium.org/2847233003/diff/40001/chromeos/components/tet...
File chromeos/components/tether/ble_advertiser_unittest.cc (right):

https://codereview.chromium.org/2847233003/diff/40001/chromeos/components/tet...
chromeos/components/tether/ble_advertiser_unittest.cc:30: const char
kFakePublicKey[] = "fakePublicKey";
Thanks :)

https://codereview.chromium.org/2847233003/diff/40001/components/cryptauth/ba...
File components/cryptauth/background_eid_generator.cc (right):

https://codereview.chromium.org/2847233003/diff/40001/components/cryptauth/ba...
components/cryptauth/background_eid_generator.cc:45: std::string
EidsToDebugString(const std::vector<DataWithTimestamp>& eids) {
Can you make this a static function on DataWithTimestamp instead?

https://codereview.chromium.org/2847233003/diff/40001/components/cryptauth/da...
File components/cryptauth/data_with_timestamp.h (right):

https://codereview.chromium.org/2847233003/diff/40001/components/cryptauth/da...
components/cryptauth/data_with_timestamp.h:26: const int64_t start_timestamp_ms;
nit: Can you add a comment saying that start is inclusive and end is exclusive?

https://codereview.chromium.org/2847233003/diff/40001/components/cryptauth/fo...
File components/cryptauth/foreground_eid_generator.h (right):

https://codereview.chromium.org/2847233003/diff/40001/components/cryptauth/fo...
components/cryptauth/foreground_eid_generator.h:73: // one for each EID seed
rotation period. If data has not been synced from the
Please revert the change.

[Tim Song, 2017-05-01 22:23:38]

https://codereview.chromium.org/2847233003/diff/1/components/proximity_auth/b...
File
components/proximity_auth/bluetooth_low_energy_connection_finder_unittest.cc
(right):

https://codereview.chromium.org/2847233003/diff/1/components/proximity_auth/b...
components/proximity_auth/bluetooth_low_energy_connection_finder_unittest.cc:54:
const int64_t kEidPeriodMs = 60 * 1000 * 15;
On 2017/04/30 03:17:42, Kyle Horimoto wrote:
> On 2017/04/29 01:07:50, Tim Song wrote:
> > On 2017/04/28 22:19:17, Kyle Horimoto wrote:
> > > nit: Use a constant kEidPeriodMinutes = 15 and use
> > > base::TimeDelta::FromMinutes() when you use it.
> > 
> > It seems cleaner to use milliseconds directly as I would need to do this
> > conversion quite a few times in the code otherwise.
> 
> Alright, can you add a "15 minutes" comment then?

Done.

https://codereview.chromium.org/2847233003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.cc
(right):

https://codereview.chromium.org/2847233003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.cc:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
On 2017/04/30 03:17:43, Kyle Horimoto wrote:
> Did you mean to change this file as part of the CL? This seems orthogonal to
the
> purpose of this one - can you submit it separately?
> 
> Same with easy_unlock_service_signin_chromeos.cc.

Done. Sorry, accidentally added this file.

https://codereview.chromium.org/2847233003/diff/40001/components/cryptauth/ba...
File components/cryptauth/background_eid_generator.cc (right):

https://codereview.chromium.org/2847233003/diff/40001/components/cryptauth/ba...
components/cryptauth/background_eid_generator.cc:45: std::string
EidsToDebugString(const std::vector<DataWithTimestamp>& eids) {
On 2017/04/30 03:17:43, Kyle Horimoto wrote:
> Can you make this a static function on DataWithTimestamp instead?

Done.

https://codereview.chromium.org/2847233003/diff/40001/components/cryptauth/da...
File components/cryptauth/data_with_timestamp.h (right):

https://codereview.chromium.org/2847233003/diff/40001/components/cryptauth/da...
components/cryptauth/data_with_timestamp.h:26: const int64_t start_timestamp_ms;
On 2017/04/30 03:17:44, Kyle Horimoto wrote:
> nit: Can you add a comment saying that start is inclusive and end is
exclusive?

Done.

https://codereview.chromium.org/2847233003/diff/40001/components/cryptauth/fo...
File components/cryptauth/foreground_eid_generator.h (right):

https://codereview.chromium.org/2847233003/diff/40001/components/cryptauth/fo...
components/cryptauth/foreground_eid_generator.h:73: // one for each EID seed
rotation period. If data has not been synced from the
On 2017/04/30 03:17:46, Kyle Horimoto wrote:
> Please revert the change.

Done.

[Tim Song, 2017-05-01 22:23:41]

The CQ bit was checked by tengs@chromium.org

[Tim Song, 2017-05-01 22:23:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
khorimoto@chromium.org
Link to the patchset: https://codereview.chromium.org/2847233003/#ps70017
(title: "fixes")

[commit-bot: I haz the power, 2017-05-01 22:24:18]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-01 23:39:14]

CQ is committing da patch.

Bot data: {"patchset_id": 70017, "attempt_start_ts": 1493677421966750,
"parent_rev": "9da565a52bf9c5becb762277ba2dd8a61c6915c6", "commit_rev":
"c7288c40df967fc96e93c1cfdb30440ea6935480"}

[commit-bot: I haz the power, 2017-05-01 23:39:33]

CQ is committing da patch.

Bot data: {"patchset_id": 70017, "attempt_start_ts": 1493677421966750,
"parent_rev": "69572327b573ca3df963d2a0d482064af289b1b5", "commit_rev":
"a9983eadb02b514a528a14120090bf88794d24e8"}

[commit-bot: I haz the power, 2017-05-01 23:39:44]

Description was changed from

==========
[EasyUnlock] Move DataWithTimestamp out of ForegroundEidGenerator so it can be
shared.

This CL also fixes the log which coverts an EID to a hex string.
==========

to

==========
[EasyUnlock] Move DataWithTimestamp out of ForegroundEidGenerator so it can be
shared.

This CL also fixes the log which coverts an EID to a hex string.

Review-Url: https://codereview.chromium.org/2847233003
Cr-Commit-Position: refs/heads/master@{#468485}
Committed:
https://chromium.googlesource.com/chromium/src/+/a9983eadb02b514a528a14120090...
==========

[commit-bot: I haz the power, 2017-05-01 23:39:45]

Committed patchset #4 (id:70017) as
https://chromium.googlesource.com/chromium/src/+/a9983eadb02b514a528a14120090...