UKM: Added support for navigation sources

UKM: Added support for navigation sources

This adds functionality for adding navigation entries as sources for UKM,
as well as unittesting utilities.

Split out from https://codereview.chromium.org/2617883004/

R=holte@chromium.org,asvitkine@chromium.org
BUG=684673
Review-Url: https://codereview.chromium.org/2649303004
Cr-Commit-Position: refs/heads/master@{#446833}
Committed: https://chromium.googlesource.com/chromium/src/+/f8206ba1cdddee202d83e2c468aef524da8ede4b

[oystein (OOO til 10th of July), 2017-01-24 21:23:40]

Description was changed from

==========
UKM: Added support for sources

This adds functionality for adding navigation entries as sources for UKM,
as well as unittesting utilities.

R=holte@chromium.org
BUG=684673
==========

to

==========
UKM: Added support for sources

This adds functionality for adding navigation entries as sources for UKM,
as well as unittesting utilities.

Split out from https://codereview.chromium.org/2617883004/

R=holte@chromium.org
BUG=684673
==========

[oystein (OOO til 10th of July), 2017-01-24 21:24:00]

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

[oystein (OOO til 10th of July), 2017-01-24 21:24:16]

ptal! Split this out from the big CL.

[commit-bot: I haz the power, 2017-01-24 21:24:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[oystein (OOO til 10th of July), 2017-01-24 21:25:20]

Description was changed from

==========
UKM: Added support for sources

This adds functionality for adding navigation entries as sources for UKM,
as well as unittesting utilities.

Split out from https://codereview.chromium.org/2617883004/

R=holte@chromium.org
BUG=684673
==========

to

==========
UKM: Added support for sources

This adds functionality for adding navigation entries as sources for UKM,
as well as unittesting utilities.

Split out from https://codereview.chromium.org/2617883004/

R=holte@chromium.org,asvitkine@chromium.org
BUG=684673
==========

[oystein (OOO til 10th of July), 2017-01-24 21:25:47]

oysteine@chromium.org changed reviewers:
+ asvitkine@chromium.org

[oystein (OOO til 10th of July), 2017-01-24 21:25:48]

+asvitkine as well

[oystein (OOO til 10th of July), 2017-01-24 21:26:01]

Description was changed from

==========
UKM: Added support for sources

This adds functionality for adding navigation entries as sources for UKM,
as well as unittesting utilities.

Split out from https://codereview.chromium.org/2617883004/

R=holte@chromium.org,asvitkine@chromium.org
BUG=684673
==========

to

==========
UKM: Added support for navigation sources

This adds functionality for adding navigation entries as sources for UKM,
as well as unittesting utilities.

Split out from https://codereview.chromium.org/2617883004/

R=holte@chromium.org,asvitkine@chromium.org
BUG=684673
==========

[commit-bot: I haz the power, 2017-01-24 21:28:22]

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

[commit-bot: I haz the power, 2017-01-24 21:28:23]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[oystein (OOO til 10th of July), 2017-01-24 21:51:00]

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

[commit-bot: I haz the power, 2017-01-24 21:51:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Steven Holte, 2017-01-24 21:55:42]

lgtm

https://codereview.chromium.org/2649303004/diff/1/components/ukm/ukm_service_...
File components/ukm/ukm_service_unittest.cc (right):

https://codereview.chromium.org/2649303004/diff/1/components/ukm/ukm_service_...
components/ukm/ukm_service_unittest.cc:17: #include
"third_party/zlib/google/compression_utils.h"
Need to add to DEPS?

https://codereview.chromium.org/2649303004/diff/1/components/ukm/ukm_service_...
components/ukm/ukm_service_unittest.cc:108: task_runner_->RunPendingTasks();
You probably don't need to RunPendingTasks and Purge here.

[oystein (OOO til 10th of July), 2017-01-24 22:09:56]

https://codereview.chromium.org/2649303004/diff/1/components/ukm/ukm_service_...
File components/ukm/ukm_service_unittest.cc (right):

https://codereview.chromium.org/2649303004/diff/1/components/ukm/ukm_service_...
components/ukm/ukm_service_unittest.cc:17: #include
"third_party/zlib/google/compression_utils.h"
On 2017/01/24 21:55:42, Steven Holte wrote:
> Need to add to DEPS?

Done.

https://codereview.chromium.org/2649303004/diff/1/components/ukm/ukm_service_...
components/ukm/ukm_service_unittest.cc:108: task_runner_->RunPendingTasks();
On 2017/01/24 21:55:42, Steven Holte wrote:
> You probably don't need to RunPendingTasks and Purge here.

Done.

[Alexei Svitkine (slow), 2017-01-25 15:35:05]

https://codereview.chromium.org/2649303004/diff/40001/components/metrics/prot...
File components/metrics/proto/ukm/source.proto (right):

https://codereview.chromium.org/2649303004/diff/40001/components/metrics/prot...
components/metrics/proto/ukm/source.proto:28: optional int64
first_contentful_paint_msec = 4;
Please land this change server-side first before landing this.

https://codereview.chromium.org/2649303004/diff/40001/components/ukm/test_ukm...
File components/ukm/test_ukm_service.h (right):

https://codereview.chromium.org/2649303004/diff/40001/components/ukm/test_ukm...
components/ukm/test_ukm_service.h:21: TestUkmService(PrefService* pref_service);
explicit

https://codereview.chromium.org/2649303004/diff/40001/components/ukm/test_ukm...
components/ukm/test_ukm_service.h:29: DISALLOW_COPY_AND_ASSIGN(TestUkmService);
Nit: Add an empty line above this.

https://codereview.chromium.org/2649303004/diff/40001/components/ukm/test_ukm...
components/ukm/test_ukm_service.h:39: TestUkmService* test_ukm_service() const {
return test_ukm_service_.get(); }
Return by const& since I assume it can't be null?

https://codereview.chromium.org/2649303004/diff/40001/components/ukm/test_ukm...
components/ukm/test_ukm_service.h:44: };
 Nit: DISALLOW_COPY_AND_ASSIGN()?

https://codereview.chromium.org/2649303004/diff/40001/components/ukm/ukm_serv...
File components/ukm/ukm_service.cc (right):

https://codereview.chromium.org/2649303004/diff/40001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:60: const size_t kMaxSourcesLength = 100;
Nit: Just kMaxSources

https://codereview.chromium.org/2649303004/diff/40001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:198: for (auto& source : sources_) {
Nit: const auto&

https://codereview.chromium.org/2649303004/diff/40001/components/ukm/ukm_serv...
File components/ukm/ukm_service.h (right):

https://codereview.chromium.org/2649303004/diff/40001/components/ukm/ukm_serv...
components/ukm/ukm_service.h:53: void RecordSource(std::unique_ptr<UkmSource>
source);
Add a comment about what this does. Mention the lifetime semantics. (Looks like
every log upload you clear the list of sources that were added, for example.)

https://codereview.chromium.org/2649303004/diff/40001/components/ukm/ukm_sour...
File components/ukm/ukm_source.h (right):

https://codereview.chromium.org/2649303004/diff/40001/components/ukm/ukm_sour...
components/ukm/ukm_source.h:38: const base::TimeDelta& first_contentful_paint) {
Nit: TimeDelta is meant to be passed by value. Same with Time above.

(See their headers for comments explaining this.)

https://codereview.chromium.org/2649303004/diff/40001/components/ukm/ukm_sour...
components/ukm/ukm_source.h:49: };
DISALLOW_COPY_AND_ASSIGN()?

