Domain Reliability: Simplify configs and reports

Domain Reliability: New config and report formats

This commit makes several intertwined changes:

1. Changes the config format to be much simpler, and incrementally
   closer to the ideas in the Navigation Error Logging spec.
   Specifically, removes resources and uses a list of safe path
   prefixes to serve the same purpose.

2. Removes the simplest configs and replaces them with an array of
   "config params" that are used to create params at runtime. This
   saves approximately 15,000 lines of source code and around a MB or
   two of binary size (on disk and at runtime).

3. Changes the report format to include full URLs when reporting to
   same-origin collectors, and partial URLs (based on the list of safe
   path prefixes) for non-same-origin collectors. Removes resource
   reports, since resources no longer exist, and makes a few other
   tweaks to the format.

4. Changes other parts of the internals of Domain Reliability to
   accommodate the above changes.

BUG=549137, 549138, 495342
Committed: https://crrev.com/ccdd6cc53cc09fcbe55c719508a49eaf6e9641ff
Cr-Commit-Position: refs/heads/master@{#361039}

[Deprecated (see juliatuttle), 2015-10-12 17:22:45]

ttuttle@chromium.org changed reviewers:
+ davidben@chromium.org

[Deprecated (see juliatuttle), 2015-10-12 17:23:16]

PTAL, davidben. :D

[davidben, 2015-10-20 23:35:54]

Only had time to do an initial pass on it before heading out. Sorry! :-( Feel
free to wrangle someone else to finish it if you like. Exciting to see this all
finally get pared down though.

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
File components/domain_reliability/beacon.h (right):

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
components/domain_reliability/beacon.h:27: struct DOMAIN_RELIABILITY_EXPORT
ReportParams {
Does this actually buy you anything over a four-argument ToValue method? Because
of the const references in the struct, the only way you can fill out this thing
is by calling the four-argument ReportParams constructor. So instead of

  base::Value* blah = blah->ToValue(blah, blah, blah, blah);

You write

  ReportParams report_params(blah, blah, blah, blah);
  base::Value* blah = blah->ToValue(report_params);

or

  base::Value* blah = blah->ToValue(ReportParams(blah, blah, blah, blah));

It's not clear to me this is any better.

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
components/domain_reliability/beacon.h:42: base::Value* ToValue(const
ReportParams& report_params) const;
Existing code, but this would probably be better returned as a scoped_ptr. (Just
checked and base::ListValue::Append has a scoped_ptr version nowadays.)

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
File components/domain_reliability/config.cc (right):

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
components/domain_reliability/config.cc:33: !url->has_password() &&
url->scheme() == "https" &&
Nit: SchemeIs("https"). scheme() actually makes a temporary string apparently.

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
components/domain_reliability/config.cc:34: url->path() == "/" &&
!url->has_query() && !url->has_ref();
Nit: path() -> path_piece() to avoid a temporary string.

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
File components/domain_reliability/context.cc (left):

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
components/domain_reliability/context.cc:39: const
DomainReliabilityConfig::Resource* config)
(I'm assuming you all decided this state was no longer useful to keep in the
report.)

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
File components/domain_reliability/context.h (right):

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
components/domain_reliability/context.h:66: std::vector<const
DomainReliabilityBeacon*>* beacons_out) const;
What is the lifetime story for these pointers? (Should have a comment probably.)

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
components/domain_reliability/context.h:77: typedef BeaconDeque::const_iterator
BeaconConstIterator;
Nit: I don't think BeaconConstIterator is used at all now with auto.
BeaconIterator still has two callers, but you may as well turn them into auto
now.

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
File components/domain_reliability/context_manager.cc (right):

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
components/domain_reliability/context_manager.cc:20: DomainReliabilityContext*
context = GetContextForHost(beacon->url.host());
How does contexts having an origin, but still having an include subdomains
notion, interact with this only paying attention to the host? It's still
ignoring the port, notably.

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
components/domain_reliability/context_manager.cc:37: // makes ASAN mad.
Merp. I don't suppose you remember what the failure was? That's confusing.

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
File components/domain_reliability/context_unittest.cc (left):

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
components/domain_reliability/context_unittest.cc:202:
TEST_F(DomainReliabilityContextTest, ReportUpload_NetworkChanged) {
What happened to this test? (I still see a network_changed field.)

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
File components/domain_reliability/context_unittest.cc (right):

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
components/domain_reliability/context_unittest.cc:137: // TODO(ttuttle): Update
string.
(What's this mean?)

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
File components/domain_reliability/google_configs.cc (right):

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
components/domain_reliability/google_configs.cc:538: return
scoped_ptr<DomainReliabilityConfig>();
This can just be return nullptr

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
components/domain_reliability/google_configs.cc:547: else {
Nit: curlies

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
File components/domain_reliability/monitor.cc (right):

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
components/domain_reliability/monitor.cc:173:
context_manager_.AddContextForConfig(google_configs.GetNextConfig());
Since you're calling AddContextForConfig which still holds onto the config at
the end of the day. Any reason you're doing this as an iterator and not just
having it fill out a std::vector for you to iterate over? That seems simpler.

[Deprecated (see juliatuttle), 2015-10-26 19:08:41]

PTAL.

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
File components/domain_reliability/beacon.h (right):

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
components/domain_reliability/beacon.h:27: struct DOMAIN_RELIABILITY_EXPORT
ReportParams {
On 2015/10/20 23:35:54, davidben (OOO 10-21 to 10-23) wrote:
> Does this actually buy you anything over a four-argument ToValue method?
Because
> of the const references in the struct, the only way you can fill out this
thing
> is by calling the four-argument ReportParams constructor. So instead of
> 
>   base::Value* blah = blah->ToValue(blah, blah, blah, blah);
> 
> You write
> 
>   ReportParams report_params(blah, blah, blah, blah);
>   base::Value* blah = blah->ToValue(report_params);
> 
> or
> 
>   base::Value* blah = blah->ToValue(ReportParams(blah, blah, blah, blah));
> 
> It's not clear to me this is any better.

Done.

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
components/domain_reliability/beacon.h:42: base::Value* ToValue(const
ReportParams& report_params) const;
On 2015/10/20 23:35:54, davidben (OOO 10-21 to 10-23) wrote:
> Existing code, but this would probably be better returned as a scoped_ptr.
(Just
> checked and base::ListValue::Append has a scoped_ptr version nowadays.)

Done.

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
File components/domain_reliability/config.cc (right):

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
components/domain_reliability/config.cc:33: !url->has_password() &&
url->scheme() == "https" &&
On 2015/10/20 23:35:54, davidben (OOO 10-21 to 10-23) wrote:
> Nit: SchemeIs("https"). scheme() actually makes a temporary string apparently.

Done.

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
components/domain_reliability/config.cc:34: url->path() == "/" &&
!url->has_query() && !url->has_ref();
On 2015/10/20 23:35:54, davidben (OOO 10-21 to 10-23) wrote:
> Nit: path() -> path_piece() to avoid a temporary string.

Done.

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
File components/domain_reliability/context.cc (left):

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
components/domain_reliability/context.cc:39: const
DomainReliabilityConfig::Resource* config)
On 2015/10/20 23:35:54, davidben (OOO 10-21 to 10-23) wrote:
> (I'm assuming you all decided this state was no longer useful to keep in the
> report.)

Yes, and it becomes meaningless once we don't have resources.

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
File components/domain_reliability/context.h (right):

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
components/domain_reliability/context.h:66: std::vector<const
DomainReliabilityBeacon*>* beacons_out) const;
On 2015/10/20 23:35:54, davidben (OOO 10-21 to 10-23) wrote:
> What is the lifetime story for these pointers? (Should have a comment
probably.)

Done.

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
components/domain_reliability/context.h:77: typedef BeaconDeque::const_iterator
BeaconConstIterator;
On 2015/10/20 23:35:54, davidben (OOO 10-21 to 10-23) wrote:
> Nit: I don't think BeaconConstIterator is used at all now with auto.
> BeaconIterator still has two callers, but you may as well turn them into auto
> now.

Done.

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
File components/domain_reliability/context_manager.cc (right):

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
components/domain_reliability/context_manager.cc:20: DomainReliabilityContext*
context = GetContextForHost(beacon->url.host());
On 2015/10/20 23:35:54, davidben (OOO 10-21 to 10-23) wrote:
> How does contexts having an origin, but still having an include subdomains
> notion, interact with this only paying attention to the host? It's still
> ignoring the port, notably.

We're still only paying attention to the host for now. I will make it more
actually-origin-like in the future, but I wanted to rev the format while I
could.

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
components/domain_reliability/context_manager.cc:37: // makes ASAN mad.
On 2015/10/20 23:35:54, davidben (OOO 10-21 to 10-23) wrote:
> Merp. I don't suppose you remember what the failure was? That's confusing.

I don't remember what it was off the top of my head, but I could replicate it
pretty easily on the trybots if you want me to add a comment.

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
File components/domain_reliability/context_unittest.cc (left):

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
components/domain_reliability/context_unittest.cc:202:
TEST_F(DomainReliabilityContextTest, ReportUpload_NetworkChanged) {
On 2015/10/20 23:35:54, davidben (OOO 10-21 to 10-23) wrote:
> What happened to this test? (I still see a network_changed field.)

I must've had a good reason for removing it at the time, but I see no good
reason now, so I'm adding it back.

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
File components/domain_reliability/context_unittest.cc (right):

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
components/domain_reliability/context_unittest.cc:137: // TODO(ttuttle): Update
string.
On 2015/10/20 23:35:54, davidben (OOO 10-21 to 10-23) wrote:
> (What's this mean?)

Whoops, must've left it from before.

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
File components/domain_reliability/google_configs.cc (right):

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
components/domain_reliability/google_configs.cc:538: return
scoped_ptr<DomainReliabilityConfig>();
On 2015/10/20 23:35:54, davidben (OOO 10-21 to 10-23) wrote:
> This can just be return nullptr

Done.

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
components/domain_reliability/google_configs.cc:547: else {
On 2015/10/20 23:35:54, davidben (OOO 10-21 to 10-23) wrote:
> Nit: curlies

Done.

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
File components/domain_reliability/monitor.cc (right):

https://codereview.chromium.org/1180223006/diff/160001/components/domain_reli...
components/domain_reliability/monitor.cc:173:
context_manager_.AddContextForConfig(google_configs.GetNextConfig());
On 2015/10/20 23:35:54, davidben (OOO 10-21 to 10-23) wrote:
> Since you're calling AddContextForConfig which still holds onto the config at
> the end of the day. Any reason you're doing this as an iterator and not just
> having it fill out a std::vector for you to iterate over? That seems simpler.

Yeah, it is.

What's the best way to convey a vector of scoped pointers? vector can't take a
scoped_ptr because there's no copy, and ScopedVector is scoped at the vector
level, not the element level (and doesn't have, like, "release this element as a
scoped_ptr", even though it has "insert this scoped_ptr as an element").

[Randy Smith (Not in Mondays), 2015-10-29 14:19:55]

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

[Randy Smith (Not in Mondays), 2015-10-29 14:19:57]

Two quick high level comments:
* Why can't #3 be done as a separate CL?
* I think this change should be attached to an issue.

[Deprecated (see juliatuttle), 2015-10-29 15:30:38]

Description was changed from

==========
Domain Reliability: New config and report formats

This commit makes several intertwined changes:

1. Changes the config format to be much simpler, and incrementally
   closer to the ideas in the Navigation Error Logging spec.
   Specifically, removes resources and uses a list of safe path
   prefixes to serve the same purpose.

2. Removes the simplest configs and replaces them with an array of
   "config params" that are used to create params at runtime. This
   saves approximately 15,000 lines of source code and around a MB or
   two of binary size (on disk and at runtime).

3. Changes the report format to include full URLs when reporting to 
   same-origin collectors, and partial URLs (based on the list of safe
   path prefixes) for non-same-origin collectors. Removes resource
   reports, since resources no longer exist, and makes a few other 
   tweaks to the format.

4. Changes other parts of the internals of Domain Reliability to 
   accommodate the above changes.
==========

to

==========
Domain Reliability: New config and report formats

This commit makes several intertwined changes:

1. Changes the config format to be much simpler, and incrementally
   closer to the ideas in the Navigation Error Logging spec.
   Specifically, removes resources and uses a list of safe path
   prefixes to serve the same purpose.

2. Removes the simplest configs and replaces them with an array of
   "config params" that are used to create params at runtime. This
   saves approximately 15,000 lines of source code and around a MB or
   two of binary size (on disk and at runtime).

3. Changes the report format to include full URLs when reporting to 
   same-origin collectors, and partial URLs (based on the list of safe
   path prefixes) for non-same-origin collectors. Removes resource
   reports, since resources no longer exist, and makes a few other 
   tweaks to the format.

4. Changes other parts of the internals of Domain Reliability to 
   accommodate the above changes.

BUG=549137,549138
==========

[Deprecated (see juliatuttle), 2015-10-29 15:31:24]

Description was changed from

==========
Domain Reliability: New config and report formats

This commit makes several intertwined changes:

1. Changes the config format to be much simpler, and incrementally
   closer to the ideas in the Navigation Error Logging spec.
   Specifically, removes resources and uses a list of safe path
   prefixes to serve the same purpose.

2. Removes the simplest configs and replaces them with an array of
   "config params" that are used to create params at runtime. This
   saves approximately 15,000 lines of source code and around a MB or
   two of binary size (on disk and at runtime).

3. Changes the report format to include full URLs when reporting to 
   same-origin collectors, and partial URLs (based on the list of safe
   path prefixes) for non-same-origin collectors. Removes resource
   reports, since resources no longer exist, and makes a few other 
   tweaks to the format.

4. Changes other parts of the internals of Domain Reliability to 
   accommodate the above changes.

BUG=549137,549138
==========

to

==========
Domain Reliability: New config and report formats

This commit makes several intertwined changes:

1. Changes the config format to be much simpler, and incrementally
   closer to the ideas in the Navigation Error Logging spec.
   Specifically, removes resources and uses a list of safe path
   prefixes to serve the same purpose.

2. Removes the simplest configs and replaces them with an array of
   "config params" that are used to create params at runtime. This
   saves approximately 15,000 lines of source code and around a MB or
   two of binary size (on disk and at runtime).

3. Changes the report format to include full URLs when reporting to 
   same-origin collectors, and partial URLs (based on the list of safe
   path prefixes) for non-same-origin collectors. Removes resource
   reports, since resources no longer exist, and makes a few other 
   tweaks to the format.

4. Changes other parts of the internals of Domain Reliability to 
   accommodate the above changes.

BUG=549137,549138,495342
==========

[Deprecated (see juliatuttle), 2015-10-29 15:31:28]

On 2015/10/29 14:19:57, rdsmith wrote:
> Two quick high level comments:
> * Why can't #3 be done as a separate CL?
> * I think this change should be attached to an issue.

#3 *could* be done as a separate issue, but #1 necessitates removing part of the
current config format (resources and resource reports), and would leave us with
reports with *no* path data at all.

I'll do you one better, I've attached it to *three* issues.

[Randy Smith (Not in Mondays), 2015-10-29 15:56:10]

On 2015/10/29 15:31:28, ttuttle wrote:
> On 2015/10/29 14:19:57, rdsmith wrote:
> > Two quick high level comments:
> > * Why can't #3 be done as a separate CL?
> > * I think this change should be attached to an issue.
> 
> #3 *could* be done as a separate issue, but #1 necessitates removing part of
the
> current config format (resources and resource reports), and would leave us
with
> reports with *no* path data at all.

So wouldn't that just mean that #3 should go in before (the remainder of) this
issue?

> I'll do you one better, I've attached it to *three* issues.

An abundance of riches!  I am well and truly satisfied.  

Unfortunately, this probably means I have no further way to procrastinate on the
review :-}.

[Deprecated (see juliatuttle), 2015-10-29 16:00:01]

On 2015/10/29 15:56:10, rdsmith wrote:
> On 2015/10/29 15:31:28, ttuttle wrote:
> > On 2015/10/29 14:19:57, rdsmith wrote:
> > > Two quick high level comments:
> > > * Why can't #3 be done as a separate CL?
> > > * I think this change should be attached to an issue.
> > 
> > #3 *could* be done as a separate issue, but #1 necessitates removing part of
> the
> > current config format (resources and resource reports), and would leave us
> with
> > reports with *no* path data at all.
> 
> So wouldn't that just mean that #3 should go in before (the remainder of) this
> issue?

No; if we do #3 first without #1, then we won't have the path prefixes that come
in the new config format, so we'll also lose path data until we get the new
format.

> > I'll do you one better, I've attached it to *three* issues.
> 
> An abundance of riches!  I am well and truly satisfied.  
> 
> Unfortunately, this probably means I have no further way to procrastinate on
the
> review :-}.

Hooray.

[Randy Smith (Not in Mondays), 2015-10-29 22:36:11]

First round of comments.

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
File components/domain_reliability/bake_in_configs.py (left):

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
components/domain_reliability/bake_in_configs.py:529: if 'monitored_domain' not
in config:
Just confirming that all the .json files being parsed are in the source tree? 
I.e. we aren't yet dealing with stuff incoming from web sites?

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
File components/domain_reliability/beacon.h (right):

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
components/domain_reliability/beacon.h:33: const ScopedVector<std::string>&
path_prefixes) const;
nit, suggestion: Comment the argument requiremets?  (I'm specifically uncertain
as to the purpose path_prefixes serve.)

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
File components/domain_reliability/config.h (right):

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
components/domain_reliability/config.h:24: class DOMAIN_RELIABILITY_EXPORT
DomainReliabilityConfig {
This looks more like a struct than a class to me.  Certainly the (public) data
members are named as if they are in a struct, not a class (per style guide
rules).

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
File components/domain_reliability/context.cc (right):

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
components/domain_reliability/context.cc:66: if
(config().DecideIfShouldReportRequest(success)) {
nit, suggestion: Reverse conditional to do early return?  That would require
pulling the final histograms to before the conditional.

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
File components/domain_reliability/context.h (right):

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
components/domain_reliability/context.h:33: // domain's config and per-resource
beacon queues.
Is this comment still valid?  I see a beacon queue, but it doesn't look
per-resource.

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
components/domain_reliability/context.h:65: // Gets the beacons queued for
upload in this context. |beacons_out| will be
nit: |*beacons_out|.

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
components/domain_reliability/context.h:67: // as long as no other requests are
reported to the DomainReliabilityMonitor.
nit, suggestion: These lifetime related requirements make me uncomfortable
because they aren't very simple.  If this wasn't test code, I'd be trying hard
to get you to change them--given that it is test code, I'll just suggest you
change them.  Looking at the usage points, I'd think you could replace this
method with two other methods, one getting the number of elements, and the other
getting a const reference to a particular element.  Const references *can* be
stored beyond the lifetime of the thing referred to, but it's a bit harder; they
tend to be used right where they are.  Just a thought.

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
File components/domain_reliability/context_manager.cc (right):

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
components/domain_reliability/context_manager.cc:37: // makes ASAN mad.
You'll hate me for this, but that's what reviewers are for.  Can you at least
reduce this to a bug filed against ASAN before landing?

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
components/domain_reliability/context_manager.cc:40: wildcard_prefix = "*.";
Why doesn't this result in only seeing subdomains and not the main domain?

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
File components/domain_reliability/google_configs.h (right):

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
components/domain_reliability/google_configs.h:15:
std::vector<DomainReliabilityConfig*>* configs_out);
nit, idea (weaker than a suggestion): Did you consider using a ScopedVector and
using weak_erase at the point of removing them from the vector?  It's all where
you push the ugliness, but that feels better to me.  (Really, ScopedVector
should have a scoped_ptr<> GetAndRemove(iterator) method, but it doesn't, and
I'm certain there are subtle C++ arguments against it doing so.)

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
File components/domain_reliability/monitor.cc (right):

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
components/domain_reliability/monitor.cc:46: // beacon being reported).
This comment needs to be updated?

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
components/domain_reliability/monitor.cc:165: << json;
I don't think a log makes sense here--it would be spam that only you would know
to interpret.  DLOG or DVLOG seems the way to go.

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
components/domain_reliability/monitor.cc:338:
CreateBeaconFromAttempt(beacon_template, attempt);
Why are we creating a new beacon rather than updating the one being passed in? 
(I.e. allocating it in this routine.)  The reason I'd rather do the allocation
here is that the name "beacon_template" feels to me like something abstract,
without any information in it relevant to the current request, whereas what's
actually going on is that we're setting some request-relevant information into
the template, then copying that into another beacon, where we set more request
relevant information.  I'd think that would be clearer if we just used one
object.

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
File components/domain_reliability/util.cc (right):

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
components/domain_reliability/util.cc:174: for (const std::string* path_prefix :
path_prefixes) {
nit, suggestion: I'd be inclined to use a const reference here, and take the
address of the reference below; I think that matches what I've seen elsewhere
better.

[Deprecated (see juliatuttle), 2015-11-02 23:19:31]

PTAL.

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
File components/domain_reliability/bake_in_configs.py (left):

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
components/domain_reliability/bake_in_configs.py:529: if 'monitored_domain' not
in config:
On 2015/10/29 22:36:10, rdsmith wrote:
> Just confirming that all the .json files being parsed are in the source tree? 
> I.e. we aren't yet dealing with stuff incoming from web sites?

Yes. We're not accepting *anything* from the web yet, even from our own origins.

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
File components/domain_reliability/beacon.h (right):

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
components/domain_reliability/beacon.h:33: const ScopedVector<std::string>&
path_prefixes) const;
On 2015/10/29 22:36:10, rdsmith wrote:
> nit, suggestion: Comment the argument requiremets?  (I'm specifically
uncertain
> as to the purpose path_prefixes serve.)

Done.

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
File components/domain_reliability/config.h (right):

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
components/domain_reliability/config.h:24: class DOMAIN_RELIABILITY_EXPORT
DomainReliabilityConfig {
On 2015/10/29 22:36:10, rdsmith wrote:
> This looks more like a struct than a class to me.  Certainly the (public) data
> members are named as if they are in a struct, not a class (per style guide
> rules).

Done.

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
File components/domain_reliability/context.cc (right):

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
components/domain_reliability/context.cc:66: if
(config().DecideIfShouldReportRequest(success)) {
On 2015/10/29 22:36:10, rdsmith wrote:
> nit, suggestion: Reverse conditional to do early return?  That would require
> pulling the final histograms to before the conditional.

Done.

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
File components/domain_reliability/context.h (right):

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
components/domain_reliability/context.h:33: // domain's config and per-resource
beacon queues.
On 2015/10/29 22:36:10, rdsmith wrote:
> Is this comment still valid?  I see a beacon queue, but it doesn't look
> per-resource.

Wow, no, that is super out of date. Fixed.

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
components/domain_reliability/context.h:65: // Gets the beacons queued for
upload in this context. |beacons_out| will be
On 2015/10/29 22:36:11, rdsmith wrote:
> nit: |*beacons_out|.

Done.

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
components/domain_reliability/context.h:67: // as long as no other requests are
reported to the DomainReliabilityMonitor.
On 2015/10/29 22:36:11, rdsmith wrote:
> nit, suggestion: These lifetime related requirements make me uncomfortable
> because they aren't very simple.  If this wasn't test code, I'd be trying hard
> to get you to change them--given that it is test code, I'll just suggest you
> change them.  Looking at the usage points, I'd think you could replace this
> method with two other methods, one getting the number of elements, and the
other
> getting a const reference to a particular element.  Const references *can* be
> stored beyond the lifetime of the thing referred to, but it's a bit harder;
they
> tend to be used right where they are.  Just a thought.

Would you be okay if I punted this to another CL to avoid more churn in the unit
tests?

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
File components/domain_reliability/context_manager.cc (right):

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
components/domain_reliability/context_manager.cc:37: // makes ASAN mad.
On 2015/10/29 22:36:11, rdsmith wrote:
> You'll hate me for this, but that's what reviewers are for.  Can you at least
> reduce this to a bug filed against ASAN before landing?

Do you know how to run standalone binaries against ASAN? I want to make a test
case but all I'm finding is instructions on building all of Chrome with it.

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
components/domain_reliability/context_manager.cc:40: wildcard_prefix = "*.";
On 2015/10/29 22:36:11, rdsmith wrote:
> Why doesn't this result in only seeing subdomains and not the main domain?

Because GetContextForHost("foo.bar") looks for "foo.bar", then "*.foo.bar", then
"*.bar".

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
File components/domain_reliability/google_configs.h (right):

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
components/domain_reliability/google_configs.h:15:
std::vector<DomainReliabilityConfig*>* configs_out);
On 2015/10/29 22:36:11, rdsmith wrote:
> nit, idea (weaker than a suggestion): Did you consider using a ScopedVector
and
> using weak_erase at the point of removing them from the vector?  It's all
where
> you push the ugliness, but that feels better to me.  (Really, ScopedVector
> should have a scoped_ptr<> GetAndRemove(iterator) method, but it doesn't, and
> I'm certain there are subtle C++ arguments against it doing so.)

I did consider it, but it felt uglier to me. :)

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
File components/domain_reliability/monitor.cc (right):

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
components/domain_reliability/monitor.cc:46: // beacon being reported).
On 2015/10/29 22:36:11, rdsmith wrote:
> This comment needs to be updated?

Done.

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
components/domain_reliability/monitor.cc:165: << json;
On 2015/10/29 22:36:11, rdsmith wrote:
> I don't think a log makes sense here--it would be spam that only you would
know
> to interpret.  DLOG or DVLOG seems the way to go.

Done.

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
components/domain_reliability/monitor.cc:338:
CreateBeaconFromAttempt(beacon_template, attempt);
On 2015/10/29 22:36:11, rdsmith wrote:
> Why are we creating a new beacon rather than updating the one being passed in?

> (I.e. allocating it in this routine.)  The reason I'd rather do the allocation
> here is that the name "beacon_template" feels to me like something abstract,
> without any information in it relevant to the current request, whereas what's
> actually going on is that we're setting some request-relevant information into
> the template, then copying that into another beacon, where we set more request
> relevant information.  I'd think that would be clearer if we just used one
> object.

I think we're doing the opposite -- creating a new beacon based on the template,
then filling in the fields in the new beacon.

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
File components/domain_reliability/util.cc (right):

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
components/domain_reliability/util.cc:174: for (const std::string* path_prefix :
path_prefixes) {
On 2015/10/29 22:36:11, rdsmith wrote:
> nit, suggestion: I'd be inclined to use a const reference here, and take the
> address of the reference below; I think that matches what I've seen elsewhere
> better.

Can't; path_prefixes is a vector of std::string*. (ScopedVector's template hides
the pointer type.)

[Randy Smith (Not in Mondays), 2015-11-03 21:48:12]

Next set of comments.

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
File components/domain_reliability/context.h (right):

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
components/domain_reliability/context.h:67: // as long as no other requests are
reported to the DomainReliabilityMonitor.
On 2015/11/02 23:19:30, ttuttle wrote:
> On 2015/10/29 22:36:11, rdsmith wrote:
> > nit, suggestion: These lifetime related requirements make me uncomfortable
> > because they aren't very simple.  If this wasn't test code, I'd be trying
hard
> > to get you to change them--given that it is test code, I'll just suggest you
> > change them.  Looking at the usage points, I'd think you could replace this
> > method with two other methods, one getting the number of elements, and the
> other
> > getting a const reference to a particular element.  Const references *can*
be
> > stored beyond the lifetime of the thing referred to, but it's a bit harder;
> they
> > tend to be used right where they are.  Just a thought.
> 
> Would you be okay if I punted this to another CL to avoid more churn in the
unit
> tests?

Sure.

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
File components/domain_reliability/context_manager.cc (right):

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
components/domain_reliability/context_manager.cc:37: // makes ASAN mad.
On 2015/11/02 23:19:30, ttuttle wrote:
> On 2015/10/29 22:36:11, rdsmith wrote:
> > You'll hate me for this, but that's what reviewers are for.  Can you at
least
> > reduce this to a bug filed against ASAN before landing?
> 
> Do you know how to run standalone binaries against ASAN? I want to make a test
> case but all I'm finding is instructions on building all of Chrome with it.

Sorry, I don't.  

I thought we decided you'd remove the comment and work this separately from this
CL?

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
components/domain_reliability/context_manager.cc:40: wildcard_prefix = "*.";
On 2015/11/02 23:19:30, ttuttle wrote:
> On 2015/10/29 22:36:11, rdsmith wrote:
> > Why doesn't this result in only seeing subdomains and not the main domain?
> 
> Because GetContextForHost("foo.bar") looks for "foo.bar", then "*.foo.bar",
then
> "*.bar".

Oh, and the only thing that wildcard_prefix is used for is creating domain to
index contexts_.  Ok, nm.

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
File components/domain_reliability/google_configs.h (right):

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
components/domain_reliability/google_configs.h:15:
std::vector<DomainReliabilityConfig*>* configs_out);
On 2015/11/02 23:19:30, ttuttle wrote:
> On 2015/10/29 22:36:11, rdsmith wrote:
> > nit, idea (weaker than a suggestion): Did you consider using a ScopedVector
> and
> > using weak_erase at the point of removing them from the vector?  It's all
> where
> > you push the ugliness, but that feels better to me.  (Really, ScopedVector
> > should have a scoped_ptr<> GetAndRemove(iterator) method, but it doesn't,
and
> > I'm certain there are subtle C++ arguments against it doing so.)
> 
> I did consider it, but it felt uglier to me. :)

Acknowledged.

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
File components/domain_reliability/monitor.cc (right):

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
components/domain_reliability/monitor.cc:338:
CreateBeaconFromAttempt(beacon_template, attempt);
On 2015/11/02 23:19:30, ttuttle wrote:
> On 2015/10/29 22:36:11, rdsmith wrote:
> > Why are we creating a new beacon rather than updating the one being passed
in?
> 
> > (I.e. allocating it in this routine.)  The reason I'd rather do the
allocation
> > here is that the name "beacon_template" feels to me like something abstract,
> > without any information in it relevant to the current request, whereas
what's
> > actually going on is that we're setting some request-relevant information
into
> > the template, then copying that into another beacon, where we set more
request
> > relevant information.  I'd think that would be clearer if we just used one
> > object.
> 
> I think we're doing the opposite -- creating a new beacon based on the
template,
> then filling in the fields in the new beacon.
That's how I'd describe what CreateBeaconFromAttempt() is doing.  But this
routine (OnRequestLegComplete) has a beacon local variable (beacon_template)
which is being partially filled in, then passed into CreateBeaconFromAttempt,
which takes that passed in beacon, copies it, and fills stuff in in the new
beacon).  

Of course, on this pass I see why you're doing it (Look!  A for loop!  :-} :-J.)
 I think I'd still prefer the alternative of creating the beacon and partially
filling it in here, then asking CreateBeaconFromAttempt to fill in the rest. 
The create partially filled template which the subroutine copies to create a new
beacon doesn't feel good to me.  There would be no more lines of code, and you'd
never use a template class.  You'd be repeatedly executing the same lines for
what's currently in the template, but that's equivalent performance wise to the
cost of the copy constructor.  

Still, I think this is a matter of style and preference, so I'm good either way.

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
File components/domain_reliability/util.cc (right):

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
components/domain_reliability/util.cc:174: for (const std::string* path_prefix :
path_prefixes) {
On 2015/11/02 23:19:31, ttuttle wrote:
> On 2015/10/29 22:36:11, rdsmith wrote:
> > nit, suggestion: I'd be inclined to use a const reference here, and take the
> > address of the reference below; I think that matches what I've seen
elsewhere
> > better.
> 
> Can't; path_prefixes is a vector of std::string*. (ScopedVector's template
hides
> the pointer type.)

Acknowledged.

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
File components/domain_reliability/config.cc (right):

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
components/domain_reliability/config.cc:46: : success_sample_rate(-1.0),
failure_sample_rate(-1.0) {
Shouldn't this also initialize the bool include_subdomains?

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
File components/domain_reliability/config.h (right):

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
components/domain_reliability/config.h:23: // with what frequency, and where the
beacons are uploaded.
Could you just add a note here that the configuration is for a particular
origin?  (Or domain, if it's per-domain.)

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
File components/domain_reliability/context.cc (right):

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
components/domain_reliability/context.cc:67:
UMA_HISTOGRAM_BOOLEAN("DomainReliability.OnBeaconDidEvict", false);
Quick comment noting that you're entering this histogram at two points?  They
could also be combined--you could pull it out of the conditional and just test
reported && beacons_.size() == kMaxQueuedBeacons--but I'd want a comment on that
as well.

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
File components/domain_reliability/context.h (right):

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
components/domain_reliability/context.h:78: typedef
std::deque<DomainReliabilityBeacon*> BeaconDeque;
If you've gotta store owned pointers in a raw pointer rather than a scoped_ptr,
at least comment it :-} :-|.  (I don't remember this applying elsewhere, but if
it does, please extend the comment--ownership should be obvious looking at the
code).

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
File components/domain_reliability/context_unittest.cc (right):

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
components/domain_reliability/context_unittest.cc:190: // TODO(ttuttle): Add
beacon_unittest.cc to test serialization.
Why not in this CL?

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
File components/domain_reliability/google_configs_unittest.cc (right):

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
components/domain_reliability/google_configs_unittest.cc:15:
GetAllGoogleConfigs(&configs);
What are you testing?  (Even if there is something, this seems a proper subset
of the following test).

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
File components/domain_reliability/monitor_unittest.cc (left):

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
components/domain_reliability/monitor_unittest.cc:142:
TEST_F(DomainReliabilityMonitorTest, NotReported) {
Hmmm.  I'm sure it's my lack of completely groking this CL, but why is this test
no longer useful?

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
File components/domain_reliability/monitor_unittest.cc (right):

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
components/domain_reliability/monitor_unittest.cc:91: DomainReliabilityContext*
CreateAndAddContext(const GURL& origin,
Don't we have a style guide rule against any but the most obvious function
overloads? 
(https://google.github.io/styleguide/cppguide.html#Function_Overloading )

[Deprecated (see juliatuttle), 2015-11-06 17:22:29]

PTAL.

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
File components/domain_reliability/config.cc (right):

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
components/domain_reliability/config.cc:46: : success_sample_rate(-1.0),
failure_sample_rate(-1.0) {
On 2015/11/03 21:48:12, rdsmith wrote:
> Shouldn't this also initialize the bool include_subdomains?

No need, but I'll set it to false just to be safe.

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
File components/domain_reliability/config.h (right):

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
components/domain_reliability/config.h:23: // with what frequency, and where the
beacons are uploaded.
On 2015/11/03 21:48:12, rdsmith wrote:
> Could you just add a note here that the configuration is for a particular
> origin?  (Or domain, if it's per-domain.)

Done.

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
File components/domain_reliability/context.cc (right):

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
components/domain_reliability/context.cc:67:
UMA_HISTOGRAM_BOOLEAN("DomainReliability.OnBeaconDidEvict", false);
On 2015/11/03 21:48:12, rdsmith wrote:
> Quick comment noting that you're entering this histogram at two points?  They
> could also be combined--you could pull it out of the conditional and just test
> reported && beacons_.size() == kMaxQueuedBeacons--but I'd want a comment on
that
> as well.

Done.

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
File components/domain_reliability/context.h (right):

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
components/domain_reliability/context.h:78: typedef
std::deque<DomainReliabilityBeacon*> BeaconDeque;
On 2015/11/03 21:48:12, rdsmith wrote:
> If you've gotta store owned pointers in a raw pointer rather than a
scoped_ptr,
> at least comment it :-} :-|.  (I don't remember this applying elsewhere, but
if
> it does, please extend the comment--ownership should be obvious looking at the
> code).

Done.

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
File components/domain_reliability/context_unittest.cc (right):

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
components/domain_reliability/context_unittest.cc:190: // TODO(ttuttle): Add
beacon_unittest.cc to test serialization.
On 2015/11/03 21:48:12, rdsmith wrote:
> Why not in this CL?

No rush; we test it above as part of uploads. I just want to refactor it into a
separate test at some point.

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
File components/domain_reliability/google_configs_unittest.cc (right):

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
components/domain_reliability/google_configs_unittest.cc:15:
GetAllGoogleConfigs(&configs);
On 2015/11/03 21:48:12, rdsmith wrote:
> What are you testing?  (Even if there is something, this seems a proper subset
> of the following test).

I do that in tests -- most of my tests have a simple case that just makes sure
the class can be created or such. If you don't like the redundancy, I can remove
this case. (It's basically there to make sure it doesn't crash.)

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
File components/domain_reliability/monitor_unittest.cc (left):

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
components/domain_reliability/monitor_unittest.cc:142:
TEST_F(DomainReliabilityMonitorTest, NotReported) {
On 2015/11/03 21:48:12, rdsmith wrote:
> Hmmm.  I'm sure it's my lack of completely groking this CL, but why is this
test
> no longer useful?

It's testing a resource with 0 failure_sample_rate, but we don't have resources
anymore.

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
File components/domain_reliability/monitor_unittest.cc (right):

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
components/domain_reliability/monitor_unittest.cc:91: DomainReliabilityContext*
CreateAndAddContext(const GURL& origin,
On 2015/11/03 21:48:12, rdsmith wrote:
> Don't we have a style guide rule against any but the most obvious function
> overloads? 
> (https://google.github.io/styleguide/cppguide.html#Function_Overloading )

Done.

[Randy Smith (Not in Mondays), 2015-11-09 21:23:25]

I presume last round :-}.

Note: I did nothing to evaluate the conversions of the .json files over to the
.cc files; the few spot checks I did suggest that there were semantic changes
done in that conversion, and I"m not competent to review the semantic changes. 
If you need the conversion itself reviewed, you either want someone who's on top
of the semantic changes or you want to split this into two CLs, one that does
the conversion without any semantic changes.  (The impression I have is that all
that's fine--I just wanted to make it explicit as part of the review.)

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
File components/domain_reliability/context_manager.cc (right):

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
components/domain_reliability/context_manager.cc:37: // makes ASAN mad.
On 2015/11/03 21:48:12, rdsmith wrote:
> On 2015/11/02 23:19:30, ttuttle wrote:
> > On 2015/10/29 22:36:11, rdsmith wrote:
> > > You'll hate me for this, but that's what reviewers are for.  Can you at
> least
> > > reduce this to a bug filed against ASAN before landing?
> > 
> > Do you know how to run standalone binaries against ASAN? I want to make a
test
> > case but all I'm finding is instructions on building all of Chrome with it.
> 
> Sorry, I don't.  
> 
> I thought we decided you'd remove the comment and work this separately from
this
> CL?

Ping?

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
File components/domain_reliability/monitor.cc (right):

https://codereview.chromium.org/1180223006/diff/180001/components/domain_reli...
components/domain_reliability/monitor.cc:338:
CreateBeaconFromAttempt(beacon_template, attempt);
On 2015/11/03 21:48:12, rdsmith wrote:
> On 2015/11/02 23:19:30, ttuttle wrote:
> > On 2015/10/29 22:36:11, rdsmith wrote:
> > > Why are we creating a new beacon rather than updating the one being passed
> in?
> > 
> > > (I.e. allocating it in this routine.)  The reason I'd rather do the
> allocation
> > > here is that the name "beacon_template" feels to me like something
abstract,
> > > without any information in it relevant to the current request, whereas
> what's
> > > actually going on is that we're setting some request-relevant information
> into
> > > the template, then copying that into another beacon, where we set more
> request
> > > relevant information.  I'd think that would be clearer if we just used one
> > > object.
> > 
> > I think we're doing the opposite -- creating a new beacon based on the
> template,
> > then filling in the fields in the new beacon.
> That's how I'd describe what CreateBeaconFromAttempt() is doing.  But this
> routine (OnRequestLegComplete) has a beacon local variable (beacon_template)
> which is being partially filled in, then passed into CreateBeaconFromAttempt,
> which takes that passed in beacon, copies it, and fills stuff in in the new
> beacon).  
> 
> Of course, on this pass I see why you're doing it (Look!  A for loop!  :-}
:-J.)
>  I think I'd still prefer the alternative of creating the beacon and partially
> filling it in here, then asking CreateBeaconFromAttempt to fill in the rest. 
> The create partially filled template which the subroutine copies to create a
new
> beacon doesn't feel good to me.  There would be no more lines of code, and
you'd
> never use a template class.  You'd be repeatedly executing the same lines for
> what's currently in the template, but that's equivalent performance wise to
the
> cost of the copy constructor.  
> 
> Still, I think this is a matter of style and preference, so I'm good either
way.
> 

Just wanted to confirm that you read this and made a choice to keep the current
form, rather than skipped over it.

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
File components/domain_reliability/context.h (right):

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
components/domain_reliability/context.h:78: typedef
std::deque<DomainReliabilityBeacon*> BeaconDeque;
On 2015/11/06 17:22:29, ttuttle wrote:
> On 2015/11/03 21:48:12, rdsmith wrote:
> > If you've gotta store owned pointers in a raw pointer rather than a
> scoped_ptr,
> > at least comment it :-} :-|.  (I don't remember this applying elsewhere, but
> if
> > it does, please extend the comment--ownership should be obvious looking at
the
> > code).
> 
> Done.

nit: Comment here, in addition to or instead of the one at the beacons_
definition?  I think of ownership as being expressed in types (e.g.
scoped_ptr<>) so even if BeaconDeque isn't used anywhere but the def of beacons,
I'd like a comment at the type def.

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
File components/domain_reliability/context_unittest.cc (right):

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
components/domain_reliability/context_unittest.cc:190: // TODO(ttuttle): Add
beacon_unittest.cc to test serialization.
On 2015/11/06 17:22:29, ttuttle wrote:
> On 2015/11/03 21:48:12, rdsmith wrote:
> > Why not in this CL?
> 
> No rush; we test it above as part of uploads. I just want to refactor it into
a
> separate test at some point.

Acknowledged.

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
File components/domain_reliability/google_configs_unittest.cc (right):

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
components/domain_reliability/google_configs_unittest.cc:15:
GetAllGoogleConfigs(&configs);
On 2015/11/06 17:22:29, ttuttle wrote:
> On 2015/11/03 21:48:12, rdsmith wrote:
> > What are you testing?  (Even if there is something, this seems a proper
subset
> > of the following test).
> 
> I do that in tests -- most of my tests have a simple case that just makes sure
> the class can be created or such. If you don't like the redundancy, I can
remove
> this case. (It's basically there to make sure it doesn't crash.)

In a unittest I don't really care; it's fine.

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
File components/domain_reliability/monitor_unittest.cc (left):

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
components/domain_reliability/monitor_unittest.cc:142:
TEST_F(DomainReliabilityMonitorTest, NotReported) {
On 2015/11/06 17:22:29, ttuttle wrote:
> On 2015/11/03 21:48:12, rdsmith wrote:
> > Hmmm.  I'm sure it's my lack of completely groking this CL, but why is this
> test
> > no longer useful?
> 
> It's testing a resource with 0 failure_sample_rate, but we don't have
resources
> anymore.

Acknowledged.

[Deprecated (see juliatuttle), 2015-11-10 17:39:49]

PTAL.

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
File components/domain_reliability/context.h (right):

https://codereview.chromium.org/1180223006/diff/200001/components/domain_reli...
components/domain_reliability/context.h:78: typedef
std::deque<DomainReliabilityBeacon*> BeaconDeque;
On 2015/11/09 21:23:24, rdsmith wrote:
> On 2015/11/06 17:22:29, ttuttle wrote:
> > On 2015/11/03 21:48:12, rdsmith wrote:
> > > If you've gotta store owned pointers in a raw pointer rather than a
> > scoped_ptr,
> > > at least comment it :-} :-|.  (I don't remember this applying elsewhere,
but
> > if
> > > it does, please extend the comment--ownership should be obvious looking at
> the
> > > code).
> > 
> > Done.
> 
> nit: Comment here, in addition to or instead of the one at the beacons_
> definition?  I think of ownership as being expressed in types (e.g.
> scoped_ptr<>) so even if BeaconDeque isn't used anywhere but the def of
beacons,
> I'd like a comment at the type def.

Done.

[Deprecated (see juliatuttle), 2015-11-13 04:53:01]

1. I just removed the ASAN comment.

2. Yes, I do prefer the template approach aesthetically. I can change it if you
disagree strongly.

[Randy Smith (Not in Mondays), 2015-11-17 22:28:27]

LGTM; sorry it took so long.

[Deprecated (see juliatuttle), 2015-11-19 00:47:24]

The CQ bit was checked by ttuttle@chromium.org

[Deprecated (see juliatuttle), 2015-11-19 00:47:25]

The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org
Link to the patchset: https://codereview.chromium.org/1180223006/#ps300001
(title: "rebase")

[commit-bot: I haz the power, 2015-11-19 00:51:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1180223006/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1180223006/300001

[commit-bot: I haz the power, 2015-11-19 01:40:21]

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

[commit-bot: I haz the power, 2015-11-19 01:40:22]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, no build URL)

[Deprecated (see juliatuttle), 2015-11-19 02:41:49]

The CQ bit was checked by ttuttle@chromium.org

[commit-bot: I haz the power, 2015-11-19 02:44:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1180223006/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1180223006/300001

[commit-bot: I haz the power, 2015-11-19 03:01:43]

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

[commit-bot: I haz the power, 2015-11-19 03:01:45]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Deprecated (see juliatuttle), 2015-11-20 17:27:22]

asvitkine, PTAL at histograms.xml.

[Deprecated (see juliatuttle), 2015-11-20 17:28:21]

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

[Deprecated (see juliatuttle), 2015-11-20 17:28:23]

Actually adding asvitkine as reviewer this time.

asvitkine, PTAL at histograms.xml.

[Alexei Svitkine (slow), 2015-11-20 17:40:58]

https://codereview.chromium.org/1180223006/diff/320001/components/domain_reli...
File components/domain_reliability/context.cc (right):

https://codereview.chromium.org/1180223006/diff/320001/components/domain_reli...
components/domain_reliability/context.cc:70:
UMA_HISTOGRAM_BOOLEAN("DomainReliability.OnBeaconDidEvict", false);
Nit: Can you make a helper function to log this histogram now that it's being
emitted from multiple places? Each UMA macro expands to a bunch of code, so this
would avoid the extra bloat.

https://codereview.chromium.org/1180223006/diff/320001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/1180223006/diff/320001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:8134: -<histogram
name="DomainReliability.AddBeaconDidEvict" enum="BooleanDidEvict">
Just to make sure I understand, AddBeaconDidEvict was never emitted?

[Deprecated (see juliatuttle), 2015-11-20 17:46:57]

PTAL, asvitkine.

https://codereview.chromium.org/1180223006/diff/320001/components/domain_reli...
File components/domain_reliability/context.cc (right):

https://codereview.chromium.org/1180223006/diff/320001/components/domain_reli...
components/domain_reliability/context.cc:70:
UMA_HISTOGRAM_BOOLEAN("DomainReliability.OnBeaconDidEvict", false);
On 2015/11/20 17:40:58, Alexei Svitkine (slow) wrote:
> Nit: Can you make a helper function to log this histogram now that it's being
> emitted from multiple places? Each UMA macro expands to a bunch of code, so
this
> would avoid the extra bloat.

Done.

https://codereview.chromium.org/1180223006/diff/320001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/1180223006/diff/320001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:8134: -<histogram
name="DomainReliability.AddBeaconDidEvict" enum="BooleanDidEvict">
On 2015/11/20 17:40:58, Alexei Svitkine (slow) wrote:
> Just to make sure I understand, AddBeaconDidEvict was never emitted?

Correct. It doesn't show up in the dashboard at all.

[Alexei Svitkine (slow), 2015-11-20 17:54:39]

lgtm

[Deprecated (see juliatuttle), 2015-11-20 17:56:27]

The CQ bit was checked by ttuttle@chromium.org

[Deprecated (see juliatuttle), 2015-11-20 17:56:29]

The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org
Link to the patchset: https://codereview.chromium.org/1180223006/#ps340001
(title: "Extract LogOnBeaconDidEvictHistogram")

[commit-bot: I haz the power, 2015-11-20 17:58:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1180223006/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1180223006/340001

[commit-bot: I haz the power, 2015-11-20 19:24:52]

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

[commit-bot: I haz the power, 2015-11-20 19:24:53]

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

[Deprecated (see juliatuttle), 2015-11-20 22:12:11]

The CQ bit was checked by ttuttle@chromium.org

[commit-bot: I haz the power, 2015-11-20 22:15:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1180223006/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1180223006/340001

[commit-bot: I haz the power, 2015-11-21 00:17:24]

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

[commit-bot: I haz the power, 2015-11-21 00:17:26]

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

[Deprecated (see juliatuttle), 2015-11-21 01:30:54]

The CQ bit was checked by ttuttle@chromium.org

[Deprecated (see juliatuttle), 2015-11-21 01:30:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, rdsmith@chromium.org
Link to the patchset: https://codereview.chromium.org/1180223006/#ps360001
(title: "Fix memory leak in unittests")

[commit-bot: I haz the power, 2015-11-21 01:34:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1180223006/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1180223006/360001

[commit-bot: I haz the power, 2015-11-21 02:54:03]

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

[commit-bot: I haz the power, 2015-11-21 02:54:05]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Deprecated (see juliatuttle), 2015-11-22 01:56:09]

The CQ bit was checked by ttuttle@chromium.org

[commit-bot: I haz the power, 2015-11-22 01:57:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1180223006/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1180223006/360001

[commit-bot: I haz the power, 2015-11-22 06:09:51]

Committed patchset #19 (id:360001)

[commit-bot: I haz the power, 2015-11-22 06:10:39]

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