Bug 41332

Created attachment 59985 [details] Patch

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

Created attachment 60052 [details] Patch

Created attachment 60378 [details] Merge with 41442

(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

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.

Created attachment 60813 [details] Patch

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.

Created attachment 60910 [details] Patch for landing

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 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'

Created attachment 61058 [details] Patch for landing

Landed r62984