More fine-grained pruning in safe browsing incident reporting service.

More fine-grained pruning in safe browsing incident reporting service.

BUG=396398
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285491

[grt (UTC plus 2), 2014-07-23 02:52:09]

gab: chrome/browser/prefs/chrome_pref_service_factory.cc
noms: chrome/browser/profiles/profile.cc
shess: chrome/browser/safe_browsing/*

thanks.

[gab, 2014-07-23 13:58:11]

prefs lgtm

+ drive-by comments on the overall change.

https://codereview.chromium.org/411793004/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/incident_reporting_service.cc (right):

https://codereview.chromium.org/411793004/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting_service.cc:180: const
PersistentIncidentState& data = states[i];
Use a std::vector::const_iterator rather than looping with an index?

https://codereview.chromium.org/411793004/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting_service.cc:182: std::string
type_string(base::IntToString(data.type));
optional: const

(I know you sometimes don't like this, but I think it makes sense to be explicit
here that this is only a cache variable)

https://codereview.chromium.org/411793004/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting_service.cc:188:
type_dict->SetIntegerWithoutPathExpansion(data.key, data.digest);
In IncidentHasBeenReported() you explicitly cast from int to uint32_t on read,
but here you don't:
 1) Is the cast implicit anyways?
 2) If your uint is bigger than MAX_INT isn't that a problem?

https://codereview.chromium.org/411793004/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting_service.cc:393: for (size_t i =
0; i < uploads_.size(); ++i)
Use iterator?

https://codereview.chromium.org/411793004/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting_service.cc:666: for (size_t i =
0; i < context->incidents.size(); ++i) {
Iterator?

https://codereview.chromium.org/411793004/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting_service.cc:668:
PersistentIncidentState state = ComputeIncidentState(*incident);
const

https://codereview.chromium.org/411793004/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/incident_reporting_service_unittest.cc
(right):

https://codereview.chromium.org/411793004/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting_service_unittest.cc:673: //
Adding the same incident again should still result in no upload.
But adding another one should result in an upload, I think you should test that
as well.

[grt (UTC plus 2), 2014-07-23 14:45:19]

Thanks! Please have a look at the changes I've made.

https://codereview.chromium.org/411793004/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/incident_reporting_service.cc (right):

https://codereview.chromium.org/411793004/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting_service.cc:180: const
PersistentIncidentState& data = states[i];
On 2014/07/23 13:58:10, gab wrote:
> Use a std::vector::const_iterator rather than looping with an index? 

I used to do that, but had the indexing hammered into me by other reviewers who
have explained that this is more readable.

https://codereview.chromium.org/411793004/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting_service.cc:182: std::string
type_string(base::IntToString(data.type));
On 2014/07/23 13:58:11, gab wrote:
> optional: const
> 
> (I know you sometimes don't like this, but I think it makes sense to be
explicit
> here that this is only a cache variable)

Yeah, I'm not too excited about that, but okay. ;-)

https://codereview.chromium.org/411793004/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting_service.cc:188:
type_dict->SetIntegerWithoutPathExpansion(data.key, data.digest);
On 2014/07/23 13:58:10, gab wrote:
> In IncidentHasBeenReported() you explicitly cast from int to uint32_t on read,
> but here you don't:
>  1) Is the cast implicit anyways?
>  2) If your uint is bigger than MAX_INT isn't that a problem?

Good catch. I'm surprised the compiler didn't warn me about this. It turns out I
was relying on undefined compiler behavior. I've switched to using a string.

https://codereview.chromium.org/411793004/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting_service.cc:668:
PersistentIncidentState state = ComputeIncidentState(*incident);
On 2014/07/23 13:58:11, gab wrote:
> const

Done.

https://codereview.chromium.org/411793004/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/incident_reporting_service_unittest.cc
(right):

https://codereview.chromium.org/411793004/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting_service_unittest.cc:673: //
Adding the same incident again should still result in no upload.
On 2014/07/23 13:58:11, gab wrote:
> But adding another one should result in an upload, I think you should test
that
> as well.

i don't think that's needed. since this test validates that the legacy pref is
cleared, adding another different incident would be testing the same thing that
TwoIncidentsTwoUploads tests.

[gab, 2014-07-23 15:00:23]

lgtm for use of string for uint.

Regarding iterator versus index for looping, I prefer iterator because:
 1) No need to call size() over and over again (perhaps arguably no worse than
calling end() over and over again though...).
 2) No need to use [i] to access elements -- arguably for vectors this is
constant-time access into an array (for other data structures though, e.g. sets,
this is not constant-time access -- so given this makes iterator better for some
data structures, I just use it for all).

Cheers,
Gab

https://codereview.chromium.org/411793004/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/incident_reporting_service_unittest.cc
(right):

https://codereview.chromium.org/411793004/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting_service_unittest.cc:673: //
Adding the same incident again should still result in no upload.
On 2014/07/23 14:45:18, grt wrote:
> On 2014/07/23 13:58:11, gab wrote:
> > But adding another one should result in an upload, I think you should test
> that
> > as well.
> 
> i don't think that's needed. since this test validates that the legacy pref is
> cleared, adding another different incident would be testing the same thing
that
> TwoIncidentsTwoUploads tests.

Good point, not worth it indeed, given you verify the pref is cleared.

[noms (inactive), 2014-07-23 17:50:40]

c/b/profiles lgtm

[Scott Hess - ex-Googler, 2014-07-23 22:05:51]

Some quibbling with the structure of the new code, possibly because I'm not
super familiar with how this code works right now.

https://codereview.chromium.org/411793004/diff/80001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/incident_reporting_service.cc (right):

https://codereview.chromium.org/411793004/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting_service.cc:63: };
Is there any system to how these are organized?  They don't seem to be
alphabetic or anything like that, nor do they seem to be grouped by related
types.

https://codereview.chromium.org/411793004/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting_service.cc:102: const
IncidentTypeHandlers* GetIncidentTypeHandlers(IncidentType type) {
This seems like a kind of elaborate setup to replace a function something like:

std::pair<std::string,uint32_t> GetIncidentInfo(IncidentType type) {
  switch(type) {
    case TRACKED_PREFERENCE:
      return std::make_pair(path(), HashMessage(tracked_preference()));
    //...
  }
}
// Or have each case vector to a helper fn, with this fn replacing the table.

Neither the CL description nor the associated bug gives me any context for
assuming that there will just happen to be appropriate callbacks out in the wild
generating the right key and digest, so is there something else driving all of
the indirections?

https://codereview.chromium.org/411793004/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting_service.cc:119: }
This seems questionable, to me.  I think that if you serialize a message where
an optional value is not set, that will be different from if you serialize a
message where an optional value is set to the default value.  In which case the
resulting hash will also be different.

https://codereview.chromium.org/411793004/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting_service.cc:174: digest ==
state.digest);
Could the last two lines be:
    digest_string == base::UintToString(state.digest)
?  It is possible that the round trip back to uint32_t will give different
results, in which case this will never return true, even though the code below
keeps setting the string from the digest.

https://codereview.chromium.org/411793004/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting_service.cc:645:
std::map<Profile*, std::vector<PersistentIncidentState> > profiles_to_state;
PersistentIncidentStateCollection?  The code belows swaps this with one of
those, so it should probalby use the same typedef, either as-is or lifted to the
anon namespace.

[grt (UTC plus 2), 2014-07-24 18:58:40]

Thanks for the comments. PTAL.

https://codereview.chromium.org/411793004/diff/80001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/incident_reporting_service.cc (right):

https://codereview.chromium.org/411793004/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting_service.cc:63: };
On 2014/07/23 22:05:51, Scott Hess wrote:
> Is there any system to how these are organized?  They don't seem to be
> alphabetic or anything like that, nor do they seem to be grouped by related
> types.

I was attempting to roughly follow the declaration order
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...),
but I see that kDefaultUploadDelayMs ended up between types. Ooops. I've moved
things a bit (see reply below) and added a few more comments.

https://codereview.chromium.org/411793004/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting_service.cc:102: const
IncidentTypeHandlers* GetIncidentTypeHandlers(IncidentType type) {
On 2014/07/23 22:05:50, Scott Hess wrote:
> This seems like a kind of elaborate setup to replace a function something
like:
> 
> std::pair<std::string,uint32_t> GetIncidentInfo(IncidentType type) {
>   switch(type) {
>     case TRACKED_PREFERENCE:
>       return std::make_pair(path(), HashMessage(tracked_preference()));
>     //...
>   }
> }
> // Or have each case vector to a helper fn, with this fn replacing the table.
> 
> Neither the CL description nor the associated bug gives me any context for
> assuming that there will just happen to be appropriate callbacks out in the
wild
> generating the right key and digest, so is there something else driving all of
> the indirections?

There will be more incident types in the future. I thought the data-driven
approach would scale better, and makes it clear to developers what two bits of
functionality they need to provide. To make this even more explicit, I've moved
the incident-specific bits into their own file (with their own tests).

https://codereview.chromium.org/411793004/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting_service.cc:119: }
On 2014/07/23 22:05:51, Scott Hess wrote:
> This seems questionable, to me.  I think that if you serialize a message where
> an optional value is not set, that will be different from if you serialize a
> message where an optional value is set to the default value.  In which case
the
> resulting hash will also be different.

Ah, yes. I hadn't thought about optional values when I was weighing this
approach. I guess it'll be a bit more work for creators of new incident types.

https://codereview.chromium.org/411793004/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting_service.cc:174: digest ==
state.digest);
On 2014/07/23 22:05:51, Scott Hess wrote:
> Could the last two lines be:
>     digest_string == base::UintToString(state.digest)
> ?

Rock on.

> It is possible that the round trip back to uint32_t will give different
> results,

I didn't realize there was ambiguity in the uint->string->uint dance. I've tried
it with all values I could come up with (2^32 of 'em), and they all round-trip
safely. What am I missing?

> in which case this will never return true, even though the code below
> keeps setting the string from the digest.

https://codereview.chromium.org/411793004/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting_service.cc:645:
std::map<Profile*, std::vector<PersistentIncidentState> > profiles_to_state;
On 2014/07/23 22:05:51, Scott Hess wrote:
> PersistentIncidentStateCollection?  The code belows swaps this with one of
> those, so it should probalby use the same typedef, either as-is or lifted to
the
> anon namespace.

Done.

[Scott Hess - ex-Googler, 2014-07-24 21:22:20]

LGTM.  You've gotten co-conspirator buy-in, and I'm mostly just lazily picking
nits rather than doing real work.  So only pay attention to my comments if you
read them and think "Hey, that's right!"

https://codereview.chromium.org/411793004/diff/80001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/incident_reporting_service.cc (right):

https://codereview.chromium.org/411793004/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting_service.cc:102: const
IncidentTypeHandlers* GetIncidentTypeHandlers(IncidentType type) {
On 2014/07/24 18:58:40, grt wrote:
> On 2014/07/23 22:05:50, Scott Hess wrote:
> > This seems like a kind of elaborate setup to replace a function something
> like:
> > 
> > std::pair<std::string,uint32_t> GetIncidentInfo(IncidentType type) {
> >   switch(type) {
> >     case TRACKED_PREFERENCE:
> >       return std::make_pair(path(), HashMessage(tracked_preference()));
> >     //...
> >   }
> > }
> > // Or have each case vector to a helper fn, with this fn replacing the
table.
> > 
> > Neither the CL description nor the associated bug gives me any context for
> > assuming that there will just happen to be appropriate callbacks out in the
> wild
> > generating the right key and digest, so is there something else driving all
of
> > the indirections?
> 
> There will be more incident types in the future. I thought the data-driven
> approach would scale better, and makes it clear to developers what two bits of
> functionality they need to provide. To make this even more explicit, I've
moved
> the incident-specific bits into their own file (with their own tests).

I like the tests, my main complaint here was mostly about creating a system
which looks a lot like what vtables already do.  I'm still not 100% convinced
that editing "a table of unique types plus function pointers which someone loops
over" is really an improvement over editing "a function with a switch
statement", regardless of where the functions either setup calls out to live,
but you'll be owning the code, so ...

https://codereview.chromium.org/411793004/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting_service.cc:119: }
On 2014/07/24 18:58:40, grt wrote:
> On 2014/07/23 22:05:51, Scott Hess wrote:
> > This seems questionable, to me.  I think that if you serialize a message
where
> > an optional value is not set, that will be different from if you serialize a
> > message where an optional value is set to the default value.  In which case
> the
> > resulting hash will also be different.
> 
> Ah, yes. I hadn't thought about optional values when I was weighing this
> approach. I guess it'll be a bit more work for creators of new incident types.

It's plausible that the code paths which lead here will always set things in the
same way, so that there is no practical possibility of divergence, or it could
always be detected and cleaned up easily.  It is also possible that these should
actually be treated as different values (effectively undef differs from a set
value).  I don't understand your overall scheme well enough to know if these
things matter.

https://codereview.chromium.org/411793004/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting_service.cc:174: digest ==
state.digest);
On 2014/07/24 18:58:40, grt wrote:
> On 2014/07/23 22:05:51, Scott Hess wrote:
> > Could the last two lines be:
> >     digest_string == base::UintToString(state.digest)
> > ?
> 
> Rock on.
> 
> > It is possible that the round trip back to uint32_t will give different
> > results,
> 
> I didn't realize there was ambiguity in the uint->string->uint dance. I've
tried
> it with all values I could come up with (2^32 of 'em), and they all round-trip
> safely. What am I missing?
> 
> > in which case this will never return true, even though the code below
> > keeps setting the string from the digest.

I wouldn't expect there to be any cases where things won't round-trip correctly,
just the observation that all uint32_t can be represented by std::string but not
all std::string can be represented by uint32_t.

[Maybe I've written too many conversions over the years and now am all calcified
:-).]

https://codereview.chromium.org/411793004/diff/100001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/incident_reporting_service.cc (right):

https://codereview.chromium.org/411793004/diff/100001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service.cc:103: if
((incident.*kIncidentTypeHandlers[i].has_type_fn)())
This is wow.  And I'm not sure there are alternatives.  Maybe

  (incident.(*kIncidentTypeHandlers[i].has_type_fn))()

I think that's still a little wiggly, because "has_type_fn" feels like a boolean
setting, but I'm not sure what else wouldn't feel like that.  Or perhaps

  (*kIncidentTypeHandlers[i].has_type_fn)(incident);

which is pretty clear about the function call.  Or perhaps leave it alone, it's
a thing you can do in C++, and it does what you need to do...

[grt (UTC plus 2), 2014-07-24 22:58:12]

Thank, Scott. PTAL.

https://codereview.chromium.org/411793004/diff/80001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/incident_reporting_service.cc (right):

https://codereview.chromium.org/411793004/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting_service.cc:102: const
IncidentTypeHandlers* GetIncidentTypeHandlers(IncidentType type) {
On 2014/07/24 21:22:19, Scott Hess wrote:
> On 2014/07/24 18:58:40, grt wrote:
> > On 2014/07/23 22:05:50, Scott Hess wrote:
> > > This seems like a kind of elaborate setup to replace a function something
> > like:
> > > 
> > > std::pair<std::string,uint32_t> GetIncidentInfo(IncidentType type) {
> > >   switch(type) {
> > >     case TRACKED_PREFERENCE:
> > >       return std::make_pair(path(), HashMessage(tracked_preference()));
> > >     //...
> > >   }
> > > }
> > > // Or have each case vector to a helper fn, with this fn replacing the
> table.
> > > 
> > > Neither the CL description nor the associated bug gives me any context for
> > > assuming that there will just happen to be appropriate callbacks out in
the
> > wild
> > > generating the right key and digest, so is there something else driving
all
> of
> > > the indirections?
> > 
> > There will be more incident types in the future. I thought the data-driven
> > approach would scale better, and makes it clear to developers what two bits
of
> > functionality they need to provide. To make this even more explicit, I've
> moved
> > the incident-specific bits into their own file (with their own tests).
> 
> I like the tests, my main complaint here was mostly about creating a system
> which looks a lot like what vtables already do.  I'm still not 100% convinced
> that editing "a table of unique types plus function pointers which someone
loops
> over" is really an improvement over editing "a function with a switch
> statement", regardless of where the functions either setup calls out to live,
> but you'll be owning the code, so ...

You've convinced me. While I liked the idea that developers would only need to
touch one thing to add support for new types, it seems that it's complex to at
least one developer-who-is-not-the-implementor, so it's better to go with
something more simple. let me know what you think.

https://codereview.chromium.org/411793004/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting_service.cc:119: }
On 2014/07/24 21:22:19, Scott Hess wrote:
> On 2014/07/24 18:58:40, grt wrote:
> > On 2014/07/23 22:05:51, Scott Hess wrote:
> > > This seems questionable, to me.  I think that if you serialize a message
> where
> > > an optional value is not set, that will be different from if you serialize
a
> > > message where an optional value is set to the default value.  In which
case
> > the
> > > resulting hash will also be different.
> > 
> > Ah, yes. I hadn't thought about optional values when I was weighing this
> > approach. I guess it'll be a bit more work for creators of new incident
types.
> 
> It's plausible that the code paths which lead here will always set things in
the
> same way, so that there is no practical possibility of divergence, or it could
> always be detected and cleaned up easily.  It is also possible that these
should
> actually be treated as different values (effectively undef differs from a set
> value).  I don't understand your overall scheme well enough to know if these
> things matter.

I'm conflicted about this. In the current implementation for the one existing
incident, there's no problem with the simple serialization (i've reverted the
impl to this). I'll make myself a per-file owner so I'm sure to verify future
implementations.

https://codereview.chromium.org/411793004/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting_service.cc:174: digest ==
state.digest);
On 2014/07/24 21:22:19, Scott Hess wrote:
> On 2014/07/24 18:58:40, grt wrote:
> > On 2014/07/23 22:05:51, Scott Hess wrote:
> > > Could the last two lines be:
> > >     digest_string == base::UintToString(state.digest)
> > > ?
> > 
> > Rock on.
> > 
> > > It is possible that the round trip back to uint32_t will give different
> > > results,
> > 
> > I didn't realize there was ambiguity in the uint->string->uint dance. I've
> tried
> > it with all values I could come up with (2^32 of 'em), and they all
round-trip
> > safely. What am I missing?
> > 
> > > in which case this will never return true, even though the code below
> > > keeps setting the string from the digest.
> 
> I wouldn't expect there to be any cases where things won't round-trip
correctly,
> just the observation that all uint32_t can be represented by std::string but
not
> all std::string can be represented by uint32_t.

Ah. I'm not concerned about that. If code outside of this module diddles with
the preference such that the string cannot be turned back into the correct int,
the right thing to do is to treat that as the incident not having been reported.
This is exactly what will happen. I appreciate the scrutiny.

https://codereview.chromium.org/411793004/diff/100001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/incident_reporting_service.cc (right):

https://codereview.chromium.org/411793004/diff/100001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service.cc:103: if
((incident.*kIncidentTypeHandlers[i].has_type_fn)())
On 2014/07/24 21:22:20, Scott Hess wrote:
> This is wow.  And I'm not sure there are alternatives.  Maybe
> 
>   (incident.(*kIncidentTypeHandlers[i].has_type_fn))()
> 
> I think that's still a little wiggly, because "has_type_fn" feels like a
boolean
> setting, but I'm not sure what else wouldn't feel like that.  Or perhaps
> 
>   (*kIncidentTypeHandlers[i].has_type_fn)(incident);
> 
> which is pretty clear about the function call.  Or perhaps leave it alone,
it's
> a thing you can do in C++, and it does what you need to do...

i was making use of the .* operator here which takes an instance on the lhs and
a pointer to a member on the rhs. a valid alternative would be to wrap
kIncidentTypeHandlers[i].has_type_fn in parens:

  (incident.*(kIncidentTypeHandlers[i].has_type_fn))()

moot now since i've removed this.

[grt (UTC plus 2), 2014-07-24 23:01:26]

On 2014/07/24 22:58:12, grt wrote:
> Thank, Scott. PTAL.

Ah, and please hit the CQ box if you think this is an improvement over the
previous patch set. Thanks.

[Scott Hess - ex-Googler, 2014-07-24 23:09:54]

LGTM - I will leave c-q unchecked so that you have the option to push it
immediately and make the suggested change later, or make the change and push it
without asking for further review.

https://codereview.chromium.org/411793004/diff/120001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/incident_reporting_service.cc (right):

https://codereview.chromium.org/411793004/diff/120001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service.cc:120: break;
The NOTREACHED() will only fire if an incident of the appropriate type is
posted.  Which is better than nothing, but if you did:
  case NUM_INCIDENT_TYPES:
    NOTREACHED();
    break;
then this would fail to compile at all.

[grt (UTC plus 2), 2014-07-24 23:21:23]

The CQ bit was checked by grt@chromium.org

[grt (UTC plus 2), 2014-07-24 23:22:08]

I like breaking compilation. Thanks for the idea.

https://codereview.chromium.org/411793004/diff/120001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/incident_reporting_service.cc (right):

https://codereview.chromium.org/411793004/diff/120001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service.cc:120: break;
On 2014/07/24 23:09:54, Scott Hess wrote:
> The NOTREACHED() will only fire if an incident of the appropriate type is
> posted.  Which is better than nothing, but if you did:
>   case NUM_INCIDENT_TYPES:
>     NOTREACHED();
>     break;
> then this would fail to compile at all.

i've added some COMPILE_ASSERTs. thanks for the idea.

[commit-bot: I haz the power, 2014-07-24 23:25:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/grt@chromium.org/411793004/140001

[Scott Hess - ex-Googler, 2014-07-25 01:08:26]

On 2014/07/24 23:22:08, grt wrote:
> I like breaking compilation. Thanks for the idea.
> 
>
https://codereview.chromium.org/411793004/diff/120001/chrome/browser/safe_bro...
> File chrome/browser/safe_browsing/incident_reporting_service.cc (right):
> 
>
https://codereview.chromium.org/411793004/diff/120001/chrome/browser/safe_bro...
> chrome/browser/safe_browsing/incident_reporting_service.cc:120: break;
> On 2014/07/24 23:09:54, Scott Hess wrote:
> > The NOTREACHED() will only fire if an incident of the appropriate type is
> > posted.  Which is better than nothing, but if you did:
> >   case NUM_INCIDENT_TYPES:
> >     NOTREACHED();
> >     break;
> > then this would fail to compile at all.
> 
> i've added some COMPILE_ASSERTs. thanks for the idea.

:-).

Actually, I meant that if you have:
  enum T {
    A = 0,
    B,
    C,
  };
  enum T x = C;
  switch(x) {
    case A: break;
    case B: break;
  }

then at least clang will throw:
path/file.cc:10:10: error: enumeration value 'C' not handled in switch
[-Werror,-Wswitch]
  switch(x) {
         ^
1 error generated.

[commit-bot: I haz the power, 2014-07-25 05:45:05]

Change committed as 285491