Add rlz_utils as a new build target to rlz/

Add rlz_utils as a new build target to rlz/

This splits constants and some of the utilities into a smaller target
that can be depended on its own in rlz.

Adds net_response_check.cc and h to split isPingResponseValid, since
this is also an independently usable convenience check. The new target
contains all enums, constants and utilities.

BUG=713956
Review-Url: https://codereview.chromium.org/2857343004
Cr-Commit-Position: refs/heads/master@{#469770}
Committed: https://chromium.googlesource.com/chromium/src/+/935ffc18282202b8b51036252ecfb9a4001dae2a

[Yusuf, 2017-05-04 22:16:58]

yusufo@chromium.org changed reviewers:
+ rogerta@chromium.org

[Yusuf, 2017-05-04 22:17:06]

The CQ bit was checked by yusufo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 22:17:27]

Dry run: 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-04 22:23:06]

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

[commit-bot: I haz the power, 2017-05-04 22:23:06]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Yusuf, 2017-05-04 22:31:03]

The CQ bit was checked by yusufo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 22:32:39]

Dry run: 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-04 22:42:07]

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

[commit-bot: I haz the power, 2017-05-04 22:42:08]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Yusuf, 2017-05-04 22:51:34]

The CQ bit was checked by yusufo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 22:53:01]

Dry run: 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-04 23:03:33]

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

[commit-bot: I haz the power, 2017-05-04 23:03:34]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Yusuf, 2017-05-04 23:21:49]

The CQ bit was checked by yusufo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 23:22:28]

Dry run: CQ is trying da patch.

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

[Yusuf, 2017-05-05 00:02:02]

The CQ bit was checked by yusufo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 00:02:57]

Dry run: 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-05 01:42:56]

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

[commit-bot: I haz the power, 2017-05-05 01:42:57]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Yusuf, 2017-05-05 05:06:48]

The CQ bit was checked by yusufo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 05:07:16]

Dry run: 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-05 06:30:13]

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

[commit-bot: I haz the power, 2017-05-05 06:30:14]

Dry run: This issue passed the CQ dry run.

[Roger Tawa OOO till Jul 10th, 2017-05-05 15:31:29]

Looks fine Yusuf.  A few comments below.  Thanks.

https://codereview.chromium.org/2857343004/diff/100001/rlz/BUILD.gn
File rlz/BUILD.gn (right):

https://codereview.chromium.org/2857343004/diff/100001/rlz/BUILD.gn#newcode60
rlz/BUILD.gn:60: "//url",
Do we still need all these deps here?

https://codereview.chromium.org/2857343004/diff/100001/rlz/lib/net_response_c...
File rlz/lib/net_response_check.h (right):

https://codereview.chromium.org/2857343004/diff/100001/rlz/lib/net_response_c...
rlz/lib/net_response_check.h:19: // is the CRC-32 checksum of the message uptil
the checksum. If
Nit: uptil --> "up to", or "up till"

https://codereview.chromium.org/2857343004/diff/100001/rlz/lib/rlz_enums.h
File rlz/lib/rlz_enums.h (right):

https://codereview.chromium.org/2857343004/diff/100001/rlz/lib/rlz_enums.h#ne...
rlz/lib/rlz_enums.h:13: 
I see why you did this, but it is sub optimal.  This file does not need
RLZ_LIB_API, and the files that do (rlz_lib.h and net_response_check.h) depend
on including this file.

