In https://codereview.chromium.org/1837233002 Shivani landed a change to use base::Optional for some of the timings used by page load metrics.
As a follow up, we should also switch all of the TimeDeltas in PageLoadTiming to use base::Optional.
We currently use a non-zero TimeDelta as an indicator that a given PageLoadTiming event occurred. For example, in DispatchObserverTimingCallbacks (https://cs.chromium.org/chromium/src/components/page_load_metrics/browser/metrics_web_contents_observer.cc?rcl=1467011815&l=207) we have logic like:
if (!new_timing.dom_content_loaded_event_start.is_zero() &&
last_timing.dom_content_loaded_event_start.is_zero())
observer->OnDomContentLoadedEventStart(new_timing, extra_info);
This ends up working correctly most of the time, but misses cases where a timing was observed but at zero milliseconds after navigation start.
The bigger issue is that this can make writing tests difficult in some cases. For instance, in https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer_unittest.cc, the test logic become unnecessarily complex as a result of having a zero TTFCP value in the test. To work around the issues, additional methods were exposed to allow the test to override certain aspects of the code behavior, which means these tests aren't actually even testing real production code flow - they have to inject certain calls that don't normally happen in production in order to exercise the intended codepaths.
To fix this, we should update the timings in PageLoadTiming to be wrapped in base::Optional.
There are a few complications that we need to address before we can do this:
1. we need to add support for IPC serialization of base::Optional. I've got a CL in progress that does this.
2. we need to avoid using PageLoadTiming with gmock, due to some rather subtle and unfortunate issues explained in https://groups.google.com/forum/#!topic/googletestframework/W-Hud3j_c6I. I started working on these and can try to finish the work.
Once the above are done, we can put together a change that updates the timings in PageLoadTiming to use base::Optional. We'll also have to update all code that uses PageLoadTiming to know how to process these Optional values, which is going to be a somewhat large task.
Comment 1 by shivanisha@chromium.org
, Jun 27 2016