Add compile time assert to help catch new fields

chrome/common/page_load_metrics/page_load_timing.cc

Index: chrome/common/page_load_metrics/page_load_timing.cc
diff --git a/chrome/common/page_load_metrics/page_load_timing.cc b/chrome/common/page_load_metrics/page_load_timing.cc
index 21c3204489a6074d4735cb44d5676af36e648587..07408bdb05313b375caa94d67dea9c7476a89614 100644
--- a/chrome/common/page_load_metrics/page_load_timing.cc
+++ b/chrome/common/page_load_metrics/page_load_timing.cc
@@ -12,6 +12,28 @@ PageLoadTiming::PageLoadTiming(const PageLoadTiming& other) = default;
 
 PageLoadTiming::~PageLoadTiming() {}
 
+namespace {
+
+// *** IMPORTANT ***: When adding fields to PageLoadTiming, make sure to update
+// operator== and IsEmpty below to reflect your changes.

[kinuko, 2016/08/02 16:09:00]

nit: "update operator==, IsEmpty and kNumTImeDeltas below to..."

nit: now we have similar comments in 3 places (here, in page_load_timing.h and
error text for static_assert).

Maybe just update the comment in page_load_timing.h to include kNumTimeDeltas,
remove this comment but use this text (with big '*** IMPORTANT ***') as the
error text for the static_assert?

[Bryan McQuade, 2016/09/07 11:13:17]

Good idea, thanks! I updated the code to reflect your suggestion.

+const int kNumTimeDeltas = 12;
+
+const size_t kSizeOfTime = sizeof(base::Time);
+const size_t kSizeOfOptionalTimeDelta = sizeof(base::Optional<base::TimeDelta>);
+const size_t kSizeOfAllOptionalTimeDeltas =
+    kSizeOfOptionalTimeDelta * kNumTimeDeltas;
+
+// This assumes that the members of PageLoadTiming are stored without any
+// padding. This is true for base::Time and base::Optional<base::TimeDelta>.
+static_assert(
+    sizeof(page_load_metrics::PageLoadTiming) ==
+        kSizeOfTime + kSizeOfAllOptionalTimeDeltas,

[kinuko, 2016/08/02 16:09:00]

Using constexpr function might make these const's + static_assert a little
prettier?
(Kinda preferential, so feel free to ignore if you disagree)

constexpr size_t CalculatePageLoadTimingSize() {
  // This assumes that the members of PageLoadTiming are stored without...
  return sizeof(base::Time) /* sizeof navigation_start */ +
      sizeof(base::Optional<base::TimeDelta>) * kNumTimeDeltas;
}

static_assert(sizeof(PageLoadTiming) == CalculatePageLoadTimingSize(), "...");

[Bryan McQuade, 2016/09/07 11:13:17]

Yes, constexpr makes this easier to read. I made this change. Thanks!

+    "If you added a new TimeDelta to PageLoadTiming, update kNumTimeDeltas "
+    "above. Make sure to also update IsEmpty and operator== to reflect any "
+    "new fields you have added.");
+
+}  // namespace
+
 bool PageLoadTiming::operator==(const PageLoadTiming& other) const {
   return navigation_start == other.navigation_start &&
          response_start == other.response_start &&