Add compile time assert to help catch new fields

chrome/common/page_load_metrics/page_load_timing.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/common/page_load_metrics/page_load_timing.h"
 
 namespace page_load_metrics {
 
 PageLoadTiming::PageLoadTiming() {}
 
 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 &&
          dom_content_loaded_event_start ==
              other.dom_content_loaded_event_start &&
          load_event_start == other.load_event_start &&
          first_layout == other.first_layout &&
          first_paint == other.first_paint &&
          first_text_paint == other.first_text_paint &&
          first_image_paint == other.first_image_paint &&
@@ skipping 14 matching lines @@
          !parse_blocked_on_script_load_from_document_write_duration;
 }
 
 PageLoadMetadata::PageLoadMetadata() {}
 
 bool PageLoadMetadata::operator==(const PageLoadMetadata& other) const {
   return behavior_flags == other.behavior_flags;
 }
 
 }  // namespace page_load_metrics