[oystein (OOO til 10th of July), 2017-01-25 18:38:05]

Thanks! New version up.

https://codereview.chromium.org/2649303004/diff/40001/components/metrics/prot...
File components/metrics/proto/ukm/source.proto (right):

https://codereview.chromium.org/2649303004/diff/40001/components/metrics/prot...
components/metrics/proto/ukm/source.proto:28: optional int64
first_contentful_paint_msec = 4;
On 2017/01/25 15:35:04, Alexei Svitkine (slow) wrote:
> Please land this change server-side first before landing this.

Acknowledged.

https://codereview.chromium.org/2649303004/diff/40001/components/ukm/test_ukm...
File components/ukm/test_ukm_service.h (right):

https://codereview.chromium.org/2649303004/diff/40001/components/ukm/test_ukm...
components/ukm/test_ukm_service.h:21: TestUkmService(PrefService* pref_service);
On 2017/01/25 15:35:04, Alexei Svitkine (slow) wrote:
> explicit

Done.

https://codereview.chromium.org/2649303004/diff/40001/components/ukm/test_ukm...
components/ukm/test_ukm_service.h:29: DISALLOW_COPY_AND_ASSIGN(TestUkmService);
On 2017/01/25 15:35:04, Alexei Svitkine (slow) wrote:
> Nit: Add an empty line above this.

