Report nav timing 2 instance as soon as it's requested.

third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp

Index: third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp
diff --git a/third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp b/third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp
index 7a1abafb002f56cdfc0163b912222f53e521d509..2170903dbceddc4219313480aba17da7468f05d6 100644
--- a/third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp
+++ b/third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp
@@ -5,162 +5,334 @@
 #include "core/timing/PerformanceNavigationTiming.h"
 
 #include "bindings/core/v8/V8ObjectBuilder.h"
+#include "core/dom/Document.h"
+#include "core/dom/DocumentTiming.h"
+#include "core/frame/LocalFrame.h"
+#include "core/loader/DocumentLoadTiming.h"
+#include "core/loader/DocumentLoader.h"
 #include "core/timing/PerformanceBase.h"
+#include "platform/network/ResourceTimingInfo.h"
 
 namespace blink {
 
 PerformanceNavigationTiming::PerformanceNavigationTiming(
-    double timeOrigin,
-    const String& requestedUrl,
-    double unloadEventStart,
-    double unloadEventEnd,
-    double loadEventStart,
-    double loadEventEnd,
-    unsigned short redirectCount,
-    double domInteractive,
-    double domContentLoadedEventStart,
-    double domContentLoadedEventEnd,
-    double domComplete,
-    NavigationType type,
-    double redirectStart,
-    double redirectEnd,
-    double fetchStart,
-    double responseEnd,
-    bool allowRedirectDetails,
-    bool hasSameOriginAsPreviousDocument,
-    ResourceLoadTiming* timing,
-    double lastRedirectEndTime,
-    double finishTime,
-    unsigned long long transferSize,
-    unsigned long long encodedBodyLength,
-    unsigned long long decodedBodyLength,
-    bool didReuseConnection)
-    : PerformanceResourceTiming("navigation",
-                                timeOrigin,
-                                timing,
-                                lastRedirectEndTime,
-                                finishTime,
-                                transferSize,
-                                encodedBodyLength,
-                                decodedBodyLength,
-                                didReuseConnection,
-                                true /*allowTimingDetails*/,  // TODO(sunjian):
-                                                              // Create an enum
-                                                              // for this.
-                                allowRedirectDetails,
-                                requestedUrl,
+    LocalFrame* frame,
+    ResourceTimingInfo* info,

[Kunihiko Sakamoto, 2017/02/13 08:22:29]

Since this constructor takes ownership of the ResourceTimingInfo, the argument
should be a PassRefPtr<ResourceTimingInfo>.

[sunjian, 2017/02/14 21:29:03]

I don't want PerformanceNavigationTiming to own ResourceTimingInfo alone. I
still want ResourceFetcher owning it at the same time.

[Kunihiko Sakamoto, 2017/02/15 06:35:30]

But once ResourceFetcher has gone, PerformanceNavigationTiming actually becomes
the owner, right?

I'm not sure whether we have clear style guideline for such shared-ownership
case. Perhaps haraken@ might know?

[sunjian, 2017/02/15 20:30:28]

Doesn't PerformanceNavigationTiming become the owner automatically?
RecourceTimingInfo will get ref()ed again when PerformanceNavigationTiming gets
constructed. If ResourceFetcher dies, ResourceTimingInfo gets deref()ed, but
ResourceTimingInfo should still be alive after that point right?

[Kunihiko Sakamoto, 2017/02/16 08:42:29]

Yeah ResourceTimingInfo* parameter will work, this is purely a style matter.
I'll defer this to OWNERS reviewer.

[sunjian, 2017/02/16 22:30:49]

Acknowledged.

+    double timeOrigin)
+    : PerformanceResourceTiming(info ? info->initialURL().getString() : "",
                                 "navigation",
-                                timeOrigin),
+                                0.0,
+                                0.0),
       m_timeOrigin(timeOrigin),
