add support for fake TimeTicks source in MetricsWebContentsObserver and PageLoadTracker |
||
Issue descriptionMany PageLoadMetricsObserver tests populate fake timings in PageLoadTiming to exercise certain code paths. However, there are additional timings recorded in MWCO and PageLoadTracker that are currently recorded using the system tick clock via TimeTicks::Now(). This makes writing tests that depend on these TimeTicks::Now() timings difficult, since it's not possible to stub those timings. To fix this we should allow for providing a fake time source. I think the way to do this is to add a base::TimeTicks Now(); method to PageLoadMetricsEmbedderInterface, which will be the source of all TimeTicks in our code. In production code this is based by TimeTicks::Now(). In tests this can be backed by something like SimpleTestTickClock: https://code.google.com/p/chromium/codesearch#chromium/src/base/test/simple_test_tick_clock.h Though since we aren't using the base class it may be overkill to actually use this class.
,
Jun 3 2016
,
Jun 3 2016
This is made increasingly complex by the fact that NavigationHandle provides its own TimeTicks for navigation start, and there's no way to stub its clock source.
,
Jun 3 2016
We could just override the time ticks taken from navigation start, and provide our own navigation_start TimeTicks in PageLoadExtraInfo.
,
Jun 3 2016
Yeah, that's a good idea. Things are getting tricky when baselining abort timings. We do things like notifying PageLoadTracker A of an abort at some other NavigationHandle's NavigationStart. Ideally we'd make it possible to stub out all times being processed but it's not immediately clear to me how to do this for the abort cases. Let me know if you see ways to go about this.
,
Oct 23
Circling back on this, I'm not sure that adding a Now() method to PageLoadMetricsEmbedderInterface is necessarily the right approach. It's one potential option but any solution that allows us to mock times would be sufficient here.
,
Oct 26
I run into this bug when I tried to add a test in core_page_load_metrics_observer_unittest. When we call web_contents()->WasHidden(), it will record a background time, but currently the time cannot be mocked because it uses Now() instead of CurrentTimeTicks() to record time. The navigation start recorded by NavigateAndCommit() also has the same issue. I've tried to make the clock mockable for background time and navigation start but not successful yet. As I've run out of time to dig into it. I guess posting the half-baked CL may help future works. Intuitively, because I want to make background time and navigation time mockable, I just need to replace all of the Now() with CurrentTimeTicks() along the path of these two variable. So I tried it in this CL: https://chromium-review.googlesource.com/c/chromium/src/+/1298324 (Sorry for the debugging lines) The result was, I could mocked background time but navigation start is still an unwanted value. I may carry on the digging later but as there is a urgent task so I have to leave it here for now. |
||
►
Sign in to add a comment |
||
Comment 1 by bmcquade@chromium.org
, Jun 3 2016Status: Started (was: Untriaged)