Done.

https://codereview.chromium.org/2649303004/diff/40001/components/ukm/test_ukm...
components/ukm/test_ukm_service.h:39: TestUkmService* test_ukm_service() const {
return test_ukm_service_.get(); }
On 2017/01/25 15:35:04, Alexei Svitkine (slow) wrote:
> Return by const& since I assume it can't be null?

Done.

https://codereview.chromium.org/2649303004/diff/40001/components/ukm/test_ukm...
components/ukm/test_ukm_service.h:44: };
On 2017/01/25 15:35:04, Alexei Svitkine (slow) wrote:
>  Nit: DISALLOW_COPY_AND_ASSIGN()?

Done.

https://codereview.chromium.org/2649303004/diff/40001/components/ukm/ukm_serv...
File components/ukm/ukm_service.cc (right):

https://codereview.chromium.org/2649303004/diff/40001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:60: const size_t kMaxSourcesLength = 100;
On 2017/01/25 15:35:04, Alexei Svitkine (slow) wrote:
> Nit: Just kMaxSources

Done.

https://codereview.chromium.org/2649303004/diff/40001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:198: for (auto& source : sources_) {
On 2017/01/25 15:35:04, Alexei Svitkine (slow) wrote:
> Nit: const auto&

Done.

https://codereview.chromium.org/2649303004/diff/40001/components/ukm/ukm_serv...
File components/ukm/ukm_service.h (right):

https://codereview.chromium.org/2649303004/diff/40001/components/ukm/ukm_serv...
components/ukm/ukm_service.h:53: void RecordSource(std::unique_ptr<UkmSource>
source);
On 2017/01/25 15:35:04, Alexei Svitkine (slow) wrote:
> Add a comment about what this does. Mention the lifetime semantics. (Looks
like
> every log upload you clear the list of sources that were added, for example.)

Done.

https://codereview.chromium.org/2649303004/diff/40001/components/ukm/ukm_sour...
File components/ukm/ukm_source.h (right):

https://codereview.chromium.org/2649303004/diff/40001/components/ukm/ukm_sour...
components/ukm/ukm_source.h:38: const base::TimeDelta& first_contentful_paint) {
On 2017/01/25 15:35:04, Alexei Svitkine (slow) wrote:
> Nit: TimeDelta is meant to be passed by value. Same with Time above.
> 
> (See their headers for comments explaining this.)

Done.

https://codereview.chromium.org/2649303004/diff/40001/components/ukm/ukm_sour...
components/ukm/ukm_source.h:49: };
On 2017/01/25 15:35:04, Alexei Svitkine (slow) wrote:
> DISALLOW_COPY_AND_ASSIGN()?

Done.

[oystein (OOO til 10th of July), 2017-01-25 18:38:12]

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

[commit-bot: I haz the power, 2017-01-25 18:38:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Alexei Svitkine (slow), 2017-01-25 18:40:45]

lgtm % remaining comment and waiting until the server-side proto lands

https://codereview.chromium.org/2649303004/diff/40001/components/ukm/ukm_serv...
File components/ukm/ukm_service.h (right):

https://codereview.chromium.org/2649303004/diff/40001/components/ukm/ukm_serv...
components/ukm/ukm_service.h:53: void RecordSource(std::unique_ptr<UkmSource>
source);
On 2017/01/25 18:38:04, oystein wrote:
> On 2017/01/25 15:35:04, Alexei Svitkine (slow) wrote:
> > Add a comment about what this does. Mention the lifetime semantics. (Looks
> like
> > every log upload you clear the list of sources that were added, for
example.)
> 
> Done.

Not done? I don't see the change.

[oystein (OOO til 10th of July), 2017-01-25 18:47:11]

On 2017/01/25 18:40:45, Alexei Svitkine (slow) wrote:
> lgtm % remaining comment and waiting until the server-side proto lands
> 
>
https://codereview.chromium.org/2649303004/diff/40001/components/ukm/ukm_serv...
> File components/ukm/ukm_service.h (right):
> 
>
https://codereview.chromium.org/2649303004/diff/40001/components/ukm/ukm_serv...
> components/ukm/ukm_service.h:53: void RecordSource(std::unique_ptr<UkmSource>
> source);
> On 2017/01/25 18:38:04, oystein wrote:
> > On 2017/01/25 15:35:04, Alexei Svitkine (slow) wrote:
> > > Add a comment about what this does. Mention the lifetime semantics. (Looks
> > like
> > > every log upload you clear the list of sources that were added, for
> example.)
> > 
> > Done.
> 
> Not done? I don't see the change.

