I think I slightly prefer macro approach in this case, as we might be adding more similar fields to the class... though the use of macro is in general discouraged :( I'll think twice if I could come up with more preferable approach.

Thanks! Yes, I have a slight preference for the macro approach as well, and I think Charles indicated the same.

I also pinged a few people on net stack team to see if they encountered any drawbacks with their net_error_list approach. If they don't have any drawbacks to report, I think that's probably the best way forward.

I'd also be open to keeping the code as is but I worry about new bugs being introduced going forward. I think the macro approach is better than leaving the code as is.

I'll put in a mild vote against the macro approach.  It's clever, but it's not a common idiom (I think there are a couple of cases of the error code list approach in the code) and it is confusing until you understand it.  I think the static assert works well, in that a test will fail if you add a new field and don't do the appropriate cleanup, and the test that fails tells you exactly what cleanup to do.  

I don't have strong feelings, but what feelings I have lean me away from the macros.

Thanks! Good point. I agree that there's value in not having to muck with the page_load_timing.h file and not depending on macros. Do others on the bug agree that the compile time assert is likely to catch the attention of developers as they add fields, to make sure they avoid adding a field w/o updating operator== and IsEmpty? If we're confident in that, then maybe the compile time assertion is the best approach since it doesn't require making page_load_timing.h ugly and doesn't depend on macros.

SGTM honestly. Can the code include a big scary warning in caps maybe?

Ok, after a few discussions I think the conclusion is:
* the macro approach makes the strongest guarantees about correctness of IsEmpty and operator== implementations
* the macro approach is disliked because it depends on macros and because it makes reading the page_load_timing.h header much harder. this is undesirable because we have many consumers of this struct, so it's a goal to make the header readable.
* the sizeof approach has some drawbacks, in particular around padding between members. fortunately, for the types we are using, there is no padding, and we can be reasonably confident that this won't change going forward (Time and TimeDelta are spec'd as 64 bits, and Optional forces member alignment)
* the sizeof approach is desirable in that it keeps the header clean
* while the sizeof approach does not directly audit operator== and IsEmpty implementations, it seems very unlikely that a developer could add a new field without seeing the static assert warning, which instructs them to update IsEmpty and operator==. even if the dev misses this, the code reviewer would see a change to kNumTimeDeltas, and the context in the diff should also show them the comment above this, which instructs to update IsEmpty and operator==. Thus it seems very unlikely that we could miss updating IsEmpty/operator== with the static assert approach.

All of this combined has me leaning towards the static_assert approach. It sounds like Charles is ok with that. Kinuko, what do you think?

Charles, regarding a big scary warning, I have added "*** IMPORTANT **" to the comment. Do you think this is sufficient? Take a look and see what you think.

The sizeof-based change is here: https://codereview.chromium.org/2191163003

The IMPORTANT comment is good enough for me. I will defer to kinuko@ on the sizeof based change though.

#6- I'm fine with the static_assert approach too. (If this type of problem arises often in our code base it might be worth thinking about writing some neat clang plugin etc, but maybe not yet)  I'll comment on the CL for nit details & thanks for closing the loop.

You started fixing this bug over two years ago. Are you still working on it?