570341 - Initial implementation of web timing specification.

Status: RESOLVED FIXED

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

Description

[Sergey Novikov]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US) AppleWebKit/533.4 (KHTML, like Gecko) Chrome/5.0.375.70 Safari/533.4
Build Identifier: 

Last web timing specification http://dev.w3.org/2006/webapi/WebTiming/

Reproducible: Always

Comment 1

[Sergey Novikov]

Attachment: Initial implementation of web timing.

Comment 2

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

Comment on attachment 449469 [details] [diff] [review]
Initial implementation of web timing.

r=biesi on the docshell changes, but see the comment below. requesting review from sicking for the dom/ part.

+++ b/docshell/base/nsDocShell.cpp
+    mTiming->NotifyRedirectStart();
     if (!(aStateFlags & STATE_IS_DOCUMENT))
         return; // not a toplevel document

Shouldn't you call NotifyRedirectStart after this if?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Using NAVIGATION_NEW_WINDOW for loads in subframes seems wrong, per that spec.

Using PR_Now for those timestamps seems incorrect as well (and in particular, it has the wrong units).

Why is nsNavigationTiming needed?

nsNavigationTiming is exploitable if it outlives the docshell, no?

Please add the new member to the end of nsIDOMWindowInternal.

Is the new member replaceable?  If not, won't it break any script that happens to use a global variable named "timing"?  Why is that acceptable?  (This is a spec issue, not an implementation issue.)

Comment 4

[Tony Gentilcore]

I'm working on landing the namespace in WebKit here:
https://bugs.webkit.org/show_bug.cgi?id=38924

The spec has been updated from "window.timing" -> "window.performance.timing". I just wanted to verify whether you are following that guideline and updating to "window.performance.timing" or sticking with the "window.timing". I'll follow suite in my WebKit patch.

Comment 5

[Tony Gentilcore]

