1299118 - Implement Time to Interactive (TTI) for Telemetry/Profiler

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[:Dominik Strohmeier [:dstrohmeier]]

Time to Interactive (TTI) celebrates the state when a page is able to process user interactions after loading. It requires the First Meaningful Visual to have fired.

PRD is here: https://docs.google.com/document/d/1RLqGVNL43B4oxovgR2DqmElXcBvJHJekT1bcfYooEbk/edit#

Comment 1

[:Dominik Strohmeier [:dstrohmeier]]

Here are the specs that currently Chromium is discussing for TTI:
https://docs.google.com/document/d/11sWqwdfd3u1TwyZhsc-fB2NcqMZ_59Kz4XKiivp1cIg/edit?pref=2&pli=1#heading=h.ir858lrmrkf5

https://github.com/tdresser/time-to-interactive

"We consider the page interactive during any time window of length > 10 seconds for which the thread executing JavaScript contains no tasks longer than 50ms. This means that, if input shows up at any time during this time window, it will be a maximum of 50ms before event handlers for this event are executed."

There are still open questions that need validation, but we'll need the probe first so that we'll be able to validate.

Comment 2

[:Dominik Strohmeier [:dstrohmeier]]

Here's an experiment that Chromium did on measuring TTI based on different definitions: https://docs.google.com/spreadsheets/d/1aAN2htljj8rtJBwRW3UvfEgrdUgSyFfp2Y8XpvM80IY/edit#gid=548327667

The definitions used were:
TTI-Navstart-based: the first point after navigation in which we can find a 5s windows with no long task.
TTI-FCP-based: the first point after first contentful paint in which we can find a 5s windows with no long task.
TTI-DCLEE-based: the first point after domContentLoadedEventEnd in which we can find a 5s windows with no long task.

Group thread is here: https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=footer#!msg/progressive-web-metrics/G83qR5CR1kg/PA1hJ1k2AQAJ

Comment 3

[:Harald Kirschner :digitarald]

Clarified the minimal scope for this bug, which is only telemetry and profiler. Developer facing numbers depend on validation and spec stabilizing.

Comment 4

[:Harald Kirschner :digitarald]

Related analysis on TTI from Google, comparing 4 different approaches on Chrome traces:

  https://docs.google.com/document/d/1jbvwxj_GJtiTTqFM8dsVzCIy5XeKL5qtIAvuimcXb1o/edit

In the discussion thread, tdresser did a great of job of visualizing the raw data:

https://groups.google.com/a/chromium.org/d/msg/progressive-web-metrics/Vba3oNMZEPk/IZIQfMuODQAJ

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP implementation of Long Tasks API internals

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP of TTI implementation for profiler

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Example profile with LongTask and TTI markers
https://perfht.ml/2O1H8l8

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP of TTI implementation for profiler

Fix time used for TTI marker.  Note: still printf's when it adds a marker, for ease in investigating TTI stability

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP implementation of Long Tasks API internals

Updated to count Idle and non-idle pauses separately

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP of TTI implementation for profiler

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Implement Long Tasks API internals (not DOM access to it)

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

Attachment: non-spec TTI (really TTFI) implementation for profiler

yes, I realize it's TTFI currently.  If you want I can change the name.   I have no particular intenttion to expose it by default to the web; it will be behind a pref like FNBP - it's mostly for our own measurement use.

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Expose TTI approximation in PerformanceTiming behind a pref (off by default)

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Measure TTI (or TTFI at the moment) in Raptor TP6

Comment 15

[Bobby Holley (:bholley)]

Comment on attachment 9014191 [details] [diff] [review]
non-spec TTI (really TTFI) implementation for profiler

Review of attachment 9014191 [details] [diff] [review]:
-----------------------------------------------------------------

I'll defer to Markus on this one.

Comment 16

[Nathan Froyd [:froydnj]]

Comment on attachment 9014188 [details] [diff] [review]
Implement  Long Tasks API internals (not DOM access to it)

Review of attachment 9014188 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with reasonable answers to the questions and suggestions below.

