Bug 41332

Summary: Implement unloadEventEnd, loadEventStart, and loadEventEnd
Product: WebKit Reporter: Tony Gentilcore <tonyg>
Component: New BugsAssignee: Tony Gentilcore <tonyg>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, commit-queue, dglazkov, japhet, pfeldman
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 30685, 41815, 41816, 41824    
Attachments:
Description Flags
Patch
none
Patch
none
Merge with 41442
none
Patch
none
Patch for landing
none
Patch for landing none

Description Tony Gentilcore 2010-06-28 21:59:15 PDT
Implement Web Timing navigationStart, fetchStart, load and unload events
Comment 1 Tony Gentilcore 2010-06-28 22:10:39 PDT
Created attachment 59985 [details]
Patch
Comment 2 Nate Chapin 2010-06-29 08:50:58 PDT
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
Comment 3 Tony Gentilcore 2010-06-29 14:12:24 PDT
Created attachment 60052 [details]
Patch
Comment 4 Tony Gentilcore 2010-07-02 10:38:57 PDT
Created attachment 60378 [details]
Merge with 41442
Comment 5 Tony Gentilcore 2010-07-02 10:41:47 PDT
(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
Comment 6 Tony Gentilcore 2010-07-07 17:27:15 PDT
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.
Comment 7 Tony Gentilcore 2010-07-07 17:35:10 PDT
Created attachment 60813 [details]
Patch
Comment 8 Nate Chapin 2010-07-08 09:42:54 PDT
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.
Comment 9 Tony Gentilcore 2010-07-08 10:57:16 PDT
Created attachment 60910 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2010-07-09 07:52:34 PDT
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'
Comment 11 WebKit Commit Bot 2010-07-09 09:17:11 PDT
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'
Comment 12 Tony Gentilcore 2010-07-09 10:35:23 PDT
Created attachment 61058 [details]
Patch for landing
Comment 13 Tony Gentilcore 2010-07-09 13:17:57 PDT
Landed r62984