-      m_unloadEventStart(unloadEventStart),
-      m_unloadEventEnd(unloadEventEnd),
-      m_loadEventStart(loadEventStart),
-      m_loadEventEnd(loadEventEnd),
-      m_redirectCount(redirectCount),
-      m_domInteractive(domInteractive),
-      m_domContentLoadedEventStart(domContentLoadedEventStart),
-      m_domContentLoadedEventEnd(domContentLoadedEventEnd),
-      m_domComplete(domComplete),
-      m_type(type),
-      m_redirectStart(redirectStart),
-      m_redirectEnd(redirectEnd),
-      m_fetchStart(fetchStart),
-      m_responseEnd(responseEnd),
-      m_allowRedirectDetails(allowRedirectDetails),
-      m_hasSameOriginAsPreviousDocument(hasSameOriginAsPreviousDocument) {}
+      m_resourceTimingInfo(info),
+      m_frame(frame) {}
 
 PerformanceNavigationTiming::~PerformanceNavigationTiming() {}
 
+DEFINE_TRACE(PerformanceNavigationTiming) {
+  visitor->trace(m_frame);
+  PerformanceEntry::trace(visitor);
+}
+
+DocumentLoadTiming* PerformanceNavigationTiming::documentLoadTiming() const {
+  DocumentLoader* loader = documentLoader();
+  if (!loader)
+    return nullptr;
+
+  return &loader->timing();
+}
+
+DocumentLoader* PerformanceNavigationTiming::documentLoader() const {
+  if (!m_frame)
+    return nullptr;
+
+  return m_frame->loader().documentLoader();
+}
+
+const DocumentTiming* PerformanceNavigationTiming::documentTiming() const {
+  if (!m_frame)
+    return nullptr;
+
+  Document* document = m_frame->document();
+  if (!document)
+    return nullptr;
+
+  return &document->timing();
+}
+
+ResourceLoadTiming* PerformanceNavigationTiming::resourceLoadTiming() const {
+  if (!m_resourceTimingInfo)
+    return nullptr;
+  return m_resourceTimingInfo->finalResponse().resourceLoadTiming();
+}
+
+ExecutionContext* PerformanceNavigationTiming::getExecutionContext() const {
+  return m_frame ? m_frame->document() : nullptr;
+}
+
+bool PerformanceNavigationTiming::getAllowRedirectDetails() const {

[Kunihiko Sakamoto, 2017/02/13 08:22:29]

Can you precompute this in the constructor and store it in a member field?

[sunjian, 2017/02/14 21:29:03]

That would be ideal. But we don't know exactly when this is going to be
triggered. We provide PerformanceNavigationTiming instance as early as it's
requested which can be as soon as Performance instance is available.
Specifically, we rely on a final ResourceResponse instance in order to compute
this flag. And we don't know when this happens.

[Kunihiko Sakamoto, 2017/02/15 06:35:30]

Does that mean this function could return different values depending on timings?
That's really weird.

Looking at the code, ResourceTimingInfo::setFinalResponse() is called from
ResourceFetcher::handleLoaderFinish(), which is called when main document has
finished loading. So, if this function is called while document is still
loading, m_resourceTimingInfo->finalResponse() below would return an
uninitialized (default-constructed) value. This is clearly not correct.

PerformanceBase::allowsTimingRedirect() only needs the url and headers of final
response. Could we get these information at PerformanceNavigationTiming
construction time?

[sunjian, 2017/02/15 20:30:28]

If we can get the final response at construction time, that would be ideal. But
the nature of this change is to create PerformanceNavigationTiming instance as
soon as it is requested which can be as early as Performance instance becomes
available. And we have no control over when exactly it will be constructed. But
good news is that when final response is an empty object,
PerformanceBase::allowsTimingRedirect allows returns false. So the return value
is actually predictable. It's false before final response arrives, and becomes
whatever it should be and stay the same after it arrives.

[panicker, 2017/02/16 00:37:23]

Can we add an explicit check here with a comment: if the document has not
finished loading return false explicitly, instead of depending on lower layers
to do something reasonable.

[Kunihiko Sakamoto, 2017/02/16 08:42:29]

So unloadEventStart, unloadEventEnd, redirectCount, redirectStart, and
redirectEnd will return zero until the main resource finishes loading... That
sounds like a bug to me, because all the information needed to do the check
should be available by the time JS can start running (and requesting
PerformanceNavigationTiming instance).

We can address that later. Could you file a bug and put a TODO comment here with
the crbug link?

For now, +1 for adding an explicit check.

[sunjian, 2017/02/16 22:30:49]

I don't think there is a reasonable way to tell if the load has finished or not.
It doesn't return a pointer. It returns a default ResourceResponse object. It
seems to me checking whether what gets returned is a default or finalized object
is kind of hacky.

[sunjian, 2017/02/16 22:30:49]

As far as i know, NT1 kind of has the same problem. The timing allow flag also
keeps changing as time goes on. It seems more like a dilemma than a bug to me.
If we want to expose NavigationTiming instance as early as possible, which means
loading might not even finish yet, we have to rely on the current state of the
loading. For instance, there is no way for you to know whether this is one
redirect that is cross-origin or not until loading finishes. I can leave a TODO
here for sure. But i think it should be a good idea to bring the people who
speced this into discussion. 

And for explicit checking, do you have any suggestions? I can't check whether
it's a nullptr or not. It returns a default-constructed object.

+  ExecutionContext* context = getExecutionContext();
+  SecurityOrigin* securityOrigin = nullptr;
+  if (context)
+    securityOrigin = context->getSecurityOrigin();
+  if (!securityOrigin)
+    return false;
+  if (!m_resourceTimingInfo)
+    return false;
+
+  return PerformanceBase::allowsTimingRedirect(
+      m_resourceTimingInfo->redirectChain(),
+      m_resourceTimingInfo->finalResponse(), *securityOrigin, context);
+}
+
 DOMHighResTimeStamp PerformanceNavigationTiming::unloadEventStart() const {
-  if (!m_allowRedirectDetails || !m_hasSameOriginAsPreviousDocument)
+  bool allowRedirectDetails = getAllowRedirectDetails();
+  DocumentLoadTiming* timing = documentLoadTiming();
+
+  if (!allowRedirectDetails || !timing ||
+      !timing->hasSameOriginAsPreviousDocument())
     return 0;
   return PerformanceBase::monotonicTimeToDOMHighResTimeStamp(
-      m_timeOrigin, m_unloadEventStart);
+      m_timeOrigin, timing->unloadEventStart());
 }
 
 DOMHighResTimeStamp PerformanceNavigationTiming::unloadEventEnd() const {
-  if (!m_allowRedirectDetails || !m_hasSameOriginAsPreviousDocument)
-    return 0;
+  bool allowRedirectDetails = getAllowRedirectDetails();
+  DocumentLoadTiming* timing = documentLoadTiming();
 
-  return PerformanceBase::monotonicTimeToDOMHighResTimeStamp(m_timeOrigin,
-                                                             m_unloadEventEnd);
+  if (!allowRedirectDetails || !timing ||
+      !timing->hasSameOriginAsPreviousDocument())
+    return 0;
+  return PerformanceBase::monotonicTimeToDOMHighResTimeStamp(
+      m_timeOrigin, timing->unloadEventEnd());
 }
 
 DOMHighResTimeStamp PerformanceNavigationTiming::domInteractive() const {
-  return PerformanceBase::monotonicTimeToDOMHighResTimeStamp(m_timeOrigin,
-                                                             m_domInteractive);
+  const DocumentTiming* timing = documentTiming();
+  if (!timing)
+    return 0.0;
+  return PerformanceBase::monotonicTimeToDOMHighResTimeStamp(
+      m_timeOrigin, timing->domInteractive());
 }
 
 DOMHighResTimeStamp PerformanceNavigationTiming::domContentLoadedEventStart()
     const {
+  const DocumentTiming* timing = documentTiming();
+  if (!timing)
+    return 0.0;
   return PerformanceBase::monotonicTimeToDOMHighResTimeStamp(
-      m_timeOrigin, m_domContentLoadedEventStart);
+      m_timeOrigin, timing->domContentLoadedEventStart());
 }
 
 DOMHighResTimeStamp PerformanceNavigationTiming::domContentLoadedEventEnd()
     const {
+  const DocumentTiming* timing = documentTiming();
+  if (!timing)
+    return 0.0;
   return PerformanceBase::monotonicTimeToDOMHighResTimeStamp(
-      m_timeOrigin, m_domContentLoadedEventEnd);
+      m_timeOrigin, timing->domContentLoadedEventEnd());
 }
 
 DOMHighResTimeStamp PerformanceNavigationTiming::domComplete() const {
-  return PerformanceBase::monotonicTimeToDOMHighResTimeStamp(m_timeOrigin,
-                                                             m_domComplete);
+  const DocumentTiming* timing = documentTiming();
+  if (!timing)
+    return 0.0;
+  return PerformanceBase::monotonicTimeToDOMHighResTimeStamp(
+      m_timeOrigin, timing->domComplete());
 }
 
 DOMHighResTimeStamp PerformanceNavigationTiming::loadEventStart() const {
-  return PerformanceBase::monotonicTimeToDOMHighResTimeStamp(m_timeOrigin,
-                                                             m_loadEventStart);
+  DocumentLoadTiming* timing = documentLoadTiming();
+  if (!timing)
+    return 0.0;
+  return PerformanceBase::monotonicTimeToDOMHighResTimeStamp(
+      m_timeOrigin, timing->loadEventStart());
 }
 
 DOMHighResTimeStamp PerformanceNavigationTiming::loadEventEnd() const {
-  return PerformanceBase::monotonicTimeToDOMHighResTimeStamp(m_timeOrigin,
-                                                             m_loadEventEnd);
+  DocumentLoadTiming* timing = documentLoadTiming();
+  if (!timing)
+    return 0.0;
+  return PerformanceBase::monotonicTimeToDOMHighResTimeStamp(
+      m_timeOrigin, timing->loadEventEnd());
 }
 
