The default bug view has changed. See this FAQ.

Implement additional NavigationTiming properties

RESOLVED FIXED

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Biesinger, Assigned: Igor Bazarny)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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

Comment 1

6 years ago
Created attachment 542485 [details] [diff] [review]
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)
Pushed to try:
http://tbpl.mozilla.org/?tree=Try&rev=5917e8c0f843
(Assignee)

Comment 3

6 years ago
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
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 5

6 years ago
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.
(Assignee)

Comment 7

6 years ago
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.
Attachment #543746 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

6 years ago
Attachment #542820 - Attachment is obsolete: true
Pushed to try:
http://tbpl.mozilla.org/?tree=Try&rev=c8c0168b9aae

Updated

6 years ago
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.