Closed
Bug 659126
Opened 14 years ago
Closed 13 years ago
Implement additional NavigationTiming properties
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
People
(Reporter: Biesinger, Assigned: igor.bazarny)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
17.96 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
bug 570341 implements basic NavigationTiming support. this bug is for implementing the additional properties, for which bug 576006 laid the groundwork.
Assignee | ||
Comment 1•13 years ago
|
||
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)
Reporter | ||
Comment 2•13 years ago
|
||
Pushed to try: http://tbpl.mozilla.org/?tree=Try&rev=5917e8c0f843
Assignee | ||
Comment 3•13 years ago
|
||
- 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)
Reporter | ||
Comment 4•13 years ago
|
||
Try liked the previous patch; pushed the new one to try as well: http://tbpl.mozilla.org/?tree=Try&rev=33f166e535cf
Comment 5•13 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-
Reporter | ||
Comment 6•13 years ago
|
||
just to confirm - currently, only HTTP channels implement nsITimedChannel.
Assignee | ||
Comment 7•13 years ago
|
||
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•13 years ago
|
Attachment #542820 -
Attachment is obsolete: true
Reporter | ||
Comment 8•13 years ago
|
||
Pushed to try: http://tbpl.mozilla.org/?tree=Try&rev=c8c0168b9aae
Updated•13 years ago
|
Attachment #543746 -
Flags: review?(Olli.Pettay) → review+
Reporter | ||
Comment 9•13 years ago
|
||
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.
Description
•