-AtomicString PerformanceNavigationTiming::type() const {
-  switch (m_type) {
-    case NavigationType::Reload:
+AtomicString PerformanceNavigationTiming::getNavigationType(
+    NavigationType type,
+    const Document* document) {
+  if (document &&
+      document->pageVisibilityState() == PageVisibilityStatePrerender) {
+    return "prerender";
+  }
+  switch (type) {
+    case NavigationTypeReload:
       return "reload";
-    case NavigationType::BackForward:
+    case NavigationTypeBackForward:
       return "back_forward";
-    case NavigationType::Prerender:
-      return "prerender";
-    case NavigationType::Navigate:
+    case NavigationTypeLinkClicked:
+    case NavigationTypeFormSubmitted:
+    case NavigationTypeFormResubmitted:
+    case NavigationTypeOther:
       return "navigate";
   }
   NOTREACHED();
   return "navigate";
 }
 
+AtomicString PerformanceNavigationTiming::type() const {
+  DocumentLoader* loader = documentLoader();
+  if (!m_frame || !loader)
+    return "";  // Not available yet.

[Kunihiko Sakamoto, 2017/02/13 08:22:29]

Can this happen? I think navigation type should be available by the time scripts
start running.

[sunjian, 2017/02/14 21:29:03]

You are probably right. I'll go ahead and replace the checking with DCHECKS
then.

+  Document* document = m_frame->document();
+  return getNavigationType(loader->getNavigationType(), document);
+}
+
 unsigned short PerformanceNavigationTiming::redirectCount() const {
-  if (!m_allowRedirectDetails)
+  bool allowRedirectDetails = getAllowRedirectDetails();
+  DocumentLoadTiming* timing = documentLoadTiming();
+  if (!allowRedirectDetails || !timing)
     return 0;
-  return m_redirectCount;
+  return timing->redirectCount();
 }
 
-DOMHighResTimeStamp PerformanceNavigationTiming::fetchStart() const {
-  return PerformanceBase::monotonicTimeToDOMHighResTimeStamp(m_timeOrigin,
-                                                             m_fetchStart);
+// Overriding PerformanceResourceTiming's attributes.
+AtomicString PerformanceNavigationTiming::initiatorType() const {
+  return "navigation";
+}
+
+DOMHighResTimeStamp PerformanceNavigationTiming::workerStart() const {

[Kunihiko Sakamoto, 2017/02/13 08:22:29]

Can we reduce code duplication between PerformanceNavigationTiming and
PerformanceResourceTiming?
For example, workerStart could be simplified like this:


DOMHighResTimeStamp PerformanceResourceTiming::workerStart(ResourceLoadTiming*
timing) const {
  // common underlying implementation using |timing|
}

// virtual function
DOMHighResTimeStamp PerformanceResourceTiming::workerStart() const {
  return workerStart(m_timing);
}

DOMHighResTimeStamp PerformanceNavigationTiming::workerStart() const {
  return workerStart(resourceLoadTiming());
}

[Kunihiko Sakamoto, 2017/02/13 08:27:40]

Or even simpler:

// Overridden by PerformanceResourceTiming
virtual ResourceLoadTiming* PerformanceResourceTiming::resourceLoadTiming() {
  return m_timing;
}

// non-virtual
DOMHighResTimeStamp PerformanceResourceTiming::workerStart() const {
  ResourceLoadTiming* timing = resourceLoadTiming();
  // ...
}

[sunjian, 2017/02/14 21:29:03]

Done.

[sunjian, 2017/02/14 21:29:03]

Acknowledged.

+  ResourceLoadTiming* timing = resourceLoadTiming();
+  if (!timing || timing->workerStart() == 0.0)
+    return 0.0;
+  return PerformanceBase::monotonicTimeToDOMHighResTimeStamp(
+      m_timeOrigin, timing->workerStart());
 }
 
 DOMHighResTimeStamp PerformanceNavigationTiming::redirectStart() const {
-  if (!m_allowRedirectDetails)
+  bool allowRedirectDetails = getAllowRedirectDetails();
+  DocumentLoadTiming* timing = documentLoadTiming();
+  if (!allowRedirectDetails || !timing)
     return 0;
-  return PerformanceBase::monotonicTimeToDOMHighResTimeStamp(m_timeOrigin,
-                                                             m_redirectStart);
+  return PerformanceBase::monotonicTimeToDOMHighResTimeStamp(
+      m_timeOrigin, timing->redirectStart());
 }
 
 DOMHighResTimeStamp PerformanceNavigationTiming::redirectEnd() const {
-  if (!m_allowRedirectDetails)
+  bool allowRedirectDetails = getAllowRedirectDetails();
+  DocumentLoadTiming* timing = documentLoadTiming();
+  if (!allowRedirectDetails || !timing)
     return 0;
+  return PerformanceBase::monotonicTimeToDOMHighResTimeStamp(
+      m_timeOrigin, timing->redirectEnd());
+}
+
+DOMHighResTimeStamp PerformanceNavigationTiming::fetchStart() const {
+  DocumentLoadTiming* timing = documentLoadTiming();
+  if (!timing)
+    return 0.0;
+  return PerformanceBase::monotonicTimeToDOMHighResTimeStamp(
+      m_timeOrigin, timing->fetchStart());
+}
+
+DOMHighResTimeStamp PerformanceNavigationTiming::domainLookupStart() const {
+  ResourceLoadTiming* timing = resourceLoadTiming();
+  if (!timing || timing->dnsStart() == 0.0)
+    return fetchStart();
+  return PerformanceBase::monotonicTimeToDOMHighResTimeStamp(
+      m_timeOrigin, timing->dnsStart());
+}
+
+DOMHighResTimeStamp PerformanceNavigationTiming::domainLookupEnd() const {
+  ResourceLoadTiming* timing = resourceLoadTiming();
+  if (!timing || timing->dnsEnd() == 0.0)
+    return domainLookupStart();
   return PerformanceBase::monotonicTimeToDOMHighResTimeStamp(m_timeOrigin,
-                                                             m_redirectEnd);
+                                                             timing->dnsEnd());
 }
 
-DOMHighResTimeStamp PerformanceNavigationTiming::responseEnd() const {
+DOMHighResTimeStamp PerformanceNavigationTiming::connectStart() const {
+  DocumentLoader* loader = documentLoader();
+  if (!loader)
+    return domainLookupEnd();
+  ResourceLoadTiming* timing = resourceLoadTiming();
+  if (!timing || timing->connectStart() == 0.0 ||
+      m_resourceTimingInfo->finalResponse().connectionReused())
+    return domainLookupEnd();
+  double connectStart = timing->connectStart();
+  if (timing->dnsEnd() > 0.0 && timing->dnsEnd() > connectStart)
+    connectStart = timing->dnsEnd();
+
   return PerformanceBase::monotonicTimeToDOMHighResTimeStamp(m_timeOrigin,
-                                                             m_responseEnd);
+                                                             connectStart);
+}
+
+DOMHighResTimeStamp PerformanceNavigationTiming::connectEnd() const {
+  DocumentLoader* loader = documentLoader();
+  if (!loader)
+    return connectStart();
+  ResourceLoadTiming* timing = resourceLoadTiming();
+  if (!timing || timing->connectEnd() == 0.0 ||
+      m_resourceTimingInfo->finalResponse().connectionReused())
+    return connectStart();
+  return PerformanceBase::monotonicTimeToDOMHighResTimeStamp(
+      m_timeOrigin, timing->connectEnd());
+}
+
+DOMHighResTimeStamp PerformanceNavigationTiming::secureConnectionStart() const {
+  DocumentLoader* loader = documentLoader();
+  if (!loader)
+    return 0.0;
+  ResourceLoadTiming* timing = resourceLoadTiming();
+  if (!timing ||
+      timing->sslStart() == 0.0)  // Secure connection not negotiated.
+    return 0.0;
+  return PerformanceBase::monotonicTimeToDOMHighResTimeStamp(
+      m_timeOrigin, timing->sslStart());
+}
+
+DOMHighResTimeStamp PerformanceNavigationTiming::requestStart() const {
+  ResourceLoadTiming* timing = resourceLoadTiming();
+  if (!timing || timing->sendStart() == 0.0)
+    return connectEnd();
+  return PerformanceBase::monotonicTimeToDOMHighResTimeStamp(
+      m_timeOrigin, timing->sendStart());
+}
+
+DOMHighResTimeStamp PerformanceNavigationTiming::responseStart() const {
+  ResourceLoadTiming* timing = resourceLoadTiming();
+  if (!timing || timing->receiveHeadersEnd() == 0.0)
+    return requestStart();
+  return PerformanceBase::monotonicTimeToDOMHighResTimeStamp(
+      m_timeOrigin, timing->receiveHeadersEnd());
+}
+
+DOMHighResTimeStamp PerformanceNavigationTiming::responseEnd() const {
+  DocumentLoadTiming* timing = documentLoadTiming();
+  if (!timing)
+    return 0.0;
+  return PerformanceBase::monotonicTimeToDOMHighResTimeStamp(
+      m_timeOrigin, timing->responseEnd());
+}
+
+unsigned long long PerformanceNavigationTiming::transferSize() const {
+  return !m_resourceTimingInfo ? 0 : m_resourceTimingInfo->transferSize();
+}
+
+unsigned long long PerformanceNavigationTiming::encodedBodySize() const {
+  return !m_resourceTimingInfo
+             ? 0
+             : m_resourceTimingInfo->finalResponse().encodedBodyLength();
+}
+
+unsigned long long PerformanceNavigationTiming::decodedBodySize() const {
+  return !m_resourceTimingInfo
+             ? 0
+             : m_resourceTimingInfo->finalResponse().decodedBodyLength();
+}
+
+// Overriding PerformanceEntry's attributes.
+DOMHighResTimeStamp PerformanceNavigationTiming::duration() const {
+  return loadEventEnd();
 }
 
 void PerformanceNavigationTiming::buildJSONValue(