Last Comment Bug 659126 - Implement additional NavigationTiming properties
: Implement additional NavigationTiming properties
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
-- normal with 1 vote (vote)
: ---
Assigned To: Igor Bazarny
: Andrew Overholt [:overholt]
Depends on: 570341 576006
Blocks: 554045
  Show dependency treegraph
Reported: 2011-05-23 14:16 PDT by Christian :Biesinger (don't email me, ping me on IRC)
Modified: 2011-07-04 09:45 PDT (History)
5 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Pick up data from the nsITimedChannel (14.50 KB, patch)
2011-06-28 08:39 PDT, Igor Bazarny
no flags Details | Diff | Splinter Review
Use nsITimedChannel from performance.timing implementation (20.59 KB, patch)
2011-06-29 08:01 PDT, Igor Bazarny
bugs: review-
Details | Diff | Splinter Review
Comments addressed (17.96 KB, patch)
2011-07-04 05:15 PDT, Igor Bazarny
bugs: review+
Details | Diff | Splinter Review

Description User image Christian :Biesinger (don't email me, ping me on IRC) 2011-05-23 14:16:49 PDT
bug 570341 implements basic NavigationTiming support. this bug is for implementing the additional properties, for which bug 576006 laid the groundwork.
Comment 1 User image Igor Bazarny 2011-06-28 08:39:31 PDT
Created attachment 542485 [details] [diff] [review]
Pick up data from the nsITimedChannel

Take 1 for the low-level properties from NavigationTiming. Implements

No data for secureConnectionStart, so this property removed from the interface (supposed to be undefined according to spec)
Comment 2 User image Christian :Biesinger (don't email me, ping me on IRC) 2011-06-29 05:04:59 PDT
Pushed to try:
Comment 3 User image Igor Bazarny 2011-06-29 08:01:45 PDT
Created attachment 542820 [details] [diff] [review]
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 User image Christian :Biesinger (don't email me, ping me on IRC) 2011-06-30 04:46:37 PDT
Try liked the previous patch; pushed the new one to try as well:
Comment 5 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2011-07-03 12:49:45 PDT
Comment on attachment 542820 [details] [diff] [review]
Use nsITimedChannel from performance.timing implementation

>-// QueryInterface implementation for nsDOMNavigationTiming
>-  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIDOMPerformanceTiming)
>-  NS_INTERFACE_MAP_ENTRY(nsIDOMPerformanceTiming)
>-  NS_INTERFACE_MAP_ENTRY(nsIDOMPerformanceNavigation)
>+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_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.

>   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 User image Christian :Biesinger (don't email me, ping me on IRC) 2011-07-03 13:19:22 PDT
just to confirm - currently, only HTTP channels implement nsITimedChannel.
Comment 7 User image Igor Bazarny 2011-07-04 05:15:25 PDT
Created attachment 543746 [details] [diff] [review]
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 User image Christian :Biesinger (don't email me, ping me on IRC) 2011-07-04 05:34:34 PDT
Pushed to try:
Comment 9 User image Christian :Biesinger (don't email me, ping me on IRC) 2011-07-04 09:45:49 PDT
try is happy, pushed to m-c:

Note You need to log in before you can comment on or make changes to this bug.