::: xpcom/threads/nsThread.cpp
@@ +653,5 @@
>    , mCurrentEventStart(TimeStamp::Now())
>    , mCurrentPerformanceCounter(nullptr)
>  {
> +  mLastLongTaskEnd = mCurrentEventStart;
> +  mLastLongNonIdleTaskEnd = mCurrentEventStart;

Can we just initialize these in the initialization list, please?

@@ +675,5 @@
>    , mCurrentEventStart(TimeStamp::Now())
>    , mCurrentPerformanceCounter(nullptr)
>  {
> +  mLastLongTaskEnd = mCurrentEventStart;
> +  mLastLongNonIdleTaskEnd = mCurrentEventStart;

Same question here.

@@ +829,5 @@
> +}
> +
> +NS_IMETHODIMP
> +nsThread::GetLastLongNonIdleTaskEnd(TimeStamp* _retval) {
> +  *_retval = mLastLongNonIdleTaskEnd;

Should these getters be main-thread-only, so accesses to them are guaranteed to not be racing with event loop processing setting them?

@@ +1251,5 @@
>  
>        event->Run();
>  
> +      mozilla::TimeDuration duration;
> +      // Remember the last 50ms+ task on mainthread for Long Task.

I think if we're going to enshrine 50ms in a comment here, we should also move LONGTASK_BUSY_WINDOW_MS to this spot as a static const thing.

::: xpcom/threads/nsThread.h
@@ +155,5 @@
>  
>    static uint32_t MaxActiveThreads();
>  
> +  const mozilla::TimeStamp& LastLongTaskEnd() { return mLastLongTaskEnd; }
> +  const mozilla::TimeStamp& LastLongNonIdleTaskEnd() { return mLastLongNonIdleTaskEnd; }

These do not appear to be used in subsequent patch sets?

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

> >    , mCurrentEventStart(TimeStamp::Now())
> >    , mCurrentPerformanceCounter(nullptr)
> >  {
> > +  mLastLongTaskEnd = mCurrentEventStart;
> > +  mLastLongNonIdleTaskEnd = mCurrentEventStart;
> 
> Can we just initialize these in the initialization list, please?

Avoiding calling ::Now() multiple times, and ensuring they're consistent.  We can't do that in the list as best I know

> > +  const mozilla::TimeStamp& LastLongTaskEnd() { return mLastLongTaskEnd; }
> > +  const mozilla::TimeStamp& LastLongNonIdleTaskEnd() { return mLastLongNonIdleTaskEnd; }
> 
> These do not appear to be used in subsequent patch sets?

The DOMNavigation changes should be using these

Comment 18

[Robert Wood [:rwood]]

Comment on attachment 9014194 [details] [diff] [review]
Measure TTI (or TTFI at the moment) in Raptor TP6

Review of attachment 9014194 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/raptor/webext/raptor/measure.js
@@ +213,5 @@
> +    // TTI requires running at least 5 seconds past last "busy" point, give 100 seconds here.
> +    // Some pages will never get 5 seconds without a busy period!
> +    if (gRetryCounter <= 50) {
> +      console.log("\TTI is not yet available (0), retry number " + gRetryCounter + "...\n");
> +      window.setTimeout(measureTTI, 200);

When running locally I'm seeing it fail to get TTI when running almost every time (i.e. every tp6 page, at least once in 25 pagecycles doesn't get TTI on-time). Same behaviour consistently in production on your try push for tp6-youtube.

So the retries should be increased. When I tested before I had 1500 retries at 100ms each and the longest I saw in my limited testing to get TTI was about 1350 retries. I filed Bug 1496544 as a follow-up to not fail out raptor if TTI is never received.

Comment 19

[Daniel Holbert [:dholbert]]

(In reply to Randell Jesup [:jesup] from comment #17)
> > >    , mCurrentEventStart(TimeStamp::Now())
> > >    , mCurrentPerformanceCounter(nullptr)
> > >  {
> > > +  mLastLongTaskEnd = mCurrentEventStart;
> > > +  mLastLongNonIdleTaskEnd = mCurrentEventStart;
> > 
> > Can we just initialize these in the initialization list, please?
> 
> Avoiding calling ::Now() multiple times, and ensuring they're consistent. 
> We can't do that in the list as best I know

Can't we do...

  , mCurrentEventStart(TimeStamp::Now())
  [...]
  , mLastLongTaskEnd(mCurrentEventStart)
  , mLastLongNonIdleTaskEnd(mCurrentEventStart)

According to https://stackoverflow.com/questions/10720377/can-member-variables-be-used-to-initialize-other-members-in-an-initialization-li , it seems like this should work, as long as the member-variable ordering is OK (which warnings-as-errors should validate for us).

Comment 20

[Robert Wood [:rwood]]

Comment on attachment 9014194 [details] [diff] [review]
Measure TTI (or TTFI at the moment) in Raptor TP6

Review of attachment 9014194 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/raptor/webext/raptor/measure.js
@@ +215,5 @@
> +    if (gRetryCounter <= 50) {
> +      console.log("\TTI is not yet available (0), retry number " + gRetryCounter + "...\n");
> +      window.setTimeout(measureTTI, 200);
> +    } else {
> +      console.log("\nunable to get a value for TTI after " + gRetryCounter + " retries\n");

If you'd like to land this now, instead of the console.log "unable to get a value'.. message here, just submit a fake result like a zero:

sendResult("tti", 0);

That would prevent the timeouts, and we can later follow up in Bug 1496544 to ignore these values when TTI is not found.

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

Attachment: change TTI to TTFI (Time To First Interactive)

Comment 22

[Bobby Holley (:bholley)]

Comment on attachment 9014843 [details] [diff] [review]
change TTI to TTFI (Time To First Interactive)

Review of attachment 9014843 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the WebIDL change.

Comment 23

[Robert Wood [:rwood]]

FYI, missed a spot in the latest patch, build error:

/mozilla-unified/dom/base/nsDOMNavigationTiming.cpp:332:83: error: use of undeclared identifier 'mTTI'; did you mean 'mTTFI'?"

Comment 24

[Robert Wood [:rwood]]

Comment on attachment 9014843 [details] [diff] [review]
change TTI to TTFI (Time To First Interactive)

Review of attachment 9014843 [details] [diff] [review]:
-----------------------------------------------------------------

Fixed build locally (Comment 23) and had a look. Raptor successfully captures TTFI numbers except for YouTube, it times out (see notes in-line).

R+ with the build fix and the issue re: TTFI retries time addressed.

Thanks :jesup!

::: testing/raptor/raptor/manifest.py
@@ +83,5 @@
>          if "fcp" in test_details['measure']:
>              test_settings['raptor-options']['measure']['fcp'] = True
>          if "hero" in test_details['measure']:
>              test_settings['raptor-options']['measure']['hero'] = test_details['hero'].split()
>          if "tti" in test_details['measure']:

Note: in the raptor-tp6.ini, it is still called 'tti', i.e.:

'measure = fnbpaint, hero, dcf, tti'

Which still works because of line 87 here. The INI and the line above should probably both be changed to 'ttfi' to match.

::: testing/raptor/webext/raptor/measure.js
@@ +214,4 @@
>      // Some pages will never get 5 seconds without a busy period!
> +    if (gRetryCounter <= 500) {
> +      console.log("\TTFI is not yet available (0), retry number " + gRetryCounter + "...\n");
> +      window.setTimeout(measureTTFI, 200);

An issue here - there is 100 seconds max given for TTFI data to be available. However in the raptor_tp6.ini, page_cycles is set to 30 seconds. Which means the raptor page_timeout overrides this 100 seconds after 30 seconds.

That is happening with YouTube - it seems with just 30 seconds, we can't get TTFI for Youtube at least most times. Which is causing the page_timeout to kick in at 30 seconds and the raptor test to fail, since we haven't reached the 100 second internal capturing max to substitute in a '0' value.

I recommend:

1. Not measuring TTFI on youtube at all, and

2. Reducing this internal TTFI capturing time limit to 25 seconds to avoid the page_timeout. There are 25 page cycles of 4 different pages (so far) and with a 25 second max, that can cause a very long overall tp6 job time.  Even with 25 seconds, that might cause the overall tp6 raptor taskcluster job time limit of 30 minutes to be exceeded. So this may need to be adjusted again after some exposure in production. Thanks!

Comment 25

[Markus Stange [:mstange]]

Comment on attachment 9014188 [details] [diff] [review]
Implement  Long Tasks API internals (not DOM access to it)

Review of attachment 9014188 [details] [diff] [review]:
-----------------------------------------------------------------

::: tools/profiler/public/ProfilerMarkerPayload.h
@@ +350,5 @@
> +class LongTaskMarkerPayload : public ProfilerMarkerPayload
> +{
> +public:
> +  LongTaskMarkerPayload(const mozilla::TimeStamp& aStartTime,
> +                   const mozilla::TimeStamp& aEndTime)

bad indent

Comment 26

[Markus Stange [:mstange]]

Comment on attachment 9014191 [details] [diff] [review]
non-spec TTI (really TTFI) implementation for profiler

Review of attachment 9014191 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDOMNavigationTiming.cpp
@@ +267,5 @@
> +{
> +  // Check TTI: see if it's been 5 seconds since the last Long Task
> +  TimeStamp now = TimeStamp::Now();
> +  if (mNonBlankPaint.IsNull()) {
> +    MOZ_CRASH("TTI timeout with no non-blank-paint?");

Could also use MOZ_RELEASE_ASSERT here. The return will never be hit anyway.

@@ +272,5 @@
> +    return;
> +  }
> +
> +  nsCOMPtr<nsIThread> mainThread = do_GetMainThread();
> +  TimeStamp longtask;

longtask -> lastLongTaskEnd?

@@ +273,5 @@
> +  }
> +
> +  nsCOMPtr<nsIThread> mainThread = do_GetMainThread();
> +  TimeStamp longtask;
> +  mainThread->GetLastLongNonIdleTaskEnd(&longtask);

Does this differ from mLastLongNonIdleTask? If so, please add a comment about how they differ. If not, why do we need it?

@@ +276,5 @@
> +  TimeStamp longtask;
> +  mainThread->GetLastLongNonIdleTaskEnd(&longtask);
> +  if (!longtask.IsNull()) {
> +    TimeDuration delta = now - longtask;
> +    if (delta.ToMilliseconds() < 5*1000) {

spaces around *

@@ +285,5 @@
> +      return;
> +    }
> +  }
> +  // To correctly implement TTI as proposed, we'd need to use FCP instead
> +  // of FNBP to start at, and not fire it until there are no more than 2

Please add the definitions of FCP and FNBP here, and say that we don't use FCP because we haven't implemented FCP.

@@ +294,5 @@
> +  // decreases to 2 (or record that point and let the normal timer here
> +  // handle it)
> +
> +  // TTI has occurred!  TTI is either FCP (aka for us NonBlankPaint) or
> +  // mDOMContentLoadedEventStart, whichever is later if there are no long

Weird, why DOMContentLoadedEventStart and not DOMContentLoadedEventEnd? In the time during which the DOMContentLoaded event handler runs, we're surely not interactive because we're running the handler.

@@ +310,5 @@
> +      mTTI = mNonBlankPaint;
> +    }
> +  } else {
> +    mTTI = longtask;
> +  }

This is a bit hard to read but seems correct. Maybe a helper function would make it more readable?

mTTI = MaxOrNull(longtask, MaxOrNull(mNonBlankPaint, mDOMContentLoadedEventStart);

However, this code actually doesn't match the comment. The comment says "If there was a long task, it's the long task time. Otherwise it's either FCP or DCLES." The code says "it's whatever completed last, out of { long task, FCP, DCLES }."
If the code is correct, please adjust the comment.

@@ +338,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  mLastLongNonIdleTask = aWhen;
> +  if (mTTITimer) {
> +    // move check time to 5 seconds from now.  Note that this may *never* end up firing

Maybe say "Reset the check time" to make it clearer that the existing timer is canceled?

@@ +340,5 @@
> +  mLastLongNonIdleTask = aWhen;
> +  if (mTTITimer) {
> +    // move check time to 5 seconds from now.  Note that this may *never* end up firing
> +    // on a site that has periodic long tasks or too many active loads!
> +    mTTITimer->InitWithNamedFuncCallback(TTITimeoutCallback, this, 5*1000,

spaces around * please

@@ +372,5 @@
>      profiler_add_marker(marker.get());
>    }
>  #endif
>  
> +  // start window for TTI

Please remove this comment or make it a full sentence. But the comment 6 lines further down already seems sufficient.

::: dom/base/nsDOMNavigationTiming.h
@@ +96,5 @@
>    DOMTimeMilliSec GetTimeToNonBlankPaint() const
>    {
>      return TimeStampToDOM(mNonBlankPaint);
>    }
> +  DOMTimeMilliSec GetTimeToTTI() const

"time to time to interactive" sounds a tad repetitive, but oh well

@@ +175,5 @@
>  
> +  static void TTITimeoutCallback(nsITimer* aTimer, void *aClosure);
> +  void TTITimeout(nsITimer* aTimer);
> +
> +  void NotifyLongTask(mozilla::TimeStamp aWhen);

Is this the time when the long task started or when it ended? Please add a comment.

Comment 27

[Randell Jesup [:jesup] (needinfo me)]

> > +  nsCOMPtr<nsIThread> mainThread = do_GetMainThread();
> > +  TimeStamp longtask;
> > +  mainThread->GetLastLongNonIdleTaskEnd(&longtask);
> 
> Does this differ from mLastLongNonIdleTask? If so, please add a comment
> about how they differ. If not, why do we need it?

Turns out we don't need it (at least for now, though we might for exposing LongTasks behind a pref) and nothing was calling NotifyLongtask().  Removed for now
 
> > +  // TTI has occurred!  TTI is either FCP (aka for us NonBlankPaint) or
> > +  // mDOMContentLoadedEventStart, whichever is later if there are no long
> 
> Weird, why DOMContentLoadedEventStart and not DOMContentLoadedEventEnd? In
> the time during which the DOMContentLoaded event handler runs, we're surely
> not interactive because we're running the handler.

yes - though the "time" of an event would be when it fires, not when user code from that firing finishes.  I used to have it as End, and reading a different doc on this from google they do say End there, so switching it.

> However, this code actually doesn't match the comment. The comment says "If
> there was a long task, it's the long task time. Otherwise it's either FCP or
> DCLES." The code says "it's whatever completed last, out of { long task,
> FCP, DCLES }."
> If the code is correct, please adjust the comment.

I adjusted the comment; the "spec" is less than clear here.

Comment 28

[Randell Jesup [:jesup] (needinfo me)]

Attachment: interdiffs for review response f=mstange

Comment 29

[Randell Jesup [:jesup] (needinfo me)]

Attachment: change TTI to TTFI (Time To First Interactive)

Comment 30

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 9016030 [details] [diff] [review]
change TTI to TTFI (Time To First Interactive)

Review of attachment 9016030 [details] [diff] [review]:
-----------------------------------------------------------------

Carry forward r+ from bholley, rwood and mstange (just rebased on top of changes to the main TTI/TTFI patch)

Comment 31

[Randell Jesup [:jesup] (needinfo me)]

Attachment: non-spec TTI (really TTFI) implementation for profiler

updated patch

Comment 32

[Markus Stange [:mstange]]

Comment on attachment 9016027 [details] [diff] [review]
interdiffs for review response f=mstange

Review of attachment 9016027 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDOMNavigationTiming.cpp
@@ +264,5 @@
>  
> +// Return the max of t1 and t2, or max N ms after the earlier one.
> +// Handle the edge case of a late wakeup where there was >5 seconds after
> +// DOMContentLoaded or lastLongTaskEnded without the other, but it happened
> +// after 5 seconds and before we woke up.

I don't understand this sentence. Can you rephrase it in terms of aT1, aT2 and aWindowSize?

@@ +266,5 @@
> +// Handle the edge case of a late wakeup where there was >5 seconds after
> +// DOMContentLoaded or lastLongTaskEnded without the other, but it happened
> +// after 5 seconds and before we woke up.
> +static TimeStamp&
> +MaxWithWindow(TimeStamp& aT1, TimeStamp& aT2, uint32_t aWindowSize)

Please use const references, and make aWindowSize a TimeDuration so that you don't have to add a comment about what unit it is in.

Comment 33

[Randell Jesup [:jesup] (needinfo me)]

Attachment: review update

Comment 34

[Randell Jesup [:jesup] (needinfo me)]

Attachment: non-spec TTI (really TTFI) implementation for profiler

Approximates TimeToIinteractive or really TimeToFirstInteractive
* * *
Bug 1299118: review update

Comment 35

[Markus Stange [:mstange]]

Comment on attachment 9016057 [details] [diff] [review]
review update

Review of attachment 9016057 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDOMNavigationTiming.cpp
@@ +266,5 @@
> +// Handles the edge case of a late wakeup: where there was more than Nms
> +// after one (of aT1 or aT2) without the other, but the other happened
> +// after Nms and before we woke up.  For example, if aT1 was 10 seconds
> +// after aT2, but we woke up late (after aT1) we don't want to return aT1
> +// if the window is 5 seconds.

Sorry I'm still having trouble understanding the "waking up" metaphor. Which time here corresponds to the time at which we woke up?

Comment 36

[Randell Jesup [:jesup] (needinfo me)]

> > +// Handles the edge case of a late wakeup: where there was more than Nms
> > +// after one (of aT1 or aT2) without the other, but the other happened
> > +// after Nms and before we woke up.  For example, if aT1 was 10 seconds
> > +// after aT2, but we woke up late (after aT1) we don't want to return aT1
> > +// if the window is 5 seconds.
> 
> Sorry I'm still having trouble understanding the "waking up" metaphor. Which
> time here corresponds to the time at which we woke up?

We start a 5s timer at a longtask at X.
At X+5 we should wake up, and if there's been no DCLEnd, we would put the TTFI at X.
If we instead wake up late (say X+10), and DCLEnd happend at X+7, we would look at that and say "ok, TTFI should be at X+7" if we don't check an explicit window (of 5s from X).

So, no time (aT1 or aT2) is when we woke up, just that waking up late might allow the second event to have occurred -- outside of the 5s window -- and we might mis-calculate.  Admittedly unlikely and non-critical, but easy to fix like this.

Comment 37

[Randell Jesup [:jesup] (needinfo me)]

Attachment: non-spec TTI (really TTFI) implementation for profiler

Approximates TimeToIinteractive or really TimeToFirstInteractive
* * *
Bug 1299118: review update

Comment 38

[Markus Stange [:mstange]]

Comment on attachment 9016292 [details] [diff] [review]
non-spec TTI (really TTFI) implementation for profiler

Review of attachment 9016292 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! It's clear to me now. Just three tiny nitpicks left.

::: dom/base/nsDOMNavigationTiming.cpp
@@ +265,5 @@
> +// Return the max of aT1 and aT2, or the lower of the two if there's more
> +// than Nms (the window size) between them.  In other words, the window
> +// starts at the lower of aT1 and aT2, and we only want to respect
> +// timestamps within the window (and pick the max of those).
> +

Sorry, really nitpicking here now, but could you make this empty line also start with // so that it's clear that the two paragraphs belong to the same comment?

@@ +266,5 @@
> +// than Nms (the window size) between them.  In other words, the window
> +// starts at the lower of aT1 and aT2, and we only want to respect
> +// timestamps within the window (and pick the max of those).
> +
> +// Handles the edge case of a late wakeup: where there was more than Nms

Could you change "Handles" to "This approach handles", so that it's clearer that the explanation that follows only describes implications of the sentence above, and doesn't add any further requirements?

@@ +272,5 @@
> +// after Nms and before we woke up.  For example, if aT1 was 10 seconds
> +// after aT2, but we woke up late (after aT1) we don't want to return aT1
> +// if the window is 5 seconds.
> +static const TimeStamp&
> +MaxWithWindow(const TimeStamp& aT1, const TimeStamp& aT2,

How would you feel about the name MaxWithinWindowBeginningAtMin, just to drive the point home?

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f7bb583fbb5
Implement  Long Tasks API internals (not DOM access to it) r=froyd,mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5adc30bdf7f
non-spec TimeToFirstInteractive implementation behind a pref r=mstange,bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/14451eb9a2b8
Measure TTI (or TTFI at the moment) in Raptor TP6 r=rwood

Comment 40

[Stefan Hindli [:stefan_hindli]]

Backed out 3 changesets (Bug 1299118) for ES lint failures

Failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed,busted,exception&classifiedState=unclassified&selectedJob=204825293
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=204825293&repo=mozilla-inbound&lineNumber=252

Comment 41

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a40850883ff
Backed out 3 changesets for ES lint failures

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f83ebaabaa3
Implement  Long Tasks API internals (not DOM access to it) r=froyd,mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa3cf6dbf466
non-spec TimeToFirstInteractive implementation behind a pref r=mstange,bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/69dd27f16b71
Measure TTI (or TTFI at the moment) in Raptor TP6 r=rwood

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd60c9679d21
Android bustage fix for no-gecko-profiler builds r=bustage

Comment 44

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/9f83ebaabaa3
https://hg.mozilla.org/mozilla-central/rev/fa3cf6dbf466
https://hg.mozilla.org/mozilla-central/rev/69dd27f16b71
https://hg.mozilla.org/mozilla-central/rev/fd60c9679d21