Bug 41332 – Implement unloadEventEnd, loadEventStart, and loadEventEnd

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tony Gentilcore]

Implement Web Timing navigationStart, fetchStart, load and unload events

[Tony Gentilcore]

Created attachment 59985 [details]
Patch

[Nate Chapin]

Comment on attachment 59985 [details]
Patch

> @@ -1496,6 +1498,9 @@ void FrameLoader::loadWithDocumentLoader(DocumentLoader* loader, FrameLoadType t
>  
>      ASSERT(m_frame->view());
>  
> +    m_frameLoadTimeline = FrameLoadTimeline();
> +    m_frameLoadTimeline.navigationStart = currentTime();
> +
>      if (m_pageDismissalEventBeingDispatched)
>          return;
>  
> @@ -2494,6 +2499,9 @@ void FrameLoader::continueLoadAfterWillSubmitForm()
>  
>      unsigned long identifier = 0;
>  
> +    ASSERT(!m_frameLoadTimeline.fetchStart);
> +    m_frameLoadTimeline.fetchStart = currentTime();
> +
>      if (Page* page = m_frame->page()) {
>          identifier = page->progress()->createUniqueIdentifier();
>          notifier()->assignIdentifierToInitialRequest(identifier, m_provisionalDocumentLoader.get(), m_provisionalDocumentLoader->originalRequest());

I don't know the precise sequence of this part of FrameLoader well enough to know for certain that these are the correct places for navigationStart and fetchStart. abarth might know better.

> +struct FrameLoadTimeline {
> +    FrameLoadTimeline()
> +        : navigationStart(0)
> +        , unloadEventStart(0)
> +        , unloadEventEnd(0)
> +        , fetchStart(0)
> +        , loadEventStart(0)
> +        , loadEventEnd(0)
> +    {
> +    }
> +
> +    double navigationStart;
> +    double unloadEventStart;
> +    double unloadEventEnd;
> +    double fetchStart;
> +    double loadEventStart;
> +    double loadEventEnd;
> +};
> +

This should be in FrameLoaderTypes.h

[Tony Gentilcore]

Created attachment 60052 [details]
Patch

[Tony Gentilcore]

Created attachment 60378 [details]
Merge with 41442

[Tony Gentilcore]

(In reply to comment #2)
> (From update of attachment 59985 [details])
> > @@ -1496,6 +1498,9 @@ void FrameLoader::loadWithDocumentLoader(DocumentLoader* loader, FrameLoadType t
> >  
> >      ASSERT(m_frame->view());
> >  
> > +    m_frameLoadTimeline = FrameLoadTimeline();
> > +    m_frameLoadTimeline.navigationStart = currentTime();
> > +
> >      if (m_pageDismissalEventBeingDispatched)
> >          return;
> >  
> > @@ -2494,6 +2499,9 @@ void FrameLoader::continueLoadAfterWillSubmitForm()
> >  
> >      unsigned long identifier = 0;
> >  
> > +    ASSERT(!m_frameLoadTimeline.fetchStart);
> > +    m_frameLoadTimeline.fetchStart = currentTime();
> > +
> >      if (Page* page = m_frame->page()) {
> >          identifier = page->progress()->createUniqueIdentifier();
> >          notifier()->assignIdentifierToInitialRequest(identifier, m_provisionalDocumentLoader.get(), m_provisionalDocumentLoader->originalRequest());
> 
> I don't know the precise sequence of this part of FrameLoader well enough to know for certain that these are the correct places for navigationStart and fetchStart. abarth might know better.

Adam, are you a good person to make sure these times are marked in the correct places? Or maybe you know someone else who should take a look?

> 
> > +struct FrameLoadTimeline {
> > +    FrameLoadTimeline()
> > +        : navigationStart(0)
> > +        , unloadEventStart(0)
> > +        , unloadEventEnd(0)
> > +        , fetchStart(0)
> > +        , loadEventStart(0)
> > +        , loadEventEnd(0)
> > +    {
> > +    }
> > +
> > +    double navigationStart;
> > +    double unloadEventStart;
> > +    double unloadEventEnd;
> > +    double fetchStart;
> > +    double loadEventStart;
> > +    double loadEventEnd;
> > +};
> > +
> 
> This should be in FrameLoaderTypes.h

[Tony Gentilcore]

To make this easier to review, I'm breaking it up into 3 patches. This patch now only implements unloadEventEnd, loadEventStart and loadEventEnd. Those marks are trivial and shouldn't need Adam to look at the trickier parts of FrameLoader.

[Tony Gentilcore]

Created attachment 60813 [details]
Patch

[Nate Chapin]

Comment on attachment 60813 [details]
Patch

r+ with a nit

> +    if (FrameLoader* frameLoader = frame() ? frame()->loader() : 0)
> +        frameLoader->frameLoadTimeline()->loadEventStart = currentTime();
>      dispatchEvent(Event::create(eventNames().loadEvent, false, false), document());
> +    if (FrameLoader* frameLoader = frame() ? frame()->loader() : 0)
> +        frameLoader->frameLoadTimeline()->loadEventEnd = currentTime();

I find these if statements a bit unclear. You don't need to null check the FrameLoader, so maybe just set a Frame* in the if() statement.

[Tony Gentilcore]

Created attachment 60910 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 60910 [details]
Patch for landing

Rejecting patch 60910 from commit-queue.

Unexpected failure when processing patch!  Please file a bug against webkit-patch.
Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 60910, '--test', '--parent-command=commit-queue', '--no-update']" exit_code: 1
Last 500 characters of output:
webkitpy/tool/commands/stepsequence.py", line 64, in _run
    step(tool, options).run(state)
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 68, in run
    if self._has_valid_reviewer(changelog_entry):
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 50, in _has_valid_reviewer
    if changelog_entry.reviewer():
AttributeError: 'NoneType' object has no attribute 'reviewer'

[WebKit Commit Bot]

Comment on attachment 60910 [details]
Patch for landing

Rejecting patch 60910 from commit-queue.

Unexpected failure when processing patch!  Please file a bug against webkit-patch.
Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 60910, '--test', '--parent-command=commit-queue', '--no-update']" exit_code: 1
Last 500 characters of output:
webkitpy/tool/commands/stepsequence.py", line 64, in _run
    step(tool, options).run(state)
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 68, in run
    if self._has_valid_reviewer(changelog_entry):
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 50, in _has_valid_reviewer
    if changelog_entry.reviewer():
AttributeError: 'NoneType' object has no attribute 'reviewer'

[Tony Gentilcore]

Created attachment 61058 [details]
Patch for landing

[Tony Gentilcore]

Landed r62984