659126 - Implement additional NavigationTiming properties

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

bug 570341 implements basic NavigationTiming support. this bug is for implementing the additional properties, for which bug 576006 laid the groundwork.

Comment 1

[Igor Bazarny]

Attachment: Pick up data from the nsITimedChannel

Take 1 for the low-level properties from NavigationTiming. Implements
domainLookupStart;
domainLookupEnd;
connectStart;
connectEnd;
requestStart;
responseStart;
responseEnd;

No data for secureConnectionStart, so this property removed from the interface (supposed to be undefined according to spec)

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

Pushed to try:
http://tbpl.mozilla.org/?tree=Try&rev=5917e8c0f843

Comment 3

[Igor Bazarny]

Attachment: Use nsITimedChannel from performance.timing implementation

- Removed channel-level methods from nsDOMNavigationTiming
- performance.timing wrapper class forwards requests to timing object or channel as appropriate
- Removed unimplemented handshakeStart (removed to secureConnectionStart in spec)
- No need to inherit from interfaces in nsDOMNavigationTiming, especially as Timing part is not fully implemented

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

Try liked the previous patch; pushed the new one to try as well:
http://tbpl.mozilla.org/?tree=Try&rev=33f166e535cf

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 542820 [details] [diff] [review]
Use nsITimedChannel from performance.timing implementation


>-// QueryInterface implementation for nsDOMNavigationTiming
>-NS_INTERFACE_MAP_BEGIN(nsDOMNavigationTiming)
>-  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIDOMPerformanceTiming)
>-  NS_INTERFACE_MAP_ENTRY(nsIDOMPerformanceTiming)
>-  NS_INTERFACE_MAP_ENTRY(nsIDOMPerformanceNavigation)
>-NS_INTERFACE_MAP_END
>+NS_IMETHODIMP nsDOMNavigationTiming::QueryInterface(REFNSIID aIID, void** aInstancePtr) 
>+{
>+  return NS_NOINTERFACE;
>+}
This is surprising and wrong.
If your object doesn't inherit nsISupports, it should have QueryInterface (in general).
Apparently you just want to implement addref/release.
There are macros for that, and you should remove NS_DECL_ISUPPORTS
from the .h file.


>+class nsDOMNavigationTiming {
{ should be in the next line.


> public:
>   nsDOMNavigationTiming();
> 
>   NS_DECL_ISUPPORTS
>-  NS_DECL_NSIDOMPERFORMANCETIMING
>   NS_DECL_NSIDOMPERFORMANCENAVIGATION
>+  NS_IMETHOD GetRedirectStart(DOMTimeMilliSec* aRedirectStart);
>+  NS_IMETHOD GetRedirectEnd(DOMTimeMilliSec* aEnd);
>+  NS_IMETHOD GetNavigationStart(DOMTimeMilliSec* aNavigationStart);
>+  NS_IMETHOD GetUnloadEventStart(DOMTimeMilliSec* aStart);
>+  NS_IMETHOD GetUnloadEventEnd(DOMTimeMilliSec* aEnd);
>+  NS_IMETHOD GetFetchStart(DOMTimeMilliSec* aStart);
>+  NS_IMETHOD GetDomLoading(DOMTimeMilliSec* aTime);
>+  NS_IMETHOD GetDomInteractive(DOMTimeMilliSec* aTime);
>+  NS_IMETHOD GetDomContentLoadedEventStart(DOMTimeMilliSec* aStart);
>+  NS_IMETHOD GetDomContentLoadedEventEnd(DOMTimeMilliSec* aEnd);
>+  NS_IMETHOD GetDomComplete(DOMTimeMilliSec* aTime);
>+  NS_IMETHOD GetLoadEventStart(DOMTimeMilliSec* aStart);
>+  NS_IMETHOD GetLoadEventEnd(DOMTimeMilliSec* aEnd);
Why are there NS_IMETHOD? Since the class doesn't implement any interface, 
there isn't any reason for these to be virtual.
Make these return nsresult.
Same thing with NS_DECL_NSIDOMPERFORMANCENAVIGATION methods.


>   if (nsGlobalWindow::HasPerformanceSupport()) {
>     if (!mPerformance) {
>       if (!mDoc) {
>         return NS_OK;
>       }
>       nsRefPtr<nsDOMNavigationTiming> timing = mDoc->GetNavigationTiming();
>-      if (timing) {
>-        mPerformance = new nsPerformance(timing);
>+      nsCOMPtr<nsITimedChannel> timedChannel(do_QueryInterface(mDoc->GetChannel()));
>+      PRBool timingEnabled = PR_FALSE;
>+      if (timing
>+        && timedChannel 
>+        && NS_SUCCEEDED(timedChannel->GetTimingEnabled(&timingEnabled))
>+        && timingEnabled) 
>+      {
&& should be in the end of a line.
{ should be after ') '


>+        mPerformance = new nsPerformance(timing, timedChannel);
>       }
>     }
>     NS_IF_ADDREF(*aPerformance = mPerformance);
>   }
>   return NS_OK;
> }
Per the spec window.performance shouldn't be null.
Do all the channels implement nsITimedChannel? If not, this code violates
the spec.

Perhaps some new tests?

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

just to confirm - currently, only HTTP channels implement nsITimedChannel.

Comment 7

[Igor Bazarny]

Attachment: Comments addressed

Thanks for comments, that was really helpful.
- nsDOMNsvigationTiming is now refcounted but doesn't implement any interfaces, and doesn't declare unused methods, including QueryInterface.
- use of nsITimedChannel is now optional--related properties revert to fetchStart if channel data are not available.

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

Pushed to try:
http://tbpl.mozilla.org/?tree=Try&rev=c8c0168b9aae

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

try is happy, pushed to m-c:
http://tbpl.mozilla.org/?tree=Firefox&rev=d7275a8a0b2a
http://hg.mozilla.org/mozilla-central/rev/d7275a8a0b2a