I think I'd rather see this duplicated in both other headers than included here,
like patchset 5.  (I'm not sure why the rlz api needs to be __cdecl).  If you
really don't want duplication, then let's create a new header file.

https://codereview.chromium.org/2857343004/diff/100001/rlz/lib/rlz_enums.h#ne...
rlz/lib/rlz_enums.h:139: LAST_EVENT
Do you plan on adding new events in a follow up CL?

[Yusuf, 2017-05-05 17:14:58]

https://codereview.chromium.org/2857343004/diff/100001/rlz/BUILD.gn
File rlz/BUILD.gn (right):

https://codereview.chromium.org/2857343004/diff/100001/rlz/BUILD.gn#newcode60
rlz/BUILD.gn:60: "//url",
On 2017/05/05 15:31:28, Roger Tawa wrote:
> Do we still need all these deps here?

We dont need zlib, but financial_ping.cc ends up using the others.

Took that one out.

https://codereview.chromium.org/2857343004/diff/100001/rlz/lib/net_response_c...
File rlz/lib/net_response_check.h (right):

https://codereview.chromium.org/2857343004/diff/100001/rlz/lib/net_response_c...
rlz/lib/net_response_check.h:19: // is the CRC-32 checksum of the message uptil
the checksum. If
On 2017/05/05 15:31:29, Roger Tawa wrote:
> Nit: uptil --> "up to", or "up till"

Done.

https://codereview.chromium.org/2857343004/diff/100001/rlz/lib/rlz_enums.h
File rlz/lib/rlz_enums.h (right):

https://codereview.chromium.org/2857343004/diff/100001/rlz/lib/rlz_enums.h#ne...
rlz/lib/rlz_enums.h:13: 
On 2017/05/05 15:31:29, Roger Tawa wrote:
> I see why you did this, but it is sub optimal.  This file does not need
> RLZ_LIB_API, and the files that do (rlz_lib.h and net_response_check.h) depend
> on including this file.
> 
> I think I'd rather see this duplicated in both other headers than included
here,
> like patchset 5.  (I'm not sure why the rlz api needs to be __cdecl).  If you
> really don't want duplication, then let's create a new header file.

Got it.

I had them duplicated, then win_clang builder got upset about the duplication. I
will add a new header file.

https://codereview.chromium.org/2857343004/diff/100001/rlz/lib/rlz_enums.h#ne...
rlz/lib/rlz_enums.h:139: LAST_EVENT
On 2017/05/05 15:31:29, Roger Tawa wrote:
> Do you plan on adding new events in a follow up CL?

I may. Not sure on that part since they will be given by the rlz team to us. I
think we may end up overriding some of these with a new custom brand.

[Yusuf, 2017-05-05 17:15:02]

The CQ bit was checked by yusufo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 17:15:39]

Dry run: 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-05 17:20:37]

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

[commit-bot: I haz the power, 2017-05-05 17:20:38]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Yusuf, 2017-05-05 17:37:55]

The CQ bit was checked by yusufo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 17:38:17]

Dry run: 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-05 19:15:36]

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

[commit-bot: I haz the power, 2017-05-05 19:15:37]

Dry run: This issue passed the CQ dry run.

[Roger Tawa OOO till Jul 10th, 2017-05-05 21:12:32]

lgtm

Thanks Yusuf

[Yusuf, 2017-05-05 21:15:56]

The CQ bit was checked by yusufo@chromium.org

[commit-bot: I haz the power, 2017-05-05 21:16:53]

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-05 21:25:18]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1494018956536040,
"parent_rev": "518c4b4529b584de845f0ec9afca90bab8021eae", "commit_rev":
"935ffc18282202b8b51036252ecfb9a4001dae2a"}

[commit-bot: I haz the power, 2017-05-05 21:26:24]

Description was changed from

==========
Add rlz_utils as a new build target to rlz/

This splits constants and some of the utilities into a smaller target
that can be depended on its own in rlz.

Adds net_response_check.cc and h to split isPingResponseValid, since
this is also an independently usable convenience check. The new target
contains all enums, constants and utilities.

BUG=713956
==========

to

==========
Add rlz_utils as a new build target to rlz/

This splits constants and some of the utilities into a smaller target
that can be depended on its own in rlz.

Adds net_response_check.cc and h to split isPingResponseValid, since
this is also an independently usable convenience check. The new target
contains all enums, constants and utilities.

BUG=713956

Review-Url: https://codereview.chromium.org/2857343004
Cr-Commit-Position: refs/heads/master@{#469770}
Committed:
https://chromium.googlesource.com/chromium/src/+/935ffc18282202b8b51036252ecf...
==========

[commit-bot: I haz the power, 2017-05-05 21:26:25]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/935ffc18282202b8b51036252ecf...