Oops, my bad, misread the comment and documented the member and not the
function. Done.

[rkaplow, 2017-01-25 19:10:14]

rkaplow@chromium.org changed reviewers:
+ rkaplow@chromium.org

[rkaplow, 2017-01-25 19:10:15]

lgtm

https://codereview.chromium.org/2649303004/diff/80001/components/ukm/ukm_serv...
File components/ukm/ukm_service.cc (right):

https://codereview.chromium.org/2649303004/diff/80001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:59: // Maximum number of Sources we'll keep around
before dropping new ones.
can you note this will drop the newer sources instead of dropping oldest sources

https://codereview.chromium.org/2649303004/diff/80001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:198: for (const auto& source : sources_) {
can you add a UMA histogram for how many sources?

https://codereview.chromium.org/2649303004/diff/80001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:261: if (sources_.size() >= kMaxSources)
can you add a UMA histogram here?

[oystein (OOO til 10th of July), 2017-01-25 21:08:18]

Patchset #6 (id:100001) has been deleted

[oystein (OOO til 10th of July), 2017-01-25 21:09:56]

https://codereview.chromium.org/2649303004/diff/80001/components/ukm/ukm_serv...
File components/ukm/ukm_service.cc (right):

https://codereview.chromium.org/2649303004/diff/80001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:59: // Maximum number of Sources we'll keep around
before dropping new ones.
On 2017/01/25 19:10:14, rkaplow wrote:
> can you note this will drop the newer sources instead of dropping oldest
sources

Reworded to make this clearer.

https://codereview.chromium.org/2649303004/diff/80001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:198: for (const auto& source : sources_) {
On 2017/01/25 19:10:14, rkaplow wrote:
> can you add a UMA histogram for how many sources?

Done.

https://codereview.chromium.org/2649303004/diff/80001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:261: if (sources_.size() >= kMaxSources)
On 2017/01/25 19:10:14, rkaplow wrote:
> can you add a UMA histogram here?

Done.

[oystein (OOO til 10th of July), 2017-01-25 21:10:03]

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

[commit-bot: I haz the power, 2017-01-25 21:10:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rkaplow, 2017-01-25 21:30:06]

lgtm

https://codereview.chromium.org/2649303004/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2649303004/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:70305: +  <summary>Counter for number of
serialized UKM sources.</summary>
when storing a UKM log.

[oystein (OOO til 10th of July), 2017-01-27 22:07:53]

The CQ bit was checked by oysteine@chromium.org

[oystein (OOO til 10th of July), 2017-01-27 22:07:53]

The patchset sent to the CQ was uploaded after l-g-t-m from holte@chromium.org,
asvitkine@chromium.org, rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/2649303004/#ps140001
(title: "Histogram description")

[commit-bot: I haz the power, 2017-01-27 22:08:33]

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-01-27 23:44:26]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1485554873130560,
"parent_rev": "97f83d816742eef5677384925fd92d10eaa7dc9a", "commit_rev":
"f8206ba1cdddee202d83e2c468aef524da8ede4b"}

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

Description was changed from

==========
UKM: Added support for navigation sources

This adds functionality for adding navigation entries as sources for UKM,
as well as unittesting utilities.

Split out from https://codereview.chromium.org/2617883004/

R=holte@chromium.org,asvitkine@chromium.org
BUG=684673
==========

to

==========
UKM: Added support for navigation sources

This adds functionality for adding navigation entries as sources for UKM,
as well as unittesting utilities.

Split out from https://codereview.chromium.org/2617883004/

R=holte@chromium.org,asvitkine@chromium.org
BUG=684673

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

[commit-bot: I haz the power, 2017-01-27 23:45:02]

Committed patchset #7 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/f8206ba1cdddee202d83e2c468ae...

[oystein (OOO til 10th of July), 2017-01-28 00:02:20]

https://codereview.chromium.org/2649303004/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2649303004/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:70305: +  <summary>Counter for number of
serialized UKM sources.</summary>
On 2017/01/25 21:30:06, rkaplow wrote:
> when storing a UKM log.

Done.