Closed Bug 659126 Opened 13 years ago Closed 13 years ago

Implement additional NavigationTiming properties

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: Biesinger, Assigned: igor.bazarny)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

bug 570341 implements basic NavigationTiming support. this bug is for implementing the additional properties, for which bug 576006 laid the groundwork.
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)
- 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
Attachment #542485 - Attachment is obsolete: true
Attachment #542820 - Flags: review?(Olli.Pettay)
Try liked the previous patch; pushed the new one to try as well:
http://tbpl.mozilla.org/?tree=Try&rev=33f166e535cf
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?
Attachment #542820 - Flags: review?(Olli.Pettay) → review-
just to confirm - currently, only HTTP channels implement nsITimedChannel.
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.
Attachment #543746 - Flags: review?(Olli.Pettay)
Attachment #542820 - Attachment is obsolete: true
Attachment #543746 - Flags: review?(Olli.Pettay) → review+
try is happy, pushed to m-c:
http://tbpl.mozilla.org/?tree=Firefox&rev=d7275a8a0b2a
http://hg.mozilla.org/mozilla-central/rev/d7275a8a0b2a
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.