(In reply to comment #4)
> I'm working on landing the namespace in WebKit here:
> https://bugs.webkit.org/show_bug.cgi?id=38924
> 
> The spec has been updated from "window.timing" -> "window.performance.timing".
> I just wanted to verify whether you are following that guideline and updating
> to "window.performance.timing" or sticking with the "window.timing". I'll
> follow suite in my WebKit patch.

FYI -- The IE9 PP3 has window.msPerformance.timing with a UA prefix and the intention of using window.performance.timing when the spec is standardized. I've updated my WebKit patch to use window.performance.timing.

Comment 6

[Johnny Stenback (:jst)]

I think we should attempt to get this into Firefox for 1.9.3 if at all possible, at least the most important parts.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 449469 [details] [diff] [review]
Initial implementation of web timing.

>+nsGlobalWindow::GetTiming(nsIDOMNavigationTiming** aNavigationTiming)
>+{
>+  FORWARD_TO_OUTER(GetTiming, (aNavigationTiming),
>+                   NS_ERROR_NOT_INITIALIZED);

You really want FORWARD_TO_INNER here. Otherwise the timing object risks outliving page transitions and interact poorly with the bfcache. I.e. what happens if you push 'back', does that update the timing information properly? And what happens if you're accessing the timing property of a document that no longer is being displayed.

I suspect that we really want to push the timing information into the document object, and have the timing object request it from there.

Please renominate if you disagree.

Comment 8

[Sergey Novikov]

Attachment: Implementation of window.performance.timing

The patch has been updated in accordance with the comments and last spec changes.

Comment 9

[Sergey Novikov]

(In reply to comment #2)
> (From update of attachment 449469 [details] [diff] [review])
> r=biesi on the docshell changes, but see the comment below. requesting review
> from sicking for the dom/ part.
> 
> +++ b/docshell/base/nsDocShell.cpp
> +    mTiming->NotifyRedirectStart();
>      if (!(aStateFlags & STATE_IS_DOCUMENT))
>          return; // not a toplevel document
> 
> Shouldn't you call NotifyRedirectStart after this if?

Done.

Comment 10

[Sergey Novikov]

(In reply to comment #3)
> Using NAVIGATION_NEW_WINDOW for loads in subframes seems wrong, per that spec.
>
NAVIGATION_FRAME was reintroduced in the spec for this case.
 
> Using PR_Now for those timestamps seems incorrect as well (and in particular,
> it has the wrong units).
>
Switched to PR_IntervalNow and milliseconds.
 
> Why is nsNavigationTiming needed?
>
Removed. nsDocShellTiming is exposed to DOM.
 
> nsNavigationTiming is exploitable if it outlives the docshell, no?
> 
See reply to the comment #7.

> Please add the new member to the end of nsIDOMWindowInternal.
> 
Done. Though it's "performance" now.

> Is the new member replaceable?  If not, won't it break any script that happens
> to use a global variable named "timing"?  Why is that acceptable?  (This is a
> spec issue, not an implementation issue.)

There are still arguing around this. So I stuck with the spec for now.
And thanks for bringing this up.

Comment 11

[Sergey Novikov]

(In reply to comment #4)
> I'm working on landing the namespace in WebKit here:
> https://bugs.webkit.org/show_bug.cgi?id=38924
> 
> The spec has been updated from "window.timing" -> "window.performance.timing".
> I just wanted to verify whether you are following that guideline and updating
> to "window.performance.timing" or sticking with the "window.timing". I'll
> follow suite in my WebKit patch.

Updated to use window.performance.timing

Comment 12

[Sergey Novikov]

(In reply to comment #7)
> (From update of attachment 449469 [details] [diff] [review])
> >+nsGlobalWindow::GetTiming(nsIDOMNavigationTiming** aNavigationTiming)
> >+{
> >+  FORWARD_TO_OUTER(GetTiming, (aNavigationTiming),
> >+                   NS_ERROR_NOT_INITIALIZED);
> 
> You really want FORWARD_TO_INNER here. Otherwise the timing object risks
> outliving page transitions and interact poorly with the bfcache. I.e. what
> happens if you push 'back', does that update the timing information properly?
> And what happens if you're accessing the timing property of a document that no
> longer is being displayed.
> 
> I suspect that we really want to push the timing information into the document
> object, and have the timing object request it from there.
> 
> Please renominate if you disagree.

Switched to FORWARD_TO_INNER.
Backward/forward navigations seem to update the timing properly.
Did this change resolve your concerns?

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 456252 [details] [diff] [review]
Implementation of window.performance.timing

You should request r? from someone. Also, could you please call the attribute window.moz_performance for now?

Comment 14

[Sergey Novikov]

(In reply to comment #13)
> (From update of attachment 456252 [details] [diff] [review])
> You should request r? from someone. 
I set jonas as a requestee for review. Have I missed something?

> Also, could you please call the attribute
> window.moz_performance for now?
Wouldn't mozPerformance be more in agreement with the code style?

Comment 15

[Sergey Novikov]

Attachment: Implementation of window.mozPerformance.timing

Renamed "window.performance" to "window.mozPerformance"

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

It seems like my concerns in comment 7 still applies.

I.e. all the timing information still lives in the docshell, and that docshells are reused across multiple pages (across multiple sites), are you *sure* that we *never* end up using the wrong performance data for the wrong page. Including in situations where the back button is being pushed, or when the bfcache is used, or if you push the back button, but before we reload the old page?

I'd really feel much more comfortable if the data lived at a location that wasn't shared between pages.

Comment 17

[Sergey Novikov]

> I'd really feel much more comfortable if the data lived at a location that
> wasn't shared between pages.

Agree. I'll modify the patch accordingly.

Comment 18

[Damon Sicore (:damons)]

Who owns this?  Need an owner ASAP.

Comment 19

[Christopher Blizzard (:blizzard)]

Sergey - have you had the time to update the patch with Jonas' comments?  Looks like we're getting to the end game for Firefox 4.  Beta 5 will likely be our feature freeze.

Comment 20

[Sergey Novikov]

Unfortunately had no time to work on this.
I'll try to finish the patch this week.

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Feature freeze for Firefox 4 is september 10th. If this patch isn't landed by then it won't make firefox 4. Note that the patch need to be landed, not just attached, so you probably need about a week to account for reviewing as well.

Comment 22

[Bjoern Kaiser]

Hi,
could you give a short update, whether this will make its way to Firefox 4 ? 
Thanks and Regards from Hamburg
Bjoern

Comment 23

[Igor Bazarny]

Attachment: Draft implementation of the web timing spec

I'd like to get this ball rolling again, will continue Sergey's work. New patch:
- Moves nsIDOMTiming implementation into dom/base
- Adds URI data to some events so it's possible to enforce same origin restriction
- Tries to enforce same origin in some cases.
- Fixed crash caused by the unload happening before channell activity on session restore
I expect a lot of comments from elders on style and content. What worries me now:
- Direct reference from nsDocShell which still collects data to nsDOMNavigationTiming implementation. Should I define some interface here?
- Timing data kept in the docshell. Some data is collected before relevant window instance is available. I need help identifying when it's OK to move data to window. Would it also make sense to collect data in window.

Things to do:
- More properties
- Corrections. I see redirect reported on session restore, Plus, redirect and loadStart events are out of order

Comment 24

[Tony Gentilcore]

I worked on the WebKit implementation, and just took a quick look over the patch to look for any glaring differences. The idl seems a bit out of date.

window.performance:
https://dvcs.w3.org/hg/webperf/raw-file/tip/specs/NavigationTiming/Overview.html#sec-window.performance-attribute
http://trac.webkit.org/browser/trunk/WebCore/page/Performance.idl

window.performance.timing:
https://dvcs.w3.org/hg/webperf/raw-file/tip/specs/NavigationTiming/Overview.html#sec-navigation-timing-interface
http://trac.webkit.org/browser/trunk/WebCore/page/PerformanceTiming.idl

window.performance.navigation:
https://dvcs.w3.org/hg/webperf/raw-file/tip/specs/NavigationTiming/Overview.html#sec-navigation-info-interface
http://trac.webkit.org/browser/trunk/WebCore/page/PerformanceNavigation.idl

Comment 25

[Johnny Stenback (:jst)]

We're not going to wait on this for Firefox 4, but this is something that we could take in a .x release.

Comment 26

[Tony Gentilcore]

FYI, we are close to removing the vendor prefix in WebKit.
https://bugs.webkit.org/show_bug.cgi?id=48922

Comment 27

[Igor Bazarny]

Attachment: Iteration of the timing API implementation

Better implementation of the timing API. Supposed to collect dom/view level timing, need channel changes to land for channel-level data.
Need to look closer into unload data collection, also need to look into less frequent scenarios like early js/meta redirects 

Changes window property name from 'mozPerformance' to 'performance'

Comment 28

[Igor Bazarny]

Attachment: Finished implementation of high-level measurements

Adding complete implementation of API, low-level data like domainLookupStart, connectStart etc. are not yet available, set all equal to fetchStart which is recorded on the first network event.

Otherwise implementation seems complete to me, who could review this patch?

Comment 29

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

Who has reviewed the draft spec? Jonas?

The patch needs tests.

Comment 30

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I went through the spec but that was a while ago. But I don't know that I know enough about the relevant code to be a good reviewer.

Comment 31

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

Comment on attachment 507393 [details] [diff] [review]
Finished implementation of high-level measurements

I'll give at least some feedback and try to find someone to review this, if I'm
not doing it myself.

Comment 32

[Igor Bazarny]

That would be awesome. I'm really new to Mozilla development and expect a lot of useful comments.

Comment 33

[Tony Gentilcore]

FYI, the spec is now at CR phase, and the vendor prefixes have been removed from IE9 and Chrome 10.

We are building a w3 test suite which may be helpful in this implementation:
http://w3c-test.org/webperf/tests/approved/

Comment 34

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

OK, starting to review the patch this weekend.
Maybe we could have it ready for FF5, though, I'd guess FF6.

Comment 35

[Steve Roussey (:sroussey)]

Can someone add firebug to the whiteboard?

Comment 36

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

Or actually, I need to review the spec first. It may or may not have bugs
(the fact that it is in CR doesn't affect to that) which I may or may not
find :)

Comment 37

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

Comment on attachment 507393 [details] [diff] [review]
Finished implementation of high-level measurements

It will be quite hard to review this with just diff -U 3.
-U 8 -p would be better.

Comment 38

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

Comment on attachment 507393 [details] [diff] [review]
Finished implementation of high-level measurements

Random comments.

>diff -r b5314bc1a926 content/base/public/Makefile.in
>--- a/content/base/public/Makefile.in	Thu Jan 27 00:08:05 2011 -0800
>+++ b/content/base/public/Makefile.in	Thu Jan 27 10:39:43 2011 +0100
>@@ -125,6 +125,7 @@
> 		nsIContentSecurityPolicy.idl \
> 		nsIFrameMessageManager.idl \
> 		nsIWebSocket.idl \
>+                nsIDocumentTiming.idl \


The patch seems to miss this file.
And I don't understand why the new interface is needed.
The same methods could be added to nsIDocument, I think.


>@@ -4178,6 +4181,8 @@
>       parent = parent->GetParentDocument();
>     } while (parent);
>   }
>+  
>+  mContentLoadedEnd = PR_Now() / PR_USEC_PER_MSEC;
> 

This should be set right after dispatching DOMContentLoaded.


> NS_IMETHODIMP
>+nsDocument::GetReadyStateLoading(DOMTimeMilliSec* aTime) {
>+  *aTime = mLoadingTime;
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsDocument::GetReadyStateInteractive(DOMTimeMilliSec* aTime) {
>+  *aTime = mInteractiveTime;
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsDocument::GetReadyStateComplete(DOMTimeMilliSec* aTime) {
>+  *aTime = mCompleteTime;
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsDocument::GetContentLoadedEventStart(DOMTimeMilliSec* aTime) {
>+  *aTime = mContentLoadedStart;
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsDocument::GetContentLoadedEventEnd(DOMTimeMilliSec* aTime) {
>+  *aTime = mContentLoadedEnd;
>+  return NS_OK;
>+}
>+

All these could be just inline methods in nsIDocument, and they
could return DOMTimeMilliSec



>+static nsDOMPerformanceNavigationType
>+ConvertLoadTypeToNavigationType(PRUint32 aLoadType)
>+{
>+    nsDOMPerformanceNavigationType result = nsIDOMPerformanceNavigation::TYPE_RESERVED;
>+    switch (aLoadType) {
>+    case LOAD_NORMAL:
>+    case LOAD_NORMAL_EXTERNAL:
>+    case LOAD_NORMAL_BYPASS_CACHE:
>+    case LOAD_NORMAL_BYPASS_PROXY:
>+    case LOAD_NORMAL_BYPASS_PROXY_AND_CACHE:
>+    case LOAD_LINK:
>+        result = nsIDOMPerformanceNavigation::TYPE_NAVIGATE;
>+        break;
>+    case LOAD_HISTORY:
>+        result = nsIDOMPerformanceNavigation::TYPE_BACK_FORWARD;
>+        break;
Based on the spec, bfcache'd session history traversal should not
change anything. Is that guaranteed by the patch?


>+nsresult 
>+nsDocShell::initTiming() 
InitTiming()


>@@ -6080,6 +6152,10 @@
>           FavorPerformanceHint(PR_FALSE, NS_EVENT_STARVATION_DELAY_HINT);
>         }
>     }
>+    // Timing is picked up by the window and got load event notifications
>+    // from the ContentViewer, we don't need it anymore
>+    mTiming = nsnull;
At this point a new page load may already have started,
so this is a wrong time to clear mTiming.


>+nsDOMNavigationTiming::NotifyNavigationStart()
>+{
>+  mNavigationStart = PR_Now()/PR_USEC_PER_MSEC;
Nit, space before and after '/'.
Same also elsewhere.

>+PRBool
>+nsDOMNavigationTiming::ReportRedirects(){
{ should be be in the next line.

>+NS_IMETHODIMP
>+nsDOMNavigationTiming::GetRedirectStart(DOMTimeMilliSec* aRedirectStart)
>+{
>+  *aRedirectStart = 0;
>+  if (ReportRedirects()) {
>+    *aRedirectStart = mRedirectStart;
>+  }
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsDOMNavigationTiming::GetRedirectEnd(DOMTimeMilliSec* aEnd)
>+{
>+  *aEnd = 0;
>+  if (ReportRedirects()) {
>+    *aEnd = mRedirectEnd;
>+  }
>+  return NS_OK;
>+}

The spec is too vague here.
In one place it talks about that redirecting should be same origin of the
destination, and in other place "if all the redirects or equivalent are from the same origin".
So I'm not sure whether ReportRedirects() does the right thing.


>+nsDOMNavigationTiming::HandleEvent(nsIDOMEvent* aEvent)
>+{
>+  nsAutoString eventType;
>+  aEvent->GetType(eventType);
>+  if (eventType.EqualsLiteral("readystatechange")) {
>+    nsCOMPtr<nsIDOMEventTarget> target;
>+    nsresult rv;
>+    rv = aEvent->GetTarget(getter_AddRefs(target));
>+    nsCOMPtr<nsIDocument> doc(do_QueryInterface(target, &rv));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    nsIDocument::ReadyState state = doc->GetReadyStateEnum();
>+    if (state == nsIDocument::READYSTATE_COMPLETE) {
>+      target->RemoveEventListener(NS_LITERAL_STRING("readystatechange"), 
>+          this, PR_FALSE);
>+    }
>+    nsCOMPtr<nsIDocumentTiming> docTiming(do_QueryInterface(target, &rv));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    mLoadedURI = doc->GetDocumentURI();
>+    docTiming->GetReadyStateLoading(&mDOMLoading);
>+    docTiming->GetReadyStateInteractive(&mDOMInteractive);
>+    docTiming->GetReadyStateComplete(&mDOMComplete);
>+    docTiming->GetContentLoadedEventStart(&mDOMContentLoadedEventStart);
>+    docTiming->GetContentLoadedEventEnd(&mDOMContentLoadedEventEnd);
>+  }
>+  return NS_OK;
nsDocument should just update these values without any event listener.


>+nsGlobalWindow::GetPerformance(nsIDOMPerformance** aPerformance)
>+{
>+  FORWARD_TO_INNER(GetPerformance, (aPerformance), NS_ERROR_NOT_INITIALIZED);
>+
>+  *aPerformance = nsnull;
>+
>+  if (!mPerformance) {
>+    return NS_ERROR_NOT_AVAILABLE;
Is throwing here the right behavior?

>+// Script "performance.timing" object
>+class nsPerformanceTiming : public nsIDOMPerformanceTiming



>+// Script "performance.navigation" object
>+class nsPerformanceNavigation : public nsIDOMPerformanceNavigation


>+// Script "performance" object
>+class nsPerformance : public nsIDOMPerformance
...
>+  nsRefPtr<nsDOMNavigationTiming> mData;
>+  nsCOMPtr<nsIDOMPerformanceTiming> mTiming;
>+  nsCOMPtr<nsIDOMPerformanceNavigation> mNavigation;

I don't understand why we need these "script" objects around the
objects which actually have the values.


>     docShell->GetRestoringDocument(&restoring);
>     if (!restoring) {
>+      nsCOMPtr<nsIDOMPerformanceTiming> timing;
>+      docShell->GetNavigationTiming(getter_AddRefs(timing));
>+      if (timing) {
>+        nsDOMNavigationTiming *collector = static_cast<nsDOMNavigationTiming *>(timing.get());
>+        collector->NotifyLoadEventStart();
>+      }
>       nsEventDispatcher::Dispatch(window, mPresContext, &event, nsnull,
>                                   &status);
>-#ifdef MOZ_TIMELINE
>+      if (timing) {
>+        nsDOMNavigationTiming *collector = static_cast<nsDOMNavigationTiming *>(timing.get());
>+        collector->NotifyLoadEventEnd();
>+      }
>+  #ifdef MOZ_TIMELINE
Extra space before '#'


unloadEventStart/End handling is wrong.
The patch checks the time just before and after firing beforeunload event, not unload event.
Although, the spec is vague when it talks about "unload event". Does that mean 
the whole "http://dev.w3.org/html5/spec/history.html#unloading-documents", or just firing the unload DOM event.
Based on other event timing handling (load and DOMContentLoaded) I assume the latter.

Comment 39

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

It looks like the spec has rather major problems, which must be
fixed before enabling this in Firefox.
If a pref is added so that we can enable/disable window.performance
easily, the code could land even while the spec is being changed and then
we could enable window.performance once the spec is fixed.

Comment 40

[Boris Zbarsky [:bzbarsky]]

Good luck with that now that it's been pushed into CR. :(

Comment 41

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

Well, most of the problems are fortunately reasonably easy to solve, I think.
But as of now it is not really possible to implement the spec so that
each feature is actually backed up by some definition in the spec.

Comment 42

[Zhiheng Wang]

   Olli, thanks for reviewing the patch and all the catches/comments towards the draft. The WPWG is always taking feedback to its drafts, CR or not. :-) It's actually important to have FF implementation account for before the draft moves forward.

Comment 43

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #39)
> It looks like the spec has rather major problems, which must be
> fixed before enabling this in Firefox.
> If a pref is added so that we can enable/disable window.performance
> easily, the code could land even while the spec is being changed and then
> we could enable window.performance once the spec is fixed.

Can we do that? It'd be a shame to have no support for this at all, even if it's behind a pref until some of the spec issues get sorted.

Thanks for reviewing the patch and pushing on this Olli.

Comment 44

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

The spec problems are being resolved, or are pretty much fixed already.
Igor has told me that he is working on to fix the patch.

So I think there is still a slight chance to get this in Fx.next, 
at least under a pref.
(Though, there isn't many days left Fx.next development.)

Comment 45

[Jan Honza Odvarko [:Honza] (always need-info? me)]

> So I think there is still a slight chance to get this in Fx.next, 
> at least under a pref.
It would be great!
Honza

Comment 46

[Igor Bazarny]

Attachment: One more iteration on the implementation

Thanks for the comments. I have incomplete patch which might be interesting to look at.

Not done:

- Still not sure what's going on with bfcache, storing timing in the doc seems like a right direction.
- Still need to find a good time/place to clean DocShell->Timing connection
- Unload handling needs more work.
- 'script' wrappers are just sign of missing knowledge. Spec says there should be 3 objects with distinct interfaces and I don't know how to make one object look like 3 on JS side.
- Also tried to hide functionality behind pref but was only able to crash Firefox
- And still there are no tests.

Done:
- nsIDocumentTiming interface removed, functionality merged into nsIDocument or nsDocument
- Easy changes like naming convention and style are done.
- No more listening to events.

Now ContentViewer and Document hold reference to the timing

Comment 47

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

(In reply to comment #46)
> - Still not sure what's going on with bfcache, storing timing in the doc seems
> like a right direction.
Yeah, that is what the spec currently says (at least sort-of)
(The fact that I don't agree with the spec should not change the
 implementation)

> - 'script' wrappers are just sign of missing knowledge. Spec says there should
> be 3 objects with distinct interfaces and I don't know how to make one object
> look like 3 on JS side.
Oh, hmm. Did I somehow misread the patch. It looked like there
are even more objects.
I'll read the patch tomorrow.

Comment 48

[Igor Bazarny]

(In reply to comment #47)
> > - 'script' wrappers are just sign of missing knowledge. Spec says there should
> > be 3 objects with distinct interfaces and I don't know how to make one object
> > look like 3 on JS side.
> Oh, hmm. Did I somehow misread the patch. It looked like there
> are even more objects.
> I'll read the patch tomorrow.
Well, it's 4--one is internal, passed from docshell to view and doc and wrapped by 'script' objects. Probably I can merge Performance implementation into internal object. I recall though that some new dependencies was needed for that.

Comment 49

[Igor Bazarny]

Attachment: Implementation and some tests

In attachment: Same implementation code as before, some tests added.

Comment 50

[Igor Bazarny]

Attachment: Updated implementation

Cleaned up a bit new implementation. Previous patch had an issue with unload event collection.
timing data moved from docshell to document. Some events still get collected in docshell, but data move to document quite early. There is not access to timing data through docshell, global window picks data from the document. Also, wrapper objects get created on demand.

I was not able to do anything better than returning null performance value if dom.enable_performance pref is not set. Need help if there's a better way.

Comment 51

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

For the pref one option is to handle it in a similar way as what
IndexedDB does now.

Comment 52

[Igor Bazarny]

Attachment: Check preference before data collection

Small change compared to the previous one--Check pref before data collection starts. I'm OK with returning null as property value--alternative is throwing exception (IndexedDB does that).

Comment 53

[Igor Bazarny]

Attachment: Test refactoring

No changes in implementation code, only test refactored. Can we get this patch into hg?

Comment 54

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

Once I (or someone else) have reviewed it.
Sorry, I was busy doing other things last week (Mozilla all-hands).

Comment 55

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

Comment on attachment 525089 [details] [diff] [review]
Test refactoring

>+nsDOMNavigationTiming*
>+nsDocument::GetNavigationTiming() const {
Nit, { should be in the next line.


>+nsDocument::SetNavigationTiming(nsDOMNavigationTiming* aTiming) {
Nit, { should be in the next line.





>+static nsDOMPerformanceNavigationType
>+ConvertLoadTypeToNavigationType(PRUint32 aLoadType)
>+{
>+    nsDOMPerformanceNavigationType result = nsIDOMPerformanceNavigation::TYPE_RESERVED;
>+    switch (aLoadType) {
>+    case LOAD_NORMAL:
>+    case LOAD_NORMAL_EXTERNAL:
>+    case LOAD_NORMAL_BYPASS_CACHE:
>+    case LOAD_NORMAL_BYPASS_PROXY:
>+    case LOAD_NORMAL_BYPASS_PROXY_AND_CACHE:
>+    case LOAD_LINK:
>+        result = nsIDOMPerformanceNavigation::TYPE_NAVIGATE;
>+        break;
>+    case LOAD_HISTORY:
>+        result = nsIDOMPerformanceNavigation::TYPE_BACK_FORWARD;
>+        break;
>+    case LOAD_RELOAD_NORMAL:
>+    case LOAD_RELOAD_CHARSET_CHANGE:
>+    case LOAD_RELOAD_BYPASS_CACHE:
>+    case LOAD_RELOAD_BYPASS_PROXY:
>+    case LOAD_RELOAD_BYPASS_PROXY_AND_CACHE:
>+        result = nsIDOMPerformanceNavigation::TYPE_RELOAD;
>+        break;
>+    case LOAD_NORMAL_REPLACE:
>+    case LOAD_STOP_CONTENT:
>+    case LOAD_STOP_CONTENT_AND_REPLACE:
>+    case LOAD_REFRESH:
>+    case LOAD_BYPASS_HISTORY:
>+    case LOAD_ERROR_PAGE:
>+    case LOAD_PUSHSTATE:
>+        result = nsIDOMPerformanceNavigation::TYPE_RESERVED;
>+        break;
>+    default:
>+        NS_NOTREACHED("Unexpected load type value");
>+    }
>+
>+    return result;
>+}
>+

>@@ -1510,16 +1551,20 @@ nsDocShell::FirePageHideNotification(PRB
>     if (mContentViewer && !mFiredUnloadEvent) {
>         // Keep an explicit reference since calling PageHide could release
>         // mContentViewer
>         nsCOMPtr<nsIContentViewer> kungFuDeathGrip(mContentViewer);
>         mFiredUnloadEvent = PR_TRUE;
> 
>         mContentViewer->PageHide(aIsUnload);
> 
>+        if (mTiming) {
>+            mTiming->NotifyLastUnload();
>+        }
You set just the ending time of upload event handling. Not starting time.
(Or to be exact, you set the starting time for some reason just after beforeunload event.)



>@@ -5940,19 +6011,27 @@ nsDocShell::OnRedirectStateChange(nsICha
>     NS_ASSERTION(aStateFlags & STATE_REDIRECTING,
>                  "Calling OnRedirectStateChange when there is no redirect");
>     if (!(aStateFlags & STATE_IS_DOCUMENT))
>         return; // not a toplevel document
> 
>     nsCOMPtr<nsIURI> oldURI, newURI;
>     aOldChannel->GetURI(getter_AddRefs(oldURI));
>     aNewChannel->GetURI(getter_AddRefs(newURI));
>+
>     if (!oldURI || !newURI) {
>         return;
>     }
>+    // On session restore we get a redirect from page to itself. Don't count it.
>+    PRBool equals = PR_FALSE;
>+    if (mTiming 
>+        && !(mLoadType == LOAD_HISTORY 
&& should be in the previous line
>+             && NS_SUCCEEDED(newURI->Equals(oldURI, &equals)) && equals)) {
ditto

>@@ -6062,16 +6144,17 @@ nsDocShell::EndPageLoad(nsIWebProgress *
>         // If all documents have completed their loading
>         // favor native event dispatch priorities
>         // over performance
>         if (--gNumberOfDocumentsLoading == 0) {
>           // Hint to use normal native event dispatch priorities 
>           FavorPerformanceHint(PR_FALSE, NS_EVENT_STARVATION_DELAY_HINT);
>         }
>     }
>+
Please, no random whitespace changes.

>@@ -6455,27 +6538,37 @@ nsDocShell::CreateAboutBlankContentViewe
>   nsCOMPtr<nsIDocShell> kungFuDeathGrip(this);
>   
>   if (mContentViewer) {
>     // We've got a content viewer already. Make sure the user
>     // permits us to discard the current document and replace it
>     // with about:blank. And also ensure we fire the unload events
>     // in the current document.
> 
>+    // Make sure timing is created. Unload gets fired first for 
>+    // document loaded from the session history.
>+    rv = InitTiming();
>+    if (mTiming) {
>+      mTiming->NotifyBeforeUnload();
>+    }
>+
>     PRBool okToUnload;
>     rv = mContentViewer->PermitUnload(PR_FALSE, &okToUnload);
> 
>     if (NS_SUCCEEDED(rv) && !okToUnload) {
>       // The user chose not to unload the page, interrupt the load.
>       return NS_ERROR_FAILURE;
>     }
> 
>     mSavingOldViewer = aTryToSaveOldPresentation && 
>                        CanSavePresentation(LOAD_NORMAL, nsnull, nsnull);
> 
>+    if (mTiming) {
>+      mTiming->NotifyUnloadAccepted(mCurrentURI);
>+    }
>     // Make sure to blow away our mLoadingURI just in case.  No loads
>     // from inside this pagehide.
>     mLoadingURI = nsnull;
>     
>     // Notify the current document that it is about to be unloaded!!
>     //
>     // It is important to fire the unload() notification *before* any state
>     // is changed within the DocShell - otherwise, javascript will get the




>@@ -2386,16 +2395,28 @@ nsDOMClassInfo::Init()
>   DOM_CLASSINFO_MAP_BEGIN(BarProp, nsIDOMBarProp)
>     DOM_CLASSINFO_MAP_ENTRY(nsIDOMBarProp)
>   DOM_CLASSINFO_MAP_END
> 
>   DOM_CLASSINFO_MAP_BEGIN(History, nsIDOMHistory)
>     DOM_CLASSINFO_MAP_ENTRY(nsIDOMHistory)
>   DOM_CLASSINFO_MAP_END
> 
>+  DOM_CLASSINFO_MAP_BEGIN(PerformanceTiming, nsIDOMPerformanceTiming)
>+    DOM_CLASSINFO_MAP_ENTRY(nsIDOMPerformanceTiming)
>+  DOM_CLASSINFO_MAP_END
>+
>+  DOM_CLASSINFO_MAP_BEGIN(PerformanceNavigation, nsIDOMPerformanceNavigation)
>+    DOM_CLASSINFO_MAP_ENTRY(nsIDOMPerformanceNavigation)
>+  DOM_CLASSINFO_MAP_END
>+
>+  DOM_CLASSINFO_MAP_BEGIN(Performance, nsIDOMPerformance)
>+    DOM_CLASSINFO_MAP_ENTRY(nsIDOMPerformance)
>+  DOM_CLASSINFO_MAP_END
>+
The interfaces need to be pref'ed of when the feature is disabled.
But for doing that you'd need part of my patch for touch events
(where I don't want to expose the interfaces in case touch events aren't supported)
So, no need to worry about the issue now.


> NS_IMETHODIMP
>+nsGlobalWindow::GetPerformance(nsIDOMPerformance** aPerformance)
>+{
>+  FORWARD_TO_INNER(GetPerformance, (aPerformance), NS_ERROR_NOT_INITIALIZED);
>+
>+  *aPerformance = nsnull;
>+  
>+  if (nsGlobalWindow::HasPerformanceSupport()) {
>+    if (!mPerformance) {
>+      nsDOMNavigationTiming* timing = mDoc->GetNavigationTiming();
>+      if (timing) {
>+        mPerformance = new nsPerformance(timing);
>+      }
>+    }
>+    NS_IF_ADDREF(*aPerformance = mPerformance);
>+  }
>+  return NS_OK;
>+}
This is not the quite right way to pref off the feature.
You'd need to 


>+nsPerformance::GetTiming(nsIDOMPerformanceTiming** aTiming)
>+{
>+  if (!mTiming) {
>+    mTiming = new nsPerformanceTiming(mData);
>+  }
>+  NS_ENSURE_TRUE(mTiming, NS_ERROR_OUT_OF_MEMORY);
No need for OOM check. 'few' is infallible.


>+
>+  /**
>+   * A namespace to hold performance related data and statistics.
>+   */
>+  readonly attribute nsIDOMPerformance performance;
So, to be able pref this off fully, you'd need to create a new interface
for this and make nsGlobalWindow to implement it.
Then in nsDOMClassInfo where DOM_CLASSINFO_MAP_BEGIN is done for the window,
add a check and the new interface should be listed only if the pref is on.
Look for DesktopNavigation as an example.


>     if (!restoring) {
>+      nsDOMNavigationTiming* timing = mDocument->GetNavigationTiming();
nsRefPtr, please. Nothing keeps timing object alive, if load event
does bad things like closes the window.


Looking good, but I'd like to see unload handling fixed, and also the new interface, so that the feature can be properly switched off.

Comment 56

[Igor Bazarny]

Attachment: pref-out 'performance' prop, unload collecting corrected

Olli, thanks a lot for comments! 
Attaching patch with corrections

Comment 57

[Igor Bazarny]

(In reply to comment #55)
> Comment on attachment 525089 [details] [diff] [review]
> Test refactoring
> 
> >+nsDOMNavigationTiming*
> >+nsDocument::GetNavigationTiming() const {
> Nit, { should be in the next line.
Done

> 
> 
> >+nsDocument::SetNavigationTiming(nsDOMNavigationTiming* aTiming) {
> Nit, { should be in the next line.
Done

> > 
> >+        if (mTiming) {
> >+            mTiming->NotifyLastUnload();
> >+        }
> You set just the ending time of upload event handling. Not starting time.
> (Or to be exact, you set the starting time for some reason just after
> beforeunload event.)
Changed. I collected start of beforeunload event by some reason (measuring the whole unload time?). 

> >+    PRBool equals = PR_FALSE;
> >+    if (mTiming 
> >+        && !(mLoadType == LOAD_HISTORY 
> && should be in the previous line
> >+             && NS_SUCCEEDED(newURI->Equals(oldURI, &equals)) && equals)) {
> ditto
Done in both cases

> >         }
> >     }
> >+
> Please, no random whitespace changes.
Empty line removed

> > 
> >+  DOM_CLASSINFO_MAP_BEGIN(PerformanceTiming, nsIDOMPerformanceTiming)
> >+    DOM_CLASSINFO_MAP_ENTRY(nsIDOMPerformanceTiming)
> >+  DOM_CLASSINFO_MAP_END
> >+
> >+  DOM_CLASSINFO_MAP_BEGIN(PerformanceNavigation, nsIDOMPerformanceNavigation)
> >+    DOM_CLASSINFO_MAP_ENTRY(nsIDOMPerformanceNavigation)
> >+  DOM_CLASSINFO_MAP_END
> >+
> >+  DOM_CLASSINFO_MAP_BEGIN(Performance, nsIDOMPerformance)
> >+    DOM_CLASSINFO_MAP_ENTRY(nsIDOMPerformance)
> >+  DOM_CLASSINFO_MAP_END
> >+
> The interfaces need to be pref'ed of when the feature is disabled.
> But for doing that you'd need part of my patch for touch events
> (where I don't want to expose the interfaces in case touch events aren't
> supported)
> So, no need to worry about the issue now.
Well, I tried to pref them out, but that doesn't remove class names from the other list and simplest thing to do here is to register empty interface lists which doesn't look good to me.

> 
> 
> > NS_IMETHODIMP
> >+nsGlobalWindow::GetPerformance(nsIDOMPerformance** aPerformance)
> >+{
> >+  FORWARD_TO_INNER(GetPerformance, (aPerformance), NS_ERROR_NOT_INITIALIZED);
> >+
> >+  *aPerformance = nsnull;
> >+  
> >+  if (nsGlobalWindow::HasPerformanceSupport()) {
> >+    if (!mPerformance) {
> >+      nsDOMNavigationTiming* timing = mDoc->GetNavigationTiming();
> >+      if (timing) {
> >+        mPerformance = new nsPerformance(timing);
> >+      }
> >+    }
> >+    NS_IF_ADDREF(*aPerformance = mPerformance);
> >+  }
> >+  return NS_OK;
> >+}
> This is not the quite right way to pref off the feature.
> You'd need to 
I decided to keep this piece, allows turning 'performance' off 
without browser reload

> 
> 
> >+nsPerformance::GetTiming(nsIDOMPerformanceTiming** aTiming)
> >+{
> >+  if (!mTiming) {
> >+    mTiming = new nsPerformanceTiming(mData);
> >+  }
> >+  NS_ENSURE_TRUE(mTiming, NS_ERROR_OUT_OF_MEMORY);
> No need for OOM check. 'few' is infallible.
Done

> 
> 
> >+
> >+  /**
> >+   * A namespace to hold performance related data and statistics.
> >+   */
> >+  readonly attribute nsIDOMPerformance performance;
> So, to be able pref this off fully, you'd need to create a new interface
> for this and make nsGlobalWindow to implement it.
> Then in nsDOMClassInfo where DOM_CLASSINFO_MAP_BEGIN is done for the window,
> add a check and the new interface should be listed only if the pref is on.
> Look for DesktopNavigation as an example.
Done, but that's a bit of combinatorial explosion. I hope there will be no third feature to move behind the pref.

> 
> 
> >     if (!restoring) {
> >+      nsDOMNavigationTiming* timing = mDocument->GetNavigationTiming();
> nsRefPtr, please. Nothing keeps timing object alive, if load event
> does bad things like closes the window.
Oops. Fixed. Thanks for catching.

Oh, one more thing corrected. Today test started failing because of sub-ms result of Date.now(). Fixed too.

Comment 58

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

> > The interfaces need to be pref'ed of when the feature is disabled.
> > But for doing that you'd need part of my patch for touch events
> > (where I don't want to expose the interfaces in case touch events aren't
> > supported)
> > So, no need to worry about the issue now.
> Well, I tried to pref them out, but that doesn't remove class names from the
> other list and simplest thing to do here is to register empty interface lists
> which doesn't look good to me.
There will be a macro for this kind of case. See bug 648573

> Done, but that's a bit of combinatorial explosion. I hope there will be no
> third feature to move behind the pref.
Yeah, we (Mozilla) need to think about how to turn on and off features in this
new fast release cycle scheme.

Comment 59

[Igor Bazarny]

Do we still have a chance of getting navigation timing into Fireofox 5?

Comment 60

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

No :(

And seems like the spec has problems with time handling
(see the recent thread in the w3c mailing list).

Comment 61

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

Though, apparently IE does already something reasonable, so
need to probably just follow what they do.
(And not do what Chrome does)

Comment 62

[Stoyan]

Some encouragement/peer pressure :)
 - Chrome 9 has window.webkitPerformance
 - Chrome 10 has window.performance
 - IE9 has window.performance too

Comment 63

[Boris Zbarsky [:bzbarsky]]

Yes, and Chrome discovered that the spec is buggy while IE9 wasn't implementing the spec to start with...

Comment 64

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

Conclusion from today's webtiming conference call: Implementations should use a monotonic clock relative to document creation time

Implementation-wise, I think that means this in Firefox:
- On document creation, store PR_Now() as well as TimeStamp
- Store all future times as TimeStamp instead of PRTime
- In the DOM getters, return the value document_creation_prnowtime + (value_timestamp - document_creation_timestamp).ToMilliSeconds()

Comment 65

[Boris Zbarsky [:bzbarsky]]

> Implementation-wise, I think that means this in Firefox:

Looks correct to me.

Comment 66

[Igor Bazarny]

Note that some events may happen before the document creation, so using of PR_Now() at navigationStart moment would be a bit simpler implementation-wise and barely distinguishable to user. Alternatively, we may use time relative to PR_Now() value at the moment close to the time when scripts in a page get a chance to execute Date.now(), say domLoading moment. Or, just moment of first performance property access. I like the last option most--gives pretty consistent measurements to the script. So, we just store all times as timestamps and fix base wall time and timestamp at the moment of the first access, than count backwards for all (or most of) recorded moments.

Comment 67

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

Good points. The spec now suggests in http://dvcs.w3.org/hg/webperf/raw-file/tip/specs/NavigationTiming/Overview.html#mono-clock to use the navigation start as the base time, and the phone conference agreed to make that normative, so let's just go with that?

Comment 68

[Boris Zbarsky [:bzbarsky]]

That sounds good.

Comment 69

[Igor Bazarny]

Attachment: Updated to use intervals

Updated to match source changes by 2011/05/12, uses timeouts to measure time.

Comment 70

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

Ready for another review?

Comment 71

[Igor Bazarny]

Yes, please take a look

Comment 72

[Honza Bambas (:mayhemer)]

Instead of PR_Interval* we should use mozilla::TimeStamp.  PR_Interval has a really bad resolution on some platforms.  mozilla::TimeStamp is about to be fixed to have the highest resolution possible.  See bug 539093.

Also mentioned in Christian's comment 64.

Comment 73

[Igor Bazarny]

Thanks for the pointer, I didn't know about TimeStamp existence and misinterpreted Christian's comment. Will update code tomorrow.

Comment 74

[Igor Bazarny]

Attachment: intervals replaced by mozilla::TimeStamp

New version with mozilla:TimeStamp used to measure intervals.

Comment 75

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

Comment on attachment 532204 [details] [diff] [review]
intervals replaced by mozilla::TimeStamp

Adding this back to my review queue so that I might remember to review this ;)

Comment 76

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

(In reply to comment #73)
> Thanks for the pointer, I didn't know about TimeStamp existence and
> misinterpreted Christian's comment. Will update code tomorrow.

My apologies for not being clearer in my comment.

Comment 77

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

Comment on attachment 532204 [details] [diff] [review]
intervals replaced by mozilla::TimeStamp


> nsDocument::SetReadyStateInternal(ReadyState rs)
> {
>   mReadyState = rs;
>+  if (mTiming) {
>+    switch (rs) {
>+    case READYSTATE_LOADING:
>+      mTiming->NotifyDOMLoading(nsIDocument::GetDocumentURI());
>+      break; 
>+    case READYSTATE_INTERACTIVE:
>+      mTiming->NotifyDOMInteractive(nsIDocument::GetDocumentURI()); 
>+      break; 
>+    case READYSTATE_COMPLETE:
>+      mTiming->NotifyDOMComplete(nsIDocument::GetDocumentURI());
>+      break; 
>+    default:
>+      NS_WARNING("Unexpected ReadyState value");    
>+      break;
>+    }
>+  }
Nit, case parts should be indented.
It should be
     switch (expr) {
       case FOOBAR:
         stmt;
         break;
       ...
     }


>+  // At the time of loading start, we don't have timing object, record time.
>+  if (READYSTATE_LOADING == rs) {
>+    mLoadingTimeStamp = mozilla::TimeStamp::Now();
>+  }
I wonder if we could get the timing object from docshell at this point.
File a followup bug, please.


>+nsDocShell::InitTiming() 
>+{
>+    if (mTiming) {
>+        return NS_OK;
>+    }
>+
>+    PRBool enabled;
>+    nsresult rv = mPrefs->GetBoolPref("dom.enable_performance", &enabled);
>+    if (NS_SUCCEEDED(rv) && enabled) {
>+      mTiming = new nsDOMNavigationTiming();
>+      NS_ENSURE_TRUE(mTiming, NS_ERROR_OUT_OF_MEMORY);
'new' is infallible, so no need for OOM check.


>     if ((~aStateFlags & (STATE_START | STATE_IS_NETWORK)) == 0) {
>+        // Save timing statistics.
>+        nsCOMPtr<nsIChannel> channel(do_QueryInterface(aRequest));
>+        // Make sure timing is created
>+        rv = InitTiming();
>+        NS_ENSURE_SUCCESS(rv, rv);
If InitTiming fails for some reason, it shouldn't cause rest of the method to fail.
Also, since InitTiming may or may not actually initialize something (depending on the pref), 
could you call it MaybeInitTiming().


>     nsCOMPtr<nsIURI> oldURI, newURI;
>     aOldChannel->GetURI(getter_AddRefs(oldURI));
>     aNewChannel->GetURI(getter_AddRefs(newURI));
>+
>     if (!oldURI || !newURI) {
>         return; 
>     }
No random whitespace changes, please.


>+NS_IMETHODIMP
>+nsDOMNavigationTiming::GetDomainLookupEnd(DOMTimeMilliSec* aEnd)
>+{
>+  // TODO: Implement me!
Please file a followup bug to implement all the todos and
add the bug number to code where TODO is mentioned.


>     if (!restoring) {
>+      nsDOMNavigationTiming* timing = mDocument->GetNavigationTiming();
nsRefPtr, otherwise you may crash after dispatching load event,
and that crash would be security critical.


with those r=me

Comment 78

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

Attachment: merged to trunk & review comments fixed

Comment 79

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

Followup bugs filed:
bug 659126 TODOs for additional properties
bug 659130 get object from docshell

Comment 80

[Jim Mathies [:jimm]]

This was backed out due to various test failures:

597 ERROR TEST-UNEXPECTED-FAIL | /tests/docshell/test/test_bug570341.html | Expected evt_load to happen before loadEventEnd, got evt_load = 1306196608474, loadEventEnd = 1306196608473
579 ERROR TEST-UNEXPECTED-FAIL | /tests/docshell/test/test_bug570341.html | Expected navigationStart to happen before unloadEventStart, got navigationStart = , unloadEventStart =
580 ERROR TEST-UNEXPECTED-FAIL | /tests/docshell/test/test_bug570341.html | Expected unloadEventStart to happen before evt_unload, got unloadEventStart = , evt_unload = 1306196531069
581 ERROR TEST-UNEXPECTED-FAIL | /tests/docshell/test/test_bug570341.html | Expected evt_unload to happen before unloadEventEnd, got evt_unload = 1306196531069, unloadEventEnd =
582 ERROR TEST-UNEXPECTED-FAIL | /tests/docshell/test/test_bug570341.html | Expected navigationStart to happen before fetchStart, got navigationStart = , fetchStart =
583 ERROR TEST-UNEXPECTED-FAIL | /tests/docshell/test/test_bug570341.html | Expected fetchStart to happen before domainLookupStart, got fetchStart = , domainLookupStart =
584 ERROR TEST-UNEXPECTED-FAIL | /tests/docshell/test/test_bug570341.html | Expected domainLookupStart to happen before domainLookupEnd, got domainLookupStart = , domainLookupEnd =
585 ERROR TEST-UNEXPECTED-FAIL | /tests/docshell/test/test_bug570341.html | Expected domainLookupEnd to happen before connectStart, got domainLookupEnd = , connectStart =
586 ERROR TEST-UNEXPECTED-FAIL | /tests/docshell/test/test_bug570341.html | Expected connectStart to happen before connectEnd, got connectStart = , connectEnd =
587 ERROR TEST-UNEXPECTED-FAIL | /tests/docshell/test/test_bug570341.html | Expected connectEnd to happen before requestStart, got connectEnd = , requestStart =
588 ERROR TEST-UNEXPECTED-FAIL | /tests/docshell/test/test_bug570341.html | Expected requestStart to happen before responseStart, got requestStart = , responseStart =
589 ERROR TEST-UNEXPECTED-FAIL | /tests/docshell/test/test_bug570341.html | Expected responseStart to happen before responseEnd, got responseStart = , responseEnd =
590 ERROR TEST-UNEXPECTED-FAIL | /tests/docshell/test/test_bug570341.html | Expected responseStart to happen before domLoading, got responseStart = , domLoading =
591 ERROR TEST-UNEXPECTED-FAIL | /tests/docshell/test/test_bug570341.html | Expected domLoading to happen before domInteractive, got domLoading = , domInteractive =
592 ERROR TEST-UNEXPECTED-FAIL | /tests/docshell/test/test_bug570341.html | Expected domInteractive to happen before domComplete, got domInteractive = , domComplete =

Comment 81

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

Attachment: attempt to fix tests

Comment 82

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

Since Olli is probably asleep for a few more hours and I'd like this in Firefox 6-

Boris, Jonas, any thoughts on enabling webtiming support by default? Currently the patch disables the code in opt builds, but Google would be very happy if Firefox users would actually have this enabled :)


(Sorry for noticing this issue so late before the cutoff...)

Comment 83

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

Attachment: timing enabled by default

bz agrees to enable timing by default, this patch does that

Comment 84

[Dão Gottwald [:dao]]

Apparently the test failure is not fixed.

Comment 85

[Igor Bazarny]

Attachment: Actually enabling performance support

It seems user_pref change doesn't enable performance, need pref. Tests corrected a bit to make missing performance object clear.

Comment 86

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

That was a silly error :(

Pushed to try to see if this works now:
http://tbpl.mozilla.org/?tree=Try&rev=0c9401e68377

Comment 87

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

Comment on attachment 534776 [details] [diff] [review]
Actually enabling performance support

So what is the difference between this and the previous patch?

Comment 88

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

The difference between Igor's patch and my last patch is to fix a typo in all.js (user_pref -> pref) and a small test improvement.

The difference between the last patch you reviewed and this patch is enabling timing by default (and fixing the review comments), which should also fix the mochitest failures.

Comment 89

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

Though it looks like try doesn't like this patch, this time a crashtest failure due to an assertion:

###!!! ASSERTION: Unexpected load type value: 'Not Reached', file /builds/slave/try-osx-dbg/build/docshell/base/nsDocShell.cpp, line 712
ConvertLoadTypeToNavigationType [docshell/base/nsDocShell.cpp:715]
nsDocShell::OnStateChange [docshell/base/nsDocShell.cpp:5900]
nsDocLoader::FireOnStateChange [uriloader/base/nsDocLoader.cpp:1323]
nsDocLoader::doStartDocumentLoad [uriloader/base/nsDocLoader.cpp:856]
nsDocLoader::OnStartRequest [uriloader/base/nsDocLoader.cpp:557]
nsLoadGroup::AddRequest [netwerk/base/src/nsLoadGroup.cpp:595]
nsURILoader::OpenChannel [uriloader/base/nsURILoader.cpp:960]
nsURILoader::OpenChannel [uriloader/base/nsURILoader.cpp:985]
[...]

REFTEST TEST-UNEXPECTED-FAIL | file:///Users/cltbld/talos-slave/test/build/reftest/tests/content/media/test/crashtests/481136-1.html | assertion count 1 is more than expected 0 assertions

Comment 90

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

Comment on attachment 534776 [details] [diff] [review]
Actually enabling performance support

The problem mentioned in comment 89 needs to be fixed.

Comment 91

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

Any update here. We have now branched for Fx6, and if this is wanted for Fx7,
it is better to have a working patch sooner than later so that regressions etc
can be hopefully fixed before the release.

Comment 92

[Igor Bazarny]

Attachment: Added protection for not initialized nsDocShell::mLoadType

Try server failures caused by scenarios when nsDocShell::mLoadingTime is not initialized, e.g. in case of object tag referring to supported xml content (say mathml). Added protection so that timing is not collected in this case.

Comment 93

[Igor Bazarny]

(In reply to comment #91)
> Any update here. We have now branched for Fx6, and if this is wanted for Fx7,
> it is better to have a working patch sooner than later so that regressions
> etc
> can be hopefully fixed before the release.

There is an updated patch, I need help in running it trough the tryserver.

Comment 94

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

Comment on attachment 536050 [details] [diff] [review]
Added protection for not initialized nsDocShell::mLoadType

Ok, at least for now.
Need to file a followup bug to make object handling work better.

I'll try to get this pushed to tryserver.

Comment 95

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

Though, the patch doesn't apply cleanly to trunk.
Any chance you could update the patch?

Comment 96

[Igor Bazarny]

Attachment: Updated to the current head state, otherwise unchanged

Updated to the current head state, conflicts resolved.

Comment 97

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

Pushed to try. Look for "nav.timing"

Comment 98

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

http://tbpl.mozilla.org/?tree=Try&rev=9b5dba99a472

Most failures fixed! But there's still two timeouts of the test, and on windows there is still a mismatch between the event times and the NavigationTiming times :(

Comment 99

[Igor Bazarny]

(In reply to comment #98)
> http://tbpl.mozilla.org/?tree=Try&rev=9b5dba99a472
> 
> Most failures fixed! But there's still two timeouts of the test, and on
> windows there is still a mismatch between the event times and the
> NavigationTiming times :(

Thanks for checking. I need to investigate, possibly change tests a bit. Mismatch in times looks pretty strange, especially on single platforn.

Comment 100

[Igor Bazarny]

Attachment: Tests updated, hopefully fixes timeout and failures on try server

- Rounding correction--was rounding to closest int, now rounds down.
- Test: Correlation between navigation.timing and Date.now() at time of events removed--precision seems too different to check ordering reliably.
- Test: Added internal time limit (5s). Doesn't seem to cause failures, but need confirmation on try server.

Comment 101

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

tryserver seems to like the new patch:
http://tbpl.mozilla.org/?tree=Try&rev=0525e4e15dcb

Comment 102

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

Attachment: diff-of-diffs

This is a diff between attachment 536050 [details] [diff] [review] (last attachment with r=smaug) and attachment 540060 [details] [diff] [review]. I hand-edited to remove irrelevant changes (changed context, changed modification dates of files, etc)

This is not a real interdiff (because interdiff didn't like the patches), but I hope that it's still possible to make sense of it. I found it reasonably readable in vim with syntax highlighting.

Comment 103

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

(actually attachment 537130 [details] [diff] [review], but there's no actual difference between those two)

Comment 104

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

Comment on attachment 541142 [details]
diff-of-diffs

Ms2ger noticed the strange (int) casting.
It is C casting and using int.
Better to use C++ casting and PRInt32

Comment 105

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

Attachment: updated per review comment

sr=biesi. transferring r=smaug

Comment 106

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

Attachment: merged to trunk and slight reformatting

Comment 107

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

Attachment: merged to trunk again

Comment 108

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

http://tbpl.mozilla.org/?tree=Firefox&rev=c931c8b1d8f6
http://hg.mozilla.org/mozilla-central/rev/c931c8b1d8f6

Let's hope it sticks this time!

Comment 109

[Eric Shepherd [:sheppy]]

Does bug 579571 supersede this?

Comment 110

[:Gavin Sharp [email: [e-mail]]]

(In reply to Eric Shepherd [:sheppy] from comment #109)
> Does bug 579571 supersede this?

No, these two things are entirely different (this is new hotness, that is old grossness).

Comment 111

[Eric Shepherd [:sheppy]]

(In reply to Gavin Sharp from comment #110)
> (In reply to Eric Shepherd [:sheppy] from comment #109)
> > Does bug 579571 supersede this?
> 
> No, these two things are entirely different (this is new hotness, that is
> old grossness).

Let me rephrase my question: do both need documentation, or should this be documented instead of the other.

Comment 112

[:Gavin Sharp [email: [e-mail]]]

They're entirely unrelated. You could document the code removal in bug 579571, but it was mostly broken and not-built-by-default internal code, so I doubt anyone really cared about it. This is a new Web-exposed API and therefore should be much higher on the list of documentation priorities.

Comment 113

[Daniel Holbert [:dholbert]]

Attachment: followup to fix build warning

This bug's cset added "mLoadType" to the nsDocShell constructor init list, but it added it out of order, introducing this warning:
> nsDocShell.h: In constructor 'nsDocShell::nsDocShell()':
> nsDocShell.h:790: warning: 'nsDocShell::mPreviousTransIndex' will be initialized after
> nsDocShell.h:779: warning:   'PRUint32 nsDocShell::mLoadType'
> nsDocShell.cpp:718: warning:   when initialized here

Attached followup-patch moves mLoadType in the init list to fix the warning.

Comment 114

[Daniel Holbert [:dholbert]]

Landed followup to fix init list:
  http://hg.mozilla.org/integration/mozilla-inbound/rev/36e5a31b8f88

Comment 115

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/36e5a31b8f88

Comment 116

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en/Firefox_7_for_developers#Web_timing
https://developer.mozilla.org/en/Navigation_timing